fix(flows): persist + rehydrate the Copilot chat across reload (B11)#4663
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCopilot thread ID caching moves from an in-memory Map to user-scoped localStorage with error handling. useWorkflowBuilderChat adds displayMessages (filtering interim agent messages) and rehydrates persisted threads via seedThreadId, recovering from stale/not-found threads. threadSlice merges local and fetched messages and evicts stale thread state on ThreadNotFound errors. ChangesCopilot transcript filtering and thread recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant useWorkflowBuilderChat
participant threadSlice
participant ChatRuntimeCore
useWorkflowBuilderChat->>useWorkflowBuilderChat: derive displayMessages from messages
useWorkflowBuilderChat->>threadSlice: loadThreadMessages(seedThreadId)
useWorkflowBuilderChat->>ChatRuntimeCore: fetchAndHydrateTurnState(threadId)
useWorkflowBuilderChat->>ChatRuntimeCore: fetchAndHydrateTurnHistory(threadId)
threadSlice-->>useWorkflowBuilderChat: merged messages or THREAD_NOT_FOUND_MESSAGE
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ffef9d0ba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export function getCopilotThreadId(flowId: string | null): string | null { | ||
| return copilotThreadByFlow.get(copilotThreadKey(flowId)) ?? null; | ||
| try { | ||
| return window.localStorage.getItem(storageKey(flowId)); |
There was a problem hiding this comment.
When a cached id points at a thread that no longer exists (for example the workflow-builder thread was deleted/purged), this now returns that stale seed on every open. useWorkflowBuilderChat.send only creates a new thread when threadId is null, so the panel keeps trying to append to the missing thread and shows an error instead of recovering; clear the cache/null out the seed on a not-found rehydrate or append failure so the next send can create a fresh builder thread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Root cause confirmed: useWorkflowBuilderChat's local threadId state never reset when the seeded thread was gone, so the panel kept retrying the dead thread on every send (and every rehydrate).
- 51a6e23 (threadSlice.ts): loadThreadMessages now detects ThreadNotFound the same way addMessageLocal/addInferenceResponse/generateThreadTitleIfNeeded already do — evicts the stale thread from Redux and rejects with THREAD_NOT_FOUND_MESSAGE instead of just recording messagesError with no cleanup.
- 42cb9be (useWorkflowBuilderChat.ts): on that rejection — whether it surfaces during the mount rehydrate or during a send()'s addMessageLocal append — the hook now nulls out its threadId. WorkflowCopilotPanel already reports every threadId change back up via onThreadIdChange, so this cascades into FlowCanvasPage clearing the stale workflowCopilotThreads.ts cache entry, and the next send creates a fresh builder thread instead of erroring against the dead one.
Added coverage in useWorkflowBuilderChat.test.ts (both the rehydrate-clears-threadId and send-clears-threadId paths) and threadSlice.test.ts (the new ThreadNotFound branch of loadThreadMessages).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/components/flows/workflowCopilotThreads.test.ts (1)
63-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for
setItem/removeItemfailure paths.Only
getItemthrowing is exercised;setCopilotThreadId's own try/catch (write and remove paths) is never triggered by a failinglocalStorage.setItem/removeItem, leaving that resilience branch unverified.✅ Suggested additional test
+ it('degrades to a no-op when localStorage.setItem throws (write path)', () => { + const original = window.localStorage.setItem; + window.localStorage.setItem = () => { + throw new Error('quota exceeded'); + }; + try { + expect(() => setCopilotThreadId('flow-1', 'thread-abc')).not.toThrow(); + } finally { + window.localStorage.setItem = original; + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/flows/workflowCopilotThreads.test.ts` around lines 63 - 75, The existing test only covers `getCopilotThreadId` when `localStorage.getItem` throws, but it does not exercise the failure handling inside `setCopilotThreadId`. Add coverage in `workflowCopilotThreads.test.ts` that simulates `window.localStorage.setItem` and `window.localStorage.removeItem` throwing, then verify `setCopilotThreadId` still degrades to a no-op without throwing. Use the `setCopilotThreadId` and `getCopilotThreadId` helpers to keep the test aligned with the resilience paths in the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/flows/workflowCopilotThreads.ts`:
- Around line 44-62: Add debug logging to the new localStorage persistence flow
so failures are observable: in `getCopilotThreadId` and `setCopilotThreadId`,
replace the silent `catch` blocks with calls to the existing `log(...)` helper
using a stable prefix similar to `useWorkflowBuilderChat`’s rehydration logging,
and include safe context like `flowId` and whether the operation is read or
write. Keep the error handling non-fatal, but make sure any quota/private-mode
or unexpected exception is logged before returning/nulling out so thread
restore/persist issues can be diagnosed.
In `@app/src/hooks/useWorkflowBuilderChat.ts`:
- Around line 186-213: The rehydrate effect in useWorkflowBuilderChat is still
able to overwrite an in-flight turn because it only skips threads created
locally via createdThreadIdRef, not existing seedThreadId values. Update the
rehydration logic around the useEffect that dispatches loadThreadMessages,
fetchAndHydrateTurnState, and fetchAndHydrateTurnHistory to also detect an
active send/turn-in-flight state and skip reloading until it settles, or change
the loadThreadMessages.fulfilled handling so it merges fetched messages into
messagesByThreadId[threadId] instead of replacing the array wholesale.
---
Nitpick comments:
In `@app/src/components/flows/workflowCopilotThreads.test.ts`:
- Around line 63-75: The existing test only covers `getCopilotThreadId` when
`localStorage.getItem` throws, but it does not exercise the failure handling
inside `setCopilotThreadId`. Add coverage in `workflowCopilotThreads.test.ts`
that simulates `window.localStorage.setItem` and
`window.localStorage.removeItem` throwing, then verify `setCopilotThreadId`
still degrades to a no-op without throwing. Use the `setCopilotThreadId` and
`getCopilotThreadId` helpers to keep the test aligned with the resilience paths
in the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdef21b4-7d1d-44c7-a3cd-b07f743ff148
📒 Files selected for processing (7)
app/src/components/flows/WorkflowCopilotPanel.test.tsxapp/src/components/flows/WorkflowCopilotPanel.tsxapp/src/components/flows/workflowCopilotThreads.test.tsapp/src/components/flows/workflowCopilotThreads.tsapp/src/hooks/useWorkflowBuilderChat.test.tsapp/src/hooks/useWorkflowBuilderChat.tsapp/src/providers/ChatRuntimeProvider.tsx
…l/replace Two related threadSlice.ts gaps surfaced by PR tinyhumansai#4663 review: - loadThreadMessages was the only read/write thunk in this file that did not special-case ThreadNotFound like addMessageLocal/addInferenceResponse/ generateThreadTitleIfNeeded already do — a deleted/purged thread (e.g. a stale id cached by workflowCopilotThreads.ts) just recorded messagesError with no eviction, so the stale thread lingered in Redux forever. Now it evicts via the same evictStaleThread() path and rejects with THREAD_NOT_FOUND_MESSAGE. - loadThreadMessages.fulfilled wholesale-replaced messagesByThreadId[id], which could clobber a message that landed locally (via a concurrent addMessageLocal/addInferenceResponse append) but predates this fetch's snapshot. useWorkflowBuilderChat's copilot rehydrate effect can race a WorkflowCopilotPanel auto-send (repairSeed/buildSeed) on the very same mount, so this is reachable, not just theoretical. Merge the fetched list with any local-only ids instead of replacing. (addresses @coderabbitai on app/src/hooks/useWorkflowBuilderChat.ts:213, foundation for the stale-thread-id fix on workflowCopilotThreads.ts:46)
|
Addressed the nitpick from the CHANGES_REQUESTED review ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/store/threadSlice.ts (1)
115-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a debug log at the ThreadNotFound detection point.
Only the nested
evictStaleThreadrefresh-failure path logs (and only underIS_DEV); the actual eviction decision — a materially "changed flow" for stale-thread recovery — has no log line withthreadIdas a correlation field.🔍 Suggested addition
if (isThreadNotFoundCoreRpcError(error, threadId)) { + if (IS_DEV) { + console.debug('[threadSlice] loadThreadMessages: thread not found, evicting', { + threadId, + }); + } await evictStaleThread(threadId, dispatch); return rejectWithValue(THREAD_NOT_FOUND_MESSAGE); }As per coding guidelines: "On new or changed flows, add verbose debug logging with stable prefixes, correlation fields, and no secrets or full PII; incomplete changes are those lacking logging."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/store/threadSlice.ts` around lines 115 - 129, Add a debug log in the getThreadMessages catch block at the isThreadNotFoundCoreRpcError check before calling evictStaleThread. The ThreadNotFound branch currently changes the recovery flow without any log, so emit a verbose debug message with a stable prefix and threadId as the correlation field, then keep the existing evictStaleThread and rejectWithValue behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/store/threadSlice.ts`:
- Around line 115-129: Add a debug log in the getThreadMessages catch block at
the isThreadNotFoundCoreRpcError check before calling evictStaleThread. The
ThreadNotFound branch currently changes the recovery flow without any log, so
emit a verbose debug message with a stable prefix and threadId as the
correlation field, then keep the existing evictStaleThread and rejectWithValue
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b4fa251-be88-4bf8-82e9-c7e95bec6729
📒 Files selected for processing (6)
app/src/components/flows/workflowCopilotThreads.test.tsapp/src/components/flows/workflowCopilotThreads.tsapp/src/hooks/useWorkflowBuilderChat.test.tsapp/src/hooks/useWorkflowBuilderChat.tsapp/src/store/__tests__/threadSlice.test.tsapp/src/store/threadSlice.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/components/flows/workflowCopilotThreads.ts
- app/src/components/flows/workflowCopilotThreads.test.ts
- app/src/hooks/useWorkflowBuilderChat.ts
The Copilot chat thread id was only cached in a module-level Map, and
useWorkflowBuilderChat never reloaded a thread's messages/turn state on
mount — so a reload (or a fresh app launch) always started the copilot
transcript from empty even though the thread itself is durable
server-side (threadApi persists every turn).
- workflowCopilotThreads.ts: back the flow -> thread-id cache with
localStorage (namespaced `${userId}:copilot-thread:<flow>`, matching
the existing per-user localStorage convention) instead of an in-memory
Map, wrapped in try/catch so private-mode/quota errors degrade to a
no-op rather than throwing.
- useWorkflowBuilderChat: on mount, when `seedThreadId` names a thread
this hook did not just create itself via `send()` (tracked via
`createdThreadIdRef`), dispatch `loadThreadMessages` +
`fetchAndHydrateTurnState` + `fetchAndHydrateTurnHistory` to rehydrate
the full transcript + turn state from the core. The `createdThreadIdRef`
guard avoids racing an in-flight turn when a caller echoes a
just-created thread id back in as `seedThreadId`.
Depends on B4 (#<B4 PR>) — rehydration reloads the FULL transcript
(including interim narration bubbles), so without B4's `displayMessages`
filter, those would render as bubbles again after a reload.
Split from tinyhumansai#4650.
…sses @coderabbitai on workflowCopilotThreads.ts:62) getCopilotThreadId/setCopilotThreadId touch localStorage with no logging, so a quota/private-mode failure (or a get/set miss) leaves no trace to debug why a copilot thread failed to persist/restore.
…l/replace Two related threadSlice.ts gaps surfaced by PR tinyhumansai#4663 review: - loadThreadMessages was the only read/write thunk in this file that did not special-case ThreadNotFound like addMessageLocal/addInferenceResponse/ generateThreadTitleIfNeeded already do — a deleted/purged thread (e.g. a stale id cached by workflowCopilotThreads.ts) just recorded messagesError with no eviction, so the stale thread lingered in Redux forever. Now it evicts via the same evictStaleThread() path and rejects with THREAD_NOT_FOUND_MESSAGE. - loadThreadMessages.fulfilled wholesale-replaced messagesByThreadId[id], which could clobber a message that landed locally (via a concurrent addMessageLocal/addInferenceResponse append) but predates this fetch's snapshot. useWorkflowBuilderChat's copilot rehydrate effect can race a WorkflowCopilotPanel auto-send (repairSeed/buildSeed) on the very same mount, so this is reachable, not just theoretical. Merge the fetched list with any local-only ids instead of replacing. (addresses @coderabbitai on app/src/hooks/useWorkflowBuilderChat.ts:213, foundation for the stale-thread-id fix on workflowCopilotThreads.ts:46)
@chatgpt-codex-connector on workflowCopilotThreads.ts:46) A cached thread id in workflowCopilotThreads.ts can point at a thread that was deleted/purged since it was cached. useWorkflowBuilderChat.send only creates a new thread when threadId is null, so the panel kept retrying the missing thread and surfacing an error instead of recovering. loadThreadMessages/addMessageLocal now reject with THREAD_NOT_FOUND_MESSAGE for this case (previous commit) — null out this hook's threadId in response, both on a not-found rehydrate (mount) and a not-found append (send). WorkflowCopilotPanel already reports every threadId change back up via onThreadIdChange, so this cascades into FlowCanvasPage clearing the stale workflowCopilotThreads.ts cache entry too, letting the next send create a fresh builder thread.
… cache (addresses @coderabbitai nitpick on workflowCopilotThreads.test.ts:63-75) CodeRabbit flagged that only getItem's throw path was exercised; setCopilotThreadId's own try/catch (write and remove) was never triggered by a failing setItem/removeItem.
graycyrus
force-pushed
the
feat/flows-copilot-chat-persistence
branch
from
4894b9f to
1085ffe
Compare
1 participant
Stacks on PR-B4 (#4662) — merge that first; this diff shows B4's changes until then.
The bug
The Flows Copilot chat is not persistent: reopening the panel, switching flows and back, or reloading the app always started the conversation empty. Two gaps:
workflowCopilotThreads.tscached each flow's copilot thread id in a module-levelMap— lost on any reload.useWorkflowBuilderChatnever re-fetched that thread's messages/turn state from the core on mount.messagesByThreadIdisn't in the redux-persist whitelist, so it starts empty on a fresh app load. The messages themselves are durable server-side (threadApi.appendMessagepersists every turn) — they just never got loaded back.The fix
workflowCopilotThreads.ts: back the flow → thread-id cache withlocalStorageinstead of an in-memoryMap, namespaced${userId}:copilot-thread:<flow>(the same${userId}:conventionuserScopedStorage/clearAllAppDatause elsewhere, [Bug] Auth state leaks across users on same device — user B sees user A's threads, accounts, notifications #900/Fix account switching causing data loss and forced re-onboarding after re-login #983) so an identity flip or a per-user "clear my data" can't leak/destroy another user's mapping. Wrapped in try/catch so private-mode/quota errors degrade to a no-op (copilot just starts a fresh thread) instead of throwing.useWorkflowBuilderChat: on mount, whenseedThreadIdnames a thread this hook did not just create itself viasend()(tracked via acreatedThreadIdRefguard), dispatchloadThreadMessages+fetchAndHydrateTurnState+fetchAndHydrateTurnHistoryto rehydrate the full transcript + turn state from the core (mirrorsConversations.tsx's thread-switch effect).createdThreadIdRefguard matters becauseWorkflowCopilotPanelreports everythreadIdchange back up viaonThreadIdChange, and a caller re-passes that straight back in asseedThreadIdon the next render — without the guard, a freshly-created thread's first render would look identical to "an existing persisted thread was just selected" and trigger a rehydrate that races the in-flight turn (loadThreadMessages.fulfilledreplaces the thread's message array wholesale, so a fetch resolving before the backend catches up would wipe the just-appended turn back out).Why this depends on B4
Rehydration reloads the thread's full transcript, including the between-tool narration bubbles tagged
isInterimin B4. Without B4'sdisplayMessagesfilter inWorkflowCopilotPanel, those interim messages would render as bubbles again every time the panel reopens/reloads — this PR's rehydration effect is only correct on top of B4.Verification
pnpm typecheck,eslint,format:check— clean on all changed files.vitest runon the touched test files — 35/35 passing (localStorage round-trip + user-scoping + reload simulation inworkflowCopilotThreads.test.ts; rehydration-dispatch +createdThreadIdRefrace-guard tests inuseWorkflowBuilderChat.test.ts).vitest runsuite: 8399 passed / 2 failed / 2 skipped. Both failures are pre-existing and unrelated to this diff:rpcMethods.test.ts's drift guard hitsENOENTbecause thevendor/tinychannelssubmodule isn't initialized in this worktree, andIntelligenceOrchestrationTab.test.tsxpasses cleanly in isolation (flaky under full-suite parallelism, unrelated to Flows/Copilot).Note
Split from #4650 (previously combined with B4 in one PR since they touched the same files).
Summary by CodeRabbit
New Features
Bug Fixes