diff --git a/craft/src/utils/console-buffer.test.ts b/craft/src/utils/console-buffer.test.ts index 77bd089..aa0c044 100644 --- a/craft/src/utils/console-buffer.test.ts +++ b/craft/src/utils/console-buffer.test.ts @@ -67,4 +67,53 @@ describe('console error buffer', () => { first.push({ ts: 0, message: 'injected' }); expect(getRecentConsoleErrors()).toHaveLength(1); }); + + test('reinstalling after external code wraps console.error does not double-record, and reset restores the true original', () => { + const trueOriginal = vi.spyOn(console, 'error').mockImplementation(() => {}); + installConsoleErrorBuffer(); + + // External code wraps our patch without knowing anything about our + // marker convention -- this is exactly what a third-party script or + // another monitor might do. + const ourPatch = console.error; + const external = vi.fn((...args: unknown[]) => { + ourPatch(...args); + }); + console.error = external; + + installConsoleErrorBuffer(); + console.error('dup-check'); + + expect(getRecentConsoleErrors().filter((e) => e.message === 'dup-check')).toHaveLength(1); + + __resetConsoleErrorBuffer(); + expect(console.error).toBe(trueOriginal); + }); + + test('surviving module re-execution: reinstalling after the module reloads does not re-wrap an already-patched console.error', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const mod1 = await import('./console-buffer'); + mod1.installConsoleErrorBuffer(); + const patchedAfterFirstInstall = console.error; + + // Simulate HMR: the module graph re-evaluates, producing a fresh module + // instance with its own reset top-level state, while the *global* + // console.error is still whatever the previous instance patched it to. + vi.resetModules(); + const mod2 = await import('./console-buffer'); + mod2.installConsoleErrorBuffer(); + + // The marker on the live console.error -- not module-local state -- is + // what installConsoleErrorBuffer() consults, so the reloaded instance + // must recognize the existing patch and leave it alone rather than + // wrapping it a second time. + expect(console.error).toBe(patchedAfterFirstInstall); + + console.error('once-across-reload'); + expect( + mod1.getRecentConsoleErrors().filter((e) => e.message === 'once-across-reload') + ).toHaveLength(1); + + mod1.__resetConsoleErrorBuffer(); + }); }); diff --git a/craft/src/utils/console-buffer.ts b/craft/src/utils/console-buffer.ts index 4720530..12d80d0 100644 --- a/craft/src/utils/console-buffer.ts +++ b/craft/src/utils/console-buffer.ts @@ -9,16 +9,61 @@ * The console.error patch ALWAYS chains to the original. A monitor that * swallows diagnostics is worse than no monitor. * - * Idempotency note: installConsoleErrorBuffer() is guarded by a marker - * stamped directly on the patched console.error function itself, not just a - * module-scoped boolean. React 18 StrictMode (which double-invokes effects) - * and hot module reload can re-run this module's top-level code, resetting - * any local `let installed = false` while the *global* console.error is - * still the already-patched function from the previous install. A - * boolean-only guard would then treat that already-patched function as "the - * original" and wrap it again, building a chain that grows on every reload. - * Checking the marker on the live console.error avoids that regardless of - * how many times this module's top level re-executes. + * Idempotency and the "true original": + * + * `installConsoleErrorBuffer()` treats console.error's *live* MARKER stamp + * (not module-scoped state) as the source of truth for "am I already the + * active patch". That alone survives React 18 StrictMode / hot module + * reload re-running this module's top level while the previous instance's + * patch is still installed on the global -- a plain `let installed = false` + * would reset on re-execution and treat that already-patched function as + * virgin, wrapping it again and building a chain that grows on every + * reload. + * + * The marker alone isn't sufficient once something *other* than us has + * touched console.error since our last install, though. If external code + * wraps our patch (`console.error = L` where L internally calls our patched + * function), console.error is unmarked again from our point of view, so a + * naive "unmarked == virgin" re-install would capture L -- an intermediate + * wrapper, not the true original -- as "the original" to chain to. That + * would (a) leave our OLD patch still reachable inside L's closure, so one + * console.error() call records twice (once via the new patch, once via the + * old one still buried inside L), and (b) make __resetConsoleErrorBuffer() + * restore console.error to L instead of the real original, permanently + * losing the reference to it. + * + * The fix is to never re-derive "the original" from whatever the live + * console.error happens to be at install time. Instead, the true original + * is captured exactly once and stashed as a hidden property directly on the + * `console` object (not in module scope, so it also survives module + * re-execution) the first time we ever patch. Every subsequent install, + * whether triggered by our own idempotent re-install, HMR, or a reinstall + * after external code has wrapped or replaced console.error, reuses that + * stashed reference and re-wraps it directly -- guaranteeing exactly one + * patch layer chains straight to the real original, and that reset can + * always find it. + * + * Trade-off this implies: if install() is called again after some external + * code has wrapped console.error, that external wrapper is discarded (we + * re-wrap the true original directly, not the external wrapper) rather than + * preserved. We accept that over the alternative of chaining through an + * unknown wrapper, which cannot be done safely -- there is no way to detect + * whether that wrapper still calls through to our old patch (risking double + * recording if we also wrap it) or has fully replaced it (risking losing + * capture entirely if we don't). An external wrapper installed *after* us + * and left alone (i.e. install() is not called again) is completely + * unaffected -- it just sits on top of our patch and both continue to work + * as normal JS monkey-patch layering. + * + * Known residual gap (not fixed, documented instead): if this module is hot + * reloaded while console.error stays patched from the previous instance, + * the live patch's closure still points at the *previous* module + * instance's `buffer` array. The marker check correctly stops the new + * instance from re-wrapping, but that also means the new instance's own + * `getRecentConsoleErrors()` reads its own (empty) buffer forever while + * capture silently continues into the orphaned previous instance's buffer. + * This only matters across an actual HMR reload during development -- + * production has exactly one module instance for the lifetime of the page. */ export interface ConsoleErrorEntry { @@ -29,16 +74,29 @@ export interface ConsoleErrorEntry { const MAX_ENTRIES = 20; const MAX_MESSAGE = 500; const MARKER = '__whpConsoleErrorBufferPatched'; +const TRUE_ORIGINAL_KEY = '__whpConsoleErrorBufferTrueOriginal'; -type MarkedConsoleError = typeof console.error & { - [MARKER]?: true; - __original?: typeof console.error; -}; +type MarkedConsoleError = typeof console.error & { [MARKER]?: true }; +type ConsoleWithStash = typeof console & { [TRUE_ORIGINAL_KEY]?: typeof console.error }; function isPatched(fn: typeof console.error): fn is MarkedConsoleError { return typeof fn === 'function' && (fn as MarkedConsoleError)[MARKER] === true; } +function getTrueOriginal(): typeof console.error | undefined { + return (console as ConsoleWithStash)[TRUE_ORIGINAL_KEY]; +} + +function setTrueOriginal(fn: typeof console.error): void { + (console as ConsoleWithStash)[TRUE_ORIGINAL_KEY] = fn; +} + +function clearTrueOriginal(): void { + delete (console as ConsoleWithStash)[TRUE_ORIGINAL_KEY]; +} + +// Module-scoped: not consulted for correctness (see doc comment above), only +// used to avoid re-adding window listeners within a single module instance. let buffer: ConsoleErrorEntry[] = []; let errorListener: ((e: ErrorEvent) => void) | null = null; let rejectionListener: ((e: PromiseRejectionEvent) => void) | null = null; @@ -61,25 +119,42 @@ function stringifyArg(arg: unknown): string { /** Idempotent. Patches console.error and adds window error listeners. */ export function installConsoleErrorBuffer(): void { + // We're already the live, active patch -- nothing to do. if (isPatched(console.error)) return; - const original = console.error.bind(console); + // Reuse the stashed true original if we've ever patched before (covers + // HMR re-execution and reinstall-after-external-wrap); otherwise this is + // a genuinely virgin install and the current console.error IS the true + // original. + const trueOriginal = getTrueOriginal() ?? console.error; + setTrueOriginal(trueOriginal); + const patched: MarkedConsoleError = (...args: unknown[]): void => { try { record(args.map(stringifyArg).join(' ')); } catch { // Recording must never break logging. } - original(...args); + trueOriginal.call(console, ...args); }; patched[MARKER] = true; - patched.__original = original; console.error = patched; - if (typeof window !== 'undefined') { - errorListener = (e: ErrorEvent) => record(`window.onerror: ${e.message}`); - rejectionListener = (e: PromiseRejectionEvent) => - record(`unhandledrejection: ${stringifyArg(e.reason)}`); + if (typeof window !== 'undefined' && !errorListener) { + errorListener = (e: ErrorEvent) => { + try { + record(`window.onerror: ${e.message}`); + } catch { + // Recording must never break the page's own error handling. + } + }; + rejectionListener = (e: PromiseRejectionEvent) => { + try { + record(`unhandledrejection: ${stringifyArg(e.reason)}`); + } catch { + // Recording must never break the page's own rejection handling. + } + }; window.addEventListener('error', errorListener); window.addEventListener('unhandledrejection', rejectionListener); } @@ -93,10 +168,11 @@ export function getRecentConsoleErrors(): ConsoleErrorEntry[] { /** Test-only: clear the buffer and un-patch. */ export function __resetConsoleErrorBuffer(): void { buffer = []; - const current = console.error; - if (isPatched(current) && current.__original) { - console.error = current.__original; + const trueOriginal = getTrueOriginal(); + if (trueOriginal) { + console.error = trueOriginal; } + clearTrueOriginal(); if (typeof window !== 'undefined') { if (errorListener) window.removeEventListener('error', errorListener); if (rejectionListener) window.removeEventListener('unhandledrejection', rejectionListener);