improvement: remove content picker#353
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR removes the content-picker flow and replaces it with an in-app screen-selection overlay. It also updates capture, socket, Tauri, graphics, tests, and frontend code to use direct screen-selection and new screen-share permission handling. ChangesScreen selection replaces content picker
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/capture/capturer.rs (1)
356-377: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the capturer lock between polls
Holdingcapturer.lock()across the sleep loop blocks the sameDesktopCapturerfrom being driven byrun_capture_frame(), soget_source_rect()can stay empty and the caller can wait the full timeout. Re-lock on each iteration instead of sleeping with the mutex held.🤖 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 `@core/src/capture/capturer.rs` around lines 356 - 377, The polling loop in capturer acquisition is holding the DesktopCapturer mutex across sleeps, which can block run_capture_frame() from updating the source rect. Update the logic in capturer / get_selected_monitor so the lock is acquired only for each call to get_source_rect(), then released before sleeping and before the next iteration. Keep the monitor matching behavior the same, but avoid keeping capturer.lock() alive for the entire loop so the capturer can make progress between polls.core/tests/src/remote_cursor.rs (1)
289-304: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnused
sender/event_socketwill failcargo clippy -D warnings.With the multi-resolution/content loop removed, neither
sendernorevent_socketis used anywhere else intest_cursor_move, which will trigger unused-variable warnings promoted to errors under CI's clippy gate.🐛 Proposed fix
- let (sender, event_socket) = screenshare_client::start_screenshare_session()?; + let (_sender, _event_socket) = screenshare_client::start_screenshare_session()?;As per coding guidelines, "
core/**/*.rs: ... Ensure code passescargo clippy -D warningsin CI".🤖 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 `@core/tests/src/remote_cursor.rs` around lines 289 - 304, The `test_cursor_move` function in `remote_cursor.rs` creates `sender` and `event_socket` from `screenshare_client::start_screenshare_session()` but never uses them, which will trip `cargo clippy -D warnings`. Remove the unused bindings or change the destructuring so only the needed value is bound, keeping the rest of the logic in `test_cursor_move` and `internal_cursor_move` unchanged.Source: Coding guidelines
🧹 Nitpick comments (2)
core/tests/src/camera.rs (1)
324-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate env-parsing logic instead of reusing
screen_id().This block re-implements the exact same
HOPP_TEST_SCREEN_IDparsing/fallback logic asscreenshare_client::screen_id(). Since that helper is currently private (fn screen_id()), consider making itpub(crate)and calling it here to avoid divergence if the env var name or fallback ever changes.♻️ Proposed dedup
- let screen_id = std::env::var("HOPP_TEST_SCREEN_ID") - .ok() - .and_then(|s| s.parse().ok()) - .unwrap_or(0u32); + let screen_id = screenshare_client::screen_id();(requires making
screen_id()pub(crate)inscreenshare_client.rs)🤖 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 `@core/tests/src/camera.rs` around lines 324 - 335, The test setup in camera.rs duplicates the HOPP_TEST_SCREEN_ID parsing and fallback logic instead of reusing the existing screenshare_client::screen_id() helper. Make screen_id() in screenshare_client.rs pub(crate), then call that helper here so the env parsing lives in one place and stays consistent if the default or variable name changes.core/tests/src/screenshare_client.rs (1)
172-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_every_monitorno longer tests every monitor.The function is rewritten to test only Rewrote
test_every_monitor()to test a single screenshare session forscreen_id()(call start → request screenshare → wait → stop → call end), removing the previous iteration over multiple monitors derived from available content. Keeping the old name is misleading — it now exercises a single, env-configured screen rather than iterating over every monitor. Consider renaming (and updating theEveryMonitorCLI variant / call site inmain.rs) to something liketest_single_monitorto reflect actual behavior.🤖 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 `@core/tests/src/screenshare_client.rs` around lines 172 - 201, `test_every_monitor` now performs only one screen-share session for a single `screen_id()` instead of iterating over all monitors, so the name is misleading. Rename the function to match its actual behavior (for example, `test_single_monitor`) and update any related CLI variant and call site in `main.rs` that still refers to `EveryMonitor` so the entrypoint and test name stay consistent.
🤖 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.
Inline comments:
In `@core/socket_lib/src/lib.rs`:
- Around line 705-717: The ping is being asserted on the wrong endpoint in
test_request_response_pattern: server_sender.send(Message::Ping) is delivered to
the peer-side receiver, so update the test to read from client_events.events
instead of server_events.events and adjust the accompanying comment to match the
peer endpoint. Use the existing test_pair setup and the
test_request_response_pattern function to locate the change.
In `@core/src/graphics/graphics_context.rs`:
- Around line 273-276: The `set_screen_selection` method in `GraphicsContext`
has a dead commented-out redraw send that leaves the `Activity` import unused;
remove the stale comment and drop the unused import if this setter should remain
a plain state update. If `screen_selection` is still supposed to trigger
repainting, restore the `redraw_thread_sender.send(Activity)` path in real code
instead of leaving it commented out.
In `@core/src/lib.rs`:
- Around line 86-88: The keyboard imports in core/src/lib.rs are unused, so
clean up the import list by removing Key and NamedKey from the winit::keyboard
import in lib.rs; keep the remaining event and event_loop imports intact, and
only reintroduce those symbols if keyboard handling is added in a referenced
function or event handler later.
---
Outside diff comments:
In `@core/src/capture/capturer.rs`:
- Around line 356-377: The polling loop in capturer acquisition is holding the
DesktopCapturer mutex across sleeps, which can block run_capture_frame() from
updating the source rect. Update the logic in capturer / get_selected_monitor so
the lock is acquired only for each call to get_source_rect(), then released
before sleeping and before the next iteration. Keep the monitor matching
behavior the same, but avoid keeping capturer.lock() alive for the entire loop
so the capturer can make progress between polls.
In `@core/tests/src/remote_cursor.rs`:
- Around line 289-304: The `test_cursor_move` function in `remote_cursor.rs`
creates `sender` and `event_socket` from
`screenshare_client::start_screenshare_session()` but never uses them, which
will trip `cargo clippy -D warnings`. Remove the unused bindings or change the
destructuring so only the needed value is bound, keeping the rest of the logic
in `test_cursor_move` and `internal_cursor_move` unchanged.
---
Nitpick comments:
In `@core/tests/src/camera.rs`:
- Around line 324-335: The test setup in camera.rs duplicates the
HOPP_TEST_SCREEN_ID parsing and fallback logic instead of reusing the existing
screenshare_client::screen_id() helper. Make screen_id() in
screenshare_client.rs pub(crate), then call that helper here so the env parsing
lives in one place and stays consistent if the default or variable name changes.
In `@core/tests/src/screenshare_client.rs`:
- Around line 172-201: `test_every_monitor` now performs only one screen-share
session for a single `screen_id()` instead of iterating over all monitors, so
the name is misleading. Rename the function to match its actual behavior (for
example, `test_single_monitor`) and update any related CLI variant and call site
in `main.rs` that still refers to `EveryMonitor` so the entrypoint and test name
stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c877cc2-db8a-453e-874f-96ef63f9c574
⛔ Files ignored due to path filters (1)
core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
core/Cargo.tomlcore/socket_lib/src/lib.rscore/src/capture/capturer.rscore/src/capture/linux.rscore/src/capture/macos.rscore/src/capture/windows.rscore/src/graphics/graphics_context.rscore/src/graphics/iced_canvas.rscore/src/graphics/iced_renderer.rscore/src/lib.rscore/src/window/camera_window.rscore/src/window_manager.rscore/tests/src/camera.rscore/tests/src/local_drawing.rscore/tests/src/main.rscore/tests/src/remote_clipboard.rscore/tests/src/remote_cursor.rscore/tests/src/remote_drawing.rscore/tests/src/remote_keyboard.rscore/tests/src/screenshare_client.rstauri/contentPicker.htmltauri/src-tauri/capabilities/migrated.jsontauri/src-tauri/src/app_activation.rstauri/src-tauri/src/main.rstauri/src-tauri/src/shortcuts.rstauri/src/App.csstauri/src/components/ui/call-center.tsxtauri/src/core_payloads.tstauri/src/windows/contentPicker/main.tsxtauri/src/windows/window-utils.tstauri/vite.config.ts
💤 Files with no reviewable changes (6)
- tauri/src-tauri/src/app_activation.rs
- tauri/vite.config.ts
- tauri/src/App.css
- tauri/contentPicker.html
- core/Cargo.toml
- tauri/src/windows/contentPicker/main.tsx
| fn test_request_response_pattern() { | ||
| let ((server_sender, server_events), (client_sender, client_events)) = test_pair(); | ||
| let ((server_sender, server_events), (_client_sender, _client_events)) = test_pair(); | ||
|
|
||
| // Client sends request | ||
| client_sender.send(Message::GetAvailableContent).unwrap(); | ||
| server_sender.send(Message::Ping).unwrap(); | ||
|
|
||
| // Server receives event | ||
| let msg = server_events | ||
| .events | ||
| .recv_timeout(Duration::from_secs(5)) | ||
| .unwrap(); | ||
| assert!(matches!(msg, Message::GetAvailableContent)); | ||
|
|
||
| // Server sends response | ||
| server_sender | ||
| .send(Message::AvailableContent(AvailableContentMessage { | ||
| content: vec![], | ||
| })) | ||
| .unwrap(); | ||
|
|
||
| // Client receives response on responses channel | ||
| let response = client_events | ||
| .responses | ||
| .recv_timeout(Duration::from_secs(5)) | ||
| .unwrap(); | ||
| assert!(matches!(response, Message::AvailableContent(_))); | ||
| assert!(matches!(msg, Message::Ping)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm socket direction: sender delivers to the peer's EventSocket.
fd -t f 'lib.rs' core/socket_lib/src --exec sed -n '400,520p' {}Repository: gethopp/hopp
Length of output: 4153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test helper and the test in question.
file=core/socket_lib/src/lib.rs
echo '--- test_pair and nearby helpers ---'
rg -n "fn test_pair|test_request_response_pattern|test_send_recv_event|test_send_recv_response|struct SocketSender|struct EventSocket|impl SocketSender|impl EventSocket" "$file"
echo '--- relevant source around test_pair ---'
sed -n '620,760p' "$file"Repository: gethopp/hopp
Length of output: 3758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=core/socket_lib/src/lib.rs
echo '--- SocketSender and EventSocket implementations ---'
sed -n '240,360p' "$file"
echo '--- test_pair and related tests ---'
sed -n '520,725p' "$file"Repository: gethopp/hopp
Length of output: 11167
Route the ping to the peer-side receiver server_sender.send(Message::Ping) is delivered to client_events.events, not server_events.events, so this test will time out on the wrong channel. Swap the receiver (and comment) to match the peer endpoint.
🤖 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 `@core/socket_lib/src/lib.rs` around lines 705 - 717, The ping is being
asserted on the wrong endpoint in test_request_response_pattern:
server_sender.send(Message::Ping) is delivered to the peer-side receiver, so
update the test to read from client_events.events instead of
server_events.events and adjust the accompanying comment to match the peer
endpoint. Use the existing test_pair setup and the test_request_response_pattern
function to locate the change.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Tests