fix(highlight): screen highlight clipping on macOS#237
fix(highlight): screen highlight clipping on macOS#237Andrii-Vovk wants to merge 1 commit intowebadderallorg:mainfrom
Conversation
On macOS, the screen highlight overlay's right and bottom edges were running off-screen. macOS clamps window positions below the menu bar, so the outward padding only appeared on the left/top while the right/bottom extended beyond the visible area. Use display workArea instead of full bounds for screen highlights on macOS, and keep the border/glow within the overlay window.
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
macOS Display Highlight Handler electron/ipc/handlers.ts |
Added getDisplayWorkAreaForSource() to compute display bounds using workArea. Updated show-source-highlight handler to use this function for macOS screen sources. Adjusted highlight padding from 6 to 0 and introduced conditional styling variables for borderRadius, glowInset, glowRadius, and glowPad based on platform. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
🖥️ A highlight dance on Apple's screen,
Work areas now more keen,
Borders sharp and glows subdued,
macOS styling renewed! ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | 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 (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main change: fixing screen highlight clipping on macOS, which aligns with the primary objective of the pull request. |
| Description check | ✅ Passed | The PR description comprehensively covers the issue, changes, and testing approach with clear sections matching template requirements. |
✏️ 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/ipc/handlers.ts (1)
2348-2354: Consider extracting a platform-neutral display resolver.This works, but
getDisplayWorkAreaForSource()now depends onresolveWindowsCaptureDisplay()for a macOS path. The logic is generic, so the Windows-specific name is starting to obscure what this helper really does. A neutral resolver name/module would make future platform-specific highlight work easier to follow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 2348 - 2354, The helper getDisplayWorkAreaForSource is calling resolveWindowsCaptureDisplay which is a Windows-named function but contains generic selection logic; extract and rename that logic to a platform-neutral resolver (e.g., resolveCaptureDisplay or getCaptureDisplayForSource) in a shared module, update getDisplayWorkAreaForSource to call the new neutral function, and keep any Windows-specific branching inside the resolver (guarded by platform checks) so callers no longer reference Windows-specific names like resolveWindowsCaptureDisplay.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 2348-2354: The helper getDisplayWorkAreaForSource is calling
resolveWindowsCaptureDisplay which is a Windows-named function but contains
generic selection logic; extract and rename that logic to a platform-neutral
resolver (e.g., resolveCaptureDisplay or getCaptureDisplayForSource) in a shared
module, update getDisplayWorkAreaForSource to call the new neutral function, and
keep any Windows-specific branching inside the resolver (guarded by platform
checks) so callers no longer reference Windows-specific names like
resolveWindowsCaptureDisplay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75689eaf-c853-4a55-bcf2-bdfafec3a348
📒 Files selected for processing (1)
electron/ipc/handlers.ts
|
thanks for the contribution, could you please fix merge conflicts and then it'll be good to go :) |
|
Will fix conflicts this weekend, sorry for the delay |
|
Quick triage pass: I don’t see any active review threads on this one at the moment either. It currently looks like a clean-review candidate that’s just blocked on the merge conflict, so a rebase onto current main would be the next useful step. |
Summary
On macOS, the screen highlight overlay's right and bottom edges were running off-screen. macOS clamps window positions below the menu bar, so the outward padding only appeared on the left/top while the right/bottom extended beyond the visible area.
Related to this issue - this is why i thought there wasn't any highlighting
Changes
workAreainstead of fullboundsfor screen highlights on macOS, keeping the overlay within the visible screen areaTest plan
Unfortunately i don't have a windows machine on hand to check this myself
Summary by CodeRabbit