fix(provider): isolate profile mutations from focused tasks - #1087
Conversation
📝 WalkthroughWalkthroughThe change persists mode-specific API configurations, serializes provider profile mutations, supports targeted mode switching without unnecessary task updates, and expands tests for concurrency, recovery, state notifications, configuration persistence, and profile restoration. ChangesProfile and mode configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant ClineProvider
participant ProviderSettingsManager
participant Task
participant Webview
API->>ProviderSettingsManager: Set mode-specific configuration
API->>Webview: Post synchronized configuration state
ClineProvider->>ClineProvider: Queue profile or mode mutation
ClineProvider->>ProviderSettingsManager: Activate profile
ProviderSettingsManager-->>ClineProvider: Return profile data
ClineProvider->>Task: Update targeted task when enabled
ClineProvider->>Webview: Post state and events when not suppressed
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/vscode-e2e/src/suite/subtasks.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/webview/ClineProvider.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
4daeec7 to
57e9032
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
4a976b5 to
bb936be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/webview/ClineProvider.ts (1)
1799-1853: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
contextProxywrites are not gated byskipCurrentTaskRebuild.
activateProviderProfileUnlockedskips task rebuild (line 1838), sticky-history persistence (lines 1842-1844),postStateToWebview(lines 1846-1848), and theProviderProfileChangedemit (lines 1850-1852) whenskipCurrentTaskRebuildis true. But the earlier, unconditional block still runs:await Promise.all([ this.contextProxy.setValue("listApiConfigMeta", await this.providerSettingsManager.listConfig()), this.contextProxy.setValue("currentApiConfigName", name), this.contextProxy.setProviderSettings(providerSettings), ])This writes the pending-child profile's
currentApiConfigNameand provider settings into the sharedcontextProxyregardless ofskipCurrentTaskRebuild. Any other consumer that readsgetState().apiConfiguration/currentApiConfigNameafterward (a different code path callingpostStateToWebview, or a brand-new task readinggetState()for its own configuration) will observe the pending child's profile instead of the still-focused task's actual profile. This directly contradicts the stated goal of preparing a pending child "without rebuilding, reconfiguring, or posting the currently focused task" — the focused task object is left alone, but the shared "current" configuration state is not.The test in
ClineProvider.apiHandlerRebuild.spec.ts(lines 475-513) does not assert oncontextProxy/getState().apiConfigurationafterhandleModeSwitch(..., null), so this gap is not currently caught.Gate this block on
skipCurrentTaskRebuildtoo, or persist the pending-child profile through a mechanism that does not overwrite the globally-visible "current" configuration.🤖 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/ClineProvider.ts` around lines 1799 - 1853, Gate the contextProxy writes in activateProviderProfileUnlocked behind !skipCurrentTaskRebuild so pending-child activation does not overwrite the globally visible current configuration. Preserve the existing writes for normal activation, including listApiConfigMeta, currentApiConfigName, and providerSettings.
🤖 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/ClineProvider.ts`:
- Around line 199-229: Update enqueueProviderProfileMutation and
withProviderProfileMutationTimeout so a timed-out mutation cannot permanently
block provider-profile or mode-switch operations. Log when the timeout fires,
and attach settlement logging to the abandoned raw run if it later resolves or
rejects. Advance or reset providerProfileMutationQueue through a bounded
recovery path after timeout while preserving serialized execution for active
mutations.
- Around line 1532-1543: Defer resolving the default task in handleModeSwitch
until the queued mutation executes, rather than using the parameter default on
handleModeSwitch. Preserve explicit Task and null arguments, while resolving
undefined via getCurrentTask inside the closure passed to
enqueueProviderProfileMutation before calling handleModeSwitchUnlocked.
---
Outside diff comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 1799-1853: Gate the contextProxy writes in
activateProviderProfileUnlocked behind !skipCurrentTaskRebuild so pending-child
activation does not overwrite the globally visible current configuration.
Preserve the existing writes for normal activation, including listApiConfigMeta,
currentApiConfigName, and providerSettings.
🪄 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: 20f79951-58fb-470c-ae35-d458672683e1
📒 Files selected for processing (6)
apps/vscode-e2e/src/suite/subtasks.test.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tssrc/core/webview/__tests__/ClineProvider.lockApiConfig.spec.tssrc/extension/__tests__/api-configuration.spec.tssrc/extension/api.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.apiHandlerRebuild.spec.ts`:
- Around line 528-529: Replace the double assertions in the firstTask and
secondTask _taskMode expectations with TypeScript bracket-notation access,
preserving the existing assertions and Mode typing. If bracket notation is not
viable, retain the casts only with a concise comment explaining why they are
necessary as a last resort.
🪄 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: d6739a34-d626-4b7a-b15b-fba2aefc8564
📒 Files selected for processing (2)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/webview/ClineProvider.ts
5ddb92c to
6a03068
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/webview/ClineProvider.ts (2)
206-238: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not continue child creation while a timed-out mutation is still running.
The caller promise rejects after 30 seconds, but
runcontinues.delegateParentAndOpenChildcatches thehandleModeSwitcherror and creates the child immediately. If the raw mutation later activates a profile,getCurrentTask()can return the new child. The late mutation can then rebuild its API handler, update sticky history, and post state.Wait for the raw mutation to settle before creating the child, or stop and roll back the child flow on timeout.
🤖 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/ClineProvider.ts` around lines 206 - 238, Update the child-creation flow around delegateParentAndOpenChild so a timeout from handleModeSwitch does not immediately continue to child creation while the underlying provider profile mutation is still running. Await the raw mutation’s settlement before proceeding, or abort and roll back the child flow on timeout, ensuring late mutation side effects cannot affect the newly created child.
198-208: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute
upsertProviderProfilethrough the provider-profile queue.
providerProfileMutationQueueis an instance field, while shared webview commands select a visibleClineProviderviaClineProvider.getVisibleInstance().handleModeSwitchandactivateProviderProfileserialize through that field, butupsertProviderProfilepersists settings before activation, so anupsertApiConfigurationcall can overlap with another instance’s mode/profile activation. Move serialization beyondupsertProviderProfileor reuse shared serialization state.🤖 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/ClineProvider.ts` around lines 198 - 208, The upsertProviderProfile flow must use the same provider-profile mutation serialization as handleModeSwitch and activateProviderProfile, including persistence before activation. Update upsertProviderProfile to route its full mutation through enqueueProviderProfileMutation on the selected visible ClineProvider, or otherwise reuse shared serialization state so calls across provider instances cannot overlap.
🧹 Nitpick comments (1)
src/extension/__tests__/api-configuration.spec.ts (1)
50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed test double or document the cast.
Lines [56-57] coerce partial objects into concrete production types with double assertions. Use a typed test double. If the constructor type makes the assertion unavoidable, add a comment that explains why.
As per coding guidelines, use precise test doubles and explain any unavoidable double assertion.
🤖 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/extension/__tests__/api-configuration.spec.ts` around lines 50 - 57, Update the test setup around the provider and outputChannel fixtures to use typed test doubles matching the required ClineProvider and vscode.OutputChannel shapes instead of unexplained double assertions. If the constructor contract makes a double assertion unavoidable, retain it only with a concise comment documenting why it is necessary.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.
Outside diff comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 206-238: Update the child-creation flow around
delegateParentAndOpenChild so a timeout from handleModeSwitch does not
immediately continue to child creation while the underlying provider profile
mutation is still running. Await the raw mutation’s settlement before
proceeding, or abort and roll back the child flow on timeout, ensuring late
mutation side effects cannot affect the newly created child.
- Around line 198-208: The upsertProviderProfile flow must use the same
provider-profile mutation serialization as handleModeSwitch and
activateProviderProfile, including persistence before activation. Update
upsertProviderProfile to route its full mutation through
enqueueProviderProfileMutation on the selected visible ClineProvider, or
otherwise reuse shared serialization state so calls across provider instances
cannot overlap.
---
Nitpick comments:
In `@src/extension/__tests__/api-configuration.spec.ts`:
- Around line 50-57: Update the test setup around the provider and outputChannel
fixtures to use typed test doubles matching the required ClineProvider and
vscode.OutputChannel shapes instead of unexplained double assertions. If the
constructor contract makes a double assertion unavoidable, retain it only with a
concise comment documenting why it is necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95228718-d72d-42a4-a06d-609b7db9a03b
📒 Files selected for processing (3)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tssrc/extension/__tests__/api-configuration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/webview/tests/ClineProvider.apiHandlerRebuild.spec.ts
71ba421 to
18df117
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/webview/ClineProvider.ts (1)
1813-1817: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe flag name does not describe its effect here.
skipCurrentTaskRebuildsuppresses sticky-profile persistence and task-history writes in this method. No rebuild happens here. Consider a name such asskipCurrentTaskUpdates, which describes both this method andupdateTaskApiHandlerIfNeeded.🤖 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/ClineProvider.ts` around lines 1813 - 1817, Rename the options flag in persistStickyProviderProfileToCurrentTask and all call sites from skipCurrentTaskRebuild to skipCurrentTaskUpdates, preserving its behavior of skipping sticky-profile persistence and task-history updates; use the same name consistently with updateTaskApiHandlerIfNeeded.src/extension/__tests__/api-configuration.spec.ts (1)
45-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared provider fixture.
Both tests build the same provider double and output channel. Extract a factory to remove the duplication and keep each test focused on its assertions.
♻️ Proposed refactor
+const createHarness = () => { + const setValues = vi.fn().mockResolvedValue(undefined) + const saveConfig = vi.fn().mockResolvedValue("default-id") + const setModeConfig = vi.fn().mockResolvedValue(undefined) + const postStateToWebview = vi.fn().mockResolvedValue(undefined) + // Double assertion: the tests only exercise the small provider surface used by + // API.setConfiguration, so a full ClineProvider instance is not required. + const provider = { + context: {}, + on: vi.fn(), + contextProxy: { setValues }, + providerSettingsManager: { saveConfig, setModeConfig }, + postStateToWebview, + } as unknown as ClineProvider + const outputChannel = { appendLine: vi.fn() } as unknown as vscode.OutputChannel + return { api: new API(outputChannel, provider), setValues, saveConfig, setModeConfig, postStateToWebview } +}Then use
const { api, setModeConfig, postStateToWebview } = createHarness()in each test.🤖 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/extension/__tests__/api-configuration.spec.ts` around lines 45 - 64, Extract the duplicated provider and output-channel setup from the tests into a shared createHarness factory that returns api, setModeConfig, and postStateToWebview. Update each test to initialize these values through createHarness while preserving the existing mocks and assertions.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.
Inline comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 236-243: Ensure the queue comment matches the actual mutation
behavior: either add abort-signal checks immediately before every state write in
handleModeSwitchUnlocked, activateProviderProfileUnlocked, and
upsertProviderProfile (including writes after awaits), or weaken the comment to
state only the guarantee currently provided by advancing from callerResult.
Preserve the existing timeout and queue semantics.
- Around line 1867-1882: Update the profile activation flow around
activateProfile so skipCurrentTaskRebuild also prevents synchronizing the active
profile into ContextProxy. Either call
providerSettingsManager.activateProfile(args) only when rebuilding, or provide
an equivalent preparation-only path that avoids persisting currentApiConfigName;
preserve the existing synchronization behavior when skipCurrentTaskRebuild is
false.
---
Nitpick comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 1813-1817: Rename the options flag in
persistStickyProviderProfileToCurrentTask and all call sites from
skipCurrentTaskRebuild to skipCurrentTaskUpdates, preserving its behavior of
skipping sticky-profile persistence and task-history updates; use the same name
consistently with updateTaskApiHandlerIfNeeded.
In `@src/extension/__tests__/api-configuration.spec.ts`:
- Around line 45-64: Extract the duplicated provider and output-channel setup
from the tests into a shared createHarness factory that returns api,
setModeConfig, and postStateToWebview. Update each test to initialize these
values through createHarness while preserving the existing mocks and assertions.
🪄 Autofix
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: 721d5e82-8315-4bce-b1ae-67f37a4459d1
📒 Files selected for processing (8)
apps/vscode-e2e/src/suite/subtasks.test.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tssrc/core/webview/__tests__/ClineProvider.lockApiConfig.spec.tssrc/core/webview/__tests__/ClineProvider.sticky-mode.spec.tssrc/eslint-suppressions.jsonsrc/extension/__tests__/api-configuration.spec.tssrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/eslint-suppressions.json
- src/extension/api.ts
- apps/vscode-e2e/src/suite/subtasks.test.ts
- src/core/webview/tests/ClineProvider.sticky-mode.spec.ts
- src/core/webview/tests/ClineProvider.lockApiConfig.spec.ts
- src/core/webview/tests/ClineProvider.apiHandlerRebuild.spec.ts
| // Advance from the timeout-bounded result. Each fn checks its AbortSignal before | ||
| // writing state, so advancing the queue on timeout cannot produce stale overwrites. | ||
| this.providerProfileMutationQueue = callerResult.then( | ||
| () => undefined, | ||
| () => undefined, | ||
| ) | ||
| return callerResult | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The comment claims a stronger guarantee than the code provides.
The queue advances from callerResult, so after a 30-second timeout a new mutation starts while the abandoned mutation may still run. The comment states that each fn checks its AbortSignal before writing state. The current implementations do not meet that condition:
handleModeSwitchUnlockedwrites task history,_taskMode,mode, and emitsModeChanged(lines 1588-1612) before any abort check.handleModeSwitchUnlockedwriteslistApiConfigMeta(line 1632) and callssetModeConfig(line 1668) after the last abort check, with several awaits in between.activateProviderProfileUnlockedperforms all context and profile writes after its single check at line 1869.upsertProviderProfileperforms the whole activation block (lines 1747-1777) after its single check at line 1745.
An abandoned mutation can therefore overwrite state written by the mutation that followed it in the queue. The caller also receives a rejection while the write can still land later, so the reported outcome and the persisted state can disagree.
Add an abort check immediately before each state write, or route writes through a helper that returns early when the signal is aborted. Alternatively, weaken the comment to describe the actual guarantee.
🤖 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/ClineProvider.ts` around lines 236 - 243, Ensure the queue
comment matches the actual mutation behavior: either add abort-signal checks
immediately before every state write in handleModeSwitchUnlocked,
activateProviderProfileUnlocked, and upsertProviderProfile (including writes
after awaits), or weaken the comment to state only the guarantee currently
provided by advancing from callerResult. Preserve the existing timeout and queue
semantics.
| const { name, id, ...providerSettings } = await this.providerSettingsManager.activateProfile(args) | ||
|
|
||
| if (signal?.aborted) return | ||
|
|
||
| const persistModeConfig = options?.persistModeConfig ?? true | ||
| const persistTaskHistory = options?.persistTaskHistory ?? true | ||
|
|
||
| // See `upsertProviderProfile` for a description of what this is doing. | ||
| await Promise.all([ | ||
| this.contextProxy.setValue("listApiConfigMeta", await this.providerSettingsManager.listConfig()), | ||
| this.contextProxy.setValue("currentApiConfigName", name), | ||
| this.contextProxy.setProviderSettings(providerSettings), | ||
| ]) | ||
| const skipCurrentTaskRebuild = options?.skipCurrentTaskRebuild ?? false | ||
|
|
||
| if (!skipCurrentTaskRebuild) { | ||
| // See `upsertProviderProfile` for a description of what this is doing. | ||
| await Promise.all([ | ||
| this.contextProxy.setValue("listApiConfigMeta", await this.providerSettingsManager.listConfig()), | ||
| this.contextProxy.setValue("currentApiConfigName", name), | ||
| this.contextProxy.setProviderSettings(providerSettings), | ||
| ]) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'ProviderSettingsManager.ts' --exec ast-grep outline {} --items all
fd -t f 'ProviderSettingsManager.ts' --exec rg -n -C 25 'activateProfile' {}Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3025
Avoid synchronizing the store when skipCurrentTaskRebuild is true.
activateProfile writes currentApiConfigName to providerProfiles; this path then leaves ContextProxy.currentApiConfigName unchanged. Call providerSettingsManager.activateProfile(args) only in the non-skipped branch, or do not persist the active profile in activateProfile for this preparation-only path.
🤖 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/ClineProvider.ts` around lines 1867 - 1882, Update the
profile activation flow around activateProfile so skipCurrentTaskRebuild also
prevents synchronizing the active profile into ContextProxy. Either call
providerSettingsManager.activateProfile(args) only when rebuilding, or provide
an equivalent preparation-only path that avoids persisting currentApiConfigName;
preserve the existing synchronization behavior when skipCurrentTaskRebuild is
false.
Summary
PR 2 of 3 for #369, extracted from #1046.
modeApiConfigsmappings throughProviderSettingsManager.setModeConfig().This prepares safe provider mode/profile handling for future concurrent delegation, but does not enable concurrency or fan-out. #1085 is the complementary task-local isolation extraction.
Validation
pnpm --dir src exec vitest run core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tspnpm --dir src exec vitest run extension/__tests__/api-configuration.spec.tspnpm --dir src exec tsc --noEmit -p tsconfig.jsonSummary by CodeRabbit