feat: add cursor highlight overlay to video editor and export pipeline#240
feat: add cursor highlight overlay to video editor and export pipeline#240loookashow wants to merge 1 commit intosiddharthvaddem:mainfrom
Conversation
Render cursor position overlay on recorded video using existing 10Hz cursor telemetry data. Four highlight styles: dot, circle, ring, and glow (soft radial gradient). Settings include color, size, opacity, and stroke width with live PixiJS preview and canvas-based export. Key implementation details: - Cursor telemetry remapped from display bounds to workArea coords using display metadata saved in cursor.json - Preview renders via PixiJS Graphics/Sprite in cameraContainer - Export renders via 2D canvas after annotations with rounded clip mask - Single "Cursor Highlight" settings panel with 4 styles - Full persistence, undo/redo, and dirty detection support
📝 WalkthroughWalkthroughThis PR adds a complete cursor highlight overlay feature to the video editor, enabling users to capture, store, and render cursor position data during playback and export. The implementation includes display metadata capture in the Electron layer, UI controls for cursor styling and visibility, frame rendering logic with position interpolation, and support for exporting cursor overlays in GIF and MP4 formats. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Recording)
participant Electron as Electron IPC
participant FS as File System
participant App as React App
User->>Electron: Start recording
Electron->>Electron: Clear captureDisplayInfo
Note over Electron: First cursor sample arrives
Electron->>Electron: Capture display bounds & work area into captureDisplayInfo
loop Subsequent samples
Electron->>Electron: Reuse captureDisplayInfo for cursor data
end
User->>Electron: Stop recording
Electron->>FS: Write cursor.json with samples + display metadata
Electron->>Electron: Reset captureDisplayInfo to null
App->>Electron: getCursorTelemetry()
Electron->>FS: Read cursor.json
Electron->>App: Return samples + display metadata
App->>App: Store in cursorDisplayInfo state
sequenceDiagram
participant Player as Video Playback
participant Renderer as Frame Renderer
participant Canvas as Canvas/PixiJS
rect rgba(100, 150, 200, 0.5)
Note over Player,Renderer: Playback Rendering
Player->>Player: currentTime updates
Player->>Renderer: updateCursorGraphics(currentTime)
Renderer->>Renderer: interpolateCursorPosition(samples, currentTime)
Renderer->>Renderer: Remap via cursorDisplayInfo (bounds → work area)
Renderer->>Renderer: Check cropRegion culling
Renderer->>Renderer: Transform to video canvas coordinates
Renderer->>Canvas: Draw cursor shape (dot/circle/ring/glow)
Canvas->>Player: Display updated cursor overlay
end
rect rgba(100, 200, 150, 0.5)
Note over Player,Renderer: Export Rendering
Player->>Renderer: FrameRenderer with cursor config
Renderer->>Renderer: Interpolate & render each frame
Renderer->>Renderer: Apply scale/zoom transforms
Renderer->>Renderer: Clip to masked video region
Renderer->>Canvas: Composite cursor into frame
Canvas->>Player: Return rendered frame
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![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
openscreen/electron/ipc/handlers.ts
Lines 213 to 217 in c4e38ae
Record cursor positions relative to window captures
This path always derives cursor telemetry from the containing display (display.bounds immediately below), but the source picker still exposes types: ["screen", "window"] in src/components/launch/SourceSelector.tsx and useScreenRecorder.ts records whichever selectedSource.id was chosen. For a window recording, the video is window-bounded while these samples are monitor-bounded, so the new highlight is shifted or completely off-frame for every window capture. We need the captured window rect here, or the feature should be disabled for window sources.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const absX = cx * di.boundsWidth + di.boundsX; | ||
| const absY = cy * di.boundsHeight + di.boundsY; | ||
| const waW = Math.max(1, di.workAreaWidth); | ||
| const waH = Math.max(1, di.workAreaHeight); | ||
| cx = (absX - di.workAreaX) / waW; |
There was a problem hiding this comment.
The recorder already stores cx/cy normalized to display.bounds in sampleCursorPoint(), so converting them a second time into workArea coordinates here changes every point on displays with a dock/taskbar. Since the capture path in src/hooks/useScreenRecorder.ts records the raw selectedSource.id surface without cropping out that reserved strip, normal screen recordings will show the cursor highlight visibly offset from the real pointer, and it disappears entirely when the pointer moves onto the taskbar/menu-bar area.
Useful? React with 👍 / 👎.
| // Render cursor overlay on top of everything | ||
| if (this.config.showCursorHighlight && this.compositeCtx) { | ||
| this.renderCursor(this.compositeCtx, timeMs); |
There was a problem hiding this comment.
In the preview, the webcam <video> and annotation overlay are stacked above the Pixi canvas in src/components/video-editor/VideoPlayback.tsx, so the cursor highlight disappears behind them. Export reverses that order by calling renderCursor() after compositeWithShadows() and renderAnnotations(), which means any cursor path that passes under the webcam bubble or a text/image/figure annotation will export on top of it instead of matching what the editor shows.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/video-editor/projectPersistence.test.ts (1)
66-111: Add normalization tests forcursorOpacityandcursorStrokeWidth.The new suite is strong, but it currently doesn’t assert invalid/out-of-range handling for opacity/stroke width, which are also persisted cursor fields.
➕ Suggested test additions
describe("normalizeProjectEditor cursor fields", () => { + it("clamps/falls back cursor opacity", () => { + expect(normalizeProjectEditor({ cursorOpacity: -1 }).cursorOpacity).toBe(0.6); + expect(normalizeProjectEditor({ cursorOpacity: 2 }).cursorOpacity).toBe(0.6); + expect(normalizeProjectEditor({ cursorOpacity: 0.4 }).cursorOpacity).toBe(0.4); + }); + + it("clamps/falls back cursor stroke width", () => { + expect(normalizeProjectEditor({ cursorStrokeWidth: 0 }).cursorStrokeWidth).toBe(2); + expect(normalizeProjectEditor({ cursorStrokeWidth: 999 }).cursorStrokeWidth).toBe(2); + expect(normalizeProjectEditor({ cursorStrokeWidth: 4 }).cursorStrokeWidth).toBe(4); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.test.ts` around lines 66 - 111, Add tests for cursorOpacity and cursorStrokeWidth in the normalizeProjectEditor suite: assert the default values (expect normalizeProjectEditor({}).cursorOpacity toBe 0.6 and cursorStrokeWidth toBe 2), assert pass-through for valid numeric inputs (e.g., cursorOpacity: 0.8 and cursorStrokeWidth: 3), and add tests for invalid/out-of-range values (e.g., non-numeric strings and values outside allowed ranges) to confirm they fall back or clamp to the expected defaults/limits; use normalizeProjectEditor as in the other tests and mirror the patterns used for cursorSize and cursorColor assertions.src/lib/exporter/gifExporter.ts (1)
55-64: ExtractcursorDisplayInfointo a shared exported type.This inline shape is repeated across declarations/exporters; centralizing it will reduce type drift risk.
♻️ Suggested refactor
-import type { - AnnotationRegion, - CropRegion, - CursorStyle, - CursorTelemetryPoint, - SpeedRegion, - TrimRegion, - ZoomRegion, -} from "@/components/video-editor/types"; +import type { + AnnotationRegion, + CropRegion, + CursorStyle, + CursorTelemetryPoint, + CursorDisplayInfo, + SpeedRegion, + TrimRegion, + ZoomRegion, +} from "@/components/video-editor/types"; @@ - cursorDisplayInfo?: { - boundsX: number; - boundsY: number; - boundsWidth: number; - boundsHeight: number; - workAreaX: number; - workAreaY: number; - workAreaWidth: number; - workAreaHeight: number; - } | null; + cursorDisplayInfo?: CursorDisplayInfo | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.ts` around lines 55 - 64, The inline shape used for cursorDisplayInfo should be extracted to a single exported type (e.g., CursorDisplayInfo) and reused across exporters to avoid duplication: create and export an interface/type named CursorDisplayInfo with the fields boundsX, boundsY, boundsWidth, boundsHeight, workAreaX, workAreaY, workAreaWidth, workAreaHeight, replace the inline union (cursorDisplayInfo?: { ... } | null) in gifExporter.ts with cursorDisplayInfo?: CursorDisplayInfo | null, and update any other exporter/declaration files that repeat the shape to import and use CursorDisplayInfo from the shared module.src/components/video-editor/projectPersistence.ts (1)
328-329: Consider hoisting constants outside the function.
HEX_COLOR_REandVALID_CURSOR_STYLESare recreated on every call tonormalizeProjectEditor. Moving them to module scope would be a minor optimization.Suggested refactor
+const HEX_COLOR_RE = /^#[0-9a-fA-F]{6}$/; +const VALID_CURSOR_STYLES = new Set<CursorStyle>(["dot", "circle", "ring", "glow"]); + export function normalizeProjectEditor(editor: Partial<ProjectEditorState>): ProjectEditorState { const validAspectRatios = new Set<AspectRatio>(ASPECT_RATIOS); // ... existing code ... - const HEX_COLOR_RE = /^#[0-9a-fA-F]{6}$/; - const VALID_CURSOR_STYLES = new Set<CursorStyle>(["dot", "circle", "ring", "glow"]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 328 - 329, Move the constants HEX_COLOR_RE and VALID_CURSOR_STYLES out of the normalizeProjectEditor function into module scope so they are created once rather than on every call; update any references inside normalizeProjectEditor to use the hoisted constants and ensure VALID_CURSOR_STYLES remains a Set<CursorStyle> initialized with ["dot","circle","ring","glow"] and HEX_COLOR_RE keeps the /^#[0-9a-fA-F]{6}$/ pattern.src/components/video-editor/VideoPlayback.tsx (2)
1221-1222: Variable shadowing may cause confusion.The variables
cxandcyat lines 1221-1222 shadow the outer-scopecxandcy(lines 1130-1131 and modified at 1172-1173). While this works correctly because they're only used within the canvas drawing context, it reduces readability.Suggested fix
if (ctx) { - const cx = texSize / 2; - const cy = texSize / 2; + const centerX = texSize / 2; + const centerY = texSize / 2; const r = texSize / 2; - const gradient = ctx.createRadialGradient(cx, cy, 0, cx, cy, r); + const gradient = ctx.createRadialGradient(centerX, centerY, 0, centerX, centerY, r);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoPlayback.tsx` around lines 1221 - 1222, Inner-scope variables cx and cy inside the canvas drawing block shadow outer-scope cx/cy in the VideoPlayback component; rename the inner ones (e.g., texCx/texCy or innerCx/innerCy) and update all uses within that canvas/draw block (where texSize is used) to avoid shadowing and improve readability while leaving the outer cx/cy untouched.
1132-1174: Consider extracting shared cursor interpolation logic.The cursor position interpolation (lines 1132-1160) and display remapping (lines 1163-1174) are duplicated in
frameRenderer.ts. While the rendering contexts differ (PixiJS vs Canvas 2D), the coordinate calculation logic is identical and could be extracted to a shared utility.This is a good-to-have improvement for maintainability; both implementations are currently correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoPlayback.tsx` around lines 1132 - 1174, Extract the duplicated cursor interpolation and display-remapping logic into a shared utility function (e.g., getCursorPositionAtTime or computeCursorPosition) that accepts (samples, timeMs, cursorDisplayInfo) and returns {cx, cy}; replace the block in VideoPlayback.tsx (the interpolation loop and the bounds->video remap using cursorDisplayInfo) and the equivalent block in frameRenderer.ts to call this utility so both PixiJS and Canvas 2D renderers reuse the exact same coordinate-calculation logic.src/lib/exporter/frameRenderer.ts (1)
756-759: Minor: Duplicate variable declarations.
previewWandpreviewH(lines 756-757) duplicatepreviewWidthandpreviewHeight(lines 740-741). Consider reusing the existing variables.Suggested fix
const scaleX = this.config.width / previewWidth; const scaleY = this.config.height / previewHeight; const canvasScaleFactor = (scaleX + scaleY) / 2; // ... - const previewW = this.config.previewWidth || 1920; - const previewH = this.config.previewHeight || 1080; - const canvasScale = Math.min(this.config.width / previewW, this.config.height / previewH); + const canvasScale = Math.min(this.config.width / previewWidth, this.config.height / previewHeight);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/frameRenderer.ts` around lines 756 - 759, The code defines duplicate local variables previewW/previewH while previewWidth/previewHeight already exist; remove previewW and previewH and update uses to reference previewWidth and previewHeight (e.g., in the canvasScale calculation and scaledBorderRadius computation) so canvasScale = Math.min(this.config.width / previewWidth, this.config.height / previewHeight) and scaledBorderRadius uses the existing preview dimensions and zoomScale; adjust anywhere in the frame rendering logic in frameRenderer.ts that currently uses previewW/previewH to use previewWidth/previewHeight instead.
🤖 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/projectPersistence.ts`:
- Line 370: The persisted default for cursorSize in projectPersistence.ts is
inconsistent with the UI defaults: change the fallback value used in the
persistence export (the expression using isFiniteNumber(editor.cursorSize) ?
clamp(editor.cursorSize, 16, 64) : 53) to match the UI default of 32 so projects
without a saved cursorSize use 32; keep the clamp bounds and existing
isFiniteNumber check intact and ensure the symbol names cursorSize,
isFiniteNumber, and clamp are the ones adjusted to avoid the mismatch with
SettingsPanel.tsx and VideoPlayback.tsx.
---
Nitpick comments:
In `@src/components/video-editor/projectPersistence.test.ts`:
- Around line 66-111: Add tests for cursorOpacity and cursorStrokeWidth in the
normalizeProjectEditor suite: assert the default values (expect
normalizeProjectEditor({}).cursorOpacity toBe 0.6 and cursorStrokeWidth toBe 2),
assert pass-through for valid numeric inputs (e.g., cursorOpacity: 0.8 and
cursorStrokeWidth: 3), and add tests for invalid/out-of-range values (e.g.,
non-numeric strings and values outside allowed ranges) to confirm they fall back
or clamp to the expected defaults/limits; use normalizeProjectEditor as in the
other tests and mirror the patterns used for cursorSize and cursorColor
assertions.
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 328-329: Move the constants HEX_COLOR_RE and VALID_CURSOR_STYLES
out of the normalizeProjectEditor function into module scope so they are created
once rather than on every call; update any references inside
normalizeProjectEditor to use the hoisted constants and ensure
VALID_CURSOR_STYLES remains a Set<CursorStyle> initialized with
["dot","circle","ring","glow"] and HEX_COLOR_RE keeps the /^#[0-9a-fA-F]{6}$/
pattern.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1221-1222: Inner-scope variables cx and cy inside the canvas
drawing block shadow outer-scope cx/cy in the VideoPlayback component; rename
the inner ones (e.g., texCx/texCy or innerCx/innerCy) and update all uses within
that canvas/draw block (where texSize is used) to avoid shadowing and improve
readability while leaving the outer cx/cy untouched.
- Around line 1132-1174: Extract the duplicated cursor interpolation and
display-remapping logic into a shared utility function (e.g.,
getCursorPositionAtTime or computeCursorPosition) that accepts (samples, timeMs,
cursorDisplayInfo) and returns {cx, cy}; replace the block in VideoPlayback.tsx
(the interpolation loop and the bounds->video remap using cursorDisplayInfo) and
the equivalent block in frameRenderer.ts to call this utility so both PixiJS and
Canvas 2D renderers reuse the exact same coordinate-calculation logic.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 756-759: The code defines duplicate local variables
previewW/previewH while previewWidth/previewHeight already exist; remove
previewW and previewH and update uses to reference previewWidth and
previewHeight (e.g., in the canvasScale calculation and scaledBorderRadius
computation) so canvasScale = Math.min(this.config.width / previewWidth,
this.config.height / previewHeight) and scaledBorderRadius uses the existing
preview dimensions and zoomScale; adjust anywhere in the frame rendering logic
in frameRenderer.ts that currently uses previewW/previewH to use
previewWidth/previewHeight instead.
In `@src/lib/exporter/gifExporter.ts`:
- Around line 55-64: The inline shape used for cursorDisplayInfo should be
extracted to a single exported type (e.g., CursorDisplayInfo) and reused across
exporters to avoid duplication: create and export an interface/type named
CursorDisplayInfo with the fields boundsX, boundsY, boundsWidth, boundsHeight,
workAreaX, workAreaY, workAreaWidth, workAreaHeight, replace the inline union
(cursorDisplayInfo?: { ... } | null) in gifExporter.ts with cursorDisplayInfo?:
CursorDisplayInfo | null, and update any other exporter/declaration files that
repeat the shape to import and use CursorDisplayInfo from the shared module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8467811e-9815-417d-bdbf-8536013dccb1
📒 Files selected for processing (14)
electron/electron-env.d.tselectron/ipc/handlers.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/hooks/useEditorHistory.tssrc/lib/exporter/cursorUtils.test.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.tssrc/vite-env.d.ts
| typeof editor.cursorColor === "string" && HEX_COLOR_RE.test(editor.cursorColor) | ||
| ? editor.cursorColor | ||
| : "#ffcc00", | ||
| cursorSize: isFiniteNumber(editor.cursorSize) ? clamp(editor.cursorSize, 16, 64) : 53, |
There was a problem hiding this comment.
Default cursorSize mismatch between persistence and UI.
The default value here is 53, but SettingsPanel.tsx (line 230) and VideoPlayback.tsx (line 156) both default to 32. This inconsistency means projects without a saved cursorSize will use 53, while new UI defaults show 32.
Suggested fix
- cursorSize: isFiniteNumber(editor.cursorSize) ? clamp(editor.cursorSize, 16, 64) : 53,
+ cursorSize: isFiniteNumber(editor.cursorSize) ? clamp(editor.cursorSize, 16, 64) : 32,📝 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.
| cursorSize: isFiniteNumber(editor.cursorSize) ? clamp(editor.cursorSize, 16, 64) : 53, | |
| cursorSize: isFiniteNumber(editor.cursorSize) ? clamp(editor.cursorSize, 16, 64) : 32, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/projectPersistence.ts` at line 370, The persisted
default for cursorSize in projectPersistence.ts is inconsistent with the UI
defaults: change the fallback value used in the persistence export (the
expression using isFiniteNumber(editor.cursorSize) ? clamp(editor.cursorSize,
16, 64) : 53) to match the UI default of 32 so projects without a saved
cursorSize use 32; keep the clamp bounds and existing isFiniteNumber check
intact and ensure the symbol names cursorSize, isFiniteNumber, and clamp are the
ones adjusted to avoid the mismatch with SettingsPanel.tsx and
VideoPlayback.tsx.
|
Please add photos or/ and videos in your PR. |
2 participants
Summary
Test plan
npx tsc --noEmit— 0 errorsnpx vitest run— 27 tests pass (includes interpolation, hex color, persistence normalization)npm run lint— cleanSummary by CodeRabbit
Release Notes