Skip to content

VC-216: adopt react/globals, react/purity and react/use-memo - #897

Merged
hussainph merged 3 commits into
mainfrom
volli/VC-216-react-compiler-lint-first-slice
Oct 18, 2026
Merged

hussainph merged 3 commits into
mainfrom
volli/VC-216-react-compiler-lint-first-slice

Conversation

@hussainph

@hussainph hussainph commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

First slice of VC-216: turn react/globals, react/purity and react/use-memo back on and fix what they find. The other seven rules (refs, set-state-in-effect, exhaustive-effect-dependencies, hooks, immutability, memo-dependencies, preserve-manual-memoization) stay off. They're tracked in VC-784, which lists current finding counts per rule (376 in total).

The ticket's triage said 8 findings for these rules. On current main there are 31: 21 globals, 8 purity and 2 use-memo. 20 of the globals findings are in test files.

Findings and fixes

react/purity (8)

Where Fix
components/ui/sidebar.tsx SidebarMenuSkeleton — Math.random() in useMemo(…, []) useState(() => …): the width is still drawn once per mount, and now that's guaranteed rather than a memo hint
components/chat/empty/streak-visual.tsx — useMemo(() => Date.now(), []) useState(() => Date.now()): same "taken once per mount" rule as the existing comment
components/usage-limits/usage-limits-popover.tsx — useRef(Date.now()) (called on every render, only the first value kept) Mount time read once through a lazy useState, then used as the ref's initial value. The focus gate behaves the same
components/chat/chat-plane.tsx MessageDelivery — Date.now() in render to work out the remaining wait The clock is read once at mount (useState), and remaining is clamped to 600ms. When the stamp is there at mount (the normal path, including a held → durable handoff, which keeps its stamp), the tab still shows at since + 600. Before, every re-render restarted the timer with a smaller delay; now it doesn't restart. One behaviour difference: a stamp that arrives after the tab mounted, while its label stays non-failure, ends the wait at 600ms after mount. That can be earlier than since + 600, but it's never sooner than 600ms after mount. The old code ended it at about since + 600. I found no production path that stamps a mounted tab late; the new test reaches it by amending a held message directly. Both cases are covered by new tests (below)
components/chat/chat-plane.tsx pendingSince — Date.now() in useMemo Commented disable on that one line. The memo stamps each id with the time it was first seen pending, and every later pass keeps that stamp, so re-running the memo can't move a clock. The stamp lives in a ref written during render, which is a react/refs finding. A clean fix needs that ledger restructured, which is outside this slice, so this line moves with the refs adoption
lab/scratches/host-connection.tsx (×2) — Date.now() in the fake source's useMemo openedAt is read once through useState and becomes a memo dependency (it never changes)
lab/scratches/split-tabs.tsx — useRef(Date.now()) then reading .current in render useState(() => Date.now())

react/use-memo (2)

Where Fix
lab/scratches/ticket-kickoff.tsx — useMemo(labModelAccess, []) The stateful client is held in useState(labModelAccess), the same way as model-tiers
lab/scratches/model-tiers.tsx — useMemo(labModelAccess, [generation]) Inlining it makes react-hooks/exhaustive-deps flag generation as unused, so the client and its generation now sit in one useState, and Reset swaps in a fresh client. Behaviour is the same: Reset remounts the provider with a new client

react/globals (21)

  • 20 in test probes (14 test files). These are function Probe() { handle = useThing(); return null; } capture harnesses and renders += 1 render counters. I added one overrides entry in vite.config.ts that turns react/globals off for **/*.test.ts(x), with a comment explaining why. A probe exists to hand render-time values to its assertions; for render counters, the render count is the thing being measured. No test is compiled by React Compiler. Product code keeps the rule at error. I chose this over rewriting 20 working probes in 14 files that other PRs touch tonight. If you'd rather have those probes moved to renderHook, that's a mechanical follow-up.
  • 1 in lab/scratches/chat-performance.tsx: CountedTurn's turnRenders += 1 is the scratch's measurement, and its doc comment already explains why it's a memo wrapper rather than a Profiler. It gets a commented disable on that one line.

So there are two oxlint-disable-next-line comments, each covering one line, naming one rule and giving its reason. There are no blanket suppressions.

Verification

  • vp lint: 0 warnings, 0 errors (it was 31 errors with the three rules on before these fixes).
  • vp fmt --check on the changed files: clean.
  • vp run --filter @volli/desktop typecheck and vp exec tsc --noEmit -p tsconfig.json: clean.
  • New regression tests in chat-plane.window.test.tsx:
    • keeps the wait's clock from the hold when ownership hands off part-way through it: the hold, then a durable command.recorded at 300ms; the tab is absent at 599ms and shows at 600ms. This fails if since is ignored.
    • never holds a wait longer than 600ms after the tab mounted when its stamp arrives late: the stamp arrives 300ms after mount, and the tab shows at 600ms after mount. This fails without the Math.min(600, …) clamp, where it would show at 1200ms.
    • I checked both failure cases by temporarily mutating the code.
  • Focused tests: chat-plane.window and chat-plane.progress pass (81 tests) after the follow-up commit. Earlier: 6 files and 120 tests passed — chat-plane.progress, chat-plane.window, chat-plane, chat-plane.acceptance (these cover the delivery footer and its delayed "Pending" tab), usage-limits-popover and chat-empty-state (streak). None of the touched files are on a protected coverage list. The only fix that changes observable behaviour is MessageDelivery, and the two tests above cover it.
  • The lab scratches have no tests. They are dev-server only and typecheck clean.

Overlap with open PRs

chat-plane.tsx is also touched by #873 (VC-614, large) and #878 (VC-516). Their hunks are at ~174/253/335/1894/2979. Mine are at ~1056 (pendingSince) and ~3012 (MessageDelivery), a few lines each, so I expect clean merges.

Re-enable three of the ten React Compiler rules oxlint 1.79 split out and
fix their findings on main:

- purity: read the clock or RNG once per mount through a lazy useState
  initializer (sidebar skeleton width, streak window, usage popover's
  initial askedAt, MessageDelivery's wait, two lab scratches).
- purity: chat-plane's pendingSince first-seen stamp keeps a commented
  disable; the stamp is idempotent per id and moves with the ref-held
  ledger when react/refs is adopted.
- use-memo: inline the lab model-access factories (model-tiers holds its
  client in state so Reset needs no unused memo dependency).
- globals: test probe components keep capturing hook values and counting
  renders through a test-file override; the chat-performance scratch's
  render counter keeps a commented disable.

The other seven compiler rules stay off for their own follow-ups.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 2a10b3ee-1003-4b47-97b8-b8874df2869d
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

- Regression tests: a held -> durable handoff part-way through the wait
  still ends it 600ms from the hold, and a pending stamp that arrives after
  the tab mounted never holds the wait past 600ms after mount (the clamp).
- MessageDelivery's comment states the late-stamp case precisely; the
  pendingSince comment describes behaviour only.
- ticket-kickoff scratch holds its client in useState like model-tiers.
- vite.config.ts names the follow-up (VC-784) and rewraps its comment.
@hussainph
hussainph merged commit 883c547 into main Oct 18, 2026
23 checks passed
hussainph added a commit that referenced this pull request Oct 18, 2026
…ld-toggles-and-persists-the-sid

VC-808: sidebar ⌘B accepts ⌘ only, so ⌃B moves the caret again (stacked on #897 — merge after it)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant