feat: multiple UI improvements and new features#698
Closed
nino-chavez wants to merge 10 commits intoOpenCut-app:mainfrom
Closed
feat: multiple UI improvements and new features#698nino-chavez wants to merge 10 commits intoOpenCut-app:mainfrom
nino-chavez wants to merge 10 commits intoOpenCut-app:mainfrom
Conversation
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@nino-chavez is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new public About page, enhances the Projects interface with a theme toggle and inline open-in-new-tab controls, refactors keyboard shortcuts recording with add/replace/remove modes, restructures UI component composition patterns, adds a cut-selected editor action, implements file size validation, improves audio resampling via linear interpolation, strengthens resource cleanup in media processing, and modernizes button/link DOM structures across multiple components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx (1)
186-226:⚠️ Potential issue | 🟠 MajorAlign displayed keys with raw keys to avoid removing the wrong binding.
displayKeysdrops Cmd variants when Ctrl equivalents exist, butrawKeys[index]is still derived from the unfiltered list. That can remove/replace a different binding than the one shown (e.g., Cmd+Z instead of Ctrl+Z). Use a single filtered raw-key list (or pair{displayKey, rawKey}) so the mapping stays stable.🛠️ Proposed fix
- const displayKeys = shortcut.keys.filter((key: string) => { - if ( - key.includes("Cmd") && - shortcut.keys.includes(key.replace("Cmd", "Ctrl")) - ) - return false; - - return true; - }); - - // Get raw keys for remove functionality (before formatting) - const { keybindings } = useKeybindingsStore(); - const rawKeys = Object.entries(keybindings) - .filter(([, action]) => action === shortcut.action) - .map(([key]) => key as ShortcutKey); + // Get raw keys for remove functionality (before formatting) + const { keybindings } = useKeybindingsStore(); + const rawKeys = Object.entries(keybindings) + .filter(([, action]) => action === shortcut.action) + .map(([key]) => key as ShortcutKey); + const displayKeys = rawKeys.filter((key) => { + if (key.includes("Cmd") && rawKeys.includes(key.replace("Cmd", "Ctrl"))) { + return false; + } + return true; + }); @@ - const rawKey = rawKeys[index]; + const rawKey = key as ShortcutKey;
🤖 Fix all issues with AI agents
In `@apps/web/src/app/projects/page.tsx`:
- Around line 598-614: The icon "open in new tab" control inside the project
card (the button using project.id, HugeiconsIcon and LinkSquare02Icon) must be
removed from inside the main Link and converted into a sibling anchor overlay so
it's not a nested interactive element; replace the inline window.open with a
real <a> element that uses target="_blank" and rel="noopener noreferrer", add an
accessible label (aria-label) and keep a title element for the icon (or include
readable text for screen readers), and position it as a sibling overlay similar
to the checkbox overlay in both grid and list card render paths so it remains
clickable without breaking the parent Link.
In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx`:
- Around line 238-250: Add explicit type and accessibility attributes and expand
short parameter names: update the icon-only Button that calls onStartAdding so
it includes type="button" and aria-label="Add another shortcut", rename the
inline onClick parameter from e to event in that handler, and add title="Add
another shortcut" to the HugeiconsIcon; likewise, add type="button" to the other
Button (the one around line 282) and rename the parameters for handleClick and
handleRightClick from (e) to (event) to use full names consistently.
🧹 Nitpick comments (1)
apps/web/src/hooks/actions/use-editor-actions.ts (1)
276-301: Extract shared clipboard-building logic to avoid duplication.
This block repeats the copy-selected logic; a small helper will keep copy/cut behavior consistent and easier to maintain. As per coding guidelines, Create helper functions for multi-step operations instead of inline complex logic.♻️ Suggested refactor
export function useEditorActions() { const editor = useEditor(); const activeProject = editor.project.getActive(); const { selectedElements, setElementSelection } = useElementSelection(); const { clipboard, setClipboard, toggleSnapping } = useTimelineStore(); + + const buildClipboardItems = (elementsToCopy) => { + const elementsWithTracks = editor.timeline.getElementsWithTracks({ + elements: elementsToCopy, + }); + return elementsWithTracks.map(({ track, element }) => { + const { ...elementWithoutId } = element; + return { + trackId: track.id, + trackType: track.type, + element: elementWithoutId, + }; + }); + }; useActionHandler( "copy-selected", () => { if (selectedElements.length === 0) return; - const results = editor.timeline.getElementsWithTracks({ - elements: selectedElements, - }); - const items = results.map(({ track, element }) => { - const { ...elementWithoutId } = element; - return { - trackId: track.id, - trackType: track.type, - element: elementWithoutId, - }; - }); - - setClipboard({ items }); + const clipboardItems = buildClipboardItems(selectedElements); + setClipboard({ items: clipboardItems }); }, undefined, ); useActionHandler( "cut-selected", () => { if (selectedElements.length === 0) return; - // Copy elements to clipboard - const results = editor.timeline.getElementsWithTracks({ - elements: selectedElements, - }); - const items = results.map(({ track, element }) => { - const { ...elementWithoutId } = element; - return { - trackId: track.id, - trackType: track.type, - element: elementWithoutId, - }; - }); - setClipboard({ items }); + const clipboardItems = buildClipboardItems(selectedElements); + setClipboard({ items: clipboardItems }); // Delete the selected elements editor.timeline.deleteElements({ elements: selectedElements, });
Comment on lines
+598
to
+614
| <div className="flex items-center gap-1.5"> | ||
| <h3 className="group-hover:text-foreground/90 line-clamp-2 text-sm leading-snug font-medium"> | ||
| {project.name} | ||
| </h3> | ||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| window.open(`/editor/${project.id}`, "_blank"); | ||
| }} | ||
| className="opacity-0 group-hover:opacity-100 transition-opacity p-0.5 hover:bg-accent rounded shrink-0" | ||
| title="Open in new tab" | ||
| > | ||
| <HugeiconsIcon icon={LinkSquare02Icon} className="size-3.5 text-muted-foreground" /> | ||
| </button> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Fix nested interactive elements and harden the “open in new tab” control.
The icon button sits inside a Link (both grid and list views), which is invalid HTML and problematic for accessibility. Also, window.open without noopener enables tabnabbing, and the icon-only control needs an accessible label/title. A safer pattern is to move the icon link outside the main Link and use a real anchor with target="_blank" + rel, plus aria-label and an icon title.
🔧 Proposed approach (apply to both grid + list)
- <div className="flex items-center gap-1.5">
+ <div className="flex items-center gap-1.5">
<h3 className="group-hover:text-foreground/90 line-clamp-2 text-sm leading-snug font-medium">
{project.name}
</h3>
- <button
- type="button"
- onClick={(e) => {
- e.preventDefault();
- e.stopPropagation();
- window.open(`/editor/${project.id}`, "_blank");
- }}
- className="opacity-0 group-hover:opacity-100 transition-opacity p-0.5 hover:bg-accent rounded shrink-0"
- title="Open in new tab"
- >
- <HugeiconsIcon icon={LinkSquare02Icon} className="size-3.5 text-muted-foreground" />
- </button>
</div>- <Link href={`/editor/${project.id}`} className="flex-1 min-w-0">
+ <Link href={`/editor/${project.id}`} className="flex-1 min-w-0">
{listRowContent}
</Link>
+ <Link
+ href={`/editor/${project.id}`}
+ target="_blank"
+ rel="noopener noreferrer"
+ aria-label="Open in new tab"
+ className="opacity-0 group-hover:opacity-100 transition-opacity p-0.5 hover:bg-accent rounded shrink-0"
+ >
+ <HugeiconsIcon
+ icon={LinkSquare02Icon}
+ className="size-3.5 text-muted-foreground"
+ title="Open in new tab"
+ />
+ </Link>For grid cards, place the icon link as a sibling overlay (similar to the checkbox) so it’s not nested inside the main Link.
As per coding guidelines: Don't use target="_blank" without rel="noopener"; Always include a title element for icons unless there's text beside the icon; Make sure anchors have content that's accessible to screen readers.
Also applies to: 640-656
🤖 Prompt for AI Agents
In `@apps/web/src/app/projects/page.tsx` around lines 598 - 614, The icon "open in
new tab" control inside the project card (the button using project.id,
HugeiconsIcon and LinkSquare02Icon) must be removed from inside the main Link
and converted into a sibling anchor overlay so it's not a nested interactive
element; replace the inline window.open with a real <a> element that uses
target="_blank" and rel="noopener noreferrer", add an accessible label
(aria-label) and keep a title element for the icon (or include readable text for
screen readers), and position it as a sibling overlay similar to the checkbox
overlay in both grid and list card render paths so it remains clickable without
breaking the parent Link.
Comment on lines
+238
to
+250
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onStartAdding(); | ||
| }} | ||
| title="Add another shortcut" | ||
| className={isRecording && recordingMode === "add" ? "ring-2 ring-primary" : ""} | ||
| > | ||
| <HugeiconsIcon icon={PlusSignIcon} className="size-3" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the shortcuts-dialog.tsx file around the mentioned lines
wc -l apps/web/src/components/editor/dialogs/shortcuts-dialog.tsxRepository: OpenCut-app/OpenCut
Length of output: 126
🏁 Script executed:
# Read the relevant sections of the file
sed -n '230,300p' apps/web/src/components/editor/dialogs/shortcuts-dialog.tsxRepository: OpenCut-app/OpenCut
Length of output: 1453
🏁 Script executed:
# Find and examine the HugeiconsIcon component
fd HugeiconsIcon --type f -e tsx -e ts | head -20Repository: OpenCut-app/OpenCut
Length of output: 45
🏁 Script executed:
# Search for HugeiconsIcon component definition
rg "HugeiconsIcon" --type tsx --type ts -A 5 | head -100Repository: OpenCut-app/OpenCut
Length of output: 90
🏁 Script executed:
# Also search for where it's defined as a component
rg "export.*HugeiconsIcon|function HugeiconsIcon" --type tsx --type tsRepository: OpenCut-app/OpenCut
Length of output: 90
🏁 Script executed:
# Look for the import statement in the shortcuts-dialog file
head -50 apps/web/src/components/editor/dialogs/shortcuts-dialog.tsxRepository: OpenCut-app/OpenCut
Length of output: 1221
🏁 Script executed:
# Search for HugeiconsIcon usage patterns to see if title prop is ever used
rg "HugeiconsIcon" -A 3 | head -100Repository: OpenCut-app/OpenCut
Length of output: 6537
🏁 Script executed:
# Check the hugeicons package documentation or type definitions
fd "hugeicons" --type dRepository: OpenCut-app/OpenCut
Length of output: 45
🏁 Script executed:
# Look for node_modules or package files that might have hugeicons info
find . -name "package.json" -type f -exec grep -l "hugeicons" {} \;Repository: OpenCut-app/OpenCut
Length of output: 86
🏁 Script executed:
# Check the package.json for hugeicons version
grep -A 2 -B 2 "hugeicons" apps/web/package.jsonRepository: OpenCut-app/OpenCut
Length of output: 291
🏁 Script executed:
# Search for other usages of HugeiconsIcon with multiple props to understand the API better
rg "HugeiconsIcon" -B 1 -A 5 | grep -A 6 "title\|aria-label\|role"Repository: OpenCut-app/OpenCut
Length of output: 1435
🏁 Script executed:
# Check the entire shortcuts-dialog file to see full context
cat -n apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx | tail -70Repository: OpenCut-app/OpenCut
Length of output: 1943
🏁 Script executed:
# Check the Button component to see if it already sets type="button"
rg "export.*Button|function Button" apps/web/src/components/ui/button.tsx -A 30 | head -50Repository: OpenCut-app/OpenCut
Length of output: 689
🏁 Script executed:
# Look for any HugeiconsIcon usage with title prop to see if it's even supported
rg "HugeiconsIcon.*title|title.*HugeiconsIcon"Repository: OpenCut-app/OpenCut
Length of output: 45
🏁 Script executed:
# Check for documentation or type info by looking at imports and how HugeiconsIcon is typed
rg "from.*@hugeicons/react" -B 2 -A 10Repository: OpenCut-app/OpenCut
Length of output: 26963
🏁 Script executed:
# Look for the Button component full implementation
cat apps/web/src/components/ui/button.tsxRepository: OpenCut-app/OpenCut
Length of output: 2206
🏁 Script executed:
# Check if there are other examples of title being used on SVG or icon components in the codebase
rg "title=" apps/web/src/components -A 1 | grep -i "icon\|svg" -B 1 -A 1Repository: OpenCut-app/OpenCut
Length of output: 472
🏁 Script executed:
# Look for HugeiconsIconProps type definition in node_modules or see if it extends common props
find . -name "*hugeicons*" -type d 2>/dev/null | head -5Repository: OpenCut-app/OpenCut
Length of output: 45
🏁 Script executed:
# Check the learnings - they mention title for icons
grep -i "title.*icon\|icon.*title" << 'EOF'
Always include a `title` element for icons unless there's text beside the icon
Always include a `title` element for SVG elements
EOFRepository: OpenCut-app/OpenCut
Length of output: 141
🏁 Script executed:
# Check if the Button's title attribute is sufficient or if icon needs separate title
rg "title=" apps/web/src/components/ui -A 2 -B 2 | head -40Repository: OpenCut-app/OpenCut
Length of output: 972
🏁 Script executed:
# Look at what HugeiconsIconProps might include
rg "HugeiconsIconProps" -A 5Repository: OpenCut-app/OpenCut
Length of output: 803
🏁 Script executed:
# Let's verify HugeiconsIcon accepts standard HTML props by checking one more usage
rg "HugeiconsIcon" apps/web/src/components/ui/spinner.tsx -A 8Repository: OpenCut-app/OpenCut
Length of output: 498
Add button type attributes, use full parameter names, and include accessible titles for icon-only buttons.
All instances need:
type="button"on Button elements (lines 238, 282)- Rename abbreviated
eparameter toevent(lines 241, 267, 273) - Add
aria-label="Add another shortcut"to the Button at line 238 - Add
title="Add another shortcut"to the HugeiconsIcon at line 249
Changes needed
At lines 238-250, add type="button" and aria-label to Button, rename parameter, and add title to icon:
<Button
+ type="button"
variant="outline"
size="sm"
- onClick={(e) => {
- e.preventDefault();
- e.stopPropagation();
+ onClick={(event) => {
+ event.preventDefault();
+ event.stopPropagation();
onStartAdding();
}}
title="Add another shortcut"
+ aria-label="Add another shortcut"
className={isRecording && recordingMode === "add" ? "ring-2 ring-primary" : ""}
>
- <HugeiconsIcon icon={PlusSignIcon} className="size-3" />
+ <HugeiconsIcon icon={PlusSignIcon} className="size-3" title="Add another shortcut" />
</Button>At lines 267-287:
- Button (line 282): add
type="button" - Handlers: rename
(e)to(event)in bothhandleClick(line 267) andhandleRightClick(line 273)
Per guidelines: "Always include a type attribute for button elements", "Never abbreviate variable and parameter names", and "Always include a title element for icons unless there's text beside the icon".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| variant="outline" | |
| size="sm" | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onStartAdding(); | |
| }} | |
| title="Add another shortcut" | |
| className={isRecording && recordingMode === "add" ? "ring-2 ring-primary" : ""} | |
| > | |
| <HugeiconsIcon icon={PlusSignIcon} className="size-3" /> | |
| </Button> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| size="sm" | |
| onClick={(event) => { | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| onStartAdding(); | |
| }} | |
| title="Add another shortcut" | |
| aria-label="Add another shortcut" | |
| className={isRecording && recordingMode === "add" ? "ring-2 ring-primary" : ""} | |
| > | |
| <HugeiconsIcon icon={PlusSignIcon} className="size-3" title="Add another shortcut" /> | |
| </Button> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx` around lines 238
- 250, Add explicit type and accessibility attributes and expand short parameter
names: update the icon-only Button that calls onStartAdding so it includes
type="button" and aria-label="Add another shortcut", rename the inline onClick
parameter from e to event in that handler, and add title="Add another shortcut"
to the HugeiconsIcon; likewise, add type="button" to the other Button (the one
around line 282) and rename the parameters for handleClick and handleRightClick
from (e) to (event) to use full names consistently.
- Add theme toggle to Projects page header (#689) - Fix nested button HTML validation errors in header and hero (#687) - Use asChild prop to render Links with button styles - Add ability to add multiple keyboard shortcuts per action (#696) - New "+" button to add additional shortcuts - Replace mode only removes the specific key being edited - Right-click to remove a shortcut when multiple exist Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add LinkSquare02Icon to grid and list views on Projects page - Icon appears on hover and opens project editor in new tab - Prevents default link navigation to allow new tab opening Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Validates file sizes before processing media uploads: - Image limit: 50MB - Video limit: 500MB - Audio limit: 100MB Shows clear error messages with actual file size when exceeded. Valid files still process even when some files are rejected. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds a "cut-selected" action that copies selected elements to clipboard and then deletes them, enabling standard cut behavior. Note: Drag/drop elements to other tracks was already implemented. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Creates a new /about page with: - Project mission and vision - Key features overview - Open source information - Community links - Call to action Updates footer to link to /about instead of GitHub README. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The VideoCache was creating Input objects but not storing them, so input.dispose() was never called. This caused WebCodecs VideoDecoder resources to leak, leading to browser crashes during video playback. Changes: - Store Input object in VideoSinkData - Call input.dispose() on initialization errors - Call input.dispose() when clearing video from cache - Clear frame references when disposing This follows the same pattern as AudioManager which properly manages Input disposal. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use linear interpolation for resampling instead of nearest neighbor - Clamp output samples to [-1, 1] range to prevent distortion from overlapping audio clips exceeding valid amplitude range This should reduce audio artifacts in exported videos, especially when multiple audio sources overlap. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The eye icon in the track labels was not responding to clicks because the onClick handler was placed directly on HugeiconsIcon which doesn't forward click events to the underlying SVG. Fixed by wrapping the icon in a button element. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a video file uses a codec not supported by WebCodecs (like H.265/HEVC from Clipchamp), show a helpful toast message suggesting to convert to H.264/MP4 format instead of failing silently. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Multiple functions were creating mediabunny Input objects without disposing them, causing WebCodecs decoder resources to leak. This led to RAM consumption spiraling until crash when importing video clips. Fixed by adding try/finally blocks to ensure input.dispose() is called: - getVideoInfo(): leaked Input when getting video metadata - generateThumbnail(): leaked Input when generating thumbnails - decodeAndMixAudioSource(): leaked Input when mixing audio Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/editor/panels/timeline/index.tsx (1)
529-551:⚠️ Potential issue | 🟠 MajorIcon-only
<button>is missing an accessible name.The
TrackToggleIconrenders a button whose only content is an icon with noaria-labelon the<button>and notitleonHugeiconsIcon. Screen readers will announce it as a completely unlabeled button, and the mute/visibility distinction is lost entirely.The component needs a
labelprop that is forwarded asaria-label(or as atitleon the icon). Since the two call sites already have context (mute vs. visibility), they can supply the appropriate string.♿ Proposed fix
function TrackToggleIcon({ isOff, icons, onClick, + label, }: { isOff: boolean; icons: { on: IconSvgElement; off: IconSvgElement; }; onClick: () => void; + label: string; }) { return ( <button type="button" onClick={onClick} + aria-label={label} className="flex items-center justify-center" > <HugeiconsIcon icon={isOff ? icons.off : icons.on} className={`size-4 ${isOff ? "text-destructive" : "text-muted-foreground"}`} /> </button> ); }Update the two call sites:
{canTracktHaveAudio(track) && ( <TrackToggleIcon isOff={track.muted} icons={{ on: VolumeHighIcon, off: VolumeOffIcon }} onClick={() => editor.timeline.toggleTrackMute({ trackId: track.id })} + label={track.muted ? "Unmute track" : "Mute track"} /> )} {canTrackBeHidden(track) && ( <TrackToggleIcon isOff={track.hidden} icons={{ on: ViewIcon, off: ViewOffSlashIcon }} onClick={() => editor.timeline.toggleTrackVisibility({ trackId: track.id })} + label={track.hidden ? "Show track" : "Hide track"} /> )}As per coding guidelines: "Always include a
titleelement for icons unless there's text beside the icon" and "Give all elements requiring alt text meaningful information for screen readers."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/timeline/index.tsx` around lines 529 - 551, The TrackToggleIcon button is icon-only and lacks an accessible name; update TrackToggleIcon to accept a label prop (e.g., label: string) and forward it as aria-label on the <button> (and/or pass it as title to HugeiconsIcon) so screen readers can announce the action; update call sites that render TrackToggleIcon (the mute and visibility toggles) to pass meaningful labels like "Mute track" / "Unmute track" or "Hide track" / "Show track" as appropriate and keep the existing isOff logic unchanged.apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx (1)
197-230:⚠️ Potential issue | 🟠 Major
rawKeys[index]correlates withdisplayKeysby position, but the arrays can be misaligned — wrong key gets replaced or removed.
displayKeysis derived fromshortcut.keyswith Cmd-variants filtered out, whilerawKeyscomes directly fromObject.entries(keybindings)with no filtering and no guaranteed order. These two arrays can diverge in both length and ordering.Concrete failure scenario:
shortcut.keys = ["Ctrl+C", "Cmd+C"]→displayKeys = ["Ctrl+C"](Cmd filtered out)rawKeysfrom the store =["Cmd+C", "Ctrl+C"](insertion order differs)displayKeys[0]is "Ctrl+C", butrawKey = rawKeys[0]= "Cmd+C" → replacing the displayed "Ctrl+C" slot actually replaces "Cmd+C" in the storeA secondary impact: the
displayKeys.length > 1guard on line 223 uses the filtered count to decide if removal is allowed, butrawKeys.length(unfiltered) might be larger — meaning the remove action stays disabled even when multiple bindings exist.The fix is to look up the matching raw key by string comparison rather than positional index:
🐛 Proposed fix
const rawKeys = Object.entries(keybindings) .filter(([, action]) => action === shortcut.action) .map(([key]) => key as ShortcutKey); // ...inside the displayKeys.map: - const rawKey = rawKeys[index]; + const rawKey = rawKeys.find( + (rk) => rk.toLowerCase() === key.toLowerCase() + ); // Update remove guard to use total raw key count - displayKeys.length > 1 && rawKey + rawKeys.length > 1 && rawKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx` around lines 197 - 230, The bug is that rawKeys is indexed by position while displayKeys is a filtered/reordered view, causing replace/remove to target the wrong binding; update the code in the EditableShortcutKey rendering (where rawKey = rawKeys[index] is computed) to locate the corresponding raw key by value instead of by index: compute rawKey by searching rawKeys for the entry whose original key string, when transformed the same way displayKeys are derived (i.e., the same normalization/filter that removes Cmd-variants), equals the displayed key; then use that found rawKey for onStartReplacing and onRemoveKey and base the removal-enabled check on the actual count of rawKeys that map to this shortcut (not merely displayKeys.length). Reference symbols: useKeybindingsStore, rawKeys, displayKeys, EditableShortcutKey, onStartReplacing, onRemoveKey.
♻️ Duplicate comments (2)
apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx (2)
238-250: Previously flagged unresolved issues: missingtype="button", abbreviated parametere, and missingaria-labelon icon-only button.These were raised in the last review and remain unaddressed:
- Line 238:
<Button>has notype="button".- Line 241:
(e)should be(event).- Line 249:
<HugeiconsIcon>is the sole button content; the wrapping<Button>needsaria-label="Add another shortcut"since decorative icons should be inside elements that already have accessible text, and meaningful icon-only buttons must have a label.🐛 Proposed fix
<Button + type="button" variant="outline" size="sm" - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); + onClick={(event) => { + event.preventDefault(); + event.stopPropagation(); onStartAdding(); }} title="Add another shortcut" + aria-label="Add another shortcut" className={isRecording && recordingMode === "add" ? "ring-2 ring-primary" : ""} > <HugeiconsIcon icon={PlusSignIcon} className="size-3" /> </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx` around lines 238 - 250, Update the icon-only <Button> used to trigger adding a shortcut: set an explicit type="button" on the <Button>, rename the onClick handler parameter from (e) to (event) and use event.preventDefault()/event.stopPropagation(), and add aria-label="Add another shortcut" to the <Button> (the <HugeiconsIcon> can remain decorative with no accessible name); keep the existing title prop and conditional className and call to onStartAdding().
273-286:⚠️ Potential issue | 🟡 MinorAbbreviated parameter
e(previously flagged) and right-click removal is inaccessible via keyboard.
- Line 273:
(e: React.MouseEvent)→ useeventper naming guidelines. (Previously flagged forhandleClick/handleRightClick.)onContextMenuis the sole mechanism for triggering key removal — keyboard users have no way to invoke it. Consider surfacing a small visible remove button (conditionally rendered whenonRemoveis defined) rather than relying on right-click as the only path.🐛 Proposed fix for parameter name
- const handleRightClick = (e: React.MouseEvent) => { - e.preventDefault(); - e.stopPropagation(); + const handleRightClick = (event: React.MouseEvent) => { + event.preventDefault(); + event.stopPropagation(); if (onRemove) { onRemove(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx` around lines 273 - 286, Rename the abbreviated parameter e to event in the handleRightClick (and ensure handleClick follows the same naming) to follow the event naming guideline, and replace the inaccessible right-click-only removal by rendering a small, visible, keyboard-accessible remove control when onRemove is provided: add a conditional remove Button/IconButton (with an explicit aria-label like "Remove shortcut"), attach onClick to call onRemove, and ensure it is reachable by keyboard (focusable, has keyboard handlers or is a native button) so keyboard users can remove shortcuts without relying on onContextMenu; keep the existing handleRightClick as a secondary (mouse) path.
🧹 Nitpick comments (4)
apps/web/src/hooks/actions/use-editor-actions.ts (1)
282-295: Extract clipboard copy logic into a helper to avoid duplication.The multi-step copy block now exists in both copy-selected and cut-selected. A small helper (e.g.,
copySelectionToClipboard) reduces drift and keeps future changes in one place.As per coding guidelines, "Create helper functions for multi-step operations instead of inline complex logic."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/actions/use-editor-actions.ts` around lines 282 - 295, Extract the repeated multi-step copy logic into a helper named e.g. copySelectionToClipboard(editor, selectedElements, setClipboard): move the block that calls editor.timeline.getElementsWithTracks({ elements: selectedElements }), maps results to { trackId: track.id, trackType: track.type, element: elementWithoutId } and calls setClipboard({ items }), then replace the inline blocks inside the copy-selected and cut-selected handlers with a single call to copySelectionToClipboard(...). Ensure the helper strips element IDs the same way and is imported/used where both copy-selected and cut-selected live.apps/web/src/components/editor/panels/assets/views/assets.tsx (2)
113-147: Simplify by passingFile[]directly (avoidDataTransfer).
processMediaAssetsalready acceptsFile[], so building aDataTransferadds an extra DOM dependency and copy without benefits. PassingvalidFilesdirectly is simpler and more robust.♻️ Proposed simplification
- // Create a new FileList-like object from valid files - const dataTransfer = new DataTransfer(); - for (const file of validFiles) { - dataTransfer.items.add(file); - } - const validFileList = dataTransfer.files; - setIsProcessing(true); setProgress(0); try { const processedAssets = await processMediaAssets({ - files: validFileList, + files: validFiles, onProgress: (progress: { progress: number }) => setProgress(progress.progress), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/assets.tsx` around lines 113 - 147, The DataTransfer creation and validFileList are unnecessary because processMediaAssets already accepts File[]; remove the DataTransfer block (the DataTransfer instantiation and the loop that adds files to dataTransfer and the validFileList variable) and pass the existing validFiles array directly to processMediaAssets (replace the files: validFileList argument with files: validFiles), keeping setIsProcessing, setProgress and error-toasting logic unchanged.
48-85: Prefer shared media-type utility to avoid drift.The new
getMediaTypeFromFileduplicates logic likely already in the media utilities (processMediaAssets relies on similar detection). If a shared helper exists (or can be exported), reuse it here to keep MIME handling consistent across the app.
Based on learnings “Import and use media utility functions from apps/web/src/lib/media (processMediaAssets, decodeAudioToFloat32, generateThumbnail, etc.) for media operations.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/assets.tsx` around lines 48 - 85, This duplicates MIME-detection logic—replace the local getMediaTypeFromFile with the shared helper from the media utilities and align size checks with existing constants: import the media-type helper (the function used by processMediaAssets in apps/web/src/lib/media) and use it in validateFileSize (and remove getMediaTypeFromFile), keep FILE_SIZE_LIMITS here or refactor to use any shared limit constants if present, and if the helper is not exported, export it from the media module so validateFileSize, processMediaAssets, and generateThumbnail all use the same detection logic.apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx (1)
197-200: Prefer the store'sgetKeybindingsForActionover inline manual filtering.The keybindings store already exposes
getKeybindingsForAction(action)that does exactly what lines 198-200 do. Subscribing to the rawkeybindingsmap also causesShortcutItemto re-render on any keybinding change across the entire store, not just ones relevant to this shortcut.♻️ Proposed refactor
- const { keybindings } = useKeybindingsStore(); - const rawKeys = Object.entries(keybindings) - .filter(([, action]) => action === shortcut.action) - .map(([key]) => key as ShortcutKey); + const { getKeybindingsForAction } = useKeybindingsStore(); + const rawKeys = getKeybindingsForAction(shortcut.action);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx` around lines 197 - 200, Replace the manual filtering of the entire keybindings map with the store's selector so ShortcutItem only subscribes to relevant keys: instead of reading keybindings via useKeybindingsStore() and computing rawKeys by filtering, call getKeybindingsForAction(shortcut.action) from useKeybindingsStore (or pass it as the selector) to obtain rawKeys; remove the keybindings subscription so changes to unrelated bindings don't re-render ShortcutItem. Ensure you reference useKeybindingsStore, getKeybindingsForAction, rawKeys, ShortcutItem, and shortcut.action when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/hooks/actions/use-editor-actions.ts`:
- Around line 277-300: The cut-selected handler currently copies items to
clipboard and calls editor.timeline.deleteElements but leaves selectedElements
pointing to deleted items; update the "cut-selected" useActionHandler to clear
the selection after calling editor.timeline.deleteElements (same approach used
in the delete-selected handler) so the UI doesn't hold stale references—invoke
the selection-clearing logic used elsewhere (e.g., the same function or action
that delete-selected calls) immediately after deleteElements and before
returning.
In `@apps/web/src/lib/media/mediabunny.ts`:
- Around line 21-37: The local variable named fps should be renamed to a
descriptive identifier (e.g., framesPerSecond or averageFramesPerSecond) to
follow the no-abbreviations guideline; update the assignment from
packetStats.averagePacketRate to that new name and keep the returned object
property as fps: framesPerSecond (or fps: averageFramesPerSecond) so external
consumers still receive the fps field while the internal variable is
descriptive; change references in the function computeDuration /
getPrimaryVideoTrack / packetStats usage accordingly.
- Around line 160-173: Rename the abbreviated loop variables in the
mixing/resampling loop to descriptive names: change ch to channelIndex, i to
resampledSampleIndex (or sampleIndex), outputIdx to outputSampleIndex, and
sourceIdx to sourceSampleIndex; update all usages inside the loop where
mixBuffers, buffer.getChannelData, resampleRatio, outputStartSample,
totalSamples, and NUM_CHANNELS are referenced (the loop that iterates channels
and the inner loop that computes resampledLength and indexes) so the new names
are used consistently and preserve existing logic and bounds checks.
---
Outside diff comments:
In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx`:
- Around line 197-230: The bug is that rawKeys is indexed by position while
displayKeys is a filtered/reordered view, causing replace/remove to target the
wrong binding; update the code in the EditableShortcutKey rendering (where
rawKey = rawKeys[index] is computed) to locate the corresponding raw key by
value instead of by index: compute rawKey by searching rawKeys for the entry
whose original key string, when transformed the same way displayKeys are derived
(i.e., the same normalization/filter that removes Cmd-variants), equals the
displayed key; then use that found rawKey for onStartReplacing and onRemoveKey
and base the removal-enabled check on the actual count of rawKeys that map to
this shortcut (not merely displayKeys.length). Reference symbols:
useKeybindingsStore, rawKeys, displayKeys, EditableShortcutKey,
onStartReplacing, onRemoveKey.
In `@apps/web/src/components/editor/panels/timeline/index.tsx`:
- Around line 529-551: The TrackToggleIcon button is icon-only and lacks an
accessible name; update TrackToggleIcon to accept a label prop (e.g., label:
string) and forward it as aria-label on the <button> (and/or pass it as title to
HugeiconsIcon) so screen readers can announce the action; update call sites that
render TrackToggleIcon (the mute and visibility toggles) to pass meaningful
labels like "Mute track" / "Unmute track" or "Hide track" / "Show track" as
appropriate and keep the existing isOff logic unchanged.
---
Duplicate comments:
In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx`:
- Around line 238-250: Update the icon-only <Button> used to trigger adding a
shortcut: set an explicit type="button" on the <Button>, rename the onClick
handler parameter from (e) to (event) and use
event.preventDefault()/event.stopPropagation(), and add aria-label="Add another
shortcut" to the <Button> (the <HugeiconsIcon> can remain decorative with no
accessible name); keep the existing title prop and conditional className and
call to onStartAdding().
- Around line 273-286: Rename the abbreviated parameter e to event in the
handleRightClick (and ensure handleClick follows the same naming) to follow the
event naming guideline, and replace the inaccessible right-click-only removal by
rendering a small, visible, keyboard-accessible remove control when onRemove is
provided: add a conditional remove Button/IconButton (with an explicit
aria-label like "Remove shortcut"), attach onClick to call onRemove, and ensure
it is reachable by keyboard (focusable, has keyboard handlers or is a native
button) so keyboard users can remove shortcuts without relying on onContextMenu;
keep the existing handleRightClick as a secondary (mouse) path.
---
Nitpick comments:
In `@apps/web/src/components/editor/dialogs/shortcuts-dialog.tsx`:
- Around line 197-200: Replace the manual filtering of the entire keybindings
map with the store's selector so ShortcutItem only subscribes to relevant keys:
instead of reading keybindings via useKeybindingsStore() and computing rawKeys
by filtering, call getKeybindingsForAction(shortcut.action) from
useKeybindingsStore (or pass it as the selector) to obtain rawKeys; remove the
keybindings subscription so changes to unrelated bindings don't re-render
ShortcutItem. Ensure you reference useKeybindingsStore, getKeybindingsForAction,
rawKeys, ShortcutItem, and shortcut.action when making the change.
In `@apps/web/src/components/editor/panels/assets/views/assets.tsx`:
- Around line 113-147: The DataTransfer creation and validFileList are
unnecessary because processMediaAssets already accepts File[]; remove the
DataTransfer block (the DataTransfer instantiation and the loop that adds files
to dataTransfer and the validFileList variable) and pass the existing validFiles
array directly to processMediaAssets (replace the files: validFileList argument
with files: validFiles), keeping setIsProcessing, setProgress and error-toasting
logic unchanged.
- Around line 48-85: This duplicates MIME-detection logic—replace the local
getMediaTypeFromFile with the shared helper from the media utilities and align
size checks with existing constants: import the media-type helper (the function
used by processMediaAssets in apps/web/src/lib/media) and use it in
validateFileSize (and remove getMediaTypeFromFile), keep FILE_SIZE_LIMITS here
or refactor to use any shared limit constants if present, and if the helper is
not exported, export it from the media module so validateFileSize,
processMediaAssets, and generateThumbnail all use the same detection logic.
In `@apps/web/src/hooks/actions/use-editor-actions.ts`:
- Around line 282-295: Extract the repeated multi-step copy logic into a helper
named e.g. copySelectionToClipboard(editor, selectedElements, setClipboard):
move the block that calls editor.timeline.getElementsWithTracks({ elements:
selectedElements }), maps results to { trackId: track.id, trackType: track.type,
element: elementWithoutId } and calls setClipboard({ items }), then replace the
inline blocks inside the copy-selected and cut-selected handlers with a single
call to copySelectionToClipboard(...). Ensure the helper strips element IDs the
same way and is imported/used where both copy-selected and cut-selected live.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/web/src/app/about/page.tsxapps/web/src/app/projects/page.tsxapps/web/src/components/editor/dialogs/shortcuts-dialog.tsxapps/web/src/components/editor/panels/assets/views/assets.tsxapps/web/src/components/editor/panels/timeline/index.tsxapps/web/src/components/footer.tsxapps/web/src/components/header.tsxapps/web/src/components/landing/hero.tsxapps/web/src/hooks/actions/use-editor-actions.tsapps/web/src/lib/actions/definitions.tsapps/web/src/lib/media/audio.tsapps/web/src/lib/media/mediabunny.tsapps/web/src/lib/media/processing.tsapps/web/src/services/video-cache/service.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/components/landing/hero.tsx
- apps/web/src/lib/actions/definitions.ts
- apps/web/src/app/projects/page.tsx
- apps/web/src/components/header.tsx
- apps/web/src/lib/media/audio.ts
Comment on lines
+277
to
+300
| useActionHandler( | ||
| "cut-selected", | ||
| () => { | ||
| if (selectedElements.length === 0) return; | ||
|
|
||
| // Copy elements to clipboard | ||
| const results = editor.timeline.getElementsWithTracks({ | ||
| elements: selectedElements, | ||
| }); | ||
| const items = results.map(({ track, element }) => { | ||
| const { ...elementWithoutId } = element; | ||
| return { | ||
| trackId: track.id, | ||
| trackType: track.type, | ||
| element: elementWithoutId, | ||
| }; | ||
| }); | ||
| setClipboard({ items }); | ||
|
|
||
| // Delete the selected elements | ||
| editor.timeline.deleteElements({ | ||
| elements: selectedElements, | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
Clear selection after cutting to avoid stale references.
After editor.timeline.deleteElements, the selection still points at deleted elements. This can leave the UI in an inconsistent state and break follow-up actions. Consider clearing selection here just like in the delete-selected handler.
✅ Suggested fix
// Delete the selected elements
editor.timeline.deleteElements({
elements: selectedElements,
});
+ editor.selection.clearSelection();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useActionHandler( | |
| "cut-selected", | |
| () => { | |
| if (selectedElements.length === 0) return; | |
| // Copy elements to clipboard | |
| const results = editor.timeline.getElementsWithTracks({ | |
| elements: selectedElements, | |
| }); | |
| const items = results.map(({ track, element }) => { | |
| const { ...elementWithoutId } = element; | |
| return { | |
| trackId: track.id, | |
| trackType: track.type, | |
| element: elementWithoutId, | |
| }; | |
| }); | |
| setClipboard({ items }); | |
| // Delete the selected elements | |
| editor.timeline.deleteElements({ | |
| elements: selectedElements, | |
| }); | |
| }, | |
| useActionHandler( | |
| "cut-selected", | |
| () => { | |
| if (selectedElements.length === 0) return; | |
| // Copy elements to clipboard | |
| const results = editor.timeline.getElementsWithTracks({ | |
| elements: selectedElements, | |
| }); | |
| const items = results.map(({ track, element }) => { | |
| const { ...elementWithoutId } = element; | |
| return { | |
| trackId: track.id, | |
| trackType: track.type, | |
| element: elementWithoutId, | |
| }; | |
| }); | |
| setClipboard({ items }); | |
| // Delete the selected elements | |
| editor.timeline.deleteElements({ | |
| elements: selectedElements, | |
| }); | |
| editor.selection.clearSelection(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/hooks/actions/use-editor-actions.ts` around lines 277 - 300, The
cut-selected handler currently copies items to clipboard and calls
editor.timeline.deleteElements but leaves selectedElements pointing to deleted
items; update the "cut-selected" useActionHandler to clear the selection after
calling editor.timeline.deleteElements (same approach used in the
delete-selected handler) so the UI doesn't hold stale references—invoke the
selection-clearing logic used elsewhere (e.g., the same function or action that
delete-selected calls) immediately after deleteElements and before returning.
Comment on lines
+21
to
+37
| try { | ||
| const duration = await input.computeDuration(); | ||
| const videoTrack = await input.getPrimaryVideoTrack(); | ||
|
|
||
| if (!videoTrack) { | ||
| throw new Error("No video track found in the file"); | ||
| } | ||
|
|
||
| const packetStats = await videoTrack.computePacketStats(100); | ||
| const fps = packetStats.averagePacketRate; | ||
| if (!videoTrack) { | ||
| throw new Error("No video track found in the file"); | ||
| } | ||
|
|
||
| return { | ||
| duration, | ||
| width: videoTrack.displayWidth, | ||
| height: videoTrack.displayHeight, | ||
| fps, | ||
| }; | ||
| const packetStats = await videoTrack.computePacketStats(100); | ||
| const fps = packetStats.averagePacketRate; | ||
|
|
||
| return { | ||
| duration, | ||
| width: videoTrack.displayWidth, | ||
| height: videoTrack.displayHeight, | ||
| fps, | ||
| }; |
Contributor
There was a problem hiding this comment.
Rename fps to a full name to satisfy naming guideline.
fps is an abbreviation; use a descriptive variable name and map it to the fps property. As per coding guidelines, "Never abbreviate variable and parameter names. Use event not e, element not el."
Suggested fix
- const fps = packetStats.averagePacketRate;
+ const framesPerSecond = packetStats.averagePacketRate;
...
- fps,
+ fps: framesPerSecond,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const duration = await input.computeDuration(); | |
| const videoTrack = await input.getPrimaryVideoTrack(); | |
| if (!videoTrack) { | |
| throw new Error("No video track found in the file"); | |
| } | |
| const packetStats = await videoTrack.computePacketStats(100); | |
| const fps = packetStats.averagePacketRate; | |
| if (!videoTrack) { | |
| throw new Error("No video track found in the file"); | |
| } | |
| return { | |
| duration, | |
| width: videoTrack.displayWidth, | |
| height: videoTrack.displayHeight, | |
| fps, | |
| }; | |
| const packetStats = await videoTrack.computePacketStats(100); | |
| const fps = packetStats.averagePacketRate; | |
| return { | |
| duration, | |
| width: videoTrack.displayWidth, | |
| height: videoTrack.displayHeight, | |
| fps, | |
| }; | |
| try { | |
| const duration = await input.computeDuration(); | |
| const videoTrack = await input.getPrimaryVideoTrack(); | |
| if (!videoTrack) { | |
| throw new Error("No video track found in the file"); | |
| } | |
| const packetStats = await videoTrack.computePacketStats(100); | |
| const framesPerSecond = packetStats.averagePacketRate; | |
| return { | |
| duration, | |
| width: videoTrack.displayWidth, | |
| height: videoTrack.displayHeight, | |
| fps: framesPerSecond, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/media/mediabunny.ts` around lines 21 - 37, The local
variable named fps should be renamed to a descriptive identifier (e.g.,
framesPerSecond or averageFramesPerSecond) to follow the no-abbreviations
guideline; update the assignment from packetStats.averagePacketRate to that new
name and keep the returned object property as fps: framesPerSecond (or fps:
averageFramesPerSecond) so external consumers still receive the fps field while
the internal variable is descriptive; change references in the function
computeDuration / getPrimaryVideoTrack / packetStats usage accordingly.
Comment on lines
+160
to
+173
| for (let ch = 0; ch < NUM_CHANNELS; ch++) { | ||
| const sourceChannel = Math.min(ch, buffer.numberOfChannels - 1); | ||
| const channelData = buffer.getChannelData(sourceChannel); | ||
| const outputChannel = mixBuffers[ch]; | ||
|
|
||
| const resampledLength = Math.floor(channelData.length * resampleRatio); | ||
| for (let i = 0; i < resampledLength; i++) { | ||
| const outputIdx = outputStartSample + i; | ||
| if (outputIdx < 0 || outputIdx >= totalSamples) continue; | ||
| const resampledLength = Math.floor(channelData.length * resampleRatio); | ||
| for (let i = 0; i < resampledLength; i++) { | ||
| const outputIdx = outputStartSample + i; | ||
| if (outputIdx < 0 || outputIdx >= totalSamples) continue; | ||
|
|
||
| const sourceIdx = Math.floor(i / resampleRatio); | ||
| if (sourceIdx < channelData.length) { | ||
| outputChannel[outputIdx] += channelData[sourceIdx]; | ||
| const sourceIdx = Math.floor(i / resampleRatio); | ||
| if (sourceIdx < channelData.length) { | ||
| outputChannel[outputIdx] += channelData[sourceIdx]; | ||
| } |
Contributor
There was a problem hiding this comment.
Expand abbreviated loop variable names.
Short names like ch, i, outputIdx, and sourceIdx violate the naming guideline; use full, descriptive names instead. As per coding guidelines, "Never abbreviate variable and parameter names. Use event not e, element not el."
Suggested fix
- for (let ch = 0; ch < NUM_CHANNELS; ch++) {
- const sourceChannel = Math.min(ch, buffer.numberOfChannels - 1);
- const channelData = buffer.getChannelData(sourceChannel);
- const outputChannel = mixBuffers[ch];
+ for (let channelIndex = 0; channelIndex < NUM_CHANNELS; channelIndex++) {
+ const sourceChannel = Math.min(
+ channelIndex,
+ buffer.numberOfChannels - 1,
+ );
+ const channelData = buffer.getChannelData(sourceChannel);
+ const outputChannel = mixBuffers[channelIndex];
...
- for (let i = 0; i < resampledLength; i++) {
- const outputIdx = outputStartSample + i;
- if (outputIdx < 0 || outputIdx >= totalSamples) continue;
-
- const sourceIdx = Math.floor(i / resampleRatio);
- if (sourceIdx < channelData.length) {
- outputChannel[outputIdx] += channelData[sourceIdx];
+ for (let sampleIndex = 0; sampleIndex < resampledLength; sampleIndex++) {
+ const outputIndex = outputStartSample + sampleIndex;
+ if (outputIndex < 0 || outputIndex >= totalSamples) continue;
+
+ const sourceIndex = Math.floor(sampleIndex / resampleRatio);
+ if (sourceIndex < channelData.length) {
+ outputChannel[outputIndex] += channelData[sourceIndex];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (let ch = 0; ch < NUM_CHANNELS; ch++) { | |
| const sourceChannel = Math.min(ch, buffer.numberOfChannels - 1); | |
| const channelData = buffer.getChannelData(sourceChannel); | |
| const outputChannel = mixBuffers[ch]; | |
| const resampledLength = Math.floor(channelData.length * resampleRatio); | |
| for (let i = 0; i < resampledLength; i++) { | |
| const outputIdx = outputStartSample + i; | |
| if (outputIdx < 0 || outputIdx >= totalSamples) continue; | |
| const resampledLength = Math.floor(channelData.length * resampleRatio); | |
| for (let i = 0; i < resampledLength; i++) { | |
| const outputIdx = outputStartSample + i; | |
| if (outputIdx < 0 || outputIdx >= totalSamples) continue; | |
| const sourceIdx = Math.floor(i / resampleRatio); | |
| if (sourceIdx < channelData.length) { | |
| outputChannel[outputIdx] += channelData[sourceIdx]; | |
| const sourceIdx = Math.floor(i / resampleRatio); | |
| if (sourceIdx < channelData.length) { | |
| outputChannel[outputIdx] += channelData[sourceIdx]; | |
| } | |
| for (let channelIndex = 0; channelIndex < NUM_CHANNELS; channelIndex++) { | |
| const sourceChannel = Math.min( | |
| channelIndex, | |
| buffer.numberOfChannels - 1, | |
| ); | |
| const channelData = buffer.getChannelData(sourceChannel); | |
| const outputChannel = mixBuffers[channelIndex]; | |
| const resampledLength = Math.floor(channelData.length * resampleRatio); | |
| for (let sampleIndex = 0; sampleIndex < resampledLength; sampleIndex++) { | |
| const outputIndex = outputStartSample + sampleIndex; | |
| if (outputIndex < 0 || outputIndex >= totalSamples) continue; | |
| const sourceIndex = Math.floor(sampleIndex / resampleRatio); | |
| if (sourceIndex < channelData.length) { | |
| outputChannel[outputIndex] += channelData[sourceIndex]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/media/mediabunny.ts` around lines 160 - 173, Rename the
abbreviated loop variables in the mixing/resampling loop to descriptive names:
change ch to channelIndex, i to resampledSampleIndex (or sampleIndex), outputIdx
to outputSampleIndex, and sourceIdx to sourceSampleIndex; update all usages
inside the loop where mixBuffers, buffer.getChannelData, resampleRatio,
outputStartSample, totalSamples, and NUM_CHANNELS are referenced (the loop that
iterates channels and the inner loop that computes resampledLength and indexes)
so the new names are used consistently and preserve existing logic and bounds
checks.
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
This PR includes several UI improvements and new features:
asChildprop pattern to avoid invalid nested<button>elementsRelated Issues
Closes #624, #644, #653, #672, #687, #689, #696
Test plan
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI/UX