test(e2e): add view state isolation tests and update webview coverage - #1186
test(e2e): add view state isolation tests and update webview coverage#1186easonLiangWorldedtech wants to merge 4 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 change adds durable per-view state isolation for modes and provider profiles, view-state identification across webview boundaries, task-control APIs, stronger message typing, resilient Kimi Code model discovery, and VS Code E2E coverage for parallel views. ChangesPer-view state isolation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Webview
participant webviewMessageHandler
participant ClineProvider
participant GlobalState
Webview->>webviewMessageHandler: Send webviewDidLaunch with viewStateId
webviewMessageHandler->>ClineProvider: Set view-state ID
ClineProvider->>GlobalState: Load persisted view-local state
ClineProvider->>Webview: Publish merged provider state
Webview->>webviewMessageHandler: Send mode or profile mutation
webviewMessageHandler->>ClineProvider: Apply view-local mutation
ClineProvider->>GlobalState: Queue persisted view-state update
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: 4
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/webviewMessageHandler.ts (1)
616-630: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRepair the view-local profile name, not only the global one.
Line 617 now reads
currentApiConfigNamefromprovider.getState(), which returns the view-local value when one exists. The repair on line 623 writes only to global state throughupdateGlobalState. Two problems follow:
- If
listApiConfig[0]?.nameisundefined, the code clears the global value and returns without callingactivateProviderProfile. The view keeps the invalidcurrentApiConfigName, sogetState()still returns it on the next read.- When
nameexists, the global write is redundant becauseactivateProviderProfilealready persists the name into view state.Drop the global write and rely on
activateProviderProfile, or clear the view-local value explicitly when no replacement profile exists.🐛 Proposed fix
if (currentConfigName) { if (!(await provider.providerSettingsManager.hasConfig(currentConfigName))) { // Current config name not valid, get first config in list. const name = listApiConfig[0]?.name - await updateGlobalState("currentApiConfigName", name) if (name) { await provider.activateProviderProfile({ name }) return } + + // No replacement profile exists. Clear the stale selection for this view + // so getState() stops returning an unresolvable profile name. + await provider.setValue("currentApiConfigName", undefined) } }🤖 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 616 - 630, Update the invalid-profile repair in the provider state handling to avoid writing only global state via updateGlobalState. When listApiConfig provides a replacement name, call activateProviderProfile({ name }) and rely on it to persist the view-local value; when no name exists, explicitly clear the view-local currentApiConfigName so the invalid value is not retained.
🧹 Nitpick comments (7)
src/extension/api.ts (1)
388-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the undocumented double assertion.
task as unknown as TaskAskControllerhides the real relationship betweenTaskand the two methods used here. IfTaskalready exposesapproveAskandhandleWebviewAskResponse, store the task directly and typeRegisteredTask.taskas the task type. If a structural type is preferred, add a comment that states why the double assertion is required.The coding guidelines require documenting double assertions.
🤖 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 388 - 391, Update registerListeners and the RegisteredTask.task type to store the Task directly when it exposes approveAsk and handleWebviewAskResponse, removing the undocumented task as unknown as TaskAskController assertion; otherwise retain the assertion only with a comment documenting why it is required.Source: Coding guidelines
apps/vscode-e2e/src/suite/view-state.test.ts (1)
212-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the round count from the plan instead of the literal
10.
view-state.tsdefinesROUNDS = 10and exportsroundsper task. This test repeats10at Line 214 and Line 228. IfROUNDSchanges, the fixtures and the assertions drift apart silently.♻️ Proposed change
- const expectedSwitches = plan.length * 10 + const expectedSwitches = plan.reduce((total, taskPlan) => total + taskPlan.rounds.length, 0) return modeEvents.length >= expectedSwitches- for (let roundIndex = 0; roundIndex < 10; roundIndex++) { + const roundCount = Math.max(...plan.map((taskPlan) => taskPlan.rounds.length)) + for (let roundIndex = 0; roundIndex < roundCount; roundIndex++) {🤖 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 - 245, Replace the hardcoded round count of 10 in the expected event calculation and round-validation loop with the shared plan-derived round count, reusing the existing ROUNDS or rounds length symbol defined by the test fixtures. Ensure both expectedSwitches and iteration bounds stay synchronized with the plan when ROUNDS changes.src/core/webview/__tests__/webviewMessageHandler.spec.ts (2)
251-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the single
setImmediateflush withvi.waitFor.The handler starts
providerSettingsManager.listConfig().then(...)without awaiting it. That chain contains several awaits. OnesetImmediatetick happens to drain them today, but any addedawaitinside the chain makes this test fail intermittently.vi.waitForremoves that coupling.♻️ Proposed change
await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch", viewStateId: "view-1" }) - await new Promise((resolve) => setImmediate(resolve)) - const providerAccess = mockClineProvider as ProviderWithPrivateMethods expect(providerAccess.setViewStateId).toHaveBeenCalledWith("view-1") - expect(providerAccess.providerSettingsManager.hasConfig).toHaveBeenCalledWith("view-local-profile") + await vi.waitFor(() => { + expect(providerAccess.providerSettingsManager.hasConfig).toHaveBeenCalledWith("view-local-profile") + }) expect(providerAccess.providerSettingsManager.hasConfig).not.toHaveBeenCalledWith("shared-profile")🤖 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__/webviewMessageHandler.spec.ts` around lines 251 - 259, Update the “validates the view-local currentApiConfigName on launch” test to replace the single setImmediate flush with vi.waitFor. Wait until the expected providerSettingsManager.hasConfig assertion state is reached before performing the existing assertions, without changing the handler or test expectations.
1636-1652: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo tests assert the same behavior.
The block at lines 1636-1644 and the block starting at line 1646 both set
getCurrentTasktoundefinedand assert the same error message. Remove one.🤖 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__/webviewMessageHandler.spec.ts` around lines 1636 - 1652, Remove the duplicate “no active task” test for the downloadErrorDiagnostics message handler, keeping only one test that mocks getCurrentTask as undefined and asserts the “No active task to generate diagnostics for” error.src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts (1)
877-895: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated assertion.
Lines 890 and 891 assert the same condition.
♻️ Proposed cleanup
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 877 - 895, Remove the duplicated viewLocalState assertion in the test case “should not update viewLocalState when durable view-state persistence fails”, keeping a single assertion that viewLocalState does not contain “mode”.webview-ui/src/utils/__tests__/vscode.spec.ts (1)
24-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for a non-object persisted state.
getViewStateIdguards against a stored state that is not a plain object. No test covers that branch. Add a case wherevscodeStateholds an array or a string, and assert that a new id is generated.🤖 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/__tests__/vscode.spec.ts` around lines 24 - 45, Add a VSCodeAPIWrapper test covering a non-object persisted vscodeState, such as an array or string, and assert that getViewStateId generates a new identifier instead of reusing persisted data. Keep the existing storage setup and test structure consistent with the valid persisted-state case.src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
unknown[]instead ofany[].Line 49 already uses
unknown[]for the new mock. Align these two forwarders for consistency and to satisfy the repository rule againstany.As per coding guidelines: "Avoid `as any`; use typed APIs, bracket notation for private members, or precise test doubles and type guards."♻️ Proposed change
vi.mock("../../../api/providers/fetchers/modelCache", () => ({ - getModels: (...args: any[]) => getModelsMock(...args), - flushModels: (...args: any[]) => flushModelsMock(...args), + getModels: (...args: unknown[]) => getModelsMock(...args), + flushModels: (...args: unknown[]) => flushModelsMock(...args), }))🤖 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__/webviewMessageHandler.routerModels.spec.ts` around lines 43 - 46, Update the getModels and flushModels mock forwarders in the vi.mock factory to use unknown[] for their rest parameters, matching the nearby mock and repository typing guidelines; leave their forwarding behavior unchanged.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/src/runTest.ts`:
- Around line 136-164: In the fixture setup near the mode-switch responses,
remove the broad /^<environment_details>/ fixture and its reused Ask response
ID. Retain the toolResultContains fixture for "call_modes_switch_002" so the
Debug flow remains scoped by its tool call ID.
In `@apps/vscode-e2e/src/suite/view-state.test.ts`:
- Around line 33-39: Update the test task lifecycle around teardown and the
second test’s task startup to track all three started task IDs, then wait for
completion or cancel each task before removing messageHandler or allowing the
suite to continue. Ensure teardown handles every tracked task rather than
relying only on globalThis.api.cancelCurrentTask(), preventing late requests and
events from prior tasks.
In `@src/core/webview/ClineProvider.ts`:
- Around line 642-648: Update the on and off overrides in ClineProvider to
remove the eslint suppression and as any cast, using a typed EventEmitter
signature cast for the superclass method before invoking it. Preserve the
existing event and listener parameters and return behavior.
- Around line 354-355: Update loadViewState to merge loaded values into the
existing viewLocalState object instead of replacing it, preserving mutations
made while the asynchronous load is pending. Keep setViewStateId’s full
replacement behavior because it changes view identity, and distinguish these
paths explicitly if sharing loading logic.
---
Outside diff comments:
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 616-630: Update the invalid-profile repair in the provider state
handling to avoid writing only global state via updateGlobalState. When
listApiConfig provides a replacement name, call activateProviderProfile({ name
}) and rely on it to persist the view-local value; when no name exists,
explicitly clear the view-local currentApiConfigName so the invalid value is not
retained.
---
Nitpick comments:
In `@apps/vscode-e2e/src/suite/view-state.test.ts`:
- Around line 212-245: Replace the hardcoded round count of 10 in the expected
event calculation and round-validation loop with the shared plan-derived round
count, reusing the existing ROUNDS or rounds length symbol defined by the test
fixtures. Ensure both expectedSwitches and iteration bounds stay synchronized
with the plan when ROUNDS changes.
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 877-895: Remove the duplicated viewLocalState assertion in the
test case “should not update viewLocalState when durable view-state persistence
fails”, keeping a single assertion that viewLocalState does not contain “mode”.
In `@src/core/webview/__tests__/webviewMessageHandler.routerModels.spec.ts`:
- Around line 43-46: Update the getModels and flushModels mock forwarders in the
vi.mock factory to use unknown[] for their rest parameters, matching the nearby
mock and repository typing guidelines; leave their forwarding behavior
unchanged.
In `@src/core/webview/__tests__/webviewMessageHandler.spec.ts`:
- Around line 251-259: Update the “validates the view-local currentApiConfigName
on launch” test to replace the single setImmediate flush with vi.waitFor. Wait
until the expected providerSettingsManager.hasConfig assertion state is reached
before performing the existing assertions, without changing the handler or test
expectations.
- Around line 1636-1652: Remove the duplicate “no active task” test for the
downloadErrorDiagnostics message handler, keeping only one test that mocks
getCurrentTask as undefined and asserts the “No active task to generate
diagnostics for” error.
In `@src/extension/api.ts`:
- Around line 388-391: Update registerListeners and the RegisteredTask.task type
to store the Task directly when it exposes approveAsk and
handleWebviewAskResponse, removing the undocumented task as unknown as
TaskAskController assertion; otherwise retain the assertion only with a comment
documenting why it is required.
In `@webview-ui/src/utils/__tests__/vscode.spec.ts`:
- Around line 24-45: Add a VSCodeAPIWrapper test covering a non-object persisted
vscodeState, such as an array or string, and assert that getViewStateId
generates a new identifier instead of reusing persisted data. Keep the existing
storage setup and test structure consistent with the valid persisted-state case.
🪄 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: f7671d56-9304-4e8a-9a8a-7d71bdfec931
📒 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 (2)
- webview-ui/src/App.tsx
- src/eslint-suppressions.json
| mock.addFixture({ | ||
| match: { | ||
| predicate: (req) => toolResultContains(req, "call_modes_switch_001", []), | ||
| }, | ||
| response: { | ||
| toolCalls: [ | ||
| { | ||
| name: "attempt_completion", | ||
| arguments: JSON.stringify({ result: "Switched to ❓ Ask mode as requested." }), | ||
| id: "call_modes_post_switch_001", | ||
| }, | ||
| ], | ||
| }, | ||
| }) | ||
|
|
||
| mock.addFixture({ | ||
| match: { | ||
| predicate: (req) => toolResultContains(req, "call_modes_switch_002", []), | ||
| }, | ||
| response: { | ||
| toolCalls: [ | ||
| { | ||
| name: "attempt_completion", | ||
| arguments: JSON.stringify({ result: "Switched to 🪲 Debug mode as requested." }), | ||
| id: "call_modes_post_switch_002", | ||
| }, | ||
| ], | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine aimock fixture match precedence (programmatic vs. registration order, specificity).
fd -t f 'package.json' -d 4 apps/vscode-e2e | xargs -I{} rg -n 'aimock' {}
rg -n --iglob '*aimock*' -g '!**/node_modules/**' -l . 2>/dev/null | head -20
rg -nP --type=ts -C4 'addFixture|findFixture|matchFixture' -g '!apps/vscode-e2e/src/runTest.ts' | head -60Repository: Zoo-Code-Org/Zoo-Code
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runTest outline =="
ast-grep outline apps/vscode-e2e/src/runTest.ts --view expanded 2>/dev/null || true
echo "== relevant runTest sections =="
sed -n '1,230p' apps/vscode-e2e/src/runTest.ts | cat -n
echo "== aimock usages =="
rg -n --iglob '*aimock*' -g '!**/node_modules/**' -l . 2>/dev/null || true
echo "== package references =="
grep -R '"`@copilotkit/aimock`"' . -g 'package.json' 2>/dev/null || true
echo "== lockfile/fetch package metadata =="
fd -t f 'pnpm-lock.yaml|package-lock.json|yarn.lock|package.json' apps/vscode-e2e -d 3 -x sh -c 'echo "--- $1"; sed -n "1,80p" "$1"' sh {}Repository: Zoo-Code-Org/Zoo-Code
Length of output: 10721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== npm metadata for `@copilotkit/aimock` 1.35.0 =="
npm view `@copilotkit/aimock`@1.35.0 version deps dist.tarball name --json
echo "== fetch package contents and relevant source candidates =="
tmpdir="$(mktemp -d)"
tarball="$(npm view `@copilotkit/aimock`@1.35.0 dist.tarball)"
curl -fsSL "$tarball" -o "$tmpdir/pkg.tgz"
tar -tzf "$tmpdir/pkg.tgz" | rg 'package/(dist|cjs|mjs|src|lib).*|package/(test|tests).*|package/README|package/package.json' | head -200
mkdir "$tmpdir/pkg"
tar -xzf "$tmpdir/pkg.tgz" -C "$tmpdir/pkg"
echo "== source search for fixture matching methods =="
rg -n 'addFixture|loadFixture|findFixture|matchFixture|specificity|match' "$tmpdir/pkg/package" \
-g '*.js' -g '*.ts' -g '*.mjs' -g '*.cjs' -g '*.d.ts' -C 3 | head -240
echo "== inspect matching implementation files =="
python3 - <<'PY'
import os, subprocess
pkgdir = os.environ["TMPDIR"]+"/pkg/package" if "TMPDIR" in os.environ else None
if pkgdir is None:
print("pkgdir not set")
PY
ls "$tmpdir/pkg/package"
echo "== inspect likely fixture matching files =="
for f in $(rg -l 'function .*match|addFixture|loadFixture' "$tmpdir/pkg/package" || true); do
echo "--- $f"
wc -l "$f"
sed -n '1,260p' "$f" | cat -n
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
export tmpdir
tarball="$(npm view `@copilotkit/aimock`@1.35.0 dist.tarball)"
mkdir -p "$tmpdir/pkg"
tar -xzf "$tarball" -C "$tmpdir/pkg"
pkg="$tmpdir/pkg/package"
echo "== fixture loader and match diagnostic source =="
for f in $(rg -l 'matchFixtureDiagnostic|loadFixturesFromDir|loadFixtureFile|fixture-loader|fixtures' "$pkg/src" "$pkg/dist" -g '*.ts' -g '*.js' || true); do
echo "--- $f"
wc -l "$f"
rg -n -C 6 'matchFixtureDiagnostic|loadFixturesFromDir|loadFixtureFile|addFixture|lastUserMessage|userMessage|predicate|model' "$f" || true
done
echo "== deterministic match precedence probe from source text =="
python3 - <<'PY'
from pathlib import Path
import re
pkg = Path("/tmp") / Path(__file__).parent.name / "pkg/package" if False else None
# We don't import repo sources; parse distribution source snippets from prior result is not local.
PY
echo "== search for fixture-array ordering in runtime =="
rg -n 'find.*fixture|fixture.*find|filter|sort|reverse|push|splice|matchFixture|userMessage|predicate|model' "$pkg/dist" "$pkg/src" -g '*.js' -g '*.ts' -C 2Repository: Zoo-Code-Org/Zoo-Code
Length of output: 667
🌐 Web query:
@copilotkit/aimock LLMock addFixture fixture matching order predicate userMessage model documentation
💡 Result:
In @copilotkit/aimock (formerly @copilotkit/llmock), the LLMock fixture matching system prioritizes the order in which fixtures are added [1][2]. Fixtures are evaluated in the order they appear in the internal list [1]. Key matching mechanisms include: Fixture Matching Order and Addition - Precedence: When multiple fixtures might match a request, the one added earliest to the server typically takes precedence if they are in the same registry [1]. - Programmatic Control: You can use mockServer.prependFixture to insert a fixture at the beginning of the list (index 0) to ensure it is evaluated before previously registered file-based or programmatic fixtures [1]. - Appending: Standard addFixture or shorthand methods (like.onMessage) generally append to the list [2]. Matching Criteria (Match Object) You can define a fixture using a match object, which supports several properties [2]: - userMessage: Matches based on the user's input (typically as a substring) [1][2]. - model: Restricts the fixture to a specific model identifier [2]. - predicate: A function that receives the request and returns a boolean [2]. This is the most flexible way to match, allowing you to check message roles (e.g., tool results), headers, or other request metadata [1][2]. Because predicates cannot be serialized, they must be registered programmatically rather than via JSON files [2]. Summary of Methods: - Shorthand methods like mock.onMessage(userMessage, response) or mock.on(matchObject, response) simplify registration [2]. - For complex logic, use mock.addFixture({ match: { predicate:... }, response:... }) [2]. - Use mock.prependFixture if you need a catch-all or high-priority override (e.g., handling tool-result messages) to trigger before standard fixtures [1]. Documentation Note: The class name remains LLMock for backward compatibility following the package rename from @copilotkit/llmock to @copilotkit/aimock [3][4]. Refer to the official aimock documentation for the most current API details [5][6].
Citations:
- 1: https://github.com/ag-ui-protocol/ag-ui/blob/1cedb73e/apps/dojo/e2e/llmock-setup.ts
- 2: https://aimock.copilotkit.dev/fixtures
- 3: https://github.com/copilotkit/aimock
- 4: https://www.npmjs.com/package/@copilotkit/aimock
- 5: https://aimock.copilotkit.dev/docs/
- 6: https://aimock.copilotkit.dev/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find fixtures related to modes/config/files =="
git ls-files 'apps/vscode-e2e/fixtures/*' | sed -n '1,120p'
echo "== search local fixture content for environment_details/model regex/tool ids =="
rg -n -C 3 'environment_details|openai/gpt-4-1|call_modes_switch|tools|message' apps/vscode-e2e/fixtures apps/vscode-e2e/src || true
echo "== determine recorded fixture order by parsing repo files =="
python3 - <<'PY'
from pathlib import Path
base = Path("apps/vscode-e2e/fixtures")
print("files:", [str(p) for p in sorted(base.glob("*")) if p.is_file()])
for p in sorted(base.glob("*")):
if not p.is_file():
continue
text = p.read_text(errors="replace")
hits = []
for i,line in enumerate(text.splitlines(),1):
if any(s in line for s in ["environment_details", "openai/gpt-4", "call_modes_switch", "gpt-4.1", "model"]):
hits.append((i,line.strip()))
if hits:
print(f"--- {p}")
for i,line in hits:
print(f"{i}: {line}")
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
Remove the broad /^<environment_details>/ fixture now that both switch flows match by tool call ID.
mock.addFixture() registers in evaluation order, so the new tool-result predicates added before this regex fixture already cover the ask turn for openai/gpt-4.1. Keep only the call_modes_switch_002 request scoped by toolResultContains(req, "call_modes_switch_002", []); delete the broad regex fixture that reuses the Ask response 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 `@apps/vscode-e2e/src/runTest.ts` around lines 136 - 164, In the fixture setup
near the mode-switch responses, remove the broad /^<environment_details>/
fixture and its reused Ask response ID. Retain the toolResultContains fixture
for "call_modes_switch_002" so the Debug flow remains scoped by its tool call
ID.
| teardown(async () => { | ||
| try { | ||
| await globalThis.api.cancelCurrentTask() | ||
| } catch { | ||
| // Task might not be running. | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Stop the three tab tasks before the test ends.
The second test removes messageHandler in finally as soon as the mode assertions pass. The three tasks are still running at that point, and their attempt_completion asks are no longer approved. The teardown hook only calls cancelCurrentTask(), which cancels one task. The remaining tasks stay active and can emit late API requests and events during later suites.
Track the started task IDs and wait for or cancel each one before the suite continues.
The E2E guidelines require accounting for late asynchronous requests from prior tasks.
Also applies to: 267-270
🤖 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 33 - 39, Update
the test task lifecycle around teardown and the second test’s task startup to
track all three started task IDs, then wait for completion or cancel each task
before removing messageHandler or allowing the suite to continue. Ensure
teardown handles every tracked task rather than relying only on
globalThis.api.cancelCurrentTask(), preventing late requests and events from
prior tasks.
Source: Coding guidelines
| // Load initial state from global state into viewLocalState buffer after dependencies used by getState are ready. | ||
| void this.loadViewState() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
loadViewState can discard view-local values written before it resolves.
The constructor starts loadViewState() without awaiting it. loadViewState then assigns this.viewLocalState = loadedState, which replaces the whole object. Any mutation that lands during the pending load (for example setValue("mode", ...) or saveViewState("apiConfiguration", ...)) is lost when the load resolves. The same overwrite happens in setViewStateId, which the webview triggers on webviewDidLaunch.
Merge the loaded values instead of replacing the object, or track a load generation and skip stale results.
🐛 Proposed fix to merge instead of replace
- this.viewLocalState = loadedState
+ // Merge so mutations that landed while the load was in flight are preserved.
+ this.viewLocalState = { ...loadedState, ...this.viewLocalState }
this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`)For setViewStateId, a full replace is correct because the view identity changed. Consider passing an explicit flag so the two cases stay distinct.
Also applies to: 590-621
🤖 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 354 - 355, Update
loadViewState to merge loaded values into the existing viewLocalState object
instead of replacing it, preserving mutations made while the asynchronous load
is pending. Keep setViewStateId’s full replacement behavior because it changes
view identity, and distinguish these paths explicitly if sharing loading logic.
| override on<K extends keyof TaskProviderEvents>( | ||
| event: K, | ||
| listener: (...args: TaskProviderEvents[K]) => void | Promise<void>, | ||
| ): this { | ||
| return super.on(event, listener as any) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return (super.on as any)(event, listener) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the as any casts with a typed cast.
The coding guidelines require new TypeScript code to avoid as any and to avoid lint suppressions. Cast to the EventEmitter signature instead.
♻️ Proposed typed cast
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)
+ return (super.on as (event: K, listener: (...args: TaskProviderEvents[K]) => void) => this)(
+ event,
+ listener as (...args: TaskProviderEvents[K]) => void,
+ )
}Apply the same change to off.
Also applies to: 657-658
🤖 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 - 648, Update the on and
off overrides in ClineProvider to remove the eslint suppression and as any cast,
using a typed EventEmitter signature cast for the superclass method before
invoking it. Preserve the existing event and listener parameters and return
behavior.
Source: Coding guidelines
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Add comprehensive end-to-end tests verifying per-view state isolation correctness across different scenarios. Covers cross-panel, follow-up mode, and view-local value isolation behavior.
Changes
view-state.test.ts(272 lines) covering:modes.json+view-state.tsfixture setupFiles Changed (7 files, +496 / -3)
suite/view-state.test.tsfixtures/view-state.tsrunTest.tsfixtures/modes.jsonExtensionStateContext.spec.tsxApp.tsxApp.spec.tsxTest Coverage
Test Notes
This PR includes commits from base-1, base-2, and base-3 (linear dependency chain), so all tests pass. The E2E tests specifically verify that
setValues()from base-2 correctly persists per-view state.Related