Repository navigation
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
All contributors have signed the CLA ✍️ ✅ |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
20f4f28 to
7eca905
Compare
|
@shawnborton @QichenZhu One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Let us know when this is ready for @Expensify/design review. |
brunovjk
changed the title
…ased callback names.
Accessibility reviewI found one new accessibility gap. Screen reader users can't reorder fields on mobile. The rest of the refactor keeps the same accessible names, roles, and states as 1. You can only reorder on mobile with a touch drag, and screen readers have no other way to do it This PR adds drag-to-reorder on native for the first time. On
With VoiceOver or TalkBack on, double-tap-and-hold may start the drag. However, the user can't then move the row to a target. Nothing tells them that long-press reorders the row. On web, dnd-kit's Suggested fix: add "Move up" and "Move down" What I checked with no regressions found
I reviewed the code only and didn't run a screen reader. |
QichenZhu
left a comment
QichenZhu
left a comment
There was a problem hiding this comment.
Some fields may be redundant, since other fields already include their info. For example, Posted date is already in Date.
Please ignore my comments on the unit tests if we want AI to maintain them and don't care about their human readability.
| CONST.SEARCH.TABLE_COLUMNS.TAX_AMOUNT, | ||
| CONST.SEARCH.TABLE_COLUMNS.ATTENDEES, | ||
| CONST.SEARCH.TABLE_COLUMNS.REIMBURSABLE, | ||
| CONST.SEARCH.TABLE_COLUMNS.BILLABLE, |
There was a problem hiding this comment.
When Track billable expenses is disabled in workspace settings, staging doesn't show the Billable field for a non-billable expense, but this PR shows it.
| Staging | PR | |
|---|---|---|
| Non-billable expense |  |
 |
| Billable expense |  |
 |
The Reimbursable field has the same issue if Cash expenses is set to Always reimbursable or Always non-reimbursable.
| expect(jest.mocked(Navigation.goBack)).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it('uses the expense heading while saving the same report column preference', () => { |
There was a problem hiding this comment.
Unit testing the heading is fine, but I'd rather not highlight it in the case description or the code comment.
| } | ||
| const reordered = [list.data.at(2), list.data.at(0), list.data.at(1), list.data.at(3)].filter((item): item is ColumnItem => !!item); | ||
|
|
||
| // When the drag finishes with date above amount and the required amount row is tapped before saving. |
There was a problem hiding this comment.
"with date above amount and the required amount row is tapped before saving" just rephrases the code above, and the comment is in the wrong place.
| fireEvent.press(screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}${TOTAL_AMOUNT}`)); | ||
| fireEvent.press(screen.getByText('common.save')); | ||
|
|
||
| // Then the chosen order reaches the shared preferences and the required field cannot be turned off. |
There was a problem hiding this comment.
"Then the chosen order reaches the shared preferences" and "the required field cannot be turned off" should be two different test cases.
| } | ||
| }); | ||
|
|
||
| it('moves a newly selected field into the draggable area and returns a deselected field to the fixed area', () => { |
There was a problem hiding this comment.
The test below doesn't check the position of the deselected field, only that it's on the screen.
| type EditableFieldProps = PropsWithChildren< | ||
| Pick<OfflineWithFeedbackProps, 'pendingAction'> & | ||
| Pick<MenuItemRootProps, 'onPress' | 'accessibilityLabel'> & { | ||
| name: string; | ||
| value?: string; | ||
| isValueSelectable?: boolean; | ||
| /** Defaults to 2; use 0 to display the full value. */ | ||
| numberOfLinesValue?: number; | ||
| error?: ReactNode; | ||
| valueComponent?: ReactNode; | ||
| hintText?: ReactNode; | ||
| footer?: ReactNode; | ||
| } | ||
| >; |
There was a problem hiding this comment.
Each prop needs a JSDoc.
| @@ -0,0 +1,85 @@ | |||
| // Keeps an expense field's offline state and validation feedback together. | |||
There was a problem hiding this comment.
| // Keeps an expense field's offline state and validation feedback together. |
| type AmountFieldProps = Pick<OfflineWithFeedbackProps, 'pendingAction'> & | ||
| Pick<MenuItemProps, 'title' | 'description' | 'hintText' | 'errorText' | 'copyValue' | 'onPress'> & { | ||
| canEdit?: boolean; | ||
| shouldShowPaid?: boolean; | ||
| }; |
There was a problem hiding this comment.
Each prop needs a JSDoc.
| value = convertToDisplayString(amount, currency); | ||
| break; | ||
| } | ||
| case CONST.SEARCH.TABLE_COLUMNS.TOTAL_PER_ATTENDEE: |
There was a problem hiding this comment.
TOTAL_PER_ATTENDEE is redundant since the ATTENDEES field already has that info.
| } | ||
| value = convertToDisplayString(getAmount(transaction, isFromExpenseReport) / attendeeCount, getCurrency(transaction)); | ||
| break; | ||
| case CONST.SEARCH.TABLE_COLUMNS.POSTED: |
There was a problem hiding this comment.
POSTED is redundant since the DATE field already has that info.
|
Great! Im about to leave now. I'll get back here first thing tomorrow. Thanks a lot for the review. |
QichenZhu
left a comment
QichenZhu
left a comment
There was a problem hiding this comment.
We can remove them since they are confusing and don't add info.
| @@ -0,0 +1,48 @@ | |||
| // Shares the offline layout of expense toggles while callers compose any supporting content. | |||
There was a problem hiding this comment.
| // Shares the offline layout of expense toggles while callers compose any supporting content. |
| @@ -0,0 +1,79 @@ | |||
| // Displays the merchant and lets users look up the original card merchant when available. | |||
There was a problem hiding this comment.
| // Displays the merchant and lets users look up the original card merchant when available. |
| @@ -0,0 +1,43 @@ | |||
| // Preserves rich expense descriptions alongside their offline and validation feedback. | |||
There was a problem hiding this comment.
| // Preserves rich expense descriptions alongside their offline and validation feedback. |
| @@ -0,0 +1,70 @@ | |||
| // Keeps billable policy violations next to the expense's billable control. | |||
There was a problem hiding this comment.
| // Keeps billable policy violations next to the expense's billable control. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Code reviewI found three behaviour issues worth fixing before merge, plus two smaller ones. I left out anything already raised in the review threads (Billable visibility, redundant fields, JSDoc, tests, mobile screen-reader reorder). The extracted field components match 1. Hidden fields also hide their violations (and existing users lose editors on deploy)
2. Users with no saved preference get new MCC and Exchange rate rows 3. One save in Customize fields rewrites the report table's columns
Smaller items
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
This comment was marked as resolved.
This comment was marked as resolved.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
This comment was marked as resolved.
This comment was marked as resolved.
Preserve the extracted expense fields while integrating current tag, vendor, reimbursement, reservation and attendee behavior from main.
Respect workspace boolean visibility, omit redundant fields, retain required editors, and preserve table-only columns and unavailable-vendor fallback. Add focused regression coverage.
Expose valid Move up and Move down actions through the shared selection rows, preserve required fields, and add localized labels and native regression tests.
Document useful prop semantics, remove redundant headers, split independent test cases, and make test descriptions and assertions easier to review.
brunovjk
changed the title
Display Held and map supporting columns to their existing expense editors without losing information. Preserve unavailable Vendor preferences and keep deselected editors from being restored by hidden columns.
Use the existing hint slot and verify reservation details stay outside the trailing controls.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@MelvinBot, could you help me understand why |
|
Codex works elsewhere in the repo, so this looks specific to this PR. The failure happens on Codex's side, and nothing in the PR blocks it. Codex has failed all 3 runs here, with Here's what I'd try:
Details
|
![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: cf004921f3
ℹ️ 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".
| ) { | ||
| continue; | ||
| } | ||
| columnsToSave.splice(Math.min(index, columnsToSave.length), 0, column); |
There was a problem hiding this comment.
When the saved table preference contains POSTED without DATE (or TOTAL_PER_ATTENDEE without ATTENDEES), getMoneyRequestViewFields exposes the alias as Date/Attendees in selectedColumnIds. After any unrelated field addition or reorder, columnsToSave therefore already contains the synthesized editor, and this splice inserts the original alias too, so the report table unexpectedly gains an extra Date/Attendees column just from customizing the expense view. Preserve the alias by replacing its synthesized editor rather than retaining both.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Explanation of Change
This PR adds support for displaying the report's configured custom columns as read-only fields in single-expense report and expense views.
It also adds the Customize fields option to allow configuring which fields are displayed, reusing the same column preferences used by the report view.
Fixed Issues
$ #101300
PROPOSAL: N/A
Tests
Test Steps
Offline tests
Same as Test Steps
QA Steps
Same as Test Steps
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
Screen.Recording.2026-10-06.at.10.33.27.mov
Android: mWeb Chrome
iOS: Native
102457_ios_native.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
102457_web_chrome.mov