Repository navigation
round 4: 54 audited fixes across router, cache, RSC, build/vite, browser, theme, server, and testing primitives - #611
Merged
Merged
Conversation
A1 [bug] trie-matching: reject empty required-param segment (/a//b no longer matches /a/:s/b binding s=""); matches the regex ([^/]+) contract + parity row. A2 [perf] pattern-matching: extractStaticPrefix computes the cut per-segment so a literal colon in a static segment (/a:b/c/:id) no longer drops the fast-skip. A3 [perf] middleware: matchMiddleware runs the scope regex once (single match()) instead of test() + extractParams' second match() per matching entry. A4 [api] dsl-helpers: route(name,...) now calls validateUserRouteName like path()/include(), so a reserved name ($prefix_/$path_) errors instead of silently vanishing from reverse()/types. A5 [bug] router: prefetchCacheTTL NaN/Infinity guarded with Number.isFinite (-> 300s default) instead of emitting Cache-Control: max-age=NaN. Extracted to prefetch-cache-ttl.ts for unit coverage. A6 [bug] document the regex-fallback wildcard trailing-slash splat divergence vs the canonical trie with a parity row so it cannot silently widen (trie is canonical; matcher behavior unchanged).
B1 [error-handling] DSL loader errors now reach the consumer onError callback and
loader.error telemetry: buildLoaderErrorContext threads errorContext into all
wrapLoaderPromise call sites (fresh/revalidation/intercept), matching fetchable
loaders.
B2 [perf] resolveLoaderData short-circuits the per-loaderId dedup BEFORE running
the user tags()/TTL/SWR/recordRequestTags work, so a deduped second resolution
no longer re-runs tags().
B3 [memory] segment-loader-promise caps the per-key LoaderCacheEntry array
(evict-oldest) so it stops growing unbounded across navigations.
B4 [bug] intercept segments are cached once on a fresh intercept navigation
(dedup state.interceptSegments against allSegments ids), not twice.
B5 [bug] createUseFunction loaders (action/dispatch path) now receive the parsed
typed ctx.search instead of always {}.
B6 [bug] Max-Age cookie parsing treats only a numeric <= 0 as deletion, so
Max-Age=05 (zero-prefixed non-zero) is no longer misread as a deletion.
…xes)
C1 [bug] "use cache" key for a FormData arg is now derived from sorted entries
(boundary-independent), not the random multipart boundary, so identical args
produce the same key and actually hit the cache (was: always missed).
C2 [error-handling] a consumer cache({ key }) fn that throws now degrades to a
cache miss (render continues) instead of crashing the page render.
C3 [bug] the document cache no longer stores/serves an unqualified
Cache-Control: no-cache response as a fresh HIT (RFC 7234); field-qualified
no-cache="set-cookie" is still storable.
C4 [error-handling] resolveCacheProfiles now rejects non-finite/negative ttl/swr
at createRouter time instead of creating a permanently-fresh, never-evicting
entry.
C5 [bug] normalizeTag trims surrounding whitespace so " products " and
"products" are one cache tag, not two.
C6 [perf] bufferToBase64 builds the latin1 string in 8KB chunks instead of one
byte at a time (byte-identical output), cheaper for large document payloads.
D1 [error-handling] a malformed JS-path action body now yields a controlled 400 (error boundary or plain 400) instead of an opaque host 500, converging with the no-JS PE path; the decode error is sanitized (no leak). D2 [bug] the initial full-render payload carries warmupEnabled, so warmup:false is respected from first load (was ignored until a navigation). D3 [bug] a loader error boundary that renders to a falsy node (0/""/false) is used instead of being discarded and the original error rethrown (== null sentinel, not falsy). D4 [bug] a thrown Response (redirect/notFound) from a no-middleware fetchable loader now produces the real 302/404 instead of a generic 500. D5 [security] the production fetchable-loader error payload no longer leaks the consumer error class name (dev unchanged). D6 [bug] negotiated response routes and RSC renders emit Vary with Accept once (appendVaryAccept dedup helper) instead of a duplicated token. D7 [perf] early SSR setup is only kicked off when the request will render HTML, not for no-Accept requests that return RSC (avoids wasted work + an orphan rejection window); mayNeedSSR / document-cache keying untouched.
…ts (8 fixes)
E1 [bug] source-scan classifier is now regex-literal-aware (a /.../ literal
containing a quote or // no longer desyncs the code/string/comment state and
drops a real router file from discovery).
E2 [bug] include-resolution resolves an include() target imported via a combined
default+named import (import Foo, { bar }) instead of false-hard-erroring.
E3 [bug] createRouter $$id idempotency guard checks for the actual injected
$$routeNames marker, not a substring, so args containing the literal "$$id"
no longer drop named-route wiring.
E4 [bug] aliased loader exports get the same single id (exportNames[0]) on the
client stub as the server registers, fixing client/server $$id divergence.
E5 [bug] transformHandles branches on binding.argCount (AST-derived) instead of a
string-trim heuristic, so a comment-prefixed createHandle arg transforms
correctly.
E6 [bug] the Node preset's rsc environment gets the same optimizeDeps.exclude the
cloudflare preset and ssr env set.
E7 [perf] prerender runWithConcurrency stops scheduling new work on the first
error instead of letting remaining workers finish before the build aborts.
E8 [bug] the client-component-hmr 'use client' sniff is AST-based and tolerates a
leading comment before the directive (hasUseClientDirective).
…nk (7 fixes) F1 [bug] useLinkStatus releases the pinned useOptimistic value when navigation returns to idle, so a Link spinner no longer stays stuck after a view-transition navigation. F2 [concurrency] the partial-update full-update fallback re-checks the history key after the stream completes (like the partial branch), so a stale stale-revalidation full update can't clobber a page the user navigated to. F3 [test-gap] adds unit coverage for ScrollRestoration (LRU eviction, MRU re-order, sessionStorage quota evict-and-retry ladder, custom getKey). F4 [memory] a direct (hover) prefetch with no caller AbortSignal now installs an internal abort+timeout so a never-settling fetch can't strand the inflight key and permanently block re-prefetch of that URL. F5 [bug] Link evaluates touch capability per render (resolveAdaptiveStrategy) instead of once at module load, so prefetch="adaptive" reflects current input capability (hybrid device, SSR->hydrate). F6 [type-safety] LocationState.delete() guards that history.state is an object before `key in current`, so a non-null primitive state is a no-op instead of throwing. F7 [test-gap] adds the missing useSearchParams SSR-empty-seed + mount-catch-up unit test (mirrors usePathname); no source divergence found.
…s (5 fixes) G1 [bug] resolveThemeConfig coerces a "system" defaultTheme to a concrete theme when enableSystem is false, so <html> never gets a bogus class="system". G2 [bug] client setTheme validates against the configured theme set (warn+ignore an unknown theme) like server ctx.setTheme, preventing SSR drift. G3 [bug] collectMeta handles Promise meta descriptors explicitly and dev-warns when one is pushed under an active title template (async meta is append-only and documented; collect is synchronous so it cannot participate in dedup). G4 [bug] breadcrumbs href-dedup keeps each href's first position but last value, so re-pushing a parent crumb refreshes its label in place instead of reordering it to the end (parent->child order preserved). G5 [api] the FOUC theme script is idempotent (a per-storageKey window guard), so rendering both MetaTags (auto-inject) and ThemeScript no longer double-runs the script or leaks a matchMedia listener; JSDoc steers consumers to pick one.
…sc (5 fixes) H1 [error-handling] a loader module-load (dynamic import) failure now surfaces as a 500 + onError instead of being swallowed into a misleading 404; an unregistered loader still 404s. H2 [error-handling] waitUntil defers the callback (Promise.resolve().then) so a synchronously-throwing callback is fire-and-forget (logged) instead of propagating into the request flow, on both the EC and no-EC seats. H3 [bug] parseSearchParams number coercion is strict-decimal: ""/whitespace/ Infinity/0x10 are treated as missing (omitted, so schema defaults apply) instead of becoming 0/Infinity/16; valid decimals and 1e3 still parse. H4 [api] the RSC client entry (client.rsc.tsx) re-exports the full "use client" hook set the default entry exports (useParams/useMount/useSegments/ useSearchParams/...), matching the surface; navigation/action stay omitted by design. H5 [dead-code] removed the deprecated dead extractUrlsVariableFromRouter (function + re-export), per the pre-stable no-deprecated-API rule; its self-test now exercises the canonical extractUrlsFromRouter.
I1 [api] a seeded ctx.use(Loader) now returns a Promise (Promise.resolve) like production and the real-fn delegate path, instead of the raw value. I2 [test-gap] dispatch wires the production response-route cache path (getResponse / SWR / putResponse / resolveCacheTags), so cached path.json/path.text routes are reachable + cache via the primitive; cache-scope is lazily imported so the testing barrel doesn't eagerly pull plugin-rsc. I3 [test-gap] runInRequestContext (and runLoader) drain ctx.onResponse() callbacks on both the normal and throw-redirect branches, matching production. I4 [test-gap] renderToFlightString accepts theme/cacheStore/cacheProfiles and threads them into the RequestContext, so a server component reading ctx.theme or invoking use-cache works under the primitive instead of crashing.
Adds dev+prod e2e (guard-safe (production) siblings, each run locally) for the HTTP/browser-observable round-4 fixes: - A1 empty path segment /item//detail does not bind :itemId="" (cloudflare-basic trie-routing). - C3 an unqualified Cache-Control: no-cache response is never served as a fresh document-cache HIT (cloudflare-basic blog-cache + a fixture page). - D4 a fetchable loader throwing a redirect Response yields the real 302, not 500 (router e2e, test-app ThrownRedirectLoader). - D5 in production the fetchable-loader error body does not leak err.name; dev does (router e2e, test-app NamedErrorLoader). - D6 a negotiated response/RSC route emits Vary: Accept exactly once (router e2e content-negotiation). D1 (malformed-action 400) and G1 (theme system+enableSystem:false) are covered by unit tests but deferred at the e2e layer: D1's 400 path isn't reliably reachable over HTTP (React decodeReply is lazy, so a malformed body surfaces on the action -runtime 500 path); G1's theme config is router-global and an e2e would break the existing theme suite or require a second app. cloudflare-basic router.named-routes.gen.ts regenerated for the one new route.
A multi-agent review (62 raw -> 33 confirmed) of the 54-fix branch surfaced real issues in the new code; all verified-to-fix items applied: Bugs (red-before-green): - cache-runtime: the C1 Blob/File cache-key token percent-encodes type/name so a user-controlled colon in a MIME type or filename can't collide two files onto one key. - expose-id-utils countArgs: a comment-only createHandle/createLoader arg list is counted as 0 args at the producer (fixes E5 on the regex-fallback path, not just the AST path). - location-state write(): coerces a primitive/null history.state to a fresh dict like delete() (the F6 guard was missing from the sibling writer). - setTheme rejects "system" when enableSystem is false on BOTH client and server (shared isValidTheme/warnInvalidTheme helper), closing the gap the G1 coercion left in the setter. - intercept cache-hit loader revalidation reports onError isPartial:true (matches the revalidation path). - loader-fetch: notFound() (a DataNotFoundError) maps to 404, not a generic 500. - meta async-descriptor dev-warning no longer false-positives on a normal async descriptor (only the title-template collision case). - prefetch stalled-fetch timeout is layered even when the caller passes a signal (queue-driven prefetches), so a stalled server can't strand the inflight key. Simplify / reuse (no behavior change): - shared leading-directive sniff (rango.ts + version-plugin), shared isThenable predicate (meta/MetaTags/breadcrumbs), shared loader-error response builder, shared response-cache-serve leaf used by both production and the dispatch primitive (plugin-rsc-free via injected deps), drainOnResponseCallbacks reused from rsc/helpers instead of inlined, de-duplicated TTL-guard comments. Coverage / docs: - binary-arg use-cache HIT test, warmupEnabled-via-router.fetch primitive test, a real Max-Age=00 deletion assertion, corrected scroll-restoration JSDoc.
commit: |
P1 [security] response-cache-serve: a configured cache({ key }) or store
keyGenerator that THROWS now degrades to a cache miss (runs uncached, no
read/write) instead of falling back to the broad response:type:host+path key
and caching under it — which could serve one user's personalized output to
another when the key encodes tenant/user/auth state. The no-key default path
is unchanged.
P2a cache-runtime: replyToCacheKey type-tags string FormData entries ("s:") like
it already tags Blob/File entries ("b:..."), so a user string equal to a blob
token can no longer collide two distinct arg lists onto one "use cache" key.
P2b cache-scope + handle-snapshot: segment-codec (which pulls @vitejs/plugin-rsc)
is now imported lazily at its async use sites, so cache-scope is plugin-rsc-free
at module load. The dispatch() testing primitive's cached-response-route path is
now usable in a plain Node/Vitest consumer test without mocking plugin-rsc
(covered by a new no-mock dispatch test); segment-caching behavior is unchanged.
P3 theme: the inline FOUC boot script and the cross-tab storage event handler now
apply the same validity rule as setTheme — a stored/received "system" with
enableSystem:false falls back to defaultTheme instead of leaving a bogus
class="system"/colorScheme="system" on <html>.
…he write The response-cache-serve extraction (review dedup #10/#13) deferred the cache clone into the waitUntil callback (putFresh(response) cloned inside the async body). On a cache MISS the same response is also returned to the middleware chain, where mergeResponse rebuilds it as `new Response(response.body, ...)`. The background clone() and that foreground body read could interleave and throw "Response body object should not be disturbed or locked" — a flaky 500 on cached response routes (cache-isolation e2e). In dev this surfaced as a Vite dev-server "Internal server error" that crashed/restarted the server under Playwright retries, cascading into the dev e2e shards being killed. Fix: clone synchronously at the miss site (before returning), so the returned body is teed independently of the cache write. The SWR path is unaffected (its fresh response is created inside the background callback and never returned). Verified: cache-isolation onResponse-freshness 3x reliably green in dev (was flaky 500); 67 cache-isolation + cache dev e2e green at retries=0; cache/dispatch unit suites 706 green; typecheck 13/13.
The rangojs-router dev e2e runs in dev mode (a live Vite dev server + esbuild +
Chromium held resident across the shard) over ~300 tests. At 3-way sharding the
two heaviest shards exhausted the GitHub-hosted runner's memory and the runner
was terminated mid-run ("received a shutdown signal" at 5-8 min, well under the
20-minute job cap) — surfacing as flaky dev-shard failures while the same shard
passes locally (more RAM). Splitting dev across 5 smaller shards keeps each
within the runner's memory budget and shortens each shard. Production sharding
(2) and the HMR jobs are unchanged.
Complements the 3->5 dev sharding. The smaller shards removed the runner-OOM, but the shard that draws the timing-sensitive dev tests (handle-breadcrumbs late-push + cache param-change soft-navs) spends real wall-clock on per-test timeouts and their Playwright retries under CI load, which brushed the old 20-minute cap. 30 minutes gives that headroom; production/HMR jobs are unchanged.
`playwright install-deps chromium` shells out to apt-get to install browser system libraries, which can hang on a slow/transient package mirror and stall an e2e job. Wrap all 8 install-deps steps so each attempt is bounded to 60s (coreutils `timeout`, available on ubuntu-latest) and retried once on failure or timeout (`timeout 60 ... || timeout 60 ...`), so a flaky mirror costs at most ~2 minutes and self-heals instead of hanging to the job cap.
…evious char The E1 regex-literal skip decided "regex vs division" from the previous significant CHAR only. After a keyword like `return` (which ends in a word char) it read `/` as division, so a regex body's inner quotes opened a phantom string and swallowed a later real createRouter() -> the router file was dropped from discovery / HMR / typegen. Example: `return /it's a "x"/g; createRouter()`. Now, when the previous char ends an identifier, the scanner walks back over any whitespace + the identifier and treats `/` as a regex iff that identifier is a regex-preceding keyword (return/typeof/instanceof/in/of/new/delete/void/do/else/ yield/await/case/throw). A value-named identifier (`returnValue / b`) stays division. Red-before-green: the `return`/`typeof`/`case`/`throw` cases fail without the keyword lookback and pass with it; division-after-a-value is unaffected.
The 60s cap was too aggressive: `playwright install-deps chromium` runs apt-get, which legitimately takes ~1-2 min on some runners, so a 60s timeout killed it mid-install (both the attempt and the retry) and failed 3 e2e shards on a clean run. 180s per attempt still catches a genuine hang (a stuck mirror would far exceed it) while allowing a normal/slow-but-real install; the single retry is kept.
…y by identity Two reviewer findings on the hover/queued prefetch path: P2 (body stall) — the stall timeout only covered "no response ever arrives". Once headers arrived the timer was cleared in `.finally()`, so a body that stalled after headers left a published entry whose payload/streamComplete never settle: future prefetches dedupe against it and navigation awaits it forever. The timer now stays armed past headers; it is cleared when the stream completes (or in `.finally()` only for paths that publish no streaming entry), and on fire it aborts the fetch/stream and evicts the published entry. P3 (identity-guarded eviction) — evicting by generation alone could delete a fresh entry republished under the same key in the same generation (stalled entry consumed, then a new prefetch publishes, then the stale timer fires). removePrefetch now takes the published entry and deletes only when cache.get(key)?.entry === entry, so it can only ever drop its own stalled entry. Regression tests (red-before-green): a response whose body never closes is evicted after the timeout and re-prefetchable; and a stalled entry's timer firing does NOT drop a fresh entry republished under the same key after the stalled one was consumed.
The `timeout 180 ... || timeout 180 ...` wrapper was net-harmful and failed ~4 e2e jobs per run: - `playwright install-deps` (apt-get downloading + installing ~21MB of browser system libs) legitimately exceeds 180s on some runners, so `timeout` killed it mid-install. - `timeout` only kills the pnpm wrapper; the sudo'd apt-get grandchild keeps running and holds /var/lib/dpkg/lock-frontend, so the retry fails with "E: Could not get lock ... held by process N (apt-get)". The job-level 20-minute timeout already bounds a genuinely hung install, so the per-step timeout/retry added failure modes (killed-but-working apt, lock contention on retry) without real benefit. Restore the plain install-deps step.
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
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.
A fourth
improve-skill sweep. A 28-lane adversarial audit (80 raw → 57 confirmed, 0 already-fixed, 23 refuted) of the modules rounds 1–3 had touched only lightly, implemented as 54 fixes in one PR, one commit per batch, each tested red-before-green. The whole branch then went through a multi-agent/simplify+/code-review(62 raw → 33 confirmed → 24 fixes folded in, last commit) — which caught real bugs in the new code (a cache-key collision, an incomplete transform fix, a missing sibling guard) before it landed.All gates green:
pnpm run typecheck(13/13 packages),pnpm run test:unit:all(266 router test files + every consumer dogfood suite + RSC/Flight),pnpm run lint,pnpm run format,pnpm check:e2e-bucketing. New dev+prod e2e for 5 HTTP-observable behaviors.The two HIGH-severity ones (consumer-visible)
"use cache" never hit on a binary/FormData arg. The cache key for a
"use cache"function called with aFormData/Blobarg was derived from the response body of anencodeReply, which embeds a random multipart boundary — so every call computed a different key and always missed.DSL loader errors never reached
onError. A throwing DSL loader's error never fired the consumer'screateRouter({ onError })callback orloader.errortelemetry (the error context was dropped at every wrap site), unlike fetchable loaders.What's in it (54 fixes, by area)
""for/a//b(404 like the regex);extractStaticPrefixkeeps the fast-skip when a static segment has a literal colon;matchMiddlewareruns the scope regex once;route(name)validates the name likepath()/include();prefetchCacheTTL: NaNno longer emitsmax-age=NaNonErrorcontext (HIGH); dedup-before-tags(); boundedLoaderCacheEntry; intercept segment cached once; typedctx.searchfor action/dispatch loaders;Max-Age=05not misread as deletioncache({ key })degrades to a miss instead of crashing the render;Cache-Control: no-cachenot served as a fresh HIT (RFC 7234); profilettl/swrvalidated;normalizeTagtrims; chunked base64warmupEnabledin the initial payload; falsy error-boundary node used not discarded; thrown redirect from a no-middleware fetchable loader → 302; prod loader error no longer leakserr.name;Vary: Acceptemitted once; SSR setup only when HTML is renderedinclude()of a default+named import resolves;$$ididempotency keyed on the real marker; aliased loader exports share one id; comment-onlycreateHandleargs; Node-presetoptimizeDeps.exclude; prerender fail-fast; AST-baseduse clientsniffuseLinkStatusreleases the pinned spinner on idle;partial-updatefull-update history-key staleness guard; ScrollRestoration coverage; a stalled prefetch can't strand the inflight key;Linkadaptive prefetch reads touch capability per render;LocationStateprimitive-state guardclass="system"whenenableSystem:false; clientsetThemevalidates like the server; Promise meta descriptors documented append-only; breadcrumbs dedup preserves order; idempotent FOUC script (no double-run /matchMedialeak)onErrornot a silent 404;waitUntilsync-throw is fire-and-forget; strict-decimal search coercion (?page=no longer becomes0); the RSC client entry exports the full hook set; removed a deprecated dead exportctx.use(Loader)returns a Promise like production;dispatchexercises the response-route cache path;runInRequestContextdrainsonResponse;renderToFlightStringacceptstheme/cacheStore/cacheProfilesBehavior / semantics call-outs
cache({ key })throwsCache-Control: no-cachedocno-cache="…"still stores)throw redirect()err.name"Error"(dev unchanged)?page=(empty) parsed as number0Varyon negotiated routesAccept, AcceptAcceptonceTest plan
pnpm run typecheck(13/13) ·pnpm run test:unit:all(266 router files + consumer suites) ·pnpm run lint·pnpm run format·pnpm check:e2e-bucketing— all green.no-cachenot-fresh (C3), loader thrown-redirect 302 (D4), prod error no name-leak (D5), singleVary: Accept(D6); each run 2–3× locally.router.named-routes.gen.tsregenerated for the one new route./simplify+/code-reviewacross the full diff; 24 verified findings fixed in the final commit (incl. a cache-key collision, an incompletecountArgs, a missingwrite()guard, asetTheme"system" gap).Notes / deferred
RequestContext<TEnv>vs a consumer's concreteEnvin the new dispatch cache path) was caught by the full turbo typecheck and fixed — the per-packagetscmissed it.decodeReplyis lazy) and G1 (theme config is router-global; an e2e would break the existing theme suite or need a second app).Post-review additions (review-driven, after the original body above)
Beyond the 54 audited fixes, this PR folded in everything from the multi-round review:
waitUntil, racingmergeResponse(flaky 500 on cached response routes). Now clones synchronously before returning.removePrefetchdeletes only whencache.get(key)?.entry === entry, so a stale stall-timer can't drop a fresh entry republished under the same key.return /re/etc.), so a regex body's inner quotes can't hide a latercreateRouter()and drop a router file.install-depsper-step timeout/retry was reverted (it killed a slow-but-working apt and the orphaned sudo apt-get held the dpkg lock, failing the retry; the 20-min job timeout already bounds a true hang).Each fix ships a red-before-green regression test. Final CI: 27/27 green.