feat(webview): add viewStateId generation and persistence infrastructure - #1183
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
📝 WalkthroughWalkthroughThe PR adds persistent identifiers for webview instances and stores per-view mode and provider configuration in validated global settings. ChangesPer-view state persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
src/core/webview/ClineProvider.ts (3)
556-565: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
clearPersistedViewStatewrites back without pruning.
savePersistedViewStateappliesprunePersistedViewStatesbefore writing.clearPersistedViewStatewritesstatesdirectly. The two paths therefore apply different invariants to the same key. A clear operation can restore an over-cap map that a prior save had trimmed, because the fresh read returns whatever is currently stored.Apply the same pruning in both paths.
♻️ Proposed fix
const states = this.getPersistedViewStates({ fresh: true }) delete states[viewStateId] - await this.contextProxy.setValue("viewStates", states) + await this.contextProxy.setValue("viewStates", this.prunePersistedViewStates(states))🤖 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 556 - 565, Update clearPersistedViewState to pass the states through prunePersistedViewStates before setValue, matching the existing savePersistedViewState write path and preserving the persisted view-state cap.
1927-1942: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant view-state writes for one logical mutation.
saveViewStatealready calls_saveViewLocalStateFromMutation. This block callssaveViewStatetwice inside thePromise.all, then calls_saveViewLocalStateFromMutationagain with the samecurrentApiConfigNameandapiConfiguration. The result is thatcurrentApiConfigNameis persisted twice and the local cache is written three times, and two separate entries are queued onpersistedViewStateWriteQueue.Keep the single explicit call and drop the two
saveViewStatecalls from thePromise.all.♻️ Proposed fix
await Promise.all([ this.updateGlobalState("listApiConfigMeta", listApiConfigMeta), this.updateGlobalState("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), this.contextProxy.setProviderSettings(providerSettings), - this.saveViewState("currentApiConfigName", name), - this.saveViewState("apiConfiguration", providerSettings), ]) await this._saveViewLocalStateFromMutation({ listApiConfigMeta, currentApiConfigName: name, apiConfiguration: providerSettings, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/ClineProvider.ts` around lines 1927 - 1942, Remove the saveViewState calls for “currentApiConfigName” and “apiConfiguration” from the Promise.all in the provider settings mutation, leaving the existing explicit _saveViewLocalStateFromMutation call to persist both values once. Keep the other state updates and providerSettingsManager operations unchanged.
507-509: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd provider-level tests for the view-state lifecycle.
This PR adds view-state persistence tests for the webview wrapper and the types package, but no test covers the provider side. The untested behavior includes: merge precedence in
getState, pruning at the 50-entry cap, serialization throughpersistedViewStateWriteQueue, and re-loading aftersetViewStateId.src/core/webview/__tests__/ClineProvider.spec.tsalready exists and is the right layer for these.As per coding guidelines: "Place tests in the narrowest layer that proves the behavior: package-local unit tests for pure logic and similar concerns; integration tests for cooperating internal modules."
🤖 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 507 - 509, The provider-level view-state lifecycle is missing coverage. Extend ClineProvider.spec.ts with tests for getState merge precedence, 50-entry pruning, persistence through persistedViewStateWriteQueue, and reloading after setViewStateId, using the existing provider test setup and preserving current behavior.Source: Coding guidelines
webview-ui/src/utils/__tests__/vscode.spec.ts (1)
47-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
crypto-absent fallback branch.The three tests all stub or rely on
crypto.randomUUID. TheDate.now()/Math.random()fallback increateViewStateIdhas no test. That branch is the safety net for restricted or insecure contexts, which is the same environment class this PR targets.💚 Proposed additional test
it("generates a viewStateId without crypto.randomUUID", () => { Object.defineProperty(globalThis, "crypto", { configurable: true, value: {} }) const storage = createMockStorage() Object.defineProperty(globalThis, "localStorage", { configurable: true, value: storage }) const wrapper = new VSCodeAPIWrapper() const viewStateId = wrapper.getViewStateId() expect(viewStateId).toMatch(/^[a-z0-9]+-[a-z0-9]+$/) expect(wrapper.getViewStateId()).toBe(viewStateId) })🤖 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 47 - 61, Add a test covering the crypto-absent fallback in createViewStateId by defining globalThis.crypto without randomUUID, then instantiate VSCodeAPIWrapper with mocked localStorage and verify getViewStateId returns the fallback-shaped identifier and persists the same value across repeated calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 642-648: Update the on and corresponding off overrides in
ClineProvider to resolve the listener variance mismatch without casting super.on
or super.off to any. Remove both inline ESLint suppressions and use a type-safe
invocation that preserves the TaskProviderEvents listener signatures.
- Around line 590-621: Update src/core/webview/ClineProvider.ts lines 590-621 in
loadViewState to merge loaded values into the existing viewLocalState buffer,
and assign currentApiConfigName only after getProfile succeeds; update lines
354-356 to retain the constructor-started loadViewState promise and await it in
getState() before returning state.
- Around line 3012-3013: Update the destructiveCommandGuardEnabled field in the
return object to read from mergedStateValues instead of stateValues, while
preserving the existing nullish fallback to
DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED.
- Around line 1987-1990: In deleteProviderProfile, replace the
_updateViewLocalStateFromMutation call with _saveViewLocalStateFromMutation so
the new currentApiConfigName and listApiConfigMeta are persisted in viewStates
as well as updated in memory.
- Around line 575-584: Update setViewStateId to compare the value that will
actually be stored in this.viewStateId, preventing repeated loadViewState calls
for equivalent handshakes. Avoid the lossy character replacement for viewStates
keys; either retain the trimmed id unchanged or validate and reject ids
containing unexpected characters before updating state.
- Line 1764: Replace the double assertion in the task mode assignment with
bracket notation to access the private _taskMode member directly, preserving the
existing newMode value and its string | undefined typing.
- Around line 3269-3285: Update the provider-settings branch around
PROVIDER_SETTINGS_KEYS and providerSettingsUpdate so single-key mutations,
including apiProvider, merge into the existing
this.viewLocalState.apiConfiguration instead of replacing it; preserve any
intentional stale-key clearing only if explicitly documented and safe. Replace
the reduce-based object construction with a single-pass mutation or equivalent
accumulator that avoids repeated object spreads and O(n²) allocations.
- Around line 2999-3002: Update the apiConfiguration construction in
ClineProvider to replace the shared configuration entirely when a view-local
profile is selected, rather than spreading providerSettings with
mergedStateValues.apiConfiguration. Strip only the profile name as currently
required, and compose secrets from the selected profile so model IDs, URLs,
provider fields, and secret state cannot be inherited from the shared profile.
In `@webview-ui/src/context/ExtensionStateContext.tsx`:
- Around line 514-517: Update the webviewDidLaunch message type to declare
viewStateId, then in the corresponding handler call
provider.setViewStateId(message.viewStateId) before or while processing the
launch event. Ensure the webview-generated ID is propagated so per-view state no
longer falls back to the default viewId.
In `@webview-ui/src/utils/vscode.ts`:
- Around line 26-32: Update createViewStateId to verify crypto.randomUUID is
callable before invoking it, and guard the invocation against insecure-context
failures so getViewStateId falls back to the existing timestamp/random value
when UUID generation is unavailable.
---
Nitpick comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 556-565: Update clearPersistedViewState to pass the states through
prunePersistedViewStates before setValue, matching the existing
savePersistedViewState write path and preserving the persisted view-state cap.
- Around line 1927-1942: Remove the saveViewState calls for
“currentApiConfigName” and “apiConfiguration” from the Promise.all in the
provider settings mutation, leaving the existing explicit
_saveViewLocalStateFromMutation call to persist both values once. Keep the other
state updates and providerSettingsManager operations unchanged.
- Around line 507-509: The provider-level view-state lifecycle is missing
coverage. Extend ClineProvider.spec.ts with tests for getState merge precedence,
50-entry pruning, persistence through persistedViewStateWriteQueue, and
reloading after setViewStateId, using the existing provider test setup and
preserving current behavior.
In `@webview-ui/src/utils/__tests__/vscode.spec.ts`:
- Around line 47-61: Add a test covering the crypto-absent fallback in
createViewStateId by defining globalThis.crypto without randomUUID, then
instantiate VSCodeAPIWrapper with mocked localStorage and verify getViewStateId
returns the fallback-shaped identifier and persists the same value across
repeated calls.
🪄 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: 5d4dd981-b4b5-48c1-ac3b-4d1010fef6b9
📒 Files selected for processing (7)
packages/types/src/__tests__/index.test.tspackages/types/src/global-settings.tssrc/core/webview/ClineProvider.tssrc/eslint-suppressions.jsonwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/utils/__tests__/vscode.spec.tswebview-ui/src/utils/vscode.ts
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
| public async setViewStateId(viewStateId: string | undefined): Promise<void> { | ||
| const normalizedViewStateId = viewStateId?.trim() | ||
|
|
||
| if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) { | ||
| return | ||
| } | ||
|
|
||
| this.viewStateId = normalizedViewStateId.replace(/[^A-Za-z0-9_-]/g, "_") | ||
| await this.loadViewState() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Compare the sanitized id, and reconsider sanitizing at all.
Two problems here:
- The equality check uses
normalizedViewStateId, but the field stores the sanitized value. If the incoming id contains any character outside[A-Za-z0-9_-], the check never matches and every handshake re-runsloadViewState(). replace(/[^A-Za-z0-9_-]/g, "_")is many-to-one. Two distinct webviews whose ids differ only in a replaced character map to the same persisted key and overwrite each other's mode and profile. The value is used only as an object key in theviewStatesrecord, so no escaping is required.
Sanitize first, then compare. If you keep the sanitizer, reject unexpected ids instead of collapsing them.
🐛 Proposed fix
public async setViewStateId(viewStateId: string | undefined): Promise<void> {
- const normalizedViewStateId = viewStateId?.trim()
-
- if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) {
- return
- }
-
- this.viewStateId = normalizedViewStateId.replace(/[^A-Za-z0-9_-]/g, "_")
- await this.loadViewState()
+ const normalizedViewStateId = viewStateId?.trim().replace(/[^A-Za-z0-9_-]/g, "_")
+
+ if (!normalizedViewStateId || normalizedViewStateId === this.viewStateId) {
+ return
+ }
+
+ this.viewStateId = normalizedViewStateId
+ await this.loadViewState()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 575 - 584, Update
setViewStateId to compare the value that will actually be stored in
this.viewStateId, preventing repeated loadViewState calls for equivalent
handshakes. Avoid the lossy character replacement for viewStates keys; either
retain the trimmed id unchanged or validate and reject ids containing unexpected
characters before updating state.
| private async loadViewState(): Promise<void> { | ||
| try { | ||
| const persisted = this.getPersistedViewStates()[this.viewStateId] | ||
| const loadedState: Partial<ExtensionState> = {} | ||
|
|
||
| if (persisted?.mode) { | ||
| loadedState.mode = persisted.mode | ||
| } | ||
|
|
||
| if (persisted?.currentApiConfigName) { | ||
| loadedState.currentApiConfigName = persisted.currentApiConfigName | ||
|
|
||
| try { | ||
| const { name: _name, ...apiConfiguration } = await this.providerSettingsManager.getProfile({ | ||
| name: persisted.currentApiConfigName, | ||
| }) | ||
| loadedState.apiConfiguration = apiConfiguration as ProviderSettings | ||
| } catch (error) { | ||
| this.log( | ||
| `[loadViewState] Unable to resolve API profile '${persisted.currentApiConfigName}' for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| this.viewLocalState = loadedState | ||
| this.log(`[loadViewState] Loaded state for viewId ${this.viewId}`) | ||
| } catch (error) { | ||
| this.log( | ||
| `[loadViewState] Error loading state for viewId ${this.viewId}: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Async loadViewState both races the first read and discards concurrent writes. Both sites stem from one root cause: loadViewState is asynchronous, it awaits providerSettingsManager.getProfile(), and it finishes by replacing the whole viewLocalState object. The constructor starts it without awaiting, so a window exists in which getState() reads an unloaded buffer and other code writes into a buffer that is about to be thrown away.
src/core/webview/ClineProvider.ts#L590-L621: merge the loaded values into the existing buffer instead of assigning over it, and setcurrentApiConfigNameonly whengetProfile()succeeds.src/core/webview/ClineProvider.ts#L354-L356: store the returned promise in a field and await it ingetState(), so the first read after construction observes the loaded per-view values.
📍 Affects 1 file
src/core/webview/ClineProvider.ts#L590-L621(this comment)src/core/webview/ClineProvider.ts#L354-L356
🤖 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 590 - 621, Update
src/core/webview/ClineProvider.ts lines 590-621 in loadViewState to merge loaded
values into the existing viewLocalState buffer, and assign currentApiConfigName
only after getProfile succeeds; update lines 354-356 to retain the
constructor-started loadViewState promise and await it in getState() before
returning state.
| 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 added as any casts and inline ESLint suppressions.
Both overrides now carry // eslint-disable-next-line @typescript-eslint/no-explicit-any`` with (super.on as any) and `(super.off as any)`. The coding guidelines require fixing lint violations in new TypeScript code rather than suppressing them, and require avoiding `as any`. This PR also removes suppressions elsewhere, so adding two inline ones here works against that goal.
The variance mismatch can be resolved without any.
♻️ Proposed fix
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(event as never, listener as never)
} override off<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.off as any)(event, listener)
+ return super.off(event as never, listener as never)
}As per coding guidelines: "Fix lint violations in new TypeScript code rather than suppressing them" and "Avoid as any; use typed APIs, bracket notation for private members, or precise test doubles and type guards."
Also applies to: 653-659
🤖 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
corresponding off overrides in ClineProvider to resolve the listener variance
mismatch without casting super.on or super.off to any. Remove both inline ESLint
suppressions and use a type-safe invocation that preserves the
TaskProviderEvents listener signatures.
Source: Coding guidelines
|
|
||
| // Only update the task's mode after successful persistence. | ||
| ;(task as any)._taskMode = newMode | ||
| ;(task as unknown as Record<string, string>)._taskMode = newMode |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use bracket notation instead of an undocumented double assertion.
(task as unknown as Record<string, string>) is a double assertion, and it is inaccurate: Task._taskMode is typed string | undefined, and Task is not a string-valued record. The guidelines name bracket notation as the preferred way to reach a private member, and permit double assertions only as a documented last resort.
♻️ Proposed fix
- ;(task as unknown as Record<string, string>)._taskMode = newMode
+ task["_taskMode"] = newModeAs per coding guidelines: "Avoid as any; use typed APIs, bracket notation for private members, or precise test doubles and type guards. Use double assertions only as a last resort and document them."
📝 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.
| ;(task as unknown as Record<string, string>)._taskMode = newMode | |
| task["_taskMode"] = newMode |
🤖 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` at line 1764, Replace the double assertion
in the task mode assignment with bracket notation to access the private
_taskMode member directly, preserving the existing newMode value and its string
| undefined typing.
Source: Coding guidelines
| this._updateViewLocalStateFromMutation({ | ||
| currentApiConfigName: profileToActivate, | ||
| listApiConfigMeta: entries, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Profile deletion updates the cache but not the persisted view state.
deleteProviderProfile switches currentApiConfigName to profileToActivate and calls _updateViewLocalStateFromMutation, which only touches the in-memory cache. The persisted viewStates entry keeps the deleted profile name. After a reload, loadViewState calls getProfile with that deleted name, the call fails, and the per-view profile selection is lost with an error in the log.
Use _saveViewLocalStateFromMutation so the persisted entry is updated too.
🐛 Proposed fix
- this._updateViewLocalStateFromMutation({
- currentApiConfigName: profileToActivate,
- listApiConfigMeta: entries,
- })
+ await this._saveViewLocalStateFromMutation({
+ currentApiConfigName: profileToActivate,
+ listApiConfigMeta: entries,
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this._updateViewLocalStateFromMutation({ | |
| currentApiConfigName: profileToActivate, | |
| listApiConfigMeta: entries, | |
| }) | |
| await this._saveViewLocalStateFromMutation({ | |
| currentApiConfigName: profileToActivate, | |
| listApiConfigMeta: entries, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 1987 - 1990, In
deleteProviderProfile, replace the _updateViewLocalStateFromMutation call with
_saveViewLocalStateFromMutation so the new currentApiConfigName and
listApiConfigMeta are persisted in viewStates as well as updated in memory.
| apiConfiguration: { | ||
| ...providerSettings, | ||
| ...mergedStateValues.apiConfiguration, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect getProviderSettings composition to judge the merge precedence.
set -euo pipefail
fd -t f 'ContextProxy.ts' --exec rg -n -C10 'getProviderSettings|setProviderSettings' {}Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and symbols, then inspect the focused implementation slices.
printf 'Files:\n'
fd -t f 'ClineProvider.ts|ContextProxy.ts|.*schemas.*|.*settings.*' src | sed -n '1,120p'
printf '\nContextProxy get/set and state shape:\n'
wc -l src/i18n/ContextProxy.ts
sed -n '430,560p' src/i18n/ContextProxy.ts
printf '\nClineProvider focused slice:\n'
wc -l src/core/webview/ClineProvider.ts
sed -n '2950,3035p' src/core/webview/ClineProvider.ts
printf '\nSearch loadViewState and providerSettingsSchema definitions:\n'
rg -n -C8 'function loadViewState|const loadViewState|providerSettingsSchema|loadViewState\(' srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'ContextProxy get/set and state shape:\n'
wc -l src/core/config/ContextProxy.ts
sed -n '430,590p' src/core/config/ContextProxy.ts
printf '\nClineProvider focused slice:\n'
wc -l src/core/webview/ClineProvider.ts
sed -n '2950,3035p' src/core/webview/ClineProvider.ts
printf '\nSearch loadViewState and providerSettingsSchema definitions:\n'
rg -n -C8 'function loadViewState|const loadViewState|providerSettingsSchema|loadViewState\(' srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 14980
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'loadViewState implementation:\n'
sed -n '585,820p' src/core/webview/ClineProvider.ts
printf '\nState type definitions around ExtensionState / viewLocalState:\n'
rg -n -C5 'interface ExtensionState|type ExtensionState|viewLocalState|PERSISTED_VIEW_STATES|persistedViewStates|apiConfiguration' src/core/webview/ClineProvider.ts
printf '\nType definitions from packages (if present):\n'
fd -t f '.*types.*|.*schema.*|.*state.*' . --exclude node_modules --exclude dist --exclude build | sed -n '1,160p'
rg -n -C5 'interface ProviderSettings|type ProviderSettings|providerSettingsSchema|apiConfiguration|APIConfiguration|SecretStateKey|isSecretStateKey' src packages 2>/dev/null | sed -n '1,220p' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 45687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'ProviderSettingsManager get/set profile slices:\n'
sed -n '1,220p' src/core/config/ProviderSettingsManager.ts
sed -n '350,660p' src/core/config/ProviderSettingsManager.ts
printf '\nSaved mutation helpers slices:\n'
sed -n '1480,1560p' src/core/webview/ClineProvider.ts
sed -n '3300,3325p' src/core/webview/ClineProvider.ts
printf '\nBehavioral probe for state merge shape from static sources:\n'
python3 - <<'PY'
from pathlib import Path
src = Path('src/core/webview/ClineProvider.ts').read_text()
checks = {
'viewLocalState initialized empty': 'private viewLocalState: Partial<ExtensionState> = {}' in src,
'loadViewState unsets missing persisted entries': 'Missing entries are intentionally left unset' in src,
'loadViewState strips only profile name': '.{ name: _name, ...apiConfiguration }' in src or "name: _name, ...apiConfiguration" in src,
'loadViewState uses ProviderSettingsManager.getProfile': 'providerSettingsManager.getProfile' in src,
'getState merges stateValues then viewLocalState': '{ ...stateValues, ...this.viewLocalState }' in src,
'getState spreads providerSettings then merged apiConfiguration': '...providerSettings,\n\t\t\t...mergedStateValues.apiConfiguration,' in src,
}
for name, ok in checks.items():
print(f'{name}: {ok}')
print('all true:', all(checks.values()))
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 20544
Replace the whole apiConfiguration when the view-local profile is present.
getProfile() returns the selected profile as ProviderSettings, but only name is stripped before assigning viewLocalState.apiConfiguration. The current spread merges provider keys, so a view-local provider can inherit model IDs, base URLs, or other fields from the shared profile. Also ensure secrets are composed correctly for the selected profile instead of leaking the shared profile’s secret state.
🤖 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 2999 - 3002, Update the
apiConfiguration construction in ClineProvider to replace the shared
configuration entirely when a view-local profile is selected, rather than
spreading providerSettings with mergedStateValues.apiConfiguration. Strip only
the profile name as currently required, and compose secrets from the selected
profile so model IDs, URLs, provider fields, and secret state cannot be
inherited from the shared profile.
| destructiveCommandGuardEnabled: | ||
| stateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
destructiveCommandGuardEnabled still reads stateValues.
Every neighbouring field in this return object reads mergedStateValues. This one reads stateValues. The two are equivalent today, because viewLocalState only ever holds mode, currentApiConfigName, and apiConfiguration. The asymmetry becomes a bug if the view-local buffer later gains this key.
🐛 Proposed fix
destructiveCommandGuardEnabled:
- stateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED,
+ mergedStateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED,📝 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.
| destructiveCommandGuardEnabled: | |
| stateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, | |
| destructiveCommandGuardEnabled: | |
| mergedStateValues.destructiveCommandGuardEnabled ?? DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 3012 - 3013, Update the
destructiveCommandGuardEnabled field in the return object to read from
mergedStateValues instead of stateValues, while preserving the existing nullish
fallback to DEFAULT_DESTRUCTIVE_COMMAND_GUARD_ENABLED.
| } else if (PROVIDER_SETTINGS_KEYS.some((key) => key in values)) { | ||
| const providerSettingsUpdate = PROVIDER_SETTINGS_KEYS.reduce((acc, key) => { | ||
| if (key in values) { | ||
| return { ...acc, [key]: values[key as keyof RooCodeSettings] } | ||
| } | ||
|
|
||
| return acc | ||
| }, {} as ProviderSettings) | ||
|
|
||
| this.viewLocalState.apiConfiguration = | ||
| "apiProvider" in providerSettingsUpdate | ||
| ? providerSettingsUpdate | ||
| : { | ||
| ...(this.viewLocalState.apiConfiguration ?? {}), | ||
| ...providerSettingsUpdate, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A single provider-key mutation can wipe the view-local apiConfiguration.
Two problems in this branch.
- When the mutation contains
apiProvider, Line 3280 assignsproviderSettingsUpdatedirectly and discards the existingthis.viewLocalState.apiConfiguration.providerSettingsUpdateholds only the keys present in this one mutation. A call such assetValue("apiProvider", "anthropic")therefore reduces the whole view-local configuration to{ apiProvider: "anthropic" }, andgetState()then falls back to the shared profile for every other field. If the intent is to clear stale keys from the previous provider, state that in a comment and confirm it is safe for single-key callers. - The
reduceallocates a new object on every iteration ofPROVIDER_SETTINGS_KEYS. That list is large, so this is O(n²) allocation on a path reached by everysetValueandsetValuesthat touches a provider key.
♻️ Proposed fix for the allocation, plus a guard for single-key mutations
} else if (PROVIDER_SETTINGS_KEYS.some((key) => key in values)) {
- const providerSettingsUpdate = PROVIDER_SETTINGS_KEYS.reduce((acc, key) => {
- if (key in values) {
- return { ...acc, [key]: values[key as keyof RooCodeSettings] }
- }
-
- return acc
- }, {} as ProviderSettings)
+ const providerSettingsUpdate: ProviderSettings = {}
+ for (const key of PROVIDER_SETTINGS_KEYS) {
+ if (key in values) {
+ ;(providerSettingsUpdate as Record<string, unknown>)[key] = values[key as keyof RooCodeSettings]
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 3269 - 3285, Update the
provider-settings branch around PROVIDER_SETTINGS_KEYS and
providerSettingsUpdate so single-key mutations, including apiProvider, merge
into the existing this.viewLocalState.apiConfiguration instead of replacing it;
preserve any intentional stale-key clearing only if explicitly documented and
safe. Replace the reduce-based object construction with a single-pass mutation
or equivalent accumulator that avoids repeated object spreads and O(n²)
allocations.
| vscode.postMessage({ | ||
| type: "webviewDidLaunch", | ||
| viewStateId: typeof vscode.getViewStateId === "function" ? vscode.getViewStateId() : undefined, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace the viewStateId handshake from webview message type to provider consumption.
set -euo pipefail
# 1. Does the WebviewMessage type declare viewStateId?
rg -n -C3 'viewStateId' --type=ts --type=tsx -g '!**/node_modules/**' src packages webview-ui
# 2. Locate the webviewDidLaunch handler and check whether it reads viewStateId.
rg -n -C10 '"webviewDidLaunch"|case "webviewDidLaunch"' --type=ts -g '!**/node_modules/**' src
# 3. Find call sites of setViewStateId.
ast-grep run --pattern '$_.setViewStateId($$$)' --lang typescript srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## files matching relevant names"
git ls-files | rg '(^|/)(ExtensionStateContext|webview|ClineProvider|Provider|Message|types)\.(ts|tsx)$|webview-ui/src' | head -200
echo
echo "## viewStateId occurrences"
rg -n -C3 'viewStateId' --glob '*.ts' --glob '*.tsx' .
echo
echo "## webviewDidLaunch occurrences"
rg -n -C10 '"webviewDidLaunch"|case "webviewDidLaunch"|webviewDidLaunch' --glob '*.ts' --glob '*.tsx' .
echo
echo "## setViewStateId occurrences"
rg -n -C4 'setViewStateId' --glob '*.ts' --glob '*.tsx' .Repository: Zoo-Code-Org/Zoo-Code
Length of output: 11579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate key symbols"
rg -n -C5 'export .*setViewStateId|setViewStateId|viewStateId|webviewDidLaunch|ViewStateId' --glob '*.ts' --glob '*.tsx' .
echo
echo "## ClineProvider outline"
ast-grep outline src/core/webview/ClineProvider.ts --view compact | sed -n '1,220p'
echo
echo "## ClineProvider relevant lines"
rg -n -C8 'viewStateId|setViewStateId|webviewDidLaunch|webviewStateId' src/core/webview/ClineProvider.ts
echo
echo "## App outline"
ast-grep outline webview-ui/src/App.tsx --view compact | sed -n '1,220p'
echo
echo "## webview-ui App state/message relevant lines"
rg -n -C8 'setViewStateId|viewStateId|viewId|webviewDidLaunch|onMessage|postMessage' webview-ui/src/App.tsx
echo
echo "## ExtensionStateContext relevant lines"
fd -a 'ExtensionStateContext.tsx' . | while read -r f; do
echo "--- $f"
wc -l "$f"
rg -n -C8 'viewStateId|setViewStateId|webviewDidLaunch' "$f"
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 24268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact contract/handler body and tests around webviewDidLaunch.
sed -n '430,480p' packages/types/src/vscode-extension-host.ts
sed -n '810,835p' src/core/webview/__tests__/ClineProvider.spec.ts
sed -n '560,590p' src/core/webview/webviewMessageHandler.ts
sed -n '570,592p' src/core/webview/ClineProvider.ts
echo
echo "## all postMessage webviewDidLaunch occurrences in tracked TS/TSX"
python3 - <<'PY'
import subprocess
files = subprocess.check_output(['git', 'ls-files'], text=True).splitlines()
for f in files:
if f.endswith('.ts') or f.endswith('.tsx'):
text = open(f, encoding='utf-8', errors='ignore').read()
if 'webviewDidLaunch' in text:
for i, line in enumerate(text.splitlines(), 1):
if 'webviewDidLaunch' in line:
print(f"{f}:{i}: {line.strip()}")
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 5674
Forward viewStateId in the webviewDidLaunch handler.
WebviewMessage only declares type for webviewDidLaunch, and the handler ignores any extra payload without calling ClineProvider.setViewStateId. Add viewStateId to the supported payload shape and call provider.setViewStateId(message.viewStateId) in the webviewDidLaunch case so persisted per-view state uses the webview-generated id instead of the default viewId.
🧰 Tools
🪛 GitHub Actions: E2E Tests (Mocked) / 0_e2e-mock.txt
[error] 516-516: TypeScript build failed in '@roo-code/vscode-webview' while running 'tsc -b && vite build': object literal specifies unknown property 'viewStateId', which does not exist in type 'WebviewMessage' (TS2353).
🪛 GitHub Actions: E2E Tests (Mocked) / e2e-mock
[error] 516-516: TypeScript build failed in '@roo-code/vscode-webview' during 'tsc -b && vite build': Object literal may only specify known properties, and 'viewStateId' does not exist in type 'WebviewMessage' (TS2353).
🪛 GitHub Actions: Release Validation / 0_validate-release.txt
[error] 516-516: TypeScript build failed: Object literal specifies unknown property 'viewStateId', which does not exist in type 'WebviewMessage' (TS2353). Failed command: pnpm --filter @roo-code/vscode-webview build.
🪛 GitHub Actions: Release Validation / validate-release
[error] 516-516: TypeScript build failed during 'pnpm --filter @roo-code/vscode-webview build': object literal includes unknown property 'viewStateId', which does not exist in type 'WebviewMessage' (TS2353).
🤖 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/ExtensionStateContext.tsx` around lines 514 - 517,
Update the webviewDidLaunch message type to declare viewStateId, then in the
corresponding handler call provider.setViewStateId(message.viewStateId) before
or while processing the launch event. Ensure the webview-generated ID is
propagated so per-view state no longer falls back to the default viewId.
| private createViewStateId(): string { | ||
| if (typeof crypto !== "undefined" && "randomUUID" in crypto) { | ||
| return crypto.randomUUID() | ||
| } | ||
|
|
||
| return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}` | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
crypto.randomUUID secure context requirement browser availability
💡 Result:
The crypto.randomUUID method is strictly restricted to secure contexts [1][2]. This means it is only available in environments delivered over HTTPS or on localhost (including 127.0.0.1 and file://) [3][2]. If you attempt to call crypto.randomUUID in a non-secure context (such as an insecure HTTP connection over a local network or internal hostname), the crypto object may not contain the method, or the call will fail, typically resulting in an error such as crypto.randomUUID is not a function [3][2]. There is no intention to drop this secure context requirement, as browser vendors and the W3C have standardized this to ensure cryptographic security [3]. Developers encountering this issue in development environments on internal networks are advised to serve their applications over HTTPS, for example by using tools like mkcert to generate locally trusted certificates [3][2]. In terms of availability, crypto.randomUUID is widely supported in all modern browsers [4]. Support began appearing in major browsers around 2021 (e.g., Chrome 92, Firefox 95) [4][5]. It is available for use in both standard browser windows and Web Workers [1]. For environments outside of browsers, such as Node.js, the secure context requirement does not apply; in Node.js, crypto.randomUUID is available as of version 14.17 [2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
- 2: https://techearl.com/javascript-generate-uuid
- 3: crypto.randomUUID() not available on internal HTTP environment (non-secure context) prisma/prisma#29534
- 4: https://caniuse.com/mdn-api_crypto_randomuuid
- 5: https://bugzilla.mozilla.org/show_bug.cgi?id=1723674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching vscode/SettingsView/ExtensionStateContext:\n'
git ls-files | rg '(^|/)(vscode\.(ts|tsx)|SettingsView\.(ts|tsx)|ExtensionStateContext\.(ts|tsx))$' || true
printf '\n--- webview-ui/src/utils/vscode.ts outline ---\n'
ast-grep outline webview-ui/src/utils/vscode.ts --view expanded || true
printf '\n--- webview-ui/src/utils/vscode.ts relevant lines ---\n'
cat -n webview-ui/src/utils/vscode.ts | sed -n '1,90p'
printf '\n--- Search for getViewStateId/createViewStateId usages ---\n'
rg -n "createViewStateId|getViewStateId|webviewDidLaunch|ExtensionStateContext" webview-ui/src -SRepository: Zoo-Code-Org/Zoo-Code
Length of output: 37160
🌐 Web query:
MDN Crypto randomUUID secure context TypeError outside secure context
💡 Result:
The TypeError "crypto.randomUUID is not a function" occurs because the crypto.randomUUID method is strictly restricted to secure contexts [1][2][3]. What constitutes a secure context: - HTTPS: Any page served over a secure HTTPS connection is a secure context [1][4]. - localhost: Browsers treat http://localhost as a secure context, allowing crypto.randomUUID to function even without HTTPS [2][3][4]. - Other origins: Arbitrary HTTP origins (e.g., internal network IP addresses like 192.168.x.x or custom non-HTTPS hostnames) are not considered secure contexts [5][4][6]. If your application is accessed via these addresses, crypto.randomUUID will be undefined, and calling it will result in a TypeError [3][4]. Why this restriction exists: The Web Crypto API is designed to provide cryptographic primitives [7]. To prevent passive eavesdroppers or man-in-the-middle attackers from observing or tampering with cryptographic operations, browser vendors and the W3C restricted access to these methods to secure contexts [3][4][6]. Recommended solutions: 1. Use HTTPS: The most robust solution is to serve your application over HTTPS, even for internal network deployments [3][4][6]. Tools like mkcert can help generate locally trusted certificates for development [4]. 2. Check Availability: If you must support insecure environments, implement a feature check before calling the method [3]: if (typeof crypto?.randomUUID === 'function') { const id = crypto.randomUUID; } else { // Fallback to a secure library or an alternative ID generation method } 3. Use a Polyfill/Fallback: If crypto.randomUUID is unavailable, you can use crypto.getRandomValues to generate a UUID yourself, or use a well-vetted library to ensure collision resistance [3][5][6]. Do not use insecure random number generators (like Math.random) for creating unique IDs if they are required to be cryptographically secure [8][3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
- 2: https://stackoverflow.com/questions/77980939/typeerror-crypto-randomuuid-is-not-a-function
- 3: https://github.com/kjuhwa/skills-hub/blob/main/knowledge/pitfall/crypto-randomuuid-requires-secure-context.md
- 4: crypto.randomUUID() not available on internal HTTP environment (non-secure context) prisma/prisma#29534
- 5: https://stackoverflow.com/questions/74911304/crypto-module-not-loading-randomuuid-when-viewing-a-local-network-ip-address
- 6: https://blog.openreplay.com/generate-unique-ids-web-crypto-api/
- 7: https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API
- 8: Consider dropping the secure context requirement for
randomUUID()w3c/webcrypto#408
Guard crypto.randomUUID() against insecure-context failures.
"randomUUID" in crypto tests for the property only; outside a secure context it is unavailable, and calling crypto.randomUUID() can fail. Wrap the call or check typeof crypto.randomUUID === "function" before calling it so getViewStateId() falls back to the deterministic fallback instead of failing during webview initialization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/utils/vscode.ts` around lines 26 - 32, Update
createViewStateId to verify crypto.randomUUID is callable before invoking it,
and guard the invocation against insecure-context failures so getViewStateId
falls back to the existing timestamp/random value when UUID generation is
unavailable.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Generate and persist a unique
viewStateIdfor each webview instance, establishing the foundation infrastructure for per-view state. This is the first step of the entire per-view isolation architecture.Changes
getViewStateId()method for external usecrypto.randomUUID()with fallback mechanismglobal-settings.tsFiles Changed (7 files, +559 / -124)
src/core/webview/ClineProvider.tswebview-ui/src/utils/vscode.tswebview-ui/src/context/ExtensionStateContext.tsxpackages/types/src/global-settings.tswebview-ui/src/utils/__tests__/vscode.spec.tspackages/types/src/__tests__/index.test.tssrc/eslint-suppressions.jsonTest Notes
setValues()method that base-1's persistence logic depends on. The infrastructure is in place but not yet wired to actual state updates.Related