fix(webview): isolate parallel provider view state#909
Open
easonLiangWorldedtech wants to merge 7 commits into
Open
fix(webview): isolate parallel provider view state#909easonLiangWorldedtech wants to merge 7 commits into
easonLiangWorldedtech wants to merge 7 commits into
Conversation
Add view-local state loading and syncing for parallel ClineProvider instances, with safe initialization and configuration listener handling for test/shim environments. Key changes: - Add viewId property for unique provider instance identification - Implement viewLocalState buffer to isolate mode, profile, and apiConfig per view - Load view-local state after provider dependencies are initialized - Merge viewLocalState in getState() with local apiConfiguration taking precedence - Clear local overrides when saveViewState receives undefined or null values - Sync mode/profile changes through saveViewState(), loadViewState(), and profile activation - Preserve restored task mode in both global state and viewLocalState - Guard global configuration listener setup when VS Code workspace events are unavailable - Keep sticky-mode task restore compatible with view-local state isolation Tests: - Add 22 parallel mode cases covering viewId uniqueness, state isolation, save/load behavior, merge precedence, local override clearing, config listener handling, mode switching, profile activation, and multi-instance isolation - Verify sticky-mode restore compatibility
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClineProvider now isolates mode and API profile state per view instance, synchronizes local state with shared context changes, and merges local overrides into ExtensionState. View-state identifiers flow through webview launch messages, and API configuration updates use the provider mutation path. ChangesParallel view state isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Webview
participant webviewMessageHandler
participant ClineProvider
participant ContextProxy
Webview->>webviewMessageHandler: webviewDidLaunch(viewStateId)
webviewMessageHandler->>ClineProvider: setViewStateId(viewStateId)
ClineProvider->>ContextProxy: loadViewState()
ContextProxy-->>ClineProvider: return persisted view-local state
Webview->>ClineProvider: switch mode or activate provider profile
ClineProvider->>ClineProvider: saveViewState()
ClineProvider->>ContextProxy: update shared state
ClineProvider-->>Webview: post merged ExtensionState
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 1118-1143: Update the mode-switch section of the test “should
handle mode switch in one instance without affecting others” to call
provider1.handleModeSwitch("architect") instead of the private saveViewState
method, while preserving the existing state assertions for both providers.
- Around line 829-857: Update the configuration-change test around ClineProvider
and setupGlobalStateListener to capture the registered onDidChangeConfiguration
handler, spy on loadViewState(), invoke the handler with configChangeEvent, and
assert that loadViewState() is called. Remove the unused event-only setup and
the weak disposables.length assertion.
In `@src/core/webview/ClineProvider.ts`:
- Around line 449-458: Update saveViewState() to persist each value through a
view-specific ContextProxy key derived from the current viewId, rather than the
shared ExtensionState key. Keep the existing viewLocalState cache update
behavior, and ensure corresponding view-state reads use the same derived key so
recreated views restore their own mode, profile, and configuration.
- Around line 1600-1602: Update the mode-switch flow around
saveViewState("mode", newMode) so the no-saved-configuration path reads the
profile configuration name from this view’s getState() result rather than shared
currentApiConfigName. Preserve the existing mode-switch behavior while ensuring
another tab’s global state cannot supply this view’s configuration.
- Around line 2674-2676: Update the state mutation paths around setMode(),
setValues(), profile upsert/deletion, and resetState() so local-key changes also
update or invalidate viewLocalState instead of being hidden by
mergedStateValues. Centralize the local-key mutation handling, and explicitly
clear viewLocalState during resetState() so reset returns current persisted
defaults rather than stale mode, profile, or configuration values.
- Around line 241-243: Update the viewId initialization in the ClineProvider
constructor to use a monotonically increasing instance identifier rather than
ClineProvider.activeInstances.size, ensuring IDs remain unique after instances
are disposed; keep activeInstances tracking unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b3c9918-8ed9-4d37-8d1e-80201da59ec5
📒 Files selected for processing (2)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 750-752: Update the view-state key construction used by
saveViewState and the related test expectation so it uses a stable per-view
identifier rather than the construction-order suffix from viewId. Ensure
restored tabs consistently read and write their own persisted mode and API
configuration, and remove reliance on the shared-key fallback for this state.
- Around line 862-877: Update loadViewState in ClineProvider to read
apiConfiguration from the view-specific persisted state key rather than shared
provider settings, preserving the tab’s model/provider configuration across
reloads. Add or adjust coverage in the parallel-mode tests by seeding per-view
apiConfiguration and asserting it is restored after loading, while keeping
existing failure behavior unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 530ae001-d224-4694-b647-70aef27ee0f4
📒 Files selected for processing (5)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/extension/__tests__/api-set-configuration.spec.tssrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/webview/ClineProvider.ts
github-actions
Bot
added
the
awaiting-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/core/webview/webviewMessageHandler.ts`:
- Around line 561-562: Update downstream state access in the webview message
handler to use view-local state after setViewStateId initializes the provider.
Replace the currentApiConfigName read near line 607 with the current value from
await provider.getState(), and replace both updateGlobalState("mode", ...) calls
near lines 2200 and 2296 with await provider.handleModeSwitch(...) so mode
changes apply to the active tab.
In `@webview-ui/src/App.tsx`:
- Line 194: Remove the duplicate webviewDidLaunch useEffect from App, keeping
the launch handshake in ExtensionStateContextProvider; update
webview-ui/src/App.tsx lines 194-194 and leave
webview-ui/src/context/ExtensionStateContext.tsx lines 492-492 unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 045a32de-dd60-4dec-9b66-4f1e3da5467f
📒 Files selected for processing (8)
packages/types/src/vscode-extension-host.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/App.tsxwebview-ui/src/__tests__/App.spec.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/utils/vscode.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/webview/ClineProvider.ts
- src/core/webview/tests/ClineProvider.parallelMode.spec.ts
Comment on lines
+561
to
+562
| await provider.setViewStateId(message.viewStateId) | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update downstream accesses to use view-local state.
While setViewStateId correctly initializes the view-local buffer, several downstream handlers in this file still bypass it by using getGlobalState or updateGlobalState for isolated fields (mode and currentApiConfigName). This causes them to interact with the shared fallback state rather than the current tab's isolated state.
Please update the following locations to use view-local methods:
- Line 607:
const currentConfigName = getGlobalState("currentApiConfigName")should use(await provider.getState()).currentApiConfigNameso it validates the local profile. - Lines 2200 & 2296:
updateGlobalState("mode", ...)should useawait provider.handleModeSwitch(...)so the current tab actually switches into the created/default mode.
🤖 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 `@src/core/webview/webviewMessageHandler.ts` around lines 561 - 562, Update
downstream state access in the webview message handler to use view-local state
after setViewStateId initializes the provider. Replace the currentApiConfigName
read near line 607 with the current value from await provider.getState(), and
replace both updateGlobalState("mode", ...) calls near lines 2200 and 2296 with
await provider.handleModeSwitch(...) so mode changes apply to the active tab.
|
|
||
| // Tell the extension that we are ready to receive messages. | ||
| useEffect(() => vscode.postMessage({ type: "webviewDidLaunch" }), []) | ||
| useEffect(() => vscode.postMessage({ type: "webviewDidLaunch", viewStateId: vscode.getViewStateId() }), []) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Duplicate webviewDidLaunch messages on startup.
Both App and ExtensionStateContextProvider broadcast the webviewDidLaunch message when they mount. Since App is wrapped by this context provider, the backend receives two concurrent launch messages on every reload, causing its full initialization sequence (fetching custom modes, themes, API profiles, MCP servers) to execute twice.
Please remove the duplicate call from one of the files:
webview-ui/src/App.tsx#L194-L194: Remove thisuseEffectif the context provider is responsible for the launch handshake.webview-ui/src/context/ExtensionStateContext.tsx#L492-L492: Alternatively, remove thisuseEffectifAppshould drive the handshake.
📍 Affects 2 files
webview-ui/src/App.tsx#L194-L194(this comment)webview-ui/src/context/ExtensionStateContext.tsx#L492-L492
🤖 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 `@webview-ui/src/App.tsx` at line 194, Remove the duplicate webviewDidLaunch
useEffect from App, keeping the launch handshake in
ExtensionStateContextProvider; update webview-ui/src/App.tsx lines 194-194 and
leave webview-ui/src/context/ExtensionStateContext.tsx lines 492-492 unchanged.
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
2 participants
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
Fixes multi-tab mode/model settings being overwritten by global state from other tabs. Each ClineProvider instance (sidebar, editor tab) now maintains its own isolated
mode,apiConfiguration, and profile selection via a per-view local state buffer with view-specific persistence keys.Problem
In multi-tab mode, when users switch between Roo Code tabs with different mode/API configuration selections, the active tabs settings get overwritten by the global state from another tab. This happens because all tabs share the same ContextProxy singleton — any
handleModeSwitch()oractivateProviderProfile()call in one tab updates shared global state that other tabs then read.Solution
Implemented view-local state isolation using a per-provider-instance buffer with view-specific persistence keys:
Architecture
nextViewIdstatic counter for stable, monotonically increasing view IDs ({renderContext}-{counter})viewLocalState: Partial<ExtensionState>buffer per provider instance__view_state_{viewId}_{key}so recreated views restore their own values instead of the last writer shared stategetState()to mergeviewLocalStateon top of global state with proper precedenceKey Changes in
src/core/webview/ClineProvider.ts:loadViewState()__view_state_{id}_mode), falls back to shared keys for backward compatibilitysaveViewState(key, value)_updateViewLocalStateFromMutation(values)setValues(),setValue(), profile upsert/activation/deletion, orresetState()setupGlobalStateListener()handleModeSwitch()_updateViewLocalStateFromMutation+ reads currentApiConfigName fromgetState()(not direct global read) beforeupdateGlobalState()activateProviderProfile()/deleteProviderProfile()_updateViewLocalStateFromMutationfor consistent synccreateTaskWithHistoryItem()setValue()/setValues()_updateViewLocalStateFromMutationresetState()_clearViewLocalState()after resetting ContextProxyView-Specific Persistence Keys:
Merge Precedence in
getState():Additional Changes (beyond ClineProvider)
src/extension/api.tsAPI.setConfiguration()now callssidebarProvider.setValues(values)instead of directly callingcontextProxy.setValues(), ensuring_updateViewLocalStateFromMutationis triggeredwebview-ui/src/utils/vscode.tsviewStateIdto webview launch handshake — each tab gets a unique ID stored in ContextProxy for cross-session persistencewebview-ui/src/App.tsx/ExtensionStateContext.tsxviewStateIdfrom extension host contextTesting
22+ new unit tests across multiple test files:
ClineProvider.parallelMode.spec.ts— viewId uniqueness, mode/apiConfig isolation, saveViewState/loadViewState with undefined clearing, getState merge precedence (local overrides global), GlobalState listener handling, handleModeSwitch/activateProviderProfile integration, multi-instance isolation (3+ concurrent providers)api-set-configuration.spec.ts— verifiesAPI.setConfiguration()triggers_updateViewLocalStateFromMutationand propagates to view-local stateRegression: All 2179 existing core tests pass ✅
Related Issue
Closes #908
Summary by CodeRabbit
New Features
viewStateIdso view state can be reliably correlated.Bug Fixes
Tests
viewStateIdpropagation, and configuration-state merging.