Skip to content

Conversation

@iparaskev
Copy link
Contributor

@iparaskev iparaskev commented Feb 1, 2026

Closes #243

Summary by CodeRabbit

  • New Features

    • Enhanced multi-monitor support with improved monitor identification across platforms.
    • Added handling for window resize events to improve overlay responsiveness.
  • Bug Fixes

    • Improved error handling when screen sources become unavailable during sharing.
    • Fixed overlay window positioning to better accommodate monitor configuration changes.

✏️ Tip: You can customize this high-level summary in your review settings.

@netlify
Copy link

netlify bot commented Feb 1, 2026

Deploy Preview for hoppdocs ready!

Name Link
🔨 Latest commit 30c90a9
🔍 Latest deploy log https://app.netlify.com/projects/hoppdocs/deploys/697f300bda7484000818cf1f
😎 Deploy Preview https://deploy-preview-250--hoppdocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Feb 1, 2026

📝 Walkthrough

Walkthrough

This pull request introduces a platform-agnostic monitor identification system via a new MonitorId enum with three variants, implementing it across Linux, macOS, and Windows. The window manager is refactored to track windows by monitor ID instead of position, with enhanced handling for monitor disconnection events and improved overlay window positioning logic.

Changes

Cohort / File(s) Summary
Monitor Identification System
core/src/capture/capturer.rs, core/src/capture/linux.rs, core/src/capture/macos.rs, core/src/capture/windows.rs
Introduces MonitorId enum with variants Numeric(u32), Named(String), Position(PhysicalPosition<i32>) and implements ScreenshareExt::get_monitor_id() trait method across all platforms. Linux returns position-based ID; macOS and Windows return numeric native IDs. Platform-specific constructors and defaults also added.
Window Manager Refactoring
core/src/window_manager.rs
Replaces position-based window tracking with monitor-ID-based tracking. Introduces get_window_position_for_monitor() helper, refactors update() to reconcile windows by monitor ID with enhanced logging, clears active monitor on disconnect, and adds public is_active_window() method. Expanded error handling for new monitor-id logic.
Window Event and Positioning Updates
core/src/lib.rs
Refactors overlay window positioning to compute window_outer_position upfront; adds explicit WindowEvent::Resized handling to update WindowManager; changes window_event parameter from _window_id to window_id.
Documentation
core/CLAUDE.md
New placeholder file created.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • fix: wrong display position #241: Modifies the same window management and overlay positioning code (core/src/window_manager.rs and core/src/lib.rs), likely related through shared window/monitor handling refactoring.

Poem

🐰 A rabbit hops through monitors with glee,
With IDs not positions, now handles disconnect with care,
When plugged or unplugged, no crash shall we see,
The window manager bounces from monitor to monitor fair! 🎪✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: plug unplug monitor' directly addresses the core issue in the changeset: handling monitor plug/unplug events gracefully.
Linked Issues check ✅ Passed The PR implements error handling for unplugged monitors [#243] and tracking monitors by ID to prevent window placement errors when displays are added/removed.
Out of Scope Changes check ✅ Passed All changes are directly aligned with fixing monitor plug/unplug handling and window positioning: error handling for missing monitors, monitor ID-based tracking, window event handling, and graceful degradation.
Docstring Coverage ✅ Passed Docstring coverage is 94.74% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_plug_unplug_monitor

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.

❤️ Share

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

@iparaskev iparaskev merged commit 2d36990 into main Feb 1, 2026
19 checks passed
@iparaskev iparaskev deleted the fix_plug_unplug_monitor branch February 1, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Unplugging/Plugging monitor without closing share

2 participants