timeline: suggest zoom tracks from click clusters instead of dwell heuristics#405
Merged
Conversation
…uristics Replace the dwell-ranking-based auto-zoom algorithm with a pure click-cluster approach: - Extract explicit click events (click, double-click, right-click, middle-click) from cursor telemetry via detectInteractionCandidates - Chain-merge clicks that occur within 2500 ms of each other into one cluster - Expand each cluster to a zoom window of [firstClick - 500 ms, lastClick + 500 ms] - Skip clusters that overlap already-placed zoom regions (reservedSpans) New exported constants: CLICK_CLUSTER_MERGE_GAP_MS (2500) and CLICK_CLUSTER_PAD_MS (500). Adds zoomSuggestionUtils.test.ts with 7 unit tests covering the new logic.
Contributor
📝 WalkthroughWalkthroughClusters explicit click interactions by temporal proximity and emits one padded zoom window per cluster (clamped to timeline bounds), skipping regions overlapping reserved spans; candidates are tagged with ChangesClick-Cluster Zoom Suggestions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/timeline/zoomSuggestionUtils.ts (1)
348-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow single-click telemetry through this guard.
With the new click-driven algorithm, a payload containing exactly one explicit click is already enough to build a cluster. This
normalizedSamples.length < 2check still returns"no-telemetry"for that valid case, so isolated clicks get dropped unless callers pad the array with synthetic move events.Suggested fix
- if (normalizedSamples.length < 2) { + if (normalizedSamples.length === 0) { return { status: "no-telemetry", suggestions: [] }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts` around lines 348 - 349, The current guard returns { status: "no-telemetry" } whenever normalizedSamples.length < 2, which drops valid single-click telemetry; change the check to only return "no-telemetry" when there are zero samples or when there is exactly one sample that is not an explicit click. Concretely, replace the condition using normalizedSamples.length < 2 with logic that: if normalizedSamples.length === 0 return no-telemetry; else if normalizedSamples.length === 1 then allow the sample through when normalizedSamples[0].isExplicitClick (or normalizedSamples[0].type === 'click' if your samples use type), otherwise return no-telemetry; keep all downstream code that builds clusters unchanged (refer to normalizedSamples and the returned status "no-telemetry").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts`:
- Around line 352-355: The current clickCandidates selection still admits
dwell-derived suggestions because detectInteractionCandidates() returns mixed
kinds; update the logic so candidates come only from explicit UI click
telemetry: either (A) change detectInteractionCandidates or its caller to
propagate an explicit source/interactionType flag (e.g.,
candidate.interactionType or candidate.source) when it detects uiohook click
events, then filter clickCandidates by that flag (e.g.,
candidate.interactionType === "explicit-click" or candidate.source ===
"uiohook"); or (B) skip detectInteractionCandidates here and instead build
clickCandidates by scanning normalizedSamples for explicit click sample types
before passing to suggestion logic. Ensure you reference
detectInteractionCandidates, clickCandidates, normalizedSamples and the new
candidate.interactionType/source flag when making the change.
---
Outside diff comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts`:
- Around line 348-349: The current guard returns { status: "no-telemetry" }
whenever normalizedSamples.length < 2, which drops valid single-click telemetry;
change the check to only return "no-telemetry" when there are zero samples or
when there is exactly one sample that is not an explicit click. Concretely,
replace the condition using normalizedSamples.length < 2 with logic that: if
normalizedSamples.length === 0 return no-telemetry; else if
normalizedSamples.length === 1 then allow the sample through when
normalizedSamples[0].isExplicitClick (or normalizedSamples[0].type === 'click'
if your samples use type), otherwise return no-telemetry; keep all downstream
code that builds clusters unchanged (refer to normalizedSamples and the returned
status "no-telemetry").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b126b100-9a86-45bd-bf24-346e3d83fe63
📒 Files selected for processing (2)
src/components/video-editor/timeline/zoomSuggestionUtils.test.tssrc/components/video-editor/timeline/zoomSuggestionUtils.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/timeline/zoomSuggestionUtils.ts (1)
191-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a click-type allowlist instead of a denylist for explicit candidates.
Line 191 currently treats any non-
move/non-mouseupinteraction as “explicit,” which can admit non-click events and violate the PR contract (“click, double-click, right-click, middle-click only”). Also, Line 355-361 duplicates this logic separately.Suggested patch
+const EXPLICIT_CLICK_TYPES = new Set([ + "click", + "double-click", + "right-click", + "middle-click", +]); + +function isExplicitClickType(interactionType: CursorTelemetryPoint["interactionType"]): boolean { + return typeof interactionType === "string" && EXPLICIT_CLICK_TYPES.has(interactionType); +} + export function detectInteractionCandidates( samples: CursorTelemetryPoint[], ): CursorInteractionCandidate[] { // --- Phase 1: Explicit interaction events (from uiohook telemetry) --- const clickEvents = samples.filter( - (s) => s.interactionType && s.interactionType !== "move" && s.interactionType !== "mouseup", + (s) => isExplicitClickType(s.interactionType), );- if ( - normalizedSamples.length === 1 && - normalizedSamples[0].interactionType !== "click" && - normalizedSamples[0].interactionType !== "double-click" && - normalizedSamples[0].interactionType !== "right-click" && - normalizedSamples[0].interactionType !== "middle-click" - ) { + if (normalizedSamples.length === 1 && !isExplicitClickType(normalizedSamples[0].interactionType)) { return { status: "no-telemetry", suggestions: [] }; }Also applies to: 355-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts` around lines 191 - 193, The filter treating anything not "move"/"mouseup" as a click is too permissive; replace that denylist with an explicit allowlist of click interaction types (e.g., "click", "doubleclick", "rightclick", "middleclick") by changing the samples.filter call that defines clickEvents to check membership in the allowlist, and remove the duplicated denylist logic at lines 355–361 by extracting a single helper/constant (e.g., isClickInteraction or ALLOWED_CLICK_INTERACTIONS) and using it in both places (the clickEvents filter and the logic at 355–361) so the same explicit click-definition is applied consistently.
🧹 Nitpick comments (1)
src/components/video-editor/timeline/zoomSuggestionUtils.test.ts (1)
31-184: ⚡ Quick winAdd explicit tests for
right-clickandmiddle-clickinputs.The suite is strong, but the contract explicitly includes right/middle clicks; adding one parameterized test for those interaction types would prevent regression on accepted click sources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/zoomSuggestionUtils.test.ts` around lines 31 - 184, Add a parameterized test to the existing suite that verifies right-click and middle-click are accepted like a left-click: use the makeClick helper to create clicks with interactionType 'right' and 'middle' (e.g., makeClick(5000, 'right')) and pass them through withMoves/TOTAL_MS into buildInteractionZoomSuggestions, then assert result.status === "ok", suggestions.length === 1 and the suggestion start/end match the CLICK_CLUSTER_PAD_MS padding (use CLICK_CLUSTER_PAD_MS to compute expectations); place this alongside the existing single-click tests so regressions for non-left click types are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts`:
- Around line 191-193: The filter treating anything not "move"/"mouseup" as a
click is too permissive; replace that denylist with an explicit allowlist of
click interaction types (e.g., "click", "doubleclick", "rightclick",
"middleclick") by changing the samples.filter call that defines clickEvents to
check membership in the allowlist, and remove the duplicated denylist logic at
lines 355–361 by extracting a single helper/constant (e.g., isClickInteraction
or ALLOWED_CLICK_INTERACTIONS) and using it in both places (the clickEvents
filter and the logic at 355–361) so the same explicit click-definition is
applied consistently.
---
Nitpick comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.test.ts`:
- Around line 31-184: Add a parameterized test to the existing suite that
verifies right-click and middle-click are accepted like a left-click: use the
makeClick helper to create clicks with interactionType 'right' and 'middle'
(e.g., makeClick(5000, 'right')) and pass them through withMoves/TOTAL_MS into
buildInteractionZoomSuggestions, then assert result.status === "ok",
suggestions.length === 1 and the suggestion start/end match the
CLICK_CLUSTER_PAD_MS padding (use CLICK_CLUSTER_PAD_MS to compute expectations);
place this alongside the existing single-click tests so regressions for non-left
click types are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dbe4a32-2c0e-4d1f-a3d7-6d6457850c36
📒 Files selected for processing (2)
src/components/video-editor/timeline/zoomSuggestionUtils.test.tssrc/components/video-editor/timeline/zoomSuggestionUtils.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/video-editor/timeline/zoomSuggestionUtils.test.ts (1)
195-206: ⚡ Quick winBoundary test doesn’t exercise end-clamping path yet.
With
makeClick(200)andtotalMs: 1000,endbecomes700, so a regression in end clamping could slip through. Add a near-end click and assert exact clamp values.Suggested test hardening
it("clamps start to 0 and end to totalMs at video boundaries", () => { - const result = buildInteractionZoomSuggestions({ - cursorTelemetry: withMoves([makeClick(200)], 1_000), - totalMs: 1_000, - defaultDurationMs: 3_000, - }); - - expect(result.status).toBe("ok"); - const [s] = result.suggestions; - expect(s.start).toBeGreaterThanOrEqual(0); - expect(s.end).toBeLessThanOrEqual(1_000); + const startClamp = buildInteractionZoomSuggestions({ + cursorTelemetry: withMoves([makeClick(200)], 1_000), + totalMs: 1_000, + defaultDurationMs: 3_000, + }); + expect(startClamp.status).toBe("ok"); + expect(startClamp.suggestions[0]).toMatchObject({ start: 0, end: 700 }); + + const endClamp = buildInteractionZoomSuggestions({ + cursorTelemetry: withMoves([makeClick(900)], 1_000), + totalMs: 1_000, + defaultDurationMs: 3_000, + }); + expect(endClamp.status).toBe("ok"); + expect(endClamp.suggestions[0]).toMatchObject({ start: 400, end: 1_000 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/zoomSuggestionUtils.test.ts` around lines 195 - 206, Update the test using buildInteractionZoomSuggestions so it triggers the end-clamping path by using a near-end click (e.g. makeClick close to totalMs) instead of makeClick(200); call withMoves([makeClick(900)], 1_000) (or similar near-1000 value) and then assert exact clamp values: expect(s.start).toBe(0) and expect(s.end).toBe(1_000). Keep references to buildInteractionZoomSuggestions, withMoves, makeClick, and the suggestions result status check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.test.ts`:
- Around line 195-206: Update the test using buildInteractionZoomSuggestions so
it triggers the end-clamping path by using a near-end click (e.g. makeClick
close to totalMs) instead of makeClick(200); call withMoves([makeClick(900)],
1_000) (or similar near-1000 value) and then assert exact clamp values:
expect(s.start).toBe(0) and expect(s.end).toBe(1_000). Keep references to
buildInteractionZoomSuggestions, withMoves, makeClick, and the suggestions
result status check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd6353d7-a4cb-4ed8-9872-d9dd21418bc8
📒 Files selected for processing (2)
src/components/video-editor/timeline/zoomSuggestionUtils.test.tssrc/components/video-editor/timeline/zoomSuggestionUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/timeline/zoomSuggestionUtils.ts
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the dwell-ranking-based auto-zoom suggestion algorithm with a pure click-cluster approach.
New behaviour
firstClick − 500 ms … lastClick + 500 ms, clamped to the video boundsreservedSpans) are skippedChanged files
zoomSuggestionUtils.tsbuildInteractionZoomSuggestionsbody; addedbuildClickClustershelper; exportedCLICK_CLUSTER_MERGE_GAP_MS(2500) andCLICK_CLUSTER_PAD_MS(500)zoomSuggestionUtils.test.tsNo breaking changes
The
buildInteractionZoomSuggestionsfunction signature is unchanged; the call site inTimelineEditor.tsxrequires no edits.Summary by CodeRabbit
Tests
Refactor