fix: prevent chat history loss during cloud/settings navigation (#11371)#11372
Conversation
- Add postStateToWebviewWithoutClineMessages() for cloud event handlers - Add clineMessagesSeq counter to reject stale state pushes - Fix orphaned messageUpdated handler to append instead of drop - Add 6 unit tests for seq-based clineMessages protection
Reviewed the incremental change (1 file, 1 commit). The latest commit reverts the orphaned Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
daniel-lxs
left a comment
There was a problem hiding this comment.
Thanks for the thorough analysis and well-structured fix. Layers 1 and 2 make a lot of sense — separating cloud state pushes from clineMessages (via postStateToWebviewWithoutClineMessages()) addresses the root cause, and the sequence numbering provides solid defense-in-depth.
Question about Layer 3 (orphaned messageUpdated append):
The change to append orphaned messages in the messageUpdated handler (instead of silently dropping) concerns me:
-
If layers 1+2 work correctly, this path should never be hit. Cloud events no longer push stale
clineMessages, and the seq guard rejects out-of-order pushes. What scenario would still trigger this recovery path? -
Blind appending risks incorrect UI state.
messageUpdatedis designed for updating existing messages (e.g., streaming partial content). If a message's timestamp isn't found, appending it at the end places it out of chronological order. While a subsequent full state push would self-correct, there's a visible window of broken state for the user. -
The silent drop was a useful signal. If
messageUpdatedarrives for a nonexistent timestamp, that indicates something unexpected happened. Silently recovering hides that signal and could mask future bugs. -
No test coverage for this specific behavior change. The new tests cover the seq guard logic but not the append-on-miss path.
Could you walk us through the specific scenario where layers 1+2 are working but a messageUpdated still arrives for a missing message? If there's a real case, would it make sense to at least add a console.warn so the recovery is observable?
Address reviewer feedback: blind append risks incorrect UI ordering. With layers 1+2 working, this path should rarely trigger. Replace append with console.warn to preserve debugging signal.
|
Thanks for the thorough review @daniel-lxs! You raise excellent points about Layer 3. You're absolutely right on all counts. I've updated the PR in commit 5fba2ee to address your feedback:
The silent drop was indeed a useful signal — now it's an explicit, observable signal rather than a fully silent one. |
…) (#11372) Co-authored-by: Sannidhya <sann@Sannidhyas-MacBook-Pro.local>
…) (#11372) Co-authored-by: Sannidhya <sann@Sannidhyas-MacBook-Pro.local>
…) (#11372) Co-authored-by: Sannidhya <sann@Sannidhyas-MacBook-Pro.local>
* fix: cancel backend auto-approval timeout when auto-approve is toggled off mid-countdown (#11439) Co-authored-by: Sannidhya <sann@Sannidhyas-MacBook-Pro.local> * fix: prevent chat history loss during cloud/settings navigation (#11371) (#11372) Co-authored-by: Sannidhya <sann@Sannidhyas-MacBook-Pro.local> * fix: preserve pasted images in chatbox during chat activity (#11375) Co-authored-by: Roo Code <roomote@roocode.com> * fix: resolve chat scroll anchoring and task-switch scroll race condit… (#11385) * fix: avoid zsh process-substitution false positives in assignments (#11365) * fix(editor): make tab close best-effort in DiffViewProvider.open (#11363) * fix(checkpoints): canonicalize core.worktree comparison to prevent Windows path mismatch failures (#11346) * fix: prevent double notification sound playback (#11283) * fix: prevent false unsaved changes prompt with OpenAI Compatible headers (#8230) (#11334) fix: prevent false unsaved changes prompt with OpenAI Compatible headers Mark automatic header syncs in ApiOptions and OpenAICompatible as non-user actions (isUserAction: false) and enhance SettingsView change detection to skip automatic syncs with semantically equal values. Root cause: two components (ApiOptions and OpenAICompatible) manage openAiHeaders state and automatically sync it back on mount/remount. These syncs were treated as user changes, triggering a false dirty state. Co-authored-by: Robert McIntyre <robertjmcintyre@users.noreply.github.com> * fix: remove noisy console.warn logs from NativeToolCallParser (#11264) Remove two console.warn messages that fire excessively when loading tasks from history: - 'Attempting to finalize unknown tool call' in finalizeStreamingToolCall() - 'Received chunk for unknown tool call' in processStreamingChunk() The defensive null-return behavior is preserved; only the log output is removed. * refactor: remove footgun prompting (file-based system prompt override) (#11387) * refactor: delete orphaned per-provider caching transform files (#11388) * feat: add disabledTools setting to globally disable native tools (#11277) * feat: add disabledTools setting to globally disable native tools Add a disabledTools field to GlobalSettings that allows disabling specific native tools by name. This enables cloud agents to be configured with restricted tool access. Schema: - Add disabledTools: z.array(toolNamesSchema).optional() to globalSettingsSchema - Add disabledTools to organizationDefaultSettingsSchema.pick() - Add disabledTools to ExtensionState Pick type Prompt generation (tool filtering): - Add disabledTools to BuildToolsOptions interface - Pass disabledTools through filterSettings to filterNativeToolsForMode() - Remove disabled tools from allowedToolNames set in filterNativeToolsForMode() Execution-time validation (safety net): - Extract disabledTools from state in presentAssistantMessage - Convert disabledTools to toolRequirements format for validateToolUse() Wiring: - Add disabledTools to ClineProvider getState() and getStateToPostToWebview() - Pass disabledTools to all buildNativeToolsArrayWithRestrictions() call sites EXT-778 * fix: check toolRequirements before ALWAYS_AVAILABLE_TOOLS Moves the toolRequirements check before the ALWAYS_AVAILABLE_TOOLS early-return in isToolAllowedForMode(). This ensures disabledTools can block always-available tools (switch_mode, new_task, etc.) at execution time, making the validation layer consistent with the filtering layer. * feat: add support for .agents/skills directory (#11181) * feat: add support for .agents/skills directory This change adds support for discovering skills from the .agents/skills directory, following the Agent Skills convention for sharing skills across different AI coding tools. Priority order (later entries override earlier ones): 1. Global ~/.agents/skills (shared across AI coding tools, lowest priority) 2. Project .agents/skills 3. Global ~/.roo/skills (Roo-specific) 4. Project .roo/skills (highest priority) Changes: - Add getGlobalAgentsDirectory() and getProjectAgentsDirectoryForCwd() functions to roo-config - Update SkillsManager.getSkillsDirectories() to include .agents/skills - Update SkillsManager.setupFileWatchers() to watch .agents/skills - Add tests for new functionality * fix: clarify skill priority comment to match actual behavior * fix: clarify skill priority comment to explain Map.set replacement mechanism --------- Co-authored-by: Roo Code <roomote@roocode.com> * feat(history): render nested subtasks as recursive tree (#11299) * feat(history): render nested subtasks as recursive tree * fix(lockfile): resolve missing ai-sdk provider entry * fix: address review feedback — dedupe countAll, increase SubtaskRow max-h - HistoryView: replace local countAll with imported countAllSubtasks from types.ts - SubtaskRow: increase nested children max-h from 500px to 2000px to match TaskGroupItem * perf(refactor): consolidate getState calls in resolveWebviewView (#11320) * perf(refactor): consolidate getState calls in resolveWebviewView Replace three separate this.getState().then() calls with a single await this.getState() and destructuring. This avoids running the full getState() method (CloudService calls, ContextProxy reads, etc.) three times during webview view resolution. * fix: keep getState consolidation non-blocking to avoid delaying webview render --------- Co-authored-by: daniel-lxs <ricciodaniel98@gmail.com> * fix: harden command auto-approval against inline JS false positives (#11382) * feat: rename search_and_replace tool to edit and unify edit-family UI (#11296) * Revert "refactor: delete orphaned per-provider caching transform files (#11388)" This reverts commit 13a45b0. * chore: regenerate built-in-skills.ts with updated formatting * fix: add missing maxReadFileLine property to test baseState The ExtensionState type now requires maxReadFileLine property (added in commit 63e3f76). Update the test to include this property with the default value of -1 (unlimited reading). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * feat: add pnpm serve command for code-server development (#10964) Co-authored-by: Roo Code <roomote@roocode.com> * chore: remove Feature Request from issue template options (#11141) Co-authored-by: Roo Code <roomote@roocode.com> * refactor(docs-extractor): simplify mode to focus on raw fact extraction (#11129) * Add cli support for linux (#11167) * fix: replace heredocs with echo statements in cli-release workflow (#11168) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * Drop MacOS-13 cli support (#11169) * fix(cli): correct example in install script (#11170) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * feat: add Kimi K2.5 model to Fireworks provider (#11177) * feat(cli): improve dev experience and roo provider API key support (#11203) - Allow --api-key and ROO_API_KEY env var for the roo provider instead of requiring cloud auth token - Switch dev/start scripts to use tsx for running directly from source without building first - Fix path resolution (version.ts, extension.ts, extension-host.ts) to work from both source and bundled locations - Disable debug log file (~/.roo/cli-debug.log) unless --debug is passed - Update README with complete env var table and dev workflow docs Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * Roo Code CLI v0.0.50 (#11204) * Roo Code CLI v0.0.50 * docs(cli): add --exit-on-error to changelog --------- Co-authored-by: Roo Code <roomote@roocode.com> * feat(cli): update default model from Opus 4.5 to Opus 4.6 (#11273) Co-authored-by: Roo Code <roomote@roocode.com> * feat(web): replace Roomote Control with Linear Integration in cloud features grid (#11280) Co-authored-by: Roo Code <roomote@roocode.com> * Add linux-arm64 for the roo cli (#11314) * chore: clean up repo-facing mode rules (#11410) * Make CLI auto-approve by default with require-approval opt-in (#11424) Co-authored-by: Roo Code <roomote@roocode.com> * Add new code owners to CODEOWNERS file * Update next.js (#11108) * feat(web): Replace bespoke navigation menu with shadcn navigation menu (#11117) Co-authored-by: Roo Code <roomote@roocode.com> --------- Co-authored-by: SannidhyaSah <sah_sannidhya@outlook.com> Co-authored-by: Sannidhya <sann@Sannidhyas-MacBook-Pro.local> Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com> Co-authored-by: Roo Code <roomote@roocode.com> Co-authored-by: Hannes Rudolph <hrudolph@gmail.com> Co-authored-by: 0xMink <dennis@dennismink.com> Co-authored-by: Robert McIntyre <robertjmcintyre@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Matt Rubens <mrubens@users.noreply.github.com> Co-authored-by: Chris Estreich <cestreich@gmail.com>
Related GitHub Issue
Closes: #11371
Roo Code Task Context (Optional)
No Roo Code task context for this PR
Description
This PR fixes a race condition where chat history messages are lost (rolled back to an earlier state) when navigating from the Chat view to the Cloud/Settings pages while a task is actively running.
Root cause: Multiple cloud event handlers (
authStateChangedHandler,settingsUpdatedHandler,userInfoHandler) andsyncCloudProfiles()all callpostStateToWebview(), which asynchronously capturesclineMessagesfrom the current task. When a cloud-triggered snapshot captures staleclineMessageswhile the task is actively streaming newer ones, and the stale state arrives at the frontend after a newer task-triggered state,mergeExtensionState()overwrites the newer messages with the stale snapshot via last-writer-wins semantics.Three-layer fix:
Primary fix — Cloud events no longer push
clineMessages: AddedpostStateToWebviewWithoutClineMessages()method that omits bothclineMessagesandtaskHistoryfrom state pushes. Cloud event handlers and the custom modes manager callback now use this method, since they update cloud/mode state and have no business touching chat messages. This follows the existingpostStateToWebviewWithoutTaskHistory()pattern already in the codebase.Defense in depth — Sequence numbering: Added a monotonically increasing
clineMessagesSeqcounter that is included in state pushes that carryclineMessages. The frontend'smergeExtensionState()now rejectsclineMessagesfrom state pushes whose sequence number is not strictly greater than the last applied one. This protects against any remaining concurrentpostStateToWebview()races.Resilience — Orphaned
messageUpdatedrecovery: ThemessageUpdatedhandler previously silently dropped updates for messages whose timestamp was not found in the frontend'sclineMessagesarray. Now it appends orphaned messages instead, providing a recovery path for messages that exist in the backend but are missing from the frontend due to prior state drift.Test Procedure
Unit tests (automated):
cd webview-ui && npx vitest run src/context/__tests__/ExtensionStateContext.spec.tsx— 15 tests pass (9 existing + 6 new)Backend tests (automated):
cd src && npx vitest run core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts— 4 passcd src && npx vitest run core/webview/__tests__/ClineProvider.taskHistory.spec.ts— 15 passcd src && npx vitest run core/webview/__tests__/ClineProvider.sticky-mode.spec.ts— 19 passcd src && npx vitest run core/task/__tests__/Task.spec.ts— 38 pass, 4 skippedType checking: All 14
check-typestasks pass across the entire monorepoLinting: All 14
linttasks pass across the entire monorepoManual reproduction test:
Pre-Submission Checklist
Screenshots / Videos
No UI changes in this PR — this is an internal state management fix
Documentation Updates
Additional Notes
clineMessagesSeqis an optional field. State pushes without it (from old code paths or tests) are handled gracefully — backward compatibility is fully preserved.cloudIsAuthenticated,cloudUserInfo,cloudOrganizations,sharingEnabled, etc.) are still pushed by cloud event handlers. OnlyclineMessagesandtaskHistoryare omitted from those pushes.postStateToWebviewWithoutClineMessages()follows the same pattern as the existingpostStateToWebviewWithoutTaskHistory()method.Get in Touch
@SannidhyaSah
Important
Fixes chat history loss by preventing stale state overwrites through sequence numbering and refined state updates in cloud event handling.
postStateToWebviewWithoutClineMessages()inClineProvider.tsto prevent cloud events from pushingclineMessagesandtaskHistory.clineMessagesSeq) inClineProvider.tsto ensure only newerclineMessagesare applied inmergeExtensionState()inExtensionStateContext.tsx.messageUpdatedhandler inExtensionStateContext.tsxto append orphaned messages.ExtensionStateContext.spec.tsxto verify sequence number logic and state merging behavior.clineMessages.extension.tsto usepostStateToWebviewWithoutClineMessages()for cloud event handlers.clineMessagesSeqtoExtensionStateinvscode-extension-host.ts.This description was created by
for 5fba2ee. You can customize this summary. It will automatically update as commits are pushed.