Repository navigation
Restore: Show up to three report fields per row and edit them inline - #102142
Conversation
Reverts the revert in PR 101854, restoring the changes originally merged in PR 100735 so the work is available on a branch for regression triage. Co-authored-by: daledah <daledah@users.noreply.github.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. Alternatively, leave it as is and reply with |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Android test steps verification results: ✅ pass This PR has no written test steps — the One thing worth a look, not a failure: the dropdown field's tap target reads narrower than its row. Taps landed reliably on the label and value text but not across the full row width. The PR body says it deliberately restores the reverted code unchanged so the known regressions can be listed against it. This Android pass did not reproduce any of those regressions on a phone layout — if the regressions you want captured are wide-layout or three-per-row specific, they need a web or tablet run instead. This PR also carries the |
|
🤖 Web test steps verification results: ✅ pass All seven checks passed on dev NewDot web: report fields render as an inline grid — three per row on a wide layout, one per row at mobile width — and text, date and dropdown fields all edit and save in place without navigating to a separate page. No failed network calls or error dialogs occurred during the flow. This PR still has no written test steps (the The PR body says it deliberately restores the reverted code unchanged so the known regressions can be listed against it. Neither this web pass nor the earlier Android pass reproduced a regression on the derived steps, so the regressions you want captured are probably in a flow these checks do not reach — a specific field type, a violation or required-field case, an offline or pending state, or a particular field count. Tell me which flow to drive and I will rerun against it. |
|
@MelvinBot please open this PR |
|
Done — this PR is out of draft and open for review. Two things to know before a reviewer picks it up:
|
|
@MelvinBot please look at Blocker list above and fix issue 1, issue 4 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d42b70101
ℹ️ 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".
| it('shows up to three fields per row on a wide layout', async () => { | ||
| await renderReportFields(7); |
There was a problem hiding this comment.
Add Given/When/Then documentation to every added test
The newly added test cases use ad-hoc comments (or none) instead of the required // Given, // When, and // Then sections explaining why each scenario matters. Add all three sections to every case in this test file (and the other newly added/modified tests) so the test intent remains maintainable.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| if (trimmedValue === lastSavedValue.trim()) { | ||
| setValidationError(''); | ||
| return; |
There was a problem hiding this comment.
Restore the displayed value when a trimmed edit is skipped
For an existing non-empty text field, entering only leading or trailing whitespace (for example, changing Value to Value) takes this equality branch and intentionally skips the update, but leaves draftValue as the unsaved text. Since the value prop does not change when no update is sent, the inline field continues displaying a value that will disappear after remount or refresh; reset the draft to the saved value before returning.
Useful? React with 👍 / 👎.
| ); | ||
|
|
||
| return ( | ||
| // No `popoverWidth`, so the dropdown falls back to `CONST.POPOVER_DROPDOWN_WIDTH` — the same width every |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
The comment uses an em dash in its own sentence ("...falls back to CONST.POPOVER_DROPDOWN_WIDTH — the same width every Spend filter dropdown uses..."). Comments should read as plain sentences without em/en dashes.
Split the thought into separate sentences or use a comma instead:
// No `popoverWidth`, so the dropdown falls back to `CONST.POPOVER_DROPDOWN_WIDTH`, the same width every
// Spend filter dropdown uses. Matching the field's own width instead made narrow fields open odd, cramped
// popovers and wide ones open oversized.Reviewed at: 1d42b70 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| errorText={violationError} | ||
| inputStyle={styles.pointerEventsNone} | ||
| icon={icons.DownArrow} | ||
| // `pr0` makes the icon container's padding asymmetric, so it has to flip in place — |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
The comment ends with an em dash joining two sentences ("...so it has to flip in place — flipUpsideDown would rotate..."). Write the two thoughts as separate sentences.
// `pr0` makes the icon container's padding asymmetric, so it has to flip in place.
// `flipUpsideDown` would rotate that padding too and shift the caret left when open.Reviewed at: 1d42b70 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const {sortedPolicyReportFields, fieldValues, fieldsByName} = useMemo(() => { | ||
| const {fieldValues: values, fieldsByName: byName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); | ||
| const sorted = Object.values(byName) | ||
| // Only used to decide whether the report field block is worth rendering — `MoneyRequestViewReportFields` builds and |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
The comment uses an em dash in its own sentence ("...whether the report field block is worth rendering — MoneyRequestViewReportFields builds and resolves..."). Replace the em dash with a period and a new sentence.
// Only used to decide whether the report field block is worth rendering.
// `MoneyRequestViewReportFields` builds and resolves the fields it displays itself.Reviewed at: 1d42b70 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| onSubmit: (form: Record<string, string>) => void; | ||
|
|
||
| /** Whether the search input is shown. Callers with a short list hide it, since there is little to search through. */ | ||
| shouldShowTextInput?: boolean; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
This change adds five new boolean configuration flags at once (shouldShowTextInput, shouldShowRecentlyUsedOptions, shouldPinSelectedOption, shouldShowSectionTitles, shouldUseCompactRows), each of which drives an if/ternary branch inside the component to toggle the presence of the search input, the recently-used section, section titles, selection pinning, and row height. Controlling feature presence and layout via boolean flags couples the component to each consumer and grows its API every time a caller needs a new variant.
Prefer composition: expose these as composable behaviors the caller opts into (e.g. <EditReportFieldDropdown.Search />, <EditReportFieldDropdown.RecentlyUsed />) or pass the already-computed section data / list config in, so the component body stays stable rather than branching on each flag. At minimum, consolidate the presentation variants (compact vs. page rows, titles on/off) rather than threading each as an independent boolean.
Reviewed at: 1d42b70 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Not changed. daledah, let me know if you want this refactor anyway.
EditReportFieldDropdownis a thin wrapper aroundSelectionListWithSections. The rule exempts wrappers around list components.- Four of the five flags do not branch the render.
shouldShowTextInputgoes straight to the list's own prop.shouldShowRecentlyUsedOptions,shouldPinSelectedOptionandshouldShowSectionTitlesonly change the section data built bygetReportFieldOptionsSection. shouldUseCompactRowsonly changes row height.- A compound-component refactor would also change the RHP caller in
DynamicEditReportFieldPage. That is a lot of scope for a restore PR that is working through a blocker list.
| // Same override the Spend single-select dropdowns use to shorten the default 64px option row. It also makes | ||
| // the rows match the 52px `getSelectionListPopoverHeight` already assumes, so the popover stops being sized | ||
| // for less content than it holds. | ||
| style={shouldUseCompactRows ? {listItemWrapperStyle: {minHeight: variables.optionRowHeightCompact}} : undefined} |
There was a problem hiding this comment.
❌ UI-3 (docs)
This passes an inline style object literal ({minHeight: variables.optionRowHeightCompact}) into the list's listItemWrapperStyle. Inline style objects allocate a new object on every render and duplicate a value that belongs in the style system. Since variables.optionRowHeightCompact is static, define a named style helper and reference it instead.
// in styles
compactOptionRow: {minHeight: variables.optionRowHeightCompact},
// usage
style={shouldUseCompactRows ? {listItemWrapperStyle: styles.compactOptionRow} : undefined}Reviewed at: 1d42b70 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@MelvinBot please check the comments from codex above |
|
@AndrewGable i updated, please check again |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 AndrewGable 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/AndrewGable in version: 9.5.8-0 🚀
|
|
No help site update is needed. The help site has no step that tells members to open a report field on a page to edit it. This PR moves that editing inline, but it doesn't make any existing sentence wrong. Enable Report Fields only covers how admins set up and delete fields in workspace settings, and that flow is unchanged. |
|
Deploy Blocker #103860 was identified to be related to this PR. |
|
Deploy Blocker #103909 was identified to be related to this PR. |
|
Deploy Blocker #103913 was identified to be related to this PR. |
|
Deploy Blocker #103917 was identified to be related to this PR. |
|
Deploy Blocker #103918 was identified to be related to this PR. |
|
Deploy Blocker #103922 was identified to be related to this PR. |
|
Deploy Blocker #103926 was identified to be related to this PR. |
|
Working on the revert for all the blockers. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.8-5 🚀
Bundle Size Analysis (Sentry): |


















Explanation of Change
This PR restores the work that was reverted, unchanged. It is a revert of the revert in [CP Staging] Revert: Show up to three report fields per row and edit them inline, which backed out Show up to three report fields per row and edit them inline because of regressions in the report fields UI.
daledahasked for the reverted changes to be put back on a branch as-is so the regressions can be listed against them. No fixes have been applied — the known regressions from the original PR are still present. Nothing here is new work.The restore was produced by reverse-applying the revert diff onto current
main. The resulting diff is the exact inverse of the revert, file for file and line for line. The only hunk that needed a hand edit wasconfig/eslint/eslint.seatbelt.tsv, whose surrounding entries had drifted onmain; its two target entries were set back to the values they held at the original merge commit44bd04c— thesrc/pages/DynamicEditReportFieldPage.tsxno-unsafe-type-assertionentry removed, andsrc/components/ReportActionItem/MoneyReportView.tsxset back to1.What the restored change does (from the original PR)
Report fields used to render as a vertical stack of full-width rows, each one navigating to the report field editor page to change a value. With nine report fields that filled the whole screen and pushed the expense list out of view, and every edit cost a navigation.
Up to three fields per row.
MoneyRequestViewReportFieldschunks the sorted fields into rows ofCONST.REPORT_FIELDS_PER_ROW(3) and lays each row out as aflexRowof equal-width cells. A partly filled last row is padded with empty cells so its fields keep the same width as the rows above. On a narrow layout the column count drops to 1. Both existing call sites — the report list header and the empty report view — render the same component.Inline editing.
ReportFieldInlineInputrenders each field as one of our regular 52px inputs instead of a row that navigates away:textTextInput, saved on blur and on submit, skipped when unchangeddateDatePicker— the calendar opens over the reportdropdownEditReportFieldDropdown, so search and recently-used values are preservedformulaShared save path. The save logic that lived inline in
DynamicEditReportFieldPagemoved intouseSaveReportField, so violation, ASAP-submit and recently-used handling only exists in one place.Fixed Issues
$ #100259
PROPOSAL: #100259 (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.
Offline tests
QA Steps
Precondition: A Control workspace with Report fields enabled and these report fields:
Text A: Text, with initial valuexText B: Text, with any initial valueDate: DateList: List, with at least 8 valuesFormula: FormulaText A, type a new value, and click away. Verify the value saves without leaving the report, and a system message shows the change.Text Aand press Enter, then click away. Verify the empty value saves with no blocking error, and a "required" violation shows under the field.Text Ais still empty.Date. Verify the calendar opens in place. Click the month or year header and verify the Month/Year picker opens with the background dimmed. Pick a date and verify it saves.List. Verify the option list opens under the field with a search input, and stays open. Pick a value and verify it saves.Text Bso the keyboard is open, then tapList. Verify the option list opens and stays open.Formulais read-only.List. Verify there is no extra blank space at the bottom of the option list. Go back online.Text BandDate. Go back to the report.Text BandDatestill show their values, each with a trash button on the right edge. VerifyListandFormulahave no trash button.Dateand confirm "Delete field". Verify the field is removed. Repeat forText B.Text BandDatestay removed.AI Tests
Automated checks run locally on this branch:
npm run typechecknpm run lint-changednpm run spell-changednpm run react-compiler-compliance-check check(9 changed components/hooks)src/components/DatePicker/index.tsxfails, but pre-existing — theorigin/mainversion of that file fails identically ("Cannot access refs during render", both babel and oxc), so this is not a regressionnpm test -- tests/ui/MoneyRequestViewReportFieldsTest.tsx tests/ui/EditReportFieldDropdownTest.tsx tests/unit/ReportFieldOptionsListUtilsTest.ts tests/ui/MoneyReportViewTest.tsxnpm test -- tests/actions/ReportFieldTest.ts tests/ui/DynamicReportDetailsPageTest.tsx tests/ui/MoneyRequestReportViewTest.tsx tests/ui/MoneyRequestReportTransactionsNavigationTest.tsxnpm run lint(full repo)lint-changedcovers every file in this diffoxfmt --checkPR 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