Skip to content

fix(router): let a pending navigation finish when invalidate runs - #46

Merged
vincentkoc merged 3 commits into
openclaw:mainfrom
SebTardif:fix/invalidate-pending-nav
Oct 1, 2026
Merged

vincentkoc merged 3 commits into
openclaw:mainfrom
SebTardif:fix/invalidate-pending-nav

Conversation

@SebTardif

@SebTardif SebTardif commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes: untargeted invalidate or revalidate can cancel a pending navigation and leave history on the destination while router state remains on the previous match.

User Impact

User impact: refreshing current route data while another match is loading no longer cancels that navigation, including transitions between two loaderDeps identities of the same route. Invalidated data remains stale when the previous active match moves into cache, so returning to it refetches as expected. Explicit route refresh behavior is unchanged.

Why This Change Was Made

Untargeted refresh compares the live navigation's match identity with the active match identity. Cache transfer also re-reads the match store instead of reusing an earlier navigation snapshot, preserving invalidation that occurs while the destination loads.

Evidence

Validated at 1a24efa9ce742be23b6bc542f10c7f1a65cf637c with Node 26.8.1 and pnpm 11.27.1:

  • pnpm run test -- test/router-invalidate-pending.test.ts: 9 files and 58 tests passed.
  • pnpm run check: format, build, typecheck, lint, all 58 tests, and packed-package import passed.
  • git diff --check: passed.
  • Focused coverage verifies untargeted invalidate and revalidate during /chat?session=one to /chat?session=two, where loaderDeps gives each location a distinct match identity.
  • A positive-staleTime regression defers route b's component, invalidates while b is pending, confirms b still finishes, then returns to route a and confirms its loader refetches.

The earlier built-package proof at 14e966dc7b3fcbdd60af919500ae762f27429d77 also showed history, location, active route, and data settling on /b after untargeted invalidate and revalidate during a two-route navigation.

@SebTardif
SebTardif requested a review from a team as a code owner October 1, 2026 01:50
@clawsweeper

clawsweeper Bot commented Oct 1, 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 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 Oct 1, 2026
@clawsweeper

clawsweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed October 1, 2026, 3:47 AM ET / 07:47 UTC (Revision 3).

ClawSweeper review

What this changes

Preserve pending navigation during untargeted invalidation or revalidation, and retain invalidated data when the previous route moves into cache.

Merge readiness

✅ Ready for maintainer review

This PR remains necessary because main still cancels a pending destination during untargeted refresh. Both prior findings are resolved, and no blocking correctness concern remains.

Priority: P2
Reviewed head: 1a24efa9ce742be23b6bc542f10c7f1a65cf637c

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with applicable package-run proof, successful reported validation, and both prior correctness findings resolved.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): Retained built-package Node terminal proof exercises the production router during two-route navigation and both untargeted refresh methods, observing destination history, state, and data after completion. Current-head regressions supplement this with dependency identities and invalidated-cache return behavior; no stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): Retained built-package Node terminal proof exercises the production router during two-route navigation and both untargeted refresh methods, observing destination history, state, and data after completion. Current-head regressions supplement this with dependency identities and invalidated-cache return behavior; no stored-data contract changes.
Evidence reviewed 7 items Introduced patch: The pinned main-to-head diff changes only the router, its changelog, and five focused regression tests. Refresh guards compare match identity, while cache transfers read the latest stored match.
Prior findings resolved: The identity guards cover separate loaderDeps identities of one route. The cache helper replaces the previous snapshot transfer, and the positive-staleTime regression verifies that returning to the invalidated route reloads its data. The earlier router blob was inspected through GitHub after local historical blob inspection failed.
Still needed on main: Pinned main invokes active-route navigation from both untargeted refresh methods without checking a different pending destination. Navigation cancels the current run. The repository PR listing contains no merged replacement for this fix.
Findings None None.
Security None None.

How this fits together

The standalone UI router receives application routes, locations, and loading callbacks. It coordinates navigation and cached data, then exposes route state for applications to present.

flowchart TD
  A[Application navigation] --> C[Router transitions]
  B[Refresh request] --> D[Compare match identities]
  D --> C
  C --> E[Component and data loading]
  C --> F[Preserve cached invalidation]
  E --> G[Published route state]
  F --> G
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC production +46/-41 (net +5); tests +238/-0 The small production increase implements shared cache transfer and identity guards, supported by five regression cases.

Technical review

Best possible solution:

Keep navigation and history aligned while preserving invalidated cached data and explicit route-refresh semantics.

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

Yes, source establishes the trigger: start loading another match while one remains active, then call untargeted invalidate or revalidate; main cancels the destination run. This review did not execute a reproduction.

Is this the best way to solve the issue?

Yes, comparing match identities and caching the latest stored state repairs the concurrency defect without expanding the public API.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): Retained built-package Node terminal proof exercises the production router during two-route navigation and both untargeted refresh methods, observing destination history, state, and data after completion. Current-head regressions supplement this with dependency identities and invalidated-cache return behavior; no stored-data contract changes.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.

Label justifications:

  • P2: This fixes a bounded concurrency defect affecting navigation state and route-cache invalidation.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): Retained built-package Node terminal proof exercises the production router during two-route navigation and both untargeted refresh methods, observing destination history, state, and data after completion. Current-head regressions supplement this with dependency identities and invalidated-cache return behavior; no stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. Retained built-package Node terminal proof exercises the production router during two-route navigation and both untargeted refresh methods, observing destination history, state, and data after completion. Current-head regressions supplement this with dependency identities and invalidated-cache return behavior; no stored-data contract changes.

Evidence

What I checked:

  • Introduced patch: The pinned main-to-head diff changes only the router, its changelog, and five focused regression tests. Refresh guards compare match identity, while cache transfers read the latest stored match. (src/router.ts:173, 1a24efa9ce74)
  • Prior findings resolved: The identity guards cover separate loaderDeps identities of one route. The cache helper replaces the previous snapshot transfer, and the positive-staleTime regression verifies that returning to the invalidated route reloads its data. The earlier router blob was inspected through GitHub after local historical blob inspection failed. (test/router-invalidate-pending.test.ts:198, 1a24efa9ce74)
  • Still needed on main: Pinned main invokes active-route navigation from both untargeted refresh methods without checking a different pending destination. Navigation cancels the current run. The repository PR listing contains no merged replacement for this fix. (src/router.ts:592, a17a88d49566)
  • Release check: GitHub identifies v0.1.1, published August 10, 2026, as the latest release. Current main still lacks the requested fix, so neither release nor main supports closing this PR as implemented. (f5ce7c0d7c04)
  • Retained real behavior proof: The captured PR body retains built-package Node proof at 14e966d: untargeted invalidate and revalidate during two-route navigation settled history, location, active route, and data on /b. The previous completed review classified that terminal proof as sufficient. Current-head tests supplement it with same-route identity and cache-return scenarios; the original terminal trace is not reproduced in the current body. (14e966dc7b3f)
  • Validation and scope: The captured body reports pnpm run check, all 58 tests, and packed-package import passing at the exact head; supplied platform checks are successful. Read-only diff inspection confirms production +46/-41, tests +238/-0, and a clean diff check. No tests or builds were executed during this review. (test/router-invalidate-pending.test.ts:125, 1a24efa9ce74)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

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 (2 earlier review cycles)
  • reviewed 2026-10-01T01:53:25.007Z sha 14e966d :: needs changes before merge. :: [P2] Preserve pending matches within the same route
  • reviewed 2026-10-01T07:39:42.961Z sha dcbc0c3 :: blocked before merge. :: [P2] Preserve invalidation when caching the previous active match

SebTardif and others added 2 commits October 1, 2026 09:33
invalidate() and revalidate() without a route id started a navigation of the active route, which aborted an in-flight navigation to a different destination.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@vincentkoc
vincentkoc force-pushed the fix/invalidate-pending-nav branch from 14e966d to dcbc0c3 Compare October 1, 2026 07:34
@vincentkoc

Copy link
Copy Markdown
Member

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Oct 1, 2026
@vincentkoc

Copy link
Copy Markdown
Member

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Oct 1, 2026
@vincentkoc
vincentkoc merged commit 5dc5da2 into openclaw:main Oct 1, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants