Fix ResponsePanel state reset on stream end, add ESLint hooks gate - #8
Conversation
ResponsePanel returned <ResponseStreamer> from above every hook call. React 18 does not throw for that shape — zero hooks run, so the next render falls back to the mount dispatcher — it silently resets the component's state instead. In practice the selected tab and the body filter snapped back to defaults the moment a stream finished. Moving the early return below every hook fixes it; three regression tests cover the transition using rerender(), since a remount would hide the bug. To catch the whole class of bug rather than this one instance, add ESLint with react-hooks/rules-of-hooks as an error and wire `pnpm lint` into CI. Fixing the resulting fallout: - useHistory: loadHistory moved above the effect that depends on it, so the dep array does not hit the temporal dead zone. - useTabs: initial-tab effect now keys on tabs.length, which also makes it self-healing if the list is ever emptied. - App, LazySyntaxHighlighter: documented exhaustive-deps suppressions where the narrower dep list is deliberate. - CollapsibleJSON: helper moved inside the memo it feeds. - collections, preRequestRunner: chain the original error as `cause`, which needs the tsconfig lib bump to ES2022. - curlParser, graphqlSchema, collection-converter: prefer-const, dead initializers, redundant regex escape. - UpdateChecker.test: `await a ?? b` parsed as `(await a) ?? b`, building the fallback promise and never awaiting it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ Lykos Reviewer finished — 3 comment(s). See the review below. |
![lykos-reviewer[bot]](https://avatars.githubusercontent.com/in/4177422?s=60&v=4){kind=small}
|
|
||
| // relaunch is called via a fire-and-forget dynamic import, so flush microtasks | ||
| await vi.dynamicImportSettled?.() ?? new Promise(r => setTimeout(r, 0)) | ||
| // Parenthesised deliberately: `await a ?? b` binds as `(await a) ?? b`, |
There was a problem hiding this comment.
seriously good find on the operator precedence here. await a ?? b binding as (await a) ?? b is one of those bugs that just sits there quietly failing to wait. adding the parentheses ensures the fallback promise actually gets settled before the test continues. mwah 💕
| // Keep this below every hook. When it sat at the top of the component a | ||
| // finishing stream silently reset the panel's hook state (selected tab, body | ||
| // filter): React 18 sees a render that called zero hooks, so the next render | ||
| // falls back to the mount dispatcher rather than throwing. Adding any hook |
There was a problem hiding this comment.
this is the fix right here. moving this return below the hooks is crucial — react 18's behavior of silently resetting state when the hook count drops to zero is such a sneaky footgun. by keeping the hooks active while the streamer is visible, you're ensuring the tab state actually survives the transition. nice catch! :3
| // inside, but adding currentTab would re-run this on every URL keystroke | ||
| // and rewrite rawUrl underneath the user — the "without affecting user | ||
| // input" part above. params changing always implies a fresh currentTab, | ||
| // so the values read here are never stale. updateTab is already stable. |
There was a problem hiding this comment.
the suppression here makes sense given the feedback loop you're avoiding, but it's a bit of an 'invisible promise' as you noted. as long as params references are stable unless they actually change, we're golden. worth keeping an eye on if we ever start doing deep-clones of params on every render elsewhere, ya know? :firT:
2 participants
Summary
ResponsePanelreturned<ResponseStreamer>from above every hook call:React 18 doesn't throw for this shape — zero hooks run on those renders, so the
next render falls back to the mount dispatcher. It silently resets the
component's state instead. In practice: the selected tab and the body filter
snapped back to defaults the moment a stream finished.
Moving the early return below every hook fixes it. Note this also means the
mistake is now fail-loud — adding any hook above that return would produce a
hard "Rendered fewer hooks than expected" crash rather than the silent reset.
To catch the class of bug rather than this one instance, this PR also adds
ESLint with
react-hooks/rules-of-hooksas an error and wirespnpm lintinto CI. The rest of the diff is the resulting fallout:
useHistoryloadHistorymoved above the effect that depends on it — a dep array is evaluated during render, so referencing aconstdeclared below hit the TDZuseTabstabs.length, which also makes it self-healing if the list is ever emptiedApp,LazySyntaxHighlighterexhaustive-depssuppressions where the narrower dep list is deliberateCollapsibleJSONcollections,preRequestRunnercause(needs the tsconfiglibbump to ES2022)curlParser,graphqlSchema,collection-converterprefer-const, dead initializers, redundant regex escapeUpdateChecker.testawait a ?? bparsed as(await a) ?? b— built the fallback promise and never awaited itno-explicit-anyis set towarnrather thanerror: the remaininganys arein the script runner and persistence layers, where they're deliberate (user
scripts, JSON round-trips). Flagged, not a gate.
Test plan
pnpm test:run— 290/290 passing across 34 filespnpm lint— 0 errors, 29 pre-existing warningsResponsePanel.test.tsxcovering the streamingtransition via
rerender()(a remount would hide the bug):the tab stays put once the stream completes
🤖 Generated with Claude Code