test(webview): e2e tests + API task controls (branch 3/3) - #1140
test(webview): e2e tests + API task controls (branch 3/3)#1140easonLiangWorldedtech wants to merge 18 commits into
Conversation
📝 WalkthroughWalkthroughThe change adds durable per-view mode and provider-profile state, stable webview state IDs, task-control API methods, resilient configuration handling, and comprehensive unit and VS Code E2E coverage for concurrent view isolation. ChangesView-state contracts and webview wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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)
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/fixtures/view-state.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/vscode-e2e/src/runTest.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. apps/vscode-e2e/src/suite/view-state.test.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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/extension/api.ts (1)
390-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the double assertion on the registered task.
The coding guidelines require a comment next to an unavoidable double assertion.
task as unknown as TaskAskControllerhas none. Either add a short comment that explains why theTasktype cannot be narrowed structurally, or widenTaskAskControllerso the cast becomes unnecessary.♻️ Proposed change
provider.on(RooCodeEventName.TaskCreated, (task) => { + // The emitted Task type is not structurally assignable to TaskAskController, + // so an unknown bridge is required to store only the ask-control surface. this.tasksById.set(task.taskId, { task: task as unknown as TaskAskController, provider })As per coding guidelines: "If a lint suppression is unavoidable, document the reason in a comment next to the cast or suppression." and "Use double assertions only as a last resort and explain them with a comment."
🤖 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` at line 390, Add a concise comment beside the double assertion in the task registration code explaining why Task cannot be structurally narrowed to TaskAskController; retain the cast only if it is unavoidable, otherwise widen TaskAskController to eliminate it.Source: Coding guidelines
src/core/webview/ClineProvider.ts (2)
1833-1847: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listApiConfigMetain the mutation payload is ignored.
_updateViewLocalStateFromMutationhandles onlymode,currentApiConfigName,apiConfiguration, and the flat provider-settings keys._persistViewLocalStateFromMutationpersists onlymodeandcurrentApiConfigName. PassinglistApiConfigMetahas no effect. Drop the field from both call sites, or add explicit handling if per-view profile lists are intended.Also applies to: 1936-1949
🤖 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 1833 - 1847, Remove listApiConfigMeta from the mutation payloads passed to _saveViewLocalStateFromMutation at both affected call sites, since the downstream mutation and persistence handlers do not process it. Keep the existing global-state update for listApiConfigMeta unchanged.
498-507: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
clearPersistedViewStatehelper.This method has no callers, adds dead code to
ClineProvider.ts, and is not wired intodispose()or the persisted viewState cleanup 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 498 - 507, Remove the unused ClineProvider.clearPersistedViewState method, including its persisted-view-state queue handling, since it has no callers and is not part of the active cleanup flow.webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx (1)
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
as anycast with a typedModevalue.The coding guidelines require avoiding
as anyand documenting any unavoidable cast. Import theModetype and pass a typed slug instead.♻️ Proposed change
- <button data-testid="set-local-mode" onClick={() => setMode("ask" as any)}> + <button data-testid="set-local-mode" onClick={() => setMode("ask" satisfies Mode)}>As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."🤖 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/context/__tests__/ExtensionStateContext.spec.tsx` at line 89, Replace the any cast in the set-local-mode button’s onClick handler with a typed Mode value: import Mode in ExtensionStateContext.spec.tsx and use a valid Mode slug when calling setMode. Do not introduce another broad cast.Source: Coding guidelines
src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (1)
694-695: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the typed private-access bridge consistently instead of
(provider as any).Two tests in this file already declare a typed accessor (lines 849-853 and 1065-1070). The remaining tests use
(provider as any), which the coding guidelines discourage. Reuse one shared helper that exposessaveViewState,setViewStateId,loadViewState,viewLocalState, andprunePersistedViewStates.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."Also applies to: 717-718, 1002-1002
🤖 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 694 - 695, Replace the `(provider as any)` private-member access in the affected tests with one shared typed accessor helper. Define or reuse a helper exposing `saveViewState`, `setViewStateId`, `loadViewState`, `viewLocalState`, and `prunePersistedViewStates`, then update the tests around `saveViewState` and the other referenced call sites to use it consistently without `as any`.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 `@apps/vscode-e2e/fixtures/modes.json`:
- Around line 17-29: Update the match object for the first debug-mode
switch_mode response in modes.json to include sequenceIndex set to 0, preserving
the existing userMessage matcher and response so replay associates the
subsequent call_modes_switch_002 fixture with the correct request sequence.
In `@apps/vscode-e2e/src/fixtures/view-state.ts`:
- Line 1: Update the addViewStateFixtures parameter type to use the imported
LLMock type directly instead of InstanceType<typeof LLMock>; keep the existing
ChatCompletionRequest and ChatMessage typing unchanged.
In `@apps/vscode-e2e/src/suite/view-state.test.ts`:
- Around line 212-270: Update the completion wait in the test around the
existing mode-event assertions so it awaits waitUntilCompleted for every task ID
in taskIds before entering the finally block. Keep the listeners, including
messageHandler, installed until all tab tasks have completed, while preserving
the existing mode-event validation and cleanup behavior.
In `@src/core/webview/ClineProvider.ts`:
- Around line 228-232: Update the view-state persistence flow around
ClineProvider.viewStateId so the reload-unstable fallback identifier is never
used as a durable storage key: either prune the fallback key during activation
or defer saving and loading view-local state until setViewStateId receives the
stable webview-reported id. Ensure providers cannot restore another view’s
pre-launch state after nextViewId resets.
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 612-613: The invalid-profile fallback at line 619 writes the
fallback profile through updateGlobalState, which persists it to the shared
ContextProxy instead of keeping it view-local. This allows one view with a stale
currentApiConfigName to overwrite the shared default for all views. Either
persist the fallback profile through the provider's view-local state path
(matching the read pattern from provider.getState() at line 612) instead of
updateGlobalState, or delegate the fallback update responsibility to
activateProviderProfile so it owns the state transition. Update the launch test
to assert that currentApiConfigName is not written through
contextProxy.setValue.
In `@src/eslint-suppressions.json`:
- Around line 1062-1076: Replace the any-based test doubles with typed
parameters, mock returns, or intersection types so no-explicit-any suppression
counts do not increase. Apply this in
src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts (42-50),
src/core/webview/__tests__/webviewMessageHandler.spec.ts (210-230), and
src/extension/__tests__/api-set-configuration.spec.ts (24-36); then update the
corresponding suppression entries in src/eslint-suppressions.json (1062-1076,
1162-1165, and 1187-1191) to reflect counts that decrease or remain unchanged.
---
Nitpick comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 694-695: Replace the `(provider as any)` private-member access in
the affected tests with one shared typed accessor helper. Define or reuse a
helper exposing `saveViewState`, `setViewStateId`, `loadViewState`,
`viewLocalState`, and `prunePersistedViewStates`, then update the tests around
`saveViewState` and the other referenced call sites to use it consistently
without `as any`.
In `@src/core/webview/ClineProvider.ts`:
- Around line 1833-1847: Remove listApiConfigMeta from the mutation payloads
passed to _saveViewLocalStateFromMutation at both affected call sites, since the
downstream mutation and persistence handlers do not process it. Keep the
existing global-state update for listApiConfigMeta unchanged.
- Around line 498-507: Remove the unused ClineProvider.clearPersistedViewState
method, including its persisted-view-state queue handling, since it has no
callers and is not part of the active cleanup flow.
In `@src/extension/api.ts`:
- Line 390: Add a concise comment beside the double assertion in the task
registration code explaining why Task cannot be structurally narrowed to
TaskAskController; retain the cast only if it is unavoidable, otherwise widen
TaskAskController to eliminate it.
In `@webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx`:
- Line 89: Replace the any cast in the set-local-mode button’s onClick handler
with a typed Mode value: import Mode in ExtensionStateContext.spec.tsx and use a
valid Mode slug when calling setMode. Do not introduce another broad cast.
🪄 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: 60c26621-74b4-483a-9d88-d0b074b92bb0
📒 Files selected for processing (25)
apps/vscode-e2e/fixtures/modes.jsonapps/vscode-e2e/src/fixtures/view-state.tsapps/vscode-e2e/src/runTest.tsapps/vscode-e2e/src/suite/view-state.test.tspackages/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/App.tsxwebview-ui/src/__tests__/App.spec.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsxwebview-ui/src/utils/__tests__/vscode.spec.tswebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- webview-ui/src/App.tsx
| { | ||
| "match": { | ||
| "userMessage": "Use the `switch_mode` tool to switch to debug mode." | ||
| }, | ||
| "response": { | ||
| "toolCalls": [ | ||
| { | ||
| "name": "switch_mode", | ||
| "arguments": "{\"mode_slug\":\"debug\",\"reason\":\"User requested to switch to debug mode.\"}", | ||
| "id": "call_modes_switch_002" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)modes\.json$|apps/vscode-e2e/src/runTest\.ts$|modes|debug|call_modes_switch' || true
echo
echo "Modes fixture:"
if [ -f apps/vscode-e2e/fixtures/modes.json ]; then
cat -n apps/vscode-e2e/fixtures/modes.json
fi
echo
echo "runTest references to match/sequenceIndex/fixture tools:"
if [ -f apps/vscode-e2e/src/runTest.ts ]; then
rg -n "sequenceIndex|match|toolCalls|fixture|call_modes_switch|modes" apps/vscode-e2e/src/runTest.ts -C 3
fi
echo
echo "Other fixtures with sequenceIndex:"
rg -n '"sequenceIndex"\s*:\s*0|sequenceIndex' apps/vscode-e2e/fixtures -S -C 2 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 10770
Mark the first debug tool response as turn zero.
The JSON fixture does not include sequenceIndex, while later fixtures consume call_modes_switch_002. Add "sequenceIndex": 0 to the match object for this turn so replay associates the follow-up response with the same request sequence.
Proposed fix
"match": {
- "userMessage": "Use the `switch_mode` tool to switch to debug mode."
+ "userMessage": "Use the `switch_mode` tool to switch to debug mode.",
+ "sequenceIndex": 0
}📝 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.
| { | |
| "match": { | |
| "userMessage": "Use the `switch_mode` tool to switch to debug mode." | |
| }, | |
| "response": { | |
| "toolCalls": [ | |
| { | |
| "name": "switch_mode", | |
| "arguments": "{\"mode_slug\":\"debug\",\"reason\":\"User requested to switch to debug mode.\"}", | |
| "id": "call_modes_switch_002" | |
| } | |
| ] | |
| } | |
| { | |
| "match": { | |
| "userMessage": "Use the `switch_mode` tool to switch to debug mode.", | |
| "sequenceIndex": 0 | |
| }, | |
| "response": { | |
| "toolCalls": [ | |
| { | |
| "name": "switch_mode", | |
| "arguments": "{\"mode_slug\":\"debug\",\"reason\":\"User requested to switch to debug mode.\"}", | |
| "id": "call_modes_switch_002" | |
| } | |
| ] | |
| } |
🤖 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 `@apps/vscode-e2e/fixtures/modes.json` around lines 17 - 29, Update the match
object for the first debug-mode switch_mode response in modes.json to include
sequenceIndex set to 0, preserving the existing userMessage matcher and response
so replay associates the subsequent call_modes_switch_002 fixture with the
correct request sequence.
Source: Coding guidelines
| @@ -0,0 +1,95 @@ | |||
| import type { ChatCompletionRequest, ChatMessage, LLMock } from "@copilotkit/aimock" | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/vscode-e2e/src/fixtures/view-state.ts"
if rg -q 'import type .*LLMock' "$file" && rg -qF 'InstanceType<typeof LLMock>' "$file"; then
echo "LLMock is type-only imported and used in a typeof type query."
exit 1
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/vscode-e2e/src/fixtures/view-state.ts"
printf 'Relevant imports and function signature:\n'
sed -n '1,12p;45,58p' "$file"
printf '\nTypeScript tooling / config evidence:\n'
for f in package.json tsconfig.json apps/vscode-e2e/tsconfig.json; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
printf '\nAttempt type-only import usage probe without building repository code:\n'
tmpdir="$(mktemp -d)"
cat > "$tmpdir/main.ts" <<'TS'
import type { LLMock } from './LLMOCK'
export function addViewStateFixtures(mock: InstanceType<typeof LLMock>) {
return mock
}
TS
cat > "$tmpdir/LLMOCK.ts" <<'TS'
export class LLMock {
constructor(message: string) {}
}
TS
cat > "$tmpdir/tsconfig.json" <<'JSON'
{
"compilerOptions": {
"module": "esnext",
"target": "esnext",
"strict": true,
"noEmit": true
}
}
JSON
if command -v npx >/dev/null 2>&1; then
cd "$tmpdir" && npx -y --no-install typescript@latest --project tsconfig.json
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4968
Use LLMock directly as the instance type.
LLMock is imported as a type but addViewStateFixtures uses InstanceType<typeof LLMock>. A typeof type query needs a value binding, and this type-only import also violates the app’s type-check setup. Use LLMock directly for the parameter type.
Proposed fix
-export function addViewStateFixtures(mock: InstanceType<typeof LLMock>) {
+export function addViewStateFixtures(mock: LLMock) {🤖 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 `@apps/vscode-e2e/src/fixtures/view-state.ts` at line 1, Update the
addViewStateFixtures parameter type to use the imported LLMock type directly
instead of InstanceType<typeof LLMock>; keep the existing ChatCompletionRequest
and ChatMessage typing unchanged.
| await waitFor( | ||
| () => { | ||
| const expectedSwitches = plan.length * 10 | ||
| return modeEvents.length >= expectedSwitches | ||
| }, | ||
| { timeout: 30_000 }, | ||
| ).catch((error) => { | ||
| const counts = plan.map((taskPlan) => { | ||
| const taskId = taskIds.get(taskPlan.taskName) | ||
| return `${taskPlan.taskName}:${taskId ? modeCountForTask(taskId) : 0}` | ||
| }) | ||
| throw new Error( | ||
| `Timed out after ${releasedRounds} coordinated rounds; mode event counts: ${counts.join(", ")}; pending suggestions: ${pendingSuggestions.size}. ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| }) | ||
|
|
||
| for (let roundIndex = 0; roundIndex < 10; roundIndex++) { | ||
| const actualRoundModes = plan.map((taskPlan) => { | ||
| const taskId = taskIds.get(taskPlan.taskName) | ||
| assert.ok(taskId, `Expected task id for task ${taskPlan.taskName}`) | ||
| return modeEvents.filter((event) => event.taskId === taskId).map((event) => event.mode)[roundIndex] | ||
| }) | ||
| const expectedRoundModes = plan.map((taskPlan) => { | ||
| const round = taskPlan.rounds[roundIndex] | ||
| assert.ok(round, `Expected round ${roundIndex + 1} for task ${taskPlan.taskName}`) | ||
| return round.mode | ||
| }) | ||
|
|
||
| assert.deepStrictEqual( | ||
| actualRoundModes, | ||
| expectedRoundModes, | ||
| `Round ${roundIndex + 1} should count only after all three tasks switch once`, | ||
| ) | ||
| } | ||
|
|
||
| for (const taskPlan of plan) { | ||
| const taskId = taskIds.get(taskPlan.taskName) | ||
| assert.ok(taskId, `Expected task id for task ${taskPlan.taskName}`) | ||
| assert.deepStrictEqual( | ||
| modeEvents.filter((event) => event.taskId === taskId).map((event) => event.mode), | ||
| taskPlan.rounds.map((round) => round.mode), | ||
| ) | ||
| } | ||
|
|
||
| const viewStates = globalThis.api.getGlobalState("viewStates") as GlobalState["viewStates"] | ||
| assert.ok(viewStates, "Expected persisted viewStates to exist") | ||
|
|
||
| for (const [viewStateId, entry] of Object.entries(viewStates)) { | ||
| const secretStatePath = findSecretStatePath(entry) | ||
| assert.strictEqual( | ||
| secretStatePath, | ||
| undefined, | ||
| `Persisted viewStates.${viewStateId} leaked secret state at ${secretStatePath}`, | ||
| ) | ||
| } | ||
| } finally { | ||
| globalThis.api.off(RooCodeEventName.Message, messageHandler) | ||
| globalThis.api.off(RooCodeEventName.TaskModeSwitched, modeHandler) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for every tab task to complete.
Line 212 only waits for the last mode-switch event. The fixture emits attempt_completion later, and messageHandler at Lines 159-177 approves that ask. The finally block can remove the handler first, leaving tab tasks pending and allowing late work into later tests. Await waitUntilCompleted for every task while the listeners remain installed.
Proposed fix
await waitFor(
() => {
const expectedSwitches = plan.length * 10
return modeEvents.length >= expectedSwitches
},
{ timeout: 30_000 },
)
+
+ await Promise.all(
+ taskIdsInPlanOrder().map((taskId) => waitUntilCompleted({ api: globalThis.api, taskId })),
+ )As per coding guidelines, E2E suites must account for late asynchronous requests from prior tasks.
🤖 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 `@apps/vscode-e2e/src/suite/view-state.test.ts` around lines 212 - 270, Update
the completion wait in the test around the existing mode-event assertions so it
awaits waitUntilCompleted for every task ID in taskIds before entering the
finally block. Keep the listeners, including messageHandler, installed until all
tab tasks have completed, while preserving the existing mode-event validation
and cleanup behavior.
Source: Coding guidelines
| /** | ||
| * Stable identifier for persisted per-view state keys. | ||
| * Defaults to viewId until the webview reports its VS Code-persisted id. | ||
| */ | ||
| private viewStateId: string |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the only stable-id source is the webview message, and check whether fallback viewId keys are ever cleaned up.
rg -nP --type=ts -C4 'setViewStateId' src
rg -nP --type=ts -C4 'clearPersistedViewState|nextViewId' srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 20923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant persistence and lifecycle methods around the reported concern.
sed -n '220,270p' src/core/webview/ClineProvider.ts
sed -n '460,535p' src/core/webview/ClineProvider.ts
sed -n '550,570p' src/core/webview/webviewMessageHandler.ts
# Search for all usages of clearPersistedViewState and dispose/loadViewState patterns.
rg -nP --type=ts -C3 'clearPersistedViewState|dispose\(|loadViewState|saveViewState|viewStates' src/core/webview/ClineProvider.ts src/core/webview/webviewMessageHandler.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 16276
Don’t use viewId as a durable viewStateId.
viewStateId starts as ${renderContext}-${ClineProvider.nextViewId++}, and nextViewId resets with each extension reload. That means pre-launch edits can persist under shared fallback keys like sidebar-0, and the next reload can load them into a different provider. Prefer pruning the fallback key on activation, or avoid saving/loading view-local state until setViewStateId receives the webview-reported stable id.
🤖 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 228 - 232, Update the
view-state persistence flow around ClineProvider.viewStateId so the
reload-unstable fallback identifier is never used as a durable storage key:
either prune the fallback key during activation or defer saving and loading
view-local state until setViewStateId receives the stable webview-reported id.
Ensure providers cannot restore another view’s pre-launch state after nextViewId
resets.
| const currentState = await provider.getState() | ||
| const currentConfigName = currentState.currentApiConfigName |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the fallback profile selection view-local.
Line 613 reads currentApiConfigName from the current view. The invalid-profile branch then writes the fallback through updateGlobalState at Line 619. That call writes to the shared ContextProxy, so one view with a stale profile can change the shared fallback read by another view without a local override. Persist the fallback through the provider view-local state path, or let activateProviderProfile own the update. Extend the launch test to assert that currentApiConfigName is not written through contextProxy.setValue.
🤖 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 612 - 613, The
invalid-profile fallback at line 619 writes the fallback profile through
updateGlobalState, which persists it to the shared ContextProxy instead of
keeping it view-local. This allows one view with a stale currentApiConfigName to
overwrite the shared default for all views. Either persist the fallback profile
through the provider's view-local state path (matching the read pattern from
provider.getState() at line 612) instead of updateGlobalState, or delegate the
fallback update responsibility to activateProviderProfile so it owns the state
transition. Update the launch test to assert that currentApiConfigName is not
written through contextProxy.setValue.
| "core/webview/ClineProvider.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 12 | ||
| "count": 16 | ||
| } | ||
| }, | ||
| "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 34 | ||
| } | ||
| }, | ||
| "core/webview/__tests__/ClineProvider.parallelMode.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 143 | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status/stat =="
git status --short
git diff --stat || true
echo "== changed files mentioning eslint-suppressions/specs =="
git diff --name-only | sed -n '1,200p'
echo "== eslint-suppressions snippets =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/eslint-suppressions.json')
lines=p.read_text().splitlines()
for start in [1045, 1145, 1175]:
print(f"-- {p}:{start}-{start+30} --")
for i in range(max(0,start), min(len(lines), start+30)):
print(f"{i+1:5d}: {lines[i]}")
PY
echo "== relevant source snippets with line numbers =="
for f in \
"src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts" \
"src/core/webview/__tests__/webviewMessageHandler.spec.ts" \
"src/extension/__tests__/api-set-configuration.spec.ts"
do
if [ -f "$f" ]; then
echo "-- $f --"
wc -l "$f"
sed -n '1,280p' "$f" | nl -ba
else
echo "missing $f"
fi
done
echo "== eslint suppression count objects for mentioned files =="
python3 - <<'PY'
import json, re
from pathlib import Path
data=json.loads(Path('src/eslint-suppressions.json').read_text())
targets=[
"core/webview/ClineProvider.ts",
"core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts",
"core/webview/__tests__/ClineProvider.parallelMode.spec.ts",
"core/webview/__tests__/webviewMessageHandler.spec.ts",
"extension/__tests__/api-set-configuration.spec.ts",
]
for t in targets:
if t in data:
print(t, data[t])
PY
echo "== any occurrences in changed/relevant test files =="
rg -n "\bany\b|as any" src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts src/core/webview/__tests__/webviewMessageHandler.spec.ts src/extension/__tests__/api-set-configuration.spec.ts || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package/tooling hints for eslint config =="
git ls-files | rg '(^|/)(package\.json|eslint\.config\.(js|mjs|cjs|ts)|\.eslintrc|tsconfig\.json)$' | sed -n '1,160p'
echo "== eslint suppressions file size/checksum =="
wc -l src/eslint-suppressions.json
md5sum src/eslin-t-suppressions.json || true
md5sum src/eslint-suppressions.json || true
echo "== git diff focused lines for suppressions =="
git diff --unified=80 -- src/eslint-suppressions.json | sed -n '1,260p'
echo "== git diff focused lines for test files =="
for f in \
"src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts" \
"src/core/webview/__tests__/webviewMessageHandler.spec.ts" \
"src/extension/__tests__/api-set-configuration.spec.ts"
do
echo "-- $f --"
git diff --unified=80 -- "$f" | sed -n '1,260p'
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read relevant test snippets =="
python3 - <<'PY'
from pathlib import Path
files=[
"src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts",
"src/core/webview/__tests__/webviewMessageHandler.spec.ts",
"src/extension/__tests__/api-set-configuration.spec.ts",
]
for path in files:
p=Path(path)
print(f"-- {path}: {p.stat().st_size} bytes --")
lines=p.read_text().splitlines()
if path.endswith("routerModels.spec.ts"):
for i in range(max(0,25), min(len(lines), 65)):
print(f"{i+1:4d}: {lines[i]}")
elif path.endswith("webviewMessageHandler.spec.ts"):
for i in range(max(0,200), min(len(lines), 240)):
print(f"{i+1:4d}: {lines[i]}")
elif path.endswith("api-set-configuration.spec.ts"):
for i in range(max(0,15), min(len(lines), 45)):
print(f"{i+1:4d}: {lines[i]}")
PY
echo "== suppressions exact entries =="
python3 - <<'PY'
import json
from pathlib import Path
data=json.loads(Path("src/eslint-suppressions.json").read_text())
targets=[
"core/webview/ClineProvider.ts",
"core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts",
"core/webview/__tests__/ClineProvider.parallelMode.spec.ts",
"core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts",
"core/webview/__tests__/webviewMessageHandler.spec.ts",
"extension/__tests__/api-set-configuration.spec.ts",
]
for t in targets:
print(t, "exists=", t in data, "entry=", data.get(t))
PY
echo "== changed files via HEAD diff =="
git rev-parse --short HEAD || true
git diff --name-status HEAD~1..HEAD 2>/dev/null | sed -n '1,200p' || true
git diff --name-only 2>/dev/null | sed -n '1,200p' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 6478
Keep no-explicit-any suppressions from growing.
src/eslint-suppressions.json already contains no-explicit-any entries for the changed test files, and those files use any escape hatches. If the suppression count is meant to never increase, replace the test doubles with typed parameters/mock returns/intersection types and let the counts decrease or stay equal instead of rising.
📍 Affects 4 files
src/eslint-suppressions.json#L1062-L1076(this comment)src/eslint-suppressions.json#L1162-L1165src/eslint-suppressions.json#L1187-L1191src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts#L42-L50src/core/webview/__tests__/webviewMessageHandler.spec.ts#L210-L230src/extension/__tests__/api-set-configuration.spec.ts#L24-L36
🤖 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/eslint-suppressions.json` around lines 1062 - 1076, Replace the any-based
test doubles with typed parameters, mock returns, or intersection types so
no-explicit-any suppression counts do not increase. Apply this in
src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts (42-50),
src/core/webview/__tests__/webviewMessageHandler.spec.ts (210-230), and
src/extension/__tests__/api-set-configuration.spec.ts (24-36); then update the
corresponding suppression entries in src/eslint-suppressions.json (1062-1076,
1162-1165, and 1187-1191) to reflect counts that decrease or remain unchanged.
Source: Coding guidelines
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
Summary
E2E tests, API task control tests, and review feedback fixes. This is Branch 3 of 3 split from PR #977 for easier review.
Changes
test(webview): mock workspace tracker init(+1 / -1)test(vscode-e2e): cross-panel isolation(+191 / -2)test(vscode-e2e): follow-up mode isolation(+308 / -8)test(api): task controls + view-local values(+291)fix(webview): address review feedback(+79 / -30)~806 lines changed (incremental)
Related
Summary by CodeRabbit
New Features
Bug Fixes