feat: recording screen area and Media Foundation video encoder and s…#156
Closed
mahdyarief wants to merge 7 commits intowebadderallorg:mainfrom
Closed
feat: recording screen area and Media Foundation video encoder and s…#156mahdyarief wants to merge 7 commits intowebadderallorg:mainfrom
mahdyarief wants to merge 7 commits intowebadderallorg:mainfrom
Conversation
…reen capture recording infrastructure
Contributor
|
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:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
IPC types & preload electron/electron-env.d.ts, electron/preload.ts |
Expose new area APIs: openAreaSelector, cancelAreaSelector, setSelectedArea, getSelectedArea, onAreaHighlightData (with unsubscribe). |
Main IPC handlers & orchestration electron/ipc/handlers.ts, electron/main.ts |
Add selectedArea state, area:custom pseudo-source, handlers for open/cancel/set/get selected area, lifecycle management for countdown and area highlight overlay, persist selected area, propagate cursor hidden telemetry, and update IPC handler signatures to include area selector factory. |
Window factories & positioning electron/windows.ts |
Add createAreaSelectorWindow(options?), createAreaHighlightWindow(bounds), change createCountdownWindow(bounds?) to center on bounds, and adjust source-selector sizing/positioning. |
Renderer UI & routing src/App.tsx, src/components/launch/AreaSelector.tsx, src/components/launch/AreaHighlight.tsx |
Add full-screen interactive AreaSelector canvas and AreaHighlight overlay components, register highlight IPC, and route new windowType values. |
Recording UI & wiring src/components/launch/LaunchWindow.tsx, src/components/launch/SourceSelector.tsx |
Switch selectedSource to object, add Recording Mode quick switcher (Full/Area), open area selector for area mode, update labels, dropdown logic, and handle area:custom. |
Stream cropping & recorder hook src/hooks/useScreenRecorder.ts |
Add createCroppedStream for browser-side cropping, treat area:* as native-capture-eligible, and handle browser-capture cropping via getSelectedArea(). |
Cursor telemetry & playback src/components/video-editor/types.ts, src/components/video-editor/videoPlayback/cursorRenderer.ts, src/components/video-editor/timeline/zoomSuggestionUtils.ts |
Add optional hidden?: boolean to telemetry points, propagate through interpolation, hide Pixi overlay when hidden, and exclude hidden samples from dwell detection. |
Localization src/i18n/locales/en/launch.json |
Add selectArea, selectAreaPlaceholder, and recordArea localization strings. |
macOS native cropping electron/native/ScreenCaptureKitRecorder.swift |
Decode optional crop params in CaptureConfig, make output dims mutable, and apply streamConfig.sourceRect plus recalc exposed output dims when cropping is specified. |
Windows capture core & monitor utils electron/native/wgc-capture/src/main.cpp, .../monitor_utils.cpp |
Parse crop params, set DPI-awareness, improve monitor matching/fallbacks and diagnostics, clamp/coerce crop to even dims, and optionally perform GPU-side cropping via an offscreen texture. |
Windows encoder & sink changes electron/native/wgc-capture/src/mf_encoder.cpp, .../mf_encoder.h |
Enable low-latency sink attributes, add stateful PTS tracking with gap-filling, accept null textures, add writeInternalSample and markEndOfStream(). |
Windows audio loopback electron/native/wgc-capture/src/wasapi_loopback.cpp |
Track elapsed time and add silence-padding helper to write silence frames to keep audio timeline aligned. |
FFmpeg / validation & muxing electron/ipc/handlers.ts |
Adjust FFmpeg validation (stdout+stderr parsing), change failure messages, remove -map 0:v:0 and drop -shortest for Windows muxing cases. |
Miscellaneous electron/ipc/handlers.ts, electron/windows.ts, electron/main.ts |
Countdown/overlay lifecycle cleanup, DPI and cropping math for Windows WGC, IPC signature tweak to accept area selector factory. |
Sequence Diagram
sequenceDiagram
autonumber
actor User
participant Renderer as Renderer (React)
participant Preload as Preload Bridge
participant Main as Electron Main
participant AreaSel as Area Selector Window
participant Overlay as Area Highlight Window
participant Native as Native Capture (macOS/Windows)
User->>Renderer: Click "Select Area"
Renderer->>Preload: openAreaSelector(displayId?)
Preload->>Main: open-area-selector
Main->>Main: createAreaSelectorWindow(options)
Main-->>AreaSel: area-selector window shown (renderer loads)
User->>AreaSel: Draw / move selection
AreaSel->>Preload: setSelectedArea({x,y,w,h})
Preload->>Main: set-selected-area
Main->>Main: validate/convert coords, persist selectedArea
Main->>Main: createAreaHighlightWindow(selectedArea)
Main-->>Overlay: send area-highlight-data (x,y,w,h,winX,winY)
User->>Renderer: Start recording
Renderer->>Preload: start-capture with selected source/area
Preload->>Main: start-capture
Main->>Native: start native capture with crop params
Native->>Native: apply GPU crop, encode frames
Native-->>Main: deliver frames / timing
Main-->>Renderer: countdown updates / overlay signals
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly Related PRs
- webadderall/Recordly#122 — Overlapping native capture, monitor-matching, and wgc-capture/main.cpp changes; likely to intersect on cropping/DPI logic.
- webadderall/Recordly#38 — Overlaps cursor-telemetry and cursor-rendering type/logic changes.
"I nibble pixels, hop on frames,
I carve a box with gentle aims;
A glowing border, badge of cheer,
I mark the area drawing near. 🐇"
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 2.33% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The PR title is incomplete and truncated ('feat: recording screen area and Media Foundation video encoder and s…'), making it vague and unclear about the full scope of changes. | Complete the title with a clear summary of primary changes, e.g., 'feat: add screen area selection and media encoding improvements' or split into focused features. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check | ✅ Passed | The PR description is comprehensive, well-structured with clear sections (Description, Motivation, Type of Change, Related Issues, Implementation Plan), includes screenshots, and thoroughly documents the feature including multi-monitor support, IPC endpoints, and native/browser fallback implementations. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 @coderabbitai help to get the list of available commands and usage tips.
Collaborator
|
Wow, thanks for contributing! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/ipc/handlers.ts (1)
2851-2864:⚠️ Potential issue | 🟠 MajorRound-trip the new
hiddenflag through IPC too.These samples now persist
hidden, butget-cursor-telemetrystill drops that property when it normalizes JSON back intoCursorTelemetryPoints. After the recording is saved, playback will see every cursor as visible again.Suggested fix
const samples: CursorTelemetryPoint[] = rawSamples .filter((sample: unknown) => Boolean(sample && typeof sample === 'object')) .map((sample: unknown) => { const point = sample as Partial<CursorTelemetryPoint> return { timeMs: typeof point.timeMs === 'number' && Number.isFinite(point.timeMs) ? Math.max(0, point.timeMs) : 0, cx: typeof point.cx === 'number' && Number.isFinite(point.cx) ? clamp(point.cx, 0, 1) : 0.5, cy: typeof point.cy === 'number' && Number.isFinite(point.cy) ? clamp(point.cy, 0, 1) : 0.5, + hidden: point.hidden === true ? true : undefined, interactionType: point.interactionType === 'click' || point.interactionType === 'double-click' || point.interactionType === 'right-click'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 2851 - 2864, The get-cursor-telemetry IPC path is dropping the new hidden flag when serializing/deserializing cursor samples; update the IPC handlers and normalization logic to round-trip hidden. Specifically, ensure pushCursorSample (which now stores hidden) and the IPC handler that responds to "get-cursor-telemetry" (and any helper that converts raw JSON into CursorTelemetryPoint objects) include the hidden property in the serialized payload and in the CursorTelemetryPoint reconstruction so saved recordings preserve hidden state during playback.electron/native/wgc-capture/src/mf_encoder.cpp (1)
150-213:⚠️ Potential issue | 🟠 MajorGap filling currently duplicates the wrong frame.
When a real frame arrives after a gap, this code overwrites
nv12Buffer_with the new texture before the backfill loop runs. Every synthesized sample betweenlastWrittenPtsHns_andtimestampHnstherefore contains the future frame, so the picture jumps early and then freezes. Backfill with the previous buffer first, then encode the newly arrived texture at the current PTS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/native/wgc-capture/src/mf_encoder.cpp` around lines 150 - 213, The code currently decodes the incoming texture directly into nv12Buffer_ (overwriting the previous frame) before executing the backfill loop, so synthetic frames are filled with the future frame; fix by decoding the incoming texture into a separate temporary buffer (e.g., newNv12) instead of nv12Buffer_, then perform the temporal gap backfill using the existing nv12Buffer_ and writeInternalSample(nextExpectedPts) to emit duplicates of the previous frame (using lastWrittenPtsHns_), and only after backfilling copy/move newNv12 into nv12Buffer_ and write the actual current sample for timestampHns, updating lastWrittenPtsHns_ accordingly; touch symbols: stagingTexture_, nv12Buffer_, hasValidFrame_, lastWrittenPtsHns_, timestampHns, frameDurationHns, writeInternalSample().
🧹 Nitpick comments (6)
electron/native/wgc-capture/src/monitor_utils.cpp (1)
19-23: Consider making debug logging conditional.The added
std::wcerrandstd::cerrlogging in the monitor enumeration callback will produce output on every enumeration call in production builds. This could clutter stderr output, especially since enumeration happens each time monitor lookup is performed.Consider wrapping these in a debug macro or making them conditional on a verbose/debug flag.
💡 Suggested approach
+#ifdef _DEBUG std::wcerr << L" - Handle: " << hMonitor << L" Rect: " << info.x << L"," << info.y << L" " << info.width << L"x" << info.height << L" Device: " << info.deviceName << std::endl; +#endifSimilarly for line 30:
std::vector<MonitorInfo> enumerateMonitors() { +#ifdef _DEBUG std::cerr << "Enumerating monitors:" << std::endl; +#endifAlso applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/native/wgc-capture/src/monitor_utils.cpp` around lines 19 - 23, The stderr/wcerr prints inside the monitor enumeration callback (the std::wcerr and std::cerr statements that emit Handle/Rect/Device info) should be made conditional so they don't run in production; wrap those prints in a debug/verbose guard (e.g. `#ifdef` DEBUG or a project LOG_DEBUG/VERBOSE flag) or check a runtime verbose flag before printing, and apply the same guard to the other similar print on line 30; ensure you use the existing project logging facility if one exists and add any needed include or macro so release builds omit the output.electron/native/ScreenCaptureKitRecorder.swift (1)
17-20: Naming inconsistency with Windows crop parameters.The macOS
CaptureConfigusescropWidth/cropHeight, but the Windows path inhandlers.ts(lines 3684-3687) usescropW/cropH. While these are separate native implementations, maintaining consistent naming across platforms aids maintainability.Consider aligning the naming convention across platforms, or document the difference in a comment if intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/native/ScreenCaptureKitRecorder.swift` around lines 17 - 20, CaptureConfig in ScreenCaptureKitRecorder.swift declares cropWidth/cropHeight while the Windows handler uses cropW/cropH; align naming for consistency by renaming the macOS properties (or the Windows keys) so both platforms use the same identifiers, e.g., change CaptureConfig's cropWidth and cropHeight to cropW and cropH (or update handlers.ts to cropWidth/cropHeight), and update any code that references CaptureConfig, its initializer, and the deserialization/parsing code in handlers.ts to use the chosen names consistently; if you intentionally keep them different, add a concise comment near the CaptureConfig declaration and in handlers.ts explaining the platform-specific naming choice.src/components/video-editor/videoPlayback/cursorRenderer.ts (1)
1254-1256: Consider addinghiddencheck for consistency.The
drawCursorOnCanvasfunction doesn't checktarget.hidden, unlikePixiCursorOverlay.update. While context snippets show this function isn't currently used by the main rendering paths, adding the check would maintain consistency if this function is used elsewhere or in future code.♻️ Suggested fix
const target = interpolateCursorPosition(samples, timeMs); - if (!target) return; + if (!target || target.hidden) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/cursorRenderer.ts` around lines 1254 - 1256, The drawCursorOnCanvas function should skip rendering when the interpolated cursor target is marked hidden; after calling interpolateCursorPosition(samples, timeMs) and confirming target exists, add a guard that returns early if target.hidden is true (same behavior as PixiCursorOverlay.update). Update the logic around the call to interpolateCursorPosition(samples, timeMs) in drawCursorOnCanvas to check target.hidden and return without drawing when it's set.src/components/launch/LaunchWindow.tsx (1)
180-211: Consider using explicit types instead ofany.The
selectedSourcestate andlastScreenSourceRefuseanytype. Using the existingDesktopSourceinterface (or a union type) would improve type safety and IDE support.♻️ Suggested improvement
- const [selectedSource, setSelectedSource] = useState<any>({ name: "Screen" }); + const [selectedSource, setSelectedSource] = useState<DesktopSource | { name: string; id?: string }>({ name: "Screen" }); const [hasSelectedSource, setHasSelectedSource] = useState(false); const [, setRecordingsDirectory] = useState<string | null>(null); ... // Tracks the last real (non-area) screen source so we can revert from area mode - const lastScreenSourceRef = useRef<any>(null); + const lastScreenSourceRef = useRef<DesktopSource | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow.tsx` around lines 180 - 211, selectedSource and lastScreenSourceRef are typed as any; replace them with the DesktopSource type (or a DesktopSource | AreaSource union if area-mode uses a different shape) to improve type safety. Change the state declaration useState<any>({ name: "Screen" }) to useState<DesktopSource | null>(initialValue) or useState<DesktopSource | AreaSource>(initialValue) and adjust the initial object to conform to that type, and change lastScreenSourceRef from useRef<any>(null) to useRef<DesktopSource | null>(null) (or the union type). Update any code paths that set/consume selectedSource or lastScreenSourceRef to satisfy the chosen type (e.g., checks for null or area-mode discriminants).src/components/launch/SourceSelector.tsx (1)
22-52: Consider typing the source parameter.The
anytype reduces type safety. Since this function processes sources from the Electron API, consider using a more specific type.♻️ Suggested improvement
-function parseSourceMetadata(source: any) { +function parseSourceMetadata(source: { id: string; name: string; sourceType?: 'screen' | 'window'; appName?: string; windowTitle?: string }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/SourceSelector.tsx` around lines 22 - 52, The source parameter in parseSourceMetadata is typed as any; replace it with the appropriate Electron DesktopCapturerSource (or a local interface matching its fields) to regain type safety: update the function signature parseSourceMetadata(source: Electron.DesktopCapturerSource | { id: string; name: string; sourceType?: string; appName?: string; windowTitle?: string; }) and adjust usages inside (e.g., source.id, source.name, source.sourceType, source.appName, source.windowTitle) to rely on those typed properties so the compiler catches missing/optional fields; add the necessary import (or local type) and mark optional properties with ? where applicable.electron/preload.ts (1)
73-77: Consider adding explicit types for the callback parameter.The
anytypes forcallbackandpayloadare flagged by static analysis. While the existing codebase uses similar patterns, adding explicit types would improve type safety.♻️ Suggested improvement
- onAreaHighlightData: (callback: (data: any) => void) => { - const listener = (_event: Electron.IpcRendererEvent, payload: any) => callback(payload); + onAreaHighlightData: (callback: (data: { x: number; y: number; width: number; height: number; winX: number; winY: number }) => void) => { + const listener = (_event: Electron.IpcRendererEvent, payload: { x: number; y: number; width: number; height: number; winX: number; winY: number }) => callback(payload); ipcRenderer.on("area-highlight-data", listener); return () => ipcRenderer.removeListener("area-highlight-data", listener); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/preload.ts` around lines 73 - 77, The onAreaHighlightData handler uses broader any types; replace them with a concrete payload type (e.g., define and export a type/interface like AreaHighlightData) and update the function signature and internal listener to use that type: change callback: (data: any) => void to callback: (data: AreaHighlightData) => void and payload: any to payload: AreaHighlightData, keeping the existing Electron.IpcRendererEvent type for _event; update imports/exports where needed so other modules can reuse the AreaHighlightData type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 5374-5376: The area highlight overlay created by
createAreaHighlightWindow is only cleaned up from the interval callback, so when
the cancel-countdown path clears the timer the areaHighlightWindow can remain
open; update the shared cancel/cleanup path invoked by the cancel-countdown
handler to also close and null out areaHighlightWindow (call its close/destroy
method and set areaHighlightWindow = null) and ensure the same fix is applied to
the other identical cleanup sites handling the countdown cancellation so the
overlay is always removed on direct cancel.
- Around line 3379-3403: The handler ipcMain.handle('set-selected-area') must
reject rectangles spanning multiple displays: compute which displays from
screen.getAllDisplays() intersect the computed globalArea (by checking overlap
between globalArea and each display.bounds) and if more than one display has a
non-zero intersection, do not set selectedArea/selectedSource and return an
error/false result to the caller; otherwise proceed as existing (use
screen.getDisplayMatching/globalArea, set selectedArea and selectedSource).
Ensure you reference the existing symbols (globalArea, selectedArea,
selectedSource, screen.getDisplayMatching) so the check is inserted before
assigning selectedArea/selectedSource.
- Around line 3409-3417: The settings file is being written with the
overlay-local rectangle variable area instead of the screen-global rectangle;
update the persist logic so that RECORDINGS_SETTINGS_FILE stores
lastSelectedArea: globalArea (or whichever variable holds the converted global
coordinates) rather than area. Locate the block around the write to
RECORDINGS_SETTINGS_FILE in the handler (the try that reads/writes existing and
builds merged) and replace the merged assignment to use globalArea (or compute
the global rect from area before merging) so the saved lastSelectedArea matches
the global coordinates the loader expects.
In `@electron/native/ScreenCaptureKitRecorder.swift`:
- Around line 137-149: The crop-handling branch may produce odd output
dimensions for H.264 because outputWidth/outputHeight are set to Int(cw) *
scaleFactor without enforcing evenness; update the block that sets outputWidth
and outputHeight (and then streamConfig.width/height) to round/align the
computed values to even numbers (e.g., compute w = max(2, Int(cw) *
scaleFactor), then if w % 2 != 0 decrement or increment to make it even, same
for h) before assigning to outputWidth, outputHeight and
streamConfig.width/height; keep the cropRect/streamConfig.sourceRect and scaling
logic intact and reference scaleFactor, config.cropWidth, config.cropHeight,
outputWidth, outputHeight, and streamConfig.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 285-297: The code must fail-fast if CreateTexture2D fails: after
calling session.device()->CreateTexture2D(&desc, nullptr,
cropTexture.GetAddressOf()) check the returned HRESULT and if it indicates
failure, handle it by clearing needsCropping and aborting initialization (or
returning an error) so later code doesn't try to use a null cropTexture with
captureWidth/captureHeight; update the block around needsCropping, cropTexture
and session.device()->CreateTexture2D to assert or propagate the failure (log
and return/throw) when the call fails to prevent repeated runtime copy errors.
In `@src/components/launch/AreaHighlight.tsx`:
- Around line 68-78: The badge in AreaHighlight.tsx can be clipped when relY <
22 because the inline style sets top to relY - 22; update the positioning logic
in the AreaHighlight component to clamp or toggle placement: compute a safeTop =
Math.max(0, relY - 22) and use it for the style or, when relY is small, place
the badge below the selection by using top = relY + selectionHeight or a small
offset; ensure you adjust left (relX) if needed and keep the className and
visual appearance unchanged.
In `@src/components/launch/AreaSelector.tsx`:
- Around line 105-114: When moving a selection in the isMoving branch, clamp
calculated newX/newY before calling setSelection so the selection cannot go
negative or extend past the overlay; compute maxX = (overlayWidth ||
window.innerWidth) - selection.width and maxY = (overlayHeight ||
window.innerHeight) - selection.height, then set newX = Math.max(0,
Math.min(newX, maxX)) and newY = Math.max(0, Math.min(newY, maxY)) and pass
those clamped values into setSelection({...selection, x: newX, y: newY}) (use
whatever overlay size variables/refs exist in this component if present instead
of window dimensions).
In `@src/hooks/useScreenRecorder.ts`:
- Around line 286-323: createCroppedStream currently calls void video.play() and
ignores the returned Promise so play failures are swallowed; change this to
await or attach .catch to handle errors from video.play(), abort drawing and
cleanup (cancelAnimationFrame(animationFrameId), set video.srcObject = null) if
play rejects, and ensure the returned stream is not used when video playback
fails — update createCroppedStream to handle the play Promise rejection and
clean up videoTrack.stop override and animationFrameId in the error path.
- Around line 959-976: The saved custom-area path creates an `area:custom`
selectedSource without `scaleFactor` and `displayBounds`, causing fallbacks in
`useScreenRecorder` to miscompute monitor-relative pixels; when restoring the
saved area, copy `targetDisplay.scaleFactor` and `targetDisplay.bounds` (or
equivalent `displayBounds`) into the created `selectedSource` the same way the
new-area code path does so it has `scaleFactor` and `displayBounds` set before
`createCroppedStream` is called (refer to `selectedSource`, `targetDisplay`,
`scaleFactor`, `displayBounds`, `stream.current`, and `createCroppedStream` to
locate and update the code).
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2851-2864: The get-cursor-telemetry IPC path is dropping the new
hidden flag when serializing/deserializing cursor samples; update the IPC
handlers and normalization logic to round-trip hidden. Specifically, ensure
pushCursorSample (which now stores hidden) and the IPC handler that responds to
"get-cursor-telemetry" (and any helper that converts raw JSON into
CursorTelemetryPoint objects) include the hidden property in the serialized
payload and in the CursorTelemetryPoint reconstruction so saved recordings
preserve hidden state during playback.
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 150-213: The code currently decodes the incoming texture directly
into nv12Buffer_ (overwriting the previous frame) before executing the backfill
loop, so synthetic frames are filled with the future frame; fix by decoding the
incoming texture into a separate temporary buffer (e.g., newNv12) instead of
nv12Buffer_, then perform the temporal gap backfill using the existing
nv12Buffer_ and writeInternalSample(nextExpectedPts) to emit duplicates of the
previous frame (using lastWrittenPtsHns_), and only after backfilling copy/move
newNv12 into nv12Buffer_ and write the actual current sample for timestampHns,
updating lastWrittenPtsHns_ accordingly; touch symbols: stagingTexture_,
nv12Buffer_, hasValidFrame_, lastWrittenPtsHns_, timestampHns, frameDurationHns,
writeInternalSample().
---
Nitpick comments:
In `@electron/native/ScreenCaptureKitRecorder.swift`:
- Around line 17-20: CaptureConfig in ScreenCaptureKitRecorder.swift declares
cropWidth/cropHeight while the Windows handler uses cropW/cropH; align naming
for consistency by renaming the macOS properties (or the Windows keys) so both
platforms use the same identifiers, e.g., change CaptureConfig's cropWidth and
cropHeight to cropW and cropH (or update handlers.ts to cropWidth/cropHeight),
and update any code that references CaptureConfig, its initializer, and the
deserialization/parsing code in handlers.ts to use the chosen names
consistently; if you intentionally keep them different, add a concise comment
near the CaptureConfig declaration and in handlers.ts explaining the
platform-specific naming choice.
In `@electron/native/wgc-capture/src/monitor_utils.cpp`:
- Around line 19-23: The stderr/wcerr prints inside the monitor enumeration
callback (the std::wcerr and std::cerr statements that emit Handle/Rect/Device
info) should be made conditional so they don't run in production; wrap those
prints in a debug/verbose guard (e.g. `#ifdef` DEBUG or a project
LOG_DEBUG/VERBOSE flag) or check a runtime verbose flag before printing, and
apply the same guard to the other similar print on line 30; ensure you use the
existing project logging facility if one exists and add any needed include or
macro so release builds omit the output.
In `@electron/preload.ts`:
- Around line 73-77: The onAreaHighlightData handler uses broader any types;
replace them with a concrete payload type (e.g., define and export a
type/interface like AreaHighlightData) and update the function signature and
internal listener to use that type: change callback: (data: any) => void to
callback: (data: AreaHighlightData) => void and payload: any to payload:
AreaHighlightData, keeping the existing Electron.IpcRendererEvent type for
_event; update imports/exports where needed so other modules can reuse the
AreaHighlightData type.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 180-211: selectedSource and lastScreenSourceRef are typed as any;
replace them with the DesktopSource type (or a DesktopSource | AreaSource union
if area-mode uses a different shape) to improve type safety. Change the state
declaration useState<any>({ name: "Screen" }) to useState<DesktopSource |
null>(initialValue) or useState<DesktopSource | AreaSource>(initialValue) and
adjust the initial object to conform to that type, and change
lastScreenSourceRef from useRef<any>(null) to useRef<DesktopSource | null>(null)
(or the union type). Update any code paths that set/consume selectedSource or
lastScreenSourceRef to satisfy the chosen type (e.g., checks for null or
area-mode discriminants).
In `@src/components/launch/SourceSelector.tsx`:
- Around line 22-52: The source parameter in parseSourceMetadata is typed as
any; replace it with the appropriate Electron DesktopCapturerSource (or a local
interface matching its fields) to regain type safety: update the function
signature parseSourceMetadata(source: Electron.DesktopCapturerSource | { id:
string; name: string; sourceType?: string; appName?: string; windowTitle?:
string; }) and adjust usages inside (e.g., source.id, source.name,
source.sourceType, source.appName, source.windowTitle) to rely on those typed
properties so the compiler catches missing/optional fields; add the necessary
import (or local type) and mark optional properties with ? where applicable.
In `@src/components/video-editor/videoPlayback/cursorRenderer.ts`:
- Around line 1254-1256: The drawCursorOnCanvas function should skip rendering
when the interpolated cursor target is marked hidden; after calling
interpolateCursorPosition(samples, timeMs) and confirming target exists, add a
guard that returns early if target.hidden is true (same behavior as
PixiCursorOverlay.update). Update the logic around the call to
interpolateCursorPosition(samples, timeMs) in drawCursorOnCanvas to check
target.hidden and return without drawing when it's set.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d307f1f6-68b1-4074-9581-918933ff304b
📒 Files selected for processing (21)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/native/ScreenCaptureKitRecorder.swiftelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.helectron/native/wgc-capture/src/monitor_utils.cppelectron/native/wgc-capture/src/wasapi_loopback.cppelectron/preload.tselectron/windows.tssrc/App.tsxsrc/components/launch/AreaHighlight.tsxsrc/components/launch/AreaSelector.tsxsrc/components/launch/LaunchWindow.tsxsrc/components/launch/SourceSelector.tsxsrc/components/video-editor/timeline/zoomSuggestionUtils.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/hooks/useScreenRecorder.tssrc/i18n/locales/en/launch.json
Contributor
Author
welcome, i hope you like my work |
Contributor
Author
|
if i can suggest, may i do split code on your repo? handler file it going 5k line of code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/handlers.ts (1)
2865-2878:⚠️ Potential issue | 🟠 MajorFinish the
hiddenschema rollout in the read path too.This now persists
hidden, butget-cursor-telemetrystill reconstructs eachCursorTelemetryPointwithout copying that boolean back out. After a recording is reloaded, every sample loses the flag and the new off-capture cursor filtering stops working.Suggested follow-up in
get-cursor-telemetryhidden: typeof point.hidden === 'boolean' ? point.hidden : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 2865 - 2878, get-cursor-telemetry currently rebuilds each CursorTelemetryPoint but omits the persisted hidden flag, causing samples to lose the boolean after reload; update the reconstruction in get-cursor-telemetry (where CursorTelemetryPoint objects are created from stored points) to copy the hidden value using a safe check like: set hidden to point.hidden if typeof point.hidden === 'boolean', otherwise undefined, so the boolean is preserved when present and absent remains undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/native/ScreenCaptureKitRecorder.swift`:
- Around line 137-151: The crop origin must be converted from global to the
target display's local coordinates and all CGRect values should be explicit
CGFloats: compute localX = CGFloat(config.cropX) - display.bounds.origin.x and
localY = CGFloat(config.cropY) - display.bounds.origin.y, then build cropRect =
CGRect(x: localX, y: localY, width: CGFloat(config.cropWidth), height:
CGFloat(config.cropHeight)) and assign streamConfig.sourceRect = cropRect; keep
the scaled output dimension logic using scaleFactor to set
outputWidth/outputHeight and streamConfig.width/height, ensuring you still
enforce even dimensions (w % 2 checks) after converting to Ints.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 430-437: The comparison uses an unadjusted frame timeline; compute
an adjusted last-frame delta by subtracting g_accumulatedPausedHns from
(g_lastFrameTimestampHns.load() - g_firstFrameTimestampHns.load()) and compare
finalElapsedHns to that adjusted delta before calling
encoder.writeFrame(nullptr, finalElapsedHns). In other words, derive an
adjustedLastElapsedHns (using g_lastFrameTimestampHns, g_firstFrameTimestampHns
and g_accumulatedPausedHns.load()) and use that in the if-check so pause/resume
and stopping while paused do not re-append the paused tail.
In `@src/components/launch/AreaSelector.tsx`:
- Around line 142-146: In confirmSelection, don't unconditionally close the
overlay after calling window.electronAPI.setSelectedArea; instead await the
call's result (the setSelectedArea IPC), check the returned object for success
(e.g., result.success === true), only call window.close() when success is true,
and when success is false preserve the overlay and surface the error (e.g., show
a toast or validation message) so the user can adjust the selection; update
references in confirmSelection and any UI error handler to read the response
from window.electronAPI.setSelectedArea and act accordingly.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2865-2878: get-cursor-telemetry currently rebuilds each
CursorTelemetryPoint but omits the persisted hidden flag, causing samples to
lose the boolean after reload; update the reconstruction in get-cursor-telemetry
(where CursorTelemetryPoint objects are created from stored points) to copy the
hidden value using a safe check like: set hidden to point.hidden if typeof
point.hidden === 'boolean', otherwise undefined, so the boolean is preserved
when present and absent remains undefined.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d36e21d-3d09-42b5-8fdc-e35b606e502d
📒 Files selected for processing (6)
electron/ipc/handlers.tselectron/native/ScreenCaptureKitRecorder.swiftelectron/native/wgc-capture/src/main.cppsrc/components/launch/AreaHighlight.tsxsrc/components/launch/AreaSelector.tsxsrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useScreenRecorder.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/native/wgc-capture/src/main.cpp (1)
306-321:⚠️ Potential issue | 🟠 MajorHandle
resume→stopbefore the next frame arrives.
g_lastFrameTimestampHnskeeps advancing on dropped paused frames because it is updated before the pause guard. If the user resumes and stops before a new frame clearsg_resumePending, the EOS logic still compares against that paused timestamp and stretches the file past the last encoded sample. Treatg_resumePendinglike the paused case here, or materialize the pending pause before the comparison.🛠️ Minimal fix
- if (!g_pauseRequested.load() && finalElapsedHns > adjustedLastElapsedHns) { + if (!g_pauseRequested.load() && !g_resumePending.load() && + finalElapsedHns > adjustedLastElapsedHns) { encoder.writeFrame(nullptr, finalElapsedHns); }Also applies to: 430-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/native/wgc-capture/src/main.cpp` around lines 306 - 321, The problem is g_lastFrameTimestampHns is updated before the pause/resume guards so a resume→stop that happens before the next frame leaves g_resumePending stale and causes EOS timestamp comparisons to use a paused frame; fix by treating resume like pause or materializing the pending pause before comparing: move the assignment to g_lastFrameTimestampHns to after the g_stopRequested/g_pauseRequested checks (or if keeping it earlier, check g_resumePending and apply the same early-return behavior as g_pauseRequested or call the code that materializes pause state using g_pauseStartTimestampHns and g_accumulatedPausedHns before any EOS timestamp comparison), updating references in the block that handles g_resumePending so the EOS logic compares against a non-paused timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/electron-env.d.ts`:
- Line 84: Update the TypeScript declaration for setSelectedArea so its Promise
return type includes the error field used by IPC failures; replace the current {
success: boolean; message?: string } shape with one that also exposes error?:
string (or Error) so renderer code can reliably read error details when success
is false; adjust the union/shape for setSelectedArea to reflect { success:
boolean; message?: string; error?: string } (or error?: Error) to match the
handler implementation that returns error on failure.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 259-270: Validate the entire crop rectangle (config.cropX,
config.cropY, config.cropW, config.cropH) before using it to set
captureWidth/captureHeight or needsCropping: ensure cropW and cropH are >0, even
(match the UI floor to even), and that cropX/cropY plus cropW/cropH lie fully
inside the source bounds (monitorWidth/monitorHeight); if the rectangle is
invalid (out-of-bounds or larger than the source) reject it (return an error or
fall back to full-monitor capture explicitly) instead of silently clamping
left/top or treating zero-sized crops as full-monitor — update the logic around
the captureWidth/captureHeight assignment and the needsCropping computation (and
apply the same guard at the other occurrence around lines 331-348) so the
encoder is only initialized with a validated crop rectangle.
---
Outside diff comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 306-321: The problem is g_lastFrameTimestampHns is updated before
the pause/resume guards so a resume→stop that happens before the next frame
leaves g_resumePending stale and causes EOS timestamp comparisons to use a
paused frame; fix by treating resume like pause or materializing the pending
pause before comparing: move the assignment to g_lastFrameTimestampHns to after
the g_stopRequested/g_pauseRequested checks (or if keeping it earlier, check
g_resumePending and apply the same early-return behavior as g_pauseRequested or
call the code that materializes pause state using g_pauseStartTimestampHns and
g_accumulatedPausedHns before any EOS timestamp comparison), updating references
in the block that handles g_resumePending so the EOS logic compares against a
non-paused timestamp.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 232da344-47d7-4a92-9e69-a644e5cdd079
📒 Files selected for processing (4)
electron/electron-env.d.tselectron/native/ScreenCaptureKitRecorder.swiftelectron/native/wgc-capture/src/main.cppsrc/components/launch/AreaSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/launch/AreaSelector.tsx
- electron/native/ScreenCaptureKitRecorder.swift
webadderall
reviewed
Apr 2, 2026
Collaborator
There was a problem hiding this comment.
Blocking Issues
macOS: Area selector doesn't work
- Y-coordinate not flipped for ScreenCaptureKit —
streamConfig.sourceRectuses bottom-left origin but coordinates from Electron are top-left. The crop region will be vertically inverted.ScreenCaptureKitRecorder.swift:139needs a Y-flip:displayBounds.height - localY - cropHeight. - No Retina (
devicePixelRatio) scaling in AreaSelector.tsx — RawclientX/clientYCSS pixels are sent as selection coordinates. On 2x Retina displays the crop area is half the intended size. Canvas also doesn't scale fordevicePixelRatio, so the overlay renders blurry. - Transparent
BrowserWindow.getBounds()unreliable on macOS —set-selected-areaconverts window-relative to global coords viawin.getBounds(), but transparent frameless windows on macOS can report incorrect bounds.
Cross-platform
selectedSourcetyped asany—LaunchWindow.tsxchangeduseState("Screen")touseState<any>({ name: "Screen" }). Should be a proper interface.set-selected-arearesponse key mismatch — Handler returns{ success: false, error: ... }but the TypeScript type declares{ success: boolean; message?: string }.markEndOfStreamsends stream tick at timestamp 0 — Should be the final PTS, not 0.sinkWriter_->SendStreamTick(streamIndex_, 0)may confuse the sink writer after frames at later timestamps.confirmSelectionstale closure —AreaSelector.tsxuseEffectkeydown handler capturesselectionbutconfirmSelectionisn'tuseCallback-wrapped, so pressing Enter can fire with stale/null selection.videoTrack.stopmonkey-patched increateCroppedStream— If the browser revokes the track (e.g. permission change), the cleanup lambda won't run.
Also, please change the recording mode from a slider toggle to a button for "select area"
Contributor
Author
Implementation Plan - Screen Recording & UI HardeningThis plan outlines the steps to resolve the critical blocking issues identified in the screen recording infrastructure and implement the requested UI refinements. 1. macOS Coordinate & Scaling Fixes
2. Cross-Platform Reliability
3. UI/UX Refinements
Verification Plan
|
…xes & Cross-Platform Reliability
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/launch/AreaSelector.tsx (1)
167-178: Consider addingconfirmSelectionto the dependency array.The
useEffectcallsconfirmSelectionbut only depends on[selection]. While this works becauseconfirmSelectionis rebuilt whenselectionchanges, explicitly including the callback in the dependency array is more robust and aligns with exhaustive-deps lint rules.useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { if (e.key === "Escape") { window.electronAPI.cancelAreaSelector(); } else if (e.key === "Enter") { confirmSelection(); } }; window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); - }, [selection]); + }, [confirmSelection]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/AreaSelector.tsx` around lines 167 - 178, The effect registers a keydown handler that calls confirmSelection but only lists [selection] as a dependency; add confirmSelection to the useEffect dependency array so the effect is recreated whenever the confirmSelection callback changes. Update the useEffect that defines handleKeyDown (which calls window.electronAPI.cancelAreaSelector and confirmSelection) to depend on [selection, confirmSelection] (or just [confirmSelection] if selection is captured inside confirmSelection) to satisfy exhaustive-deps and ensure the latest callback is used.src/components/launch/LaunchWindow.tsx (1)
210-211: Replaceanywith properDesktopSource | nulltype.The
lastScreenSourceRefandapplySelectedSourceparameter useany, but theDesktopSourceinterface is already defined in this file (lines 47-56).Proposed fix
// Tracks the last real (non-area) screen source so we can revert from area mode - const lastScreenSourceRef = useRef<any>(null); + const lastScreenSourceRef = useRef<DesktopSource | null>(null);And at line 341:
- const applySelectedSource = (source: any | null | undefined) => { + const applySelectedSource = (source: DesktopSource | null | undefined) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow.tsx` around lines 210 - 211, Replace the loose any types with the defined DesktopSource type: change the ref declaration lastScreenSourceRef to use useRef<DesktopSource | null>(null) and update the applySelectedSource parameter type from any to DesktopSource | null (or DesktopSource where appropriate) so the ref and function signatures consistently use DesktopSource and null instead of any; ensure imports/definitions for DesktopSource in this file are used, and update any related usages to satisfy the narrower type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 154-158: The JSON key names for crop width/height are
inconsistent: main.cpp reads config.cropW/config.cropH via
findInt("cropW")/findInt("cropH") while the custom-area path in handlers.ts
writes cropWidth/cropHeight, causing silent failure; update the parsing in
main.cpp (around config.cropX/Y/W/H and the findInt calls) to accept the
alternate names (e.g., if findInt("cropW")/findInt("cropH") return -1, fall back
to findInt("cropWidth")/findInt("cropHeight") or otherwise normalize both keys
into config.cropW and config.cropH) so both handlers.ts code paths populate the
crop config correctly.
- Around line 348-370: Declare and initialize inputTexture before the crop block
so it exists when calling encoder.writeFrame; specifically, add a variable named
inputTexture with the same type expected by encoder.writeFrame (e.g.,
ID3D11Texture2D* or ComPtr<ID3D11Texture2D>) initialized to the original
texture, then inside the cropTexture branch set inputTexture = cropTexture.Get()
(or assign the ComPtr) after the CopySubresourceRegion call so
encoder.writeFrame(inputTexture, adjustedTimestampHns) compiles.
In `@src/components/launch/AreaSelector.tsx`:
- Around line 147-165: The confirmSelection callback currently allows selections
where either width or height meets the minimum (selection.width >= 10 ||
selection.height >= 10); change this to require both dimensions (selection.width
>= 10 && selection.height >= 10) so tiny one-dimensional areas are rejected.
Update the conditional inside the useCallback for confirmSelection (which calls
window.electronAPI.setSelectedArea and uses toast.error/window.close) to use &&
with the existing selection check and keep the rest of the logic unchanged.
- Around line 149-156: The selection is being double-scaled: AreaSelector.tsx
multiplies selection by devicePixelRatio, then downstream consumers use
scaleFactor/sf again (in useScreenRecorder.ts and handlers.ts), causing a 2x2
overscale; fix by removing DPR scaling in AreaSelector.tsx so it sends the
selection in CSS/DIP coordinates (remove the multiplication by
window.devicePixelRatio and the scaledSelection object), leaving any DPI ->
physical pixel conversion to the capture backends in useScreenRecorder.ts (where
sf is applied) and handlers.ts; update related comments in AreaSelector.tsx to
state that the area is in DIP/CSS pixels and ensure consumers still expect DIP
input.
---
Nitpick comments:
In `@src/components/launch/AreaSelector.tsx`:
- Around line 167-178: The effect registers a keydown handler that calls
confirmSelection but only lists [selection] as a dependency; add
confirmSelection to the useEffect dependency array so the effect is recreated
whenever the confirmSelection callback changes. Update the useEffect that
defines handleKeyDown (which calls window.electronAPI.cancelAreaSelector and
confirmSelection) to depend on [selection, confirmSelection] (or just
[confirmSelection] if selection is captured inside confirmSelection) to satisfy
exhaustive-deps and ensure the latest callback is used.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 210-211: Replace the loose any types with the defined
DesktopSource type: change the ref declaration lastScreenSourceRef to use
useRef<DesktopSource | null>(null) and update the applySelectedSource parameter
type from any to DesktopSource | null (or DesktopSource where appropriate) so
the ref and function signatures consistently use DesktopSource and null instead
of any; ensure imports/definitions for DesktopSource in this file are used, and
update any related usages to satisfy the narrower type.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f30c185d-dbe2-4a0f-9eee-55b6d72cc06e
📒 Files selected for processing (7)
electron/electron-env.d.tselectron/native/ScreenCaptureKitRecorder.swiftelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppsrc/components/launch/AreaSelector.tsxsrc/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/native/wgc-capture/src/mf_encoder.cpp
…, thumbnails, preview and export rendering
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/ipc/handlers.ts (1)
2651-2656:⚠️ Potential issue | 🟠 MajorComplete the
hiddenflag plumbing throughget-cursor-telemetry.These samples now capture/persist
hidden, but the read path still strips that property during deserialization. After a save/reload, hidden-cursor transitions disappear even though the sidecar JSON contains them.Also applies to: 2871-2877
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 2651 - 2656, The deserialization path for cursor telemetry is dropping the optional hidden flag so saved hidden transitions are lost; update the get-cursor-telemetry read/deserialization logic to preserve and populate the hidden property into CursorTelemetryPoint objects (ensure any parser or mapper used by get-cursor-telemetry reads the hidden boolean from stored samples and assigns it to each CursorTelemetryPoint), and apply the same fix to the analogous deserialization block referenced around the other occurrence (the block at ~2871-2877) so both read paths consistently carry the hidden flag through.electron/native/wgc-capture/src/main.cpp (1)
341-345:⚠️ Potential issue | 🟠 MajorStopping right after resume still reintroduces the paused gap.
g_accumulatedPausedHnsis only advanced on the first post-resume frame. If the screen stays static and stop happens before that frame arrives,finalElapsedHnsstill contains the just-finished pause and the terminal frame stretches the video by that paused tail.Also applies to: 454-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/native/wgc-capture/src/main.cpp` around lines 341 - 345, The bug is that g_accumulatedPausedHns is only updated on the first frame after resume (g_resumePending.exchange(false)), so if capture stops before that frame the paused interval is never added and finalElapsedHns includes the pause; fix by adding the same pause-accounting logic in the stop/teardown path (the code that computes finalElapsedHns) and also ensure the resume branch always clears g_pauseStartTimestampHns and g_resumePending after adding the paused delta; specifically, in the g_resumePending handling (where g_pauseStartTimestampHns is read) ensure you add (timestampHns - pauseStart) and reset pauseStart to 0, and mirror that exact logic in the stop code path (the block that produces finalElapsedHns) so if g_resumePending is set at stop you compute and add (stopTimestampHns - g_pauseStartTimestampHns.load()), update g_accumulatedPausedHns, clear g_pauseStartTimestampHns and clear g_resumePending before computing finalElapsedHns.
♻️ Duplicate comments (1)
electron/native/wgc-capture/src/main.cpp (1)
264-284:⚠️ Potential issue | 🟠 MajorReject malformed crop rectangles instead of silently snapping to a different capture.
Missing/negative
cropXorcropYare normalized to0, and any invalid rectangle just falls back to full-monitor capture. That means stale settings or bad IPC input can record a different region than the user selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/native/wgc-capture/src/main.cpp` around lines 264 - 284, The code currently normalizes negative cropX/cropY to 0 and silently falls back to full-monitor capture; instead, treat any malformed crop rectangle as invalid and reject it without snapping. Use the raw config.cropX and config.cropY (do not coerce to 0) when validating the rectangle: require config.cropW>0, config.cropH>0, config.cropX>=0, config.cropY>=0, and ensure config.cropX+evenCropW<=monitorWidth and config.cropY+evenCropH<=monitorHeight; if any check fails, leave captureWidth/captureHeight unchanged, set isValidCrop=false, and emit a clear error (including the offending config values and monitor size) so the caller can handle the invalid crop (e.g., reject the capture request) rather than silently falling back.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 183-195: The pending start-countdown promise is left unresolved on
cancel because stopCountdown clears the interval before resolving the promise;
fix by introducing/using a stored resolver (e.g., countdownResolve) that
startCountdown assigns when creating the promise and ensure cancel-countdown or
stopCountdown calls countdownResolve({ success: false, cancelled: true }) (and
then nulls countdownResolve) before/when clearing countdownTimer and setting
countdownInProgress to false so any awaiting caller is always resolved on
cancellation; update stopCountdown, the cancel-countdown handler, and
startCountdown to set/clear this resolver consistently.
- Around line 3162-3179: The restored area/IPC paths trust unvalidated rectangle
data from RECORDINGS_SETTINGS_FILE and IPC, allowing zero/NaN/off-screen
rectangles to be used; add a shared validator function (e.g.,
validateRecordingArea) that checks each rect has finite numeric
x,y,width,height, width/height > 0, and that exactly one display intersects the
rect (use screen.getDisplayMatching / screen.getAllDisplays for intersection
check) and call it before assigning to selectedArea and before
creating/assigning selectedSource; if validation fails, ignore the stored/IPC
rect and fall back to existing defaults or reject the IPC message.
- Around line 3397-3405: The code currently uses
BrowserWindow.fromWebContents(event.sender) and win.getBounds() to compute
globalArea (variables: win, win.getBounds, globalArea, area), but
frameless/transparent overlay windows on macOS can drift; instead accept global
screen coordinates from the renderer (send area already in screen space) or
compute globalArea by anchoring to the original display origin used when
creating the selector window (store and use selectorWindowOrigin rather than
win.getBounds()). Update the IPC handler to trust renderer-supplied global
coordinates (or add a stored selectorWindowOrigin lookup) and remove reliance on
win.getBounds() when composing globalArea.
In `@src/components/launch/AreaSelector.tsx`:
- Around line 63-64: The minimum-size check is duplicated/misaligned: extract a
single validation (e.g., isSelectionTooSmall or isSelectionValid) that
encapsulates the rule (width < 100 || height < 100) and use it both where the
overlay label/button compute isTooSmall and inside confirmSelection(); update
confirmSelection() to early-return or reject when that helper says the selection
is too small so Enter/keyboard confirmation cannot bypass the UI rule, and
ensure the same helper is used in all places referenced (lines around
isTooSmall, overlay/button logic, and confirmSelection).
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2651-2656: The deserialization path for cursor telemetry is
dropping the optional hidden flag so saved hidden transitions are lost; update
the get-cursor-telemetry read/deserialization logic to preserve and populate the
hidden property into CursorTelemetryPoint objects (ensure any parser or mapper
used by get-cursor-telemetry reads the hidden boolean from stored samples and
assigns it to each CursorTelemetryPoint), and apply the same fix to the
analogous deserialization block referenced around the other occurrence (the
block at ~2871-2877) so both read paths consistently carry the hidden flag
through.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 341-345: The bug is that g_accumulatedPausedHns is only updated on
the first frame after resume (g_resumePending.exchange(false)), so if capture
stops before that frame the paused interval is never added and finalElapsedHns
includes the pause; fix by adding the same pause-accounting logic in the
stop/teardown path (the code that computes finalElapsedHns) and also ensure the
resume branch always clears g_pauseStartTimestampHns and g_resumePending after
adding the paused delta; specifically, in the g_resumePending handling (where
g_pauseStartTimestampHns is read) ensure you add (timestampHns - pauseStart) and
reset pauseStart to 0, and mirror that exact logic in the stop code path (the
block that produces finalElapsedHns) so if g_resumePending is set at stop you
compute and add (stopTimestampHns - g_pauseStartTimestampHns.load()), update
g_accumulatedPausedHns, clear g_pauseStartTimestampHns and clear g_resumePending
before computing finalElapsedHns.
---
Duplicate comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 264-284: The code currently normalizes negative cropX/cropY to 0
and silently falls back to full-monitor capture; instead, treat any malformed
crop rectangle as invalid and reject it without snapping. Use the raw
config.cropX and config.cropY (do not coerce to 0) when validating the
rectangle: require config.cropW>0, config.cropH>0, config.cropX>=0,
config.cropY>=0, and ensure config.cropX+evenCropW<=monitorWidth and
config.cropY+evenCropH<=monitorHeight; if any check fails, leave
captureWidth/captureHeight unchanged, set isValidCrop=false, and emit a clear
error (including the offending config values and monitor size) so the caller can
handle the invalid crop (e.g., reject the capture request) rather than silently
falling back.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18eda06f-859d-49e2-bfe8-aa8bf3f6a61b
📒 Files selected for processing (3)
electron/ipc/handlers.tselectron/native/wgc-capture/src/main.cppsrc/components/launch/AreaSelector.tsx
…ording management
Collaborator
|
Rejecting PR because previous PRs submitted by author were AI slop |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screen Recording Area
Description
This PR implements the "Select Area/Region Recording" feature, providing a robust infrastructure for capturing specific portions of the screen. It integrates a multi-monitor transparent overlay for region selection and handles the complex coordinate translation between the Electron frontend and the native Windows capture engine.
Motivation
This change is necessary to allow users to record precise areas of their desktop with native-level performance, rather than being restricted to full-monitor captures. It solves the problem of recording sensitive information or focusing only on a specific application window during a session.
Type of Change
Related Issue(s)
https://github.com/webadderall/Recordly/issues/126
Screenshots / Video
Screenshot (Transparent Selection Overlay):
Cant give screen shoot
Implementation Plan: Select Area/Region Recording
This document outlines the implementation of the "Select Area/Region Recording" feature in Recordly. It allows users to record a specific portion of their screen with precise coordinate selection and native-level performance.
1. Core Architecture Changes (Finalized)
The feature uses a multi-monitor transparent overlay for selection and communicates the region to both the browser-based recorder (fallback) and the native capture engine (primary).
IPC Interface
open-area-selector: (Implemented) Opens a transparent, full-screenBrowserWindowspanning all monitors for region selection.get-selected-area: (Implemented) Retrieves the currently active bounds in global DIP coordinates.set-selected-area: (Implemented) Persists selection and updates the HUD state.cancel-area-selector: (Implemented) Closes the selector and reverts the source.2. Implementation Details
Phase 1: Area Selection UI (✅ Complete)
AreaSelectorComponent: Located insrc/components/launch/AreaSelector.tsx.electron/windows.tscontainscreateAreaSelectorWindow.totalBoundsto span all connected displays simultaneously.Phase 2: Source Selection Integration (✅ Complete)
SourceSelector.tsx: Updated with an "Area" mode toggle and a "Select Area" trigger.LaunchWindow.tsx:"area:custom"in the recording HUD.Phase 3: Recording Logic (✅ Complete)
wgc-capture):main.cppwas updated to acceptcropX,cropY,cropW, andcropHparameters via JSON config.handlers.tsIPC layer scales the DIP (Display Independent Pixel) selection to raw monitor pixels using the target display'sscaleFactor.useScreenRecorder.tsimplements a hidden<canvas>crop fallback.3. User Experience & Multi-Monitor support
Interactive Highlighting
AreaHighlight.tsx: A subtle, pulsed blue frame appears during the countdown to confirm the recording region.winX/winYoffset system in the highlight window to ensure the overlay correctly positions itself on secondary monitors with negative coordinate spaces.Persistence
recordings-settings.jsonand restored when the user re-enters area selection mode.4. Technical Achievements
ID3D11DeviceContext::CopySubresourceRegionfor near-zero CPU overhead during native cropping on Windows.Summary by CodeRabbit
New Features
Bug Fixes
Improvements