Conversation
It seems that there is a case where the overlay window for a monitor is in the wrong place, but because we have cached the position we don't reposition it.
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe pull request refactors core graphics and window management components. It renames internal fields in GraphicsContext for clarity, extends the Drop trait to properly clean up framebuffer state, restructures window positioning logic to compute positions dynamically rather than storing them, and increments the version number. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/window_manager.rs`:
- Around line 209-223: The code currently calls
entry.window.outer_position().unwrap_or_default() which hides errors and yields
(0,0); instead match on entry.window.outer_position(): on Ok(physical_pos)
convert it to logical via .to_logical(monitor.scale_factor()) and compare to
get_window_position_for_monitor(monitor), and on Err(e) log a warning including
entry.monitor_id and the error and skip repositioning; keep the reposition call
using entry.window.set_outer_position(expected_position) when positions differ.
Ensure you reference outer_position, to_logical(monitor.scale_factor()),
get_window_position_for_monitor, entry.monitor_id, and
entry.window.set_outer_position in the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/graphics/graphics_context.rscore/src/window_manager.rstauri/src-tauri/tauri.conf.json
| // Reposition if the window's actual position doesn't match expected. | ||
| let expected_position = get_window_position_for_monitor(monitor); | ||
| let actual_position: LogicalPosition<f64> = entry | ||
| .window | ||
| .outer_position() | ||
| .unwrap_or_default() | ||
| .to_logical(monitor.scale_factor()); | ||
| if actual_position != expected_position { | ||
| log::info!( | ||
| "WindowManager::update: repositioning window for monitor {:?} from {:?} to {:?}", | ||
| entry.monitor_id, | ||
| entry.position, | ||
| new_position | ||
| actual_position, | ||
| expected_position | ||
| ); | ||
| entry.window.set_outer_position(new_position); | ||
| entry.position = new_position; | ||
| entry.window.set_outer_position(expected_position); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 'outer_position\(\)\s*\.unwrap_or_default\(\)\s*\.to_logical|actual_position\s*!=\s*expected_position' core/src/window_manager.rsRepository: gethopp/hopp
Length of output: 457
🏁 Script executed:
# Get broader context around lines 209-223
sed -n '200,235p' core/src/window_manager.rs | cat -nRepository: gethopp/hopp
Length of output: 2157
🏁 Script executed:
# Check get_window_position_for_monitor implementation
rg -n 'fn get_window_position_for_monitor' core/src/window_manager.rsRepository: gethopp/hopp
Length of output: 144
🏁 Script executed:
# Check if monitor has a position() method
rg -n 'monitor\.position\(\)' core/src/Repository: gethopp/hopp
Length of output: 445
🏁 Script executed:
# Check what Monitor type is being used and its API
rg -n 'struct Monitor|impl.*Monitor' core/src/window_manager.rs -A 5 | head -50Repository: gethopp/hopp
Length of output: 38
🏁 Script executed:
# Get the full get_window_position_for_monitor function
sed -n '35,60p' core/src/window_manager.rs | cat -nRepository: gethopp/hopp
Length of output: 1131
🏁 Script executed:
# Check Monitor type and its position() method return type
rg -n 'position\(\)' core/src/window_manager.rs -B2 -A2Repository: gethopp/hopp
Length of output: 826
🏁 Script executed:
# Get more context on how monitor.position() is used
sed -n '30,45p' core/src/window_manager.rs | cat -nRepository: gethopp/hopp
Length of output: 680
🏁 Script executed:
# Check the imports to understand MonitorHandle
head -20 core/src/window_manager.rs | cat -nRepository: gethopp/hopp
Length of output: 834
Handle outer_position() errors explicitly instead of silently defaulting to (0,0).
The unwrap_or_default() call masks position-query errors and defaults to (0,0), which can trigger unnecessary reposition operations. Explicitly handle the error case rather than silently continuing with a potentially invalid position.
Note: The suggested diff in the original comment has a type error—it uses monitor.position() (which returns PhysicalPosition<i32>) directly instead of converting to LogicalPosition<f64>, causing a mismatch when compared with actual_position. The fix should match actual_position to the same coordinate space (logical) and properly propagate or handle the error case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/window_manager.rs` around lines 209 - 223, The code currently calls
entry.window.outer_position().unwrap_or_default() which hides errors and yields
(0,0); instead match on entry.window.outer_position(): on Ok(physical_pos)
convert it to logical via .to_logical(monitor.scale_factor()) and compare to
get_window_position_for_monitor(monitor), and on Err(e) log a warning including
entry.monitor_id and the error and skip repositioning; keep the reposition call
using entry.window.set_outer_position(expected_position) when positions differ.
Ensure you reference outer_position, to_logical(monitor.scale_factor()),
get_window_position_for_monitor, entry.monitor_id, and
entry.window.set_outer_position in the change.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores