Repository navigation
Gate company card and Expensify Card feed selection behind a Save button (a11y On Input, PR 9) - #99240
Conversation
…ton (a11y On Input, PR 9) Co-authored-by: truph01 <truph01@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
🤖 Web testing — blocked on account dataI could not live-verify the Save-gated feed selectors on Web. The change lives at Workspace → Company cards and Workspace → Expensify Card, but the CI test account has no workspace at all (the Workspaces tab shows "No workspaces yet"), so neither feed-selector page is reachable. Even creating a fresh workspace wouldn't help: a new workspace has zero company-card and zero Expensify Card feeds connected (that needs a real bank/card connection), so there'd still be no feed rows to stage/select between. What I could not verify (needs a workspace with ≥1 feed per surface):
Code-level corroboration (read-only, not a live test)The PR code implements exactly the described behavior on both pages:
This confirms the logic is wired as intended, but it is not a substitute for the live UI verification above. To get a full Web pass, this needs a test account/session pre-provisioned with a workspace that already has ≥1 company-card feed and ≥1 Expensify Card feed connected so the feed selector actually renders. Happy to re-run once such an account is available. |
|
@MelvinBot Mark this PR as ready |
|
🤖 Done — this PR is now marked Ready for review (out of draft). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d8f57d164
ℹ️ 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 selectFeed = (feed: ExpensifyFeedListItem) => { | ||
| setDraftFundID(feed.value); | ||
| }; |
There was a problem hiding this comment.
Apply the draft before starting card issuance
When multiple primary feeds exist, selecting feed B now only updates draftFundID, but the still-visible Issue card footer continues to derive issueCardFundID from lastSelectedExpensifyCardFeedID. If the user selects B and then taps Issue card without first pressing Save, handleAddCardPress writes the old feed A back to Onyx, and the issuance flow uses that old feed via useDefaultFundID, potentially issuing the card under the wrong program. Derive this footer action from currentSelectedFundID or disable it until the draft has been saved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Check this comment.
Do we have same issue with src/pages/workspace/companyCards/WorkspaceCompanyCardFeedSelectorPage.tsx?
There was a problem hiding this comment.
No — WorkspaceCompanyCardFeedSelectorPage does not have this problem. The Codex finding on the Expensify Card page is real, though.
Why the Expensify Card page is affected: the footer's Issue card action derives its fund ID from the committed value, then writes it back to Onyx:
getIssueCardFundIDmatches againstlastSelectedExpensifyCardFeedID, notcurrentSelectedFundID.handleAddCardPressthen callsupdateSelectedExpensifyCardFeed(issueCardFundID, policyID)— re-committing feed A after the user staged feed B.
Why the company cards page is not: its footer action, onAddCardsPress, never reads or writes the selected feed — it only calls clearAddNewCardFlow() and navigates. There is no equivalent of issueCardFundID, so there's no stale value to write back. The add-new flow later calls updateSelectedFeed with the newly created feed (CardInstructionsStep.tsx:62, BankConnection/index.tsx:156), never a stale one.
The only other updateSelectedFeed call on that page is selectOtherFeed, which commits the feed the user just tapped — correct by construction.
One shared minor behavior worth a decision (not the Codex bug)
On both pages, if a user stages a feed and then taps a "from other workspaces" row, the staged draft is silently discarded and the tapped other-workspace feed is committed instead. That's per the PR's stated design (footer rows stay immediate because they kick off async link/verify flows), and the committed feed is the one the user explicitly tapped — so it isn't a stale-write. Flagging only in case you'd rather clear the draft explicitly for clarity.
Verified against the merged head a07eb68. This is a static read of the code — I did not live-test it.
There was a problem hiding this comment.
Coming from #99240 (comment):
@MelvinBot Disable it until the draft has been saved.
There was a problem hiding this comment.
Done — the Issue card footer row is now disabled while an unsaved feed is staged. Pushed as 5a0c71f3.
The gate is a single derived flag, applied only to the row's press target:
WorkspaceExpensifyCardFeedSelectorPage.tsx:111—hasUnsavedFeedSelectionis true when a feed has been staged and differs from the saved one.WorkspaceExpensifyCardFeedSelectorPage.tsx:238—isDisabled={hasIssueCardFundID && hasUnsavedFeedSelection}on theMenuItemAction.
So handleAddCardPress can no longer fire while the list shows one feed and issueCardFundID still points at another. Pressing Save commits the draft, which makes draftFundID equal the saved feed and re-enables the row.
Why the other footer rows are left alone
The && hasIssueCardFundID guard matters — the row has two variants and only one is affected:
hasIssueCardFundIDtrue → "Issue card" runshandleAddCardPress, which writesissueCardFundIDback to Onyx. This is the one Codex flagged, and the one now gated.hasIssueCardFundIDfalse → "Issue new card" runshandleSetUpNewProgramPress, which only navigates to bank account setup and never touches the selected feed. Disabling it would block program setup for no reason, so it stays enabled.
The "from other workspaces" rows also stay immediate: selectOtherFeed commits feed.value from the row that was actually tapped, so it can't go stale against a draft.
Checks run
Passing: typecheck on the app tsconfig.json ✅, react-compiler-compliance-check check ✅, cspell ✅.
Couldn't run in this environment: eslint and prettier — eslint-seatbelt and @trivago/prettier-plugin-sort-imports are missing from node_modules, and lint-changed additionally can't resolve a merge-base in this shallow clone. typecheck reports two pre-existing errors in scripts/lint.ts from that same missing package, unrelated to this change. CI will cover all of these.
I also attempted a web run to see the disabled state in the UI, but the browser session never rendered past a blank page, so this change is not runtime-verified. Worth noting that reproducing it by hand needs a workspace with two or more Expensify Card programs.
|
I looked into the two failing checks — neither is a code problem, so there's nothing for me to push a fix for.
Both are process gates that only a human reviewer can satisfy — they're on the standard ignore list precisely because they can't be resolved with a code change. Every actual code/build/test check (ESLint, Oxfmt, typecheck, React Compiler, Storybook, Bun/unit tests, perf, builds) is green ✅. No commit was pushed. |
|
Same situation as my previous comment — the only two red checks are human-only process gates, not anything I can fix with code:
Every actual code/build/test check is green ✅. There's no code failure here, so I haven't pushed anything. These will resolve on their own once a reviewer approves and fills out the checklist. |
|
@MelvinBot Merge main |
|
Merged Merge commit: Checks on the new commit are green so far — ESLint, Oxfmt, React Compiler Compliance, Bun tests, Jest (jobs 2–8), Storybook, spellcheck, knip, and all builds pass. Evidence (1)Recordings- [▶ Session recording of the verification run](https://melvinbot-assets.exops.io/session-33320889613-1-12ec853e-a887-4b65-8fd2-324b0d44e5f9.webm) |
|
No code fix is needed — nothing is actually broken. Every build, lint, type, test, and perf check on this PR passes. The one red check is To turn it green, this PR needs an approving review from an independent Expensify reviewer. Nothing to push. Check status
|
…t and while offline Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
I fixed both review comments in acf98d9 and replied in each thread. Both pages had both gaps, so each fix covers the company card page and the Expensify Card page.
I added tests for both cases in |
|
@lakchote Could you trigger adhoc build and request QA to test this issue? |
|
Triggered adhoc build here: https://github.com/Expensify/App/actions/runs/36599425839 |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Asked QA for a retest here |
|
@MelvinBot Resolve conflicts |
…-selectors-save Co-authored-by: truph01 <truph01@users.noreply.github.com> # Conflicts: # src/pages/workspace/expensifyCard/WorkspaceExpensifyCardFeedSelectorPage.tsx
|
I merged
Typecheck passes, and so does |
|
@MelvinBot Resolve conflict |
…-selectors-save Co-authored-by: truph01 <truph01@users.noreply.github.com> # Conflicts: # src/pages/workspace/companyCards/WorkspaceCompanyCardFeedSelectorPage.tsx # src/pages/workspace/expensifyCard/WorkspaceExpensifyCardFeedSelectorPage.tsx
|
I resolved the merge conflicts with Both conflicts came from the main change that removed the dead
The TypeScript check passes. ESLint reports four errors in these files, but |
|
@mountiny all yours |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.6-0 🚀
|
|
A help site update is needed. I opened Docs updates for E/A#99240 Add Save step when selecting a company card feed. This PR means choosing a company card feed no longer applies it until you select Save. Three help site sentences described the old behavior, where choosing a feed applied it right away:
Each sentence now ends with "then select Save." No Expensify Card article mentions the removed Issue card row on the feed selector, so those articles need no change. @truph01, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #103551 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |



Explanation of Change
This addresses a WCAG 3.2.2 "On Input" accessibility violation, part of the batched plan in issue #74834 (PR 9). These two near-identical feed selectors committed the selected feed and navigated back the instant a row was tapped, so the layout/context changed without the user explicitly confirming — disorienting for screen-reader and low-vision users.
Both pages now stage the tapped feed in local state and only commit it when the user presses the Save button (via
confirmButtonOptionsonSelectionList), matching the pattern already shipped in PRs 2–8. The staged row is highlighted. Save stays enabled for whichever row is checked — matching the old tap-to-commit behaviour, where every row was submittable — and is only disabled when no row on the page is checked.Every selectable row on both pages is gated behind Save, including the rows under From other workspaces. Those rows render as ordinary selectable list rows — identical in appearance to the primary feed rows — so leaving them immediate reintroduced the very same violation on the very same page, and caused real confusion in review. Tapping one now only stages it; the link/verify flow runs from Save.
WorkspaceCompanyCardFeedSelectorPage—selectFeedstages both the primary rows and the other-workspace rows. Save runsupdateSelectedFeed+goBackfor a primary feed, orlinkOtherWorkspaceFeed(link the feed to this policy, then select it; may first route to add/verify work email) when the staged feed belongs to another workspace. The Add cards footer row is a navigation action rather than a selectable row, so it stays immediate.WorkspaceExpensifyCardFeedSelectorPage— the same split: Save runsresetCardFlowState+updateSelectedExpensifyCardFeed+goBackfor a primary feed, orlinkOtherWorkspaceFeedfor an other-workspace feed.One consequence of gating the other-workspace rows, worth a look on review:
ScrollViewinstead of aSelectionList, so it has noconfirmButtonOptionsfooter — gating the rows without adding a button would have made them dead ends. It is aFixedFooter+ButtonmirroringSelectionList's ownFooter, and theScrollViewgives upaddBottomSafeAreaPaddingwhile it renders so the padding is not applied twice.Save's disabled rule is membership-based (
is the checked feed actually a row on this page) rather than an inequality against the active feed. Inequality would have permanently disabled Save for a fallback feed that lands inotherFeedswhile still resolving as the default fund, so that feed could never be linked; and a bare truthiness check would have enabled Save in the no-primary-feeds state, where the default fund resolves to the workspace account ID rather than to any listed feed.🤖 Generated by MelvinBot. Checks run locally on the latest commit:
lint-changed✅,spell-changed✅,typecheck✅,react-compiler-compliance-check✅ (babel=compiled, oxc=compiled on both files), and the 6 related Jest suites ✅ (60 tests). Neither page was verified in a browser — see the note in the Tests section.Fixed Issues
$ #74834
PROPOSAL: #74834 (comment)
Tests
Precondition: an admin on a workspace with at least two company card feeds, and an admin on a workspace with at least two Expensify Card feeds.
1. Company cards — a feed choice is only submitted by pressing Save
2. Expensify Card — a feed choice is only submitted by pressing Save
Offline tests
QA Steps
Same as Tests 1 and 2 above, run on staging. The QA account needs to be an admin on a workspace with at least two company card feeds and on a workspace with at least two Expensify Card feeds; step 6 of each test additionally needs a feed that lives on a different workspace.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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