fix(examples-chat): collapse mode routes via UrlMatcher (unbreaks e2e + Vercel)#504
Merged
Conversation
…omponent instance PR #500 added a route entry per (mode, hasThreadId) — six entries total, two per mode. Navigating from `/embed` to `/embed/<id>` was a route CHANGE (different entry), which tore down EmbedMode and remounted it, killing the active stream when the agent auto-created a thread mid-send. Symptom: `examples/chat — e2e` test "failed stream surfaces an alert and the next send recovers" + "core controls expose expected accessible names" both fail because the assistant message never renders (stream died at remount time). Vercel deploy gates on e2e, so prod has been stuck on the pre-#500 bundle. Fix: collapse the per-mode pair into a single route entry via UrlMatcher. Both `/embed` and `/embed/<threadId>` now resolve to the same route, so the component instance survives the navigation and the stream keeps flowing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
5 tasks
blove
added a commit
that referenced
this pull request
May 21, 2026
…hread PR #500 introduced URL-based thread routing with the intent that the URL would replace localStorage as the persistence layer. PR #514 partly walked that back by re-introducing a localStorage `threadId` fallback to fix mode-switch sync — but that fallback conflates URL state with browser-local state and silently teleports users to old threads when they navigate to bare-mode URLs (paste link, back button). This finishes the URL-as-truth migration: - Drops `threadId` from `PaletteState`. - Removes the persistence write effect + persistence-read fallback in the URL→signal sync and `threadIdSignal` initialiser. - Removes the persistence clear in `validateUrlThreadId`'s 404 handler. - Keeps PR #514's `untracked` guard on the URL→signal effect — that guard prevents the stamp-in-progress signal from being cleared during the async URL navigation gap. It works without the persistence layer. - Keeps PR #504's `UrlMatcher` collapse (the stream-survival fix). - Keeps PR #500's `getThread()` validator + 404 redirect. Mode-switch UI continues to preserve the active thread across mode boundaries via `onModeChange` (URL navigation to `/<next-mode>/<id>`), which was the bug PR #514 was trying to fix. That path didn't need localStorage — it just needed the URL navigation to carry the id. Tests: - "does not write the active thread id to localStorage (URL is the source of truth)" — new - "ignores any legacy persisted threadId — bare mode URLs start fresh" — new (covers users who upgrade with legacy localStorage state) - "hydrates the active thread id from /<mode>/<threadId> URLs" — new - "does not clear an agent-created thread id while URL navigation is still pending" — retained from PR #514 Spec at `docs/superpowers/specs/2026-05-20-url-thread-routing-design.md` rewritten to match the simplified architecture; was describing the pre-#504 6-route world and the pre-#514 sync flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
blove
added a commit
that referenced
this pull request
May 21, 2026
…hread PR #500 introduced URL-based thread routing with the intent that the URL would replace localStorage as the persistence layer. PR #514 partly walked that back by re-introducing a localStorage `threadId` fallback to fix mode-switch sync — but that fallback conflates URL state with browser-local state and silently teleports users to old threads when they navigate to bare-mode URLs (paste link, back button). This finishes the URL-as-truth migration: - Drops `threadId` from `PaletteState`. - Removes the persistence write effect + persistence-read fallback in the URL→signal sync and `threadIdSignal` initialiser. - Removes the persistence clear in `validateUrlThreadId`'s 404 handler. - Keeps PR #514's `untracked` guard on the URL→signal effect — that guard prevents the stamp-in-progress signal from being cleared during the async URL navigation gap. It works without the persistence layer. - Keeps PR #504's `UrlMatcher` collapse (the stream-survival fix). - Keeps PR #500's `getThread()` validator + 404 redirect. Mode-switch UI continues to preserve the active thread across mode boundaries via `onModeChange` (URL navigation to `/<next-mode>/<id>`), which was the bug PR #514 was trying to fix. That path didn't need localStorage — it just needed the URL navigation to carry the id. Tests: - "does not write the active thread id to localStorage (URL is the source of truth)" — new - "ignores any legacy persisted threadId — bare mode URLs start fresh" — new (covers users who upgrade with legacy localStorage state) - "hydrates the active thread id from /<mode>/<threadId> URLs" — new - "does not clear an agent-created thread id while URL navigation is still pending" — retained from PR #514 Spec at `docs/superpowers/specs/2026-05-20-url-thread-routing-design.md` rewritten to match the simplified architecture; was describing the pre-#504 6-route world and the pre-#514 sync flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
blove
added a commit
that referenced
this pull request
May 21, 2026
…hread (#518) * refactor(examples-chat): URL is the sole source of truth for active thread PR #500 introduced URL-based thread routing with the intent that the URL would replace localStorage as the persistence layer. PR #514 partly walked that back by re-introducing a localStorage `threadId` fallback to fix mode-switch sync — but that fallback conflates URL state with browser-local state and silently teleports users to old threads when they navigate to bare-mode URLs (paste link, back button). This finishes the URL-as-truth migration: - Drops `threadId` from `PaletteState`. - Removes the persistence write effect + persistence-read fallback in the URL→signal sync and `threadIdSignal` initialiser. - Removes the persistence clear in `validateUrlThreadId`'s 404 handler. - Keeps PR #514's `untracked` guard on the URL→signal effect — that guard prevents the stamp-in-progress signal from being cleared during the async URL navigation gap. It works without the persistence layer. - Keeps PR #504's `UrlMatcher` collapse (the stream-survival fix). - Keeps PR #500's `getThread()` validator + 404 redirect. Mode-switch UI continues to preserve the active thread across mode boundaries via `onModeChange` (URL navigation to `/<next-mode>/<id>`), which was the bug PR #514 was trying to fix. That path didn't need localStorage — it just needed the URL navigation to carry the id. Tests: - "does not write the active thread id to localStorage (URL is the source of truth)" — new - "ignores any legacy persisted threadId — bare mode URLs start fresh" — new (covers users who upgrade with legacy localStorage state) - "hydrates the active thread id from /<mode>/<threadId> URLs" — new - "does not clear an agent-created thread id while URL navigation is still pending" — retained from PR #514 Spec at `docs/superpowers/specs/2026-05-20-url-thread-routing-design.md` rewritten to match the simplified architecture; was describing the pre-#504 6-route world and the pre-#514 sync flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(examples-chat): update e2e specs for URL-as-truth Four specs were reading the active thread id from \`localStorage['ngaf-chat-demo:palette'].threadId\` — which no longer exists after the persistence layer was dropped. One spec asserted cross-mode persistence via bare /<mode> navigation, which now lands on the welcome state by design (URL is the sole source of truth). Changes: - New helper \`activeThreadIdFromUrl(page)\` in test-helpers.ts — parses \`/<mode>/<threadId>\` URL shape. - lifecycle.spec.ts:27 — "New chat (sidenav)…" now asserts URL flips to bare /embed on welcome state, then sends again to verify a fresh thread id replaces the prior one (reads from URL, not localStorage). - mode-routing.spec.ts:39 — "cross-mode persistence…" captures the thread id after first send, then navigates to /<other-mode>/<id> explicitly. Bare /<mode> would clear the thread by design. - model-picker.spec.ts:12 — reads threadId from URL via the helper. - regenerate.spec.ts:5 — same. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(examples-chat): correct lifecycle 'New chat' URL expectation The sidenav 'New chat' button calls \`onNewThread\` which creates a new thread server-side and sets \`threadIdSignal\` to the new id — the signal→URL effect then navigates to /embed/<new-thread-id>. The URL does NOT go back to bare /embed; the welcome state renders because the new thread is empty, not because the URL is bare. Drops the incorrect \`expect(page).toHaveURL(/\\/embed\$/)\` assertion and removes the redundant second send. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 21, 2026
blove
added a commit
that referenced
this pull request
May 25, 2026
* test(examples-chat): pin no-nav-loop invariant in URL↔signal sync Adds a regression guard for the invariant every PR in the routing chain (#500/#504/#514/#518/#527) was dancing around: when the URL→ signal effect hydrates threadIdSignal from /embed/<id>, the signal→URL effect MUST see signal === urlState().threadId and short-circuit. Without that guard we'd loop: URL → signal → router.navigate → URL → ... The test asserts zero NavigationEnd events fire between the initial navigateByUrl and the end of detectChanges — proving the compare- and-set guard at demo-shell.component.ts (signal→URL effect) does its job. 33/33 examples-chat-angular unit tests passing locally. * test: remove unused vi import (broke Angular build)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#500 broke
examples/chat — e2eandCanonical demo → Vercelon main. Investigation:Diagnosis: #500 added two route entries per mode (
embed,embed/:threadId, etc — six entries total). Navigating from/embedto/embed/<id>is a route change (different entry), which makes Angular tear down + remount the mode component — killing any active stream.Symptom: e2e test
error-handling.spec.ts:5andkeyboard-accessibility.spec.ts:29both depend on sending a message and seeing the assistant message render. After #500, the agent auto-creates a thread mid-send → signal→URL effect firesrouter.navigate('/embed/<new-id>')→ component remounts → stream dies → assistant message never arrives → test fails.Cascade:
Canonical demo → Vercelgates on e2e green, so prod has been stuck on the pre-#500 bundle since merge.Fix
Collapse the per-mode pair into one route entry via
UrlMatcher. Both/<mode>and/<mode>/<threadId>now resolve to the same route, so the component instance survives the navigation and the stream keeps flowing.Test plan
nx build examples-chat-angulargreenexamples/chat — e2e)🤖 Generated with Claude Code