Skip to content

[Internal QA] Fix Account and Wallet RBR still showing for personal cards past the 90-day grace period - #96364

Merged
MariaHCD merged 16 commits into
Expensify:mainfrom
wildan-m:wildan/91451-personal-card-rbr-followup
Aug 18, 2026
Merged

[Internal QA] Fix Account and Wallet RBR still showing for personal cards past the 90-day grace period#96364
MariaHCD merged 16 commits into
Expensify:mainfrom
wildan-m:wildan/91451-personal-card-rbr-followup

Conversation

@wildan-m

@wildan-m wildan-m commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Scope update — please re-review. This PR originally covered only the personal-card half. Part 2 (company cards) was added after the first review round (commits aed1471, 0b71530): it adds a field to the shared CardFeedErrorState derived value and re-points three consumers (useAssignCard, WorkspaceCompanyCardsTable, the Home-task hook).

Follow-up to #93523. That PR added the 90-day grace period and correctly removed the Home task, but both halves of the issue's requirement were still wrong on production. @joekaufmanexpensify reported each one:

  • Personal cards: Remove the indicator on Account and red dot on Wallet row in the account left hand bar. But keep the error on the card itself on the Wallet page so the card can still be fixed.
  • Company cards: Remove the red dots leading to the broken connection error on the company cards page in the workspace. But leave the error itself there so the user can still fix it.

Part 1 — personal cards: the dots never turned off. (report) The Account / Wallet dots only ask "does this card have an error?" — via hasPaymentMethodError() and the derived shouldShowRBR — and we deliberately keep that error so the card stays fixable, so they need the grace check applied explicitly. Two things then stopped that check from ever firing:

  1. isBrokenConnectionPastDismissThreshold() parsed card.lastScrape with the strict Expensify DB format (yyyy-MM-dd HH:mm:ss) only, so an ISO 8601 value (2024-01-01T00:00:00Z) → NaN → never past-grace.
  2. The one that matched the reported account: the check required isCardConnectionBroken(), but the server sets the connection error on card.errors even when lastScrapeResult is one of the ignored statuses (e.g. 434). Those cards are "not broken" per that check, so they could never be dismissed — their error kept the dots lit forever. (That account's cards were status 434, last successful sync ~700 days ago, plus 438 ones the existing check did cover.)

Fix: key the dismissal on how long the card has gone without a successful sync via a new isLastScrapePastDismissThreshold() (DB format, then ISO fallback), and gate the personal-card error paths on it. The Home task keeps the narrow broken-connection check, and card.errors is left untouched on the card, so it keeps its red dot in the Wallet list and — for a genuine broken status like 403 — its details page still shows "Your card connection is broken." with a working Fix card button. (For an ignored status like 434 that prompt never rendered in the first place, since the details page gates it on isCardConnectionBroken; the dot is the surviving signal.)

Part 2 — company cards: the error itself disappeared, so the feed became unfixable. (report) #93523 also ANDed the 90-day check into the company path, zeroing cardFeedErrors[feed].isFeedConnectionBroken. That same flag renders the Company cards page's "Card feed connection is broken — log into your bank" banner, which is the only labelled entry point into the reconnect flow from that page. It also gates updateBrokenConnection(), the code that clears the error after a successful reconnect. So past 90 days the feed could no longer be fixed at all, while Settings > Wallet still told the user to go fix it there.

Fix: that flag was doing two jobs — "is this feed broken?" (the banner and the reconnect, which must keep working so the feed stays fixable) and "should we still prompt about it?" (the RBR dots, the Home task, assign-blocking, and per-row error suppression). Only the first should survive the grace period, so they're split: isFeedConnectionBroken stays truthful, and a new shouldPromptBrokenConnection drives everything in the second group.

So past the grace period assigning is no longer blocked and per-row card errors are no longer suppressed. Both are deliberate: the flag is OR'd across the whole feed, so one long-dead card would otherwise disable assigning on every row forever — and a commercial/CSV feed can't be reconnected by a bank login at all (RefreshCardFeedConnectionPage 404s for non-direct feeds), so it would never resolve.

Consumer Reads Past 90 days
getShouldShowBrokenConnectionError → Company cards banner isFeedConnectionBroken shows (direct feeds only)
useUpdateFeedBrokenConnection → reconnect clears the error cardsWithBrokenFeedConnection works
getShouldShowRBR → all RBR dots shouldPromptBrokenConnection hidden
useBrokenDirectCompanyCardFeedsForAdmin → Home task grace filter at the hook hidden
useAssignCardAssign button shouldPromptBrokenConnection enabled
WorkspaceCompanyCardsTable → per-row card errors shouldPromptBrokenConnection shown

Net behaviour past 90 days: no Home task and no navigational RBR dots (Account button, Wallet row, Workspaces row) — but the card keeps its error on the Wallet page, and a broken direct company feed keeps its "log into your bank" banner (including the red dot that is part of that banner) plus a working reconnect. Within 90 days of the last successful sync nothing changes — note the gate deliberately measures time-since-last-successful-sync rather than time-since-broken, because the server marks 434-status cards with the connection error while isCardConnectionBroken reports them healthy. New unit tests cover the ignored-status (434) case, both date formats, within/past grace, the company-feed banner-vs-RBR split, and that assigning stays possible on a dismissed feed. (The per-row error suppression is a rendering-level prop, so it's covered by the QA steps rather than a unit test.)

Fixed Issues

$ #91451
PROPOSAL: #91451 (comment)

Tests

This is shared logic (no platform-specific code), so it behaves the same on web, mWeb, iOS and Android.

Important — what actually reproduces this. It shows on a personal card (added on Account > Wallet, fundID absent or '0') that carries the server-set connection error in card.errors while the 90-day gate can't fire for it. The production case: lastScrapeResult: 434 — an "ignored" status that isCardConnectionBroken treats as not broken, so the old gate (which required a broken connection) could never dismiss it even with the last successful sync ~700 days ago. An ISO-format lastScrape failing the old strict date parse triggers the same outcome.

The script below has two modes:

  • MODE = 'inspect' (default, read-only — safe on any account, including a real one): prints every card already on the account (its lastScrape + detected format, whether it's broken, daysBroken, and pastGrace) plus the derived cardFeedErrors. Use this to see real cards' data and whether the gate fired.
  • MODE = 'simulate' (dev/adhoc accounts only): injects synthetic broken personal cards so you don't have to wait 90 days for real data. MODE = 'reset' removes them.
(async () => {
  // ── config ──────────────────────────────────────────────────────────────
  const MODE = 'inspect'; // 'inspect'  = read-only report of the cards already on this account (SAFE on any account,
                          //               including a real/customer one — it never writes to Onyx).
                          // 'simulate' = inject synthetic broken personal cards (dev/adhoc accounts only).
                          // 'reset'    = remove the synthetic cards a previous 'simulate' added.
  // only used by MODE = 'simulate':
  const DAYS_AGO = 100;   // >= 90 → past the grace period. Set to 1 for the "recently broken" baseline.
  const STATUS   = 434;   // 434 = an "ignored" scrape status that still carries the server error (the production case).
                          // 403 = a plain broken-connection status.
  const FORMAT   = 'db';  // 'db' = "yyyy-MM-dd HH:mm:ss" (what the production account had). 'iso' = ISO 8601.
  const COUNT    = 4;     // how many broken personal cards to inject.
  // ────────────────────────────────────────────────────────────────────────
  try {
    if (typeof Onyx === 'undefined' || typeof Onyx.get !== 'function') {
      console.error('[card-sim] window.Onyx unavailable — use a non-production (dev/adhoc) build.');
      return;
    }
    const SIM_PREFIX = 'sim_broken_';
    const IGNORED = [200, 434, 531, 530, 500, 666]; // CONST.COMPANY_CARDS.BROKEN_CONNECTION_IGNORED_STATUSES
    const GRACE = 90;                               // CONST.COMPANY_CARDS.BROKEN_CONNECTION_DISMISS_AFTER_DAYS
    const isPersonal = (c) => !c?.fundID || c.fundID === '0'; // ~ CardUtils.isPersonalCard (personal cards live in `cardList`)
    const isBroken = (c) => c?.lastScrapeResult !== undefined && !IGNORED.includes(c.lastScrapeResult);
    const parseScrape = (s) => {
      if (!s) return null;
      // same idea as the fix: ISO parses via new Date(); the DB "space" format is normalised to ISO first.
      const d = new Date(String(s).includes('T') ? s : String(s).replace(' ', 'T'));
      return Number.isNaN(d.getTime()) ? null : d;
    };
    const formatOf = (s) => (!s ? 'empty' : /T/.test(String(s)) ? 'ISO' : /^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/.test(String(s)) ? 'DB' : 'other');

    // Read-only report of every card already on the account + the derived RBR state.
    const report = async () => {
      const cardList = (await Onyx.get('cardList')) ?? {};
      const rows = Object.entries(cardList).map(([key, c]) => {
        const d = parseScrape(c.lastScrape);
        const daysBroken = d ? Math.floor((Date.now() - d.getTime()) / 864e5) : null;
        return {
          card: c.cardID ?? key,
          type: isPersonal(c) ? 'personal' : 'workspace',
          bank: c.bank || '(none)',
          broken: isBroken(c),
          lastScrape: c.lastScrape || '(empty)',
          format: formatOf(c.lastScrape),
          daysBroken,
          pastGrace: daysBroken !== null && daysBroken >= GRACE,
          errors: Object.keys(c.errors ?? {}).length,
          errorFields: Object.keys(c.errorFields ?? {}).join(',') || '-',
        };
      });
      const cfe = await Onyx.get('cardFeedErrors');
      console.log(`[card-sim] ${rows.length} card(s) — copy the JSON block below (shouldShowRBR drives the Wallet row dot; the Account dot is computed separately by hasPaymentMethodError):`);
      console.log(JSON.stringify({cards: rows, derived: {personalCard: cfe?.personalCard, companyCards: cfe?.companyCards, all: cfe?.all}}, null, 2));
      console.log('[card-sim] Expected with the fix: a personal card with pastGrace=true (last successful sync 90+ days ago, any scrape status) no longer lights personalCard.shouldShowRBR. If a stale card shows pastGrace=false, look at its `format` — the gate needs a parseable date.');
    };

    if (MODE === 'reset') {
      const cardList = (await Onyx.get('cardList')) ?? {};
      const del = {};
      for (const id of Object.keys(cardList)) {
        if (id.startsWith(SIM_PREFIX)) {
          del[id] = null;
        }
      }
      await Onyx.merge('cardList', del);
      console.log(`[card-sim] removed ${Object.keys(del).length} simulated card(s)`);
      return;
    }

    if (MODE === 'simulate') {
      const p = (n) => String(n).padStart(2, '0');
      const d = new Date(Date.now() - DAYS_AGO * 864e5);
      const db = `${d.getFullYear()}-${p(d.getMonth() + 1)}-${p(d.getDate())} ${p(d.getHours())}:${p(d.getMinutes())}:${p(d.getSeconds())}`;
      const lastScrape = FORMAT === 'db' ? db : d.toISOString();
      const upd = {};
      for (let i = 0; i < COUNT; i++) {
        const id = `${SIM_PREFIX}${i}`;
        upd[id] = {
          cardID: id, accountID: 0, bank: 'Wells Fargo', cardName: `Broken card ${i + 1}`,
          domainName: '', fraud: 'none', lastFourPAN: String(1000 + i), state: 3,
          lastScrapeResult: STATUS, lastScrape, errors: {'1700000000000001': 'Your card connection is broken.'},
        };
      }
      await Onyx.merge('cardList', upd);
      await new Promise((r) => setTimeout(r, 700));
      console.log(`[card-sim] injected ${COUNT} broken personal card(s) with lastScrape "${lastScrape}" (${FORMAT}, ${DAYS_AGO}d ago). Re-run with MODE='reset' to remove them.`);
      await report();
      return;
    }

    // MODE === 'inspect' (default, read-only)
    await report();
  } catch (e) {
    console.error('[card-sim] failed:', e);
  }
})();
  1. Inspect (any account, read-only): run with MODE = 'inspect' and read the printed JSON. On an account with long-broken personal cards (any scrape status), confirm the 90+ day ones show pastGrace: true and the derived personalCard.shouldShowRBR is false (the fix) — while each card is still listed on the Wallet page with its red dot.
  2. Simulate a baseline (dev account): set MODE = 'simulate', DAYS_AGO = 1 — confirm the Account button dot and the Wallet row dot both show (recently broken, unchanged behaviour).
  3. Set DAYS_AGO = 100 and re-run — confirm the Account dot and the Wallet row dot are gone, and the log shows personalCard.shouldShowRBR: false. This is the production case: status 434 cards are "not broken" per isCardConnectionBroken, so before this fix they could never be dismissed.
  4. Confirm the cards are untouched on the Wallet page: still listed with their own red dot. With STATUS = 403 (a genuine broken status) opening one also still shows "Your card connection is broken." with a working Fix card button — that prompt reads errorFields.lastScrape, so it never appears for the 434 case.
  5. (Optional) repeat with STATUS = 403 (a plain broken status — also verifies the Home task appears at DAYS_AGO = 1 and disappears at 100), and with FORMAT = 'iso' (the ISO date shape behaves the same). Re-run with MODE = 'reset' to remove the simulated cards.

Past grace, the derived personalCard should read:

// with this fix:      {"shouldShowRBR":false,"hasFeedErrors":false,"hasWorkspaceErrors":false,"isFeedConnectionBroken":false,"shouldPromptBrokenConnection":false}
// before the fix:     {"shouldShowRBR":true, "hasFeedErrors":true, "hasWorkspaceErrors":false,"isFeedConnectionBroken":false}   <- 434: "not broken", so never dismissible

personalCard.shouldShowRBR drives the Wallet row dot; the Account button dot is computed separately by hasPaymentMethodError(), which reads card.errors directly — this PR gates both on the same 90-day check. isFeedConnectionBroken drives the Home task. Before the fix a status-434 card fails the isCardConnectionBroken precondition, so the 90-day gate never runs and its error keeps shouldShowRBR stuck on true — exactly what the production account showed.

Part 2 — company cards (the broken feed must stay fixable)

You need a workspace where you're an admin with company cards enabled. Paste this in the console on a dev build to give it a direct (OAuth) feed whose last successful sync is ~5 years ago — the reported production shape. Replace DOMAIN with the workspace's policyAccountID ((await Onyx.get('policy_<policyID>'))?.policyAccountID):

(async () => {
  const DOMAIN = 22714765;      // <- the workspace's policyAccountID
  const FEED = 'oauth.brex.com';
  const DAYS_AGO = 1900;        // >= 90 -> past the grace period. Use 1 for the "recently broken" baseline.
  const p = (n) => String(n).padStart(2, '0');
  const d = new Date(Date.now() - DAYS_AGO * 864e5);
  const lastScrape = `${d.getFullYear()}-${p(d.getMonth() + 1)}-${p(d.getDate())} ${p(d.getHours())}:${p(d.getMinutes())}:${p(d.getSeconds())}`;
  await Onyx.merge(`sharedNVP_private_domain_member_${DOMAIN}`, {
    settings: {
      companyCards: {[FEED]: {pending: false, liabilityType: 'corporate', statementPeriodEndDay: 1}},
      oAuthAccountDetails: {[FEED]: {accountList: ['Brex ****4444'], credentials: 'sim', expiration: new Date(Date.now() + 90 * 864e5).toISOString()}},
    },
  });
  const mk = (id, pan) => ({cardID: id, accountID: 0, bank: FEED, cardName: `Brex ${pan}`, domainName: '', fraud: 'none', fundID: String(DOMAIN), lastFourPAN: pan, state: 3, lastScrapeResult: 438, lastScrape, lastUpdated: ''});
  await Onyx.merge(`cards_${DOMAIN}_${FEED}`, {2001: mk(2001, '4444'), 2002: mk(2002, '5555')});
  await new Promise((r) => setTimeout(r, 1000));
  const cfe = await Onyx.get('cardFeedErrors');
  console.log('[feed-sim] ' + JSON.stringify(cfe?.cardFeedErrors?.[`${FEED}#${DOMAIN}`]));
  console.log('[feed-sim] re-run with the two Onyx.merge values set to null to remove the simulated feed.');
})();
  1. With DAYS_AGO = 1900, open Workspaces > [workspace] > Company cards. Confirm the "Card feed connection is broken. Please log into your bank…" banner shows and its link opens the reconnect flow — this is the fix; before it, the banner was gone and the feed could not be fixed at all.
  2. Confirm there is no red dot on the workspace row in the Workspaces list, and no time-sensitive task on Home — prompting stays off past 90 days.
  3. Confirm the green Assign button on an unassigned row is still clickable (a dismissed feed must not block assigning — a commercial/CSV feed can't be reconnected at all, so blocking would never resolve).
  4. Re-run with DAYS_AGO = 1 and confirm the baseline is unchanged: the Home task "Fix … company card connection" and the workspace red dot both come back, alongside the banner.
  5. The log line shows the split directly — past grace {"isFeedConnectionBroken":true,"shouldPromptBrokenConnection":false,"shouldShowRBR":false}; within grace all three are true.
  • Verify that no errors appear in the JS console

Offline tests

Same as the Tests above. This is a derived value computed from data already in Onyx plus the device date, so it behaves identically offline — the task and indicators stay hidden for a past-grace connection, and the card keeps its error (and its Fix card action where that prompt applies).

QA Steps

Please treat this as a regression check around the card feature. Behaviour is the same on all platforms, so any one platform is fine.

  1. With a personal card whose connection broke recently (under 90 days), confirm it still surfaces as before: the task on Home, the red dot on the Account button, and the red dot on the Wallet row. This is unchanged and must keep working.
  2. With a personal card whose connection has been broken for 90 days or more, confirm the Home task, the Account red dot, and the Wallet row red dot are all gone — while the card is still listed on the Wallet page with its red dot, and a genuinely broken card (e.g. status 403) still offers Fix card on its details page.
  3. With a direct (OAuth/Plaid) company card feed broken recently (under 90 days), confirm it still surfaces as before: the Home task, the red dot on the workspace row, and the "log into your bank" banner on the Company cards page.
  4. With a direct company card feed broken for 90 days or more, confirm the Home task and the workspace red dot are gone, but the Company cards page still shows the "Card feed connection is broken — log into your bank" banner and reconnecting still clears the error. Also confirm the Assign button still works on that feed. (Commercial/CSV feeds never show this banner — that's pre-existing behaviour, not a regression.)
  5. Confirm nothing else in the card feature regressed — adding/removing personal cards, and assigning/unassigning company cards, still behave as before. Note the intended trade-off: a personal card's card.errors aren't separable per-error, so once a personal card hasn't synced in 90+ days none of them light the Account/Wallet dots (other errorFields entries, e.g. a failed reimbursable update, still do).

If a 90-days-broken connection isn't available as test data, the behaviour is also covered by automated unit tests; the essential manual checks are steps 1 and 3 (recently-broken still prompts) and step 5 (no regressions).

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Kapture.2026-07-18.at.10.46.14.mp4

…race period

The 90-day grace correctly removed the home task, but the Account indicator and the
Wallet row red dot stayed lit for personal cards. Both are driven by the card's own
error, which the issue requires us to keep so the card stays fixable:

- getShouldShowRBR short-circuits on hasFeedErrors before the grace check, so a
  past-grace card's error still forced the RBR.
- hasPaymentMethodError counts any personal card with errors, and it feeds both the
  Account indicator (useAccountIndicatorChecks) and the Wallet row (InitialSettingsPage).

Gate both on isBrokenConnectionPastDismissThreshold, leaving card.errors untouched so
the Wallet-page card row keeps its error and Fix card action.

Adds regression tests for a past-grace card that still carries the error (the case the
original fixtures missed), plus guards that within-grace and unrelated errors still show.
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
...onents/Tables/WorkspaceCompanyCardsTable/index.tsx 66.19% <100.00%> (ø)
src/hooks/useAssignCard.ts 81.81% <100.00%> (ø)
src/hooks/useCardFeedErrors.ts 100.00% <ø> (ø)
src/libs/CardUtils.ts 85.39% <100.00%> (+0.06%) ⬆️
...libs/actions/OnyxDerived/configs/cardFeedErrors.ts 97.87% <100.00%> (+2.41%) ⬆️
src/libs/actions/PaymentMethods.ts 34.18% <100.00%> (+1.70%) ⬆️
...n/hooks/useBrokenDirectCompanyCardFeedsForAdmin.ts 95.00% <100.00%> (+0.55%) ⬆️
... and 11 files with indirect coverage changes

@wildan-m
wildan-m marked this pull request as ready for review July 18, 2026 03:51
@wildan-m
wildan-m requested review from a team as code owners July 18, 2026 03:51
@melvin-bot
melvin-bot Bot requested a review from jayeshmangwani July 18, 2026 03:51
@melvin-bot

melvin-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown

@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]

@melvin-bot
melvin-bot Bot requested review from joekaufmanexpensify and removed request for a team July 18, 2026 03:51

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac02fdb2c8

ℹ️ 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".

Comment thread src/libs/actions/OnyxDerived/configs/cardFeedErrors.ts Outdated
Comment thread src/libs/actions/PaymentMethods.ts Outdated
@wildan-m

Copy link
Copy Markdown
Contributor Author

@jayeshmangwani @joekaufmanexpensify The follow up PR is ready

Addresses Codex review. The previous change dropped ALL of a stale-broken personal
card's errors/errorFields, so an unrelated actionable error also lost its RBR — a failed
reimbursable/start-date update (another errorFields entry) or a failed remove (card.errors).

Now only errorFields.lastScrape (the broken-connection error the card detail page reads)
is excluded from the RBR once past the grace period; every other error is kept. Reverts
the over-broad hasPaymentMethodError gate, since card.errors on a personal card is an
actionable error, not the connection error.

Adds regression tests for a past-grace card that also carries an unrelated field error
and one that carries a card-level error.
@jayeshmangwani

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4c2ad732aa

ℹ️ 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".

const isPastDismissThreshold = isBrokenConnectionPastDismissThreshold(card);
const errorFieldsForRBR =
isPastDismissThreshold && card.errorFields ? Object.fromEntries(Object.entries(card.errorFields).filter(([field]) => field !== 'lastScrape')) : card.errorFields;
const hasCardErrors = !isEmptyObject(card.errors) || !isEmptyObject(errorFieldsForRBR);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Suppress dismissed broken-connection card errors

For past-grace personal cards this only drops errorFields.lastScrape, but card.errors is still treated as RBR-worthy. In the Account indicator path, useAccountIndicatorChecks still calls hasPaymentMethodError(), which returns true for any personal card with card.errors, so a dismissed broken connection stored as a card-level error such as errors.brokenConnection will continue to light the Account dot and Wallet row even though isPastDismissThreshold is true. Please filter the broken-connection card-level error in the same past-grace case, while preserving unrelated card errors.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed in e50ca23235c.

You're right that the broken connection is also stored as a card.errors entry (server-set) — that's what keeps lighting the Account button via hasPaymentMethodError and the Wallet row, so dropping only errorFields.lastScrape wasn't enough. This also corrects my earlier note on the PaymentMethods thread: the connection error does land in card.errors, not just errorFields.

Past the grace period we now also skip a broken personal card's card.errors when deciding the RBR, in both the derived value and hasPaymentMethodError, so both surfaces clear. It's gated by isBrokenConnectionPastDismissThreshold at the card level rather than by an errors.brokenConnection key — there's no dedicated key, the error is server-set and uses the generic shape. Any actionable error kept in a separate errorFields entry (e.g. a failed reimbursable/start-date update) still surfaces. The one case this can't separate is a card that is both past-grace broken and carries a fresh card.errors failure (e.g. a failed remove); it's treated as the dismissed connection, but that's a rare/transient state and the error itself stays on the card either way.

Added regression tests for the Account indicator and the derived value.

A broken personal card's connection is also stored as a server-set card.errors
entry, which lit the Account button (via hasPaymentMethodError) and the Wallet row
even past the 90-day grace period. Gate it by isBrokenConnectionPastDismissThreshold
in both the derived value and hasPaymentMethodError, so those surfaces clear while
actionable errors kept in a separate errorFields entry (reimbursable/start-date)
still surface. Adds Account-indicator and derived-value regression tests.
@github-actions

Copy link
Copy Markdown
Contributor

🚧 joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here.

@github-actions

This comment has been minimized.

@jayeshmangwani

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e50ca23235

ℹ️ 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".

Comment thread src/libs/actions/PaymentMethods.ts Outdated
return true;
// A broken personal-card connection is surfaced as a card error too, so once it is unresolved past the grace
// period we stop leading the user to it and it must no longer light the RBR (any other error still does).
return !CardUtils.isBrokenConnectionPastDismissThreshold(card);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve unrelated personal-card errors past threshold

When a personal card is past the broken-connection threshold, this return ignores every root-level card.errors entry on that card, not just the stale connection error. Personal-card actions can create unrelated root-level errors—for example unassignCard failure writes errors into CARD_LIST, and the personal-card details menu can invoke that path—so a card that is also long-broken would no longer light the Account/Wallet indicators for a new actionable failure. Please distinguish the broken-connection error from other card.errors instead of suppressing the whole card.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged as a deliberate trade-off, and called out in the PR description.

card.errors isn't separable per-entry here: the server re-stamps the connection error into card.errors with a fresh microsecond key on every OpenApp (confirmed from the reporter's real account data), so neither the key nor its age distinguishes it from a client-written failure like unassignCard. Matching on the message text would be worse.

The failure isn't invisible though — the card's own row in the Wallet list keeps its red dot (that path is ungated), so only the account-level indicators are held back. Errors written to a separate errorFields entry are unaffected, and as of cebad6d that now includes a failed manual sync.

@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

Tested, and the changes still aren't working. I still see the red dot indicator on the account button and the wallet row.
image

@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

Let me know if you need any more information from me to investigate further!

@jayeshmangwani

Copy link
Copy Markdown
Contributor

@joekaufmanexpensify , This PR is working for me locally for personal cards. Just following up on the two points below:

  1. We're seeing the red dot on the Account button, which might be because there's a subscription-related error (for example, "Your payment could not be processed") under Subscription. We can also see the red dot on the Subscription row in Screenshot.
Screenshot 2026-07-22 at 4 00 04 PM
  1. Just confirming, are we sure that all of the personal card errors are more than 90 days old? The screenshot shows multiple cards, so I wanted to double-check.
Screenshot 2026-07-22 at 4 00 10 PM

@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

We're seeing the red dot on the Account button, which might be because there's a subscription-related error (for example, "Your payment could not be processed") under Subscription. We can also see the red dot on the Subscription row in Screenshot.

This isn't why. There is a separate error on their subscription page. Also, each of these four cards in the wallet page has a broken connection error.

Just confirming, are we sure that all of the personal card errors are more than 90 days old? The screenshot shows multiple cards, so I wanted to double-check.

Confirmed, I just checked again. Each of these four personal cards has been broken since 2024 at the latest.

@jayeshmangwani

Copy link
Copy Markdown
Contributor

@wildan-m Could you please take a look at these comments #96364 (comment) and #96364 (comment) ?

@wildan-m

Copy link
Copy Markdown
Contributor Author

checking.. @joekaufmanexpensify Is it possible that I'm logging in using your test account which contains that error data? Alternatively, please let me know a more precise method to perform e2e testing aside from the aforementioned script injection

… runs

Personal cards store lastScrape as ISO 8601 (2024-01-01T00:00:00Z), which the
strict yyyy-MM-dd HH:mm:ss parse rejected -> NaN -> isBrokenConnectionPastDismissThreshold
returned false, so the grace gate never fired and the Account/Wallet RBR persisted
past 90 days (company cards use the DB format, so their Home task cleared correctly).
Fall back to new Date() for ISO. Adds regression tests for ISO (with/without Z) and
an unparseable value.
@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

@jayeshmangwani could you try approving again to see if this routes to someone else internally now?

@quinthar quinthar removed the #convert label Aug 13, 2026
@jayeshmangwani

Copy link
Copy Markdown
Contributor

@joekaufmanexpensify, approving doesn't trigger the review. I think the issue is that I’m currently OOO, so I’m not in the GitHub C+ auto-assignment team. Because of that, my approval isn’t triggering the internal engineer review.

@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

I see. At this point, I think it's fine if the review waits until Monday when Maria is back, given her context. Going to assign her since she reviewed the other PR 👍

MariaHCD
MariaHCD previously approved these changes Aug 18, 2026
@MariaHCD
MariaHCD marked this pull request as draft August 18, 2026 08:45
@MariaHCD
MariaHCD marked this pull request as ready for review August 18, 2026 08:45

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b71530aee

ℹ️ 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".

Comment on lines +75 to +76
const errorFieldsForRBR =
isPastDismissThreshold && card.errorFields ? Object.fromEntries(Object.entries(card.errorFields).filter(([field]) => field !== 'lastScrape')) : card.errorFields;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve fresh manual-sync failures past the threshold

When a past-threshold personal card's Update card request fails, syncCard() writes a fresh generic failure to errorFields.lastScrape (src/libs/actions/Card.ts lines 374–387), but this filter removes every lastScrape field error solely because the previous successful scrape is old. Consequently, after leaving the card details page, the Wallet row has no RBR for that newly failed action. Fresh evidence beyond the earlier field-error report is that lastScrape is also the failure field for a user-initiated sync, rather than exclusively the stale server connection error, so the filter needs to distinguish those cases.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wildan-m @jayeshmangwani what do you both think of this? Is it possible to land in this scenario where a newly failed user-initiated sync won't show the RBR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and fixed in cebad6d. errorFields.lastScrape is only ever written when a user-initiated action fails (syncCard and updatePersonalCardConnection both write it in their failureData) — the stale server connection error lives in card.errors, which is gated separately — so filtering it suppressed the wrong thing. Dropped the filter, so a freshly failed sync keeps its RBR while the dismissed connection stays quiet; both cases now have tests.

Comment thread src/hooks/useAssignCard.ts Outdated
Comment thread src/libs/CardUtils.ts Outdated
Comment thread src/libs/actions/OnyxDerived/configs/cardFeedErrors.ts Outdated
@MariaHCD

Copy link
Copy Markdown
Contributor

@wildan-m could you also address the review comments regarding code comment style?

errorFields.lastScrape is only ever written when a user-initiated action fails:
syncCard ('Update card') and updatePersonalCardConnection ('Fix card') both write
it in their failureData. It is never the stale server connection error, which lives
in card.errors and is gated separately. Filtering it therefore suppressed the wrong
thing: a sync the user had just triggered could fail and leave no RBR once they
navigated away from the card, while the stale error it was meant to hide was never
in that field. Drop the filter and cover both cases with tests.
@wildan-m

Copy link
Copy Markdown
Contributor Author

@MariaHCD bot feedbacks sorted

@MariaHCD
MariaHCD merged commit a06b85f into Expensify:main Aug 18, 2026
41 of 44 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🚧 MariaHCD has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

Nice, thanks everyone!

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.4.56-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

Help site docs — changes required ✅

I reviewed this PR and yes, a help site update is warranted. This PR changes how broken card-connection indicators behave once a card/feed has gone 90 days or more without a successful sync — the proactive reminders (the Time Sensitive task on Home, and the RBR red dots on the Wallet/workspace rows) stop showing, while the error stays on the card/feed so it remains fixable.

Two published articles document those exact indicators as the way to detect a broken connection, but don't mention that they stop appearing after ~90 days:

I opened a draft docs PR that adds a short clarifying note plus an FAQ entry to each article (additive only — no existing labels/steps changed):

➡️ #98930

Two notes:

  • The Account > Wallet navigation labels were verified against the live web UI. The broken-connection-specific labels (Time Sensitive, Fix card, the “Card feed connection is broken” banner) weren't reachable in the test account (it has no cards or workspaces), so I reused them verbatim from the already-published articles rather than inventing anything.
  • I couldn't auto-assign wildan-m — GitHub reports they aren't an assignable collaborator on Expensify/App, so the assignment was silently dropped. You're tagged in the PR body instead; feel free to self-assign if the option is available to you.

@wildan-m, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR Ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants