feat(editor): implement blur annotation with CSS preview and premium cursor overlay#131
feat(editor): implement blur annotation with CSS preview and premium cursor overlay#131mahdyarief wants to merge 8 commits intowebadderallorg:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Blur Annotation Feature src/components/video-editor/types.ts, src/components/video-editor/AnnotationOverlay.tsx, src/components/video-editor/AnnotationSettingsPanel.tsx, src/components/video-editor/SettingsPanel.tsx, src/components/video-editor/VideoEditor.tsx, src/lib/exporter/annotationRenderer.ts |
Adds blur to AnnotationType and blurIntensity?: number to regions. UI: new blur tab and slider (1–100) wired via onBlurIntensityChange → onAnnotationBlurIntensityChange. Overlay renders blur; exporter adds renderBlur(...) and changes renderAnnotations to return boolean success. |
Cursor Rendering Refactor src/components/video-editor/VideoPlayback.tsx, src/components/video-editor/videoPlayback/cursorRenderer.ts, src/lib/exporter/frameRenderer.ts, src/lib/exporter/frameRenderer.test.ts |
Removes PixiCursorOverlay and sway/motion-blur logic. Introduces canvas cursor layer, SmoothedCursorState, drawCursorOnCanvas drawing per-frame, and ref synchronization for regions. Exporter now draws cursor onto composite canvas; tests/mocks updated accordingly. |
Misc / Build & Tests .gitignore, src/lib/exporter/frameRenderer.test.ts |
Normalizes .gitignore entries and adds local artifact ignores. Updates test mocks to reflect cursor and annotationRenderer signature/behavior changes (async renderAnnotations, SmoothedCursorState, drawCursorOnCanvas). |
Sequence Diagram(s)
mermaid
sequenceDiagram
participant UI as AnnotationSettingsPanel
participant SP as SettingsPanel
participant VE as VideoEditor
participant Renderer as annotationRenderer
participant Export as FrameExporter
UI->>SP: onBlurIntensityChange(id, value)
SP->>VE: onAnnotationBlurIntensityChange(id, value)
VE->>VE: update annotationRegions[id].blurIntensity = value
VE->>Renderer: renderAnnotations(...annotationRegions...)
Renderer->>Renderer: renderBlur(ctx, annotation, x,y,w,h, scale)
Renderer->>Export: draw blurred region onto canvas
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- webadderall/Recordly#122: Implements the same "blur" annotation end-to-end (types, UI, state, overlay, exporter).
- webadderall/Recordly#124: Reverts/removes the blur feature — likely directly conflicting with these changes.
- webadderall/Recordly#38: Also modifies cursor rendering and Pixi overlay behavior — high overlap with the cursor refactor.
Poem
🐇 Soft hops of code across the screen,
I blur the edges where content had been,
Slider nudged, a subtle sheen,
Canvas hums where Pixi had been,
A rabbit cheers for clearer scenes.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the primary changes: implementing a blur annotation feature with CSS preview and moving the cursor to a top-level overlay for premium experience. |
| Description check | ✅ Passed | The PR description covers all required template sections: a clear description of the blur annotation feature, motivation explaining the move from PixiJS to CSS backdrop-filter, correct type of change selection, related issue link, screenshot/video demonstrating the feature, testing guide with specific steps, and completed checklist items. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/AnnotationSettingsPanel.tsx`:
- Around line 518-520: The UI shows annotation.blurIntensity with a "%" suffix
which is misleading because the blur is applied in pixels; update the display in
AnnotationSettingsPanel (the span currently rendering {annotation.blurIntensity
?? 12}%) to show pixels (e.g., "px") or a mapped pixel value instead: either
change the suffix to "px" and ensure the blur style uses
`${annotation.blurIntensity}px`, or introduce a single helper like
mapBlurIntensityToPx(value) and use it both for the CSS blur (backdrop-filter:
blur(...)) and for the displayed label so the units and value are consistent
across AnnotationSettingsPanel and wherever blur is applied.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 361-399: Expand the sampled ImageData by a blur-dependent padding
(e.g., const pad = Math.ceil(intensity * 2)) so the offscreen capture includes
neighboring pixels, but keep the clipping path at the original annotation
bounds; compute paddedSrcX = Math.max(0, srcX - pad), paddedSrcY = Math.max(0,
srcY - pad) and paddedWidth/Height clamped to ctx.canvas dimensions, create the
offscreen with paddedWidth/paddedHeight, put the captured image into the
offscreen, then apply ctx.filter = `blur(${intensity}px)` and draw the offscreen
back onto the main canvas at (paddedSrcX, paddedSrcY) so the blur has room to
bleed while the existing clip (ctx.roundRect/ctx.rect + ctx.clip) still confines
the visible result to the original x,y,width,height; update usages of
srcX/srcY/srcWidth/srcHeight to use the padded variants for capture and the
original x,y,width,height for the clip.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: df66444a-0dd4-4ec5-8c5e-36bdad4e17a4
📒 Files selected for processing (7)
src/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/types.tssrc/lib/exporter/annotationRenderer.ts
There was a problem hiding this comment.
Actionable comments posted: 2
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/VideoPlayback.tsx (1)
730-736:⚠️ Potential issue | 🟡 MinorReplace the empty catch blocks with explicit handling.
Biome is already flagging these blocks, and the two
play()branches also hide real preview/webcam resume failures when sync breaks. A small explicit no-op for pointer capture and at least debug-level handling for rejectedplay()promises would keep the lint pass green and preserve diagnostics.🛠️ Minimal cleanup
- } catch { } + } catch { + // Pointer capture may already be released. + } ... - video.play().catch(() => { }); + video.play().catch((error) => { + console.debug("Preview resume was rejected.", error); + }); ... - playPromise.catch(() => { }); + playPromise.catch((error) => { + console.debug("Webcam resume was rejected.", error); + });Also applies to: 913-917, 985-989
🤖 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 730 - 736, The empty catch blocks must be replaced with explicit handling: in endFocusDrag (and the similar pointer-capture handlers around lines referenced) replace the bare catch with a no-op comment or a small debug log so pointer capture failures are acknowledged (e.g., "pointer capture not available" or a debug-level logger call), and for the two video.play() branches (the play() calls flagged at the other locations) attach .catch handlers that log at debug level the rejection/error (include the video element id or context) instead of swallowing the error; update functions endFocusDrag and the analogous pointer handlers and the code paths that call video.play() so failures are logged for diagnostics.
🧹 Nitpick comments (2)
.gitignore (1)
39-45:docs/is a broad ignore pattern—confirm this scope.Ignoring the entire
docs/directory can unintentionally drop legitimate documentation updates from future commits. Consider narrowing to generated docs paths only if that was the goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 39 - 45, The current broad ignore pattern "docs/" in .gitignore will exclude all documentation; if the intent is to ignore only generated docs, replace that entry with specific generated paths (for example the build output like docs/_build/ or docs/site/), or add negation rules to keep source docs (e.g., keep docs/source/ or README in docs/) so authored docs are tracked; update the .gitignore entry "docs/" to a narrower pattern and add a short comment explaining the intent.src/components/video-editor/VideoPlayback.tsx (1)
1067-1087: Remove the orphaned Pixi cursor path after the canvas migration.Nothing in this file drives
cursorOverlayRefanymore, but the mount path still allocatescursorContainer/PixiCursorOverlayand the follow-up effect still reconfigures it. That keeps two cursor-rendering implementations alive and pays mount-time asset/GPU cost for a renderer this file no longer uses.Also applies to: 1454-1476
🤖 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 1067 - 1087, Remove the unused Pixi cursor mount path: delete the creation and addition of cursorContainer (the Container instance assigned to cursorContainerRef and added via cameraContainer.addChild) and the conditional allocation of PixiCursorOverlay (the PixiCursorOverlay instance assigned to cursorOverlayRef when cursorOverlayEnabled is true); also remove or no-op any follow-up effects that reconfigure cursorOverlayRef so nothing references cursorContainerRef/cursorOverlayRef anymore (repeat the same removal where the alternate mount exists), and ensure any related refs are set to null to avoid retained GPU/asset cost.
🤖 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/VideoPlayback.tsx`:
- Around line 1403-1441: The top-level cursor overlay is being drawn in raw CSS
pixels and using the full canvas viewport, causing drift under zoom/crop and
blurring on HiDPI; update the cursor draw to (1) size the backing store using
devicePixelRatio (set cursorCanvas.width = cursorCanvas.clientWidth *
devicePixelRatio and height similarly) and scale the 2D context by
devicePixelRatio, and (2) compute and pass the same transformed viewport the
preview uses (derive from animationStateRef.current / the visible video rect or
cropRegionRef.current) into drawCursorOnCanvas instead of the full canvas rect
so the cursor is rendered in the preview’s coordinate space; adjust calls using
cursorCanvasRef, smoothedCursorStateRef, cursorTelemetryRef and
drawCursorOnCanvas (also apply same fix at the other location mentioned).
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 377-401: The code currently unsafely casts getContext('2d') to
CanvasRenderingContext2D and uses an any cast on drawImage; fix by guarding the
context and typing the offscreen image correctly: call
configureHighQuality2DContext or otherwise check that offscreen.getContext('2d')
returns non-null and assign to a properly typed variable before calling
putImageData, and remove the `as any` by ensuring the value passed to
ctx.drawImage is a CanvasImageSource (branch on offscreen being an
OffscreenCanvas vs HTMLCanvasElement or cast to CanvasImageSource only after the
runtime type check); keep ctx.filter/clip logic the same but only execute it
after validating ctx is non-null.
---
Outside diff comments:
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 730-736: The empty catch blocks must be replaced with explicit
handling: in endFocusDrag (and the similar pointer-capture handlers around lines
referenced) replace the bare catch with a no-op comment or a small debug log so
pointer capture failures are acknowledged (e.g., "pointer capture not available"
or a debug-level logger call), and for the two video.play() branches (the play()
calls flagged at the other locations) attach .catch handlers that log at debug
level the rejection/error (include the video element id or context) instead of
swallowing the error; update functions endFocusDrag and the analogous pointer
handlers and the code paths that call video.play() so failures are logged for
diagnostics.
---
Nitpick comments:
In @.gitignore:
- Around line 39-45: The current broad ignore pattern "docs/" in .gitignore will
exclude all documentation; if the intent is to ignore only generated docs,
replace that entry with specific generated paths (for example the build output
like docs/_build/ or docs/site/), or add negation rules to keep source docs
(e.g., keep docs/source/ or README in docs/) so authored docs are tracked;
update the .gitignore entry "docs/" to a narrower pattern and add a short
comment explaining the intent.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1067-1087: Remove the unused Pixi cursor mount path: delete the
creation and addition of cursorContainer (the Container instance assigned to
cursorContainerRef and added via cameraContainer.addChild) and the conditional
allocation of PixiCursorOverlay (the PixiCursorOverlay instance assigned to
cursorOverlayRef when cursorOverlayEnabled is true); also remove or no-op any
follow-up effects that reconfigure cursorOverlayRef so nothing references
cursorContainerRef/cursorOverlayRef anymore (repeat the same removal where the
alternate mount exists), and ensure any related refs are set to null to avoid
retained GPU/asset cost.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1c42f199-38df-4505-ae77-d77e63cb5660
⛔ Files ignored due to path filters (3)
electron/native/bin/win32-x64/cursor-monitor.exeis excluded by!**/*.exeelectron/native/bin/win32-x64/wgc-capture.exeis excluded by!**/*.exepackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.gitignoreelectron/native/bin/win32-x64/helpers-manifest.jsonsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/exporter/annotationRenderer.ts
✅ Files skipped from review due to trivial changes (1)
- electron/native/bin/win32-x64/helpers-manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/AnnotationSettingsPanel.tsx
| // Update cursor overlay on separate top-level canvas | ||
| const cursorCanvas = cursorCanvasRef.current; | ||
| const smoothedState = smoothedCursorStateRef.current; | ||
| if (cursorCanvas && smoothedState && cursorTelemetryRef.current && showCursorRef.current) { | ||
| const timeMs = currentTimeRef.current; | ||
| cursorOverlay.update( | ||
| cursorTelemetryRef.current, | ||
| timeMs, | ||
| baseMaskRef.current, | ||
| showCursorRef.current, | ||
| !isPlayingRef.current || isSeekingRef.current, | ||
| ); | ||
| const ctx = cursorCanvas.getContext('2d'); | ||
| if (ctx) { | ||
| // Match canvas size to container | ||
| if (cursorCanvas.width !== cursorCanvas.clientWidth || cursorCanvas.height !== cursorCanvas.clientHeight) { | ||
| cursorCanvas.width = cursorCanvas.clientWidth; | ||
| cursorCanvas.height = cursorCanvas.clientHeight; | ||
| } | ||
|
|
||
| ctx.clearRect(0, 0, cursorCanvas.width, cursorCanvas.height); | ||
|
|
||
| // Draw cursor on top using utility | ||
| drawCursorOnCanvas( | ||
| ctx, | ||
| cursorTelemetryRef.current, | ||
| timeMs, | ||
| { | ||
| x: 0, | ||
| y: 0, | ||
| width: cursorCanvas.width, | ||
| height: cursorCanvas.height, | ||
| sourceCrop: cropRegionRef.current, | ||
| }, | ||
| smoothedState, | ||
| { | ||
| ...DEFAULT_CURSOR_CONFIG, | ||
| style: cursorStyleRef.current, | ||
| dotRadius: 28 * cursorSizeRef.current, // Matches Pixi scaling approx | ||
| smoothingFactor: cursorSmoothingRef.current, | ||
| motionBlur: cursorMotionBlurRef.current, | ||
| clickBounce: cursorClickBounceRef.current, | ||
| clickBounceDuration: cursorClickBounceDurationRef.current, | ||
| sway: cursorSwayRef.current, | ||
| } | ||
| ); |
There was a problem hiding this comment.
Draw the top-level cursor in the same space as the preview.
drawCursorOnCanvas only gets the viewport you pass here (see src/components/video-editor/videoPlayback/cursorRenderer.ts:1242-1249), but this call always uses the full overlay canvas in raw CSS pixels and never mirrors the current preview transform from animationStateRef.current / the visible video rect. That makes the cursor drift as soon as zoom, crop, or padding are active, and the clientWidth/clientHeight backing store will soften it on HiDPI displays. Use the same transformed viewport the preview uses, and size the backing store to client * devicePixelRatio before drawing.
Also applies to: 1819-1825
🤖 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 1403 - 1441, The
top-level cursor overlay is being drawn in raw CSS pixels and using the full
canvas viewport, causing drift under zoom/crop and blurring on HiDPI; update the
cursor draw to (1) size the backing store using devicePixelRatio (set
cursorCanvas.width = cursorCanvas.clientWidth * devicePixelRatio and height
similarly) and scale the 2D context by devicePixelRatio, and (2) compute and
pass the same transformed viewport the preview uses (derive from
animationStateRef.current / the visible video rect or cropRegionRef.current)
into drawCursorOnCanvas instead of the full canvas rect so the cursor is
rendered in the preview’s coordinate space; adjust calls using cursorCanvasRef,
smoothedCursorStateRef, cursorTelemetryRef and drawCursorOnCanvas (also apply
same fix at the other location mentioned).
| let offscreen: HTMLCanvasElement | OffscreenCanvas; | ||
| if (typeof document !== 'undefined') { | ||
| offscreen = document.createElement('canvas'); | ||
| } else { | ||
| offscreen = new OffscreenCanvas(srcWidth, srcHeight); | ||
| } | ||
| offscreen.width = srcWidth; | ||
| offscreen.height = srcHeight; | ||
|
|
||
| const offCtx = offscreen.getContext('2d') as CanvasRenderingContext2D; | ||
| offCtx.putImageData(imageData, 0, 0); | ||
|
|
||
| // Create rounded rect clipping path (matches UI's rounded-lg approx 8px) | ||
| const radius = 8 * scaleFactor; | ||
| ctx.beginPath(); | ||
| if (ctx.roundRect) { | ||
| ctx.roundRect(x, y, width, height, radius); | ||
| } else { | ||
| ctx.rect(x, y, width, height); | ||
| } | ||
| ctx.clip(); | ||
|
|
||
| // Apply the blur filter and draw the captured region back at its source position | ||
| ctx.filter = `blur(${intensity}px)`; | ||
| ctx.drawImage(offscreen as any, srcX, srcY); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/lib/exporter/annotationRenderer.ts | sed -n '370,410p'Repository: webadderall/Recordly
Length of output: 1528
🏁 Script executed:
cat tsconfig.json | head -50Repository: webadderall/Recordly
Length of output: 824
🏁 Script executed:
rg "drawImage" src/lib/exporter/annotationRenderer.ts -A 2 -B 2Repository: webadderall/Recordly
Length of output: 357
🏁 Script executed:
fd -e json "biome\|tsconfig\|eslint" src root -type f 2>/dev/null | head -20Repository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
ls -la | grep -E "biome|eslint|tsconfig"Repository: webadderall/Recordly
Length of output: 333
🏁 Script executed:
cat biome.jsonRepository: webadderall/Recordly
Length of output: 4909
🏁 Script executed:
rg "getContext.*2d" src/lib/exporter/annotationRenderer.ts -B 2 -A 3Repository: webadderall/Recordly
Length of output: 298
🏁 Script executed:
python3 << 'EOF'
# Check what the Canvas drawImage signature expects
# and whether HTMLCanvasElement | OffscreenCanvas is compatible
# The DOM lib defines drawImage as accepting CanvasImageSource
# which is a union of:
# - HTMLCanvasElement
# - HTMLImageElement
# - HTMLVideoElement
# - ImageBitmap
# - OffscreenCanvas
# - VideoFrame (newer)
# So HTMLCanvasElement | OffscreenCanvas should be valid
# Let's check if there's actually a type error by looking at what
# might cause Biome to flag it
print("Checking Canvas.drawImage type signature...")
print("\nExpected overloads for drawImage:")
print("drawImage(image: CanvasImageSource, dx: number, dy: number): void;")
print("drawImage(image: CanvasImageSource, dx: number, dy: number, dw: number, dh: number): void;")
print("\nCanvasImageSource is defined as:")
print("type CanvasImageSource = HTMLCanvasElement | HTMLImageElement | HTMLVideoElement |")
print(" ImageBitmap | OffscreenCanvas | VideoFrame;")
print("\nSo HTMLCanvasElement | OffscreenCanvas should be valid...")
print("\nHowever, the `as any` cast is still being used.")
print("This suggests either:")
print("1. TypeScript is not recognizing the union as compatible")
print("2. There's a version mismatch in the DOM types")
print("3. There's a circular type issue or generic constraint")
EOFRepository: webadderall/Recordly
Length of output: 775
🏁 Script executed:
rg "getContext.*2d" src --type ts --type tsx -B 2 -A 5Repository: webadderall/Recordly
Length of output: 91
🏁 Script executed:
rg "getContext.*2d" src -A 5 -B 2 | head -100Repository: webadderall/Recordly
Length of output: 6923
🏁 Script executed:
rg "configureHighQuality2DContext" src/lib/exporter/ -A 10 -B 2 | head -50Repository: webadderall/Recordly
Length of output: 2975
🏁 Script executed:
cat src/lib/exporter/annotationRenderer.ts | sed -n '375,410p'Repository: webadderall/Recordly
Length of output: 1140
🏁 Script executed:
grep -A 2 "noExplicitAny" biome.jsonRepository: webadderall/Recordly
Length of output: 175
Guard the context and avoid the any cast on drawImage.
Line 386's as CanvasRenderingContext2D unsafely suppresses the null return from getContext('2d'), leaving line 387's putImageData call unguarded. Additionally, line 401's as any violates Biome's noExplicitAny: "error" rule. Use a proper null check (like configureHighQuality2DContext in frameRenderer.ts) and handle the type properly—either branch the drawImage call by canvas type or ensure the union type is correctly typed as a CanvasImageSource.
🧰 Tools
🪛 Biome (2.4.9)
[error] 401-401: Unexpected any. Specify a different type.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/annotationRenderer.ts` around lines 377 - 401, The code
currently unsafely casts getContext('2d') to CanvasRenderingContext2D and uses
an any cast on drawImage; fix by guarding the context and typing the offscreen
image correctly: call configureHighQuality2DContext or otherwise check that
offscreen.getContext('2d') returns non-null and assign to a properly typed
variable before calling putImageData, and remove the `as any` by ensuring the
value passed to ctx.drawImage is a CanvasImageSource (branch on offscreen being
an OffscreenCanvas vs HTMLCanvasElement or cast to CanvasImageSource only after
the runtime type check); keep ctx.filter/clip logic the same but only execute it
after validating ctx is non-null.
|
I'll take a look at this after we get a stable release of Recordly out, you're welcomed to help! |
|
ahh okay, |
|
@webadderall Is Blur Annotation included in the official release yet? |
|
Reviewing |
|
@mahdyarief Amazing! I really needed this blur feature. Thank you for your contribution. |
|
3 issues:
|
83f0474 to
e2da2d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/components/video-editor/VideoPlayback.tsx (1)
1381-1403:⚠️ Potential issue | 🟠 MajorDraw the top-level cursor in the preview’s transformed viewport.
This still renders against the full canvas
{ x: 0, y: 0, width: clientWidth, height: clientHeight }and sizes the backing store in raw CSS pixels. Once padding, crop, or zoom are active, the cursor is no longer in the same space as the video, and on HiDPI it will soften. Use the preview’s transformed rect (baseMaskRef.currentplusanimationStateRef.current) and adevicePixelRatio-scaled backing store before callingdrawCursorOnCanvas.🤖 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 1381 - 1403, The cursor is being drawn against the full canvas and unscaled backing store; compute the preview's transformed rect by combining baseMaskRef.current with animationStateRef.current (apply the same transforms used to render the video) and use that rect instead of {x:0,y:0,width:clientWidth,height:clientHeight}; also set the canvas backing store to devicePixelRatio-scaled dimensions (multiply width/height by window.devicePixelRatio), adjust CSS size to client dimensions, clear and draw using the scaled coords, and pass the transformed rect (plus cropRegionRef.current) to drawCursorOnCanvas while still using cursorTelemetryRef.current and smoothedState so the cursor lines up crisply with the transformed video.src/lib/exporter/annotationRenderer.ts (1)
377-401:⚠️ Potential issue | 🟡 MinorGuard the offscreen context and stop using
anyhere.Line 386 can still return
null, and Line 401 still violates Biome'snoExplicitAnyrule. In that failure path the catch just logs and the blur disappears from export. Use a null guard and pass a real canvas source intodrawImageinstead.Proposed fix
- const offCtx = offscreen.getContext('2d') as CanvasRenderingContext2D; - offCtx.putImageData(imageData, 0, 0); + const offCtx = offscreen.getContext('2d'); + if (!offCtx) { + ctx.restore(); + return; + } + offCtx.putImageData(imageData, 0, 0); @@ - ctx.drawImage(offscreen as any, srcX, srcY); + ctx.drawImage(offscreen, srcX, srcY);#!/bin/bash set -euo pipefail sed -n '377,401p' src/lib/exporter/annotationRenderer.ts rg -nC2 '"noExplicitAny"|noTsIgnore' --iglob 'biome.json' --iglob 'tsconfig.json'Expected result: the current block should still show the
as anycast, and the repo lint config should confirm why that cast is being flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/annotationRenderer.ts` around lines 377 - 401, The offscreen canvas context (offCtx from offscreen.getContext) can be null and drawImage currently uses an any cast; guard the null case and ensure a proper CanvasImageSource is always passed to ctx.drawImage so no any is needed. Specifically, after creating offscreen (symbol: offscreen) and obtaining offCtx via offscreen.getContext('2d'), check if offCtx is null; if so create a fallback HTMLCanvasElement, set its width/height, get a non-null 2D context on it, and put the ImageData (imageData) into that canvas; otherwise use the existing offCtx.putImageData. Then call ctx.drawImage with the fallback or original offscreen (typed as CanvasImageSource) and remove the as any cast; also avoid proceeding with blur if you cannot obtain a drawable canvas.
🤖 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/VideoPlayback.tsx`:
- Around line 1142-1143: smoothedCursorStateRef.current is instantiated with
DEFAULT_CURSOR_CONFIG and never updated, so the preview remains stuck at the
default smoothing and still references cursorMotionBlur/cursorSway which are
no-ops; fix by updating the SmoothedCursorState whenever the live motion
settings change: call the appropriate setter(s) on
smoothedCursorStateRef.current (e.g., setSmoothingFactor) inside useEffect hooks
that depend on cursorSmoothing (and remove or stop threading
cursorMotionBlur/cursorSway into the preview path if the canvas no longer uses
them), ensuring you reference the existing SmoothedCursorState instance rather
than recreating it.
- Around line 733-734: Replace the empty catch blocks with an explicit no-op
handler to satisfy noEmptyBlockStatements; for each occurrence (the try around
event.currentTarget.releasePointerCapture(event.pointerId) and the similar
blocks at the other locations) change the empty catch to include a comment or
minimal statement such as catch (err) { /* no-op: ignore releasePointerCapture
failures */ } or catch { void 0 /* no-op */ } so the catch is non-empty; locate
the blocks by the call to releasePointerCapture and update them accordingly in
the pointer event handlers in VideoPlayback.tsx.
In `@src/lib/exporter/frameRenderer.ts`:
- Line 140: Import MotionBlurFilter from 'pixi-filters/motion-blur' and update
the motionBlurFilter field type from "any | null" to "MotionBlurFilter | null"
in the class, then remove the `@ts-ignore` before the code that references
MotionBlurFilter (ensure usages like where MotionBlurFilter is
instantiated/assigned now reference the imported type). This ensures the file
declares: import MotionBlurFilter from 'pixi-filters/motion-blur'; and the class
field: private motionBlurFilter: MotionBlurFilter | null = null; and eliminates
the `@ts-ignore` on the MotionBlurFilter usage.
- Around line 735-753: The cursor is being drawn into this.compositeCtx before
compositeWithShadows() clears it, so move the drawCursorOnCanvas(...) call (and
its guard) to after compositeWithShadows() and after the blur-annotation pass
(the code in annotationRenderer.ts that samples composite pixels) so the cursor
survives compositing and isn’t re-blurred; update the guard to also check
this.config.showCursor and this.smoothedCursorState && this.compositeCtx, and
keep the same cursor config construction (style/size/smoothing/motionBlur/etc.)
when you relocate drawCursorOnCanvas to run after compositeWithShadows() and the
annotation blur step.
---
Duplicate comments:
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1381-1403: The cursor is being drawn against the full canvas and
unscaled backing store; compute the preview's transformed rect by combining
baseMaskRef.current with animationStateRef.current (apply the same transforms
used to render the video) and use that rect instead of
{x:0,y:0,width:clientWidth,height:clientHeight}; also set the canvas backing
store to devicePixelRatio-scaled dimensions (multiply width/height by
window.devicePixelRatio), adjust CSS size to client dimensions, clear and draw
using the scaled coords, and pass the transformed rect (plus
cropRegionRef.current) to drawCursorOnCanvas while still using
cursorTelemetryRef.current and smoothedState so the cursor lines up crisply with
the transformed video.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 377-401: The offscreen canvas context (offCtx from
offscreen.getContext) can be null and drawImage currently uses an any cast;
guard the null case and ensure a proper CanvasImageSource is always passed to
ctx.drawImage so no any is needed. Specifically, after creating offscreen
(symbol: offscreen) and obtaining offCtx via offscreen.getContext('2d'), check
if offCtx is null; if so create a fallback HTMLCanvasElement, set its
width/height, get a non-null 2D context on it, and put the ImageData (imageData)
into that canvas; otherwise use the existing offCtx.putImageData. Then call
ctx.drawImage with the fallback or original offscreen (typed as
CanvasImageSource) and remove the as any cast; also avoid proceeding with blur
if you cannot obtain a drawable canvas.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d416ba54-baae-448e-947e-3257a51c2c6a
📒 Files selected for processing (11)
.gitignoresrc/components/video-editor/AnnotationOverlay.tsxsrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/frameRenderer.ts
✅ Files skipped from review due to trivial changes (2)
- src/components/video-editor/AnnotationOverlay.tsx
- src/components/video-editor/types.ts
| event.currentTarget.releasePointerCapture(event.pointerId); | ||
| } catch {} | ||
| } catch { } |
There was a problem hiding this comment.
Replace the empty catches with explicit no-ops.
Biome already flags these blocks as noEmptyBlockStatements, so this will fail lint as written. If these failures are intentionally ignored, make that explicit with a non-empty handler.
Example cleanup
- } catch { }
+ } catch (error) {
+ void error;
+ }
- video.play().catch(() => { });
+ video.play().catch(() => undefined);
- playPromise.catch(() => { });
+ playPromise.catch(() => undefined);Also applies to: 914-915, 985-987
🧰 Tools
🪛 Biome (2.4.9)
[error] 734-734: Unexpected empty block.
(lint/suspicious/noEmptyBlockStatements)
🤖 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 733 - 734,
Replace the empty catch blocks with an explicit no-op handler to satisfy
noEmptyBlockStatements; for each occurrence (the try around
event.currentTarget.releasePointerCapture(event.pointerId) and the similar
blocks at the other locations) change the empty catch to include a comment or
minimal statement such as catch (err) { /* no-op: ignore releasePointerCapture
failures */ } or catch { void 0 /* no-op */ } so the catch is non-empty; locate
the blocks by the call to releasePointerCapture and update them accordingly in
the pointer event handlers in VideoPlayback.tsx.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/exporter/annotationRenderer.ts (1)
377-387:⚠️ Potential issue | 🟡 MinorType the offscreen canvas path instead of casting it away.
Line 386 suppresses a nullable
getContext('2d'), and Line 403'sas anywill trip Biome'snoExplicitAnyrule.offscreenis alreadyHTMLCanvasElement | OffscreenCanvas, so a null-check plusctx.drawImage(offscreen, ...)or aCanvasImageSourcealias should be enough here.Also applies to: 403-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/annotationRenderer.ts` around lines 377 - 387, The code assigns offscreen as HTMLCanvasElement | OffscreenCanvas then force-casts getContext and later uses an any cast; instead perform proper null checks and type the path: after creating offscreen (variable offscreen) check whether it's an HTMLCanvasElement to call getContext('2d') and handle a possible null return (e.g., guard if offCtx is null and throw or fallback), then use a CanvasImageSource-typed value when calling ctx.drawImage(offscreen, ...) to avoid any casts; update references to offCtx, getContext and the drawImage call in annotationRenderer.ts to remove the explicit casts and satisfy noExplicitAny.
🤖 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/VideoPlayback.tsx`:
- Around line 1068-1070: Remove the duplicate state update: in the VideoPlayback
component where setPixiReady(true) is called twice (both occurrences shown in
the diff), keep a single setPixiReady(true) call and delete the redundant one so
the Pixi-ready state is only set once.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 399-402: The export rendering currently sets ctx.filter =
`blur(${intensity}px) brightness(1.02)` (and applies an edge stroke elsewhere),
which makes exported annotations brighter and outlined compared to the editor
preview that only uses blur; update the export code in annotationRenderer.ts
(both places where ctx.filter is set around the blur application) to match the
preview by using only `blur(${intensity}px)` and remove any additional
brightness or edge-stroke drawing for the exported path so the exported output
is visually identical to the AnnotationOverlay preview (see the blur application
sites in annotationRenderer.ts and the editor's AnnotationOverlay blur usage).
- Around line 369-371: The blur-render path in annotationRenderer.ts currently
logs a warning on failure (around the srcWidth/srcHeight check and the
subsequent blur-render block) but continues the export, risking unblurred
sensitive regions; update the blur-render error handling (the block referencing
srcWidth, srcHeight, and ctx.restore()) so that after detecting a failed blur
(e.g., srcWidth <= 0 || srcHeight <= 0 or other render error) you restore the
canvas state and abort the export flow—either by returning a failure value from
the enclosing function (e.g., return false) or throwing an error—so callers of
the renderer (the export frame function) can stop and not produce a completed
exported frame without the blur applied. Ensure the change is applied to the
same failure checks and the later warning branch referenced in the review (the
lines that currently only warn) so both paths restore ctx and exit consistently.
---
Duplicate comments:
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 377-387: The code assigns offscreen as HTMLCanvasElement |
OffscreenCanvas then force-casts getContext and later uses an any cast; instead
perform proper null checks and type the path: after creating offscreen (variable
offscreen) check whether it's an HTMLCanvasElement to call getContext('2d') and
handle a possible null return (e.g., guard if offCtx is null and throw or
fallback), then use a CanvasImageSource-typed value when calling
ctx.drawImage(offscreen, ...) to avoid any casts; update references to offCtx,
getContext and the drawImage call in annotationRenderer.ts to remove the
explicit casts and satisfy noExplicitAny.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 42881673-e227-4a9b-9b08-a50e72542ad2
📒 Files selected for processing (3)
src/components/video-editor/VideoPlayback.tsxsrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/frameRenderer.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/lib/exporter/annotationRenderer.ts (1)
394-408:⚠️ Potential issue | 🟡 MinorGuard the offscreen context and remove the
as anycast.Line 394 unsafely casts
getContext('2d')toCanvasRenderingContext2Dwithout a null check—if the context fails to create,putImageDataon line 395 will throw. Line 408'sas anycast violates Biome'snoExplicitAnyrule.Proposed fix
- const offCtx = (offscreen as HTMLCanvasElement).getContext('2d', { alpha: false }) as CanvasRenderingContext2D; + const offCtx = (offscreen as HTMLCanvasElement).getContext('2d', { alpha: false }); + if (!offCtx) { + console.warn('[AnnotationRenderer] Failed to get offscreen 2D context for blur'); + ctx.restore(); + return false; + } offCtx.putImageData(imageData, 0, 0);For line 408, cast to
CanvasImageSourceinstead ofany:- ctx.drawImage(offscreen as any, srcX, srcY); + ctx.drawImage(offscreen as CanvasImageSource, srcX, srcY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/annotationRenderer.ts` around lines 394 - 408, The code unsafely assumes the offscreen canvas context exists and uses an `as any` cast for drawImage; guard the result of (offscreen as HTMLCanvasElement).getContext('2d') by checking offCtx !== null before calling offCtx.putImageData (return or handle error if null) and remove the `as any` cast on the drawImage call by casting the offscreen to CanvasImageSource (or ensuring its type is compatible) so ctx.drawImage(offscreen as CanvasImageSource, srcX, srcY) is used and noExplicitAny is satisfied; update references in the function where offCtx, offscreen, getContext, putImageData, and ctx.drawImage are used.src/components/video-editor/VideoPlayback.tsx (2)
734-734:⚠️ Potential issue | 🟡 MinorReplace empty catch block with explicit no-op.
Biome flags this as
noEmptyBlockStatements. Make the intentional ignore explicit.Proposed fix
- } catch { } + } catch { + // Pointer capture may already be released + }Apply similar fixes at lines 919 and 991.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoPlayback.tsx` at line 734, The empty catch block in VideoPlayback.tsx should be made explicit to satisfy the noEmptyBlockStatements rule: replace the bare "catch { }" with an explicit no-op such as "catch (err) { /* intentionally ignored */ }" or "catch (_) { /* no-op */ }" so the intent is clear; update the same pattern in the other occurrences noted around lines where VideoPlayback functions or blocks use "catch { }" (also apply the same explicit no-op fix at the other reported locations).
1378-1416:⚠️ Potential issue | 🟠 MajorCursor canvas sizing doesn't account for HiDPI displays.
The canvas backing store is set to
clientWidth/clientHeightwithout multiplying bydevicePixelRatio. On HiDPI displays, this will make the cursor appear blurry. Additionally, the viewport passed todrawCursorOnCanvasuses the full canvas dimensions rather than the transformed preview rect, which may cause cursor drift when zoom/crop/padding are active.Proposed fix for HiDPI
- if (cursorCanvas.width !== cursorCanvas.clientWidth || cursorCanvas.height !== cursorCanvas.clientHeight) { - cursorCanvas.width = cursorCanvas.clientWidth; - cursorCanvas.height = cursorCanvas.clientHeight; - } + const dpr = window.devicePixelRatio || 1; + const targetWidth = Math.floor(cursorCanvas.clientWidth * dpr); + const targetHeight = Math.floor(cursorCanvas.clientHeight * dpr); + if (cursorCanvas.width !== targetWidth || cursorCanvas.height !== targetHeight) { + cursorCanvas.width = targetWidth; + cursorCanvas.height = targetHeight; + ctx.scale(dpr, dpr); + }For the viewport, consider deriving it from
baseMaskRef.currentto match the preview's coordinate space.🤖 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 1378 - 1416, The canvas is being sized to clientWidth/clientHeight which omits devicePixelRatio and the viewport passed to drawCursorOnCanvas uses full canvas dims instead of the preview's transformed rect; fix by sizing the backing store to clientWidth * devicePixelRatio and clientHeight * devicePixelRatio, then set the CSS size to clientWidth/clientHeight and call ctx.scale(devicePixelRatio, devicePixelRatio) so drawing stays sharp (adjust any pixel-sized options like dotRadius as needed to account for DPR), and derive the viewport passed into drawCursorOnCanvas from baseMaskRef.current (or the preview/crop transform) converted into canvas coordinate space so cursor coordinates align with zoom/crop/padding rather than using the raw canvas width/height.
🧹 Nitpick comments (1)
src/lib/exporter/frameRenderer.test.ts (1)
173-173: Consider typing the mock getContext signature more specifically.While acceptable for test code, the
any[]triggers Biome'snoExplicitAny. You could useunknown[]or the actual overload types.Optional fix
- getContext: vi.fn((..._args: any[]) => context), + getContext: vi.fn((..._args: unknown[]) => context),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/frameRenderer.test.ts` at line 173, The mock for getContext currently uses vi.fn((..._args: any[]) => context) which triggers noExplicitAny; update the mock signature to avoid any by using unknown[] or the actual overload parameter types (e.g., vi.fn((..._args: unknown[]) => context) or the precise parameter types matching getContext) so TypeScript/Biome linting passes while preserving behavior; update the vi.fn call for getContext accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/video-editor/VideoPlayback.tsx`:
- Line 734: The empty catch block in VideoPlayback.tsx should be made explicit
to satisfy the noEmptyBlockStatements rule: replace the bare "catch { }" with an
explicit no-op such as "catch (err) { /* intentionally ignored */ }" or "catch
(_) { /* no-op */ }" so the intent is clear; update the same pattern in the
other occurrences noted around lines where VideoPlayback functions or blocks use
"catch { }" (also apply the same explicit no-op fix at the other reported
locations).
- Around line 1378-1416: The canvas is being sized to clientWidth/clientHeight
which omits devicePixelRatio and the viewport passed to drawCursorOnCanvas uses
full canvas dims instead of the preview's transformed rect; fix by sizing the
backing store to clientWidth * devicePixelRatio and clientHeight *
devicePixelRatio, then set the CSS size to clientWidth/clientHeight and call
ctx.scale(devicePixelRatio, devicePixelRatio) so drawing stays sharp (adjust any
pixel-sized options like dotRadius as needed to account for DPR), and derive the
viewport passed into drawCursorOnCanvas from baseMaskRef.current (or the
preview/crop transform) converted into canvas coordinate space so cursor
coordinates align with zoom/crop/padding rather than using the raw canvas
width/height.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 394-408: The code unsafely assumes the offscreen canvas context
exists and uses an `as any` cast for drawImage; guard the result of (offscreen
as HTMLCanvasElement).getContext('2d') by checking offCtx !== null before
calling offCtx.putImageData (return or handle error if null) and remove the `as
any` cast on the drawImage call by casting the offscreen to CanvasImageSource
(or ensuring its type is compatible) so ctx.drawImage(offscreen as
CanvasImageSource, srcX, srcY) is used and noExplicitAny is satisfied; update
references in the function where offCtx, offscreen, getContext, putImageData,
and ctx.drawImage are used.
---
Nitpick comments:
In `@src/lib/exporter/frameRenderer.test.ts`:
- Line 173: The mock for getContext currently uses vi.fn((..._args: any[]) =>
context) which triggers noExplicitAny; update the mock signature to avoid any by
using unknown[] or the actual overload parameter types (e.g., vi.fn((..._args:
unknown[]) => context) or the precise parameter types matching getContext) so
TypeScript/Biome linting passes while preserving behavior; update the vi.fn call
for getContext accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a8d0b0a9-fab1-44cb-82ff-05bb25e1b0c0
📒 Files selected for processing (4)
src/components/video-editor/VideoPlayback.tsxsrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/frameRenderer.ts
|
Seems to still have a lot of issues, please review your code carefully before making a PR as it uses a lot of time that could be spent working on other bugs The branch still isn’t mergeable in its current form:
|
…obust canvas export pipeline
… sits above annotations
… native whisper binaries
… whisper native binaries
…hronization and add necessary whisper binaries
…ns and cursor features
41d7e6d to
f1d5a27
Compare
|
Still not correctly rebased to main and many regressions still introduced, please review your PRs |
Description
This PR implements a high-performance Blur Annotation feature with a professional-grade implementation that resolves the coordinate 'gap' issue seen in earlier iterations.
Motivation
Previously, the blur annotation relied on a complex PixiJS-based localized filter that was difficult to synchronize with the editor's UI overlay, leading to visual misalignments (gaps). This change moves the preview to a hardware-accelerated CSS \�ackdrop-filter\ and moves the cursor to a top-level layer for maximum clarity and a 'premium' experience.
Type of Change
Related Issue(s)
https://github.com/webadderall/Recordly/issues/127
Screenshots / Video
The editor now features a perfectly aligned blur preview and a high-performance cursor overlay that sits on top of all annotations.

Testing Guide
Checklist
Summary by CodeRabbit
New Features
Improvements