diff --git a/browse/src/cdp-inspector.ts b/browse/src/cdp-inspector.ts index 293b6eb8b..fd4a938d1 100644 --- a/browse/src/cdp-inspector.ts +++ b/browse/src/cdp-inspector.ts @@ -139,7 +139,41 @@ async function getOrCreateSession(page: Page): Promise { // ─── Modification History ─────────────────────────────────────── +// Bounded FIFO of style modifications. Pre-cap, this was an unbounded +// module-scoped array that grew for every CSS edit made through $B css +// across the whole browser session — small per-entry footprint but no +// upper bound, the kind of slow leak that compounds over multi-day +// inspector use. The cap is 200 because per-session undo workflows +// rarely walk back more than a handful of edits, and a user who really +// wants to roll a long change back can `$B css reset` to revert all of +// them. totalPushed is monotonic across the session so undoModification +// can tell the user when their target index has been evicted, instead +// of just "no modification at index N". +const MOD_HISTORY_CAP = 200; const modificationHistory: StyleModification[] = []; +let modHistoryTotalPushed = 0; + +function pushModification(mod: StyleModification): void { + modificationHistory.push(mod); + modHistoryTotalPushed++; + while (modificationHistory.length > MOD_HISTORY_CAP) { + modificationHistory.shift(); + } +} + +// Test-only entry: exposes the history-cap mechanics (push, reset, cap value) +// without requiring a CDP-driven Page. Production code must go through +// modifyStyle / undoModification / resetModifications. +export const __testInternals = { + pushModification, + MOD_HISTORY_CAP, + getRawHistory: () => modificationHistory.slice(), + getTotalPushed: () => modHistoryTotalPushed, + resetForTest: () => { + modificationHistory.length = 0; + modHistoryTotalPushed = 0; + }, +}; // ─── Specificity Calculation ──────────────────────────────────── @@ -568,7 +602,7 @@ export async function modifyStyle( method, }; - modificationHistory.push(modification); + pushModification(modification); return modification; } @@ -578,7 +612,12 @@ export async function modifyStyle( export async function undoModification(page: Page, index?: number): Promise { const idx = index ?? modificationHistory.length - 1; if (idx < 0 || idx >= modificationHistory.length) { - throw new Error(`No modification at index ${idx}. History has ${modificationHistory.length} entries.`); + const evictedNote = modHistoryTotalPushed > MOD_HISTORY_CAP + ? ` (most recent ${MOD_HISTORY_CAP} only — ${modHistoryTotalPushed - MOD_HISTORY_CAP} earlier entries evicted at the cap)` + : ''; + throw new Error( + `No modification at index ${idx}. History has ${modificationHistory.length} entries${evictedNote}.`, + ); } const mod = modificationHistory[idx]; @@ -657,6 +696,7 @@ export async function resetModifications(page: Page): Promise { } } modificationHistory.length = 0; + modHistoryTotalPushed = 0; } /** diff --git a/browse/test/cdp-inspector-history-cap.test.ts b/browse/test/cdp-inspector-history-cap.test.ts new file mode 100644 index 000000000..21b2d6c22 --- /dev/null +++ b/browse/test/cdp-inspector-history-cap.test.ts @@ -0,0 +1,95 @@ +import { describe, test, expect, beforeEach } from 'bun:test'; +import type { Page } from 'playwright'; +import { + __testInternals, + undoModification, +} from '../src/cdp-inspector'; + +// Regression tests for the modificationHistory cap (D6 / smoking gun #2). +// Pre-cap, the module-scoped array grew unbounded across the session. Cap is +// 200 entries, oldest evicted on push past the cap. undoModification reports +// "evicted at the cap" in the error message so a user who asks for a +// no-longer-available index understands what happened (instead of seeing the +// pre-cap "No modification at index 500" with no context). + +const { pushModification, MOD_HISTORY_CAP, getRawHistory, getTotalPushed, resetForTest } = __testInternals; + +function fakeMod(id: number) { + return { + selector: `#node-${id}`, + property: 'color', + oldValue: 'red', + newValue: 'blue', + source: 'inline' as const, + timestamp: id, + method: 'setProperty' as 'setProperty', + }; +} + +beforeEach(() => { + resetForTest(); +}); + +describe('modificationHistory cap', () => { + test('1. push under cap keeps every entry', () => { + for (let i = 0; i < 50; i++) pushModification(fakeMod(i)); + expect(getRawHistory().length).toBe(50); + expect(getTotalPushed()).toBe(50); + expect(getRawHistory()[0].timestamp).toBe(0); + expect(getRawHistory()[49].timestamp).toBe(49); + }); + + test('2. push exactly cap keeps every entry', () => { + for (let i = 0; i < MOD_HISTORY_CAP; i++) pushModification(fakeMod(i)); + expect(getRawHistory().length).toBe(MOD_HISTORY_CAP); + expect(getTotalPushed()).toBe(MOD_HISTORY_CAP); + expect(getRawHistory()[0].timestamp).toBe(0); + }); + + test('3. push past cap evicts oldest, keeps length at cap', () => { + const total = MOD_HISTORY_CAP + 50; + for (let i = 0; i < total; i++) pushModification(fakeMod(i)); + expect(getRawHistory().length).toBe(MOD_HISTORY_CAP); + expect(getTotalPushed()).toBe(total); + // Oldest 50 dropped — entry that was #0 is gone; new oldest is #50. + expect(getRawHistory()[0].timestamp).toBe(50); + expect(getRawHistory()[MOD_HISTORY_CAP - 1].timestamp).toBe(total - 1); + }); + + test('4. resetForTest clears both buffer and totalPushed', () => { + for (let i = 0; i < 10; i++) pushModification(fakeMod(i)); + resetForTest(); + expect(getRawHistory().length).toBe(0); + expect(getTotalPushed()).toBe(0); + }); +}); + +describe('undoModification eviction-aware error', () => { + // Stub Page: undoModification throws before any await when idx is out of + // range, so the stub never actually gets called. + const stubPage = {} as unknown as Page; + + test('5. out-of-range BEFORE any eviction → no evicted note', async () => { + for (let i = 0; i < 5; i++) pushModification(fakeMod(i)); + await expect(undoModification(stubPage, 99)).rejects.toThrow( + 'No modification at index 99. History has 5 entries.', + ); + }); + + test('6. out-of-range AFTER eviction → message names the evicted count', async () => { + const total = MOD_HISTORY_CAP + 73; + for (let i = 0; i < total; i++) pushModification(fakeMod(i)); + // 273 pushed, 200 in buffer, 73 evicted. Ask for idx=400 (above buffer). + await expect(undoModification(stubPage, 400)).rejects.toThrow( + `No modification at index 400. History has ${MOD_HISTORY_CAP} entries ` + + `(most recent ${MOD_HISTORY_CAP} only — 73 earlier entries evicted at the cap).`, + ); + }); + + test('7. negative explicit index throws cleanly (no NaN propagation)', async () => { + for (let i = 0; i < 10; i++) pushModification(fakeMod(i)); + await expect(undoModification(stubPage, -1)).rejects.toThrow( + 'No modification at index -1.', + ); + }); +});