chore(release): release v1.9.0 into main - #288
Conversation
📝 WalkthroughWalkthroughThe package version in ChangesPackage release metadata
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
2879746 to
7385656
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ai-edition/NewEditorShell.tsx (1)
686-701: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStart recording in one control path.
If the user selects Save or Discard, Line 686 starts recording and resolves the prompt. Line 699 then starts recording again. One user action can send two
startNewRecordingIPC requests.Remove the
action === "record"start fromhandleConfirmUnsaved. Keep recording startup inhandleNewRecordingafterpromptUnsavedresolves.Proposed fix
- const { action, resolve } = unsavedPrompt; + const { resolve } = unsavedPrompt; ... - if (action === "record") { - void window.electronAPI?.startNewRecording?.().catch((err) => { - console.warn("[editor] failed to start a new recording:", err); - }); - } resolve(choice);🤖 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 `@src/components/ai-edition/NewEditorShell.tsx` around lines 686 - 701, Remove the action === "record" startNewRecording call from handleConfirmUnsaved, leaving it responsible only for resolving the prompt. Keep handleNewRecording's startNewRecording call after promptUnsaved resolves for non-cancel choices, ensuring each recording action sends only one IPC request.electron/ipc/handlers.ts (1)
2801-2812: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not attach webcam files before the helper finalizes them.
waitForNativeWindowsCaptureStopcan returnok: trueafter emittingrecording-stoppedwithwebcamPath, even when the helper later exits 1 fromERROR: Failed to finalize the webcam recordingor is terminated duringwebcam-encoder-finalize. The current parent-side logic treats any readablepreferredWebcamPathas good, so an unindexed webcam MP4 can be stored in the session manifest and media links while the screen video is correctly retained.Gate
webcamVideoPathby the salvageable-output threshold before registering media links, or have the helper emitwebcamPathonly afterwebcamEncoder.finalize()succeeds. Keep the stricter contract change focused on the webcam emission so the screen file is not dropped.🤖 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/ipc/handlers.ts` around lines 2801 - 2812, The webcam path in the recording-session setup must not be accepted solely because it is readable. Update the flow around waitForNativeWindowsCaptureStop and the webcamPath emission so webcamVideoPath is only attached after webcamEncoder.finalize() succeeds or the helper explicitly reports a salvageable finalized output; preserve the screenVideoPath and session creation when webcam finalization fails or the helper is terminated.
🧹 Nitpick comments (4)
AGENTS.md (1)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the final full-suite policy.
The canonical guidance conflicts with the developer-agent guidance. The release workflow guide requires one final
npm run testbefore pushing. Keep that requirement consistent.
AGENTS.md#L50-L52: remove the exception that permits targeted tests plus CI instead of the final full run..harness/reins/openscreen-dev/agent.md#L29-L31: replace “if at all” with the final full-suite requirement.🤖 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 `@AGENTS.md` around lines 50 - 52, Align the final test policy across both guidance files: in AGENTS.md lines 50-52, require one final npm run test and remove the exception allowing targeted tests plus CI; in .harness/reins/openscreen-dev/agent.md lines 29-31, replace “if at all” with the same final full-suite requirement.electron/ipc/handlers.ts (1)
2446-2453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the partial output when the start fails.
The discard path and the stop-failure path both call
removeNativeWindowsCaptureOutputs. This path does not. A helper that spawned, created the MP4, and then failed beforeRecording startedleaves an unindexed stub inRECORDINGS_DIRthat no session manifest references. The size gate already classifies such a file as unusable, so the same call is safe here.Note the ordering constraint: read the paths into locals before
resetNativeWindowsCaptureState()clears them.♻️ Proposed refactor
} catch (error) { console.error("Failed to start native Windows recording:", error); + const failedScreenPath = nativeWindowsCaptureTargetPath; + const failedWebcamPath = nativeWindowsCaptureWebcamTargetPath; nativeWindowsCaptureProcess?.kill(); detachNativeWindowsCaptureOutputDrain(); resetNativeWindowsCaptureState(); await stopCursorRecording(); + await removeNativeWindowsCaptureOutputs(failedScreenPath, failedWebcamPath, { + onlyIfUnusable: true, + }); return { success: false, error: String(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/ipc/handlers.ts` around lines 2446 - 2453, Update the start-failure catch block in the native Windows recording flow to capture the current output paths in locals before calling resetNativeWindowsCaptureState(), then invoke removeNativeWindowsCaptureOutputs with those paths. Preserve the existing cleanup and failure response while ensuring any partially created MP4 output is discarded.scripts/test-windows-wgc-helper.mjs (1)
44-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the numeric env values against NaN.
Number(process.env[...])returns NaN for a malformed value. A typo inOPENSCREEN_WGC_STOP_BUDGET_MSmakesSTOP_BUDGET_MSNaN, soSTOP_HANG_LIMIT_MSis NaN,setTimeout(..., NaN)fires on the next tick, and the harness kills the helper and reports the issue#252hang it exists to detect.String(NaN)also reaches the child environment, wherereadEnvIntdiscards it and silently uses its own default — so the harness and the helper no longer agree, which is the drift this constant was added to prevent.Parse with a finite-value check and fall back to the documented default.
♻️ Proposed refactor
+const readEnvMs = (name, fallback) => { + const parsed = Number(process.env[name]); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +}; -const STALL_READBACK_MS = Number(process.env[STALL_READBACK_ENV] ?? 60_000); +const STALL_READBACK_MS = readEnvMs(STALL_READBACK_ENV, 60_000); const STOP_BUDGET_ENV = "OPENSCREEN_WGC_STOP_BUDGET_MS"; @@ -const STOP_BUDGET_MS = Number(process.env[STOP_BUDGET_ENV] ?? 50_000); +const STOP_BUDGET_MS = readEnvMs(STOP_BUDGET_ENV, 50_000);🤖 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/test-windows-wgc-helper.mjs` around lines 44 - 61, Guard the numeric environment parsing for STALL_READBACK_MS and STOP_BUDGET_MS with a finite-value check, falling back to their documented defaults when parsing yields NaN or another non-finite value. Ensure the validated STOP_BUDGET_MS is used consistently to derive STOP_HANG_LIMIT_MS and pass the child environment value.vitest.config.ts (1)
23-23: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftScope the 15-second timeout to slow tests.
testTimeoutis a global per-test timeout. Raising it from Vitest’s 5-second default to 15 seconds also delays failure reporting for unrelated hangs. Keep the suite default lower and apply 15 seconds only to the measured slow files or tests, unless the full-suite failure budget is an explicit release requirement. Vitest definestestTimeoutas the default timeout of a test in milliseconds. (v4.vitest.dev)🤖 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 `@vitest.config.ts` at line 23, Keep the global testTimeout at Vitest’s default or a lower suite-wide value, and move the 15-second timeout to only the specific slow test files or tests that require it. Update the Vitest configuration around testTimeout without changing unrelated test settings.
🤖 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 @.github/workflows/build.yml:
- Around line 533-542: Update the prerelease previous-tag search in the workflow
around IS_PRERELEASE and RC_NUMBER to derive and reuse the current prerelease
kind, rather than hardcoding “rc” in CANDIDATE. Preserve the existing numeric
countdown and tag-existence check so alpha, beta, and rc releases select the
preceding tag of the same kind.
In `@electron/ai-edition/agent-tools.ts`:
- Around line 373-375: Bound the batch sizes accepted by addTrimsArgs and the
corresponding addZoom batch schema, and update the iteration flow around the
unitary executor at the 710-735 range to enforce that limit while preserving
sequential semantics. Avoid repeatedly copying the accumulated ranges on each
successful addTrim or addZoom operation by using an internal accumulator or
equivalent linear-time approach.
In `@electron/media/mediaLinksRegistry.test.ts`:
- Around line 280-297: Update the “survives the directory disappearing while the
refresh is queued” test to instrument the refresh write and wait until that
write is pending before removing tempDir. Assert the findMediaLinksByFingerprint
lookup result while the write barrier remains held, then release the barrier and
verify withoutUnhandledRejections still reports no rejections; keep the existing
warning handling and cleanup.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 1052-1060: Update the beginStopStep lambda so it assigns
currentStepDeadlineMs before publishing the new currentStopStep value. Preserve
the existing deadline calculation and timing log, ensuring the watchdog cannot
observe a new step name paired with the previous expired deadline.
In `@electron/native/wgc-capture/src/wgc_session.cpp`:
- Around line 278-282: Replace the single quiesced_ state in WgcSession with
separate closed_ and handlerRevoked_ flags. Update quiesceCapture so retries
after a drain timeout repeat the drain and close sequence, while the early
return only reports success when closed_ is true; set closed_ immediately before
the successful return. Guard callback-handler revocation and frameCallback_
clearing with handlerRevoked_ to avoid repeating those operations.
In `@scripts/test-windows-wgc-helper.mjs`:
- Around line 60-61: Update STOP_LATENCY_BUDGET_MS and its comment in the test
harness so a legitimate encoder-finalize lasting up to the configured
OPENSCREEN_WGC_STOP_BUDGET_MS does not fail the helper; either set the overall
budget above STOP_BUDGET_MS or limit this timeout to pre-finalize steps.
Preserve the existing stopHung and required-step assertions as the checks for
hung or incomplete helpers.
In `@src/hooks/useScreenRecorder.nativeStopFailure.test.tsx`:
- Around line 20-41: Pin process.platform to "win32" in the native Windows test
suite’s beforeEach and restore the original platform value in afterEach. Keep
stubElectronAPI focused on Electron API mocks, and ensure cleanup runs after
every test so the host platform is not altered for other suites.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1200-1211: Track ownership transfer for the local recorder created
in the macOS path around createRecorderHandle, and when startup fails or the
countdown becomes inactive before webcamRecorder.current receives it, stop and
drain the recorder before calling discard(). Apply the same ownership tracking
and cleanup to the Linux path around its createRecorderHandle call in
src/hooks/useScreenRecorder.ts:1392-1401; ensure both sites preserve the
existing cleanup when ownership is transferred.
In `@src/native/sceneDescription.test.ts`:
- Line 1347: In the affected test scope, remove the duplicate const area
declarations and retain a single area helper declaration for the test’s
calculations. Ensure all existing usages continue referencing that remaining
declaration without changing its behavior.
In `@technical-documentation/testing/writing-tests.md`:
- Around line 21-42: Update the testing guidance to state that jsdom is required
only for tests using DOM APIs, including applicable *.test.tsx files, rather
than every TSX test. Revise the documented file pattern to match
vitest.config.ts by covering test and spec files across JavaScript and
TypeScript extensions, while retaining node as the default environment and the
explicit jsdom opt-in guidance.
In `@workbench/lib/oracles.ts`:
- Around line 297-305: Update the array handling in the result oracle so every
applied entry must be an object containing an identifier recognized by ID_KEYS;
return false for non-object entries or objects without such an identifier.
Preserve claimSurvives validation for entries that pass this structural check
and for non-array results.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2801-2812: The webcam path in the recording-session setup must not
be accepted solely because it is readable. Update the flow around
waitForNativeWindowsCaptureStop and the webcamPath emission so webcamVideoPath
is only attached after webcamEncoder.finalize() succeeds or the helper
explicitly reports a salvageable finalized output; preserve the screenVideoPath
and session creation when webcam finalization fails or the helper is terminated.
In `@src/components/ai-edition/NewEditorShell.tsx`:
- Around line 686-701: Remove the action === "record" startNewRecording call
from handleConfirmUnsaved, leaving it responsible only for resolving the prompt.
Keep handleNewRecording's startNewRecording call after promptUnsaved resolves
for non-cancel choices, ensuring each recording action sends only one IPC
request.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 50-52: Align the final test policy across both guidance files: in
AGENTS.md lines 50-52, require one final npm run test and remove the exception
allowing targeted tests plus CI; in .harness/reins/openscreen-dev/agent.md lines
29-31, replace “if at all” with the same final full-suite requirement.
In `@electron/ipc/handlers.ts`:
- Around line 2446-2453: Update the start-failure catch block in the native
Windows recording flow to capture the current output paths in locals before
calling resetNativeWindowsCaptureState(), then invoke
removeNativeWindowsCaptureOutputs with those paths. Preserve the existing
cleanup and failure response while ensuring any partially created MP4 output is
discarded.
In `@scripts/test-windows-wgc-helper.mjs`:
- Around line 44-61: Guard the numeric environment parsing for STALL_READBACK_MS
and STOP_BUDGET_MS with a finite-value check, falling back to their documented
defaults when parsing yields NaN or another non-finite value. Ensure the
validated STOP_BUDGET_MS is used consistently to derive STOP_HANG_LIMIT_MS and
pass the child environment value.
In `@vitest.config.ts`:
- Line 23: Keep the global testTimeout at Vitest’s default or a lower suite-wide
value, and move the 15-second timeout to only the specific slow test files or
tests that require it. Update the Vitest configuration around testTimeout
without changing unrelated test settings.
🪄 Autofix
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: 8d9eeb2f-e20a-4ad0-9a4e-f39d57ee17c7
📒 Files selected for processing (106)
.github/scripts/discord-thread-validator.mjs.github/scripts/discord-thread-validator.test.mjs.github/workflows/build.yml.github/workflows/diagnostic-artifact.yml.harness/docs/git-workflow.md.harness/reins/openscreen-dev/agent.md.harness/reins/openscreen-tester/agent.mdAGENTS.mdcrates/compositor/src/compositor_linux.rscrates/compositor/src/compositor_macos.rscrates/compositor/src/compositor_windows.rscrates/compositor/src/frame_geometry.rscrates/compositor/src/live.rscrates/compositor/src/timeline_walk.rscrates/poc-d3d/src/bench.rselectron-builder.json5electron/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.compaction.test.tselectron/ai-edition/chat-service.tselectron/ai-edition/deep-agent/service.test.tselectron/ai-edition/deep-agent/service.tselectron/ai-edition/document-service.test.tselectron/electron-env.d.tselectron/ipc/handlers.tselectron/media/mediaLinksRegistry.test.tselectron/media/mediaLinksRegistry.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native/wgc-capture/src/cursor-sampler.cppelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/wgc_session.cppelectron/native/wgc-capture/src/wgc_session.helectron/preload.tselectron/recording/nativeWindowsCaptureStop.test.tselectron/recording/nativeWindowsCaptureStop.tselectron/recording/webm-seek-index.test.tselectron/windows.tspackage.jsonscripts/diagnostic-tool/diagnostic.mjsscripts/test-windows-wgc-helper.mjssrc/components/ai-edition/CaptionsPane.gating.test.tsxsrc/components/ai-edition/ChatWelcome.test.tsxsrc/components/ai-edition/ColorField.test.tsxsrc/components/ai-edition/EditorEmptyState.test.tsxsrc/components/ai-edition/ExportDialog.tsxsrc/components/ai-edition/Modals.tsxsrc/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/NewProjectModal.test.tsxsrc/components/ai-edition/Preview.test.tsxsrc/components/ai-edition/PreviewCanvas.tsxsrc/components/ai-edition/RightPanes.i18n.test.tsxsrc/components/ai-edition/TranscriptPane.gating.test.tsxsrc/components/ai-edition/TranscriptPane.keyboardCut.test.tsxsrc/components/ai-edition/TranscriptPane.sharedMedia.test.tsxsrc/components/ai-edition/TransportBar.test.tsxsrc/components/ai-edition/VirtualPreview.playback.test.tsxsrc/components/ai-edition/WebcamOverlay.test.tsxsrc/components/ai-edition/backgroundImageUpload.test.tsxsrc/components/ai-edition/chatBudget.tssrc/components/ai-edition/v4/EditorTopBar.test.tsxsrc/components/ai-edition/v4/RecStage.tsxsrc/components/ai-edition/v4/SpeedControl.test.tsxsrc/components/ai-edition/v4/V4Timeline.geometry.test.tsxsrc/components/launch/LaunchWindow.test.tsxsrc/components/launch/NotesToolbar.test.tsxsrc/components/launch/NotesWindow.csssrc/components/launch/NotesWindow.editable.test.tsxsrc/components/launch/NotesWindow.test.tsxsrc/components/launch/NotesWindow.tsxsrc/components/launch/SourceSelector.test.tsxsrc/components/ui/gradient-editor.test.tsxsrc/components/video-editor/editorDefaults.tssrc/hooks/recorderHandle.test.tssrc/hooks/useAudioPeaks.test.tssrc/hooks/useCameraDevices.test.tssrc/hooks/useScreenRecorder.nativeStopFailure.test.tsxsrc/hooks/useScreenRecorder.tssrc/hooks/webcamAsset.test.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/transcriptionStore.test.tssrc/lib/ai-edition/store/useSequentialTimelineOps.test.tssrc/lib/ai-edition/store/useTimeline.test.tssrc/lib/ai-edition/store/useTimeline.tssrc/lib/ai-edition/timeline/duration.test.tssrc/lib/ai-edition/timeline/pointer-drag.test.tsxsrc/lib/captioning/transcribe.test.tssrc/native/hooks/useCompositorBackend.test.tssrc/native/hooks/useNativeCompositorView.test.tssrc/native/sceneDescription.test.tssrc/native/sceneDescription.tssrc/utils/platformUtils.test.tstechnical-documentation/architecture/ai-agent.mdtechnical-documentation/architecture/recording.mdtechnical-documentation/engineering/build-and-packaging.mdtechnical-documentation/engineering/ci-workflows.mdtechnical-documentation/engineering/release-and-secrets.mdtechnical-documentation/testing/writing-tests.mdtests/e2e/windows-native-checklist.spec.tsvitest.config.tsworkbench/l0/oracles.wb.tsworkbench/lib/fixtures.tsworkbench/lib/oracles.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ai-edition/NewEditorShell.tsx (1)
686-701: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStart recording in one control path.
If the user selects Save or Discard, Line 686 starts recording and resolves the prompt. Line 699 then starts recording again. One user action can send two
startNewRecordingIPC requests.Remove the
action === "record"start fromhandleConfirmUnsaved. Keep recording startup inhandleNewRecordingafterpromptUnsavedresolves.Proposed fix
- const { action, resolve } = unsavedPrompt; + const { resolve } = unsavedPrompt; ... - if (action === "record") { - void window.electronAPI?.startNewRecording?.().catch((err) => { - console.warn("[editor] failed to start a new recording:", err); - }); - } resolve(choice);🤖 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 `@src/components/ai-edition/NewEditorShell.tsx` around lines 686 - 701, Remove the action === "record" startNewRecording call from handleConfirmUnsaved, leaving it responsible only for resolving the prompt. Keep handleNewRecording's startNewRecording call after promptUnsaved resolves for non-cancel choices, ensuring each recording action sends only one IPC request.electron/ipc/handlers.ts (1)
2801-2812: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not attach webcam files before the helper finalizes them.
waitForNativeWindowsCaptureStopcan returnok: trueafter emittingrecording-stoppedwithwebcamPath, even when the helper later exits 1 fromERROR: Failed to finalize the webcam recordingor is terminated duringwebcam-encoder-finalize. The current parent-side logic treats any readablepreferredWebcamPathas good, so an unindexed webcam MP4 can be stored in the session manifest and media links while the screen video is correctly retained.Gate
webcamVideoPathby the salvageable-output threshold before registering media links, or have the helper emitwebcamPathonly afterwebcamEncoder.finalize()succeeds. Keep the stricter contract change focused on the webcam emission so the screen file is not dropped.🤖 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/ipc/handlers.ts` around lines 2801 - 2812, The webcam path in the recording-session setup must not be accepted solely because it is readable. Update the flow around waitForNativeWindowsCaptureStop and the webcamPath emission so webcamVideoPath is only attached after webcamEncoder.finalize() succeeds or the helper explicitly reports a salvageable finalized output; preserve the screenVideoPath and session creation when webcam finalization fails or the helper is terminated.
🧹 Nitpick comments (4)
AGENTS.md (1)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the final full-suite policy.
The canonical guidance conflicts with the developer-agent guidance. The release workflow guide requires one final
npm run testbefore pushing. Keep that requirement consistent.
AGENTS.md#L50-L52: remove the exception that permits targeted tests plus CI instead of the final full run..harness/reins/openscreen-dev/agent.md#L29-L31: replace “if at all” with the final full-suite requirement.🤖 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 `@AGENTS.md` around lines 50 - 52, Align the final test policy across both guidance files: in AGENTS.md lines 50-52, require one final npm run test and remove the exception allowing targeted tests plus CI; in .harness/reins/openscreen-dev/agent.md lines 29-31, replace “if at all” with the same final full-suite requirement.electron/ipc/handlers.ts (1)
2446-2453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the partial output when the start fails.
The discard path and the stop-failure path both call
removeNativeWindowsCaptureOutputs. This path does not. A helper that spawned, created the MP4, and then failed beforeRecording startedleaves an unindexed stub inRECORDINGS_DIRthat no session manifest references. The size gate already classifies such a file as unusable, so the same call is safe here.Note the ordering constraint: read the paths into locals before
resetNativeWindowsCaptureState()clears them.♻️ Proposed refactor
} catch (error) { console.error("Failed to start native Windows recording:", error); + const failedScreenPath = nativeWindowsCaptureTargetPath; + const failedWebcamPath = nativeWindowsCaptureWebcamTargetPath; nativeWindowsCaptureProcess?.kill(); detachNativeWindowsCaptureOutputDrain(); resetNativeWindowsCaptureState(); await stopCursorRecording(); + await removeNativeWindowsCaptureOutputs(failedScreenPath, failedWebcamPath, { + onlyIfUnusable: true, + }); return { success: false, error: String(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/ipc/handlers.ts` around lines 2446 - 2453, Update the start-failure catch block in the native Windows recording flow to capture the current output paths in locals before calling resetNativeWindowsCaptureState(), then invoke removeNativeWindowsCaptureOutputs with those paths. Preserve the existing cleanup and failure response while ensuring any partially created MP4 output is discarded.scripts/test-windows-wgc-helper.mjs (1)
44-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the numeric env values against NaN.
Number(process.env[...])returns NaN for a malformed value. A typo inOPENSCREEN_WGC_STOP_BUDGET_MSmakesSTOP_BUDGET_MSNaN, soSTOP_HANG_LIMIT_MSis NaN,setTimeout(..., NaN)fires on the next tick, and the harness kills the helper and reports the issue#252hang it exists to detect.String(NaN)also reaches the child environment, wherereadEnvIntdiscards it and silently uses its own default — so the harness and the helper no longer agree, which is the drift this constant was added to prevent.Parse with a finite-value check and fall back to the documented default.
♻️ Proposed refactor
+const readEnvMs = (name, fallback) => { + const parsed = Number(process.env[name]); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +}; -const STALL_READBACK_MS = Number(process.env[STALL_READBACK_ENV] ?? 60_000); +const STALL_READBACK_MS = readEnvMs(STALL_READBACK_ENV, 60_000); const STOP_BUDGET_ENV = "OPENSCREEN_WGC_STOP_BUDGET_MS"; @@ -const STOP_BUDGET_MS = Number(process.env[STOP_BUDGET_ENV] ?? 50_000); +const STOP_BUDGET_MS = readEnvMs(STOP_BUDGET_ENV, 50_000);🤖 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/test-windows-wgc-helper.mjs` around lines 44 - 61, Guard the numeric environment parsing for STALL_READBACK_MS and STOP_BUDGET_MS with a finite-value check, falling back to their documented defaults when parsing yields NaN or another non-finite value. Ensure the validated STOP_BUDGET_MS is used consistently to derive STOP_HANG_LIMIT_MS and pass the child environment value.vitest.config.ts (1)
23-23: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftScope the 15-second timeout to slow tests.
testTimeoutis a global per-test timeout. Raising it from Vitest’s 5-second default to 15 seconds also delays failure reporting for unrelated hangs. Keep the suite default lower and apply 15 seconds only to the measured slow files or tests, unless the full-suite failure budget is an explicit release requirement. Vitest definestestTimeoutas the default timeout of a test in milliseconds. (v4.vitest.dev)🤖 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 `@vitest.config.ts` at line 23, Keep the global testTimeout at Vitest’s default or a lower suite-wide value, and move the 15-second timeout to only the specific slow test files or tests that require it. Update the Vitest configuration around testTimeout without changing unrelated test settings.
🤖 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 @.github/workflows/build.yml:
- Around line 533-542: Update the prerelease previous-tag search in the workflow
around IS_PRERELEASE and RC_NUMBER to derive and reuse the current prerelease
kind, rather than hardcoding “rc” in CANDIDATE. Preserve the existing numeric
countdown and tag-existence check so alpha, beta, and rc releases select the
preceding tag of the same kind.
In `@electron/ai-edition/agent-tools.ts`:
- Around line 373-375: Bound the batch sizes accepted by addTrimsArgs and the
corresponding addZoom batch schema, and update the iteration flow around the
unitary executor at the 710-735 range to enforce that limit while preserving
sequential semantics. Avoid repeatedly copying the accumulated ranges on each
successful addTrim or addZoom operation by using an internal accumulator or
equivalent linear-time approach.
In `@electron/media/mediaLinksRegistry.test.ts`:
- Around line 280-297: Update the “survives the directory disappearing while the
refresh is queued” test to instrument the refresh write and wait until that
write is pending before removing tempDir. Assert the findMediaLinksByFingerprint
lookup result while the write barrier remains held, then release the barrier and
verify withoutUnhandledRejections still reports no rejections; keep the existing
warning handling and cleanup.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 1052-1060: Update the beginStopStep lambda so it assigns
currentStepDeadlineMs before publishing the new currentStopStep value. Preserve
the existing deadline calculation and timing log, ensuring the watchdog cannot
observe a new step name paired with the previous expired deadline.
In `@electron/native/wgc-capture/src/wgc_session.cpp`:
- Around line 278-282: Replace the single quiesced_ state in WgcSession with
separate closed_ and handlerRevoked_ flags. Update quiesceCapture so retries
after a drain timeout repeat the drain and close sequence, while the early
return only reports success when closed_ is true; set closed_ immediately before
the successful return. Guard callback-handler revocation and frameCallback_
clearing with handlerRevoked_ to avoid repeating those operations.
In `@scripts/test-windows-wgc-helper.mjs`:
- Around line 60-61: Update STOP_LATENCY_BUDGET_MS and its comment in the test
harness so a legitimate encoder-finalize lasting up to the configured
OPENSCREEN_WGC_STOP_BUDGET_MS does not fail the helper; either set the overall
budget above STOP_BUDGET_MS or limit this timeout to pre-finalize steps.
Preserve the existing stopHung and required-step assertions as the checks for
hung or incomplete helpers.
In `@src/hooks/useScreenRecorder.nativeStopFailure.test.tsx`:
- Around line 20-41: Pin process.platform to "win32" in the native Windows test
suite’s beforeEach and restore the original platform value in afterEach. Keep
stubElectronAPI focused on Electron API mocks, and ensure cleanup runs after
every test so the host platform is not altered for other suites.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1200-1211: Track ownership transfer for the local recorder created
in the macOS path around createRecorderHandle, and when startup fails or the
countdown becomes inactive before webcamRecorder.current receives it, stop and
drain the recorder before calling discard(). Apply the same ownership tracking
and cleanup to the Linux path around its createRecorderHandle call in
src/hooks/useScreenRecorder.ts:1392-1401; ensure both sites preserve the
existing cleanup when ownership is transferred.
In `@src/native/sceneDescription.test.ts`:
- Line 1347: In the affected test scope, remove the duplicate const area
declarations and retain a single area helper declaration for the test’s
calculations. Ensure all existing usages continue referencing that remaining
declaration without changing its behavior.
In `@technical-documentation/testing/writing-tests.md`:
- Around line 21-42: Update the testing guidance to state that jsdom is required
only for tests using DOM APIs, including applicable *.test.tsx files, rather
than every TSX test. Revise the documented file pattern to match
vitest.config.ts by covering test and spec files across JavaScript and
TypeScript extensions, while retaining node as the default environment and the
explicit jsdom opt-in guidance.
In `@workbench/lib/oracles.ts`:
- Around line 297-305: Update the array handling in the result oracle so every
applied entry must be an object containing an identifier recognized by ID_KEYS;
return false for non-object entries or objects without such an identifier.
Preserve claimSurvives validation for entries that pass this structural check
and for non-array results.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2801-2812: The webcam path in the recording-session setup must not
be accepted solely because it is readable. Update the flow around
waitForNativeWindowsCaptureStop and the webcamPath emission so webcamVideoPath
is only attached after webcamEncoder.finalize() succeeds or the helper
explicitly reports a salvageable finalized output; preserve the screenVideoPath
and session creation when webcam finalization fails or the helper is terminated.
In `@src/components/ai-edition/NewEditorShell.tsx`:
- Around line 686-701: Remove the action === "record" startNewRecording call
from handleConfirmUnsaved, leaving it responsible only for resolving the prompt.
Keep handleNewRecording's startNewRecording call after promptUnsaved resolves
for non-cancel choices, ensuring each recording action sends only one IPC
request.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 50-52: Align the final test policy across both guidance files: in
AGENTS.md lines 50-52, require one final npm run test and remove the exception
allowing targeted tests plus CI; in .harness/reins/openscreen-dev/agent.md lines
29-31, replace “if at all” with the same final full-suite requirement.
In `@electron/ipc/handlers.ts`:
- Around line 2446-2453: Update the start-failure catch block in the native
Windows recording flow to capture the current output paths in locals before
calling resetNativeWindowsCaptureState(), then invoke
removeNativeWindowsCaptureOutputs with those paths. Preserve the existing
cleanup and failure response while ensuring any partially created MP4 output is
discarded.
In `@scripts/test-windows-wgc-helper.mjs`:
- Around line 44-61: Guard the numeric environment parsing for STALL_READBACK_MS
and STOP_BUDGET_MS with a finite-value check, falling back to their documented
defaults when parsing yields NaN or another non-finite value. Ensure the
validated STOP_BUDGET_MS is used consistently to derive STOP_HANG_LIMIT_MS and
pass the child environment value.
In `@vitest.config.ts`:
- Line 23: Keep the global testTimeout at Vitest’s default or a lower suite-wide
value, and move the 15-second timeout to only the specific slow test files or
tests that require it. Update the Vitest configuration around testTimeout
without changing unrelated test settings.
🪄 Autofix
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: 8d9eeb2f-e20a-4ad0-9a4e-f39d57ee17c7
📒 Files selected for processing (106)
.github/scripts/discord-thread-validator.mjs.github/scripts/discord-thread-validator.test.mjs.github/workflows/build.yml.github/workflows/diagnostic-artifact.yml.harness/docs/git-workflow.md.harness/reins/openscreen-dev/agent.md.harness/reins/openscreen-tester/agent.mdAGENTS.mdcrates/compositor/src/compositor_linux.rscrates/compositor/src/compositor_macos.rscrates/compositor/src/compositor_windows.rscrates/compositor/src/frame_geometry.rscrates/compositor/src/live.rscrates/compositor/src/timeline_walk.rscrates/poc-d3d/src/bench.rselectron-builder.json5electron/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.compaction.test.tselectron/ai-edition/chat-service.tselectron/ai-edition/deep-agent/service.test.tselectron/ai-edition/deep-agent/service.tselectron/ai-edition/document-service.test.tselectron/electron-env.d.tselectron/ipc/handlers.tselectron/media/mediaLinksRegistry.test.tselectron/media/mediaLinksRegistry.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native/wgc-capture/src/cursor-sampler.cppelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/wgc_session.cppelectron/native/wgc-capture/src/wgc_session.helectron/preload.tselectron/recording/nativeWindowsCaptureStop.test.tselectron/recording/nativeWindowsCaptureStop.tselectron/recording/webm-seek-index.test.tselectron/windows.tspackage.jsonscripts/diagnostic-tool/diagnostic.mjsscripts/test-windows-wgc-helper.mjssrc/components/ai-edition/CaptionsPane.gating.test.tsxsrc/components/ai-edition/ChatWelcome.test.tsxsrc/components/ai-edition/ColorField.test.tsxsrc/components/ai-edition/EditorEmptyState.test.tsxsrc/components/ai-edition/ExportDialog.tsxsrc/components/ai-edition/Modals.tsxsrc/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/NewProjectModal.test.tsxsrc/components/ai-edition/Preview.test.tsxsrc/components/ai-edition/PreviewCanvas.tsxsrc/components/ai-edition/RightPanes.i18n.test.tsxsrc/components/ai-edition/TranscriptPane.gating.test.tsxsrc/components/ai-edition/TranscriptPane.keyboardCut.test.tsxsrc/components/ai-edition/TranscriptPane.sharedMedia.test.tsxsrc/components/ai-edition/TransportBar.test.tsxsrc/components/ai-edition/VirtualPreview.playback.test.tsxsrc/components/ai-edition/WebcamOverlay.test.tsxsrc/components/ai-edition/backgroundImageUpload.test.tsxsrc/components/ai-edition/chatBudget.tssrc/components/ai-edition/v4/EditorTopBar.test.tsxsrc/components/ai-edition/v4/RecStage.tsxsrc/components/ai-edition/v4/SpeedControl.test.tsxsrc/components/ai-edition/v4/V4Timeline.geometry.test.tsxsrc/components/launch/LaunchWindow.test.tsxsrc/components/launch/NotesToolbar.test.tsxsrc/components/launch/NotesWindow.csssrc/components/launch/NotesWindow.editable.test.tsxsrc/components/launch/NotesWindow.test.tsxsrc/components/launch/NotesWindow.tsxsrc/components/launch/SourceSelector.test.tsxsrc/components/ui/gradient-editor.test.tsxsrc/components/video-editor/editorDefaults.tssrc/hooks/recorderHandle.test.tssrc/hooks/useAudioPeaks.test.tssrc/hooks/useCameraDevices.test.tssrc/hooks/useScreenRecorder.nativeStopFailure.test.tsxsrc/hooks/useScreenRecorder.tssrc/hooks/webcamAsset.test.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/transcriptionStore.test.tssrc/lib/ai-edition/store/useSequentialTimelineOps.test.tssrc/lib/ai-edition/store/useTimeline.test.tssrc/lib/ai-edition/store/useTimeline.tssrc/lib/ai-edition/timeline/duration.test.tssrc/lib/ai-edition/timeline/pointer-drag.test.tsxsrc/lib/captioning/transcribe.test.tssrc/native/hooks/useCompositorBackend.test.tssrc/native/hooks/useNativeCompositorView.test.tssrc/native/sceneDescription.test.tssrc/native/sceneDescription.tssrc/utils/platformUtils.test.tstechnical-documentation/architecture/ai-agent.mdtechnical-documentation/architecture/recording.mdtechnical-documentation/engineering/build-and-packaging.mdtechnical-documentation/engineering/ci-workflows.mdtechnical-documentation/engineering/release-and-secrets.mdtechnical-documentation/testing/writing-tests.mdtests/e2e/windows-native-checklist.spec.tsvitest.config.tsworkbench/l0/oracles.wb.tsworkbench/lib/fixtures.tsworkbench/lib/oracles.ts
🛑 Comments failed to post (11)
.github/workflows/build.yml (1)
533-542: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the current prerelease kind when selecting the previous tag.
Line 536 always searches for
-rc.N. The validation at Line 493 also accepts-beta.Nand-alpha.N. Av1.9.0-beta.2release therefore skipsv1.9.0-beta.1and can generate notes from the wrong range.Proposed fix
if [[ "$IS_PRERELEASE" == "true" ]]; then + PRERELEASE_KIND="${VERSION#*-}" + PRERELEASE_KIND="${PRERELEASE_KIND%%.*}" RC_NUMBER="${VERSION##*.}" for (( n = RC_NUMBER - 1; n >= 1; n-- )); do - CANDIDATE="v${STABLE_VERSION}-rc.${n}" + CANDIDATE="v${STABLE_VERSION}-${PRERELEASE_KIND}.${n}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if [[ "$IS_PRERELEASE" == "true" ]]; then PRERELEASE_KIND="${VERSION#*-}" PRERELEASE_KIND="${PRERELEASE_KIND%%.*}" RC_NUMBER="${VERSION##*.}" for (( n = RC_NUMBER - 1; n >= 1; n-- )); do CANDIDATE="v${STABLE_VERSION}-${PRERELEASE_KIND}.${n}" if git rev-parse -q --verify "refs/tags/${CANDIDATE}" >/dev/null; then NOTES_START_TAG="$CANDIDATE" break fi done fi🤖 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 @.github/workflows/build.yml around lines 533 - 542, Update the prerelease previous-tag search in the workflow around IS_PRERELEASE and RC_NUMBER to derive and reuse the current prerelease kind, rather than hardcoding “rc” in CANDIDATE. Preserve the existing numeric countdown and tag-existence check so alpha, beta, and rc releases select the preceding tag of the same kind.electron/ai-edition/agent-tools.ts (1)
373-375: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound or linearize batch execution.
Lines 373-375 and 441-443 accept unbounded lists. Each successful iteration at Line 721 replays a unitary executor against
current.addTrimandaddZoomthen copy the accumulated range array on every append.A large model-generated batch therefore has quadratic array-copy cost and can monopolize the agent tool loop. Preserve sequential semantics, but use a bounded work budget or an internal accumulator that avoids copying all prior successful ranges for each item.
Also applies to: 441-443, 710-735
🤖 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-tools.ts` around lines 373 - 375, Bound the batch sizes accepted by addTrimsArgs and the corresponding addZoom batch schema, and update the iteration flow around the unitary executor at the 710-735 range to enforce that limit while preserving sequential semantics. Avoid repeatedly copying the accumulated ranges on each successful addTrim or addZoom operation by using an internal accumulator or equivalent linear-time approach.electron/media/mediaLinksRegistry.test.ts (1)
280-297: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Synchronize the directory-removal race.
findMediaLinksByFingerprintfirst awaits fingerprint and registry reads. Line 291 can removemovedor the registry before the background refresh is queued. The test can then fail from the lookup itself instead of testing a detached refresh failure.Wait for an instrumented refresh write to become pending before removing
tempDir. Also assert the lookup result before releasing the write barrier.🤖 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.test.ts` around lines 280 - 297, Update the “survives the directory disappearing while the refresh is queued” test to instrument the refresh write and wait until that write is pending before removing tempDir. Assert the findMediaLinksByFingerprint lookup result while the write barrier remains held, then release the barrier and verify withoutUnhandledRejections still reports no rejections; keep the existing warning handling and cleanup.electron/native/wgc-capture/src/main.cpp (1)
1052-1060: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Publish the new deadline before the new step name.
beginStopStepwritescurrentStopStepfirst andcurrentStepDeadlineMssecond. The watchdog reads both independently. If the previous step returned just after its deadline passed — the watchdog polls only every 50 ms, so a step can finish at 8005 ms against an 8000 ms deadline — the watchdog can observe the new step name together with the old, already-expired deadline and callTerminateProcessat the very start of a step that had its full budget. Forencoder-finalizethat kills the process before the MP4 index is written.Store the deadline first. The watchdog then never sees a name paired with a stale deadline.
🔧 Proposed fix
auto beginStopStep = [&](const char* step, int budgetMs) { - currentStopStep = step; // Clamped to the ceiling: no sequence of individually-patient steps can // add up to a shutdown the app has already given up on. currentStepDeadlineMs = std::min<int64_t>(stopElapsedMs() + budgetMs, shutdownBudgetMs); + // After the deadline, never before: the watchdog reads the two + // separately, and a new name paired with the previous step's expired + // deadline would abandon a step that still had its full budget. + currentStopStep = step; std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs() << " phase=begin" << std::endl; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.auto beginStopStep = [&](const char* step, int budgetMs) { // Clamped to the ceiling: no sequence of individually-patient steps can // add up to a shutdown the app has already given up on. currentStepDeadlineMs = std::min<int64_t>(stopElapsedMs() + budgetMs, shutdownBudgetMs); // After the deadline, never before: the watchdog reads the two // separately, and a new name paired with the previous step's expired // deadline would abandon a step that still had its full budget. currentStopStep = step; std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs() << " phase=begin" << std::endl; };🤖 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/wgc-capture/src/main.cpp` around lines 1052 - 1060, Update the beginStopStep lambda so it assigns currentStepDeadlineMs before publishing the new currentStopStep value. Preserve the existing deadline calculation and timing log, ensuring the watchdog cannot observe a new step name paired with the previous expired deadline.electron/native/wgc-capture/src/wgc_session.cpp (1)
278-282: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A retry after a failed drain returns true without closing the session.
quiesced_is set before the drain. If the drain times out, the function returns false withsession_andframePool_still open. A later call —stop(), or~WgcSession()viastop()— takes the early return at Line 279. If the callback has finished by then, it returnstrue, andstop()releases the D3D device while the capture session and frame pool were never closed. The header documentsquiesceCaptureas idempotent, so a second call that reports success must have completed the close.Latch the flag only once the close block has run. Then a retry redoes the drain and the close.
🔧 Proposed fix
bool WgcSession::quiesceCapture(int drainTimeoutMs) { - if (quiesced_) { + // Only a call that got all the way through the close below counts as + // quiesced. A drain that timed out left session_/framePool_ open, so a + // retry has to run the close rather than report success on its behalf. + if (closed_) { return callbacksInFlight_.load() == 0; } - quiesced_ = true; + if (!handlerRevoked_) { + handlerRevoked_ = true; + // ... revoke + clear frameCallback_ (once is enough) ... + }Add the two flags to
wgc_session.hin place ofquiesced_, and setclosed_ = trueimmediately beforereturn true;at Line 339. Keep the revoke and theframeCallback_clear guarded byhandlerRevoked_so a retry does not revoke a token twice.🤖 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/wgc-capture/src/wgc_session.cpp` around lines 278 - 282, Replace the single quiesced_ state in WgcSession with separate closed_ and handlerRevoked_ flags. Update quiesceCapture so retries after a drain timeout repeat the drain and close sequence, while the early return only reports success when closed_ is true; set closed_ immediately before the successful return. Guard callback-handler revocation and frameCallback_ clearing with handlerRevoked_ to avoid repeating those operations.scripts/test-windows-wgc-helper.mjs (1)
60-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The stop latency budget can fail a helper that is still working.
The comment says a healthy stop is well under a second, and the value is 15 s. Neither matches the helper's contract.
main.cppgivesencoder-finalizethe wholeOPENSCREEN_WGC_STOP_BUDGET_MSceiling on purpose, because a long recording through the software encoder legitimately spends seconds inIMFSinkWriter::Finalize— that is issue#34, cited in theSTOP_BUDGET_MScomment directly above. A run that finalizes for 20 s therefore stops cleanly and still throws here.The harness already asserts the real invariant:
stopHungcatches a helper that never ended itself, and the required steps catch an incomplete sequence. Either raise this budget aboveSTOP_BUDGET_MS, or apply it to the pre-finalize steps only and correct the comment to state the value it enforces.🔧 Proposed fix
-/** A healthy stop is well under a second. */ -const STOP_LATENCY_BUDGET_MS = 15_000; +/** + * A stop that finished but took longer than the helper's own ceiling means the + * ceiling was not enforced. Kept above STOP_BUDGET_MS so a legitimate slow + * `encoder-finalize` (issue `#34`) is not reported as a failure. + */ +const STOP_LATENCY_BUDGET_MS = STOP_BUDGET_MS;Also applies to: 168-172
🤖 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/test-windows-wgc-helper.mjs` around lines 60 - 61, Update STOP_LATENCY_BUDGET_MS and its comment in the test harness so a legitimate encoder-finalize lasting up to the configured OPENSCREEN_WGC_STOP_BUDGET_MS does not fail the helper; either set the overall budget above STOP_BUDGET_MS or limit this timeout to pre-finalize steps. Preserve the existing stopHung and required-step assertions as the checks for hung or incomplete helpers.src/hooks/useScreenRecorder.nativeStopFailure.test.tsx (1)
20-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin
process.platformtowin32in this Windows-specific suite.
stubElectronAPIonly controlswindow.electronAPI.getPlatform(). Direct platform checks in the hook dependency chain can still use the host platform. Setprocess.platforminbeforeEachand restore it inafterEach.Proposed fix
const SOURCE = { id: "screen:0:0", name: "Screen 1", display_id: "1", thumbnail: "" }; +const REAL_PLATFORM = process.platform; + +const setPlatform = (value: NodeJS.Platform) => + Object.defineProperty(process, "platform", { value, configurable: true }); let api: Record<string, ReturnType<typeof vi.fn>>; beforeEach(() => { vi.useFakeTimers(); + setPlatform("win32"); stubElectronAPI(); }); afterEach(() => { + setPlatform(REAL_PLATFORM); vi.useRealTimers(); vi.restoreAllMocks(); });As per coding guidelines, “Pin
process.platformin platform-conditional tests so CI does not hide Windows/macOS failures.”Also applies to: 72-80
🤖 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 `@src/hooks/useScreenRecorder.nativeStopFailure.test.tsx` around lines 20 - 41, Pin process.platform to "win32" in the native Windows test suite’s beforeEach and restore the original platform value in afterEach. Keep stubElectronAPI focused on Electron API mocks, and ensure cleanup runs after every test so the host platform is not altered for other suites.Source: Coding guidelines
src/hooks/useScreenRecorder.ts (1)
1200-1211: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up the local webcam recorder when native startup does not transfer ownership.
Both paths create
nativeWebcamRecorderbefore native startup completes. If startup throws, or the countdown becomes inactive after startup, the function returns before assigning that recorder towebcamRecorder.current. The outer cleanup only clears refs and media tracks. It cannot callRecorderHandle.discard(). With streaming enabled, this can leave a partial webcam file and an active recorder.
src/hooks/useScreenRecorder.ts#L1200-L1211: Track whether the macOS path transfers ownership. If it does not, stop and drain the local recorder, then calldiscard().src/hooks/useScreenRecorder.ts#L1392-L1401: Apply the same cleanup for the Linux path.📍 Affects 1 file
src/hooks/useScreenRecorder.ts#L1200-L1211(this comment)src/hooks/useScreenRecorder.ts#L1392-L1401🤖 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 `@src/hooks/useScreenRecorder.ts` around lines 1200 - 1211, Track ownership transfer for the local recorder created in the macOS path around createRecorderHandle, and when startup fails or the countdown becomes inactive before webcamRecorder.current receives it, stop and drain the recorder before calling discard(). Apply the same ownership tracking and cleanup to the Linux path around its createRecorderHandle call in src/hooks/useScreenRecorder.ts:1392-1401; ensure both sites preserve the existing cleanup when ownership is transferred.src/native/sceneDescription.test.ts (1)
1347-1347: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate
areadeclarations.Line 1347 declares the same
const areathree times in one test scope. TypeScript reports a block-scoped redeclaration error. Keep one declaration.Proposed fix
const blocked = byClip[0]?.screenRect; const alone = byClip[1]?.screenRect; if (!blocked || !alone) throw new Error("entree manquante"); // Meme source (1920x1080) des deux cotes : la seule difference est la camera. const area = (r: { width: number; height: number }) => r.width * r.height; - const area = (r: { width: number; height: number }) => r.width * r.height; - const area = (r: { width: number; height: number }) => r.width * r.height; expect(area(alone)).toBeGreaterThan(area(blocked) * 1.2);🤖 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 `@src/native/sceneDescription.test.ts` at line 1347, In the affected test scope, remove the duplicate const area declarations and retain a single area helper declaration for the test’s calculations. Ensure all existing usages continue referencing that remaining declaration without changing its behavior.technical-documentation/testing/writing-tests.md (1)
21-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the unit-test environment and file-pattern guidance.
Do not require jsdom for every
*.test.tsxfile. Use jsdom only when the test needs DOM APIs. Match the documented pattern tovitest.config.ts, which includestestandspecfiles across JavaScript and TypeScript extensions.Proposed fix
-**File pattern:** `{src,electron,.github}/**/*.test.{ts,tsx}` +**File pattern:** `{src,electron,.github}/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}` -That is also the fix when a new test dies on `document is not defined` or -`ReferenceError: window is not defined`. Every `*.test.tsx` needs it; a `*.test.ts` needs -it only if it renders a component, uses `renderHook`, or reaches for a browser global. +Use the docblock when a test needs `document`, `window`, Testing Library rendering, +`renderHook`, or another browser API. File extension alone does not require jsdom.As per coding guidelines, “Use the default
nodeVitest environment unless the test explicitly needs the DOM.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.**Config:** `vitest.config.ts` **Runs in:** Node by default; jsdom only for files that ask for it **File pattern:** `{src,electron,.github}/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}` **CI command:** `npm run test` Use unit tests for pure logic, utility functions, data transformations, and React behavior. ### Environment: node by default, jsdom on request Building a jsdom for a test that never touches the DOM was this suite's single largest cost — 719s of cumulative environment setup against 89s of actual test time. So `vitest.config.ts` sets `environment: "node"`, and the 37 files that genuinely need a DOM opt in with a docblock on line 1:🤖 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 `@technical-documentation/testing/writing-tests.md` around lines 21 - 42, Update the testing guidance to state that jsdom is required only for tests using DOM APIs, including applicable *.test.tsx files, rather than every TSX test. Revise the documented file pattern to match vitest.config.ts by covering test and spec files across JavaScript and TypeScript extensions, while retaining node as the default environment and the explicit jsdom opt-in guidance.Source: Coding guidelines
workbench/lib/oracles.ts (1)
297-305: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed
appliedentries.The new batch branch returns
truefor a non-object entry. It also returnstruefor{}becauseclaimSurvivesaccepts results without an ID key.A successful
addTrimsoraddZoomsresult must contain a document-backed identifier for everyappliedentry. Returnfalsewhen an entry is not an object or has noID_KEYSidentifier. Otherwise this oracle accepts malformed batch results and cannot detect a broken tool response.Proposed fix
if (Array.isArray(applied)) { - return applied.every((entry) => - entry && typeof entry === "object" - ? claimSurvives(after, entry as Record<string, unknown>) - : true, - ); + return ( + applied.length > 0 && + applied.every((entry) => { + if (!entry || typeof entry !== "object") return false; + const claim = entry as Record<string, unknown>; + return ID_KEYS.some((key) => typeof claim[key] === "string") && claimSurvives(after, claim); + }) + ); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const applied = result.applied; if (Array.isArray(applied)) { return ( applied.length > 0 && applied.every((entry) => { if (!entry || typeof entry !== "object") return false; const claim = entry as Record<string, unknown>; return ID_KEYS.some((key) => typeof claim[key] === "string") && claimSurvives(after, claim); }) ); } return claimSurvives(after, result);🤖 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 `@workbench/lib/oracles.ts` around lines 297 - 305, Update the array handling in the result oracle so every applied entry must be an object containing an identifier recognized by ID_KEYS; return false for non-object entries or objects without such an identifier. Preserve claimSurvives validation for entries that pass this structural check and for non-array results.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@package.json`:
- Line 4: Synchronize the root package metadata in package-lock.json with the
1.9.0 version declared by package.json, either by regenerating the lockfile or
updating its root version fields while preserving all dependency data.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| "name": "openscreen", | ||
| "private": true, | ||
| "version": "1.8.0", | ||
| "version": "1.9.0", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Synchronize package-lock.json with the release version.
package.json declares 1.9.0, but package-lock.json still declares root version 1.8.0 at Lines 1-10. Regenerate the lockfile or update its root metadata before merging so package metadata remains consistent.
🤖 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 `@package.json` at line 4, Synchronize the root package metadata in
package-lock.json with the 1.9.0 version declared by package.json, either by
regenerating the lockfile or updating its root version fields while preserving
all dependency data.
1 participant
Sync main with the released snapshot (RC + cherry-picked bugfixes + version bump). Rebase-merged via PAT; bypass applies because EtienneLescot is a ruleset bypass actor.
Summary by CodeRabbit