Conversation
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Types src/components/video-editor/types.ts |
Added AnnotationType "blur" and optional blurIntensity?: number on AnnotationRegion. |
Overlay src/components/video-editor/AnnotationOverlay.tsx |
Added case "blur" to render a full-size overlay <div> using backdropFilter/WebkitBackdropFilter with annotation.blurIntensity ?? 12; import ordering/formatting adjusted. |
Settings UI src/components/video-editor/AnnotationSettingsPanel.tsx |
Added "blur" tab and TabsContent with a Slider (1–100) showing/setting annotation.blurIntensity (default 12); new prop onBlurIntensityChange?: (intensity:number)=>void. |
Panel Prop Forwarding src/components/video-editor/SettingsPanel.tsx |
Added prop onAnnotationBlurIntensityChange?: (id:string,intensity:number)=>void; forwards blur updates to AnnotationSettingsPanel and uses optional chaining for other annotation callbacks. Also added play() error handling for hover. |
Editor State & Handlers src/components/video-editor/VideoEditor.tsx |
Initialize blur annotations with content = "Blur Region" and default blurIntensity = 12 if undefined; added handleAnnotationBlurIntensityChange and forwarded to SettingsPanel; formatting/import tweaks. |
Playback / Sync / Logging src/components/video-editor/VideoPlayback.tsx |
Synced annotationRegionsRef, safer PIXI container null-checks, changed ignored play() rejections to console.warn, and minor formatting edits. |
Exporter / Rendering src/lib/exporter/annotationRenderer.ts |
Added renderBlur(...) and renderAnnotations branch for "blur": captures intersecting pixels into offscreen canvas, applies ctx.filter = "blur(...px)", clips to rounded rect (fallback), redraws region; wrapped in try/catch with warning. |
| Formatting / Misc multiple files ( .../AnnotationOverlay.tsx, .../VideoEditor.tsx, .../VideoPlayback.tsx, etc.) |
Whitespace/indentation, import reordering, and minor control-flow formatting changes with no behavioral impact. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant UI as AnnotationSettingsPanel
participant Panel as SettingsPanel
participant Editor as VideoEditor
participant Exporter as annotationRenderer
participant Canvas as Canvas/Offscreen
User->>UI: adjust blur slider
UI->>Panel: onBlurIntensityChange(intensity)
Panel->>Editor: onAnnotationBlurIntensityChange(id, intensity)
Editor->>Editor: update annotationRegions.blurIntensity
Note over Exporter,Canvas: On export/render-to-canvas
Exporter->>Canvas: renderBlur(annotation.blurIntensity, region)
Canvas-->>Exporter: blurred pixels drawn back
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- feat: Advanced Video Editor Implementation, Native WGC Integration, and AI Auto-Captions #122: Implements the same "blur" annotation feature across types, UI slider, overlay, exporter, and editor plumbing.
- Revert "feat: Advanced Video Editor Implementation, Native WGC Integration, and AI Auto-Captions" #124: Modifies/reverts parts of the blur annotation support touching the same annotation types, renderer, and UI code paths.
Poem
🐰 I nudged the slider, soft and shy,
Frames sighed and smudged beneath my eye,
From panel to canvas, a fuzzy art,
I hopped, I blurred — a gentle part,
Hoppity hop, a misty byte.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 7.69% 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 'feat: blur annotate' is concise and directly describes the main feature being added—blur annotation functionality. |
| Description check | ✅ Passed | The PR description comprehensively covers all required sections: clear description, motivation, type of change, related issues, testing guide, and checklist items all completed. |
✏️ 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: 5
🧹 Nitpick comments (1)
src/components/video-editor/SettingsPanel.tsx (1)
219-219: Tighten the blur callback contract.
onAnnotationBlurIntensityChangeis optional here, so a caller can render blur controls that silently no-op.src/components/video-editor/VideoEditor.tsx:3820-3835already passes this handler unconditionally, so making it required here—or explicitly hiding the blur UI when it is missing—would make the new feature much harder to wire up incorrectly.Possible tightening
- onAnnotationBlurIntensityChange?: (id: string, intensity: number) => void; + onAnnotationBlurIntensityChange: (id: string, intensity: number) => void; ... - onBlurIntensityChange={ - onAnnotationBlurIntensityChange - ? (intensity) => onAnnotationBlurIntensityChange(selectedAnnotation.id, intensity) - : undefined - } + onBlurIntensityChange={(intensity) => + onAnnotationBlurIntensityChange(selectedAnnotation.id, intensity) + }Also applies to: 1291-1295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` at line 219, The prop onAnnotationBlurIntensityChange on SettingsPanel.tsx is currently optional which allows rendering blur controls that no-op; update the component contract so the blur handler is required (make onAnnotationBlurIntensityChange: (id: string, intensity: number) => void) or, alternatively, guard the blur UI inside SettingsPanel to hide/disable those controls when the prop is undefined; ensure the change is reflected where SettingsPanel is consumed (VideoEditor component that currently passes the handler unconditionally) and apply the same tightening to the other identical prop occurrence around the 1291-1295 area.
🤖 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 519-520: The blur unit label is wrong: the UI shows
annotation.blurIntensity with a "%" sign but the value is used as pixel radius;
update the display and any related tooltip/aria-label in AnnotationSettingsPanel
(where annotation.blurIntensity is rendered) to show "px" instead of "%" and
ensure any places that format the value (e.g., the span rendering
{annotation.blurIntensity ?? 12}% ) are changed to use "px" so the
preview/export matches the UI.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 814-815: Update the hover playback error handling so the
onMouseEnter play() promise ignores expected AbortError failures instead of
logging them: in the onMouseEnter handler where
e.currentTarget.play().catch(...) is used, change the catch to check err.name
=== 'AbortError' (or err instanceof DOMException && err.name === 'AbortError')
and return silently for that case; only call console.warn for other errors. Keep
the existing onMouseLeave logic (e.currentTarget.pause();
e.currentTarget.currentTime = 0;) unchanged.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 730-733: The empty catch block after calling
event.currentTarget.releasePointerCapture(event.pointerId) should include a
brief comment explaining the swallow is intentional to satisfy the Biome linter;
update the catch in the anonymous pointer handler (the try {
event.currentTarget.releasePointerCapture(...) } catch { }) to contain a
one-line comment like "// ignore: releasePointerCapture may throw on detached
targets" so the noEmptyBlockStatements rule is satisfied.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 355-356: The export currently multiplies annotation.blurIntensity
by scaleFactor (intensity = (annotation.blurIntensity ?? 12) * scaleFactor)
which makes exported blur different from the editor preview that treats blur as
raw pixels (see AnnotationOverlay.tsx usage); change the export to use the raw
pixel value for blur (i.e., set intensity from annotation.blurIntensity without
scaling) so semantics match WYSIWYG, keeping the existing fallback and the
early-return checks (variables: annotation.blurIntensity, intensity,
scaleFactor, width, height).
- Line 398: Remove the unnecessary "as any" cast on the drawImage call: in the
function/method where ctx.drawImage(offscreen as any, srcX, srcY) is used (in
annotationRenderer.ts), change it to ctx.drawImage(offscreen, srcX, srcY) since
offscreen is already typed as HTMLCanvasElement | OffscreenCanvas which
satisfies CanvasImageSource.
---
Nitpick comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Line 219: The prop onAnnotationBlurIntensityChange on SettingsPanel.tsx is
currently optional which allows rendering blur controls that no-op; update the
component contract so the blur handler is required (make
onAnnotationBlurIntensityChange: (id: string, intensity: number) => void) or,
alternatively, guard the blur UI inside SettingsPanel to hide/disable those
controls when the prop is undefined; ensure the change is reflected where
SettingsPanel is consumed (VideoEditor component that currently passes the
handler unconditionally) and apply the same tightening to the other identical
prop occurrence around the 1291-1295 area.
🪄 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: b8ebb2f3-eb33-42d0-956f-5e7083bcc17d
📒 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
| {annotation.blurIntensity ?? 12}% | ||
| </span> |
There was a problem hiding this comment.
Blur unit label is incorrect (% vs px).
Line 519 displays a percentage, but this value is used as pixel blur radius in preview/export. This can mislead users.
Suggested fix
- {annotation.blurIntensity ?? 12}%
+ {annotation.blurIntensity ?? 12}px📝 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.
| {annotation.blurIntensity ?? 12}% | |
| </span> | |
| {annotation.blurIntensity ?? 12}px | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/AnnotationSettingsPanel.tsx` around lines 519 -
520, The blur unit label is wrong: the UI shows annotation.blurIntensity with a
"%" sign but the value is used as pixel radius; update the display and any
related tooltip/aria-label in AnnotationSettingsPanel (where
annotation.blurIntensity is rendered) to show "px" instead of "%" and ensure any
places that format the value (e.g., the span rendering {annotation.blurIntensity
?? 12}% ) are changed to use "px" so the preview/export matches the UI.
| try { | ||
| event.currentTarget.releasePointerCapture(event.pointerId); | ||
| } catch {} | ||
| } catch { } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify empty catch blocks in the changed playback file.
rg -n --type tsx 'catch\s*\{\s*\}' src/components/video-editor/VideoPlayback.tsxRepository: webadderall/Recordly
Length of output: 91
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and read the relevant section
if [ -f "src/components/video-editor/VideoPlayback.tsx" ]; then
echo "=== File found. Reading lines 725-740 ==="
sed -n '725,740p' src/components/video-editor/VideoPlayback.tsx | cat -n
else
echo "File not found at specified path. Searching..."
find . -name "VideoPlayback.tsx" -type f 2>/dev/null
fiRepository: webadderall/Recordly
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Check for linter configuration files (Biome, ESLint, etc.)
echo "=== Checking for linter configurations ==="
ls -la | grep -E "(biome|eslint|prettier)"
ls -la src/components/video-editor/ 2>/dev/null | head -20Repository: webadderall/Recordly
Length of output: 1571
🏁 Script executed:
#!/bin/bash
# Search for empty catch blocks without strict file type
rg -n 'catch\s*\{\s*\}' src/components/video-editor/Repository: webadderall/Recordly
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Check Biome configuration for catch block rules
cat biome.jsonRepository: webadderall/Recordly
Length of output: 4909
🏁 Script executed:
#!/bin/bash
# Check ESLint configuration
cat .eslintrc.cjsRepository: webadderall/Recordly
Length of output: 518
🌐 Web query:
Biome noEmptyBlockStatements rule empty catch blocks
💡 Result:
Biome's noEmptyBlockStatements rule disallows empty block statements and static blocks in JavaScript, including empty catch blocks in try-catch statements. It flags them as suspicious because they often result from incomplete refactoring and can confuse readers. Empty catch blocks without content trigger the rule, as shown in the invalid example: try { doSomething; } catch(ex) {} To make it valid, add a comment inside the block: try { doSomething; } catch (ex) { // continue regardless of error } The rule ignores blocks containing comments, allowing intentional empty blocks (like catch/finally) to be documented. Configure it in biome.json under linter.rules.suspicious.noEmptyBlockStatements, defaulting to "error" severity. This behavior is consistent across official Biome documentation as of the latest version.
Citations:
- 1: https://biomejs.dev/linter/rules/no-empty-block-statements/
- 2: https://biomejs.cn/pt-br/linter/rules/no-empty-block-statements
- 3: https://github.com/biomejs/biome/blob/main/crates/biome_js_analyze/src/lint/suspicious/no_empty_block_statements.rs
Add comment to empty catch block to satisfy Biome linter.
Line 732 has an empty catch block, which is flagged as an error by Biome's noEmptyBlockStatements rule. Add a comment inside the block to document the intentional empty handler.
Suggested fix
- } catch { }
+ } catch {
+ // Pointer may already be released; safe to ignore.
+ }📝 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.
| try { | |
| event.currentTarget.releasePointerCapture(event.pointerId); | |
| } catch {} | |
| } catch { } | |
| }; | |
| try { | |
| event.currentTarget.releasePointerCapture(event.pointerId); | |
| } catch { | |
| // Pointer may already be released; safe to ignore. | |
| } | |
| }; |
🧰 Tools
🪛 Biome (2.4.10)
[error] 732-732: 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 730 - 733, The
empty catch block after calling
event.currentTarget.releasePointerCapture(event.pointerId) should include a
brief comment explaining the swallow is intentional to satisfy the Biome linter;
update the catch in the anonymous pointer handler (the try {
event.currentTarget.releasePointerCapture(...) } catch { }) to contain a
one-line comment like "// ignore: releasePointerCapture may throw on detached
targets" so the noEmptyBlockStatements rule is satisfied.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/video-editor/SettingsPanel.tsx (2)
1008-1017: Hardcoded strings should use i18n translation functions.The error and success toast messages in the video upload handler use hardcoded English strings, while the rest of the file uses
tSettings()andt()for internationalization.♻️ Suggested fix using i18n
if (!isVideoWallpaperSource(filePath)) { - toast.error("Unsupported format", { - description: "Please select a video file (mp4, webm, mov, etc.)", - }); + toast.error(tSettings("background.videoUploadError", "Unsupported format"), { + description: tSettings("background.videoUploadErrorDescription", "Please select a video file (mp4, webm, mov, etc.)"), + }); return; } setCustomImages((prev) => [filePath, ...prev]); onWallpaperChange(filePath); - toast.success("Video background added"); + toast.success(tSettings("background.videoUploadSuccess", "Video background added")); } catch { - toast.error("Failed to import video background"); + toast.error(tSettings("background.videoUploadFailed", "Failed to import video background")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 1008 - 1017, Replace the hardcoded toast messages in the video upload handler with i18n calls: use tSettings()/t() instead of literal strings for the "Unsupported format", "Please select a video file (mp4, webm, mov, etc.)", "Video background added", and "Failed to import video background" messages; update the calls around setCustomImages, onWallpaperChange, and the toast.error/toast.success invocations to pass translated strings (e.g., tSettings('...') or t('...')) consistent with other messages in this file so all user-facing text is localized.
1302-1315: Unnecessary optional chaining after guard condition.The guard at lines 1291-1297 already confirms
onAnnotationContentChange,onAnnotationTypeChange,onAnnotationStyleChange, andonAnnotationDeleteare truthy. The optional chaining (?.) on lines 1302-1304 and 1315 is redundant and inconsistent with howonAnnotationFigureDataChangeis handled on line 1307.♻️ Suggested cleanup for consistency
return ( <AnnotationSettingsPanel annotation={selectedAnnotation} - onContentChange={(content) => onAnnotationContentChange?.(selectedAnnotation.id, content)} - onTypeChange={(type) => onAnnotationTypeChange?.(selectedAnnotation.id, type)} - onStyleChange={(style) => onAnnotationStyleChange?.(selectedAnnotation.id, style)} + onContentChange={(content) => onAnnotationContentChange(selectedAnnotation.id, content)} + onTypeChange={(type) => onAnnotationTypeChange(selectedAnnotation.id, type)} + onStyleChange={(style) => onAnnotationStyleChange(selectedAnnotation.id, style)} onFigureDataChange={ onAnnotationFigureDataChange ? (figureData) => onAnnotationFigureDataChange(selectedAnnotation.id, figureData) : undefined } onBlurIntensityChange={ onAnnotationBlurIntensityChange ? (intensity) => onAnnotationBlurIntensityChange(selectedAnnotation.id, intensity) : undefined } - onDelete={() => onAnnotationDelete?.(selectedAnnotation.id)} + onDelete={() => onAnnotationDelete(selectedAnnotation.id)} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 1302 - 1315, The handlers were already guarded earlier, so remove the redundant optional chaining when calling them: change onAnnotationContentChange?. to onAnnotationContentChange, onAnnotationTypeChange?. to onAnnotationTypeChange, onAnnotationStyleChange?. to onAnnotationStyleChange, and onAnnotationDelete?. to onAnnotationDelete so each callback is invoked directly with selectedAnnotation.id (keep the existing onAnnotationFigureDataChange and onAnnotationBlurIntensityChange conditional patterns as-is); update the calls in the SettingsPanel JSX where onContentChange, onTypeChange, onStyleChange, and onDelete are passed.
🤖 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/SettingsPanel.tsx`:
- Around line 1008-1017: Replace the hardcoded toast messages in the video
upload handler with i18n calls: use tSettings()/t() instead of literal strings
for the "Unsupported format", "Please select a video file (mp4, webm, mov,
etc.)", "Video background added", and "Failed to import video background"
messages; update the calls around setCustomImages, onWallpaperChange, and the
toast.error/toast.success invocations to pass translated strings (e.g.,
tSettings('...') or t('...')) consistent with other messages in this file so all
user-facing text is localized.
- Around line 1302-1315: The handlers were already guarded earlier, so remove
the redundant optional chaining when calling them: change
onAnnotationContentChange?. to onAnnotationContentChange,
onAnnotationTypeChange?. to onAnnotationTypeChange, onAnnotationStyleChange?. to
onAnnotationStyleChange, and onAnnotationDelete?. to onAnnotationDelete so each
callback is invoked directly with selectedAnnotation.id (keep the existing
onAnnotationFigureDataChange and onAnnotationBlurIntensityChange conditional
patterns as-is); update the calls in the SettingsPanel JSX where
onContentChange, onTypeChange, onStyleChange, and onDelete are passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5088cde-29be-4f30-b06a-1d6133a98b0e
📒 Files selected for processing (3)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/exporter/annotationRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/exporter/annotationRenderer.ts
ac8a725 to
26a87c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/exporter/annotationRenderer.ts`:
- Around line 186-190: renderText currently calls ctx.roundRect(...) without a
fallback, which can break in browsers that lack roundRect; make it consistent
with renderBlur by checking if ctx.roundRect exists and using it when available,
otherwise use ctx.rect (or an equivalent non-rounded path) before calling
ctx.fill(); update the code inside renderText where ctx.roundRect(bgX, bgY,
bgWidth, bgHeight, borderRadius) is invoked so it mirrors the fallback logic
used in renderBlur.
- Around line 353-354: The call to offscreen.getContext("2d") can return null;
modify the code around offCtx in annotationRenderer.ts to check that offCtx is
not null before calling offCtx.putImageData(imageData, 0, 0) — if null, handle
gracefully by logging or throwing a clear error (e.g., via processLogger or
throwing a descriptive Error) so you avoid relying on exceptions from
putImageData; refer to the offscreen variable, offCtx identifier, and the
putImageData call to find and update the exact spot.
🪄 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: cb5c8628-e329-484a-83c9-dbd6bffc8929
📒 Files selected for processing (6)
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/lib/exporter/annotationRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/video-editor/VideoEditor.tsx
- src/components/video-editor/AnnotationOverlay.tsx
- src/components/video-editor/AnnotationSettingsPanel.tsx
- src/components/video-editor/SettingsPanel.tsx
26a87c9 to
aedd3f2
Compare
Description
This PR implements the Blur Annotation feature, allowing users to obscure sensitive information or regions within their video recordings directly in the editor.
Key Changes:
bluras a newAnnotationTypewith adjustableblurIntensity.AnnotationSettingsPanelandSettingsPanel.annotationRenderer.tsto support high-quality Gaussian blur during export and preview using an offscreen canvas.Motivation
Privacy is a core requirement for screen recording. Users need a way to hide credentials, PII, or distracting backgrounds without leaving the Recordly ecosystem.
Type of Change
Related Issue(s)
Relates to
feat/blur-annotate.Screenshots / Video
Screenshot:
Relates to
feat/blur-annotate.Video:
Relates to
feat/blur-annotate.Testing Guide
npm run dev.Checklist
Summary by CodeRabbit
New Features
Bug Fixes