Skip to content

Commit 735dc22

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
unit 3a: answer the CodeRabbit rows on comments, dispose behaviour, and delivery tests
ClineProvider.ts 174: the two dangling JSDoc blocks now sit on the declarations they describe. The buffer and durable note sits above type PersistedViewState. The narrow-generic note sits above savePersistedViewState, where the sentence named saveViewState, a name no declaration in this file carries, and now names the queued write that path takes. ClineProvider.ts 283: the orphan note about the fields a view owns describes type ViewLocalStateField, and no declaration in this tree carries that name. git blame attributes the note to 76ab52f, so it is PR-carried, and it is deleted here as dead text to return with the unit that carries the declaration. ClineProvider.ts 516: dispose now bumps viewStateGeneration at its start and detaches this provider's entries from the shared write queue through the splice path the queue helper already provides, clearing entry.provider so a disposed view stops being retained behind another view's write. The comment at 503 and the block at 506 claimed both, and the code did neither. Two tests added, one that fails without the bump and one that fails without the detach. ClineProvider.ts 1045: three tests added for the survivors this chain owes itself, the shared view id counter, the per-field write version record, and the boolean delivery result reporting an undelivered message after dispose. webviewMessageHandler.spec.ts 2853: the ordering assertion is now causal. The row establishes isViewLaunched false, captures it inside the flush spy, and asserts the transition, because clearAllMocks resets call records and not properties. The rejected flush row the reviewer said was untested is added beside it, asserting the launch still completes. ProviderSettingsManager.ts 521 is not touched here. The guard is main content that predates this chain, and the file reaches this PR only through unit 2.
1 parent 76ab52f commit 735dc22

3 files changed

Lines changed: 97 additions & 12 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -160,18 +160,12 @@ import { REQUESTY_BASE_URL } from "../../shared/utils/requesty"
160160
import { validateAndFixToolResultIds } from "../task/validateToolResultIds"
161161
import { PendingEditOperationStore, type PendingEditOperationInput } from "./PendingEditOperationStore"
162162

163-
type PersistedViewState = NonNullable<GlobalState["viewStates"]>[string]
164-
165163
/**
166164
* Values that can be held in a view-local state buffer (in-memory) and, for the
167165
* non-secret subset, persisted durably per stable view id.
168166
*/
169167

170-
/**
171-
* The subset of view-local values that has a durable per-view record. `saveViewState`
172-
* and the mutation path handle exactly these keys, so the generic stays narrow enough
173-
* for the compiler to reject e.g. `writeDelayMs`.
174-
*/
168+
type PersistedViewState = NonNullable<GlobalState["viewStates"]>[string]
175169

176170
/**
177171
* https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts
@@ -280,8 +274,6 @@ type ViewStateWriteEntry = {
280274
readonly run: (provider: ClineProvider) => Promise<void>
281275
}
282276

283-
/** The fields a view owns in its buffer and in its durable record. */
284-
285277
/** Writes one field of a partial record, or deletes it when the update is nullish. */
286278
function assignOrClearField<T, K extends keyof T>(target: Partial<T>, key: K, value: T[K] | null | undefined): void {
287279
if (value === undefined || value === null) {
@@ -1489,6 +1481,22 @@ export class ClineProvider
14891481
}
14901482

14911483
this._disposed = true
1484+
// Detach this provider from the shared view-state write queue and retire its generation,
1485+
// so a closure that starts after this point drops its effects instead of applying them
1486+
// to a disposed provider and holding it retained behind another view's write.
1487+
this.viewStateGeneration++
1488+
1489+
for (const entry of this.scheduledViewStateWrites) {
1490+
const index = ClineProvider.pendingViewStateWrites.indexOf(entry)
1491+
1492+
if (index !== -1) {
1493+
ClineProvider.pendingViewStateWrites.splice(index, 1)
1494+
}
1495+
1496+
entry.provider = null
1497+
}
1498+
1499+
this.scheduledViewStateWrites = []
14921500
this._postStateToWebviewThrottled.cancel()
14931501
this.stopWebviewWatchdog()
14941502
this.diagnosticsRecorder.record({ boundary: "provider", phase: "start", type: "dispose" })

‎src/core/webview/__tests__/ClineProvider.spec.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2799,6 +2799,64 @@ describe("ClineProvider", () => {
27992799
expect(mockWebviewPanel.webview.html).toBe(html)
28002800
})
28012801

2802+
test("retires the view-state generation when the provider disposes", async () => {
2803+
await provider.resolveWebviewView(mockWebviewPanel)
2804+
const generation = provider["viewStateGeneration"]
2805+
2806+
await provider.dispose()
2807+
2808+
expect(provider["viewStateGeneration"]).toBe(generation + 1)
2809+
expect(provider["isViewStateGenerationLive"](generation)).toBe(false)
2810+
})
2811+
2812+
test("detaches this provider's queued view-state writes when it disposes", async () => {
2813+
await provider.resolveWebviewView(mockWebviewPanel)
2814+
const entry: {
2815+
provider: ClineProvider | null
2816+
generation: number
2817+
run: (owner: ClineProvider) => Promise<void>
2818+
} = {
2819+
provider,
2820+
generation: provider["viewStateGeneration"],
2821+
run: async () => {},
2822+
}
2823+
// The pump advances on the fake clock, so the entry is read while it still waits on the queue.
2824+
const queued = ClineProvider["enqueuePersistedViewStateWrite"](entry)
2825+
2826+
expect(ClineProvider["pendingViewStateWrites"]).toContain(entry)
2827+
expect(provider["scheduledViewStateWrites"]).toContain(entry)
2828+
2829+
await provider.dispose()
2830+
2831+
expect(ClineProvider["pendingViewStateWrites"]).not.toContain(entry)
2832+
expect(provider["scheduledViewStateWrites"]).toEqual([])
2833+
expect(entry.provider).toBeNull()
2834+
2835+
void queued
2836+
})
2837+
2838+
test("numbers this provider's view id from the shared counter", () => {
2839+
const suffix = Number(String(provider["viewId"]).split("-").pop())
2840+
2841+
expect(Number.isNaN(suffix)).toBe(false)
2842+
expect(ClineProvider["nextViewId"]).toBeGreaterThan(suffix)
2843+
})
2844+
2845+
test("keeps the per-field view-state write versions the record gates on", () => {
2846+
expect(provider["viewLocalStateWriteVersions"]).toEqual({
2847+
mode: 0,
2848+
currentApiConfigName: 0,
2849+
apiConfiguration: 0,
2850+
})
2851+
})
2852+
2853+
test("reports a message as undelivered once the provider is disposed", async () => {
2854+
await provider.resolveWebviewView(mockWebviewPanel)
2855+
await provider.dispose()
2856+
2857+
expect(await provider.postMessageToWebview({ type: "mcpServers", mcpServers: [] })).toBe(false)
2858+
})
2859+
28022860
test.each([false, true])(
28032861
"replaces the tab host without disposing its provider or task (interrupted: %s)",
28042862
async (interrupted) => {

‎src/core/webview/__tests__/webviewMessageHandler.spec.ts‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2841,15 +2841,34 @@ describe("webviewMessageHandler - telemetrySetting", () => {
28412841
// the gate exists to close.
28422842
it("flushes the provider's pending view-state claims on webviewDidLaunch, after isViewLaunched is set", async () => {
28432843
vi.mocked(mockClineProvider.customModesManager.getCustomModes).mockResolvedValue([])
2844-
let launchedWhenFlushed: boolean | undefined
2844+
// Establish the false value inside this row: every beforeEach in this file runs
2845+
// clearAllMocks, which resets call records and not properties, so a value an earlier row
2846+
// left set could make the ordering pass on its own.
2847+
mockClineProvider.isViewLaunched = false
2848+
const launchedWhenFlushed: boolean[] = []
28452849
vi.mocked(mockClineProvider.deliverPendingViewStateClaims).mockImplementationOnce(async () => {
2846-
launchedWhenFlushed = mockClineProvider.isViewLaunched
2850+
launchedWhenFlushed.push(mockClineProvider.isViewLaunched)
28472851
})
28482852

28492853
await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch" })
28502854

28512855
expect(mockClineProvider.deliverPendingViewStateClaims).toHaveBeenCalledTimes(1)
2852-
expect(launchedWhenFlushed).toBe(true)
2856+
expect(launchedWhenFlushed).toEqual([true])
2857+
})
2858+
2859+
it("contains a rejected claim flush so webviewDidLaunch still completes", async () => {
2860+
vi.mocked(mockClineProvider.customModesManager.getCustomModes).mockResolvedValue([])
2861+
vi.mocked(mockClineProvider.deliverPendingViewStateClaims).mockImplementationOnce(() => {
2862+
throw new Error("Claim flush failed")
2863+
})
2864+
2865+
await expect(webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch" })).resolves.toBeUndefined()
2866+
2867+
expect(mockClineProvider.deliverPendingViewStateClaims).toHaveBeenCalledTimes(1)
2868+
2869+
expect(mockClineProvider.log).toHaveBeenCalledWith(
2870+
expect.stringContaining("Flushing pending claims on launch failed"),
2871+
)
28532872
})
28542873

28552874
// CodeRabbit follow-up on the finding #12 fix: webviewDidLaunch's telemetry init read state

0 commit comments

Comments
 (0)