refactor(ui): fix the react-hooks 7 compiler-era findings, and unify the app on one data layer - #9873
Merged
Merged
Conversation
…nts errors (#9588) useLocalStorage is rebuilt on useSyncExternalStore -- localStorage IS an external store, and subscribing to it directly removes both the mount-time setState-in-effect and the render-phase ref write the old shape needed. The snapshot is the raw string, a primitive, so it is stable by construction; the parse happens once per change. Incidentally removes the first-paint flash, since the stored value is now available on the first client render rather than one effect later. The docs route uses the client loader's useContent(path), which returns react NODES, instead of getComponent(path), which mints a component during render -- a fresh component identity each render remounts the whole page body. RetryCountdown reads a 250ms clock through useSyncExternalStore rather than seeding useState with Date.now() and ticking it from an effect. The snapshot is cached because a bare () => Date.now() is never stable and would re-render forever; one shared interval now serves every mounted countdown.
…riting them from effects (#9588) useApiResource keys its settled result by the exact inputs it was fetched for, so both "loading" and the stale-response guard (#7785) fall out of comparing that key against the current inputs -- the request-id ref did the same job by hand, and a disabled resource no longer renders a frame of loading before an effect corrects it. usePreviewDataState derives isLoading from which version has settled, so bumping the version re-enters loading on the same render that requested the refresh. ApiProgressBar shows on the render that sees activity start, via React's documented adjust-state-during-render pattern; only the 240ms trailing fade remains a timer, whose setState runs in the timer callback rather than an effect body.
…bar cookie (#9588) The api-status-banner effect changed no rendered output: `dismissed` already compares the stored key against the current one, so a stale key reads as not-dismissed by derivation. The effect only scrubbed state nothing consulted. The command palette resets its highlighted index during render, so the reset lands in the same commit as the new filtered list rather than a frame after it. The sidebar's open state is a cookie -- an external store, now read as one instead of copied into state by a mount effect. The server snapshot stays undefined, which is what the hydration gate already keys on, so output is unchanged.
…s external stores (#9588) github-stats-chip read sessionStorage into state from a mount effect; sessionStorage IS an external store, so it is read as one. The snapshot memoizes on the raw string, because re-parsing per call returns a fresh object every render and loops forever. useCountUp sampled prefers-reduced-motion and tab visibility inside its effect and then wrote the final value synchronously when either said "no animation". Both are genuinely subscribable -- a matchMedia change list and visibilitychange -- so they are a store, which lets the no-animation case be derived: the value simply IS the target. Only the rAF and safety-net callbacks still set state, which they may.
…9588) chat-qa-panel back-filled its select with the first eligible PR from an effect; the list arrives asynchronously, so that left one render showing an empty select before correcting itself. The value is now derived from the choice and the list together. commands-panel keys its preview by the exact inputs it was fetched for, so clearing on change is a derivation rather than a synchronous write at the top of the effect -- and a stale in-flight response now lands under the old key and can never be shown.
…repo field during render (#9588) The try-it panel cleared five fields from an effect whenever the operation changed -- a hand-maintained list that would silently miss any field added later. It is now keyed on the operation, which is React's documented way to reset state on an input change, and the stored token is read in a lazy initializer instead. maintainer-panel had two effects both seeding the same repo field. They are one render-time adjustment now, so the field is populated on the render that first sees the options; `appliedSeed` is what keeps a user's own edit from being overwritten.
…rolled effect (#9588) The app already ran a QueryClient at its root while this hook maintained a second, parallel data layer for 80+ call sites. Backing it with react-query removes the mount effect that called setState, the manual request-id guard against out-of-order responses, and the bespoke reload plumbing -- all of which the library does by keying on the request instead of by hand. retry, refetchOnWindowFocus and gcTime are pinned EXPLICITLY rather than inherited: react-query defaults to three retries and a focus refetch, neither of which this hook did. Adopting the library therefore changes no observable behaviour at any call site; loosening them is a deliberate per-surface decision, not a side effect. Test context comes from one shared helper rather than a wrapper per file. Several files had already grown their own copy, and sharing it is what keeps the client's test defaults consistent -- a retrying client turns an asserted error state into a timeout, and a shared cache lets one case's response answer the next case's.
…ough react-query (#9588) Both hand-rolled the load/cancel/error state machine, including a `cancelled` callback to drop a response a newer repo selection had superseded -- which is just keying, so the query key does it now. AI-review settings seeds EDITABLE fields from its response, so the seed is gated on dataUpdatedAt and applied during render: a fresh response re-seeds, and a user's own edits survive every re-render in between.
…ugh react-query (#9588) Same hand-rolled load/cancel/error machinery as the two panels before them; the query key does the supersession the `cancelled` callback did by hand. The onboarding card's unparseable-target case is now a DERIVED error rather than a fetch that never happens -- it depends only on the target, so it needs no request and no state to represent it.
…h react-query (#9588) Both editors keep their content in state because both are edited; the seed is gated on dataUpdatedAt and applied during render, so a fresh response re-seeds while a maintainer's in-progress edits survive every re-render in between. The focus-manifest read still renders a failure as an empty editor rather than an error surface, exactly as before.
…9588) Later pages are appended by loadMore, so the rendered list stays state -- seeded from each new first-page response, which is also what resets pagination when the filters change. A role that cannot read the feed is now a derived error rather than a fetch that never happens. The generation ref that invalidates an in-flight loadMore is bumped from an effect: a ref may be written there, but not touched during render.
…effect (#9588) The event subscription stays an effect -- that is what effects are for. What moved out is the mount-time fetch. The query owns only WHEN the read runs; the session itself stays in the state that sign-in, sign-out and the preview path already write, so every consumer keeps seeing one value from one place.
…long to (#9588) The typing progress belongs to one scene and the TOC to one route, so both are stored under that key -- advancing the scene or navigating resets them by derivation rather than by setState calls at the top of an effect. Reduced motion needs no state at all: the finished frame is the pure value. purity, refs and static-components are back at error, where the recommended preset puts them. set-state-in-effect stays at warn for the single remaining site: the docs rail reads its headings from the rendered DOM, and sourcing them from the compiled MDX toc needs the layout/child-route split resolved first (#9872).
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 8decdcf | Commit Preview URL Branch Preview URL |
Jul 29 2026, 05:31 PM |
Contributor
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-29 17:36:33 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Bundle ReportChanges will increase total bundle size by 2.0kB (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
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.
Closes #9588.
What this fixes
eslint-plugin-react-hooks5 → 7 (forced by the eslint 10 security chain in #8608) introduced four React-Compiler-era rules that flagged 27 sites across 24 files. #8608 demoted them towarnrather than smuggle real UI refactoring into a dependency bump. This does the refactoring.26 of the 27 are fixed.
purity,refsandstatic-componentsare back aterror, where the recommended preset puts them.The through-line: nothing that can be derived is stored
Most of these were state written into an effect that could simply be computed. Recurring shapes:
useApiResource's manual request-id guard,commands-panel's reset-then-fetch,audit-feed's pagination reset, the docs rail's per-route reset, and the terminal's per-scene typing progress. It also drops superseded in-flight responses for free — they land under the old key and can never be read.prefers-reduced-motion, tab visibility and "have we hydrated" are alluseSyncExternalStorenow. Each snapshot is a primitive or memoized on one, because a fresh object per call re-renders forever.key. The try-it panel cleared five fields from an effect — a hand-maintained list that would silently miss any field added later. It is keyed on the operation instead.Two effects turned out to change no rendered output at all and were deleted:
api-status-banneralready deriveddismissedcorrectly, andchat-qa-panelback-filled a value that could be computed.The structural part: one data layer, not two
The app already ran a
QueryClientat its root whileuseApiResourcemaintained a second, parallel data layer for 80+ call sites. React's own guidance — linked from the rule's message — is that fetch-on-mount belongs in a data-fetching library, so that is what the fetch-shaped sites now use.retry,refetchOnWindowFocusandgcTimeare pinned explicitly rather than inherited: react-query defaults to three retries and a focus refetch, neither of which these call sites did. Adopting the library therefore changes no observable behaviour anywhere; loosening them is a deliberate per-surface decision, not a side effect of this PR.Test context comes from one shared helper (
src/lib/test-query-client.tsx) rather than a wrapper per file — several files had already grown their own copy. Sharing it is what keeps the client's test defaults consistent: a retrying client turns an asserted error state into a timeout, and a shared cache lets one case's response answer the next case's request.The one site left, and why
set-state-in-effectstays atwarnfordocs-toc, which builds the "On this page" rail by querying the rendered DOM. The real fix is fumadocs' compiledtoc, but the component renders in the docs layout while that data resolves in the child route — and it also serves docs pages that are not MDX at all and so have no compiled toc. Hacking around either would be worse than the scrape. Tracked with the full analysis in #9872; the rule goes toerrorthere.Validation
ui:typecheckclean,ui:buildclean,git diff --checkclean.dataUpdatedAtso a maintainer's in-progress edits survive re-renders.