feat(api): add task control and global state APIs - #1185
feat(api): add task control and global state APIs#1185easonLiangWorldedtech wants to merge 3 commits into
Conversation
- Export VSCodeAPIWrapper class and add getViewStateId() method - Generate unique viewStateId using crypto.randomUUID() with fallback - Persist viewStateId in localStorage for dev server compatibility - Add viewStateSchema to global-settings.ts for type safety - Send viewStateId during webviewDidLaunch handshake
- Add setValues method to update view-local state without affecting global settings - Persist mode selection per viewStateId for tab isolation - Update webviewMessageHandler to support new view state flow - Add parallel mode switching tests for sidebar and tab panel
📝 WalkthroughWalkthroughThis PR adds durable per-view state for parallel webviews, propagates stable view identifiers through launch messages, adds task-control APIs, improves provider synchronization, and strengthens message-handler typing and error handling with expanded test coverage. ChangesPer-view state and launch flow
Task control and message handling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/extension/__tests__/api-set-configuration.spec.ts (1)
26-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test double with the declared type.
ProviderDoubleextendsEventEmitter, but the object literal at Line 46-57 is a plain object cast withas ProviderDouble. The cast holds only becauseregisterListenerscalls the stubbedon.src/extension/__tests__/api-task-control.spec.tsLine 74 builds a realEventEmitterinstead. Use the same pattern here so a future event-driven assertion does not fail on a missing emitter.🤖 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-set-configuration.spec.ts` around lines 26 - 57, Update the ProviderDouble setup in the setConfiguration test to instantiate a real EventEmitter, matching the pattern used by api-task-control.spec.ts, and assign the mocked provider properties onto it. Preserve the existing mocked on, setValues, contextProxy, providerSettingsManager, and postStateToWebview behavior while ensuring the test object genuinely satisfies the EventEmitter type.src/extension/__tests__/api-task-control.spec.ts (1)
227-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a failing mode switch.
This suite covers an unknown mode but not a rejecting
handleModeSwitch. That is the failure path I raised atsrc/extension/api.tsLine 355-367, where the rejection currently escapes and the answer is never forwarded. Add the case together with the fix.💚 Proposed test addition
+ it("still responds when the mode switch fails", async () => { + const task = createTask("task-mode-switch-error") + sidebarProvider.handleModeSwitch.mockRejectedValue(new Error("switch failed")) + sidebarProvider.emit(RooCodeEventName.TaskCreated, task) + + await expect( + api.selectTaskFollowupSuggestion({ taskId: task.taskId, answer: "Use architect", mode: "architect" }), + ).resolves.toBe(true) + + expect(task.handleWebviewAskResponse).toHaveBeenCalledWith("messageResponse", "Use architect") + })🤖 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-task-control.spec.ts` around lines 227 - 242, Add a test beside the invalid-mode case that mocks handleModeSwitch to reject, then verify selectTaskFollowupSuggestion still resolves successfully, forwards the answer through task.handleWebviewAskResponse, and records the failure appropriately. Update API#selectTaskFollowupSuggestion so a rejected handleModeSwitch is caught and does not prevent answer forwarding.src/core/webview/ClineProvider.ts (3)
642-659: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
as anycasts with a documented typed assertion.Both overrides add an
as anycast plus an ESLint suppression. The coding guidelines require avoidingas anyand fixing lint violations instead of suppressing them, and they allow a double assertion only as a last resort with documentation. Cast the listener to the emitter's parameter type instead, so no suppression is needed.♻️ Proposed refactor
override on<K extends keyof TaskProviderEvents>( event: K, listener: (...args: TaskProviderEvents[K]) => void | Promise<void>, ): this { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - return (super.on as any)(event, listener) + // EventEmitter listeners are typed as returning void; async listeners are + // intentionally allowed here, so the return type is widened for the base call. + return super.on(event, listener as (...args: TaskProviderEvents[K]) => void) }Apply the same change to the
offoverride at Line 653-659.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members, or precise test doubles and type guards. Use double assertions only as a last resort and document them."🤖 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 642 - 659, Replace the `as any` casts and ESLint suppressions in the `on` and `off` overrides with documented typed assertions that adapt the listener to EventEmitter’s parameter type. Preserve the existing event and listener behavior, and apply the same precise assertion approach to both `super.on` and `super.off`.Source: Coding guidelines
3269-3285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the replace-versus-merge rule for flat provider settings.
When the mutation contains
apiProvider, the cachedapiConfigurationis replaced by only the keys present in that mutation. When it does not, the keys are merged into the existing cache. The test atsrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tsLine 1159 depends on this distinction. Add a short comment so a future change does not turn the replace branch into a merge.♻️ Proposed refactor
+ // A mutation that sets apiProvider is a provider switch: replace the cached + // configuration so keys from the previous provider do not leak through. + // A mutation without apiProvider only edits fields of the current provider. this.viewLocalState.apiConfiguration = "apiProvider" in providerSettingsUpdate ? providerSettingsUpdate🤖 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 3269 - 3285, Add a brief comment beside the apiConfiguration assignment in the provider settings update branch explaining that mutations containing apiProvider replace the cached configuration with only supplied keys, while mutations without it merge into the existing configuration. Preserve the current conditional behavior and reference the providerSettingsUpdate distinction clearly.
1927-1943: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate view-state writes in
upsertProviderProfileandactivateProviderProfileUnlocked.saveViewStatealready delegates to_saveViewLocalStateFromMutation, so both methods persistcurrentApiConfigNametwice and update the view-local cache twice. Each duplicate write also enqueues an extra job on the shared staticpersistedViewStateWriteQueue.
src/core/webview/ClineProvider.ts#L1927-L1943: remove the twosaveViewStatecalls from thePromise.alland keep only the_saveViewLocalStateFromMutationcall.src/core/webview/ClineProvider.ts#L2059-L2073: remove the twosaveViewStatecalls from thePromise.alland keep only the_saveViewLocalStateFromMutationcall.🤖 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 1927 - 1943, Remove the redundant saveViewState calls from the Promise.all blocks in upsertProviderProfile at src/core/webview/ClineProvider.ts#L1927-L1943 and activateProviderProfileUnlocked at src/core/webview/ClineProvider.ts#L2059-L2073. Keep each _saveViewLocalStateFromMutation call and all other state updates unchanged so each profile update is persisted once.src/extension/api.ts (1)
389-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the double assertion and the registry lifetime.
Two points on this line:
task as unknown as TaskAskControlleris an undocumented double assertion. The coding guidelines allow a double assertion only as a last resort and require documentation. Add a short comment stating thatTaskstructurally satisfiesTaskAskControllerbut is not declared as implementing it.- The registry removes an entry on
TaskUnfocused(Line 421). A delegating parent emitsTaskUnfocusedwhen its child takes the stack, so the parent becomes unreachable throughapproveTaskAskwhile it is only paused. Record that rule next to the registry so callers understand which tasks are addressable.As per coding guidelines: "Use double assertions only as a last resort and document them."
🤖 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/api.ts` around lines 389 - 391, Document the double assertion in the TaskCreated handler by noting that Task structurally satisfies TaskAskController but is not declared to implement it. Also add a concise comment by the tasksById registry explaining that TaskUnfocused removes entries, including the delegating-parent/paused-child behavior that makes paused parents unreachable through approveTaskAsk.Source: Coding guidelines
src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (3)
925-937: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the empty-state assertion.
viewLocalStatestarts as{}, sovi.waitForsucceeds on the first tick and never waits for the constructor'sloadViewState()to settle. Await the load explicitly, then assert.💚 Proposed fix
- await vi.waitFor(() => { - expect(asProviderAccess(provider).viewLocalState).toEqual({}) - }) + await asProviderAccess(provider).loadViewState() + expect(asProviderAccess(provider).viewLocalState).toEqual({})🤖 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/__tests__/ClineProvider.parallelMode.spec.ts` around lines 925 - 937, Update the “should keep viewLocalState empty when no stable per-view values exist” test to explicitly await the provider’s constructor-triggered loadViewState() completion before asserting viewLocalState. Remove the vi.waitFor-based assertion and keep the empty-state and subsequent getState expectations after loading has settled.
890-891: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated assertion.
Line 890 and Line 891 are identical. Delete one.
💚 Proposed fix
expect(asProviderAccess(provider).viewLocalState).not.toHaveProperty("mode") - expect(asProviderAccess(provider).viewLocalState).not.toHaveProperty("mode") expect(provider.contextProxy.getValue("viewStates")).toBeUndefined()🤖 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/__tests__/ClineProvider.parallelMode.spec.ts` around lines 890 - 891, Remove the duplicated expect assertion in the parallel mode test, keeping a single assertion that viewLocalState does not have the mode property.
1316-1341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a durable-persistence assertion for profile deletion.
This test only checks the in-memory result of
getState(). It does not check theviewStatesentry. That gap hides the issue I raised atsrc/core/webview/ClineProvider.tsLine 1987-1991, wheredeleteProviderProfileupdates the cache but does not persist the replacement selection.Set a stable view-state id first, then assert the persisted entry.
💚 Proposed test addition
const state = await provider.getState() expect(state.currentApiConfigName).toBe("replacement-profile") + expect(provider.contextProxy.getValue("viewStates")).toMatchObject({ + "stable-sidebar-view": { currentApiConfigName: "replacement-profile" }, + })Add
await asProviderAccess(provider).setViewStateId("stable-sidebar-view")before the deletion so the entry has a stable key.🤖 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/__tests__/ClineProvider.parallelMode.spec.ts` around lines 1316 - 1341, Add durable persistence coverage to the test around deleteProviderProfile: call asProviderAccess(provider).setViewStateId("stable-sidebar-view") before deletion, then assert the persisted viewStates entry for that key contains the replacement profile selection, in addition to the existing getState assertions.webview-ui/src/utils/vscode.ts (1)
79-89: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefer the in-memory fallback when a storage write failed.
setStatealways assignsthis.fallbackState, but thelocalStorage.setItemcall can fail silently.getStatethen returns the olderlocalStoragevalue and ignores the newer in-memory value. In that case the comment at Line 115-116 does not hold, becausegetViewStateIdcan return a different id than the one just written.Track whether the last storage write succeeded, or read the in-memory value first when it is set.
♻️ Proposed refactor
try { if (typeof localStorage?.getItem === "function") { const state = localStorage.getItem("vscodeState") - return state ? JSON.parse(state) : this.fallbackState + if (this.fallbackState !== undefined) { + return this.fallbackState + } + + return state ? JSON.parse(state) : undefined } } catch { return this.fallbackState }🤖 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/utils/vscode.ts` around lines 79 - 89, Update setState and getState so a failed localStorage.setItem causes subsequent reads to use the current in-memory fallbackState rather than stale storage data. Track storage-write success or prioritize the in-memory state when set, while preserving the existing fallback behavior for unavailable or invalid storage.
🤖 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 3012-3013: Confirm the intended scope of
destructiveCommandGuardEnabled in the surrounding object literal: if it is
intentionally global and cannot be overridden by a view, keep the stateValues
read and add a brief comment documenting that constraint; otherwise, switch this
property to mergedStateValues to match the other fields.
- Around line 575-584: Update setViewStateId to sanitize the trimmed viewStateId
before comparing it with this.viewStateId. Return early when the sanitized value
is empty or unchanged, then assign that same sanitized value and call
loadViewState only when it differs.
- Around line 1987-1991: Update deleteProviderProfile to call
_saveViewLocalStateFromMutation instead of _updateViewLocalStateFromMutation
when applying the replacement currentApiConfigName and listApiConfigMeta,
ensuring the new profile selection and metadata are persisted to viewStates.
In `@src/extension/api.ts`:
- Around line 355-367: Update selectTaskFollowupSuggestion around
entry.provider.handleModeSwitch so a rejected mode switch is caught and logged
without propagating the error. Always continue to
entry.task.handleWebviewAskResponse("messageResponse", answer) and return true
after a valid mode switch attempt, preserving the existing unknown-mode
behavior.
---
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 925-937: Update the “should keep viewLocalState empty when no
stable per-view values exist” test to explicitly await the provider’s
constructor-triggered loadViewState() completion before asserting
viewLocalState. Remove the vi.waitFor-based assertion and keep the empty-state
and subsequent getState expectations after loading has settled.
- Around line 890-891: Remove the duplicated expect assertion in the parallel
mode test, keeping a single assertion that viewLocalState does not have the mode
property.
- Around line 1316-1341: Add durable persistence coverage to the test around
deleteProviderProfile: call
asProviderAccess(provider).setViewStateId("stable-sidebar-view") before
deletion, then assert the persisted viewStates entry for that key contains the
replacement profile selection, in addition to the existing getState assertions.
In `@src/core/webview/ClineProvider.ts`:
- Around line 642-659: Replace the `as any` casts and ESLint suppressions in the
`on` and `off` overrides with documented typed assertions that adapt the
listener to EventEmitter’s parameter type. Preserve the existing event and
listener behavior, and apply the same precise assertion approach to both
`super.on` and `super.off`.
- Around line 3269-3285: Add a brief comment beside the apiConfiguration
assignment in the provider settings update branch explaining that mutations
containing apiProvider replace the cached configuration with only supplied keys,
while mutations without it merge into the existing configuration. Preserve the
current conditional behavior and reference the providerSettingsUpdate
distinction clearly.
- Around line 1927-1943: Remove the redundant saveViewState calls from the
Promise.all blocks in upsertProviderProfile at
src/core/webview/ClineProvider.ts#L1927-L1943 and
activateProviderProfileUnlocked at
src/core/webview/ClineProvider.ts#L2059-L2073. Keep each
_saveViewLocalStateFromMutation call and all other state updates unchanged so
each profile update is persisted once.
In `@src/extension/__tests__/api-set-configuration.spec.ts`:
- Around line 26-57: Update the ProviderDouble setup in the setConfiguration
test to instantiate a real EventEmitter, matching the pattern used by
api-task-control.spec.ts, and assign the mocked provider properties onto it.
Preserve the existing mocked on, setValues, contextProxy,
providerSettingsManager, and postStateToWebview behavior while ensuring the test
object genuinely satisfies the EventEmitter type.
In `@src/extension/__tests__/api-task-control.spec.ts`:
- Around line 227-242: Add a test beside the invalid-mode case that mocks
handleModeSwitch to reject, then verify selectTaskFollowupSuggestion still
resolves successfully, forwards the answer through
task.handleWebviewAskResponse, and records the failure appropriately. Update
API#selectTaskFollowupSuggestion so a rejected handleModeSwitch is caught and
does not prevent answer forwarding.
In `@src/extension/api.ts`:
- Around line 389-391: Document the double assertion in the TaskCreated handler
by noting that Task structurally satisfies TaskAskController but is not declared
to implement it. Also add a concise comment by the tasksById registry explaining
that TaskUnfocused removes entries, including the delegating-parent/paused-child
behavior that makes paused parents unreachable through approveTaskAsk.
In `@webview-ui/src/utils/vscode.ts`:
- Around line 79-89: Update setState and getState so a failed
localStorage.setItem causes subsequent reads to use the current in-memory
fallbackState rather than stale storage data. Track storage-write success or
prioritize the in-memory state when set, while preserving the existing fallback
behavior for unavailable or invalid storage.
🪄 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: 7be9ad30-1680-4bd1-a995-c13c5d89d706
📒 Files selected for processing (18)
packages/types/src/__tests__/index.test.tspackages/types/src/api.tspackages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/__tests__/ClineProvider.sticky-mode.spec.tssrc/core/webview/__tests__/webviewMessageHandler.routerModels.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tssrc/eslint-suppressions.jsonsrc/extension/__tests__/api-set-configuration.spec.tssrc/extension/__tests__/api-task-control.spec.tssrc/extension/api.tswebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/utils/__tests__/vscode.spec.tswebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
| public async setViewStateId(viewStateId: string | undefined): Promise<void> { | ||
| const normalizedViewStateId = viewStateId?.trim() | ||
|
|
||
| if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) { | ||
| return | ||
| } | ||
|
|
||
| this.viewStateId = normalizedViewStateId.replace(/[^A-Za-z0-9_-]/g, "_") | ||
| await this.loadViewState() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare the sanitized id, not the raw id.
this.viewStateId stores the sanitized value, but the early return compares the raw trimmed value. When the webview reports an id that contains a character outside [A-Za-z0-9_-], every repeated report fails the equality check and triggers another loadViewState(). Sanitize first, then compare.
🐛 Proposed fix
public async setViewStateId(viewStateId: string | undefined): Promise<void> {
- const normalizedViewStateId = viewStateId?.trim()
+ const normalizedViewStateId = viewStateId?.trim().replace(/[^A-Za-z0-9_-]/g, "_")
if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) {
return
}
- this.viewStateId = normalizedViewStateId.replace(/[^A-Za-z0-9_-]/g, "_")
+ this.viewStateId = normalizedViewStateId
await this.loadViewState()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async setViewStateId(viewStateId: string | undefined): Promise<void> { | |
| const normalizedViewStateId = viewStateId?.trim() | |
| if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) { | |
| return | |
| } | |
| this.viewStateId = normalizedViewStateId.replace(/[^A-Za-z0-9_-]/g, "_") | |
| await this.loadViewState() | |
| } | |
| public async setViewStateId(viewStateId: string | undefined): Promise<void> { | |
| const normalizedViewStateId = viewStateId?.trim().replace(/[^A-Za-z0-9_-]/g, "_") | |
| if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) { | |
| return | |
| } | |
| this.viewStateId = normalizedViewStateId | |
| await this.loadViewState() | |
| } |
🤖 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 575 - 584, Update
setViewStateId to sanitize the trimmed viewStateId before comparing it with
this.viewStateId. Return early when the sanitized value is empty or unchanged,
then assign that same sanitized value and call loadViewState only when it
differs.
| this._updateViewLocalStateFromMutation({ | ||
| currentApiConfigName: profileToActivate, | ||
| listApiConfigMeta: entries, | ||
| }) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the replacement profile selection after a deletion.
deleteProviderProfile calls _updateViewLocalStateFromMutation, which updates only the in-memory cache. upsertProviderProfile (Line 1938) and activateProviderProfileUnlocked (Line 2069) call _saveViewLocalStateFromMutation, which also writes the durable viewStates entry.
Because the deletion is not persisted, the viewStates entry keeps the deleted profile name. After a reload, loadViewState restores that name, providerSettingsManager.getProfile fails, and the view reports a profile that no longer exists.
🐛 Proposed fix
- this._updateViewLocalStateFromMutation({
- currentApiConfigName: profileToActivate,
- listApiConfigMeta: entries,
- })
+ await this._saveViewLocalStateFromMutation({
+ currentApiConfigName: profileToActivate,
+ listApiConfigMeta: entries,
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._updateViewLocalStateFromMutation({ | |
| currentApiConfigName: profileToActivate, | |
| listApiConfigMeta: entries, | |
| }) | |
| await this._saveViewLocalStateFromMutation({ | |
| currentApiConfigName: profileToActivate, | |
| listApiConfigMeta: entries, | |
| }) |
🤖 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 1987 - 1991, Update
deleteProviderProfile to call _saveViewLocalStateFromMutation instead of
_updateViewLocalStateFromMutation when applying the replacement
currentApiConfigName and listApiConfigMeta, ensuring the new profile selection
and metadata are persisted to viewStates.
| destructiveCommandGuardEnabled: | ||
| stateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Confirm that destructiveCommandGuardEnabled must stay global.
Every other field in this object literal now reads from mergedStateValues. This one still reads stateValues. If the guard must remain a global setting that a view cannot override, add a short comment stating that. If the global read is an oversight, switch it to mergedStateValues for consistency.
🤖 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 3012 - 3013, Confirm the
intended scope of destructiveCommandGuardEnabled in the surrounding object
literal: if it is intentionally global and cannot be overridden by a view, keep
the stateValues read and add a brief comment documenting that constraint;
otherwise, switch this property to mergedStateValues to match the other fields.
| if (mode) { | ||
| const { customModes } = await entry.provider.getState() | ||
| const isValidMode = getAllModes(customModes).some((modeConfig) => modeConfig.slug === mode) | ||
|
|
||
| if (isValidMode) { | ||
| await entry.provider.handleModeSwitch(mode) | ||
| } else { | ||
| this.log(`[API#selectTaskFollowupSuggestion] ignoring unknown mode "${mode}" for task ${taskId}`) | ||
| } | ||
| } | ||
|
|
||
| entry.task.handleWebviewAskResponse("messageResponse", answer) | ||
| return true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle a failing mode switch so the answer still reaches the task.
entry.provider.handleModeSwitch(mode) can reject. ClineProvider.handleModeSwitchUnlocked rethrows persistence failures, and enqueueProviderProfileMutation rejects after the 30-second mutation timeout. When that happens, selectTaskFollowupSuggestion throws before Line 366, the answer is never forwarded, and the task stays blocked on the pending ask. Every other failure path in this method returns a boolean.
🐛 Proposed fix
if (isValidMode) {
- await entry.provider.handleModeSwitch(mode)
+ try {
+ await entry.provider.handleModeSwitch(mode)
+ } catch (error) {
+ const errorMessage = error instanceof Error ? error.message : String(error)
+ this.log(
+ `[API#selectTaskFollowupSuggestion] mode switch to "${mode}" failed for task ${taskId}: ${errorMessage}`,
+ )
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (mode) { | |
| const { customModes } = await entry.provider.getState() | |
| const isValidMode = getAllModes(customModes).some((modeConfig) => modeConfig.slug === mode) | |
| if (isValidMode) { | |
| await entry.provider.handleModeSwitch(mode) | |
| } else { | |
| this.log(`[API#selectTaskFollowupSuggestion] ignoring unknown mode "${mode}" for task ${taskId}`) | |
| } | |
| } | |
| entry.task.handleWebviewAskResponse("messageResponse", answer) | |
| return true | |
| if (mode) { | |
| const { customModes } = await entry.provider.getState() | |
| const isValidMode = getAllModes(customModes).some((modeConfig) => modeConfig.slug === mode) | |
| if (isValidMode) { | |
| try { | |
| await entry.provider.handleModeSwitch(mode) | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : String(error) | |
| this.log( | |
| `[API#selectTaskFollowupSuggestion] mode switch to "${mode}" failed for task ${taskId}: ${errorMessage}`, | |
| ) | |
| } | |
| } else { | |
| this.log(`[API#selectTaskFollowupSuggestion] ignoring unknown mode "${mode}" for task ${taskId}`) | |
| } | |
| } | |
| entry.task.handleWebviewAskResponse("messageResponse", answer) | |
| return true |
🤖 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/api.ts` around lines 355 - 367, Update
selectTaskFollowupSuggestion around entry.provider.handleModeSwitch so a
rejected mode switch is caught and logged without propagating the error. Always
continue to entry.task.handleWebviewAskResponse("messageResponse", answer) and
return true after a valid mode switch attempt, preserving the existing
unknown-mode behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Add task control and global state APIs at the Extension API layer, allowing external tools to control tasks and read view-local state. This is the API surface exposed by per-view isolation.
Changes
approveAsk(),selectFollowUpSuggestion()task control methodsFiles Changed (4 files, +422 / -5)
src/extension/api.tsapi-task-control.spec.tsapi-set-configuration.spec.tspackages/types/src/api.tsDesign Notes
setConfigurationview-local state sync mechanism is integratedTest Notes
This PR includes commits from base-1 and base-2 (linear dependency chain), so tests pass. If reviewed in isolation without base-2's
setValues()method, the persistence layer would fail.Related