Repository navigation
Flag the group's own snapshot entry when a whole-group delete removes it - #100675
Conversation
|
@cretadn22 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19582b148a
ℹ️ 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".
|
@nkdengineer The flicker still happens when deleting a group's last expense through its own thread Screen.Recording.2026-09-12.at.17.05.01.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-09-12.at.17.32.07.movAndroid: mWeb ChromeScreen.Recording.2026-09-12.at.17.35.52.moviOS: HybridAppScreen.Recording.2026-09-12.at.17.36.22.moviOS: mWeb SafariScreen.Recording.2026-09-12.at.17.34.05.movMacOS: Chrome / SafariScreen.Recording.2026-09-12.at.17.30.05.mov |
|
I only noticed one bug (in this comment), the rest of the code changes are solid and testing well. |
9a33d90 to
4f21c34
Compare
|
@cretadn22 I fixed this issue. |
Co-authored-by: cretadn22 <168617111+cretadn22@users.noreply.github.com>
|
@nkdengineer Please check again |
|
@neil-marcellini All done. |
neil-marcellini
left a comment
There was a problem hiding this comment.
Good to go, thanks for the updates!
AI found another possible issue, but we can fix it in a follow up.
One new issue, non-blocking. Making isEntireGroupSelected sufficient in getSelectedGroupKeys means a stale flag can now over-report. Reconcile copies the flag forward verbatim without re-evaluating it against the new data. So: on group-by:from, select a whole 2-expense group, then create a third expense that lands in that group (the create path bumps count and adds the row, but doesn't touch the selection). The two originals keep isEntireGroupSelected: true, delete flags the whole group, and the surviving third expense's group row vanishes until the next Search response. Before this commit the live selectedCount === groupCount check was self-correcting here.
Narrow, and the same staleness already existed for the unknown-count branch, so I'd treat it as a follow-up rather than a blocker. The clean fix is for reconcile to recompute the flag — it already has the group's transactions and count in hand at that point.
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for the update!
Two more NAB comments from the AI. They can be addressed in a follow up. The first one is about performance so maybe not a problem unless we prove it's actually non-performant and under which conditions.
stampGroupCoverageFlags is called once per group with the entire accumulated newTransactionList, and it spreads and scans that object twice each time. With a select-all across many groups this goes quadratic in a hook that reruns on every filteredData change. The block already knows the group's own children, so computing the flag once and assigning it to those entries would avoid the whole-selection scans.
Line 219 deliberately leaves groupKey undefined for isExpenseReportType, and the new loop then sets it anyway for every group type. Downstream consumers all gate on the group_ prefix so I can't find a behavior change, but it reads like the guard was undone by accident — worth a word from the author on whether that's intended.
|
Dang, since I authored some of the commits on this PR, we need another reviewer. |
|
@cretadn22 @neil-marcellini 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] |
|
(Neil's AI agent) @Beamanator you've been assigned as an independent reviewer here via PullerBear. PullerBear wasn't working on this PR, so I ran it on a dummy PR in Web-Expensify instead and carried the result over: https://github.com/Expensify/Web-Expensify/pull/50616 (PullerBear picked Beamanator there, and that PR is now closed again). Context on why another reviewer is needed: the |
|
🚧 Beamanator has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.4.81-0 🚀
|
|
🤖 Yes — help site changes were required. Draft PR: Docs updates for E/A#100675 deleting every expense in a group This PR settles what a member sees on the Spend page when they delete expenses inside a grouped search, and none of it was documented. Added two sections to that article:
The truncated case is the one worth writing down — it reads as a bug to a member who deletes "everything" and watches the group survive, and it's now deliberate behavior. One pre-existing fix included, not caused by this PR: the article called the bulk action button Selected in three places. The live UI renders it with the selection count ( How the labels were verifiedDriven in the live web app, not inferred from code: the Spend tab, the Expenses sub-tab, the group row checkbox, the X selected bulk action button, its menu options (Export, Hold, Move to report, Split, Duplicate expense, Delete), and the confirmation modal (Delete expense / Delete / Cancel). Written against @Beamanator, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀
|
Explanation of Change
Deleting every expense in a group made the group row flash: it disappeared, came back, then disappeared again.
A grouped search reads from two Onyx keys. The main snapshot holds one
group_<id>entry per group and nothing else —getMemberSectionsbuilds one row per group entry withtransactions: []. Each group's children live under a separate hash, pulled in byuseSearchSnapshotviauseMultipleSnapshots.The group row had no
pendingActionof its own — it derived one from its children, and that derivation requiredtransactions.length > 0. Since the two keys are cleared at different times, that produced three states:DELETE→OfflineWithFeedbackhides the row's content. Row gone.transactions.length === 0, nothing left to derive from →pendingActionisundefined. Row back — the flash.Searchresponse drops the group entry. Row gone for good.The fix moves the delete state onto the group's own snapshot entry, which is the only thing that outlives its children:
pendingActionlives on a sharedSearchGroupBaseinSearchResults.ts, which also folds up thecount/total/currencythat all ten group models repeated.getMemberSectionsalready spreads the group entry into the row, soitem.pendingActionlights up and the existingisRowDeletedchecks just work. The header greys out from that flag only — there is no children-derived pending-delete fallback.Which groups get that flag is coverage, not "clicked the group checkbox":
isSelectedViaGroupstill means the user selected through the group header. Export uses it so a group-row click can export as a group even whenlimit:left children unloaded.getSelectedGroupKeys, which includes a group only when the selected children cover the group's snapshotcount(or, when count is missing, whenisEntireGroupSelectedis set). Selecting every loaded child one by one is enough when that loaded set is the whole group. Clicking the group checkbox withlimit:Y < X is not.stampGroupCoverageFlagswritesisEntireGroupSelectedwithout touchingisSelectedViaGroup.A
limit:smaller than the group count only deletes the loaded rows, so the group stays in the list. In that case:pendingActiononly, so a truncated delete does not grey out a group that still has expenses.The flag rides in the delete request's own
optimisticData/failureData, so a failed delete puts the row back. Delete runs once per transaction, so all of a group's requests carry the same idempotent merge and any one failing restores the row.No
successDatais needed: theSearchresponse replaces the whole snapshot, so a surviving group loses the flag and a deleted group loses its entry.The view drops flagged rows from the list when online so the row collapses away instead of leaving a zero-height wrapper; offline it stays put with the usual pending-delete styling.
One deliberate trade-off: a fully deleted group row now pops out rather than playing
FadeOutUp, because it leaves the list on the same render the flag arrives. The surviving rows still slide up viaLinearTransition, and the fade was mostly invisible anyway —hideChildrenhad already collapsed the row's content a step earlier.Fixed Issues
$ #100122
PROPOSAL:
Tests
Offline tests
QA Steps
Same as test
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-09-09.at.15.25.42.mov
Android: mWeb Chrome
Screen.Recording.2026-09-09.at.15.22.49.mov
iOS: Native
Screen.Recording.2026-09-09.at.15.27.46.mov
iOS: mWeb Safari
Screen.Recording.2026-09-09.at.15.23.28.mov
MacOS: Chrome / Safari
Screen.Recording.2026-09-09.at.15.19.21.mov
Screen.Recording.2026-09-09.at.15.20.56.mov
Screen.Recording.2026-09-09.at.15.56.01.mov