Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .claude/skills/coding-standards/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ Coding standards for the Expensify App. Each standard is a standalone file in `r
- [UI-2](rules/ui-2-new-page-scrollview.md) — New pages must be scrollable
- [UI-3](rules/ui-3-no-inline-styles.md) — Do not use inline style objects

### Onyx
- [ONYX-1](rules/onyx-1-no-render-reachable-onyx-read.md) — Keep Onyx reads off the render path and out of a written tick

## Usage

**During development**: When writing or modifying `src/` files, consult the relevant standard files for detailed conditions, examples, and exceptions.
Expand Down

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions config/eslint/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ const config = defineConfig([
'rulesdir/no-direct-pre-insert-fullscreen-under-rhp': 'error',
'rulesdir/no-raw-typography': 'error',
'rulesdir/require-locale-for-localized-date-format': 'error',
'rulesdir/no-unsafe-onyx-read': 'error',
'rulesdir/prefer-narrow-hook-dependencies': [
'error',
{
Expand Down
157 changes: 157 additions & 0 deletions contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Different platforms come with varying storage capacities and Onyx has a way to g
There are only two ways to read Onyx data, and `Onyx.connect` is deprecated:
1. **`useOnyx`** (from `@hooks/useOnyx`) — the default for anything a React component renders.
2. **`Onyx.connectWithoutView`** — an imperative subscription for non-render logic, used only when `useOnyx` genuinely does not fit.
3. **`Onyx.get()`**: an asynchronous, one-shot read of the cache that never subscribes, for non-render code that needs a value at the moment it runs.

### - Prefer a pure function over reading Onyx at all
A pure function does not read Onyx itself — it receives the data it needs as parameters, and its caller does the reading (with `useOnyx` or `Onyx.connectWithoutView`) and passes it in. Before adding either subscription, check whether the code can be a pure function instead: it needs no connection, is trivial to test, and cannot cause extra rerenders. Prefer this even when it means passing more arguments. This takes precedence over everything below.
Expand All @@ -60,6 +61,162 @@ Add an inline comment at each new `Onyx.connectWithoutView` call stating why the
### - Using `Onyx.connectWithoutView` in a component for performance REQUIRES @frontend-performance approval
In rare cases a component that subscribes to multiple large collections through `useOnyx` suffers a significant performance regression. Reaching for `Onyx.connectWithoutView` to avoid that is an explicit exception, not a self-serve option: it MUST be approved by the `@frontend-performance` team on Slack, and the PR description MUST link to that discussion.

### - `Onyx.get()` is ONLY for code that runs on an event, never during render
It returns what is in the cache right now and never subscribes, so a value it returns is frozen at the moment of the read. Use it in action creators, libraries, network handlers, and callbacks such as `useCallback`, `useEffect` and event handlers. A collection key returns every member, exactly as `useOnyx` does. The same rule covers the value it produces: a value read this way MUST NOT reach rendered output, because nothing will re-render when the key changes.

It also resolves only after `Onyx.init` has hydrated the cache, so it cannot be used by anything that runs on the boot path before then. `src/libs/actions/OnyxDerived/index.ts` is the standing example: its restore-from-disk read runs in the same synchronous stretch as `Onyx.init`, so it reads the library's cache directly rather than through the wrapper.

```typescript
// GOOD ✅
async function submitExpense(transactionID: string) {
const transaction = await Onyx.get(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);
// ...act on it here, at event time
}

// BAD ❌ a component cannot await, so reaching the read from render means use() or .then()
function ReportName({reportID}: Props) {
const report = use(Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`)); // never updates again
return <Text>{report?.reportName}</Text>;
}
```

### - A value read with `Onyx.get()` MUST NOT be parked where render reads it
The ban on reaching rendered output covers the indirect route as well. Putting the value in `useState`, in a `useRef`, or in a module-level variable that a component reads leaves the screen showing a snapshot of the moment of the read, and the key changing will never update it. If it renders, it comes from `useOnyx`.

```typescript
// BAD ❌ the title freezes at the moment of the tap
function ReportTitle({reportID}: Props) {
const [title, setTitle] = useState<string>();
const onPress = async () => setTitle((await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`))?.reportName);
return <Text onPress={onPress}>{title}</Text>;
}
```

### - A function that reads with `Onyx.get()` MUST NOT be passed where render can call it
Passing the function as a prop moves the decision into the receiving component: the read is correct where it is written, and the call that breaks it is in another file. A prop named for an event (`onPress`, `onSelectRow`) that the child only attaches to an event is fine. A prop the child invokes in its own body, in its JSX, or in a `useMemo` is a render read. Check the receiver before passing a reader down, and check it again when a new receiver appears.

```typescript
// BAD ❌ src/pages/ReportScreen.tsx passes a reader down
<ReportRow getTotal={async () => (await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`))?.total} />

// src/components/ReportRow.tsx calls it during render
function ReportRow({getTotal}: Props) {
return <Text>{use(getTotal())}</Text>; // never updates again
}
```

### - Every caller of a function that reads with `Onyx.get()` MUST also be off the render path
A read written correctly in a library function becomes a render-time read the moment a component or hook calls that function, and neither file shows the problem on its own. Adding the call is enough to break it, so a diff containing no Onyx code at all can be the diff that introduces the defect. Either take the value as a parameter, or keep every caller off the render path and check that again whenever a caller is added.

A widely called function usually cannot host the read at all. One render call site anywhere in `src/` settles it, however many callers would benefit, so sweep every call site before moving a read down into a shared function.

```typescript
// src/libs/ReportUtils.ts, correct in isolation
async function getOwnerAccountID(reportID: string) {
return (await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`))?.ownerAccountID;
}

// BAD ❌ src/hooks/useOwnerName.ts, the entire diff: that read now runs during render
function useOwnerName(reportID: string) {
return getOwnerAccountID(reportID);
}
```

### - All `Onyx.get()` reads MUST come before the first write in that tick, or after an `await`
Most writes apply to the cache after the call returns, so a read that follows one resolves to the pre-write value. Treat every write the same way: which ones land before returning is version-dependent, and any `set` inside an `Onyx.update()` batch is deferred regardless.

Awaiting the read is not the fix. `Onyx.get()` samples the cache when it is called and the Promise defers delivery rather than the read, so a write queued before it cannot land in time however many `await`s follow. Await the **write's own promise**, or do the read first.

```typescript
// BAD ❌
Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: true});
const account = await Onyx.get(ONYXKEYS.ACCOUNT); // isLoading is still the old value
```

### - A key and a value derived from it MUST NOT be read in a tick that wrote either
A `set` lands at once but the derivation's own write does not, so the source and the derived value end up a revision apart. Check this by hand whenever a conversion touches a `DERIVED` key, since the write is often in a caller and the reads in a callee.

```typescript
// BAD ❌
Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`, transaction);
const t = await Onyx.get(`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`); // new revision
const derived = await Onyx.get(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES); // still the old one
```

### - `Onyx.get()` MUST NOT run at module scope
A module body runs at import time and cannot `await`, so the value can only reach a module variable through `.then()`, where it is a one-shot snapshot that never updates when the key changes. Move the read into the function that needs it, so it runs at event time and reads the current value. When a module genuinely has to track a key, subscribe with `Onyx.connectWithoutView()` rather than caching one read.

Hydration is not the reason. `Onyx.get()` resolves only after `Onyx.init()` has hydrated the cache, so a boot-path read is no longer a hazard on that count.

```typescript
// BAD ❌ a snapshot taken at import time, stale from the next write onwards
let preferredLocale;
Onyx.get(ONYXKEYS.NVP_PREFERRED_LOCALE).then((locale) => { preferredLocale = locale; });

// GOOD ✅ read where it is used
async function applyPreferredLocale() {
setLocale((await Onyx.get(ONYXKEYS.NVP_PREFERRED_LOCALE)) ?? CONST.LOCALES.DEFAULT);
}
```

### - Each synchronous stretch MUST do its own reads
One read block per synchronous stretch, not per function. Code after an `await`, a `runAfterTransitions` or any other deferral runs in a later tick and is meant to see the writes the earlier stretch made, so hoisting a read above the deferral hands it a value that is one tick stale. The ordering rule above is satisfied here, so nothing flags it; cover it with a test that asserts the post-write value.

### - A subscription with a `selector` MUST have that selector reapplied at the read site
`useOnyx(key, {selector})` hands the component a projection of the stored value. `Onyx.get()` returns the stored value itself. Copying the key across and dropping the selector changes the shape silently: it compiles, and the difference only surfaces where the value is used. Call the same selector on the result, or keep the subscription.

```typescript
// BAD ❌ a boolean becomes the whole NVP object
const [isSelfTourViewed] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {selector: hasSeenTourSelector});
const isSelfTourViewed = await Onyx.get(ONYXKEYS.NVP_ONBOARDING);

// GOOD ✅
const isSelfTourViewed = hasSeenTourSelector(await Onyx.get(ONYXKEYS.NVP_ONBOARDING));
```

### - The result of `Onyx.get()` MUST NOT be mutated
A single-key read resolves to the cached object itself, not a copy, so assigning to a property of the result writes the cache with no subscriber told. A collection resolves frozen and throws instead, which makes the single-key case the silent one. This bites hardest when a function is converted off a parameter it used to be free to mutate.

```typescript
// BAD ❌ writes straight into the cache
const report = await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
report.reportName ??= CONST.REPORT.DEFAULT_NAME;

// GOOD ✅
const report = await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
const named = {...report, reportName: report?.reportName ?? CONST.REPORT.DEFAULT_NAME};
```

### - The Search snapshot keys MUST stay on `useOnyx`
`@hooks/useOnyx` is not the library hook. Inside a `SearchScopeProvider` subtree it rewrites the key: for the keys in `CONST.SEARCH.SNAPSHOT_ONYX_KEYS` it subscribes to `snapshot_<hash>` and extracts the requested key out of that blob. `Onyx.get()` always reads the global key, so a conversion on one of them would silently swap snapshot data for live data.

Nothing here is left to judgment. `rulesdir/no-unsafe-onyx-read` fails the build on these keys. A key it cannot resolve statically is an error too, so routing one in through a variable or a helper does not get past it. There is no provider tree to walk: the keys are simply off limits, whichever subtree the read sits in.

```typescript
// BAD ❌ lint error: report_ is redirected to a Search snapshot
const report = await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);

// BAD ❌ lint error: the key cannot be resolved, so it cannot be cleared
const report = await Onyx.get(buildReportKey(reportID));
```

The rule reads `CONST.SEARCH.SNAPSHOT_ONYX_KEYS` and `src/ONYXKEYS.ts` and derives the banned access paths itself, so the two cannot drift apart. If a key genuinely cannot be written statically, disable the rule on the line and say in the comment why that key can never be a Search snapshot key. If the snapshot redesign ever makes these keys pointer-based, the ban can be lifted in one place.

### - A subscription that exists to trigger work MUST NOT be replaced with `Onyx.get()`
Ask what each subscription is for. A **source** supplies a value the code reads. A **trigger** schedules work when the key changes, and the value it carries is incidental. Converting a trigger makes the dependency stable and the effect stops re-running. No position check catches it, because nothing renders the value and nothing reads it during render. What does catch the two plainest shapes is the diff itself: a `useOnyx` deleted while a read of the same key appears inside an effect body, and a `useOnyx` deleted along with the variable's name in a dependency array. Anything longer than one hop stays manual. The chain hides easily: a value feeding a `useCallback` that feeds another `useCallback` that reaches an effect's dependency array is still a trigger, and a wrapper such as `useDebounce(useCallback(fn, deps))` swallows a link.

```typescript
// BAD ❌ deleting this subscription freezes the effect
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${id}`);
const build = useCallback(() => compute(report), [report]);
useEffect(() => {
build();
}, [build]); // the subscription is what re-runs this
```

### - Converting a value the user acted on REQUIRES a deliberate decision
Conversion changes when a value is sampled, from the caller's last render to the moment the handler runs. That only breaks something when the value was **on screen** in the view the handler belongs to: a dialog confirming an amount MUST act on the amount it displayed. An invisible input to a decision, such as a route, an eligibility check or a request field, is not this case, and event time is usually the more correct reading for it, so "a handler reads Onyx" is not a problem on its own. State which of the two a value is. QA cannot settle it, because the window is one render commit wide.

## Onyx Derived Values

Derived values are special Onyx keys which contain values derived from other Onyx values. These are available as a performance optimization, so that if the result of a common computation of Onyx values is needed in many places across the app, the computation can be done only as needed in a centralized location, and then shared across the app. Once created, Onyx derived values are stored and consumed just like any other Onyx value.
Expand Down
Loading
Loading