Gate workspace picker selection behind a Save button (a11y On Input, PR 11) - #99236
Conversation
…PR 11) Co-authored-by: truph01 <truph01@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🤖 Ran the testing steps on Web (dev NewDot). Result: ✅ pass — tapping a workspace row now stages the selection (highlights it and enables Save) instead of committing and navigating away; the change only commits when Save is pressed. Change workspace on an expense report (
Set default workspace ( Domain group preferred workspace ( No error dialogs/toasts were observed during the flow. EvidenceChange workspace picker open — current workspace checked, Save dimmed/disabledOther workspace row tapped — staged (checked, promoted to top), still on picker, Save now enabledAfter pressing Save — "You moved this report!" confirmation, picker closedReport header now shows the report on the newly selected Workspace 1 |
|
🤖 Ran the testing steps on Android (standalone NewDot, emulator). Result: ✅ pass — tapping a workspace row now stages the selection (highlights it and enables Save) instead of committing and navigating away; the change only commits when Save is pressed. Change workspace on an expense report (
The other two pages in this PR weren't exercised on Android: Set default workspace ( No JS console errors or error toasts observed during the flow. Note: "Change workspace" only appears in the More menu while the report is still Open/Draft, so the flow was exercised against a draft expense. EvidenceChange workspace picker open — current workspace checked, Save disabledOther workspace row tapped — staged (checked), still on picker, Save now enabledAfter pressing Save — "You moved this report!" confirmationReport now on the newly selected Workspace 1 |
|
@MelvinBot Mark this PR as ready |
|
🤖 Done — this PR is now marked Ready for review. |
![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: f75446aef0
ℹ️ 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".
| currentUserLogin: session?.email, | ||
| shouldShowPendingDeletePolicy: false, | ||
| selectedPolicyIDs: report.policyID ? [report.policyID] : undefined, | ||
| selectedPolicyIDs: currentSelection ? [currentSelection] : undefined, |
There was a problem hiding this comment.
When a user selects a workspace that is not already first—particularly while searching—passing the changing currentSelection to useWorkspaceList triggers its default shouldSortSelectedToTop behavior, immediately moving that row to the top before Save. During an active search, SelectionList retains its numeric focused index, so focus can now point to a different workspace and the next Enter press selects that unintended row; the immediate reorder also preserves the input-driven context shift this change is meant to prevent. Set shouldSortSelectedToTop: false here and in the corresponding SetDefaultWorkspacePage call while retaining selectedPolicyIDs for highlighting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Set shouldSortSelectedToTop: false
@MelvinBot, check this. I still want to move the selected option to the top, but not the checked option without saving
There was a problem hiding this comment.
Done — kept the saved workspace pinned to the top, and stopped the checked row from being pinned before Save. Pushed in 2d89ed7.
useWorkspaceList used one list (selectedPolicyIDs) for both the checkmark and the sort-to-top, so staging a draft selection reordered the list. I split them: a new optional policyIDsToSortToTop drives the sort and falls back to selectedPolicyIDs when omitted, so every existing caller is unchanged.
src/pages/DynamicReportChangeWorkspacePage.tsx:225— pinsreport.policyID(the saved workspace) while the checkmark follows the draft. Same order as before this PR; checking a row no longer moves it.src/pages/SetDefaultWorkspacePage.tsx:80—shouldSortSelectedToTop: false. Judgment call: this page passedselectedPolicyIDs: undefinedbefore the PR, so nothing was ever pinned here and the list was plain alphabetical. I kept that rather than newly pinning the active workspace. Say the word if you'd rather pin the active default here too.DomainGroupPreferredWorkspacePageneeded nothing — it sorts bycreateddate, so checking a row never reordered it.
That also covers the search-focus concern in the parent comment: with the order stable, the focused index can't end up pointing at a different workspace after a check.
Checks run
npm run typecheck✅eslinton the 3 changed files ✅ — 3 pre-existingno-inline-useOnyx-selectorerrors on untouched lines 93–95 ofDynamicReportChangeWorkspacePage.tsx(grandfathered by the seatbelt ratchet)react-compiler-compliance-checkon the 3 changed files ✅PolicyUtilsTest+tokenizedSearchTest(the suites coveringsortWorkspacesBySelectedandtokenizedSearch) ✅ — 406 testslint-changed/spell-changedcouldn't run: they needgit merge-base, and this is a depth-1 CI checkout
I could not verify this in the browser — the prestarted web session went blank and unresponsive (0 DOM nodes, no network activity) even though the dev server built fine, so there are no screenshots for this change. Worth a manual pass on the reorder behavior with 3+ workspaces.
|
The only failing checks are non-code gates that require human action, not a code fix — there's nothing here I can push a fix for:
Every actual code/build check is green: So no fix is needed — these two checks will clear on their own once the PR gets a reviewer + independent approval. |
|
No change since my last analysis — the only two failing checks ( |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
@Expensify/design is this new pattern on your radar to disable the confirmation button if the value selected hasn't changed? 🤔
I thought we had decided (maybe elsewhere) that we would still let them click Save even if the value didn't change? |
|
There's nothing for me to fix here — the only two failing checks are process/review checks, not code failures:
All build/test/lint/typecheck checks are passing. Neither failure can be resolved by pushing code to the branch, so I haven't made any changes. Next step: have a reviewer complete their checklist and approve to clear both checks. |
This is what I would assume we'd do too. |
|
@MelvinBot Merge main |
…-pickers-save Co-authored-by: truph01 <truph01@users.noreply.github.com>
The draft state, the checkmark and the Save button are one concern, so owning them in the shared component instead of threading draftPolicyID and confirmButtonOptions through each caller. Callers now pass a single shouldConfirmSelection flag, and onSelectWorkspace means the same thing in both modes: commit this workspace. This also makes the broken prop combinations unrepresentable - there is no longer a way to pass a draft with no Save button, or a Save button with no draft.
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-09-10.at.03.47.01.movScreen.Recording.2026-09-10.at.03.49.04.movScreen.Recording.2026-09-10.at.03.53.07.movAndroid: mWeb ChromeScreen.Recording.2026-09-10.at.03.58.07.moviOS: HybridAppScreen.Recording.2026-09-10.at.03.59.56.moviOS: mWeb SafariScreen.Recording.2026-09-10.at.04.01.11.movMacOS: Chrome / SafariScreen.Recording.2026-09-10.at.03.54.05.mov |
|
@codex review |
|
@MelvinBot Please fill in the Test and QA Test sections of the author checklist. Keep them as short as possible. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Filled in both sections in the PR body. Tests — steps 3-5 are the assertion set, reused across all three pickers so the list stays short:
QA Steps — I left every checkbox unchecked and did not touch the Two things worth a look while you're in there:
Steps 6 and 7 are derived from the navigation code, not from a live run. |
| showButton: true, | ||
| text: translate('common.save'), | ||
| onConfirm: () => selectPolicy(currentSelection), | ||
| isDisabled: currentSelection === activePolicyID, |
There was a problem hiding this comment.
Save stays disabled for the already-active workspace
Unlike the other two pages, selectPolicy here also navigates to navigateTo, so picking the current activePolicyID is a real action — could isDisabled be just !currentSelection?
There was a problem hiding this comment.
You're right that confirming the active workspace is a real action here, and I've changed it — but I gated on the draft rather than on !currentSelection, in 80cc7fb.
The actual defect is that the two were keyed off different values: the checkmark came from draftPolicyID, while isDisabled compared currentSelection to activePolicyID. So tapping the already-active workspace checked the row and left Save dead, with no way forward — exactly the case you spotted.
onConfirm: () => selectPolicy(draftPolicyID),
isDisabled: !draftPolicyID,Save is now enabled precisely when a row is checked. currentSelection is gone, since its activePolicyID fallback was only feeding those two lines.
Why not !currentSelection literally
currentSelection is draftPolicyID ?? activePolicyID, so !currentSelection would enable Save on open whenever activePolicyID is set but nothing is checked — and confirming would write that policy back as the default.
That state is reachable. The page only opens when shouldSelectPolicy is true, which requires falling past the activePolicy branch in usePolicyForMovingExpenses.ts:129-136. activePolicy comes from ActivePolicyProvider.tsx:19, whose selector returns undefined for a personal policy — but SetDefaultWorkspacePage.tsx:44 reads the raw NVP. So a user whose default is a personal policy (or one that's since been removed) opens this page with activePolicyID set and no row checked.
Gating on draftPolicyID handles both cases: nothing checked means disabled, and any checked row — including the active one — means enabled.
On reachability of your original case: the active workspace only appears in the list when it's a group policy that got disqualified upstream, which past this entry point means the Teachers Unite policy. Narrow, but the checkmark/Save mismatch was worth fixing on its own.
Checks: lint-changed ✅, typecheck ✅, React Compiler check ✅, spell-changed ✅. There's no test file for this page, and the dead-end path needs a specifically-shaped account, so I verified by reading the guard chain rather than in a browser.
|
@MelvinBot Remove all redundant comments you added in this PR |
selectPolicy on this page also advances the flow - it navigates to navigateTo when the picked policy has categories enabled - so confirming the already-active workspace is a real action, unlike on the other two pickers where it would be a no-op. isDisabled compared currentSelection against activePolicyID while the checkmark was driven by draftPolicyID, so the two could disagree: tapping the active workspace checked the row but left Save dead, with no way to proceed. Gate on draftPolicyID instead, which is exactly the row the list shows as checked. Guarding on !currentSelection would instead enable Save on open whenever activePolicyID is set but no row is checked - reachable, since the page opens with a set activePolicyID when that policy is personal or missing - and confirming would write that policy back as the default.
Drops the comments that restated the code they sat on, and trims the rest back to the one-line style of their surrounding files: - useWorkspaceList: drop the policyIDsToSortToTop JSDoc block. No other param in that type is documented, and the name plus the 'policyIDsToSortToTop ?? selectedPolicyIDs' fallback already say it. - DynamicReportChangeWorkspacePage: drop the policyIDsToSortToTop comment, which restated the prop name and its value. - BaseDomainGroupPreferredWorkspacePage: drop the file-header sentence that duplicated the shouldConfirmSelection prop doc, drop the checkedPolicyID comment (it described lines other than its own), and restore selectedPolicyID/onSelectWorkspace to their original wording. - SetDefaultWorkspacePage: keep both comments but cut the halves that restated the flag they annotate. Comment-only change; no logic touched.
|
Done — pushed in Removed outright (each restated the code it sat on):
I also reverted Kept, since they carry a reason the code can't:
Tell me if you'd rather I strip any of those four too. VerificationComment-only change — no logic touched, which
|
| showButton: true, | ||
| text: translate('common.save'), | ||
| onConfirm: () => selectPolicy(draftPolicyID), | ||
| // selectPolicy also advances the flow, so re-confirming the already-active workspace is a real action rather than a no-op. |
There was a problem hiding this comment.
Removed in 72574f1d654.
You're right that it was dead weight. The comment justified not comparing draftPolicyID against activePolicyID — but that comparison was already gone as of 80cc7fb, so it was explaining an absence. isDisabled: !draftPolicyID says "enabled when a row is checked" on its own.
Comment-only change. lint-changed ✅, typecheck ✅, React Compiler check ✅, spell-changed ✅.
The comment justified not comparing draftPolicyID against activePolicyID, but that comparison is already gone as of 80cc7fb. 'isDisabled: !draftPolicyID' reads plainly on its own.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thank you! Gonna move this ahead to avoid conflicts
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 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.4.77-0 🚀
|
|
Yes, help site changes are required. Draft PR: #101009 Two articles told members that tapping More > Change workspace moves the report. After this PR, the pick is staged and nothing happens until Save is pressed, so both now include the Save step:
The first file also wrote the label as "Change Workspace" in five places. I verified against the running app that the real label is Change workspace (sentence case), so I corrected those per What I verified in the live UI, and what I deliberately left aloneVerified on web rather than inferred from the diff:
No changes needed for the other two pickers in this PR:
I also left One thing worth a second look: I could not reach Domains in the test account, because creating a domain hit a magic-code account-validation wall. So the exact Preferred workspace row label is unconfirmed. That does not affect this PR, since I made no domain doc changes, but if you think the domain group picker deserves its own step-by-step section, say so and I'll add one.
@truph01, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #101013 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.77-4 🚀
Bundle Size Analysis (Sentry): |
8 participants
Explanation of Change
This addresses a WCAG 3.2.2 "On Input" accessibility violation, part of the batched plan in issue #74834 (PR 11). Three workspace-picker pages committed the selection (and navigated away) the instant a row was tapped, so the layout/context changed without the user explicitly confirming — disorienting for screen-reader and low-vision users.
All three pages now stage the tapped workspace in local state and only commit it when the user presses the Save button (via
confirmButtonOptionsonSelectionList), matching the pattern already shipped in PRs 3–8. The staged row is highlighted viaselectedPolicyIDs, and the Save button is disabled until the staged selection differs from the current value:DomainGroupPreferredWorkspacePage— plain preferred-workspace write, committed on Save.SetDefaultWorkspacePage— theareCategoriesEnabledbranch (navigate onward vs.goBack) and thenavigateToguard are preserved by having Save call the existingselectPolicy.DynamicReportChangeWorkspacePage— the billingRESTRICTED_ACTIONearly-return, the commuter-exclusion distance guard, and the per-report-type API calls (moveIOUReportToPolicy…/changeReportPolicyAndInviteSubmitter/changeReportPolicy) are all preserved by having Save call the existingselectPolicy.🤖 Generated by MelvinBot. Checks run locally:
lint-changed✅,typecheck-tsgo✅. React Compiler compliance check couldn't run in this environment (base-ref resolution); no manual memoization was added, so CI will validate. Fullnpm test/ storybook smoke deferred to CI.Fixed Issues
$ #74834
PROPOSAL: #74834 (comment)
Tests
All three pickers behave identically, so steps 3-5 are the assertion set reused below.
Offline tests
QA Steps
Same as Tests.
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