fix(webview): isolate parallel mode and provider profile writes (vps2 F3) - #1560
fix(webview): isolate parallel mode and provider profile writes (vps2 F3)#1560easonLiangWorldedtech wants to merge 30 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Fix the failing required CI checks; awaiting-maintainer requires CI and automated review completion. Review-state labels are managed by this workflow; do not edit them manually. |
8da5c6e to
7d91ff4
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
39423f2 to
510689f
Compare
510689f to
7f69ecf
Compare
…en view-identity tests Track the in-flight tab panel creation with a module-level promise so concurrent openClineInNewTab calls reuse one panel and provider (adds a Promise.all regression test). ClineProvider.spec sets the private view via the public resolveWebviewView() instead of a ts-ignore assignment. registerCommands.spec types evictCurrentTask/refreshWorkspace on the fixture and drops the as any attachment. eslint-suppressions: prune the registerCommands.spec.ts entry (two as any suppressions removed).
…n the concurrency assertion
…-bar posts - openClineInNewTab: extract the unserialized creation body into createTabPanelUnlocked and guard the in-flight slot clear so a settled creation cannot clobber a replacement already stored in the slot. - onDidDispose: clear the tracked tab ref only when the disposing panel is still the tracked one, so a late disposal of a replaced panel cannot clobber the replacement's ref. - MDM lookup failure: log the fallback to the output channel instead of swallowing it silently. - Route the six title-bar button handlers through a shared postActions helper that posts each action in order and logs failures with the handler-specific prefix. - package.json: add the four InTab commands to the command palette, scoped to the active tab panel. - Tests: handler-level regression for openInNewTab + popoutButtonClicked started before the first creation resolves; fresh-creation test for a settled in-flight promise; stale-panel disposal regression; retained panel assertion for disposed tab instances; rightmost-editor column placement assertion; MDM fallback output assertion; %s placeholders for primitive it.each titles. - Stryker directives for the two equivalent setPanel type-literal mutants (setPanel branches only on type === sidebar).
7f69ecf to
f9233a4
Compare
Replace the weak toBeDefined() assertion in the dispose spec with an identity check against the panel returned during creation, per the CodeRabbit actionable comment on this PR (review run 7c4cfeb3-6dd9-4615- 9a58-70cfc705eca2). The tracked tab is now pinned with toBe(panel) before the dispose assertions, so a wrong or duplicated tracked panel fails the suite instead of passing a defined-only check. Upstream: Zoo-Code-Org#1528 (vps2 F0)
cac941f to
210899f
Compare
Retain the tracked tab panel in the InTab handler cases and assert that getInstanceForView was called with that exact panel, per the CodeRabbit actionable comment on this PR (review run 4afe1273-8739-4235-90d3-311db5f6ccb9, inline comment 3952466254 on the tabHandlerCases spec). A handler resolving any other view now fails instead of passing on the stubbed provider result alone; the same identity pin is applied to plusButtonClickedInTab. Upstream: Zoo-Code-Org#1528 (vps2 F0)
…States Each ClineProvider instance now owns a unique viewId (renderContext plus a monotonic counter) and registers a stable viewStateId for durable persistence. - Per-view state buffer (viewLocalState) holds mode / currentApiConfigName / apiConfiguration overrides in memory; saveViewState persists the non-secret subset durably under the active view id, rekeyed to the stable id on registration. - viewStates is stored as a map pruned to the newest 50 entries; writes go through a serialized queue so concurrent provider instances merge without lost updates. - setViewStateId sanitizes ids and rejects "__proto__" so a per-view entry can never be keyed through the Object.prototype setter. - postMessageToWebview no longer awaits the webview ack: a remounted or disposed page never acknowledges, and awaiting would wedge task-critical callers. - History restore falls back to the default mode view-locally instead of writing the shared global mode. - GlobalState gains the "viewStates" key and GLOBAL_STATE_KEYS tracks it. Adds F1a coverage in ClineProvider.spec.ts (viewId uniqueness, saveViewState persistence semantics, loadViewState fallback and failure, pruning, the __proto__ guard) and adapts the two history-restore tests in ClineProvider.sticky-mode.spec.ts to the view-local restore. getState() merging of hydrated per-view values and the remaining view-state suites land in the follow-up (F1b).
…lude viewStates from settings transfer
210899f to
8576b18
Compare
…ions through the view-local buffer
… mocks API.setConfiguration now routes through ClineProvider.setValues (8576b18 "fix(api): route setConfiguration through ClineProvider.setValues"), but the pre-existing spec still shaped the provider mock around the old contextProxy.setValues call. Both tests in this file failed with "this.sidebarProvider.setValues is not a function" on the platform-unit-test runs (F3 Zoo-Code-Org#1560 windows unit, job 101863508169, run 34161293841). Add the provider-level setValues mock to both tests to match the new routing. Upstream: Zoo-Code-Org#1560 (vps2 F3)
2d46019 to
b80d59a
Compare
…nd target tab-instance commands Reapply in-flight view-local fields with Object.is identity so a field cleared during the load window stays cleared; route mode switches through setValue so the in-memory buffer and durable write agree, with rollback on failure; refresh cross-instance view-local state on profile upsert, activate and delete and re-pin the buffer after a delete; point focusInput and active-panel re-registration at the tracked tab provider and panel; log dropped webview postMessage failures with the message type; pin tab-instance, focusInput and active-panel identity in the registerCommands tests and type the mdm double in the provider spec.
…overrides Fold ClineProvider viewLocalState on top of ContextProxy values in getState() (mode, apiConfiguration, and all per-view fields) so each webview reports its own selections while falling back to shared global state for everything else. Ports the getState-merging and local-state-isolation spec coverage from the superseded vps2 source. Also pins the full default surface of the merged read path, including the apiConfiguration provider fill-in when provider settings sanitize the raw value away (mutation-diff gate).
…tore deleteProviderProfile only rewrote the UI-facing listApiConfigMeta and currentApiConfigName in ContextProxy, leaving the profile's settings in the ProviderSettingsManager store (context.secrets). Per-mode mappings (modeApiConfigs) that still pointed at the deleted profile re-activated its stale settings on the next handleModeSwitch, clobbering the active configuration: the subtask child profile's gpt-4.1-mini leaked into ask-mode tasks, breaking downstream e2e suites (60s timeouts on search_files no-match and terminal reuse after zero-chunk shell race). Purge the profile from the store on delete so dangling mode mappings can no longer resolve it: listConfig().find(id) fails and handleModeSwitch continues with the current configuration. The F3 mode/profile isolation commit further up the chain introduces the same purge plus per-view pin handling. Regression test: sticky-profile spec "deleteProviderProfile removes the stored profile so a dangling mode mapping can no longer re-activate it".
handleModeSwitchUnlocked persisted the switched mode only through the deprecated updateGlobalState, so the in-memory viewLocalState buffer kept serving a stale restored mode: getValues() merges viewLocalState on top of the ContextProxy values and would shadow the fresh switch for consumers. Route the write through setValue so the buffer and the durable global state stay in sync, and cover it with a regression test for switching after a restored view state. Also assert the restored mode in public state, and harden the import viewStates test with a write-tracking proxy across all write paths (setValues/setValue/setProviderSettings) for seeded and fresh machines.
…isolate launch-suite provider doubles ProviderSettingsManager.deleteConfig now throws ProviderSettingsNotFoundError for a missing config and rethrows it unwrapped, so ClineProvider.deleteProviderProfile branches on the type instead of matching the not-found message text that a profile name could spoof; the sticky-mode handleModeSwitch test additionally pins the durable per-view persisted mode and getValues(); the webviewDidLaunch tests restore the mockClineProvider members they replace after each test so launch stubs cannot leak.
… state WMH webviewDidLaunch persists the webview view-state id via provider.setViewStateId and re-pins the view-local currentApiConfigName through provider.saveViewState when the view-local profile is missing but the shared global selection is still valid. updateSettings is routed through provider.setValue so view-local buffer and pin sync stay consistent with the other mutation paths. The webview VSCodeAPIWrapper gains a stable getViewStateId persisted via setState (with an in-memory fallback) and the launch effect posts the id with the webviewDidLaunch message.
…ate ids getViewStateId now trims and rewrites unsafe characters before reuse, mirroring ClineProvider.setViewStateId, and rejects whitespace-only and __proto__ values by generating a fresh id. Regression coverage: normalized reuse, whitespace-only, and __proto__.
…d profile has no name The launch-time re-pin guard required the first listed profile to carry a name, so with a legacy nameless profile the still-valid shared global selection fell into the repair branch and was cleared with undefined for every view. Drop the name requirement: the view-local re-pin only needs the shared selection to resolve. Cover the combination (valid global selection + nameless first listed profile) and assert the shared selection is never written as undefined. Also pin the exact fixture values for the getState merging case instead of toBeDefined.
…l buffer Flat provider-settings writes now flow through the ContextProxy only; merging them into viewLocalState.apiConfiguration turned them into a per-view override that masked later shared updates from other views. Add an updateSettings test pinning the provider.setValue write path, and retarget the tests that pinned the removed merge behavior.
…d dispose retention
… describe by subject Tie-case pruning and editor-teardown retention now assert the full survivor objects instead of mere presence, and the durable viewStates persistence tests move out of the getState merging block into a persisted view state describe so each block name matches its subject.
Port vps2 F3 (mode/profile writes) from upstream 978, hunk-by-hunk against the F1c..CS residual: - ClineProvider: add repointPersistedViewStates() to re-point currentApiConfigName across per-view entries when a profile is renamed or replaced, and prune orphaned entries; validate handleModeSwitch slugs against the custom-modes manager and no-op with a log on unknown modes; drop the as-any cast in delegateParentAndOpenChild. - Task: route mode switches through provider.handleModeSwitch(task) and keep the submitted message on failure instead of setMode(). - SwitchModeTool and specs: durable per-view mode writes. - webviewMessageHandler: no change vs base - the kimi-code OAuth hunk in the residual is CS-only divergence (not-ported register item 1), not part of Zoo-Code-Org#978/Zoo-Code-Org#979. - webviewMessageHandler.spec: ported only Zoo-Code-Org#979's 4 mock fields + defaultModeSlug import; the stack-side legacy-repair test, Key-aware getValue mock and em-dash comment are retained (register item 4). Zoo-Code-Org#979's mode-routing WMH.spec describe ("routes mode messages through handleModeSwitch instead of writing ContextProxy directly") exists in neither CS nor the stack and is ported by no unit (open question, logged). - Tests: H3/H4 durable handleModeSwitch writes in ClineProvider.spec.ts; profile-mutation, profile-activation and handleModeSwitch-integration describes (incl. A4 non-focused-target regression and new mutation-killing tests) in ClineProvider.parallelMode.spec.ts; sticky-mode and webviewMessageHandler spec updates; retain the setViewStateId __proto__ guard + spec test - shipped F1a hardening; the residual's guard removal is lineage divergence, not F3 content. - eslint-suppressions.json: no-explicit-any counts decrease for core/webview/ClineProvider.ts (12 -> 11) and core/webview/__tests__/ClineProvider.sticky-mode.spec.ts (36 -> 33). Upstream: Zoo-Code-Org#978 (vps2 F3) - issue Zoo-Code-Org#978; content ported hunk-by-hunk from the F1c..CS residual, cross-checked against upstream PR
Route the CS API setConfiguration write through ClineProvider.setValues instead of the raw context proxy so the view-local subset stays in sync with the sidebar view's state. A direct context-proxy write leaves the view-local apiConfiguration stale; the mode-switch / profile-activation paths re-apply the current provider settings to the focused task on every user message, so a task started after an API setConfiguration runs with the stale configuration (wrong model / reasoning flags / endpoint). That is the deterministic 13-test e2e provider-suite failure (72/13) on this head: the provider suites configure the provider via setConfiguration and the mock serves fixtures by the model the request actually carries. Ported from the F5 head (vps2 F5) where the e2e provider suites pass; includes the unit spec asserting the routing. Upstream: Zoo-Code-Org#982 (vps2 F3)
… mocks API.setConfiguration now routes through ClineProvider.setValues (8576b18 "fix(api): route setConfiguration through ClineProvider.setValues"), but the pre-existing spec still shaped the provider mock around the old contextProxy.setValues call. Both tests in this file failed with "this.sidebarProvider.setValues is not a function" on the platform-unit-test runs (F3 Zoo-Code-Org#1560 windows unit, job 101863508169, run 34161293841). Add the provider-level setValues mock to both tests to match the new routing. Upstream: Zoo-Code-Org#1560 (vps2 F3)
The F1a buffer routing and the new delete-time activation branch interacted in two ways: the unrelated-pin branch re-wrote currentApiConfigName through setValue although the view-local buffer already holds the surviving pin (drop the write: it only triggered a viewStates prune pass and could clobber the pin with the shared slot value), and deleting the current profile now activates the fallback, which reads its settings via providerSettingsManager, so the spec test uses a test double like its siblings in the describe.
…tion mutations - E1: move the provider-release switch test into switchModeTool.spec where the switchModeTool layer is the narrowest proof - E2: assert the state postMessage shape in the mode-lock switch test - E3: prune invalid persisted view-state entries before applying the cap - wrap the upsert and activate buffer-refresh calls so the view-local apiConfiguration buffer tracks the activated profile again; flat provider-settings keys no longer merge into the buffer
b80d59a to
065116c
Compare
Draft PR — vps2 unit F3 (mode/profile writes).
Tracking: easonLiangWorldedtech#41 (vps2 series ledger). Upstream issue: #1559 (this series' gap record; the original upstream bug is #978). Port source: the F1c-head-to-CS residual, cross-checked against upstream PR #979 (fix(webview): isolate parallel mode and provider profile writes) — closed/superseded by the vps2 series; the #41 ledger names #979 as the F3 port source.
Scope
10 files, 919 insertions, 65 deletions (measured vs stack base aa0f3b1):
Budget
Mutation gate trust note
Port fidelity (coordinator-verified against the vps2 CS e9a44b2)
Structural note on the parallelMode spec (coordinator-verified)
The CS parallelMode.spec.ts is 1790 lines: a shared preamble (L1-672), an F1-series test section (L673-1363: viewId uniqueness, local state isolation, saveViewState, stale temporary-id load), the F3 describes (L1364-1673), a blank separator (L1674), and the F4 multi-instance describes (L1675-1790). In this series the F1-series section lives in ClineProvider.spec.ts (shipped by F1a/F1b/F1c — the deliberate F1-series describe restructure), and the F2 unit shipped the persisted-pruning and #1065 retention tests inside the parallelMode file (they are absent from the CS parallelMode file). Both placements are behaviorally covered; the divergence is structural, not a coverage gap.
Series mechanics