feat: add drawing in sharer #235
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds end-to-end local drawing: socket message for enabling drawing, LocalDrawing state and pencil cursor, per-path expiry/auto-clear, draw publish APIs in RoomService, Tauri UI/state and commands, input/graphics plumbing, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Frontend UI
participant Tauri as Tauri Backend
participant Core as Core App
participant DrawMgr as DrawManager
participant RoomSvc as RoomService
participant LiveKit as LiveKit
User->>UI: Click "Enable Drawing" (permanent flag)
UI->>Tauri: invoke enableDrawing(permanent)
Tauri->>Core: send Message::DrawingEnabled(permanent)
Core->>Core: set LocalDrawing state, load pencil cursor
Core->>DrawMgr: add_participant(sid, color, auto_clear = !permanent)
rect rgba(0,100,200,0.5)
Note over User,Core: User draws on overlay
User->>Core: DrawStart / AddPoint / End
Core->>RoomSvc: publish_draw_start/add_point/end
RoomSvc->>LiveKit: publish draw events
end
rect rgba(200,100,0,0.5)
Note over Core,DrawMgr: Auto-clear expired finished paths
Core->>DrawMgr: update_auto_clear()
DrawMgr->>DrawMgr: clear_expired_paths() -> expired_ids
DrawMgr->>RoomSvc: publish_draw_clear_paths(expired_ids)
RoomSvc->>LiveKit: publish DrawClearPath events
end
User->>UI: Disable Drawing
UI->>Tauri: (optional) enableDrawing(false)
Tauri->>Core: send Message::DrawingEnabled(permanent=false)
Core->>Core: clear local paths, restore controllers/cursor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@core/src/lib.rs`:
- Around line 1069-1151: The LocalDrawing disable path unconditionally
re-enables controllers; preserve and restore the prior controller state instead:
add a previous_controllers_enabled field to the LocalDrawing struct, set it when
enabling drawing (read remote_control.cursor_controller.is_controllers_enabled()
or equivalent and store into self.local_drawing.previous_controllers_enabled)
before calling set_controllers_enabled(false) and disabling keyboard_controller,
and on disable use that stored value to restore
remote_control.cursor_controller.set_controllers_enabled(self.local_drawing.previous_controllers_enabled)
and
remote_control.keyboard_controller.set_enabled(self.local_drawing.previous_controllers_enabled)
(use the same boolean for both or track keyboard separately if needed) so
disabling local drawing restores the exact pre-drawing permission state rather
than unconditionally enabling controllers.
🧹 Nitpick comments (3)
tauri/src-tauri/src/main.rs (1)
470-487: Consider returning a Result to propagate socket errors to the frontend.The socket send error is logged but the frontend has no way to know if enabling drawing failed. The frontend already has error handling with
toast.error()but it can't distinguish between a Tauri invoke failure and a successful invoke where the core message failed.♻️ Suggested change
#[tauri::command] -fn enable_drawing(app: tauri::AppHandle, permanent: bool) { +fn enable_drawing(app: tauri::AppHandle, permanent: bool) -> Result<(), String> { log::info!("enable_drawing: permanent={permanent}"); let data = app.state::<Mutex<AppData>>(); let mut data = data.lock().unwrap(); let res = data .socket .send_message(Message::DrawingEnabled(DrawingEnabled { permanent })); if let Err(e) = res { log::error!("enable_drawing: failed to send message: {e:?}"); + return Err(format!("Failed to enable drawing: {e}")); } drop(data); // Hide main window if let Some(window) = app.get_webview_window("main") { let _ = window.hide(); } + Ok(()) }core/src/lib.rs (2)
133-134: Dead code:pencil_cursoris created but never used.The comments on lines 1091 and 1194-1195 indicate that
window.set_cursor(remote_control.pencil_cursor.clone())doesn't work. Consider removing this field and the cursor loading code (lines 496-519) until it can be used, or add a tracking issue.
493-519: Cursor loading may fail silently on missing file, but error is handled.The pencil cursor loading is well-implemented with proper error propagation. However, since the cursor isn't actually being used (per the comments), this code path adds initialization overhead without benefit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/lib.rs`:
- Around line 1086-1107: Move the mutations to local_drawing
(local_drawing.enabled, local_drawing.permanent, local_drawing.cursor_set_times
and previous_controllers_enabled) so they are only applied after
window.set_cursor_hittest(true) succeeds; locate the block around
remote_control.gfx.window().set_cursor_hittest(true) and defer setting those
fields until after the fallible call returns Ok. Additionally, treat non-fatal
errors from set_cursor_hittest (e.g., Error::NotSupported or Error::Ignored) as
expected by logging them at debug/info and continuing with the drawing setup,
but log and return on Os-level errors; ensure you still update
previous_controllers_enabled and disable remote control only after
success/fallback handling.
| let remote_control = &mut self.remote_control.as_mut().unwrap(); | ||
| if !self.local_drawing.enabled { | ||
| // Enable drawing mode | ||
| self.local_drawing.enabled = true; | ||
| self.local_drawing.permanent = drawing_enabled.permanent; | ||
|
|
||
| let window = remote_control.gfx.window(); | ||
|
|
||
| // Enable cursor hittest so we can receive mouse events | ||
| if let Err(e) = window.set_cursor_hittest(true) { | ||
| log::error!("user_event: Failed to enable cursor hittest: {e:?}"); | ||
| return; | ||
| } | ||
|
|
||
| // Reset cursor set times counter | ||
| self.local_drawing.cursor_set_times = 0; | ||
|
|
||
| // Store the current controller state before disabling | ||
| self.local_drawing.previous_controllers_enabled = | ||
| remote_control.cursor_controller.is_controllers_enabled(); | ||
|
|
||
| // Disable remote control |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l core/src/lib.rsRepository: gethopp/hopp
Length of output: 76
🏁 Script executed:
sed -n '1080,1115p' core/src/lib.rsRepository: gethopp/hopp
Length of output: 1771
🏁 Script executed:
# Search for local_drawing.enabled to understand its usage patterns
rg "local_drawing\.enabled" core/src/lib.rs -B 2 -A 2Repository: gethopp/hopp
Length of output: 1633
🏁 Script executed:
# Check how drawing mode is used after being enabled
rg "self\.local_drawing\b" core/src/lib.rs -A 1 | head -50Repository: gethopp/hopp
Length of output: 2270
🌐 Web query:
winit set_cursor_hittest error conditions and recommended handling
💡 Result:
What can go wrong with Window::set_cursor_hittest (winit)
set_cursor_hittest(hittest: bool) -> Result<(), ExternalError> toggles whether the window receives mouse/cursor events (false = “click-through”). On iOS / Android / Web / Orbital it always fails with ExternalError::NotSupported. [1]
Error variants you should expect
ExternalError can be: [2]
ExternalError::NotSupported(...): backend/platform doesn’t implement hit-testing (or the current backend can’t do it). This is the documented outcome for iOS/Android/Web/Orbital, and historically the feature was added for Windows/macOS/Wayland (not X11). [1][3]ExternalError::Ignored: winit accepted the request but didn’t apply it (safe to treat as “no-op”). [2]ExternalError::Os(...): the OS refused/failed to apply the change (treat as a real failure; typically log + continue with hit-testing enabled). [2]
Note: Some winit surfaces also expose a trait form returning
RequestErrorinstead ofExternalError; the practical handling is the same: treat “not supported” as optional behavior. [4]
Recommended handling pattern
- Treat NotSupported and Ignored as non-fatal (feature is optional; continue normally).
- Treat Os as “couldn’t apply”; log/telemetry it and continue with the default behavior (hit-testing enabled).
- If you require click-through for UX, keep your own
bool click_through_effectiveand only rely on it when the call succeeds.
use winit::error::ExternalError;
match window.set_cursor_hittest(false) {
Ok(()) => { /* click-through enabled */ }
Err(ExternalError::NotSupported(_)) | Err(ExternalError::Ignored) => {
// Optional feature; ignore/fallback
}
Err(ExternalError::Os(e)) => {
// Log and fallback (keep normal hit-testing)
eprintln!("set_cursor_hittest failed: {e}");
}
}Sources: winit Window::set_cursor_hittest docs (platform support) [1], ExternalError variants [2], changelog note about supported platforms [3], trait form returning RequestError [4].
[1] https://docs.rs/winit/latest/winit/window/struct.Window.html#method.set_cursor_hittest
[2] https://docs.rs/winit/latest/winit/error/enum.ExternalError.html
[3] https://docs.rs/winit/latest/winit/changelog/v0_27/index.html
[4] https://rust-windowing.github.io/winit/winit/window/trait.Window.html#tymethod.set_cursor_hittest
Prevent inconsistent state if cursor hittest configuration fails.
local_drawing.enabled is set to true before the fallible set_cursor_hittest(true) call. If that call fails and returns early, the flag remains true but the window cursor state isn't configured—causing subsequent event handlers (MouseInput, CursorMoved, KeyboardInput) to process input as though the precondition is met, creating inconsistent state.
Move the state update (enabled, permanent, cursor_set_times) to occur only after the set_cursor_hittest call succeeds. Additionally, consider treating NotSupported and Ignored errors as non-fatal (expected on platforms like iOS/Android/Web), logging only Os-level errors and allowing the feature to proceed with fallback behavior.
🤖 Prompt for AI Agents
In `@core/src/lib.rs` around lines 1086 - 1107, Move the mutations to
local_drawing (local_drawing.enabled, local_drawing.permanent,
local_drawing.cursor_set_times and previous_controllers_enabled) so they are
only applied after window.set_cursor_hittest(true) succeeds; locate the block
around remote_control.gfx.window().set_cursor_hittest(true) and defer setting
those fields until after the fallible call returns Ok. Additionally, treat
non-fatal errors from set_cursor_hittest (e.g., Error::NotSupported or
Error::Ignored) as expected by logging them at debug/info and continuing with
the drawing setup, but log and return on Os-level errors; ensure you still
update previous_controllers_enabled and disable remote control only after
success/fallback handling.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/lib.rs (1)
191-266: Reset local drawing state when the overlay is torn down.If screensharing stops while local drawing is enabled,
local_drawingflags/counters persist into the next session, which can require a double-toggle and reuse stale path/cursor state. Consider resettingLocalDrawinginstop_screenshare/destroy_overlay_window(or right before a new overlay is created) so each session starts clean.
🤖 Fix all issues with AI agents
In `@tauri/src-tauri/src/main.rs`:
- Around line 472-489: The enable_drawing function currently hides the main
window regardless of whether
socket.send_message(Message::DrawingEnabled(DrawingEnabled { permanent }))
succeeds; change enable_drawing to return a Result<(), String> (or a suitable
error type), attempt sending and if it returns Err propagate that error to the
caller (do not hide the window), and only call
app.get_webview_window("main").hide() when the send succeeded; update callers if
needed so the frontend handles the error. Use the existing AppData,
socket.send_message, Message::DrawingEnabled, and DrawingEnabled symbols to
locate and adjust the logic.
🧹 Nitpick comments (2)
core/src/input/mouse_macos.rs (1)
277-287: UseCGMouseButton::LeftforLeftMouseDraggedevents to follow convention.For
LeftMouseDraggedevents, themouseButtonparameter is ignored by macOS; however, convention is to passCGMouseButton::Leftinstead ofCGMouseButton::Centerfor code clarity and consistency with the event type. This improves readability without affecting functionality.♻️ Suggested change
let event_type = if click_down { CGEventType::LeftMouseDragged } else { CGEventType::MouseMoved }; + let mouse_button = if click_down { + CGMouseButton::Left + } else { + CGMouseButton::Center + }; let event = CGEvent::new_mouse_event( event_source, event_type, CGPoint::new(position.x, position.y), - CGMouseButton::Center, + mouse_button, );core/src/lib.rs (1)
1193-1222: Avoid a tight redraw loop while local drawing is idle.
request_redraw()on everyRedrawRequestedwhen local drawing is enabled can keep the event loop spinning even without input, which can raise CPU/battery use. Consider scheduling redraws on a fixed timer (e.g., ~50 FPS) or only when drawing/auto‑clear has pending work.
| #[tauri::command] | ||
| fn enable_drawing(app: tauri::AppHandle, permanent: bool) { | ||
| log::info!("enable_drawing: permanent={permanent}"); | ||
| let data = app.state::<Mutex<AppData>>(); | ||
| let mut data = data.lock().unwrap(); | ||
| let res = data | ||
| .socket | ||
| .send_message(Message::DrawingEnabled(DrawingEnabled { permanent })); | ||
| if let Err(e) = res { | ||
| log::error!("enable_drawing: failed to send message: {e:?}"); | ||
| } | ||
| drop(data); | ||
|
|
||
| // Hide main window | ||
| if let Some(window) = app.get_webview_window("main") { | ||
| let _ = window.hide(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t hide the main window if enabling drawing fails.
Right now the window is hidden even when the send fails, leaving the user with no UI and no feedback. Return a Result and only hide on success (and let the frontend handle the error).
🛠️ Proposed fix
-#[tauri::command]
-fn enable_drawing(app: tauri::AppHandle, permanent: bool) {
+#[tauri::command]
+fn enable_drawing(app: tauri::AppHandle, permanent: bool) -> Result<(), String> {
log::info!("enable_drawing: permanent={permanent}");
let data = app.state::<Mutex<AppData>>();
let mut data = data.lock().unwrap();
let res = data
.socket
.send_message(Message::DrawingEnabled(DrawingEnabled { permanent }));
if let Err(e) = res {
log::error!("enable_drawing: failed to send message: {e:?}");
+ return Err("Failed to send message to hopp_core".to_string());
}
drop(data);
// Hide main window
if let Some(window) = app.get_webview_window("main") {
let _ = window.hide();
}
+ Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tauri::command] | |
| fn enable_drawing(app: tauri::AppHandle, permanent: bool) { | |
| log::info!("enable_drawing: permanent={permanent}"); | |
| let data = app.state::<Mutex<AppData>>(); | |
| let mut data = data.lock().unwrap(); | |
| let res = data | |
| .socket | |
| .send_message(Message::DrawingEnabled(DrawingEnabled { permanent })); | |
| if let Err(e) = res { | |
| log::error!("enable_drawing: failed to send message: {e:?}"); | |
| } | |
| drop(data); | |
| // Hide main window | |
| if let Some(window) = app.get_webview_window("main") { | |
| let _ = window.hide(); | |
| } | |
| } | |
| #[tauri::command] | |
| fn enable_drawing(app: tauri::AppHandle, permanent: bool) -> Result<(), String> { | |
| log::info!("enable_drawing: permanent={permanent}"); | |
| let data = app.state::<Mutex<AppData>>(); | |
| let mut data = data.lock().unwrap(); | |
| let res = data | |
| .socket | |
| .send_message(Message::DrawingEnabled(DrawingEnabled { permanent })); | |
| if let Err(e) = res { | |
| log::error!("enable_drawing: failed to send message: {e:?}"); | |
| return Err("Failed to send message to hopp_core".to_string()); | |
| } | |
| drop(data); | |
| // Hide main window | |
| if let Some(window) = app.get_webview_window("main") { | |
| let _ = window.hide(); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In `@tauri/src-tauri/src/main.rs` around lines 472 - 489, The enable_drawing
function currently hides the main window regardless of whether
socket.send_message(Message::DrawingEnabled(DrawingEnabled { permanent }))
succeeds; change enable_drawing to return a Result<(), String> (or a suitable
error type), attempt sending and if it returns Err propagate that error to the
caller (do not hide the window), and only call
app.get_webview_window("main").hide() when the send succeeded; update callers if
needed so the frontend handles the error. Use the existing AppData,
socket.send_message, Message::DrawingEnabled, and DrawingEnabled symbols to
locate and adjust the logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/lib.rs`:
- Around line 213-221: The reset() method currently omits resetting the
cursor_set_times counter, causing it to persist across sessions; update the
reset() implementation to also set cursor_set_times back to 0 (e.g.,
self.cursor_set_times = 0) so the cursor-setting logic in the redraw handler
behaves correctly for new screenshare sessions; locate the reset() method in
core/src/lib.rs and add the assignment alongside the other field resets.
🧹 Nitpick comments (1)
core/src/lib.rs (1)
1221-1236: Cursor management workaround lacks documentation.The pattern of calling
focus_window(), toggling cursor visibility, and limiting to 500 iterations suggests this is a platform-specific workaround. Without comments explaining the underlying issue being addressed, this code will be difficult to maintain or debug.Consider adding a comment explaining:
- Why
focus_window()is needed during redraw- Why cursor visibility must be toggled (false then true)
- What the 500-iteration limit prevents
📝 Suggested documentation
if self.local_drawing.last_redraw_time.elapsed() > std::time::Duration::from_millis(20) { if self.local_drawing.cursor_set_times < 500 { + // WORKAROUND: On some platforms, the custom cursor may not + // persist across redraws. We focus the window and toggle + // visibility to ensure the pencil cursor remains active. + // Limited to 500 times to avoid excessive overhead after + // the cursor stabilizes. let window = remote_control.gfx.window(); window.focus_window(); window.set_cursor_visible(false); window.set_cursor_visible(true); window.set_cursor(remote_control.pencil_cursor.clone()); self.local_drawing.cursor_set_times += 1; }
| fn reset(&mut self) { | ||
| self.enabled = false; | ||
| self.permanent = false; | ||
| self.left_mouse_pressed = false; | ||
| self.current_path_id = 0; | ||
| self.last_cursor_position = None; | ||
| self.last_redraw_time = std::time::Instant::now(); | ||
| self.previous_controllers_enabled = false; | ||
| } |
There was a problem hiding this comment.
reset() does not reset cursor_set_times.
The cursor_set_times field is not reset in the reset() method, which could cause the counter to persist across multiple screenshare sessions. If a user starts a second screenshare after the counter reaches 500, the cursor-setting logic in the redraw handler would be skipped.
🔧 Proposed fix
fn reset(&mut self) {
self.enabled = false;
self.permanent = false;
self.left_mouse_pressed = false;
self.current_path_id = 0;
self.last_cursor_position = None;
self.last_redraw_time = std::time::Instant::now();
self.previous_controllers_enabled = false;
+ self.cursor_set_times = 0;
}🤖 Prompt for AI Agents
In `@core/src/lib.rs` around lines 213 - 221, The reset() method currently omits
resetting the cursor_set_times counter, causing it to persist across sessions;
update the reset() implementation to also set cursor_set_times back to 0 (e.g.,
self.cursor_set_times = 0) so the cursor-setting logic in the redraw handler
behaves correctly for new screenshare sessions; locate the reset() method in
core/src/lib.rs and add the assignment alongside the other field resets.
Resolved conflict in core/src/lib.rs by keeping both local_drawing and window_manager fields, as both are required for their respective features. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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 (1)
.cursor/commands/openspec-proposal.md (1)
24-24:⚠️ Potential issue | 🟡 MinorAdd RENAMED to the requirement change types.
The spec delta guidance is missing the
RENAMEDoption. Based on learnings, the complete list should be## ADDED|MODIFIED|REMOVED|RENAMED Requirementsto cover all requirement change scenarios, including when requirements are renamed or their identifiers change.📝 Proposed fix to include RENAMED
-5. Draft spec deltas in `changes/<id>/specs/<capability>/spec.md` (one folder per capability) using `## ADDED|MODIFIED|REMOVED Requirements` with at least one `#### Scenario:` per requirement and cross-reference related capabilities when relevant. +5. Draft spec deltas in `changes/<id>/specs/<capability>/spec.md` (one folder per capability) using `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` with at least one `#### Scenario:` per requirement and cross-reference related capabilities when relevant.
🤖 Fix all issues with AI agents
In `@core/src/lib.rs`:
- Around line 1201-1323: When left_mouse_pressed is set while
last_cursor_position is None, the initial draw_start is never emitted causing
the first point to be dropped; fix by emitting draw_start as soon as we have the
first CursorMoved after a press (or emit immediately on press if you prefer). In
practice, inside WindowEvent::CursorMoved check: if
self.local_drawing.left_mouse_pressed and there is no in-progress path,
increment self.local_drawing.current_path_id, call
remote_control.gfx.draw_start("local", pos, self.local_drawing.current_path_id),
call remote_control.cursor_controller.trigger_render(), and if room_service is
Some send room_service.publish_draw_start(...) using the normalized point from
remote_control.cursor_controller.get_overlay_window().get_local_percentage_from_pixel(pos.x,
pos.y); then proceed to call draw_add_point as currently implemented. This
ensures the first point is not lost; use symbols local_drawing,
last_cursor_position, left_mouse_pressed, current_path_id,
remote_control.gfx.draw_start, cursor_controller.get_overlay_window(), and
room_service.publish_draw_start.
In `@tauri/src/components/ui/call-center.tsx`:
- Around line 281-303: The icon-only buttons (the button that calls
handleEnableDrawing with PiScribbleLoopBold and the DropdownMenuTrigger button
with ChevronDownIcon) need accessible labels so screen readers announce them;
update those button elements to include descriptive aria-label attributes (e.g.,
aria-label="Enable drawing" and aria-label="Open menu") or include visually
hidden text inside the button, and if the dropdown trigger opens a popup add
aria-haspopup="menu" and aria-expanded tied to dropdownOpen; ensure the props
are added to the actual button elements used by
TooltipTrigger/DropdownMenuTrigger so assistive tech can discover them.
- Around line 258-265: When persisting the drawing-permanent preference in
handlePermanentToggle, capture the previous state (before calling
setDrawingPermanent) then attempt tauriUtils.setDrawingPermanent(checked); if
that call throws, revert the UI by restoring the previous state via
setDrawingPermanent(previousValue) and surface an error to the user (e.g., call
an existing notification/toast helper or show an alert) so the toggle reflects
actual persisted state; ensure the try/catch around
tauriUtils.setDrawingPermanent references the captured previousValue and the
error notification includes context.
| WindowEvent::MouseInput { state, button, .. } => { | ||
| if self.local_drawing.enabled { | ||
| if button == winit::event::MouseButton::Left { | ||
| if state == winit::event::ElementState::Pressed { | ||
| self.local_drawing.left_mouse_pressed = true; | ||
| // Start a new path if we have a cursor position | ||
| if let Some(position) = self.local_drawing.last_cursor_position { | ||
| if let Some(remote_control) = &mut self.remote_control { | ||
| self.local_drawing.current_path_id += 1; | ||
| remote_control.gfx.draw_start( | ||
| "local", | ||
| position, | ||
| self.local_drawing.current_path_id, | ||
| ); | ||
| remote_control.cursor_controller.trigger_render(); | ||
|
|
||
| // Send LiveKit event | ||
| if let Some(room_service) = &self.room_service { | ||
| let overlay_window = | ||
| remote_control.cursor_controller.get_overlay_window(); | ||
| let normalized_point = overlay_window | ||
| .get_local_percentage_from_pixel( | ||
| position.x, position.y, | ||
| ); | ||
| room_service.publish_draw_start( | ||
| room_service::DrawPathPoint { | ||
| point: room_service::ClientPoint { | ||
| x: normalized_point.x, | ||
| y: normalized_point.y, | ||
| }, | ||
| path_id: self.local_drawing.current_path_id, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| log::debug!( | ||
| "Local draw_start at {:?} with path_id {}", | ||
| position, | ||
| self.local_drawing.current_path_id | ||
| ); | ||
| } | ||
| } | ||
| } else { | ||
| self.local_drawing.left_mouse_pressed = false; | ||
| // End the current path | ||
| if let Some(position) = self.local_drawing.last_cursor_position { | ||
| if let Some(remote_control) = &mut self.remote_control { | ||
| remote_control.gfx.draw_end("local", position); | ||
| remote_control.cursor_controller.trigger_render(); | ||
|
|
||
| // Send LiveKit event | ||
| if let Some(room_service) = &self.room_service { | ||
| let overlay_window = | ||
| remote_control.cursor_controller.get_overlay_window(); | ||
| let normalized_point = overlay_window | ||
| .get_local_percentage_from_pixel( | ||
| position.x, position.y, | ||
| ); | ||
| room_service.publish_draw_end(room_service::ClientPoint { | ||
| x: normalized_point.x, | ||
| y: normalized_point.y, | ||
| }); | ||
| } | ||
|
|
||
| log::debug!("Local draw_end at {:?}", position); | ||
| } | ||
| } | ||
| } | ||
| } else if button == winit::event::MouseButton::Right | ||
| && state == winit::event::ElementState::Pressed | ||
| { | ||
| if let Some(remote_control) = &mut self.remote_control { | ||
| // Clear all local drawing paths | ||
| remote_control.gfx.draw_clear_all_paths("local"); | ||
| remote_control.cursor_controller.trigger_render(); | ||
|
|
||
| // Send LiveKit event to clear all paths | ||
| if let Some(room_service) = &self.room_service { | ||
| room_service.publish_draw_clear_all_paths(); | ||
| } | ||
| log::debug!("Local draw_clear_all_paths on right click"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| WindowEvent::CursorMoved { position, .. } => { | ||
| if self.local_drawing.enabled { | ||
| let display_scale = if let Some(remote_control) = &mut self.remote_control { | ||
| remote_control | ||
| .cursor_controller | ||
| .get_overlay_window() | ||
| .get_display_scale() | ||
| } else { | ||
| 1.0 | ||
| }; | ||
| // Convert physical position to our Position type | ||
| let pos = Position { | ||
| x: position.x / display_scale, | ||
| y: position.y / display_scale, | ||
| }; | ||
| self.local_drawing.last_cursor_position = Some(pos); | ||
|
|
||
| // If we're actively drawing, add a point | ||
| if self.local_drawing.left_mouse_pressed { | ||
| if let Some(remote_control) = &mut self.remote_control { | ||
| remote_control.gfx.draw_add_point("local", pos); | ||
| remote_control.cursor_controller.trigger_render(); | ||
|
|
||
| // Send LiveKit event | ||
| if let Some(room_service) = &self.room_service { | ||
| let overlay_window = | ||
| remote_control.cursor_controller.get_overlay_window(); | ||
| let normalized_point = | ||
| overlay_window.get_local_percentage_from_pixel(pos.x, pos.y); | ||
| room_service.publish_draw_add_point(room_service::ClientPoint { | ||
| x: normalized_point.x, | ||
| y: normalized_point.y, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find draw_add_point function definition
rg -n -C5 --type=rust "fn draw_add_point"Repository: gethopp/hopp
Length of output: 3340
🏁 Script executed:
#!/bin/bash
# Find where entries are added to the draws map
rg -n --type=rust "draws\." | grep -i "insert\|register\|new_participant\|init" | head -20Repository: gethopp/hopp
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Examine draw_start implementation with more context to see if it creates the entry
rg -n -B10 -A10 "pub fn draw_start" core/src/graphics/draw.rsRepository: gethopp/hopp
Length of output: 825
🏁 Script executed:
#!/bin/bash
# Find context around line 263 to see which function creates new entries
rg -n -B15 -A5 "self.draws.insert(sid, Draw::new" core/src/graphics/draw.rsRepository: gethopp/hopp
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Find context around line 263 with escaped regex
rg -n -B15 -A5 "draws.insert" core/src/graphics/draw.rsRepository: gethopp/hopp
Length of output: 762
🏁 Script executed:
#!/bin/bash
# Find where add_participant is called, particularly for "local"
rg -n "add_participant" core/src/lib.rs | head -20Repository: gethopp/hopp
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search more broadly for add_participant calls
rg -n "add_participant" --type=rust | head -30Repository: gethopp/hopp
Length of output: 370
🏁 Script executed:
#!/bin/bash
# Check the context where add_participant is called in iced_canvas
rg -n -B10 -A5 "self.draws.add_participant" core/src/graphics/iced_canvas.rsRepository: gethopp/hopp
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Search for where add_draw_participant is called, especially with "local"
rg -n "add_draw_participant" --type=rustRepository: gethopp/hopp
Length of output: 843
🏁 Script executed:
#!/bin/bash
# Examine the Draw struct and its start_path/add_point methods
rg -n -A10 "pub fn start_path\|pub fn add_point" core/src/graphics/draw.rs | head -50Repository: gethopp/hopp
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Find the Draw struct definition and its methods
rg -n "impl Draw\|struct Draw" core/src/graphics/draw.rsRepository: gethopp/hopp
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for Draw struct definition
rg -n -B2 -A20 "^struct Draw" core/src/graphics/draw.rsRepository: gethopp/hopp
Length of output: 585
🏁 Script executed:
#!/bin/bash
# Find Draw impl block and its methods
rg -n -A200 "impl Draw" core/src/graphics/draw.rs | head -100Repository: gethopp/hopp
Length of output: 3514
Add guard to emit draw_start on first point if not yet started.
When the left mouse button is pressed before any cursor movement, last_cursor_position is None, so draw_start is not emitted. Subsequently, when the cursor moves and draw_add_point is called, the underlying Draw::add_point (line 95-106 in core/src/graphics/draw.rs) checks if let Some(in_progress_path) and silently skips the point with a warning log if no path is in progress. This causes the initial drawing strokes to be lost.
Either track a pending-start flag and issue draw_start on the first CursorMoved event if not yet started, or ensure draw_start is called immediately when left_mouse_pressed is set to true, even without a cursor position.
🤖 Prompt for AI Agents
In `@core/src/lib.rs` around lines 1201 - 1323, When left_mouse_pressed is set
while last_cursor_position is None, the initial draw_start is never emitted
causing the first point to be dropped; fix by emitting draw_start as soon as we
have the first CursorMoved after a press (or emit immediately on press if you
prefer). In practice, inside WindowEvent::CursorMoved check: if
self.local_drawing.left_mouse_pressed and there is no in-progress path,
increment self.local_drawing.current_path_id, call
remote_control.gfx.draw_start("local", pos, self.local_drawing.current_path_id),
call remote_control.cursor_controller.trigger_render(), and if room_service is
Some send room_service.publish_draw_start(...) using the normalized point from
remote_control.cursor_controller.get_overlay_window().get_local_percentage_from_pixel(pos.x,
pos.y); then proceed to call draw_add_point as currently implemented. This
ensures the first point is not lost; use symbols local_drawing,
last_cursor_position, left_mouse_pressed, current_path_id,
remote_control.gfx.draw_start, cursor_controller.get_overlay_window(), and
room_service.publish_draw_start.
| const handlePermanentToggle = async (checked: boolean) => { | ||
| setDrawingPermanent(checked); | ||
| try { | ||
| await tauriUtils.setDrawingPermanent(checked); | ||
| } catch (error) { | ||
| console.error("Failed to save drawing permanent preference:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Keep preference state consistent if persistence fails.
If setDrawingPermanent throws, the UI remains toggled even though the preference wasn’t saved. Consider reverting and notifying the user.
🛠️ Suggested fix
- const handlePermanentToggle = async (checked: boolean) => {
- setDrawingPermanent(checked);
- try {
- await tauriUtils.setDrawingPermanent(checked);
- } catch (error) {
- console.error("Failed to save drawing permanent preference:", error);
- }
- };
+ const handlePermanentToggle = async (checked: boolean) => {
+ const previous = drawingPermanent;
+ setDrawingPermanent(checked);
+ try {
+ await tauriUtils.setDrawingPermanent(checked);
+ } catch (error) {
+ console.error("Failed to save drawing permanent preference:", error);
+ setDrawingPermanent(previous);
+ toast.error("Failed to save drawing preference", { duration: 2500 });
+ }
+ };🤖 Prompt for AI Agents
In `@tauri/src/components/ui/call-center.tsx` around lines 258 - 265, When
persisting the drawing-permanent preference in handlePermanentToggle, capture
the previous state (before calling setDrawingPermanent) then attempt
tauriUtils.setDrawingPermanent(checked); if that call throws, revert the UI by
restoring the previous state via setDrawingPermanent(previousValue) and surface
an error to the user (e.g., call an existing notification/toast helper or show
an alert) so the toggle reflects actual persisted state; ensure the try/catch
around tauriUtils.setDrawingPermanent references the captured previousValue and
the error notification includes context.
| <button | ||
| type="button" | ||
| onClick={handleEnableDrawing} | ||
| className="size-9 flex items-center justify-center rounded-l-lg border border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors" | ||
| > | ||
| <PiScribbleLoopBold className="size-3.5" /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom">Enable drawing</TooltipContent> | ||
| </Tooltip> | ||
| </TooltipProvider> | ||
| <DropdownMenu open={dropdownOpen} onOpenChange={setDropdownOpen}> | ||
| <TooltipProvider> | ||
| <Tooltip delayDuration={100}> | ||
| <TooltipTrigger asChild> | ||
| <DropdownMenuTrigger asChild> | ||
| <button | ||
| type="button" | ||
| className="size-9 flex items-center justify-center rounded-r-lg border-y border-r border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors" | ||
| > | ||
| <ChevronDownIcon className="size-3.5" /> | ||
| </button> | ||
| </DropdownMenuTrigger> |
There was a problem hiding this comment.
Add accessible labels to icon-only buttons.
Screen readers won’t announce these controls without labels.
🧩 Suggested fix
- <button
- type="button"
- onClick={handleEnableDrawing}
- className="size-9 flex items-center justify-center rounded-l-lg border border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors"
- >
+ <button
+ type="button"
+ aria-label="Enable drawing"
+ onClick={handleEnableDrawing}
+ className="size-9 flex items-center justify-center rounded-l-lg border border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors"
+ >
@@
- <button
- type="button"
- className="size-9 flex items-center justify-center rounded-r-lg border-y border-r border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors"
- >
+ <button
+ type="button"
+ aria-label="Drawing settings"
+ className="size-9 flex items-center justify-center rounded-r-lg border-y border-r border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| type="button" | |
| onClick={handleEnableDrawing} | |
| className="size-9 flex items-center justify-center rounded-l-lg border border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors" | |
| > | |
| <PiScribbleLoopBold className="size-3.5" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom">Enable drawing</TooltipContent> | |
| </Tooltip> | |
| </TooltipProvider> | |
| <DropdownMenu open={dropdownOpen} onOpenChange={setDropdownOpen}> | |
| <TooltipProvider> | |
| <Tooltip delayDuration={100}> | |
| <TooltipTrigger asChild> | |
| <DropdownMenuTrigger asChild> | |
| <button | |
| type="button" | |
| className="size-9 flex items-center justify-center rounded-r-lg border-y border-r border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors" | |
| > | |
| <ChevronDownIcon className="size-3.5" /> | |
| </button> | |
| </DropdownMenuTrigger> | |
| <button | |
| type="button" | |
| aria-label="Enable drawing" | |
| onClick={handleEnableDrawing} | |
| className="size-9 flex items-center justify-center rounded-l-lg border border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors" | |
| > | |
| <PiScribbleLoopBold className="size-3.5" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom">Enable drawing</TooltipContent> | |
| </Tooltip> | |
| </TooltipProvider> | |
| <DropdownMenu open={dropdownOpen} onOpenChange={setDropdownOpen}> | |
| <TooltipProvider> | |
| <Tooltip delayDuration={100}> | |
| <TooltipTrigger asChild> | |
| <DropdownMenuTrigger asChild> | |
| <button | |
| type="button" | |
| aria-label="Drawing settings" | |
| className="size-9 flex items-center justify-center rounded-r-lg border-y border-r border-slate-300 dark:border-slate-600 bg-white dark:bg-slate-800 hover:bg-slate-50 dark:hover:bg-slate-700 transition-colors" | |
| > | |
| <ChevronDownIcon className="size-3.5" /> | |
| </button> | |
| </DropdownMenuTrigger> |
🤖 Prompt for AI Agents
In `@tauri/src/components/ui/call-center.tsx` around lines 281 - 303, The
icon-only buttons (the button that calls handleEnableDrawing with
PiScribbleLoopBold and the DropdownMenuTrigger button with ChevronDownIcon) need
accessible labels so screen readers announce them; update those button elements
to include descriptive aria-label attributes (e.g., aria-label="Enable drawing"
and aria-label="Open menu") or include visually hidden text inside the button,
and if the dropdown trigger opens a popup add aria-haspopup="menu" and
aria-expanded tied to dropdownOpen; ensure the props are added to the actual
button elements used by TooltipTrigger/DropdownMenuTrigger so assistive tech can
discover them.
konsalex
left a comment
konsalex
left a comment
There was a problem hiding this comment.
Added a minor cosmetic change:
one thing to reflect, is that this "control" panel will end up being pretty complex if we keep adding stuff here.
Also many unrelated to this PR changes to .md files
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.