Fix region capture, power ownership, click-source reporting, strict timing
Template tests / tests (pull_request) Failing after 33s

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 <[email protected]>
This commit is contained in:
2026-07-03 11:50:44 -07:00
co-authored by Claude Fable 5
parent c916234ae8
commit f62e3e19cb
4 changed files with 352 additions and 66 deletions
+26 -11
View File
@@ -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 };