feat: add blur annotation and Whisper model selection for autocaption#116
Closed
mahdyarief wants to merge 6 commits intowebadderall:mainfrom
Closed
feat: add blur annotation and Whisper model selection for autocaption#116mahdyarief wants to merge 6 commits intowebadderall:mainfrom
mahdyarief wants to merge 6 commits intowebadderall:mainfrom
Conversation
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Electron IPC & Types electron/electron-env.d.ts, electron/preload.ts, electron/ipc/handlers.ts |
Replaced fixed “small” Whisper APIs with model-parameterized APIs (getWhisperModelStatus/downloadWhisperModel/deleteWhisperModel). Progress events include model. Added auto-caption progress/chunk subscriptions and runWhisperWithProgress (stderr % parsing). |
Native capture & monitor utilities electron/native/wgc-capture/src/main.cpp, electron/native/wgc-capture/src/monitor_utils.cpp, electron/native/wgc-capture/src/monitor_utils.h |
Added findMonitorByBounds, extended CaptureConfig with display bounds, added fallback monitor lookup heuristics, diagnostic logging, and wstring→string helper. |
Whisper runtime manifest & packaging electron/native/bin/win32-x64/whisper-runtime.json, electron-builder.json5 |
Added Whisper runtime manifest for win32-x64. Updated Windows packaging fields (executableName, templated artifactName) and added top-level copyright. |
Build scripts & CMake discovery scripts/build-cursor-monitor.mjs, scripts/build-whisper-runtime.mjs, scripts/build-windows-capture.mjs, .gitignore |
Added project-local vendored CMake probe at .cmake_ext/.../cmake.exe on Windows; adjusted archive extraction to use project-relative paths; added ignore patterns for .cache/, .cmake_ext/, ebcache/. |
Video editor — model selection, captions & playback src/components/video-editor/SettingsPanel.tsx, src/components/video-editor/VideoEditor.tsx, src/components/video-editor/VideoPlayback.tsx, src/components/video-editor/editorPreferences.ts, src/components/video-editor/projectPersistence.ts, src/components/video-editor/types.ts |
Added WhisperModel selection (persisted), replaced single-model UI with Select, wired IPC per selected model, added auto-caption progress/subscriptions, cue editing/selection, time-range generation, and exposed seek(time) on VideoPlaybackRef. |
Annotation blur feature & exporter src/components/video-editor/AnnotationOverlay.tsx, src/components/video-editor/AnnotationSettingsPanel.tsx, src/lib/exporter/annotationRenderer.ts |
Introduced "blur" annotation and blurIntensity (DEFAULT_BLUR_INTENSITY=12). Added settings slider, overlay backdrop-blur rendering, and canvas export helper renderBlur with clipping, filter, and error containment. |
Timeline / caption timeline integration src/components/video-editor/timeline/TimelineEditor.tsx, src/components/video-editor/timeline/TimelineWrapper.tsx, src/components/video-editor/timeline/Item.tsx, src/components/video-editor/timeline/ItemGlass.module.css |
Added caption row/items (caption/caption-range variants), caption styling (glassCyan), span normalization/clamping, selection/time-range drag selection, and changed onItemSpanChange to include rowId. |
Styling & UI tweaks src/index.css, src/components/video-editor/timeline/KeyframeMarkers.tsx |
Added .subtle-scrollbar WebKit scrollbar utility. Prevented undefined sidebarWidth by defaulting to 0 in KeyframeMarkers. |
Misc scripts & tooling package.json, scripts/build-whisper-runtime.mjs |
Added typecheck npm script (tsc --noEmit) and adjusted build extraction to use project-relative paths. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant Renderer as Renderer Process
participant Main as Main Process
participant FS as File System
User->>Renderer: select model (e.g. "small")
Renderer->>Main: getWhisperModelStatus(modelName)
Main->>FS: check model file
FS-->>Main: exists / not exists
Main-->>Renderer: status (exists/path)
alt not downloaded
User->>Renderer: downloadWhisperModel(modelName)
Renderer->>Main: download request
Main->>Main: fetch model, write to disk
Main-->>Renderer: onWhisperModelDownloadProgress(model, progress)
Renderer->>Renderer: update UI
Main-->>Renderer: download result (path/success)
end
User->>Renderer: generate-auto-captions(range?)
Renderer->>Main: generate-auto-captions(opts...)
Main->>Main: for each chunk: runWhisperWithProgress(...)
Main-->>Renderer: onAutoCaptionProgress(progress)
Main-->>Renderer: onAutoCaptionChunk({cues})
Main-->>Renderer: final generate-auto-captions result
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
- Add custom cursor packs and harden native postinstall build flow #111 — Overlapping changes to build scripts and vendored CMake discovery; likely related to the same CMake fallback additions.
Poem
🐰 I hopped through models, tiny to grand,
I renamed "small" and gave them all a band.
I blurred the frames with a gentle hand,
I chunked the captions, progress on demand.
CMake found local — builds hum like a band!
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 2.44% 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 and concisely summarizes the two main features added: blur annotation and Whisper model selection for autocaption. |
| Description check | ✅ Passed | The PR description is well-structured and comprehensive, covering motivation, type of change, testing instructions, and includes supporting screenshots demonstrating both features. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
⚔️ Resolve merge conflicts
- Resolve merge conflict in branch
feat/blur-annotation-and-model-selection
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/SettingsPanel.tsx (1)
1419-1447:⚠️ Potential issue | 🟠 MajorSeparate downloaded-model state from manually picked model paths.
This branch keys off
whisperModelPath, but that same state is also populated by the manual picker insrc/components/video-editor/VideoEditor.tsx. After a user picks a local model file, the UI flips toDelete Model, and that callback deletes the cached file forselectedModelrather than clearing the picked path. At minimum, drive this branch fromwhisperModelDownloadStatus === "downloaded"; ideally keep downloaded-model state separate from arbitrary local paths.🤖 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 1419 - 1447, The UI currently uses whisperModelPath to decide between "Download" vs "Delete", which conflates manually-picked local paths with downloaded/cached models; change the branch to drive the UI from whisperModelDownloadStatus (check for whisperModelDownloadStatus === "downloaded") or introduce a separate state like downloadedWhisperModelPath/hasDownloadedWhisperModel to represent cached downloads, and update the conditional in SettingsPanel.tsx accordingly; also ensure onDeleteWhisperModel only clears the downloaded/cached model (and its downloaded state) and does not clear the manually-picked whisperModelPath/selectedModel, and ensure the manual picker in VideoEditor.tsx continues to set whisperModelPath without toggling the download status.
🧹 Nitpick comments (7)
src/index.css (1)
125-140: Add Firefox styling parity for.subtle-scrollbar.Currently, only WebKit scrollbars are styled; Firefox users see the browser's default scrollbar. Add
scrollbar-widthandscrollbar-colorto.subtle-scrollbarfor consistent cross-browser visuals. Firefox 64+ supports the standard CSS Scrollbars Styling Module properties.♻️ Proposed cross-browser enhancement
+ .subtle-scrollbar { + scrollbar-width: thin; /* Firefox */ + scrollbar-color: rgba(255, 255, 255, 0.1) transparent; /* thumb track */ + } + .subtle-scrollbar::-webkit-scrollbar { width: 4px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` around lines 125 - 140, The .subtle-scrollbar rules only target WebKit; add Firefox-compatible properties to the .subtle-scrollbar selector by setting scrollbar-width (e.g., thin) and scrollbar-color (thumb color and track color) so Firefox shows the same subtle appearance—update the .subtle-scrollbar block (the selector .subtle-scrollbar) to include scrollbar-width and scrollbar-color values that match the existing rgba thumb and transparent track used in the ::-webkit-scrollbar rules.src/components/video-editor/AnnotationOverlay.tsx (1)
114-123: Remove redundantbackdrop-blur-mdclass.The
backdrop-blur-mdTailwind class is overridden by the inlinebackdropFilterstyle, making it redundant. Removing it avoids confusion about which value is applied.🧹 Proposed cleanup
case 'blur': return ( <div - className="w-full h-full rounded-lg backdrop-blur-md" + className="w-full h-full rounded-lg" style={{ backdropFilter: `blur(${annotation.blurIntensity ?? 12}px)`, WebkitBackdropFilter: `blur(${annotation.blurIntensity ?? 12}px)` }} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationOverlay.tsx` around lines 114 - 123, The JSX for the 'blur' case in AnnotationOverlay.tsx currently uses both the Tailwind class backdrop-blur-md and an inline style setting backdropFilter/WebkitBackdropFilter with annotation.blurIntensity, which is redundant; remove the className token "backdrop-blur-md" from the div in the 'blur' branch (the case handling in the switch that returns the div using annotation.blurIntensity) so the inline backdropFilter styles are the single source of truth for the blur intensity.src/components/video-editor/projectPersistence.ts (1)
498-500: Consider validatingselectedModelagainst theWhisperModelunion type.The current normalization accepts any string without verifying it's a valid
WhisperModelvalue. This could allow invalid model names to be persisted. Consider validating against the allowed values similar to other normalization functions in this file.♻️ Proposed validation
+const VALID_WHISPER_MODELS = ['tiny', 'base', 'small', 'medium', 'large'] as const; + +function isValidWhisperModel(value: unknown): value is typeof VALID_WHISPER_MODELS[number] { + return typeof value === 'string' && VALID_WHISPER_MODELS.includes(value as any); +} + // In normalizedAutoCaptionSettings: -selectedModel: typeof rawAutoCaptionSettings.selectedModel === "string" - ? rawAutoCaptionSettings.selectedModel - : DEFAULT_AUTO_CAPTION_SETTINGS.selectedModel, +selectedModel: isValidWhisperModel(rawAutoCaptionSettings.selectedModel) + ? rawAutoCaptionSettings.selectedModel + : DEFAULT_AUTO_CAPTION_SETTINGS.selectedModel,🤖 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 498 - 500, The normalization for selectedModel currently accepts any string from rawAutoCaptionSettings.selectedModel; instead validate that value is one of the WhisperModel union members before persisting. Update the selectedModel assignment (and/or add a helper like normalizeAutoCaptionSettings used elsewhere) to check rawAutoCaptionSettings.selectedModel against the allowed WhisperModel values (or an array/set of those values) and fall back to DEFAULT_AUTO_CAPTION_SETTINGS.selectedModel when it is not a valid WhisperModel; reference the symbols selectedModel, rawAutoCaptionSettings, DEFAULT_AUTO_CAPTION_SETTINGS, and WhisperModel to locate and change the logic.electron/ipc/handlers.ts (1)
1015-1015: Replaceanywith a more specific type for unused event parameters.Static analysis flags
anyusage. While these_eventparameters are unused, usingunknownorElectron.IpcMainInvokeEventwould be more type-safe.🔧 Proposed type fix
-async function getWhisperModelStatus(_event: any, modelName: string) { +async function getWhisperModelStatus(_event: Electron.IpcMainInvokeEvent, modelName: string) {-async function deleteWhisperModel(_event: any, modelName: string) { +async function deleteWhisperModel(_event: Electron.IpcMainInvokeEvent, modelName: string) {Also applies to: 1148-1148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` at line 1015, The handler functions use a parameter typed as any (e.g., async function getWhisperModelStatus(_event: any, modelName: string)) which static analysis flags; change the unused _event param to a safer type such as Electron.IpcMainInvokeEvent (preferred for IPC handlers) or unknown to avoid any, update the signature for getWhisperModelStatus and the other similar handler at the second occurrence so the parameter becomes _event: Electron.IpcMainInvokeEvent (or _event: unknown) and ensure any imports/types are available.src/components/video-editor/editorPreferences.ts (1)
46-46: Consider using theWhisperModeltype for stronger type safety.The
whisperSelectedModelproperty is typed asstringbut should use theWhisperModelunion type fromtypes.tsfor consistency. The normalization also doesn't validate against valid model names, which could allow invalid values to persist.♻️ Proposed type refinement
+import { type WhisperModel, DEFAULT_AUTO_CAPTION_SETTINGS } from "./types"; + +const VALID_WHISPER_MODELS = new Set<WhisperModel>(["tiny", "base", "small", "medium", "large"]); + export interface EditorPreferences extends PersistedEditorControls { customAspectWidth: string; customAspectHeight: string; customWallpapers: string[]; whisperExecutablePath: string | null; whisperModelPath: string | null; - whisperSelectedModel: string; + whisperSelectedModel: WhisperModel; }And in normalization:
whisperSelectedModel: - typeof raw.whisperSelectedModel === "string" - ? raw.whisperSelectedModel + typeof raw.whisperSelectedModel === "string" && VALID_WHISPER_MODELS.has(raw.whisperSelectedModel as WhisperModel) + ? (raw.whisperSelectedModel as WhisperModel) : fallback.whisperSelectedModel,Also applies to: 225-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/editorPreferences.ts` at line 46, Change the whisperSelectedModel property to use the WhisperModel union type instead of string (e.g., whisperSelectedModel: WhisperModel) and update the normalization logic (the block around the current normalization at lines ~225-228) to validate incoming values against the allowed WhisperModel options at runtime: import or derive a runtime list of valid models, check the loaded value is one of them (use a small helper isValidWhisperModel or VALID_WHISPER_MODELS.includes(value)), and if invalid fall back to the existing default model; ensure the import of WhisperModel from types.ts is added where needed so both the interface/type and normalization use the strongly typed model set.src/lib/exporter/annotationRenderer.ts (1)
321-331: Potential type mismatch with OffscreenCanvas context.When using
OffscreenCanvas,getContext('2d')returnsOffscreenCanvasRenderingContext2D | null, notCanvasRenderingContext2D. While they share most methods, this cast may cause issues in stricter TypeScript configurations.🔧 Proposed type fix
-const offCtx = offscreen.getContext('2d') as CanvasRenderingContext2D; +const offCtx = offscreen.getContext('2d'); +if (!offCtx) { + ctx.restore(); + return; +} offCtx.putImageData(imageData, 0, 0);🤖 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 321 - 331, The code casts offscreen.getContext('2d') to CanvasRenderingContext2D which is unsafe for OffscreenCanvas; change how offCtx is obtained in the block using the offscreen variable: declare offCtx with the union type OffscreenCanvasRenderingContext2D | CanvasRenderingContext2D | null (or use a type guard), call offscreen.getContext('2d') without forcing a CanvasRenderingContext2D, then explicitly check for null (throw or handle error) before calling putImageData(imageData, 0, 0); update references to offCtx accordingly so both OffscreenCanvas and DOM canvas contexts are handled safely (symbols: offscreen, offCtx, getContext, putImageData, imageData, srcW/srcH).electron/electron-env.d.ts (1)
150-175: Use a sharedWhisperModelunion on the bridge.These methods still accept raw
strings, which is why the renderer now needsas anycasts to setselectedModel. A shared"tiny" | "base" | "small" | "medium" | "large"type here and inelectron/preload.tswould remove those casts and stop unsupported ids from crossing the renderer/preload boundary.♻️ Proposed refactor
+type WhisperModel = "tiny" | "base" | "small" | "medium" | "large"; + interface Window { electronAPI: { getWhisperModelStatus: ( - modelName: string, + modelName: WhisperModel, ) => Promise<{ success: boolean; exists: boolean; path?: string | null; error?: string; }>; downloadWhisperModel: ( - modelName: string, + modelName: WhisperModel, ) => Promise<{ success: boolean; path?: string; alreadyDownloaded?: boolean; error?: string; }>; - deleteWhisperModel: (modelName: string) => Promise<{ success: boolean; error?: string }>; + deleteWhisperModel: (modelName: WhisperModel) => Promise<{ success: boolean; error?: string }>; onWhisperModelDownloadProgress: ( callback: (state: { status: "idle" | "downloading" | "downloaded" | "error"; progress: number; - model: string; + model: WhisperModel; path?: string | null; error?: string; }) => void,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/electron-env.d.ts` around lines 150 - 175, Introduce a shared WhisperModel union type (e.g. type WhisperModel = "tiny" | "base" | "small" | "medium" | "large") and replace all raw string parameters/fields with that type across the bridge: update getWhisperModelStatus(modelName: string), downloadWhisperModel(modelName: string), deleteWhisperModel(modelName: string), and the model field in onWhisperModelDownloadProgress callback to use WhisperModel; also export or import the same WhisperModel type in electron/preload.ts so the renderer can use it (remove any as any casts and prevent unsupported ids crossing renderer/preload).
🤖 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 148-158: Replace hardcoded UI strings in the Blur tab with i18n
calls: change the TabsTrigger text "Blur" and the label text "Blur Intensity:
{annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY}px" to use the translation
function t(), e.g. t('annotation.blur.title') for the tab and
t('annotation.blur.intensity', { value: annotation.blurIntensity ??
DEFAULT_BLUR_INTENSITY }) or a two-part translation like
t('annotation.blur.intensityLabel') + interpolated value; update the code around
TabsTrigger and TabsContent (and the label rendering using
DEFAULT_BLUR_INTENSITY and annotation.blurIntensity) to call t(), and add
corresponding translation keys to the locale files.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1434-1452: The effect should clear the previous model file path
when the selected model changes: inside the useEffect in VideoEditor, stop using
setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null) and
instead set the path deterministically—if result.exists and result.path use
setWhisperModelPath(result.path), otherwise call setWhisperModelPath(null) in
the missing-model branch; if you need to preserve a user-picked file across
model switches, keep that value in a separate state (e.g.,
manualWhisperModelPath) and do not mix it with the automatic whisperModelPath.
- Around line 380-383: Validate initialEditorPreferences.whisperSelectedModel
against the supported Whisper model whitelist before seeding autoCaptionSettings
state: instead of casting with "as any" in the VideoEditor useState initializer,
check that initialEditorPreferences.whisperSelectedModel is one of
["tiny","base","small","medium","large"] and only use it if valid; otherwise
default to "small". Update normalizeEditorPreferences() (and any call sites that
seed state like the autoCaptionSettings initializer), remove the unsafe cast,
and ensure frontend state never contains an unsupported model so IPC calls (and
handlers like deleteWhisperModel and getWhisperModelStatus) receive only known
model IDs.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 333-341: The fallback using ctx.rect produces sharp corners;
replace it with a manual rounded-rect path when ctx.roundRect is missing: in
annotationRenderer.ts where radius, x, y, width, height and ctx are used (the
block that currently checks if (ctx.roundRect) ... else ctx.rect(...);
ctx.clip()), implement a path that moves to the top-left+radius, draws lines and
quadratic/arc corners (or arcTo) to create rounded corners using the same
radius*scaleFactor and then call ctx.clip(); keep the existing branch for
ctx.roundRect but ensure the manual path produces the same rounded blur region
as the UI.
---
Outside diff comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1419-1447: The UI currently uses whisperModelPath to decide
between "Download" vs "Delete", which conflates manually-picked local paths with
downloaded/cached models; change the branch to drive the UI from
whisperModelDownloadStatus (check for whisperModelDownloadStatus ===
"downloaded") or introduce a separate state like
downloadedWhisperModelPath/hasDownloadedWhisperModel to represent cached
downloads, and update the conditional in SettingsPanel.tsx accordingly; also
ensure onDeleteWhisperModel only clears the downloaded/cached model (and its
downloaded state) and does not clear the manually-picked
whisperModelPath/selectedModel, and ensure the manual picker in VideoEditor.tsx
continues to set whisperModelPath without toggling the download status.
---
Nitpick comments:
In `@electron/electron-env.d.ts`:
- Around line 150-175: Introduce a shared WhisperModel union type (e.g. type
WhisperModel = "tiny" | "base" | "small" | "medium" | "large") and replace all
raw string parameters/fields with that type across the bridge: update
getWhisperModelStatus(modelName: string), downloadWhisperModel(modelName:
string), deleteWhisperModel(modelName: string), and the model field in
onWhisperModelDownloadProgress callback to use WhisperModel; also export or
import the same WhisperModel type in electron/preload.ts so the renderer can use
it (remove any as any casts and prevent unsupported ids crossing
renderer/preload).
In `@electron/ipc/handlers.ts`:
- Line 1015: The handler functions use a parameter typed as any (e.g., async
function getWhisperModelStatus(_event: any, modelName: string)) which static
analysis flags; change the unused _event param to a safer type such as
Electron.IpcMainInvokeEvent (preferred for IPC handlers) or unknown to avoid
any, update the signature for getWhisperModelStatus and the other similar
handler at the second occurrence so the parameter becomes _event:
Electron.IpcMainInvokeEvent (or _event: unknown) and ensure any imports/types
are available.
In `@src/components/video-editor/AnnotationOverlay.tsx`:
- Around line 114-123: The JSX for the 'blur' case in AnnotationOverlay.tsx
currently uses both the Tailwind class backdrop-blur-md and an inline style
setting backdropFilter/WebkitBackdropFilter with annotation.blurIntensity, which
is redundant; remove the className token "backdrop-blur-md" from the div in the
'blur' branch (the case handling in the switch that returns the div using
annotation.blurIntensity) so the inline backdropFilter styles are the single
source of truth for the blur intensity.
In `@src/components/video-editor/editorPreferences.ts`:
- Line 46: Change the whisperSelectedModel property to use the WhisperModel
union type instead of string (e.g., whisperSelectedModel: WhisperModel) and
update the normalization logic (the block around the current normalization at
lines ~225-228) to validate incoming values against the allowed WhisperModel
options at runtime: import or derive a runtime list of valid models, check the
loaded value is one of them (use a small helper isValidWhisperModel or
VALID_WHISPER_MODELS.includes(value)), and if invalid fall back to the existing
default model; ensure the import of WhisperModel from types.ts is added where
needed so both the interface/type and normalization use the strongly typed model
set.
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 498-500: The normalization for selectedModel currently accepts any
string from rawAutoCaptionSettings.selectedModel; instead validate that value is
one of the WhisperModel union members before persisting. Update the
selectedModel assignment (and/or add a helper like normalizeAutoCaptionSettings
used elsewhere) to check rawAutoCaptionSettings.selectedModel against the
allowed WhisperModel values (or an array/set of those values) and fall back to
DEFAULT_AUTO_CAPTION_SETTINGS.selectedModel when it is not a valid WhisperModel;
reference the symbols selectedModel, rawAutoCaptionSettings,
DEFAULT_AUTO_CAPTION_SETTINGS, and WhisperModel to locate and change the logic.
In `@src/index.css`:
- Around line 125-140: The .subtle-scrollbar rules only target WebKit; add
Firefox-compatible properties to the .subtle-scrollbar selector by setting
scrollbar-width (e.g., thin) and scrollbar-color (thumb color and track color)
so Firefox shows the same subtle appearance—update the .subtle-scrollbar block
(the selector .subtle-scrollbar) to include scrollbar-width and scrollbar-color
values that match the existing rgba thumb and transparent track used in the
::-webkit-scrollbar rules.
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 321-331: The code casts offscreen.getContext('2d') to
CanvasRenderingContext2D which is unsafe for OffscreenCanvas; change how offCtx
is obtained in the block using the offscreen variable: declare offCtx with the
union type OffscreenCanvasRenderingContext2D | CanvasRenderingContext2D | null
(or use a type guard), call offscreen.getContext('2d') without forcing a
CanvasRenderingContext2D, then explicitly check for null (throw or handle error)
before calling putImageData(imageData, 0, 0); update references to offCtx
accordingly so both OffscreenCanvas and DOM canvas contexts are handled safely
(symbols: offscreen, offCtx, getContext, putImageData, imageData, srcW/srcH).
🪄 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: e4937188-42bb-439d-b18c-af4f11a350fa
⛔ Files ignored due to path filters (5)
electron/native/bin/win32-x64/whisper-bench.exeis excluded by!**/*.exeelectron/native/bin/win32-x64/whisper-cli.exeis excluded by!**/*.exeelectron/native/bin/win32-x64/whisper-quantize.exeis excluded by!**/*.exeelectron/native/bin/win32-x64/whisper-server.exeis excluded by!**/*.exeelectron/native/bin/win32-x64/whisper-vad-speech-segments.exeis excluded by!**/*.exe
📒 Files selected for processing (18)
.gitignoreelectron/electron-env.d.tselectron/ipc/handlers.tselectron/native/bin/win32-x64/whisper-runtime.jsonelectron/preload.tsscripts/build-cursor-monitor.mjsscripts/build-whisper-runtime.mjsscripts/build-windows-capture.mjssrc/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/editorPreferences.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/index.csssrc/lib/exporter/annotationRenderer.ts
Comment on lines
+148
to
+158
| <TabsTrigger value="blur" className="data-[state=active]:bg-[#2563EB] data-[state=active]:text-white text-slate-400 py-2 rounded-lg transition-all gap-2"> | ||
| <Droplets className="w-4 h-4" /> | ||
| Blur | ||
| </TabsTrigger> | ||
| </TabsList> | ||
|
|
||
| <TabsContent value="blur" className="mt-0 space-y-4"> | ||
| <div> | ||
| <label className="text-xs font-medium text-slate-200 mb-2 block"> | ||
| Blur Intensity: {annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY}px | ||
| </label> |
There was a problem hiding this comment.
Missing i18n translation for "Blur" label and intensity text.
The "Blur" tab trigger (line 150) and "Blur Intensity" label (lines 156-157) use hardcoded English strings while all other tabs and labels use t() for localization.
🌐 Proposed fix for i18n consistency
<TabsTrigger value="blur" className="data-[state=active]:bg-[`#2563EB`] data-[state=active]:text-white text-slate-400 py-2 rounded-lg transition-all gap-2">
<Droplets className="w-4 h-4" />
- Blur
+ {t('annotations.blur')}
</TabsTrigger> <label className="text-xs font-medium text-slate-200 mb-2 block">
- Blur Intensity: {annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY}px
+ {t('annotations.blurIntensity', undefined, { intensity: annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY })}
</label>📝 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.
Suggested change
| <TabsTrigger value="blur" className="data-[state=active]:bg-[#2563EB] data-[state=active]:text-white text-slate-400 py-2 rounded-lg transition-all gap-2"> | |
| <Droplets className="w-4 h-4" /> | |
| Blur | |
| </TabsTrigger> | |
| </TabsList> | |
| <TabsContent value="blur" className="mt-0 space-y-4"> | |
| <div> | |
| <label className="text-xs font-medium text-slate-200 mb-2 block"> | |
| Blur Intensity: {annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY}px | |
| </label> | |
| <TabsTrigger value="blur" className="data-[state=active]:bg-[`#2563EB`] data-[state=active]:text-white text-slate-400 py-2 rounded-lg transition-all gap-2"> | |
| <Droplets className="w-4 h-4" /> | |
| {t('annotations.blur')} | |
| </TabsTrigger> | |
| </TabsList> | |
| <TabsContent value="blur" className="mt-0 space-y-4"> | |
| <div> | |
| <label className="text-xs font-medium text-slate-200 mb-2 block"> | |
| {t('annotations.blurIntensity', undefined, { intensity: annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY })} | |
| </label> |
🤖 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 148 -
158, Replace hardcoded UI strings in the Blur tab with i18n calls: change the
TabsTrigger text "Blur" and the label text "Blur Intensity:
{annotation.blurIntensity ?? DEFAULT_BLUR_INTENSITY}px" to use the translation
function t(), e.g. t('annotation.blur.title') for the tab and
t('annotation.blur.intensity', { value: annotation.blurIntensity ??
DEFAULT_BLUR_INTENSITY }) or a two-part translation like
t('annotation.blur.intensityLabel') + interpolated value; update the code around
TabsTrigger and TabsContent (and the label rendering using
DEFAULT_BLUR_INTENSITY and annotation.blurIntensity) to call t(), and add
corresponding translation keys to the locale files.
Comment on lines
+380
to
+383
| const [autoCaptionSettings, setAutoCaptionSettings] = useState<AutoCaptionSettings>(() => ({ | ||
| ...DEFAULT_AUTO_CAPTION_SETTINGS, | ||
| selectedModel: (initialEditorPreferences.whisperSelectedModel as any) || "small", | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the relevant files
find src -name "VideoEditor.tsx" -o -name "editorPreferences.ts" | head -20Repository: webadderall/Recordly
Length of output: 156
🏁 Script executed:
# Read the VideoEditor.tsx file around lines 380-383
sed -n '370,395p' src/components/video-editor/VideoEditor.tsxRepository: webadderall/Recordly
Length of output: 1717
🏁 Script executed:
# Check the editorPreferences.ts file to understand whisperSelectedModel handling
cat -n src/components/video-editor/editorPreferences.tsRepository: webadderall/Recordly
Length of output: 11269
🏁 Script executed:
# Check what DEFAULT_AUTO_CAPTION_SETTINGS contains and what supported models are defined
rg "DEFAULT_AUTO_CAPTION_SETTINGS|whisperModels|supported.*model|SUPPORTED" --type typescript -A 3 -B 3 | head -100Repository: webadderall/Recordly
Length of output: 98
🏁 Script executed:
# Search for where autoCaptionSettings.selectedModel is used in IPC calls
rg "selectedModel|whisper" src/components/video-editor/VideoEditor.tsx -A 2 -B 2 | head -150Repository: webadderall/Recordly
Length of output: 4306
🏁 Script executed:
# Look for types or constants defining supported Whisper models
fd . src -name "*.ts" -o -name "*.tsx" | xargs rg "small|base|medium|large" | grep -i "model\|whisper" | head -50Repository: webadderall/Recordly
Length of output: 2489
🏁 Script executed:
# Check the IPC handler for whisper model operations to see if validation exists
rg "getWhisperModelStatus|downloadWhisperModel|deleteWhisperModel" electron/ipc/handlers.ts -A 10 -B 2Repository: webadderall/Recordly
Length of output: 2520
🏁 Script executed:
# Check if there's a whitelist of valid models in the handlers
rg "tiny|base|small|medium|large" electron/ipc/handlers.ts -B 5 -A 5Repository: webadderall/Recordly
Length of output: 6021
Validate whisperSelectedModel against the supported Whisper models before seeding state.
The normalizeEditorPreferences() function only checks that whisperSelectedModel is a string (lines 225-228 of editorPreferences.ts), without validating it against the supported models ("tiny", "base", "small", "medium", "large"). Combined with the as any cast at line 381, an invalid or stale model ID from storage will pass through to state and propagate to IPC handlers. While downloadWhisperModel validates on the backend, deleteWhisperModel and getWhisperModelStatus do not, creating inconsistent behavior. Validate against the whitelist of supported models on the frontend and fall back to "small" instead of using as any.
🧰 Tools
🪛 Biome (2.4.9)
[error] 382-382: 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/components/video-editor/VideoEditor.tsx` around lines 380 - 383, Validate
initialEditorPreferences.whisperSelectedModel against the supported Whisper
model whitelist before seeding autoCaptionSettings state: instead of casting
with "as any" in the VideoEditor useState initializer, check that
initialEditorPreferences.whisperSelectedModel is one of
["tiny","base","small","medium","large"] and only use it if valid; otherwise
default to "small". Update normalizeEditorPreferences() (and any call sites that
seed state like the autoCaptionSettings initializer), remove the unsafe cast,
and ensure frontend state never contains an unsupported model so IPC calls (and
handlers like deleteWhisperModel and getWhisperModelStatus) receive only known
model IDs.
Comment on lines
+1434
to
+1452
| useEffect(() => { | ||
| void (async () => { | ||
| const result = await window.electronAPI.getWhisperSmallModelStatus(); | ||
| const result = await window.electronAPI.getWhisperModelStatus( | ||
| autoCaptionSettings.selectedModel, | ||
| ); | ||
| if (!result.success) { | ||
| return; | ||
| } | ||
|
|
||
| if (result.exists && result.path) { | ||
| setDownloadedWhisperModelPath(result.path); | ||
| setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null); | ||
| setWhisperModelDownloadStatus("downloaded"); | ||
| setWhisperModelDownloadProgress(100); | ||
| return; | ||
| } else { | ||
| setWhisperModelDownloadStatus("idle"); | ||
| setWhisperModelDownloadProgress(0); | ||
| } | ||
|
|
||
| setDownloadedWhisperModelPath(null); | ||
| setWhisperModelDownloadStatus("idle"); | ||
| setWhisperModelDownloadProgress(0); | ||
| })(); | ||
|
|
||
| return () => unsubscribe?.(); | ||
| }, []); | ||
| }, [autoCaptionSettings.selectedModel]); |
There was a problem hiding this comment.
Reset whisperModelPath when the selected model changes.
This effect keeps the previous path alive in both cases that matter: currentPath ?? result.path preserves the old file when the newly selected model is already downloaded, and the missing-model branch never clears the old file. After a model switch, Generate Captions can stay enabled and still run the previous model.
🐛 Proposed fix
useEffect(() => {
+ let cancelled = false;
+ setWhisperModelPath(null);
+ setWhisperModelDownloadStatus("idle");
+ setWhisperModelDownloadProgress(0);
+
void (async () => {
const result = await window.electronAPI.getWhisperModelStatus(
autoCaptionSettings.selectedModel,
);
- if (!result.success) {
+ if (cancelled || !result.success) {
return;
}
if (result.exists && result.path) {
- setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null);
+ setWhisperModelPath(result.path);
setWhisperModelDownloadStatus("downloaded");
setWhisperModelDownloadProgress(100);
- } else {
- setWhisperModelDownloadStatus("idle");
- setWhisperModelDownloadProgress(0);
}
})();
+
+ return () => {
+ cancelled = true;
+ };
}, [autoCaptionSettings.selectedModel]);If manual model-file selection is still supported, keep that path in separate state before applying this reset.
📝 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.
Suggested change
| useEffect(() => { | |
| void (async () => { | |
| const result = await window.electronAPI.getWhisperSmallModelStatus(); | |
| const result = await window.electronAPI.getWhisperModelStatus( | |
| autoCaptionSettings.selectedModel, | |
| ); | |
| if (!result.success) { | |
| return; | |
| } | |
| if (result.exists && result.path) { | |
| setDownloadedWhisperModelPath(result.path); | |
| setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null); | |
| setWhisperModelDownloadStatus("downloaded"); | |
| setWhisperModelDownloadProgress(100); | |
| return; | |
| } else { | |
| setWhisperModelDownloadStatus("idle"); | |
| setWhisperModelDownloadProgress(0); | |
| } | |
| setDownloadedWhisperModelPath(null); | |
| setWhisperModelDownloadStatus("idle"); | |
| setWhisperModelDownloadProgress(0); | |
| })(); | |
| return () => unsubscribe?.(); | |
| }, []); | |
| }, [autoCaptionSettings.selectedModel]); | |
| useEffect(() => { | |
| let cancelled = false; | |
| setWhisperModelPath(null); | |
| setWhisperModelDownloadStatus("idle"); | |
| setWhisperModelDownloadProgress(0); | |
| void (async () => { | |
| const result = await window.electronAPI.getWhisperModelStatus( | |
| autoCaptionSettings.selectedModel, | |
| ); | |
| if (cancelled || !result.success) { | |
| return; | |
| } | |
| if (result.exists && result.path) { | |
| setWhisperModelPath(result.path); | |
| setWhisperModelDownloadStatus("downloaded"); | |
| setWhisperModelDownloadProgress(100); | |
| } | |
| })(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [autoCaptionSettings.selectedModel]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` around lines 1434 - 1452, The
effect should clear the previous model file path when the selected model
changes: inside the useEffect in VideoEditor, stop using
setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null) and
instead set the path deterministically—if result.exists and result.path use
setWhisperModelPath(result.path), otherwise call setWhisperModelPath(null) in
the missing-model branch; if you need to preserve a user-picked file across
model switches, keep that value in a separate state (e.g.,
manualWhisperModelPath) and do not mix it with the automatic whisperModelPath.
Comment on lines
+333
to
+341
| // 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(); |
There was a problem hiding this comment.
Rounded corners fallback produces different visual result.
When ctx.roundRect is unavailable (older browsers), the fallback uses ctx.rect, resulting in sharp corners instead of the rounded blur region shown in the UI preview. This creates a visual inconsistency between preview and export on older browsers.
Consider implementing a manual rounded rect path as fallback:
🔧 Manual roundRect implementation
const radius = 8 * scaleFactor;
ctx.beginPath();
if (ctx.roundRect) {
ctx.roundRect(x, y, width, height, radius);
} else {
- ctx.rect(x, y, width, height);
+ // Manual rounded rect for older browsers
+ const r = Math.min(radius, width / 2, height / 2);
+ ctx.moveTo(x + r, y);
+ ctx.lineTo(x + width - r, y);
+ ctx.quadraticCurveTo(x + width, y, x + width, y + r);
+ ctx.lineTo(x + width, y + height - r);
+ ctx.quadraticCurveTo(x + width, y + height, x + width - r, y + height);
+ ctx.lineTo(x + r, y + height);
+ ctx.quadraticCurveTo(x, y + height, x, y + height - r);
+ ctx.lineTo(x, y + r);
+ ctx.quadraticCurveTo(x, y, x + r, y);
+ ctx.closePath();
}
ctx.clip();🤖 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 333 - 341, The fallback
using ctx.rect produces sharp corners; replace it with a manual rounded-rect
path when ctx.roundRect is missing: in annotationRenderer.ts where radius, x, y,
width, height and ctx are used (the block that currently checks if
(ctx.roundRect) ... else ctx.rect(...); ctx.clip()), implement a path that moves
to the top-left+radius, draws lines and quadratic/arc corners (or arcTo) to
create rounded corners using the same radius*scaleFactor and then call
ctx.clip(); keep the existing branch for ctx.roundRect but ensure the manual
path produces the same rounded blur region as the UI.
… introduce video editor UI components.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/components/video-editor/SettingsPanel.tsx (2)
1532-1552: Add accessible label to the cue text editor.The textarea for editing caption text lacks an accessible name, which makes it difficult for screen reader users to understand its purpose.
♿ Proposed fix
<textarea value={cue.text} + aria-label={`${tSettings("captions.editCueText", "Edit caption text")} ${index + 1}`} onChange={(e) => { const newCaptions = [...autoCaptions]; newCaptions[index] = { ...cue, text: e.target.value }; onAutoCaptionsChange?.(newCaptions); }}🤖 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 1532 - 1552, The caption textarea in SettingsPanel (the element bound to cue.text with onChange updating autoCaptions via onAutoCaptionsChange) lacks an accessible name; add an accessible label by supplying either an aria-label or aria-labelledby that uniquely identifies the field (for example "Caption text X" where X is index+1) so screen readers can announce which cue is being edited, or render a visually-hidden <label> and reference it with aria-labelledby; keep the existing handlers (onChange, onInput, ref) unchanged while adding the aria attribute to the textarea to ensure accessibility.
1471-1491: Improve button disabled state to prevent clicks before model selection.The button is disabled only during generation (
isGeneratingCaptions), allowing users to click "Generate Captions" without a selected model. While the handler catches this and shows an error toast (line 1558-1561 in VideoEditor.tsx), it's better to prevent the action at the UI level.Also consider preventing clicks while a model is downloading.
Suggested improvement:
🛡️ Better disable logic
+ const canGenerateCaptions = Boolean(whisperModelPath) && whisperModelDownloadStatus !== "downloading"; <Button type="button" onClick={onGenerateAutoCaptions} - disabled={isGeneratingCaptions} + disabled={isGeneratingCaptions || !canGenerateCaptions} className="relative h-10 w-full overflow-hidden rounded-xl bg-[`#2563EB`] px-4 text-sm font-medium text-white hover:bg-[`#2563EB`]/90 disabled:bg-[`#2563EB`]/50" >🤖 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 1471 - 1491, The Generate Captions button currently only disables during generation (isGeneratingCaptions) which still allows clicks when no model is selected or while a model is downloading; update the disabled condition on the Button that wraps onGenerateAutoCaptions to also check model selection and download state (e.g., disable when !selectedModelId or !selectedModel or isModelDownloading in addition to isGeneratingCaptions) so clicks are prevented before a valid model is ready; ensure the same variables referenced in the handler (onGenerateAutoCaptions) are used to keep UI and logic consistent and optionally reflect the state in the button label/tooltip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron-builder.json5`:
- Line 68: The CI will break because electron-builder's artifactName was changed
to "${productName}-Setup-${version}.${ext}" (artifactName) but the release
workflow (.github/workflows/release.yml) still expects fixed filenames (e.g.,
Rename-Item -Path release/Recordly.exe and uploads expecting
release/Recordly-windows-x64.exe and release-assets/Recordly-windows-x64.exe);
update the workflow to match the new naming either by using a glob or computed
name when renaming/uploading Windows artifacts (e.g., replace fixed
"release/Recordly.exe" and upload paths with a pattern like
"release/Recordly-Setup-*.exe" and ensure final uploaded name
"Recordly-windows-x64.exe"), or revert artifactName to the previous fixed name
to keep current workflow unchanged.
In `@electron/ipc/handlers.ts`:
- Around line 1052-1067: getWhisperModelStatus is swallowing all failures as
"exists: false"; change it so errors from getWhisperModelPath are treated as
invalid/unsupported input (return success: false and surface the error), while
filesystem failures are distinguished: if fs.access throws ENOENT return {
success: true, exists: false, path: null }, otherwise return { success: false,
error: <error message> }. Update the try/catch to call getWhisperModelPath
outside the fs.access try, catch and inspect error.code on fs.access (use
fsConstants.R_OK) and include identifying symbols getWhisperModelStatus,
getWhisperModelPath, fs.access, and fsConstants.R_OK when locating the code to
implement these branches.
- Around line 1154-1165: The caller is renaming tempPath to modelPath
immediately after downloadFileWithProgress resolves, but that function currently
resolves on the write stream's 'finish' which doesn't guarantee the file handle
is closed on Windows; update the code so downloadFileWithProgress only resolves
when the write stream emits 'close' (or await a promise that listens for both
'finish' and 'close'), or alternatively add explicit waiting here after
downloadFileWithProgress for the stream to close before calling fs.rename;
change the downloadFileWithProgress implementation (or its return) to ensure the
file descriptor is released, then proceed with fs.rename(tempPath, modelPath)
and continue using sendWhisperModelDownloadProgress as before.
- Around line 3195-3205: The display bounds are currently taken from Electron in
DIP but the native code expects device pixels; update the block that computes
config.displayX/Y/W/H (using getScreen().getAllDisplays(), the found display and
display.bounds) to multiply bounds.x, bounds.y, bounds.width and bounds.height
by the display.scaleFactor (falling back to a sensible default if missing) and
then Math.round the results before assigning to config.displayX,
config.displayY, config.displayW and config.displayH so the values sent to the
native monitor matcher are in device pixels.
- Around line 1039-1050: The sendWhisperModelDownloadProgress helper and the
progress callbacks that call webContents.send must be guarded to avoid "Object
has been destroyed" crashes: wrap each webContents.send(...) invocation in a
check that webContents.isDestroyed() is false (or early-return if destroyed).
Specifically, add the guard inside sendWhisperModelDownloadProgress (the
function that sends "whisper-model-download-progress") and also apply the same
check before calling send in the progress callbacks inside downloadWhisperModel
and generateAutoCaptionsFromVideo so they no-op when the target WebContents has
been destroyed.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1455-1458: The JSX in SettingsPanel currently renders a hardcoded
English string "No local model selected"; replace that literal with the i18n
helper (tSettings) used elsewhere in the component (e.g., call
tSettings('noLocalModelSelected') or the appropriate key) in the same JSX branch
inside SettingsPanel, and add the corresponding key/value to the translation
resources for all supported locales so the message is localized.
- Around line 1391-1393: The onValueChange callback currently uses a broad cast
"as any" which bypasses TypeScript checks; replace it with the proper union
model type from WhisperModelInfo by importing WhisperModelInfo and typing the
value parameter (e.g. onValueChange={(value: WhisperModelInfo['model']) =>
updateAutoCaptionSettings({ selectedModel: value })}) or cast to
WhisperModelInfo['model'] instead of any; ensure
autoCaptionSettings.selectedModel and updateAutoCaptionSettings expect that same
WhisperModelInfo model union so types align.
- Around line 1502-1531: The map over autoCaptions uses key={cue.id || index},
which causes React to reuse DOM nodes when an item is deleted; replace the index
fallback with a stable unique identifier (e.g., ensure each cue has an id when
created or derive a stable composite key like `${cue.startMs}-${cue.endMs}`) and
update any creation logic so new cues get a persistent id; also add an
accessible name to the delete button (e.g., aria-label or title) in the
component that renders the Trash2 button so screen readers can identify the
action.
---
Nitpick comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1532-1552: The caption textarea in SettingsPanel (the element
bound to cue.text with onChange updating autoCaptions via onAutoCaptionsChange)
lacks an accessible name; add an accessible label by supplying either an
aria-label or aria-labelledby that uniquely identifies the field (for example
"Caption text X" where X is index+1) so screen readers can announce which cue is
being edited, or render a visually-hidden <label> and reference it with
aria-labelledby; keep the existing handlers (onChange, onInput, ref) unchanged
while adding the aria attribute to the textarea to ensure accessibility.
- Around line 1471-1491: The Generate Captions button currently only disables
during generation (isGeneratingCaptions) which still allows clicks when no model
is selected or while a model is downloading; update the disabled condition on
the Button that wraps onGenerateAutoCaptions to also check model selection and
download state (e.g., disable when !selectedModelId or !selectedModel or
isModelDownloading in addition to isGeneratingCaptions) so clicks are prevented
before a valid model is ready; ensure the same variables referenced in the
handler (onGenerateAutoCaptions) are used to keep UI and logic consistent and
optionally reflect the state in the button label/tooltip.
🪄 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: e6a3a9ee-ff0d-4929-b173-82b55b703856
📒 Files selected for processing (10)
electron-builder.json5electron/electron-env.d.tselectron/ipc/handlers.tselectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/monitor_utils.cppelectron/native/wgc-capture/src/monitor_utils.helectron/preload.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/video-editor/types.ts
- electron/preload.ts
- electron/electron-env.d.ts
electron-builder.json5
Outdated
| "artifactName": "Recordly.exe" | ||
| "icon": "icons/icons/win/icon.ico", | ||
| "executableName": "Recordly", | ||
| "artifactName": "${productName}-Setup-${version}.${ext}" |
There was a problem hiding this comment.
Breaking change: artifactName update will cause CI/CD workflow failures.
The new artifact naming template ${productName}-Setup-${version}.${ext} will produce files like Recordly-Setup-1.0.0.exe, but the release workflow in .github/workflows/release.yml expects the old fixed name Recordly.exe:
- Line 204:
Rename-Item -Path release/Recordly.exewill fail becauserelease/Recordly.exewon't exist. - Lines 206-211: The artifact upload expects
release/Recordly-windows-x64.exe, which won't be created. - Lines 298-303: The release step expects
release-assets/Recordly-windows-x64.exe.
You need to update the workflow to match the new naming convention. For example:
# In .github/workflows/release.yml, update the Windows build step:
# Replace the fixed filename with a glob or computed name
Rename-Item -Path "release/Recordly-Setup-*.exe" -NewName "Recordly-windows-x64.exe"Alternatively, consider keeping the artifact name consistent with other platforms (macOS uses Recordly.dmg, Linux uses Recordly.AppImage) by reverting to a fixed name:
- "artifactName": "${productName}-Setup-${version}.${ext}"
+ "artifactName": "Recordly.exe"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron-builder.json5` at line 68, The CI will break because
electron-builder's artifactName was changed to
"${productName}-Setup-${version}.${ext}" (artifactName) but the release workflow
(.github/workflows/release.yml) still expects fixed filenames (e.g., Rename-Item
-Path release/Recordly.exe and uploads expecting
release/Recordly-windows-x64.exe and release-assets/Recordly-windows-x64.exe);
update the workflow to match the new naming either by using a glob or computed
name when renaming/uploading Windows artifacts (e.g., replace fixed
"release/Recordly.exe" and upload paths with a pattern like
"release/Recordly-Setup-*.exe" and ensure final uploaded name
"Recordly-windows-x64.exe"), or revert artifactName to the previous fixed name
to keep current workflow unchanged.
Comment on lines
+1039
to
+1050
| function sendWhisperModelDownloadProgress( | ||
| webContents: Electron.WebContents, | ||
| payload: { | ||
| status: "idle" | "downloading" | "downloaded" | "error"; | ||
| progress: number; | ||
| model: string; | ||
| path?: string | null; | ||
| error?: string; | ||
| }, | ||
| ) { | ||
| webContents.send("whisper-model-download-progress", payload); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context around the flagged lines
cd electron/ipc && wc -l handlers.tsRepository: webadderall/Recordly
Length of output: 80
🏁 Script executed:
# Read the function at lines 1039-1050 and surrounding context
sed -n '1030,1060p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 687
🏁 Script executed:
# Read the second location at lines 1566-1580 and surrounding context
sed -n '1560,1590p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 1251
🏁 Script executed:
# Search for other webContents.send calls in the handlers file to understand the pattern
rg "webContents\.send" electron/ipc/handlers.ts -n -A 2 -B 2Repository: webadderall/Recordly
Length of output: 2496
🏁 Script executed:
# Check if there are any existing guard patterns for webContents in the codebase
rg "isDestroyed|\.destroyed" electron/ipc/ -nRepository: webadderall/Recordly
Length of output: 610
🏁 Script executed:
# Look for any event listener cleanup patterns when dealing with progress/streams
rg "removeListener|off\(" electron/ipc/handlers.ts -n -B 2 -A 2Repository: webadderall/Recordly
Length of output: 1557
🏁 Script executed:
# Find calls to sendWhisperModelDownloadProgress
rg "sendWhisperModelDownloadProgress" electron/ipc/handlers.ts -n -B 3 -A 3Repository: webadderall/Recordly
Length of output: 1813
🏁 Script executed:
# Find the downloadWhisperModel function and see how it uses the progress callback
rg "downloadWhisperModel|downloadFileWithProgress" electron/ipc/handlers.ts -n -B 2 -A 5 | head -80Repository: webadderall/Recordly
Length of output: 1180
🏁 Script executed:
# Find where generateAutoCaptionsFromVideo is called from
rg "generateAutoCaptionsFromVideo" electron/ipc/handlers.ts -n -B 2 -A 2Repository: webadderall/Recordly
Length of output: 351
🏁 Script executed:
# Look at the actual downloadFileWithProgress to understand the callback pattern
rg "function downloadFileWithProgress|const downloadFileWithProgress" electron/ipc/ -n -A 15Repository: webadderall/Recordly
Length of output: 1197
🏁 Script executed:
# Check what happens when send is called - look for any try/catch around webContents.send in downloadFileWithProgress context
sed -n '1070,1130p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 1948
🏁 Script executed:
# Check what happens in downloadWhisperModel around the callback
sed -n '1134,1185p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 1383
🏁 Script executed:
# Check IPC handler wrapper - how is downloadWhisperModel called and what happens on error
sed -n '4108,4130p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 824
🏁 Script executed:
# Check if there's any existing error handling or try-catch when using webContents that was destroyed
rg "try.*webContents|webContents.*catch" electron/ipc/handlers.ts -A 3 -B 3Repository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
# Check generateAutoCaptionsFromVideo and its progress callbacks around lines 1566-1580
sed -n '1514,1590p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 2957
🏁 Script executed:
# Look for how generateAutoCaptionsFromVideo is called and what webContents is
sed -n '4150,4170p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 757
🏁 Script executed:
# Search Electron documentation behavior - verify what happens when webContents.send is called on destroyed context
web_search
Electron WebContents.send() destroyed error behaviorRepository: webadderall/Recordly
Length of output: 252
🌐 Web query:
Electron WebContents.send() on destroyed window throws error or fails silently
💡 Result:
Yes, calling webContents.send on a destroyed WebContents throws an "Object has been destroyed" error. This is a well-documented behavior in Electron across multiple versions. To prevent the error, always check webContents.isDestroyed before calling send: if (!win.webContents.isDestroyed) { win.webContents.send('channel', data); } This check is explicitly recommended in the official Electron documentation, which provides the isDestroyed method to determine if the web page is destroyed. Multiple GitHub issues and Stack Overflow reports confirm that attempting to send IPC messages after destruction (e.g., after window close) triggers this error, and the standard solution is the isDestroyed check. The official docs do not indicate that send fails silently; it throws an error, though some related serialization errors may log to console without throwing in specific cases (unrelated to destruction).
Citations:
- 1: https://stackoverflow.com/questions/76011371/object-has-been-destroyed-at-win-webcontents-send-electron-node-pty
- 2: "Object has been destroyed" error electron/electron#4279
- 3: https://stackoverflow.com/questions/55395748/object-has-been-destroyed-when-sending-through-ipc
- 4: https://electronjs.org/docs/latest/api/web-contents
- 5: https://www.electronjs.org/docs/latest/api/web-contents
Guard webContents.send() calls in progress callbacks to prevent main-process crashes.
These functions call webContents.send() from asynchronous progress callbacks without checking if the WebContents is destroyed. If the user closes the window during a long download or caption operation, Electron will throw "Object has been destroyed" when the callback fires, crashing the main process if the exception is unhandled. Add !webContents.isDestroyed() checks before each send call. The codebase already uses this pattern elsewhere (see lines 90, 1891, 2165, 2209); apply the same guard here.
Locations:
- Line 1049:
sendWhisperModelDownloadProgress()helper function - Lines 1157, 1165, 1174: progress callbacks in
downloadWhisperModel() - Lines 1567, 1579: progress callbacks in
generateAutoCaptionsFromVideo()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 1039 - 1050, The
sendWhisperModelDownloadProgress helper and the progress callbacks that call
webContents.send must be guarded to avoid "Object has been destroyed" crashes:
wrap each webContents.send(...) invocation in a check that
webContents.isDestroyed() is false (or early-return if destroyed). Specifically,
add the guard inside sendWhisperModelDownloadProgress (the function that sends
"whisper-model-download-progress") and also apply the same check before calling
send in the progress callbacks inside downloadWhisperModel and
generateAutoCaptionsFromVideo so they no-op when the target WebContents has been
destroyed.
Comment on lines
+1052
to
+1067
| async function getWhisperModelStatus(_event: any, modelName: string) { | ||
| try { | ||
| const modelPath = getWhisperModelPath(modelName); | ||
| await fs.access(modelPath, fsConstants.R_OK); | ||
| return { | ||
| success: true, | ||
| exists: true, | ||
| path: modelPath, | ||
| }; | ||
| } catch { | ||
| return { | ||
| success: true, | ||
| exists: false, | ||
| path: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Don't swallow unsupported models as “not downloaded.”
This helper currently converts every failure into { success: true, exists: false }, including unknown model names from getWhisperModelPath(...) and non-missing filesystem errors. That makes corrupted preferences or bad IPC input look like a missing file and leaves the renderer in a misleading idle state.
🧰 Tools
🪛 Biome (2.4.9)
[error] 1052-1052: 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 `@electron/ipc/handlers.ts` around lines 1052 - 1067, getWhisperModelStatus is
swallowing all failures as "exists: false"; change it so errors from
getWhisperModelPath are treated as invalid/unsupported input (return success:
false and surface the error), while filesystem failures are distinguished: if
fs.access throws ENOENT return { success: true, exists: false, path: null },
otherwise return { success: false, error: <error message> }. Update the
try/catch to call getWhisperModelPath outside the fs.access try, catch and
inspect error.code on fs.access (use fsConstants.R_OK) and include identifying
symbols getWhisperModelStatus, getWhisperModelPath, fs.access, and
fsConstants.R_OK when locating the code to implement these branches.
electron/ipc/handlers.ts
Outdated
Comment on lines
+1154
to
+1165
| try { | ||
| await fs.rm(tempPath, { force: true }); | ||
| await downloadFileWithProgress(model.url, tempPath, (progress) => { | ||
| sendWhisperModelDownloadProgress(webContents, { | ||
| status: "downloading", | ||
| progress, | ||
| model: modelName, | ||
| path: null, | ||
| }); | ||
| }); | ||
| await fs.rename(tempPath, modelPath); | ||
| sendWhisperModelDownloadProgress(webContents, { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and examine the context around lines 1154-1165
fd handlers.ts | head -5Repository: webadderall/Recordly
Length of output: 152
🏁 Script executed:
# Read the relevant section of the file
wc -l electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 93
🏁 Script executed:
# Get the context around the suspicious code
sed -n '1140,1180p' electron/ipc/handlers.ts | cat -nRepository: webadderall/Recordly
Length of output: 1396
🏁 Script executed:
# Find the downloadFileWithProgress function definition
rg -n "downloadFileWithProgress" --type tsRepository: webadderall/Recordly
Length of output: 228
🏁 Script executed:
# Get the downloadFileWithProgress function starting at line 1070
sed -n '1070,1120p' electron/ipc/handlers.ts | cat -nRepository: webadderall/Recordly
Length of output: 2187
🏁 Script executed:
# Get the rest of the function
sed -n '1120,1135p' electron/ipc/handlers.ts | cat -nRepository: webadderall/Recordly
Length of output: 417
Wait for the file handle to close before renaming.
The downloadFileWithProgress function resolves on the 'finish' event of the write stream (line 1050), but this does not guarantee the underlying file descriptor is closed. On Windows, NTFS file locking prevents rename operations on files with open handles, causing fs.rename to fail with EPERM or EBUSY errors, especially for large files. The fix is to wait for the 'close' event in addition to 'finish', or restructure the stream handling to ensure the handle is fully released before the promise resolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 1154 - 1165, The caller is renaming
tempPath to modelPath immediately after downloadFileWithProgress resolves, but
that function currently resolves on the write stream's 'finish' which doesn't
guarantee the file handle is closed on Windows; update the code so
downloadFileWithProgress only resolves when the write stream emits 'close' (or
await a promise that listens for both 'finish' and 'close'), or alternatively
add explicit waiting here after downloadFileWithProgress for the stream to close
before calling fs.rename; change the downloadFileWithProgress implementation (or
its return) to ensure the file descriptor is released, then proceed with
fs.rename(tempPath, modelPath) and continue using
sendWhisperModelDownloadProgress as before.
Comment on lines
+3195
to
+3205
| // Include display bounds for more robust matching in the native helper | ||
| const allDisplays = getScreen().getAllDisplays() | ||
| const display = allDisplays.find((d) => String(d.id) === String(config.displayId)) | ||
| || getScreen().getPrimaryDisplay() | ||
|
|
||
| if (display) { | ||
| config.displayX = Math.round(display.bounds.x) | ||
| config.displayY = Math.round(display.bounds.y) | ||
| config.displayW = Math.round(display.bounds.width) | ||
| config.displayH = Math.round(display.bounds.height) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the context around the flagged lines
head -n 3210 electron/ipc/handlers.ts | tail -n 30Repository: webadderall/Recordly
Length of output: 1266
🏁 Script executed:
# Search for GetMonitorInfoW usage to understand the native code comparison
rg "GetMonitorInfoW" --type cppRepository: webadderall/Recordly
Length of output: 246
🏁 Script executed:
# Look for DIP-related code or coordinate conversion in the codebase
rg "DIP|device.*pixel|coordinate.*convert" --type cpp --type ts -i -A 2 -B 2Repository: webadderall/Recordly
Length of output: 3670
🏁 Script executed:
# Check Electron's screen.getAllDisplays documentation or type definitions
fd "screen" --type f | grep -E "\.(d\.ts|ts)$" | head -20Repository: webadderall/Recordly
Length of output: 130
🏁 Script executed:
# Search for any existing DIP to device pixel conversion patterns
rg "ScaleFactorForView|dpi.*convert|bounds.*device" --type cpp -A 2 -B 2Repository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
# Examine the monitor_utils.cpp file to understand how the bounds comparison works
cat -n electron/native/wgc-capture/src/monitor_utils.cpp | head -150Repository: webadderall/Recordly
Length of output: 5804
🏁 Script executed:
# Search for how monitor matching is done - what exactly is being compared
rg "displayId|displayX|displayY|displayW|displayH" -A 5 -B 5 electron/native/Repository: webadderall/Recordly
Length of output: 13011
🏁 Script executed:
# Look for any scale/DIP conversion in the native code
rg "scale.*factor|device.*pixel|dpi.*convert" electron/native/ --type cpp -i -A 2 -B 2Repository: webadderall/Recordly
Length of output: 46
🌐 Web query:
Electron screen.getAllDisplays() bounds units DIP or device pixels
💡 Result:
The bounds returned by Electron's screen.getAllDisplays are in device-independent pixels (DIP), not raw device pixels.
Citations:
Convert Electron display bounds from DIP to device pixels before sending to native code.
screen.getAllDisplays().bounds returns coordinates in device-independent pixels (DIP), but the native monitor matching uses GetMonitorInfoW which returns device pixels. On mixed-DPI multi-monitor setups, comparing DIP coordinates directly against device pixel rectangles can fail to match the correct monitor, potentially selecting a different one and recording the wrong screen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 3195 - 3205, The display bounds are
currently taken from Electron in DIP but the native code expects device pixels;
update the block that computes config.displayX/Y/W/H (using
getScreen().getAllDisplays(), the found display and display.bounds) to multiply
bounds.x, bounds.y, bounds.width and bounds.height by the display.scaleFactor
(falling back to a sensible default if missing) and then Math.round the results
before assigning to config.displayX, config.displayY, config.displayW and
config.displayH so the values sent to the native monitor matcher are in device
pixels.
Comment on lines
+1391
to
+1393
| <Select | ||
| value={autoCaptionSettings.selectedModel || "small"} | ||
| onValueChange={(value) => updateAutoCaptionSettings({ selectedModel: value as any })} |
There was a problem hiding this comment.
Replace any with proper type for model selection.
The as any cast bypasses TypeScript's type checking. Use the defined union type from WhisperModelInfo instead.
🔧 Proposed fix
<Select
value={autoCaptionSettings.selectedModel || "small"}
- onValueChange={(value) => updateAutoCaptionSettings({ selectedModel: value as any })}
+ onValueChange={(value) => updateAutoCaptionSettings({ selectedModel: value as WhisperModelInfo["value"] })}
>📝 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.
Suggested change
| <Select | |
| value={autoCaptionSettings.selectedModel || "small"} | |
| onValueChange={(value) => updateAutoCaptionSettings({ selectedModel: value as any })} | |
| <Select | |
| value={autoCaptionSettings.selectedModel || "small"} | |
| onValueChange={(value) => updateAutoCaptionSettings({ selectedModel: value as WhisperModelInfo["value"] })} |
🧰 Tools
🪛 Biome (2.4.9)
[error] 1393-1393: 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/components/video-editor/SettingsPanel.tsx` around lines 1391 - 1393, The
onValueChange callback currently uses a broad cast "as any" which bypasses
TypeScript checks; replace it with the proper union model type from
WhisperModelInfo by importing WhisperModelInfo and typing the value parameter
(e.g. onValueChange={(value: WhisperModelInfo['model']) =>
updateAutoCaptionSettings({ selectedModel: value })}) or cast to
WhisperModelInfo['model'] instead of any; ensure
autoCaptionSettings.selectedModel and updateAutoCaptionSettings expect that same
WhisperModelInfo model union so types align.
Comment on lines
+1455
to
+1458
| ) : ( | ||
| <div className="flex h-10 w-full items-center justify-center rounded-xl bg-white/5 px-4 text-[10px] text-slate-500 italic"> | ||
| No local model selected | ||
| </div> |
There was a problem hiding this comment.
Use i18n for user-facing text.
The string "No local model selected" is hardcoded in English while the rest of the component uses the tSettings() function for internationalization.
🌐 Proposed fix
) : (
<div className="flex h-10 w-full items-center justify-center rounded-xl bg-white/5 px-4 text-[10px] text-slate-500 italic">
- No local model selected
+ {tSettings("captions.noLocalModelSelected", "No local model selected")}
</div>
)}📝 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.
Suggested change
| ) : ( | |
| <div className="flex h-10 w-full items-center justify-center rounded-xl bg-white/5 px-4 text-[10px] text-slate-500 italic"> | |
| No local model selected | |
| </div> | |
| ) : ( | |
| <div className="flex h-10 w-full items-center justify-center rounded-xl bg-white/5 px-4 text-[10px] text-slate-500 italic"> | |
| {tSettings("captions.noLocalModelSelected", "No local model selected")} | |
| </div> | |
| )} |
🤖 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 1455 - 1458, The
JSX in SettingsPanel currently renders a hardcoded English string "No local
model selected"; replace that literal with the i18n helper (tSettings) used
elsewhere in the component (e.g., call tSettings('noLocalModelSelected') or the
appropriate key) in the same JSX branch inside SettingsPanel, and add the
corresponding key/value to the translation resources for all supported locales
so the message is localized.
Comment on lines
+1502
to
+1531
| {autoCaptions.map((cue, index) => { | ||
| const isActive = currentTime >= cue.startMs / 1000 && currentTime <= cue.endMs / 1000; | ||
| return ( | ||
| <div | ||
| key={cue.id || index} | ||
| className={cn( | ||
| "group flex flex-col gap-1 rounded-lg p-2 transition-colors", | ||
| isActive ? "bg-white/10" : "hover:bg-white/5" | ||
| )} | ||
| > | ||
| <div className="flex items-center justify-between"> | ||
| <button | ||
| type="button" | ||
| onClick={() => onSeek?.(cue.startMs / 1000)} | ||
| className="text-[10px] font-medium text-slate-500 hover:text-[#2563EB]" | ||
| > | ||
| {(cue.startMs / 1000).toFixed(2)}s – {(cue.endMs / 1000).toFixed(2)}s | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| const newCaptions = [...autoCaptions]; | ||
| newCaptions.splice(index, 1); | ||
| onAutoCaptionsChange?.(newCaptions); | ||
| }} | ||
| className="opacity-0 group-hover:opacity-100 transition-opacity p-1 text-slate-500 hover:text-red-400" | ||
| > | ||
| <Trash2 className="h-3 w-3" /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Using array index as key fallback can cause rendering issues when deleting cues.
When cue.id is undefined, the fallback to index causes React to incorrectly reuse DOM elements after deletions. For example, deleting cue at index 1 shifts all subsequent keys, potentially causing the wrong textarea content to appear in each row.
Additionally, the delete button lacks an accessible name for screen readers.
♻️ Proposed fix
{autoCaptions.map((cue, index) => {
const isActive = currentTime >= cue.startMs / 1000 && currentTime <= cue.endMs / 1000;
return (
<div
- key={cue.id || index}
+ key={cue.id ?? `cue-${cue.startMs}-${cue.endMs}`}
className={cn(
"group flex flex-col gap-1 rounded-lg p-2 transition-colors",
isActive ? "bg-white/10" : "hover:bg-white/5"
)}
>
<div className="flex items-center justify-between">
<button
type="button"
onClick={() => onSeek?.(cue.startMs / 1000)}
className="text-[10px] font-medium text-slate-500 hover:text-[`#2563EB`]"
>
{(cue.startMs / 1000).toFixed(2)}s – {(cue.endMs / 1000).toFixed(2)}s
</button>
<button
type="button"
+ aria-label={tSettings("captions.deleteCue", "Delete cue")}
onClick={() => {
const newCaptions = [...autoCaptions];
newCaptions.splice(index, 1);
onAutoCaptionsChange?.(newCaptions);
}}
className="opacity-0 group-hover:opacity-100 transition-opacity p-1 text-slate-500 hover:text-red-400"
>
<Trash2 className="h-3 w-3" />
</button>
</div>📝 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.
Suggested change
| {autoCaptions.map((cue, index) => { | |
| const isActive = currentTime >= cue.startMs / 1000 && currentTime <= cue.endMs / 1000; | |
| return ( | |
| <div | |
| key={cue.id || index} | |
| className={cn( | |
| "group flex flex-col gap-1 rounded-lg p-2 transition-colors", | |
| isActive ? "bg-white/10" : "hover:bg-white/5" | |
| )} | |
| > | |
| <div className="flex items-center justify-between"> | |
| <button | |
| type="button" | |
| onClick={() => onSeek?.(cue.startMs / 1000)} | |
| className="text-[10px] font-medium text-slate-500 hover:text-[#2563EB]" | |
| > | |
| {(cue.startMs / 1000).toFixed(2)}s – {(cue.endMs / 1000).toFixed(2)}s | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => { | |
| const newCaptions = [...autoCaptions]; | |
| newCaptions.splice(index, 1); | |
| onAutoCaptionsChange?.(newCaptions); | |
| }} | |
| className="opacity-0 group-hover:opacity-100 transition-opacity p-1 text-slate-500 hover:text-red-400" | |
| > | |
| <Trash2 className="h-3 w-3" /> | |
| </button> | |
| </div> | |
| {autoCaptions.map((cue, index) => { | |
| const isActive = currentTime >= cue.startMs / 1000 && currentTime <= cue.endMs / 1000; | |
| return ( | |
| <div | |
| key={cue.id ?? `cue-${cue.startMs}-${cue.endMs}`} | |
| className={cn( | |
| "group flex flex-col gap-1 rounded-lg p-2 transition-colors", | |
| isActive ? "bg-white/10" : "hover:bg-white/5" | |
| )} | |
| > | |
| <div className="flex items-center justify-between"> | |
| <button | |
| type="button" | |
| onClick={() => onSeek?.(cue.startMs / 1000)} | |
| className="text-[10px] font-medium text-slate-500 hover:text-[`#2563EB`]" | |
| > | |
| {(cue.startMs / 1000).toFixed(2)}s – {(cue.endMs / 1000).toFixed(2)}s | |
| </button> | |
| <button | |
| type="button" | |
| aria-label={tSettings("captions.deleteCue", "Delete cue")} | |
| onClick={() => { | |
| const newCaptions = [...autoCaptions]; | |
| newCaptions.splice(index, 1); | |
| onAutoCaptionsChange?.(newCaptions); | |
| }} | |
| className="opacity-0 group-hover:opacity-100 transition-opacity p-1 text-slate-500 hover:text-red-400" | |
| > | |
| <Trash2 className="h-3 w-3" /> | |
| </button> | |
| </div> |
🤖 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 1502 - 1531, The
map over autoCaptions uses key={cue.id || index}, which causes React to reuse
DOM nodes when an item is deleted; replace the index fallback with a stable
unique identifier (e.g., ensure each cue has an id when created or derive a
stable composite key like `${cue.startMs}-${cue.endMs}`) and update any creation
logic so new cues get a persistent id; also add an accessible name to the delete
button (e.g., aria-label or title) in the component that renders the Trash2
button so screen readers can identify the action.
There was a problem hiding this comment.
Actionable comments posted: 4
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/SettingsPanel.tsx (1)
1412-1459:⚠️ Potential issue | 🟠 MajorDon’t route custom models through the built-in delete action.
Once a local file is picked, this branch still flips to the generic
Delete Modelbutton as soon aswhisperModelPathis truthy.VideoEditor.handleDeleteWhisperModel()deletes by model ID, so this click becomes a backend delete for"custom"instead of just clearing the selected local path.🤖 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 1412 - 1459, The UI currently shows the generic "Delete Model" button whenever whisperModelPath is truthy, which causes onDeleteWhisperModel (and ultimately VideoEditor.handleDeleteWhisperModel) to be called for custom/local files; instead, change the rendering logic so that when whisperModelPath is truthy AND autoCaptionSettings.selectedModel === "custom" you do NOT render the backend delete button but render a local-clear action; specifically, update the conditional around whisperModelPath to: if whisperModelPath && autoCaptionSettings.selectedModel !== "custom" keep the existing onDeleteWhisperModel button, and add a new branch for whisperModelPath && autoCaptionSettings.selectedModel === "custom" that shows a "Clear Selection" (or similar) button wired to a new onClearWhisperModelSelection handler which only clears the local whisperModelPath/selection in state (do not call handleDeleteWhisperModel), leaving handleDeleteWhisperModel/onDeleteWhisperModel reserved for deleting server-side models by ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 837-843: hasAnyTimelineBlocks now treats autoCaptions as part of
"select all" but deleteAllBlocks() doesn't remove them, causing selected
captions to remain; update deleteAllBlocks to also clear autoCaptions (and any
caption-related state/dispatch used in TimelineEditor) so that the bulk delete
removes caption blocks, or alternatively remove autoCaptions from
hasAnyTimelineBlocks to keep behavior consistent—locate the hasAnyTimelineBlocks
computation and the deleteAllBlocks function and ensure autoCaptions is handled
symmetrically in both places.
- Around line 1450-1457: allRegionSpans currently includes captions so
TimelineWrapper.clampToNeighbours() will treat caption boundaries as hard stops;
remove autoCaptions from the array used for clamping by excluding captions from
allRegionSpans (or create a separate clampedRegionSpans without autoCaptions) so
caption spans are not considered when clamping resize/move operations; update
the useMemo that builds allRegionSpans (and its dependency array) to omit
autoCaptions and ensure hasOverlap() still allows captions to overlap as
intended.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1569-1574: Remove the production console debug statements in
VideoEditor.handleGenerateAutoCaptions that print sensitive filesystem paths and
generation results (specifically the console.log that outputs sourcePath,
whisperExecutablePath, whisperModelPath, and autoCaptionSettings.language and
the subsequent logs around the generation result). Instead either delete those
console.log calls or replace them with non-sensitive, environment-gated debug
logging (e.g., a debug logger that only runs in dev builds or logs only
high-level status messages), ensuring handleGenerateAutoCaptions no longer emits
raw paths or content to the renderer console.
- Around line 382-386: selectedCaptionId is managed directly and its setter
(setSelectedCaptionId) is passed raw to children and not reconciled when the
captions list is replaced, causing non-exclusive selections and stale sidebar
focus; fix by centralizing caption selection lifecycle like other tracks: update
the caption regeneration path (the function that replaces the cues list) to
revalidate/clear selectedCaptionId when cues change, and replace raw setter
usage with a wrapped setter that enforces exclusivity (clears zoom/trim/audio
selection and validates existence in the new cues list) before setting; ensure
places referenced in the diff such as selectedCaptionId, setSelectedCaptionId,
and the caption-regeneration codepaths are updated so rerunning captions clears
or remaps selection to a valid cue.
---
Outside diff comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1412-1459: The UI currently shows the generic "Delete Model"
button whenever whisperModelPath is truthy, which causes onDeleteWhisperModel
(and ultimately VideoEditor.handleDeleteWhisperModel) to be called for
custom/local files; instead, change the rendering logic so that when
whisperModelPath is truthy AND autoCaptionSettings.selectedModel === "custom"
you do NOT render the backend delete button but render a local-clear action;
specifically, update the conditional around whisperModelPath to: if
whisperModelPath && autoCaptionSettings.selectedModel !== "custom" keep the
existing onDeleteWhisperModel button, and add a new branch for whisperModelPath
&& autoCaptionSettings.selectedModel === "custom" that shows a "Clear Selection"
(or similar) button wired to a new onClearWhisperModelSelection handler which
only clears the local whisperModelPath/selection in state (do not call
handleDeleteWhisperModel), leaving handleDeleteWhisperModel/onDeleteWhisperModel
reserved for deleting server-side models by ID.
🪄 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: 41bff4fc-27e2-4c4b-9149-e849dcdf8813
📒 Files selected for processing (6)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/TimelineWrapper.tsx
Comment on lines
837
to
+843
| const hasAnyTimelineBlocks = | ||
| zoomRegions.length > 0 || | ||
| trimRegions.length > 0 || | ||
| annotationRegions.length > 0 || | ||
| speedRegions.length > 0 || | ||
| audioRegions.length > 0; | ||
| audioRegions.length > 0 || | ||
| autoCaptions.length > 0; |
There was a problem hiding this comment.
Select-all now includes captions, but bulk delete still can’t remove them.
With autoCaptions.length > 0 in hasAnyTimelineBlocks, the Cmd/Ctrl+A flow will now treat caption blocks as part of “all”, yet deleteAllBlocks() still only clears zoom/trim/annotation/speed/audio. The UI ends up highlighting captions as selected and then leaving them behind when Delete runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 837 -
843, hasAnyTimelineBlocks now treats autoCaptions as part of "select all" but
deleteAllBlocks() doesn't remove them, causing selected captions to remain;
update deleteAllBlocks to also clear autoCaptions (and any caption-related
state/dispatch used in TimelineEditor) so that the bulk delete removes caption
blocks, or alternatively remove autoCaptions from hasAnyTimelineBlocks to keep
behavior consistent—locate the hasAnyTimelineBlocks computation and the
deleteAllBlocks function and ensure autoCaptions is handled symmetrically in
both places.
Comment on lines
1450
to
+1457
| const allRegionSpans = useMemo(() => { | ||
| const zooms = zoomRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | ||
| const trims = trimRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | ||
| const speeds = speedRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | ||
| const audios = audioRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | ||
| return [...zooms, ...trims, ...speeds, ...audios]; | ||
| }, [zoomRegions, trimRegions, speedRegions, audioRegions]); | ||
| const captions = autoCaptions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | ||
| return [...zooms, ...trims, ...speeds, ...audios, ...captions]; | ||
| }, [zoomRegions, trimRegions, speedRegions, audioRegions, autoCaptions]); |
There was a problem hiding this comment.
Keep caption spans out of neighbour clamping.
TimelineWrapper.clampToNeighbours() treats every entry in allRegionSpans as a hard stop. Adding captions here means resizing a zoom/trim/speed/audio item can snap to a caption boundary even though hasOverlap() explicitly allows captions to overlap.
🐛 Minimal fix
const allRegionSpans = useMemo(() => {
const zooms = zoomRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs }));
const trims = trimRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs }));
const speeds = speedRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs }));
const audios = audioRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs }));
- const captions = autoCaptions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs }));
- return [...zooms, ...trims, ...speeds, ...audios, ...captions];
- }, [zoomRegions, trimRegions, speedRegions, audioRegions, autoCaptions]);
+ return [...zooms, ...trims, ...speeds, ...audios];
+ }, [zoomRegions, trimRegions, speedRegions, audioRegions]);📝 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.
Suggested change
| const allRegionSpans = useMemo(() => { | |
| const zooms = zoomRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| const trims = trimRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| const speeds = speedRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| const audios = audioRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| return [...zooms, ...trims, ...speeds, ...audios]; | |
| }, [zoomRegions, trimRegions, speedRegions, audioRegions]); | |
| const captions = autoCaptions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| return [...zooms, ...trims, ...speeds, ...audios, ...captions]; | |
| }, [zoomRegions, trimRegions, speedRegions, audioRegions, autoCaptions]); | |
| const allRegionSpans = useMemo(() => { | |
| const zooms = zoomRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| const trims = trimRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| const speeds = speedRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| const audios = audioRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); | |
| return [...zooms, ...trims, ...speeds, ...audios]; | |
| }, [zoomRegions, trimRegions, speedRegions, audioRegions]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 1450 -
1457, allRegionSpans currently includes captions so
TimelineWrapper.clampToNeighbours() will treat caption boundaries as hard stops;
remove autoCaptions from the array used for clamping by excluding captions from
allRegionSpans (or create a separate clampedRegionSpans without autoCaptions) so
caption spans are not considered when clamping resize/move operations; update
the useMemo that builds allRegionSpans (and its dependency array) to omit
autoCaptions and ensure hasOverlap() still allows captions to overlap as
intended.
Comment on lines
+382
to
+386
| const [selectedCaptionId, setSelectedCaptionId] = useState<string | null>(null); | ||
| const [autoCaptionSettings, setAutoCaptionSettings] = useState<AutoCaptionSettings>(() => ({ | ||
| ...DEFAULT_AUTO_CAPTION_SETTINGS, | ||
| selectedModel: (initialEditorPreferences.whisperSelectedModel as any) || "small", | ||
| })); |
There was a problem hiding this comment.
Caption selection is missing the same lifecycle handling as the other timeline tracks.
selectedCaptionId is introduced here, but both children get the raw setter and the regeneration path replaces the whole cue list without revalidating it. That leaves caption selection non-exclusive with zoom/trim/audio selection, and rerunning captions can leave the sidebar pointed at a cue that no longer exists while other actions still target the old selection.
Also applies to: 1595-1601, 3426-3433, 3542-3543
🧰 Tools
🪛 Biome (2.4.9)
[error] 385-385: 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/components/video-editor/VideoEditor.tsx` around lines 382 - 386,
selectedCaptionId is managed directly and its setter (setSelectedCaptionId) is
passed raw to children and not reconciled when the captions list is replaced,
causing non-exclusive selections and stale sidebar focus; fix by centralizing
caption selection lifecycle like other tracks: update the caption regeneration
path (the function that replaces the cues list) to revalidate/clear
selectedCaptionId when cues change, and replace raw setter usage with a wrapped
setter that enforces exclusivity (clears zoom/trim/audio selection and validates
existence in the new cues list) before setting; ensure places referenced in the
diff such as selectedCaptionId, setSelectedCaptionId, and the
caption-regeneration codepaths are updated so rerunning captions clears or
remaps selection to a valid cue.
Comment on lines
+1569
to
+1574
| console.log("[VideoEditor] handleGenerateAutoCaptions: starting", { | ||
| sourcePath, | ||
| whisperExecutablePath, | ||
| whisperModelPath, | ||
| language: autoCaptionSettings.language, | ||
| }); |
There was a problem hiding this comment.
Remove the caption-generation debug logs.
These statements dump local filesystem paths and the generation result into the renderer console. That is sensitive user/project data in production builds.
Also applies to: 1586-1587
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` around lines 1569 - 1574, Remove
the production console debug statements in
VideoEditor.handleGenerateAutoCaptions that print sensitive filesystem paths and
generation results (specifically the console.log that outputs sourcePath,
whisperExecutablePath, whisperModelPath, and autoCaptionSettings.language and
the subsequent logs around the generation result). Instead either delete those
console.log calls or replace them with non-sensitive, environment-gated debug
logging (e.g., a debug logger that only runs in dev builds or logs only
high-level status messages), ensuring handleGenerateAutoCaptions no longer emits
raw paths or content to the renderer console.
Owner
|
this is neat, please resolve merge conflicts and fix these issues:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
electron/ipc/handlers.ts (4)
1039-1049:⚠️ Potential issue | 🔴 CriticalGuard IPC sends after the renderer closes.
These async progress paths still call
webContents.send()unconditionally. If the editor window is closed mid-download or transcription, Electron can throw"Object has been destroyed"here and take the main process down. Gate each send on!webContents.isDestroyed().Also applies to: 1600-1605, 1644-1644, 1668-1668
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 1039 - 1049, The IPC send calls can throw if the renderer window is closed; update sendWhisperModelDownloadProgress to check webContents.isDestroyed() before calling webContents.send — e.g., wrap the send in if (!webContents.isDestroyed()) { webContents.send(...); }. Apply the same guard to the other async send sites mentioned (the send calls around the transcription/download progress paths referenced in the comment) so every webContents.send is gated by !webContents.isDestroyed() to avoid "Object has been destroyed" exceptions.
3260-3270:⚠️ Potential issue | 🔴 CriticalConvert display bounds to device pixels before passing them to native capture.
Electron’s
display.boundsare in DIP, but the Windows helper matches against monitor rectangles in device pixels. On mixed-DPI setups, Lines 3266-3269 can still select the wrong monitor. Multiply bydisplay.scaleFactorbefore rounding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 3260 - 3270, The display bounds from getScreen().getAllDisplays() are in DIP but the native Windows helper expects device pixels; update the block that sets config.displayX/displayY/displayW/displayH (using display.bounds.x/y/width/height) to multiply each bound by display.scaleFactor before Math.round so the monitor matching uses device pixels (e.g., use display.bounds.x * display.scaleFactor, etc.), ensuring fallback to getPrimaryDisplay() remains unchanged.
1154-1165:⚠️ Potential issue | 🟠 MajorWait for the temp file handle to close before renaming.
Line 1164 renames immediately after
downloadFileWithProgress()resolves, but that helper still resolves on the write stream’sfinish. On Windows, the handle can still be open at that point, so the rename intermittently fails withEPERM/EBUSY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 1154 - 1165, The rename is happening immediately after downloadFileWithProgress resolves, but that helper resolves on the write stream’s 'finish' and the file handle may still be open (causing EPERM/EBUSY on Windows); modify the flow in the block using tempPath, downloadFileWithProgress, and fs.rename to explicitly wait for the temporary file handle to be fully closed before calling fs.rename(modelPath) — e.g., obtain or expose the stream/handle from downloadFileWithProgress (or open and close a new handle) and await its 'close' event, then call fs.rename(tempPath, modelPath) and only after that sendWhisperModelDownloadProgress; ensure references are to downloadFileWithProgress, tempPath, fs.rename, modelPath, and sendWhisperModelDownloadProgress so the change is easy to locate.
1052-1067:⚠️ Potential issue | 🟡 MinorDon't collapse invalid model names into “not downloaded”.
This catch turns unsupported models and non-
ENOENTfilesystem failures into{ success: true, exists: false }. That makes stale/corrupt selections look like a normal cache miss and pushes the caller down the wrong path. SplitgetWhisperModelPath()validation fromfs.access(), and only mapENOENTtoexists: false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 1052 - 1067, The catch currently hides invalid model names and non-ENOENT FS errors; update getWhisperModelStatus to first validate the modelName via getWhisperModelPath() and throw or return an explicit error if the path resolution/validation fails, then call await fs.access(modelPath, fsConstants.R_OK) inside its own try/catch that only maps fs errors with code 'ENOENT' to { success: true, exists: false, path: null }; any other thrown errors from fs.access or validation should be propagated or returned as { success: false, error: <error> } so callers can distinguish unsupported models and real IO failures (refer to getWhisperModelStatus, getWhisperModelPath, fs.access, fsConstants.R_OK and inspect error.code === 'ENOENT').src/components/video-editor/timeline/TimelineEditor.tsx (2)
1578-1585:⚠️ Potential issue | 🟠 MajorCaption spans still shouldn't participate in neighbour clamping.
hasOverlap()explicitly allows captions to overlap, butallRegionSpansstill feeds them intoTimelineWrapperas hard boundaries. That makes zoom/trim/speed/audio edits snap to caption edges even though captions are not supposed to constrain those rows.🐛 Minimal fix
const allRegionSpans = useMemo(() => { const zooms = zoomRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); const trims = trimRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); const speeds = speedRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); const audios = audioRegions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); - const captions = autoCaptions.map((r) => ({ id: r.id, start: r.startMs, end: r.endMs })); - return [...zooms, ...trims, ...speeds, ...audios, ...captions]; -}, [zoomRegions, trimRegions, speedRegions, audioRegions, autoCaptions]); + return [...zooms, ...trims, ...speeds, ...audios]; +}, [zoomRegions, trimRegions, speedRegions, audioRegions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 1578 - 1585, allRegionSpans is including autoCaptions which makes caption spans act as hard boundaries in TimelineWrapper even though hasOverlap() allows captions to overlap; fix by removing captions (autoCaptions) from the array used for neighbor clamping — i.e., change the useMemo that builds allRegionSpans to only combine zoomRegions, trimRegions, speedRegions and audioRegions (leave captions out) or create a separate allNonCaptionRegionSpans used by TimelineWrapper and clamping logic while leaving autoCaptions untouched for rendering; update any references to allRegionSpans in TimelineWrapper/clamping code to use the new non-caption list.
78-81:⚠️ Potential issue | 🟠 MajorCaption items are selectable, but the timeline still can't delete them.
There is no caption-delete callback in the props,
deleteAllBlocks()only clears zoom/trim/annotation/speed/audio, and the Delete-key branch never checksselectedCaptionId. Cmd/Ctrl+A + Delete and single-item Delete both leave caption blocks behind.Also applies to: 962-997, 1474-1490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 78 - 81, The timeline doesn't support deleting captions because there's no delete callback and keyboard/delete logic doesn't consider selectedCaptionId; add a new prop like onDeleteCaption?: (id: string) => void (and optionally onDeleteAllCaptions?: () => void) to the component props, call it from the single-item delete path (the branch that currently checks selectedCaptionId) and from the Cmd/Ctrl+A + Delete branch, and update deleteAllBlocks() (or the routine that clears all timeline state) to either call onDeleteAllCaptions() or ensure it removes caption items by invoking onDeleteCaption for each selected caption; update any keyboard handler (e.g., handleKeyDown or the Delete-key branch) to check selectedCaptionId and invoke the new prop so caption blocks are actually removed.src/components/video-editor/VideoEditor.tsx (3)
1461-1479:⚠️ Potential issue | 🔴 CriticalStill reset
whisperModelPathwhen the selected model changes.This effect still preserves the previous path via
currentPath ?? result.pathand never clears it when the newly selected model is missing. After a switch,Generate CaptionsandDelete Modelcan stay wired to the old file.🐛 Minimal fix
useEffect(() => { + let cancelled = false; + setWhisperModelPath(null); + setWhisperModelDownloadStatus("idle"); + setWhisperModelDownloadProgress(0); + void (async () => { const result = await window.electronAPI.getWhisperModelStatus( autoCaptionSettings.selectedModel, ); - if (!result.success) { + if (cancelled || !result.success) { return; } if (result.exists && result.path) { - setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null); + setWhisperModelPath(result.path); setWhisperModelDownloadStatus("downloaded"); setWhisperModelDownloadProgress(100); - } else { - setWhisperModelDownloadStatus("idle"); - setWhisperModelDownloadProgress(0); } })(); + + return () => { + cancelled = true; + }; }, [autoCaptionSettings.selectedModel]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 1461 - 1479, The effect that checks getWhisperModelStatus for autoCaptionSettings.selectedModel is incorrectly preserving the previous whisperModelPath using setWhisperModelPath((currentPath) => currentPath ?? result.path ?? null), so when a newly selected model is missing the UI still shows the old path; change the logic in the useEffect to setWhisperModelPath(result.exists && result.path ? result.path : null) (i.e., clear the path when the model does not exist), and ensure you still update setWhisperModelDownloadStatus ("downloaded" vs "idle") and setWhisperModelDownloadProgress (100 vs 0) based on result.exists to keep Generate Captions/Delete Model wired to the correct file.
1619-1628:⚠️ Potential issue | 🟠 MajorCaption selection still needs lifecycle handling.
The generation/clear paths replace
autoCaptions, butselectedCaptionIdis left untouched, and both children still receive the raw setter. That leaves the sidebar/timeline pointing at deleted cues and makes caption selection non-exclusive with the other tracks.Also applies to: 1655-1658, 3467-3468, 3578-3579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 1619 - 1628, When replacing or clearing autoCaptions in places like the generator success path (where setAutoCaptions and setAutoCaptionSettings are called), also update selectedCaptionId via setSelectedCaptionId so it doesn't point to deleted cues — either clear it (null) or set it to a valid new cue id (e.g., first cue in cuesWithIds). Additionally, ensure the caption-selection setter passed into children (sidebar/timeline) is wrapped so selecting an auto caption clears selections on other tracks (making selection exclusive) and vice-versa; update usages of setAutoCaptions and any clear paths to also call setSelectedCaptionId and use the wrapped setter instead of exposing the raw setter.
1582-1587:⚠️ Potential issue | 🟠 MajorThe caption-generation debug logs are still leaking local paths.
These
console.logcalls print renderer-side filesystem paths and generation results in production, which is sensitive user/project data.Also applies to: 1617-1617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 1582 - 1587, The console.log in handleGenerateAutoCaptions is leaking local filesystem paths and should be removed or gated out of production; update the function (handleGenerateAutoCaptions) and any other logging spots (e.g., the call around line 1617) to either remove sensitive console.log calls or replace them with safe, non-path debug logging that only runs when a dev/debug flag is enabled (e.g., check a DEBUG/ENV flag) and ensure autoCaptionSettings and variables like sourcePath, whisperExecutablePath, whisperModelPath are never logged in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 1552-1554: Selected-range caption generation currently can
overshoot the requested end because chunks always use CHUNK_SIZE_MS + OVERLAP_MS
and progress is calculated with absolute offsets; fix by clamping each chunk to
the remaining range and computing progress relative to startTimeMs: compute
endTimeMs = totalDurationMs > 0 ? startTimeMs + totalDurationMs : Infinity
(already present), then for every chunk calculation (where CHUNK_SIZE_MS +
OVERLAP_MS is used and where offsetMs is used) set chunkDuration =
Math.min(CHUNK_SIZE_MS + OVERLAP_MS, Math.max(0, endTimeMs - chunkStartMs)) and
chunkEndMs = chunkStartMs + chunkDuration (or min(chunkStartMs + chunkSize,
endTimeMs)); when computing progress use progress = totalDurationMs > 0 ?
Math.min(1, Math.max(0, (offsetMs - startTimeMs) / totalDurationMs)) : 0; update
all places referencing CHUNK_SIZE_MS+OVERLAP_MS, offsetMs, and progress (the
blocks that handle chunk creation and progress updates) to use these clamped
values so selections with non-zero startTimeMs and short durations do not
overshoot.
- Around line 4197-4205: The two IPC handlers are inconsistent:
'delete-whisper-model' takes a modelName and validates it, while
'generate-auto-captions' accepts an arbitrary whisperModelPath, allowing the
renderer to reference stale or deleted files; unify them by changing both IPC
handlers ('delete-whisper-model' and 'generate-auto-captions') to accept a
single selectedModel parameter (name-based) and an optional flag or separate
field for customFilePath for user-provided files, resolve built-in model paths
in the main process (use the same whitelist/lookup used by deleteWhisperModel),
validate selectedModel against the allowed list in main, and implement an
explicit branch in generateAutoCaptions that, if customFilePath is provided,
uses it after validating existence/permissions, otherwise resolves the built-in
path from selectedModel before proceeding; update calls to deleteWhisperModel
and sendWhisperModelDownloadProgress to use the unified selectedModel identity.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 994-1030: The guard applyingHistoryRef.current is being cleared
immediately after queuing state updates, causing the history-recording effect to
treat the applied snapshot as a new edit; remove the line that sets
applyingHistoryRef.current = false here and instead clear
applyingHistoryRef.current inside the history-consumption effect (the effect
that reads the snapshot and records history) after it has consumed the snapshot;
update the history effect to set applyingHistoryRef.current = false (or clear a
pendingSnapshotRef) only once it has processed the snapshot so functions/refs
like setZoomRegions, setTrimRegions, setSpeedRegions, setAnnotationRegions,
setAudioRegions, setAutoCaptions and nextAnnotationZIndexRef are fully applied
before the guard is released.
---
Duplicate comments:
In `@electron/ipc/handlers.ts`:
- Around line 1039-1049: The IPC send calls can throw if the renderer window is
closed; update sendWhisperModelDownloadProgress to check
webContents.isDestroyed() before calling webContents.send — e.g., wrap the send
in if (!webContents.isDestroyed()) { webContents.send(...); }. Apply the same
guard to the other async send sites mentioned (the send calls around the
transcription/download progress paths referenced in the comment) so every
webContents.send is gated by !webContents.isDestroyed() to avoid "Object has
been destroyed" exceptions.
- Around line 3260-3270: The display bounds from getScreen().getAllDisplays()
are in DIP but the native Windows helper expects device pixels; update the block
that sets config.displayX/displayY/displayW/displayH (using
display.bounds.x/y/width/height) to multiply each bound by display.scaleFactor
before Math.round so the monitor matching uses device pixels (e.g., use
display.bounds.x * display.scaleFactor, etc.), ensuring fallback to
getPrimaryDisplay() remains unchanged.
- Around line 1154-1165: The rename is happening immediately after
downloadFileWithProgress resolves, but that helper resolves on the write
stream’s 'finish' and the file handle may still be open (causing EPERM/EBUSY on
Windows); modify the flow in the block using tempPath, downloadFileWithProgress,
and fs.rename to explicitly wait for the temporary file handle to be fully
closed before calling fs.rename(modelPath) — e.g., obtain or expose the
stream/handle from downloadFileWithProgress (or open and close a new handle) and
await its 'close' event, then call fs.rename(tempPath, modelPath) and only after
that sendWhisperModelDownloadProgress; ensure references are to
downloadFileWithProgress, tempPath, fs.rename, modelPath, and
sendWhisperModelDownloadProgress so the change is easy to locate.
- Around line 1052-1067: The catch currently hides invalid model names and
non-ENOENT FS errors; update getWhisperModelStatus to first validate the
modelName via getWhisperModelPath() and throw or return an explicit error if the
path resolution/validation fails, then call await fs.access(modelPath,
fsConstants.R_OK) inside its own try/catch that only maps fs errors with code
'ENOENT' to { success: true, exists: false, path: null }; any other thrown
errors from fs.access or validation should be propagated or returned as {
success: false, error: <error> } so callers can distinguish unsupported models
and real IO failures (refer to getWhisperModelStatus, getWhisperModelPath,
fs.access, fsConstants.R_OK and inspect error.code === 'ENOENT').
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 1578-1585: allRegionSpans is including autoCaptions which makes
caption spans act as hard boundaries in TimelineWrapper even though hasOverlap()
allows captions to overlap; fix by removing captions (autoCaptions) from the
array used for neighbor clamping — i.e., change the useMemo that builds
allRegionSpans to only combine zoomRegions, trimRegions, speedRegions and
audioRegions (leave captions out) or create a separate allNonCaptionRegionSpans
used by TimelineWrapper and clamping logic while leaving autoCaptions untouched
for rendering; update any references to allRegionSpans in
TimelineWrapper/clamping code to use the new non-caption list.
- Around line 78-81: The timeline doesn't support deleting captions because
there's no delete callback and keyboard/delete logic doesn't consider
selectedCaptionId; add a new prop like onDeleteCaption?: (id: string) => void
(and optionally onDeleteAllCaptions?: () => void) to the component props, call
it from the single-item delete path (the branch that currently checks
selectedCaptionId) and from the Cmd/Ctrl+A + Delete branch, and update
deleteAllBlocks() (or the routine that clears all timeline state) to either call
onDeleteAllCaptions() or ensure it removes caption items by invoking
onDeleteCaption for each selected caption; update any keyboard handler (e.g.,
handleKeyDown or the Delete-key branch) to check selectedCaptionId and invoke
the new prop so caption blocks are actually removed.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1461-1479: The effect that checks getWhisperModelStatus for
autoCaptionSettings.selectedModel is incorrectly preserving the previous
whisperModelPath using setWhisperModelPath((currentPath) => currentPath ??
result.path ?? null), so when a newly selected model is missing the UI still
shows the old path; change the logic in the useEffect to
setWhisperModelPath(result.exists && result.path ? result.path : null) (i.e.,
clear the path when the model does not exist), and ensure you still update
setWhisperModelDownloadStatus ("downloaded" vs "idle") and
setWhisperModelDownloadProgress (100 vs 0) based on result.exists to keep
Generate Captions/Delete Model wired to the correct file.
- Around line 1619-1628: When replacing or clearing autoCaptions in places like
the generator success path (where setAutoCaptions and setAutoCaptionSettings are
called), also update selectedCaptionId via setSelectedCaptionId so it doesn't
point to deleted cues — either clear it (null) or set it to a valid new cue id
(e.g., first cue in cuesWithIds). Additionally, ensure the caption-selection
setter passed into children (sidebar/timeline) is wrapped so selecting an auto
caption clears selections on other tracks (making selection exclusive) and
vice-versa; update usages of setAutoCaptions and any clear paths to also call
setSelectedCaptionId and use the wrapped setter instead of exposing the raw
setter.
- Around line 1582-1587: The console.log in handleGenerateAutoCaptions is
leaking local filesystem paths and should be removed or gated out of production;
update the function (handleGenerateAutoCaptions) and any other logging spots
(e.g., the call around line 1617) to either remove sensitive console.log calls
or replace them with safe, non-path debug logging that only runs when a
dev/debug flag is enabled (e.g., check a DEBUG/ENV flag) and ensure
autoCaptionSettings and variables like sourcePath, whisperExecutablePath,
whisperModelPath are never logged in production.
🪄 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: e20aa624-419b-4a82-a7f1-00df48e1331b
📒 Files selected for processing (12)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tspackage.jsonsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/KeyframeMarkers.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/video-editor/projectPersistence.ts
- src/components/video-editor/timeline/ItemGlass.module.css
- electron/preload.ts
Comment on lines
+1552
to
+1554
| const startTimeMs = options.startTimeMs || 0; | ||
| const totalDurationMs = options.durationMs || 0; | ||
| const endTimeMs = totalDurationMs > 0 ? startTimeMs + totalDurationMs : Infinity; |
There was a problem hiding this comment.
Selected-range caption generation can run past the requested end.
For short selected ranges, Line 1584 still extracts CHUNK_SIZE_MS + OVERLAP_MS, and Lines 1636-1638 keep everything from that final chunk instead of clipping to endTimeMs. On top of that, Line 1601 mixes absolute offsetMs with relative totalDurationMs, so non-zero startTimeMs selections jump almost straight to 99%. Clamp each chunk to the remaining range and compute progress from offsetMs - startTimeMs.
Also applies to: 1579-1585, 1599-1605, 1633-1639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 1552 - 1554, Selected-range caption
generation currently can overshoot the requested end because chunks always use
CHUNK_SIZE_MS + OVERLAP_MS and progress is calculated with absolute offsets; fix
by clamping each chunk to the remaining range and computing progress relative to
startTimeMs: compute endTimeMs = totalDurationMs > 0 ? startTimeMs +
totalDurationMs : Infinity (already present), then for every chunk calculation
(where CHUNK_SIZE_MS + OVERLAP_MS is used and where offsetMs is used) set
chunkDuration = Math.min(CHUNK_SIZE_MS + OVERLAP_MS, Math.max(0, endTimeMs -
chunkStartMs)) and chunkEndMs = chunkStartMs + chunkDuration (or
min(chunkStartMs + chunkSize, endTimeMs)); when computing progress use progress
= totalDurationMs > 0 ? Math.min(1, Math.max(0, (offsetMs - startTimeMs) /
totalDurationMs)) : 0; update all places referencing CHUNK_SIZE_MS+OVERLAP_MS,
offsetMs, and progress (the blocks that handle chunk creation and progress
updates) to use these clamped values so selections with non-zero startTimeMs and
short durations do not overshoot.
Comment on lines
+4197
to
4205
| ipcMain.handle('delete-whisper-model', async (event, modelName: string) => { | ||
| try { | ||
| await deleteWhisperSmallModel() | ||
| await deleteWhisperModel(event, modelName) | ||
| sendWhisperModelDownloadProgress(event.sender, { | ||
| status: 'idle', | ||
| progress: 0, | ||
| model: modelName, | ||
| path: null, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'delete-whisper-model'
rg -n -C2 'generate-auto-captions|whisperModelPath'Repository: webadderall/Recordly
Length of output: 13489
🏁 Script executed:
# Find the deleteWhisperModel function implementation
rg -n 'deleteWhisperModel' electron/ipc/handlers.ts -A 20 | head -60
# Find whisper model resolution logic
rg -n 'resolveWhisperModel|Unsupported Whisper model'
# Check if there's model name to path resolution
rg -n 'whisperSelectedModel|selectedModel' electron/ipc/handlers.ts | head -20Repository: webadderall/Recordly
Length of output: 1597
🏁 Script executed:
# Find getWhisperModelPath implementation
rg -n 'getWhisperModelPath' -A 30
# Check the error context at lines 71 and 1140
sed -n '65,80p' electron/ipc/handlers.ts
sed -n '1135,1150p' electron/ipc/handlers.ts
# Find selectedModel usage to understand model selection flow
rg -n 'whisperSelectedModel' electron/ipc/handlers.ts -B 3 -A 3Repository: webadderall/Recordly
Length of output: 7745
🏁 Script executed:
# View the full generate-auto-captions handler starting at line 4213
sed -n '4213,4280p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 2603
🏁 Script executed:
# Find generateAutoCaptionsFromVideo function implementation
rg -n 'generateAutoCaptionsFromVideo' -A 50 | head -80Repository: webadderall/Recordly
Length of output: 5601
🏁 Script executed:
# Check how the renderer calls these IPC handlers
rg -n 'generateAutoCaptions|deleteWhisperModel' src/components/video-editor/ -B 2 -A 2 | head -100
# Also check what happens when whisperModelPath is null/undefined
rg -n 'whisperModelPath.*null|whisperModelPath.*undefined' src/components/video-editor/VideoEditor.tsx -B 2 -A 2Repository: webadderall/Recordly
Length of output: 1566
🏁 Script executed:
# View the full generateAutoCaptions call context
sed -n '1605,1620p' src/components/video-editor/VideoEditor.tsx
# Check if there are other places calling delete-whisper-model
rg -n 'deleteWhisperModel' src/components/ -B 3 -A 3
# Check what autoCaptionSettings.selectedModel is and if it could be undefined
rg -n 'autoCaptionSettings.*selectedModel|selectedModel.*=' src/components/video-editor/VideoEditor.tsx -B 2 -A 2 | head -40Repository: webadderall/Recordly
Length of output: 2285
🏁 Script executed:
# Check the autoCaptionSettings structure and initialization
rg -n 'autoCaptionSettings.*=' src/components/video-editor/VideoEditor.tsx -B 1 -A 1 | head -40
# Check the default value for selectedModel
rg -n 'selectedModel.*=' src/components/video-editor/ -B 2 -A 2 | head -50
# Look at the flow when model download completes - line 1425
sed -n '1415,1445p' src/components/video-editor/VideoEditor.tsxRepository: webadderall/Recordly
Length of output: 3335
Unify model identity across the IPC boundary.
delete-whisper-model validates model identity against a whitelist (via modelName parameter), but generate-auto-captions accepts an arbitrary file path with no model validation. This split leaves the main process unable to enforce model selection: the renderer's whisperModelPath state can drift from the actual selected model, allowing transcription with stale or deleted .bin files. Redesign both handlers to accept a single selectedModel input (name-based), resolve built-in model paths in main, and add a separate explicit branch for custom user-provided files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 4197 - 4205, The two IPC handlers are
inconsistent: 'delete-whisper-model' takes a modelName and validates it, while
'generate-auto-captions' accepts an arbitrary whisperModelPath, allowing the
renderer to reference stale or deleted files; unify them by changing both IPC
handlers ('delete-whisper-model' and 'generate-auto-captions') to accept a
single selectedModel parameter (name-based) and an optional flag or separate
field for customFilePath for user-provided files, resolve built-in model paths
in the main process (use the same whitelist/lookup used by deleteWhisperModel),
validate selectedModel against the allowed list in main, and implement an
explicit branch in generateAutoCaptions that, if customFilePath is provided,
uses it after validating existence/permissions, otherwise resolves the built-in
path from selectedModel before proceeding; update calls to deleteWhisperModel
and sendWhisperModelDownloadProgress to use the unified selectedModel identity.
Comment on lines
+994
to
+1030
| applyingHistoryRef.current = true; | ||
| const cloned = cloneSnapshot(snapshot); | ||
| setZoomRegions(cloned.zoomRegions); | ||
| setTrimRegions(cloned.trimRegions); | ||
| setSpeedRegions(cloned.speedRegions); | ||
| setAnnotationRegions(cloned.annotationRegions); | ||
| setAudioRegions(cloned.audioRegions); | ||
| setAutoCaptions(cloned.autoCaptions); | ||
| setSelectedZoomId(cloned.selectedZoomId); | ||
| setSelectedTrimId(cloned.selectedTrimId); | ||
| setSelectedSpeedId(cloned.selectedSpeedId); | ||
| setSelectedAnnotationId(cloned.selectedAnnotationId); | ||
| setSelectedAudioId(cloned.selectedAudioId); | ||
| setZoomRegions(cloneStructured(snapshot.zoomRegions)); | ||
| setTrimRegions(cloneStructured(snapshot.trimRegions)); | ||
| setSpeedRegions(cloneStructured(snapshot.speedRegions)); | ||
| setAnnotationRegions(cloneStructured(snapshot.annotationRegions)); | ||
| setAudioRegions(cloneStructured(snapshot.audioRegions)); | ||
| setAutoCaptions(cloneStructured(snapshot.autoCaptions)); | ||
| setSelectedZoomId(snapshot.selectedZoomId); | ||
| setSelectedTrimId(snapshot.selectedTrimId); | ||
| setSelectedSpeedId(snapshot.selectedSpeedId); | ||
| setSelectedAnnotationId(snapshot.selectedAnnotationId); | ||
| setSelectedAudioId(snapshot.selectedAudioId); | ||
| setSelectedCaptionId(snapshot.selectedCaptionId); | ||
|
|
||
| nextZoomIdRef.current = deriveNextId( | ||
| "zoom", | ||
| cloned.zoomRegions.map((region) => region.id), | ||
| snapshot.zoomRegions.map((region: ZoomRegion) => region.id), | ||
| ); | ||
| nextTrimIdRef.current = deriveNextId( | ||
| "trim", | ||
| cloned.trimRegions.map((region) => region.id), | ||
| snapshot.trimRegions.map((region: TrimRegion) => region.id), | ||
| ); | ||
| nextSpeedIdRef.current = deriveNextId( | ||
| "speed", | ||
| cloned.speedRegions.map((region) => region.id), | ||
| snapshot.speedRegions.map((region: SpeedRegion) => region.id), | ||
| ); | ||
| nextAnnotationIdRef.current = deriveNextId( | ||
| "annotation", | ||
| cloned.annotationRegions.map((region) => region.id), | ||
| snapshot.annotationRegions.map((region: AnnotationRegion) => region.id), | ||
| ); | ||
| nextAudioIdRef.current = deriveNextId( | ||
| "audio", | ||
| cloned.audioRegions.map((region) => region.id), | ||
| snapshot.audioRegions.map((region: AudioRegion) => region.id), | ||
| ); | ||
| nextAnnotationZIndexRef.current = | ||
| cloned.annotationRegions.reduce((max, region) => Math.max(max, region.zIndex), 0) + 1; | ||
| snapshot.annotationRegions.reduce((max: number, region: AnnotationRegion) => Math.max(max, region.zIndex), 0) + 1; | ||
| applyingHistoryRef.current = false; |
There was a problem hiding this comment.
Keep applyingHistoryRef true until the history effect consumes the snapshot.
Line 1030 clears the guard before React applies the queued state updates above. The history-recording effect then sees the undo/redo result as a fresh edit and pushes it straight back into history.
🐛 Minimal fix
const applyHistorySnapshot = useCallback(
(snapshot: EditorHistorySnapshot) => {
applyingHistoryRef.current = true;
setZoomRegions(cloneStructured(snapshot.zoomRegions));
setTrimRegions(cloneStructured(snapshot.trimRegions));
setSpeedRegions(cloneStructured(snapshot.speedRegions));
setAnnotationRegions(cloneStructured(snapshot.annotationRegions));
setAudioRegions(cloneStructured(snapshot.audioRegions));
setAutoCaptions(cloneStructured(snapshot.autoCaptions));
setSelectedZoomId(snapshot.selectedZoomId);
setSelectedTrimId(snapshot.selectedTrimId);
setSelectedSpeedId(snapshot.selectedSpeedId);
setSelectedAnnotationId(snapshot.selectedAnnotationId);
setSelectedAudioId(snapshot.selectedAudioId);
setSelectedCaptionId(snapshot.selectedCaptionId);
@@
nextAnnotationZIndexRef.current =
snapshot.annotationRegions.reduce((max: number, region: AnnotationRegion) => Math.max(max, region.zIndex), 0) + 1;
- applyingHistoryRef.current = false;
},
[cloneSnapshot],
);📝 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.
Suggested change
| applyingHistoryRef.current = true; | |
| const cloned = cloneSnapshot(snapshot); | |
| setZoomRegions(cloned.zoomRegions); | |
| setTrimRegions(cloned.trimRegions); | |
| setSpeedRegions(cloned.speedRegions); | |
| setAnnotationRegions(cloned.annotationRegions); | |
| setAudioRegions(cloned.audioRegions); | |
| setAutoCaptions(cloned.autoCaptions); | |
| setSelectedZoomId(cloned.selectedZoomId); | |
| setSelectedTrimId(cloned.selectedTrimId); | |
| setSelectedSpeedId(cloned.selectedSpeedId); | |
| setSelectedAnnotationId(cloned.selectedAnnotationId); | |
| setSelectedAudioId(cloned.selectedAudioId); | |
| setZoomRegions(cloneStructured(snapshot.zoomRegions)); | |
| setTrimRegions(cloneStructured(snapshot.trimRegions)); | |
| setSpeedRegions(cloneStructured(snapshot.speedRegions)); | |
| setAnnotationRegions(cloneStructured(snapshot.annotationRegions)); | |
| setAudioRegions(cloneStructured(snapshot.audioRegions)); | |
| setAutoCaptions(cloneStructured(snapshot.autoCaptions)); | |
| setSelectedZoomId(snapshot.selectedZoomId); | |
| setSelectedTrimId(snapshot.selectedTrimId); | |
| setSelectedSpeedId(snapshot.selectedSpeedId); | |
| setSelectedAnnotationId(snapshot.selectedAnnotationId); | |
| setSelectedAudioId(snapshot.selectedAudioId); | |
| setSelectedCaptionId(snapshot.selectedCaptionId); | |
| nextZoomIdRef.current = deriveNextId( | |
| "zoom", | |
| cloned.zoomRegions.map((region) => region.id), | |
| snapshot.zoomRegions.map((region: ZoomRegion) => region.id), | |
| ); | |
| nextTrimIdRef.current = deriveNextId( | |
| "trim", | |
| cloned.trimRegions.map((region) => region.id), | |
| snapshot.trimRegions.map((region: TrimRegion) => region.id), | |
| ); | |
| nextSpeedIdRef.current = deriveNextId( | |
| "speed", | |
| cloned.speedRegions.map((region) => region.id), | |
| snapshot.speedRegions.map((region: SpeedRegion) => region.id), | |
| ); | |
| nextAnnotationIdRef.current = deriveNextId( | |
| "annotation", | |
| cloned.annotationRegions.map((region) => region.id), | |
| snapshot.annotationRegions.map((region: AnnotationRegion) => region.id), | |
| ); | |
| nextAudioIdRef.current = deriveNextId( | |
| "audio", | |
| cloned.audioRegions.map((region) => region.id), | |
| snapshot.audioRegions.map((region: AudioRegion) => region.id), | |
| ); | |
| nextAnnotationZIndexRef.current = | |
| cloned.annotationRegions.reduce((max, region) => Math.max(max, region.zIndex), 0) + 1; | |
| snapshot.annotationRegions.reduce((max: number, region: AnnotationRegion) => Math.max(max, region.zIndex), 0) + 1; | |
| applyingHistoryRef.current = false; | |
| applyingHistoryRef.current = true; | |
| setZoomRegions(cloneStructured(snapshot.zoomRegions)); | |
| setTrimRegions(cloneStructured(snapshot.trimRegions)); | |
| setSpeedRegions(cloneStructured(snapshot.speedRegions)); | |
| setAnnotationRegions(cloneStructured(snapshot.annotationRegions)); | |
| setAudioRegions(cloneStructured(snapshot.audioRegions)); | |
| setAutoCaptions(cloneStructured(snapshot.autoCaptions)); | |
| setSelectedZoomId(snapshot.selectedZoomId); | |
| setSelectedTrimId(snapshot.selectedTrimId); | |
| setSelectedSpeedId(snapshot.selectedSpeedId); | |
| setSelectedAnnotationId(snapshot.selectedAnnotationId); | |
| setSelectedAudioId(snapshot.selectedAudioId); | |
| setSelectedCaptionId(snapshot.selectedCaptionId); | |
| nextZoomIdRef.current = deriveNextId( | |
| "zoom", | |
| snapshot.zoomRegions.map((region: ZoomRegion) => region.id), | |
| ); | |
| nextTrimIdRef.current = deriveNextId( | |
| "trim", | |
| snapshot.trimRegions.map((region: TrimRegion) => region.id), | |
| ); | |
| nextSpeedIdRef.current = deriveNextId( | |
| "speed", | |
| snapshot.speedRegions.map((region: SpeedRegion) => region.id), | |
| ); | |
| nextAnnotationIdRef.current = deriveNextId( | |
| "annotation", | |
| snapshot.annotationRegions.map((region: AnnotationRegion) => region.id), | |
| ); | |
| nextAudioIdRef.current = deriveNextId( | |
| "audio", | |
| snapshot.audioRegions.map((region: AudioRegion) => region.id), | |
| ); | |
| nextAnnotationZIndexRef.current = | |
| snapshot.annotationRegions.reduce((max: number, region: AnnotationRegion) => Math.max(max, region.zIndex), 0) + 1; | |
| }, | |
| [cloneSnapshot], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` around lines 994 - 1030, The
guard applyingHistoryRef.current is being cleared immediately after queuing
state updates, causing the history-recording effect to treat the applied
snapshot as a new edit; remove the line that sets applyingHistoryRef.current =
false here and instead clear applyingHistoryRef.current inside the
history-consumption effect (the effect that reads the snapshot and records
history) after it has consumed the snapshot; update the history effect to set
applyingHistoryRef.current = false (or clear a pendingSnapshotRef) only once it
has processed the snapshot so functions/refs like setZoomRegions,
setTrimRegions, setSpeedRegions, setAnnotationRegions, setAudioRegions,
setAutoCaptions and nextAnnotationZIndexRef are fully applied before the guard
is released.
- Hardened IPC with safety guards for all webContents.send calls - Scrubbed sensitive absolute paths from caption logs - Realigned Windows artifact naming logic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces two major features to Recordly: a "Blur Area" annotation for redacting sensitive content and a model selection dropdown for Whisper auto-captioning.
Motivation
Type of Change
Related Issue(s)
Supports roadmap for advanced annotations and improved accessibility.
Screenshots / Video
Blur Annotation in Editor:
Testing Guide
Blur Annotation:
Model Selection:
Screenshots / Video
Select Model Captions:
Checklist
userData.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Packaging