Feature#548/media preview panel #556
Conversation
|
@ayush18pop is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
WalkthroughAdds a media-preview feature: a Zustand media-preview store, UI wiring to open media previews (double-click/Enter) from media items, PreviewPanel dual-mode rendering with a PreviewModeToggle, and player components extended with preview-mode callbacks and sync. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MP as Media Panel (DraggableMediaItem)
participant MPS as MediaPreviewStore
participant PP as PreviewPanel
participant VP as Video/Audio Player
U->>MP: Double-click / Enter on media item
MP->>MPS: setPreviewMedia(mediaItem)
MPS-->>PP: isMediaPreviewMode=true, previewMedia set
PP->>VP: renderMediaPreview(isPlaying, onTimeUpdate, onPlay, onPause)
VP-->>MPS: onPlay/onPause/onTimeUpdate updates media state
sequenceDiagram
participant U as User
participant PT as PreviewModeToggle / PreviewToolbar
participant MPS as MediaPreviewStore
participant TL as Timeline Playback Store
U->>PT: Toggle mode
PT->>MPS: togglePreviewMode()
alt entering media preview while timeline playing
MPS->>TL: pause()
end
MPS-->>PT: isMediaPreviewMode updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
apps/web/src/components/ui/draggable-item.tsx (2)
110-114: Make the double‑click target keyboard-accessible.The handler is on a non-interactive element. Add keyboard access (Enter/Space) and an accessible name; otherwise, keyboard users can’t trigger preview. This also aligns with the guidelines to make static elements with interaction accessible.
Apply this diff:
- <div - ref={dragRef} - className="relative group w-28 h-28" - onDoubleClick={handleDoubleClick} - > + <div + ref={dragRef} + className="relative group w-28 h-28" + onDoubleClick={handleDoubleClick} + role="button" + tabIndex={0} + aria-label={`Preview ${name}`} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleDoubleClick(); + } + }} + >Note: Using role="button" and tabIndex={0} here avoids wrapping existing interactive children in a real , which would be invalid HTML and break nested controls.
151-155: Mirror keyboard accessibility for the compact variant.Add the same keyboard support and accessible label to the compact row container to keep behaviors consistent across variants.
- <div - ref={dragRef} - className="relative group w-full" - onDoubleClick={handleDoubleClick} - > + <div + ref={dragRef} + className="relative group w-full" + onDoubleClick={handleDoubleClick} + role="button" + tabIndex={0} + aria-label={`Preview ${name}`} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleDoubleClick(); + } + }} + >apps/web/src/stores/media-preview-store.ts (1)
11-18: Add an explicit exitPreviewMode action to avoid external setState.Downstream code (use-editor-actions.ts) directly mutates store state to leave preview mode. Provide a first-class action to centralize this transition, avoid ad-hoc mutations, and preserve invariants.
Apply these diffs:
Interface addition:
// Actions setMediaCurrentTime: (time: number) => void; setMediaIsPlaying: (playing: boolean) => void; setPreviewMedia: (media: MediaItem | null) => void; togglePreviewMode: () => void; clearPreview: () => void; toggleMediaPlayback: () => void; + exitPreviewMode: () => void;Action implementation:
clearPreview: () => set({ previewMedia: null, isMediaPreviewMode: false, mediaCurrentTime: 0, mediaIsPlaying: false }) + , + exitPreviewMode: () => + set({ + isMediaPreviewMode: false, + mediaIsPlaying: false, + })After adding this, downstream code can call useMediaPreviewStore.getState().exitPreviewMode().
Also applies to: 69-75
apps/web/src/hooks/use-editor-actions.ts (1)
34-41: Prefer a store action over direct setState when exiting preview.Directly mutating isMediaPreviewMode bypasses store semantics. Use a dedicated action to keep transitions consistent and maintainable.
Apply this diff (assuming exitPreviewMode is added to the store):
- if (isMediaPreviewMode && !isPlaying) { - // Pause any media preview playback - setMediaIsPlaying(false); - // Switch back to timeline preview mode by setting isMediaPreviewMode to false - useMediaPreviewStore.setState({ isMediaPreviewMode: false }); - } + if (isMediaPreviewMode && !isPlaying) { + // Pause any media preview playback + setMediaIsPlaying(false); + // Exit media preview mode + useMediaPreviewStore.getState().exitPreviewMode(); + }apps/web/src/components/ui/audio-player.tsx (1)
151-160: Avoid overloading clipStartTime in preview mode.Using clipStartTime to mean “media current time” in preview mode is confusing and error-prone. Consider a separate prop (e.g., previewCurrentTime) or a mode-specific prop bag to make intent explicit. You can continue to support the current API for compatibility and deprecate it gradually.
apps/web/src/components/ui/video-player.tsx (4)
151-161: Reduce media-preview time sync tolerance to improve scrubbing responsivenessA 0.5s threshold can feel laggy when scrubbing or nudging by small deltas. Lowering the tolerance makes the element seek more promptly to externally set preview time without noticeable thrashing.
Apply this diff:
- // In media preview mode, sync with clipStartTime (which is actually mediaCurrentTime) - if (Math.abs(video.currentTime - clipStartTime) > 0.5) { + // In media preview mode, sync with clipStartTime (mediaCurrentTime) + if (Math.abs(video.currentTime - clipStartTime) > 0.05) { video.currentTime = clipStartTime; }
172-186: Prefer lighter preload in preview mode
preload="auto"eagerly downloads media. For ad-hoc previews,metadatareduces bandwidth and time-to-interaction. You can keepautofor timeline mode and usemetadatafor preview mode.Apply this diff:
- preload="auto" + preload={isMediaPreviewMode ? "metadata" : "auto"}
123-150: Consider reporting duration to preview consumers on metadata loadIn media preview mode, downstream UI (e.g., progress bar) may not know the duration if
previewMedia.durationis unset. Consider adding an optionalonLoadedMetadata?: (duration: number) => voidprop and invoking it from the video element’sonLoadedMetadatahandler. This lets the preview store updatepreviewMedia.durationso scrubbing works even when duration wasn’t precomputed.If you want, I can wire this up end-to-end (prop typing, handler, and store update) in a follow-up patch.
172-186: Caption tracks missing on the video elementAccessibility guideline: include caption tracks for audio/video. Consider supporting an optional
tracksprop and rendering<track>elements (e.g., captions/subtitles) to improve accessibility for previews.apps/web/src/components/editor/preview-panel.tsx (5)
12-12: Avoid default React import in TSX filesPer the project guidelines, don’t import React itself. Keep only named hooks and import the React namespace as a type if you need
React.*types.Apply this diff:
-import React, { useEffect, useRef, useState, useCallback } from "react"; +import { useEffect, useRef, useState, useCallback } from "react"; +import type React from "react";
488-553: Render logic for media preview: good separation; minor trim props tweakThe preview renderer cleanly routes video/image/audio. For video/audio, you’re passing
trimEnd={previewMedia.duration ?? 0}. That value is only used in timeline mode, but settingtrimEndequal to the full duration is odd and could confuse future readers. Keep trim neutral in preview mode.Apply this diff to both video and audio cases:
- trimEnd={previewMedia.duration ?? 0} - clipDuration={previewMedia.duration ?? 0} + trimEnd={0} + clipDuration={previewMedia.duration ?? 0}
744-801: Add accessible labels to icon-only playback buttons (fullscreen)Buttons with only icons should expose an accessible name. Add dynamic
aria-label(and optionaltitle) to the play/pause button.Apply this diff:
<Button variant="text" size="icon" onClick={getCurrentToggle()} disabled={!hasAnyElements && !isMediaPreviewMode} className="h-auto p-0 text-foreground hover:text-foreground/80" - type="button" + type="button" + aria-label={getCurrentIsPlaying() ? "Pause" : "Play"} + title={getCurrentIsPlaying() ? "Pause" : "Play"} >Note: SkipBack/SkipForward already have descriptive titles.
996-1010: Add accessible labels to icon-only playback button (collapsed toolbar)Same rationale as fullscreen: add
aria-label(and optionaltitle) for the icon-only play/pause button.Apply this diff:
<Button variant="text" size="icon" onClick={currentToggle} disabled={!hasAnyElements && !isMediaPreviewMode} className="h-auto p-0" - type="button" + type="button" + aria-label={currentIsPlaying ? "Pause" : "Play"} + title={currentIsPlaying ? "Pause" : "Play"} >
676-686: Guard scrubbing when duration is unknownWhen
previewMedia.durationis initially 0/undefined, the computedtotalDurationis 0, making scrubbing no-op. Consider updatingpreviewMedia.durationon media metadata load (see suggestion in VideoPlayer) so preview scrubbing works immediately after load.I can add an
onLoadedMetadatacallback path and update the media preview store to set duration once available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/web/src/components/editor/media-panel/views/media.tsx(2 hunks)apps/web/src/components/editor/preview-panel.tsx(20 hunks)apps/web/src/components/ui/audio-player.tsx(3 hunks)apps/web/src/components/ui/draggable-item.tsx(6 hunks)apps/web/src/components/ui/video-player.tsx(4 hunks)apps/web/src/hooks/use-editor-actions.ts(2 hunks)apps/web/src/stores/media-preview-store.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{jsx,tsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include alangattribute on the html element.
Always include atitleattribute for iframe elements.
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress.
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur.
Include caption tracks for audio and video elements.
Use semantic elements instead of role attributes in JSX.
Make sure all anchors are valid and navigable.
Ensure all ARIA properties (aria-*) are valid.
Use valid, non-abstract ARIA roles for elements with...
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/audio-player.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't use the TypeScript directive @ts-ignore.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Use either T[] or Array consistently
Don't use the any type
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/hooks/use-editor-actions.tsapps/web/src/stores/media-preview-store.tsapps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/audio-player.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use the return value of React.render.
Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use primitive type aliases or misleading types.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of...
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/hooks/use-editor-actions.tsapps/web/src/stores/media-preview-store.tsapps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/audio-player.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{tsx,jsx}: Always include a title element for icons unless there's text beside the icon
Always include a type attribute for button elements
Accompany onClick with at least one of: onKeyUp, onKeyDown, or onKeyPress
Accompany onMouseOver/onMouseOut with onFocus/onBlur
Don't import React itself
Don't define React components inside other components
Don't use both children and dangerouslySetInnerHTML on the same element
Don't insert comments as text nodes
Use <>...</> instead of ...
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/audio-player.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use the comma operator
Use for...of statements instead of Array.forEach
Don't initialize variables to undefined
Use .flatMap() instead of map().flat() when possible
Don't assign a value to itself
Avoid unused imports and variables
Don't use await inside loops
Don't hardcode sensitive data like API keys and tokens
Don't use the delete operator
Don't use global eval()
Use String.slice() instead of String.substr() and String.substring()
Don't use else blocks when the if block breaks early
Put default function parameters and optional function parameters last
Use new when throwing an error
Use String.trimStart() and String.trimEnd() over String.trimLeft() and String.trimRight()
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/hooks/use-editor-actions.tsapps/web/src/stores/media-preview-store.tsapps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/audio-player.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
🧬 Code Graph Analysis (6)
apps/web/src/components/ui/draggable-item.tsx (2)
apps/web/src/stores/media-store.ts (1)
MediaItem(8-28)apps/web/src/stores/media-preview-store.ts (1)
useMediaPreviewStore(20-75)
apps/web/src/hooks/use-editor-actions.ts (2)
apps/web/src/stores/media-preview-store.ts (1)
useMediaPreviewStore(20-75)apps/web/src/constants/actions.ts (1)
useActionHandler(191-251)
apps/web/src/stores/media-preview-store.ts (2)
apps/web/src/stores/media-store.ts (1)
MediaItem(8-28)apps/web/src/stores/playback-store.ts (1)
usePlaybackStore(75-174)
apps/web/src/components/ui/audio-player.tsx (1)
apps/web/src/stores/playback-store.ts (1)
usePlaybackStore(75-174)
apps/web/src/components/ui/video-player.tsx (1)
apps/web/src/stores/playback-store.ts (1)
usePlaybackStore(75-174)
apps/web/src/components/editor/preview-panel.tsx (9)
apps/web/src/stores/media-preview-store.ts (1)
useMediaPreviewStore(20-75)apps/web/src/components/ui/video-player.tsx (1)
VideoPlayer(21-187)apps/web/src/components/ui/audio-player.tsx (1)
AudioPlayer(21-183)apps/web/src/components/editor/layout-guide-overlay.tsx (1)
LayoutGuideOverlay(20-27)apps/web/src/stores/media-store.ts (1)
MediaItem(8-28)apps/web/src/stores/project-store.ts (1)
DEFAULT_FPS(11-11)apps/web/src/components/ui/editable-timecode.tsx (1)
EditableTimecode(18-138)apps/web/src/lib/time.ts (1)
formatTimeCode(8-29)apps/web/src/lib/utils.ts (1)
cn(6-8)
🔇 Additional comments (10)
apps/web/src/components/ui/draggable-item.tsx (2)
15-17: Preview-store integration is clean and cohesive.Importing the preview store and setting preview media on double-click is straightforward and aligns with the store contract (pausing timeline on entry).
Also applies to: 56-63
31-31: Prop surface extension is backward-compatible.Adding optional mediaItem and destructuring it preserves existing call sites and enables the preview pathway without impacting drag/timeline behavior.
Also applies to: 47-47
apps/web/src/components/editor/media-panel/views/media.tsx (2)
505-506: LGTM: Passing mediaItem enables the preview pathway.Providing the full MediaItem to DraggableMediaItem ensures the new double‑click preview works in grid view without changing drag/timeline flows.
545-546: LGTM: Consistent preview support in list view.Parity with the grid view is maintained by passing mediaItem here as well.
apps/web/src/stores/media-preview-store.ts (1)
20-43: Store behavior is cohesive and respects timeline playback.Pausing timeline playback when entering media preview mode and resetting preview state is correct and prevents dual playback. The state shape and actions are sensible for the feature set.
apps/web/src/components/ui/audio-player.tsx (1)
14-19: Media-preview mode support is well-factored and minimally invasive.
- Opt-in via isPlaying prop is a clean switch; timeline behavior remains intact.
- Skipping timeline event wiring in preview mode prevents double-driving the element.
- Forwarding time/play/pause callbacks enables the preview store/toolbar to stay in sync.
Also applies to: 29-34, 38-49, 114-122, 123-149, 151-160
apps/web/src/components/ui/video-player.tsx (2)
15-18: Dual-mode external control API looks solidProp-driven media preview mode is cleanly isolated from timeline mode. The prop renames and gating via
externalIsPlaying !== undefinedkeep the component’s public surface area minimal while enabling external control callbacks.Also applies to: 30-35, 39-47
55-56: Event source for “playback-update” verifiedI located where the event is dispatched and where it’s being listened to:
- Dispatch in apps/web/src/stores/playback-store.ts (≈line 57):
window.dispatchEvent(new CustomEvent("playback-update", { detail: { time: newTime } }))- Listener in apps/web/src/components/ui/video-player.tsx (lines 90–92)
- (Also mirrored in audio-player.tsx)
No further changes needed—the playback subsystem properly emits “playback-update,” so the timeline sync path will be exercised.
apps/web/src/components/editor/preview-panel.tsx (2)
563-591: Conditional dual-mode mount looks correctThe switch between blur/timeline elements and media preview is straightforward and keeps preview logic contained. Passing through
renderMediaPreview()keeps fullscreen parity. Nicely done.
603-606: Store wiring and prop forwarding: LGTMThe preview-mode-related props (
isMediaPreviewMode,togglePreviewMode,previewMedia,clearPreview) are consistently threaded through PreviewToolbar and FullscreenPreview/Toolbar and align with the new store API.Also applies to: 624-629, 922-926
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| "toggle-play", | ||
| () => { | ||
| // If we're in media preview mode and trying to resume timeline playback | ||
| if (isMediaPreviewMode && !isPlaying) { |
There was a problem hiding this comment.
The space bar toggle logic only switches from media preview back to timeline mode when the timeline isn't already playing, which can cause inconsistent behavior.
View Details
📝 Patch Details
diff --git a/apps/web/src/hooks/use-editor-actions.ts b/apps/web/src/hooks/use-editor-actions.ts
index 00fe4b2..9e01cbc 100644
--- a/apps/web/src/hooks/use-editor-actions.ts
+++ b/apps/web/src/hooks/use-editor-actions.ts
@@ -31,8 +31,8 @@ export function useEditorActions() {
useActionHandler(
"toggle-play",
() => {
- // If we're in media preview mode and trying to resume timeline playback
- if (isMediaPreviewMode && !isPlaying) {
+ // If we're in media preview mode, always switch back to timeline mode
+ if (isMediaPreviewMode) {
// Pause any media preview playback
setMediaIsPlaying(false);
// Switch back to timeline preview mode by setting isMediaPreviewMode to false
diff --git a/bun.lock b/bun.lock
index ea7c8c9..018ca78 100644
--- a/bun.lock
+++ b/bun.lock
@@ -1320,10 +1320,10 @@
"motion/framer-motion/motion-utils": ["[email protected]", "", {}, "sha512-eAWoPgr4eFEOFfg2WjIsMoqJTW6Z8MTUCgn/GZ3VRpClWBdnbjryiA3ZSNLyxCTmCQx4RmYX6jX1iWHbenUPNQ=="],
- "opencut/@types/node/undici-types": ["[email protected]", "", {}, "sha512-t5Fy/nfn+14LuOc2KNYg75vZqClpAiqscVvMygNnlsHBFpSXdJaYtXMcdNLpl/Qvc3P2cB3s6lOV51nqsFq4ag=="],
-
"next/postcss/nanoid": ["[email protected]", "", { "bin": { "nanoid": "bin/nanoid.cjs" } }, "sha512-N8SpfPUnUp1bK+PMYW8qSWdl9U+wwNWI4QKxOYDy9JAro3WMX7p2OeVRF9v+347pnakNevPmiHhNmZ2HbFA76w=="],
+ "opencut/@types/node/undici-types": ["[email protected]", "", {}, "sha512-t5Fy/nfn+14LuOc2KNYg75vZqClpAiqscVvMygNnlsHBFpSXdJaYtXMcdNLpl/Qvc3P2cB3s6lOV51nqsFq4ag=="],
+
"opencut/next/@next/env": ["@next/[email protected]", "", {}, "sha512-ruM+q2SCOVCepUiERoxOmZY9ZVoecR3gcXNwCYZRvQQWRjhOiPJGmQ2fAiLR6YKWXcSAh7G79KEFxN3rwhs4LQ=="],
"opencut/next/@next/swc-darwin-arm64": ["@next/[email protected]", "", { "os": "darwin", "cpu": "arm64" }, "sha512-84dAN4fkfdC7nX6udDLz9GzQlMUwEMKD7zsseXrl7FTeIItF8vpk1lhLEnsotiiDt+QFu3O1FVWnqwcRD2U3KA=="],
Analysis
The toggle-play action handler has logic to switch from media preview mode back to timeline mode, but it only triggers when isMediaPreviewMode && !isPlaying (line 35). This means if the timeline is already playing when a user presses the space bar while in media preview mode, it won't switch back to timeline mode - it will just call toggle() which may not have the expected effect.
This creates inconsistent behavior where the space bar's effect depends on the current timeline playback state, which could confuse users who expect it to always control timeline playback when pressed.
Recommendation
Modify the condition to handle media preview mode consistently regardless of timeline state. Consider changing the logic to:
if (isMediaPreviewMode) {
// Always pause media preview and switch to timeline mode
setMediaIsPlaying(false);
useMediaPreviewStore.setState({ isMediaPreviewMode: false });
}
toggle();This ensures the space bar always switches back to timeline mode when in media preview, providing consistent behavior.
| poster={previewMedia.thumbnailUrl} | ||
| clipStartTime={mediaCurrentTime} | ||
| trimStart={0} | ||
| trimEnd={previewMedia.duration ?? 0} |
There was a problem hiding this comment.
The trimEnd parameter is incorrectly set to the full media duration, which will make the video unplayable in media preview mode.
View Details
📝 Patch Details
diff --git a/apps/web/src/components/editor/preview-panel.tsx b/apps/web/src/components/editor/preview-panel.tsx
index 0b249fc..aebaa01 100644
--- a/apps/web/src/components/editor/preview-panel.tsx
+++ b/apps/web/src/components/editor/preview-panel.tsx
@@ -501,7 +501,7 @@ export function PreviewPanel() {
poster={previewMedia.thumbnailUrl}
clipStartTime={mediaCurrentTime}
trimStart={0}
- trimEnd={previewMedia.duration ?? 0}
+ trimEnd={0}
clipDuration={previewMedia.duration ?? 0}
trackMuted={false}
isPlaying={mediaIsPlaying}
@@ -537,7 +537,7 @@ export function PreviewPanel() {
src={previewMedia.url}
clipStartTime={mediaCurrentTime}
trimStart={0}
- trimEnd={previewMedia.duration ?? 0}
+ trimEnd={0}
clipDuration={previewMedia.duration ?? 0}
trackMuted={false}
isPlaying={mediaIsPlaying}
Analysis
In the renderMediaPreview function for video content, trimEnd is set to previewMedia.duration ?? 0 (line 504). This is incorrect because trimEnd represents how much time to trim from the end of the media, not the total duration.
The video player calculates the clip end time using: clipEndTime = clipStartTime + (clipDuration - trimStart - trimEnd). With the current implementation:
clipDuration= media duration (e.g., 60 seconds)trimStart= 0trimEnd= media duration (60 seconds)
This results in: clipEndTime = clipStartTime + (60 - 0 - 60) = clipStartTime + 0, meaning the clip has zero duration and becomes unplayable.
The same issue exists for audio content on line 540.
Recommendation
Change trimEnd={previewMedia.duration ?? 0} to trimEnd={0} on both lines 504 and 540. For media preview mode, users typically want to see the full media without any trimming, so both trimStart and trimEnd should be 0.
… and state management : Changes recommended by CodeRabbit
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/web/src/components/ui/draggable-item.tsx (1)
64-72: Type nit + consider complete keyboard semantics
- Nit: Prefer importing the event type instead of
React.KeyboardEventto avoid relying on the globalReactnamespace in types.- Optional: For button-like semantics, Space should also activate the control. If global Space shortcuts must be preserved, consider handling Space only when the wrapper is focused and stop propagation.
Apply this small typing diff:
- const handleKeyDown = (e: React.KeyboardEvent) => { + const handleKeyDown = (e: KeyboardEvent<HTMLDivElement>) => {Add this import (outside the selected range) to support the change:
import type { KeyboardEvent } from "react";If you decide to support Space as an activator:
- const handleKeyDown = (e: KeyboardEvent<HTMLDivElement>) => { - // Only handle Enter key for media preview - let space bubble up for global shortcuts - if (e.key === "Enter" && mediaItem) { + const handleKeyDown = (e: KeyboardEvent<HTMLDivElement>) => { + // Handle Enter (and optionally Space) for media preview + if ((e.key === "Enter" /* || e.code === "Space" */) && mediaItem) { e.preventDefault(); + // e.stopPropagation(); // uncomment if enabling Space to avoid interfering with global shortcuts handleDoubleClick(); } // Don't handle space key here to allow global keybindings to work };apps/web/src/components/ui/video-player.tsx (5)
15-18: Clarify external-control props semantics (preview mode) with brief docsGood addition. To make usage unambiguous for callers, add lightweight JSDoc indicating that providing these props switches the component into externally controlled “media preview” mode and that callbacks only fire in that mode.
Apply this diff:
interface VideoPlayerProps { src: string; poster?: string; className?: string; clipStartTime: number; trimStart: number; trimEnd: number; clipDuration: number; trackMuted?: boolean; - isPlaying?: boolean; - onTimeUpdate?: (time: number) => void; - onPlay?: () => void; - onPause?: () => void; + /** + * Media-preview external control props. When any is provided, + * the component enters media-preview mode and ignores timeline play/seek events. + */ + isPlaying?: boolean; + /** Mirrors the <video> timeupdate event in media-preview mode (seconds). */ + onTimeUpdate?: (time: number) => void; + /** Fired when <video> starts playing in media-preview mode. */ + onPlay?: () => void; + /** Fired when <video> pauses in media-preview mode. */ + onPause?: () => void; } export function VideoPlayer({ src, poster, className = "", clipStartTime, trimStart, trimEnd, clipDuration, trackMuted = false, // Media preview props isPlaying: externalIsPlaying, onTimeUpdate, onPlay, onPause, }: VideoPlayerProps) {Also applies to: 30-35
37-47: Decouple “media preview mode” detection from isPlaying presenceRight now, mode detection hinges on externalIsPlaying !== undefined. That couples “mode” to a single prop and can surprise consumers (e.g., wanting preview mode without controlling play state yet). Consider an explicit flag to opt into preview mode.
Apply this diff:
interface VideoPlayerProps { @@ /** Fired when <video> pauses in media-preview mode. */ onPause?: () => void; + /** Force media-preview mode regardless of isPlaying presence. */ + forceMediaPreviewMode?: boolean; } @@ trackMuted = false, // Media preview props isPlaying: externalIsPlaying, onTimeUpdate, onPlay, onPause, + forceMediaPreviewMode, }: VideoPlayerProps) { const videoRef = useRef<HTMLVideoElement>(null); const timelineStore = usePlaybackStore(); // Use external props if provided (media preview mode), otherwise use timeline const isPlaying = externalIsPlaying !== undefined ? externalIsPlaying : timelineStore.isPlaying; const currentTime = timelineStore.currentTime; const { volume, speed, muted } = timelineStore; // Determine if this is media preview mode - const isMediaPreviewMode = externalIsPlaying !== undefined; + const isMediaPreviewMode = (forceMediaPreviewMode ?? false) || externalIsPlaying !== undefined;
123-150: Broaden event coverage for snappier external sync (seeking/initial load)In preview mode, consider also invoking onTimeUpdate during seeking and on loadedmetadata so external UIs reflect scrubs and initial time immediately.
Apply this diff:
video.addEventListener('timeupdate', handleTimeUpdate); + video.addEventListener('seeking', handleTimeUpdate); + video.addEventListener('loadedmetadata', handleTimeUpdate); video.addEventListener('play', handlePlay); video.addEventListener('pause', handlePause); @@ return () => { video.removeEventListener('timeupdate', handleTimeUpdate); + video.removeEventListener('seeking', handleTimeUpdate); + video.removeEventListener('loadedmetadata', handleTimeUpdate); video.removeEventListener('play', handlePlay); video.removeEventListener('pause', handlePause); };
151-161: Avoid fighting natural playback with aggressive time syncThe 0.05s tolerance is tight and applied unconditionally; if parents update “seek” frequently, this will cause churn while playing. Gate by paused state or use a looser threshold while playing.
Apply this diff:
// In media preview mode, sync with clipStartTime (mediaCurrentTime) - if (Math.abs(video.currentTime - clipStartTime) > 0.05) { - video.currentTime = clipStartTime; - } + const delta = Math.abs(video.currentTime - clipStartTime); + const threshold = video.paused ? 0.05 : 0.25; + if (delta > threshold) { + video.currentTime = clipStartTime; + }
6-19: Add caption/subtitle track support to meet accessibility guidelineRepo guideline: “Include caption tracks for audio and video elements.” Expose an optional tracks prop and render elements. This also future-proofs for subtitles and metadata.
Apply this diff:
"use client"; import { useRef, useEffect } from "react"; import { usePlaybackStore } from "@/stores/playback-store"; +type TextTrackSource = { + kind: "captions" | "subtitles" | "descriptions" | "chapters" | "metadata"; + src: string; + srcLang?: string; + label?: string; + default?: boolean; +}; + interface VideoPlayerProps { src: string; poster?: string; className?: string; clipStartTime: number; trimStart: number; trimEnd: number; clipDuration: number; trackMuted?: boolean; + /** Optional text tracks (captions/subtitles/etc.). */ + tracks?: TextTrackSource[]; /** * Media-preview external control props. When any is provided, * the component enters media-preview mode and ignores timeline play/seek events. */ isPlaying?: boolean; /** Mirrors the <video> timeupdate event in media-preview mode (seconds). */ onTimeUpdate?: (time: number) => void; /** Fired when <video> starts playing in media-preview mode. */ onPlay?: () => void; /** Fired when <video> pauses in media-preview mode. */ onPause?: () => void; -} + /** Force media-preview mode regardless of isPlaying presence. */ + forceMediaPreviewMode?: boolean; +} export function VideoPlayer({ src, poster, className = "", clipStartTime, trimStart, trimEnd, clipDuration, trackMuted = false, + tracks = [], // Media preview props isPlaying: externalIsPlaying, onTimeUpdate, onPlay, onPause, + forceMediaPreviewMode, }: VideoPlayerProps) { @@ return ( <video ref={videoRef} src={src} poster={poster} className={`max-w-full max-h-full object-contain ${className}`} playsInline preload={isMediaPreviewMode ? "metadata" : "auto"} controls={false} disablePictureInPicture disableRemotePlayback style={{ pointerEvents: "none" }} onContextMenu={(e) => e.preventDefault()} - /> + > + {tracks.map((t) => ( + <track + key={`${t.kind}:${t.src}${t.srcLang ? ":" + t.srcLang : ""}`} + kind={t.kind} + src={t.src} + srcLang={t.srcLang} + label={t.label} + default={t.default} + /> + ))} + </video> ); }Also applies to: 21-35, 172-186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/src/components/editor/preview-panel.tsx(20 hunks)apps/web/src/components/ui/audio-player.tsx(3 hunks)apps/web/src/components/ui/draggable-item.tsx(6 hunks)apps/web/src/components/ui/video-player.tsx(5 hunks)apps/web/src/hooks/use-editor-actions.ts(2 hunks)apps/web/src/stores/media-preview-store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/hooks/use-editor-actions.ts
- apps/web/src/stores/media-preview-store.ts
- apps/web/src/components/ui/audio-player.tsx
- apps/web/src/components/editor/preview-panel.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{jsx,tsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include alangattribute on the html element.
Always include atitleattribute for iframe elements.
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress.
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur.
Include caption tracks for audio and video elements.
Use semantic elements instead of role attributes in JSX.
Make sure all anchors are valid and navigable.
Ensure all ARIA properties (aria-*) are valid.
Use valid, non-abstract ARIA roles for elements with...
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/ui/video-player.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't use the TypeScript directive @ts-ignore.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Use either T[] or Array consistently
Don't use the any type
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/ui/video-player.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use the return value of React.render.
Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use primitive type aliases or misleading types.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of...
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/ui/video-player.tsx
**/*.{tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{tsx,jsx}: Always include a title element for icons unless there's text beside the icon
Always include a type attribute for button elements
Accompany onClick with at least one of: onKeyUp, onKeyDown, or onKeyPress
Accompany onMouseOver/onMouseOut with onFocus/onBlur
Don't import React itself
Don't define React components inside other components
Don't use both children and dangerouslySetInnerHTML on the same element
Don't insert comments as text nodes
Use <>...</> instead of ...
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/ui/video-player.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use the comma operator
Use for...of statements instead of Array.forEach
Don't initialize variables to undefined
Use .flatMap() instead of map().flat() when possible
Don't assign a value to itself
Avoid unused imports and variables
Don't use await inside loops
Don't hardcode sensitive data like API keys and tokens
Don't use the delete operator
Don't use global eval()
Use String.slice() instead of String.substr() and String.substring()
Don't use else blocks when the if block breaks early
Put default function parameters and optional function parameters last
Use new when throwing an error
Use String.trimStart() and String.trimEnd() over String.trimLeft() and String.trimRight()
Files:
apps/web/src/components/ui/draggable-item.tsxapps/web/src/components/ui/video-player.tsx
🧬 Code Graph Analysis (2)
apps/web/src/components/ui/draggable-item.tsx (2)
apps/web/src/stores/media-store.ts (1)
MediaItem(8-28)apps/web/src/stores/media-preview-store.ts (1)
useMediaPreviewStore(21-82)
apps/web/src/components/ui/video-player.tsx (1)
apps/web/src/stores/playback-store.ts (1)
usePlaybackStore(75-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vade Review
🔇 Additional comments (7)
apps/web/src/components/ui/draggable-item.tsx (4)
15-16: Type-only import and store hook usage look goodImporting the preview store and using
import typeforMediaItemaligns with our TS guidelines. No issues here.
31-31: Prop plumbing for media preview is correctAdding
mediaItem?: MediaItemto the props and destructuring it in the component is straightforward and non-breaking. This keeps the preview behavior opt-in per usage.Also applies to: 47-47
56-56: Good: scoped store usageGrabbing
setPreviewMediafrom the preview store at the component level is appropriate and avoids prop-drilling.
58-62: Double-click handler is minimal and safeGuarding on
mediaItemprevents accidental preview attempts when not provided. This is correct and side-effect free.apps/web/src/components/ui/video-player.tsx (3)
50-50: Clip-range short-circuit in preview mode is correctShort-circuiting isInClipRange to true during preview avoids timeline-based pausing. This is the right behavior.
52-56: Correctly guarding timeline event subscriptions with preview modeSkipping timeline window-event wiring in preview mode and re-subscribing when mode toggles (dependency includes isMediaPreviewMode) looks good.
Also applies to: 109-109
179-179: Dynamic preload policy looks goodmetadata in preview mode vs auto in timeline mode is a sensible balance between responsiveness and bandwidth.
| <div | ||
| ref={dragRef} | ||
| className={cn("relative group", containerClassName ?? "w-28 h-28")} | ||
| className="relative group w-28 h-28" | ||
| onDoubleClick={handleDoubleClick} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={`Preview ${name}. Press Enter to preview.`} | ||
| onKeyDown={handleKeyDown} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose interactive semantics only when preview is available
When mediaItem is undefined, the wrapper currently remains focusable with role="button", which violates our guideline “Don't assign tabIndex to non-interactive HTML elements” and misleads assistive tech. Make these attributes conditional on mediaItem.
Apply this diff:
- <div
- ref={dragRef}
- className="relative group w-28 h-28"
- onDoubleClick={handleDoubleClick}
- role="button"
- tabIndex={0}
- aria-label={`Preview ${name}. Press Enter to preview.`}
- onKeyDown={handleKeyDown}
- >
+ <div
+ ref={dragRef}
+ className="relative group w-28 h-28"
+ onDoubleClick={mediaItem ? handleDoubleClick : undefined}
+ role={mediaItem ? "button" : undefined}
+ tabIndex={mediaItem ? 0 : undefined}
+ aria-label={mediaItem ? `Preview ${name}. Press Enter to preview.` : undefined}
+ onKeyDown={mediaItem ? handleKeyDown : undefined}
+ >📝 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.
| <div | |
| ref={dragRef} | |
| className={cn("relative group", containerClassName ?? "w-28 h-28")} | |
| className="relative group w-28 h-28" | |
| onDoubleClick={handleDoubleClick} | |
| role="button" | |
| tabIndex={0} | |
| aria-label={`Preview ${name}. Press Enter to preview.`} | |
| onKeyDown={handleKeyDown} | |
| > | |
| <div | |
| ref={dragRef} | |
| className="relative group w-28 h-28" | |
| onDoubleClick={mediaItem ? handleDoubleClick : undefined} | |
| role={mediaItem ? "button" : undefined} | |
| tabIndex={mediaItem ? 0 : undefined} | |
| aria-label={mediaItem ? `Preview ${name}. Press Enter to preview.` : undefined} | |
| onKeyDown={mediaItem ? handleKeyDown : undefined} | |
| > |
🤖 Prompt for AI Agents
In apps/web/src/components/ui/draggable-item.tsx around lines 119 to 127, the
wrapper element currently always exposes interactive semantics (role="button",
tabIndex, aria-label, onDoubleClick, onKeyDown) even when mediaItem is
undefined; make those attributes and event handlers conditional so they are only
applied when mediaItem is present. Remove or avoid setting
tabIndex/role/aria-label and the keyboard/mouse handlers when mediaItem is falsy
(or alternatively set tabIndex to undefined and omit role/aria-label) so the
element is not focusable or announced as interactive unless a preview exists;
implement this by conditionally spreading an object of interactive props when
mediaItem is truthy.
| <div | ||
| ref={dragRef} | ||
| className="relative group w-full" | ||
| onDoubleClick={handleDoubleClick} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={`Preview ${name}. Press Enter to preview.`} | ||
| onKeyDown={handleKeyDown} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same as above: conditionalize interactive affordances for compact variant
Mirror the conditional semantics in the compact variant to avoid focusable-but-inert UI when mediaItem is absent.
Apply this diff:
- <div
- ref={dragRef}
- className="relative group w-full"
- onDoubleClick={handleDoubleClick}
- role="button"
- tabIndex={0}
- aria-label={`Preview ${name}. Press Enter to preview.`}
- onKeyDown={handleKeyDown}
- >
+ <div
+ ref={dragRef}
+ className="relative group w-full"
+ onDoubleClick={mediaItem ? handleDoubleClick : undefined}
+ role={mediaItem ? "button" : undefined}
+ tabIndex={mediaItem ? 0 : undefined}
+ aria-label={mediaItem ? `Preview ${name}. Press Enter to preview.` : undefined}
+ onKeyDown={mediaItem ? handleKeyDown : undefined}
+ >📝 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.
| <div | |
| ref={dragRef} | |
| className="relative group w-full" | |
| onDoubleClick={handleDoubleClick} | |
| role="button" | |
| tabIndex={0} | |
| aria-label={`Preview ${name}. Press Enter to preview.`} | |
| onKeyDown={handleKeyDown} | |
| > | |
| <div | |
| ref={dragRef} | |
| className="relative group w-full" | |
| onDoubleClick={mediaItem ? handleDoubleClick : undefined} | |
| role={mediaItem ? "button" : undefined} | |
| tabIndex={mediaItem ? 0 : undefined} | |
| aria-label={mediaItem ? `Preview ${name}. Press Enter to preview.` : undefined} | |
| onKeyDown={mediaItem ? handleKeyDown : undefined} | |
| > |
🤖 Prompt for AI Agents
In apps/web/src/components/ui/draggable-item.tsx around lines 164 to 172, the
interactive attributes (role, tabIndex, aria-label, onKeyDown, onDoubleClick)
are applied unconditionally which makes compact items focusable even when
mediaItem is absent; mirror the existing conditional used elsewhere by only
adding these interactive affordances when mediaItem is present (or when variant
!== 'compact' and mediaItem exists) so the compact/inert UI is not
focusable—move or wrap role, tabIndex, aria-label, onKeyDown and onDoubleClick
behind the same mediaItem presence/compact-variant check used elsewhere.
| <Button | ||
| variant="text" | ||
| size="icon" | ||
| onClick={getCurrentToggle()} |
There was a problem hiding this comment.
The play/pause button in fullscreen mode calls the toggle function immediately during render instead of passing it as an event handler.
View Details
Analysis
On line 780, onClick={getCurrentToggle()} is calling the function immediately with parentheses, which executes either toggleMediaPlayback() or toggle() during render and passes the result (undefined) as the onClick handler. This means the play/pause button will not respond to clicks.
The getCurrentToggle function returns the appropriate toggle function based on whether the user is in media preview mode or timeline mode: () => isMediaPreviewMode ? toggleMediaPlayback : toggle. By calling it with parentheses, the returned function is executed immediately instead of being passed as the event handler.
Recommendation
Change onClick={getCurrentToggle()} to onClick={getCurrentToggle} to pass the function reference instead of calling it immediately.
…Name prop with accessibility improvements
ayush18pop
force-pushed
the
feature#548/media-preview-panel
branch
from
1076495 to
1ecc9a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/web/src/components/editor/media-panel/views/media.tsx (1)
532-546: Same as above (list view) — looks goodThe list view mirrors the grid change by passing mediaItem, keeping behavior consistent across views.
apps/web/src/components/editor/preview-panel.tsx (1)
780-786: Fix: don’t call getCurrentToggle during renderonClick={getCurrentToggle()} executes immediately and passes undefined as the handler. Pass the function reference instead.
Apply this diff:
- onClick={getCurrentToggle()} + onClick={getCurrentToggle}
🧹 Nitpick comments (2)
apps/web/src/components/ui/video-player.tsx (2)
123-149: Surface “ended” to the parent in media preview modeCurrently you propagate play/pause/timeupdate but not ended. Hooking ended to call onPause (and optionally resetting external isPlaying if desired) avoids stale “playing” state at media end.
Apply this diff:
// Handle time updates for media preview mode useEffect(() => { const video = videoRef.current; if (!video || !isMediaPreviewMode) return; const handleTimeUpdate = () => { onTimeUpdate?.(video.currentTime); }; const handlePlay = () => { onPlay?.(); }; const handlePause = () => { onPause?.(); }; + const handleEnded = () => { + // Treat ended equivalently to paused for external state + onPause?.(); + }; - video.addEventListener('timeupdate', handleTimeUpdate); - video.addEventListener('play', handlePlay); - video.addEventListener('pause', handlePause); + video.addEventListener("timeupdate", handleTimeUpdate); + video.addEventListener("play", handlePlay); + video.addEventListener("pause", handlePause); + video.addEventListener("ended", handleEnded); return () => { - video.removeEventListener('timeupdate', handleTimeUpdate); - video.removeEventListener('play', handlePlay); - video.removeEventListener('pause', handlePause); + video.removeEventListener("timeupdate", handleTimeUpdate); + video.removeEventListener("play", handlePlay); + video.removeEventListener("pause", handlePause); + video.removeEventListener("ended", handleEnded); }; }, [isMediaPreviewMode, onTimeUpdate, onPlay, onPause]);
173-186: Consider adding captions support when availablePer the project’s accessibility guidelines (“Include caption tracks for audio and video elements”), consider passing an optional track list (e.g., WebVTT) and rendering children when present. This can be a no-op when no captions exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/web/src/components/editor/media-panel/views/media.tsx(2 hunks)apps/web/src/components/editor/preview-panel.tsx(20 hunks)apps/web/src/components/ui/audio-player.tsx(3 hunks)apps/web/src/components/ui/draggable-item.tsx(6 hunks)apps/web/src/components/ui/video-player.tsx(5 hunks)apps/web/src/hooks/use-editor-actions.ts(2 hunks)apps/web/src/stores/media-preview-store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/hooks/use-editor-actions.ts
- apps/web/src/stores/media-preview-store.ts
- apps/web/src/components/ui/audio-player.tsx
- apps/web/src/components/ui/draggable-item.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{jsx,tsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include alangattribute on the html element.
Always include atitleattribute for iframe elements.
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress.
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur.
Include caption tracks for audio and video elements.
Use semantic elements instead of role attributes in JSX.
Make sure all anchors are valid and navigable.
Ensure all ARIA properties (aria-*) are valid.
Use valid, non-abstract ARIA roles for elements with...
Files:
apps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't use the TypeScript directive @ts-ignore.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Use either T[] or Array consistently
Don't use the any type
Files:
apps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use the return value of React.render.
Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use primitive type aliases or misleading types.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of...
Files:
apps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{tsx,jsx}: Always include a title element for icons unless there's text beside the icon
Always include a type attribute for button elements
Accompany onClick with at least one of: onKeyUp, onKeyDown, or onKeyPress
Accompany onMouseOver/onMouseOut with onFocus/onBlur
Don't import React itself
Don't define React components inside other components
Don't use both children and dangerouslySetInnerHTML on the same element
Don't insert comments as text nodes
Use <>...</> instead of ...
Files:
apps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use the comma operator
Use for...of statements instead of Array.forEach
Don't initialize variables to undefined
Use .flatMap() instead of map().flat() when possible
Don't assign a value to itself
Avoid unused imports and variables
Don't use await inside loops
Don't hardcode sensitive data like API keys and tokens
Don't use the delete operator
Don't use global eval()
Use String.slice() instead of String.substr() and String.substring()
Don't use else blocks when the if block breaks early
Put default function parameters and optional function parameters last
Use new when throwing an error
Use String.trimStart() and String.trimEnd() over String.trimLeft() and String.trimRight()
Files:
apps/web/src/components/editor/media-panel/views/media.tsxapps/web/src/components/ui/video-player.tsxapps/web/src/components/editor/preview-panel.tsx
🧬 Code Graph Analysis (2)
apps/web/src/components/ui/video-player.tsx (1)
apps/web/src/stores/playback-store.ts (1)
usePlaybackStore(75-174)
apps/web/src/components/editor/preview-panel.tsx (6)
apps/web/src/stores/media-preview-store.ts (1)
useMediaPreviewStore(21-82)apps/web/src/components/ui/video-player.tsx (1)
VideoPlayer(21-187)apps/web/src/components/ui/audio-player.tsx (1)
AudioPlayer(21-183)apps/web/src/stores/project-store.ts (1)
DEFAULT_FPS(11-11)apps/web/src/components/ui/editable-timecode.tsx (1)
EditableTimecode(18-138)apps/web/src/lib/time.ts (1)
formatTimeCode(8-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vade Review
🔇 Additional comments (3)
apps/web/src/components/editor/media-panel/views/media.tsx (1)
491-506: Passing full MediaItem to DraggableMediaItem (grid) looks correctWiring the full mediaItem prop enables the double-click-to-preview flow without re-querying by id. This aligns with the updated DraggableMediaItem API.
apps/web/src/components/ui/video-player.tsx (1)
39-47: Clean dual-mode detection and state sourcingUsing the presence of isPlaying to infer media preview mode is simple and effective, and keeps timeline vs. preview responsibilities cleanly separated. LGTM.
apps/web/src/components/editor/preview-panel.tsx (1)
499-514: Video media-preview wiring is solidYou correctly pass mediaCurrentTime, isPlaying, and the media callbacks to VideoPlayer, with trimStart/trimEnd at 0 for full-length preview. Matches the intended behavior.
| if(previewMedia.type === "image" && previewMedia.url){ | ||
| return ( | ||
| <div className="absolute inset-0 flex items-center justify-center"> | ||
| <img | ||
| src={previewMedia.url} | ||
| alt={previewMedia.name} | ||
| className="max-w-full max-h-full object-contain" | ||
| draggable={false} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace with Next.js
in media preview
This project uses Next.js (“use client”), and guidelines prohibit using . In Line 520-526, swap to next/image to benefit from optimization and satisfy the rule.
Apply this diff:
- <div className="absolute inset-0 flex items-center justify-center">
- <img
- src={previewMedia.url}
- alt={previewMedia.name}
- className="max-w-full max-h-full object-contain"
- draggable={false}
- />
- </div>
+ <div className="absolute inset-0">
+ <Image
+ src={previewMedia.url}
+ alt={previewMedia.name}
+ fill
+ // object-contain keeps the whole image visible within the preview box
+ className="object-contain"
+ draggable={false}
+ priority
+ />
+ </div>Also add the import at the top of this file:
import Image from "next/image";Note: The blur-background image path elsewhere in this file still uses . Consider applying the same change there for consistency and compliance.
🤖 Prompt for AI Agents
In apps/web/src/components/editor/preview-panel.tsx around lines 517 to 526,
replace the plain <img> used for image preview with Next.js's Image component:
add the import `import Image from "next/image";` at the top of the file, then
swap the <img> element for <Image> passing the same src and alt values, apply
the existing classes (or use className plus style to preserve object-contain and
max sizing), and keep draggable={false}; use either fill layout (with parent
position set) or explicit width/height consistent with the container so the
image scales like the original; also update the other blur-background <img> in
this file similarly for consistency and compliance with Next.js rules.
| <div className="flex-1 flex items-center gap-2"> | ||
| <div | ||
| className={cn( | ||
| "absolute top-0 left-0 h-full rounded-full bg-foreground", | ||
| !isDragging && "duration-100" | ||
| "relative h-1 rounded-full cursor-pointer flex-1 bg-foreground/20", | ||
| (!hasAnyElements && !isMediaPreviewMode) && "opacity-50 cursor-not-allowed" | ||
| )} | ||
| style={{ width: `${progress}%` }} | ||
| /> | ||
| <div | ||
| className="absolute top-1/2 w-3 h-3 rounded-full -translate-y-1/2 -translate-x-1/2 shadow-xs bg-foreground border border-black/20" | ||
| style={{ left: `${progress}%` }} | ||
| /> | ||
| onClick={(hasAnyElements || isMediaPreviewMode) ? handleTimelineClick : undefined} | ||
| onMouseDown={(hasAnyElements || isMediaPreviewMode) ? handleTimelineDrag : undefined} | ||
| style={{ userSelect: "none" }} | ||
| > | ||
| <div | ||
| className={cn( | ||
| "absolute top-0 left-0 h-full rounded-full bg-foreground", | ||
| !isDragging && "duration-100" | ||
| )} | ||
| style={{ width: `${progress}%` }} | ||
| /> | ||
| <div | ||
| className="absolute top-1/2 w-3 h-3 rounded-full -translate-y-1/2 -translate-x-1/2 shadow-xs bg-foreground border border-black/20" | ||
| style={{ left: `${progress}%` }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accessibility: avoid click handlers on non-interactive elements; make the scrubber keyboard-accessible
Per guidelines, don’t attach event handlers to non-interactive elements and accompany onClick with keyboard handlers. Convert the progress container from a div to a button, preserve styling, and add onKeyDown for Left/Right keys.
Apply this diff:
- <div
+ <button
className={cn(
"relative h-1 rounded-full cursor-pointer flex-1 bg-foreground/20",
(!hasAnyElements && !isMediaPreviewMode) && "opacity-50 cursor-not-allowed"
)}
onClick={(hasAnyElements || isMediaPreviewMode) ? handleTimelineClick : undefined}
onMouseDown={(hasAnyElements || isMediaPreviewMode) ? handleTimelineDrag : undefined}
- style={{ userSelect: "none" }}
+ style={{ userSelect: "none" }}
+ type="button"
+ aria-label="Scrub preview timeline"
+ onKeyDown={(e) => {
+ if (!hasAnyElements && !isMediaPreviewMode) return;
+ if (e.key === "ArrowLeft" || e.key === "ArrowRight") {
+ e.preventDefault();
+ const delta = e.key === "ArrowLeft" ? -1 : 1; // 1s step
+ const newTime = Math.max(0, Math.min(getCurrentTime() + delta, totalDuration));
+ handleTimeChange(newTime);
+ } else if (e.key === "Home") {
+ e.preventDefault();
+ handleTimeChange(0);
+ } else if (e.key === "End") {
+ e.preventDefault();
+ handleTimeChange(totalDuration);
+ }
+ }}
>
<div
className={cn(
"absolute top-0 left-0 h-full rounded-full bg-foreground",
!isDragging && "duration-100"
)}
style={{ width: `${progress}%` }}
/>
<div
className="absolute top-1/2 w-3 h-3 rounded-full -translate-y-1/2 -translate-x-1/2 shadow-xs bg-foreground border border-black/20"
style={{ left: `${progress}%` }}
/>
- </div>
+ </button>📝 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.
| <div className="flex-1 flex items-center gap-2"> | |
| <div | |
| className={cn( | |
| "absolute top-0 left-0 h-full rounded-full bg-foreground", | |
| !isDragging && "duration-100" | |
| "relative h-1 rounded-full cursor-pointer flex-1 bg-foreground/20", | |
| (!hasAnyElements && !isMediaPreviewMode) && "opacity-50 cursor-not-allowed" | |
| )} | |
| style={{ width: `${progress}%` }} | |
| /> | |
| <div | |
| className="absolute top-1/2 w-3 h-3 rounded-full -translate-y-1/2 -translate-x-1/2 shadow-xs bg-foreground border border-black/20" | |
| style={{ left: `${progress}%` }} | |
| /> | |
| onClick={(hasAnyElements || isMediaPreviewMode) ? handleTimelineClick : undefined} | |
| onMouseDown={(hasAnyElements || isMediaPreviewMode) ? handleTimelineDrag : undefined} | |
| style={{ userSelect: "none" }} | |
| > | |
| <div | |
| className={cn( | |
| "absolute top-0 left-0 h-full rounded-full bg-foreground", | |
| !isDragging && "duration-100" | |
| )} | |
| style={{ width: `${progress}%` }} | |
| /> | |
| <div | |
| className="absolute top-1/2 w-3 h-3 rounded-full -translate-y-1/2 -translate-x-1/2 shadow-xs bg-foreground border border-black/20" | |
| style={{ left: `${progress}%` }} | |
| /> | |
| </div> | |
| <div className="flex-1 flex items-center gap-2"> | |
| <button | |
| className={cn( | |
| "relative h-1 rounded-full cursor-pointer flex-1 bg-foreground/20", | |
| (!hasAnyElements && !isMediaPreviewMode) && "opacity-50 cursor-not-allowed" | |
| )} | |
| onClick={(hasAnyElements || isMediaPreviewMode) ? handleTimelineClick : undefined} | |
| onMouseDown={(hasAnyElements || isMediaPreviewMode) ? handleTimelineDrag : undefined} | |
| style={{ userSelect: "none" }} | |
| type="button" | |
| aria-label="Scrub preview timeline" | |
| onKeyDown={(e) => { | |
| if (!hasAnyElements && !isMediaPreviewMode) return; | |
| if (e.key === "ArrowLeft" || e.key === "ArrowRight") { | |
| e.preventDefault(); | |
| const delta = e.key === "ArrowLeft" ? -1 : 1; // 1s step | |
| const newTime = Math.max(0, Math.min(getCurrentTime() + delta, totalDuration)); | |
| handleTimeChange(newTime); | |
| } else if (e.key === "Home") { | |
| e.preventDefault(); | |
| handleTimeChange(0); | |
| } else if (e.key === "End") { | |
| e.preventDefault(); | |
| handleTimeChange(totalDuration); | |
| } | |
| }} | |
| > | |
| <div | |
| className={cn( | |
| "absolute top-0 left-0 h-full rounded-full bg-foreground", | |
| !isDragging && "duration-100" | |
| )} | |
| style={{ width: `${progress}%` }} | |
| /> | |
| <div | |
| className="absolute top-1/2 w-3 h-3 rounded-full -translate-y-1/2 -translate-x-1/2 shadow-xs bg-foreground border border-black/20" | |
| style={{ left: `${progress}%` }} | |
| /> | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In apps/web/src/components/editor/preview-panel.tsx around lines 806 to 827, the
progress scrubber is a non-interactive div with click/mouse handlers which
breaks accessibility; replace that div with a semantic button (type="button")
that preserves the current className/styling and disabled state when
(!hasAnyElements && !isMediaPreviewMode) so it is skipped by keyboard when
inactive, add onKeyDown to handle ArrowLeft/ArrowRight (and Home/End if desired)
calling the same logic as handleTimelineClick/handleTimelineDrag (or dedicated
functions to step backward/forward), keep the existing onClick and onMouseDown
handlers wired to the button, and ensure appropriate aria-label (e.g., "Timeline
scrubber") and aria-disabled when disabled to make the scrubber
keyboard-accessible and screen-reader friendly.
|
@mazeincoding, check this out! |
You should remove the comments made by your AI first |
|
Yeah sure, but any implementation related changes?
|
As a video editor, I personally prefer a small note saying you're viewing your media or just no note (just being able to preview your media). You should be able to just deselect what you have selected instead of switching between viewing your timeline items and your media. It's your choice though, maybe you could wait for Maze to check. |
|
@ayush18pop i think it would look good if it looks something like this, although it is a very subjective opinion, once check this how they done in clipchamp , it looks simple and very smooth preview-opencut.mp4@mazeincoding @enkeii64 |
|
Yeahh I think it's easier to implement this than what I did
|
3 participants
Description
Implements a media preview panel that allows users to preview individual media items (video, audio, images) outside the timeline context. Users can double-click any media item to enter preview mode and toggle between timeline and media preview modes seamlessly.
Key Features:
Fixes #548
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Manual Testing Performed:
Test Configuration:
Screenshots (if applicable)
Media Preview Mode Toggle
The toggle buttons allow switching between Timeline and Media preview modes
Normal Preview Panel in Media Mode
Double clicking any media file triggers preview-panel to convert into media preview mode
Fullscreen Media Preview with Improved Layout
Media preview mode also works in fullscreen with redesigned toolbar layout
Checklist:
Additional context
Technical Implementation:
media-preview-store.ts- Dedicated Zustand store for media preview state managementpreview-panel.tsxto handle dual modes with conditional renderingArchitecture Benefits:
This feature enhances the user experience by allowing quick preview of media items without affecting the timeline workflow. The implementation maintains clean code architecture and follows the project's existing patterns.
Files Changed:
apps/web/src/stores/media-preview-store.ts- New Zustand store for media preview stateapps/web/src/components/editor/preview-panel.tsx- Updated for dual mode supportapps/web/src/components/ui/video-player.tsx- Added media preview mode supportapps/web/src/components/ui/audio-player.tsx- Added media preview mode supportapps/web/src/components/ui/draggable-item.tsx- Added double-click to previewapps/web/src/hooks/use-editor-actions.ts- Updated playback actions for preview modeapps/web/src/components/editor/media-panel/views/media.tsx- Integration with preview functionalitySummary by CodeRabbit
New Features
Refactor
Accessibility