feat(skip): add layout safety observation foundations#1672
feat(skip): add layout safety observation foundations#1672NathanDrake2406 wants to merge 7 commits into
Conversation
commit: |
Introduce the per-layout observation primitives that later slices need to decide whether retained static layouts can be reused. This slice adds the machinery only; nothing reads the observations yet, so behavior is unchanged. - app-layout-param-observation: per-layout completeness, param keys, structural scope, finite revalidate, request API, cacheLife, cache tag, cacheable fetch, dynamic fetch, and unstable_cache observation tracker. - app-page-probe: bounded React server subtree probe that walks layout-returned children including memo, forwardRef, and React.lazy wrappers, enforces depth and node limits, and refuses single-use iterable children. - shims: thenable params observer for sync, await, destructure, and enumeration. Cache, fetch, and unified request context record unstable_cache, cacheable fetch, and request-scoped observation state. - app-page-params: structural segment-param scope so empty optional catch-all targets are not treated as param-free downstream. No skip transport behavior. Later slices wire the planner, manifest, and encoder onto these observations.
NathanDrake2406
force-pushed
the
nathan/726-skip-01-observation
branch
from
26ce54b to
8f61b49
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: #726-SKIP observation foundations
Clean slice — the observation surface is well-isolated, the test coverage is good, and the PR description is clear about what's in scope and what isn't. A few things worth addressing before merge:
Issues
-
currentFetchSoftTagsnot isolated in probe scope —runWithIsolatedProbeDependenciesresetscurrentRequestTags,cacheableFetchUrls,dynamicFetchUrls, etc. but does not resetcurrentFetchSoftTags. Soft tags are path-derived implicit tags set viasetPathTags()for fetch cache reads. If a parent scope has soft tags set, they leak into the probe and affect cacheable fetch behavior insiderecordProbeDependencies. Compare withrunWithFetchCachewhich does resetcurrentFetchSoftTags. This is probably harmless today since the probe doesn't actually run real fetches, but it's an inconsistency that could bite when real fetch integration lands. -
stringifyCacheLifeSnapshotdoes unnecessary work — The function serializes to JSON just to check non-nullness. SincerecordProbeDependenciesonly needs a boolean (cacheLifeObserved), the JSON.stringify is wasted._peekRequestScopedCacheLife() !== nullwould suffice. -
isPromiseLikeis now duplicated in 3 files —app-layout-param-observation.ts,app-page-probe.ts, andapp-page-execution.tsall have identical copies. Worth extracting to a shared utility. Not blocking for this PR, but worth tracking.
Observation
runLayoutProbeerror path doesn't mark complete — On rejection,recordProbeDependenciesruns butmarkProbeCompletedoes not. This means the observation stayscompleteness: "unknown"on error. The JSDoc onprobeReactServerSubtreesays "probe failures only make static-layout skip fall back to render-and-send", which is consistent. Just confirming this is intentional — a comment in the rejection handler would help future readers understand this is deliberate.
| ctx.renderRequestApiUsage = new Set(); | ||
| ctx.requestScopedCacheLife = null; | ||
| ctx.unstableCacheObservations = new Map(); | ||
| }, probe); |
There was a problem hiding this comment.
currentFetchSoftTags is not reset here, unlike runWithFetchCache which resets it alongside currentRequestTags and dynamicFetchUrls. This means soft tags from an outer scope leak into the isolated probe. Probably harmless now since probes don't issue real fetches, but it's an inconsistency with the other reset sites.
| }, probe); | |
| return runWithUnifiedStateMutation((ctx) => { | |
| ctx.cacheableFetchUrls = new Set(); | |
| ctx.currentFetchSoftTags = []; | |
| ctx.currentRequestTags = []; | |
| ctx.dynamicFetchUrls = new Set(); | |
| ctx.dynamicUsageDetected = false; | |
| ctx.renderRequestApiUsage = new Set(); | |
| ctx.requestScopedCacheLife = null; | |
| ctx.unstableCacheObservations = new Map(); | |
| }, probe); |
| const stringifyCacheLifeSnapshot = (): string | null => { | ||
| const cacheLife = _peekRequestScopedCacheLife(); | ||
| return cacheLife === null ? null : JSON.stringify(cacheLife); |
There was a problem hiding this comment.
This serializes to JSON just to check for non-null. Since recordProbeDependencies only sets observation.cacheLifeObserved = true, the JSON round-trip is wasted work.
| const stringifyCacheLifeSnapshot = (): string | null => { | |
| const cacheLife = _peekRequestScopedCacheLife(); | |
| return cacheLife === null ? null : JSON.stringify(cacheLife); | |
| const hasCacheLifeSnapshot = (): boolean => { | |
| return _peekRequestScopedCacheLife() !== null; | |
| }; |
Then update the call site at line 139 to if (hasCacheLifeSnapshot()) {.
| }, | ||
| (error: unknown) => { | ||
| recordProbeDependencies(layoutId); | ||
| throw error; |
There was a problem hiding this comment.
Intentional that markProbeComplete is skipped on error (so the observation stays completeness: "unknown" and the planner falls back to render-and-send). A brief comment here would help future readers understand this is deliberate rather than a missed cleanup.
| throw error; | |
| (error: unknown) => { | |
| // Intentionally do NOT markProbeComplete — an incomplete probe | |
| // keeps completeness: "unknown" so the planner falls back to | |
| // full render-and-send instead of reusing a stale layout. | |
| recordProbeDependencies(layoutId); | |
| throw error; | |
| }, |
| : revalidateSeconds === false | ||
| ? false | ||
| : null, | ||
| tagCount: tags.length, |
There was a problem hiding this comment.
Nit: recordUnstableCacheObservation runs on every unstable_cache call, including cache hits. The observation is keyed by cacheKey (which includes args), so two calls with different args create separate observations even for the same wrapped function. Is this the intended granularity? The test only covers same-args hit/miss, not different-args calls to the same cached function.
Not blocking — just want to confirm the dedup-by-key-hash-including-args is deliberate for the planner's purposes.
| if (isPromiseContinuation(prop)) { | ||
| const value = Reflect.get(target, prop, receiver); | ||
| if (typeof value !== "function") return value; | ||
| return (...args: unknown[]) => { | ||
| observeAllParamKeys(observer, plain); | ||
| return Reflect.apply(value, target, args); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The then/catch/finally interception wraps the method to call observeAllParamKeys before forwarding. This means chaining .then().then() on the same thenable records an all-keys observation on the first .then() call. The second .then() is on a plain Promise (not the Proxy), so it doesn't observe again. This is fine, but worth noting that observation happens eagerly at chain-construction time, not at resolution time.
| throw new AppPageSubtreeProbeLimitError("App page layout subtree probe exceeded max depth"); | ||
| } | ||
|
|
||
| if (isUnknownFunction(type)) { |
There was a problem hiding this comment.
When type is a function (server component), this calls type(props) to probe it. If the component throws synchronously (not a Promise rejection), the error propagates up through visit → probeReactServerSubtree and becomes a rejection of the outer promise. This is fine for the current use case (probe failures fall back gracefully), but be aware that a component with side effects that throws will have already executed those side effects before the error is caught.
| if (value == null || typeof value === "boolean" || typeof value === "number") return; | ||
| if (typeof value === "string" || typeof value === "bigint") return; | ||
| if (isPromiseLike(value)) { | ||
| await visit(await value, depth); |
There was a problem hiding this comment.
Awaiting a promise at the same depth (not depth + 1) means a chain of nested promises doesn't increment the depth counter. Only the resolved value's structural nesting counts. This seems intentional — async server components that return promises shouldn't eat up depth budget — but it's a subtle choice worth a comment.
|
Review posted. Here's a summary of the feedback: Two actionable suggestions:
One documentation suggestion: Two informational notes: |
- Add currentFetchSoftTags reset in runWithIsolatedProbeDependencies to prevent soft tag leak from parent scope (matches runWithFetchCache isolation) - Replace unnecessary JSON.stringify in cacheLife check with direct null comparison - Add comment documenting intentional omission of markProbeComplete on probe errors (keeps completeness as unknown, safe fallback to render-and-send)
…rst await The patched fetch deferred observation and tag collection past await buildFetchCacheKey(). If a layout probe started a cacheable fetch and returned synchronously without awaiting it, the probe snapshot would see cacheableFetchCount: 0 -- a false proof that could let the skip planner reuse a stale retained layout. Move observation and tag recording before the first await in the cacheable branch. If key generation later fails and the fetch falls back to dynamic, recording both is conservative (false unsafe costs performance, not correctness). Add an integration test that exercises the real patched fetch shim without mocks to verify synchronous observation capture.
Do not restore globalThis.fetch in test cleanup. ensureFetchPatch sets a global _PATCH_KEY guard; restoring fetch without clearing that marker makes later calls skip reinstallation silently. Catch the background fetch promise inside the probe so the patched fetch continuation (cache lookup, network fetch) cannot produce an unhandled rejection after the synchronous assertions complete.
|
/bigbonk review |
There was a problem hiding this comment.
Review: #726-SKIP observation foundations (slice 1/4)
The previous review's three actionable suggestions (soft-tag reset, JSON.stringify removal, error-path comment) have all been addressed. The code is clean and well-structured.
I've gone through all 12 changed files. The observation surface is genuinely isolated — nothing reads the observations yet, and the isAppLayoutObservationUnsafeForStaticReuse function is a clear readability win for future planner consumers. A few notes below, none blocking.
Notes (non-blocking)
-
fetch-cache tag recording was moved before
await buildFetchCacheKey()— this is a subtle but correct behavioral change. Tags are now recorded synchronously so probes can snapshot them before the fetch settles. Thecatchfallback path at line 939 still recordsrecordDynamicFetchObservation, which means a fetch that fails key generation gets recorded as both cacheable and dynamic. The comment at lines 907-913 explicitly acknowledges this as conservative (false-unsafe costs performance, not correctness). Good. -
probeReactServerSubtreesequential traversal — thevisitfunction awaits each child serially (for...of+await). This is deliberate given the probe's purpose (observing dependencies in deterministic order for snapshot stability), but worth noting that deeply nested trees with many async server components will be probe-latency-bound. Not a problem at the default limits (32 depth, 1000 nodes). -
resolveReactLazyTypecorrectly handles the React lazy throw-promise pattern —_init(payload)throws a promise on first call, then returns the resolved module on second call after the promise settles. The two-call pattern matches React's internallazyInitializerbehavior. -
isIterablecheck excludes strings explicitly — good, since strings are iterable but should be treated as leaf values in the React tree, not iterated character-by-character.
| if (value == null || typeof value === "boolean" || typeof value === "number") return; | ||
| if (typeof value === "string" || typeof value === "bigint") return; | ||
| if (isPromiseLike(value)) { | ||
| await visit(await value, depth); |
There was a problem hiding this comment.
Informational: awaiting a promise at the same depth (not depth + 1) means a chain of nested promises doesn't consume depth budget — only the resolved value's structural nesting counts. This is the right call for async server components (their async-ness shouldn't eat depth budget), but a short comment would help future readers understand the choice.
| } | ||
|
|
||
| if (isUnknownFunction(type)) { | ||
| await visit(type(props), depth + 1); |
There was a problem hiding this comment.
Nit: when type is a function (server component), type(props) executes it synchronously. If it throws synchronously (not a Promise rejection), the error propagates through visit → probeReactServerSubtree and becomes a rejection of the outer promise. This is fine — probe failures fall back gracefully — but a component with side effects that throws will have already executed those side effects. Just confirming this is acceptable since the JSDoc says "probe failures only make static-layout skip fall back to render-and-send".
| }; | ||
| } | ||
|
|
||
| if (typeof prop === "string" && !isWellKnownProperty(prop)) { |
There was a problem hiding this comment.
Nit: the observation fires for any non-well-known string property access, even for properties that don't exist in plain. For example, params.nonExistent would observe ["nonExistent"] even though it returns undefined. This is actually correct for the skip planner's purposes (the layout tried to read a param, so it depends on the param set), but it's a subtlety worth being aware of.
|
Review posted and approved. Here's a summary: Verdict: Approved All three actionable suggestions from the previous review have been addressed (soft-tag reset, JSON.stringify removal, error-path comment). The observation surface is well-isolated, test coverage is solid (37 tests across 4 files), and the PR correctly scopes itself to observation primitives without wiring anything into the request lifecycle. Three non-blocking informational notes were left inline:
|
2 participants
What this changes
Adds the per-layout observation primitives that future #726-SKIP slices will use to decide whether a retained static layout can be reused. Nothing reads these observations yet, so request behavior is unchanged.
Why
Skip transport is only correct when the retained client layout is indistinguishable from the target server layout for this request. That requires observing every signal a layout depends on (params, request APIs, cacheLife, fetch tags, cacheable/dynamic fetches, unstable_cache, finite revalidate) before any reuse decision is made. This PR contributes the observation surface in isolation so it can be reviewed without the planner, manifest, or encoder churn from later slices.
Approach
Validation
Risks / follow-ups
Refs #726