fix(cursor): make automatic cursor overlay navigation-safe - #43
Merged
Conversation
Follow-up to #42. Three issues in the `framenavigated` reinjection path: 1. `document.body` can be null at navigation commit. Playwright emits `framenavigated` when the nav commits, which can precede the parser producing <body> (a render/parser-blocking resource in <head> widens the window). `cursorHighlight()` ended in `document.body.appendChild`, throwing `TypeError: Cannot read properties of null` — a message that is not in cursor.ts's disposal-error allowlist, so it surfaced as a warning and left the page permanently cursor-less. Injection now defers to DOMContentLoaded when body is absent, with a generation counter so a later call supersedes a queued install. 2. Same-document navigations reinjected needlessly. Playwright dispatches `navigated` from `frameCommittedSameDocumentNavigation` too, so every pushState/hash route change rebuilt the ring: it reset to left:-100px (invisible until the next mouse event) and the replace path removed the element without invoking the `__cleanup` stored on it, leaking a capturing mousemove + click listener per route change (N leaks => N stacked ripples per click). Adds an internal `ensureCursorHighlight()` that no-ops when a ring is present or queued; the replace path now runs the previous cleanup. 3. The paint nudge was chained behind the cursor evaluate. That listener exists to unstick CDP promptly after a navigation, and the evaluate blocks on the new document's execution context. Paint now fires immediately and again once the cursor lands. `ensureCursorHighlight` is deliberately not re-exported from index.ts — public API surface is unchanged. Tests execute the injected browser function against a DOM stub, so the injection logic itself is covered rather than just its arguments. Each new test was verified to fail against the pre-fix implementation.
This was referenced Aug 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #42 (thanks @OleksandrKucherenko!). The config → CLI →
record()→ env → runtime wiring in that PR was complete and correct; this fixes three edge cases in the newframenavigatedreinjection path.1.
document.bodycan benullat navigation commitPlaywright emits
framenavigatedwhen the navigation commits, which can precede the parser producing<body>— a parser-blocking<script src>or stylesheet in<head>widens that window to hundreds of ms.cursorHighlight()ended indocument.body.appendChild(dot), so it threwTypeError: Cannot read properties of null (reading 'appendChild'). That message isn't incursor.ts's disposal-error allowlist, so it surfaced asWarning: cursor highlight failed: …and the ring was gone for the rest of that page (no furtherframenavigatedto retry).Injection now defers to
DOMContentLoadedwhenbodyis absent, guarded by a generation counter so a later call supersedes a still-queued install rather than stacking two rings.2. Same-document navigations reinjected needlessly
Playwright dispatches
navigatedfromframeCommittedSameDocumentNavigationas well (frames.js:186→frameDispatcher.js:57→client/frame.js:82), so everypushState/replaceState/hash route change hit the listener even though the document was never replaced. Two consequences:left: -100px; top: -100px, so the cursor disappears from the recording after every SPA route change until the next mouse event;cursorHighlight()'s replace path removed the old element but never invoked the__cleanupit stashed on it, leaking a capturingmousemove+clicklistener on a detached node per route change (N leaks → N stacked ripples per click).Adds an internal
ensureCursorHighlight()that no-ops when a ring is already present or queued. The explicit-replace path now runs the previous cleanup first, socursorHighlight()called twice in one document no longer leaks either.3. The paint nudge was chained behind the cursor evaluate
That
framenavigatedlistener exists to unstick CDP promptly afterpage.goto()lands (heavy SPAs otherwise hold the stale pre-nav JPEG as gap-fill source). Chaining_triggerPaint()onto the cursor injection deferred it by the evaluate round-trip plus the wait for the new document's execution context — lengthening the exact window it was written to shorten. Paint now fires immediately and again once the cursor lands.Notes
ensureCursorHighlightis intentionally not re-exported fromindex.ts— public API surface is unchanged.this._recordingPage = pageout of the legacyelsebranch (feat(recording): add automatic cursor overlay #42) means_triggerPaint()frommark()now actually runs on the chromium +jpeg-stitchCDP-direct path, where it was previously a silent no-op. That's a latent bug fixed incidentally by feat(recording): add automatic cursor overlay #42 and worth keeping.Testing
npx tsc --noEmitnpx vitest run— 683 passing. The 3 failures (tests/e2e/record.e2e.test.ts,tests/transitions/shader-render.test.ts) reproduce on a cleanmainand are missing local Playwright browsers, not related to this change.page.evaluateand execute it against a minimal DOM stub, so the injection logic is covered rather than just its arguments — null-body deferral, supersede-on-requeue,ensureno-op paths, and cleanup-on-replace.__cleanupcall on replace →replacing a ring runs the previous cleanup so listeners do not leaknudges paint after navigation without waiting on the cursor injection