feat: auto-zoom on clicks + exclude OpenScreen windows from display capture#640
Open
realkim93 wants to merge 2 commits into
Open
feat: auto-zoom on clicks + exclude OpenScreen windows from display capture#640realkim93 wants to merge 2 commits into
realkim93 wants to merge 2 commits into
Conversation
…play capture Cursor telemetry already records click events (interactionType), but the suggestion algorithm only looks at cursor dwell. This wires clicks through to the suggestion pipeline so the "Suggest Zooms from Cursor" button can produce zooms where the user actually clicked, not just where the cursor paused. - Pass interactionType through readCursorTelemetryFile (was stripped on load). - Add detectZoomClickCandidates with 700ms clustering for double/triple clicks. - detectZoomCandidates combines click + dwell with clicks ranked stronger. - TimelineEditor switches to the combined detector. Separately, full-screen recordings include the OpenScreen HUD because the helper passes excludingWindows: []. Allow the renderer to pass its own pid through the request and have the SCK helper exclude any windows owned by that process / bundle identifier from the SCContentFilter. Tests: add zoomSuggestionUtils.test.ts (5 cases).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds interactionType to cursor telemetry, implements click-clustering zoom candidates (merged with dwell candidates), updates the timeline to use merged candidates, and adds macOS screen-capture exclusions by bundle ID or process ID (electron passes current PID when capturing a display). ChangesCursor telemetry enrichment, click detection, and macOS app exclusion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Preserve interactionType during telemetry normalization
detectZoomCandidates now relies on click metadata, but normalizeCursorTelemetry strips interactionType in normalizeTelemetrySample, so every sample passed from TimelineEditor loses click/double-click/right-click markers before detection. In recordings where zoom suggestions should come from clicks, this makes the new click-based path return no click candidates and effectively disables the feature in normal UI flow.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
electron/ipc/handlers.ts (1)
510-520:⚠️ Potential issue | 🟠 Major | ⚡ Quick wininteractionType is still getting flattened before this mapping
nice pass-through here, but lowkey risky:
normalizeCursorSamplecurrently downgrades anything outside"click" | "mouseup" | "move"to"move", so"double-click","right-click", and"middle-click"never survive to the renderer.Suggested fix
diff --git a/src/native/contracts.ts b/src/native/contracts.ts @@ export interface CursorRecordingSample extends CursorTelemetryPoint { assetId?: string | null; visible?: boolean; cursorType?: NativeCursorType | null; - interactionType?: "move" | "click" | "mouseup"; + interactionType?: "move" | "click" | "double-click" | "right-click" | "middle-click" | "mouseup"; } diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts @@ function normalizeCursorSample(sample: unknown): CursorRecordingSample | null { @@ - const interactionType = - point.interactionType === "click" || - point.interactionType === "mouseup" || - point.interactionType === "move" - ? point.interactionType - : "move"; + const interactionType = + point.interactionType === "click" || + point.interactionType === "double-click" || + point.interactionType === "right-click" || + point.interactionType === "middle-click" || + point.interactionType === "mouseup" || + point.interactionType === "move" + ? point.interactionType + : "move";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/handlers.ts` around lines 510 - 520, The mapping in readCursorTelemetryFile is losing non-standard interaction types because normalizeCursorSample coerces anything outside "click" | "mouseup" | "move" to "move"; update the code so original interaction types are preserved: either stop coercing unknown values in normalizeCursorSample or retain the original value (e.g., rawInteractionType) on the sample and return that in readCursorTelemetryFile (in function readCursorTelemetryFile use sample.rawInteractionType or the un-normalized field instead of the coerced sample.interactionType) so "double-click", "right-click", etc. are passed through to the renderer.
🧹 Nitpick comments (1)
src/components/video-editor/timeline/zoomSuggestionUtils.test.ts (1)
46-61: ⚡ Quick winAdd one integration-ish test for normalized telemetry path
nit: cleaner coverage would run
normalizeCursorTelemetrybeforedetectZoomCandidates, so we catch metadata drop regressions early.Test addition
-import { detectZoomCandidates, detectZoomClickCandidates } from "./zoomSuggestionUtils"; +import { + detectZoomCandidates, + detectZoomClickCandidates, + normalizeCursorTelemetry, +} from "./zoomSuggestionUtils"; @@ describe("detectZoomCandidates", () => { + it("keeps click interactions after normalization", () => { + const raw: CursorTelemetryPoint[] = [ + { timeMs: 100, cx: 0.4, cy: 0.4, interactionType: "click" }, + { timeMs: 700, cx: 0.4, cy: 0.4, interactionType: "move" }, + ]; + const normalized = normalizeCursorTelemetry(raw, 2_000); + const candidates = detectZoomCandidates(normalized); + expect(candidates.some((c) => c.source === "click")).toBe(true); + }); + it("returns click candidates ahead of dwell candidates", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/timeline/zoomSuggestionUtils.test.ts` around lines 46 - 61, The test should feed detectZoomCandidates with telemetry that's been through normalizeCursorTelemetry to catch metadata-drop regressions: modify the test to call normalizeCursorTelemetry(samples) (using the same CursorTelemetryPoint[] format) and pass its output into detectZoomCandidates, keeping the existing assertions (clickIndex/dwellIndex ordering); ensure any required metadata produced by normalizeCursorTelemetry is preserved in the normalized output before calling detectZoomCandidates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.ts`:
- Around line 90-97: detectZoomClickCandidates is filtering by
sample.interactionType but normalizeCursorTelemetry removes that field, so when
TimelineEditor passes normalized CursorTelemetryPoint samples clickSamples is
always empty; fix by preserving the interaction type through normalization or by
changing detectZoomClickCandidates to read the normalized property (e.g.
sample.interaction / sample.eventType / sample.rawInteraction) that
normalizeCursorTelemetry emits. Locate normalizeCursorTelemetry and either add
back interactionType (copy normalized value into interactionType) or update
detectZoomClickCandidates to use CLICK_INTERACTIONS against the normalized
field, and ensure TimelineEditor still passes the normalized samples and
existing tests are updated accordingly.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 510-520: The mapping in readCursorTelemetryFile is losing
non-standard interaction types because normalizeCursorSample coerces anything
outside "click" | "mouseup" | "move" to "move"; update the code so original
interaction types are preserved: either stop coercing unknown values in
normalizeCursorSample or retain the original value (e.g., rawInteractionType) on
the sample and return that in readCursorTelemetryFile (in function
readCursorTelemetryFile use sample.rawInteractionType or the un-normalized field
instead of the coerced sample.interactionType) so "double-click", "right-click",
etc. are passed through to the renderer.
---
Nitpick comments:
In `@src/components/video-editor/timeline/zoomSuggestionUtils.test.ts`:
- Around line 46-61: The test should feed detectZoomCandidates with telemetry
that's been through normalizeCursorTelemetry to catch metadata-drop regressions:
modify the test to call normalizeCursorTelemetry(samples) (using the same
CursorTelemetryPoint[] format) and pass its output into detectZoomCandidates,
keeping the existing assertions (clickIndex/dwellIndex ordering); ensure any
required metadata produced by normalizeCursorTelemetry is preserved in the
normalized output before calling detectZoomCandidates.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 214a2c36-8524-4391-891b-c6f3cf8ebf8b
📒 Files selected for processing (7)
electron/ipc/handlers.tselectron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swiftsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/zoomSuggestionUtils.test.tssrc/components/video-editor/timeline/zoomSuggestionUtils.tssrc/lib/nativeMacRecording.tssrc/native/contracts.ts
Codex flagged that normalizeCursorTelemetry was stripping interactionType in normalizeTelemetrySample, so the click-based detector never saw any clicks in the real UI flow (TimelineEditor always normalizes before detecting). CodeRabbit additionally pointed out that normalizeCursorSample in handlers.ts was coercing anything outside "click" | "mouseup" | "move" down to "move", so "double-click", "right-click", and "middle-click" were being lost upstream before they could ever reach the renderer. - Pass interactionType through normalizeTelemetrySample. - Widen normalizeCursorSample's allow-list to all five click variants. - Widen CursorRecordingSample.interactionType to the same union. - Add a normalize -> detect integration test so a future stripping regression fails fast.
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
Two small quality-of-life improvements that pair well together for the typical "record fullscreen, then add zooms" workflow.
1. Click-based auto-zoom suggestions
The recording layer already captures click events on
CursorRecordingSample.interactionType, but two things were preventing them from reaching the suggestion algorithm:readCursorTelemetryFilewas strippinginteractionTypewhen loading telemetry off disk.detectZoomDwellCandidatesonly considers cursor dwell (pause), not clicks.This PR:
interactionTypethroughreadCursorTelemetryFile(electron/ipc/handlers.ts).detectZoomClickCandidatesinzoomSuggestionUtils.tswith 700ms clustering (so a double/triple click becomes a single candidate).detectZoomCandidatesthat merges click + dwell candidates, click-sourced ones ranked with higherstrengthso they win the dedup race.TimelineEditorswitches the "Suggest Zooms from Cursor" handler to the combined detector.End result: clicking the button on a recording with clear UI clicks now places zooms where the user actually interacted, not just where they happened to pause.
2. Exclude OpenScreen's own windows from full-screen capture (macOS / SCK)
SCContentFilter(display:, excludingWindows: [])was leaving the OpenScreen HUD visible in the recorded output. Added an optionalexcludedApps: [{ bundleIdentifier?, processID? }]field to the helper request and have the renderer passprocess.pidfor display recordings. The SCK helper then filterscontent.windowsbyowningApplication.processID/bundleIdentifierand feeds the result toSCContentFilter.Window-target capture is unchanged.
Test plan
npm test— full suite (156 tests) passes, including 5 new tests inzoomSuggestionUtils.test.ts.tsc --noEmitclean.biome checkclean.npm run build:native:mac+npm run dev: clicked around during a fullscreen recording, then pressed "Suggest Zooms from Cursor" — zooms land on each click cluster. The HUD no longer appears in the recorded output.Notes:
interactionTypein its telemetry, so it should benefit once the samereadCursorTelemetryFilepass-through reaches its loader (it already does — single function for both platforms).excludedApps).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests