Use allReportActionsList param instead of module-level getAllReportActions in updateSplitTransactions - #98301
Conversation
…tions in updateSplitTransactions Co-authored-by: cretadn22 <cretadn22@users.noreply.github.com>
…f undefined Co-authored-by: cretadn22 <cretadn22@users.noreply.github.com>
|
Fixed the failing Root cause: This PR replaced the module-level Fix: That test now fetches the real The other two red checks are not related to this JS-only change:
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
All code checks are green now — the latest CI run passed The two remaining red checks aren't code failures and there's nothing for me to push:
Both are human-gated gates, so no further code fix is needed from me. |
|
All code checks are now green — the The only two remaining red checks aren't code failures and can't be resolved by a push:
Both need a human — nothing further for me to fix here. |
|
@MelvinBot mark as ready |
|
@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] |
|
✅ Marked this PR as ready for review. CI status: All real checks pass. The only two failing checks — Automated coverage of the fix: The full Note on manual web verification: I tried to also run the split-expense edit flow on web to confirm no |
|
@cretadn22 thanks for pushing this ahead 🙇 can you please complete the checklist with videos? |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-12.at.19.04.41.movAndroid: mWeb ChromeScreen.Recording.2026-08-12.at.19.02.15.moviOS: HybridAppScreen.Recording.2026-08-12.at.19.02.59.moviOS: mWeb SafariScreen.Recording.2026-08-12.at.19.01.33.movMacOS: Chrome / SafariScreen.Recording.2026-08-12.at.19.00.20.mov |
|
@MelvinBot My checklist is done. Please fill out the Tests section and check off your remaining author checklist items. |
|
Done — filled out the Tests, Offline tests, and QA Steps sections and checked off the author checklist items I can genuinely stand behind. Tests now walk through the split-update path that crashed: submit an expense → split it → edit/delete a split → confirm the flow completes with no red error screen and no Boxes I intentionally left unchecked (I won't check what I can't honestly attest to):
Given that, the confidence here rests on the automated suites rather than a manual drive: the change is behavior-preserving, and
|
|
I looked into the failing checks — there's nothing here for me to fix by pushing code. The only two failing checks are non-code process gates, and every actual code/test check is green. Failing checks:
Passing: Bun tests, all 8 Both failing checks are on the ignore list for automated fixes, so I didn't push anything. To get them green: complete the author checklist items above, and have a reviewer approve the PR. |
|
@MelvinBot I've verified everything on all platforms for you. Please check off all the remaining boxes in the author checklist so the pipeline check can pass |
|
Thanks for testing this on all platforms, Those boxes are first-person attestations of manual work I didn't actually do — native runs on all 5 platforms, offline, a High Traffic account, screenshots/videos, and the console-error check. Checking them as the PR author would put testing claims on the record that I can't personally stand behind, even with your verification, so I'd rather not. One of them also can't be honestly completed by anyone yet: "I included screenshots or videos for tests on all platforms" — the Screenshots/Videos section is still empty, and Since you're the one who verified across platforms (and you have write access to the PR), the accurate way to reflect that is for you to tick those author boxes and drop the platform videos you captured into the Screenshots/Videos section yourself. That keeps the checklist honest about who did the testing and satisfies the video request — and once the videos are in, the For the record, I've already filled in the Tests / Offline / QA sections and checked every box that reflects work I genuinely did (code patterns, unit tests added, the N/A conditionals). Happy to jump on anything code-, test-, or CI-related. |
|
Still nothing here for me to fix by pushing code — the state is unchanged from my previous comment. The only two failing checks are non-code process gates, and all 31 code/test checks pass.
Both are on the ignore list for automated fixes, so I pushed nothing. To get them green: complete the author checklist items, and have a reviewer approve the PR. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ 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". |
mountiny
left a comment
There was a problem hiding this comment.
Thanks for following through
|
The only failing check here is PR Author Checklist / checklist, and there's nothing to fix in code. That check failed only because the author checklist in the PR body has unchecked boxes (run log):
Two notes:
No push needed. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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/mountiny in version: 9.4.53-0 🚀
|
|
🤖 Help site review: no docs changes required. I reviewed the changes in this PR against Expensify's help site content in This PR is a behavior-preserving internal refactor: in Since there is no change to any user-facing behavior, UI, feature, tab, setting, button, or product copy, there is nothing for the public help site to document. No draft help site PR was created. If you believe a specific help article is affected, let me know which flow and I'll take another look. |
Explanation of Change
updateSplitTransactions(inSplitTransactionUpdate.ts) was calling the module-levelgetAllReportActions(reportID)helper in 6 places. That helper readsReportActionsUtils' module-scopedallReportActions(its ownOnyx.connectsource), which is exactly the binding that surfaced in production asReferenceError: getAllReportActions is not defined(Sentry APP-JWE / APP-JWD, inside arequestAnimationFramecallback).updateSplitTransactionsalready receives the same collection as itsallReportActionsListparameter — every caller sources it fromuseOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS)— and the parameter form is already used elsewhere in the same function. This change replaces the 6getAllReportActions(<id>)calls with the equivalent parameter lookup and drops the now-unused import:The trailing
?? {}preservesgetAllReportActions' never-undefinedcontract for theObject.values(...)consumers. This is behavior-preserving in production (where the collection is always populated) and removes the module-level dependency that produced the crash.Test updates: the split test suites previously passed
allReportActionsList: undefinedat several call sites and relied on the module-level function as the real data source. Those sites now pass the realREPORT_ACTIONScollection (matching production), so the affected split tests exercise the same data path the app does. Call sites that intentionally test the empty/absent case are left as-is. The fulltests/actions/IOUTest/SplitTest.ts(125 tests) andSplitSelfDMTest.tssuites pass.Fixed Issues
$ #98105
PROPOSAL: #98105 (comment)
Tests
Precondition: be on a workspace where the split-expense feature is available, and open the browser JS console (so you can watch for errors).
ReferenceError: getAllReportActions is not defined(or any uncaughtReferenceError) in the JS console.getAllReportActions is not definederror appears in the console.Offline tests
Same as Tests. This is a behavior-preserving refactor that only changes where
updateSplitTransactionsreads report actions from (a parameter it already receives viauseOnyxinstead of a module-level helper), so offline behavior is unchanged: performing the split/edit while offline should optimistically update as before with no console error.QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Not yet tested — needs manual QA.
Android: mWeb Chrome
Not yet tested — needs manual QA.
iOS: Native
Not yet tested — needs manual QA.
iOS: mWeb Safari
Not yet tested — needs manual QA.
MacOS: Chrome / Safari
Not yet tested — needs manual QA.