Skip to content

fix(windows): honor selected window in native capture#421

Open
crbender wants to merge 1 commit intowebadderallorg:mainfrom
crbender:fix/windows-native-window-selector-capture
Open

fix(windows): honor selected window in native capture#421
crbender wants to merge 1 commit intowebadderallorg:mainfrom
crbender:fix/windows-native-window-selector-capture

Conversation

@crbender
Copy link
Copy Markdown

@crbender crbender commented May 4, 2026

Description

Fixes Windows native capture so selecting a window:* source records that window instead of falling back to full-display capture.

Motivation

The native WGC helper supports window capture via windowHandle, but the Windows start path was always building display-target config. This caused selected window sources to record the monitor.

Type of Change

  • New Feature
  • Bug Fix
  • Refactor / Code Cleanup
  • Documentation Update
  • Other (please specify)

Related Issue(s)

Closes #420

Testing Guide

  1. Build and run Windows x64 package.
  2. In source selector, pick a window:* source and start recording.
  3. Confirm output captures only that window.
  4. Pick a screen:* source and confirm full-display capture still works.

Checklist

  • I have performed a self-review of my code.
  • I have added any necessary screenshots or videos.
  • I have linked related issue(s) and updated the changelog if applicable.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Windows screen recording to correctly handle both window and display capture sources with accurate geometry information.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d6fb809b-483e-4a36-b3f8-c695b3e7ee4c

📥 Commits

Reviewing files that changed from the base of the PR and between 42c6cf4 and 4b9a2bc.

📒 Files selected for processing (1)
  • electron/ipc/register/recording.ts

📝 Walkthrough

Walkthrough

The PR fixes a bug where selecting a window source on Windows would record the full display instead of the specific window. Windows native start handler now parses window:* sources to extract window handles, conditionally setting either config.windowHandle or display geometry fields based on source type.

Changes

Windows Window-vs-Display Recording Configuration

Layer / File(s) Summary
Source Parsing
electron/ipc/register/recording.ts (lines 239–250)
Compute isWindowSource and parse parsedWindowHandle from source ID; derive displayBounds conditionally (null for valid window handles, resolved display bounds otherwise).
Recording Configuration
electron/ipc/register/recording.ts (lines 251–269)
Set either config.windowHandle (window source) or config.displayId + rounded config.displayX/Y/W/H (display source); remove unconditional display resolution logic.
Diagnostics
electron/ipc/register/recording.ts (updated logging)
Report displayBounds in logs only when a display was resolved, not for window-handle paths.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • webadderallorg/Recordly#314: Modifies the same Windows recording start logic in electron/ipc/register/recording.ts to handle window source differentiation.

Suggested labels

Checked

Poem

🐰 A window's trapped within the display,
But now you'll catch just what you see,
Parse the handle, route the way—
One frame recorded, wild and free! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main fix: honoring the selected window in native Windows capture instead of falling back to display capture.
Description check ✅ Passed The description covers all key template sections: a clear problem statement, motivation explaining why the fix is needed, correct bug fix type selection, linked issue reference, and comprehensive testing guide with acceptance criteria.
Linked Issues check ✅ Passed The changes fully address issue #420's requirements: parsing window sources to extract handles, setting windowHandle in config when valid, and populating display fields only for non-window sources.
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving the window capture issue on Windows; no extraneous modifications to unrelated functionality are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@yusefren
Copy link
Copy Markdown

yusefren commented May 4, 2026

Heads up that this routing fix on its own is necessary but not sufficient. While testing the same change against a locally patched build, I hit two further bugs in the WGC helper that only surface once window:* sources actually reach it:

  1. Frame-pool dimensions come from captureItem.Size(), which is often odd for windows (DWM shadow margin, fractional DPI). The encoder rounds to even for H.264 alignment, so CopyResource then silently fails against the staging texture and the recording comes out fully black. Reproduces against Windows Terminal and other windows whose pixel size happens to be odd.
  2. IncludeSecondaryWindows is never enabled on IGraphicsCaptureSession6, so popups, dialogs, and tooltips owned by the captured window are missing on Windows 11 24H2+.

Both fixes are small (a few lines in electron/native/wgc-capture/src/wgc_session.cpp) and live in #422 along with the same routing change. Happy to defer to whatever the maintainer prefers: merge this and I'll rebase #422 down to just the C++ fixes, or merge #422 in place of this and close. Whichever lands, the C++ fixes are needed before window capture is actually usable end-to-end.

Related: #420, #423.

@meiiie
Copy link
Copy Markdown
Collaborator

meiiie commented May 7, 2026

Quick triage pass: this is a useful narrow fix for the normal window:* path, but I still see one blocker before it should be considered merge-ready.

If the selected source starts with window: and the handle is stale or cannot be parsed, this branch falls back to display capture. That can recreate the privacy-sensitive failure mode where Recordly records the whole screen even though the user selected a window. The safer behavior is to fail the native window capture explicitly and let a higher-level fallback decide what to do.

There is also overlap with #422, which covers that invalid-window guard and adds native helper fixes for odd initial window dimensions. I would treat this PR as superseded or update it with the same guard before another review pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows: selected window source records full display instead of the window

3 participants