refactor(app-router): gate browser state writes with approved visible commits#1090
Conversation
… commits Browser router commits currently mutate visible state from raw pending navigation actions after separate disposition checks. That leaves lifecycle approval implicit at each writer, which makes later stale-operation and planner work easier to bypass by accident. Introduce CommitDecision and ApprovedVisibleCommit as the proof object for visible browser commits. Route navigation, same-URL action, and HMR state writes through the approved commit path while preserving the legacy dispatch, skip, and hard-navigate dispositions for current callers. Tests cover approved navigate, replace, traverse, stale no-commit, and hard-navigation decisions, alongside the existing browser lifecycle regression coverage.
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.
Review: #726-CORE-02/03 — Approved Visible Commit Gate
Read #726 before reviewing. This is a well-scoped step toward the lifecycle authority spine described there. The extraction is clean: the controller gets simpler, the new module owns the commit proof, and the brand prevents external construction. Tests cover the happy path, lane guard, non-commit decisions, and all three action types through the approval path.
Overall this is ready to land. A few observations follow.
What's good
dispatchBrowserTree's 12-parameter signature collapsing intodispatchApprovedVisibleCommit(commit, pending, transition)is a real readability win. The controller lost ~50 lines and the intent at each call site is now obvious.- The brand symbol keeping
createApprovedVisibleCommitprivate is the right call. External callers can't forge proof, which is the whole point of the boundary. approveHmrVisibleCommitwith the lane guard is a good pattern — named trusted paths instead of generic escape hatches.- The
CommitApprovaldiscriminated union (VisibleCommitApproval | NonVisibleCommitApproval) makes it impossible to forget the null check at the type level. - Tests cover the key sequences: commit, no-commit, hard-navigate, HMR lane guard rejection, replace-without-merge, traverse-with-slot-cleanup.
Observations
See inline comments for specifics. Summary:
ClassifiedPendingNavigationCommitcarries bothdisposition(legacy) anddecision(new). Fine for the transitional step, but worth a comment noting which is authoritative.- The
approvedCommit === nullthrow inrenderNavigationPayloadis structurally unreachable after the two preceding guards. Current code is correct; a type predicate could eliminate the runtime check later. commitSameUrlNavigatePayloaddestructuresdisposition— the only use is=== "hard-navigate". Could switch todecision.dispositionand let a follow-up dropcommitDecisionToPendingNavigationCommitDispositionentirely.- Test coverage suggestion: the stale/hard-navigate non-approval test (line 659) could also assert trace reason codes (
NC_STALEandNC_ROOT) to verify the trace carries through the new approval layer.
| decision: CommitDecision; | ||
| disposition: PendingNavigationCommitDisposition; | ||
| pending: PendingNavigationCommit; | ||
| trace: NavigationTrace; |
There was a problem hiding this comment.
The ClassifiedPendingNavigationCommit type now carries both disposition (legacy PendingNavigationCommitDisposition) and decision (new CommitDecision), which are semantically redundant — one is always a mechanical translation of the other via commitDecisionToPendingNavigationCommitDisposition.
This is fine for the transitional step (the PR description is clear this is intentional), but it would help future readers to have a doc comment here noting that decision is the authoritative field and disposition exists only for legacy caller compatibility. Otherwise someone maintaining commitSameUrlNavigatePayload might not know which one to trust.
| }; | ||
| type VisibleCommitApproval = { | ||
| approvedCommit: ApprovedVisibleCommit; | ||
| decision: VisibleCommitDecision; |
There was a problem hiding this comment.
Nit: VisibleCommitApproval.decision is always the same reference as approvedCommit.decision (set at line 137). Having both means callers can read the decision from two places. For the approval result, decision is useful because callers need it before (or without) unwrapping the commit — the non-commit paths need decision.disposition without an approvedCommit. Current shape is fine, just noting the duplication for awareness.
|
|
||
| const approvedCommit = approval.approvedCommit; | ||
| if (approvedCommit === null) { | ||
| throw new Error("[vinext] Commit decision did not approve a visible commit"); |
There was a problem hiding this comment.
This throw is structurally unreachable: the two preceding guards ("no-commit" at line 425 and "hard-navigate" at line 432) return early for every non-commit decision, so approval.decision.disposition must be "commit" here, which means approval is VisibleCommitApproval and approvedCommit is non-null.
TypeScript can't narrow through the prior returns across the discriminated union, so the runtime check is defensible as a safety net. But if you want to eliminate it, a type assertion function on CommitApproval would work:
function assertVisibleCommitApproval(
approval: CommitApproval,
): asserts approval is VisibleCommitApproval {
if (approval.approvedCommit === null) {
throw new Error("...");
}
}Not blocking — the current code is correct.
| @@ -522,11 +486,11 @@ export function createAppBrowserNavigationController( | |||
| // need a stronger commit-version gate than activeNavigationId alone. | |||
| const { | |||
| disposition, | |||
There was a problem hiding this comment.
This is the last consumer of the legacy disposition field from resolveAndClassifyNavigationCommit. The only use is disposition === "hard-navigate" at line 506. Since decision is also available through the destructured result, you could check decision.disposition === "hard-navigate" instead, which would let a follow-up PR remove commitDecisionToPendingNavigationCommitDisposition and the legacy disposition field from ClassifiedPendingNavigationCommit entirely. Not blocking since preserving the legacy contract is a stated non-goal.
| }; | ||
| } | ||
|
|
||
| export function approveHmrVisibleCommit(pending: PendingNavigationCommit): ApprovedVisibleCommit { |
There was a problem hiding this comment.
The lane guard here is good — enforces that the HMR approval path can only be used by HMR operations.
PendingNavigationCommit.action.operation is always PendingOperationRecord (state "pending") by construction from createPendingNavigationCommit, so the type system already prevents CommittedOperationRecord from appearing here. Just noting this assumption is correct today and would need re-evaluation if PendingNavigationCommit objects were ever reused after commit.
|
Review posted on PR #1090 with 5 inline comments and a summary. The review covers:
Overall: the PR is a clean, well-scoped step toward the #726 lifecycle authority spine. The controller simplification from 12-param |
Visible commit classification still exposed a translated legacy disposition beside the authoritative decision. That left same-URL server action handling able to read two equivalent fields and made the approval boundary less clear than it needs to be. Use the commit decision directly, remove the translation helper, keep the old state-level disposition private, and assert the non-visible trace reason codes in the approval tests.
|
/bigbonk review |
There was a problem hiding this comment.
Re-review: #726-CORE-02/03 — Approved Visible Commit Gate
Previous review observations have been addressed:
-
ClassifiedPendingNavigationCommitdual-field redundancy — Fixed. The legacydispositionfield was removed fromClassifiedPendingNavigationCommit. The type now carries onlydecision: CommitDecisionas the authoritative field. -
Legacy
dispositionincommitSameUrlNavigatePayload— Fixed. Now destructuresdecisionand checksdecision.disposition === "hard-navigate"directly. ThecommitDecisionToPendingNavigationCommitDispositionfunction (which never existed in this PR but was a potential follow-up concern) is no longer relevant since the legacy shape was cleaned up in the same PR. -
Trace reason code assertions — Added. The stale/hard-navigate non-approval test (line 659) now asserts
NavigationTraceReasonCodes.staleOperationandNavigationTraceReasonCodes.rootBoundaryChanged, verifying the trace carries through the approval layer. -
Unreachable null throw (line 439-441 of controller) — Retained as a defensive safety net, which was the recommendation. Fine.
-
VisibleCommitApproval.decisionduplication and HMR lane guard — Both were acknowledged observations, not blocking items. No change needed.
Verification
- The
dispatchBrowserTree12-param function is fully removed. No references remain. LayoutFlagsandPendingOperationRecordimports were correctly pruned from the controller.PendingNavigationCommitis correctly re-exported for the visible-commit module to consume.- The
commitSameUrlNavigatePayloadpath'sif (approvedCommit)truthiness check is semantically equivalent to the oldif (disposition === "dispatch")—approvedCommitis non-null only for commit decisions. navigationSnapshotpropagation through the approval boundary is correct: it lives insidecommit.actionfor all three paths (navigation, HMR, same-URL action).- Brand symbol is properly confined to
app-browser-visible-commit.ts.
Clean, well-scoped step. Ready to land.
|
Review posted on PR #1090 with an approval. All 5 observations from the previous review were addressed or acknowledged:
The PR is a clean, well-scoped extraction that replaces the 12-parameter |
What this changes
Implements
#726-CORE-02/03from #726.This introduces
CommitDecisionandApprovedVisibleCommitas the explicit proof object for visible App Router browser commits. Navigation and same-URL server-action state writes now flow through lifecycle approval before visible browser state can be reduced, and HMR uses a named recovery approval path instead of generic proof construction.Why
Issue #726 is moving the browser navigation lifecycle toward explicit commit authority. The current code already had operation records and
visibleCommitVersion, but each browser writer still held its own implicit approval check before calling the reducer. That made future stale-result gates and planner work easier to bypass accidentally.This PR is the intended thin spine step: no proof, no visible commit transaction.
Approach
app-browser-visible-commit.tsas the visible browser mutation authority module.app-browser-state.tsfocused on state shape, reducer mechanics, pending commit construction, history helpers, server-action request headers, and legacy disposition tracing.dispatch,skip, andhard-navigatesemantics tocommit,no-commit, andhard-navigatedecisions.ApprovedVisibleCommitas a nominal commit proof and route visible state reduction throughapplyApprovedVisibleCommit.app-browser-visible-commit.ts.approvePendingNavigationCommit()as the navigation lifecycle gate.approveHmrVisibleCommit()as the named trusted HMR recovery path, with a lane guard so non-HMR pending commits cannot use it as a generic approval escape hatch.dispatchBrowserTreepath withdispatchApprovedVisibleCommitfor navigation, same-URL action payloads, and HMR.CommitDecisiondirectly, with the legacy disposition shape kept private to the older state-level classifier.Non-goals for this PR, matching
#726-CORE-02/03:NavigationPlannerValidation
vp check packages/vinext/src/server/app-browser-state.ts packages/vinext/src/server/app-browser-visible-commit.ts packages/vinext/src/server/app-browser-navigation-controller.ts tests/app-browser-entry.test.tsvp test run tests/app-browser-entry.test.ts tests/app-route-graph.test.tsvp run vinext#buildvp check --fixandknip --no-progressBonk
Bonk, please read issue #726 before reviewing so the
ApprovedVisibleCommitboundary is evaluated in the larger lifecycle and planner roadmap context, not as an isolated refactor.