From 34cc358902cdd6e1057100f40a770fc62c3c6f2c Mon Sep 17 00:00:00 2001 From: Iisyourdad Date: Fri, 12 Jun 2026 08:40:33 -0500 Subject: [PATCH] Never take a post-click screenshot when a pre-click frame exists The remaining 'captured slightly after the click' reports came from the fresh-shot fallback, which grabs the screen when the click is processed (after it). The previous lead change made that fallback *more* likely: a frame now had to be >=120ms before the click to qualify, so on machines where the capture stream can't always keep a frame that old buffered, more clicks fell through to the post-click shot. Make the click-lead a two-tier preference instead of a hard gate in selectFrameForClick: 1. newest frame captured at least leadMs before the click (ideal margin), else 2. newest frame captured before the click at all. Only when no pre-click frame exists does the caller fresh-shot. leadMs is threaded through the stream backend to the worker so both selection paths agree. Verified end to end: frames land ~120-170ms before each click, markers stay at 0.00%, and the 8-click burst still saves all 8. Co-Authored-By: Claude Fable 5 --- app/capture.js | 24 ++++++++++----------- app/click-frames.js | 37 ++++++++++++++++++++++++++------- app/renderer/capture-worker.js | 1 + app/stream-backend.js | 3 ++- docs/CHANGELOG.md | 10 +++++---- tests/unit/capture.test.js | 4 ++-- tests/unit/click-frames.test.js | 33 +++++++++++++++++++++++++++++ 7 files changed, 84 insertions(+), 28 deletions(-) diff --git a/app/capture.js b/app/capture.js index 4401076..5d9c1bd 100644 --- a/app/capture.js +++ b/app/capture.js @@ -557,31 +557,29 @@ class CaptureService { async frameForClick(clickPos = null, clickAt = Date.now()) { const mode = this.settings.get('capture.mode') || 'fullscreen'; const grabMode = mode === 'region' ? 'fullscreen' : mode; - const rawClickTime = Number.isFinite(clickAt) ? clickAt : Date.now(); - // Click lead: aim selection at a moment slightly *before* the hook + const clickTime = Number.isFinite(clickAt) ? clickAt : Date.now(); + // Click lead: prefer a frame captured a little *before* the hook // timestamp. The hook fires on button-down, but the visible UI often - // starts reacting within a frame or two of that (hover→press states, - // the cursor settling), and capture-stream pixels lag the real screen - // by a frame. Targeting clickTime - leadMs keeps the saved screenshot - // clear of the click's own onset so the step shows the screen the user - // was about to act on. Tunable via capture.clickLeadMs. + // starts reacting within a frame or two (hover→press states, the cursor + // settling) and capture-stream pixels lag the real screen slightly, so a + // frame timestamped right at the click can still show the click's onset. + // The lead is a *preference*: selection falls back to any pre-click + // frame when none is old enough, so it never forces a post-click fresh + // shot. Tunable via capture.clickLeadMs. const leadMs = Math.max(0, Number(this.settings.get('capture.clickLeadMs')) || 0); - const clickTime = rawClickTime - leadMs; const strict = this.strictClickFrames(); const opts = { clickAt: clickTime, + leadMs, clickPos, mode: grabMode, strict, - // The lead shifts the target earlier, so widen the staleness budget by - // the same amount — a frame that was fresh enough for the real click - // is still fresh enough for the lead-adjusted target. - maxAgeMs: CLICK_FRAME_MAX_AGE_MS + leadMs, + maxAgeMs: CLICK_FRAME_MAX_AGE_MS, startSlackMs: CLICK_FRAME_START_SLACK_MS, }; if (this.streamBackend && this.streamBackend.isActive() && grabMode === 'fullscreen') { - const frame = await this.streamBackend.frameForClick({ clickPos, clickAt: clickTime, strict }); + const frame = await this.streamBackend.frameForClick({ clickPos, clickAt: clickTime, strict, leadMs }); if (frame) return frame; // No qualifying frame (or the backend just went unhealthy): fall // through to the loop buffer / fresh-shot fallbacks below. diff --git a/app/click-frames.js b/app/click-frames.js index f46c4c1..4246892 100644 --- a/app/click-frames.js +++ b/app/click-frames.js @@ -125,14 +125,7 @@ function frameUsableForClick(frame, { return startedAt <= clickTime + startSlackMs; } -/** - * Best already-buffered frame for a click: the newest frame that qualifies - * under frameUsableForClick. Buffered frames are by definition completed, so - * in-flight acceptance never applies here. Returns null when nothing - * qualifies and the caller must wait for the in-flight grab or fall back to - * a fresh shot. - */ -function selectFrameForClick(frames, opts = {}) { +function newestUsableFrame(frames, opts) { let best = null; for (const frame of frames || []) { if (!frameUsableForClick(frame, { ...opts, allowInFlight: false })) continue; @@ -141,6 +134,34 @@ function selectFrameForClick(frames, opts = {}) { return best; } +/** + * Best already-buffered frame for a click, in two tiers: + * 1. with a click lead (opts.leadMs > 0): the newest frame captured at least + * leadMs *before* the click, so the step shows the screen the user was + * about to act on — clear of the click's own onset; + * 2. failing that, the newest frame captured before the click at all. + * + * The two tiers matter for correctness, not just polish: the lead is a + * *preference*, never a hard gate. If it were a gate, a click with no frame + * old enough to satisfy the lead would fall through to the caller's fresh + * shot — which captures the screen *after* the click. The tier-2 fallback + * guarantees that as long as any pre-click frame exists, we use it rather + * than shooting post-click. Buffered frames are always completed, so + * in-flight acceptance never applies here. + */ +function selectFrameForClick(frames, opts = {}) { + const leadMs = Math.max(0, Number(opts.leadMs) || 0); + const clickAt = Number.isFinite(opts.clickAt) ? opts.clickAt : Date.now(); + if (leadMs > 0) { + // Widen the staleness budget by the lead so a frame that was fresh + // enough for the real click is still fresh enough for the lead target. + const maxAgeMs = (opts.maxAgeMs == null ? DEFAULT_MAX_AGE_MS : opts.maxAgeMs) + leadMs; + const led = newestUsableFrame(frames, { ...opts, clickAt: clickAt - leadMs, maxAgeMs }); + if (led) return led; + } + return newestUsableFrame(frames, { ...opts, clickAt }); +} + const api = { FrameRing, frameUsableForClick, diff --git a/app/renderer/capture-worker.js b/app/renderer/capture-worker.js index 3870d28..2e0d216 100644 --- a/app/renderer/capture-worker.js +++ b/app/renderer/capture-worker.js @@ -147,6 +147,7 @@ await sampleFrame(state); const frame = StepForgeClickFrames.selectFrameForClick(state.ring.frames(), { clickAt: cmd.clickAt, + leadMs: cmd.leadMs || 0, mode: 'fullscreen', strict: cmd.strict !== false, }); diff --git a/app/stream-backend.js b/app/stream-backend.js index b56ff16..97f9032 100644 --- a/app/stream-backend.js +++ b/app/stream-backend.js @@ -210,7 +210,7 @@ class StreamCaptureBackend { * Resolves null when no frame qualifies (caller falls back) — and also on * timeout, which additionally counts toward unhealthiness. */ - frameForClick({ clickPos = null, clickAt = Date.now(), strict = true } = {}) { + frameForClick({ clickPos = null, clickAt = Date.now(), strict = true, leadMs = 0 } = {}) { if (!this.active || !this.host) return Promise.resolve(null); const displays = [...this.streams.values()].filter((s) => s.ready).map((s) => s.display); const display = clickPos ? displayForDipPoint(clickPos, displays) : (displays[0] || null); @@ -234,6 +234,7 @@ class StreamCaptureBackend { displayId: display.id, clickAt, strict, + leadMs, }); }); } diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index b2a4fd6..948d84d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -24,10 +24,12 @@ Keep-a-Changelog conventions; versions follow semver. - Physical→DIP coordinate conversion is multi-monitor and scale-factor aware (`screen.screenToDipPoint` on Windows, display-geometry math elsewhere), fixing marker drift on displays scaled away from 100%. - - A configurable click-lead (`capture.clickLeadMs`, default 120ms) targets - the screen just before each click so the saved step shows what the user - was about to act on, not the click's onset; the stream sampling cadence - was tightened to 50ms so a frame near that target always exists. + - A configurable click-lead (`capture.clickLeadMs`, default 120ms) prefers + a frame captured a little before each click so the saved step shows what + the user was about to act on, not the click's onset; the stream sampling + cadence was tightened to 50ms so a frame near that target always exists. + The lead is a preference, not a gate: selection falls back to the newest + frame still before the click, so it never forces a post-click screenshot. ### Fixed diff --git a/tests/unit/capture.test.js b/tests/unit/capture.test.js index f975aa8..f08ab4d 100644 --- a/tests/unit/capture.test.js +++ b/tests/unit/capture.test.js @@ -798,8 +798,8 @@ test('click frames come from the stream backend when it is active', async () => assert.equal(result.ok, true); assert.deepEqual(added, ['stream-frame']); - assert.deepEqual(requests, [{ clickPos: { x: 10, y: 10 }, clickAt, strict: true }], - 'the worker receives the hook-time click timestamp and strictness'); + assert.deepEqual(requests, [{ clickPos: { x: 10, y: 10 }, clickAt, strict: true, leadMs: 0 }], + 'the worker receives the hook-time click timestamp, strictness, and lead'); }); test('a stream backend with no qualifying frame falls through to the fresh-shot path', async () => { diff --git a/tests/unit/click-frames.test.js b/tests/unit/click-frames.test.js index d4623e5..8475113 100644 --- a/tests/unit/click-frames.test.js +++ b/tests/unit/click-frames.test.js @@ -124,6 +124,39 @@ test('frames of the wrong capture mode are rejected', () => { assert.equal(frameUsableForClick(f, { clickAt, mode: 'window' }), true); }); +test('the click lead prefers a frame captured at least leadMs before the click', () => { + const clickAt = 10_000; + const frames = [ + frame('with-margin', { startedAt: 9780, capturedAt: 9800 }), // 200ms before + frame('right-at-click', { startedAt: 9970, capturedAt: 9985 }), // 15ms before + ]; + + const chosen = selectFrameForClick(frames, { clickAt, mode: 'fullscreen', strict: true, leadMs: 120 }); + + assert.equal(chosen.name, 'with-margin', + 'with a lead, the frame clear of the click onset wins over the one right at it'); +}); + +test('the click lead falls back to any pre-click frame rather than forcing a post-click shot', () => { + // The whole point of the two-tier rule: when nothing satisfies the lead, + // we still return a pre-click frame (caller would otherwise fresh-shot + // *after* the click). Only "right-before" exists here. + const clickAt = 10_000; + const frames = [frame('right-before', { startedAt: 9960, capturedAt: 9980 })]; + + const chosen = selectFrameForClick(frames, { clickAt, mode: 'fullscreen', strict: true, leadMs: 120 }); + + assert.equal(chosen.name, 'right-before'); +}); + +test('the click lead still returns null when no frame precedes the click at all', () => { + const clickAt = 10_000; + const frames = [frame('after', { startedAt: 10_050, capturedAt: 10_080 })]; + + assert.equal(selectFrameForClick(frames, { clickAt, strict: true, leadMs: 120 }), null, + 'a post-click frame is never selected; the caller takes the fresh-shot fallback'); +}); + test('a frame without startedAt falls back to capturedAt for the strict check', () => { const clickAt = 10_000; const before = frame('legacy-before', { capturedAt: 9950 });