[codex] fix examples chat thread sync#514
Merged
Merged
Conversation
|
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 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
/embed,/popup, and/sidebarkeep the same conversation when no explicit URL thread id is present.test-results/trace path on failure.Root Cause
The shell's URL-to-signal synchronization tracked
threadIdSignal()while comparing it tourlThreadId(). When the agent created a thread, the signal update could retrigger the URL sync effect while the URL was still/embed, clearing the thread back tonull. The backend run completed, but the UI returned to the welcome state, causing Playwright to wait until timeout for missing chat messages.Test Plan
npx nx test examples-chat-angular -- --runInBandCI=1 npx nx e2e examples-chat-angular --skip-nx-cachenode --test scripts/ci-workflow.spec.mjs