Repository navigation
Conversation
A report whose rows are selected via the group checkbox never gets a selection entry for a row queued for deletion, so requiring every row to be selected dropped the report from selectedReports. That hid bulk actions gated on the report count -- Merge reports among them -- after an offline expense delete. Derive the check from the selectable rows instead, matching getGroupCheckboxState. Co-authored-by: Alberto Gonzalez-Cela <Gonals@users.noreply.github.com>
|
|
|
🤖 Web test steps verification results: ✅ pass On dev NewDot web, after deleting one expense from a 2-expense report while offline, selecting both draft reports in Spend > Reports still shows
view run · no recording available |
|
Nothing to fix — every code check passes. The one failing check just needs a human approval.
The PR has zero reviews so far ( Full check statusAll other checks on the latest run are green, including the ones most likely to catch a problem with this change:
An earlier run also failed One unrelated thing worth correcting while you're in here: the |
dylanexpensify
left a comment
There was a problem hiding this comment.
Steps make sense, looks good from a product pov
|
The failing checks are not caused by this PR — #100881 merged to The fix belongs on Why this is main and not this PR — evidenceThis PR changes two files:
The |
|
Nothing left to fix on this PR —
A fix is already open: #101195. Once it merges, re-run the checks here — no change to this branch is needed. The typecheck failure I fixed earlier ( How I confirmed it's
|
| Commit | Declarations |
|---|---|
50b3c85 (base parent) |
1, at line 40 |
fa1af59 (branch parent) |
1, at line 42 |
ad2bdf1 (the merge) |
2, at lines 40 and 47 |
Both sides added the same line at slightly different offsets, so Git kept both without reporting a conflict.
The failing tests are all sidebar-rendering UI tests, none of them Search: tests/ui/PaginationTest.tsx, tests/ui/UnreadIndicatorsTest.tsx, tests/ui/GroupChatNameTests.tsx, tests/ui/SessionTest.tsx. Each fails with the same Babel parse error on that file. This PR's own suite passes — I ran npm test -- --silent tests/unit/Search tests/unit/hooks/useSearchBulkActionsTest.ts locally at 460ec04: 53 suites, 1647 tests passed, and npm run typecheck passed cleanly.
Check independent approval and checklist are also red, but those are the standard human-gated checks, not code failures.
I did not push anything, since fixing an unrelated file from this branch would mix a main repair into a deploy-blocker PR.
|
Lots of checks failing. Please merge main |
|
@MelvinBot assign me |
|
Done, |
|
@MelvinBot pull main |
…flineDelete Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
I merged the latest |
|
@codex review |
|
@MelvinBot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf18000d9a
ℹ️ 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".
| } | ||
|
|
||
| describe('deriveSelectedReports', () => { | ||
| it('includes a report whose every transaction is selected', () => { |
There was a problem hiding this comment.
Document every added test with Given/When/Then sections
The six new test cases omit the required // Given, // When, and // Then comments explaining why each scenario is covered; the lone comment in the pending-delete case does not provide all three sections. Add those sections to every case so future maintainers retain the required behavioral context.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
|
The fix looks correct, and I found no logic bugs. Two small items need attention before merge: the new tests lack the required Given/When/Then comments, and the The change is sound.
CI is green after the Please fix:
Out of scope, not blocking
|
|
I found no functional regressions. The four side effects below only happen offline, while a report has a pending-delete expense. None of them block the merge. Possible regressions
My read: none of these block the merge. Item 1 is the only behavior change. The PR's test I checked these by reading the code. I didn't run them in the app. Checked and not a problem
|
|
@MelvinBot address #100513 (comment), #100513 (comment) |
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
The commit only adds comments, and no code changed. I could not run Jest or lint on it in this environment, so CI will do that check. |
|
@MelvinBot pull main |
…flineDelete Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
I merged the latest |
|
No code fix is needed. The only failing check is All code checks on b6dfba1a07 pass, including Jest, TypeScript, ESLint, and Reassure. The three remote native builds were still pending when I checked. |
|
@MelvinBot pull main |
…flineDelete Co-authored-by: Situ Chandra Shil <situchan@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. |
|
@MelvinBot fix lint errors |
…flineDelete Co-authored-by: Alberto Gonzalez-Cela <Gonals@users.noreply.github.com>
|
Fixed the failing ESLint check by merging The only lint error was in
|
|
@Gonals this might require another engineer to merge since you're co-authored |







Explanation of Change
Why this is the cause, and not a stale search snapshot
The first hypothesis was that the offline optimistic delete leaves a stale transaction count or total in the search snapshot. That is not what happens:
canMergeReportsandisMoneyRequestReportEligibleForMergenever readtotal,nonReimbursableTotal,transactionCount, or any transaction list.DeleteMoneyRequest.tsonly touchesTRANSACTION,TRANSACTION_VIOLATIONS,REPORT, andREPORT_ACTIONS.The divergence is in the selection layer, which is what this PR changes. This also explains the "only appears after opening the report" half of the report: once the deleted transaction leaves the row's transaction list, the old
every()check passes again and the option returns.Related observations, deliberately left out of scope
Two things noticed while tracing this that are not changed here, to keep the deploy-blocker fix minimal:
useSearchBulkActions.ts:2433hard-disablesMerge reportswheneverareAllMatchingItemsSelectedis set, so selecting via the header select-all also hides it. That may well be intentional, since an "all matching" selection is unbounded.currentSearchResultsdirectly rather than thesearchResultsfallback used elsewhere in that hook.Worth a follow-up decision, but neither is required to unblock the deploy.
Fixed Issues
$ #100502
PROPOSAL: #100502 (comment)
AI Tests
Run by MelvinBot on this branch. This section records automated checks only — it is not a substitute for the human
TestsandQA Stepssections below.npm test -- --silent tests/unit/Search tests/ui/GroupHeaderTest.tsxnpm run typechecknpm run lint-changednpm run spell-changedThe new unit tests were confirmed to actually catch the bug: with the one-line source change reverted, 3 of the 6 cases in
deriveSelectedReportsTest.tsfail; with it applied, all 6 pass.Not verified in a browser. The attempt to drive the offline repro against this branch was blocked by an unrelated environment failure — the test session's page rendered blank (0 accessibility nodes, no network activity) for the whole run, while the dev server itself compiled cleanly. The behavioural fix therefore rests on unit-test and code evidence, not on a live run. The original bug was reproduced live on
mainbefore this change (evidence), so the repro steps are known-good and worth re-running by hand.npm run lint,npm test(full suite), andnpm run storybook -- --smoke-test --ciwere not run — the Search suites above cover every consumer of the changed function, and no component or story was touched.Tests
Expected Result:
In Step 9, Merge reports option will be shown when selecting both reports with same status.
Offline tests
None
QA Steps
Same as tests
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)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