experimental: migrate from react -> redact (thanks to Tanner Linsley and team)#100
experimental: migrate from react -> redact (thanks to Tanner Linsley and team)#100nikilok wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds Changes@ss/redact end-to-end introduction
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
nikilok
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
packages/redact/src/react/shared-internals.ts-53-63 (1)
53-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefactor singleton initialization to avoid assignment-in-expression lint error.
Biome flags this as
lint/suspicious/noAssignInExpressions; if lint is gating, this will fail CI.Suggested fix
-export const ReactSharedInternals: SharedInternals = - g[KEY] ?? - (g[KEY] = { - H: null, - T: null, - S: null, - currentFiber: null, - currentRoot: null, - currentHook: null, - hookIndex: 0, - }) +let shared = g[KEY] +if (!shared) { + shared = { + H: null, + T: null, + S: null, + currentFiber: null, + currentRoot: null, + currentHook: null, + hookIndex: 0, + } + g[KEY] = shared +} + +export const ReactSharedInternals: SharedInternals = shared🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/react/shared-internals.ts` around lines 53 - 63, The current export uses an assignment inside the nullish coalescing expression (g[KEY] ?? (g[KEY] = {...})) which triggers the lint rule noAssignInExpressions; fix by performing the singleton initialization in separate statements: read g[KEY] into a local variable, if it's falsy create the SharedInternals object and assign it to g[KEY], then export that local variable as ReactSharedInternals (referencing ReactSharedInternals, SharedInternals, g, and KEY to locate the code).packages/redact/scripts/build.mjs-103-107 (1)
103-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow declaration build failures.
If
bun x tscfails, the package can be published with stale/missing types while CI still appears green. Make this step fail fast.Suggested fix
try { execSync(`bun x tsc -p ${tsconfigPath}`, { cwd: pkgDir, stdio: 'inherit' }) - } catch { - // Non-fatal: declarations may be incomplete, but JS still builds. + } catch (error) { + throw new Error( + `Type declaration generation failed for `@ss/redact`. Aborting build.`, + { cause: error as Error }, + ) } finally { rmSync(tsconfigPath, { force: true }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/scripts/build.mjs` around lines 103 - 107, The current try/catch around the execSync call that runs `bun x tsc -p ${tsconfigPath}` silently swallows failures; update the try/catch in packages/redact/scripts/build.mjs so that declaration build failures fail fast by either removing the catch or capturing the caught error (e) and rethrowing it (or calling process.exit(1)); specifically modify the block that surrounds execSync (using symbols execSync, tsconfigPath, pkgDir) to let errors propagate or explicitly throw the error after logging so CI fails on tsc failures.packages/redact/src/server/walk.ts-266-269 (1)
266-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPop the select context in a
finally.If rendering a
<select>'s children throws or suspends, control exits beforepopSelectContext(). That leaked state can make later<option>s elsewhere in the tree serialize as selected against the wrong parent<select>.♻️ Suggested fix
if (isSelect) { const val = props.value != null ? props.value : props.defaultValue pushSelectContext(val) } @@ - if (dangerouslyHtml != null) { - opts.emit(String(dangerouslyHtml)) - } else { - walkNode(props.children, childOpts) - } - opts.emit(`</${tag}>`) - if (isSelect) popSelectContext() + try { + if (dangerouslyHtml != null) { + opts.emit(String(dangerouslyHtml)) + } else { + walkNode(props.children, childOpts) + } + opts.emit(`</${tag}>`) + } finally { + if (isSelect) popSelectContext() + }Also applies to: 345-351
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/walk.ts` around lines 266 - 269, The code pushes select context with pushSelectContext(val) when isSelect is true but does not guarantee popSelectContext() runs if rendering children throws or suspends; wrap the push/pop pair in a try/finally so popSelectContext() always executes (e.g., call pushSelectContext(val) then try { /* render children */ } finally { popSelectContext() }), and apply the same change to the second occurrence referenced around the push/pop at the other block (the code around lines 345-351) to prevent leaked select state.packages/redact/src/server/renderToString.ts-31-32 (1)
31-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't strip every HTML comment in static markup mode.
This removes user content too, not just hydration markers. Comments coming from CMS HTML,
dangerouslySetInnerHTML, or raw script/style payloads will be silently mangled. Strip only the renderer's own marker shapes here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/renderToString.ts` around lines 31 - 32, The current replacement in the renderToString export indiscriminately strips all HTML comments from the renderer output (return renderToString(children, options).replace(...)), which removes user content; change it to only remove the renderer's own hydration/marker comment shapes instead of every comment. Locate the return that calls renderToString(children, options) and replace the blanket comment-strip with a targeted removal that matches only the renderer-specific marker pattern(s) your renderer emits (use a regex or sanitizer that matches those unique marker shapes) so CMS/dangerouslySetInnerHTML comments remain untouched. Ensure the change preserves other behavior of renderToString in this module.packages/redact/src/server/walk.ts-437-443 (1)
437-443:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe fallback wrapper
divchanges the document structure.Wrapping every pending boundary in
<div id="B:...">is not neutral HTML. Inside parents like<p>,<table>,<select>, or inline text flows, the browser will reparent or rewrite the DOM before hydration, so the boundary can no longer hydrate against what was emitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/walk.ts` around lines 437 - 443, emitBoundary currently wraps the fallback HTML in a visible <div id="B:..."> which mutates document structure; change it to emit an inert <template id="B:..."> wrapper instead so the fallback is not reparented or cause invalid nesting (keep the existing leading/trailing comments used for hydration). Update emitBoundary (and any callers that rely on the id) to emit: start comment, opening <template id="B:${id}">, fallbackHTML, closing </template>, end comment via opts.emit; ensure WalkOptions.emit usage and the id/fallbackHTML parameters remain the same.packages/redact/src/server/walk.ts-391-405 (1)
391-405:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed
textStatewhen rendering Suspense children and fallback.These nested
walkNode()calls bypasswalk(), so adjacent text inside a Suspense boundary won't emit the<!-- -->separators that hydration relies on. A boundary containing['a', 'b']currently serializes as one text node instead of two.♻️ Suggested fix
walkNode(props.children, { emit: childEmit, onSuspend: opts.onSuspend, nextBoundaryId: opts.nextBoundaryId, + textState: { lastWasText: false }, }) @@ walkNode(props.fallback, { emit: (s) => fallbackParts.push(s), onSuspend: opts.onSuspend, nextBoundaryId: opts.nextBoundaryId, + textState: { lastWasText: false }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/walk.ts` around lines 391 - 405, The nested walkNode calls for props.children and props.fallback bypass walk's text-state handling so adjacent text inside a Suspense boundary is merged; fix by seeding/passing a textState in the options when invoking walkNode (instead of relying on walk), e.g. derive a childTextState = opts.textState ?? { /* initial text state */ } and include it in the options passed to walkNode for both props.children and props.fallback so separators (the <!-- --> markers) are emitted correctly; update the walkNode invocations referenced in the try/catch around props.children and inside the fallback handling to pass this textState.packages/redact/src/scheduler/index.ts-23-49 (1)
23-49:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUpdate
unstable_scheduleCallbackto match the public scheduler API contract.The callback must receive a
didTimeout: booleanparameter and handle continuation functions. Currently,fn()is invoked with no arguments and the return value is ignored. The correct signature is:(didTimeout: boolean) => void | ((didTimeout: boolean) => void | null)Libraries relying on actual scheduler semantics (timeout tracking, work resumption) will break with this shim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/scheduler/index.ts` around lines 23 - 49, unstable_scheduleCallback currently invokes task.callback with no args and ignores its return; update the run closure in unstable_scheduleCallback to call the callback with a didTimeout boolean (true when invoked via setTimeout, false for microtask), capture its return which may be a continuation function or null, and if a continuation is returned set task.callback to that continuation (bound to accept didTimeout on next run) so work can resume; preserve cancellation checks (task.cancelled) and keep the existing error re-throw logic via setTimeout, and ensure when no continuation is returned you clear task.callback to null.packages/redact/src/server/stream.ts-92-95 (1)
92-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reporting the same fatal error twice.
streamHtml()already callsoptions.onErrorbefore rethrowing. The rejection branch inrenderToPipeableStream()calls it again aftershellReady, so the Node API double-logs the same failure.Suggested fix
(err) => { if (!shellReady) { options.onShellError?.(err) } else { - options.onError?.(err) if (dest) dest.end() } },Also applies to: 276-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/stream.ts` around lines 92 - 95, streamHtml() already invokes options.onError before rethrowing, causing duplicate error reports when renderToPipeableStream() also calls options.onError in its rejection path; modify renderToPipeableStream() so that when state.errored is set (the path where streamHtml() has already called options.onError) it does not call options.onError again—use the existing state.errored sentinel (and the same branch around shellReady/rejection handling) to guard the extra options.onError invocation and only call options.onError from renderToPipeableStream() when state.errored is not already set.packages/redact/src/server/stream.ts-234-238 (1)
234-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
options.signalinto the pipeable path too.
PipeableOptionsinheritssignal, butrenderToPipeableStream()never subscribes to it. Callers using the same cancellation path asrenderToReadableStream()get a no-op on Node unless they also remember to callhandle.abort().Suggested fix
export function renderToPipeableStream( children: ReactNode, options: PipeableOptions = {}, ): PipeableHandle { @@ + const abort = () => { + aborted = true + state.closed = true + if (dest) dest.end() + } + + if (options.signal?.aborted) abort() + else options.signal?.addEventListener('abort', abort, { once: true }) + return { @@ abort(_reason?: unknown) { - aborted = true - state.closed = true - if (dest) dest.end() + abort() }, } }Also applies to: 240-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/stream.ts` around lines 234 - 238, The PipeableOptions.signal is not wired into renderToPipeableStream, so callers using an AbortSignal get no cancellation; update renderToPipeableStream to subscribe to options.signal (if present), add an 'abort' listener that invokes the same abort/cleanup path used by renderToReadableStream (e.g., call the stream/controller/handle abort method or the internal cleanup function used by renderToReadableStream), and ensure the listener is removed when onShellReady/onAllReady/onShellError or final cleanup runs; reference PipeableOptions, renderToPipeableStream, options.signal and the existing abort/cleanup handler so the cancellation behavior matches the readable-stream path.packages/redact/src/server/stream.ts-150-164 (1)
150-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
bootstrapScripts[].async.
bootstrapTag()always emitsasyncfor classic scripts, so{ async: false }can never be expressed. That changes bootstrap execution ordering and breaks the public option contract.Suggested fix
function bootstrapTag( entry: string | { src: string; async?: boolean; nonce?: string }, kind: 'script' | 'module', defaultNonce: string | undefined, ): string { @@ const nAttr = nonce ? ` nonce="${escapeAttr(nonce)}"` : '' const srcAttr = escapeAttr(src) if (kind === 'module') return `<script type="module"${nAttr} src="${srcAttr}"></script>` - return `<script async${nAttr} src="${srcAttr}"></script>` + const asyncAttr = + typeof entry === 'string' || entry.async !== false ? ' async' : '' + return `<script${asyncAttr}${nAttr} src="${srcAttr}"></script>` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/stream.ts` around lines 150 - 164, bootstrapTag currently always emits the async attribute for classic scripts, ignoring an entry object's async flag and breaking bootstrapScripts[].async semantics; change bootstrapTag to compute an isAsync boolean (e.g., treat string entries as async by default, but for object entries use entry.async if provided) and only include the " async" token in the returned classic-script tag when isAsync is true (leave module branch unchanged); update the classic return that builds the tag (the code using nAttr and srcAttr) to conditionally insert the async attribute based on that computed value.apps/web/package.json-55-56 (1)
55-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClarify PR intent—React is actively used throughout the codebase.
The PR title indicates a migration from React to redact, but the code shows the opposite: React remains heavily integrated with 17 direct imports across routes, hooks, and components (useState, useEffect, useRef, useCallback, useId, Suspense, createPortal, flushSync). Meanwhile,
@ss/redactis not imported anywhere in the codebase.Either the PR title is incorrect, or the migration is incomplete. Clarify:
- Is this PR actually migrating to redact, or does it serve a different purpose?
- If redact is intended as a replacement, why does the implementation still rely on React?
- If they must coexist, document the transition plan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/package.json` around lines 55 - 56, The PR claims a migration to "@ss/redact" but package.json still lists "react" and "react-dom" and the codebase continues to import React APIs (useState, useEffect, useRef, useCallback, useId, Suspense, createPortal, flushSync) while there are no imports of `@ss/redact`; update the PR to accurately reflect intent: either (A) actually migrate by replacing React usage with `@ss/redact` equivalents and remove/adjust "react" and "react-dom" entries in package.json, ensuring new imports of "@ss/redact" appear in the components/hooks that currently use useState/useEffect/etc., or (B) if React is to remain, change the PR title/description to remove "migration" language and add a short coexistence/transition plan documenting which modules will convert to `@ss/redact` and a timeline; ensure consistency between package.json dependencies and actual imports (react/react-dom vs `@ss/redact`) and update documentation accordingly.packages/redact/tests/create-root-clear-container.test.tsx-17-18 (1)
17-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImport
createRootfrom@ss/redact/dom-clientinstead ofreact-dom/client.The test uses
jsxImportSource: "@ss/redact", which means JSX elements compile to@ss/redactruntime calls. However,createRootis imported from React'sreact-dom/client(line 18). This creates an inconsistency—the test should import from@ss/redact/dom-clientto match the JSX compilation and test@ss/redact'screateRootAPI consistently.Change line 18 to:
import { createRoot } from '@ss/redact/dom-client'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/tests/create-root-clear-container.test.tsx` around lines 17 - 18, The test imports createRoot from react-dom/client but the file is compiled with jsxImportSource "@ss/redact", so update the import to use the redact DOM runtime: replace the existing import of createRoot from 'react-dom/client' with import { createRoot } from '@ss/redact/dom-client' so the test uses `@ss/redact`'s createRoot implementation consistently with the JSX runtime.apps/web/vite.config.ts-15-15 (1)
15-15:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffRemove
viteReact()and configurejsxImportSourcein tsconfig.json.Given the PR objective to migrate from React to redact, keeping
@vitejs/plugin-reactactive (line 51) creates risk and confusion:
- The
redactvite plugin (line 15) aliasesreact/jsx-runtime→@ss/redact/jsx-runtimeand is designed as a React drop-in replacement.viteReact()is a Babel-based React JSX transformer that operates independently of redact's aliases, creating potential JSX transform conflicts.tsconfig.jsonis missingjsxImportSource: "@ss/redact"—thejsx-runtime.tsfile explicitly expects this configuration (see comment at line 18-19), but tsconfig only specifiesjsx: "react-jsx"without it. This causes TypeScript to default to"react"instead of@ss/redact.If redact is intended as a full React replacement, remove
viteReact()and addjsxImportSource: "@ss/redact"toapps/web/tsconfig.jsonundercompilerOptions. If both plugins are intentionally required (e.g., for fast refresh or experimental features), document the rationale and ensure explicit JSX configuration is in place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/vite.config.ts` at line 15, The build currently mixes the redact Vite plugin and `@vitejs/plugin-react` which can conflict: remove the viteReact() plugin from the Vite plugins array (leave redact() in place) and update the TypeScript config under compilerOptions to add "jsxImportSource": "@ss/redact" alongside jsx: "react-jsx" so the JSX runtime resolves to `@ss/redact`; if you intentionally need both plugins, instead document the rationale in a comment and ensure tsconfig's jsxImportSource is set to "@ss/redact" anyway.packages/redact/src/dom/features/memo/full.ts-14-23 (1)
14-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch React's shallow prop comparison.
This comparator returns
truefor different prop shapes like{ a: undefined }vs{ b: undefined }because it doesn't check thatbowns each key froma. It also diverges from React onNaNand-0because it uses===instead ofObject.is.React-compatible comparator
function shallowEqual(a: any, b: any): boolean { - if (a === b) return true + if (Object.is(a, b)) return true if (!a || !b) return false const ak = Object.keys(a) const bk = Object.keys(b) if (ak.length !== bk.length) return false for (const k of ak) { - if (a[k] !== b[k]) return false + if (!Object.prototype.hasOwnProperty.call(b, k) || !Object.is(a[k], b[k])) { + return false + } } return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/features/memo/full.ts` around lines 14 - 23, The shallowEqual implementation incorrectly treats different shapes like {a: undefined} vs {b: undefined} as equal and uses === which mishandles NaN and -0; update the shallowEqual function to (1) ensure both inputs are non-null objects before comparing, (2) after comparing key lengths iterate keys from Object.keys(a) and for each key verify Object.prototype.hasOwnProperty.call(b, key) (so b actually owns the key), and (3) use Object.is(a[key], b[key]) instead of === to match React's semantics; keep the initial fast-path a === b check and the existing key-length check in shallowEqual.packages/redact/src/dom/features/context/full.ts-48-54 (1)
48-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExtend type matcher to handle React 19 Context-as-Provider syntax.
The type matcher only recognizes
REACT_PROVIDER_TYPEandREACT_CONSUMER_TYPEmarkers. React 19 supports rendering Context objects directly as providers using<MyContext value={x}>instead of<MyContext.Provider value={x}>. In this case, the JSX type is the Context object itself with$$typeof === REACT_CONTEXT_TYPE.When a Context object flows through the matcher, it returns
nulland falls back to thetypeof type === 'function'check. Since Context objects are plain objects (not functions), they would default toFiberTag.Hostinstead ofFiberTag.Provider, breaking context propagation for descendants.Add a case to map
REACT_CONTEXT_TYPEtoFiberTag.Provider:Suggested fix
registerTypeMatcher((_type, marker) => marker === REACT_PROVIDER_TYPE ? FiberTag.Provider : marker === REACT_CONSUMER_TYPE ? FiberTag.Consumer : marker === REACT_CONTEXT_TYPE ? FiberTag.Provider : null, )Also import
REACT_CONTEXT_TYPEfrom../../../react.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/features/context/full.ts` around lines 48 - 54, The type matcher registered via registerTypeMatcher currently maps REACT_PROVIDER_TYPE and REACT_CONSUMER_TYPE but misses React 19's Context-as-Provider objects, causing Context objects (with $$typeof === REACT_CONTEXT_TYPE) to fall through to Host; update the matcher to also return FiberTag.Provider when marker === REACT_CONTEXT_TYPE and add an import for REACT_CONTEXT_TYPE from ../../../react so Context objects are recognized as Provider; keep the existing returns for REACT_PROVIDER_TYPE and REACT_CONSUMER_TYPE and only add the new branch mapping to FiberTag.Provider.packages/redact/src/react/index.ts-111-149 (1)
111-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the default export in sync with the named surface.
useSyncExternalStoreWithSelectoris exported on Lines 35-35 but never added to the default object, soReact.useSyncExternalStoreWithSelectorbreaks even though the named import works. Deriving the default export from the named bindings would avoid this drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/react/index.ts` around lines 111 - 149, The default export object is missing the useSyncExternalStoreWithSelector symbol (exported as a named binding), causing React.useSyncExternalStoreWithSelector to be undefined; update the default export in this module to include useSyncExternalStoreWithSelector (or derive the default export programmatically from the named exports) so the default object contains the same surface as the named exports (e.g., include useSyncExternalStoreWithSelector alongside createElement, useState, useSyncExternalStore, etc.).packages/redact/src/dom/features/suspense/full.ts-119-123 (1)
119-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark streamed pending boundaries as suspended.
For
kind === 'pending', storingsuspended: falselets any rerender before$RHfires tryprops.childrenagainst a boundary whose real DOM has not been revealed yet. That can mount real content early and then tear it back down again whenrehydrateBoundary()runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/features/suspense/full.ts` around lines 119 - 123, The memoizedState for streamed boundaries incorrectly sets suspended: false for pending boundaries; change the initialization in the fiber.memoizedState assignment so that when the boundary kind is 'pending' it sets suspended: true (e.g., suspended: kind !== 'pending' or an explicit conditional), ensuring pending streamed boundaries are treated as suspended until rehydrateBoundary() runs.packages/redact/src/dom/features/hydration/full.ts-163-166 (1)
163-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat a missing keyed head attribute as a match.
The comment says “if one defines it, they must match”, but this branch currently accepts candidates where one side is missing
href/name/src/etc. That can claim the wrong<link>/<meta>/<script>node and suppress mounting the real one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/features/hydration/full.ts` around lines 163 - 166, The current logic treats a missing keyed attribute as a match; keep the check that skips when both propVal and elVal are null but change the tolerant branch that allows one side to be missing: in the hydration comparison block replace the "if (propVal == null || elVal == null) continue" behavior so that if exactly one of propVal or elVal is null it returns false (non-match) instead of continuing, ensuring the element with a missing keyed attribute cannot be claimed as a match.packages/redact/src/dom/features/hydration/full.ts-207-217 (1)
207-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack nesting when searching for the closing Suspense marker.
This stops at the first
<!--/$-->afterstartMark, so an outer streamed boundary containing an inner boundary will pair with the inner close marker and corrupt the hydration cursor range. The scan needs a depth counter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/features/hydration/full.ts` around lines 207 - 217, The scan for the closing Suspense marker must track nested boundaries: change the loop that walks from startMark.nextSibling to maintain a depth counter (initialize depth = 0); when you see a comment node with (comment as Comment).data === '$' increment depth, when you see (comment as Comment).data === '/$' decrement and if decrementing brings depth to -1 or if depth is 0 when encountering '/$' treat that '/$' as the matching endMark (set endMark = scan and break). Update references in the loop that currently only checks for '/$' so nested opening markers (Comment.data === '$') are accounted for and the correct closing marker is selected.packages/redact/src/dom/features/suspense/full.ts-175-185 (1)
175-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset hydration state in a
finally.
reconcileChildren()can still throw here. If it does,clearHydrationCursor(fiber)androot.hydrating = falseare skipped, leaving the root stuck in hydration mode and poisoning later updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/features/suspense/full.ts` around lines 175 - 185, The block that sets root.hydrating and calls reconcileChildren can throw and currently skips cleanup; wrap the hydrating region so that after creating the HydrationCursor and calling setHydrationCursor and reconcileChildren you always run clearHydrationCursor(fiber) and reset root.hydrating = false in a finally block. Concretely, keep creating the HydrationCursor and calling setHydrationCursor(fiber, cursor) then call reconcileChildren(...) inside a try, put clearHydrationCursor(fiber) and root.hydrating = false in the finally, and call runEffects(root) after the try/finally to ensure the hydration state is never left set on the root (referencing withCurrentRoot, HydrationCursor, setHydrationCursor, reconcileChildren, clearHydrationCursor, root.hydrating, and runEffects).packages/redact/src/dom/dispatcher.ts-241-272 (1)
241-272:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-subscribe when the store callbacks change.
The subscription is installed only once (when
hook.cleanup == null), so if a component switches stores or changes itssubscribe/getSnapshotafter mount, the old subscription persists and the component never resubscribes to the new source. Add dependency tracking to detect changes in thesubscribeandgetSnapshotparameters and re-subscribe accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/dispatcher.ts` around lines 241 - 272, The current subscription is only set when hook.cleanup == null, so changes to the subscribe/getSnapshot functions after mount won't re-subscribe; modify the logic in the block that uses hook.cleanup, getSnapshot and subscribe to track previous subscribe/getSnapshot on the hook (e.g., store hook._subscribe and hook._getSnapshot), compare them to the current subscribe and getSnapshot, and if they differ call the existing cleanup (if any) to unsubscribe, clear hook.cleanup, and proceed to re-create forceUpdate, call subscribe to get a new unsubscribe, assign hook.cleanup and update the stored hook._subscribe/hook._getSnapshot; ensure you still push the unsubscribe onto fiber.cleanups and preserve the isHydrating/getServerSnapshot post-hydration queueMicrotask behavior.packages/redact/src/dom/dispatcher.ts-204-210 (1)
204-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the
useIdcounter to the root instead of module-level scope.The global
idCountercouples independent roots together. When multiple roots use the sameidentifierPrefix(the default:r), they generate different IDs for identical subtrees based on mount order, causing collisions and hydration mismatches. Each root should maintain its own ID counter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/dispatcher.ts` around lines 204 - 210, The module-level idCounter causes cross-root ID coupling; instead store and increment the counter on the root object. In useId(), after getting the current fiber and root via getCurrentFiber() and findRootFromFiber(), initialize a per-root counter field (e.g., root.idCounter or root.__idCounter) if undefined and use that to build the id with root.identifierPrefix, incrementing that root counter for each new id; remove reliance on the module-scoped idCounter so identical subtrees in different roots produce stable, root-local ids.packages/redact/src/react/class.ts-30-34 (1)
30-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
setStatesilently ignore calls when unmounted, consistent withforceUpdatebehavior.
setStatethrows "Cannot call setState on an unmounted component", butforceUpdate(line 37–40) silently returns when the updater hook is absent. This inconsistency breaks React-compatible code: asyncsetStatecalls from unresolved promises or timers after unmount will crash the app instead of being ignored. The framework's ownexternal-store-unmount.test.tsxdemonstrates the design philosophy—post-unmount callbacks should be safely ignored, not throw.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/react/class.ts` around lines 30 - 34, The setState method currently throws when this._enqueueUpdate is missing; change it to mirror forceUpdate by silently returning when unmounted instead of throwing. In the setState implementation (method name: setState, symbol: this._enqueueUpdate), remove the throw and add an early return when this._enqueueUpdate is falsy so callers from async promises/timers are ignored safely; otherwise call this._enqueueUpdate(updater, callback) as before.packages/redact/src/dom/dispatcher.ts-189-197 (1)
189-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack ref parameter changes separately from
deps.The early return on stable
depsprevents ref assignment from running, so a new ref object/function never receives the handle value.useImperativeHandleshould update the ref even when the handle is memoized, since the ref parameter can change independently of the dependency array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/dispatcher.ts` around lines 189 - 197, The early return in useImperativeHandle prevents assigning the handle to a newly passed ref when deps are stable; modify the hook state to track hook.ref and hook.value (in addition to hook.deps) retrieved via nextHook(), and change the logic so that if depsEqual(hook.deps, deps) you still check whether ref !== hook.ref — if the ref changed, assign the existing hook.value to the new ref (call ref(value) or set ref.current) without re-running factory; if deps differ, call factory(), set hook.value and hook.deps and then assign to ref; ensure you update hook.ref whenever you assign so future calls can detect ref changes.
🟡 Minor comments (9)
packages/redact/tests/synthetic-event-shape.test.tsx-64-64 (1)
64-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit
typefor the buttonLine 64 uses
<button>withouttype, which triggers the Biome a11y rule and can default to unintended submit behavior.Suggested patch
- return <button onClick={(e: any) => (captured = e)}>go</button> + return <button type="button" onClick={(e: any) => (captured = e)}>go</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/tests/synthetic-event-shape.test.tsx` at line 64, The test's JSX returns a bare <button> which can default to submit and trigger the a11y rule; update the returned element in the test (the component/return that sets captured via onClick) to include an explicit type="button" attribute so it won't act as a submit button—modify the JSX where captured is assigned (the onClick handler that sets captured = e) to add the type property.packages/redact/tests/ssr.test.tsx-78-80 (1)
78-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit button type in this test.
This
<button>defaults totype="submit"and trips the a11y lint, even though the assertion only cares about event-handler stripping.Minimal fix
- expect(renderToString(<button onClick={() => {}}>x</button>)).toBe('<button>x</button>') + expect(renderToString(<button type="button" onClick={() => {}}>x</button>)).toBe( + '<button type="button">x</button>', + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/tests/ssr.test.tsx` around lines 78 - 80, The test "skips event handlers" uses a plain <button> which defaults to type="submit" and triggers the a11y lint; update the JSX passed to renderToString in that test to include an explicit type="button" attribute on the <button> element so the assertion still verifies event-handler stripping but the button is not treated as a submit control.packages/redact/tests/memo-state-rerender.test.tsx-43-45 (1)
43-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
voidfrom the unsubscribe callback.The teardown currently returns
Set.delete()’s boolean, which trips Biome’s callback-return rule and widens the cleanup contract from() => voidto a value-returning function.Minimal fix
subscribe: (cb) => { listeners.add(cb) - return () => listeners.delete(cb) + return () => { + listeners.delete(cb) + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/tests/memo-state-rerender.test.tsx` around lines 43 - 45, The unsubscribe callback currently returns Set.delete(cb)'s boolean; update the subscribe implementation so the returned teardown is a void-returning function (e.g., return () => { listeners.delete(cb); } or return () => { void listeners.delete(cb); }) to ensure subscribe's cleanup callback signature remains () => void; target the subscribe implementation that captures listeners and returns the unsubscribe closure.packages/redact/tests/memo-state-rerender.test.tsx-141-150 (1)
141-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a non-anchor element for this test host.
This node is only a render marker, so
<a>withouthrefadds link semantics the test does not need and fails the a11y lint.Minimal fix
- return <a data-testid="leaf">{v}</a> + return <span data-testid="leaf">{v}</span>Update the later selectors from
a[data-testid="leaf"]to eitherspan[data-testid="leaf"]or just[data-testid="leaf"].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/tests/memo-state-rerender.test.tsx` around lines 141 - 150, The test uses an anchor element in the Leaf component causing unnecessary link semantics and a11y lint failures; change the JSX in function Leaf to render a non-anchor (e.g., a span or a neutral element) for the test marker and update any test selectors that query "a[data-testid=\"leaf\"]" to either "span[data-testid=\"leaf\"]" or simply "[data-testid=\"leaf\"]" so selectors match the new element; locate the Leaf function and the corresponding test assertions/selectors in the memo-state-rerender.test.tsx file and make these two consistent replacements.packages/redact/tests/floating-ui-pattern.test.tsx-108-113 (1)
108-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGive the trigger button an explicit type.
This button is not submitting anything, and leaving the default
submittype in place trips the a11y lint for no gain.Minimal fix
- <button ref={setReference}>trigger</button> + <button type="button" ref={setReference}>trigger</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/tests/floating-ui-pattern.test.tsx` around lines 108 - 113, The trigger button in the App component created by useFloating currently has no explicit type and defaults to "submit", causing accessibility linter failures; update the <button ref={setReference}> in the App function to include an explicit type attribute (type="button") so it won't act as a form submitter, leaving useFloating, setReference and setFloating unchanged.packages/redact/src/dom/root.ts-68-87 (1)
68-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComment overstates the implementation — documentElement is not "adopted as a child."
The comment claims "we adopt documentElement as a CHILD of the root, not as the root itself." However, the code simply assigns
target(theDocument) directly torootFiber.dom. During hydration, the existingdocumentElementis reused in-place, not created as a child fiber. The tests confirm this works correctly:doc.documentElementremains the same reference before and afterhydrateRoot, and content renders into it without duplicate<html>elements.Clarify the comment to match the actual behavior: the root's container is the
Document, and hydration reuses the existingdocumentElementrather than nesting it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/root.ts` around lines 68 - 87, The current comment incorrectly says we "adopt documentElement as a CHILD of the root"; update the comment near the target/rootFiber.dom assignment to state that the root's container can be the Document and that during hydration we reuse the existing document.documentElement in-place (not creating a child fiber or nesting <html>), so rootFiber.dom/ rootFiber.stateNode are set to the Document (target) and hydrateRoot reuses documentElement; reference the target variable and assignments to rootFiber.dom and rootFiber.stateNode when making the clarification.packages/redact/src/react/hooks.ts-93-109 (1)
93-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStub form-action hooks silently swallow user actions — consider adding a dev warning so misuse is observable.
useActionState,useFormStatus, anduseOptimisticall return fixed placeholders, and the dispatch functions (() => {}) silently discard payloads. Any consumer code that wires these into<form action={...}>or progressive-enhancement flows will appear to work (no exceptions, no type errors) but will never execute the action.If these are deliberate stubs while the broader Action support is built out, a single dev-mode
console.warnat first invocation makes the gap discoverable instead of producing silent UX bugs in apps under migration:🛡️ Suggested guard
export function useActionState<S, P>( _action: (state: Awaited<S>, payload: P) => S | Promise<S>, initial: Awaited<S>, ): [Awaited<S>, (payload: P) => void, boolean] { + if (process.env.NODE_ENV !== 'production') { + console.warn( + '[`@ss/redact`] useActionState is a stub in this build; the action will not be invoked.', + ) + } return [initial, () => {}, false] }(Same pattern for
useFormStatus/useOptimistic.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/react/hooks.ts` around lines 93 - 109, The three stub hooks (useActionState, useFormStatus, useOptimistic) silently swallow actions; add a one-time dev-mode warning in each hook (useActionState, useFormStatus, useOptimistic) that runs only when not in production (e.g. NODE_ENV !== 'production' or similar) to alert developers that these are stubs and dispatch callbacks are no-ops, ensure the warning is emitted on first invocation only (use a module-scoped boolean flag per hook) and keep the existing return shapes so behavior in production remains unchanged.packages/redact/src/dom/reconcile.ts-405-410 (1)
405-410:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVariable name
unkeyedNewis misleading — either rename with documentation or restrict the count to unkeyed children only.
unkeyedOldcounts only unkeyed existing fibers (line 407), butunkeyedNewcounts all non-null new children, including keyed ones (line 409). This asymmetry feeds intobudget = unkeyedNew - unkeyedOld, which the cursor walk uses at type-mismatch sites.The comment on line 392 states the walk is "unkeyed only," yet the counting logic includes keyed children. The variable name strongly suggests it counts unkeyed children exclusively, making this a maintenance risk — a future reviewer will likely "fix" it to match the name and inadvertently break the heuristic.
If the all-non-null count is intentional (to approximate available slots for the budget heuristic), rename to something like
newSlotsTotaland add a comment explaining why keyed children are included in the budget calculation. If it's an oversight, restrict the count to unkeyed entries and run the reconciler tests to confirm no keyed-+-unkeyed-mix scenarios regress.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/dom/reconcile.ts` around lines 405 - 410, The code incorrectly names and computes unkeyedNew (it currently counts all non-null newChildren while unkeyedOld counts only unkeyed existing fibers), causing a misleading budget used by the cursor walk; either (A) restrict the new count to unkeyed children only (mirror unkeyedOld) so unkeyedNew truly represents unkeyed new children, or (B) if including keyed children was intentional, rename unkeyedNew to something like newSlotsTotal and add a comment explaining why keyed children are counted for the budget; update the calculation of budget (budget = newSlotsTotal - unkeyedOld or budget = unkeyedNew - unkeyedOld accordingly) and run the reconciler tests to verify no regressions (refer to variables unkeyedOld, unkeyedNew, budget, existing, newChildren).packages/redact/src/react/hooks.ts-142-189 (1)
142-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefer
instmutations touseEffectto match React's reference implementation.React's
useSyncExternalStoreWithSelectorreference implementation writesinst.hasValueandinst.valueinside auseEffect(() => { inst.hasValue = true; inst.value = value }, [value]). The current code performs these mutations inline during render (lines 187–188), which creates two risks:
- In a sync renderer with Suspense paths (as in
reconcile.ts), if a render aborts or is discarded,instretains a value that was never committed. The next render'sisEqualgate then uses this phantom "previous selection" that never reached the user.- Side-effects during render are an anti-pattern that React's reference implementation explicitly avoids.
Align with React's implementation—
useEffectis already exported from this module:Proposed fix
const value = useSyncExternalStore(subscribe, getSelection, getServerSelection) useDebugValue(value) - inst.hasValue = true - inst.value = value + useEffect(() => { + inst.hasValue = true + inst.value = value + }, [value]) return value }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/react/hooks.ts` around lines 142 - 189, The render currently mutates the render-time ref (inst.hasValue and inst.value) after calling useSyncExternalStore, which risks using a non-committed selection and causes side-effects during render; move those assignments into a useEffect so the ref is updated only after commit: remove the inline mutations inst.hasValue = true and inst.value = value and add a useEffect(() => { instRef.current!.hasValue = true; instRef.current!.value = value }, [value]) (referencing instRef/inst, value, useSyncExternalStore, getSelection/getServerSelection) so updates happen post-commit and mirror React's useSyncExternalStoreWithSelector behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d71b8cda-a2de-444c-ac52-48de22027270
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (106)
apps/web/package.jsonapps/web/vite.config.tspackages/redact/package.jsonpackages/redact/scripts/build.mjspackages/redact/src/core/index.tspackages/redact/src/core/internal.tspackages/redact/src/core/types.tspackages/redact/src/dom/_all.tspackages/redact/src/dom/client.tspackages/redact/src/dom/dispatcher.tspackages/redact/src/dom/dom.tspackages/redact/src/dom/event-replay.tspackages/redact/src/dom/features/class/full.tspackages/redact/src/dom/features/class/index.tspackages/redact/src/dom/features/class/stub.tspackages/redact/src/dom/features/context/full.tspackages/redact/src/dom/features/context/index.tspackages/redact/src/dom/features/context/stub.tspackages/redact/src/dom/features/forward-ref/full.tspackages/redact/src/dom/features/forward-ref/index.tspackages/redact/src/dom/features/forward-ref/stub.tspackages/redact/src/dom/features/hydration/full.tspackages/redact/src/dom/features/hydration/index.tspackages/redact/src/dom/features/hydration/stub.tspackages/redact/src/dom/features/index.tspackages/redact/src/dom/features/lazy/full.tspackages/redact/src/dom/features/lazy/index.tspackages/redact/src/dom/features/lazy/stub.tspackages/redact/src/dom/features/memo/full.tspackages/redact/src/dom/features/memo/index.tspackages/redact/src/dom/features/memo/stub.tspackages/redact/src/dom/features/portal/full.tspackages/redact/src/dom/features/portal/index.tspackages/redact/src/dom/features/portal/stub.tspackages/redact/src/dom/features/suspense/full.tspackages/redact/src/dom/features/suspense/index.tspackages/redact/src/dom/features/suspense/stub.tspackages/redact/src/dom/index.tspackages/redact/src/dom/portal.tspackages/redact/src/dom/reconcile.tspackages/redact/src/dom/root.tspackages/redact/src/dom/test-utils.tspackages/redact/src/react/children.tspackages/redact/src/react/class.tspackages/redact/src/react/compiler-runtime.tspackages/redact/src/react/context.tspackages/redact/src/react/element.tspackages/redact/src/react/hooks.tspackages/redact/src/react/index.tspackages/redact/src/react/jsx-runtime.tspackages/redact/src/react/memo.tspackages/redact/src/react/portal.tspackages/redact/src/react/shared-internals.tspackages/redact/src/react/suspense.tspackages/redact/src/scheduler/index.tspackages/redact/src/server/bootstrap-script.tspackages/redact/src/server/dispatcher.tspackages/redact/src/server/escape.tspackages/redact/src/server/index.tspackages/redact/src/server/renderToString.tspackages/redact/src/server/stream.tspackages/redact/src/server/walk.tspackages/redact/src/vite/index.tspackages/redact/tests/basic.test.tsxpackages/redact/tests/callback-ref-commit-phase.test.tsxpackages/redact/tests/child-reorder.test.tsxpackages/redact/tests/compiler-runtime.test.tsxpackages/redact/tests/controlled-input.test.tsxpackages/redact/tests/create-root-clear-container.test.tsxpackages/redact/tests/document-hydration.test.tsxpackages/redact/tests/external-store-unmount.test.tsxpackages/redact/tests/feature-stubs.test.tsxpackages/redact/tests/floating-ui-pattern.test.tsxpackages/redact/tests/global.d.tspackages/redact/tests/hooks.test.tsxpackages/redact/tests/hydration.test.tsxpackages/redact/tests/memo-state-rerender.test.tsxpackages/redact/tests/place-children-anchor.test.tsxpackages/redact/tests/portal-doctype.test.tsxpackages/redact/tests/public-exports.test.tspackages/redact/tests/react-integration/attributes.test.tsxpackages/redact/tests/react-integration/basic.test.tsxpackages/redact/tests/react-integration/class-context-type.test.tsxpackages/redact/tests/react-integration/context.test.tsxpackages/redact/tests/react-integration/elements.test.tsxpackages/redact/tests/react-integration/form-controls.test.tsxpackages/redact/tests/react-integration/fragment.test.tsxpackages/redact/tests/react-integration/harness.tsxpackages/redact/tests/react-integration/hooks.test.tsxpackages/redact/tests/react-integration/modes.test.tsxpackages/redact/tests/react-integration/reconnecting.test.tsxpackages/redact/tests/react-integration/select.test.tsxpackages/redact/tests/react-integration/special-types.test.tsxpackages/redact/tests/rsc-hydration-adopt.test.tsxpackages/redact/tests/rsc-proxy-child.test.tsxpackages/redact/tests/rsc-raw-lazy.test.tsxpackages/redact/tests/security.test.tsxpackages/redact/tests/ssr-context.test.tsxpackages/redact/tests/ssr.test.tsxpackages/redact/tests/streaming-hydration.test.tsxpackages/redact/tests/suspense.test.tsxpackages/redact/tests/synthetic-event-shape.test.tsxpackages/redact/tests/tsconfig.jsonpackages/redact/tests/use-effect-coalesced-renders.test.tsxpackages/redact/tests/vitest.config.tspackages/redact/tsconfig.json
| export const enum FiberTag { | ||
| Host = 0, | ||
| Text = 1, | ||
| Function = 2, | ||
| Class = 3, | ||
| Fragment = 4, | ||
| Portal = 5, | ||
| Provider = 6, | ||
| Consumer = 7, | ||
| ForwardRef = 8, | ||
| Memo = 9, | ||
| Lazy = 10, | ||
| Suspense = 11, | ||
| Root = 12, | ||
| } | ||
|
|
||
| export const enum FiberFlag { | ||
| None = 0, | ||
| Placement = 1 << 0, | ||
| Update = 1 << 1, | ||
| Deletion = 1 << 2, | ||
| Ref = 1 << 3, | ||
| Effect = 1 << 4, | ||
| LayoutEffect = 1 << 5, | ||
| ContentReset = 1 << 6, | ||
| DidCapture = 1 << 7, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect tsconfig settings relevant to const enum compatibility for the redact package.
fd -a -t f 'tsconfig*.json' packages/redact || true
fd -a -t f 'tsconfig*.json' --max-depth 2 . | head -n 20
# Print the keys we care about from any tsconfig found in the redact package.
fd -t f 'tsconfig*.json' packages/redact -x sh -c '
echo "==> {}";
cat {} | tr -d "\r"
'
# Confirm cross-module usage of FiberTag / FiberFlag (any one of these crossing files implies the const enum risk applies).
rg -nP '\b(FiberTag|FiberFlag)\.' --type=ts -C1Repository: nikilok/learn-tanstack-start
Length of output: 16396
Replace const enum declarations with regular enum or refactor to support isolatedModules.
The tsconfig.json sets "isolatedModules": true, which is incompatible with const enum when members are imported across module boundaries. FiberTag and FiberFlag are used extensively throughout the package (dom/reconcile.ts, root.ts, features/, etc.), and TypeScript cannot safely inline these values during transpilation under this configuration. Vite/esbuild will not handle the inlining correctly.
Options:
- Change
const enumto regularenum(simplest, adds small runtime overhead) - Convert to
as constobject literals (preserves const semantics without TypeScript enum behavior) - Add
"preserveConstEnums": trueto tsconfig (requires realtscbuild step, not esbuild alone) - Disable
"isolatedModules"if your build pipeline supports it
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/redact/src/core/internal.ts` around lines 3 - 29, Replace the two
TypeScript const enum declarations (FiberTag and FiberFlag) so the code is
compatible with isolatedModules: either change `export const enum FiberTag` and
`export const enum FiberFlag` to regular `export enum FiberTag` and `export enum
FiberFlag`, or convert them into `export const` object literals with `as const`
to preserve values; update any imports/usages in code (references to FiberTag
and FiberFlag in reconcile/root/features code) accordingly so consumers access
the enum/object members at runtime without relying on const enum inlining.
| const events = q.splice(0) | ||
| for (const [type, target] of events) { | ||
| try { | ||
| const ev = createEvent(type) | ||
| target.dispatchEvent(ev) | ||
| } catch {} |
There was a problem hiding this comment.
Don't redispatch buffered DOM events as native events.
The browser already applied the original event before hydration. Dispatching a second click/submit/change here can replay native side effects, e.g. toggling a checkbox twice or submitting a form again after hydration. This needs a synthetic-handler replay path, or the queue must exclude events with native defaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/redact/src/dom/event-replay.ts` around lines 25 - 30, The current
replay loop in event-replay.ts takes events from q, recreates them with
createEvent and calls target.dispatchEvent(ev), which will re-run native browser
defaults (e.g., toggling checkboxes or resubmitting forms); instead, stop
dispatching as native events and either (a) route to your synthetic-handler
replay path (invoke the component/library-level handler replay function for the
recorded [type,target] entry) or (b) filter out event types that cause native
side-effects before dispatching (e.g., "click","submit","change") and skip them;
locate the loop using q.splice(0), the createEvent call and target.dispatchEvent
and replace dispatching with a call into your synthetic replay API or an
explicit skip for native-defaulting event types.
| let frame: SSRFrame | null = null | ||
|
|
||
| export function beginSSR(identifierPrefix = ':R'): void { | ||
| frame = { idCounter: 0, identifierPrefix, contextStack: [] } |
There was a problem hiding this comment.
The module-global SSR frame is unsafe for overlapping renders.
Every server hook reads from this singleton frame. As soon as two SSR/streaming requests overlap, one request can overwrite another request's context stack and useId counter, which risks cross-request data leakage in rendered HTML. This state needs to be isolated per render instead of living in one module slot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/redact/src/server/dispatcher.ts` around lines 9 - 12, The
module-global frame variable (frame) used by beginSSR/SSRFrame is unsafe for
concurrent renders; instead create per-render state and avoid a shared module
slot: either (A) change beginSSR to return a new SSRFrame object (containing
idCounter, identifierPrefix, contextStack) and update all functions that
currently read the module-scoped frame to accept that SSRFrame as an explicit
parameter (including any useId-like helpers), or (B) use Node's
AsyncLocalStorage to store the SSRFrame per request and replace the
module-global frame reads/writes with AsyncLocalStorage.getStore()/run()
calls—ensure you remove the shared frame variable and update usages of frame,
beginSSR, SSRFrame, idCounter, identifierPrefix and contextStack accordingly so
each render has isolated state.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/redact/src/server/stream.ts (1)
13-21: 💤 Low value
progressiveChunkSizeis declared but never used and contradicts the summary.The AI summary states this option was removed from
StreamOptions, but it's still declared on line 20 and never referenced anywhere in this file. Either drop it from the type to match the intended API surface, or wire it through to actually chunk progressively.♻️ Proposed cleanup
export interface StreamOptions { identifierPrefix?: string nonce?: string bootstrapScripts?: ReadonlyArray<string | { src: string; async?: boolean; nonce?: string }> bootstrapModules?: ReadonlyArray<string | { src: string; nonce?: string }> onError?: (error: unknown) => string | void signal?: AbortSignal - progressiveChunkSize?: number }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/redact/src/server/stream.ts` around lines 13 - 21, The StreamOptions interface currently declares progressiveChunkSize but it is unused; remove the progressiveChunkSize property from the StreamOptions type to match the intended API surface (or alternatively, if progressive chunking is desired, thread this option into the stream creation/response code paths where chunks are produced—e.g., whichever function consumes StreamOptions that handles chunking). Update any related types/tests/docs that reference StreamOptions.progressiveChunkSize so the API and implementation stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/redact/src/server/stream.ts`:
- Around line 240-246: The abort handler currently only registers via
options.signal?.addEventListener and therefore ignores a signal that is already
aborted; update the logic in the stream setup (the block that touches
options.signal, state.closed, state.resolveClosed, and controller.close) to
synchronously check options.signal?.aborted before adding the listener and, if
true, immediately set state.closed = true, call state.resolveClosed(), and
attempt controller.close() (same error-safe behavior as the listener); keep the
existing addEventListener for future aborts and consider mirroring this
pre-abort check where renderToPipeableStream handles options.signal for
consistency.
- Around line 293-316: The microtask can call options.onShellReady after a
synchronous rejection because the rejection handler doesn't mark that the shell
failed; add a flag (e.g., shellErrored) and set it to true in the rejection
branch where !shellReady triggers options.onShellError (and ensure you still end
dest if needed), then update the queueMicrotask callback to return early if
aborted || finished || shellErrored before setting shellReady and calling
options.onShellReady; locate these changes around streamHtml(...).then(...) and
the queueMicrotask(...) block and update the variables shellReady/finished
checks accordingly.
---
Nitpick comments:
In `@packages/redact/src/server/stream.ts`:
- Around line 13-21: The StreamOptions interface currently declares
progressiveChunkSize but it is unused; remove the progressiveChunkSize property
from the StreamOptions type to match the intended API surface (or alternatively,
if progressive chunking is desired, thread this option into the stream
creation/response code paths where chunks are produced—e.g., whichever function
consumes StreamOptions that handles chunking). Update any related
types/tests/docs that reference StreamOptions.progressiveChunkSize so the API
and implementation stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e7afbe3-5a0f-45e0-87cf-8348c1b8bf87
📒 Files selected for processing (6)
apps/web/vite.config.tspackages/redact/src/dom/dispatcher.tspackages/redact/src/react/shared-internals.tspackages/redact/src/server/escape.tspackages/redact/src/server/stream.tspackages/redact/tests/security.test.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/redact/src/server/escape.ts
- packages/redact/src/dom/dispatcher.ts
- packages/redact/src/react/shared-internals.ts
| options.signal?.addEventListener('abort', () => { | ||
| state.closed = true | ||
| state.resolveClosed() | ||
| try { | ||
| controller.close() | ||
| } catch {} | ||
| }) |
There was a problem hiding this comment.
Already-aborted signal is silently ignored.
addEventListener('abort', …) does nothing if options.signal.aborted is already true at call time, so a consumer that passes a pre-aborted signal will see the stream proceed (and potentially hang on a never-settling <Suspense> boundary) instead of closing immediately. Worth handling the pre-aborted case before subscribing — renderToPipeableStream similarly never reads options.signal at all, which may also be worth aligning.
🔧 Suggested fix
- options.signal?.addEventListener('abort', () => {
- state.closed = true
- state.resolveClosed()
- try {
- controller.close()
- } catch {}
- })
+ const onAbort = () => {
+ state.closed = true
+ state.resolveClosed()
+ try {
+ controller.close()
+ } catch {}
+ }
+ if (options.signal?.aborted) {
+ onAbort()
+ } else {
+ options.signal?.addEventListener('abort', onAbort, { once: true })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| options.signal?.addEventListener('abort', () => { | |
| state.closed = true | |
| state.resolveClosed() | |
| try { | |
| controller.close() | |
| } catch {} | |
| }) | |
| const onAbort = () => { | |
| state.closed = true | |
| state.resolveClosed() | |
| try { | |
| controller.close() | |
| } catch {} | |
| } | |
| if (options.signal?.aborted) { | |
| onAbort() | |
| } else { | |
| options.signal?.addEventListener('abort', onAbort, { once: true }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/redact/src/server/stream.ts` around lines 240 - 246, The abort
handler currently only registers via options.signal?.addEventListener and
therefore ignores a signal that is already aborted; update the logic in the
stream setup (the block that touches options.signal, state.closed,
state.resolveClosed, and controller.close) to synchronously check
options.signal?.aborted before adding the listener and, if true, immediately set
state.closed = true, call state.resolveClosed(), and attempt controller.close()
(same error-safe behavior as the listener); keep the existing addEventListener
for future aborts and consider mirroring this pre-abort check where
renderToPipeableStream handles options.signal for consistency.
| streamHtml(children, emit, options, state).then( | ||
| () => { | ||
| finished = true | ||
| if (dest) dest.end() | ||
| options.onAllReady?.() | ||
| }, | ||
| (err) => { | ||
| if (!shellReady) { | ||
| options.onShellError?.(err) | ||
| } else { | ||
| options.onError?.(err) | ||
| if (dest) dest.end() | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| // We call onShellReady once the first synchronous emit has landed. | ||
| // streamHtml above runs the shell synchronously before the first await in drain(), | ||
| // so we can schedule onShellReady right after the first microtask. | ||
| queueMicrotask(() => { | ||
| if (aborted || finished) return | ||
| shellReady = true | ||
| options.onShellReady?.() | ||
| }) |
There was a problem hiding this comment.
onShellReady fires even after onShellError on synchronous shell failures.
When streamHtml rejects synchronously (e.g., walk throws during shell rendering), the microtask ordering ends up:
.then(_, errh)is attached at line 293 → because the returned promise is already rejected,errhis queued first → callsoptions.onShellErrorwithshellReady === false.queueMicrotask(...)at line 312 queues second → seesaborted === falseandfinished === false, setsshellReady = true, and callsoptions.onShellReady.
So consumers receive both onShellError and onShellReady for the same failed render, which violates the React-compatible contract that exactly one of those fires for the shell phase. The microtask needs to be gated on a flag mutated by the rejection path.
🔧 Suggested fix
const buffers: string[] = []
let dest: NodeJS.WritableStream | null = null
let shellReady = false
+ let shellErrored = false
let finished = false
let aborted = false
@@
// Kick off rendering
streamHtml(children, emit, options, state).then(
() => {
finished = true
if (dest) dest.end()
options.onAllReady?.()
},
(err) => {
if (!shellReady) {
+ shellErrored = true
options.onShellError?.(err)
} else {
options.onError?.(err)
if (dest) dest.end()
}
},
)
@@
queueMicrotask(() => {
- if (aborted || finished) return
+ if (aborted || finished || shellErrored) return
shellReady = true
options.onShellReady?.()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| streamHtml(children, emit, options, state).then( | |
| () => { | |
| finished = true | |
| if (dest) dest.end() | |
| options.onAllReady?.() | |
| }, | |
| (err) => { | |
| if (!shellReady) { | |
| options.onShellError?.(err) | |
| } else { | |
| options.onError?.(err) | |
| if (dest) dest.end() | |
| } | |
| }, | |
| ) | |
| // We call onShellReady once the first synchronous emit has landed. | |
| // streamHtml above runs the shell synchronously before the first await in drain(), | |
| // so we can schedule onShellReady right after the first microtask. | |
| queueMicrotask(() => { | |
| if (aborted || finished) return | |
| shellReady = true | |
| options.onShellReady?.() | |
| }) | |
| streamHtml(children, emit, options, state).then( | |
| () => { | |
| finished = true | |
| if (dest) dest.end() | |
| options.onAllReady?.() | |
| }, | |
| (err) => { | |
| if (!shellReady) { | |
| shellErrored = true | |
| options.onShellError?.(err) | |
| } else { | |
| options.onError?.(err) | |
| if (dest) dest.end() | |
| } | |
| }, | |
| ) | |
| // We call onShellReady once the first synchronous emit has landed. | |
| // streamHtml above runs the shell synchronously before the first await in drain(), | |
| // so we can schedule onShellReady right after the first microtask. | |
| queueMicrotask(() => { | |
| if (aborted || finished || shellErrored) return | |
| shellReady = true | |
| options.onShellReady?.() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/redact/src/server/stream.ts` around lines 293 - 316, The microtask
can call options.onShellReady after a synchronous rejection because the
rejection handler doesn't mark that the shell failed; add a flag (e.g.,
shellErrored) and set it to true in the rejection branch where !shellReady
triggers options.onShellError (and ensure you still end dest if needed), then
update the queueMicrotask callback to return early if aborted || finished ||
shellErrored before setting shellReady and calling options.onShellReady; locate
these changes around streamHtml(...).then(...) and the queueMicrotask(...) block
and update the variables shellReady/finished checks accordingly.
1 participant
First-paint JS — preview (
@ss/redact) vs production (vanilla React)Preview —
@ss/redactindex-MmSRWcLX.jsroutes-Cxa-tpbo.jslink-ClPyco1r.jscanonical-CZVYjyxZ.jsProduction — vanilla React (
sponsorsearch.co.uk)index-CvS0zZmT.jsindex-Bcwa-P1L.jsDeltas
Summary by CodeRabbit
New Features
Chores