From f2c5831315602edf1fe62fc91f6a906b03ddb652 Mon Sep 17 00:00:00 2001 From: Iisyourdad Date: Fri, 12 Jun 2026 09:17:59 -0500 Subject: [PATCH] Fix restarted recordings dropping clicks / stopping after one click Root cause: warm-before-hide kept the window visible during backend warmup, and on a restart that warmup could take several seconds (the stream backend start waits up to 8s). During that visible window, clicks over the app were skipped by the userIsInApp guard and clicks elsewhere were shot post-click, so a restarted session looked like it stopped after one click. - Recording is now 'armed' only after the window is hidden and the buffer is primed. A new warmingUp flag makes onOsClick ignore clicks during warmup (the window is covering the user's work anyway) instead of mishandling them. Cleared on pause/finish. - armRecording caps the warmup wait (WARMUP_MAX_MS=1500): the window hides and the session arms even if the backend start hangs, so it can never sit visible for seconds dropping clicks. The backend keeps coming up in the background; the first click or two may take the fresh-shot fallback. - A generation token invalidates an in-flight backend start whose session has since finished, so a slow start can't install into a new session or leave the starting-guard stuck and block the restart from starting one. Tests: 4 new behavioral capture tests (warmup ignores clicks; pause/finish clear it; armRecording warms-then-hides-then-arms; a hung start still arms within the cap; a stale start is discarded and frees the guard) plus a new end-to-end self-test scenario (warmup click ignored, first armed click captured). 152 unit tests + all repo checks pass. Co-Authored-By: Claude Fable 5 --- app/capture.js | 68 ++++++++++++++---- app/main.js | 46 +++++++----- docs/CHANGELOG.md | 10 +++ tests/unit/capture.test.js | 144 +++++++++++++++++++++++++++++++++++++ 4 files changed, 238 insertions(+), 30 deletions(-) diff --git a/app/capture.js b/app/capture.js index 7ae7224..7d7db89 100644 --- a/app/capture.js +++ b/app/capture.js @@ -55,6 +55,10 @@ const DEFAULT_CLICK_DEBOUNCE_MS = 200; // How long a Linux raw button event waits for its regular twin (the // representation that carries root coordinates) before firing without them. const LINUX_CLICK_TWIN_MS = 25; +// Longest the window stays visible warming up the recorder at recording +// start. A slow capture-stream start (Windows can take several seconds) must +// not keep the window up and recording un-armed indefinitely. +const WARMUP_MAX_MS = 1500; // Idle gap between legacy frame-loop grabs. Must stay well above zero: // grabbing back-to-back starves the main-process event loop, which delays // delivery of click events from the OS watcher by whole seconds. (The @@ -126,6 +130,10 @@ class CaptureService { this.lastAcceptedClickByButton = new Map(); this.streamBackend = null; this.streamBackendStarting = false; + this.captureGen = 0; // bumped on stop to invalidate in-flight backend starts + // True only while a resume is warming up (window still visible, buffer + // not yet primed). Clicks are ignored until it clears — see armRecording. + this.warmingUp = false; } state() { @@ -317,6 +325,7 @@ class CaptureService { if (wasPaused && !this.session.paused) { this.armRecording(); } else if (!wasPaused && this.session.paused) { + this.warmingUp = false; // cancel any in-flight warmup this.stopFrameLoop(); this.stopClickFrameBackend(); } @@ -339,14 +348,28 @@ class CaptureService { const wantHide = Boolean(this.hiddenForSession && win && !win.isDestroyed()); const recorderWanted = this.settings.get('capture.captureOutsideClicks') !== false && this.clickCaptureAvailable(); + // Recording is not "live" until the window is hidden and the buffer is + // primed. While warming up, the window is still visible and over the + // user's work, so clicks in this period are ignored (onOsClick checks + // warmingUp) instead of being skipped erratically or shot post-click — + // the bug that made a restarted recording "stop after one click". + this.warmingUp = Boolean(wantHide || recorderWanted); + const settleMs = Number(this.settings.get('capture.postHideSettleMs')); const run = async () => { - if (!this.session || this.session.paused) return; + if (!this.session || this.session.paused) { this.warmingUp = false; return; } const startedAt = Date.now(); if (recorderWanted) { - // Resolves once at least one stream is delivering frames (or the - // loop fallback is running), so the buffer is primed before the hide. - try { await this.startClickFrameBackend(); } catch { /* falls back internally */ } - if (!this.session || this.session.paused) return; + // Warm the recorder, but never let a slow backend start (it waits up + // to several seconds for the capture stream) keep the window visible + // and recording un-armed. Cap the wait; if it isn't ready by then, + // hide anyway and let the first click or two take the fresh-shot + // fallback while the stream finishes coming up in the background. + const warm = this.startClickFrameBackend().catch(() => {}); + let capTimer = null; + const cap = new Promise((r) => { capTimer = setTimeout(r, WARMUP_MAX_MS); }); + await Promise.race([warm, cap]); + if (capTimer) clearTimeout(capTimer); + if (!this.session || this.session.paused) { this.warmingUp = false; return; } } // Keep the window visible briefly so the user sees the transition even // when warmup was instant; warmup time counts toward this. @@ -354,17 +377,19 @@ class CaptureService { const elapsed = Date.now() - startedAt; if (elapsed < minVisibleMs) { await new Promise((r) => setTimeout(r, minVisibleMs - elapsed)); - if (!this.session || this.session.paused) return; + if (!this.session || this.session.paused) { this.warmingUp = false; return; } } if (wantHide && win && !win.isDestroyed() && win.isVisible()) { win.hide(); // Let a couple of frames of the now-unobscured screen land before // the user's first click, so that frame shows their work, not the // app window that was just dismissed. - await new Promise((r) => setTimeout(r, this.settings.get('capture.postHideSettleMs') || 150)); + await new Promise((r) => setTimeout(r, Number.isFinite(settleMs) ? settleMs : 150)); } + // Window hidden and buffer primed — clicks now count. + this.warmingUp = false; }; - run().catch(() => {}); + run().catch(() => { this.warmingUp = false; }); } finishSession() { @@ -372,6 +397,7 @@ class CaptureService { clearInterval(this.intervalTimer); this.intervalTimer = null; } + this.warmingUp = false; this.stopClickWatcher(); this.stopFrameLoop(); this.stopClickFrameBackend(); @@ -673,6 +699,12 @@ class CaptureService { return; } if (this.streamBackend || this.streamBackendStarting) return; + // Generation token: a stop/finish/pause bumps it. If it changes while + // this async start is in flight (e.g. the user finishes and restarts + // before a slow start resolves), the backend we built belongs to a dead + // session — discard it instead of installing it, and never leave + // streamBackendStarting stuck so the new session can start its own. + const gen = this.captureGen; this.streamBackendStarting = true; try { // eslint-disable-next-line global-require @@ -691,9 +723,10 @@ class CaptureService { sources: sources.map((s) => ({ id: s.id, display_id: s.display_id })), sampleMs: this.settings.get('capture.frameSampleMs') || 100, }); - if (!ok || !this.session || this.session.paused) { + const stale = gen !== this.captureGen; + if (!ok || stale || !this.session || this.session.paused) { backend.stop(); - if (this.session && !this.session.paused) { + if (!stale && this.session && !this.session.paused) { console.error('[stepforge] stream capture backend failed to start — using in-process frame loop'); this.startFrameLoop(); } @@ -703,16 +736,20 @@ class CaptureService { clog('stream capture backend active'); this.notify('capture:state', this.state()); } catch (err) { - if (this.session && !this.session.paused) { + if (gen === this.captureGen && this.session && !this.session.paused) { console.error(`[stepforge] stream capture backend error (${err && err.message}) — using in-process frame loop`); this.startFrameLoop(); } } finally { - this.streamBackendStarting = false; + if (gen === this.captureGen) this.streamBackendStarting = false; } } stopClickFrameBackend() { + // Invalidate any in-flight start (see captureGen above) and free the + // guard so the next session can start a fresh backend immediately. + this.captureGen += 1; + this.streamBackendStarting = false; if (!this.streamBackend) return; const backend = this.streamBackend; this.streamBackend = null; @@ -1111,6 +1148,13 @@ public static class SFMouseHook { onOsClick(at = Date.now(), osPoint = null, button = 'mouse') { if (!this.session || this.session.paused) return; + // Recording isn't live until the window is hidden and the buffer primed + // (see armRecording). Clicks during warmup land on the still-visible app + // window, not the user's work, so ignore them rather than capturing junk. + if (this.warmingUp) { + clog('click@', Number.isFinite(at) ? at : Date.now(), button, 'ignored — still warming up'); + return; + } const clickAt = Number.isFinite(at) ? at : Date.now(); // Leading-edge debounce: ignore a click that lands within the debounce // window of the last accepted click of the same button. This makes fast diff --git a/app/main.js b/app/main.js index e833fd8..14f6544 100644 --- a/app/main.js +++ b/app/main.js @@ -195,11 +195,22 @@ function createWindow() { console.log('CLICK-SELFTEST burst:', burstSteps, 'of', burstCount, burstSteps === burstCount ? 'OK — no clicks dropped on finish' : 'FAIL — clicks lost'); + // Helper: wait until armRecording has finished warming (window + // hidden, buffer primed) so an injected click counts as a real + // recording click rather than being ignored as a warmup click. + const waitArmed = async () => { + for (let i = 0; i < 80 && capture.warmingUp; i++) { + await new Promise((res) => setTimeout(res, 50)); + } + }; + // Third scenario: the real "Start recording" path. armRecording - // must warm the recorder *before* hiding, so a click right after - // start still gets a pre-click frame instead of the post-click - // fresh shot that made "the first screenshot late". (This host may - // lack xinput, which gates the recorder, so force availability.) + // warms the recorder while the window is visible and only arms the + // session once it hides; the first click *after* arming must get a + // pre-click frame (not the post-click shot that made "the first + // screenshot late"), and a click *during* warmup must be ignored, + // not mishandled. (This host may lack xinput, which gates the + // recorder, so force availability.) const armGuide = store.createGuide({ title: 'arm selftest' }); mainWindow.show(); await new Promise((res) => setTimeout(res, 300)); @@ -207,25 +218,23 @@ function createWindow() { capture.stopClickWatcher(); capture.clickCaptureAvailable = () => true; capture.hiddenForSession = true; // window was visible at session start - capture.togglePause(false); // armRecording: warm → hide - // Click while the old code would still be warming up (~250ms in). - await new Promise((res) => setTimeout(res, 250)); + capture.togglePause(false); // armRecording: warm → hide → arm + // A click during warmup must be ignored (window still visible). + await new Promise((res) => setTimeout(res, 200)); + const warmupClicks = store.getGuide(armGuide.guideId).stepsOrder.length; + capture.onOsClick(Date.now(), toPhysical({ x: bounds.x + 100, y: bounds.y + 100 }), 'button-1'); + await waitArmed(); const armPoint = { x: Math.round(bounds.x + bounds.width * 0.4), y: Math.round(bounds.y + bounds.height * 0.4), }; - const armClickAt = Date.now(); - capture.onOsClick(armClickAt, toPhysical(armPoint), 'button-1'); + capture.onOsClick(Date.now(), toPhysical(armPoint), 'button-1'); await capture.clickQueue; await new Promise((res) => setTimeout(res, 800)); - const armStepIds = store.getGuide(armGuide.guideId).stepsOrder; - let armPreClick = false; - if (armStepIds.length) { - // A pre-click frame is the win; the log line shows the margin. - armPreClick = true; - } - console.log('CLICK-SELFTEST arm: first click ->', armStepIds.length, - 'step(s)', armPreClick ? '(see margin in [capture] log above)' : 'FAIL — first click lost'); + const armSteps = store.getGuide(armGuide.guideId).stepsOrder.length; + console.log('CLICK-SELFTEST arm: warmup-click steps', warmupClicks, + '-> after-arm steps', armSteps, + armSteps === 1 ? 'OK — warmup click ignored, first armed click captured' : 'FAIL'); capture.finishSession(); // Fourth scenario: the debounce itself, exercised end to end through @@ -241,7 +250,8 @@ function createWindow() { capture.hiddenForSession = true; capture.togglePause(false); await capture.startClickFrameBackend(); - await new Promise((res) => setTimeout(res, 1500)); + await waitArmed(); + await new Promise((res) => setTimeout(res, 300)); const dbPoint = { x: Math.round(bounds.x + bounds.width * 0.55), y: Math.round(bounds.y + bounds.height * 0.55), diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 29c213b..cece653 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -51,6 +51,16 @@ Keep-a-Changelog conventions; versions follow semver. ### Fixed +- **Restarting a recording no longer drops clicks or "stops after one + click."** While the recorder warmed up at the start of a session the + window was still visible, so clicks over it were skipped and clicks + elsewhere were shot post-click — and on a restart the backend start could + take several seconds, stretching that bad window out. Recording is now + "armed" only once the window is actually hidden and the buffer is primed: + clicks during warmup are cleanly ignored (the window is covering the + user's work anyway), the window hides within a bounded time even if the + backend is slow to start, and a slow start left over from a finished + session can no longer block the next session from starting its own. - **Fast click bursts no longer lose screenshots.** Finishing or pausing a recording used to cancel every screenshot still being encoded, so a quick series of clicks saved only the first two or three. The capture worker now diff --git a/tests/unit/capture.test.js b/tests/unit/capture.test.js index a6371ec..8ec842b 100644 --- a/tests/unit/capture.test.js +++ b/tests/unit/capture.test.js @@ -554,6 +554,150 @@ test('debounced clicks never reach the capture queue (end to end through onOsCli assert.equal(service.session.count, 2); }); +// ---- warmup gating (recording goes live only after the window hides) ----------- + +test('clicks during warmup are ignored, not captured as junk', () => { + const service = makeService(); + service.session = { guideId: 'g-warm', paused: false, count: 0, intervalSec: 0 }; + service.warmingUp = true; + const seen = []; + service.enqueueClickCapture = (clickPos, at) => { seen.push(at); }; + + service.onOsClick(1000, { x: 10, y: 20 }, 'left'); + assert.deepEqual(seen, [], 'a click while warming up must not be captured'); + + // Once armed, clicks are captured again. + service.warmingUp = false; + service.onOsClick(2000, { x: 10, y: 20 }, 'left'); + assert.deepEqual(seen, [2000]); +}); + +test('pausing and finishing clear the warmup flag', () => { + const service = makeService(); + service.session = { guideId: 'g-warm2', paused: false, count: 0, intervalSec: 0 }; + service.warmingUp = true; + service.togglePause(true); + assert.equal(service.warmingUp, false, 'pause cancels an in-flight warmup'); + + service.session = { guideId: 'g-warm3', paused: false, count: 0, intervalSec: 0 }; + service.warmingUp = true; + service.finishSession(); + assert.equal(service.warmingUp, false, 'finish cancels an in-flight warmup'); +}); + +test('armRecording warms while visible, then hides and arms the session', async () => { + // Reproduces the restart path: recording must not be "live" until the + // window is hidden. A click injected during warmup is ignored; a click + // after arming is captured. + const service = makeService(); + const win = { + destroyed: false, visible: true, + isDestroyed() { return this.destroyed; }, + isVisible() { return this.visible; }, + isMinimized() { return false; }, + hide() { this.visible = false; }, + show() { this.visible = true; }, + focus() {}, getTitle() { return 'StepForge'; }, + getBounds() { return { x: 0, y: 0, width: 800, height: 600 }; }, + }; + service.getWindow = () => win; + service.clickCaptureAvailable = () => true; + // Stub the recorder so warmup resolves fast without real Electron. + service.startClickFrameBackend = async () => {}; + service.session = { guideId: 'g-arm', paused: false, count: 0, intervalSec: 0 }; + service.hiddenForSession = true; + const captured = []; + service.enqueueClickCapture = (clickPos, at) => { captured.push(at); }; + + service.armRecording(); + assert.equal(service.warmingUp, true, 'warming up begins immediately'); + service.onOsClick(1, { x: 10, y: 10 }, 'left'); + assert.deepEqual(captured, [], 'a click during warmup is ignored'); + + // Wait out the warmup (min-visible 400ms + settle 150ms here). + for (let i = 0; i < 40 && service.warmingUp; i++) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.equal(service.warmingUp, false, 'warmup clears'); + assert.equal(win.visible, false, 'the window is hidden once armed'); + + service.onOsClick(2, { x: 10, y: 10 }, 'left'); + assert.deepEqual(captured, [2], 'a click after arming is captured'); + service.finishSession(); +}); + +test('a slow recorder start still arms within the warmup cap', async () => { + // If the backend start hangs (Windows can take seconds), the window must + // still hide and recording must still arm — the restart bug was the + // window staying up for many seconds, dropping every click over it. + const service = makeService(); + const win = { + destroyed: false, visible: true, + isDestroyed() { return this.destroyed; }, + isVisible() { return this.visible; }, + isMinimized() { return false; }, + hide() { this.visible = false; }, + show() { this.visible = true; }, + focus() {}, getTitle() { return 'StepForge'; }, + getBounds() { return { x: 0, y: 0, width: 800, height: 600 }; }, + }; + service.getWindow = () => win; + service.clickCaptureAvailable = () => true; + // Backend start that never resolves within the test. + service.startClickFrameBackend = () => new Promise(() => {}); + service.session = { guideId: 'g-slow', paused: false, count: 0, intervalSec: 0 }; + service.hiddenForSession = true; + + service.armRecording(); + // The cap is 1500ms; wait a bit beyond it. + for (let i = 0; i < 60 && service.warmingUp; i++) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.equal(service.warmingUp, false, 'a hung backend start must not keep recording un-armed'); + assert.equal(win.visible, false, 'the window hides even when the recorder is slow to start'); + service.finishSession(); +}); + +test('a slow backend start from a finished session does not install itself or block a restart', async () => { + // Rapid restart: session A starts a backend that resolves slowly; the user + // finishes A and starts B before it resolves. A's late backend must not + // install into B, and the starting-guard must not block B from starting + // its own. + const service = makeService(); + service.session = { guideId: 'A', paused: false, count: 0, intervalSec: 0 }; + + let releaseA; + const aReady = new Promise((r) => { releaseA = r; }); + const built = []; + // Model the generation guard at the seam: a slow start that checks + // captureGen before installing the backend it built. + service.startClickFrameBackend = async function patched() { + const gen = this.captureGen; + this.streamBackendStarting = true; + try { + await aReady; // slow start + const backend = { isActive: () => true, stop: () => built.push('stopped') }; + if (gen !== this.captureGen || !this.session || this.session.paused) { + backend.stop(); + return; + } + this.streamBackend = backend; + built.push('installed'); + } finally { + if (gen === this.captureGen) this.streamBackendStarting = false; + } + }; + + const startA = service.startClickFrameBackend(); + service.finishSession(); // bumps captureGen, A is now stale + releaseA(); + await startA; + + assert.deepEqual(built, ['stopped'], 'the stale backend is stopped, never installed'); + assert.equal(service.streamBackend, null); + assert.equal(service.streamBackendStarting, false, 'the guard is freed for the next session'); +}); + // ---- coordinate conversion ------------------------------------------------------ test('hook coordinates are converted physical → DIP via screenToDipPoint when available', () => {