Repository navigation
Conversation
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Android test steps verification results: Standalone Android NewDot testing was blocked by the required cash-expense policy’s paid Control-plan gate. No upgrade was confirmed; onboarding completed, Submission frequency was set to Instantly, and Approvals and Payments were verified off.
|
|
🤖 Web test steps verification results: Workspace chat verified on dev NewDot web. Configuring Cash expenses encountered a paid upgrade gate; no upgrade was confirmed, and dependent expense/report checks remain blocked.
|
|
@MelvinBot resolve conflicts |
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
🤖 Web test steps verification results: ❌ fail Verification was blocked by the missing Bun binary during postinstall. No UI testing was performed against a potentially stale build.
Conflict resolution committed and pushed: 9cefbab. GitHub still reports the previous head’s mergeability status. Storybook, spelling, and code review passed. Jest failed with dependency-loading errors; lint, typecheck, and compiler checks were blocked by Bun setup. Updated the PR’s AI Tests section with these limitations. |
|
|
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Pushed ceed574. The Jest failure came from three stale footer assertions retained during conflict resolution. Updated them to match the approved creation thresholds; production code is unchanged.
|
|
|
Krishna2323
left a comment
Krishna2323
left a comment
There was a problem hiding this comment.
@MelvinBot Please address the inline comments and then finish the PR.
PR description
- Update Explanation of Change to remove the mention of keeping the creation threshold.
- Fill in the Tests, Offline tests, and QA Steps sections using the steps below.
- Complete the applicable items in the PR Author Checklist. Leave the screenshot and platform-specific items for me.
Setup
- Use a Collect workspace with:
- Submission frequency set to Instantly.
- Approvals off.
- Payments off.
- Under Rules > Cash expenses, set the option to Always non-reimbursable.
- Create two expense reports, each containing only non-reimbursable expenses, and then retract both reports.
Tests
- Open the workspace chat and start creating a manual expense.
- On the confirmation page, verify that the Report field shows New report and can be pressed.
- Press the Report field. Verify that the picker shows only Create report and None, and that neither of the retracted reports appears.
- Submit the expense and verify that it lands on a new report.
- Repeat steps 1–4 using the global create (+) button.
- Add a reimbursable expense to one of the retracted reports, then start creating another expense. Verify that this report now appears in the picker and that the expense lands on it when selected.
- As a workspace admin, move an existing expense from one report to another. Verify that the move picker remains unchanged, including the admin bypass.
- On a new workspace with no open reports, start creating an expense. Verify that the Report field can be pressed and offers Create report and None.
Offline tests
Repeat the relevant steps from the Tests section while offline. Verify that expenses created offline are assigned to a new report.
QA Steps
Use the same steps listed in the Tests section.
Once everything passes, mark the PR as ready for review.
| let hasSelectableReports = (!isCreatingExpense && !mayHideListOptions) || shouldReportFieldBeEditable(eligibleOutstandingReports, selectedReportID, isUnreported); | ||
| if (isCreatingExpense && !isUnreported) { | ||
| // Keep a single eligible alternative read-only when the automatic destination is "New report". | ||
| hasSelectableReports = eligibleOutstandingReports.length > 1; |
There was a problem hiding this comment.
@MelvinBot This brings back the > 1 threshold for create, and that undoes #105939. #105939 made the field always editable on create, because the picker always offers "Create report" and "None". With this change, a workspace with 0 or 1 open reports shows a read-only field, so the user can't pick "None" or "Create report". Please remove eligibleOutstandingReports, hasSelectableReports and the isCreatingExpense block, and restore main's shouldReportBeEditable line and its comment. The default-value check and the picker filter already fix the bug.
| // so there the row needs another report to choose from. Creating from inside an expense report locks the row, | ||
| // since the expense can only go on that report. | ||
| const eligibleOutstandingReports = isCreatingExpense ? outstandingReports.filter((report) => canAddTransaction(report, rules)) : outstandingReports; | ||
| // Edits on a report can hide the new-report option; other non-creation flows always offer a choice. |
There was a problem hiding this comment.
@MelvinBot This comment uses a semicolon to join two clauses (CONSISTENCY-15). Restoring main's comment fixes it.
| }); | ||
|
|
||
| it('should allow editing report field when there is only 1 outstanding report and creating from policy chat', async () => { | ||
| it('should disable report field when there is only 1 outstanding report and creating from policy chat', async () => { |
There was a problem hiding this comment.
#105939 added these tests for the behavior we want, so they weren't stale. Please revert these 3 tests and the beforeEach session setup to main, unless the other tests still need the session.
| }); | ||
|
|
||
| it('opens the list when the workspace has no report open yet', async () => { | ||
| it('keeps creation locked when the workspace has no report open yet', async () => { |
There was a problem hiding this comment.
Please revert this test and the one at line 180 to main. With no open report, or only the one the expense goes on, the row should still open, because the list offers "Create report" and "None".
| await waitForBatchedUpdates(); | ||
| }; | ||
|
|
||
| it.each([false, true])('shows a locked New report field with only non-reimbursable reports (global create: %s)', async (isFromGlobalCreate) => { |
There was a problem hiding this comment.
With the threshold removed, the field should show "New report" and stay interactive. Please keep the mockReportName assertion, expect mockIsInteractive to be true, and rename both tests to match.
| {isCreatingExpense: true, isEditing: false, shouldShowReport: false}, | ||
| {isCreatingExpense: false, isEditing: true, shouldShowReport: true}, | ||
| {isCreatingExpense: false, isEditing: false, shouldShowReport: true}, | ||
| ])('preserves move permissions but uses save eligibility for creation: %j', async ({isCreatingExpense, isEditing, shouldShowReport}) => { |
There was a problem hiding this comment.
These tests set isCreatingExpense directly, so nothing checks that useReportPickerActions maps action === CREATE to it. Please add one test that renders through the hook, or through ReportFieldDropdown with action: CREATE, and checks that an ineligible report is hidden.
There was a problem hiding this comment.
…orwarding Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
🤖 Web test steps verification results: Created a Collect workspace on dev NewDot web without upgrading. The browser driver subsequently timed out on interactions, screenshots, and snapshots, blocking further testing.
Addressed all six inline comments and pushed 501323b. Updated the explanation, testing sections, and completed checklist items; screenshot and platform-specific items remain untouched. Full Jest passed: 2,347 suites, 39,089 tests. Typecheck, lint, spelling, Storybook, and code review passed. Local formatting remains unverified. Kept the PR as a draft: browser/offline verification is incomplete, and fresh CI checks are pending. view run · no recording available |
|
|
|
|
|
@MelvinBot resolve the conflicts and mark the PR as ready for review. |
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@Krishna2323 Please 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] |
Codex Review Summary: ✅ CompletedThis 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. |
|
🤖 Web test steps verification results: ❌ fail Web verification was blocked by a blank handed-over session. No expense/report behavior could be verified.
Conflicts resolved and PR marked ready for review. Pushed 83bccaa; GitHub confirms it is mergeable. Preserved main’s intentional revert of None and unconditional Report editability, and updated the PR description accordingly. All 38 targeted tests passed. Full Jest/lint timed out; typecheck encountered a missing workspace-package dependency. These limitations are documented in AI Tests. view run · no recording available |
![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: 83bccaa98a
ℹ️ 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".
| /** Uses creation eligibility instead of the admin/manager exceptions for moving saved expenses. Defaults to false. */ | ||
| isCreatingExpense?: boolean; |
There was a problem hiding this comment.
When this shared picker is used for expense creation, correctness now depends on every caller remembering the optional isCreatingExpense flag; omitting it silently restores the admin/manager move exceptions and can expose destinations that the creation save path rejects. This adds behavioral boolean configuration to an already broad component API, contrary to CLEAN-REACT-PATTERNS-1; split creation and move eligibility into composed wrappers/blocks so each flow selects the correct behavior structurally rather than through an optional unsafe default.
AGENTS.md reference: AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Investigated; no code change is required for this finding.
- CLEAN-REACT-PATTERNS-1 explicitly exempts optional props used to compute derived values. This flag selects report eligibility rather than a rendering mode.
- Both creation picker paths use the shared hook, which explicitly forwards creation eligibility. No production CREATE caller omitting it was found. Existing move callers intentionally retain admin/manager exceptions.
Re-ran the relevant suite: 17 tests passed, including real-hook CREATE/EDIT eligibility coverage.
Krishna2323
left a comment
Krishna2323
left a comment
There was a problem hiding this comment.
@MelvinBot after the inline code fixes:
- Update Tests step 2 to "verify the Report field shows New report and is disabled". Remove step 3, since the field can't be opened. Step 6 stays as is: once one report is eligible, the field is enabled and lists that report. Step 8 stays as written, because it matches main.
- Update the "Explanation of Change" so it says editability in create mode counts only eligible reports.
- Run Jest for the four report suites, lint, typecheck, the React Compiler check and spell check. Post the results.
| } else { | ||
| reportToUse = availableOutstandingReports.find((r) => r?.reportID === reportIDToUse); | ||
| } | ||
| if (isCreatingExpense && reportToUse && !canAddTransaction(reportToUse, rules)) { |
There was a problem hiding this comment.
@MelvinBot main reverted #105939 in 9eb336b, so the reason for my earlier comment about dropping the threshold is gone. The issue expects the Report field to be disabled when every report holds only non-reimbursable expenses. Right now it stays enabled, because line 135 checks editability against every outstanding report, including the ones the create picker hides. Example: with 2 retracted non-reimbursable-only reports, the field shows "New report" and can be pressed, and the list has only "Create report". In create mode, please check editability against the reports that pass canAddTransaction(report, rules). A single eligible report should still keep the field editable.
There was a problem hiding this comment.
Addressed in 3848247. Creation now passes only reports accepted by canAddTransaction into the existing editability selector. Zero eligible reports disables New report; a single eligible alternative remains interactive. No new count threshold was added, and other actions retain their existing report list. All four report suites pass (38 tests).
There was a problem hiding this comment.
Already addressed in 3848247:
- Creation editability considers only
canAddTransaction(report, rules)-eligible reports. - No eligible reports → disabled New report.
- One eligible alternative → enabled field, without a
> 1override.
Regression tests cover both workspace and global creation. All four report suites passed: 38 tests.
| await waitForBatchedUpdates(); | ||
| }; | ||
|
|
||
| it.each([false, true])('shows an interactive New report field with only non-reimbursable reports (global create: %s)', async (isFromGlobalCreate) => { |
There was a problem hiding this comment.
This test asserts the bug: mockIsInteractive is true when every report is ineligible. After the fix, expect false and rename the test. Keep the "New report" name assertion. The test at line 232 (one eligible report) should still expect the field to be interactive.
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@Krishna2323 @deetergp 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] |
|
MelvinBot has made 6 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. |
|
🤖 Web test steps verification results: ❌ fail Web verification was blocked by a blank handed-off session. No Report-field behavior could be verified.
Pushed 3848247, addressing both inline fixes. Updated the explanation and Tests step 2, removed old step 3, and preserved steps 6 and 8.
Results are recorded in AI Tests. view run · no recording available |
Explanation of Change
Align expense creation's Report field default, editability, and picker with
canAddTransaction(report, rules), so the UI does not offer destinations that the save path rejects. Ineligible automatic destinations display "New report" instead. Editability in create mode counts only eligible reports: the field is disabled when none are eligible, while a single eligible alternative keeps it enabled.AI Tests
Eligibility editability fix (2026-11-13): Create-mode editability now uses only reports accepted by
canAddTransaction(report, rules). All-ineligible cases show a disabled New report field, while a single eligible alternative remains interactive. Move/edit/submit behavior is unchanged.origin/; the runner denied overriding that environment setting.prettiernpm script is absent, but the pre-commit Oxfmt check passed on both files changed by this fix.Latest conflict-resolution update (2026-11-12): Merged main while preserving its intentional revert of the report-creation options feature. Retained the PR's default eligibility check, create-only picker filter, and label/interactivity regression assertions. Restored the footer fixture's signed-in owner so eligibility checks keep its valid default report.
@expensify/new-expensify-cloudflare-worker/adhoc-configafter the upstream merge. Theprettiernpm script remains absent.Review update (2026-11-12): Restored main's creation editability and original footer tests, retained the New report default checks, and added real-hook CREATE/EDIT filtering coverage.
prettiernpm script is absent, so that formatting gate remains unverified locally.Contributor branch access
This PR uses an upstream branch because Melvin cannot push to
Krishna2323's fork. To copy the branch to that fork:You can then close this PR and open a replacement from your fork if needed.
Fixed Issues
$ #104162
PROPOSAL: #104162 (comment)
Tests
Setup
Manual test plan — runtime verification is still pending as described in AI Tests. Updated for main's intentional revert of the "None" option and unconditional creation editability.
Offline tests
Repeat the relevant steps from the Tests section while offline. Verify that expenses created offline are assigned to a new report. This verification is still pending.
QA Steps
Use the same setup and steps listed in the Tests section on staging.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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