fix(wgc): add GPU DXGI path for Windows capture readback - #304
Closed
Seb1900 wants to merge 35 commits into
Closed
Conversation
electron-builder signed the bundle itself until 26.15.3. Its macPackager
carried a `noIdentity && fallBackToAdhoc` branch handing back
`Identity("-")` when no certificate was found — mandatory on arm64, where
an unsigned binary will not launch. 26.15.3 replaced that path with
`findSigningIdentity`, which returns null instead, so `sign()` leaves on
`return false` and nothing signs the bundle. What ships is the bare
linker signature on the Electron binary: `Identifier=Electron`,
`Sealed Resources=none`.
macOS keys TCC grants to an app's code signature, so such a bundle can
never hold one. v1.9.0-rc.1 asked for Accessibility, the user granted it,
`AXIsProcessTrusted()` still returned false, and the editable-cursor
preflight re-opened the same dialog on every press of record. Recording
was impossible on macOS.
Sign ad-hoc ourselves with the runtime and entitlements electron-builder
would have applied, on both arches — 26.8.1 only fell back on arm64, so
Intel DMGs were never signed at all.
The verification step that should have caught this was gated on signing
being enabled, i.e. it never ran for the only builds that could be
unsigned. Make it unconditional, and assert the signing identifier
against the bundle id: `codesign --verify` passes on the bare linker
signature too, so the identifier is the only thing that separates a
bundle macOS can attach permissions to from one it cannot.
On the macOS and Linux native capture paths the webcam handle was built with no file name, which selects in-memory buffering. Nothing reached disk during capture, and finalize had to flatten the whole clip into one ArrayBuffer to hand it across IPC. Past ~2GB — a take of roughly 20 minutes at BITRATE_BASE — that allocation throws. The throw was swallowed twice over: fixWebmDuration catches its own FileReader failure and returns the unpatched blob, then the finalize catch logged to console and returned undefined, which made the attach guard skip attachNative*WebcamRecording entirely. The session was written screen-only and the editor opened as if nothing had happened, with the camera simply absent. A 23-minute take lost its webcam this way; shorter takes in the same app session saved fine. Pass the webcam file name on both native paths so chunks stream to disk as they arrive, the way the legacy path and the Windows helper already do. Finalize now branches on isStreaming(): a streamed clip hands over its name alone and the main process closes the stream and patches the WebM duration on disk, so nothing multi-gigabyte is ever flattened or sent across IPC. Buffered short takes keep the existing behaviour. Every failure now comes back with a reason and reaches the user as a toast. Silently discarding a completed take is the worst available outcome, and it was the one that shipped. Because the bytes now land on disk during capture, a webcam stream that isn't folded into a saved session is closed and its partial file removed — otherwise a discarded or failed take orphans a half-written .webm. The macOS and Linux finalizers were line-for-line copies, so the shared logic moves into finalizeWebcamAsset() rather than being duplicated again. Its tests pin both halves of the fix: that a streamed clip is never read into memory, and that a failure is never silent. Fixes getopenscreen#253
reindexRecordingOnDisk is Linux-gated by intent — Windows and macOS record through native helpers that write indexed files at the source, so the wrapper returns `unsupported-platform` before touching anything else. The suite never accounted for that: six of its cases inject a fake remux service and assert the remuxed result, so on a macOS or Windows checkout they stop at the guard and fail. They were red on a clean tree, for environmental reasons, with nothing to distinguish them from a real regression. Pin process.platform to linux in beforeEach and restore the real value in afterEach, so the cases exercise the wrapper's own logic on every platform. The technique is the one the suite's last case already used inline; hoisting it removes the per-test save/restore boilerplate and makes the restore hold even when a case throws part-way. The guard itself stays covered, now over both platforms it exists for rather than win32 alone, so pinning to Linux can't quietly become the only thing the suite exercises. No production code changes. Verified the cases still have teeth: removing the empty-output size check from reindexRecordingOnDisk fails the truncated-file case, which had been passing vacuously on macOS. `npx vitest --run` is now green on macOS: 137 files, 1626 passing.
The four notarization steps carried `&& !contains(github.ref_name, '-')`, which skipped them for every pre-release. Two costs, and the second is the one that mattered. Testers paid the first. A DMG signed with Developer ID but not notarized is still refused by Gatekeeper — `spctl` answers `rejected, source= Unnotarized Developer ID` — so anyone testing an RC had to know about `xattr -rd com.apple.quarantine` before they could open the build they were being asked to try. The release paid the second. Notarization never ran until the stable tag, so the first exercise of the credentials, the certificate chain and Apple's acceptance of every nested Mach-O landed on the highest-stakes build there is. The run that first enabled signing died in `Package .app bundle` on a malformed `MAC_CSC_NAME`; it was caught only because a full build was dispatched deliberately. Notarizing each RC makes every candidate a rehearsal. The trade is a few minutes per macOS job and a dependency on Apple's notary service being reachable, with `--wait` capped at 15 minutes. If that turns flaky enough to block RCs, the answer is `continue-on-error` on pre-releases rather than skipping them again. Five documentation sites asserted the old behaviour and are corrected here, so nothing claims RCs are unnotarized after this lands.
The appx manifest advertised en-US and fr-FR only, so the Microsoft Store product page listed two supported languages for an app that ships thirteen (SUPPORTED_LOCALES in src/i18n/config.ts), and the listing never surfaced in Store searches run in the other eleven. Bare tags for the region-less locales so every region of that language matches, which is what the renderer's own locale resolution does.
`CSC_NAME` must name the identity without its certificate type;
electron-builder chooses the type itself and refuses a qualified name:
⨯ Please remove prefix "Developer ID Application:" from the specified
name — appropriate certificate will be chosen automatically
It refuses at `Package .app bundle`, which runs after the ffmpeg build
and the compositor addon — about twelve minutes into the macOS job, and
nowhere else. That is what happened the first time signing was enabled
here: twelve minutes to learn that a secret had four extra words.
The mistake is easy to make because the same secret also feeds
`codesign --sign` at `Sign DMG`, and codesign accepts the full common
name, so the qualified form looks correct right up until
electron-builder sees it. The short form satisfies both, since codesign
matches on a substring of the common name.
Check it in `Resolve macOS signing`, where every other signing input is
already validated, and fail in seconds with the value to use instead.
Only prefixes ending in a colon match, so a company whose name starts
with one of these words is not caught.
The helper's stop wait was gated on the frame mutex:
std::unique_lock lock(mutex);
control.cv.wait(lock, [&]{ return control.stopRequested.load(); });
`stopRequested` is an atomic with no relationship to what that mutex
protects, but `condition_variable::wait` has to re-acquire it before it
can return -- and that mutex is held across uninterruptible D3D11 work:
the WGC frame callback's CopyResource, and the video writer's
Map(D3D11_MAP_READ) readback. One stalled driver call and the main thread
never came back, before the first [stop-timing] line was ever printed.
That is why issue getopenscreen#252 arrived with an empty diagnostic log.
Give stop its own mutex/CV pair that no frame thread ever touches, route
all nine stop sites through requestStop(), and bound the wait. Then bound
the shutdown itself: every step after the wait calls into a driver or
joins a thread that does, so decoupling the wait alone would only have
moved the hang. Each step gets a deadline under a global ceiling, and a
watchdog force-exits the process naming the step it died in.
Report success as soon as the MP4 index is written rather than at the end
of the process's life, so a wedged GPU teardown no longer costs a
recording that is already complete on disk. Check what finalize()
returns while doing it.
On the app side: keep a listener on the helper for the whole recording so
its diagnostics reach the bug report instead of being dropped between
start and stop, short-circuit a helper that already exited rather than
burning the timeout on a 'close' that can never arrive, and let discard
escape a wedged helper immediately.
The follow-on "Native Windows capture is not running." was separate: the
main process releases its helper handle unconditionally, the renderer did
not, so the next Record click sent a second stop. The same gap existed on
macOS and Linux.
The underlying stall is untouched -- the readback still runs inside the
frame lock, and the D3D adapter is still whichever one Windows hands us
on a four-adapter machine. What changes is that neither can hang the app.
Refs getopenscreen#252
…ture Review follow-up. The single `Recording stopped. Output path:` line was gated on `screenFinalized && webcamFinalized`, and the app treats that line as the only proof a recording is worth keeping. So an optional second file could veto a complete one: `webcamEncoder.finalize()` returning false discarded a perfectly indexed screen MP4, which was a regression against the previous behaviour where its result was ignored entirely. The same gate had a second way to fire. Both finalize steps clamp their deadline to the shared global ceiling, so a screen finalize that spent most of it left the webcam step already past its deadline, and the shutdown watchdog killed the process before the announcement ran. Announce after the screen finalize and before the webcam's, gated on the screen file alone. A failed webcam finalize is now an ERROR on stderr and a non-zero exit, which is what it always should have been -- not a lost recording. Also from the review: - Log the `quiesceCapture()` drain outcome at `wgc-quiesce`. It decides whether `wgc-session-close` releases the device or skips it, so a report that omits it cannot be read. - Keep `phase=` when parsing `[stop-timing]`. The diagnostic tool discarded it, which is the one field naming the step that hung, and the summary listed every step twice because begin and end lines both matched. Same double-count in the helper harness. - Pin `OPENSCREEN_WGC_STOP_BUDGET_MS` into the harness's child env and derive its hang limit from it. The limit was 30s against a 50s ceiling, so a long software-encoder finalize -- the case issue getopenscreen#34 exists for -- would have been killed and reported as the getopenscreen#252 hang. - Correct the shutdown budgets in the architecture doc: 8s per step, 50s overall, not 10s. - Clear `pendingCursorRecordingData` on the failed-stop path, matching the discard path.
The job filtered on `main`, so retargeting this PR at `release/v1.9.0` removed the artifact its own testing steps tell a reviewer to download. A recording fix aimed at a release is precisely when someone needs the compiled helper without a local MSVC toolchain.
…s clips The layout preset is global — one panel for the whole timeline — but the camera is per clip: a project mixes a screen+webcam recording with a plain import without one. `LiveParams::has_webcam` already carried that distinction, but only `live.rs` derived it, so the preview was right and every export was wrong. An export sets its `LiveParams` once for the whole timeline (`compositor-view-napi`), keeping the `true` default. And `ExportDialog` sends the SCREEN path as `webcamPath` when a clip has no camera, purely so the decoder has something valid to open — so the PiP box was drawn with the screen recording behind it, duplicated into its own corner. That is the mirror reported in getopenscreen#248, which the greyed-out Layout panel (correctly gated on `hasAnyClipWithCamera`) then left no way to turn off. `webcam_is_real` moves next to the field it decides, and `walk_composited_timeline` — shared by MP4 and GIF on all three backends — rebinds it per clip. A targeted `set_has_webcam` rather than a per-clip `set_live_params`, which would clobber the settings the caller posted. Verified on a real export (`run_composited_multi`, h264_amf) with the camera path equal to the screen path: the thumbnail is gone. The bench gains a `--webcam` override because the no-camera case is not different *content* but an identical *path*, and cannot be replayed otherwise. Refs getopenscreen#248
The preset is global — one panel for the whole timeline — but the camera is per clip. `layoutByClip` already carried a resolved layout per visible clip; it just never asked whether that clip had a camera, so every clip got the preset whether or not it had anything to put in it. Gating the camera's draw (`has_webcam`, previous commit) is not enough, and this is the half that actually shows. The block presets — `dual-frame`, `vertical-stack` — size the SCREEN off the block: they reserve the camera's half of the frame. A camera-less clip therefore kept its screen squeezed into that half with nothing beside it, which no draw-time gate can undo. Under picture-in-picture the defect is invisible, since the screen stays full-frame there and only a thumbnail is added. So a clip with no camera now lays out as if the preset were "no-webcam" — which `computeCompositeLayout` already implements (full-frame screen, no webcam rect). The predicate matches the `webcamPath` sent with the clip exactly, so the layout and the decoder cannot disagree. It is deliberately NOT `hasAnyClipWithCamera`, which gates the Layout panel and ignores `visible` on purpose so the panel stays reachable to un-hide a camera. `PreviewCanvas` had the same hole, and its own comment named it: it hid the webcam SLOT for a camera-less clip but left the screen geometry alone. Five regression tests, all of which fail on the parent commit: one per preset for "only the clip that has a camera gets a webcam rect", and one per block preset for "the camera-less clip gets its full frame back". The existing webcamRect test asserted a PiP rect for an asset with no camera — that premise was the bug, so its fixture gains the camera its preset presupposes and it goes on testing the px→fraction conversion it describes. Refs getopenscreen#248
`NotesWindow.module.css` was imported for its side effect only, with no binding. Rollup tree-shakes a binding-less CSS-module import out of the production bundle, so the shipped app carried zero `.tiptap` rules — dev looked fine because Vite injects module CSS at runtime there. Without `height: 100%` + `overflow-y: auto` the note body stops being a scroll container and grows to the height of its content. Paste a long note and ProseMirror scrolls the caret into view, which now scrolls the `overflow: hidden` shell instead: the toolbar leaves the viewport and the wheel cannot bring it back. Measured on a 400x540 window with ~5.8k characters pasted — editor 6360px tall, `scrollTop` pinned, toolbar at y=-5913, zero buttons hit-testable. Every selector in the file was already `:global`, so the CSS module bought nothing: rename it to plain `.css` and drop the wrappers. A plain CSS import is always emitted. This also restores the mirror transform, list markers, code blocks and heading sizes in production.
The wallpaper and a 50% padding were already on out of the box, but roundness, shadow and motion blur all defaulted to 0. The result was a hard-edged rectangle pasted onto a decorative background: the padding looked like wasted space rather than a deliberate margin, which is what issue getopenscreen#271 reported before blaming the padding for it. Ship the rest of the look instead of removing the half that was there: roundness 40px, shadow 20%, motion blur 20% (values picked on Windows). Padding stays at 50. Projects where the user already moved one of these sliders keep their value -- `legacyEditor` only stores keys that were explicitly patched -- so only projects that never touched them pick up the new look. Closes getopenscreen#271
cursor-sampler.exe shipped with no dpiAware manifest and never called SetProcessDpiAwareness*, so the process was DPI-unaware and Win32 handed it *virtualized* coordinates: GetCursorInfo().ptScreenPos and GetWindowRect both come back divided by the primary display's scale factor. b31bb71 assumed the opposite ("reports raw x/y in physical screen pixels") and converted the Electron display bounds to physical before normalizing. Both sides then lived in different spaces, so on a scaled display normalizeSample produced cx = (x/s)/W instead of x/W: the preview cursor sits at 1/s of its real offset from the top-left, an error that grows with the distance to the display origin (at 150% on a 2560px-wide screen, ~850px short at the right edge). Opt the helper into per-monitor-v2 awareness rather than walking the normalization back to DIPs: the numbers really do become physical, which is what every consumer already assumes, and unlike DIP normalization it also holds on mixed-DPI multi-monitor setups (virtualization always uses the primary display's scale, whatever monitor the cursor is actually on). Window captures were never affected: there the sampler supplies its own GetWindowRect bounds, so numerator and denominator were virtualized together and the ratio came out right either way. payload.x/y being physical now, the asset's display lookup needs screenToDipPoint -- screen.getDisplayNearestPoint works in DIPs and would otherwise pick the wrong monitor. macOS and Linux are unaffected: the SCK helper reports its capture frame in points, the same space as screen.getCursorScreenPoint(), and the PipeWire helper normalizes against the stream's own pixel dimensions. Verified: GetProcessDpiAwareness on the rebuilt binary reports PER_MONITOR_AWARE (was UNAWARE), and the reported position is unchanged at 100% scaling. Fixes getopenscreen#272
The HUD asked to be input-transparent twice: once here, at construction, and once from the renderer's mount effect. The first one is the whole bug in getopenscreen#266 — the app running, the bar painted, and every click, drag and button dead, forever, from the very first launch. On Windows `setIgnoreMouseEvents(true, { forward: true })` is a global WH_MOUSE_LL hook, and that hook is the only route back: Chromium delivers no pointermove to a window it has made input-transparent, so the renderer cannot ask to leave the state it is stuck in. Electron latches the install behind `forwarding_mouse_messages_` and only retries after a setIgnoreMouseEvents(false) — the call the dead hook prevents. So one hook that is refused, or that Windows revokes for overrunning the 300 ms LowLevelHooksTimeout, bricks the UI with no way out. Construction time is the worst possible moment to ask for it: that hook callback runs on the main thread, which is still booting the app. The renderer asks a frame or two later, over IPC, on a thread that is provably pumping messages — and if that ask never comes, the bar stays clickable instead of turning into a ghost. Costs an invisible rectangle that can swallow one desktop click in the two frames between show and mount. Refs getopenscreen#266.
Two halves, because both are load-bearing after getopenscreen#266: nothing may call setIgnoreMouseEvents while the HUD window is being constructed, and the renderer must still ask for it once it has mounted. The window is recreated through the app's own path (second-instance → showMainWindow → createHudOverlayWindow) with the native call taped, and the tape is read back synchronously — no await between arming it and snapshotting, so no renderer IPC can slip into what is meant to be construction only. The source selector is opened first purely to keep the window list non-empty while the HUD is destroyed: emptying it fires window-all-closed, which quits the app under the test. Ablated: restoring the deleted line turns duringConstruction into [[true, {forward: true}]] and the first assertion fails, so the test does cover the regression it claims to.
"A frame or two" was an assumption. Timing the real app from ready-to-show to the renderer's first hud-overlay-ignore-mouse-events puts it at 83 and 90 ms over two clean runs — roughly forty times what the comment claimed, and the number a reviewer should be weighing against getopenscreen#266. (A third run read -403 ms: the tape catches setIgnoreMouseEvents on the prototype, so an IPC still in flight from the destroyed HUD's renderer lands on the new window and dates the ask before the window exists. The probe was throwaway; the two clean runs are the ones quoted.)
`getTranscript` sliced at 800 segments under a comment reading "segments only — words would blow the context", written believing a segment was a phrase. On the production path a segment IS one word: whisper's word timings are mapped one-to-one in src/lib/captioning/transcribe.ts, and the real fixture has 129 words for 129 segments. So the cap cut at the 800th WORD. At a normal speaking rate that is the fifth minute, and nothing in the payload said so — the model read a sixth of a half-hour recording, trimmed the silences it could see, and reported the job done. Asking it afterwards does not help either: the history sent back carries message text, not tool results, so the next turn re-reads the same 800 words. A whole 30-minute transcript is ~285k characters, ~70k tokens — well inside every model this app talks to. The cap was a guess, not a measurement, so it is gone rather than raised. If a recording ever does approach a window, the fix is to know the window; there is no per-model context budget in the app today, and picking another number here would only move the silence. The workbench already carried a `longTranscript` fixture documenting this defect and never wired it to a scenario. Its comment is now true instead of a to-do.
`ai-agent.md` still announced "up to 800 transcript segments" — the number this branch removed, and the one that quietly cut a half-hour recording at its fifth minute. Reference that repeats a limit the code dropped is how the limit gets re-added.
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: getopenscreen#254 and getopenscreen#261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
…d trip
The measured cost of an auto-enhance turn is not its context, it is its shape:
19 tool calls in series, six addTrim and nine addZoom one at a time, each a full
round trip to the provider. `deep-agent/service.ts` says as much where it raises
`recursionLimit` — "one step per silence". On a half-hour recording that is
hundreds of round trips for a decision the model made in one breath.
Two batch tools, and nothing else changes:
- The element schema IS the unitary schema (`z.array(addTrimArgs)`), and the
executor REPLAYS the unitary tool per item, folding the document forward. Clip
resolution, anchoring, clamping, the wording of every refusal — identical by
construction rather than by a second implementation staying in step. A batch of
N is exactly N unitary calls minus N-1 round trips, and a test asserts that
against a document built the long way.
- Partial application, deliberately. `replaceTimeline` is the repo's other
array-taking tool and refuses in one block; that is right for rebuilding a
timeline and ruinous for adding ten independent cuts. Each item stands alone,
the result leads with requested / appliedCount / refusedCount, and `refused`
names the index and the unitary reason. `ok:false` is kept for the one case
where nothing landed — the only one where the document did not move, and the
only one where chat-service's applied-calls list may legitimately stay empty.
- No cap on the array, for the reason `getTranscript` just taught us: a number
guessed here would be the next thing to cut a recording in half in silence.
The unitary tools stay. A one-off correction should not need a one-element array,
and `setTrim` — the tool for "move that trim" — was never in scope for a batch.
Two invariants that had to be found rather than assumed:
- `diffMatches` reads an id at the TOP level of the tool result, so a batch would
have made the only check that catches a tool lying about its own writes return
vacuously true — silently, on the calls that write the most. It now verifies
every entry of `applied`. Removing that branch turns the new oracle test red.
- Overlapping zooms in one batch stay overlapping, because two `addZoom` calls
already do: no `add*` path clamps against neighbours, in the agent or the UI.
Deconflicting inside the batch would make the result depend on how the model
chose to group its calls. Pinned by a test so it stays a decision.
`MUTATING_TOOL_NAMES` carries both names, which is what puts them behind the
"project edits disabled" wall — the existing loop over that table ("no write
escapes by being added later") covers them without a new test.
`addTrims`/`addZooms` promise the model that each entry stands or falls by
itself, and `applyBatch` is written for exactly that. The batch schemas were
not: `z.array(addTrimArgs)` rejected the whole call the moment one entry was
malformed, so nine good cuts died with the tenth and `refused[index]` could
never name the culprit. Worse, LangChain parses the tool schema BEFORE calling
us, so on the product path the call never reached `applyBatch` at all — it
threw "Received tool input did not match expected schema".
The element schema is now advertised rather than enforced, as a union with
`unknown`. The model still reads the full shape in the tool's JSON schema
(`anyOf: [addTrim, {}]`), a bad entry reaches the unitary executor, and that
executor refuses it with the wording it always uses. The container is still
validated: a missing, empty or non-array `ranges` is refused as a whole.
Also corrects two stale tool counts left at 20/19 by the previous commit.
Both CodeRabbit findings on getopenscreen#258.
…to the void `findMediaLinksByFingerprint` is a read that writes: when the file has moved, it refreshes `lastKnownPath` so the next lookup is cheaper. Not awaiting that write is right — a lookup should not pay for it — but `void promise` is not fire-and-forget, it is fire-and-crash. Nothing was watching, so any failure surfaced as a process-level `unhandledRejection`. That is what made the CI Test job intermittent: vitest reports an unhandled rejection from OUTSIDE every test and fails the run, so the job went red under a green summary — 1628 passing tests and a stack pointing at a temp directory a finished suite had already removed (run 30935779600). In the packaged app the same rejection lands in the main process, where the recovery is worse than a missed refresh ever was. The refresh stays unawaited and now logs what it could not do, which is the same answer `readRegistry` already gives a registry it cannot read. While in there, `withWriteLock` drops its queue entry once the chain drains, guarded by an identity check so a writer that queued behind us is not stranded. The map is keyed by an arbitrary directory path and grew for the life of the process. This is the pattern `DocumentService.writeProject` already uses, so the two write queues now read the same. Two cases cover it. The one that fails on the old code makes the directory read-only, so the write fails deterministically while the read still works, and asserts that nothing escapes and the lookup still answers. The second reproduces the CI shape — remove the directory while the refresh is queued — and asserts only that no rejection escapes, since either side of that race is acceptable.
…e temp dir "logs a refresh it cannot write" arranged its failure with `chmod(tempDir, 0o555)`. That is not a portable way to make a write fail: on Windows the bit lands on a directory attribute that does not stop a file being created inside, so the refresh succeeded, nothing warned, and the case failed on every Windows dev machine. `skipIf(process.getuid?.() === 0)` could not catch it either — `getuid` is undefined there, so the guard read as "not root" and ran anyway. CI only runs the suite on ubuntu-latest, so this stayed green there while being permanently red locally — which is the worst shape a test can have: it teaches you to skim past a red suite on the machine the app is actually developed on. The write is now failed by injection (`vi.spyOn(fs, "writeFile")`), which asserts the same thing on every platform and at any privilege level, and the spy is asserted to have been called so the case cannot pass with the refresh path deleted. Verified by ablation: dropping the `.catch()` in findMediaLinksByFingerprint still turns it red, with the rejection escaping as the unhandled one it exists to prevent.
The suite built a full jsdom for every one of its 140 test files. Only 37 of them ever touch a DOM, so the other 103 paid for one and threw it away: 719s of cumulative environment setup against 89s of actual test time. Flipping the default to `node` and letting the 37 opt back in with a `@vitest-environment` docblock takes the full run from 175s to 81s, back to back on the same machine, with no change to what any test asserts. The 37 were derived by running the suite under `--environment=node` and taking the files that failed, not by guessing from imports — 12 of them are `.ts` files (zustand stores, hooks, `platformUtils`) that a `.tsx`-only heuristic would have missed. `electron/media/audioPeaks.test.ts` already carried the same docblock the other way round to escape the global jsdom; that one is now redundant, and left alone. testTimeout goes to 15s in the same change because the two interact: with the machine loaded, 11 tests fail and 9 of those are purely "Test timed out in 5000ms" — ordinary component tests that pass in 200ms idle. A single jsdom file needs ~9.5s just to boot React, so a 5s budget was never a signal about the test. The one deliberately slow test (20 interleaved real disk writes) keeps its own longer override. The config comment also records the four things that look like speedups and are not — `--no-isolate`, `deps.optimizer.web`, `--pool=threads`, `--maxWorkers` — with what each measured, so the next person does not re-run that benchmark. `--no-isolate` is the tempting one: ~20% faster, but it shares one module registry per worker, which breaks `vi.mock`, and 29 test files depend on it.
Every agent-facing surface in the repo defined "done" as `npm run test` — the full suite. AGENTS.md, both harness reins and the git-workflow doc all said it, so an agent ran ~1670 tests after each edit and turned short tasks into long ones. They now say: targeted run while working (`npx vitest --run <path>`, or the new `npm run test:changed`), tsc and biome as the inner loop since they are seconds, and one full run at the end or left to CI. Also removes a tier of tests that does not exist. `npm run test:browser` and `vitest.browser.config.ts` were documented across four files and 80 lines of writing-tests.md, with a worked example and a timeouts section — but there is no script, no config, no `*.browser.test.ts` file and no CI job for any of it. An agent reading that runs a command that cannot work. What actually covers real codecs and GPU is the Rust suites under `crates/` and the manual checklist, so the table now points there. The remaining testing docs pick up the two rules this branch had to learn the hard way: the environment is node unless a file opts into jsdom, and anything gated on `process.platform` has to pin it, because CI is Linux-only and an unpinned Linux-only path is green there and red on every other machine.
`clearTimeout` sat on the line after the `await`, so only the success path ever reached it. When `fetch` rejects — a real network error, and the case the test file already drives — control jumped straight to `catch` and the 5s abort timer stayed armed, firing `controller.abort()` long after the function returned. `callDiscord` in the sibling `discord-bot-api.mjs` already had this right with a `try`/`finally`; this is the same shape. The new test pins it with fake timers on both the rejecting and the non-ok path: reverting the `finally` makes it fail with `expected 1 to be +0`.
Audit of every `void <call>` in src/ and electron/ after 2c4c426 fixed one of them, since `void` marks a promise as intentionally detached but does nothing about its rejection. 122 sites; most are fine and are left alone. The three in electron/ are the ones that can kill the app, because `installMainProcessErrorGuards` re-throws every unhandled rejection whose code is not EPIPE/ECONNRESET/ERR_STREAM_DESTROYED. One was the registry write fixed in 2c4c426; the other two are already correct — `document-service` voids a promise that is already `.catch()`ed, and `cliMain`'s chain ends in a `.catch` that exits non-zero. Neither gets ceremony added. In the renderer a rejection is console noise rather than a crash, so the fixes here are the sites where a rejection is not hypothetical: * `play()` and `requestFullscreen()` reject routinely (autoplay policy, a load interrupting a pending play). VirtualPreview and WebcamOverlay already caught theirs; Modals and NewEditorShell did not. Play state is driven by the element's own play/pause events in both, so a rejection leaves nothing to reconcile — same commented swallow as the existing two. * Bare `ipcRenderer.invoke` calls reject when the main handler throws: revealInFolder, startNewRecording, and the recording-prefs and selected-source reads. These log, because a failed IPC means something is genuinely broken. * The background duration probe in `useTimeline` ends in `saveDocument`, which throws on a failed write — the same shape as the registry bug. Not fixed here, deliberately: 12 sites where a user-initiated timeline mutation (removeRegion, removeClip, duplicateClip, insertClipAt, applyTimelineOp) can fail silently, because neither `useTimeline` nor `useSequentialTimelineOps` catches anything. Twelve scattered `console.warn`s would bury a real user-facing failure; they all route through two functions, so the fix belongs there with a toast — which is a UX change, not an audit cleanup. `handleSave` already does exactly that, so the pattern to copy is next door.
Every turn began by measuring the history against `DEFAULT_BUDGET_TOKENS = 80_000` and, past 70% of it, blocking on a whole extra summarizer call before the user's request was even sent — a call that also pays the reasoning budget, so on a thinking model it is not a cheap one. That 80k was invented. The app cannot ask a provider how big its context window is, so the number could not be right for anything: it discards context at 5% fill on a 1M-token Gemini, and would be far too generous elsewhere. It is the same mistake `getTranscript` made with its 800 segments, and it gets the same answer — a guessed limit is deleted, not retuned. Until the app can learn a real window, the only honest trigger is a person deciding they want one, which is the button that already exists. The same guess also gated the manual path: `compactSessionNow` went through the identical heuristic, so on any ordinary conversation pressing Compact did nothing at all, silently. Pressing it is now the decision. The one refusal left is "fewer than 4 messages", which is not a guess about anyone's context window — folding a single exchange into a summary cannot make it shorter — plus the existing measured guard that refuses a summary no smaller than what it replaces. `compactionBlocked` goes with it: it existed only to stop the automatic path re-buying the same useless summary, and nothing retries on its own any more. `DEFAULT_BUDGET_TOKENS` survives as the context pill's denominator and nothing else; both it and its renderer twin now say in writing that they must never regain a decision. Verified by ablation: re-adding the automatic trigger turns four of the six compaction tests red, including the one that pins that a turn never summarizes anything by itself.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 task
Collaborator
|
Rebased your commit onto Main change: every DXGI setup failure now falls back to the CPU path instead of returning false. As written, the hard error sat between the default sink-writer attempt and the software H.264 retry, and since Also shortened the bridge Two things measurement turned up on a working machine, both worth knowing for your prototype:
We have no hardware that reproduces this, so #305 only proves the GPU path is correct and the fallbacks work. Could you run it on the machine that fails? Closing this in favour of #305, which carries your commit. Thanks for the prototype and the trace, both did the hard part. |
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
2 participants
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
IMFVideoSampleAllocatorExpreferSoftwareEncoderRoot cause
On the Windows 10 / WDDM 2.7 machine from #252, the existing staging-texture readback reaches
ID3D11DeviceContext::Unmapand never returns. Since that work runs under the shared frame lock on the same device/context as WGC, the WGC drain and video-writer join also stall.The DXGI path copies the live WGC frame into an owned texture while the capture callback is active, transfers it to a second D3D11 device through a keyed-mutex bridge, performs GPU BGRA-to-NV12 conversion, and submits allocator-owned DXGI samples to the hardware H.264 sink writer. It does not call
MaporUnmap.The existing CPU path remains active when software encoding is requested or when webcam PiP must be composited into the screen buffer.
Validation
Tested on the machine that reproduces #252:
ffprobeThe native helper builds successfully with MSVC 2022 and Windows SDK 26100. The software-encoder/readback path is preserved and compiles, but cannot complete on this machine because its original
Unmaphang is the hardware-specific failure being addressed.Closes #252.