Skip to content

Commit caabac7

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(webview): retire a confirmed experiment only while the cache still holds it
The confirmation effect gated retirement on one comparison: the live value against the value this view submitted. The cache copy was never consulted, so a toggle made after the save - which re-adds the id to the change set and writes the newer value - was overwritten when the host answer for the earlier value arrived, and its change-set entry was deleted in the same step. Since the save payload is built by iterating the change set, that next save then posted nothing for the experiment and the user's newer intent was lost. Retire the session inside the state updater, which is the only place the authoritative cached copy is readable: when the cache still equals the submitted value the session ends and the confirmed live value stands; when it does not, the submitted baseline is retired while the change-set entry survives, so the next save posts the newer value instead of the host's older answer. Both ref deletes are idempotent, so React invoking the updater twice retires the same entries it would retire once - that is what makes the decision safe to take inside the updater rather than outside it. Test: keeps a toggle made after the save when the host confirms the earlier value. Against the previous head it fails with "expected true to be false" on the checkbox; with this fix the file is 5 tests green on pinned Node 22.23.4 and on box Node 25.9.0, webview-ui src/components/settings is 61 files / 788 tests, and the package's own leg command (vitest run --coverage) is 188 files / 2,499 tests with no failing file on 22.23.4.
1 parent 710a443 commit caabac7

2 files changed

Lines changed: 61 additions & 11 deletions

File tree

‎webview-ui/src/components/settings/SettingsView.tsx‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -273,33 +273,48 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
273273
// confirmed live value stand in the cache. Without this, an experiment this view edited and
274274
// saved stays in the change set, so a later unrelated Save compares this view's cached copy
275275
// against a newer live value written elsewhere and posts the older copy back over it.
276-
// Experiments that are still being edited, or submitted and not yet confirmed, keep their
277-
// cached value, so a newer unsaved edit is never dropped.
276+
// A confirmation retires the session only while the cache still holds the submitted value:
277+
// a toggle made after the save is a newer intent than the host's answer, so it keeps its own
278+
// cache copy and its change-set entry, and the next Save posts it.
278279
useEffect(() => {
279280
const live = extensionState.experiments ?? experimentDefault
280-
const confirmed = new Map<ExperimentId, boolean>()
281-
for (const [id, value] of submittedExperiments.current) {
281+
const confirmations: { id: ExperimentId; submitted: boolean; liveValue: boolean }[] = []
282+
for (const [id, submitted] of submittedExperiments.current) {
282283
const liveValue = live[id] ?? experimentDefault[id]
283-
if (liveValue !== value) {
284+
if (liveValue !== submitted) {
284285
continue
285286
}
286-
submittedExperiments.current.delete(id)
287-
changedExperiments.current.delete(id)
288-
confirmed.set(id, liveValue)
287+
confirmations.push({ id, submitted, liveValue })
289288
}
289+
const adopted = new Map<ExperimentId, boolean>()
290290
for (const [id, liveValue] of Object.entries(live)) {
291291
const experimentId = id as ExperimentId
292292
if (submittedExperiments.current.has(experimentId) || changedExperiments.current.has(experimentId)) {
293293
continue
294294
}
295-
confirmed.set(experimentId, liveValue)
295+
adopted.set(experimentId, liveValue)
296296
}
297-
if (confirmed.size === 0) {
297+
if (confirmations.length === 0 && adopted.size === 0) {
298298
return
299299
}
300300
setCachedState((prevState) => {
301+
// The cached copy is only readable here, and both ref deletes are idempotent, so React
302+
// invoking this updater twice retires the same entries it would retire once.
301303
const experiments = { ...prevState.experiments } as Record<ExperimentId, boolean>
302-
for (const [id, liveValue] of confirmed) {
304+
for (const confirmation of confirmations) {
305+
const cached = experiments[confirmation.id] ?? experimentDefault[confirmation.id]
306+
submittedExperiments.current.delete(confirmation.id)
307+
if (cached !== confirmation.submitted) {
308+
// Newer than what was submitted: keep the user's copy and keep the entry in the
309+
// change set, so the next Save still posts it instead of the host's older answer.
310+
continue
311+
}
312+
changedExperiments.current.delete(confirmation.id)
313+
if (experiments[confirmation.id] !== confirmation.liveValue) {
314+
experiments[confirmation.id] = confirmation.liveValue
315+
}
316+
}
317+
for (const [id, liveValue] of adopted) {
303318
if (experiments[id] !== liveValue) {
304319
experiments[id] = liveValue
305320
}

‎webview-ui/src/components/settings/__tests__/SettingsView.experimentRetirement.spec.tsx‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,4 +183,39 @@ describe("SettingsView - experiment edits after the host confirms them", () => {
183183
expect(experiments).not.toHaveProperty(String(IMAGE_ID))
184184
expect(experiments).toEqual({ [FOCUS_ID]: false })
185185
}, 20000)
186+
187+
it("keeps a toggle made after the save when the host confirms the earlier value", async () => {
188+
await openExperimentalTab()
189+
190+
// Save posts the experiment on, and the user toggles it back off before the host answers.
191+
await toggleExperiment(IMAGE_LABEL)
192+
await clickSave()
193+
const firstSave = posted("updateSettings").at(-1)
194+
expect(firstSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: true })
195+
await toggleExperiment(IMAGE_LABEL)
196+
expect(requireCheckbox(IMAGE_LABEL).checked).toBe(false)
197+
198+
// The answer to the earlier save arrives: it confirms the value that was submitted, not the
199+
// value the user wants now, so it may neither flip the checkbox nor retire the edit.
200+
await act(async () => {
201+
hostApi?.setExperimentEnabled(IMAGE_ID, true)
202+
})
203+
await waitFor(() => expect(requireCheckbox(IMAGE_LABEL).checked).toBe(false))
204+
205+
// The newer intent is still an edit this view owes the host, so the next save posts it.
206+
await clickSave()
207+
const secondSave = posted("updateSettings").at(-1)
208+
expect(secondSave?.updatedSettings?.experiments).toEqual({ [IMAGE_ID]: false })
209+
210+
// Once the host confirms that value, the session is retired and a later save leaves the
211+
// experiment out of the payload entirely.
212+
await act(async () => {
213+
hostApi?.setExperimentEnabled(IMAGE_ID, false)
214+
})
215+
await toggleExperiment(FOCUS_LABEL)
216+
await clickSave()
217+
const thirdExperiments = posted("updateSettings").at(-1)?.updatedSettings?.experiments ?? {}
218+
expect(thirdExperiments).not.toHaveProperty(String(IMAGE_ID))
219+
expect(thirdExperiments).toEqual({ [FOCUS_ID]: false })
220+
}, 20000)
186221
})

0 commit comments

Comments
 (0)