Remove useDeepCompareRef from MoneyRequestReportTransactionList#83432
Remove useDeepCompareRef from MoneyRequestReportTransactionList#83432
Conversation
Replace the useDeepCompareRef anti-pattern with a primitive joined-string cache key for the visualOrderTransactionIDs effect. The string key gives React a cheap equality check that prevents the effect from re-firing when the array content is unchanged, matching the previous behavior without deep comparison overhead or React Compiler incompatibility. Co-authored-by: Cursor <cursoragent@cursor.com>
melvin-bot
bot
requested review from
JmillsExpensify and
huult
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| clearActiveTransactionIDs(); | ||
| }; | ||
| }, [visualOrderTransactionIDsDeepCompare]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable suppresses react-hooks/exhaustive-deps but has no accompanying comment explaining why the rule is disabled. The intent here is that visualOrderTransactionIDsKey is used as a proxy for the visualOrderTransactionIDs array to avoid reference-based re-fires, but that reasoning should be documented inline.
Add a justification comment, for example:
// eslint-disable-next-line react-hooks/exhaustive-deps
// visualOrderTransactionIDsKey is a joined-string proxy for visualOrderTransactionIDs — the effect uses the array in its body but depends on the primitive key to avoid re-firing on referential changes
}, [visualOrderTransactionIDsKey]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved in ac0b517 — the join approach and all associated eslint-disable comments have been removed. The effect now uses visualOrderTransactionIDs directly, and the idempotent guard lives in setActiveTransactionIDs.
tests/unit/MoneyRequestReportTransactionListActiveTransactionIDsTest.tsx
Show resolved
Hide resolved
| setActiveTransactionIDs(visualOrderTransactionIDs); | ||
| return () => { | ||
| clearActiveTransactionIDs(); | ||
| }; |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable suppresses react-hooks/exhaustive-deps but has no accompanying comment explaining why the rule is disabled.
Add a justification comment, for example:
// eslint-disable-next-line react-hooks/exhaustive-deps
// visualOrderTransactionIDsKey is a joined-string proxy for visualOrderTransactionIDs — the effect uses the array in its body but depends on the primitive key to avoid re-firing on referential changes
}, [visualOrderTransactionIDsKey]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved in ac0b517 — the join approach and all associated eslint-disable comments have been removed. The effect now uses visualOrderTransactionIDs directly, and the idempotent guard lives in setActiveTransactionIDs.
|
I can review this PR if needed. |
The react-compiler ESLint plugin is not configured in the test environment, so disabling it causes a "Definition for rule not found" error in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
Address CONSISTENCY-5 review feedback: explain why react-hooks/exhaustive-deps is disabled on the useEffect that uses visualOrderTransactionIDsKey as a primitive proxy. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@huult that would be great, mainly testing for edge cases |
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Move the duplicate-write guard from the component layer into
setActiveTransactionIDs itself, using a local module variable
to track the last-written IDs. This eliminates the join(',')
workaround, eslint-disable comments, and benefits all callers.
Made-with: Cursor
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
@huult @TMisiukiewicz ready |
![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: ac0b5176d4
ℹ️ 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".
| clearActiveTransactionIDs(); | ||
| }; | ||
| }, [visualOrderTransactionIDsDeepCompare]); | ||
| }, [visualOrderTransactionIDs]); |
There was a problem hiding this comment.
Using visualOrderTransactionIDs as a raw useEffect dependency makes this effect rerun whenever the array reference changes, even if the IDs are unchanged; on each rerun React executes the cleanup first, so clearActiveTransactionIDs() writes null before setting the same IDs again. Because cleanup resets the new lastSetIDs cache, the idempotent guard in setActiveTransactionIDs cannot short-circuit these same-content rerenders, which reintroduces extra Onyx churn and can briefly drop prev/next transaction context in the RHP.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9cb1ede — split the effect into two:
- Set effect (
[visualOrderTransactionIDs]dep) — callssetActiveTransactionIDson changes; no cleanup. - Unmount-only effect (
[]dep) — callsclearActiveTransactionIDsonly when the component unmounts.
This eliminates the null → same-IDs flash on referential re-renders since clearActiveTransactionIDs no longer runs during the cleanup phase of re-fires. The idempotent guard in setActiveTransactionIDs still prevents redundant Onyx writes when the content hasn't changed.
Tests updated accordingly — the reference-change test now explicitly asserts that clearActiveTransactionIDs is NOT called on re-render.
…nder When visualOrderTransactionIDs changes by reference only (same content), the previous combined effect would clear → re-set the IDs, causing a brief null state in Onyx. By separating clearActiveTransactionIDs into its own unmount-only effect, referential re-renders only call setActiveTransactionIDs (which the idempotent guard skips) without touching the clear path. Made-with: Cursor
useEffect cleanup functions must return void, not Promise<void>. The arrow shorthand was implicitly returning the promise from clearActiveTransactionIDs(). Wrapping in a block body discards it. Made-with: Cursor
|
Reviewing... |
| if (lastSetIDs?.length === ids.length && lastSetIDs.every((id, i) => id === ids.at(i))) { | ||
| return Promise.resolve(); | ||
| } | ||
| lastSetIDs = ids; |
There was a problem hiding this comment.
| if (lastSetIDs?.length === ids.length && lastSetIDs.every((id, i) => id === ids.at(i))) { | |
| return Promise.resolve(); | |
| } | |
| lastSetIDs = ids; | |
| // Create a shallow copy to avoid reference mutation | |
| const idsCopy = [...ids]; | |
| if (lastSetIDs?.length === idsCopy.length && | |
| lastSetIDs.every((id, i) => id === idsCopy.at(i))) { | |
| return Promise.resolve(); | |
| } | |
| lastSetIDs = idsCopy; |
What do you think about creating a shallow copy to avoid reference mutation?
There was a problem hiding this comment.
Good question! I don't think the shallow copy is needed here because:
- The
idsargument always comes fromvisualOrderTransactionIDs, which is auseMemoresult — React convention treats these as immutable (replaced on change, never mutated in-place). - The elements are primitive strings, so they can't be mutated individually.
- It would add an allocation on every call, including when the idempotent guard short-circuits — which works against the goal of reducing unnecessary work.
If the caller were external/untrusted I'd agree with defensive copying, but since this is an internal action called only from a single useEffect, the extra copy doesn't buy us correctness here.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-26.at.14.57.57.mp4Android: mWeb ChromeScreen.Recording.2026-02-26.at.15.02.24.mp4iOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-02-26.at.15.07.24.mp4MacOS: Chrome / SafariScreen.Recording.2026-02-26.at.14.51.02.mp4 |
|
@huult addressed your comment, did it test well for you? |
|
@mountiny Yes, I tested it and it works well. I didn’t see any issues |
|
Thank you! @huult |
|
🚧 @mollfpr 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.3.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
6 participants
Explanation of Change
Removes the
useDeepCompareRefanti-pattern fromMoneyRequestReportTransactionList(1 of 3 call-sites tracked in the linked issue).What changed:
TransactionThreadNavigation.ts--setActiveTransactionIDsis now idempotent. It tracks the last-written IDs in a module-level variable and skips the Onyx write when the content hasn't changed.clearActiveTransactionIDsresets this tracking. NoOnyx.connectis needed because these two functions are the only writers to this key.MoneyRequestReportTransactionList.tsx-- TheuseDeepCompareRefwrapper and the intermediatejoin(',')workaround are both removed. TheuseEffectnow usesvisualOrderTransactionIDsdirectly as its dependency, with noeslint-disablecomments. Referential re-fires are harmless becausesetActiveTransactionIDsguards against duplicate Onyx writes.Test file -- The helper hook mirrors the simplified component code. The "same content, new reference" test now verifies the effect does fire (since the reference changed), documenting that the idempotent guard lives in the action layer, not the component.
Why:
useDeepCompareRefis marked as an anti-pattern in its own JSDoc. It runsdeepEqualevery render, violates exhaustive-deps conventions (risk of stale closures), and is incompatible with React Compiler optimizations. Moving the guard into the action layer is cleaner -- it benefits all callers, avoids eslint-disable comments, and uses idiomatic React patterns.Fixed Issues
$ #83431
Tests
SEARCH_REPORTroute is focused)Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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