[POC] Remove dependencies argument from useOnyx - #93439
Closed
fabioh8010 wants to merge 34 commits into
Closed
Conversation
…onyx-store-poc-baseline
Contributor
|
|
…onyx-store-poc-baseline
Onyx store-based PR 3 removes the 3rd `dependencies` argument from `useOnyx`. Migrate all 64 consumers: - Selector-less calls drop the redundant 3rd argument. - Inline selectors are extracted into `useCallback`-memoized selectors using the same dependency list (the established app idiom for useOnyx selectors). - Factory-produced selectors use `useCallback((input) => factory(args)(input), [deps])`, extracting optional-chained dependencies into locals so React Compiler can preserve the memoization. Typecheck, ESLint, and the React Compiler compliance check all pass.
…gine `useParticipantsPolicies` / `useParticipantsPolicyTags` memoized their useOnyx selector with the `participants` array as the dependency. When a caller passes a fresh array with identical contents (e.g. `transaction?.participants ?? []` after the transaction is cleared), the selector identity changes every render, which defeats useOnyx's internal selector memoization and makes it re-subscribe on every render — under the store-based `useSyncExternalStore` engine that never settles and hangs (surfaced by the IOU confirmation UI tests during `afterEach` Onyx.clear()). Key the memoized selector on the participants' policy IDs (the only data the selector reads) instead of the array reference.
Audit follow-up to the participant-policy fix: harden the other migrated useOnyx selectors whose memoization dependency is a non-primitive (array/Set) reference that can be rebuilt every render, which defeats useOnyx's selector memoization and never settles under the store-based engine. - useArchivedReportsIDSet: memoize the returned Set at the source (helps every consumer, e.g. WorkspaceRoomsPage and OptionListContextProvider). - useTransactionViolationOfWorkspace / PopoverReactionList: key the selector on the contents of the freshly-built Set / destructured account-ID array. - useTransactionViolation / ReactionTooltipContent / InvoiceSenderField: key the selector on the contents of the Set param / prop array (callers pass fresh refs, e.g. `selectedParticipantsProp.filter(...)`).
# Conflicts: # src/components/ReportActionItem/MoneyRequestReceiptView.tsx # src/hooks/useTransactionViolationOfWorkspace.tsx # src/pages/Debug/Report/DebugReportPage.tsx # src/pages/inbox/report/ReportActionItemCreated.tsx
The content-key + eslint-disable selector-memoization pattern (e.g.
`useCallback(sel, [ids.join(',')])`) tripped React Compiler's
"missing/extra memoization dependencies" check, because the dep array
no longer matched the values the selector closure reads.
Replace it with source-stabilization: project each array/Set to the
primitive(s) or ID-array the selector actually consumes, stabilize that
reference with `useStableArrayReference` (set-state-during-render, no dep
array to mismatch), then key the selector memo on the stable value. Deps
now match the closure (compiler-happy) and stay reference-stable across
renders (no re-subscribe loop under the store-based engine).
- useParticipantsPolicies / useParticipantsPolicyTags: select on stable policyID array
- ReactionTooltipContent / PopoverReactionList: stable accountID array
- ExpenseReportListItem: stable transactionID array
- useTransactionViolation / useTransactionViolationOfWorkspace: stable sorted ID array, Set rebuilt in selector
- InvoiceSenderField: project to an isInvoiceRoom boolean (no array dep at all)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 56ba8d2)
PR 3 wrapped migrated useOnyx selectors in useCallback to keep them
reference-stable after the dependencies arg was removed. React Compiler
already memoizes selectors (inline or named) keyed on their closed-over
values, so the useCallback wrappers are redundant in compiled files —
the project convention is to let the compiler handle memoization.
- Inline the selector directly into useOnyx({selector: ...}) for files
React Compiler compiles; the compiler memoizes the inline arrow + the
options object exactly as useCallback would.
- Keep useCallback only for the 4 files that bail out of the compiler
(BaseKYCWall, SuggestionMention, usePaginatedReportActions,
usePaymentOptions) — there the compiler can't memoize, and the
no-inline-useOnyx-selector lint rule enforces it.
- useTransactionsByID keeps a named selector (inlining collided a param
name with an outer variable); it still compiles and memoizes.
- Input stabilization (useStableArrayReference / primitive projection)
is unchanged — it remains the load-bearing piece the compiler keys on.
Verified: typecheck clean, eslint no hard errors, react-compiler
per-file scan (only the 2 known bailouts), affected jest suites green
with no re-subscribe hangs on the store-based engine.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 58935de)
…e-pr-3 # Conflicts: # src/components/MoneyReportHeaderActions/MoneyReportHeaderSecondaryActions.tsx # src/pages/inbox/report/ReportActionItemCreated.tsx # src/pages/workspace/WorkspacesListPage.tsx
Selector-input stabilization is only load-bearing under the store-based (eager) engine in PR 5; on PR 3's engine it's at most a redundant recompute. Revert the 7 sites to plain inline selectors closing over their original input so PR 3 stays a focused "remove the dependencies argument" change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e-pr-3 # Conflicts: # package-lock.json # package.json # src/components/MoneyRequestConfirmationList/sections/ReportField.tsx # src/components/ReportActionItem/MoneyRequestView.tsx # src/hooks/useArchivedReportsIDSet.ts # src/pages/workspace/rooms/WorkspaceRoomsPage.tsx
Contributor
Author
|
@codex review |
![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: fa1b539d25
ℹ️ 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".
| "resolved": "https://registry.npmjs.org/react-native-onyx/-/react-native-onyx-3.0.85.tgz", | ||
| "integrity": "sha512-F4rhP9IHDoXejZjMhq4HTl6+p8xih5TdrEVRaaA9SSW1dGwF08c1mUQjnDmZ1Pq81OIVWhbkYnfVhYNKwgSaIA==", | ||
| "version": "3.0.86", | ||
| "resolved": "git+ssh://git@github.com/Expensify/react-native-onyx.git#5739db80220ed6bd205ae6d5114657eed727307e", |
There was a problem hiding this comment.
This lockfile entry now resolves react-native-onyx through git+ssh://git@github.com/... even though package.json pins the same SHA via git+https://.... The setup action runs npm ci on cache misses (.github/actions/composite/setupNode/action.yml), and npm uses the lockfile's resolved git URL, so fresh CI runners or developer machines without GitHub SSH credentials will fail to install this dependency. Please regenerate or edit the lockfile so the resolved URL stays on HTTPS.
Useful? React with 👍 / 👎.
Julesssss
self-requested a review
June 24, 2026 22:05
42 tasks
…e-pr-3 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing in favor of #97594 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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