Repository navigation
Conversation
|
|
02c7b44 to
f27fb19
Compare
|
I’ll keep it as a draft until the Onyx change is merged and we can properly bump the Onyx version here |
72e9d18 to
a8691eb
Compare
a8691eb to
4eff1c8
Compare
|
@MelvinBot review |
Code reviewThe main risk: both lint rules can be bypassed without any Rule gaps (reads that are not flagged)
Config robustness
Workflow (
Repo conventions (App/CLAUDE.md)
ScopeI reviewed the diff at |
14c55ac to
b2af92b
Compare
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
| const DIRECTIVE_PATTERNS = { | ||
| disable: /^\s*eslint-disable(?<kind>-next-line|-line)?(?=\s|$)(?<args>[\s\S]*)$/, | ||
| enable: /^\s*eslint-enable(?=\s|$)(?<args>[\s\S]*)$/, | ||
| }; |
There was a problem hiding this comment.
NAB: The bypass scripts only check for eslint-disable, so these rules can still be turned off with /* eslint rulesdir/no-unsafe-onyx-read: off */. Same for no-onyx-connect. Not a huge problem but maybe we can catch that too
|
@LukasMod Looking good to me, could you check these comments I got from Claude during my review? Details1. Object-literal and class-property handlers are never escalatedThe highest-value finding, because the docs claim otherwise.
const api = {load() { return Onyx.get(ONYXKEYS.SESSION); }};
export function Three() { return <div>{String(api.load())}</div>; } // not flagged
export class Two extends React.Component {
load = () => Onyx.get(ONYXKEYS.SESSION);
render() { return <div>{String(this.load())}</div>; } // not flagged
}
const handlers = {load: () => Onyx.get(ONYXKEYS.SESSION)};
useEffect(() => { void handlers.load(); }, []); // not flaggedReplace Why it matters beyond the edge: ONYX-1's enforcement table ( Two honest ways out: extend the bindings to 2. The snapshot rule matches key paths;
|
|
Oh I didn't know this is urgent PR. |
|
Reviewing... |
|
This PR adds a new |
|
I start checking feedback comments, will answer soon and look for easy solution |
5410455
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@TaduJR I added small update related to previous feedback @fabioh8010 All 4 addressed:
|
TaduJR
left a comment
TaduJR
left a comment
There was a problem hiding this comment.
Thanks @LukasMod. Reviewed. the Onyx side looks good expect some minor follow-ups. I will comment on Expensify/react-native-onyx#850 or post it here. It's NAB.
The enforcement has a few holes that need no eslint-disable, see inline. The by-reference reads, the seatbelt route, the useCallback render callback and the ONYX-1 contradiction should be fixed before approval.
CI cache. .github/workflows/lint.yml:73 keys the ESLint cache only on eslint.config.mjs, config/eslint/** and lint-deps-lock.json, and CI clears it only after a failed run. Changes to the rule code, or to SnapshotUtils.ts, ONYXKEYS.ts or runtimeDefaults.ts, would reuse cached passes on unchanged files. Could those paths go into the cache key?
Also could you add native screenshots (sign in, send a message, sign out and back in)? Could QA cover sign-in/out and two web tabs?
|
|
||
| **A. Position.** Render reaches a read wherever it's written. Lint counts a function as a render body when it's named like a component or hook, has a top-level `return <JSX>`, is wrapped in `memo` or `forwardRef`, or is passed to a `useMemo`, `useState`, `useReducer` or `useSyncExternalStore` slot or a `selector` option, inline or by name. It follows a helper the render body calls in the same file, through aliases and `useCallback`, but misses one imported from another file. It also misses a helper that returns JSX from inside an `if` or `switch`, a component passed inline to any call other than `memo` or `forwardRef` (such as a HOC), and a function passed to a child that calls it during render. When one of these helpers is `async`, `tsc` catches it only where the Promise lands in a slot typed `ReactElement`, such as `renderItem`; a JSX child or a `render*` prop typed `ReactNode` accepts a Promise, so check for a type error before relying on it. | ||
|
|
||
| **B. Tick.** Awaiting a read doesn't wait for an earlier write. `Onyx.get` captures the cached value when it's called, and `merge` and `update` apply to the cache later, so a read queued behind them sees the old value. `set` lands at once today, but don't rely on it: await the write, or read first. A derived key (`ONYXKEYS.DERIVED.*`) is recomputed on a microtask after its source changes, so a read in the same synchronous stretch returns the old derived value, even after a `set` that already updated the source. Read a derived key only before writing its sources. |
There was a problem hiding this comment.
This says not to rely on set landing at once, but B1 (line 155) says a read after set is fine. Could this match B1?
|
|
||
| const name = 'no-unsafe-onyx-read'; | ||
|
|
||
| const READ_ALLOWED_DIRECTORIES = ['src/components/', 'src/pages/', 'src/hooks/', 'tests/']; |
There was a problem hiding this comment.
The path check is per file, so an exported helper in src/pages/**/utils.ts can read Onyx and be imported by src/libs. Is review the intended stop for that?
| } | ||
|
|
||
| // `useOnyx(key, {selector})` inline, or `const options = {selector}; useOnyx(key, options)` | ||
| function isHookOption(property, sourceCode) { |
There was a problem hiding this comment.
useOnyx(key, {selector: () => Onyx.get(ONYXKEYS.SESSION)} as const) passes. Could this unwrap the cast before checking the hook argument?
| * render body or at module scope, or an alias used that way), otherwise null. `classifyCall` is injected from | ||
| * classifyPosition, which this recursion feeds back into for the call sites. | ||
| */ | ||
| function getEscalatedPosition(binding, sourceCode, seen, classifyCall) { |
There was a problem hiding this comment.
This walk and the effect walk at line 389 follow const run = load, but not useCallback(load, []), useEffectEvent(load) or a useMemo(() => () => Onyx.get(...)) factory. Reads through useCallback and the useMemo factory pass in render and in effects. Reads through useEffectEvent pass in effects.
| // `const read = useCallback(() => Onyx.get(key), []); read();` in a render body | ||
| const handler = getHandlerBinding(functionNode, parent, grandparent); | ||
|
|
||
| return (handler && getEscalatedPosition(handler, sourceCode, seen, classifyCall)) ?? DEFERRED; |
There was a problem hiding this comment.
A render callback wrapped in useCallback returns DEFERRED here without the returnsJSX check, so this passes:
const renderItem = useCallback(({item}) => {
const value = use(Onyx.get(ONYXKEYS.SESSION));
return <Row value={value} />;
}, []);Could this return RENDER when returnsJSX(functionNode) is true?
| const {getCalledReadMethod} = createOnyxReadTracker(sourceCode); | ||
|
|
||
| return { | ||
| CallExpression(node) { |
There was a problem hiding this comment.
Both rules only see direct calls, so these pass with no errors, and the workflow regex misses them too:
await Onyx.get.call(Onyx, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`); // snapshot key
await Promise.all(reportKeys.map(Onyx.get)); // snapshot keys
export const readOnyx = Onyx.get; // in src/libsCould the rules report any reference to Onyx.get/Onyx.multiGet that isn't a direct call?
| message: 'Onyx.connect() is banned and the ban cannot be bypassed with eslint-disable or an eslint config comment. Use the useOnyx() hook to read Onyx data instead.', | ||
| }; | ||
|
|
||
| const ONYX_READ_BAN: BannedRule = { |
There was a problem hiding this comment.
A seatbelt row also silences this rule. SEATBELT_INCREASE=rulesdir/no-unsafe-onyx-read npm run lint writes one to eslint.seatbelt.tsv, and the error becomes a warning. Could this check fail when the TSV has any row for either new rule?
This branch has not been deployed
10 participants
Explanation of Change
react-native-onyxfrom3.0.117to3.0.119. The bump contains 2 Onyx PRs:UnknownError: Internal error.(fix(storage): reopen the IDB connection onUnknownError: Internal error.react-native-onyx#853). Onyx classifiedUnknownError: Internal error.as an unclassifiable error, so the operation layer retried each write six times and gave up, while the connection layer never reopened the database. This maps that exact wording to the class the connection layer already owns, so a failed read or write now drops the cached connection, reopens it, and retries (budgeted at 3 attempts, reset on any success). If the reopen does not help, the existing heal budget exhausted alert fires and the write is dropped, instead of every operation burning its full retry budget.Onyx.get(key)andOnyx.multiGet(keys)become public and typed (Add public Onyx.get - mutable react-native-onyx#850). No app code reads with them yet. The only source change issrc/setup/addUtilsToWindow.tsandreact-native-onyx.d.tsdropping the hand-madeconnectWithoutViewshim and itsWindowdeclaration (16 lines), so the dev consoleOnyx.getandOnyx.loggo through the library method.errorrepo-wide.rulesdir/no-unsafe-onyx-readguards where a read may run: only insrc/components,src/pages,src/hooksandtests, and never during render, at module scope or inside an effect (a same-file helper that render calls counts as render). A one-shot read in those places returns stale data or re-runs on every render, so there is no valid case and the rule cannot be disabled.rulesdir/no-onyx-get-snapshot-keyguards which keys may be read: it bans the Search snapshot keys (SEARCH_SNAPSHOT_ONYX_KEYS) and any key lint cannot resolve, including eachmultiGetelement, becauseuseOnyxredirects those keys to a Search snapshot andOnyx.getwould return live data where the component saw the snapshot. It is a separate rule because some reads are known safe (a migrated module-levelOnyx.connect, auseOnyxWithoutSnapshotssubscription), so it can be disabled with a mandatory reason.REPORTkey, but Search never returns it, so it is always read live. The exemption applies when the ID is a same-fileconstread fromONYXKEYS.CONCIERGE_REPORT_ID.scripts/onyxLintBypass.ts(renamed fromonyxConnectBypass.ts) fails lint on any inline disable ofno-onyx-connect,no-unsafe-onyx-reador the OnyxUtils import restriction.scripts/onyxLintDisableReason.tsaccepts a disable ofno-onyx-get-snapshot-keyonly in the-lineor-next-lineform with at least 12 letters or digits after--.ONYX-DATA-MANAGEMENT.mdlistsOnyx.getandOnyx.multiGetas read paths, names when a live read is right (a migrated module-levelOnyx.connectused only by handlers), and shows the disable shape, including where it sits for amultiGet..github/workflows/onyxGetReviewers.ymlrequests the Onyx performance reviewers and leaves a comment whenever a non-draft PR adds a net-newOnyx.getorOnyx.multiGetcall undersrc/, ignoring mocks, comment lines and the dev console'swindow.Onyx.get. It mirrorsonyxConnectWithoutViewReviewers.yml.Automated coverage:
tests/unit/NoUnsafeOnyxReadRuleTest.ts(141 cases),tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts(88, including an exhaustive check that the rule rejects exactly theONYXKEYSentriesuseOnyxwould redirect),tests/unit/OnyxLintBypassTest.ts(38) andtests/unit/OnyxLintDisableReasonTest.ts(10), all passing.Fixed Issues
$ #101910
PROPOSAL:
Tests
Onyx 3.0.118
The steps below cover Onyx #853 (reopen the IDB connection on
UnknownError: Internal error.).Test 1: Data loads and persists after the IndexedDB database is deleted (web only)
indexedDB.deleteDatabase('OnyxDB');.(await import('/node_modules/idb-keyval/dist/index.js')).get('test')in the console and confirm it resolves without an error.Onyx 3.0.119
The steps below cover the public
Onyx.getandOnyx.multiGetAPI, the two lint rules with their scripts, and the reviewers workflow. No extra code is needed.Test 2: Dev console helpers go through the library method (no other usage currently)
await Onyx.get('session')in the console and confirm it returns the session object.Onyx.log('session')and confirm the session is logged.Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
TEST 1:
onyxget-web-1.mov
TEST 2:
onyxget-web-2.mov