Repository navigation
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!
|
|
🤖 Android test steps verification results: Tap-to-stage works — tapping a row moves the checkmark and no longer closes the page, so the autosave-on-input this PR targets is gone. But the Save button never renders, so a staged list-field selection can never be committed.
Setup: created a workspace, upgraded to Control in-app to enable Report Fields, added a list field "Project" (Alpha/Beta/Gamma, initial value Alpha), a text field "Notes", and a date field "Due Date", then created an expense report on that workspace. No JS console errors observed. |
|
🤖 Web test steps verification results: ❌ fail Tap-to-stage works — clicking a row moves the checkmark and keeps the page open, so the save-on-input this PR targets is gone. But the Save button never renders for the list-type report field picker in any state, so a staged change can never be committed. This reproduces the same root cause found on Android:
Setup: workspace on the Control plan with Report Fields enabled and three fields — Project (list: Alpha/Beta/Gamma, initial value Alpha), Notes (text), Due Date (date) — then an expense report on that workspace. Browser console inspection is out of scope for the web harness, so console errors were not captured; no in-app error banners appeared during the session. |
|
@MelvinBot Merge main and mark this PR as ready |
…eld-dropdown-save Co-authored-by: truph01 <truph01@users.noreply.github.com> # Conflicts: # src/pages/EditReportFieldDropdown.tsx
|
I merged I resolved one conflict in ESLint and TypeScript pass locally. |
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: ff34cb7259
ℹ️ 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".
| onSelectRow={(option) => onSubmit({[fieldKey]: option?.text ?? ''})} | ||
| // Re-selecting the value the field already holds stages that same value rather than an empty string, | ||
| // which leaves '' free to mean "cleared". Staging the stored value keeps Save disabled. | ||
| onSelectRow={(option) => setDraftValue(option?.text ?? '')} |
There was a problem hiding this comment.
When the user taps the currently checked option—or selects a new option and taps it again—this always stages option.text, so the checkmark cannot be removed and onSubmit can never receive ''. This makes clearing an optional list report field impossible through this picker; compare the selected option with currentValue and stage an empty string when they match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think we should change this. On main, tapping the selected list value no longer clears the field. Restore: Show up to three report fields per row and edit them inline made that change on purpose. It replaced the old fieldValue === option.text ? '' toggle with submitting the tapped value.
This PR only moves the save behind a Save button for WCAG 3.2.2. It shouldn't bring back toggle-to-clear. Tapping the selected row now picks the same value, so Save stays disabled. The test in EditReportFieldDropdownTest.tsx covers this.
If you want toggle-to-clear back, let me know and I'll add it.
| onSelectRow={(option) => onSubmit({[fieldKey]: option?.text ?? ''})} | ||
| // Re-selecting the value the field already holds stages that same value rather than an empty string, | ||
| // which leaves '' free to mean "cleared". Staging the stored value keeps Save disabled. | ||
| onSelectRow={(option) => setDraftValue(option?.text ?? '')} |
There was a problem hiding this comment.
The existing cases in tests/ui/EditReportFieldDropdownTest.tsx (lines 104–129) press a row and immediately assert that onSubmit was called. Since this callback now only updates local state, both tests fail whenever the UI suite runs. Update them to assert that row selection does not submit, press the new Save button, and then verify the submitted value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already handled in e5fdb99. The EditReportFieldDropdownTest.tsx tests now check three things:
- Picking a row doesn't submit.
- Pressing Save submits the picked value.
- Picking the stored value again leaves Save disabled.
That commit also fixed SelectionListWithSections so it renders the Save button. No further change needed.
…sts for Save Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Fixed the failing
The dropdown and SelectionList tests, lint, and typecheck pass locally. |
truph01
left a comment
truph01
left a comment
There was a problem hiding this comment.
@MelvinBot Check this comment
| // Tapping a row used to call `onSubmit` straight away, which closed the page on input (WCAG 3.2.2 On Input). | ||
| // The tap now only stages the value, and `onSubmit` runs from an explicit Save. | ||
| // An empty string is a meaningful staged value (it clears the field), so `undefined` means "nothing staged yet". |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Re-selecting the value the field already holds stages that same value rather than an empty string, | ||
| // which leaves '' free to mean "cleared". Staging the stored value keeps Save disabled. |
| onSelectRow={(option) => onSubmit({[fieldKey]: option?.text ?? ''})} | ||
| // Re-selecting the value the field already holds stages that same value rather than an empty string, | ||
| // which leaves '' free to mean "cleared". Staging the stored value keeps Save disabled. | ||
| onSelectRow={(option) => setDraftValue(option?.text ?? '')} |
| onSelectRow={(option) => onSubmit({[fieldKey]: option?.text ?? ''})} | ||
| // Re-selecting the value the field already holds stages that same value rather than an empty string, | ||
| // which leaves '' free to mean "cleared". Staging the stored value keeps Save disabled. | ||
| onSelectRow={(option) => setDraftValue(option?.text ?? '')} |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
All four comments you flagged already have replies. No further change is needed. I confirmed this against the PR head, d076696.
|
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Is this PR necessary anymore due to #104124?
2026-10-22_21-36-41.mp4
lakchote
left a comment
lakchote
left a comment
There was a problem hiding this comment.
Agree with @trjExpensify. Since #104124, list report fields are picked from an inline dropdown on the report, and the full page picker is only reachable from a direct URL. I'd close this one. If we still want that page covered, the Save gate should only apply to DynamicEditReportFieldPage.
| // signalling "no change" this way is no longer needed and leaves '' free to mean "cleared". | ||
| onSelectRow={(option) => onSubmit({[fieldKey]: option?.text ?? ''})} | ||
| onSelectRow={(option) => setDraftValue(option?.text ?? '')} | ||
| confirmButtonOptions={{ |
There was a problem hiding this comment.
This is also the list in the inline report field dropdown from #104124 (ReportFieldInlineInput), so the change hits that dropdown too. Picking a value there stops saving, and the Save button goes into a popover sized with hasButton: false, so a short list gets squeezed to about one visible row. If we keep this PR, the Save gate should only apply to DynamicEditReportFieldPage, the way #101686 kept it to isEditingTask.
| const optionSections = getReportFieldOptionsSection({ | ||
| searchValue: debouncedSearchValue, | ||
| // Live value drives the checkmark, so tapping a row marks it immediately. | ||
| // Staged value drives the checkmark, so tapping a row marks it immediately. |
There was a problem hiding this comment.
NAB: the comment above still says the live value drives the checkmark, it's the staged value now. Can we update it so the two agree?
|
@truph01 should we close this? |
|
@MelvinBot Merge main |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
🤖 Web test steps verification results: ❌ fail The web app rendered a blank page, blocking sign-in and dropdown verification. No source was edited during testing.
Merged |
|
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. |
|
Closed since it is redundant now |
Explanation of Change
PR 16 of the continue plan for the WCAG 3.2.2 "On Input" batch (Tier C — the reusable dropdown child).
EditReportFieldDropdowncalled itsonSubmitprop directly fromonSelectRow, so tapping a row saved the report field and closed the page on input. The tap now only stages the value in local state, andonSubmitruns from an explicit Save.Call-site audit (the gate the plan asked for):
EditReportFieldDropdownhas exactly one consumer —DynamicEditReportFieldPage, forREPORT_FIELD_TYPES.LIST. Its sibling children (EditReportFieldText,EditReportFieldDate) already submit through a form with its own Save button, so this change makes the list variant consistent with them rather than diverging from any other caller.Notes on the implementation:
onSubmitreceives the same shape it did before.undefined(not'') means "nothing staged yet", because an empty string is itself a meaningful value here — it clears the field.initiallyFocusedItemKeystill uses the frozenuseInitialSelectionvalue, so the pinned "Selected" section and the initial focus target don't move while the user browses.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 flow this PR touches:
Offline tests
Same as tests — the write behind
onSubmitwas 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