From f62e3e19cb9d09ae23681ee504fc52b1b2ab447e Mon Sep 17 00:00:00 2001 From: Twest2 Date: Fri, 3 Jul 2026 11:50:44 -0700 Subject: [PATCH] Fix region capture, power ownership, click-source reporting, strict timing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of the improvement plan (PR 5 of the sequence). Several confirmed capture defects from the audit. Region capture: - regionCapture returned { ok, step } wrapping storeFrameAsStep's own { ok, step }, so the real step was at result.step.step — region selection and region auto-doc (which read result.step.stepId) broke. Return the storeFrameAsStep result directly. - pickRegion leaked the region:picked IPC listener (and the overlay/image refs) whenever the overlay was cancelled/closed rather than picked: cleanup only ran on a pick. Cleanup is now idempotent and runs on pick, close, load failure, and settle. The received rectangle is validated and clamped to the image (new overlayRectToImageRect handles negative-size drags and out-of-bounds selections) so image.crop can never read out of bounds. Power blocker ownership: - New single owner: CaptureService.syncPower() holds the blocker iff a session is actively recording, called on start/pause/resume/finish. A new session starts paused and no longer holds the blocker while idle; tray and second-instance pauses that previously bypassed main.js's stop closure now release it correctly. main.js provides the power policy (blocker + EcoQoS opt-out) via dependency injection. Explicit click-trigger source: - startEvdevWatcher never set clickWatcher, so state().clickCapture read false while evdev was actively capturing clicks. Replaced the boolean with an explicit clickSource (windows-hook | x11 | evdev-x11 | evdev-wayland | unavailable); clickCapture is now derived from it. evdev device-stream errors/closes now fall back via handleClickWatcherLoss instead of being swallowed. Strict click timing: - When no pre-click frame qualifies, strict mode previously fell through to a fresh (post-click) shot and stored it — contradicting the strict promise. It now skips with a capture:diagnostic instead. Non-strict (balanced) mode keeps the fresh-shot fallback. Existing tests that exercised the fallback now run in balanced mode; the strict test asserts the skip. Other: - pathToFileURL replaces file://${p} concatenation for step image URLs and export previews (correct for spaces, #, %, drive letters). - Best-effort click-queue drain on app shutdown (before-quit) so a burst just before quit is not lost; bounded so quit never hangs. Tests: region rect clamping/normalization, power-held-only-while-recording (incl. finish releases), click-source reporting incl. evdev, drain deadline. 230 unit tests pass. Click self-test: markers 3/3 (strict) and burst 8/8 deterministic across runs; the burst scenario runs in balanced mode because it tests the drain, not strict timing. (arm/debounce remain the pre-existing Linux capture failures untouched by this PR.) Co-Authored-By: Claude Fable 5 --- app/capture.js | 175 ++++++++++++++++++++++++++----- app/main.js | 86 +++++++++------ tests/unit/capture-fixes.test.js | 120 +++++++++++++++++++++ tests/unit/capture.test.js | 37 +++++-- 4 files changed, 352 insertions(+), 66 deletions(-) create mode 100644 tests/unit/capture-fixes.test.js diff --git a/app/capture.js b/app/capture.js index f10e046..54d0194 100644 --- a/app/capture.js +++ b/app/capture.js @@ -198,11 +198,18 @@ class CaptureService { notify, screenApi = screen, textIntel = null, + powerPolicy = null, }) { this.store = store; this.settings = settings; this.getWindow = getWindow; this.notify = notify; + // Single owner of OS power/throttling state for the capture lifecycle. + // setRecording(true) is called exactly while a session is actively + // recording (session present and not paused); setRecording(false) whenever + // it pauses or ends. No-op by default so tests and non-Electron hosts work. + this.powerPolicy = powerPolicy || { setRecording() {} }; + this._recordingPower = false; // Injectable for tests; the click/coordinate paths must never reach for // the global `screen` directly so coordinate handling stays testable. this.screen = screenApi; @@ -213,6 +220,10 @@ class CaptureService { this.session = null; // { guideId, paused, count, intervalSec } this.intervalTimer = null; this.clickWatcher = null; + // Explicit trigger source rather than a bare boolean, so the UI can tell + // the truth about how clicks are being captured (or that they are not): + // windows-hook | x11 | evdev-x11 | evdev-wayland | unavailable + this.clickSource = 'unavailable'; this.frameLoopTimer = null; this.frameLoopRunning = false; this.frameWaiters = []; @@ -240,6 +251,22 @@ class CaptureService { this.warmingUp = false; } + /** + * Reconcile OS power state with the actual recording state. Called after + * every session transition so there is exactly one owner: the blocker is + * held iff a session exists and is not paused. Idempotent. + */ + syncPower() { + const recording = Boolean(this.session && !this.session.paused); + if (recording === this._recordingPower) return; + this._recordingPower = recording; + try { + this.powerPolicy.setRecording(recording); + } catch { + // power management is best-effort; never break capture over it + } + } + state() { return this.session ? { @@ -248,12 +275,21 @@ class CaptureService { guideId: this.session.guideId, count: this.session.count, intervalSec: this.session.intervalSec || 0, - clickCapture: Boolean(this.clickWatcher), + // clickCapture reflects any live global click source (xinput/evdev/ + // Windows hook), not just the spawned-process watcher — evdev has no + // child process, so the old Boolean(this.clickWatcher) reported false + // while clicks were in fact being captured. + clickCapture: this.clickSource !== 'unavailable', + clickSource: this.clickSource, clickCaptureAvailable: this.clickCaptureAvailable(), clickFrameSource: this.streamBackend ? 'stream' : (this.frameLoopRunning ? 'loop' : 'idle'), strictClickFrames: this.strictClickFrames(), } - : { active: false, clickCaptureAvailable: this.clickCaptureAvailable() }; + : { + active: false, + clickSource: this.clickSource, + clickCaptureAvailable: this.clickCaptureAvailable(), + }; } /** @@ -331,6 +367,9 @@ class CaptureService { this.session = { guideId, paused: true, count: 0, intervalSec: interval }; if (this.settings.get('capture.captureOutsideClicks') !== false) this.startClickWatcher(); this.applyInterval(); + // A new session starts paused, so it must NOT hold the power blocker yet + // (it did before, leaking the blocker while idle). syncPower reconciles. + this.syncPower(); this.notify('capture:state', this.state()); // (Skipped for the dev screenshot hook, which needs a visible page.) @@ -476,6 +515,9 @@ class CaptureService { this.stopFrameLoop(); this.stopClickFrameBackend(); } + // Recording only while unpaused: this is the one place tray/second-instance + // pauses previously bypassed, leaking the power blocker. syncPower owns it. + this.syncPower(); if (this.rebuildTrayMenu) this.rebuildTrayMenu(); this.notify('capture:state', this.state()); } @@ -555,6 +597,9 @@ class CaptureService { this.stopClickFrameBackend(); this.destroySessionTray(); this.session = null; + // A finished session never records: release the power blocker here so it + // can't outlive the recording (finish from any path — bar, tray, quit). + this.syncPower(); if (this.hiddenForSession) { this.hiddenForSession = false; this.showWindow(); @@ -562,6 +607,23 @@ class CaptureService { this.notify('capture:state', this.state()); } + /** + * Best-effort drain of clicks still encoding in the queue, for application + * shutdown. Resolves when the queue settles or the deadline passes so quit + * is never blocked indefinitely. Clicks captured mid-session are pinned to + * their guide id and stored even after the session ends (see onOsClick). + */ + async drainPendingClicks(timeoutMs = 2000) { + try { + await Promise.race([ + this.clickQueue.catch(() => {}), + new Promise((resolve) => setTimeout(resolve, Math.max(0, timeoutMs))), + ]); + } catch { + // best effort + } + } + /** * True when the user is interacting with StepForge itself. Deliberately * based on cursor position over the visible window, not isFocused(): @@ -624,11 +686,22 @@ class CaptureService { if (result.ok) this.noteStepAdded(result.step, trigger, guideId); return result; } - // No usable frame: fall through to a one-off fresh shot — but only - // while still recording. After a stop, a fresh shot would show - // whatever replaced the user's workflow on screen. - clog('click@', clickAt, 'no frame qualified — falling back to a fresh (post-click) shot'); if (!sessionLive) return { ok: false, reason: 'session ended before the fallback shot' }; + // No usable pre-click frame. In strict mode a fresh shot now would be a + // POST-click frame — exactly what strict mode promises never to store. + // Skip with a visible diagnostic instead of silently labeling a + // post-click fallback as strict. Non-strict mode keeps the legacy + // fresh-shot fallback below. + if (this.strictClickFrames()) { + clog('click@', clickAt, 'strict mode — no pre-click frame; skipping rather than storing a post-click shot'); + this.notify('capture:diagnostic', { + kind: 'strict-click-skipped', + guideId, + reason: 'No pre-click frame was ready for this click. In strict timing mode StepForge skips the shot rather than capturing the screen after the click.', + }); + return { ok: false, reason: 'strict mode: no pre-click frame available for this click' }; + } + clog('click@', clickAt, 'no frame qualified — falling back to a fresh (post-click) shot'); } // For non-click triggers (interval, hotkey, manual) pull the latest frame @@ -1009,6 +1082,7 @@ class CaptureService { ? ['stdbuf', '-oL', 'xinput', 'test-xi2', '--root'] : ['xinput', 'test-xi2', '--root']; this.clickWatcher = spawn(argv[0], argv.slice(1), { stdio: ['ignore', 'pipe', 'ignore'] }); + this.clickSource = 'x11'; this.clickWatcher.stdout.on('data', (chunk) => { this.ingestClickWatcherChunk(chunk.toString(), 'linux'); }); @@ -1374,6 +1448,7 @@ public static class SFHook { stdio: ['ignore', 'pipe', 'pipe'], windowsHide: true, }); + this.clickSource = 'windows-hook'; this.clickWatcher.stdout.on('data', (chunk) => { this.ingestClickWatcherChunk(chunk.toString(), 'win32'); }); @@ -1425,6 +1500,7 @@ public static class SFHook { this.clickWatcher = null; } this.stopEvdevWatcher(); + this.clickSource = 'unavailable'; this.clickWatcherBuf = ''; this.linuxEvent = null; this.discardPendingRawClick(); @@ -1450,26 +1526,48 @@ public static class SFHook { buf = rest; for (const button of presses) this.onOsClick(Date.now(), null, button); }); - // A device can disappear (unplugged); just drop that stream. - stream.on('error', () => {}); + // A device can disappear (unplugged): drop that stream, and if it was + // the last live click source, fall back like any other watcher loss + // instead of silently going dark. + stream.on('error', () => this.handleEvdevStreamLoss(stream, 'device read error')); + stream.on('close', () => this.handleEvdevStreamLoss(stream, 'device closed')); this.evdevStreams.push(stream); } catch { // Node became unreadable between enumeration and open — skip it. } } if (!this.evdevStreams.length) { - console.error('[stepforge] no readable mouse input devices — add your user to the "input" group for per-click capture: sudo usermod -aG input "$USER" (then log out and back in)'); + this.clickSource = 'unavailable'; + console.error('[stepforge] no readable mouse input devices for per-click capture (see docs/linux for the least-privilege device-access setup)'); } else { + // evdev carries no cursor position on Wayland (no marker); on X11 without + // xinput it is the click source but likewise has no root coordinates. + this.clickSource = this.onWayland() ? 'evdev-wayland' : 'evdev-x11'; console.log(`[stepforge] per-click capture via evdev on ${this.evdevStreams.length} device(s)${this.onWayland() ? ' (Wayland: no click marker)' : ''}`); } } + /** One evdev device stream ended; drop it and fall back if none remain. */ + handleEvdevStreamLoss(stream, reason) { + if (!this.evdevStreams) return; // already stopped deliberately + const idx = this.evdevStreams.indexOf(stream); + if (idx === -1) return; + this.evdevStreams.splice(idx, 1); + try { stream.destroy(); } catch { /* already gone */ } + if (this.evdevStreams.length === 0) { + this.evdevStreams = null; + this.clickSource = 'unavailable'; + this.handleClickWatcherLoss(`evdev ${reason}`); + } + } + stopEvdevWatcher() { if (!this.evdevStreams) return; - for (const stream of this.evdevStreams) { + const streams = this.evdevStreams; + this.evdevStreams = null; // clear first so close handlers no-op + for (const stream of streams) { try { stream.destroy(); } catch { /* already closed */ } } - this.evdevStreams = null; } /** @@ -2007,13 +2105,43 @@ public static class SFHook { const cropped = image.crop(rect); const size = cropped.getSize(); if (!size.width || !size.height) return { ok: false, reason: 'empty selection' }; - const step = await this.storeFrameAsStep(guideId, 'region', { + // storeFrameAsStep already returns { ok, step }; return it directly so + // callers see result.step.stepId — not result.step.step.stepId, which is + // what wrapping it in another { ok, step } produced (region selection and + // region auto-documentation both read result.step.stepId). + return this.storeFrameAsStep(guideId, 'region', { image: cropped, size, display, cursor: null, }, null, null); - return { ok: true, step }; + } + + /** + * Convert an overlay selection (display px) into an image-space crop rect, + * clamped to the image bounds. Returns null for an empty/invalid rect. + */ + overlayRectToImageRect(rect, display, imgSize) { + if (!rect || !imgSize) return null; + const { width: iw, height: ih } = imgSize; + if (!iw || !ih) return null; + const sx = iw / display.bounds.width; + const sy = ih / display.bounds.height; + const toNum = (v) => (Number.isFinite(v) ? v : 0); + let x = Math.round(toNum(rect.x) * sx); + let y = Math.round(toNum(rect.y) * sy); + let w = Math.round(toNum(rect.w) * sx); + let h = Math.round(toNum(rect.h) * sy); + // Normalize negative-size drags (drawn up/left). + if (w < 0) { x += w; w = -w; } + if (h < 0) { y += h; h = -h; } + // Clamp to the image so image.crop can never read out of bounds. + x = Math.min(Math.max(0, x), iw); + y = Math.min(Math.max(0, y), ih); + w = Math.min(w, iw - x); + h = Math.min(h, ih - y); + if (w <= 0 || h <= 0) return null; + return { x, y, width: w, height: h }; } /** Fullscreen overlay window that resolves with a crop rect (image px). */ @@ -2038,31 +2166,28 @@ public static class SFHook { }); // The overlay may only display region.html; deny navigation/popups. require('./security').installWindowSecurity(overlay, 'region'); + const { ipcMain } = require('electron'); let settled = false; + // Idempotent cleanup: remove the IPC listener and close the overlay + // exactly once, whether the user picked, cancelled, closed, or the page + // failed to load. Previously the listener was only removed on a pick, so + // cancelling/closing leaked it (and the captured overlay/image refs). const finish = (rect) => { if (settled) return; settled = true; + ipcMain.removeListener('region:picked', onPick); if (!overlay.isDestroyed()) overlay.close(); resolve(rect); }; - const { ipcMain } = require('electron'); const onPick = (event, rect) => { if (event.sender !== overlay.webContents) return; - ipcMain.removeListener('region:picked', onPick); if (!rect) return finish(null); - const imgSize = image.getSize(); - const sx = imgSize.width / display.bounds.width; - const sy = imgSize.height / display.bounds.height; - finish({ - x: Math.round(rect.x * sx), - y: Math.round(rect.y * sy), - width: Math.round(rect.w * sx), - height: Math.round(rect.h * sy), - }); + finish(this.overlayRectToImageRect(rect, display, image.getSize())); }; ipcMain.on('region:picked', onPick); overlay.on('closed', () => finish(null)); - overlay.loadFile(path.join(__dirname, 'renderer', 'region.html')); + overlay.webContents.on('did-fail-load', () => finish(null)); + overlay.loadFile(path.join(__dirname, 'renderer', 'region.html')).catch(() => finish(null)); }); } } diff --git a/app/main.js b/app/main.js index 3909913..8e18cab 100644 --- a/app/main.js +++ b/app/main.js @@ -3,6 +3,7 @@ const path = require('node:path'); const fs = require('node:fs'); const os = require('node:os'); +const { pathToFileURL } = require('node:url'); const { app, BrowserWindow, ipcMain, dialog, shell, nativeTheme, globalShortcut, clipboard, nativeImage, screen, powerSaveBlocker, session, desktopCapturer, @@ -204,7 +205,12 @@ function createWindow() { // Second scenario, reproducing the "I clicked many times but only // got two screenshots" report: a fast burst of clicks immediately // followed by finishing the session, so most clicks are still - // queued (frames still encoding) when the stop lands. + // queued (frames still encoding) when the stop lands. This scenario + // tests the queue DRAIN, not strict timing — 30ms-apart clicks + // outpace the frame sampler, so run it in balanced mode where every + // queued click stores. (Strict-mode skip-vs-store is covered by the + // marker scenario above and by unit tests.) + settings.set('capture.strictClickFrames', false); const burstGuide = store.createGuide({ title: 'burst selftest' }); capture.startSession(burstGuide.guideId, { intervalSec: 0 }); capture.stopClickWatcher(); @@ -228,6 +234,7 @@ function createWindow() { const burstSteps = store.getGuide(burstGuide.guideId).stepsOrder.length; console.log('CLICK-SELFTEST burst:', burstSteps, 'of', burstCount, burstSteps === burstCount ? 'OK — no clicks dropped on finish' : 'FAIL — clicks lost'); + settings.set('capture.strictClickFrames', true); // restore for later scenarios // Helper: wait until armRecording has finished warming (window // hidden, buffer primed) so an injected click counts as a real @@ -514,7 +521,12 @@ function setupIpc() { }); h('step:imagePath', ({ guideId, stepId, which }) => { const p = store.stepImagePath(guideId, stepId, which || 'working'); - return p && fs.existsSync(p) ? `file://${p}?v=${fs.statSync(p).mtimeMs}` : null; + if (!p || !fs.existsSync(p)) return null; + // pathToFileURL correctly encodes spaces, #, %, drive letters, etc.; the + // mtime is a cache-buster so the renderer reloads after an edit. + const url = pathToFileURL(p); + url.searchParams.set('v', String(fs.statSync(p).mtimeMs)); + return url.href; }, { validate: (a) => c.id(a.guideId) && c.id(a.stepId) && (a.which === undefined || a.which === null || c.oneOf(a.which, ['original', 'working'])), @@ -651,43 +663,20 @@ function setupIpc() { } return result; }, { validate: (a) => c.id(a.guideId) }); - let capturePowerBlocker = -1; - const startCapturePower = () => { - if (!powerSaveBlocker.isStarted(capturePowerBlocker)) { - capturePowerBlocker = powerSaveBlocker.start('prevent-app-suspension'); - } - }; - const stopCapturePower = () => { - if (powerSaveBlocker.isStarted(capturePowerBlocker)) { - powerSaveBlocker.stop(capturePowerBlocker); - } - }; - - // Opt every live Electron process (browser, GPU, the screen-capture utility, - // any renderers) out of EcoQoS for the duration of a recording. The hidden - // capture-worker renderer is created later, during warmup, so it opts itself - // out separately (see stream-backend.js); this covers the rest. - const keepCaptureProcessesResponsive = () => { - try { - keepProcessesResponsive(app.getAppMetrics().map((m) => m.pid)); - } catch { /* metrics unavailable — best effort */ } - }; + // Power/throttling state is owned entirely by the capture service's + // recording transitions (see createCapturePowerPolicy) so there is exactly + // one owner: it is held iff a session is actively recording, and paused, + // finished, tray, and second-instance transitions all release it correctly. h('capture:session', async ({ action, guideId, intervalSec }) => { if (action === 'start') { capture.startSession(guideId, { intervalSec: intervalSec ?? null }); - startCapturePower(); - keepCaptureProcessesResponsive(); } else if (action === 'pause') { capture.togglePause(true); - stopCapturePower(); } else if (action === 'resume') { capture.togglePause(false); - startCapturePower(); - keepCaptureProcessesResponsive(); } else if (action === 'finish') { capture.finishSession(); - stopCapturePower(); } else if (action === 'interval') { capture.setInterval(intervalSec); } @@ -854,7 +843,7 @@ function setupIpc() { }); const result = runExport(format, ast, previewDir, options || {}); producedFiles.add(result.file); - return { ok: true, file: result.file, fileUrl: `file://${result.file}` }; + return { ok: true, file: result.file, fileUrl: pathToFileURL(result.file).href }; }, { validate: (a) => c.id(a.guideId) && validFormat(a.format) && (a.options === undefined || security.isPlainArgs(a.options)), @@ -967,12 +956,36 @@ if (!gotLock) { } }; + // Single owner of OS power/throttling state for recording. The capture + // service calls setRecording(true/false) on every recording transition; + // this holds a power-save blocker and opts live Electron processes out of + // EcoQoS while recording, and releases the blocker when recording stops. + const capturePowerPolicy = (() => { + let blocker = -1; + const keepResponsive = () => { + try { keepProcessesResponsive(app.getAppMetrics().map((m) => m.pid)); } catch { /* best effort */ } + }; + return { + setRecording(recording) { + if (recording) { + if (!powerSaveBlocker.isStarted(blocker)) { + blocker = powerSaveBlocker.start('prevent-app-suspension'); + } + keepResponsive(); + } else if (powerSaveBlocker.isStarted(blocker)) { + powerSaveBlocker.stop(blocker); + } + }, + }; + })(); + capture = new CaptureService({ store, settings, getWindow: () => mainWindow, notify: captureNotify, textIntel, + powerPolicy: capturePowerPolicy, }); // Deny-by-default permission policy. The only grant in the entire app is @@ -1025,6 +1038,19 @@ if (!gotLock) { }); }); + // Drain clicks still encoding in the capture queue before the app exits, so + // a fast burst immediately before quit is not lost. Defer the quit exactly + // once with a bounded deadline, then let it proceed. + let quitDrained = false; + app.on('before-quit', (event) => { + if (quitDrained || !capture) return; + quitDrained = true; + event.preventDefault(); + // Stop new clicks from being queued, then wait for the queue to settle. + capture.stopClickWatcher(); + capture.drainPendingClicks(2000).finally(() => app.quit()); + }); + app.on('will-quit', () => { globalShortcut.unregisterAll(); if (capture) { diff --git a/tests/unit/capture-fixes.test.js b/tests/unit/capture-fixes.test.js new file mode 100644 index 0000000..9f4700c --- /dev/null +++ b/tests/unit/capture-fixes.test.js @@ -0,0 +1,120 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const CaptureService = require('../../app/capture'); + +function makeService({ settings: settingsOverrides, powerPolicy } = {}) { + const settingsData = { + 'capture.mode': 'fullscreen', + 'capture.delayMs': 0, + ...settingsOverrides, + }; + return new CaptureService({ + store: {}, + settings: { get: (k) => (k in settingsData ? settingsData[k] : null) }, + getWindow: () => null, + notify: () => {}, + powerPolicy, + screenApi: { + getCursorScreenPoint: () => ({ x: 0, y: 0 }), + getAllDisplays: () => [], + }, + }); +} + +// ---- region rect clamping (bug: out-of-bounds / negative drags) ------------- + +test('overlayRectToImageRect scales, clamps, and normalizes selections', () => { + const svc = makeService(); + const display = { bounds: { x: 0, y: 0, width: 100, height: 100 } }; + const imgSize = { width: 200, height: 200 }; // 2x DPI + + // Simple selection: display px -> image px (2x). + assert.deepEqual( + svc.overlayRectToImageRect({ x: 10, y: 20, w: 30, h: 40 }, display, imgSize), + { x: 20, y: 40, width: 60, height: 80 } + ); + + // Negative-size drag (drawn up/left) normalizes to a positive rect. + assert.deepEqual( + svc.overlayRectToImageRect({ x: 40, y: 40, w: -20, h: -20 }, display, imgSize), + { x: 40, y: 40, width: 40, height: 40 } + ); + + // Selection larger than the screen is clamped to the image bounds. + const clamped = svc.overlayRectToImageRect({ x: -10, y: -10, w: 200, h: 200 }, display, imgSize); + assert.deepEqual(clamped, { x: 0, y: 0, width: 200, height: 200 }); + + // Degenerate selections return null instead of an out-of-bounds crop. + assert.equal(svc.overlayRectToImageRect({ x: 0, y: 0, w: 0, h: 0 }, display, imgSize), null); + assert.equal(svc.overlayRectToImageRect({ x: 999, y: 999, w: 10, h: 10 }, display, imgSize), null); + assert.equal(svc.overlayRectToImageRect(null, display, imgSize), null); +}); + +// ---- power ownership follows recording state -------------------------------- + +test('the power blocker is held only while actively recording', () => { + const calls = []; + const powerPolicy = { setRecording: (on) => calls.push(on) }; + const svc = makeService({ powerPolicy }); + + // startSession begins PAUSED — must not hold power. + svc.startSession('g1', { intervalSec: 0 }); + assert.deepEqual(calls, [], 'a paused new session must not start the power blocker'); + + // Resume records -> power on. (togglePause(false) arms recording.) + svc.togglePause(false); + assert.deepEqual(calls, [true]); + + // Pause -> power off. This is the tray/second-instance path that used to leak. + svc.togglePause(true); + assert.deepEqual(calls, [true, false]); + + // Resume again -> on, finish -> off. + svc.togglePause(false); + svc.finishSession(); + assert.deepEqual(calls, [true, false, true, false]); +}); + +test('finishSession releases power even if it was recording', () => { + const calls = []; + const svc = makeService({ powerPolicy: { setRecording: (on) => calls.push(on) } }); + svc.startSession('g1', { intervalSec: 0 }); + svc.togglePause(false); + calls.length = 0; + svc.finishSession(); + assert.deepEqual(calls, [false]); +}); + +// ---- explicit click-source reporting ---------------------------------------- + +test('click source is unavailable outside a session and after stop', () => { + const svc = makeService(); + assert.equal(svc.state().clickSource, 'unavailable'); + assert.equal(svc.state().clickCapture, undefined); // no session -> no field + svc.clickSource = 'evdev-x11'; + svc.stopClickWatcher(); + assert.equal(svc.clickSource, 'unavailable'); +}); + +test('state reports clickCapture true for a non-process (evdev) source', () => { + const svc = makeService(); + svc.session = { guideId: 'g', paused: false, count: 0, intervalSec: 0 }; + // evdev has no child process; the old Boolean(clickWatcher) reported false. + svc.clickSource = 'evdev-wayland'; + const st = svc.state(); + assert.equal(st.clickCapture, true); + assert.equal(st.clickSource, 'evdev-wayland'); +}); + +// ---- drain never hangs quit ------------------------------------------------- + +test('drainPendingClicks resolves within the deadline even if the queue hangs', async () => { + const svc = makeService(); + svc.clickQueue = new Promise(() => {}); // never settles + const start = Date.now(); + await svc.drainPendingClicks(60); + assert.ok(Date.now() - start < 1000, 'drain must not block on a hung queue'); +}); diff --git a/tests/unit/capture.test.js b/tests/unit/capture.test.js index 0c5e55c..9a69ce7 100644 --- a/tests/unit/capture.test.js +++ b/tests/unit/capture.test.js @@ -68,7 +68,9 @@ function makeFrame(name, ageMs = 0, overrides = {}) { // ---- fresh-shot fallback path ---------------------------------------------- test('click-triggered session capture uses the low-latency hide pause', async () => { - const service = makeService(); + // The fresh-shot fallback only runs in non-strict (balanced) mode; strict + // mode skips rather than storing a post-click shot. + const service = makeService({ settings: { 'capture.strictClickFrames': false } }); service.session = { guideId: 'guide-1', paused: false, count: 0, intervalSec: 0 }; let seenOptions = null; @@ -1006,7 +1008,9 @@ test('a buffered frame from a different display is ignored for click capture', a }); test('a stale buffered frame is not reused — the click falls back to a fresh shot', async () => { - const service = makeService(); + // Balanced (non-strict) mode: a stale frame is rejected and the click takes + // the fresh-shot fallback. (Strict mode skips instead — see below.) + const service = makeService({ settings: { 'capture.strictClickFrames': false } }); service.session = { guideId: 'guide-stale', paused: false, count: 0, intervalSec: 0 }; service.latestFrame = makeFrame('stale-png', 10_000); @@ -1022,12 +1026,12 @@ test('a stale buffered frame is not reused — the click falls back to a fresh s assert.equal(shootCalled, true, 'a stale buffered frame must not be reused'); }); -test('strict mode: a frame whose grab started after the click is rejected', async () => { - // This replaces the old "idle click waits for the imminent loop frame" - // behavior: a grab that begins after the click can already show the - // click's effects, so strict mode takes the explicit fresh-shot fallback - // instead of passing it off as the click-time screen. - const service = makeService(); +test('strict mode: no pre-click frame is skipped, never stored as a post-click shot', async () => { + // A grab that begins after the click can already show the click's effects. + // Strict mode's promise is that a stored step never shows the post-click + // screen, so when no pre-click frame qualifies it SKIPS with a diagnostic + // rather than taking a fresh (post-click) shot. + const service = makeService(); // strict is the default service.session = { guideId: 'guide-strict', paused: false, count: 0, intervalSec: 0 }; service.frameLoopRunning = true; service.frameLoopInFlight = false; // nothing in flight at click time @@ -1041,11 +1045,18 @@ test('strict mode: a frame whose grab started after the click is rejected', asyn shootCalled = true; return { ok: true, step: { stepId: 'fresh-step' } }; }; + const diagnostics = []; + service.notify = (channel, payload) => { + if (channel === 'capture:diagnostic') diagnostics.push(payload); + }; const result = await service.sessionCapture('click', { x: 1, y: 1 }, { at: clickAt }); - assert.equal(result.ok, true); - assert.equal(shootCalled, true); + assert.equal(result.ok, false); + assert.match(result.reason, /strict/i); + assert.equal(shootCalled, false, 'strict mode must not store a post-click shot'); + assert.equal(diagnostics.length, 1); + assert.equal(diagnostics[0].kind, 'strict-click-skipped'); }); test('balanced mode keeps the legacy slack: an imminent post-click frame is accepted', async () => { @@ -1183,7 +1194,9 @@ test('click frames come from the stream backend when it is active', async () => }); test('a stream backend with no qualifying frame falls through to the fresh-shot path', async () => { - const service = makeService(); + // Balanced (non-strict) mode: the fresh-shot fallback runs. Strict mode + // would skip rather than store a post-click shot. + const service = makeService({ settings: { 'capture.strictClickFrames': false } }); service.session = { guideId: 'guide-stream-miss', paused: false, count: 0, intervalSec: 0 }; service.streamBackend = { isActive: () => true, @@ -1279,6 +1292,8 @@ test('click capture marks the click-time cursor position', async () => { if (key === 'capture.clickMarker') return true; if (key === 'capture.clickMarkerColor') return '#E5484D'; if (key === 'editor.focusedViewDefaultForNewSteps') return false; + // Exercise the fresh-shot fallback: strict mode would skip instead. + if (key === 'capture.strictClickFrames') return false; return null; }; service.session = { guideId: 'guide-4', paused: false, count: 0, intervalSec: 0 };