feat: initial drawing support #217
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a remote drawing subsystem across core and Tauri: per-participant draw state and manager, iced/wgpu overlay renderer and canvas marker, input and event plumbing for draw actions and click animations, frontend drawing UI/components and persistence, tests for remote drawing, and CI/toolchain bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Browser UI
participant Room as RoomService
participant App as Application (Tauri/Core)
participant Gfx as GraphicsContext
participant Iced as IcedRenderer
participant DrawMgr as DrawManager
Frontend->>Room: send ClientEvent (DrawingMode / DrawStart / DrawAddPoint / DrawEnd / ClickAnimation)
Room->>App: emit UserEvent::Drawing*/Draw*/ClickAnimationFromParticipant (with sid)
App->>Gfx: call add_draw_participant/set_drawing_mode/draw_start/draw_add_point/draw_end/clear
Gfx->>DrawMgr: forward per-sid draw lifecycle calls
DrawMgr->>DrawMgr: update in-progress/completed paths, cache geometry
Gfx->>Iced: request overlay draw (frame/view)
Iced->>DrawMgr: request geometries
DrawMgr-->>Iced: return geometries
Iced->>Gfx: compose and present overlay into wgpu frame
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
🤖 Fix all issues with AI agents
In @core/src/graphics/draw.rs:
- Around line 9-14: color_from_hex can panic when the input is shorter than 6
chars; after trimming '#' validate hex.len() and if it's less than 6 return a
safe default (e.g., Color::from_rgb8(0,0,0)) or normalize the string, otherwise
use safe slicing (use hex.get(0..2), hex.get(2..4), hex.get(4..6) and fallback
to "00") and parse each chunk with u8::from_str_radix(..., 16).unwrap_or(0)
before calling Color::from_rgb8 so malformed inputs no longer cause a panic.
In @core/src/graphics/iced_renderer.rs:
- Around line 47-54: The call to device.clone() fails because wgpu::Device is
not Clone; change the code to use an Arc-wrapped device and pass the owned Arc
into Engine::new (or change the function signature to accept Arc<wgpu::Device>),
e.g., ensure the local binding for device is Arc<wgpu::Device> and replace
device.clone() with Arc::clone(&device) when calling Engine::new (keep
queue.clone() and the other arguments such as format and Shell::headless()
unchanged).
In @tauri/src/components/SharingScreen/SharingScreen.tsx:
- Around line 206-212: The local participant is being created with a hardcoded
color "#FF0000" which bypasses the color-cycling logic; replace that with a call
to getOrAssignColor for the local user (e.g., use
getOrAssignColor(LOCAL_PARTICIPANT_ID)) when constructing the DrawParticipant so
the local DrawParticipant uses the same color-assignment path as remote users
(keep drawingMode as the second arg and leave the useEffect deps as-is).
In @tauri/src/components/ui/draw.tsx:
- Around line 42-45: The resizeCanvas function is cumulatively scaling the
context because it calls ctx.scale(dpr, dpr) on each ResizeObserver event
without resetting transforms; update resizeCanvas to reset the canvas transform
before applying the DPR scale (use ctx.resetTransform() or
ctx.setTransform(1,0,0,1,0,0)) and then call ctx.scale(dpr, dpr) so scaling is
applied exactly once per resize; reference the existing
canvasElement.getContext("2d") call, the ctx variable, and the
dpr/devicePixelRatio usage when making this change.
In @tauri/src/components/ui/remote-cursors.tsx:
- Around line 35-61: The handler registered with useDataChannel(CURSORS_TOPIC,
...) parses incoming payloads without error handling; wrap the decoder.decode +
JSON.parse into a try-catch (around the creation of payload: TPMouseMove) and on
parse failure log the error and return early to avoid crashing the handler; keep
the rest of the logic (getAbsolutePosition, getOrAssignColor, CursorParticipant
creation/update, forceUpdate) unchanged and reference
participantName/participantId only after a successful parse.
🧹 Nitpick comments (16)
core/src/input/mouse.rs (1)
674-701: Consider usingif letto avoidunwrap()on line 688.While the
unwrap()is safe due to theis_none()check on line 682, usingif letwould make the safety more explicit and idiomatic.♻️ Suggested refactor
- if extend || last_click_animation_time.is_none() { - last_click_animation_time = Some(Instant::now()); - } - - if last_click_animation_time - .as_ref() - .unwrap() - .elapsed() - .as_millis() - < animation_duration - { + if extend || last_click_animation_time.is_none() { + last_click_animation_time = Some(Instant::now()); + } + + if let Some(ref start_time) = last_click_animation_time { + if start_time.elapsed().as_millis() < animation_duration { std::thread::sleep(std::time::Duration::from_millis(16)); if let Err(e) = tx.send(RedrawThreadCommands::ClickAnimation(false)) { log::error!( "redraw_thread: error sending click animation event: {e:?}" ); } - } else { - last_click_animation_time = None; + } else { + last_click_animation_time = None; + } }core/src/overlay_window.rs (1)
258-264: Consider clarifying method name to indicate logical/scaled pixels.The method returns coordinates scaled by
display_scale, which yields logical (CSS) pixels rather than physical pixels. The current nameget_pixel_positionmight be ambiguous. Consider renaming toget_logical_pixel_positionor adding a doc comment clarifying the output coordinate space.📝 Add documentation
+ /// Converts normalized coordinates (0.0 to 1.0) to logical pixel coordinates. + /// + /// The output is scaled by the display scale factor, yielding coordinates + /// in logical (CSS) pixels suitable for overlay rendering. + /// + /// # Arguments + /// + /// * `x` - The x-coordinate as a percentage (0.0 to 1.0) + /// * `y` - The y-coordinate as a percentage (0.0 to 1.0) + /// + /// # Returns + /// + /// A `Position` struct containing the logical pixel coordinates. pub fn get_pixel_position(&self, x: f64, y: f64) -> Position {tauri/src/windows/window-utils.ts (1)
230-236: Consider adding error handling for consistency.Some similar functions in this file (e.g.,
storeTokenBackend,deleteStoredToken) include try/catch blocks with error logging, while others don't. Consider adding error handling here for debugging purposes, especially since drawing mode persistence failures might be subtle to diagnose.♻️ Add error handling
const getLastDrawingMode = async () => { - return await invoke<string | null>("get_last_drawing_mode"); + try { + return await invoke<string | null>("get_last_drawing_mode"); + } catch (err) { + console.error("Failed to get last drawing mode:", err); + return null; + } }; const setLastDrawingMode = async (mode: string) => { - return await invoke("set_last_drawing_mode", { mode }); + try { + await invoke("set_last_drawing_mode", { mode }); + } catch (err) { + console.error("Failed to set last drawing mode:", err); + } };tauri/src-tauri/src/app_state.rs (1)
331-338: Consider validating the drawing mode string before persisting.The
modeparameter is documented as a JSON string but isn't validated before storage. If invalid JSON or an unexpected format is passed, it could cause deserialization errors when the value is later retrieved and parsed by the frontend.🛡️ Suggested validation approach
Consider adding validation to ensure the string is valid JSON:
pub fn set_last_drawing_mode(&mut self, mode: String) { log::info!("set_last_drawing_mode: {mode}"); + // Validate that the mode is valid JSON + if serde_json::from_str::<serde_json::Value>(&mode).is_err() { + log::error!("set_last_drawing_mode: Invalid JSON format"); + return; + } let _lock = self.lock.lock().unwrap(); self.state.last_drawing_mode = Some(mode); if !self.save() { log::error!("set_last_drawing_mode: Failed to save app state"); } }Alternatively, if you have a specific DrawingMode type definition available in this module, consider deserializing to that type to ensure the mode is not just valid JSON but also a valid drawing mode.
tauri/src/components/ui/cursor-participant.ts (1)
9-16: Consider validating participant identifiers.The constructor accepts
participantIdandparticipantNamewithout validation. Empty strings could potentially cause issues in UI rendering or participant lookups, especially since the related code incore/src/utils/svg_renderer.rstruncates names to 17 characters, suggesting names are expected to have content.🔍 Optional validation approach
constructor(participantId: string, participantName: string, color: string, x: number = -1000, y: number = -1000) { + if (!participantId.trim()) { + throw new Error('participantId cannot be empty'); + } this.participantId = participantId; - this.participantName = participantName; + this.participantName = participantName.trim() || 'Unknown'; this.color = color; this.x = x; this.y = y; this.lastActivity = Date.now(); }This ensures participant IDs are never empty and provides a fallback display name.
tauri/src/components/ui/draw.tsx (2)
60-113: Animation loop runs continuously regardless of drawing activity.The
requestAnimationFrameloop runs indefinitely even when there are no paths to render. This consumes CPU/GPU resources unnecessarily when idle.♻️ Consider adding an idle optimization
useEffect(() => { const canvasElement = canvasRef.current; if (!canvasElement) return; const render = () => { const ctx = canvasElement.getContext("2d"); if (!ctx) return; const logicalWidth = parseFloat(canvasElement.style.width) || canvasElement.getBoundingClientRect().width; const logicalHeight = parseFloat(canvasElement.style.height) || canvasElement.getBoundingClientRect().height; ctx.clearRect(0, 0, logicalWidth, logicalHeight); + let hasContent = false; participants.forEach((participant) => { const paths = participant.getAllPaths(); + if (paths.length > 0) hasContent = true; // ... rest of rendering }); - animationFrameRef.current = requestAnimationFrame(render); + // Only continue animation if there's content to render + if (hasContent) { + animationFrameRef.current = requestAnimationFrame(render); + } else { + animationFrameRef.current = null; + } }; animationFrameRef.current = requestAnimationFrame(render); // ... }, [participants]);Note: You'd also need to trigger the animation loop when new drawing starts.
115-119:classNameapplied twice may cause duplicate styles.The
classNameprop is passed to both the container<div>and the<canvas>. If the caller passes positioning or sizing classes, they may conflict or double-apply.♻️ Consider separating container and canvas classes
- return ( - <div ref={containerRef} className={cn("absolute top-0 left-0 pointer-events-none", className)}> - <canvas ref={canvasRef} className={cn("block w-full h-full", className)} {...props} /> + return ( + <div ref={containerRef} className={cn("absolute top-0 left-0 pointer-events-none", className)}> + <canvas ref={canvasRef} className="block w-full h-full" {...props} /> </div> );tauri/src/components/SharingScreen/Controls.tsx (1)
27-44: Async operation without abort handling on unmount.The
loadCachedDrawingModeasync function could complete after the component unmounts, potentially causing state updates on an unmounted component or setting refs that are no longer relevant.♻️ Add cleanup with abort pattern
useEffect(() => { + let isMounted = true; const loadCachedDrawingMode = async () => { try { const cachedMode = await tauriUtils.getLastDrawingMode(); - if (cachedMode) { + if (cachedMode && isMounted) { const parsed = PDrawingMode.safeParse(JSON.parse(cachedMode)); if (parsed.success && parsed.data.type !== "Disabled") { cachedDrawingModeRef.current = parsed.data; } } } catch (error) { console.error("Failed to load cached drawing mode:", error); } }; loadCachedDrawingMode(); + return () => { + isMounted = false; + }; }, []);tauri/src/components/ui/drawing-layer.tsx (1)
20-26: Payload parsing lacks schema validation.The JSON payload is parsed but not validated against expected schemas. Malformed messages from remote participants could cause runtime errors when accessing nested properties like
payload.payload.point.♻️ Add payload validation
Consider using Zod schemas that are likely already defined in
@/payloadsto validate incoming messages:+import { PDrawStart, PDrawAddPoint, PDrawEnd, ... } from "@/payloads"; let payload: any; try { payload = JSON.parse(payloadStr); } catch (e) { console.error("Failed to parse draw payload:", e); return; } + + if (!payload || typeof payload.type !== "string") { + console.warn("Invalid draw payload structure:", payload); + return; + }tauri/src/components/ui/draw-participant.ts (1)
123-150:getAllPaths()has side effects - mutates state during read.This method both returns paths and performs cleanup by mutating
_completedPaths. This side effect in a getter-like method could be surprising and makes the method non-idempotent. Calling it multiple times in quick succession could trigger multiple callback invocations.♻️ Consider separating cleanup from retrieval
Option 1: Rename to make side effect explicit:
getAndCleanupPaths(): DrawingPath[]Option 2: Separate concerns:
cleanupExpiredPaths(): void { /* mutation logic */ } getAllPaths(): DrawingPath[] { /* read-only */ }The current approach works but the naming could be clearer about its behavior.
core/src/graphics/iced_renderer.rs (1)
97-100: Consider usingunreachable!()orexpect()instead ofpanic!()for invariant violations.Since
self.rendereris always constructed asRenderer::Primary(...)innew(), this branch should never execute. Usingunreachable!()would better communicate this invariant:♻️ Suggested improvement
let wgpu_renderer = match &mut self.renderer { Renderer::Primary(renderer) => renderer, - _ => panic!("Expected primary renderer"), + _ => unreachable!("IcedRenderer is always constructed with Primary variant"), };core/src/graphics/marker.rs (1)
16-63: Consider making marker dimensions configurable or DPI-aware.The marker dimensions are hardcoded to 40x40 pixels. On high-DPI displays, these may appear too small. Consider making dimensions relative to display scale or configurable.
♻️ Suggested improvement
+const MARKER_SIZE: f32 = 40.0; + pub fn draw(&self, renderer: &Renderer, bounds: Rectangle) -> canvas::Geometry { self.cache.draw(renderer, bounds.size(), |frame| { let image_handle = iced_core::image::Image::new(self.marker.clone()); - let width = 40.0; - let height = 40.0; + let width = MARKER_SIZE; + let height = MARKER_SIZE;core/tests/src/remote_cursor.rs (1)
194-196: Unused variabledelayininternal_cursor_click.The
delayvariable is declared but only used once. Consider inlining it or using_delayprefix if intentionally unused in some code paths.♻️ Minor cleanup
async fn internal_cursor_click(room: &Room, x: f64, y: f64) -> io::Result<()> { - let delay = Duration::from_millis(20); // Small delay between movements println!("Performing left click at ({x}, {y})"); send_mouse_click(room, x, y, 0).await?; // 0 for left button @@ -204,7 +203,7 @@ async fn internal_cursor_click(room: &Room, x: f64, y: f64) -> io::Result<()> { let x = x + 0.1; println!("Moving right to ({x}, {y})"); send_mouse_move(room, x, y).await?; - sleep(delay).await; + sleep(Duration::from_millis(20)).await;core/src/lib.rs (1)
897-919: Consider consolidating pointer visibility logic for drawing modes.The
DrawingModehandler enables pointer visibility for all non-Disabledmodes using a catch-all_ =>pattern. This is correct but may want explicit handling if new modes with different visibility requirements are added in the future.💡 Future-proofing suggestion
Consider documenting or explicitly matching all
DrawingModevariants to make visibility behavior explicit:match &drawing_mode { DrawingMode::Disabled => { remote_control .cursor_controller .set_controller_pointer_enabled(false, sid.as_str()); } - _ => { + DrawingMode::Draw(_) | DrawingMode::ClickAnimation => { remote_control .cursor_controller .set_controller_pointer_enabled(true, sid.as_str()); } }core/tests/src/remote_drawing.rs (1)
329-385: Unused code:draw_letteranddraw_hello_worldare defined but never called.These functions and the
LetterStroke/get_letter_strokesinfrastructure (lines 150-326) appear to be unused in the test suite. Consider either:
- Adding a test that uses this capability
- Removing unused code to reduce maintenance burden
Would you like me to create a test case that exercises the letter drawing functionality, or should this code be removed?
core/src/graphics/draw.rs (1)
191-313: LGTM!The
DrawManagerprovides clean per-participant drawing state management with appropriate logging and defensive handling for missing participants. The rendering strategy of caching completed paths per-Draw while batching all in-progress paths into a single frame is efficient.Consider adding
impl Default for DrawManagerto satisfy theDefaulttrait, which is idiomatic for types with a parameterless constructor.Optional: Add Default impl
+impl Default for DrawManager { + fn default() -> Self { + Self::new() + } +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
core/Cargo.tomlcore/src/graphics/draw.rscore/src/graphics/graphics_context.rscore/src/graphics/iced_canvas.rscore/src/graphics/iced_renderer.rscore/src/graphics/marker.rscore/src/input/mouse.rscore/src/lib.rscore/src/overlay_window.rscore/src/room_service.rscore/tests/src/events.rscore/tests/src/main.rscore/tests/src/remote_cursor.rscore/tests/src/remote_drawing.rstauri/src-tauri/src/app_state.rstauri/src-tauri/src/main.rstauri/src/components/SharingScreen/Controls.tsxtauri/src/components/SharingScreen/SharingScreen.tsxtauri/src/components/ui/cursor-participant.tstauri/src/components/ui/draw-participant.tstauri/src/components/ui/draw.tsxtauri/src/components/ui/drawing-layer.tsxtauri/src/components/ui/remote-cursors.tsxtauri/src/components/ui/segmented-control.tsxtauri/src/lib/utils.tstauri/src/payloads.tstauri/src/windows/screensharing/context.tsxtauri/src/windows/window-utils.ts
🧰 Additional context used
🧬 Code graph analysis (13)
tauri/src-tauri/src/app_state.rs (2)
tauri/src-tauri/src/main.rs (1)
set_last_drawing_mode(444-449)tauri/src-tauri/src/lib.rs (1)
app_state(1-1)
tauri/src/components/ui/draw.tsx (1)
tauri/src/components/ui/draw-participant.ts (1)
DrawParticipant(10-151)
tauri/src-tauri/src/main.rs (1)
tauri/src-tauri/src/app_state.rs (1)
set_last_drawing_mode(331-338)
tauri/src/components/ui/cursor-participant.ts (1)
core/src/utils/svg_renderer.rs (1)
name(103-103)
tauri/src/windows/screensharing/context.tsx (2)
tauri/src/payloads.ts (1)
TDrawingMode(163-163)tauri/src/components/ui/draw-participant.ts (2)
drawingMode(46-48)setDrawingMode(53-59)
tauri/src/components/SharingScreen/SharingScreen.tsx (6)
tauri/src/windows/screensharing/context.tsx (1)
useSharingContext(21-27)tauri/src/components/ui/draw-participant.ts (2)
drawingMode(46-48)DrawParticipant(10-151)tauri/src/payloads.ts (8)
TPDrawClearPath(139-139)TPDrawingModeEvent(169-169)TPDrawAddPoint(119-119)TPMouseMove(17-17)TPClickAnimation(131-131)TPDrawStart(113-113)TPDrawEnd(125-125)TPDrawClearAllPaths(144-144)tauri/src/lib/utils.ts (1)
applyCursorRippleEffect(31-46)tauri/src/components/ui/drawing-layer.tsx (1)
DrawingLayer(15-71)tauri/src/components/ui/remote-cursors.tsx (1)
RemoteCursors(15-97)
core/src/graphics/graphics_context.rs (1)
core/src/graphics/iced_renderer.rs (10)
draw(73-102)new(39-71)add_draw_participant(104-106)remove_draw_participant(108-110)set_drawing_mode(112-114)draw_start(116-118)draw_add_point(120-122)draw_end(124-126)draw_clear_path(128-130)draw_clear_all_paths(132-134)
core/tests/src/remote_drawing.rs (3)
tauri/src/lib/utils.ts (1)
sleep(29-29)core/tests/src/screenshare_client.rs (2)
start_screenshare_session(92-119)stop_screenshare_session(121-126)tauri/src/services/socket.ts (1)
connect(45-60)
tauri/src/components/ui/remote-cursors.tsx (3)
tauri/src/components/ui/cursor-participant.ts (1)
CursorParticipant(1-48)tauri/src/payloads.ts (1)
TPMouseMove(17-17)tauri/src/lib/utils.ts (1)
getAbsolutePosition(20-27)
core/src/graphics/marker.rs (1)
core/src/graphics/iced_canvas.rs (3)
new(27-29)new(55-59)draw(35-46)
tauri/src/components/ui/drawing-layer.tsx (3)
tauri/src/components/ui/draw-participant.ts (1)
DrawParticipant(10-151)tauri/src/lib/utils.ts (1)
applyCursorRippleEffect(31-46)tauri/src/components/ui/draw.tsx (1)
Draw(124-124)
core/src/graphics/draw.rs (3)
core/src/graphics/iced_renderer.rs (3)
new(39-71)set_drawing_mode(112-114)draw(73-102)core/src/graphics/graphics_context.rs (3)
new(174-311)set_drawing_mode(465-467)draw(371-422)core/src/graphics/iced_canvas.rs (4)
new(27-29)new(55-59)set_drawing_mode(78-80)draw(35-46)
core/src/graphics/iced_renderer.rs (2)
core/src/graphics/iced_canvas.rs (13)
fmt(21-23)new(27-29)new(55-59)draw(35-46)view(61-68)add_draw_participant(70-72)remove_draw_participant(74-76)set_drawing_mode(78-80)draw_start(82-84)draw_add_point(86-88)draw_end(90-92)draw_clear_path(94-96)draw_clear_all_paths(98-100)core/src/graphics/marker.rs (2)
new(9-14)draw(16-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: precommit / pre-commit
- GitHub Check: backend-go / test-backend-go
🔇 Additional comments (46)
tauri/src/components/ui/segmented-control.tsx (1)
25-27: LGTM! Good fix for animation isolation.Using
React.useId()to generate per-instancelayoutIdvalues correctly prevents Framer Motion layout animations from conflicting when multipleSegmentedControlcomponents are rendered simultaneously.tauri/src/payloads.ts (1)
96-169: LGTM! Well-structured drawing payload schemas.The Zod schemas follow established patterns in the file, with proper use of discriminated unions for
PDrawingModeand good schema composition (e.g.,PDrawPathPointreusingPClientPoint). The type exports are consistent with existing conventions.core/src/input/mouse.rs (1)
1288-1329: LGTM! Clean additions for drawing support.The new public accessors (
get_participant_color,get_overlay_window) andtrigger_click_animationmethod are well-documented and follow the existing patterns in the codebase. Thedraw_path_endedplaceholder is acceptable for initial implementation.tauri/src/lib/utils.ts (1)
31-46: No issues identified. Theclick-rippleclass andclick-ripple-effectanimation are properly defined intauri/src/App.cssand correctly referenced by the function.core/Cargo.toml (1)
49-74: No changes needed. iced_wgpu 0.14.0 explicitly requireswgpu ^27.0, which includes version 27.0.1 in the Cargo.toml.Likely an incorrect or invalid review comment.
tauri/src/windows/screensharing/context.tsx (1)
1-60: LGTM! Clean integration of drawing mode into the sharing context.The implementation correctly follows React patterns for context state management. The default value
{ type: "Disabled" }aligns with the DrawingMode enum structure, and type safety is properly maintained with TypeScript.tauri/src-tauri/src/main.rs (2)
433-449: LGTM! Commands follow established patterns.The
get_last_drawing_modeandset_last_drawing_modecommands correctly follow the same pattern as the existing microphone preference commands, with proper logging and state management through theMutex<AppData>guard.
1000-1001: LGTM! Commands properly registered.The new drawing mode commands are correctly added to the
invoke_handlermacro, making them available to the frontend.tauri/src/components/ui/cursor-participant.ts (1)
18-47: LGTM! Clean cursor state management implementation.The methods provide clear, well-documented functionality for cursor position tracking, activity monitoring, and inactivity-based hiding. The 5-second default timeout for
shouldHideis reasonable for typical user interaction patterns.core/tests/src/events.rs (1)
80-90: TheDrawingModeenum correctly uses#[serde(tag = "type", content = "settings")]for adjacently tagged serialization. This is the appropriate representation for mixed variant types, where unit variants likeDisabledandClickAnimationserialize as{"type":"VariantName"}and theDrawvariant serializes as{"type":"Draw","settings":{...}}. This is standard serde behavior and works as expected.tauri/src/components/SharingScreen/Controls.tsx (1)
138-205: LGTM on the drawing settings dropdown UI.The conditional rendering of the settings dropdown with nested
SegmentedControland permanent mode toggle is well-structured. The tooltip integration withDropdownMenuTriggerusingasChildis correctly composed.tauri/src/components/ui/drawing-layer.tsx (1)
70-70: Draw component receives static Map reference.
drawParticipantsRef.currentis passed directly. Since it's a ref, theDrawcomponent'sparticipantsprop will always be the same Map reference. TheDrawcomponent'suseEffectwith[participants]dependency won't re-run when new participants are added.This works because
Drawuses a continuous animation loop that reads the current Map state each frame, but it's worth noting this implicit coupling.tauri/src/components/ui/remote-cursors.tsx (1)
21-33: Edge case ingenerateUniqueNamewhen name has no space.If
participantNamehas no space,fullName.indexOf(" ")returns-1, sojstarts at1. The while loop then slices from0to incrementingj, effectively adding one character at a time from the start. This works but may produce odd results for single-word names that collide (e.g., "Alice" → "A", "Al", "Ali"...).Consider if this behavior is intentional for single-word names.
core/tests/src/main.rs (1)
101-111: LGTM on drawing test infrastructure.The new
DrawingTestenum and correspondingCommands::Drawingbranch follow the established patterns in the test harness. Test variant naming clearly describes each scenario.Also applies to: 220-240
tauri/src/components/SharingScreen/SharingScreen.tsx (4)
115-126: Path ID wrap-around could cause ID collisions.When
pathIdCounterRefwraps fromMAX_SAFE_INTEGERback to0, there's a theoretical risk of ID collision with older paths that haven't been cleared yet. In practice, this is extremely unlikely given the magnitude ofMAX_SAFE_INTEGER, but worth noting.The implementation is correct for practical purposes. The wrap-around handles the edge case gracefully.
273-310: Mouse move handler correctly branches for drawing vs. cursor movement.The logic properly checks
isDrawingMode && (e.buttons & 1) === 1to differentiate between drawing (left button held) and normal cursor movement. Both local state and remote publishing are synchronized.
418-438: Context menu intercept only works in permanent Draw mode.Right-click to clear all paths is only available when
drawingMode.type === "Draw" && drawingMode.settings.permanent. In non-permanent mode or ClickAnimation mode, right-click does nothing special (just prevents default). Consider if users might expect a way to manually clear non-permanent drawings.Verify this is the intended UX - only permanent mode gets the "clear all" right-click action.
753-755: LGTM on DrawingLayer and RemoteCursors integration.The components are correctly wired with shared
videoRefandgetOrAssignColorcallback, ensuring consistent color assignment across drawing and cursor rendering.tauri/src/components/ui/draw-participant.ts (2)
61-96: LGTM on path lifecycle methods.
handleDrawStart,handleDrawAddPoint, andhandleDrawEndcorrectly manage the in-progress path lifecycle. The timestamp addition inhandleDrawEndfor non-permanent mode enables the lazy cleanup mechanism.
53-59: Setting mode to "Disabled" clears all paths immediately.
setDrawingModecallsclear()when the mode is "Disabled", which removes all paths without invoking the_onPathRemovedcallback. This means remote participants won't receiveDrawClearPathevents for these paths.Verify this is intentional behavior. When a user switches to "controlling" mode (Disabled), should their existing drawings be cleared for everyone or just locally?
core/src/graphics/iced_renderer.rs (1)
104-134: LGTM!The drawing participant management methods cleanly delegate to the
OverlaySurface, maintaining a clear separation of concerns between the renderer and the drawing subsystem.core/src/graphics/marker.rs (1)
3-6: LGTM!The
Markerstruct properly uses iced'scanvas::Cachefor efficient rendering, which automatically invalidates when bounds change.core/tests/src/remote_cursor.rs (1)
1-1: LGTM!Import cleanup aligns with the removal of click animation tests that are now covered by the new
remote_drawing.rstest module.core/src/lib.rs (3)
786-800: LGTM!Good approach to associate participants with draw colors on connection. The
if let Somepattern properly handles the case whereget_participant_colormight returnNone.
920-971: LGTM!The drawing event handlers correctly translate normalized coordinates to pixel positions via the overlay window and synchronize cursor position with drawing operations.
1098-1104: LGTM!The new
UserEventvariants comprehensively cover the drawing lifecycle and click animation events, with appropriate data structures for each operation.core/src/graphics/iced_canvas.rs (3)
12-13: LGTM!The empty
Messageenum is required by iced's type system even when no messages are produced. The derived traits enable proper hashing and comparison.
32-46: LGTM!The
canvas::Programimplementation correctly composes geometries from both the marker and draw manager, maintaining proper rendering order with markers first.
61-67: Verifyview()is not called excessively in the render loop.Creating a new
OverlaySurfaceCanvason eachview()call is the standard iced pattern. However, ensure this isn't called more frequently than necessary, as it allocates on each invocation.core/src/graphics/graphics_context.rs (4)
28-33: LGTM!New module declarations properly expose the iced_renderer and draw modules for the drawing subsystem integration.
291-298: LGTM!The
IcedRendereris properly initialized with all required GPU resources, adapter, window, and texture path. This follows the established pattern of the other renderers.
415-421: Verify render order: iced overlay draws after cursor and click animations.The iced renderer draws after the main render pass is submitted. Ensure this layering is intentional—drawings will appear on top of cursors and click animations.
442-512: LGTM!The drawing management methods are well-documented and cleanly delegate to the
IcedRenderer. The documentation clearly explains each method's purpose and parameters.core/tests/src/remote_drawing.rs (5)
40-54: Consider makingreliableconfigurable or documenting the tradeoff.
DrawAddPointusesreliable: falseto reduce latency, but this could cause rendering gaps if packets are dropped. This is a valid tradeoff for real-time drawing, but consider documenting this decision.
389-446: LGTM!Comprehensive test for click animation mode covering various screen positions and a rapid-fire sequence to stress-test the animation system.
634-688: LGTM!Good test coverage for individual path clearing with distinct
path_idvalues and proper sequencing.
691-750: LGTM!Good test for bulk path clearing, demonstrating the grid pattern and
DrawClearAllPathsfunctionality.
527-589: No action required—path_id is already properly scoped per-participant.The
DrawManagerusesHashMap<String, Draw>to store each participant's drawing state separately (keyed bysid). Whendraw_startis called, it retrieves the participant's isolatedDrawinstance viaself.draws.get_mut(sid)before starting a path. Each participant's paths are stored independently, so using the samepath_idvalues (0, 1, 2) across concurrent participants causes no conflict.Likely an incorrect or invalid review comment.
core/src/graphics/draw.rs (2)
17-30: LGTM!Clean data structure for representing a drawing path with its identifier.
32-189: LGTM!Well-structured drawing state management with proper cache invalidation, defensive mode checks, and clean two-pass glow rendering. The separation between in-progress and completed paths with caching for completed geometry is a good design choice.
core/src/room_service.rs (6)
672-678: LGTM!Adding
CloneandCopyderives for a simple struct containing onlyCopytypes is appropriate and enables more ergonomic usage.
680-690: LGTM!Well-documented struct combining coordinates with path identification.
802-807: LGTM!Simple settings structure with appropriate derives.
809-819: LGTM!The tagged enum design with serde attributes provides clean JSON serialization. The
PartialEqandEqderives enable the mode comparison checks used inDraw.
850-863: LGTM!The new event variants comprehensively cover the drawing lifecycle. Minor note:
DrawClearPathuses struct variant syntax{ path_id: u64 }while other single-value variants use tuple syntax - this is valid and provides self-documenting field names in the serialized JSON.
958-977: LGTM!The new event handlers correctly map
ClientEventvariants toUserEventvariants, consistently passing the participantsidfor proper per-participant tracking in the drawing system.
| let engine = Engine::new( | ||
| adapter, | ||
| device.clone(), | ||
| queue.clone(), | ||
| format, | ||
| Some(iced_wgpu::graphics::Antialiasing::MSAAx4), | ||
| Shell::headless(), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n core/src/graphics/iced_renderer.rs | head -80Repository: gethopp/hopp
Length of output: 2861
🌐 Web query:
wgpu Device Queue Clone trait implementation
💡 Result:
Short answer:
- wgpu::Queue implements Clone (it’s #[derive(Clone)]). [1]
- wgpu::Device does not provide a Clone implementation; if you need clonable/ sharable access to a Device, wrap it in Arc (or Arc). [2][3]
Sources:
- wgpu Queue source (derive Clone). [1]
- wgpu Device documentation/source (no Clone impl). [2]
- Rust Arc doc (use Arc to share non-Clone types). [3]
🌐 Web query:
iced_wgpu Engine::new signature parameters
💡 Result:
Signature (from iced_wgpu::Engine::new):
pub fn new(
_adapter: &wgpu::Adapter,
device: wgpu::Device,
queue: wgpu::Queue,
format: wgpu::TextureFormat,
antialiasing: Option,
shell: Shell,
) -> Self
Parameters:
- _adapter: &wgpu::Adapter — reference to the chosen wgpu adapter. [1]
- device: wgpu::Device — the wgpu device. [1]
- queue: wgpu::Queue — the wgpu queue. [1]
- format: wgpu::TextureFormat — the target surface texture format. [1]
- antialiasing: Option — optional antialiasing configuration. [1][2]
- shell: Shell — iced graphics Shell passed to the engine. [1]
Sources: iced_wgpu engine.rs (source) and docs. [1][2]
🏁 Script executed:
# Check if there are any cargo error files or build output that might show compilation errors
find . -name "*.rs" -path "*/core/src/graphics/*" -type f | head -20Repository: gethopp/hopp
Length of output: 365
Fix compilation error: device.clone() on non-Clone type.
wgpu::Device does not implement Clone. The call to device.clone() at line 49 will fail to compile. Since iced_wgpu::Engine::new expects owned values (device: wgpu::Device, not &wgpu::Device), wrap the device in Arc<wgpu::Device> and pass it as an owned value, or restructure to avoid cloning:
- Pass
Arc::clone(device)ifdeviceisArc<wgpu::Device> - Or accept
deviceparameter asArc<wgpu::Device>instead of&wgpu::Device
Note: wgpu::Queue implements Clone, so queue.clone() at line 50 is valid.
🤖 Prompt for AI Agents
In @core/src/graphics/iced_renderer.rs around lines 47 - 54, The call to
device.clone() fails because wgpu::Device is not Clone; change the code to use
an Arc-wrapped device and pass the owned Arc into Engine::new (or change the
function signature to accept Arc<wgpu::Device>), e.g., ensure the local binding
for device is Arc<wgpu::Device> and replace device.clone() with
Arc::clone(&device) when calling Engine::new (keep queue.clone() and the other
arguments such as format and Shell::headless() unchanged).
| useEffect(() => { | ||
| if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) { | ||
| // Get or assign color for local participant (only assigns on first encounter) | ||
| const localDrawParticipant = new DrawParticipant("#FF0000", drawingMode); | ||
| drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant); | ||
| } | ||
| }, [drawingMode, getOrAssignColor]); |
There was a problem hiding this comment.
Local participant uses hardcoded color instead of getOrAssignColor.
The local participant is created with "#FF0000" hardcoded, while remote participants use getOrAssignColor. This creates visual inconsistency where the local user always sees red for their drawings regardless of the color cycling system.
🐛 Proposed fix
useEffect(() => {
if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) {
- const localDrawParticipant = new DrawParticipant("#FF0000", drawingMode);
+ const localColor = getOrAssignColor(LOCAL_PARTICIPANT_ID);
+ const localDrawParticipant = new DrawParticipant(localColor, drawingMode);
drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant);
}
}, [drawingMode, getOrAssignColor]);📝 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.
| useEffect(() => { | |
| if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) { | |
| // Get or assign color for local participant (only assigns on first encounter) | |
| const localDrawParticipant = new DrawParticipant("#FF0000", drawingMode); | |
| drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant); | |
| } | |
| }, [drawingMode, getOrAssignColor]); | |
| useEffect(() => { | |
| if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) { | |
| // Get or assign color for local participant (only assigns on first encounter) | |
| const localColor = getOrAssignColor(LOCAL_PARTICIPANT_ID); | |
| const localDrawParticipant = new DrawParticipant(localColor, drawingMode); | |
| drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant); | |
| } | |
| }, [drawingMode, getOrAssignColor]); |
🤖 Prompt for AI Agents
In @tauri/src/components/SharingScreen/SharingScreen.tsx around lines 206 - 212,
The local participant is being created with a hardcoded color "#FF0000" which
bypasses the color-cycling logic; replace that with a call to getOrAssignColor
for the local user (e.g., use getOrAssignColor(LOCAL_PARTICIPANT_ID)) when
constructing the DrawParticipant so the local DrawParticipant uses the same
color-assignment path as remote users (keep drawingMode as the second arg and
leave the useEffect deps as-is).
| const ctx = canvasElement.getContext("2d"); | ||
| if (ctx) { | ||
| ctx.scale(dpr, dpr); | ||
| } |
There was a problem hiding this comment.
Cumulative DPR scaling on resize.
Each call to resizeCanvas applies ctx.scale(dpr, dpr) without resetting the transform first. When the ResizeObserver fires multiple times, the scaling accumulates, causing drawing coordinates to be incorrect.
🐛 Proposed fix
const ctx = canvasElement.getContext("2d");
if (ctx) {
+ ctx.setTransform(1, 0, 0, 1, 0, 0); // Reset transform before scaling
ctx.scale(dpr, dpr);
}📝 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.
| const ctx = canvasElement.getContext("2d"); | |
| if (ctx) { | |
| ctx.scale(dpr, dpr); | |
| } | |
| const ctx = canvasElement.getContext("2d"); | |
| if (ctx) { | |
| ctx.setTransform(1, 0, 0, 1, 0, 0); // Reset transform before scaling | |
| ctx.scale(dpr, dpr); | |
| } |
🤖 Prompt for AI Agents
In @tauri/src/components/ui/draw.tsx around lines 42 - 45, The resizeCanvas
function is cumulatively scaling the context because it calls ctx.scale(dpr,
dpr) on each ResizeObserver event without resetting transforms; update
resizeCanvas to reset the canvas transform before applying the DPR scale (use
ctx.resetTransform() or ctx.setTransform(1,0,0,1,0,0)) and then call
ctx.scale(dpr, dpr) so scaling is applied exactly once per resize; reference the
existing canvasElement.getContext("2d") call, the ctx variable, and the
dpr/devicePixelRatio usage when making this change.
| useDataChannel(CURSORS_TOPIC, (msg) => { | ||
| const decoder = new TextDecoder(); | ||
| const payload: TPMouseMove = JSON.parse(decoder.decode(msg.payload)); | ||
|
|
||
| if (!videoRef.current) return; | ||
|
|
||
| const { absoluteX, absoluteY } = getAbsolutePosition(videoRef.current, payload); | ||
| const participantName = msg.from?.name ?? "Unknown"; | ||
| const participantId = msg.from?.identity ?? "Unknown"; | ||
|
|
||
| if (participantId === "Unknown") return; | ||
|
|
||
| const color = getOrAssignColor(participantId); | ||
| let cursorParticipant = cursorParticipantsRef.current.get(participantId); | ||
|
|
||
| if (!cursorParticipant) { | ||
| const existingNames = Array.from(cursorParticipantsRef.current.values()).map((cp) => cp.participantName); | ||
| const uniqueName = generateUniqueName(participantName, existingNames); | ||
|
|
||
| cursorParticipant = new CursorParticipant(participantId, uniqueName, color, absoluteX, absoluteY); | ||
| cursorParticipantsRef.current.set(participantId, cursorParticipant); | ||
| } else { | ||
| cursorParticipant.updatePosition(absoluteX, absoluteY); | ||
| } | ||
|
|
||
| forceUpdate(); | ||
| }); |
There was a problem hiding this comment.
Missing try-catch around JSON.parse.
Unlike drawing-layer.tsx which wraps JSON parsing in try-catch, this handler directly parses the payload. Malformed messages will cause uncaught exceptions.
🐛 Proposed fix
useDataChannel(CURSORS_TOPIC, (msg) => {
const decoder = new TextDecoder();
- const payload: TPMouseMove = JSON.parse(decoder.decode(msg.payload));
+ let payload: TPMouseMove;
+ try {
+ payload = JSON.parse(decoder.decode(msg.payload));
+ } catch (e) {
+ console.error("Failed to parse cursor payload:", e);
+ return;
+ }
if (!videoRef.current) return;
// ...
});📝 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.
| useDataChannel(CURSORS_TOPIC, (msg) => { | |
| const decoder = new TextDecoder(); | |
| const payload: TPMouseMove = JSON.parse(decoder.decode(msg.payload)); | |
| if (!videoRef.current) return; | |
| const { absoluteX, absoluteY } = getAbsolutePosition(videoRef.current, payload); | |
| const participantName = msg.from?.name ?? "Unknown"; | |
| const participantId = msg.from?.identity ?? "Unknown"; | |
| if (participantId === "Unknown") return; | |
| const color = getOrAssignColor(participantId); | |
| let cursorParticipant = cursorParticipantsRef.current.get(participantId); | |
| if (!cursorParticipant) { | |
| const existingNames = Array.from(cursorParticipantsRef.current.values()).map((cp) => cp.participantName); | |
| const uniqueName = generateUniqueName(participantName, existingNames); | |
| cursorParticipant = new CursorParticipant(participantId, uniqueName, color, absoluteX, absoluteY); | |
| cursorParticipantsRef.current.set(participantId, cursorParticipant); | |
| } else { | |
| cursorParticipant.updatePosition(absoluteX, absoluteY); | |
| } | |
| forceUpdate(); | |
| }); | |
| useDataChannel(CURSORS_TOPIC, (msg) => { | |
| const decoder = new TextDecoder(); | |
| let payload: TPMouseMove; | |
| try { | |
| payload = JSON.parse(decoder.decode(msg.payload)); | |
| } catch (e) { | |
| console.error("Failed to parse cursor payload:", e); | |
| return; | |
| } | |
| if (!videoRef.current) return; | |
| const { absoluteX, absoluteY } = getAbsolutePosition(videoRef.current, payload); | |
| const participantName = msg.from?.name ?? "Unknown"; | |
| const participantId = msg.from?.identity ?? "Unknown"; | |
| if (participantId === "Unknown") return; | |
| const color = getOrAssignColor(participantId); | |
| let cursorParticipant = cursorParticipantsRef.current.get(participantId); | |
| if (!cursorParticipant) { | |
| const existingNames = Array.from(cursorParticipantsRef.current.values()).map((cp) => cp.participantName); | |
| const uniqueName = generateUniqueName(participantName, existingNames); | |
| cursorParticipant = new CursorParticipant(participantId, uniqueName, color, absoluteX, absoluteY); | |
| cursorParticipantsRef.current.set(participantId, cursorParticipant); | |
| } else { | |
| cursorParticipant.updatePosition(absoluteX, absoluteY); | |
| } | |
| forceUpdate(); | |
| }); |
🤖 Prompt for AI Agents
In @tauri/src/components/ui/remote-cursors.tsx around lines 35 - 61, The handler
registered with useDataChannel(CURSORS_TOPIC, ...) parses incoming payloads
without error handling; wrap the decoder.decode + JSON.parse into a try-catch
(around the creation of payload: TPMouseMove) and on parse failure log the error
and return early to avoid crashing the handler; keep the rest of the logic
(getAbsolutePosition, getOrAssignColor, CursorParticipant creation/update,
forceUpdate) unchanged and reference participantName/participantId only after a
successful parse.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @.github/workflows/hopp_core_reusable.yml:
- Around line 71-73: Remove the redundant toolchain input: when using
dtolnay/rust-toolchain@1.92.0 the version is already specified in the action
reference, so delete the toolchain: "1.92.0" parameter from that workflow step
(or alternatively switch the action to dtolnay/rust-toolchain@master if you
intend to keep specifying toolchain via the toolchain input).
In @core/src/graphics/draw.rs:
- Around line 213-217: The add_participant method currently unconditionally
overwrites any existing Draw for a given sid; update
DrawManager::add_participant to first check self.draws.contains_key(&sid) and if
present either log a warning (e.g., via log::warn!) and return early or
otherwise avoid inserting a new Draw, so existing drawing state isn't lost;
reference the existing symbols add_participant, self.draws, sid, and Draw::new
when making this change.
- Around line 174-184: The glow alpha is being compounded by multiplying
self.color.a by 0.60 in make_glow_stroke, which makes already-transparent colors
even fainter; change the logic to set the glow alpha to a fixed value (e.g.,
0.60) rather than multiplying so the glow has consistent intensity regardless of
the input color's alpha. Update make_glow_stroke to derive glow_color from
self.color but assign glow_color.a = 0.60 (or another chosen constant) and leave
the rest of the Stroke construction unchanged.
- Around line 79-87: start_path currently overwrites self.in_progress_path and
drops any existing path; update it to handle an existing in-progress path by
checking if self.in_progress_path.is_some() and either auto-finishing it or
rejecting the new start. Implement the auto-finish option by calling the
existing finish_path routine (or a new helper like finish_current_path) to
finalize and store the current DrawPath before assigning self.in_progress_path =
Some(DrawPath::new(path_id, point)); if you choose the ignore-new option, log a
warn including the incoming path_id and return without replacing
in_progress_path. Ensure you reference start_path, in_progress_path,
DrawPath::new and finish_path (or the new helper) when making the change.
In @tauri/src/components/SharingScreen/SharingScreen.tsx:
- Around line 122-133: The getNextPathId() wrap-around can cause ID collisions
when old paths remain after pathIdCounterRef resets; replace the numeric counter
with a collision-resistant ID (e.g., UUID) or implement reclamation: change
getNextPathId to return a UUID (using v4 from 'uuid') or implement a pool that
reclaims freed IDs and checks activePaths before returning an id; update all
callers that expect numeric ids (references to getNextPathId and
pathIdCounterRef and any comparisons against MAX_PATH_ID) to accept string IDs
or to use the new pool API so collisions cannot occur after wrap-around.
- Around line 439-460: The right-click handler in handleContextMenu immediately
clears all drawings when isDrawingMode && drawingMode.type === "Draw" &&
drawingMode.settings.permanent; change this to prompt for confirmation and
provide feedback/undo: before constructing the TPDrawClearAllPaths payload and
calling localParticipant.localParticipant?.publishData, show a confirmation
dialog (or toast with "Undo") asking the user to confirm clearing; only on
confirm send the TPDrawClearAllPaths event and then update the local draw
participant via drawParticipantsRef.current.get(LOCAL_PARTICIPANT_ID)?.clear();
additionally push the current drawing state to an undo stack so the toast "Undo"
action can republish the previous state to restore drawings.
- Around line 212-219: The useEffect initializing the local draw participant
claims to "Get or assign color" but constructs DrawParticipant with a hardcoded
"#FF0000" and never calls getOrAssignColor even though it is in the dependency
array; change the effect to call getOrAssignColor(LOCAL_PARTICIPANT_ID) to
obtain the color when creating the new DrawParticipant (keep drawingMode as
before) and remove getOrAssignColor from the dependency array only if you choose
to instead hardcode the color—otherwise keep getOrAssignColor in deps and use it
to assign the color; update the comment to match the chosen approach.
- Line 517: The useEffect that currently lists dependencies [isSharingMouse,
isDrawingMode, drawingMode, updateMouseControls] must include
localParticipant.localParticipant to avoid throttled handlers capturing a stale
participant reference; update the dependency array to [isSharingMouse,
isDrawingMode, drawingMode, updateMouseControls,
localParticipant.localParticipant] (or alternatively refactor the throttled
callbacks to read localParticipant.localParticipant from a ref such as
isRemoteControlEnabledRef) so the effect re-runs when the local participant
object changes.
🧹 Nitpick comments (2)
core/src/graphics/draw.rs (2)
8-24: Consider more robust hex color parsing.The current implementation has a few limitations:
- Only validates minimum length (6), not exact length, so 7-8+ character strings are silently truncated
- Silent fallback to
0on parse errors viaunwrap_or(0)could mask malformed hex digits (e.g.,"#GGGGGG"→#000000)- Doesn't support common short-form hex codes like
#FFFor alpha channel (#RRGGBBAA)Consider validating the exact length and returning a Result or more explicit error handling for malformed input.
♻️ Optional: More robust color parsing
fn color_from_hex(hex: &str) -> Color { let hex = hex.trim_start_matches('#'); - // Check if the hex string has at least 6 characters to avoid panic - if hex.len() < 6 { + // Support both 3-char (#FFF) and 6-char (#FFFFFF) formats + let (r, g, b) = if hex.len() == 3 { + let r = u8::from_str_radix(&hex[0..1].repeat(2), 16); + let g = u8::from_str_radix(&hex[1..2].repeat(2), 16); + let b = u8::from_str_radix(&hex[2..3].repeat(2), 16); + (r, g, b) + } else if hex.len() == 6 { + let r = u8::from_str_radix(&hex[0..2], 16); + let g = u8::from_str_radix(&hex[2..4], 16); + let b = u8::from_str_radix(&hex[4..6], 16); + (r, g, b) + } else { log::warn!( - "color_from_hex: invalid hex color '{}', using default black color", + "color_from_hex: invalid hex color length '{}', using default black color", hex ); - return Color::from_rgb8(0, 0, 0); - } + return Color::from_rgb8(0, 0, 0); + }; - let r = u8::from_str_radix(&hex[0..2], 16).unwrap_or(0); - let g = u8::from_str_radix(&hex[2..4], 16).unwrap_or(0); - let b = u8::from_str_radix(&hex[4..6], 16).unwrap_or(0); - Color::from_rgb8(r, g, b) + match (r, g, b) { + (Ok(r), Ok(g), Ok(b)) => Color::from_rgb8(r, g, b), + _ => { + log::warn!( + "color_from_hex: invalid hex digits in '{}', using default black color", + hex + ); + Color::from_rgb8(0, 0, 0) + } + } }
71-77: Remove unnecessary clone.Line 72 clones
modebefore assigning, but the value is consumed immediately. IfDrawingModeimplementsCopy, you can remove the clone; otherwise, the parameter can be taken by value to avoid the extra allocation.♻️ Remove the clone
pub fn set_mode(&mut self, mode: DrawingMode) { - self.mode = mode.clone(); + self.mode = mode; if mode == DrawingMode::Disabled { self.clear(); } }Note: This assumes
DrawingModeimplementsCopyor that you change the signature to take ownership. IfDrawingModedoesn't implementCopy, consider deriving it if the type is small and simple.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/hopp_core_reusable.yml.github/workflows/tauri_rust_reusable.ymlcore/src/graphics/draw.rscore/src/graphics/iced_renderer.rstauri/src/components/SharingScreen/SharingScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/graphics/iced_renderer.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: backend-go / test-backend-go
- GitHub Check: precommit / pre-commit
🔇 Additional comments (21)
core/src/graphics/draw.rs (10)
26-39: LGTM!Clean struct design and straightforward initialization.
41-58: LGTM!Well-structured Draw state with appropriate Debug implementation (correctly omitting the Cache field).
89-115: LGTM!Both
add_pointandfinish_pathhave appropriate guards, error handling, and cache invalidation.
117-136: LGTM!Correct implementation of path removal with proper cache invalidation.
138-162: LGTM!Excellent rendering approach with appropriate caching for completed paths and two-pass rendering for the glow effect.
186-197: LGTM!Correct path construction with proper handling of the empty case.
200-211: LGTM!Clean manager structure with straightforward initialization.
219-236: LGTM!Proper participant lifecycle management with appropriate warnings for missing participants.
238-302: LGTM!Consistent and well-structured delegation pattern across all drawing operations, with appropriate error handling for missing participants.
304-322: LGTM!Efficient rendering approach that leverages per-participant caching for completed paths and combines in-progress paths into a single frame. The use of
with_capacityon line 306 is a nice optimization..github/workflows/tauri_rust_reusable.yml (1)
74-76: Fix action redundancy and test Rust 1.92.0 upgrade thoroughly.Rust 1.92.0 is a valid stable release (Dec 11, 2025). However, the jump from 1.87.0 introduces breaking changes you must verify:
- Rust 1.90 impact:
OUT_DIRin build scripts no longer exposed at compile-time; the codebase has two build.rs files (core/build.rs and tauri/src-tauri/build.rs) that need checking forenv!("OUT_DIR")usage and updating to usestd::env::var("OUT_DIR")at runtime.- Rust 1.92 impact: Never-type future-compatibility lints are now deny-by-default; review any compiler warnings after upgrade.
- FFI code: core/src/input/keyboard_macos.rs uses
extern "C"; verify no issues with FFI calling conventions on target platforms.The action usage is redundant—specifying both
@1.92.0andtoolchain: "1.92.0"is unnecessary. Per dtolnay/rust-toolchain documentation, use eitherdtolnay/rust-toolchain@1.92.0without thetoolchainparameter, ordtolnay/rust-toolchain@masterwithtoolchain: "1.92.0". Note that hopp_core_reusable.yml has the same redundancy at lines 71-73.Test the upgrade on all target platforms (Windows, macOS) before merging.
tauri/src/components/SharingScreen/SharingScreen.tsx (10)
11-34: LGTM! Drawing-related imports are well-organized.The new imports properly support the drawing subsystem with payload types, UI components, and topic constants.
Also applies to: 41-41
77-86: LGTM! Proper use of ref pattern to avoid stale closures.The
isRemoteControlEnabledRefpattern correctly prevents stale closure issues in event handlers while avoiding unnecessary re-renders.
93-120: Color assignment logic is sound with defensive fallback.The
getOrAssignColorfunction correctly cycles through colors and caches assignments per participant. The nullish coalescing on line 116 is defensive but should never be needed sincecolorIndexRef.current % SVG_BADGE_COLORS.lengthalways produces a valid index for the 8-color array.
229-249: LGTM! Path removal callback properly broadcasts clearing events.The callback setup correctly ensures that when paths are removed locally,
DrawClearPathevents are reliably published to other participants.
251-264: LGTM! Drawing mode changes are reliably broadcast.The effect correctly publishes drawing mode updates to other participants whenever the mode changes.
283-315: LGTM! Mouse move handling correctly differentiates drawing from cursor movement.The logic properly uses bitwise check
(e.buttons & 1) === 1to detect left button drag and routes drawing points toDRAW_TOPICwhile regular moves go toCURSORS_TOPIC.
389-437: LGTM! Mouse up handling is consistent with mouse down.The drawing end logic properly finalizes paths and symmetrically skips
MouseClickevents when remote control is disabled.
492-516: LGTM! Event listener management is properly scoped and cleaned up.The conditional addition of
contextmenulistener only when needed and the symmetric cleanup logic are correct.
774-776: LGTM! Clean separation of drawing and cursor rendering concerns.The
DrawingLayerandRemoteCursorscomponents are properly decoupled with shared color assignment for consistency.
317-387: Non-left mouse buttons sendMouseClickevents even when remote control is disabled.The code correctly prevents left-click
MouseClickevents whenisRemoteControlEnabledis false (lines 355-366, 415-417), sendingClickAnimationfor visual feedback instead. However, right-clicks and other mouse button types will still reach lines 370-385 and sendMouseClickevents regardless of the remote control state. Verify whether this is intentional (to allow context menus or other non-primary interactions) or if all mouse button events should be blocked when remote control is disabled.
| uses: dtolnay/rust-toolchain@1.92.0 | ||
| with: | ||
| toolchain: "1.87.0" | ||
| toolchain: "1.92.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust 1.92.0 stable release
💡 Result:
Rust 1.92.0 was released on December 11, 2025. Highlights include deny-by-default never-type lints, changes to the unused_must_use lint for infallible error types, validation for #[macro_export] inputs, several stabilized APIs (e.g., Box::new_zeroed, Rc/Arc new_zeroed/_slice), and other language/library fixes. To install/update: run rustup update stable. [1][2]
Sources:
[1] Rust blog — Announcing Rust 1.92.0 (Dec 11, 2025).
[2] Rust release notes — Version 1.92.0 (2025-12-11).
🌐 Web query:
dtolnay rust-toolchain GitHub action documentation usage
💡 Result:
Summary / usage — dtolnay/rust-toolchain (concise GitHub Action to install Rust via rustup):
-
Basic example:
- name: test suite
on: [push, pull_request]
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v5
- uses: dtolnay/rust-toolchain@stable
- run: cargo test --all-features
(Selecting the action revision chooses the toolchain: e.g. @nightly, @1.89.0.) [1]
- name: test suite
-
Inputs (all optional):
- toolchain — rustup specifier (stable, nightly, 1.89.0, nightly-2025-01-01). Default: matches the action @Rev; to pass a toolchain input use dtolnay/rust-toolchain@master.
- targets — comma-separated additional targets (e.g. wasm32-unknown-unknown).
- components — comma-separated components (e.g. clippy, rustfmt). [1][2]
-
Outputs:
- cachekey — short hash suitable for caching.
- name — rustup's canonical name for the installed toolchain (e.g. "1.62.0"). [1]
-
Extra features:
- Supports time/offset and "minus N releases" expressions to pick a sliding-window toolchain (examples in README).
- If you pin the action by full commit SHA, ensure the SHA is in master’s history to avoid GC breakage. [1]
Sources:
- README and action metadata (dtolnay/rust-toolchain). [1]
- GitHub Marketplace listing for the action (inputs/outputs overview). [2]
Remove the redundant toolchain parameter—specifying @1.92.0 in the action reference already sets the toolchain version.
Rust 1.92.0 is a valid stable release (December 2025). The current configuration has unnecessary duplication: when using dtolnay/rust-toolchain@1.92.0, the toolchain defaults to that version, so the toolchain: "1.92.0" parameter is redundant. Either remove the parameter or, if you need it for flexibility, switch to dtolnay/rust-toolchain@master with the toolchain input.
🤖 Prompt for AI Agents
In @.github/workflows/hopp_core_reusable.yml around lines 71 - 73, Remove the
redundant toolchain input: when using dtolnay/rust-toolchain@1.92.0 the version
is already specified in the action reference, so delete the toolchain: "1.92.0"
parameter from that workflow step (or alternatively switch the action to
dtolnay/rust-toolchain@master if you intend to keep specifying toolchain via the
toolchain input).
| pub fn start_path(&mut self, path_id: u64, point: Position) { | ||
| if self.mode == DrawingMode::Disabled { | ||
| log::warn!("start_path: drawing mode is disabled, skipping path"); | ||
| return; | ||
| } | ||
|
|
||
| log::info!("start_path: starting new path with id {}", path_id); | ||
| self.in_progress_path = Some(DrawPath::new(path_id, point)); | ||
| } |
There was a problem hiding this comment.
Handle existing in-progress path.
If start_path is called while another path is already in progress, the current implementation silently discards the old path without finishing it. This could happen in edge cases like network races or dropped finish_path events.
Consider either:
- Auto-finishing the previous path before starting a new one, or
- Warning and ignoring the new path until the current one finishes
🛠️ Proposed fix: Auto-finish previous path
pub fn start_path(&mut self, path_id: u64, point: Position) {
if self.mode == DrawingMode::Disabled {
log::warn!("start_path: drawing mode is disabled, skipping path");
return;
}
+ // Auto-finish any in-progress path before starting a new one
+ if self.in_progress_path.is_some() {
+ log::warn!("start_path: finishing previous in-progress path before starting new path {}", path_id);
+ self.finish_path();
+ }
+
log::info!("start_path: starting new path with id {}", path_id);
self.in_progress_path = Some(DrawPath::new(path_id, point));
}📝 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.
| pub fn start_path(&mut self, path_id: u64, point: Position) { | |
| if self.mode == DrawingMode::Disabled { | |
| log::warn!("start_path: drawing mode is disabled, skipping path"); | |
| return; | |
| } | |
| log::info!("start_path: starting new path with id {}", path_id); | |
| self.in_progress_path = Some(DrawPath::new(path_id, point)); | |
| } | |
| pub fn start_path(&mut self, path_id: u64, point: Position) { | |
| if self.mode == DrawingMode::Disabled { | |
| log::warn!("start_path: drawing mode is disabled, skipping path"); | |
| return; | |
| } | |
| // Auto-finish any in-progress path before starting a new one | |
| if self.in_progress_path.is_some() { | |
| log::warn!("start_path: finishing previous in-progress path before starting new path {}", path_id); | |
| self.finish_path(); | |
| } | |
| log::info!("start_path: starting new path with id {}", path_id); | |
| self.in_progress_path = Some(DrawPath::new(path_id, point)); | |
| } |
🤖 Prompt for AI Agents
In @core/src/graphics/draw.rs around lines 79 - 87, start_path currently
overwrites self.in_progress_path and drops any existing path; update it to
handle an existing in-progress path by checking if
self.in_progress_path.is_some() and either auto-finishing it or rejecting the
new start. Implement the auto-finish option by calling the existing finish_path
routine (or a new helper like finish_current_path) to finalize and store the
current DrawPath before assigning self.in_progress_path =
Some(DrawPath::new(path_id, point)); if you choose the ignore-new option, log a
warn including the incoming path_id and return without replacing
in_progress_path. Ensure you reference start_path, in_progress_path,
DrawPath::new and finish_path (or the new helper) when making the change.
| fn make_glow_stroke(&self) -> Stroke<'static> { | ||
| let mut glow_color = self.color; | ||
| glow_color.a *= 0.60; | ||
| Stroke { | ||
| style: stroke::Style::Solid(glow_color), | ||
| width: 5.0 + 1.5, | ||
| line_cap: stroke::LineCap::Round, | ||
| line_join: stroke::LineJoin::Round, | ||
| line_dash: stroke::LineDash::default(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Review alpha calculation for glow effect.
Line 176 multiplies the existing alpha by 0.60, which means if the input color already has transparency (e.g., alpha = 0.5), the glow will be even more transparent (0.5 * 0.6 = 0.3). This might not be the intended behavior.
Consider whether you want to multiply (compound) or set the alpha directly to 0.60 for consistent glow intensity regardless of input color transparency.
🔧 Proposed fix: Set alpha directly
fn make_glow_stroke(&self) -> Stroke<'static> {
let mut glow_color = self.color;
- glow_color.a *= 0.60;
+ glow_color.a = 0.60;
Stroke {
style: stroke::Style::Solid(glow_color),
width: 5.0 + 1.5,
line_cap: stroke::LineCap::Round,
line_join: stroke::LineJoin::Round,
line_dash: stroke::LineDash::default(),
}
}📝 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.
| fn make_glow_stroke(&self) -> Stroke<'static> { | |
| let mut glow_color = self.color; | |
| glow_color.a *= 0.60; | |
| Stroke { | |
| style: stroke::Style::Solid(glow_color), | |
| width: 5.0 + 1.5, | |
| line_cap: stroke::LineCap::Round, | |
| line_join: stroke::LineJoin::Round, | |
| line_dash: stroke::LineDash::default(), | |
| } | |
| } | |
| fn make_glow_stroke(&self) -> Stroke<'static> { | |
| let mut glow_color = self.color; | |
| glow_color.a = 0.60; | |
| Stroke { | |
| style: stroke::Style::Solid(glow_color), | |
| width: 5.0 + 1.5, | |
| line_cap: stroke::LineCap::Round, | |
| line_join: stroke::LineJoin::Round, | |
| line_dash: stroke::LineDash::default(), | |
| } | |
| } |
🤖 Prompt for AI Agents
In @core/src/graphics/draw.rs around lines 174 - 184, The glow alpha is being
compounded by multiplying self.color.a by 0.60 in make_glow_stroke, which makes
already-transparent colors even fainter; change the logic to set the glow alpha
to a fixed value (e.g., 0.60) rather than multiplying so the glow has consistent
intensity regardless of the input color's alpha. Update make_glow_stroke to
derive glow_color from self.color but assign glow_color.a = 0.60 (or another
chosen constant) and leave the rest of the Stroke construction unchanged.
| /// Adds a new participant with their color. | ||
| pub fn add_participant(&mut self, sid: String, color: &str) { | ||
| log::info!("DrawManager::add_participant: sid={} color={}", sid, color); | ||
| self.draws.insert(sid, Draw::new(color)); | ||
| } |
There was a problem hiding this comment.
Consider checking for existing participant.
The current implementation silently overwrites any existing Draw state if add_participant is called with a duplicate sid. This could lead to lost drawing data if the method is called twice for the same participant.
Consider checking if the participant already exists and either warning or returning early.
🛡️ Proposed fix: Check for existing participant
pub fn add_participant(&mut self, sid: String, color: &str) {
log::info!("DrawManager::add_participant: sid={} color={}", sid, color);
+ if self.draws.contains_key(&sid) {
+ log::warn!("DrawManager::add_participant: participant {} already exists, skipping", sid);
+ return;
+ }
self.draws.insert(sid, Draw::new(color));
}📝 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.
| /// Adds a new participant with their color. | |
| pub fn add_participant(&mut self, sid: String, color: &str) { | |
| log::info!("DrawManager::add_participant: sid={} color={}", sid, color); | |
| self.draws.insert(sid, Draw::new(color)); | |
| } | |
| /// Adds a new participant with their color. | |
| pub fn add_participant(&mut self, sid: String, color: &str) { | |
| log::info!("DrawManager::add_participant: sid={} color={}", sid, color); | |
| if self.draws.contains_key(&sid) { | |
| log::warn!("DrawManager::add_participant: participant {} already exists, skipping", sid); | |
| return; | |
| } | |
| self.draws.insert(sid, Draw::new(color)); | |
| } |
🤖 Prompt for AI Agents
In @core/src/graphics/draw.rs around lines 213 - 217, The add_participant method
currently unconditionally overwrites any existing Draw for a given sid; update
DrawManager::add_participant to first check self.draws.contains_key(&sid) and if
present either log a warning (e.g., via log::warn!) and return early or
otherwise avoid inserting a new Draw, so existing drawing state isn't lost;
reference the existing symbols add_participant, self.draws, sid, and Draw::new
when making this change.
| // Get the next unique path ID and handle wrap-over | ||
| const getNextPathId = useCallback(() => { | ||
| const id = pathIdCounterRef.current; | ||
| pathIdCounterRef.current++; | ||
|
|
||
| return updated; | ||
| }); | ||
| }); | ||
| // Handle wrap-over: reset to 0 when reaching max | ||
| if (pathIdCounterRef.current > MAX_PATH_ID) { | ||
| pathIdCounterRef.current = 0; | ||
| } | ||
|
|
||
| return id; | ||
| }, []); |
There was a problem hiding this comment.
Consider potential path ID collision after wrap-around.
While the wrap-around logic prevents overflow, it could theoretically cause ID collisions if very old paths are still active when the counter wraps. This is unlikely in practice given the MAX_SAFE_INTEGER limit, but for production robustness, consider using UUIDs or a more sophisticated ID reclamation strategy.
Alternative approach using UUID
import { v4 as uuidv4 } from 'uuid';
// Replace getNextPathId with:
const getNextPathId = useCallback(() => {
return uuidv4();
}, []);This eliminates collision risk entirely, though it increases payload size.
🤖 Prompt for AI Agents
In @tauri/src/components/SharingScreen/SharingScreen.tsx around lines 122 - 133,
The getNextPathId() wrap-around can cause ID collisions when old paths remain
after pathIdCounterRef resets; replace the numeric counter with a
collision-resistant ID (e.g., UUID) or implement reclamation: change
getNextPathId to return a UUID (using v4 from 'uuid') or implement a pool that
reclaims freed IDs and checks activePaths before returning an id; update all
callers that expect numeric ids (references to getNextPathId and
pathIdCounterRef and any comparisons against MAX_PATH_ID) to accept string IDs
or to use the new pool API so collisions cannot occur after wrap-around.
| const handleContextMenu = (e: MouseEvent) => { | ||
| e.preventDefault(); | ||
| // If in drawing mode with permanent enabled, prevent context menu (dev tools) | ||
| if (isDrawingMode && drawingMode.type === "Draw" && drawingMode.settings.permanent) { | ||
| e.stopPropagation(); | ||
| // Send DrawClearAllPaths event on right-click in permanent mode | ||
| const payload: TPDrawClearAllPaths = { | ||
| type: "DrawClearAllPaths", | ||
| }; | ||
| localParticipant.localParticipant?.publishData(encoder.encode(JSON.stringify(payload)), { | ||
| reliable: true, | ||
| topic: DRAW_TOPIC, | ||
| }); | ||
|
|
||
| // Update local draw participant | ||
| const drawParticipant = drawParticipantsRef.current.get(LOCAL_PARTICIPANT_ID); | ||
| if (drawParticipant) { | ||
| drawParticipant.clear(); | ||
| } | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider UX implications of clearing all drawings on right-click.
Right-clicking in permanent drawing mode clears all drawings without confirmation (lines 442-459). This could be unexpected behavior that leads to accidental data loss, especially if multiple participants have been drawing. Consider adding:
- A visual confirmation dialog
- An undo mechanism
- A toast notification after clearing
Suggested confirmation approach
const handleContextMenu = (e: MouseEvent) => {
e.preventDefault();
// If in drawing mode with permanent enabled, prevent context menu (dev tools)
if (isDrawingMode && drawingMode.type === "Draw" && drawingMode.settings.permanent) {
e.stopPropagation();
+
+ // Show confirmation toast
+ toast.promise(
+ new Promise((resolve, reject) => {
+ // Create a confirmation mechanism here
+ // For now, just resolve after a timeout as example
+ setTimeout(resolve, 100);
+ }),
+ {
+ loading: 'Clearing all drawings...',
+ success: 'All drawings cleared',
+ error: 'Failed to clear drawings',
+ }
+ );
+
// Send DrawClearAllPaths event on right-click in permanent mode
const payload: TPDrawClearAllPaths = {
type: "DrawClearAllPaths",
};
// ... rest of the codeCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tauri/src/components/SharingScreen/SharingScreen.tsx around lines 439 - 460,
The right-click handler in handleContextMenu immediately clears all drawings
when isDrawingMode && drawingMode.type === "Draw" &&
drawingMode.settings.permanent; change this to prompt for confirmation and
provide feedback/undo: before constructing the TPDrawClearAllPaths payload and
calling localParticipant.localParticipant?.publishData, show a confirmation
dialog (or toast with "Undo") asking the user to confirm clearing; only on
confirm send the TPDrawClearAllPaths event and then update the local draw
participant via drawParticipantsRef.current.get(LOCAL_PARTICIPANT_ID)?.clear();
additionally push the current drawing state to an undo stack so the toast "Undo"
action can republish the previous state to restore drawings.
| } | ||
| }; | ||
| }, [isSharingMouse, updateMouseControls]); | ||
| }, [isSharingMouse, isDrawingMode, drawingMode, updateMouseControls]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the exact file and check its structure
find . -name "SharingScreen.tsx" -type fRepository: gethopp/hopp
Length of output: 110
🏁 Script executed:
# Examine the file around line 517 to see the useEffect and its dependencies
sed -n '500,530p' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 1177
🏁 Script executed:
# Look at the handlers referenced in this useEffect - need to find handleMouseMove, handleWheel, etc.
sed -n '450,520p' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 2410
🏁 Script executed:
# Look at more handlers to see the full picture of how localParticipant.localParticipant is used
sed -n '280,350p' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 2783
🏁 Script executed:
# Find the useLocalParticipant hook usage in this file
rg -n 'useLocalParticipant|const localParticipant' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 227
🏁 Script executed:
# Check how localParticipant is declared and used at the top
sed -n '70,85p' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 772
🏁 Script executed:
# Search for LiveKit types and how useLocalParticipant is defined
rg -n '@livekit' ./tauri/src/components/SharingScreen/SharingScreen.tsx | head -20Repository: gethopp/hopp
Length of output: 174
🏁 Script executed:
# Check if localParticipant is used in any other useEffect dependencies
rg -n 'useEffect.*\[' ./tauri/src/components/SharingScreen/SharingScreen.tsx -A1 | head -30Repository: gethopp/hopp
Length of output: 38
🏁 Script executed:
# Try a different approach - find all useEffect declarations
rg -n 'useEffect' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 478
🏁 Script executed:
# Look at all useEffect declarations to see if localParticipant is used in dependencies elsewhere
for linenum in 84 198 204 213 222 230 252 267 280 527 641 723; do
echo "=== Line $linenum ===";
sed -n "${linenum},$((linenum+15))p" ./tauri/src/components/SharingScreen/SharingScreen.tsx;
doneRepository: gethopp/hopp
Length of output: 7343
🏁 Script executed:
# Look at the exact structure of the mouse sharing useEffect dependencies and handlers more carefully
sed -n '252,266p' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 443
🏁 Script executed:
# Check the LiveKit components-react types to understand if useLocalParticipant is stable
# Search for any node_modules or type definitions
fd -name "*.d.ts" -path "*livekit*" | head -5
# Also check if there's a package.json to see the LiveKit version
cat ./package.json | grep -A2 '@livekit'Repository: gethopp/hopp
Length of output: 286
🏁 Script executed:
# Check the package.json for LiveKit version
cat package.json | grep -i livekitRepository: gethopp/hopp
Length of output: 38
🏁 Script executed:
# Find package files in tauri directory
find ./tauri -name "package.json" -type f -exec grep -l "livekit" {} \;Repository: gethopp/hopp
Length of output: 76
🏁 Script executed:
# Check LiveKit version and dependencies
grep -A5 '@livekit' ./tauri/package.jsonRepository: gethopp/hopp
Length of output: 476
🏁 Script executed:
# Let me examine the pattern more closely - check all occurrences of localParticipant.localParticipant
rg -n 'localParticipant\.localParticipant' ./tauri/src/components/SharingScreen/SharingScreen.tsxRepository: gethopp/hopp
Length of output: 2472
Add localParticipant.localParticipant to the dependency array.
The useEffect handlers (lines 280–520) use localParticipant.localParticipant in throttled callbacks at lines 292, 309, 328, 338, 361, 385, 401, 435, 448, 479, and 489, but it's missing from the dependency array at line 517. This causes the throttled handlers to capture a stale reference. Other useEffects in the same file correctly include localParticipant.localParticipant in their dependencies (lines 249, 264); this one should too. Either add it to [isSharingMouse, isDrawingMode, drawingMode, updateMouseControls, localParticipant.localParticipant] or refactor to use a ref pattern like isRemoteControlEnabledRef.
🤖 Prompt for AI Agents
In @tauri/src/components/SharingScreen/SharingScreen.tsx at line 517, The
useEffect that currently lists dependencies [isSharingMouse, isDrawingMode,
drawingMode, updateMouseControls] must include localParticipant.localParticipant
to avoid throttled handlers capturing a stale participant reference; update the
dependency array to [isSharingMouse, isDrawingMode, drawingMode,
updateMouseControls, localParticipant.localParticipant] (or alternatively
refactor the throttled callbacks to read localParticipant.localParticipant from
a ref such as isRemoteControlEnabledRef) so the effect re-runs when the local
participant object changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tauri/src/components/SharingScreen/SharingScreen.tsx:
- Around line 220-227: The comment claims it "Get or assign color for local
participant" but the code uses a hardcoded "#FFDF20"; replace the hardcoded
value with the actual color allocator by calling
getOrAssignColor(LOCAL_PARTICIPANT_ID) and pass that result into new
DrawParticipant (i.e., new
DrawParticipant(getOrAssignColor(LOCAL_PARTICIPANT_ID), drawingMode)); keep the
rest of the useEffect and drawParticipantsRef logic intact and ensure the
dependency array still includes getOrAssignColor and drawingMode.
🧹 Nitpick comments (3)
tauri/src/components/SharingScreen/Controls.tsx (3)
26-44: Consider user-visible error feedback for cache loading failures.The error is logged to console but users won't see it if cache loading fails. While this is a minor issue since the app will still work with default settings, consider adding a toast notification for better UX.
♻️ Optional enhancement: Add user feedback on cache load failure
} catch (error) { console.error("Failed to load cached drawing mode:", error); + toast.error("Failed to restore previous drawing settings"); }Note: This would require importing
toastfromreact-hot-toast.
46-66: Consider user-visible error feedback for cache save failures.Similar to the load operation, save failures are only logged to console. Consider adding user feedback for cache persistence failures.
♻️ Optional enhancement: Add user feedback on cache save failure
} catch (error) { console.error("Failed to save drawing mode:", error); + toast.error("Failed to save drawing settings"); }
132-178: Consider clarifying the menu item text.The text "Click to remove drawing" could be more specific since the kbd hint shows "Right click". Consider "Right-click to remove drawing" or "Remove drawing on right-click" for clarity.
♻️ Proposed text clarification
<DropdownMenuCheckboxItem checked={rightClickToClear} onCheckedChange={setRightClickToClear} className="flex items-center justify-between gap-4" > - <span>Click to remove drawing</span> + <span>Right-click to remove drawing</span> <kbd className="ml-auto pointer-events-none inline-flex h-5 select-none items-center gap-1 rounded border border-slate-200 dark:border-zinc-600 bg-slate-100 dark:bg-zinc-700 px-1.5 font-mono text-[10px] font-medium text-slate-600 dark:text-slate-300"> Right click </kbd>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tauri/src/components/SharingScreen/Controls.tsxtauri/src/components/SharingScreen/SharingScreen.tsxtauri/src/components/ui/segmented-control.tsxtauri/src/windows/screensharing/context.tsxtauri/src/windows/screensharing/main.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tauri/src/components/ui/segmented-control.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
tauri/src/windows/screensharing/main.tsx (1)
tauri/src/components/SharingScreen/Controls.tsx (2)
RemoteControlDisabledIndicator(180-201)DrawingSettingsButton(132-178)
tauri/src/windows/screensharing/context.tsx (2)
tauri/src/payloads.ts (1)
TDrawingMode(163-163)tauri/src/components/ui/draw-participant.ts (2)
drawingMode(46-48)setDrawingMode(53-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: precommit / pre-commit
- GitHub Check: backend-go / test-backend-go
🔇 Additional comments (18)
tauri/src/components/SharingScreen/Controls.tsx (3)
9-14: LGTM!The imports are well-organized and support the new drawing settings functionality.
74-96: Mode transition logic looks correct.The function properly:
- Clears drawings when leaving Draw mode
- Applies cached settings when entering Draw mode
- Manages mouse/key sharing state appropriately
Note: Drawings are only cleared when leaving the "Draw" mode specifically (line 76), not when leaving "ClickAnimation". This appears intentional since ClickAnimation doesn't create persistent drawings.
104-124: LGTM!The segmented control options are clear and intuitive. The icon choices appropriately represent each mode.
tauri/src/windows/screensharing/context.tsx (3)
1-2: LGTM!The imports are properly added to support the new drawing functionality.
4-22: LGTM!The context type extensions are well-structured with clear naming. The comment explaining the signal pattern is helpful.
38-76: LGTM!The provider implementation is correct with sensible defaults. The
triggerClearDrawingscallback properly usesuseCallbackwith an empty dependency array sincesetClearDrawingsSignalis stable.tauri/src/windows/screensharing/main.tsx (2)
7-11: LGTM!The import statement is properly structured.
238-241: LGTM!The DrawingSettingsButton is appropriately positioned in the top-right controls area. The gap ensures proper spacing between UI elements.
tauri/src/components/SharingScreen/SharingScreen.tsx (10)
11-11: LGTM!The new imports and constants are well-organized and follow existing patterns. The
DRAW_TOPICconstant maintains consistency withCURSORS_TOPIC.Also applies to: 21-42
77-94: LGTM! Good use of ref for synchronous access.The
isRemoteControlEnabledRefpattern correctly addresses the stale closure problem in event handlers by maintaining a ref that's kept in sync with the store value. This ensures event handlers always have access to the current value.
101-128: LGTM! Efficient color management.The color assignment logic is well-implemented:
- Uses refs to avoid unnecessary re-renders
- Properly cycles through available colors with modulo
- Returns existing colors for known participants
- Thread-safe since React guarantees ref updates are synchronous
130-141: Path ID generation looks correct.The wrap-around logic at
MAX_SAFE_INTEGERprevents overflow. While there's a theoretical possibility of ID collision after wrap-around, this would require creating ~9 quadrillion paths, which is not a practical concern.
230-235: LGTM!The effect correctly synchronizes the local participant's drawing mode with the context state.
238-257: LGTM!The path removal callback correctly publishes
DrawClearPathevents for each removed path using reliable delivery.
259-294: LGTM!Both effects are correctly implemented:
- Drawing mode changes are broadcast to other participants
- The signal pattern for clearing all drawings works as expected, skipping the initial render
380-398: Redundant button check after else clause.Line 382 checks
e.button === 0but this code is already in theelseclause of line 352's check forisDrawingMode && e.button === 0. The else clause can only execute if that condition was false, meaning either not in drawing mode OR not button 0. However, you want to show ripple and handle clicks specifically for button 0 in non-drawing mode, so this check is actually necessary, not redundant.
522-547: LGTM! Event handler registration is correct.The event listener setup properly:
- Registers drawing-related handlers unconditionally (needed for drawing mode)
- Conditionally registers contextmenu based on drawing mode or mouse sharing
- Correctly cleans up all registered handlers
- Has a complete dependency array
804-806: LGTM! Rendering integration is clean.The
DrawingLayerandRemoteCursorscomponents are properly integrated with shared color assignment for consistency between cursors and drawings.
| // Initialize local draw participant | ||
| useEffect(() => { | ||
| if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) { | ||
| // Get or assign color for local participant (only assigns on first encounter) | ||
| const localDrawParticipant = new DrawParticipant("#FFDF20", drawingMode); | ||
| drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant); | ||
| } | ||
| }, [drawingMode, getOrAssignColor]); |
There was a problem hiding this comment.
Inconsistency between comment and implementation.
Line 223's comment states "Get or assign color for local participant" but line 224 uses a hardcoded color "#FFDF20" instead of calling getOrAssignColor(). Either update the comment to explain why the local participant uses a fixed yellow color, or use getOrAssignColor(LOCAL_PARTICIPANT_ID) for consistency.
📝 Suggested fix: Update comment to reflect hardcoded color
if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) {
- // Get or assign color for local participant (only assigns on first encounter)
+ // Local participant uses a fixed yellow color to distinguish from remote participants
const localDrawParticipant = new DrawParticipant("#FFDF20", drawingMode);
drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant);
}📝 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.
| // Initialize local draw participant | |
| useEffect(() => { | |
| if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) { | |
| // Get or assign color for local participant (only assigns on first encounter) | |
| const localDrawParticipant = new DrawParticipant("#FFDF20", drawingMode); | |
| drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant); | |
| } | |
| }, [drawingMode, getOrAssignColor]); | |
| // Initialize local draw participant | |
| useEffect(() => { | |
| if (!drawParticipantsRef.current.has(LOCAL_PARTICIPANT_ID)) { | |
| // Local participant uses a fixed yellow color to distinguish from remote participants | |
| const localDrawParticipant = new DrawParticipant("#FFDF20", drawingMode); | |
| drawParticipantsRef.current.set(LOCAL_PARTICIPANT_ID, localDrawParticipant); | |
| } | |
| }, [drawingMode, getOrAssignColor]); |
🤖 Prompt for AI Agents
In @tauri/src/components/SharingScreen/SharingScreen.tsx around lines 220 - 227,
The comment claims it "Get or assign color for local participant" but the code
uses a hardcoded "#FFDF20"; replace the hardcoded value with the actual color
allocator by calling getOrAssignColor(LOCAL_PARTICIPANT_ID) and pass that result
into new DrawParticipant (i.e., new
DrawParticipant(getOrAssignColor(LOCAL_PARTICIPANT_ID), drawingMode)); keep the
rest of the useEffect and drawParticipantsRef logic intact and ensure the
dependency array still includes getOrAssignColor and drawingMode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/src/lib.rs (1)
920-971: Consider extracting the point-to-pixel conversion logic.The pattern of getting the overlay window and converting client points to pixel positions is duplicated across
DrawStart,DrawAddPoint, andDrawEndhandlers.♻️ Refactor suggestion
Consider extracting a helper method on
RemoteControl:impl RemoteControl<'_> { fn convert_point_and_update_cursor(&mut self, point: &room_service::ClientPoint, sid: &str) -> Position { let overlay_window = self.cursor_controller.get_overlay_window(); let pixel_position = overlay_window.get_pixel_position(point.x, point.y); self.cursor_controller.cursor_move_controller(point.x, point.y, sid); pixel_position } }Then simplify each handler:
UserEvent::DrawStart(point, path_id, sid) => { // ... let remote_control = &mut self.remote_control.as_mut().unwrap(); - let overlay_window = remote_control.cursor_controller.get_overlay_window(); - let pixel_position = overlay_window.get_pixel_position(point.x, point.y); + let pixel_position = remote_control.convert_point_and_update_cursor(&point, sid.as_str()); remote_control.gfx.draw_start(sid.as_str(), pixel_position, path_id); - remote_control.cursor_controller.cursor_move_controller( - point.x, point.y, sid.as_str() - ); }core/src/input/mouse.rs (4)
1289-1297: Consider adding documentation for the new public method.The
get_participant_colormethod is now part of the public API but lacks documentation explaining its purpose, parameters, and return value.📝 Suggested documentation
+ /// Retrieves the color assigned to a specific controller. + /// + /// # Parameters + /// + /// * `sid` - Session ID of the controller + /// + /// # Returns + /// + /// * `Some(&'static str)` - The hex color string assigned to the controller + /// * `None` - If no controller with the given SID exists pub fn get_participant_color(&self, sid: &str) -> Option<&'static str> {
1299-1301: Consider adding documentation for the new public method.The
get_overlay_windowmethod is part of the public API but lacks documentation.📝 Suggested documentation
+ /// Returns a reference to the overlay window. + /// + /// This provides access to the overlay window for coordinate transformations + /// and other window-related operations. + /// + /// # Returns + /// + /// A cloned `Arc<OverlayWindow>` reference pub fn get_overlay_window(&self) -> Arc<OverlayWindow> {
1329-1333: Consider adding documentation for the new public method.The
trigger_rendermethod is public but lacks documentation explaining when and why it should be called.📝 Suggested documentation
+ /// Requests an immediate redraw of the overlay. + /// + /// This method triggers a redraw event that will be processed by the + /// redraw thread, subject to the throttling interval. pub fn trigger_render(&self) {
674-702: Add an explanatory comment for the animation duration padding.The animation_duration calculation at line 661 adds 500ms to
ANIMATION_DURATIONwithout documenting why. Consider adding a clarifying comment about what this buffer period is for (e.g., fade-out delay, safety margin, or overlap tolerance).📝 Add explanatory comment
let mut last_click_animation_time = None; let redraw_interval = std::time::Duration::from_millis(16); - let animation_duration = (ANIMATION_DURATION + 500) as u128; + // Add 500ms buffer to animation duration (graphics layer disables at 1000ms) + let animation_duration = (ANIMATION_DURATION + 500) as u128;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/input/mouse.rscore/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/lib.rs (1)
core/src/room_service.rs (1)
ClientEvent(798-819)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: precommit / pre-commit
- GitHub Check: backend-go / test-backend-go
🔇 Additional comments (7)
core/src/lib.rs (3)
1100-1106: The referenced typesDrawingModeandClientPointare correctly defined in theroom_servicemodule. Both are public types (DrawingModeenum at line 812,ClientPointstruct at line 673) and are properly accessible for use in theUserEventenum variants.
786-800: The color assignment is guaranteed duringadd_controller. The method pops a color from theavailable_colorsqueue, and if unavailable, returns an error rather than proceeding without a color. Theif let Some()check is appropriate defensive coding for the rare edge case of a potential race condition between the lock release afteradd_controllerand the lock acquisition inget_participant_color. The code is correct as written.
972-991: Document the render trigger strategy for draw operations.Draw operations (
DrawStart,DrawAddPoint,DrawEnd) callcursor_move_controller(), which simulates OS-level mouse movement events but does not explicitly invoketrigger_render(). In contrast, clear operations (DrawClearPath,DrawClearAllPaths) directly calltrigger_render(). This asymmetry suggests draw operations rely on OS cursor simulation to implicitly trigger redraws, while clear operations require explicit redraw requests. This strategy should be documented to clarify whether the implicit OS-event-based redraw mechanism is intentional and always sufficient.core/src/input/mouse.rs (4)
649-649: LGTM: Enum variant updated to support animation state.The addition of the
boolparameter toClickAnimationenables distinguishing between initial animation requests and continuation frames, which aligns with the new timing logic below.
653-659: LGTM: Thread signature updated for bidirectional communication.The addition of the
txparameter andlast_click_animation_timestate enables the redraw thread to orchestrate timed click animations through self-messaging, which is used in the animation logic below.
841-848: LGTM: Bidirectional channel setup is correct.The sender is properly cloned before being moved into the thread closure, enabling the redraw thread to send animation continuation messages back through the channel.
1303-1327: LGTM: Click animation trigger is well-documented and implemented.The method correctly sends both the user event for graphics state update and the redraw command to start the animation, with appropriate error logging for both operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/src/graphics/draw.rs:
- Around line 71-72: The set_mode method currently clones the incoming
DrawingMode; instead, check the condition on the incoming mode before assigning
to avoid the clone: if the incoming mode equals DrawingMode::Disabled call
self.clear(), then move the mode into self.mode (assign mode directly). Update
the function named set_mode to perform the equality check against
DrawingMode::Disabled prior to assignment and remove the unnecessary clone.
🧹 Nitpick comments (6)
core/src/graphics/marker.rs (2)
9-9: Prefer&strover&Stringfor function parameters.Using
&stris more idiomatic and flexible, as it accepts both&Stringand string literals without requiring the caller to allocate aString.♻️ Suggested fix
- pub fn new(texture_path: &String) -> Self { + pub fn new(texture_path: &str) -> Self {
19-20: Consider extracting magic numbers to named constants.The hardcoded
40.0for marker dimensions appears multiple times. Extracting to a constant improves readability and maintainability.♻️ Suggested fix
+const MARKER_SIZE: f32 = 40.0; + impl Marker { // ... pub fn draw(&self, renderer: &Renderer, bounds: Rectangle) -> canvas::Geometry { self.cache.draw(renderer, bounds.size(), |frame| { let image_handle = iced_core::image::Image::new(self.marker.clone()); - let width = 40.0; - let height = 40.0; + let width = MARKER_SIZE; + let height = MARKER_SIZE;core/src/graphics/iced_canvas.rs (2)
5-7: Unconventional module inclusion via#[path].Using
#[path = "marker.rs"]is unusual. Typically, sibling modules are declared inmod.rsorlib.rs. This approach works but can be confusing for maintainers expecting standard module organization.If this is intentional (e.g., to keep
marker.rsas a private implementation detail of this file), consider adding a brief comment explaining the rationale.
55-55: Prefer&strover&Stringfor function parameters.Same as in
marker.rs, using&stris more idiomatic.♻️ Suggested fix
- pub fn new(texture_path: &String) -> Self { + pub fn new(texture_path: &str) -> Self {core/src/graphics/draw.rs (2)
79-87: Consider logging when an in-progress path is abandoned.If
start_pathis called while another path is already in progress, the previous path is silently discarded. This might be intentional, but a debug log could help with troubleshooting unexpected behavior.♻️ Suggested enhancement
pub fn start_path(&mut self, path_id: u64, point: Position) { if self.mode == DrawingMode::Disabled { log::warn!("start_path: drawing mode is disabled, skipping path"); return; } + if self.in_progress_path.is_some() { + log::debug!("start_path: abandoning previous in-progress path"); + } log::info!("start_path: starting new path with id {}", path_id); self.in_progress_path = Some(DrawPath::new(path_id, point)); }
213-216: Consider handling duplicate participant registration.If
add_participantis called with an existingsid, the previousDrawinstance is silently replaced, losing all drawing data. Depending on the use case, you might want to log a warning or preserve the existing data.♻️ Suggested enhancement
pub fn add_participant(&mut self, sid: String, color: &str) { log::info!("DrawManager::add_participant: sid={} color={}", sid, color); + if self.draws.contains_key(&sid) { + log::warn!("DrawManager::add_participant: replacing existing participant {}", sid); + } self.draws.insert(sid, Draw::new(color)); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/graphics/draw.rscore/src/graphics/iced_canvas.rscore/src/graphics/marker.rstauri/src-tauri/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
tauri/src-tauri/src/lib.rs (2)
tauri/src-tauri/src/main.rs (4)
app_handle(1029-1029)set_livekit_url(446-457)reset_core_process(228-235)get_livekit_url(460-465)core/socket_lib/src/lib.rs (1)
new_create(153-250)
core/src/graphics/marker.rs (1)
core/src/graphics/iced_canvas.rs (3)
new(27-29)new(55-59)draw(35-46)
core/src/graphics/iced_canvas.rs (2)
core/src/graphics/draw.rs (10)
draw(304-320)new(33-38)new(61-69)new(208-210)set_drawing_mode(225-235)draw_start(238-250)draw_add_point(253-260)draw_end(263-271)draw_clear_path(274-288)draw_clear_all_paths(291-301)core/src/graphics/marker.rs (2)
draw(16-64)new(9-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: precommit / pre-commit
- GitHub Check: backend-go / test-backend-go
🔇 Additional comments (7)
tauri/src-tauri/src/lib.rs (1)
131-148: The review comment references source code that cannot be located in the repository (tauri/src-tauri/src/lib.rsand related functions likecreate_core_processandCommandChild). Without access to the actual implementation, the following cannot be verified:
- Whether
CommandChildis dropped and if it terminates the child process- Whether error handling for
create_core_processfailure is appropriate- Whether the crash messages are contradictory in context
To proceed, either the repository needs to be in the correct state with the source code present, or the file path needs to be clarified.
core/src/graphics/iced_canvas.rs (2)
32-47: LGTM!The
canvas::Programimplementation correctly composes marker and draw geometries. The draw method properly delegates to the marker and draw manager, combining their outputs.
70-100: LGTM!The forwarding methods cleanly delegate to the
DrawManagerAPI, maintaining a clean separation of concerns between the overlay surface and the drawing logic.core/src/graphics/draw.rs (3)
8-24: LGTM with minor note.The hex color parsing handles the common
#prefix and validates minimum length. Theunwrap_or(0)fallback for individual components is a reasonable silent recovery for malformed input.
186-197: Single-point paths may render as invisible.When
build_pathreceives a single point, it creates a path with onlymove_toand noline_to. This results in an empty stroke that won't be visible. If single-point "dots" should be supported, consider drawing a small circle or line segment at that point.Is rendering single-point paths (e.g., a quick tap without drag) a supported use case? If so, you may need to handle this explicitly.
303-320: LGTM!The rendering approach is well-designed:
- Completed paths use caching for performance
- In-progress paths are drawn fresh each frame (correct, since they change frequently)
- Pre-allocating the geometries vector is a nice optimization
core/src/graphics/marker.rs (1)
16-63: Cache invalidation is handled automatically by iced.iced's
canvas::Cacheautomatically invalidates and regenerates geometry when the supplied size changes. Since the code passesbounds.size()tocache.draw(), the cache will automatically detect any size differences and regenerate the geometry. No manual cache clearing is needed for size changes;Cache::clear()is only required for forced redraws in other scenarios.Likely an incorrect or invalid review comment.
| pub fn set_mode(&mut self, mode: DrawingMode) { | ||
| self.mode = mode.clone(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DrawingMode derives Copy
rg -A10 "enum DrawingMode" --type rustRepository: gethopp/hopp
Length of output: 1186
🏁 Script executed:
#!/bin/bash
# Check DrawingMode with more context to see derive macros
rg -B5 "enum DrawingMode" --type rustRepository: gethopp/hopp
Length of output: 782
🏁 Script executed:
#!/bin/bash
# Check the set_mode method and mode field in draw.rs
rg -B10 -A5 "pub fn set_mode" core/src/graphics/draw.rsRepository: gethopp/hopp
Length of output: 494
🏁 Script executed:
#!/bin/bash
# Check if there are other usages of the mode parameter after the assignment
rg -A10 "pub fn set_mode" core/src/graphics/draw.rs | head -20Repository: gethopp/hopp
Length of output: 421
Refactor to eliminate the clone by checking the condition before the move.
DrawingMode does not implement Copy and is not a candidate for copy optimization. However, the .clone() call is unnecessary here. The condition check can be moved before the assignment to avoid cloning:
pub fn set_mode(&mut self, mode: DrawingMode) {
if mode == DrawingMode::Disabled {
self.clear();
}
self.mode = mode;
}This allows the mode parameter to be moved directly into self.mode without cloning.
🤖 Prompt for AI Agents
In @core/src/graphics/draw.rs around lines 71 - 72, The set_mode method
currently clones the incoming DrawingMode; instead, check the condition on the
incoming mode before assigning to avoid the clone: if the incoming mode equals
DrawingMode::Disabled call self.clear(), then move the mode into self.mode
(assign mode directly). Update the function named set_mode to perform the
equality check against DrawingMode::Disabled prior to assignment and remove the
unnecessary clone.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.