fix(export): project native cursor onto cropped rect, not mask rect#78
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates native cursor coordinate normalization in the recording bridge and renderer, adds ChangesNative Cursor Crop Projection Fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant updateLayout
participant LayoutCache
participant drawNativeCursor
participant projectNativeCursorToLocal
updateLayout->>LayoutCache: store croppedRect
drawNativeCursor->>LayoutCache: read croppedRect
drawNativeCursor->>projectNativeCursorToLocal: project sample with croppedRect
projectNativeCursorToLocal-->>drawNativeCursor: return local cursor point or null
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/start-ct2-server.cmd (1)
1-6: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHardcoded personal paths; same issue as
ct2-build.cmd/configure-ct2-build.ps1.All three paths (model dir, server exe, log file) are pinned to one developer's worktree and will not resolve elsewhere.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/start-ct2-server.cmd` around lines 1 - 6, The start-ct2-server.cmd script is using hardcoded developer-specific paths for the model directory, server executable, and log file. Update the batch file to resolve these locations dynamically or via configurable variables, following the same approach used in ct2-build.cmd and configure-ct2-build.ps1, and make sure the script references OPENSCREEN_CT2_MODEL_DIR and the ctranslate2-server path without embedding a single worktree path.scripts/stt-wrapper.bat (1)
1-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConvert to CRLF line endings.
Static analysis flags LF-only line endings, which can cause GOTO/label parsing failures in some Windows batch parsers. Low-cost fix given the file is small.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/stt-wrapper.bat` around lines 1 - 22, The batch wrapper script uses LF-only line endings, which can break label and GOTO parsing in Windows batch execution. Convert the entire stt-wrapper.bat file to CRLF while keeping the existing logic in the parse and run sections unchanged, so the wrapper remains compatible with Windows batch parsers.Source: Linters/SAST tools
🟠 Major comments (24)
docs/architecture/ai-edition-roadmap.md-153-155 (1)
153-155: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSwap
keytarfor the lockedsafeStoragechoice.P2.2 still instructs storing secrets in
keytar, but the locked decisions explicitly chose ElectronsafeStorageinstead. Leaving both in the roadmap will send implementation toward the wrong secret store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/ai-edition-roadmap.md` around lines 153 - 155, Update the P2.2 OAuth device-flow / PAT auth item to match the locked secret-storage decision by replacing the `keytar` reference with Electron `safeStorage`. Keep the rest of the flow anchored around `llm-call.ts` and `ProviderSettings.tsx`, but revise the storage step so the device-flow access token is saved and retrieved using `safeStorage` rather than `keytar`, and make sure the roadmap text consistently points implementers to that choice.docs/architecture/ai-edition-roadmap.md-36-60 (1)
36-60: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winResolve the persisted file-extension mismatch.
docs/architecture/ai-edition-roadmap.md:36-40,57definesAxcutDocumentv3 as the canonical model and says new projects are written touserData/projects/<id>.axcut, but locked decision 7 says to keep.openscreen. Clarify which extension is the on-disk contract and keep the migration note separate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/ai-edition-roadmap.md` around lines 36 - 60, The roadmap currently contradicts itself about the persisted project extension: the AxcutDocument persistence note says projects are saved as .axcut, while the locked decisions for the ai-edition contract say to keep .openscreen. Update the canonical storage description near AxcutDocument and the project persistence wording to use the same on-disk extension as the locked decision, and keep the v2-to-v3 migration note separate so the document model and file-format contract are not mixed.docs/architecture/ai-edition-collision-analysis.md-11-18 (1)
11-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate schema documentation to reflect current v4 baseline.
The schema has already advanced to v4 (see
src/lib/ai-edition/schema/index.ts:15), and v3→v4 upgrades are handled transparently insidedocumentSchema(lines 464–480). The doc's v2/v3 andSCHEMA_VERSION = 3baseline is stale. Refresh lines 14–18 to accurately reflect that the current contract is v4, clarify that v3 documents auto-upgrade, and align the migration reference with the actualmigrateProjectDataToAxcutDocumentand schema-upgrade behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/ai-edition-collision-analysis.md` around lines 11 - 18, The architecture doc is stale: it still describes a v2/v3 baseline and a shared SCHEMA_VERSION = 3, but the actual contract is now v4. Update the “Both projects have version” section to reflect the current v4 schema in src/lib/ai-edition/schema/index.ts, note that v3 documents are transparently upgraded by documentSchema, and align the migration description with migrateProjectDataToAxcutDocument and the schema-upgrade flow rather than the old v2/v3 wording.electron/media/mediaLinksRegistry.ts-244-253 (1)
244-253: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFire-and-forget backfill has no rejection handler.
void updateRegistry(...)discards the promise without a.catch(). If the write fails (disk full, permission error, etc.) this becomes an unhandled promise rejection in the Electron main process, unlike the equivalent backfill call inhandlers.ts(resolveMediaLinksForVideo) which does attach.catch(...). Add the same handling here for consistency and to avoid crashing/warning the main process on a best-effort operation.🔧 Proposed fix
if (match.lastKnownPath !== videoPath) { - void updateRegistry(baseDir, (file) => ({ + updateRegistry(baseDir, (file) => ({ version: 1, entries: file.entries.map((e) => fingerprintsMatch(e.fingerprint, fingerprint) ? { ...e, lastKnownPath: videoPath } : e, ), - })); + })).catch((error) => console.warn("[media-links] lastKnownPath refresh failed:", error)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/media/mediaLinksRegistry.ts` around lines 244 - 253, The fire-and-forget registry backfill in mediaLinksRegistry’s path-refresh block drops the promise from updateRegistry without handling failures. Update that updateRegistry call to attach a .catch() rejection handler, matching the pattern used by resolveMediaLinksForVideo in handlers.ts, so best-effort writes don’t trigger unhandled rejections in the Electron main process.electron/media/mediaLinksRegistry.ts-32-36 (1)
32-36: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRegistry fingerprints should be hashed, not stored raw
electron/media/mediaLinksRegistry.ts:32-36,71-94,148-154
MediaFingerprintstores two 64KB samples as base64, so each entry adds roughly 175KB before the other fields. With no pruning here, the registry grows without bound, and every lookup parses — and every update reserializes — the whole file. Store digests for the samples instead of the raw base64 bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/media/mediaLinksRegistry.ts` around lines 32 - 36, The MediaFingerprint data in mediaLinksRegistry.ts is storing large raw base64 samples, which makes the registry file grow and forces expensive full-file parse/serialize on every lookup and update. Update MediaFingerprint and the registry read/write path in the MediaLinksRegistry methods that manage fingerprints to store hashed digests for the head/tail samples instead of the raw base64 content. Make sure the lookup/comparison logic still uses the same identifying fields but compares hashes, and keep the existing sizeBytes-based matching behavior intact.scripts/stt-dev-server.mjs-18-23 (1)
18-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefault port fallback never triggers — silently produces
NaN.
process.argv[process.argv.indexOf("--port") + 1]evaluates toprocess.argv[0](the node executable path) when--portis absent, sinceindexOfreturns-1and-1 + 1 = 0. That value is always a truthy string, so the?? "20199"fallback never runs, andparseInt(nodePath, 10)yieldsNaN, breakingserver.listen(NaN, ...)for anyone running the script without--port.🐛 Proposed fix
-const PORT = parseInt( - process.argv.find((a) => a.startsWith("--port="))?.split("=")[1] ?? - process.argv[process.argv.indexOf("--port") + 1] ?? - "20199", - 10, -); +const portFlagIndex = process.argv.indexOf("--port"); +const portArg = + process.argv.find((a) => a.startsWith("--port="))?.split("=")[1] ?? + (portFlagIndex !== -1 ? process.argv[portFlagIndex + 1] : undefined) ?? + "20199"; +const PORT = parseInt(portArg, 10);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/stt-dev-server.mjs` around lines 18 - 23, The PORT parsing logic in stt-dev-server.mjs is incorrectly falling through to process.argv[0] when "--port" is missing, so the default never applies and parseInt can return NaN. Update the argument lookup around the PORT constant to safely detect whether "--port" exists before reading the next element, and only then fall back to "20199" when neither "--port=" nor a valid "--port" value is present. Keep the fix localized to the PORT initialization so server.listen receives a real numeric port.scripts/e2e-stt-smoke.mjs-27-36 (1)
27-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBinary path hardcodes
win32-x64regardless ofprocess.platform.The extension is chosen based on
process.platform === "win32", implying intent to run cross-platform, but the directory component is hardcoded to"win32-x64". On macOS/Linux this will look for.../win32-x64/ctranslate2-server-ctranslate2-cpu, which won't exist (per the build script's<os>-<arch>layout), causingspawnto fail withENOENT.🐛 Proposed fix
+const PLATFORM_DIR = + process.platform === "win32" + ? "win32-x64" + : process.platform === "darwin" + ? `darwin-${process.arch}` + : `${process.platform}-${process.arch}`; const CT2_BIN = join( ROOT, "electron", "native", "bin", - "win32-x64", + PLATFORM_DIR, process.platform === "win32" ? "ctranslate2-server-ctranslate2-cpu.exe" : "ctranslate2-server-ctranslate2-cpu", );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e-stt-smoke.mjs` around lines 27 - 36, The CT2_BIN path is hardcoded to the win32-x64 directory, which breaks cross-platform smoke runs. Update the CT2_BIN construction in the e2e-stt-smoke script to derive the platform/arch directory from process.platform and process.arch, matching the build layout used by the native binaries, while keeping the existing executable name selection logic. This will ensure the spawn target resolves correctly on macOS and Linux instead of pointing at a non-existent win32-x64 path.scripts/e2e-pipeline-smoke.mjs-1-31 (1)
1-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSwitch this smoke test to
ctranslate2-server
This still shells out toelectron/native/bin/win32-x64/whisper-server-whisper-cpu.exeand aggml-small-q5_1.binmodel, but the build workflow now stages onlyctranslate2-server-*binaries. As written, this will fail on fresh builds; if it’s meant to cover the same path asscripts/e2e-stt-smoke.mjs, fold it into that flow instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e-pipeline-smoke.mjs` around lines 1 - 31, The smoke test still targets the old whisper-server binary path, but the build now stages only ctranslate2-server artifacts, so this script will fail on fresh builds. Update the e2e-pipeline-smoke.mjs flow to use the same ctranslate2-server setup as scripts/e2e-stt-smoke.mjs, including swapping the WHISPER_BIN/old model assumptions for the staged ctranslate2-server binary and matching invocation path. Keep the test aligned with the IPC-covered pipeline modules so it exercises the same runtime path without depending on electron/native/bin/win32-x64/whisper-server-whisper-cpu.exe.scripts/configure-ct2-build.ps1-1-11 (1)
1-11: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winScript only works on one developer's machine.
$rootand theCMAKE_PREFIX_PATHvalue are hardcoded toG:\repos\openscreen\.claude\worktrees\stt-migration. This will fail for anyone else, or on CI, since it doesn't derive the repo root dynamically (e.g., via$PSScriptRoot).♻️ Proposed fix
-$root = 'G:\repos\openscreen\.claude\worktrees\stt-migration' +$root = Resolve-Path (Join-Path $PSScriptRoot '..')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/configure-ct2-build.ps1` around lines 1 - 11, The configure-ct2-build.ps1 script is hardcoded to one local checkout, so it should derive paths dynamically instead of using a fixed G:\repos\openscreen\.claude\worktrees\stt-migration value. Update the $root and related path construction to use the script location (for example via $PSScriptRoot) and build $buildDir, $srcDir, and the CMAKE_PREFIX_PATH from that computed repo root so the script works on any machine and in CI.electron/preload.ts-180-182 (1)
180-182: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winType
findRecordingCamera's return instead of leakingPromise<any>.
ipcRenderer.invoketypes asPromise<any>by default. Every other new bridge method in this diff (stt.transcribe) explicitly types its invoke result; this one doesn't, introducing an implicitanyinto the exposedelectronAPIsurface. As per coding guidelines, "new code should not introduceanytypes."♻️ Proposed typed return
+type FindRecordingCameraResult = { + success: boolean; + webcamVideoPath?: string; + offsetMs?: number; + error?: string; +}; + findRecordingCamera: (videoPath: string) => { - return ipcRenderer.invoke("find-recording-camera", videoPath); + return ipcRenderer.invoke("find-recording-camera", videoPath) as Promise<FindRecordingCameraResult>; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/preload.ts` around lines 180 - 182, The electronAPI bridge method findRecordingCamera is leaking an implicit Promise<any> from ipcRenderer.invoke. Update the findRecordingCamera function in preload.ts to explicitly type the invoke result, following the pattern used by stt.transcribe, and make sure the exposed return type is a concrete Promise<T> instead of any so the electronAPI surface stays fully typed.Source: Coding guidelines
electron/native/ctranslate2-server/src/mel.cpp-83-96 (1)
83-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOut-of-bounds read in
reflect_padfor short inputs.The front-padding loop reads
x[i]up tox[pad]without checkingx.size() > pad, and the tail loop clampsidxtox.size()(one past the last valid index) instead ofx.size()-1. Ifmono_16k.size() <= pad(audio shorter thann_fft/2samples), this is a heap buffer over-read/UB that can crash the native server on a short or near-empty WAV input.🛡️ Proposed bounds-safe fix
std::vector<float> reflect_pad(const std::vector<float>& x, int pad) { - if (pad <= 0) return x; + if (pad <= 0 || x.empty()) return x; std::vector<float> out; out.reserve(x.size() + size_t(2 * pad)); for (int i = pad; i > 0; --i) { - out.push_back(x[size_t(i)]); + size_t idx = (size_t(i) < x.size()) ? size_t(i) : (x.size() - 1); + out.push_back(x[idx]); } out.insert(out.end(), x.begin(), x.end()); for (size_t i = 1; i <= size_t(pad); ++i) { - size_t idx = (i > x.size()) ? x.size() : (x.size() - i); + size_t idx = (i >= x.size()) ? (x.size() - 1) : (x.size() - i); out.push_back(x[idx]); } return out; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/ctranslate2-server/src/mel.cpp` around lines 83 - 96, The reflect_pad helper is reading past the end of the input for short vectors, so update the padding logic in reflect_pad to handle inputs smaller than pad safely. Clamp the front-padding and tail-padding indices to valid positions before indexing x, and make sure the right-side reflection never uses x.size() as an element access target; use the last valid sample instead. Verify the caller path in mel.cpp that feeds mono_16k into reflect_pad still works when the audio is shorter than the padding size.electron/main-process-errors.ts-6-6 (1)
6-6: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLimit the swallow to the IPC reply case
electron/main-process-errors.ts:6-32swallowsECONNRESETandERR_STREAM_DESTROYEDprocess-wide. Those codes can also surface from the STT HTTP/streaming paths, so this can hide real download/transcription failures. Narrow the guard to the specific IPC case, or drop the extra codes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main-process-errors.ts` at line 6, The global swallow list in main-process error handling is too broad and can hide real STT download/transcription failures. Update the logic around SWALLOWED_ERROR_CODES and the main-process error guard in electron/main-process-errors.ts so that ECONNRESET and ERR_STREAM_DESTROYED are only ignored for the IPC reply path, or remove those extra codes if that scope can’t be distinguished. Keep the suppression narrowly tied to the IPC reply case and leave other errors to surface normally.electron-builder.json5-34-42 (1)
34-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the Linux
extraResourcesmappingelectron-builder.json5:82-92already packageselectron/native/bin/<platform>-<arch>/for macOS and Windows, but Linux has no matchingelectron/native/bin/linux-*/*entry. Packaged Linux builds will missctranslate2-server, so STT won’t work there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron-builder.json5` around lines 34 - 42, The Linux packaging config is missing the `extraResources` mapping for `electron/native/bin/linux-<arch>/`, so packaged Linux builds do not include `ctranslate2-server`. Update the Linux section in `electron-builder.json5` to mirror the macOS and Windows resource mappings and ensure the existing native binary path pattern is included for the matching Linux architecture.electron/native/ctranslate2-server/src/main.cpp-503-503 (1)
503-503: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSigned integer overflow in payload limit.
2 * 1024 * 1024 * 1024is evaluated inint; the result (2147483648) exceedsINT_MAX, which is undefined behavior. The wrapped value then converts tosize_t, so the effective limit is unpredictable rather than 2 GiB.🐛 Proposed fix
- svr.set_payload_max_length(2 * 1024 * 1024 * 1024); + svr.set_payload_max_length(static_cast<size_t>(2) * 1024 * 1024 * 1024);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/ctranslate2-server/src/main.cpp` at line 503, The payload limit in set_payload_max_length is computed with an int expression that overflows before conversion, so update the CTranslate2 server setup to use an explicitly unsigned/64-bit constant for the 2 GiB limit. Locate the payload limit assignment in main.cpp near svr.set_payload_max_length and change the literal arithmetic so the value is evaluated safely and passed as the intended size_t amount.Source: Linters/SAST tools
electron/native/ctranslate2-server/src/main.cpp-484-497 (1)
484-497: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
load_tokenizercan throw uncaught, terminating the process.
load_tokenizer(Line 485) throwsstd::runtime_errorwhentokenizer.jsonis missing or malformed, but the call is outside anytryblock. The uncaught exception triggersstd::terminaterather than the cleanreturn 3path used for model-load failure. Wrap it like the model load above.🛡️ Proposed fix
- // Load the tokenizer - openscreen::ct2::WhisperTokenizer tok = load_tokenizer(cfg.model_dir); - log("tokenizer sanity: id(<|en|>)=" + std::to_string(tok.id_for("<|en|>"))); + // Load the tokenizer + std::optional<openscreen::ct2::WhisperTokenizer> tok_opt; + try { + tok_opt = load_tokenizer(cfg.model_dir); + } catch (const std::exception& e) { + std::cerr << "FATAL: tokenizer load failed: " << e.what() << std::endl; + return 3; + } + openscreen::ct2::WhisperTokenizer tok = std::move(*tok_opt); + log("tokenizer sanity: id(<|en|>)=" + std::to_string(tok.id_for("<|en|>")));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/ctranslate2-server/src/main.cpp` around lines 484 - 497, The tokenizer load path in main currently calls load_tokenizer(cfg.model_dir) without any exception handling, so a missing or malformed tokenizer.json can terminate the process instead of using the existing fatal exit flow. Wrap the tokenizer initialization and the subsequent tok.id_for sanity check in the same kind of try/catch used for the model load, and on failure log a fatal message with the exception details and return 3 so main handles tokenizer errors consistently.Source: Linters/SAST tools
electron/stt/gpuDetector.ts-92-101 (1)
92-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid
requirein this ESM module.package.jsonsets"type": "module", sorequire("node:fs")can throw here; the catch turns that intofalse, which makes CUDA detection silently fall back to CPU even when the binary exists. Use a top-levelimport { existsSync } from "node:fs"here too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/stt/gpuDetector.ts` around lines 92 - 101, The binaryAvailable helper in gpuDetector is using require("node:fs") inside an ESM module, which can fail and incorrectly mark backend binaries as unavailable. Update binaryAvailable to use the existing ESM import style by bringing in existsSync at the top of the module and calling it directly inside the candidateBinaryPaths check, so CUDA detection works reliably instead of silently falling back to CPU.electron/stt/index.test.ts-8-30 (1)
8-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove
fakeCt2Serverintovi.hoisted()
vi.mock()is hoisted ahead of top-levelconstinitialization, so this factory can hit the TDZ and fail on import. Hoist the shared fake instead.Suggested fix
-const fakeCt2Server = { +const fakeCt2Server = vi.hoisted(() => ({ start: vi.fn(), status: { backend: "ctranslate2-cpu" as const, port: 9000, @@ }, transcribe: vi.fn(), stop: vi.fn(), -}; +}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/stt/index.test.ts` around lines 8 - 30, The shared fake used by the vi.mock factory is initialized too late, so the hoisted mock can access it during import and hit the TDZ. Move fakeCt2Server into a vi.hoisted() declaration in electron/stt/index.test.ts, then keep the CTranslate2ServerManager mock class wiring to that hoisted object’s start/status/transcribe/stop members so the factory can safely reference it.electron/ai-edition/document-service.ts-144-163 (1)
144-163: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNon-atomic project writes risk corrupting
.axcutfiles on crash/interruption.
writeProjectcallsfs.writeFiledirectly against the final path. If the process crashes or is killed mid-write (e.g. during a large document save), the.axcutfile is left truncated/corrupted, andgetProject/listProjectswould then either throw or silently skip the project (Line 107-111). There's also no per-project write serialization, so two concurrentsaveProjectcalls for the same project could interleave. The media-links registry elsewhere in this PR stack uses atomic persistence with write serialization for exactly this reason — consider applying the same pattern here (write to a temp file, thenfs.renameinto place, optionally behind a per-project write queue/lock).🛡️ Suggested fix (atomic write)
private async writeProject(doc: AxcutDocument): Promise<void> { await this.ensureProjectsDir(); const filePath = this.fileFor(doc.project.id); - await fs.writeFile(filePath, JSON.stringify(doc, null, 2), "utf8"); + const tmpPath = `${filePath}.tmp-${Date.now()}`; + await fs.writeFile(tmpPath, JSON.stringify(doc, null, 2), "utf8"); + await fs.rename(tmpPath, filePath); }Also applies to: 232-237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/document-service.ts` around lines 144 - 163, The project persistence path in writeProject is currently writing directly to the final .axcut file, which can leave corrupted files on interruption and allow concurrent saveProject calls to interleave. Update writeProject to persist via a temp file followed by an atomic rename, and add per-project write serialization/locking so only one save runs at a time for a given projectId. Keep the change localized around writeProject and the saveProject flow that calls it, and apply the same atomic pattern anywhere else the project file is written.electron/ai-edition/deep-agent/agent-provider-capabilities.ts-203-212 (1)
203-212: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
openai/prefix strip breaks OpenRouter reasoning detection for OpenAI-slug models.
isOpenRouterReasoningModelcallsisOpenAIReasoningModel(model)directly without stripping anopenai/prefix first.isOpenAIReasoningModel's regex (Line 187) is anchored with^, so a slug like"openai/gpt-5"never matches (^gpt-5fails since the string starts with"openai/").The sibling implementation in
electron/ai-edition/agent-provider-capabilities.ts(Line 261) explicitly doesisOpenAIReasoningModel(normalized.replace(/^openai\//, "")), and its test suite (agent-provider-capabilities.test.tsLine 47) assertsgetReasoningCapability("openrouter", "openai/gpt-5").supported === true. This deep-agent copy would silently disable reasoning wiring for the same model on the LangChain path.🐛 Suggested fix
function isOpenRouterReasoningModel(model: string): boolean { - if (isOpenAIReasoningModel(model)) return true; + if (isOpenAIReasoningModel(model.replace(/^openai\//, ""))) return true; if ( model.startsWith("anthropic/") && isAnthropicReasoningModel(model.slice("anthropic/".length)) ) { return true; } return /deepseek-r1/i.test(model) || /qwen.*thinking/i.test(model) || /grok-4/i.test(model); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/deep-agent/agent-provider-capabilities.ts` around lines 203 - 212, `isOpenRouterReasoningModel` is missing the `openai/` slug normalization before delegating to `isOpenAIReasoningModel`, so OpenRouter models like `openai/gpt-5` are not detected as reasoning models. Update `isOpenRouterReasoningModel` to strip the `openai/` prefix before calling `isOpenAIReasoningModel`, mirroring the normalization used in the sibling `agent-provider-capabilities` implementation. Keep the existing Anthropic and regex fallback checks intact, and ensure the function still returns true for OpenRouter reasoning slugs with vendor prefixes.electron/ai-edition/deep-agent/agent-provider-capabilities.ts-56-107 (1)
56-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle MiniMax in the deep-agent reasoning branch
provider === "anthropic"excludesminimaxandminimax-token-plan, so this falls through tosupported: falsefor MiniMax even though the registry marks both providers as reasoning-capable. ThestartsWith("MiniMax-M3")check is also dead code becausenormalizeModelName()lowercases the model first. Add an explicit MiniMax branch here, matching the direct MiniMax handling inelectron/ai-edition/agent-provider-capabilities.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/deep-agent/agent-provider-capabilities.ts` around lines 56 - 107, The reasoning capability check in getReasoningCapability currently misses MiniMax because it only routes through the anthropic provider branch and also uses a dead case-sensitive MiniMax-M3 prefix check after normalizeModelName() lowercases the model. Add an explicit MiniMax/minimax-token-plan branch in this function, matching the direct MiniMax handling used in agent-provider-capabilities.ts, so the MiniMax providers return supported reasoning instead of falling through to false.electron/ai-edition/agent-provider-capabilities.ts-118-129 (1)
118-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute MiniMax through its own reasoning payload (
electron/ai-edition/agent-provider-capabilities.ts:118-129,:150-157).The
minimaxbranch still reusesopenai-responses, which produces{ reasoning: { effort } }and setsuseResponsesApi; the Anthropic transport ignores that flag and just mergesextraBody, so MiniMax never gets the flatreasoning_effortfield this branch is supposed to send. Add a MiniMax-specific strategy/body shape instead of sharing the OpenAI Responses path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/agent-provider-capabilities.ts` around lines 118 - 129, The MiniMax handling in agent-provider-capabilities should not reuse the openai-responses strategy because that produces a reasoning object and useResponsesApi, while the Anthropic transport only forwards extraBody and never emits the flat reasoning_effort field. Update the minimax/minimax-token-plan branch and the related provider-body mapping logic so MiniMax uses its own strategy/body shape with reasoning_effort at the top level, using the existing provider capability symbols to keep the routing separate from the OpenAI Responses path.electron/ai-edition/chat-service.ts-301-301 (1)
301-301: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
allowAgentEditssetting is computed but never enforced.
editsAllowedis derived fromconfig.allowAgentEdits(Line 301) but the only use isvoid editsAllowed;insideagentSink.toolStart(Line 331), which does nothing at runtime. There is no code path that prevents the deep agent from calling mutating tools (addSkip,setSkipRange,setClipRange,replaceTimeline) when the user has disabled agent edits — the setting is effectively a no-op today.🐛 Suggested direction
Thread
editsAllowedthrough toinvokeOpenScreenAgent/buildToolsand omit the mutating tools from the tool list (or makeexecuteAgentToolreject mutations) wheneditsAllowedisfalse, rather than only silencing the unused-variable warning.Also applies to: 327-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/chat-service.ts` at line 301, The allowAgentEdits setting is computed in chat-service.ts but never actually enforced, so the deep agent can still invoke mutating tools even when edits are disabled. Thread the editsAllowed flag from the chat-service flow into invokeOpenScreenAgent and buildTools, and use it to omit the mutating tools (addSkip, setSkipRange, setClipRange, replaceTimeline) from the tool list when false, or have executeAgentTool reject those mutations at runtime. Remove the no-op void editsAllowed usage and make the enforcement happen at the tool construction/execution boundary.electron/ai-edition/deep-agent/service.ts-96-181 (1)
96-181: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicated if/else pattern across all four mutating tools.
addSkip,setSkipRange,setClipRange, andreplaceTimelineeach repeat the identicalif (execution.document) {...} else {...}structure that only differs by the tool name — thesink.toolEnd(...)call is literally the same in both branches. Worth extracting into a small helper to reduce duplication.♻️ Suggested helper
+function mutatingToolHandler(holder: DocumentHolder, sink: OpenScreenAgentSink, name: string) { + return async (args: unknown) => { + sink.toolStart(name, args); + const execution = executeAgentTool(holder.current, name, JSON.stringify(args)); + if (execution.document) holder.current = execution.document; + sink.toolEnd(name, execution.ok, execution.summary); + return execution.resultJson; + }; +}Then each tool becomes e.g.
tool(mutatingToolHandler(holder, sink, "addSkip"), { name: "addSkip", description: ..., schema: addSkipArgs }).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/deep-agent/service.ts` around lines 96 - 181, The mutating tool handlers for addSkip, setSkipRange, setClipRange, and replaceTimeline repeat the same execution/document update logic with an unnecessary if/else around sink.toolEnd. Extract the shared behavior into a small helper like mutatingToolHandler that takes holder, sink, and the tool name, calls executeAgentTool, updates holder.current when execution.document exists, and always emits the same sink.toolEnd result, then wire each tool definition to use that helper.electron/ai-edition/deep-agent/service.ts-92-183 (1)
92-183: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip the generic tool lifecycle emit for the mutating tools.
addSkip,setSkipRange,setClipRange, andreplaceTimelinealready callsink.toolStart/sink.toolEndinside their handlers, so theon_tool_start/on_tool_endstream handler emits a second start/end pair for the same call. That produces duplicate lifecycle events in the UI, and the secondtoolEnddrops the summary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ai-edition/deep-agent/service.ts` around lines 92 - 183, The mutating tool handlers in buildTools already emit their own lifecycle events, so the generic stream handler is causing duplicate start/end notifications for addSkip, setSkipRange, setClipRange, and replaceTimeline. Update the tool wiring so these tools bypass the shared on_tool_start/on_tool_end emission path, or otherwise ensure sink.toolStart and sink.toolEnd are only called once per invocation; keep the existing per-tool calls in buildTools as the source of truth so the summary is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22a71f57-555c-446b-bfd2-757fd12c3af8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (253)
.github/workflows/build-ctranslate2-server.yml.github/workflows/build.yml.gitignore.opencode/prompts/playwright-test-generator.md.opencode/prompts/playwright-test-healer.md.opencode/prompts/playwright-test-planner.mdbiome.jsonct2-build.cmddesign/DESIGN.mddesign/openscreen-editor-v2.htmldesign/openscreen-editor.htmldesign/openscreen-widget.htmldocs/architecture/ai-edition-collision-analysis.mddocs/architecture/ai-edition-data-flow.mddocs/architecture/ai-edition-roadmap.mddocs/architecture/axcut-inventory.mddocs/architecture/openscreen-inventory.mddocs/axcut-ux-ui-spec.mddocs/cursor-feature-inventory.mddocs/engineering/stt-ctranslate2-implementation-status.mddocs/engineering/stt-ctranslate2-migration.mddocs/engineering/transcription-engine-migration.mddocs/openscreen-ux-ui-spec.mddocs/provider-parity-plan.mdelectron-builder.json5electron/ai-edition/agent-provider-capabilities.test.tselectron/ai-edition/agent-provider-capabilities.tselectron/ai-edition/agent-tools.test.tselectron/ai-edition/agent-tools.tselectron/ai-edition/chat-compaction.test.tselectron/ai-edition/chat-compaction.tselectron/ai-edition/chat-service.test.tselectron/ai-edition/chat-service.toolloop.test.tselectron/ai-edition/chat-service.tselectron/ai-edition/codex-session.test.tselectron/ai-edition/codex-session.tselectron/ai-edition/deep-agent/agent-provider-capabilities.tselectron/ai-edition/deep-agent/chat-model.tselectron/ai-edition/deep-agent/service.tselectron/ai-edition/document-service.test.tselectron/ai-edition/document-service.tselectron/ai-edition/llm-call.tselectron/ai-edition/llm-config-store.tselectron/ai-edition/llm-provider-auth.test.tselectron/ai-edition/llm-provider-auth.tselectron/ai-edition/provider-registry.tselectron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/nativeBridge.tselectron/main-process-errors.test.tselectron/main-process-errors.tselectron/main.tselectron/media/mediaLinksRegistry.test.tselectron/media/mediaLinksRegistry.tselectron/native-bridge/services/aiEditionService.tselectron/native/ctranslate2-server/CMakeLists.txtelectron/native/ctranslate2-server/README.mdelectron/native/ctranslate2-server/include/mel.helectron/native/ctranslate2-server/include/tokenizer.helectron/native/ctranslate2-server/include/wav.helectron/native/ctranslate2-server/src/main.cppelectron/native/ctranslate2-server/src/mel.cppelectron/native/ctranslate2-server/third_party/kissfft/LICENSEelectron/native/ctranslate2-server/third_party/kissfft/_kiss_fft_guts.helectron/native/ctranslate2-server/third_party/kissfft/kiss_fft.celectron/native/ctranslate2-server/third_party/kissfft/kiss_fft.helectron/native/ctranslate2-server/third_party/kissfft/kiss_fft_log.helectron/preload.tselectron/stt/ctranslate2Server.test.tselectron/stt/ctranslate2Server.tselectron/stt/gpuDetector.test.tselectron/stt/gpuDetector.tselectron/stt/index.test.tselectron/stt/index.tselectron/stt/modelManager.test.tselectron/stt/modelManager.tselectron/stt/transcriptionContract.tselectron/stt/wav.tspackage.jsonscripts/before-pack.cjsscripts/build-ctranslate2-server.shscripts/configure-ct2-build.ps1scripts/e2e-pipeline-smoke.mjsscripts/e2e-stt-smoke.mjsscripts/fetch-caption-model.mjsscripts/start-ct2-server.cmdscripts/stt-dev-server.mjsscripts/stt-wrapper.batspecs/README.mdspecs/computer-use/00-launch-and-hud.mdspecs/computer-use/01-source-selection-and-record.mdspecs/computer-use/02-editor-foundation.mdspecs/computer-use/03-transport-and-preview.mdspecs/computer-use/04-timeline-pan-zoom-scrub.mdspecs/computer-use/05-clip-operations.mdspecs/computer-use/06-skip-regions.mdspecs/computer-use/07-zoom-regions.mdspecs/computer-use/08-speed-regions.mdspecs/computer-use/09-annotation-regions.mdspecs/computer-use/10-properties-right-panel.mdspecs/computer-use/11-transcript-editor.mdspecs/computer-use/12-chat-panel.mdspecs/computer-use/13-provider-settings.mdspecs/computer-use/14-sessions-and-history.mdspecs/computer-use/15-export-dialog.mdspecs/computer-use/16-modal-shortcuts-i18n.mdspecs/computer-use/17-themes-and-settings.mdspecs/computer-use/18-final-qa-checklist.mdspecs/computer-use/INDEX.mdspecs/f2.6-region-snap-guide-and-tooltip.mdspecs/f2.7-multi-select-regions.mdspecs/p1.7-chat-tool-call-summaries.mdspecs/p3.1-asset-file-size.mdspecs/p3.3-right-panes-help.mdspecs/p3.7-ruler-hover-scrub.mdspecs/t19-zoom-timeline-and-preview.mdsrc/App.tsxsrc/components/ai-edition/AiEditionShell.tsxsrc/components/ai-edition/AnnotationLayer.tsxsrc/components/ai-edition/AnnotationOverlay.tsxsrc/components/ai-edition/ArrowSvgs.tsxsrc/components/ai-edition/Bottombar.tsxsrc/components/ai-edition/CursorPreviewLayer.module.csssrc/components/ai-edition/CursorPreviewLayer.test.tsxsrc/components/ai-edition/CursorPreviewLayer.tsxsrc/components/ai-edition/EditorEmptyState.test.tsxsrc/components/ai-edition/EditorEmptyState.tsxsrc/components/ai-edition/ExportDialog.tsxsrc/components/ai-edition/LeftPanel.tsxsrc/components/ai-edition/Modals.tsxsrc/components/ai-edition/NewEditorShell.module.csssrc/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/Preview.tsxsrc/components/ai-edition/PreviewCanvas.tsxsrc/components/ai-edition/ProviderSettings.tsxsrc/components/ai-edition/RegionTimeline.tsxsrc/components/ai-edition/RightPanelStack.tsxsrc/components/ai-edition/RightPanes.tsxsrc/components/ai-edition/TimelinePane.module.csssrc/components/ai-edition/TimelinePane.tsxsrc/components/ai-edition/Titlebar.tsxsrc/components/ai-edition/TransportBar.tsxsrc/components/ai-edition/VirtualPreview.module.csssrc/components/ai-edition/VirtualPreview.tsxsrc/components/ai-edition/WebcamOverlay.test.tsxsrc/components/ai-edition/WebcamOverlay.tsxsrc/components/ai-edition/ZoomFocusOverlay.module.csssrc/components/ai-edition/ZoomFocusOverlay.tsxsrc/components/ai-edition/chatBudget.tssrc/components/ai-edition/preview-compositor/PreviewCompositor.module.csssrc/components/ai-edition/preview-compositor/PreviewCompositor.tsxsrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/AddCustomFontDialog.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/BlurSettingsPanel.tsxsrc/components/video-editor/CropControl.tsxsrc/components/video-editor/EditorEmptyState.tsxsrc/components/video-editor/ExportDialog.tsxsrc/components/video-editor/FormatSelector.tsxsrc/components/video-editor/GifOptionsPanel.tsxsrc/components/video-editor/KeyboardShortcutsHelp.tsxsrc/components/video-editor/PlaybackControls.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/ShortcutsConfigDialog.tsxsrc/components/video-editor/TutorialHelp.tsxsrc/components/video-editor/UnsavedChangesDialog.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/featureFlags.tssrc/components/video-editor/index.tssrc/components/video-editor/timeline/BackgroundWaveform.tsxsrc/components/video-editor/timeline/Item.module.csssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/KeyframeMarkers.tsxsrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/Subrow.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/TimelineWrapper.tsxsrc/components/video-editor/timeline/zoomSuggestionUtils.tssrc/components/video-editor/videoPlayback/index.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/components/video-editor/videoPlayback/overlayUtils.tssrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/hooks/rendererConsoleForwarder.tssrc/hooks/useTheme.tssrc/lib/ai-edition/annotations/blurEffects.tssrc/lib/ai-edition/annotations/constants.tssrc/lib/ai-edition/annotations/textAnimation.tssrc/lib/ai-edition/document/ids.tssrc/lib/ai-edition/document/migrate.test.tssrc/lib/ai-edition/document/migrate.tssrc/lib/ai-edition/document/operations.test.tssrc/lib/ai-edition/document/operations.tssrc/lib/ai-edition/document/timeline.test.tssrc/lib/ai-edition/document/timeline.tssrc/lib/ai-edition/document/transcribe.test.tssrc/lib/ai-edition/document/transcribe.tssrc/lib/ai-edition/exporter/documentExporter.tssrc/lib/ai-edition/schema/index.test.tssrc/lib/ai-edition/schema/index.tssrc/lib/ai-edition/store/editorSettings.test.tssrc/lib/ai-edition/store/editorSettings.tssrc/lib/ai-edition/store/projectStore.test.tssrc/lib/ai-edition/store/projectStore.tssrc/lib/ai-edition/store/regionClipboard.tssrc/lib/ai-edition/store/undo.tssrc/lib/ai-edition/store/useEditorSettings.tssrc/lib/ai-edition/store/useOptimisticTimelineOps.tssrc/lib/ai-edition/store/useTimeline.test.tssrc/lib/ai-edition/store/useTimeline.tssrc/lib/ai-edition/store/zoomSuggestions.test.tssrc/lib/ai-edition/store/zoomSuggestions.tssrc/lib/ai-edition/timeline/aggregated-transcript.test.tssrc/lib/ai-edition/timeline/aggregated-transcript.tssrc/lib/ai-edition/timeline/camera.test.tssrc/lib/ai-edition/timeline/camera.tssrc/lib/ai-edition/timeline/duration.test.tssrc/lib/ai-edition/timeline/duration.tssrc/lib/ai-edition/timeline/format.tssrc/lib/ai-edition/timeline/playback-clock.test.tssrc/lib/ai-edition/timeline/playback-clock.tssrc/lib/ai-edition/timeline/pointer-drag.test.tsxsrc/lib/ai-edition/timeline/pointer-drag.tssrc/lib/ai-edition/timeline/speed.tssrc/lib/ai-edition/timeline/virtual-preview.test.tssrc/lib/ai-edition/timeline/virtual-preview.tssrc/lib/ai-edition/timeline/zoom-preview.tssrc/lib/captioning/index.tssrc/lib/captioning/leadingSilence.tssrc/lib/captioning/transcribe.test.tssrc/lib/captioning/transcribe.tssrc/lib/captioning/transcribe.worker.tssrc/lib/captioning/transcribeCore.tssrc/lib/compositeLayout.test.tssrc/lib/compositeLayout.tssrc/lib/cursor/nativeCursor.test.tssrc/lib/cursor/pixiCursorRenderer.tssrc/lib/cursor/uploadedCursorAssets.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/muxer.tssrc/lib/vite-stubs/empty-node-module.tssrc/lib/vite-stubs/onnxruntime-node-stub.tssrc/main.tsxsrc/native/browserShim.tssrc/native/client.tssrc/native/contracts.tssrc/styles/design-tokens.csstests/e2e/diagnostic.spec.tstests/e2e/roadmap-coverage.spec.tstests/e2e/seed.spec.tsvite.config.ts
💤 Files with no reviewable changes (2)
- scripts/fetch-caption-model.mjs
- scripts/before-pack.cjs
After cropping, the exported cursor drifted because the recorded sample was projected onto the screen mask rect instead of the rectangle the cropped video is actually painted on. In cover layouts where the cropped video letterboxes inside screenRect (coverOffset != 0), the cursor offset was proportional to the letterbox margin. Track the painted croppedRect in FrameRenderer layoutCache and pass it to projectNativeCursorToLocal so the cursor overlays the cropped video pixels exactly. Adds regression tests for projectNativeCursorToLocal. Fixes #64
EtienneLescot
force-pushed
the
opencode/swift-mountain
branch
from
a15f1f5 to
99d6107
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Thanks for the fix — the cursor projection is now correct for cover-layout exports, and the test coverage (identity, crop-region mapping, cover drift, culling, degenerate crop) is solid.
A few follow-ups worth considering before merge:
-
Likely same bug in the preview path.
VideoPlayback.tsx:1583,1593callsprojectNativeCursorToLocalwithbaseMaskRef.current, which is set fromresult.maskRectinlayoutUtils.ts:149(=compositeLayout.screenRect, the full screen rect). In fit-to-width/height layouts where the cropped video letterboxes inside the screen, the preview cursor will land at the wrong offset — same drift this PR fixes for export. Not a blocker for thisfix(export):PR, but worth a follow-up issue. -
Test name mixes two opposite concepts — inline comment on the test.
-
sizeNormcomment is now inaccurate — inline comment on thesizeNormblock. After this fix,layoutCache.maskRectiscroppedRect, so the two paths no longer use the same width source in letterbox layouts.
- Rename cover-letterboxed test to cover-overflowing (fixture is cover, not letterbox) - Update sizeNorm comment to acknowledge the export/preview asymmetry introduced by the croppedRect field: export now uses croppedRect.width, preview still uses screenRect.width. They agree in cover mode but differ in fit-to-height letterbox layouts.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Pushed the review fixes in fe4562e:
- Renamed the \cover-letterboxed\ test to \cover-overflowing\ (the fixture is a cover case, not letterbox).
- Updated the \sizeNorm\ comment to acknowledge the export/preview asymmetry introduced by the \croppedRect\ field — export now uses \croppedRect.width, preview still uses \screenRect.width. The follow-up preview-path fix is intentionally deferred to a separate PR to keep this one scoped to the export renderer geometry.
All 40 unit tests in \src/lib/cursor\ + \src/lib/exporter/frameRenderer.test.ts\ pass; Biome clean on both touched files.
The Windows cursor-sampler (Win32 GetCursorInfo) reports raw x/y in physical screen pixels. For display captures, normalizeSample was dividing by bounds from Electron's \screen\ API, which are in logical pixels (DIP). On a 100% DPI display these coincide and the bug is invisible, but on any high-DPI display (125%, 150%, 200%) the normalized cursor position is wrong by the scale factor. The error then compounds through projectNativeCursorToLocal in the export, producing a visible cursor offset proportional to the DPI ratio. Fix by converting the logical bounds to physical via the display's scaleFactor before normalizing. payload.bounds (from the sampler's GetWindowRect, used for window captures) is already physical and is left as-is. Pre-existing since 1.5.0; never caught because CI is Linux-only and manual Windows smoke tests ran on 100% DPI displays.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts`:
- Around line 235-242: The cursor normalization logic in
windowsNativeRecordingSession is using a single scaleFactor to convert
Display.bounds, which can misplace the origin on secondary or mixed-DPI
monitors. Update the coordinate conversion in the recording session code to use
Electron’s screen helpers, such as screen.dipToScreenRect(null, bounds) or
dipToScreenPoint, before computing normalizedX and normalizedY. Keep the
existing bounds/normalization flow, but replace the manual bounds.x/bounds.y
scaling in the relevant cursor recording path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68960589-8868-4fb8-bf71-dfad37dddfb5
📒 Files selected for processing (1)
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts
The previous fix multiplied bounds.x/y/width/height by a single
scaleFactor to convert from DIPs to physical pixels. That works for the
primary display (origin at 0,0 in the virtual screen) but misplaces the
origin on non-primary or mixed-DPI displays: a secondary 200% DPI
monitor to the right of a 100% primary has DIP bounds {x:1920, y:0,
w:1920, h:1080} but physical bounds {x:1920, y:0, w:3840, h:2160} —
multiplying the origin by 2 would push it to 3840.
Use Electron's \screen.dipToScreenRect(null, bounds)\ instead, which
picks the correct display from the rect's center and handles the
virtual-screen origin correctly across multi-monitor and mixed-DPI
setups. payload.bounds (from the sampler's GetWindowRect) is already
physical and is left as-is.
1 participant
Summary
After cropping the recorded video, the exported cursor drifted from where it should sit. The export pipeline was projecting the recorded cursor sample onto the screen mask rect (
compositeLayout.screenRect) instead of the rectangle the cropped video is actually painted on. In cover layouts where the cropped video letterboxes insidescreenRect(coverOffset != 0), the cursor's offset was proportional to the letterbox margin — visible immediately after applying a crop.This adds a
croppedRectfield toFrameRenderer'sLayoutCache(computed from the existingcoverOffsetX/YandcroppedDisplayWidth/HeightinupdateLayout) and passes it toprojectNativeCursorToLocalso the cursor lands on the same pixels the cropped video is drawn on.maskRectis still used forcursorClipToBoundsviewport clipping — that behavior is unchanged.Adds a
projectNativeCursorToLocalregression test suite covering identity, cropped-region mapping, the cover-letterbox drift case, out-of-region culling, and a degenerate crop.Follow-up: Windows cursor-position DPI normalization
While validating this fix end-to-end on Windows, a pre-existing latent bug in the native cursor pipeline surfaced (it had been there since 1.5.0 but was invisible before the native helpers were built — browser-capture fallback handled DPI internally). The Windows cursor-sampler (
Win32 GetCursorInfo) reports raw x/y in physical screen pixels, but the Electron session was normalizing against bounds from Electron'sscreenAPI, which are in logical pixels (DIP). On a 100% DPI display these coincide; on any high-DPI display the normalized cursor position is wrong by the scale factor, and the error compounds throughprojectNativeCursorToLocalin the export, producing a visible cursor offset proportional to the DPI ratio.Fix in
windowsNativeRecordingSession.ts: convert the logical bounds to physical screen coordinates viascreen.dipToScreenRect(null, bounds), which correctly handles the virtual-screen origin across multi-monitor and mixed-DPI setups. A naivebounds.x * scaleFactor(the first attempt) was wrong for non-primary displays — a secondary 200% DPI monitor to the right of a 100% primary has DIP bounds{x:1920, y:0, w:1920, h:1080}but physical bounds{x:1920, y:0, w:3840, h:2160}, so multiplying the origin by 2 would have pushed it to x=3840.payload.bounds(from the sampler'sGetWindowRect, used for window captures) is already physical and is left as-is.The macOS path (
macNativeCursorRecordingSession.ts) is unaffected:screen.getCursorScreenPoint()andscreen.getDisplayNearestPoint().boundsboth return Cocoa points, the macOS helper doesn't report rawboundsin its sample events, and the [0,1] normalization is scale-invariant between points and the video's pixels.Related issue
Fixes #64
Type of change
Release impact
Desktop impact
Screenshots / video
N/A — export rendering geometry fix.
Testing
npx tsc --noEmitclean.npx vitest run --pool=vmThreads src/lib/cursor src/lib/exporter/frameRenderer.test.ts→ 40/40 passing (12 pre-existing + 4 newprojectNativeCursorToLocalcases + the rest of the cursor/export suites).normalizedX/YmatchingrawX/Y ÷ 1920/1080instead of÷ 1536/864).Manual smoke test on real macOS is still recommended for native-capture changes per the AGENTS.md guidance, though the DPI fix is Windows-specific and the cursor-projection fix is platform-agnostic. The macOS cursor path was reviewed for the same DPI issue and is unaffected (see PR description).
Summary by CodeRabbit