Repository navigation
Gate task assignee edit selection behind a Save button (a11y On Input, PR 15) - #101686
Conversation
Co-authored-by: truph01 <truph01@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Fixed and pushed — the
DetailsFailure: the only real failing check was - const initiallyFocusedOptionKey = sections
- .flatMap((section) => section.data)
- .find((mode) => isPersistedAssignee(mode.accountID))?.keyForList;
+ const initiallyFocusedOptionKey = sections.flatMap((section) => section.data).find((mode) => isPersistedAssignee(mode.accountID))?.keyForList;Verification run locally:
Also failing, not addressed: |
|
🤖 Web test steps verification results: ❌ fail On dev NewDot web, the edit-task assignee selector correctly stages a selection without closing (the WCAG 3.2.2 goal is met) and the new-task flow is unchanged, but the promised Save button never renders, so a staged assignee can never be committed and the task assignee cannot be changed at all from this screen.
Suggested fix direction: also pass a |
|
🤖 Android test steps verification results: ❌ fail Verified on standalone Android NewDot: tapping a row on an existing task's Assignee screen correctly stages the pick and keeps the modal open, but no Save button is ever rendered, so the assignee can never be committed — the edit flow is now a dead end.
Suggested fix direction: render the confirm button in |
|
@MelvinBot Merge main and mark this PR as ready |
…gnee-save Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
I merged |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 6bb44c3956
ℹ️ 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".
| ListItem={UserListItem} | ||
| onSelectRow={selectReport} | ||
| onSelectRow={isEditingTask ? setDraftAssignee : selectReport} | ||
| confirmButtonOptions={confirmButtonOptions} |
There was a problem hiding this comment.
In the existing-task flow, selecting a row now only calls setDraftAssignee, but BaseSelectionListWithSections constructs its Footer without forwarding confirmButtonOptions (src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx:341-346). Consequently, the Save button never appears on any platform, so touch users cannot invoke selectReport and assignee edits cannot be completed; either forward these options through the sectioned list or render a working confirmation control separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This finding is valid. BaseSelectionListWithSections builds its Footer with only footerContent and addBottomSafeAreaPadding, so confirmButtonOptions never reaches it and the Save button never renders (BaseSelectionListWithSections.tsx:342-347). The earlier web and Android test runs on this PR saw the same thing: tapping a row stages the assignee, but there's no way to save it.
PR 14 doesn't hit this because it uses the flat SelectionList, which does pass the options through.
There are two ways to fix it:
- Render the Save button through
footerContentin this modal. This only touches this file. - Pass
confirmButtonOptionstoFooterinBaseSelectionListWithSections. That's a shared change: 9 other screens passconfirmButtonOptionsto the sectioned list and could start showing a button.
truph01, should I implement this? If so, I'd go with option 1.
There was a problem hiding this comment.
Confirmed — BaseSelectionListWithSections doesn't pass confirmButtonOptions to Footer (unlike BaseSelectionList), so there's no way to commit the staged assignee on touch. Could we forward it there (after checking other sectioned-list callers that pass showButton), or render the Save footer in this screen instead?
There was a problem hiding this comment.
I forwarded it in the shared component in 1e98ff2. BaseSelectionListWithSections now passes confirmButtonOptions to Footer, so the Save button renders on this screen (BaseSelectionListWithSections.tsx:342-348).
No other screen changes. This modal is the only sectioned-list caller that passes showButton, and Footer draws the button only when showButton is true. The 9 other callers pass confirmButtonOptions only for Enter handling.
The container also skips its bottom safe-area padding when the button renders, so it isn't added twice. This matches BaseSelectionList (line 104).
Typecheck, the React Compiler check, and 62 SelectionList Jest tests pass. I didn't test it in the app.
truph01
left a comment
truph01
left a comment
There was a problem hiding this comment.
@MelvinBot Check these comments
| // Editing an existing task writes the assignee to the server on select. Staging the tap and writing only on Save | ||
| // stops the modal from closing on input (WCAG 3.2.2 On Input). The new-task flow already stages via | ||
| // `setAssigneeValue` and is confirmed on the create step, so it keeps selecting in place. |
| } | ||
| selectReport(draftAssignee); | ||
| }, | ||
| isDisabled: !draftAssignee, |
There was a problem hiding this comment.
Could we use isDisabled: !draftAssignee || draftAssignee.accountID === report?.managerID so Save stays off when the current assignee is tapped again, like PR 14 does?
There was a problem hiding this comment.
Done in 4e2f867. Save is now disabled until you stage someone other than the current assignee: isDisabled: !draftAssignee || draftAssignee.accountID === report?.managerID.
… is staged Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
I fixed both of your comments in 4e2f867 and replied in each thread:
The Codex finding is valid and blocks the fix: the sectioned list never renders the Save button, so assignee edits can't be saved. Your Type check passes. I didn't re-test in the app. |
|
This change is already on the branch. Commit 4e2f867 sets the Save button to |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-22.at.17.50.23.movAndroid: mWeb ChromeScreen.Recording.2026-10-22.at.17.48.27.movMacOS: Chrome / SafariScreen.Recording.2026-10-22.at.17.45.48.mov |
|
🚧 lakchote 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/lakchote in version: 9.5.20-0 🚀
|
|
A help site update is needed. I opened Docs updates for E/A#101686 Add Save step to reassigning a task. This PR means selecting a new assignee on an existing task no longer reassigns it until you select Save. That made this sentence in Assign a Task via Chat incorrect:
The docs PR adds "and then selecting Save" to that sentence. The Create a Task steps don't change because the new-task flow still applies the assignee when you select it. @truph01, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.20-5 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
PR 15 of the continue plan for the WCAG 3.2.2 "On Input" batch (Tier B — edit-mode only).
DynamicTaskAssigneeSelectorModalwrote the new assignee to the server and closed the modal the moment a row was tapped, so the layout changed context on input. Tapping a row now only stages the assignee, andeditTaskAssigneeruns from an explicit Save, matching the pattern already shipped in PRs 3–13.The Save button is scoped to the edit flow (
!isNewTaskFlow && !!report). The new-task flow already stages its choice viasetAssigneeValueand confirms it on the create step, so it keeps selecting in place and its behavior is unchanged.Notes on the implementation:
selectReport()is untouched: theHttpUtils.cancelPendingRequestscall, thesetAssigneeValueoptimistic chat lookup, and theoption.accountID !== report.managerIDno-op guard all still run in the same order — they just run fromonConfirminstead ofonSelectRow.isPersistedAssignee()so it can be reused.initiallyFocusedItemKeynow reads fromisPersistedAssignee()instead of the row'sisSelectedflag, so staging a row can't move the initial focus target.Fixed Issues
$ #74834
PROPOSAL: #74834 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Suggested coverage for the flows this PR touches:
Offline tests
Same as tests —
editTaskAssigneewas already optimistic, and this PR only moves when it fires, not how.QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas they need to test for regressions.
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