Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughRefactors graphics and input to replace GPU/wgpu-based rendering with an iced canvas + ParticipantsManager architecture; adds a Clock abstraction and TestClock; replaces thread-based cursor wrappers with CursorState; centralizes redraw/render flows and click animations via GraphicsContext and ClickAnimationRenderer. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant GraphicsContext as "GraphicsContext\n(redraw thread)"
participant Participants as "ParticipantsManager"
participant ClickAnim as "ClickAnimationRenderer"
participant Iced as "IcedRenderer / Canvas"
participant Clock as "Clock (Real/Test)"
App->>GraphicsContext: trigger_render() / render_frame()
GraphicsContext->>Clock: now()
GraphicsContext->>Participants: request participant state (drawings, cursors)
Participants-->>GraphicsContext: geometries + cursor positions
GraphicsContext->>ClickAnim: collect active animations (positions, start times)
ClickAnim->>Clock: now()
ClickAnim-->>GraphicsContext: animation geometries
GraphicsContext->>Iced: draw(participants, click_animations, frame)
Iced->>Iced: canvas draw (images, paths, rings)
Iced-->>App: present frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/graphics/click_animation.rs (1)
155-159:⚠️ Potential issue | 🟠 MajorFix the incorrect buffer argument in
disablecall at line 158.The
disablemethod expectstransforms_bufferbut receivesradius_buffer. Since both are&wgpu::Buffer, this compiles silently but writes the off-screen position data to the wrong buffer, preventing the animation from being hidden after its duration expires.Update the
drawmethod signature to accepttransforms_buffer:Proposed fix
pub fn draw( &mut self, render_pass: &mut wgpu::RenderPass, queue: &wgpu::Queue, radius_buffer: &wgpu::Buffer, + transforms_buffer: &wgpu::Buffer, transforms_bind_group: &wgpu::BindGroup, radius_bind_group: &wgpu::BindGroup, ) {Then update the call at line 158:
- if elapsed > ANIMATION_DURATION.into() { - self.disable(queue, radius_buffer); - } + if elapsed > ANIMATION_DURATION.into() { + self.disable(queue, transforms_buffer); + }And update the call site in
ClickAnimationRenderer::draw(line 702):self.click_animations[*slot].draw( render_pass, queue, &self.radius_buffer, + &self.transforms_buffer, &self.transforms_bind_group, &self.radius_bind_group, );core/src/graphics/graphics_context.rs (1)
531-539:⚠️ Potential issue | 🟡 MinorRemove
enable_click_animation— it is superseded bytrigger_click_animation.
trigger_click_animation(line 437) performs the same operation while also signaling the redraw thread viaRedrawThreadCommands::Activity. The publicenable_click_animationmethod (line 535) is only called internally fromtrigger_click_animationand has no external callers. Remove it to reduce API surface and eliminate dead code.
🤖 Fix all issues with AI agents
In `@core/src/graphics/graphics_context.rs`:
- Around line 109-113: The MaxParticipantsReached error variant in the error
enum in graphics_context.rs is unused and triggers clippy warnings; remove the
MaxParticipantsReached variant from that enum (delete the line #[error("Maximum
number of participants reached")] MaxParticipantsReached,) so the enum contains
only actually used variants and rerun cargo clippy -D warnings; if this variant
is planned for future behavior, instead leave a TODO comment and add the variant
only when a code path returns it.
In `@core/src/graphics/participant.rs`:
- Around line 34-70: The disambiguation currently only uses parts[1]; update
generate_unique_visible_name to treat everything after the first token as the
"last" portion (e.g., let remaining = parts[1..].join(" ")), then build
progressive candidates by taking increasing character prefixes of that entire
remaining string (partial_last_name = remaining.chars().take(i).collect()) and
formatting candidates as format!("{first_name} {partial_last_name}"); keep the
original first_name-only candidate, iterate candidates to find the first unused,
and retain the existing numeric fallback using candidates.last().
- Around line 197-211: remove_participant currently logs when
self.participants.remove(sid) returns None but then calls unwrap(), causing a
panic; change the control flow to handle the Option safely (e.g., use if let
Some(participant) = self.participants.remove(sid) { ... } else { log::warn!(...)
; return; }) so you return early when the participant is missing and only push
participant.color into self.available_colors for non-"local" participants inside
the Some branch; reference the remove_participant method, the participants map,
the available_colors deque, and the sid parameter when making the fix.
In `@core/src/lib.rs`:
- Around line 463-470: The participant name passed to
graphics_context.add_participant uses "Me " with a trailing space; remove the
trailing space or explicitly trim it (replace "Me " with "Me" or call .trim())
in the add_participant call, or if the space is intentional leave "Me " but add
a clarifying comment above the add_participant invocation explaining why the
trailing space is required; reference the add_participant("local", "Me ", true)
call in core::graphics_context usage.
- Around line 1182-1191: The redraw handler currently calls
remote_control.gfx.window().focus_window(), toggles visibility, and re-sets the
pencil cursor every RedrawRequested while self.local_drawing.enabled and
cursor_set_times < 500; change this so focus_window() and the visibility toggle
run far less frequently: e.g., only when self.local_drawing.cursor_set_times ==
0 (first frame) or when a stored timestamp (add a field like
local_drawing.last_cursor_set: Instant/Option<Instant>) shows more than a
threshold (e.g., 100–200ms) has passed; still call
set_cursor(remote_control.pencil_cursor.clone()) as needed and increment
cursor_set_times, but guard focus_window() and set_cursor_visible() behind the
one-time or time-based check so they aren’t executed every RedrawRequested.
Ensure you update the struct to track last_cursor_set and use it in the
conditional alongside cursor_set_times.
In `@core/tests/src/main.rs`:
- Around line 243-250: The match arms for DrawingTest::PermanentOn and
DrawingTest::PermanentOff are inverted: swap the printed messages and the test
function calls so that DrawingTest::PermanentOn prints "permanent mode ON..."
and calls remote_drawing::test_draw_and_clear_paths_individually().await?, and
DrawingTest::PermanentOff prints "permanent mode OFF..." and calls
remote_drawing::test_draw_and_clear_all_paths().await?; update the two arms that
currently reference those functions and println! messages accordingly.
🧹 Nitpick comments (10)
core/AGENTS.md (1)
33-40: Architecture overview will become stale as wgpu rendering is removed.Line 37 states "rendering uses
wgpu", which is accurate today but contradicts the direction of this PR ("remove wgpu rendering"). Consider updating this to reflect the iced-based rendering target, or adding a note about the ongoing migration, so the docs don't immediately go stale.core/src/input/mouse.rs (2)
1060-1070: Minor inconsistency: directcursor_stateaccess onSharerCursor.
ControllerCursorexposes ahide_if_expired()wrapper (line 327), butSharerCursordoesn't — so line 1068 reaches directly intosharer_cursor.cursor_state.hide_if_expired(). Consider adding a matching wrapper onSharerCursorfor consistency.♻️ Proposed fix
Add to
SharerCursor:fn hide_if_expired(&mut self) { self.cursor_state.hide_if_expired(); }Then update line 1068:
- sharer_cursor.cursor_state.hide_if_expired(); + sharer_cursor.hide_if_expired();
286-301:set_pointer_modelogic is correct but could benefit from a brief inline comment.The conditional
!enabled && remote_control_enabled → Normalis non-obvious. A one-liner explaining the interaction between per-controller pointer mode and the global enable/disable state would help future readers.core/src/utils/clock.rs (1)
34-37: Consider documenting thatadvancepanics on poisoned mutex.This is test-only code so panicking is acceptable, but a
# Panicsdoc section would be consistent with Rust documentation conventions.core/src/graphics/cursor.rs (3)
49-61: Magic divisor2.5for cursor dimensions is undocumented.The scaling factor
/ 2.5applied to both normal and pointer cursor dimensions has no explanation. Consider extracting it as a named constant with a comment explaining how this value relates to the SVGscale_factor(which is2.0inrender_user_badge_to_png). If the intent is to downscale the 2× rendered image slightly further, the rationale should be clear for future maintainers.Proposed fix
+/// Scale divisor for cursor image dimensions. +/// The SVG is rendered at 2× scale; dividing by 2.5 produces a slightly +/// smaller on-screen cursor to match the design specification. +const CURSOR_DISPLAY_SCALE: f32 = 2.5; + let normal_cursor = ( iced_core::image::Handle::from_bytes(normal_png), ( - normal_dims.width() as f32 / 2.5, - normal_dims.height() as f32 / 2.5, + normal_dims.width() as f32 / CURSOR_DISPLAY_SCALE, + normal_dims.height() as f32 / CURSOR_DISPLAY_SCALE, ), ); let pointer_cursor = ( iced_core::image::Handle::from_bytes(pointer_png), ( - pointer_dims.width() as f32 / 2.5, - pointer_dims.height() as f32 / 2.5, + pointer_dims.width() as f32 / CURSOR_DISPLAY_SCALE, + pointer_dims.height() as f32 / CURSOR_DISPLAY_SCALE, ), );
88-110: Preferif letoveris_none()+unwrap().The early-return guard at line 90 makes the
unwrap()at line 100 safe, but idiomatic Rust would use pattern matching to avoid theunwrap()entirely.Suggested refactor
pub fn draw(&self, frame: &mut Frame) { - if self.position.is_none() { + let Some(position) = &self.position else { return; - } + }; let (handle, (width, height)) = match self.mode { CursorMode::Pointer => &self.pointer_cursor, CursorMode::Normal => &self.normal_cursor, }; let image = iced_core::image::Image::new(handle.clone()); - let position = self.position.as_ref().unwrap(); frame.draw_image( Rectangle { x: position.x as f32, y: position.y as f32, width: *width, height: *height, }, image, ); }
42-47: Full PNG decode just to read dimensions.
image::load_from_memorydecodes the entire PNG into a pixel buffer only to extract width/height. Since this runs at construction time it's unlikely to be a bottleneck, but if you want to avoid the unnecessary decode you could useimage::ImageReader::new(std::io::Cursor::new(&normal_png)).with_guessed_format()?.into_dimensions()?which only reads the header.core/src/graphics/participant.rs (3)
89-95: Redundant field name:color: color.Clippy's
redundant_field_nameslint will flagcolor: colorat line 93. Use the shorthandcolorinstead.As per coding guidelines, "Ensure code passes
cargo fmt --all -- --checkandcargo clippy -D warningsin CI".Fix
Ok(Self { draw: Draw::new(color, auto_clear), cursor: Cursor::new(color, name)?, - color: color, + color, })
166-176: Unnecessary&onDEFAULT_COLOR.
DEFAULT_COLORis already&'static str, so&DEFAULT_COLORcreates&&strwhich then gets coerced back via deref. UsingDEFAULT_COLORdirectly is clearer.Fix
self.available_colors.pop_front().unwrap_or_else(|| { log::warn!( "ParticipantsManager::add_participant: no colors available for participant {}", sid ); - &DEFAULT_COLOR + DEFAULT_COLOR })
328-340: Silent no-op for missing participants in cursor methods.
set_cursor_positionandset_cursor_modesilently ignore missing participants, while every other method inParticipantsManagerlogs a warning when a participant is not found. Consider adding consistent warning logs here too, to aid debugging.Add consistent logging
pub fn set_cursor_position(&mut self, sid: &str, position: Option<Position>) { if let Some(participant) = self.participants.get_mut(sid) { participant.cursor_mut().set_position(position); + } else { + log::warn!("ParticipantsManager::set_cursor_position: participant {} not found", sid); } } pub fn set_cursor_mode(&mut self, sid: &str, mode: CursorMode) { if let Some(participant) = self.participants.get_mut(sid) { participant.cursor_mut().set_mode(mode); + } else { + log::warn!("ParticipantsManager::set_cursor_mode: participant {} not found", sid); } }
|
|
||
| /// Maximum number of participants reached. | ||
| #[error("Maximum number of participants reached")] | ||
| MaxParticipantsReached, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=rust 'MaxParticipantsReached'Repository: gethopp/hopp
Length of output: 125
MaxParticipantsReached error variant is unused and will fail clippy checks.
This variant is defined at line 112 but has no usages in the codebase. Per the coding guidelines, code must pass cargo clippy -D warnings, which flags unused enum variants. Either remove this variant or defer adding it until a code path actually produces this error.
🤖 Prompt for AI Agents
In `@core/src/graphics/graphics_context.rs` around lines 109 - 113, The
MaxParticipantsReached error variant in the error enum in graphics_context.rs is
unused and triggers clippy warnings; remove the MaxParticipantsReached variant
from that enum (delete the line #[error("Maximum number of participants
reached")] MaxParticipantsReached,) so the enum contains only actually used
variants and rerun cargo clippy -D warnings; if this variant is planned for
future behavior, instead leave a TODO comment and add the variant only when a
code path returns it.
| fn generate_unique_visible_name(name: &str, used_names: &[String]) -> String { | ||
| let parts: Vec<&str> = name.split_whitespace().collect(); | ||
| let first_name = parts.first().unwrap_or(&name); | ||
|
|
||
| // Try progressively longer candidates | ||
| let candidates = if parts.len() > 1 { | ||
| let last_name = parts[1]; | ||
| let mut candidates = vec![first_name.to_string()]; | ||
|
|
||
| // Add candidates with increasing characters from last name | ||
| for i in 1..=last_name.chars().count() { | ||
| let partial_last_name: String = last_name.chars().take(i).collect(); | ||
| candidates.push(format!("{first_name} {partial_last_name}")); | ||
| } | ||
| candidates | ||
| } else { | ||
| vec![first_name.to_string()] | ||
| }; | ||
|
|
||
| // Find first unused candidate | ||
| for candidate in candidates.iter() { | ||
| if !used_names.contains(candidate) { | ||
| return candidate.clone(); | ||
| } | ||
| } | ||
|
|
||
| // Fall back to numbering | ||
| let base = candidates.last().unwrap().clone(); | ||
| for num in 2.. { | ||
| let candidate = format!("{base}{num}"); | ||
| if !used_names.contains(&candidate) { | ||
| return candidate; | ||
| } | ||
| } | ||
|
|
||
| unreachable!() | ||
| } |
There was a problem hiding this comment.
Name disambiguation only considers the second word.
For names with more than two parts (e.g., "John van der Berg"), the function only uses parts[1] ("van") for progressive disambiguation, skipping the rest. If two participants share "John van", it falls through to numeric suffixes ("John van2") instead of trying "John van d", "John van de", etc.
This is a minor UX edge case with a safe fallback (numbering), so not blocking.
🤖 Prompt for AI Agents
In `@core/src/graphics/participant.rs` around lines 34 - 70, The disambiguation
currently only uses parts[1]; update generate_unique_visible_name to treat
everything after the first token as the "last" portion (e.g., let remaining =
parts[1..].join(" ")), then build progressive candidates by taking increasing
character prefixes of that entire remaining string (partial_last_name =
remaining.chars().take(i).collect()) and formatting candidates as
format!("{first_name} {partial_last_name}"); keep the original first_name-only
candidate, iterate candidates to find the first unused, and retain the existing
numeric fallback using candidates.last().
| graphics_context | ||
| .add_participant("local".to_string(), "Me ", true) | ||
| .map_err(|e| { | ||
| log::error!( | ||
| "create_overlay_window: Failed to create local participant cursor: {e}" | ||
| ); | ||
| ServerError::GfxCreationError | ||
| })?; |
There was a problem hiding this comment.
Trailing space in local participant name "Me ".
The name "Me " has a trailing space. If this is intentional (e.g., to ensure consistent SVG badge padding), a comment would help clarify. If accidental, it should be trimmed to "Me".
🤖 Prompt for AI Agents
In `@core/src/lib.rs` around lines 463 - 470, The participant name passed to
graphics_context.add_participant uses "Me " with a trailing space; remove the
trailing space or explicitly trim it (replace "Me " with "Me" or call .trim())
in the add_participant call, or if the space is intentional leave "Me " but add
a clarifying comment above the add_participant invocation explaining why the
trailing space is required; reference the add_participant("local", "Me ", true)
call in core::graphics_context usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/graphics/participant.rs`:
- Around line 160-195: add_participant currently overwrites an existing
Participant for the same sid without returning its color to available_colors,
draining the color pool; modify add_participant to check for an existing
participant (e.g., lookup or remove via self.participants.get/ remove/entry for
the given sid) before inserting the new one, capture the old participant's color
(using the Participant accessor or by taking the old Participant value), and
recycle that color back into self.available_colors (push_back) unless it's the
reserved SHARER_COLOR or a fallback DEFAULT_COLOR; then proceed to insert the
new Participant via Participant::new as before.
🧹 Nitpick comments (1)
core/src/graphics/participant.rs (1)
214-312: Consider extracting repeated participant-lookup-or-warn into a helper.Every method follows the same
if let Some(p) = self.participants.get_mut(sid) { … } else { log::warn!(…) }pattern. A small private helper (e.g.,fn get_participant_mut(&mut self, sid: &str, caller: &str) -> Option<&mut Participant>) could reduce boilerplate.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/graphics/iced_canvas.rs`:
- Line 66: The constructor signature pub fn new(texture_path: &String) -> Self
will trigger clippy::ptr_arg; change it to accept a borrowed string slice (pub
fn new(texture_path: &str) -> Self) and update any internal uses of texture_path
and all call sites to pass &my_string or a &str as appropriate so the type
matches; ensure any conversions (e.g., to Path/PathBuf) still compile by calling
Path::new(texture_path) or texture_path.to_string() where needed.
🧹 Nitpick comments (5)
core/src/graphics/click_animation.rs (3)
9-10: Inconsistent types between duration constants.
ANIMATION_DURATIONisu64(andpub) whileRING_DELAY_MSisu128. This forces.into()conversions at every comparison site (e.g., Line 99, 124). Consider making both the same type for clarity.
23-28: Per-animationArc<dyn Clock>clone is unnecessary.Every
ClickAnimationstores its ownArc<dyn Clock>, but the clock is only used inenable()to snapshot the instant. SinceClickAnimationRendereralready owns the clock and has&mut selfwhen callingenable(), you could pass the instant directly toenable()and remove the clock field fromClickAnimation.Sketch
struct ClickAnimation { position: Position, enabled_instant: Option<std::time::Instant>, - clock: Arc<dyn Clock>, } impl ClickAnimation { - fn enable(&mut self, position: Position) { + fn enable(&mut self, position: Position, now: std::time::Instant) { self.position = position; - self.enabled_instant = Some(self.clock.now()); + self.enabled_instant = Some(now); }In
ClickAnimationRenderer::update():+ let now = self.clock.now(); // ... - self.click_animations[slot].enable(position); + self.click_animations[slot].enable(position, now);
75-79:enable_click_animationonly needs&self, not&mut self.
Sender::sendtakes&selfin the standard library. If this method only sends on the channel, the&mut selfrequirement is needlessly restrictive and prevents calling it while other immutable borrows are held.Proposed fix
- pub fn enable_click_animation(&mut self, position: Position) { + pub fn enable_click_animation(&self, position: Position) {core/src/graphics/graphics_context.rs (1)
257-266: Non-idiomatic error handling for adapter request.The
if let Errfollowed byunwrap()pattern is safe but not idiomatic Rust. Consider using the?operator ormatchdirectly.Proposed fix
- let adapter = pollster::block_on(instance.request_adapter(&wgpu::RequestAdapterOptions { + let adapter = pollster::block_on(instance.request_adapter(&wgpu::RequestAdapterOptions { power_preference: wgpu::PowerPreference::HighPerformance, compatible_surface: Some(&surface), force_fallback_adapter: false, - })); - if let Err(e) = adapter { - log::error!("GraphicsContext::new request_adapter: {e:?}"); - return Err(OverlayError::AdapterRequestError); - } - let adapter = adapter.unwrap(); + })) + .map_err(|e| { + log::error!("GraphicsContext::new request_adapter: {e:?}"); + OverlayError::AdapterRequestError + })?;core/src/graphics/participant.rs (1)
212-227:remove_participantno longer panics — consider usinglet-elsefor clarity.The early
returnon line 221 makes theunwrap()on line 223 safe. However, the idiomatic Rust pattern is cleaner withlet-else:Proposed fix
pub fn remove_participant(&mut self, sid: &str) { log::info!("ParticipantsManager::remove_participant: sid={}", sid); - let participant = self.participants.remove(sid); - if participant.is_none() { + let Some(participant) = self.participants.remove(sid) else { log::warn!( "ParticipantsManager::remove_participant: participant {} not found", sid ); + return; }; - let participant = participant.unwrap(); if sid != "local" { self.available_colors.push_back(participant.color); } }
| } | ||
|
|
||
| impl OverlaySurface { | ||
| pub fn new(texture_path: &String) -> Self { |
There was a problem hiding this comment.
&String parameter will trigger clippy::ptr_arg warning.
Per the coding guidelines, code must pass cargo clippy -D warnings. Prefer &str over &String.
Proposed fix
- pub fn new(texture_path: &String) -> Self {
+ pub fn new(texture_path: &str) -> Self {As per coding guidelines, "Ensure code passes cargo fmt --all -- --check and cargo clippy -D warnings in CI".
📝 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 new(texture_path: &String) -> Self { | |
| pub fn new(texture_path: &str) -> Self { |
🤖 Prompt for AI Agents
In `@core/src/graphics/iced_canvas.rs` at line 66, The constructor signature pub
fn new(texture_path: &String) -> Self will trigger clippy::ptr_arg; change it to
accept a borrowed string slice (pub fn new(texture_path: &str) -> Self) and
update any internal uses of texture_path and all call sites to pass &my_string
or a &str as appropriate so the type matches; ensure any conversions (e.g., to
Path/PathBuf) still compile by calling Path::new(texture_path) or
texture_path.to_string() where needed.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation