refactor: split large files — components (part 2/2)#290
Closed
webadderall wants to merge 10 commits intorefactor/split-part1from
Closed
refactor: split large files — components (part 2/2)#290webadderall wants to merge 10 commits intorefactor/split-part1from
webadderall wants to merge 10 commits intorefactor/split-part1from
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDeleted the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LaunchWindow
participant ActionsHook as useLaunchWindowActions
participant SetupHook as useLaunchWindowSetup
participant Electron as window.electronAPI
participant Media as navigator.mediaDevices
User->>LaunchWindow: open sources / start recording / toggle webcam
LaunchWindow->>ActionsHook: toggleDropdown("sources") / handleSourceSelect(...)
ActionsHook->>Electron: getSources()
Electron-->>ActionsHook: sources list
ActionsHook-->>LaunchWindow: setSources(...)
LaunchWindow->>SetupHook: setSelectedSource / report HUD size
LaunchWindow->>Media: getUserMedia({ video: deviceId }) (if webcam preview)
Media-->>LaunchWindow: MediaStream
LaunchWindow->>VideoElement: attach stream (video.srcObject)
sequenceDiagram
participant Editor
participant WiringHook as useEditorWiring
participant ExportHook as useEditorExport
participant ExportWorkflow as runEditorExport
participant Electron as window.electronAPI
Editor->>WiringHook: getRenderConfig()
Editor->>ExportHook: handleStartExportFromDropdown(settings)
ExportHook->>ExportWorkflow: runEditorExport(renderConfig,...)
ExportWorkflow->>Electron: saveExportedVideo / writeExportedVideoToPath
Electron-->>ExportWorkflow: success/failure
ExportWorkflow-->>ExportHook: setExportedFilePath / setExportError
ExportHook-->>Editor: update UI (progress/completion)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
src/components/launch/LaunchWindow/LaunchWindow.module.css-85-96 (1)
85-96:⚠️ Potential issue | 🟡 MinorRename keyframes to satisfy Stylelint.
Stylelint requires kebab-case keyframe names, so
updateBadgeSpinandmenuCardInwill fail the check.Proposed fix
.updateBadgeSpin { - animation: updateBadgeSpin 0.9s linear infinite; + animation: update-badge-spin 0.9s linear infinite; } -@keyframes updateBadgeSpin { +@keyframes update-badge-spin { from { transform: rotate(0deg); } to { transform: rotate(360deg); @@ pointer-events: auto; - animation: menuCardIn 0.18s ease; + animation: menu-card-in 0.18s ease; } -@keyframes menuCardIn { +@keyframes menu-card-in { from { opacity: 0; transform: translateY(12px);Also applies to: 218-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow/LaunchWindow.module.css` around lines 85 - 96, Stylelint fails because keyframe names use camelCase; rename the keyframes (e.g., updateBadgeSpin -> update-badge-spin and menuCardIn -> menu-card-in) and update all references to them (animation or animation-name in .updateBadgeSpin and wherever menuCardIn is used) so the `@keyframes` declarations and the animation properties consistently use kebab-case names.src/components/launch/LaunchWindow/helperComponents.tsx-62-64 (1)
62-64:⚠️ Potential issue | 🟡 MinorDefine the dropdown separator class or remove the variant.
styles.ddSepis referenced here, but the provided CSS module does not define.ddSep, sodropdown={true}renders an unstyled separator.Proposed fix
Add the missing CSS class in
src/components/launch/LaunchWindow/LaunchWindow.module.css:+.ddSep { + height: 1px; + width: 100%; + background: `#2a2a34`; + margin: 6px 0; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow/helperComponents.tsx` around lines 62 - 64, The Separator component references styles.ddSep which doesn't exist in the CSS module; either add a .ddSep class to the LaunchWindow CSS module (e.g., duplicate or alias the existing .sep rules so dropdown separators get the same styling) or remove the dropdown variant and always use styles.sep; update the CSS module to include .ddSep with the intended styles and keep the Separator signature (function Separator) unchanged.src/components/launch/LaunchWindow/useLaunchWindowSetup.ts-158-167 (1)
158-167:⚠️ Potential issue | 🟡 MinorAvoid collapsing the HUD during expanded-state transitions.
This cleanup runs before every dependency change, not just unmount. Switching between expanded states, such as
sources→more, briefly sendssetHudOverlayExpanded(false)before setting it back totrue.Proposed fix
useEffect(() => { const expanded = activeDropdown !== "none" || projectBrowserOpen || showRecordingWebcamPreview; window.electronAPI.setHudOverlayExpanded(expanded); +}, [activeDropdown, projectBrowserOpen, showRecordingWebcamPreview]); +useEffect(() => { return () => { window.electronAPI.setHudOverlayExpanded(false); }; -}, [activeDropdown, projectBrowserOpen, showRecordingWebcamPreview]); +}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow/useLaunchWindowSetup.ts` around lines 158 - 167, The current effect that watches activeDropdown, projectBrowserOpen, and showRecordingWebcamPreview calls window.electronAPI.setHudOverlayExpanded(false) in its cleanup, causing a brief collapse when switching expanded states; remove that cleanup and instead add a separate effect with an empty dependency array whose cleanup sets window.electronAPI.setHudOverlayExpanded(false) on unmount only. Concretely: in useLaunchWindowSetup.ts keep the useEffect that computes expanded from activeDropdown/projectBrowserOpen/showRecordingWebcamPreview and call window.electronAPI.setHudOverlayExpanded(expanded) but delete its return cleanup; then add another useEffect(() => () => window.electronAPI.setHudOverlayExpanded(false), []) so the HUD is only collapsed on component unmount.src/components/launch/LaunchWindow/index.tsx-151-157 (1)
151-157:⚠️ Potential issue | 🟡 MinorHandle failures from the recordings-directory IPC call.
If
getRecordingsDirectory()rejects,void load()turns it into an unhandled rejection. Guard the async call so the HUD still mounts cleanly when IPC fails.🛡️ Proposed fix
useEffect(() => { + let mounted = true; const load = async () => { - const result = await window.electronAPI.getRecordingsDirectory(); - if (result.success) setRecordingsDirectory(result.path); + try { + const result = await window.electronAPI.getRecordingsDirectory(); + if (mounted && result.success) setRecordingsDirectory(result.path); + } catch (error) { + console.warn("Failed to load recordings directory:", error); + } }; void load(); + return () => { + mounted = false; + }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow/index.tsx` around lines 151 - 157, The useEffect's async loader (load) calls window.electronAPI.getRecordingsDirectory() without guarding against rejections, causing unhandled promise rejections; update the load function used in the useEffect to wrap the IPC call in a try/catch (or append .catch) around getRecordingsDirectory(), handle/log the error (e.g., console.error or existing logger) and ensure setRecordingsDirectory is only called on success so the HUD mounts cleanly when the IPC call fails (referencing the load function and window.electronAPI.getRecordingsDirectory in the useEffect).src/components/video-editor/projectPersistenceNormalization.ts-98-109 (1)
98-109:⚠️ Potential issue | 🟡 MinorEdge case: crop clamp can produce
width/height≤ 0 whencropX/cropY≈ 1.When the persisted
cropRegion.xis ≥ 0.99,clamp(rawCropWidth, 0.01, 1 - cropX)is called withmax < min. Mostclampimplementations collapse tomaxin that case, producing a width of ~0 (or 0 whencropX === 1), which then flows through the whole render pipeline. Corrupted persisted state is the realistic trigger. A floor likeMath.max(0.01, 1 - cropX)for the max argument (or clampingcropXto1 - 0.01first) guarantees a valid region.🛡️ Suggested tightening
- const cropX = clamp(rawCropX, 0, 1); - const cropY = clamp(rawCropY, 0, 1); - const cropWidth = clamp(rawCropWidth, 0.01, 1 - cropX); - const cropHeight = clamp(rawCropHeight, 0.01, 1 - cropY); + const cropX = clamp(rawCropX, 0, 0.99); + const cropY = clamp(rawCropY, 0, 0.99); + const cropWidth = clamp(rawCropWidth, 0.01, Math.max(0.01, 1 - cropX)); + const cropHeight = clamp(rawCropHeight, 0.01, Math.max(0.01, 1 - cropY));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistenceNormalization.ts` around lines 98 - 109, The clamp for width/height can get max < min when persisted cropRegion.x or .y ≈ 1, producing zero/negative sizes; fix by ensuring the clamp upper bound is at least the minimum size before calling clamp: compute cropX and cropY as shown, then compute maxWidth = Math.max(0.01, 1 - cropX) and maxHeight = Math.max(0.01, 1 - cropY) and use clamp(rawCropWidth, 0.01, maxWidth) and clamp(rawCropHeight, 0.01, maxHeight) (or alternatively clamp cropX/cropY to 1 - 0.01 first) so cropWidth/cropHeight can never be ≤ 0; reference variables/functions: editor.cropRegion, DEFAULT_CROP_REGION, isFiniteNumber, clamp, rawCropWidth/rawCropHeight, cropX/cropY.src/components/video-editor/AnnotationFigureTab.tsx-37-117 (1)
37-117:⚠️ Potential issue | 🟡 MinorNon-null assertions on
annotation.figureDatacan emit malformed updates.The three
...annotation.figureData!spreads (Lines 41, 76, 111) rely onfigureDatabeing present, but the type has it as optional and callers construct the object back throughonFigureDataChange. IffigureDatais everundefined(e.g., legacy/partially initialized annotation),...undefinedis a no-op, so the emitted object will only contain the single changed field — losing the requiredarrowDirection/strokeWidth/colordiscriminants and silently corrupting the annotation.Since this is a pure refactor, behavior matches the pre-split file; still worth an early guard or a default-FigureData fallback to harden the new module boundary.
🛡️ Suggested hardening
-export function AnnotationFigureTab({ annotation, onFigureDataChange }: AnnotationFigureTabProps) { +const DEFAULT_FIGURE_DATA: FigureData = { + arrowDirection: "right", + strokeWidth: 4, + color: "#2563EB", +}; + +export function AnnotationFigureTab({ annotation, onFigureDataChange }: AnnotationFigureTabProps) { const t = useScopedT("editor"); + const figureData = annotation.figureData ?? DEFAULT_FIGURE_DATA;Then replace each
...annotation.figureData!with...figureData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationFigureTab.tsx` around lines 37 - 117, The code spreads annotation.figureData with non-null assertions (e.g., in the onClick handler, Slider onValueChange, and Popover Block onChange) which can silently drop required discriminants if figureData is undefined; fix by introducing a local fallback FigureData (e.g., const figureData: FigureData = annotation.figureData ?? { /* default arrowDirection, strokeWidth, color, ... */ }) at the top of AnnotationFigureTab and replace all uses of ...annotation.figureData! with ...figureData so onFigureDataChange always receives a complete object; ensure the fallback includes the required fields used by the component and reference the symbols annotation.figureData, figureData, onFigureDataChange, and FigureData when making the change.src/components/video-editor/hooks/useEditorPreferences.ts-82-83 (1)
82-83:⚠️ Potential issue | 🟡 MinorPersist
zoomSmoothnessandzoomClassicModewith the other editor preferences.These settings are user-editable, but they reset to hardcoded defaults because the localStorage save effect omits them.
Proposed fix
- const [zoomSmoothness, setZoomSmoothness] = useState(0.5); - const [zoomClassicMode, setZoomClassicMode] = useState(false); + const [zoomSmoothness, setZoomSmoothness] = useState(initial.zoomSmoothness ?? 0.5); + const [zoomClassicMode, setZoomClassicMode] = useState(initial.zoomClassicMode ?? false);cursorSize, cursorSmoothing, + zoomSmoothness, + zoomClassicMode, cursorMotionBlur,cursorSize, cursorSmoothing, + zoomSmoothness, + zoomClassicMode, cursorMotionBlur,Also applies to: 124-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorPreferences.ts` around lines 82 - 83, The preferences hook useEditorPreferences currently initializes zoomSmoothness and zoomClassicMode but doesn't persist them; update the load and save logic that reads/writes editor preferences from localStorage (the effect and serialization code that handles other prefs) to include zoomSmoothness and zoomClassicMode so they are restored on init and saved on change; specifically add these keys to the object used in JSON.parse/JSON.stringify and ensure setZoomSmoothness and setZoomClassicMode are invoked when loading stored preferences.src/components/video-editor/EditorSidebar.tsx-138-139 (1)
138-139:⚠️ Potential issue | 🟡 MinorLocalize the account placeholder strings.
Account coming soonandAccountbypass the i18n path used by the other sidebar labels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorSidebar.tsx` around lines 138 - 139, The hardcoded strings in EditorSidebar.tsx for the Account sidebar item (the onClick toast message "Account coming soon" and the title prop "Account") should be replaced with localized strings via the app's i18n helper (e.g., the useTranslation hook or t function). Update the component where the onClick and title are set to call the translation keys (for example sidebar.account and sidebar.accountComingSoon) instead of literal text, ensuring you import/use the same i18n instance used by other sidebar labels so the toast message and the title are both localized.src/components/video-editor/AnnotationTextTab.tsx-95-97 (1)
95-97:⚠️ Potential issue | 🟡 MinorRoute remaining visible labels through i18n.
Custom FontsandColorare user-facing strings in an otherwise localized settings panel.Also applies to: 273-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationTextTab.tsx` around lines 95 - 97, The static user-facing labels "Custom Fonts" and "Color" in AnnotationTextTab.tsx should be routed through the app i18n so they are localized; replace the literal strings in the JSX (the div rendering "Custom Fonts" around the class px-2 py-1.5... and the element rendering "Color" at the other occurrence) with translated values using the project's translation hook/function (e.g., useTranslation().t or i18n.t) and use appropriate translation keys like annotationText.customFonts and annotationText.color, adding those keys to your locale files; ensure you import/use the same translation helper already used in this component.src/components/video-editor/AnnotationBlurTab.tsx-71-99 (1)
71-99:⚠️ Potential issue | 🟡 MinorLocalize the color button labels.
Black,White, andCustom Colorare user-facing title/accessibility strings, while the surrounding controls uset(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationBlurTab.tsx` around lines 71 - 99, The button title strings "Black", "White", and "Custom Color" in AnnotationBlurTab should be localized; replace the hardcoded titles with calls to the translation function (e.g., t('...')) used in the component so accessibility/tooltips use translated text. Update the title props on the two plain buttons and the PopoverTrigger button to use t(…) keys (for example t('color.black'), t('color.white'), t('color.custom') or your existing key names), and ensure the component imports/has access to the same translation function used elsewhere in the file.src/components/video-editor/extension-manager/ExtensionManagerCards.tsx-251-277 (1)
251-277:⚠️ Potential issue | 🟡 MinorLabel the screenshot carousel controls.
The previous/next and pagination buttons are icon/dot-only, so they currently have no accessible names.
♿ Proposed fix
<button type="button" + aria-label={t("detail.previousScreenshot")} className="absolute left-1 top-1/2 -translate-y-1/2 w-6 h-6 rounded-full bg-editor-bg/80 text-foreground/80 flex items-center justify-center opacity-0 group-hover:opacity-100 transition-opacity hover:bg-editor-bg/80" @@ <button type="button" + aria-label={t("detail.nextScreenshot")} className="absolute right-1 top-1/2 -translate-y-1/2 w-6 h-6 rounded-full bg-editor-bg/80 text-foreground/80 flex items-center justify-center opacity-0 group-hover:opacity-100 transition-opacity hover:bg-editor-bg/80" @@ <button type="button" key={screenshotIndex} + aria-label={t("detail.goToScreenshot", undefined, { + number: String(screenshotIndex + 1), + })} className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/extension-manager/ExtensionManagerCards.tsx` around lines 251 - 277, The carousel controls in ExtensionManagerCards.tsx (the ChevronLeft/ChevronRight buttons and the pagination dot buttons rendered from screenshots) lack accessible names; add descriptive aria-label attributes to the previous/next buttons (e.g., "Previous screenshot" and "Next screenshot" or include current/total context) and to each dot button (e.g., `aria-label="Show screenshot {n} of {count}"`) and mark the active dot with aria-current="true" (or aria-pressed) so screen readers can identify the current slide; update the onClick handlers using setIndex as-is and ensure labels use screenshotIndex and count to generate meaningful text.src/components/video-editor/EditorToolbar.tsx-211-254 (1)
211-254:⚠️ Potential issue | 🟡 MinorAdd accessible names to the volume controls.
The mute button and invisible range input need explicit labels; otherwise screen reader users can land on an unnamed slider/control.
♿ Proposed fix
<button type="button" + aria-label={t("editor.playback.muteUnmute")} className="text-muted-foreground hover:text-foreground transition-colors" title={t("editor.playback.muteUnmute")} @@ <input type="range" + aria-label={t("editor.playback.volume")} min="0" max="1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorToolbar.tsx` around lines 211 - 254, The mute button and invisible range input in EditorToolbar.tsx lack accessible names—add an explicit aria-label (or aria-labelledby) to the mute <button> (in the block with title={t("editor.playback.muteUnmute")}) and to the <input type="range"> so screen readers announce their purpose and current value; you can reuse the existing i18n key t("editor.playback.muteUnmute") for the button and add a localized label like t("editor.playback.volume") for the slider (or give the visible percentage <span> an id and reference it from the slider via aria-labelledby) and also include aria-valuemin/aria-valuemax/aria-valuenow on the range to expose its numeric value.src/components/video-editor/EditorHeader.tsx-351-351 (1)
351-351:⚠️ Potential issue | 🟡 MinorWindows paths won't render correctly with
split("/").pop().Recordly is an Electron desktop app, and on Windows
exp.exportedFilePathis backslash-separated, sosplit("/").pop()returns the whole path instead of just the filename. Consider splitting on both separators (e.g.,exp.exportedFilePath.split(/[\\/]/).pop()) or using a path utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorHeader.tsx` at line 351, The filename extraction using exp.exportedFilePath.split("/").pop() fails on Windows because paths use backslashes; update the usage in EditorHeader (where exp.exportedFilePath is rendered) to handle both separators by splitting on a regex like /[\\/]/ or use a path utility (e.g., Node's path.basename) to reliably return only the filename from exp.exportedFilePath; replace the split("/").pop() call with the cross-platform solution in the JSX rendering.src/components/video-editor/EditorHeader.tsx-247-367 (1)
247-367:⚠️ Potential issue | 🟡 MinorHard-coded English strings bypass i18n.
Most of the header uses
t(...), but several user-facing strings in the export dropdown are hard-coded English and won't localize:
- Line 247
PLEASE, line 253report bugs, line 255with Lightning export- Line 261
Export too slow? Cancel and try Lightning export!- Line 291
Audio requires real-time playback for speed/overlay edits- Lines 300, 311, 347
Path: {label}- Line 367
Done(the completion dropdown; line 338 usest("common.actions.done")for the crop modal — same label should use the same key here)At minimum, reuse existing keys where they exist (e.g.,
t("common.actions.done")on line 367) and add i18n keys for the Lightning/Audio/Path strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorHeader.tsx` around lines 247 - 367, Replace the hard-coded English strings in EditorHeader's export dropdown with i18n lookups: change "PLEASE", "report bugs", and "with Lightning export" to a single localized sentence (suggest key editor.export.lightningReport), replace "Export too slow? Cancel and try Lightning export!" with editor.export.slowExport, replace "Audio requires real-time playback for speed/overlay edits" with editor.export.audioRealtimeNotice, replace the "Path: {exportRuntimeLabel}" occurrences with a localized prefix using editor.export.pathLabel (e.g., t("editor.export.pathLabel", "Path: {{path}}", { path: exportRuntimeLabel })), and swap the hard-coded "Done" button text to use the existing common.actions.done key; update usages around the exp object (exp.handleCancelExport, exp.handleRetrySaveExport, exp.handleExportDropdownClose) and revealExportedFile to render the new t(...) strings and add the new keys to the locale files.src/components/video-editor/hooks/useEditorProject.ts-140-141 (1)
140-141:⚠️ Potential issue | 🟡 MinorDead code:
resolveVideoUrlresult discarded.
const videoUrl = await resolveVideoUrl(sourcePath); void videoUrl;awaits a resolution whose result is never used. If this is intentional (e.g., to prime a cache or trigger Electron-side registration), please add a comment explaining why; otherwise drop the two lines. As written it reads like leftover scaffolding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorProject.ts` around lines 140 - 141, The code awaits resolveVideoUrl(sourcePath) into videoUrl and then discards it (const videoUrl = await resolveVideoUrl(sourcePath); void videoUrl;), which is dead code; either remove the await/assignment entirely or actually use the returned value. If the call is required solely for a side-effect (e.g., cache priming or Electron registration), keep the await but replace the void line with a clear comment explaining the side-effect intent; otherwise delete both lines or refactor to use videoUrl where intended (search for resolveVideoUrl and videoUrl in useEditorProject to locate the spot).src/components/video-editor/hooks/useEditorCaptions.ts-138-175 (1)
138-175:⚠️ Potential issue | 🟡 MinorValidate
whisperModelPathbefore mutating video source state.If the user hasn't selected/downloaded a Whisper model,
handleGenerateAutoCaptionsstill falls through tosetVideoSourcePath/setVideoPathandsyncActiveVideoSource(...)before reaching thewhisperModelPathguard on line 172. That causes an unnecessary source swap + remount and delays the error toast. Move the model check above the source-sync block so we bail early on missing prerequisites.🛠️ Suggested reorder
if (!sourcePath) { toast.error("No source video is loaded"); return; } + if (!whisperModelPath) { + toast.error("Select a Whisper model or download the small model first"); + return; + } + if (sourcePath !== videoSourcePath) { setVideoSourcePath(sourcePath); setVideoPath(await resolveVideoUrl(sourcePath)); } await syncActiveVideoSource(sourcePath, webcamSourcePath ?? null); - - if (!whisperModelPath) { - toast.error("Select a Whisper model or download the small model first"); - return; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorCaptions.ts` around lines 138 - 175, handleGenerateAutoCaptions currently mutates video state and calls syncActiveVideoSource before checking whisperModelPath, causing unnecessary source swaps and remounts; move the whisperModelPath guard to the top of the function (inside handleGenerateAutoCaptions, before any calls to setVideoSourcePath, setVideoPath or syncActiveVideoSource) so the function bails with toast.error("Select a Whisper model or download the small model first") immediately when whisperModelPath is falsy, preserving the existing resolution logic for sourcePath and only performing state updates/sync after the model check passes.
🧹 Nitpick comments (7)
src/components/video-editor/CursorStylePreview.tsx (1)
10-12: NarrowpreviewUrlsto cursor-style keys if possible.
Partial<Record<string, string>>allows unrelated keys and misses typos likepreviewUrls.fgima. IfCursorStyleis the complete key set, prefer typing this asPartial<Record<CursorStyle, string>>.Type-safety refinement
}: { style: CursorStyle; - previewUrls: Partial<Record<string, string>>; + previewUrls: Partial<Record<CursorStyle, string>>; }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/CursorStylePreview.tsx` around lines 10 - 12, The prop type for previewUrls is too permissive; narrow it to only accept keys from the CursorStyle union so typos are caught. Update the component prop signature (the destructured props around style: CursorStyle and previewUrls) to type previewUrls as Partial<Record<CursorStyle, string>> instead of Partial<Record<string, string>> (referencing CursorStyle and previewUrls in CursorStylePreview.tsx) so only valid cursor-style keys are allowed.src/components/video-editor/projectPersistencePaths.ts (1)
70-78: Escapeprefixbefore embedding it into aRegExp.
new RegExp(\^${prefix}-(\d+)$`)treats regex metacharacters inprefixliterally as regex syntax. Current callers pass safe literals (e.g.,"zoom"), so nothing is broken today, but if a future prefix contains.,+,(, etc., IDs will silently fail to match andderiveNextIdwill keep returning1`, producing duplicate IDs. Also resolves the static-analysis ReDoS warning.♻️ Diff
export function deriveNextId(prefix: string, ids: string[]): number { + const escaped = prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const pattern = new RegExp(`^${escaped}-(\\d+)$`); const max = ids.reduce((acc, id) => { - const match = id.match(new RegExp(`^${prefix}-(\\d+)$`)); + const match = id.match(pattern); if (!match) return acc; const value = Number(match[1]); return Number.isFinite(value) ? Math.max(acc, value) : acc; }, 0); return max + 1; }Bonus: compiling the regex once outside the
reduceavoids recreating it per element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistencePaths.ts` around lines 70 - 78, deriveNextId currently builds a RegExp from prefix inside the reduce which treats regex metacharacters in prefix as regex syntax and recreates the regex per id; fix by escaping prefix before interpolating it into the pattern and compile the RegExp once outside the reduce (use the escapedPrefix in the pattern `^${escapedPrefix}-(\d+)$`) so matching is safe for any prefix and avoids per-iteration regex construction; update deriveNextId to use the single precompiled regex when iterating over ids.src/components/video-editor/AnnotationFigureTab.tsx (1)
37-61: Addtype="button"to the arrow-direction buttons.Without an explicit type, these default to
submitand will trigger form submission if the panel is ever rendered inside a<form>. Cheap defensive win now that this is its own component.♻️ Diff
<button key={direction} + type="button" onClick={() =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationFigureTab.tsx` around lines 37 - 61, The arrow-direction buttons in AnnotationFigureTab are missing an explicit type and will default to type="submit" inside forms; update the button element rendering (the JSX that renders the button with key={direction} and onClick calling onFigureDataChange) to include type="button" so clicking ArrowComponent buttons won't submit any parent form; ensure you add type="button" to all similar arrow-direction buttons in this component.src/components/video-editor/hooks/useEditorCursorTelemetry.ts (1)
34-69: Optional: extract the retry-scheduling block to remove duplication.The "should retry? then bump attempts + schedule 350 ms timeout" sequence is repeated verbatim in both the success (Lines 43–53) and
catch(Lines 57–67) branches. A small inner helper would make the retry policy easier to evolve (e.g., backoff tuning) without two places to sync.♻️ Suggested extraction
async function loadTelemetry() { if (!videoPath || !videoSourcePath) { if (mounted) setCursorTelemetry([]); return; } + const maybeScheduleRetry = () => { + const shouldRetry = + pendingFreshRecordingAutoZoomPathRef.current === videoPath && + autoSuggestedVideoPathRef.current !== videoPath && + retryAttempts < 12; + if (!shouldRetry) return; + retryAttempts += 1; + retryTimeoutRef.current = window.setTimeout(() => { + retryTimeoutRef.current = null; + if (mounted) void loadTelemetry(); + }, 350); + }; try { const result = await window.electronAPI.getCursorTelemetry(videoSourcePath); if (!mounted) return; setCursorTelemetry(result.success ? result.samples : []); - const shouldRetry = ... - if (shouldRetry) { ... } + maybeScheduleRetry(); } catch { if (!mounted) return; setCursorTelemetry([]); - const shouldRetry = ... - if (shouldRetry) { ... } + maybeScheduleRetry(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorCursorTelemetry.ts` around lines 34 - 69, The retry scheduling logic is duplicated inside loadTelemetry's try and catch branches; extract it into a small inner helper (e.g., scheduleRetry or tryScheduleRetry) defined in the same scope as loadTelemetry that reads pendingFreshRecordingAutoZoomPathRef.current, autoSuggestedVideoPathRef.current, retryAttempts, retryTimeoutRef and mounted, computes the shouldRetry condition, increments retryAttempts, and sets retryTimeoutRef via window.setTimeout to call loadTelemetry after 350ms; then replace the duplicated blocks in loadTelemetry with a call to that helper to keep behavior identical.src/components/video-editor/projectPersistenceNormalization.ts (1)
122-122:shadowIntensityandborderRadiusacceptNaNviatypeof === "number".Unlike the surrounding fields (which use
isFiniteNumber),shadowIntensity(Line 122) andborderRadius(Line 165) only gate ontypeof === "number", so a persistedNaN/Infinitywould be passed through and poison downstream arithmetic/rendering. Likely pre-existing — worth tightening now that this module owns normalization.♻️ Diff
- shadowIntensity: typeof editor.shadowIntensity === "number" ? editor.shadowIntensity : 0.67, + shadowIntensity: isFiniteNumber(editor.shadowIntensity) ? editor.shadowIntensity : 0.67, ... - borderRadius: typeof editor.borderRadius === "number" ? editor.borderRadius : 12.5, + borderRadius: isFiniteNumber(editor.borderRadius) ? editor.borderRadius : 12.5,Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistenceNormalization.ts` at line 122, The normalization allows NaN/Infinity through because shadowIntensity and borderRadius use typeof === "number"; change both checks to use the existing isFiniteNumber helper (e.g., isFiniteNumber(editor.shadowIntensity) ? editor.shadowIntensity : 0.67 and isFiniteNumber(editor.borderRadius) ? editor.borderRadius : <existing default>) so only finite numeric values are accepted and persisted, preserving the current default fallbacks; update the assignments in the shadowIntensity and borderRadius normalization locations accordingly.src/components/video-editor/SettingsPanel.tsx (1)
40-142: PruneSettingsPanelPropsfields that are no longer destructured or forwarded.After the extraction, several props in the interface are never destructured on lines 144‑228 and are not threaded into any section component:
zoomInDurationMs/onZoomInDurationMsChange,zoomInOverlapMs/onZoomInOverlapMsChange,zoomOutDurationMs/onZoomOutDurationMsChange,connectedZoomGapMs/onConnectedZoomGapMsChange,connectedZoomDurationMs/onConnectedZoomDurationMsChange,zoomInEasing/onZoomInEasingChange,zoomOutEasing/onZoomOutEasingChange,connectedZoomEasing/onConnectedZoomEasingChange,whisperExecutablePath, andonPickWhisperExecutable. Callers can still pass them and will see no effect — which tends to hide regressions. For a pure refactor, I'd either drop them from the interface or wire them through to the matching section (ZoomSection,CaptionsSection).🤖 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 40 - 142, The SettingsPanelProps interface includes several props that are no longer used or forwarded (zoomInDurationMs, onZoomInDurationMsChange, zoomInOverlapMs, onZoomInOverlapMsChange, zoomOutDurationMs, onZoomOutDurationMsChange, connectedZoomGapMs, onConnectedZoomGapMsChange, connectedZoomDurationMs, onConnectedZoomDurationMsChange, zoomInEasing, onZoomInEasingChange, zoomOutEasing, onZoomOutEasingChange, connectedZoomEasing, onConnectedZoomEasingChange, whisperExecutablePath, onPickWhisperExecutable); either remove these fields from SettingsPanelProps (preferred for a pure refactor) or wire them through when destructuring in SettingsPanel and pass them down to the corresponding child components (ZoomSection for the zoom* props and CaptionsSection for whisperExecutablePath/onPickWhisperExecutable) so callers see effects—update the interface and the SettingsPanel component (and props passed to ZoomSection/CaptionsSection) accordingly.src/components/video-editor/EditorContent.tsx (1)
20-20: ImportVideoPlaybackRefwithtypekeyword for clarity and TypeScript best practices.
VideoPlaybackRefis a type-only export from./VideoPlayback.tsxand is used purely as a type (lines 46 and 177). While the current tsconfig (isolatedModules: true) allows this regular import to compile, explicitly using thetypekeyword follows TypeScript best practices and future-proofs the code against configurations likeverbatimModuleSyntax: true.♻️ Proposed fix
-import VideoPlayback, { VideoPlaybackRef } from "./VideoPlayback"; +import VideoPlayback, { type VideoPlaybackRef } from "./VideoPlayback";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorContent.tsx` at line 20, The import for the type-only export should use TypeScript's type-only import syntax: change the import statement that brings in VideoPlaybackRef from "./VideoPlayback" to explicitly import it as a type (i.e., use the type keyword with VideoPlaybackRef while keeping the default/component import VideoPlayback unchanged) so references to VideoPlaybackRef in this file (e.g., usages around lines referencing the component's ref) are clearly marked as type-only and future-proofed for stricter TS module settings.
🤖 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/launch/LaunchWindow/helperComponents.tsx`:
- Around line 75-78: The audio meter is being enabled for every MicDeviceRow
causing a MediaStream per device; change the enabled flag passed to
useAudioLevelMeter so it only enables for the currently selected device (e.g.,
enabled: device.deviceId === selectedDevice?.deviceId or device.deviceId ===
selectedDeviceId). Update MicDeviceRow props or its parent to pass the
selectedDevice/selectedDeviceId so the comparison is possible, and ensure
toggling enabled to false will stop/unsubscribe the meter when the device is not
selected.
In `@src/components/launch/LaunchWindow/HudControls.tsx`:
- Around line 95-148: The microphone IconButton in RecordingControls renders
based on microphoneEnabled but has no onClick; add a toggle handler prop and
wire it: extend RecordingControlsProps with a toggleMicrophone: () => void,
accept toggleMicrophone in the RecordingControls parameter list, and set
IconButton's onClick to call toggleMicrophone (keeping the existing
microphoneEnabled logic and className). Also update the parent call site to pass
the recording-safe microphone toggle function into RecordingControls.
In `@src/components/launch/LaunchWindow/index.tsx`:
- Around line 128-132: The effect handling selectedVideoDeviceId currently
ignores the "default" value and leaves the previous webcam active; update the
useEffect that watches selectedVideoDeviceId (and calls setWebcamDeviceId) so it
sets webcamDeviceId to the selected id when it’s a real device and clears it
when selectedVideoDeviceId === "default" (e.g., call setWebcamDeviceId(undefined
or empty string) for the "default" case) to ensure switching back to Default
disables the explicit webcamDeviceId.
In `@src/components/video-editor/AnnotationImageTab.tsx`:
- Around line 19-43: The current upload flow validates MIME types but doesn't
guard large files and can load huge images into memory; add a file-size check
before creating/using FileReader (e.g., define MAX_IMAGE_SIZE_BYTES and check
files[0].size) and if the file exceeds the limit call toast.error with a
descriptive message, clear event.target.value and return early; keep the
existing MIME validation, FileReader usage (reader, reader.onload,
reader.onerror), and onContentChange the same but only invoke them after the
size and type checks pass.
In `@src/components/video-editor/AnnotationTextTab.tsx`:
- Around line 61-63: In AnnotationTextTab, the textarea value uses the logical
OR (annotation.textContent || annotation.content) which prevents clearing
textContent because empty string falls back; change the value expression to use
nullish coalescing (annotation.textContent ?? annotation.content) so an
intentionally empty string is preserved, and keep the onChange handler
(onContentChange(event.target.value)) as-is.
In `@src/components/video-editor/EditorSidebar.tsx`:
- Around line 88-92: The sidebar buttons (motion.button in EditorSidebar that
calls prefs.setActiveEffectSection) remove keyboard focus styling via the class
focus-visible:outline-none, making keyboard navigation invisible; remove that
utility (or replace it with an accessible focus style such as focus-visible:ring
or focus-visible:outline) from the button's className (and the other affected
buttons at the same pattern around lines 136-140) so the keyboard focus state is
visible again while keeping existing focus:outline-none for mouse focus if
desired.
In `@src/components/video-editor/extension-manager/ExtensionDetailModal.tsx`:
- Line 9: The modal's visible heading in ExtensionDetailModal is a plain h2 and
not wired to the dialog primitive; import and use the DialogTitle component
(from the same dialog wrapper as Dialog and DialogContent) to wrap the heading
so the dialog has an accessible name. In the ExtensionDetailModal component
replace the standalone <h2> with <DialogTitle>...<DialogTitle> (keeping the same
text and styling) and ensure DialogTitle is imported alongside Dialog and
DialogContent so assistive tech receives the modal label.
In `@src/components/video-editor/extension-manager/ExtensionManagerCards.tsx`:
- Around line 37-47: The clickable card divs in ExtensionManagerCards.tsx (the
container with className built by cn and the other card at the second
occurrence) are not keyboard-accessible; change them to either a real <button>
or add role="button", tabIndex={0}, and an onKeyDown handler that triggers the
same onClick when Enter or Space is pressed (handle key === "Enter" || key === "
" / keyCode 13/32 for broader support), and add an appropriate
aria-label/aria-pressed or aria-expanded as needed; update the JSX for the
elements that currently use onClick (the container divs) and ensure onClick
remains for pointer users while onKeyDown and tabIndex enable keyboard
activation for the ExtensionManagerCards component.
In `@src/components/video-editor/ExtensionManager.tsx`:
- Around line 124-152: The handler handleMarketplaceInstall currently only uses
try/finally so rejections from marketplaceInstall can become unhandled and no
failure toast is shown; update it to catch errors from marketplaceInstall (catch
block after the try) and in that catch call toast.error with a localized message
(t("toast.marketplaceInstallFailed", ..., { name: extension.name })) including
the actual error message/stack (or error.toString()), optionally log the error,
and ensure setInstallingIds cleanup remains in finally; reference
marketplaceInstall, setInstallingIds, toast, setMarketplaceResults, and
setDetailData when making the change.
In `@src/components/video-editor/hooks/editorExportWorkflow.ts`:
- Around line 172-218: The GIF export branches need to emit the same
smoke-export report as MP4 before closing: call writeSmokeExportReport(...) with
the appropriate result metadata (path when available, canceled flag, and
error/message) before each window.close() where smokeExportConfig.enabled is
true — specifically add calls in the save-canceled branch (after
setPendingExportSave and before window.close()), in the save-success branch when
smokeExportConfig.enabled (before window.close()), in the save-failed branch
when smokeExportConfig.enabled (before window.close()), and in the export-failed
branch when smokeExportConfig.enabled (before window.close())); reference the
existing symbols saveResult, result, smokeExportConfig, smokeExportStartedAt,
setExportedFilePath, setExportError to assemble the report payload consistent
with the MP4 implementation.
- Around line 102-107: The restore of preview playback/state (capturing
config.isPlaying as wasPlaying, video.currentTime as restoreTime, and using
videoPlaybackRef.current to pause/resume) must be moved out of the try block and
executed in a finally clause before any remount/cleanup logic; update the
function containing the try so that you capture wasPlaying and restoreTime
before awaiting export/save steps, pause playback as needed inside try, and then
in finally always set video.currentTime back to restoreTime and if wasPlaying
call videoPlaybackRef.current?.play() (or resume via the same API) before
performing remount cleanup or unmount steps.
In `@src/components/video-editor/hooks/useEditorExport.ts`:
- Around line 210-233: The retry path in handleRetrySaveExport can leave
rejections unhandled because window.electronAPI.saveExportedVideo may throw;
wrap the call in a try/catch around the await to catch thrown errors from
saveExportedVideo (and any bridge/filesystem failures), and in the catch
setExportError with the error message (or a fallback), toast.error it, and
ensure you still return early; keep the existing handling for
saveResult.canceled and saveResult.success/path and retain calls to
clearPendingExportSave, setExportedFilePath, showExportSuccessToast, and
setShowExportDropdown.
In `@src/components/video-editor/hooks/useEditorWiring.ts`:
- Around line 103-137: The memoized dimension calculations (gifOutputDimensions,
desiredMp4SourceDimensions, and thus mp4OutputDimensions) currently read
videoPlaybackRef.current?.video?.videoWidth/Height but don’t include those
intrinsic dimensions in their dependency arrays, so they capture the fallback
sizes and never recompute; fix by surfacing the video’s intrinsic width/height
into reactive state (e.g., add videoWidth/videoHeight state updated from
VideoPlayback via onLoadedMetadata/onDurationChange or accept them through
UseEditorWiringParams) and then include those state values in the useMemo deps
for gifOutputDimensions and desiredMp4SourceDimensions (and consequently
mp4OutputDimensions) so they recompute when the real video metadata arrives.
In `@src/components/video-editor/projectPersistencePaths.ts`:
- Around line 38-68: fromFileUrl currently returns Windows drive paths with
forward slashes in the branch matching /^\/[A-Za-z]:/ (inside function
fromFileUrl), causing inconsistent separators vs UNC handling; update that
branch to convert forward slashes to backslashes (apply the same .replace(/\//g,
"\\") used for UNC paths) before returning so Windows drive results restore
native backslashes and round-trip with toFileUrl is consistent.
---
Minor comments:
In `@src/components/launch/LaunchWindow/helperComponents.tsx`:
- Around line 62-64: The Separator component references styles.ddSep which
doesn't exist in the CSS module; either add a .ddSep class to the LaunchWindow
CSS module (e.g., duplicate or alias the existing .sep rules so dropdown
separators get the same styling) or remove the dropdown variant and always use
styles.sep; update the CSS module to include .ddSep with the intended styles and
keep the Separator signature (function Separator) unchanged.
In `@src/components/launch/LaunchWindow/index.tsx`:
- Around line 151-157: The useEffect's async loader (load) calls
window.electronAPI.getRecordingsDirectory() without guarding against rejections,
causing unhandled promise rejections; update the load function used in the
useEffect to wrap the IPC call in a try/catch (or append .catch) around
getRecordingsDirectory(), handle/log the error (e.g., console.error or existing
logger) and ensure setRecordingsDirectory is only called on success so the HUD
mounts cleanly when the IPC call fails (referencing the load function and
window.electronAPI.getRecordingsDirectory in the useEffect).
In `@src/components/launch/LaunchWindow/LaunchWindow.module.css`:
- Around line 85-96: Stylelint fails because keyframe names use camelCase;
rename the keyframes (e.g., updateBadgeSpin -> update-badge-spin and menuCardIn
-> menu-card-in) and update all references to them (animation or animation-name
in .updateBadgeSpin and wherever menuCardIn is used) so the `@keyframes`
declarations and the animation properties consistently use kebab-case names.
In `@src/components/launch/LaunchWindow/useLaunchWindowSetup.ts`:
- Around line 158-167: The current effect that watches activeDropdown,
projectBrowserOpen, and showRecordingWebcamPreview calls
window.electronAPI.setHudOverlayExpanded(false) in its cleanup, causing a brief
collapse when switching expanded states; remove that cleanup and instead add a
separate effect with an empty dependency array whose cleanup sets
window.electronAPI.setHudOverlayExpanded(false) on unmount only. Concretely: in
useLaunchWindowSetup.ts keep the useEffect that computes expanded from
activeDropdown/projectBrowserOpen/showRecordingWebcamPreview and call
window.electronAPI.setHudOverlayExpanded(expanded) but delete its return
cleanup; then add another useEffect(() => () =>
window.electronAPI.setHudOverlayExpanded(false), []) so the HUD is only
collapsed on component unmount.
In `@src/components/video-editor/AnnotationBlurTab.tsx`:
- Around line 71-99: The button title strings "Black", "White", and "Custom
Color" in AnnotationBlurTab should be localized; replace the hardcoded titles
with calls to the translation function (e.g., t('...')) used in the component so
accessibility/tooltips use translated text. Update the title props on the two
plain buttons and the PopoverTrigger button to use t(…) keys (for example
t('color.black'), t('color.white'), t('color.custom') or your existing key
names), and ensure the component imports/has access to the same translation
function used elsewhere in the file.
In `@src/components/video-editor/AnnotationFigureTab.tsx`:
- Around line 37-117: The code spreads annotation.figureData with non-null
assertions (e.g., in the onClick handler, Slider onValueChange, and Popover
Block onChange) which can silently drop required discriminants if figureData is
undefined; fix by introducing a local fallback FigureData (e.g., const
figureData: FigureData = annotation.figureData ?? { /* default arrowDirection,
strokeWidth, color, ... */ }) at the top of AnnotationFigureTab and replace all
uses of ...annotation.figureData! with ...figureData so onFigureDataChange
always receives a complete object; ensure the fallback includes the required
fields used by the component and reference the symbols annotation.figureData,
figureData, onFigureDataChange, and FigureData when making the change.
In `@src/components/video-editor/AnnotationTextTab.tsx`:
- Around line 95-97: The static user-facing labels "Custom Fonts" and "Color" in
AnnotationTextTab.tsx should be routed through the app i18n so they are
localized; replace the literal strings in the JSX (the div rendering "Custom
Fonts" around the class px-2 py-1.5... and the element rendering "Color" at the
other occurrence) with translated values using the project's translation
hook/function (e.g., useTranslation().t or i18n.t) and use appropriate
translation keys like annotationText.customFonts and annotationText.color,
adding those keys to your locale files; ensure you import/use the same
translation helper already used in this component.
In `@src/components/video-editor/EditorHeader.tsx`:
- Line 351: The filename extraction using exp.exportedFilePath.split("/").pop()
fails on Windows because paths use backslashes; update the usage in EditorHeader
(where exp.exportedFilePath is rendered) to handle both separators by splitting
on a regex like /[\\/]/ or use a path utility (e.g., Node's path.basename) to
reliably return only the filename from exp.exportedFilePath; replace the
split("/").pop() call with the cross-platform solution in the JSX rendering.
- Around line 247-367: Replace the hard-coded English strings in EditorHeader's
export dropdown with i18n lookups: change "PLEASE", "report bugs", and "with
Lightning export" to a single localized sentence (suggest key
editor.export.lightningReport), replace "Export too slow? Cancel and try
Lightning export!" with editor.export.slowExport, replace "Audio requires
real-time playback for speed/overlay edits" with
editor.export.audioRealtimeNotice, replace the "Path: {exportRuntimeLabel}"
occurrences with a localized prefix using editor.export.pathLabel (e.g.,
t("editor.export.pathLabel", "Path: {{path}}", { path: exportRuntimeLabel })),
and swap the hard-coded "Done" button text to use the existing
common.actions.done key; update usages around the exp object
(exp.handleCancelExport, exp.handleRetrySaveExport,
exp.handleExportDropdownClose) and revealExportedFile to render the new t(...)
strings and add the new keys to the locale files.
In `@src/components/video-editor/EditorSidebar.tsx`:
- Around line 138-139: The hardcoded strings in EditorSidebar.tsx for the
Account sidebar item (the onClick toast message "Account coming soon" and the
title prop "Account") should be replaced with localized strings via the app's
i18n helper (e.g., the useTranslation hook or t function). Update the component
where the onClick and title are set to call the translation keys (for example
sidebar.account and sidebar.accountComingSoon) instead of literal text, ensuring
you import/use the same i18n instance used by other sidebar labels so the toast
message and the title are both localized.
In `@src/components/video-editor/EditorToolbar.tsx`:
- Around line 211-254: The mute button and invisible range input in
EditorToolbar.tsx lack accessible names—add an explicit aria-label (or
aria-labelledby) to the mute <button> (in the block with
title={t("editor.playback.muteUnmute")}) and to the <input type="range"> so
screen readers announce their purpose and current value; you can reuse the
existing i18n key t("editor.playback.muteUnmute") for the button and add a
localized label like t("editor.playback.volume") for the slider (or give the
visible percentage <span> an id and reference it from the slider via
aria-labelledby) and also include aria-valuemin/aria-valuemax/aria-valuenow on
the range to expose its numeric value.
In `@src/components/video-editor/extension-manager/ExtensionManagerCards.tsx`:
- Around line 251-277: The carousel controls in ExtensionManagerCards.tsx (the
ChevronLeft/ChevronRight buttons and the pagination dot buttons rendered from
screenshots) lack accessible names; add descriptive aria-label attributes to the
previous/next buttons (e.g., "Previous screenshot" and "Next screenshot" or
include current/total context) and to each dot button (e.g., `aria-label="Show
screenshot {n} of {count}"`) and mark the active dot with aria-current="true"
(or aria-pressed) so screen readers can identify the current slide; update the
onClick handlers using setIndex as-is and ensure labels use screenshotIndex and
count to generate meaningful text.
In `@src/components/video-editor/hooks/useEditorCaptions.ts`:
- Around line 138-175: handleGenerateAutoCaptions currently mutates video state
and calls syncActiveVideoSource before checking whisperModelPath, causing
unnecessary source swaps and remounts; move the whisperModelPath guard to the
top of the function (inside handleGenerateAutoCaptions, before any calls to
setVideoSourcePath, setVideoPath or syncActiveVideoSource) so the function bails
with toast.error("Select a Whisper model or download the small model first")
immediately when whisperModelPath is falsy, preserving the existing resolution
logic for sourcePath and only performing state updates/sync after the model
check passes.
In `@src/components/video-editor/hooks/useEditorPreferences.ts`:
- Around line 82-83: The preferences hook useEditorPreferences currently
initializes zoomSmoothness and zoomClassicMode but doesn't persist them; update
the load and save logic that reads/writes editor preferences from localStorage
(the effect and serialization code that handles other prefs) to include
zoomSmoothness and zoomClassicMode so they are restored on init and saved on
change; specifically add these keys to the object used in
JSON.parse/JSON.stringify and ensure setZoomSmoothness and setZoomClassicMode
are invoked when loading stored preferences.
In `@src/components/video-editor/hooks/useEditorProject.ts`:
- Around line 140-141: The code awaits resolveVideoUrl(sourcePath) into videoUrl
and then discards it (const videoUrl = await resolveVideoUrl(sourcePath); void
videoUrl;), which is dead code; either remove the await/assignment entirely or
actually use the returned value. If the call is required solely for a
side-effect (e.g., cache priming or Electron registration), keep the await but
replace the void line with a clear comment explaining the side-effect intent;
otherwise delete both lines or refactor to use videoUrl where intended (search
for resolveVideoUrl and videoUrl in useEditorProject to locate the spot).
In `@src/components/video-editor/projectPersistenceNormalization.ts`:
- Around line 98-109: The clamp for width/height can get max < min when
persisted cropRegion.x or .y ≈ 1, producing zero/negative sizes; fix by ensuring
the clamp upper bound is at least the minimum size before calling clamp: compute
cropX and cropY as shown, then compute maxWidth = Math.max(0.01, 1 - cropX) and
maxHeight = Math.max(0.01, 1 - cropY) and use clamp(rawCropWidth, 0.01,
maxWidth) and clamp(rawCropHeight, 0.01, maxHeight) (or alternatively clamp
cropX/cropY to 1 - 0.01 first) so cropWidth/cropHeight can never be ≤ 0;
reference variables/functions: editor.cropRegion, DEFAULT_CROP_REGION,
isFiniteNumber, clamp, rawCropWidth/rawCropHeight, cropX/cropY.
---
Nitpick comments:
In `@src/components/video-editor/AnnotationFigureTab.tsx`:
- Around line 37-61: The arrow-direction buttons in AnnotationFigureTab are
missing an explicit type and will default to type="submit" inside forms; update
the button element rendering (the JSX that renders the button with
key={direction} and onClick calling onFigureDataChange) to include type="button"
so clicking ArrowComponent buttons won't submit any parent form; ensure you add
type="button" to all similar arrow-direction buttons in this component.
In `@src/components/video-editor/CursorStylePreview.tsx`:
- Around line 10-12: The prop type for previewUrls is too permissive; narrow it
to only accept keys from the CursorStyle union so typos are caught. Update the
component prop signature (the destructured props around style: CursorStyle and
previewUrls) to type previewUrls as Partial<Record<CursorStyle, string>> instead
of Partial<Record<string, string>> (referencing CursorStyle and previewUrls in
CursorStylePreview.tsx) so only valid cursor-style keys are allowed.
In `@src/components/video-editor/EditorContent.tsx`:
- Line 20: The import for the type-only export should use TypeScript's type-only
import syntax: change the import statement that brings in VideoPlaybackRef from
"./VideoPlayback" to explicitly import it as a type (i.e., use the type keyword
with VideoPlaybackRef while keeping the default/component import VideoPlayback
unchanged) so references to VideoPlaybackRef in this file (e.g., usages around
lines referencing the component's ref) are clearly marked as type-only and
future-proofed for stricter TS module settings.
In `@src/components/video-editor/hooks/useEditorCursorTelemetry.ts`:
- Around line 34-69: The retry scheduling logic is duplicated inside
loadTelemetry's try and catch branches; extract it into a small inner helper
(e.g., scheduleRetry or tryScheduleRetry) defined in the same scope as
loadTelemetry that reads pendingFreshRecordingAutoZoomPathRef.current,
autoSuggestedVideoPathRef.current, retryAttempts, retryTimeoutRef and mounted,
computes the shouldRetry condition, increments retryAttempts, and sets
retryTimeoutRef via window.setTimeout to call loadTelemetry after 350ms; then
replace the duplicated blocks in loadTelemetry with a call to that helper to
keep behavior identical.
In `@src/components/video-editor/projectPersistenceNormalization.ts`:
- Line 122: The normalization allows NaN/Infinity through because
shadowIntensity and borderRadius use typeof === "number"; change both checks to
use the existing isFiniteNumber helper (e.g.,
isFiniteNumber(editor.shadowIntensity) ? editor.shadowIntensity : 0.67 and
isFiniteNumber(editor.borderRadius) ? editor.borderRadius : <existing default>)
so only finite numeric values are accepted and persisted, preserving the current
default fallbacks; update the assignments in the shadowIntensity and
borderRadius normalization locations accordingly.
In `@src/components/video-editor/projectPersistencePaths.ts`:
- Around line 70-78: deriveNextId currently builds a RegExp from prefix inside
the reduce which treats regex metacharacters in prefix as regex syntax and
recreates the regex per id; fix by escaping prefix before interpolating it into
the pattern and compile the RegExp once outside the reduce (use the
escapedPrefix in the pattern `^${escapedPrefix}-(\d+)$`) so matching is safe for
any prefix and avoids per-iteration regex construction; update deriveNextId to
use the single precompiled regex when iterating over ids.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 40-142: The SettingsPanelProps interface includes several props
that are no longer used or forwarded (zoomInDurationMs,
onZoomInDurationMsChange, zoomInOverlapMs, onZoomInOverlapMsChange,
zoomOutDurationMs, onZoomOutDurationMsChange, connectedZoomGapMs,
onConnectedZoomGapMsChange, connectedZoomDurationMs,
onConnectedZoomDurationMsChange, zoomInEasing, onZoomInEasingChange,
zoomOutEasing, onZoomOutEasingChange, connectedZoomEasing,
onConnectedZoomEasingChange, whisperExecutablePath, onPickWhisperExecutable);
either remove these fields from SettingsPanelProps (preferred for a pure
refactor) or wire them through when destructuring in SettingsPanel and pass them
down to the corresponding child components (ZoomSection for the zoom* props and
CaptionsSection for whisperExecutablePath/onPickWhisperExecutable) so callers
see effects—update the interface and the SettingsPanel component (and props
passed to ZoomSection/CaptionsSection) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+19
to
+43
| const file = files[0]; | ||
| const validTypes = ["image/jpeg", "image/jpg", "image/png", "image/gif", "image/webp"]; | ||
| if (!validTypes.includes(file.type)) { | ||
| toast.error(t("annotations.imageUploadError"), { | ||
| description: t("annotations.imageUploadErrorDescription"), | ||
| }); | ||
| event.target.value = ""; | ||
| return; | ||
| } | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = (loadEvent) => { | ||
| const dataUrl = loadEvent.target?.result as string; | ||
| if (dataUrl) { | ||
| onContentChange(dataUrl); | ||
| toast.success(t("annotations.imageUploadSuccess")); | ||
| } | ||
| }; | ||
| reader.onerror = () => { | ||
| toast.error(t("annotations.imageUploadFailed"), { | ||
| description: t("annotations.imageUploadFailedDescription"), | ||
| }); | ||
| }; | ||
|
|
||
| reader.readAsDataURL(file); |
Contributor
There was a problem hiding this comment.
Add a file-size guard before converting uploads to data URLs.
MIME validation alone still allows very large images to be loaded fully into memory and stored as base64 annotation content.
Proposed fix
const file = files[0];
+ const maxImageBytes = 10 * 1024 * 1024;
+ if (file.size > maxImageBytes) {
+ toast.error(t("annotations.imageUploadError"), {
+ description: t("annotations.imageUploadTooLargeDescription"),
+ });
+ event.target.value = "";
+ return;
+ }
+
const validTypes = ["image/jpeg", "image/jpg", "image/png", "image/gif", "image/webp"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/AnnotationImageTab.tsx` around lines 19 - 43, The
current upload flow validates MIME types but doesn't guard large files and can
load huge images into memory; add a file-size check before creating/using
FileReader (e.g., define MAX_IMAGE_SIZE_BYTES and check files[0].size) and if
the file exceeds the limit call toast.error with a descriptive message, clear
event.target.value and return early; keep the existing MIME validation,
FileReader usage (reader, reader.onload, reader.onerror), and onContentChange
the same but only invoke them after the size and type checks pass.
- AnnotationTextTab: use ?? instead of || so empty text can be cleared - ExtensionManager: add catch block for marketplace install failures - useEditorExport: wrap retry save in try/catch for unhandled rejections - editorExportWorkflow: move playback restore to finally block
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/video-editor/ExtensionManager.tsx (1)
141-149: Nit: catch block indentation inconsistent, and toast payload duplicated.The
catchblock addresses the past review feedback correctly, but its innertoast.error(...)is indented two tabs deep as if still nested inside the priorif/else, which doesn't match the siblingfinallyblock. While here, consider extracting the duplicated failure-toast call shared with theelsebranch into a small local helper to keep both failure paths in sync.♻️ Proposed tidy-up
+ const showInstallFailed = (description?: string) => + toast.error(t("toast.marketplaceInstallFailed", undefined, { name: extension.name }), { + description, + }); try { const result = await marketplaceInstall(extension.id, extension.downloadUrl); if (result.success) { @@ } else { - toast.error(t("toast.marketplaceInstallFailed", undefined, { name: extension.name }), { - description: result.error, - }); + showInstallFailed(result.error); } } catch (error) { - toast.error(t("toast.marketplaceInstallFailed", undefined, { name: extension.name }), { - description: error instanceof Error ? error.message : undefined, - }); + showInstallFailed(error instanceof Error ? error.message : undefined); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/ExtensionManager.tsx` around lines 141 - 149, The catch block in ExtensionManager.tsx has incorrect indentation and duplicates the same toast.error payload used in the else branch; fix by re-aligning the catch block to sibling level with finally and extract the duplicated failure-toast into a small local helper (e.g., showInstallFailedToast or local function used inside the install handler) that accepts the extension and an optional message, then call that helper from both the else branch and the catch (using error instanceof Error ? error.message : undefined) to keep both failure paths in sync.src/components/video-editor/AnnotationTextTab.tsx (1)
33-36: Prefer a type alias over an empty interface extendingPick.An empty interface body that only extends another type is flagged by
@typescript-eslint/no-empty-object-type/ Biome'snoEmptyInterface. A type alias is equivalent and idiomatic here.Proposed refactor
-interface AnnotationTextTabProps extends Pick< - AnnotationSettingsPanelProps, - "annotation" | "onContentChange" | "onStyleChange" -> {} +type AnnotationTextTabProps = Pick< + AnnotationSettingsPanelProps, + "annotation" | "onContentChange" | "onStyleChange" +>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationTextTab.tsx` around lines 33 - 36, Replace the empty interface AnnotationTextTabProps that simply extends Pick<AnnotationSettingsPanelProps, "annotation" | "onContentChange" | "onStyleChange"> with an equivalent type alias; change the declaration for AnnotationTextTabProps to a type alias using Pick<AnnotationSettingsPanelProps, "annotation" | "onContentChange" | "onStyleChange"> to satisfy the no-empty-interface lint rule while keeping the same shape and references to AnnotationSettingsPanelProps and the three keys.
🤖 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/AnnotationTextTab.tsx`:
- Around line 93-108: The "Custom Fonts" header and the background "Color" label
are hardcoded strings in AnnotationTextTab.tsx (they bypass the component's
existing i18n via useScopedT("editor")), so replace those literals with calls to
the scoped translator (e.g., t("annotations.customFonts") for the customFonts
section header and t("annotations.color") for the background color label) where
customFonts is rendered and where the color label is displayed (the SelectItem
block and the background color UI), adding the new keys to the editor scope
resource file if missing.
In `@src/components/video-editor/hooks/useEditorExport.ts`:
- Around line 129-139: The handler handleOpenExportDropdown currently checks
getRenderConfig().videoPath before honoring hasPendingExportSave, which blocks
reopening the retry UI when a pending export buffer exists; change the logic so
that if hasPendingExportSave (or pendingExportSaveRef indicates a saved buffer)
is true you immediately call setShowExportDropdown(true) and setExportError(...)
and return, and only if there is no pending save then validate
getRenderConfig().videoPath and toast.error("No video loaded") as before; update
handleOpenExportDropdown to use hasPendingExportSave/pendingExportSaveRef first
and only fall back to the videoPath check.
- Around line 151-163: In the GIF export path inside useEditorExport where you
read videoPlaybackRef.current.video and call calculateOutputDimensions (using
config.gifSizePreset and GIF_SIZE_PRESETS), don't fall back to hardcoded
1920x1080 when video.videoWidth/video.videoHeight are 0; instead, if exporting a
GIF and either dimension is 0, wait for the video's metadata by awaiting a
Promise that resolves on the video's "loadedmetadata" (or timeout/fail with
toast.error), then re-read video.videoWidth/video.videoHeight and call
calculateOutputDimensions; update the logic around videoPlaybackRef, the
videoWidth/videoHeight checks, and gifDimensions calculation so GIF export
blocks until real dimensions are available.
---
Nitpick comments:
In `@src/components/video-editor/AnnotationTextTab.tsx`:
- Around line 33-36: Replace the empty interface AnnotationTextTabProps that
simply extends Pick<AnnotationSettingsPanelProps, "annotation" |
"onContentChange" | "onStyleChange"> with an equivalent type alias; change the
declaration for AnnotationTextTabProps to a type alias using
Pick<AnnotationSettingsPanelProps, "annotation" | "onContentChange" |
"onStyleChange"> to satisfy the no-empty-interface lint rule while keeping the
same shape and references to AnnotationSettingsPanelProps and the three keys.
In `@src/components/video-editor/ExtensionManager.tsx`:
- Around line 141-149: The catch block in ExtensionManager.tsx has incorrect
indentation and duplicates the same toast.error payload used in the else branch;
fix by re-aligning the catch block to sibling level with finally and extract the
duplicated failure-toast into a small local helper (e.g., showInstallFailedToast
or local function used inside the install handler) that accepts the extension
and an optional message, then call that helper from both the else branch and the
catch (using error instanceof Error ? error.message : undefined) to keep both
failure paths in sync.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23c0d245-e722-4b84-a950-7649ec31bfb4
📒 Files selected for processing (4)
src/components/video-editor/AnnotationTextTab.tsxsrc/components/video-editor/ExtensionManager.tsxsrc/components/video-editor/hooks/editorExportWorkflow.tssrc/components/video-editor/hooks/useEditorExport.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/video-editor/hooks/editorExportWorkflow.ts
Comment on lines
+151
to
+163
| const video = videoPlaybackRef.current?.video; | ||
| if (!video) { | ||
| toast.error("Video not ready"); | ||
| return; | ||
| } | ||
| const sourceWidth = video.videoWidth || 1920; | ||
| const sourceHeight = video.videoHeight || 1080; | ||
| const gifDimensions = calculateOutputDimensions( | ||
| sourceWidth, | ||
| sourceHeight, | ||
| config.gifSizePreset, | ||
| GIF_SIZE_PRESETS, | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid exporting GIFs with fallback dimensions when metadata is not ready.
video.videoWidth / video.videoHeight can be 0 while metadata is still loading. Falling back to 1920x1080 can silently produce the wrong GIF aspect ratio for vertical or non-16:9 sources. For GIF exports, prefer blocking until dimensions are available.
Proposed fix
const video = videoPlaybackRef.current?.video;
if (!video) {
toast.error("Video not ready");
return;
}
- const sourceWidth = video.videoWidth || 1920;
- const sourceHeight = video.videoHeight || 1080;
- const gifDimensions = calculateOutputDimensions(
- sourceWidth,
- sourceHeight,
- config.gifSizePreset,
- GIF_SIZE_PRESETS,
- );
+ const gifDimensions =
+ config.exportFormat === "gif"
+ ? (() => {
+ if (!video.videoWidth || !video.videoHeight) {
+ toast.error("Video metadata not ready. Please try again in a moment.");
+ return null;
+ }
+ return calculateOutputDimensions(
+ video.videoWidth,
+ video.videoHeight,
+ config.gifSizePreset,
+ GIF_SIZE_PRESETS,
+ );
+ })()
+ : null;
+ if (config.exportFormat === "gif" && !gifDimensions) {
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/hooks/useEditorExport.ts` around lines 151 - 163,
In the GIF export path inside useEditorExport where you read
videoPlaybackRef.current.video and call calculateOutputDimensions (using
config.gifSizePreset and GIF_SIZE_PRESETS), don't fall back to hardcoded
1920x1080 when video.videoWidth/video.videoHeight are 0; instead, if exporting a
GIF and either dimension is 0, wait for the video's metadata by awaiting a
Promise that resolves on the video's "loadedmetadata" (or timeout/fail with
toast.error), then re-read video.videoWidth/video.videoHeight and call
calculateOutputDimensions; update the logic around videoPlaybackRef, the
videoWidth/videoHeight checks, and gifDimensions calculation so GIF export
blocks until real dimensions are available.
- Gate audio level meter to selected mic device only (helperComponents) - Clear webcam deviceId when 'Default' selected (LaunchWindow) - Wire i18n for 'Custom Fonts' and 'Color' labels (AnnotationTextTab) - Restore keyboard focus ring on sidebar buttons (EditorSidebar) - Add visually-hidden DialogTitle for a11y (ExtensionDetailModal) - Make extension cards keyboard-accessible with role/tabIndex (ExtensionManagerCards) - Write smoke export reports for GIF exports before closing (editorExportWorkflow) - Guard GIF export against zero video dimensions (useEditorExport) - Allow 'Save Again' before requiring loaded video (useEditorExport)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/video-editor/EditorSidebar.tsx (1)
136-146:⚠️ Potential issue | 🟠 MajorAccount button still removes keyboard focus indicator.
The rail buttons were updated with
focus-visible:ring-2at line 92, but the account button at lines 136–146 still hasfocus:outline-none focus-visible:outline-none, leaving keyboard users with no visible focus state here. Also,title="Account"is hardcoded while the rest of the sidebar labels go throught(...), which is inconsistent with the i18n wiring called out in this PR.Proposed fix
- <motion.button - type="button" - onClick={() => toast.info("Account coming soon")} - title="Account" - className="group relative flex h-9 w-9 items-center justify-center rounded-lg text-foreground/55 outline-none transition hover:text-foreground focus:outline-none focus-visible:outline-none" + <motion.button + type="button" + onClick={() => toast.info(t("settings.account.comingSoon", "Account coming soon"))} + title={t("settings.account.title", "Account")} + className="group relative flex h-9 w-9 items-center justify-center rounded-lg text-foreground/55 outline-none transition hover:text-foreground focus-visible:ring-2 focus-visible:ring-[`#2563EB`]/50 focus-visible:ring-offset-1" whileHover={{ opacity: 1 }} initial={{ opacity: 0.55 }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorSidebar.tsx` around lines 136 - 146, The Account button in EditorSidebar (the motion.button with User icon) removes keyboard focus because it still uses focus:outline-none and focus-visible:outline-none and has a hardcoded title; update that button to match the other rail buttons by removing those outline-none classes and adding the same visible focus classes (e.g., include focus-visible:ring-2 and the same ring color/offset classes used by the other rail buttons) so keyboard users get a visible focus ring, and replace title="Account" with the i18n call t('Account') to keep labeling consistent.src/components/video-editor/hooks/useEditorExport.ts (1)
156-167:⚠️ Potential issue | 🟡 MinorGate GIF dimension work on
exportFormat === "gif".Lines 158-161 abort the export when
video.videoWidth/videoHeightare 0, andcalculateOutputDimensionsalways runs, but both are only meaningful for GIF — MP4 gets its source size fromensureSupportedMp4SourceDimensionsinrunEditorExport. A slow-to-load video would then be blocked from an MP4 export for no reason, with a GIF-shaped error message ("Video dimensions not ready…"). Only block/compute when the user actually selected GIF.♻️ Proposed fix
- const sourceWidth = video.videoWidth; - const sourceHeight = video.videoHeight; - if (!sourceWidth || !sourceHeight) { - toast.error("Video dimensions not ready. Please wait for the video to load."); - return; - } - const gifDimensions = calculateOutputDimensions( - sourceWidth, - sourceHeight, - config.gifSizePreset, - GIF_SIZE_PRESETS, - ); + let gifDimensions: { width: number; height: number } | null = null; + if (config.exportFormat === "gif") { + const sourceWidth = video.videoWidth; + const sourceHeight = video.videoHeight; + if (!sourceWidth || !sourceHeight) { + toast.error("Video dimensions not ready. Please wait for the video to load."); + return; + } + gifDimensions = calculateOutputDimensions( + sourceWidth, + sourceHeight, + config.gifSizePreset, + GIF_SIZE_PRESETS, + ); + } @@ gifConfig: - config.exportFormat === "gif" + config.exportFormat === "gif" && gifDimensions ? { frameRate: config.gifFrameRate, loop: config.gifLoop, sizePreset: config.gifSizePreset, width: gifDimensions.width, height: gifDimensions.height, } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorExport.ts` around lines 156 - 167, The export is incorrectly aborting for slow-loading videos regardless of format; update the logic in useEditorExport (around where sourceWidth/sourceHeight and calculateOutputDimensions are used) to only check for non-zero video.videoWidth/video.videoHeight and call calculateOutputDimensions(config.gifSizePreset, GIF_SIZE_PRESETS) when exportFormat === "gif"; do not toast the "Video dimensions not ready" error or return for non-gif exports (MP4 flow should continue to rely on ensureSupportedMp4SourceDimensions in runEditorExport), and assign gifDimensions only in the gif branch so MP4 exports aren’t blocked.
🧹 Nitpick comments (2)
src/components/video-editor/AnnotationTextTab.tsx (1)
33-36: Optional: replace empty interface with a type alias.
interface AnnotationTextTabProps extends Pick<...> {}is an empty-interface pattern that triggers@typescript-eslint/no-empty-object-type(and Biome'snoEmptyInterface) under common configs. A type alias is equivalent and lint-clean.Proposed change
-interface AnnotationTextTabProps extends Pick< - AnnotationSettingsPanelProps, - "annotation" | "onContentChange" | "onStyleChange" -> {} +type AnnotationTextTabProps = Pick< + AnnotationSettingsPanelProps, + "annotation" | "onContentChange" | "onStyleChange" +>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/AnnotationTextTab.tsx` around lines 33 - 36, Replace the empty interface pattern by turning AnnotationTextTabProps into a type alias: instead of declaring "interface AnnotationTextTabProps extends Pick<AnnotationSettingsPanelProps, 'annotation' | 'onContentChange' | 'onStyleChange'> {}", declare "type AnnotationTextTabProps = Pick<AnnotationSettingsPanelProps, 'annotation' | 'onContentChange' | 'onStyleChange'>"; update any imports/usages if necessary to use the new type name (AnnotationTextTabProps) so the code compiles and satisfies the linter (noEmptyInterface / `@typescript-eslint/no-empty-object-type`).src/components/video-editor/EditorSidebar.tsx (1)
71-78: Extension section icon fallback mixes component with string type.
extensionSectionButtons[i].iconis typed asstring(seeuseEditorSideEffects.tswhere it's set top.panel.icon || ""). The fallbackb.icon || (PhPuzzle as typeof PhPuzzle | string)then yields a component value for entries with empty icons, but the rendering branch at line 110 only takes the component path whentypeof section.icon !== "string". An empty string""is falsy, so this works, but an extension providing a non-empty yet unresolvable icon id will silently fall through toExtensionIconwithout a fallback. Consider resolving the fallback inside the render branch (e.g., letExtensionIconrenderPhPuzzlewhen it can't resolve the id) for more predictable behavior, or keepiconstrictly typed asstringhere and drop the component cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorSidebar.tsx` around lines 71 - 78, The current fallback mixes a React component (PhPuzzle) with string-typed icons (set in useEditorSideEffects), which can make resolution inconsistent at render (section.icon); fix by either keeping icon strictly a string in the extensionSectionButtons mapping — replace icon: b.icon || (PhPuzzle as typeof PhPuzzle | string) with icon: b.icon || "PhPuzzle" (remove the component cast and update typings in useEditorSideEffects/extensionSectionButtons accordingly) — or implement the fallback inside the renderer (update ExtensionIcon so when it cannot resolve section.icon it returns the PhPuzzle component). Reference: extensionSectionButtons, PhPuzzle, useEditorSideEffects, section.icon, and ExtensionIcon.
🤖 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/hooks/editorExportWorkflow.ts`:
- Around line 240-503: The failure branches for GIF export (where
setExportError(...) and toast.error(...) are called), MP4 export failure branch
(the else after result.success for MP4 where setExportError(...) and
toast.error(summarizeErrorMessage(...)) are called), and the top-level catch
(where setExportError(errorMessage) and toast.error(...) are called) currently
never set keepExportDialogOpen = true so the export dropdown closes and the
exportError UI is hidden; update each of these three spots to set
keepExportDialogOpen = true immediately after setting setExportError(...) (and
before any smokeExportConfig early returns) so the dropdown remains open and the
error message is visible to the user, keeping existing smokeExportConfig
writeSmokeExportReport and window.close() behavior unchanged for smoke runs.
In `@src/components/video-editor/hooks/useEditorExport.ts`:
- Around line 193-204: handleCancelExport currently no-ops if
exporterRef.current is not set, leaving the UI stuck; change it so the UI reset
always runs while only calling exporterRef.current.cancel() when
exporterRef.current exists. In other words, in handleCancelExport (referencing
exporterRef, clearPendingExportSave, setShowExportDropdown, setIsExporting,
setExportProgress, setExportError, setExportedFilePath and toast.info) always
call toast.info("Export canceled"), clearPendingExportSave(),
setShowExportDropdown(false), setIsExporting(false), setExportProgress(null),
setExportError(null), and setExportedFilePath(undefined), and conditionally call
exporterRef.current.cancel() only if exporterRef.current is truthy; keep the
dependency list updated and note that runEditorExport's finally will reconcile
state when the in-flight export promise settles.
---
Duplicate comments:
In `@src/components/video-editor/EditorSidebar.tsx`:
- Around line 136-146: The Account button in EditorSidebar (the motion.button
with User icon) removes keyboard focus because it still uses focus:outline-none
and focus-visible:outline-none and has a hardcoded title; update that button to
match the other rail buttons by removing those outline-none classes and adding
the same visible focus classes (e.g., include focus-visible:ring-2 and the same
ring color/offset classes used by the other rail buttons) so keyboard users get
a visible focus ring, and replace title="Account" with the i18n call
t('Account') to keep labeling consistent.
In `@src/components/video-editor/hooks/useEditorExport.ts`:
- Around line 156-167: The export is incorrectly aborting for slow-loading
videos regardless of format; update the logic in useEditorExport (around where
sourceWidth/sourceHeight and calculateOutputDimensions are used) to only check
for non-zero video.videoWidth/video.videoHeight and call
calculateOutputDimensions(config.gifSizePreset, GIF_SIZE_PRESETS) when
exportFormat === "gif"; do not toast the "Video dimensions not ready" error or
return for non-gif exports (MP4 flow should continue to rely on
ensureSupportedMp4SourceDimensions in runEditorExport), and assign gifDimensions
only in the gif branch so MP4 exports aren’t blocked.
---
Nitpick comments:
In `@src/components/video-editor/AnnotationTextTab.tsx`:
- Around line 33-36: Replace the empty interface pattern by turning
AnnotationTextTabProps into a type alias: instead of declaring "interface
AnnotationTextTabProps extends Pick<AnnotationSettingsPanelProps, 'annotation' |
'onContentChange' | 'onStyleChange'> {}", declare "type AnnotationTextTabProps =
Pick<AnnotationSettingsPanelProps, 'annotation' | 'onContentChange' |
'onStyleChange'>"; update any imports/usages if necessary to use the new type
name (AnnotationTextTabProps) so the code compiles and satisfies the linter
(noEmptyInterface / `@typescript-eslint/no-empty-object-type`).
In `@src/components/video-editor/EditorSidebar.tsx`:
- Around line 71-78: The current fallback mixes a React component (PhPuzzle)
with string-typed icons (set in useEditorSideEffects), which can make resolution
inconsistent at render (section.icon); fix by either keeping icon strictly a
string in the extensionSectionButtons mapping — replace icon: b.icon ||
(PhPuzzle as typeof PhPuzzle | string) with icon: b.icon || "PhPuzzle" (remove
the component cast and update typings in
useEditorSideEffects/extensionSectionButtons accordingly) — or implement the
fallback inside the renderer (update ExtensionIcon so when it cannot resolve
section.icon it returns the PhPuzzle component). Reference:
extensionSectionButtons, PhPuzzle, useEditorSideEffects, section.icon, and
ExtensionIcon.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a56691a4-c5ee-4731-9e01-c84e7d44c972
📒 Files selected for processing (13)
src/components/launch/LaunchWindow/helperComponents.tsxsrc/components/launch/LaunchWindow/index.tsxsrc/components/video-editor/AnnotationTextTab.tsxsrc/components/video-editor/EditorSidebar.tsxsrc/components/video-editor/extension-manager/ExtensionDetailModal.tsxsrc/components/video-editor/extension-manager/ExtensionManagerCards.tsxsrc/components/video-editor/hooks/editorExportWorkflow.tssrc/components/video-editor/hooks/useEditorExport.tssrc/i18n/locales/en/editor.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/ko/editor.jsonsrc/i18n/locales/nl/editor.jsonsrc/i18n/locales/zh-CN/editor.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/launch/LaunchWindow/helperComponents.tsx
- src/components/video-editor/extension-manager/ExtensionManagerCards.tsx
- src/components/launch/LaunchWindow/index.tsx
UI decision — removes default focus ring to avoid ugly yellow selection ring.
…ix Windows paths, translate i18n - Export dropdown now stays open on GIF/MP4 failure and top-level catch so the exportError UI remains visible (previously dismissed by finally block) - handleCancelExport always resets UI state even before exporter is constructed, so Cancel works during the async setup phase - fromFileUrl() now converts forward slashes to backslashes for Windows drive paths (both primary and fallback parsing paths) - Translated customFonts and color keys in es/ko/nl/zh-CN locales
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/video-editor/EditorSidebar.tsx (1)
88-93:⚠️ Potential issue | 🟠 MajorRestore a visible keyboard focus state on the icon buttons.
focus-visible:outline-nonestill removes the keyboard focus indicator from both the section rail and account button. Since these are icon-only controls, keep mouse focus quiet if desired, but add a visiblefocus-visiblestyle.♿ Proposed fix
- {/* Intentionally removes default focus-visible ring — UI decision to avoid ugly yellow selection ring */} + {/* Keep keyboard focus visible for icon-only controls. */} <motion.button type="button" onClick={() => prefs.setActiveEffectSection(section.id)} title={section.label} - className="group relative flex h-9 w-9 items-center justify-center rounded-lg outline-none focus:outline-none focus-visible:outline-none" + className="group relative flex h-9 w-9 items-center justify-center rounded-lg outline-none focus:outline-none focus-visible:ring-2 focus-visible:ring-[`#2563EB`] focus-visible:ring-offset-2 focus-visible:ring-offset-editor-bg"- className="group relative flex h-9 w-9 items-center justify-center rounded-lg text-foreground/55 outline-none transition hover:text-foreground focus:outline-none focus-visible:outline-none" + className="group relative flex h-9 w-9 items-center justify-center rounded-lg text-foreground/55 outline-none transition hover:text-foreground focus:outline-none focus-visible:ring-2 focus-visible:ring-[`#2563EB`] focus-visible:ring-offset-2 focus-visible:ring-offset-editor-bg"Also applies to: 141-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/EditorSidebar.tsx` around lines 88 - 93, The icon-only buttons in EditorSidebar (the motion.button used to set prefs.setActiveEffectSection and the account button) currently remove keyboard focus by including focus-visible:outline-none; restore a visible keyboard focus state by removing focus-visible:outline-none from their className and adding accessible focus-visible utility classes (e.g. focus-visible:ring-2, focus-visible:ring-offset-2 and a contrasting focus-visible:ring color) so mouse focus remains quiet but keyboard users get a clear ring; update the same class changes on the other icon button (account button) to match.
🤖 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/EditorSidebar.tsx`:
- Around line 139-140: The Account button in EditorSidebar uses hardcoded
strings ("Account" and toast.info("Account coming soon")) instead of the
existing i18n flow; update the EditorSidebar component to use the same useI18n
hook used for other sidebar labels (e.g., call const { t } = useI18n()) and
replace the title and toast message with localized keys (for example
t('sidebar.account') and t('sidebar.accountComingSoon') or similar keys
consistent with your locales), then add those keys to the localization files so
the button label and toast are localized.
---
Duplicate comments:
In `@src/components/video-editor/EditorSidebar.tsx`:
- Around line 88-93: The icon-only buttons in EditorSidebar (the motion.button
used to set prefs.setActiveEffectSection and the account button) currently
remove keyboard focus by including focus-visible:outline-none; restore a visible
keyboard focus state by removing focus-visible:outline-none from their className
and adding accessible focus-visible utility classes (e.g. focus-visible:ring-2,
focus-visible:ring-offset-2 and a contrasting focus-visible:ring color) so mouse
focus remains quiet but keyboard users get a clear ring; update the same class
changes on the other icon button (account button) to match.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e86f20dd-212a-4b74-8f71-d70607178707
📒 Files selected for processing (1)
src/components/video-editor/EditorSidebar.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/video-editor/projectPersistencePaths.ts (1)
72-80: Escapeprefixbefore embedding inRegExp.
prefixis interpolated directly into a regex literal, so any regex metacharacter (.,+,(, etc.) in a future caller would either silently match unintended IDs or throw on invalid patterns. Current callers likely pass safe literals, but a one-line escape removes the footgun and also silences the static-analysis ReDoS warning.♻️ Proposed fix
export function deriveNextId(prefix: string, ids: string[]): number { + const escapedPrefix = prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const pattern = new RegExp(`^${escapedPrefix}-(\\d+)$`); const max = ids.reduce((acc, id) => { - const match = id.match(new RegExp(`^${prefix}-(\\d+)$`)); + const match = id.match(pattern); if (!match) return acc; const value = Number(match[1]); return Number.isFinite(value) ? Math.max(acc, value) : acc; }, 0); return max + 1; }Hoisting the
RegExpout of the reducer also avoids recompiling it per element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistencePaths.ts` around lines 72 - 80, The deriveNextId function embeds prefix directly into a RegExp which can misinterpret regex metacharacters or throw; fix by escaping prefix before building the pattern and hoist the RegExp construction out of the reducer so it’s compiled once (e.g., create an escapedPrefix and const re = new RegExp(`^${escapedPrefix}-(\\d+)$`) before ids.reduce), then use re.match/check inside the reducer to compute max and return max+1.src/components/video-editor/hooks/useEditorExport.ts (1)
156-167: Optional: scope GIF dimension calculation to GIF exports.The readiness check and
calculateOutputDimensions(...)run for every export, butgifDimensionsis only consumed whenconfig.exportFormat === "gif"(lines 176–185). For MP4 exports this blocks onvideo.videoWidth/videoHeighteven thoughensureSupportedMp4SourceDimensionsinrunEditorExporthandles source dimensions on its own, and the "Video dimensions not ready" copy is GIF‑specific. Consider scoping both the readiness gate and the dimension calc to the GIF branch.♻️ Proposed refactor
- const sourceWidth = video.videoWidth; - const sourceHeight = video.videoHeight; - if (!sourceWidth || !sourceHeight) { - toast.error("Video dimensions not ready. Please wait for the video to load."); - return; - } - const gifDimensions = calculateOutputDimensions( - sourceWidth, - sourceHeight, - config.gifSizePreset, - GIF_SIZE_PRESETS, - ); + let gifDimensions: { width: number; height: number } | null = null; + if (config.exportFormat === "gif") { + const sourceWidth = video.videoWidth; + const sourceHeight = video.videoHeight; + if (!sourceWidth || !sourceHeight) { + toast.error("Video dimensions not ready. Please wait for the video to load."); + return; + } + gifDimensions = calculateOutputDimensions( + sourceWidth, + sourceHeight, + config.gifSizePreset, + GIF_SIZE_PRESETS, + ); + }…and then reference
gifDimensions.width/gifDimensions.heightinside the existingconfig.exportFormat === "gif"gifConfig object (which already narrows the type).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorExport.ts` around lines 156 - 167, The GIF dimension readiness check and call to calculateOutputDimensions are currently executed for every export even though gifDimensions is only used when config.exportFormat === "gif"; move the video.videoWidth/video.videoHeight guard, the toast.error message, and the calculateOutputDimensions(...) call so they run only inside the GIF branch (the block where config.exportFormat === "gif") in useEditorExport.ts; keep MP4 handling relying on ensureSupportedMp4SourceDimensions in runEditorExport and do not show the GIF-specific "Video dimensions not ready" toast for non-GIF exports, and then reference gifDimensions.width / gifDimensions.height inside the existing gifConfig object.
🤖 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/hooks/editorExportWorkflow.ts`:
- Around line 225-239: The save-failure branches for GIF and MP4 currently set
setExportError(...) and toast.error(...) but do not set keepExportDialogOpen =
true, so the finally block will close the dropdown; update both non-smoke
save-failure arms (the GIF branch that checks saveResult and the analogous MP4
saveResult branch) to set keepExportDialogOpen = true after setting the
error/toast (leave the smokeExportConfig path that calls writeSmokeExportReport
and window.close() unchanged) so the persistent error UI remains visible and the
user can retry.
In `@src/components/video-editor/projectPersistencePaths.ts`:
- Around line 94-101: The validator validateProjectData currently accepts any
numeric version; change it to enforce or explicitly handle schema versions by
comparing project.version against PROJECT_VERSION (or an allowed range) instead
of only checking typeof number, and return false for mismatches so
normalizeProjectEditor/useEditorProject only receive supported schemas;
alternatively, if you prefer migration at call sites, keep validateProjectData
strict (project.version === PROJECT_VERSION) and add explicit version-checking +
migration or clear error paths where normalizeProjectEditor and useEditorProject
are invoked.
---
Nitpick comments:
In `@src/components/video-editor/hooks/useEditorExport.ts`:
- Around line 156-167: The GIF dimension readiness check and call to
calculateOutputDimensions are currently executed for every export even though
gifDimensions is only used when config.exportFormat === "gif"; move the
video.videoWidth/video.videoHeight guard, the toast.error message, and the
calculateOutputDimensions(...) call so they run only inside the GIF branch (the
block where config.exportFormat === "gif") in useEditorExport.ts; keep MP4
handling relying on ensureSupportedMp4SourceDimensions in runEditorExport and do
not show the GIF-specific "Video dimensions not ready" toast for non-GIF
exports, and then reference gifDimensions.width / gifDimensions.height inside
the existing gifConfig object.
In `@src/components/video-editor/projectPersistencePaths.ts`:
- Around line 72-80: The deriveNextId function embeds prefix directly into a
RegExp which can misinterpret regex metacharacters or throw; fix by escaping
prefix before building the pattern and hoist the RegExp construction out of the
reducer so it’s compiled once (e.g., create an escapedPrefix and const re = new
RegExp(`^${escapedPrefix}-(\\d+)$`) before ids.reduce), then use re.match/check
inside the reducer to compute max and return max+1.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a566c1e5-e2b9-4e8e-8de4-51aece0b4d55
📒 Files selected for processing (7)
src/components/video-editor/hooks/editorExportWorkflow.tssrc/components/video-editor/hooks/useEditorExport.tssrc/components/video-editor/projectPersistencePaths.tssrc/i18n/locales/es/editor.jsonsrc/i18n/locales/ko/editor.jsonsrc/i18n/locales/nl/editor.jsonsrc/i18n/locales/zh-CN/editor.json
Comment on lines
+94
to
+101
| export function validateProjectData(candidate: unknown): candidate is EditorProjectData { | ||
| if (!candidate || typeof candidate !== "object") return false; | ||
| const project = candidate as Partial<EditorProjectData>; | ||
| if (typeof project.version !== "number") return false; | ||
| if (typeof project.videoPath !== "string" || !project.videoPath) return false; | ||
| if (!project.editor || typeof project.editor !== "object") return false; | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
validateProjectData accepts any numeric version.
The predicate only checks typeof project.version === "number" rather than comparing against PROJECT_VERSION (or an allow-listed range). If the persisted schema evolves, stale or newer payloads will still type-narrow to EditorProjectData and reach normalizeProjectEditor/useEditorProject without any migration branch. Consider either asserting project.version === PROJECT_VERSION here or handling version mismatch explicitly at the call site so unsupported versions surface a clear error instead of silently loading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/projectPersistencePaths.ts` around lines 94 -
101, The validator validateProjectData currently accepts any numeric version;
change it to enforce or explicitly handle schema versions by comparing
project.version against PROJECT_VERSION (or an allowed range) instead of only
checking typeof number, and return false for mismatches so
normalizeProjectEditor/useEditorProject only receive supported schemas;
alternatively, if you prefer migration at call sites, keep validateProjectData
strict (project.version === PROJECT_VERSION) and add explicit version-checking +
migration or clear error paths where normalizeProjectEditor and useEditorProject
are invoked.
GIF save-failure and MP4 save-failure paths had the same bug as the export-failure paths: dropdown closes hiding the exportError UI. Now keepExportDialogOpen = true in all non-smoke failure branches.
… localize account button - RecordingControls now accepts and wires toggleMicrophone so the mic button actually works during recording - gifOutputDimensions and desiredMp4SourceDimensions now use a reactive videoDimensions state (set on loadedmetadata) instead of reading from a ref in useMemo, fixing stale 1920x1080 fallback on first render - Account button text now goes through i18n with translations for all 5 locales
The toggleMicrophone and resumeRecording props were on the same line as microphoneEnabled, causing the recording HUD buttons to break.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part 2 of 2: Refactors large files in
src/components/into focused sub-modules under 500 lines each.98 files changed — split from #288 to stay under CodeRabbit's 150-file review limit. Merge #289 first.
What changed
Approach
tsc --noEmitclean)Summary by CodeRabbit
New Features
Improvements