Skip to content

fix: use lastContext in the history listener - #24

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f002-history-last-context
Open

SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f002-history-last-context

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

Fixes an issue where consumers that call start(history, base, context) and later pass a new session or auth context to navigate or preload would still run loaders with the original start context on Back, Forward, or any other history-driven location change. README documents context as session, auth, or DI. After a later navigate("chat", { userId: "u_2" }), a popstate load still ran as u_1.

Why This Change Was Made

invalidate() already reloads with lastContext, which navigate and preload update. The history listener now reads that same stored context (falling back to the start argument only if nothing has been stored yet). No public exports, types, history adapter shape, or loader signature changed.

User Impact

After a session or auth context update, Back and Forward load the current user (or other context value), not the user from the original start() call. Apps that never change context after start() behave as before.

Evidence

Live node against the built dist/index.js on this branch, macOS 26.6.2, Node v26.7.0. The same public sequence (start as u_1, navigate as u_2, then a history emit to /settings) was run against the unpatched bundle and the patched bundle.

Unpatched dist/index.js (settings loader still sees u_1):

$ node /tmp/proof-uirouter-f002.mjs
LOADS chat:u_1 settings:u_1
ACTIVE_ROUTE settings
ACTIVE_USER u_1
STATUS success

Patched dist/index.js (settings loader sees u_2):

$ node /tmp/proof-uirouter-f002.mjs
LOADS chat:u_1 settings:u_2
ACTIVE_ROUTE settings
ACTIVE_USER u_2
STATUS success

pnpm run check passed locally (format, typecheck, lint, 21 tests, pack/import).

The listener has closed over the start argument since 5042bdc (2026-06-19, refactor: establish router foundation). 2260add (2026-06-29) added lastContext for invalidate() but did not update the listener. Adjacent but different: #20 (stale navigation cancellation) and #21 (loader redirect hop cap).

Real behavior proof

  • Behavior or issue addressed: History-driven loads (Back, Forward, adapter emit) use the latest navigate or preload context, not the context originally passed to start().

  • Real environment tested: macOS 26.6.2 arm64, Node v26.7.0, @openclaw/uirouter built from fix/f002-history-last-context at /tmp/oc-pr-uirouter-F002.

  • Exact steps or command run after this patch:

    cd /tmp/oc-pr-uirouter-F002
    node /tmp/proof-uirouter-f002.mjs
  • Evidence after fix: terminal output from the patched dist/index.js:

    $ node /tmp/proof-uirouter-f002.mjs
    LOADS chat:u_1 settings:u_2
    ACTIVE_ROUTE settings
    ACTIVE_USER u_2
    STATUS success
  • Observed result after fix: After start(..., { userId: "u_1" }) and navigate("chat", { userId: "u_2" }), a history emit to /settings loads settings as u_2. The same command against the unpatched bundle loaded settings as u_1.

  • What was not tested: A browser popstate adapter in a running OpenClaw UI shell, and loaderDeps that include the context value (same-route identity changes).

History-driven loads were closing over the context passed to start().
navigate and preload already store lastContext for invalidate(); the
listener now reads that same value so Back and Forward see the current
session or auth context.

Signed-off-by: Sebastien Tardif <[email protected]>
@SebTardif
SebTardif requested a review from a team as a code owner August 29, 2026 23:36
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 5, 2026, 2:58 PM ET / 18:58 UTC.

ClawSweeper review

What this changes

History-driven navigation uses the latest stored application context, with a regression test and changelog entry.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 3 items remain

This remains a useful fix absent from current main and v0.1.1, but the previously reported restart-context defect remains unfixed. The supplied terminal evidence demonstrates the ordinary history-navigation improvement.

Priority: P2
Reviewed head: 2036f6fe2a82d407d58691bacec20577ec9e4742

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Convincing before/after runtime evidence supports a focused fix, but the unchanged restart defect prevents merge readiness.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The captured Node terminal transcript exercises the built router's history listener and loaders, showing settings switch from u_1 to u_2 after navigation updates context; restart cases remain a correctness and regression-coverage blocker.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured Node terminal transcript exercises the built router's history listener and loaders, showing settings switch from u_1 to u_2 after navigation updates context; restart cases remain a correctness and regression-coverage blocker.
Evidence reviewed 8 items Repository policy and ownership: The remote identifies openclaw/uirouter. Root AGENTS.md was read fully; no nested AGENTS.md was found and .agents/maintainer-notes is absent. Policy establishes application-owned history adapters and loaderDeps as the match-identity boundary.
Introduced restart defect: The introduced listener reads lastContext before start stores its new argument. With an earlier stored context and a synchronous listener replay, navigate starts loading with the earlier value; the initial navigation can then join that same-match promise at lines 164–169. An unmatched initial location returns at lines 438–447 without storing the new context, leaving the next history event stale too.
Current main still needs the fix: Current main still passes the original start context directly from the history callback. Comparing main with the pinned base showed no differences in router.ts, types.ts, loading.ts, or README.md.
Findings 1 actionable finding [P2] Store the start context before subscribing to history
Security None None.

How this fits together

The standalone router receives application context and locations from navigation calls and application-provided history adapters. It selects routes, runs loaders and lifecycle hooks, and publishes route state for applications to display.

flowchart LR
  A[Application navigation] --> B[Stored context]
  C[History location change] --> D[History listener]
  B --> D
  D --> E[Route matching and loading]
  E --> F[Application route state]
Loading

Before merge

  • Store the start context before subscribing to history (P2) - After context u_2 has been stored, start(history, "", u_3) can synchronously receive /settings from listen(). This new callback selects u_2, starts its loader, and the initial navigation with u_3 joins the same-match promise at lines 164–169. Starting at an unmatched location also leaves u_2 stored for the next history event. Previously the callback used the new start argument. Initialize lastContext before subscribing and cover both cases; this previously reported blocker remains unfixed.
  • Resolve merge risk (P1) - Compatibility with reattaching history under a new context is not covered by the submitted test or terminal proof.
  • Complete next step (P2) - Store the new start context before registering history listeners, cover synchronous replay and unmatched initial locations, and run the focused tests plus the package quality gate.

Findings

  • [P2] Store the start context before subscribing to history — src/router.ts:551-553
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +2/-1; tests +13/-0; changelog +4/-0 The small production change reuses existing context storage, but its single added test covers only ordinary history navigation.

Merge-risk options

Maintainer options:

  1. Preserve context when history is reattached (recommended)
    Initialize the stored context before listener registration and add regressions for synchronous replay and an unmatched initial location.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Store the context supplied to start before registering the history listener. Add focused regressions covering an earlier stored context followed by start with synchronous listener replay and start at an unmatched location followed by a matched history event. Preserve loaderDeps identity, public APIs, and stop cleanup; run the focused history tests and pnpm run check.

Technical review

Best possible solution:

Make each start establish its supplied context before subscribing, while preserving subsequent context updates and the existing loaderDeps identity contract.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes both the current-main stale-context mechanism and the introduced restart defect: store one context, then start with another and a synchronously replaying adapter or unmatched initial location. No target code or tests were executed in this read-only review.

Is this the best way to solve the issue?

The approach is appropriate, but incomplete: reusing the existing context store is narrow and consistent with invalidation, provided start seeds it before any history callback.

Full review comments:

  • [P2] Store the start context before subscribing to history — src/router.ts:551-553
    After context u_2 has been stored, start(history, "", u_3) can synchronously receive /settings from listen(). This new callback selects u_2, starts its loader, and the initial navigation with u_3 joins the same-match promise at lines 164–169. Starting at an unmatched location also leaves u_2 stored for the next history event. Previously the callback used the new start argument. Initialize lastContext before subscribing and cover both cases; this previously reported blocker remains unfixed.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against ea06377b0e80.

Labels

Label justifications:

  • P2: This is a bounded router-context correction with a concrete restart regression.
  • merge-risk: 🚨 compatibility: Reattaching history with a new context can now invoke loaders using the previous context.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The captured Node terminal transcript exercises the built router's history listener and loaders, showing settings switch from u_1 to u_2 after navigation updates context; restart cases remain a correctness and regression-coverage blocker.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured Node terminal transcript exercises the built router's history listener and loaders, showing settings switch from u_1 to u_2 after navigation updates context; restart cases remain a correctness and regression-coverage blocker.

Evidence

Acceptance criteria:

  • [P1] pnpm run test -- test/router-subscriptions-history.test.ts.
  • [P1] pnpm run check.
  • [P1] git diff --check.
  • [P1] git diff --stat.

What I checked:

  • Repository policy and ownership: The remote identifies openclaw/uirouter. Root AGENTS.md was read fully; no nested AGENTS.md was found and .agents/maintainer-notes is absent. Policy establishes application-owned history adapters and loaderDeps as the match-identity boundary. (AGENTS.md:1, 2036f6fe2a82)
  • Introduced restart defect: The introduced listener reads lastContext before start stores its new argument. With an earlier stored context and a synchronous listener replay, navigate starts loading with the earlier value; the initial navigation can then join that same-match promise at lines 164–169. An unmatched initial location returns at lines 438–447 without storing the new context, leaving the next history event stale too. (src/router.ts:552, 2036f6fe2a82)
  • Current main still needs the fix: Current main still passes the original start context directly from the history callback. Comparing main with the pinned base showed no differences in router.ts, types.ts, loading.ts, or README.md. (src/router.ts:552, ea06377b0e80)
  • Latest release remains affected: The v0.1.1 router source retains the callback that closes over start's context. The supplied GitHub context identifies v0.1.1 as the latest release. (src/router.ts:552, f5ce7c0d7c04)
  • Public contract and production loader boundary: README documents arbitrary navigation context and application-provided history, while RouterHistory imposes no asynchronous-listener restriction. loading.ts passes the selected context to the application loader; cache identity remains explicitly controlled by loaderDeps. (README.md:55, 2036f6fe2a82)
  • Captured real behavior proof: The supplied complete PR body, captured under sourceRevision 95036e712114ff91db7bb28799a4cf7975c1d20bd64e89757b0fc91c736e0529, records Node v26.7.0 on macOS running the built package through start(u_1), navigate(u_2), and a history emit to settings. Before/after output changes settings:u_1 to settings:u_2 with success status. This exercises the changed listener and production loader path; it does not cover restarting with a new context. (2036f6fe2a82)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Shakker: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Initialize stored context before history listener registration.
  • Add synchronous-replay and unmatched-start regressions, then run focused history tests and pnpm run check.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (17 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-02T14:08:44.738Z sha 2036f6f :: needs changes before merge. :: [P2] Store the start context before subscribing to history
  • reviewed 2026-09-02T23:49:20.725Z sha 2036f6f :: blocked before merge. :: [P2] Seed the start context before subscribing to history
  • reviewed 2026-09-03T09:53:28.724Z sha 2036f6f :: blocked before merge. :: [P2] Seed the start context before registering the history listener
  • reviewed 2026-09-03T09:59:54.057Z sha 2036f6f :: blocked before merge. :: [P2] Store the start context before subscribing to history
  • reviewed 2026-09-03T20:52:13.153Z sha 2036f6f :: blocked before merge. :: [P2] Store the start context before subscribing to history
  • reviewed 2026-09-04T21:53:41.138Z sha 2036f6f :: blocked before merge. :: [P2] Store the start context before subscribing to history
  • reviewed 2026-09-05T07:03:04.897Z sha 2036f6f :: blocked before merge. :: [P2] Store the start context before subscribing to history
  • reviewed 2026-09-05T10:55:14.031Z sha 2036f6f :: blocked before merge. :: [P2] Store the start context before subscribing to history

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant