Skip to content

fix(webview): send stable view-state id on launch and re-pin per-view state - #1552

Open
easonLiangWorldedtech wants to merge 93 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:vps2/f1c-webview-identity
Open

easonLiangWorldedtech wants to merge 93 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:vps2/f1c-webview-identity

Conversation

@easonLiangWorldedtech

@easonLiangWorldedtech easonLiangWorldedtech commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Part of the vps2 durable per-view state series — tracked in fork issue 41 (cross-repo: the displayed vs-main diff includes the F1a/F1b content this unit stacks on until those merge).

Issue (created at PR-open time): #1551

What this unit lands at this head

Fix unit F1c (3/3 of the F1 split). F1a landed per-view identity and the durable viewStates pipeline, F1b merges view-local state into getState(); this unit closes the loop on the webview side:

  1. Stable view-state id at launch — the webview owns a viewStateId (VSCodeAPIWrapper.getViewStateId: reuse the id persisted in webview state, else create one via crypto.randomUUID with a timestamp+random fallback and persist via setState; in-memory fallback when storage is unavailable). It is posted with webviewDidLaunch and persisted on the provider via setViewStateId, re-keying the pre-launch temporary entry to the launching webview. Best-effort: the launch message carries viewStateId: undefined when the helper is unavailable and the provider degrades to the shared-global path.
  2. Launch-time re-pin — when the view-local API profile is invalid at launch, the view is re-pinned to the still-valid shared global selection with a view-local write only (saveViewState); the legacy global repair (global write + activateProviderProfile) runs only when the shared global selection is also invalid. Validation order: merged (view-local) name, then shared global, then first listed profile.
  3. updateSettings routed through provider.setValue so the view-local buffer/pin sync path (_saveViewLocalStateFromMutation) runs for settings edits too.
  4. F1a shared-code fixes ported in (this round): temporary-id ownership (wroteUnderTemporaryViewStateId), shared-write rollback in setValue / setValues, null-prototype getPersistedViewStates, plus the ported regression tests.

Content source

Hand-ported from CS e9a44b2fa (fork PR 43 head): the launch-identity hunks in webviewMessageHandler.ts / webview-ui/src/utils/vscode.ts and their specs; the F1a/F1b snapshot on this branch carries the same review-round fixes as the lower units' heads (ported this round, with two tests adapted to the ownership semantics).

Budget

a+d vs merge-base fafad20b0: 6234 (+5814/−420) — includes the stacked F1a/F1b content until those merge; the F1c-specific delta over the F1b unit is ~545 a+d (launch identity + re-pin + specs). Split decision belongs to maintainers.

How to test

  • pnpm --dir src exec vitest run core/webview/__tests__/ClineProvider.spec.ts core/webview/__tests__/ClineProvider.sticky-mode.spec.ts core/webview/__tests__/webviewMessageHandler.spec.ts activate/__tests__/registerCommands.spec.ts
  • pnpm --dir webview-ui exec vitest run src/utils/__tests__/vscode.spec.ts src/context/__tests__/ExtensionStateContext.spec.tsx
  • Manual: open a tab, switch its mode, reload the window — the tab restores its own mode under the same stable id; a pre-launch temporary entry is re-keyed to the launched webview's id.

Verification numbers

  • eslint --max-warnings=0 on all changed files (src + webview-ui): pass; suppression counts never increased (ClineProvider.spec.ts 196→194 re-measured, ClineProvider.ts 7, sticky-mode.spec.ts 26, registerCommands.ts 1).
  • vitest: ClineProvider.spec + sticky-mode.spec + registerCommands.spec 364 pass; webviewMessageHandler.spec 88 pass; sticky-profile + taskHistory + ContextProxy + importExport + WebviewFocusTracker specs 149 pass; webview-ui vscode.spec 15 pass + ExtensionStateContext.spec 25 pass; types global-settings.test 9 pass (3 ProviderSettingsManager migration tests fail locally only — they read a types constant the stale local types build lacks; CI builds types from source).
  • tsc --noEmit: no new real errors (all local errors are the stale-types class; CI builds types from source).
  • Negative control on the ported ownership fix: reverting the first-write claim reddens exactly the ported regression test; restored byte-exact (sha256 match).
  • prettier --check on the full merge-ref tree (LF-normalized): pass.
  • mutation-diff: advisory budget overage on the extension package is inherited from the stacked shared content, not new in this unit; maintainer decision.

Updated this round

  • Merged upstream/main (conflicts in registerCommands.ts, ClineProvider.ts, both src spec fixtures, and ExtensionStateContext.spec.tsx resolved keeping both sides; constructor call sites gained the webviewFocusTracker argument).
  • Ported the F1a review-round fixes into the shared per-view code (ownership, authorship-after-persistence, setter rollback, null-prototype map) with the regression tests; adapted two pre-existing tests to the ownership semantics.
  • eslint-suppressions re-measured; PR body rewritten.

Pre-merge ERROR-row pre-check (head 0657506)

  • Security Boundaries / view-state id rows: addressed at 0657506. setViewStateId now rejects non-string ids (crafted postMessage), bounds the key length (MAX_VIEW_STATE_ID_LENGTH), and refuses ids already owned by another live instance; a refused view stays on its temporary id against shared state. Regression tests: non-string, oversized, claimed-by-another-view. Negative controls: disabling the ownership check reddens exactly the claim test; widening the length bound reddens exactly the oversized test; restores byte-exact (hash match).
  • Persistence Integrity setValues/setValue ordering rows: verified already addressed by the pushed code: _saveViewLocalStateFromMutation persists the non-secret subset before updating the buffer; the re-key is a single queued write (no delete-then-write window; authorship clears only after the write lands); the mutation hook keeps the buffer synced across setValue/setValues and profile upsert/activation/deletion.
  • Proofs at 0657506: sticky-mode 33/33; eslint --max-warnings=0 clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.

CR round: non-activating-save refresh + overlay completeness (head 16a9ba2)

This unit already implemented both behaviors the review round named on the get-state unit (non-activating saves refresh views pinned to the saved profile; the pinned overlay is used as a complete profile snapshot with only an apiProvider fallback). The round's two regression tests now land here as a test-only commit; the cloud instance doubles gained on/off handlers because provider dispose detaches the settings listener against the getter those tests leave in place.

  • Regression tests: "refreshes views pinned to a profile saved without activation"; "does not leak shared provider keys into a pinned view's api configuration; the zoo-gateway callback test asserts the upsert path (already asserted here).
  • Negative controls: dropping the else-branch refresh reddens exactly the refresh test, reverting the merge semantics reddens exactly the leak test; restores byte-exact (hash match).
  • Proofs at 16a9ba2: ClineProvider.spec green, sticky-mode and webviewMessageHandler green, eslint --max-warnings=0 clean, tsc clean except the two known local telemetry-dist noise lines, prettier merge-ref gate ALL-MATCHED.

Follow-up: overlay read from the view buffer (required-lane fix)

The previous round's overlay-completeness change read the overlay from the merged state values, which also carry the raw apiConfiguration state key. That key is a partial patch written by shared-store paths, not a view snapshot: with no overlay present and the raw key partial, getState() served only the partial object and dropped the shared provider settings the previous merge produced (the parallel-mode activation tests caught exactly this - the activated profile's model id vanished; required lane red on the units carrying that spec). The overlay is now read from the view buffer itself, where a present value is by contract a complete profile and an absent one means the shared store serves the settings.

Proofs: ClineProvider.parallelMode.spec green where present (the six synchronization tests that regressed), ClineProvider.spec green including the overlay-completeness tests, sticky-mode and webviewMessageHandler green; negative control: reverting the overlay to a shallow merge over the shared settings still reddens exactly the leak test; restores byte-exact (hash match); eslint --max-warnings=0 clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.

Follow-up: active task handlers rebuilt on non-activating saves (head aa1fb11)

A review round on the view-identity unit found that the non-activating-save refresh updated pinned views' buffers but not their active tasks' API handlers, and the posted state prefers the task's own apiConfiguration: a conversation running in a view pinned to the saved profile kept sending the old token (e.g. after the Zoo Gateway callback rotated it) even though the view's buffer had caught up. The refresh now rebuilds the affected tasks' handlers in the pinned views, and the non-activating branch rebuilds this view's own handler when it holds the pin.

Proofs: ClineProvider.spec green (the pinned-sibling refresh test now asserts the active task's handler is rebuilt with the saved settings, not just the private buffer); negative control: dropping the rebuild from the refresh reddens exactly that assertion, restores byte-exact (hash match); parallelMode/sticky-mode/webviewMessageHandler green; eslint --max-warnings=0 clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.

Follow-up: test-quality round from the review (head 7b9f12b)

The review round on the api-wiring unit named four test-quality issues; the ones present in this unit are fixed here: the viewStates describe block moved out of the alwaysDenyUnapprovedCommands block (test names carried the wrong subject), the prune test's toBeDefined() survival checks replaced with exact-shape toEqual assertions (they pass even if pruning strips fields), the stale-temporary-entry test's fixed 10 ms wait replaced with awaiting the load itself (the negative assertion could pass vacuously on a slow load), and the readiness-cancel test's fake timers restored in a finally block (a failed assertion left fake timers active and hung a later real-timer test).

Proofs: global-settings 9/9 where present, ClineProvider.spec green, sticky-mode green where present, api-task-control green where present; negative controls: pruning that strips updatedAt reddens exactly the prune test, a loadViewState that adopts the unowned temporary-id entry reddens exactly the stale-entry test, restores byte-exact (hash match); eslint --max-warnings=0 clean (src and types); prettier merge-ref gate ALL-MATCHED.

Follow-up: review-round fixes from the series (head 8f6749a)

A review round on the mode-profile-writes unit found that the profile-deletion re-pin updated each sibling view's buffer but not its running task: the posted state prefers the task's own apiConfiguration, so a conversation in a view pinned to the deleted profile kept sending the deleted profile's settings. The re-pin now rebuilds the sibling's API handler with the replacement's settings and re-points the task's sticky profile name, mirroring the activation path.

Proofs: ClineProvider.spec and sticky-mode green; eslint --max-warnings=0 clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED. The rebuild's negative control (dropping it reddens exactly the sibling re-pin test, byte-exact restore) runs in the units that own that test.


Updated this round (CodeRabbit follow-up, head 279e9da)

  • Non-activating profile saves no longer force-rebuild the API handler of a view whose active task follows a different sticky profile: both rebuild sites are guarded by taskApiConfigName === saved profile; pinned-view overlays still refresh. Regression tests cover both sites (negative control: removing either guard reddens exactly its test).
  • Test hygiene: prune assertions compare full kept entries; the two re-key storage tests seed viewStates through the durable write path so their assertions can fail; the webview state-payload test renamed to describe what it asserts.
  • Verified: ClineProvider.spec 287 green, sticky-mode 33 green, eslint --max-warnings=0 clean, tsc real=0, prettier clean.

Updated this round (CodeRabbit follow-up 2, head 23838ac)

  • Deleted-profile re-pin: the forced task-handler rebuild and the sticky-name rewrite now require taskApiConfigName === deleted profile (the view-buffer re-pin stays unconditional), so a pinned view whose task follows another sticky profile keeps its conversation's settings and persisted profile name. Regression test included (negative control: removing the guard reddens exactly that test).
  • Verified: ClineProvider.spec 288 green, sticky-mode 33 green, eslint --max-warnings=0 clean, tsc real=0, prettier clean.

Stryker suppression comments removed

Per the AGENTS.md policy (never add Stryker directives; remove them when a file is touched), all // Stryker disable directives were dropped from this PR. Equivalence rationale for the removed suppressions:

  • Stryker disable next-line ConditionalExpression: defensive clobber guard, unreachable per the ordering argument above.
  • Stryker disable next-line StringLiteral: setPanel branches only on type === "sidebar", so any other literal routes to the identical tab-ref assignment
  • Stryker disable next-line StringLiteral: setPanel only distinguishes "sidebar"; any other value routes to the tab-ref assignment
  • Stryker disable next-line ConditionalExpression,BlockStatement: hostile non-stabilizing encodings are unreachable from the webview (its posts are plain link targets); the bound is defensive
  • Stryker disable next-line CallExpression: hostile non-stabilizing encodings are pinned by the direct decodeUntrustedPathToStable bound test; the webview never posts such values
  • Stryker disable next-line ConditionalExpression,OptionalChaining: equivalent mutant - when localStorage is unavailable the guard-false path and the throwing body both return this.fallbackState from this catch
  • Stryker disable next-line ConditionalExpression,OptionalChaining: equivalent mutant - when localStorage is unavailable the guard-false path and the throwing body both mark the write failed and return newState

CR round 2026-10-15

  • Tab disposal now re-points the tracked ref: live tab panels are tracked in a module-level set and the dispose handler falls back to the best remaining panel (active, then visible, then any) instead of clearing, so *InTab commands keep a target when the tracked panel closes while another survives in a different group. Two regression tests added.
  • The six remaining @ts-ignore readonly-manager swaps in the provider spec replaced with vi.spyOn on the real instances (customModesManager x2, providerSettingsManager x3, launch re-pin test).
  • The webviewDidLaunch suite now restores all six reassigned mockClineProvider members in its own afterEach via a module-scope snapshot; the telemetry suite's partial snapshot and 3-of-6 restore removed.

CR round 2026-10-15 (b)

  • Tab disposal listeners are now registered before the first initialization await, and initialization failure drops the half-registered panel from the live registry and disposes the provider (its disposal listener re-points or clears the tracked ref), so a close landing mid-init cannot leave the panel tracked. Two regression tests added.
  • setValues now validates mode against custom modes carried in the same payload (createTask/setConfiguration register them only after the call), so a brand-new custom mode in the payload is no longer dropped. Regression test added; the identical block in the sibling units received the same fix.

CR round 2026-10-15 (c)

  • ClineProvider.dispose() now deregisters (activeInstances removal, listener teardown, MCP provider unregistration) in a finally path, so a throwing MCP disposable cannot leave the provider registered while _disposed blocks a retry; the tab-init failure cleanup nests the dispose await so a cleanup rejection cannot replace the initialization error. Two regression tests added; the identical dispose tail in the sibling units received the same fix.

CR round 2026-10-15 (e)

  • Ported: createTask restores the previous mode selection when custom-mode registration rejects (identical flagged block; regression test added). The wider dispose() finally was not ported here: this unit's method body differs outside the flagged block.

CR round 2026-10-15 (f)

  • The createTask registration-failure rollback now restores the shared mode and the view's own mode pin independently (unset pin stays unset); divergent-pin regression test added. Same fix in all six units carrying the flagged block.

Row-9 update (head d48f192)

  • Activation writes from the stored record (upsertProviderProfile read-back feeds every post-save consumer; CR Security Boundaries).
  • Clear/re-point cache rollback on durable write failure (CR Persistence Integrity).
  • Activation rollback restores the exact pre-activation viewStates map, recovering entries the pruning evicted (CR Persistence Integrity).
  • Deletion re-points closed views' durable pins onto the surviving profile (CR Persistence Integrity).

Verification: new tests are focused negative controls (reverting each guard reddens exactly the matching test); ClineProvider.spec, webviewMessageHandler.spec spot-checks, api specs, spec-inclusive tsc --noEmit, eslint --max-warnings=0 (suppression counts unchanged) and prettier all green on this head.

Upstream refresh (head d48f192)

Merged the current upstream/main (merge commit; the row-9 fix commit is unchanged and is the first parent) to clear the review gate's mergeability hold. Upstream refactored upsertProviderProfile into a new persistProviderProfile method and added a runToCompletion queue option; this unit's transactional body was ported into the new method unchanged and upstream's Promise.allSettled activation fix was adopted in it. ClineProvider.spec, webviewMessageHandler.spec, tsc --noEmit, eslint (suppression counts unchanged) and prettier re-verified green at this head. The first refreshed head's windows unit-test lane caught that upstream's new updateProfileModel tests in ClineProvider.apiHandlerRebuild.spec.ts mock the settings manager with a static getProfile, which starves the post-save read-back; the mock now repoints getProfile at what saveConfig wrote (the real manager's behavior), and the file's 38 tests pass locally alongside the rest of the verification set.

Review round 6 - the launch re-pin seeds the view's settings (aa6037c)

The live row on this head was right about the mechanism: the launch repair re-pinned the view to the still-valid shared profile and cleared the apiConfiguration buffer, leaving a view with a name in its overlay and no settings of its own. getState merges viewLocalState over the shared values (ClineProvider.ts:4913) and a mutation carrying undefined deletes the key (:5298-5304), so the pinned view resolved apiConfiguration from whatever the shared store names next - one sibling activation is enough - while displaying its own pin. The repair now seeds the pinned profile's stored record without its name, the way loadViewState pairs a name with its settings (:1137-1140).

Two deliberate deviations from the suggested snippet: the seed is read before the pin write, so a failed seed leaves the pin unwritten and the invalid selection retryable (the property the replaced comment was protecting), and the read is wrapped so a record that vanishes between hasConfig and getProfile falls back to an empty overlay instead of costing the view the whole repair.

One part of the finding is not what the code does: nothing writes apiConfiguration into the durable view entry - _persistViewLocalStateFromMutation (:5322-5332) persists mode and currentApiConfigName only - so the seed is an in-memory overlay write and the pin write stays the only durable part of the repair.

Control: head handler with the new spec gives 3 failed | 173 passed (two cases with getProfile called zero times, one on calls[0] equal to ["apiConfiguration", undefined]); with the seed in place it is 176 passed, exit 0, and core/webview as a whole is 34 files / 1073 tests green. src tsc exit 0; eslint clean on both touched files with suppressed counts unchanged (5 and 35, total 3298); the compile-job command list is 0,0,0,0,1,1,0,0,0,0,0,0,0 with only the two box-local solheim record.sh reds (ffprobe, POSIX separators) - format:check, lint and check-types are green in this worktree.

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-author PR is waiting for the author to address requested changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants