Remove broken-card task and RBR after a 90-day grace period while keeping the error#93523
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
heyjennahay and
jayeshmangwani
and removed request for
a team
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc3bc0134b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…nection grace CARD_FEED_ERRORS depends on the current date via isBrokenConnectionPastDismissThreshold, but its Onyx dependencies were only card/feed keys. A connection crossing the 90-day threshold while the app stayed open with no card/feed update would not recompute, so the home task/RBR could linger. Add CURRENT_DATE (day-granularity) as a trigger-only dependency so the value re-derives when the day changes; the compute body already reads the live clock. Update compute() test call sites for the resulting 4-tuple input. Addresses Codex automated-review feedback on PR Expensify#93523.
# Conflicts: # src/CONST/index.ts # tests/unit/CardUtilsTest.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96096444cc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!isCardConnectionBroken(card) || !card.lastScrape) { | ||
| return false; | ||
| } | ||
| return DateUtils.getDifferenceInDaysFromNow(new Date(card.lastScrape)) >= CONST.COMPANY_CARDS.BROKEN_CONNECTION_DISMISS_AFTER_DAYS; |
There was a problem hiding this comment.
lastScrape with the app date parser
When lastScrape uses the existing backend/fixture format like 2024-11-27 11:00:53, relying on new Date(card.lastScrape) is not portable across the supported JS engines; if it produces an invalid date, differenceInDays returns NaN and the comparison is always false, so broken connections older than 90 days keep showing the task/RBR. The card detail pages already parse this field via getLocalDateFromDatetime, so this threshold check should use the same normalized parsing instead of the native parser.
Useful? React with 👍 / 👎.
| dependencies: [ONYXKEYS.CARD_LIST, ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST, ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER], | ||
| // CURRENT_DATE (day-granularity) is a trigger-only dependency: it makes this value recompute when the day rolls over so the | ||
| // 90-day broken-connection grace period (isBrokenConnectionPastDismissThreshold) is re-evaluated even if no card/feed data changes. | ||
| dependencies: [ONYXKEYS.CARD_LIST, ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST, ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER, ONYXKEYS.CURRENT_DATE], |
There was a problem hiding this comment.
This CURRENT_DATE dependency only advances where setCurrentDate is actually called; I checked the wiring and it is started from the web platform setup, while src/setup/platformSetup/index.native.ts is a no-op. On iOS/Android, if a broken card crosses the 90-day boundary while the app remains open and no card/feed data changes, the derived value never recomputes, so the Home task and RBR can persist until a restart or unrelated data update.
Useful? React with 👍 / 👎.
|
@wildan-m, I'm trying to test this using the provided steps, but the console log doesn't work for me, it doesn't do anything. I'm not sure if I need to update something in the code first. Also, what do you think about the suggestions #93523 (comment) and #93523 (comment) above? |
card.lastScrape uses the Expensify DB datetime format ("yyyy-MM-dd HH:mm:ss"). Relying on
new Date() for that non-ISO string is not portable across JS engines (Safari/JSC, Hermes) and
can produce an invalid date, making getDifferenceInDaysFromNow return NaN so the 90-day grace
would never dismiss the task/RBR. Parse explicitly with date-fns and guard against an
unparseable value (fail-safe: keep surfacing the task). Also adds an unmocked test that
exercises the real parsing. Addresses Codex review feedback on PR Expensify#93523.
Follow-up to the lastScrape parse fix: the derived-value grace-period tests used a
date-only 'yyyy-MM-dd' lastScrape, which the stricter parse('yyyy-MM-dd HH:mm:ss')
now treats as unparseable (NaN guard -> not dismissed). Use the real DB format so the
past-grace company and personal card tests exercise a valid parse.
@jayeshmangwani have you toggle on "use staging server"? have you checked my browser video? |
|
Thanks @jayeshmangwani! Suggestion 1 (date parsing) — agree, real bug, fixed in Suggestion 2 (native day-change recompute) — valid, but this should be a separate change. It's right that |
Yes, @wildan-m , I have enabled Use Staging Server and also followed the steps in your video, but it's still not working for me. I'm attaching a video below showing the exact steps I'm following. console-copy-paste.mov |
# Conflicts: # src/libs/CardUtils.ts # src/libs/actions/OnyxDerived/configs/cardFeedErrors.ts # tests/unit/CardUtilsTest.ts
|
@jayeshmangwani I've tried multiple times locally and everything works as expected. Could you please check out the fix branch again, stop the current webpack instance, and restart webpack cleanly? |
# Conflicts: # tests/unit/CardUtilsTest.ts
|
Let me try again after doing a clean restart of everything. |
|
@wildan-m Have you tested the script against the latest code? I think the error data is coming from the |
|
For me, this script is working. (async () => {
const d = new Date(Date.now() - 100 * 864e5), p = (n) => String(n).padStart(2, '0');
const old = `${d.getFullYear()}-${p(d.getMonth() + 1)}-${p(d.getDate())} ${p(d.getHours())}:${p(d.getMinutes())}:${p(d.getSeconds())}`;
await Onyx.merge('cards_22551001_oauth.mockbank.com', { '5710640066021610': { lastScrape: old } });
})(); |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppios.moviOS: mWeb Safarimweb-safati.movMacOS: Chrome / Safariweb.movNote: On the web, I ran the script from the browser console to test the PR by modifying the date to more than 90 days in the past. On the other platforms, I ran the Onyx code when the page rendered and then refreshed the app to test the PR with the same past-date data. |
|
🚧 MariaHCD has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@MariaHCD @jayeshmangwani Would you like to setup special accounts for us to run QA on as this requires some backend validations. Or you are able to QA internally? |
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.4.33-0 🚀
|
Help site review — docs update needed ✅Yes, this change affects the help site. The PR adds a 90-day grace period after which the broken-card Home task, the "Card feed connection is broken" banner, and the red dots (RBR) are removed — while the card stays broken and fixable. Two existing articles document exactly those surfaces without mentioning that they disappear after the grace period, so they're now incomplete. I created a draft docs PR adding an FAQ entry to each: 📝 Docs PR: #95841
The
@wildan-m, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
6 participants
Explanation of Change
A personal or company card whose connection has broken keeps prompting the user indefinitely: the time-sensitive home task and the red dot (RBR) pointing at the broken connection stay forever, no matter how long the connection has gone unresolved. Both surfaces are driven by a single derived signal that only looks at the latest connection status — there is no notion of how long the connection has been failing, so a years-old broken connection nags exactly as much as one that broke yesterday.
This adds a 90-day grace period to that single signal: once a connection has been broken and unresolved for 90 days, it stops feeding the task and the red dots. The age is measured from the connection's last successful refresh, which does not advance while the connection is failing, so it accurately reflects how long the connection has been broken (and if that timestamp is missing we keep prompting, to stay on the safe side). The error stored on the card is deliberately left alone, so the card still shows as broken on its own page and stays fixable — under 90 days nothing changes, and past 90 days only the proactive prompting goes away.
Fixed Issues
$ #91451
PROPOSAL: #91451 (comment)
Tests
This change only affects shared logic (no platform-specific code), so it behaves the same on web, mWeb, iOS, and Android.
A broken card connection can't be created through normal use on demand — use the Mock Bank test feed (available on staging or a dev build):
Aging a broken connection past 90 days (dev build — there's no UI to backdate it). Paste this in the browser JS console; it sets every broken card's last update to 100 days ago, and also updates the workspace feed list that the company-card surfaces read:
On mWeb the same console works; on native use the React Native debugger (same
Onyx), a connection genuinely 90+ days old, or temporarily setCONST.COMPANY_CARDS.BROKEN_CONNECTION_DISMISS_AFTER_DAYSto0.Offline tests
This change only gates a value derived from already-local card data and the device date — no network is involved. It behaves identically offline: a connection broken past the 90-day window stays unprompted, and one within the window keeps prompting, whether online or offline.
QA Steps
This change only affects how long a broken card connection keeps prompting the user, so please treat it as a regression test around the card feature. The behaviour is the same on all platforms (web, mWeb, iOS, Android), so any one platform is fine.
If a 90-days-broken connection isn't available as test data, the after-90-days removal is also covered by automated unit tests; the essential manual checks are step 1 (recently-broken connections still prompt) and step 3 (no regressions elsewhere in the card feature).
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Kapture.2026-06-26.at.04.33.41.mp4