feat(app-router): trace approved visible commit outcomes#1101
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean implementation of #726 OPS-02/03. The visible commit boundary is now the sole owner of visibleCommitVersion increments, the raw reducer is no longer exported, and the three transaction trace codes (NT_VISIBLE_COMMIT, NT_NO_COMMIT, NT_HARD_NAVIGATE) compose cleanly with the existing reason-code entries.
The move from app-browser-state.ts to app-browser-visible-commit.ts is the right direction: the old file loses ~80 lines of reducer logic and the mergeElements import, and the new file gains ownership of everything that touches visible state mutation. The Omit<AppRouterState, ...> replacement for the hand-rolled AppRouterStateWithoutCommitMetadata type is a nice simplification.
Test coverage is thorough. All three transaction outcomes are tested (visible commit, no-commit, hard-navigate), the unknown-root-layout uncertainty path is covered, the HMR path asserts the transaction trace, and the export-boundary test (Object.hasOwn(stateModule, 'routerReducer')) enforces the invariant at import time. The applyApprovedTestCommit helper reduces boilerplate substantially.
Two minor observations below — neither is blocking.
| } | ||
|
|
||
| const decision = addCommitTransactionTrace(createVisibleCommitDecision(), pending); | ||
| if (decision.disposition !== "commit") { |
There was a problem hiding this comment.
This check is structurally unreachable: createVisibleCommitDecision() always returns { disposition: "commit", ... }, and addCommitTransactionTrace preserves the disposition of the input decision (the switch is exhaustive over all three disposition variants and spreads ...decision). So decision.disposition can never be anything other than "commit" here.
As a defensive assertion it's fine — it narrows the type for the createApprovedVisibleCommit call below, so TypeScript is happy. But if the intent is purely to satisfy the type system, a comment saying so would help future readers understand this isn't guarding against a real runtime path.
Non-blocking, just a readability note.
| return prependNavigationTraceEntry(trace, code, createCommitTransactionFields(pending)); | ||
| } | ||
|
|
||
| function addCommitTransactionTrace( |
There was a problem hiding this comment.
The exhaustive switch over decision.disposition is correct and mirrors the pattern used in reduceApprovedVisibleCommitState and approvePendingNavigationCommit. Worth noting: since the three branches all perform structurally identical work (spread decision + prepend a code-specific trace entry), this could be collapsed to a single mapping from disposition to transaction code. But the exhaustive switch is arguably clearer for a boundary function like this, so this is purely a style observation, not a request.
Example of the collapsed form if you ever want it:
const transactionCodeByDisposition = {
"commit": NavigationTraceTransactionCodes.visibleCommit,
"hard-navigate": NavigationTraceTransactionCodes.hardNavigate,
"no-commit": NavigationTraceTransactionCodes.noCommit,
} as const satisfies Record<CommitDecision["disposition"], NavigationTraceTransactionCode>;
|
Review submitted. Approved with two minor non-blocking observations:
The PR is clean: ownership moves are correct, |
Visible router state currently has an approved commit path, but the raw reducer remains an exported mutation escape hatch and approval traces only explain low-level reasons. That makes it harder to audit whether a payload became visible, was discarded, or forced a hard navigation. The violated invariant is that visibleCommitVersion should have one owner behind ApprovedVisibleCommit, with every approval decision carrying an operator-visible transaction outcome. Move the reducer implementation behind applyApprovedVisibleCommit, keep visibleCommitVersion advancement in that boundary, and prepend transaction trace codes for visible commits, no-commit decisions, and hard navigations.
NathanDrake2406
force-pushed
the
nathan/726-ops-trace-invariants
branch
from
9ae01be to
a171328
Compare
2 participants
What this changes
Implements
#726-OPS-02/03from #726: visible commit invariants and transaction trace codes.This PR makes the approved visible commit path the only exported way to mutate browser-visible App Router state. The previous raw reducer export is no longer available, and the single
visibleCommitVersionincrement now lives inside theApprovedVisibleCommitboundary.It also adds transaction-level navigation trace codes for the three approval outcomes:
NT_VISIBLE_COMMITNT_NO_COMMITNT_HARD_NAVIGATEThose transaction entries are prepended to the existing reason entries such as
NC_COMMIT,NC_STALE,NC_ROOT, andNC_ROOT_UNKNOWN, so operators can see both what happened and why.Bonk: please read issue #726 to see the big picture behind this slice before reviewing the local mechanics.
Why
Issue #726 is building toward a lifecycle model where visible state mutation, stale result rejection, hard navigation fallback, and future cache or skip decisions have observable authority boundaries. Without an explicit transaction outcome in the trace,
noCommit, visible commit, and hard navigation paths are harder to audit. Without fencing the raw reducer export, future code can accidentally bypass the approved commit boundary.Approach
The browser state module still owns pending operation creation, route compatibility decisions, and reason traces. The visible commit module now owns the actual visible state reducer and the one place where
visibleCommitVersionadvances.Approval now wraps every decision trace with a transaction entry carrying the operation lane, pending operation id, and started visible commit version. Current behavior for navigate, replace, traverse, HMR recovery, stale commits, unknown-root soft fallback, and root-boundary hard navigation is preserved.
Validation
vp test run tests/app-browser-entry.test.tspasses: 66 testsvp check tests/app-browser-entry.test.ts packages/vinext/src/server/app-browser-visible-commit.ts packages/vinext/src/server/app-browser-state.ts packages/vinext/src/server/navigation-trace.tspasses with no warnings, lint errors, or type errorsvp run vinext#buildpasses, with the repo's existing virtual/external unresolved import warnings during packvp checkexits 0 across the repo, with one pre-existing warning inpackages/vinext/src/server/request-pipeline.ts:604Risks / follow-ups
This intentionally does not add broad runtime logging, cache metrics, skip metrics, stale same-URL server-action rejection, or planner behavior. It is scoped to the visible commit invariant and approval-path trace codes requested by
#726-OPS-02/03.