feat: double-click dedup and right-click filtering for auto-zoom#1665
feat: double-click dedup and right-click filtering for auto-zoom#1665namearth5005 wants to merge 8 commits intoCapSoftware:mainfrom
Conversation
…nfigurable fields
…placing hardcoded constants
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let moves = vec![ | ||
| move_event(500.0, 0.1, 0.1), | ||
| move_event(999.0, 0.5, 0.5), | ||
| move_event(1500.0, 0.9, 0.9), | ||
| ]; | ||
|
|
||
| let config = cap_project::AutoZoomConfig { | ||
| ignore_right_clicks: true, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let segments = generate_zoom_segments_from_clicks_impl(clicks, moves, 20.0, &config); | ||
|
|
||
| let has_click_segment = segments.iter().any(|s| { | ||
| let click_time_secs = 1.0; | ||
| s.start <= click_time_secs && s.end >= click_time_secs | ||
| }); | ||
|
|
||
| assert!( | ||
| !has_click_segment, | ||
| "right-click should be filtered out when ignore_right_clicks is true" | ||
| ); |
There was a problem hiding this comment.
right_click_ignored test will always fail due to movement segments
The test includes three move events with large displacements — (0.1→0.5→0.9) in both axes — that will generate movement-based zoom segments regardless of whether right-clicks are filtered.
When the move event at 999ms is processed:
- Distance ≈
sqrt((0.5-0.1)² + (0.5-0.1)²) ≈ 0.566, which far exceeds bothmovement_event_distance_threshold(0.02) andmovement_window_distance_threshold(0.08). - This produces a segment from
0.699sto2.499s, which coverst=1.0s(the click time).
Since has_click_segment checks if any segment covers t=1.0s (not just click-generated ones), the movement segment causes the assertion !has_click_segment to fail — even when right-click filtering is working correctly.
To fix the test, the movement events should be removed or replaced with small/stationary ones that won't trigger the movement detection threshold:
| let moves = vec![ | |
| move_event(500.0, 0.1, 0.1), | |
| move_event(999.0, 0.5, 0.5), | |
| move_event(1500.0, 0.9, 0.9), | |
| ]; | |
| let config = cap_project::AutoZoomConfig { | |
| ignore_right_clicks: true, | |
| ..Default::default() | |
| }; | |
| let segments = generate_zoom_segments_from_clicks_impl(clicks, moves, 20.0, &config); | |
| let has_click_segment = segments.iter().any(|s| { | |
| let click_time_secs = 1.0; | |
| s.start <= click_time_secs && s.end >= click_time_secs | |
| }); | |
| assert!( | |
| !has_click_segment, | |
| "right-click should be filtered out when ignore_right_clicks is true" | |
| ); | |
| let moves: Vec<cap_project::cursor_captures::CursorMoveEvent> = vec![]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 2675-2696
Comment:
**`right_click_ignored` test will always fail due to movement segments**
The test includes three move events with large displacements — (0.1→0.5→0.9) in both axes — that will generate movement-based zoom segments regardless of whether right-clicks are filtered.
When the move event at `999ms` is processed:
- Distance ≈ `sqrt((0.5-0.1)² + (0.5-0.1)²) ≈ 0.566`, which far exceeds both `movement_event_distance_threshold` (0.02) and `movement_window_distance_threshold` (0.08).
- This produces a segment from `0.699s` to `2.499s`, which covers `t=1.0s` (the click time).
Since `has_click_segment` checks if **any** segment covers `t=1.0s` (not just click-generated ones), the movement segment causes the assertion `!has_click_segment` to fail — even when right-click filtering is working correctly.
To fix the test, the movement events should be removed or replaced with small/stationary ones that won't trigger the movement detection threshold:
```suggestion
let moves: Vec<cap_project::cursor_captures::CursorMoveEvent> = vec![];
```
How can I resolve this? If you propose a fix, please make it concise.| let in_dead_zone = dead_zone_radius > 0.0 && click_pos.is_some() && { | ||
| let (cx, cy) = click_pos.unwrap(); | ||
| let group_positions: Vec<(f64, f64)> = group | ||
| .iter() | ||
| .filter_map(|&gi| click_positions.get(&gi).copied()) | ||
| .collect(); | ||
| if group_positions.is_empty() { | ||
| false | ||
| } else { | ||
| let count = group_positions.len() as f64; | ||
| let centroid_x = group_positions.iter().map(|(x, _)| x).sum::<f64>() / count; | ||
| let centroid_y = group_positions.iter().map(|(_, y)| y).sum::<f64>() / count; | ||
| let dx = cx - centroid_x; | ||
| let dy = cy - centroid_y; | ||
| (dx * dx + dy * dy).sqrt() < dead_zone_radius | ||
| } | ||
| }; | ||
|
|
||
| if time_and_spatial || in_dead_zone { |
There was a problem hiding this comment.
The in_dead_zone condition merges a new click into an existing group based purely on spatial proximity to the group's centroid. Unlike time_and_spatial, which enforces both time_close (< 2.5 s by default) and spatial proximity, in_dead_zone has no temporal bound.
Consider a scenario where a user clicks a toolbar button at t=5s, works for 3 minutes, then clicks the same button again at t=185s. Both clicks are within dead_zone_radius (0.1) of each other, so the second click is merged into the first group. The resulting segment spans from ~4.6s to ~186.8s — nearly the entire recording — instead of producing two short, appropriate zoom segments.
To prevent this, the dead zone check should also respect the same click_group_time_threshold_secs (or a comparable time window):
let in_dead_zone = dead_zone_radius > 0.0 && click_pos.is_some() && {
let (cx, cy) = click_pos.unwrap();
// Check time proximity first to avoid iterating positions unnecessarily
let time_close = group.iter().any(|&gi| {
let group_time = clicks[gi].time_ms / 1000.0;
(click_time - group_time).abs() < click_group_time_threshold_secs
});
if !time_close {
false
} else {
let group_positions: Vec<(f64, f64)> = group
.iter()
.filter_map(|&gi| click_positions.get(&gi).copied())
.collect();
if group_positions.is_empty() {
false
} else {
let count = group_positions.len() as f64;
let centroid_x = group_positions.iter().map(|(x, _)| x).sum::<f64>() / count;
let centroid_y = group_positions.iter().map(|(_, y)| y).sum::<f64>() / count;
let dx = cx - centroid_x;
let dy = cy - centroid_y;
(dx * dx + dy * dy).sqrt() < dead_zone_radius
}
}
};Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 2123-2141
Comment:
**Dead zone merging has no time constraint**
The `in_dead_zone` condition merges a new click into an existing group based purely on spatial proximity to the group's centroid. Unlike `time_and_spatial`, which enforces both `time_close` (< 2.5 s by default) and spatial proximity, `in_dead_zone` has **no temporal bound**.
Consider a scenario where a user clicks a toolbar button at `t=5s`, works for 3 minutes, then clicks the same button again at `t=185s`. Both clicks are within `dead_zone_radius` (0.1) of each other, so the second click is merged into the first group. The resulting segment spans from `~4.6s` to `~186.8s` — nearly the entire recording — instead of producing two short, appropriate zoom segments.
To prevent this, the dead zone check should also respect the same `click_group_time_threshold_secs` (or a comparable time window):
```rust
let in_dead_zone = dead_zone_radius > 0.0 && click_pos.is_some() && {
let (cx, cy) = click_pos.unwrap();
// Check time proximity first to avoid iterating positions unnecessarily
let time_close = group.iter().any(|&gi| {
let group_time = clicks[gi].time_ms / 1000.0;
(click_time - group_time).abs() < click_group_time_threshold_secs
});
if !time_close {
false
} else {
let group_positions: Vec<(f64, f64)> = group
.iter()
.filter_map(|&gi| click_positions.get(&gi).copied())
.collect();
if group_positions.is_empty() {
false
} else {
let count = group_positions.len() as f64;
let centroid_x = group_positions.iter().map(|(x, _)| x).sum::<f64>() / count;
let centroid_y = group_positions.iter().map(|(_, y)| y).sum::<f64>() / count;
let dx = cx - centroid_x;
let dy = cy - centroid_y;
(dx * dx + dy * dy).sqrt() < dead_zone_radius
}
}
};
```
How can I resolve this? If you propose a fix, please make it concise.| if config.double_click_threshold_ms > 0.0 { | ||
| let mut i = 0; | ||
| while i < clicks.len() { | ||
| if !clicks[i].down { | ||
| i += 1; | ||
| continue; | ||
| } | ||
| let mut j = i + 1; | ||
| while j < clicks.len() { | ||
| if !clicks[j].down { | ||
| j += 1; | ||
| continue; | ||
| } | ||
| if clicks[j].time_ms - clicks[i].time_ms > config.double_click_threshold_ms { | ||
| break; | ||
| } | ||
| if clicks[j].cursor_num == clicks[i].cursor_num { | ||
| clicks.remove(j); | ||
| } else { | ||
| j += 1; | ||
| } | ||
| } | ||
| i += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
When a duplicate down event is removed via clicks.remove(j), its paired up event (down: false) is left in the vector. For example, with a double-click sequence [1000ms↓, 1050ms↑, 1200ms↓, 1250ms↑], after dedup the vector becomes [1000ms↓, 1050ms↑, 1250ms↑] — with an orphaned up event at 1250ms.
This doesn't cause incorrect segment generation currently (grouping only iterates c.down events), but it leaves the clicks vector in an inconsistent state that could silently break future code relying on down/up pairing. Consider also removing the immediately-following up event:
if clicks[j].cursor_num == clicks[i].cursor_num {
clicks.remove(j);
// Also remove the paired up-event if it immediately follows
if j < clicks.len() && !clicks[j].down && clicks[j].cursor_num == clicks[i].cursor_num {
clicks.remove(j);
}
} else {
j += 1;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/recording.rs
Line: 2045-2069
Comment:
**Dedup removes down events but leaves orphaned up events**
When a duplicate down event is removed via `clicks.remove(j)`, its paired up event (`down: false`) is left in the vector. For example, with a double-click sequence `[1000ms↓, 1050ms↑, 1200ms↓, 1250ms↑]`, after dedup the vector becomes `[1000ms↓, 1050ms↑, 1250ms↑]` — with an orphaned up event at 1250ms.
This doesn't cause incorrect segment generation currently (grouping only iterates `c.down` events), but it leaves the `clicks` vector in an inconsistent state that could silently break future code relying on down/up pairing. Consider also removing the immediately-following up event:
```rust
if clicks[j].cursor_num == clicks[i].cursor_num {
clicks.remove(j);
// Also remove the paired up-event if it immediately follows
if j < clicks.len() && !clicks[j].down && clicks[j].cursor_num == clicks[i].cursor_num {
clicks.remove(j);
}
} else {
j += 1;
}
```
How can I resolve this? If you propose a fix, please make it concise.| autoZoomConfig: { | ||
| zoomAmount: 1.5, | ||
| clickGroupTimeThreshold: 2.5, | ||
| clickGroupSpatialThreshold: 0.15, | ||
| clickPrePadding: 0.4, | ||
| clickPostPadding: 1.8, | ||
| movementPrePadding: 0.3, | ||
| movementPostPadding: 1.5, | ||
| mergeGapThreshold: 0.8, | ||
| minSegmentDuration: 1.0, | ||
| movementEventDistanceThreshold: 0.02, | ||
| movementWindowDistanceThreshold: 0.08, | ||
| deadZoneRadius: 0.1, | ||
| doubleClickThresholdMs: 400.0, | ||
| ignoreRightClicks: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The autoZoomConfig fallback object in createStore duplicates the default values from AutoZoomConfig::default() in Rust. If a default is updated on the backend (e.g., zoom_amount is bumped from 1.5 to 2.0), the frontend fallback will silently serve the stale value to users who have never persisted a config.
Consider deriving these defaults from the serialized Tauri command response rather than hardcoding them. At a minimum, the fallback should be an empty object ({}) that lets backend-provided serde defaults fill in, rather than a manually maintained copy:
| autoZoomConfig: { | |
| zoomAmount: 1.5, | |
| clickGroupTimeThreshold: 2.5, | |
| clickGroupSpatialThreshold: 0.15, | |
| clickPrePadding: 0.4, | |
| clickPostPadding: 1.8, | |
| movementPrePadding: 0.3, | |
| movementPostPadding: 1.5, | |
| mergeGapThreshold: 0.8, | |
| minSegmentDuration: 1.0, | |
| movementEventDistanceThreshold: 0.02, | |
| movementWindowDistanceThreshold: 0.08, | |
| deadZoneRadius: 0.1, | |
| doubleClickThresholdMs: 400.0, | |
| ignoreRightClicks: true, | |
| }, | |
| }, | |
| autoZoomConfig: {} as AutoZoomConfig, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
Line: 68-84
Comment:
**Hardcoded default config values may drift from Rust defaults**
The `autoZoomConfig` fallback object in `createStore` duplicates the default values from `AutoZoomConfig::default()` in Rust. If a default is updated on the backend (e.g., `zoom_amount` is bumped from 1.5 to 2.0), the frontend fallback will silently serve the stale value to users who have never persisted a config.
Consider deriving these defaults from the serialized Tauri command response rather than hardcoding them. At a minimum, the fallback should be an empty object (`{}`) that lets backend-provided `serde` defaults fill in, rather than a manually maintained copy:
```suggestion
autoZoomConfig: {} as AutoZoomConfig,
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Adds click preprocessing to auto-zoom segment generation (refs #1646, builds on #1663, #1664):
double_click_threshold_ms(default 400ms) on the same button are collapsed to a single click, preventing redundant zoom segments from double-clicksignore_right_clicksis true (default), non-primary button clicks (cursor_num != 0) are stripped before segment generation, so context menu interactions don't trigger zoomHow it works
Preprocessing runs before click sorting and grouping:
clicks.retain(|c| c.cursor_num == 0)removes right/middle clicksConfig fields added to
AutoZoomConfigdouble_click_threshold_msignore_right_clicksTest plan
double_click_deduplication— two clicks 200ms apart produce same segments as single clickright_click_ignored— right-click produces no zoom segment when filtering enabledright_click_allowed_when_disabled— right-click works normally when filtering disabledGreptile Summary
This PR adds two click preprocessing steps to the auto-zoom segment generator — double-click deduplication (collapsing rapid same-button clicks within a configurable threshold) and right-click filtering — along with a new
AutoZoomConfigstruct that promotes all previously hardcoded constants into configurable fields. Adead_zone_radiusfeature is also quietly introduced, merging spatially co-located clicks into a single zoom group.Key changes:
AutoZoomConfigstruct added tocrates/project/src/configuration.rswith 14 fields and sensible defaults; wired intoGeneralSettingsStore, recording completion, and the Tauri editor command.generate_zoom_segments_from_clicks_implnow accepts&AutoZoomConfig, replacing allconstliterals with configurable values, and runs the two new preprocessing passes before grouping.experimental.tsxgains aSettingSlidercomponent and exposes Zoom Amount, Sensitivity, and Smoothing controls when auto-zoom is enabled.Issues found:
right_click_ignoredtest is broken: the three large movement events included in the test generate movement-based segments that covert=1.0s, makinghas_click_segmentalwaystrueand the assertion always fail — even when right-click filtering is working correctly. The moves should be removed or replaced with zero-displacement events.in_dead_zonechecks only spatial proximity to the group centroid, with no temporal bound. Two clicks at the same screen position minutes apart will be collapsed into one group, producing an erroneously long zoom segment. The check should be gated behind the sameclick_group_time_threshold_secsguard used bytime_and_spatial.clicks.remove(j)drops the duplicate down-event but leaves its paired up-event in the vector; while harmless today, it leaves the clicks slice inconsistent for any future consumer of up-events.autoZoomConfigobject inexperimental.tsxduplicates Rust-side defaults and will silently diverge if backend defaults change.Confidence Score: 2/5
right_click_ignoredtest move events and add a time constraint to thein_dead_zonegrouping condition.Important Files Changed
right_click_ignoredtest will fail due to large movement events generating segments, and the dead zone grouping condition lacks a time constraint that can create excessively long zoom segments.AutoZoomConfigstruct added with sensible defaults; clean derivation ofType,Serialize,Deserialize, and explicitDefaultimpl — no issues found.auto_zoom_configfield toGeneralSettingsStorewith#[serde(default)]and properDefaultinitialization — straightforward and correct.AppHandle, reads settings with a silent fallback to defaults (unwrap_or(None).unwrap_or_default()), and threads the config through correctly; the silent error swallowing is acceptable here but worth noting.SettingSlidercomponent and three auto-zoom sliders added; hardcoded default values in the fallback store object risk drifting from backend defaults, but the UI logic itself is correct.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[generate_zoom_segments_from_clicks_impl] --> B{ignore_right_clicks?} B -- Yes --> C[clicks.retain cursor_num == 0] B -- No --> D[Keep all clicks] C --> E[Sort clicks by time_ms] D --> E E --> F{double_click_threshold_ms > 0?} F -- Yes --> G[Deduplicate: remove same-button down-clicks within threshold window] F -- No --> H[Skip dedup] G --> I[Remove trailing clicks beyond activity_end_limit] H --> I I --> J[Build click_positions map from move events] J --> K[Group clicks by time + spatial proximity OR dead zone centroid] K --> L[Build click intervals with pre/post padding] L --> M[Build movement intervals from significant moves] M --> N[Merge overlapping intervals with gap threshold] N --> O[Filter segments below min_segment_duration] O --> P[Return ZoomSegments with zoom_amount]Prompt To Fix All With AI
Last reviewed commit: "feat(recording): add..."
(4/5) You can add custom instructions or style guidelines for the agent here!