fix(linux): stabilize portal source selection HUD#626
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a source-selection-active IPC method and state to lock the HUD during display/source selection. Main process passthrough and fallback logic now respect the lock; renderer hook activates/deactivates the lock during recording display selection. Tests and type declarations updated accordingly. ChangesHUD Source Selection Locking
Sequence DiagramsequenceDiagram
participant Renderer as useScreenRecorder
participant Preload as electronAPI (preload)
participant Main as MainProcess (windows.ts)
Renderer->>Preload: hudOverlaySetSourceSelectionActive(true)
Preload->>Main: ipcMain 'hud-overlay-set-source-selection-active' true
Main->>Main: set hudOverlaySourceSelectionActive = true\ncollapse fallback, setIgnoreMouseEvents(false)
Renderer->>Preload: hudOverlaySetSourceSelectionActive(false)
Preload->>Main: ipcMain 'hud-overlay-set-source-selection-active' false
Main->>Main: restore passthrough via setHudOverlayMousePassthrough(lastIntent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
electron/preload.ts (1)
904-904: 💤 Low valueVerify the null coalescing addition is intentional.
The change from
result.valueto(result.value ?? null)makes null-handling more explicit. Ifresult.valuecan beundefined, this ensuresnullis consistently returned instead. Confirm this subtle behavior clarification was intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/preload.ts` at line 904, The change wraps result.value with nullish coalescing in the return expression (return result?.success ? (result.value ?? null) : null), which alters behavior by converting undefined to null; decide whether you intended to normalize undefined to null—if yes, keep (result.value ?? null) and update the surrounding type annotations and any callers to expect null instead of undefined; if not, revert to returning result.value directly (return result?.success ? result.value : null) so existing callers that rely on undefined aren’t affected. Ensure whichever choice you make is reflected in the function's return type and add or update tests calling this return path.src/hooks/useScreenRecorder.ts (1)
1651-1651: ⚡ Quick winInconsistent error handling for native recording path.
Line 2017 wraps
setRecordingState(true)in try/catch for the browser recording path, but this native recording path at line 1651 does not. If the IPC call exists but fails, it could throw and break recording start. For consistency and resilience, consider wrapping this call similarly:+try { + await window.electronAPI?.setRecordingState(true); +} catch (stateError) { + console.warn("Failed to notify main process that recording started:", stateError); +} -window.electronAPI?.setRecordingState(true);Note: Other
setRecordingStatecalls (lines 1174, 1293, 1389, 2030, 2158, 2179) also lack try/catch, but those are stop/cleanup calls where failure is less critical than start.🤖 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` at line 1651, Wrap the native recording path's IPC call window.electronAPI?.setRecordingState(true) in a try/catch like the browser-start path to avoid throwing if the IPC exists but fails; catch the error, log it using the existing logger (or console.error) with context (e.g., "setRecordingState(true) failed during native start") and proceed without letting the exception abort the recording start flow so the start sequence remains resilient.
🤖 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.
Nitpick comments:
In `@electron/preload.ts`:
- Line 904: The change wraps result.value with nullish coalescing in the return
expression (return result?.success ? (result.value ?? null) : null), which
alters behavior by converting undefined to null; decide whether you intended to
normalize undefined to null—if yes, keep (result.value ?? null) and update the
surrounding type annotations and any callers to expect null instead of
undefined; if not, revert to returning result.value directly (return
result?.success ? result.value : null) so existing callers that rely on
undefined aren’t affected. Ensure whichever choice you make is reflected in the
function's return type and add or update tests calling this return path.
In `@src/hooks/useScreenRecorder.ts`:
- Line 1651: Wrap the native recording path's IPC call
window.electronAPI?.setRecordingState(true) in a try/catch like the
browser-start path to avoid throwing if the IPC exists but fails; catch the
error, log it using the existing logger (or console.error) with context (e.g.,
"setRecordingState(true) failed during native start") and proceed without
letting the exception abort the recording start flow so the start sequence
remains resilient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 582829c1-008e-4e51-8585-b18ba36019fa
📒 Files selected for processing (7)
electron/electron-env.d.tselectron/hudOverlayBounds.test.tselectron/hudOverlayBounds.tselectron/preload.tselectron/windows.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.ts
82a94db to
c0627d0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/windows.ts (1)
314-333: ⚡ Quick winDrop the redundant
interactionLockedargument inshouldResizeHudOverlayFallbackcall
electron/windows.tspasseshudOverlaySourceSelectionActiveasinteractionLocked, but thehudOverlaySourceSelectionActiveearly return runs before this call—sointeractionLockedis alwaysfalseon this runtime path (theinteractionLocked=truebehavior is only covered byelectron/hudOverlayBounds.test.ts). Simplify by omitting the 3rd argument and relying on the default.♻️ Proposed simplification
const mousePassthroughSupported = isHudOverlayMousePassthroughSupported(); if (!mousePassthroughSupported) { - if ( - shouldResizeHudOverlayFallback( - mousePassthroughSupported, - hudOverlayRecordingActive, - hudOverlaySourceSelectionActive, - ) - ) { + // Source selection is handled by the early return above, so the lock + // flag is always false here; resizing is gated only by recording state. + if (shouldResizeHudOverlayFallback(mousePassthroughSupported, hudOverlayRecordingActive)) { setHudOverlayFallbackExpanded(!ignore); } hudOverlayWindow.setIgnoreMouseEvents(false); return; }🤖 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/windows.ts` around lines 314 - 333, The call to shouldResizeHudOverlayFallback in electron/windows.ts is passing hudOverlaySourceSelectionActive as the third argument even though an early return above makes that value irrelevant; remove the third argument and call shouldResizeHudOverlayFallback(mousePassthroughSupported, hudOverlayRecordingActive) so the function uses its default for interactionLocked; update the invocation near hudOverlayWindow.setIgnoreMouseEvents(false) and confirm the function signature of shouldResizeHudOverlayFallback supports the default parameter.
🤖 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.
Nitpick comments:
In `@electron/windows.ts`:
- Around line 314-333: The call to shouldResizeHudOverlayFallback in
electron/windows.ts is passing hudOverlaySourceSelectionActive as the third
argument even though an early return above makes that value irrelevant; remove
the third argument and call
shouldResizeHudOverlayFallback(mousePassthroughSupported,
hudOverlayRecordingActive) so the function uses its default for
interactionLocked; update the invocation near
hudOverlayWindow.setIgnoreMouseEvents(false) and confirm the function signature
of shouldResizeHudOverlayFallback supports the default parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dacc3f35-d882-4536-85e0-5e5f05bbcfce
📒 Files selected for processing (7)
electron/electron-env.d.tselectron/hudOverlayBounds.test.tselectron/hudOverlayBounds.tselectron/preload.tselectron/windows.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- electron/electron-env.d.ts
- electron/hudOverlayBounds.ts
- electron/preload.ts
- src/hooks/useScreenRecorder.test.ts
- electron/hudOverlayBounds.test.ts
- src/hooks/useScreenRecorder.ts
Summary
Fixes #623.
Research notes
Verification
Risk
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests