feat: add video effects system and interactive element manipulation #582
Conversation
- Add video effects with CSS filters (brightness, contrast, saturation, etc.) - Implement effects panel with preset categories and parameter controls - Add effects timeline visualization and properties panel - Enable drag, drop, resize, and rotate for text elements in preview - Integrate effects with canvas rendering for preview and export - Add effects store with preset management and element-specific effects - Support effects application to media elements with timeline integration
|
@hovhannisians 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 complete Effects system: new types, store, utilities, UI views, properties editor, timeline overlays, and rendering pipeline integration. Introduces element transform fields and updateElement to the timeline store. Wires effects into PreviewPanel, Fullscreen rendering, export, and VideoPlayer. Numerous formatting-only edits elsewhere. Removes CONTRIBUTING.md. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MediaPanel as Media Panel (EffectsView)
participant EffectsStore as useEffectsStore
participant TimelineStore as useTimelineStore
participant Playback as usePlaybackStore
User->>MediaPanel: Drag preset / Click "Add"
MediaPanel->>EffectsStore: addEffectToTimeline(preset)
EffectsStore->>Playback: get currentTime
EffectsStore->>TimelineStore: find media track/element at time
alt element found
EffectsStore->>EffectsStore: applyEffectToElement(elementId, trackId, preset)
EffectsStore-->>User: Toast "Effect added"
else none found
EffectsStore-->>User: Toast error
end
sequenceDiagram
autonumber
participant Preview as PreviewPanel
participant EffectsStore as useEffectsStore
participant Renderer as renderTimelineFrame
participant Canvas as Canvas 2D Context
Preview->>EffectsStore: getEffectsForElement(elementId)
Preview->>Renderer: renderTimelineFrame({ getEffectsForElement })
Renderer->>EffectsStore: getEffectsForElement(elementId)
alt enabled effects exist
Renderer->>Canvas: ctx.filter = parametersToCSSFilters(merged)
Renderer->>Canvas: draw video frame
Renderer->>Canvas: ctx.filter = "none"
else no effects
Renderer->>Canvas: draw video frame (no filter)
end
sequenceDiagram
autonumber
participant VideoPlayer as VideoPlayer(video, elementId)
participant EffectsStore as useEffectsStore
participant Utils as effects-utils
VideoPlayer->>EffectsStore: getEffectsForElement(elementId)
alt has enabled effects
VideoPlayer->>Utils: parametersToCSSFilters / applyEffectsToVideo(video, params)
else none
VideoPlayer->>Utils: removeEffectsFromVideo(video)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 44
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
apps/web/src/components/editor/media-panel/views/sounds.tsx (4)
450-453: Fix a11y: onClick on non-interactive div lacks role/keyboard support.
Per guidelines, make static elements with click handlers operable via keyboard.Apply:
const handleClick = () => { onPlay(); }; + + const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + onPlay(); + } + };return ( <div className="group flex items-center gap-3 opacity-100 hover:opacity-75 transition-opacity cursor-pointer" onClick={handleClick} + role="button" + tabIndex={0} + onKeyDown={handleKeyDown} >Also applies to: 435-447
481-493: Treat heart as a toggle button; add type, aria-pressed, accessible label.
Improves screen reader clarity and satisfies button/type rules.- <Button + <Button variant="text" size="icon" className={`hover:text-foreground !opacity-100 w-auto ${ isSaved ? "text-red-500 hover:text-red-600" : "text-muted-foreground" }`} onClick={handleSaveClick} - title={isSaved ? "Remove from saved" : "Save sound"} + title={isSaved ? "Remove from saved" : "Save sound"} + type="button" + aria-pressed={isSaved} + aria-label={isSaved ? "Remove from saved" : "Save sound"} > - <HeartIcon className={`w-4 h-4 ${isSaved ? "fill-current" : ""}`} /> + <HeartIcon + className={`w-4 h-4 ${isSaved ? "fill-current" : ""}`} + aria-hidden="true" + /> + <span className="sr-only"> + {isSaved ? "Remove from saved" : "Save sound"} + </span> </Button>
139-153: Clean up Audio() object and listeners to prevent leaks.
Create/remove listeners and pause audio on unmount or when a new sound plays.Add a cleanup effect after the audio state:
const [audioElement, setAudioElement] = useState<HTMLAudioElement | null>( null ); + + useEffect(() => { + return () => { + if (audioElement) { + audioElement.pause(); + // Best-effort cleanup for browsers that support it + audioElement.src = ""; + } + }; + }, [audioElement]);And when attaching listeners, keep refs to remove if needed (optional).
Also applies to: 94-99
271-285: Apply the same Audio() cleanup pattern in SavedSoundsView.
Mirror the cleanup to avoid retained listeners and playing audio across tab switches.Add after the audio state:
const [audioElement, setAudioElement] = useState<HTMLAudioElement | null>( null ); + + useEffect(() => { + return () => { + if (audioElement) { + audioElement.pause(); + audioElement.src = ""; + } + }; + }, [audioElement]);Also applies to: 248-252
apps/web/src/components/ui/input.tsx (3)
37-50: Fix boolean coercion bug causing NaN and lost padding for icons.showPasswordToggle becomes a function reference, so Number(showPasswordToggle) yields NaN; paddingRight won’t be set.
- const showPasswordToggle = isPassword && onShowPasswordChange; + const showPasswordToggle = isPassword && Boolean(onShowPasswordChange); @@ - const iconCount = Number(showPasswordToggle) + Number(showClear); + const iconCount = (showPasswordToggle ? 1 : 0) + (showClear ? 1 : 0);
76-90: Keyboard/touch activation doesn’t clear because handler is onMouseDown only.Button lacks onClick; keyboard “click” won’t invoke onClear. Keep onMouseDown to prevent blur, but add onClick.
{showClear && ( <Button type="button" variant="text" size="icon" onMouseDown={(e) => { e.preventDefault(); onClear?.(); }} + onClick={(e) => { + e.preventDefault(); + onClear?.(); + }} className="absolute right-0 top-0 h-full px-3 text-muted-foreground !opacity-100" aria-label="Clear input" >
92-109: Add keyboard handler alongside onClick to satisfy guidelines.Mirror toggle on Enter/Space for explicit a11y compliance.
<Button type="button" variant="text" size="icon" onClick={() => onShowPasswordChange?.(!showPassword)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + onShowPasswordChange?.(!showPassword); + } + }} className={cn( "absolute top-0 h-full px-3 text-muted-foreground hover:text-foreground", showClear ? "right-10" : "right-0" )} aria-label={showPassword ? "Hide password" : "Show password"} >apps/web/src/components/footer.tsx (1)
53-61: Add accessible names for icon-only linksAnchors with only SVG icons lack an accessible name. Provide aria-labels on the links and mark icons decorative to satisfy “Make sure anchors have content that's accessible to screen readers” and the icon a11y rules.
- <Link + <Link href="https://github.com/OpenCut-app/OpenCut" className="text-muted-foreground hover:text-foreground transition-colors" target="_blank" rel="noopener noreferrer" + aria-label="OpenCut on GitHub" > - <FaGithub className="h-5 w-5" /> + <FaGithub className="h-5 w-5" aria-hidden="true" focusable="false" /> </Link>- <Link + <Link href="https://x.com/OpenCutApp" className="text-muted-foreground hover:text-foreground transition-colors" target="_blank" rel="noopener noreferrer" + aria-label="OpenCut on X" > - <RiTwitterXLine className="h-5 w-5" /> + <RiTwitterXLine className="h-5 w-5" aria-hidden="true" focusable="false" /> </Link>- <Link + <Link href="https://discord.com/invite/Mu3acKZvCp" className="text-muted-foreground hover:text-foreground transition-colors" target="_blank" rel="noopener noreferrer" + aria-label="Join the OpenCut Discord" > - <RiDiscordFill className="h-5 w-5" /> + <RiDiscordFill className="h-5 w-5" aria-hidden="true" focusable="false" /> </Link>Also applies to: 62-69, 70-77
apps/web/src/components/editor/timeline/timeline-track.tsx (1)
927-935: Bug: videos/images can be dropped onto audio tracks.
isCompatibleusescanElementGoOnTrack("media", ...)for both audio and video, which allows video/image on audio tracks. Tighten the check.- const isCompatible = isVideoOrImage - ? canElementGoOnTrack("media", track.type) - : isAudio - ? canElementGoOnTrack("media", track.type) - : false; + const isCompatible = isVideoOrImage + ? track.type === "media" + : isAudio + ? track.type === "audio" + : false;If you prefer central rules, we can also adjust
canElementGoOnTrackto distinguish audio vs non-audio elements.apps/web/src/lib/export.ts (1)
136-145: Clamp mixed samples to [-1, 1] to prevent clipping/distortionSumming channels without limiting can overflow the float range expected by WebAudio and audibly clip.
- outputData[outputIndex] += sourceData[sourceIndex]; + const mixed = outputData[outputIndex] + sourceData[sourceIndex]; + // hard-clip; consider soft limiting later + outputData[outputIndex] = Math.max(-1, Math.min(1, mixed));
🧹 Nitpick comments (73)
apps/web/src/components/ui/font-picker.tsx (1)
23-25: Prefer cn() to merge class namesUsing cn() avoids empty strings and keeps class merging consistent with the codebase.
- <SelectTrigger - type="button" - className={`w-full bg-panel-accent h-8 text-xs ${className || ""}`} - > + <SelectTrigger + type="button" + className={cn("w-full bg-panel-accent h-8 text-xs", className)} + >Add import:
+import { cn } from "@/lib/utils";apps/web/src/data/colors/syntax-ui.tsx (3)
5-7: De-duplicate repeated gradient entriesTwo gradients are duplicated (Lines 5 and 7; Lines 14 and 15). If intentional for weighting, ignore; otherwise remove to avoid confusion.
Apply this diff:
export const syntaxUIGradients = [ // Cyan to Blue gradients "linear-gradient(to right, #22d3ee, #0ea5e9, #0284c7)", "linear-gradient(to right, #bfdbfe, #a5f3fc)", - "linear-gradient(to right, #22d3ee, #0ea5e9, #0284c7)", + // (duplicate removed) // Purple gradients "linear-gradient(to right, #e9d5ff, #d8b4fe, #c084fc)", "linear-gradient(to right, #c4b5fd, #a78bfa, #8b5cf6)", // Blue gradients "linear-gradient(to right, #93c5fd, #60a5fa, #3b82f6)", - "linear-gradient(to right, #93c5fd, #60a5fa, #3b82f6)", + // (duplicate removed)Also applies to: 14-15
3-32: Prefer const assertion for stronger typing and immutabilityAdd a const assertion so consumers get a string-literal union and the array is read-only.
Apply this diff:
-export const syntaxUIGradients = [ +export const syntaxUIGradients = [ // Cyan to Blue gradients "linear-gradient(to right, #22d3ee, #0ea5e9, #0284c7)", "linear-gradient(to right, #bfdbfe, #a5f3fc)", // Purple gradients "linear-gradient(to right, #e9d5ff, #d8b4fe, #c084fc)", "linear-gradient(to right, #c4b5fd, #a78bfa, #8b5cf6)", // Blue gradients "linear-gradient(to right, #93c5fd, #60a5fa, #3b82f6)", // Green gradients "linear-gradient(to right, #6ee7b7, #34d399, #10b981)", "linear-gradient(to right, #d1fae5, #a7f3d0, #6ee7b7)", // Red gradient "linear-gradient(to right, #fca5a5, #f87171, #ef4444)", // Yellow/Orange gradient "linear-gradient(to right, #fde68a, #fbbf24, #f59e0b)", // Pink gradient "linear-gradient(to right, #fbcfe8, #f9a8d4, #f472b6)", // Neon radial gradient "radial-gradient(circle at bottom left, #ff00ff, #00ffff)", -]; + ] as const;
1-1: Nit: use .ts extension since there’s no JSXThe file is TSX but contains no JSX. Consider renaming to .ts to align with tooling expectations.
apps/web/src/lib/zk-encryption.ts (6)
6-10: Preferexport typeoverinterfacefor type-only exportsOur TS guidelines favor
export typefor types. No behavioral change.-export interface ZeroKnowledgeEncryptionResult { - encryptedData: ArrayBuffer; - key: ArrayBuffer; - iv: ArrayBuffer; -} +export type ZeroKnowledgeEncryptionResult = { + encryptedData: ArrayBuffer; + key: ArrayBuffer; + iv: ArrayBuffer; +};
25-31: Be explicit about AES-GCM tag lengthWebCrypto defaults to 128-bit tags, but making it explicit improves clarity and portability.
- const encryptedResult = await crypto.subtle.encrypt( - { name: "AES-GCM", iv }, + const encryptedResult = await crypto.subtle.encrypt( + { name: "AES-GCM", iv, tagLength: 128 }, cryptoKey, data );Also applies to: 33-38
40-46: Clarify/comment accuracy about auth tagThe result already includes the auth tag; we’re not appending anything ourselves. Consider tightening the comment to avoid confusion.
- // For AES-GCM, we need to append the authentication tag - // The encrypted result contains both ciphertext and tag + // AES-GCM output from WebCrypto includes both ciphertext and authentication tag
64-71: Optional: add Node-safe fallback for base64 decode (if used outside browser)If these helpers are ever used in Node (tests/SSR),
atobisn’t guaranteed. A guarded fallback prevents env issues without impacting browsers.export function base64ToArrayBuffer(base64: string): ArrayBuffer { - const binary = atob(base64); - const bytes = new Uint8Array(binary.length); - for (let i = 0; i < binary.length; i++) { - bytes[i] = binary.charCodeAt(i); - } - return bytes.buffer; + if (typeof atob === "function") { + const binary = atob(base64); + const bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i++) { + bytes[i] = binary.charCodeAt(i); + } + return bytes.buffer; + } + // Node/SSR fallback + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore Buffer is Node-only; guarded by runtime check + if (typeof Buffer !== "undefined") { + // Create a tightly-sized ArrayBuffer + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const buf = Buffer.from(base64, "base64"); + return buf.buffer.slice(buf.byteOffset, buf.byteOffset + buf.byteLength); + } + throw new Error("base64 decoding not supported in this environment"); }
15-47: Consider returning a serialized (base64) key/iv variant or adding decrypt helperCallers often need a transport-friendly representation and a matching
decryptWithKeyto round-trip. Not blocking this PR, but adding either (or examples) will reduce misuse risk (e.g., mutating the returned buffers).Happy to draft a decrypt helper and an example showing encode/decode flows.
52-59: Chunk string concatenation in arrayBufferToBase64 for linear performanceRefactor arrayBufferToBase64 to build 32 KB chunks and join them instead of char-by-char concatenation to avoid O(n²) behavior and stack/memory spikes:
-export function arrayBufferToBase64(buffer: ArrayBuffer): string { - const bytes = new Uint8Array(buffer); - let binary = ""; - for (let i = 0; i < bytes.byteLength; i++) { - binary += String.fromCharCode(bytes[i]); - } - return btoa(binary); -} +export function arrayBufferToBase64(buffer: ArrayBuffer): string { + const bytes = new Uint8Array(buffer); + const chunkSize = 0x8000; // 32KB blocks to avoid stack/memory spikes + const chunks: string[] = []; + for (let i = 0; i < bytes.length; i += chunkSize) { + chunks.push(String.fromCharCode(...bytes.subarray(i, i + chunkSize))); + } + return btoa(chunks.join("")); +}No server-side or SSR imports detected—
btoa/atobusage is confined to client-side code.apps/web/src/components/editor/media-panel/views/sounds.tsx (3)
471-479: Add type and visible/sr-only label for “Add to timeline” control.
Meets the “type attribute” and icon labeling rules.- <Button + <Button variant="text" size="icon" className="text-muted-foreground hover:text-foreground !opacity-100 w-auto" onClick={handleAddToTimeline} title="Add to timeline" + type="button" + aria-label="Add to timeline" > - <PlusIcon className="w-4 h-4" /> + <PlusIcon className="w-4 h-4" aria-hidden="true" /> + <span className="sr-only">Add to timeline</span> </Button>
359-365: Explicit button type in dialog actions.
Avoid implicit submit behavior and meet guideline.- <Button + <Button variant="text" size="sm" className="h-auto text-muted-foreground hover:text-destructive !opacity-100" + type="button" >- <Button variant="text" onClick={() => setShowClearDialog(false)}> + <Button variant="text" onClick={() => setShowClearDialog(false)} type="button">- <Button + <Button variant="destructive" onClick={async () => { await clearSavedSounds(); setShowClearDialog(false); }} + type="button" >Also applies to: 375-377, 379-387
106-116: Stabilize useEffect deps or document intent.
loadSavedSounds,scrollAreaRef, andscrollPositionare referenced with an empty deps array. Either include them or annotate intentionally to avoid stale closures.Option A (include deps and guard scrolling):
- useEffect(() => { + useEffect(() => { loadSavedSounds(); - if (scrollAreaRef.current && scrollPosition > 0) { const timeoutId = setTimeout(() => { scrollAreaRef.current?.scrollTo({ top: scrollPosition }); }, 100); return () => clearTimeout(timeoutId); } - }, []); + }, [loadSavedSounds, scrollAreaRef, scrollPosition]);Option B (keep on-mount semantics but document):
// eslint-disable-next-line react-hooks/exhaustive-depsapps/web/src/app/api/transcribe/route.ts (3)
55-58: Harden rate-limit key derivation; don’t trust raw X-Forwarded-For.Use
request.ipwhen available; otherwise extract the first IP from XFF. Also remove the unusedorigin.- const ip = request.headers.get("x-forwarded-for") ?? "anonymous"; - const { success } = await baseRateLimit.limit(ip); - const origin = request.headers.get("origin"); + const ip = + request.ip ?? + request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? + "unknown"; + const { success } = await baseRateLimit.limit(ip);
71-78: Don’t expose internal config details in client errors.Returning missing env var names to clients increases information disclosure. Keep details in server logs; return a generic message.
- return NextResponse.json( - { - error: "Transcription not configured", - message: `Auto-captions require environment variables: ${transcriptionCheck.missingVars.join(", ")}. Check README for setup instructions.`, - }, - { status: 503 } - ); + return NextResponse.json( + { + error: "Transcription not configured", + message: "Auto-captions are temporarily unavailable. Please try again later.", + }, + { status: 503 } + );Also applies to: 73-75
14-31: DRY up response schemas.
modalResponseSchemaandapiResponseSchemaduplicate the same shape. Define a single base schema and reuse it to reduce drift.Example:
const transcriptSchema = z.object({ /* shared fields */ }); const modalResponseSchema = transcriptSchema; const apiResponseSchema = transcriptSchema;Also applies to: 33-50
apps/web/src/hooks/use-highlight-scroll.ts (2)
11-17: Stabilize ref-callback to avoid redundant mount/unmount cycles.Wrap registerElement in useCallback so React doesn’t invoke it twice when its identity changes; helps when used as a ref callback.
-import { useEffect, useState, useRef } from "react"; +import { useEffect, useState, useRef, useCallback } from "react"; @@ - const registerElement = (id: string, element: HTMLElement | null) => { + const registerElement = useCallback((id: string, element: HTMLElement | null) => { if (element) { elementRefs.current.set(id, element); } else { elementRefs.current.delete(id); } - }; + }, []);
24-26: Optional: smoother UX when auto-scrolling to highlights.Consider smooth scroll to reduce jumpiness.
- target?.scrollIntoView({ block: "center" }); + target?.scrollIntoView({ block: "center", behavior: "smooth" });apps/web/src/hooks/use-edge-auto-scroll.ts (1)
72-79: Avoid redundant DOM writes.Skip assignments when scrollLeft wouldn’t change; reduces layout work in hot path.
- if (scrollSpeed !== 0) { - const newScrollLeft = Math.max( + if (scrollSpeed !== 0) { + const newScrollLeft = Math.max( 0, Math.min(scrollMax, rulerViewport.scrollLeft + scrollSpeed) ); - rulerViewport.scrollLeft = newScrollLeft; - tracksViewport.scrollLeft = newScrollLeft; + if (newScrollLeft !== rulerViewport.scrollLeft) { + rulerViewport.scrollLeft = newScrollLeft; + tracksViewport.scrollLeft = newScrollLeft; + } }apps/web/src/types/media.ts (1)
1-1: Optional: derive MediaType from an as-const source of truth.
Aligns with guideline to prefer as const over literal unions and centralizes allowed values.Apply:
-export type MediaType = "image" | "video" | "audio"; +export const MEDIA_TYPES = ["image", "video", "audio"] as const; +export type MediaType = (typeof MEDIA_TYPES)[number];apps/web/src/hooks/use-sound-search.ts (1)
96-96: Debounce races: add AbortController to cancel in-flight requests.
Prevents late responses from stale searches overwriting current state.Apply:
let ignore = false; - const timeoutId = setTimeout(async () => { + const timeoutId = setTimeout(async () => { + const ac = new AbortController(); try { setSearching(true); setSearchError(null); resetPagination(); - const response = await fetch( - `/api/sounds/search?q=${encodeURIComponent(query)}&type=effects&page=1` - ); + const response = await fetch( + `/api/sounds/search?q=${encodeURIComponent( + query + )}&type=effects&page=1&commercial_only=${commercialOnly}`, + { signal: ac.signal } + ); ... return () => { clearTimeout(timeoutId); ignore = true; + // Cancel any in-flight request for the previous query + try { + ac.abort(); + } catch {} };Also applies to: 102-105, 129-133
apps/web/src/stores/playback-store.ts (1)
40-42: Avoid console usage per guidelines.
Replace console.error with a non-console mechanism or remove the log; fallback behavior already handles unset FPS.Apply:
- if (!projectFps) - console.error( - "Project FPS is not set, assuming " + DEFAULT_FPS + "fps" - ); + // If FPS is not set, fall back to DEFAULT_FPS without logging to console.apps/web/src/app/api/get-upload-url/route.ts (3)
71-90: Bind expected Content-Type in the presigned requestIncluding Content-Type in the signature constrains uploads to the intended media type derived from the validated extension.
Apply:
- // Initialize R2 client + // Initialize R2 client const client = new AwsClient({ accessKeyId: env.R2_ACCESS_KEY_ID, secretAccessKey: env.R2_SECRET_ACCESS_KEY, }); @@ - const signed = await client.sign(new Request(url, { method: "PUT" }), { + const contentTypeMap: Record<string, string> = { + wav: "audio/wav", + mp3: "audio/mpeg", + m4a: "audio/mp4", + flac: "audio/flac", + }; + const contentType = contentTypeMap[fileExtension]; + + const signed = await client.sign( + new Request(url, { method: "PUT", headers: { "Content-Type": contentType } }), + { aws: { signQuery: true }, - }); + } + );
35-47: Replace console usage with centralized loggingGuidelines prohibit console.*. Swap to your logger utility (e.g., pino/next-axiom or internal logger) and include structured context.
Would you like a quick wrapper (logger.ts) and a follow-up diff?
114-127: Normalize error responses to avoid leaking internalsAvoid returning raw error messages. Return a stable code and generic message; log details server-side.
Apply:
- return NextResponse.json( - { - error: "Failed to generate upload URL", - message: - error instanceof Error - ? error.message - : "An unexpected error occurred", - }, - { status: 500 } - ); + return NextResponse.json( + { error: "Failed to generate upload URL", code: "UPLOAD_URL_ERROR" }, + { status: 500 } + );apps/web/src/components/editor/panel-base-view.tsx (1)
19-31: Avoid rendering "undefined" in classNameWhen className is undefined, the template string will include "undefined".
Apply:
return ( <ScrollArea className="flex-1"> - <div className={`p-5 h-full ${className}`}>{children}</div> + <div className={cn("p-5 h-full", className)}>{children}</div> </ScrollArea> );apps/web/src/hooks/use-infinite-scroll.ts (1)
20-32: Throttle scroll handler with rAF to reduce layout thrashOptional improvement to avoid invoking onLoadMore multiple times per frame on fast scroll.
Apply:
- const handleScroll = useCallback( - (event: React.UIEvent<HTMLDivElement>) => { + const rafIdRef = useRef<number | null>(null); + + const handleScroll = useCallback( + (event: React.UIEvent<HTMLDivElement>) => { if (!enabled) return; - - const { scrollTop, scrollHeight, clientHeight } = event.currentTarget; - const nearBottom = scrollTop + clientHeight >= scrollHeight - threshold; - - if (nearBottom && hasMore && !isLoading) { - onLoadMore(); - } + if (rafIdRef.current != null) cancelAnimationFrame(rafIdRef.current); + rafIdRef.current = requestAnimationFrame(() => { + const { scrollTop, scrollHeight, clientHeight } = event.currentTarget; + const nearBottom = scrollTop + clientHeight >= scrollHeight - threshold; + if (nearBottom && hasMore && !isLoading) onLoadMore(); + }); }, [onLoadMore, hasMore, isLoading, threshold, enabled] );apps/web/src/lib/schemas/waitlist.ts (1)
3-5: Trim user input in schema to avoid trailing/leading whitespace issuesAdd
.trim()so emails like " [email protected] " validate and normalize consistently before DB checks.export const exportWaitlistSchema = z.object({ - email: z.string().email().max(320), + email: z.string().trim().email().max(320), });apps/web/src/app/api/waitlist/export/route.ts (3)
3-3: Remove unusedeqafter switching to idempotent insertYou won’t need
eqif you adopt the on-conflict insert flow below; drop it to avoid lint failures for unused imports.-import { db, exportWaitlist, eq } from "@opencut/db"; +import { db, exportWaitlist } from "@opencut/db";
15-19: Harden client IP extraction for rate limitingParse the first XFF entry and fall back to CF/Next-provided values to reduce header spoofing and comma-delimited lists.
- const ip = request.headers.get("x-forwarded-for") ?? "anonymous"; - const { success } = await baseRateLimit.limit(ip); + const xff = request.headers.get("x-forwarded-for"); + const ip = + (xff && xff.split(",")[0].trim()) || + request.headers.get("cf-connecting-ip") || + (request as any).ip || + "anonymous"; + const { success } = await baseRateLimit.limit(ip);
77-81: Replace console logging with structured server loggerProject guidelines disallow
console. Route handlers should use a central logger (e.g., pino/winston/Sentry). Replace when logging infra is available.apps/web/src/lib/editor-utils.ts (2)
15-21: Guard against invalid aspect ratiosAvoid NaN/Infinity and non-positive inputs to prevent odd custom-dimension math.
export function findBestCanvasPreset(aspectRatio: number): CanvasSize { - // Calculate aspect ratio for each preset and find the closest match + // Validate aspect ratio + if (!Number.isFinite(aspectRatio) || aspectRatio <= 0) { + return { width: 1920, height: 1080 }; + } + // Calculate aspect ratio for each preset and find the closest match
3-8: Mark presets as readonly for safety
as constprevents accidental mutation and aligns with the guidelines.-const DEFAULT_CANVAS_PRESETS = [ +const DEFAULT_CANVAS_PRESETS = [ { name: "16:9", width: 1920, height: 1080 }, { name: "9:16", width: 1080, height: 1920 }, { name: "1:1", width: 1080, height: 1080 }, { name: "4:3", width: 1440, height: 1080 }, -]; + ] as const;apps/web/src/components/ui/editable-timecode.tsx (1)
111-120: Expose validation state to assistive techSet
aria-invalidon the input when parsing fails.className={cn( "text-xs font-mono bg-transparent border-none outline-none", "focus:bg-background focus:border focus:border-primary focus:px-1 focus:rounded", "tabular-nums text-primary", hasError && "text-destructive focus:border-destructive", className )} + aria-invalid={hasError || undefined} style={{ width: `${formattedTime.length + 1}ch` }} placeholder={formattedTime}apps/web/src/components/theme-toggle.tsx (2)
4-4: Remove unused import
Moonisn’t used.-import { Sun, Moon } from "lucide-react"; +import { Sun } from "lucide-react";
15-23: Optional: reflect toggle state for assistive tech
aria-pressedcommunicates current theme state on the toggle control.- <Button + <Button size="icon" variant="text" className="h-7" type="button" - onClick={() => setTheme(theme === "dark" ? "light" : "dark")} + aria-pressed={theme === "dark"} + onClick={() => setTheme(theme === "dark" ? "light" : "dark")} >apps/web/src/components/footer.tsx (1)
19-21: Avoid console in client codeConsider removing or gating console.error to comply with code guidelines.
- console.error("Failed to fetch GitHub stars", err); + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.error("Failed to fetch GitHub stars", err); + }apps/web/src/app/api/sounds/search/route.ts (1)
162-178: Compose a single filter string instead of repeating filter paramsFreesound’s API expects one filter expression; combine clauses with AND/OR to reduce ambiguity around repeated query keys.
If keeping multiple params is intentional and proven, ignore this.
apps/web/src/components/providers/global-prefetcher.ts (2)
23-68: Abort fetch on unmount and avoid dangling requestsUse AbortController instead of an ignore flag to cancel the request and free resources.
const prefetchTopSounds = async () => { try { - if (!ignore) { + if (!ignore) { setLoading(true); setError(null); } - const response = await fetch( - "/api/sounds/search?page_size=50&sort=downloads" - ); + const controller = new AbortController(); + const response = await fetch( + "/api/sounds/search?page_size=50&sort=downloads", + { signal: controller.signal } + ); if (!ignore) { if (!response.ok) { throw new Error(`Failed to fetch: ${response.status}`); } const data = await response.json(); setTopSoundEffects(data.results); setHasLoaded(true); @@ } finally { if (!ignore) { setLoading(false); } } }; const timeoutId = setTimeout(prefetchTopSounds, 100); return () => { clearTimeout(timeoutId); ignore = true; + // Best-effort abort if the request is in-flight + try { + // controller may not exist if fetch didn't start yet + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (globalThis as any).AbortController && new AbortController().abort(); + } catch {} };
50-53: Avoid console in client codeRemove or gate console.error per guidelines.
- console.error("Failed to prefetch top sounds:", error); + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.error("Failed to prefetch top sounds:", error); + }apps/web/src/app/editor/[project_id]/layout.tsx (1)
10-12: LGTM.Hook usage and client-only boundary look correct. Consider wrapping children in a semantic
for a11y, but optional.apps/web/src/stores/text-properties-store.ts (1)
25-33: Persist config: consider adding version/migrate.Zustand persist without versioning/migration may lock in state shape. Add { version, migrate } to ease future tab additions/renames.
export const useTextPropertiesStore = create<TextPropertiesState>()( persist( (set) => ({ activeTab: "transform", setActiveTab: (tab) => set({ activeTab: tab }), }), - { name: "text-properties" } + { name: "text-properties", version: 1, migrate: (state, v) => state } ) );apps/web/src/components/ui/input-with-back.tsx (2)
6-7: Animation library inconsistency with the rest of the app.This file imports from "motion/react" while other components (e.g., language-select) use framer-motion. Recommend standardizing to one lib to avoid double bundles and API drift.
Would you like a follow-up diff switching this file to framer-motion for consistency?
32-37: Offset calc won’t update on resize.The buttonOffset derives from initial bounding box only. Consider a ResizeObserver or window resize listener to keep alignment after layout changes.
apps/web/src/stores/sounds-store.ts (1)
246-248: MIME/extension assumption.Hardcoding “.mp3” and type "audio/mpeg" may mismatch preview formats. If upstream previews change (e.g., OGG), detect from response.headers.get("Content-Type") and set name/Type accordingly.
apps/web/src/components/language-select.tsx (2)
125-137: Flag/globe icon a11y.When showing Globe with “Auto”, text is present—OK. For flags, screen readers may announce country codes. Consider aria-hidden on flags and keep the readable language name next to them.
- <ReactCountryFlag + <ReactCountryFlag + aria-hidden countryCode={selectedCountry} svg style={{ width: "1.05rem", height: "1.05rem" }} />
96-117: Add ARIA state to the toggle button.Expose expanded state for assistive tech.
- <motion.button + <motion.button type="button" className={cn( "absolute w-full h-full flex flex-col overflow-hidden items-start justify-between z-10 rounded-lg px-3 cursor-pointer", "!bg-foreground/10 backdrop-blur-md text-foreground py-0", "transition-[color,box-shadow] focus:border-ring focus:ring-ring/50 focus:ring-[1px]" )} + aria-expanded={expanded} + aria-controls="language-list"And set id on the list container:
- <div className="flex flex-col gap-2 my-2.5 w-full overflow-y-auto scrollbar-hidden"> + <div id="language-list" className="flex flex-col gap-2 my-2.5 w-full overflow-y-auto scrollbar-hidden">apps/web/src/types/timeline.ts (2)
1-1: Use type-only import for types.Complies with “Use import type for types”.
-import { MediaType } from "@/types/media"; +import type { MediaType } from "@/types/media";
61-63: Avoid React namespace types in .ts; use imported types.Prevents implicit global React type dependency.
-export interface TimelineElementProps { +import type { MouseEvent } from "react"; + +export interface TimelineElementProps { element: TimelineElement; track: TimelineTrack; zoomLevel: number; isSelected: boolean; - onElementMouseDown: (e: React.MouseEvent, element: TimelineElement) => void; - onElementClick: (e: React.MouseEvent, element: TimelineElement) => void; + onElementMouseDown: (e: MouseEvent, element: TimelineElement) => void; + onElementClick: (e: MouseEvent, element: TimelineElement) => void; }apps/web/src/types/effects.ts (2)
1-21: Derive EffectType from a const array to reuse at runtime (UI lists, validation).Keeps type and runtime source of truth in sync.
-export type EffectType = - | "blur" - | "brightness" - | "contrast" - | "saturation" - | "hue" - | "gamma" - | "sepia" - | "grayscale" - | "invert" - | "vintage" - | "dramatic" - | "warm" - | "cool" - | "cinematic" - | "vignette" - | "grain" - | "sharpen" - | "emboss" - | "edge" - | "pixelate"; +export const EFFECT_TYPES = [ + "blur", + "brightness", + "contrast", + "saturation", + "hue", + "gamma", + "sepia", + "grayscale", + "invert", + "vintage", + "dramatic", + "warm", + "cool", + "cinematic", + "vignette", + "grain", + "sharpen", + "emboss", + "edge", + "pixelate", +] as const; +export type EffectType = (typeof EFFECT_TYPES)[number];
23-31: Preferexport type { ... } = { ... }over interfaces for consistency.Matches repo guidance (“Use export type for types”); no behavioral change.
-export interface EffectPreset { +export type EffectPreset = { id: string; name: string; description: string; category: EffectCategory; icon: string; parameters: EffectParameters; preview?: string; -} +}; @@ -export interface EffectParameters { +export type EffectParameters = { // Basic adjustments brightness?: number; // -100 to 100 contrast?: number; // -100 to 100 saturation?: number; // -100 to 100 hue?: number; // -180 to 180 gamma?: number; // 0.1 to 3.0 @@ pixelate?: number; // 1 to 50 -} +}; @@ -export interface TimelineEffect { +export type TimelineEffect = { id: string; elementId: string; trackId: string; effectType: EffectType; parameters: EffectParameters; startTime: number; endTime: number; enabled: boolean; -} +}; @@ -export interface EffectInstance { +export type EffectInstance = { id: string; name: string; effectType: EffectType; parameters: EffectParameters; elementId: string; trackId: string; startTime: number; endTime: number; enabled: boolean; createdAt: Date; updatedAt: Date; -} +};Also applies to: 41-72, 74-83, 85-97
apps/web/src/components/editor/timeline/timeline-track.tsx (2)
689-696: Remove console logging in UI code.Debug logs should be gated or removed per guidelines.
- // Debug logging - console.log( - JSON.stringify({ - message: "Drop event started in timeline track", - dataTransferTypes: Array.from(e.dataTransfer.types), - trackId: track.id, - trackType: track.type, - }) - ); + // Debug: drop event context (intentionally left out in production)
1076-1107: Avoid await in loops; batch async file adds.This loop serializes I/O and violates repo rule. Use Promise.all and fetch latest store state when needed.
// Sketch (outside diff): batch add then place on tracks const { activeProject } = useProjectStore.getState(); const { addMediaFile } = useMediaStore.getState(); await Promise.all(processedItems.map((p) => addMediaFile(activeProject.id!, p))); const currentMediaFiles = useMediaStore.getState().mediaFiles; for (const processedItem of processedItems) { const addedItem = currentMediaFiles.find( (it) => it.name === processedItem.name && it.url === processedItem.url ); if (!addedItem) continue; const trackType: TrackType = addedItem.type === "audio" ? "audio" : "media"; const targetTrackId = insertTrackAt(trackType, 0); addElementToTrack(targetTrackId, { type: "media", mediaId: addedItem.id, name: addedItem.name, duration: addedItem.duration || 5, startTime: currentTime, trimStart: 0, trimEnd: 0, width: addedItem.width || 200, height: addedItem.height || 150, x: 0, y: 0, rotation: 0, opacity: 1, }); }apps/web/src/lib/export.ts (2)
52-54: Prefer OfflineAudioContext or close AudioContext to avoid leaksYou’re instantiating a live AudioContext for offline mixing and never closing it. Either switch to OfflineAudioContext for rendering or at least close the context after mixing to free audio threads.
Example minimal fix:
// Mix all audio elements for (const element of audioElements) { if (element.muted) continue; ... } - return outputBuffer; + await audioContext.close().catch(() => {}); + return outputBuffer;
66-68: Remove redundant ternaryYou already guard with
if (element.type !== "media") continue;. The ternary is dead code.- const mediaItem = - element.type === "media" ? mediaMap.get(element.mediaId) : null; + const mediaItem = mediaMap.get(element.mediaId);apps/web/src/components/editor/timeline/index.tsx (1)
506-512: Center new media by default (comment currently misleading)
x: 0, y: 0is top-left; comment says “Center position”. Either fix the comment or compute canvas-centered placement.- // Set initial dimensions and position - width: addedItem.width || 200, - height: addedItem.height || 150, - x: 0, // Center position - y: 0, // Center position + // Initial dimensions and centered position on canvas + width: addedItem.width ?? 200, + height: addedItem.height ?? 150, + x: Math.max( + 0, + ((useProjectStore.getState().activeProject?.canvasSize.width ?? 0) - + (addedItem.width ?? 200)) / 2 + ), + y: Math.max( + 0, + ((useProjectStore.getState().activeProject?.canvasSize.height ?? 0) - + (addedItem.height ?? 150)) / 2 + ), rotation: 0, opacity: 1,apps/web/src/stores/timeline-store.ts (1)
1472-1478: Saner initial media dimensionsUsing raw media pixel dimensions can create gigantic boxes on small canvases. Consider capping to canvas or a preset.
If project canvas is known in this context, cap width/height to a max (e.g., 80% of canvas) or normalize by aspect ratio. I can draft a helper if helpful.
Also applies to: 1505-1511
apps/web/src/components/editor/media-panel/views/effects.tsx (6)
38-39: Remove unused import-import { useInfiniteScroll } from "@/hooks/use-infinite-scroll";
9-22: Prune unused icon imports- Plus, - Eye, - EyeOff, - Trash2, - Settings,
192-201: Remove unused variables- const { currentTime } = usePlaybackStore(); const { tracks } = useTimelineStore();Also drop the unused TooltipProvider import if not used elsewhere.
- TooltipProvider,
224-243: Simplify add flow toast duplicationEffectsStore.applyEffectToElement already toasts success/failure; the extra success toast here duplicates messaging.
You can rely on the store toast and only surface errors here.
246-251: Remove console logs in production- console.log("Media tracks:", mediaTracks.length, "Total media elements:", totalMediaElements);
253-259: Redundant memoeffectsToDisplay is identical to filteredEffects. Remove the extra memo and reference filteredEffects directly.
- const effectsToDisplay = useMemo(() => { - if (localSearchQuery.trim()) { - return filteredEffects; - } - return filteredEffects; - }, [filteredEffects, localSearchQuery]);EFFECTS_README.md (3)
54-71: Fix file paths to match repo structure-1. **Effects Types** (`src/types/effects.ts`) +1. **Effects Types** (`apps/web/src/types/effects.ts`) ... -2. **Effects Store** (`src/stores/effects-store.ts`) +2. **Effects Store** (`apps/web/src/stores/effects-store.ts`) ... -3. **Effects View** (`src/components/editor/media-panel/views/effects.tsx`) +3. **Effects View** (`apps/web/src/components/editor/media-panel/views/effects.tsx`) ... -4. **Effects Properties** (`src/components/editor/properties-panel/effects-properties.tsx`) +4. **Effects Properties** (`apps/web/src/components/editor/properties-panel/effects-properties.tsx`) ... -5. **Effects Utils** (`src/lib/effects-utils.ts`) +5. **Effects Utils** (`apps/web/src/lib/effects-utils.ts`)
101-108: Use canvas filter example (matches implementation)-Effects are applied using CSS filters for real-time preview: -```typescript -// Example: Brightness and contrast effect -const filterString = "brightness(1.2) contrast(1.3)"; -videoElement.style.filter = filterString; -``` +Effects are applied to the 2D canvas context for real-time preview: +```typescript +ctx.save(); +ctx.filter = "brightness(1.2) contrast(1.3)"; +// draw your frame/image here +ctx.restore(); +```
183-188: Avoid recommending enums (project guideline forbids TS enums)Switch guidance to discriminated unions/const unions.
-1. Add effect type to `EffectType` enum +1. Add effect id to the `EffectType` string union (e.g., `export type EffectType = "brightness" | ...`)apps/web/src/components/editor/properties-panel/effects-properties.tsx (2)
122-135: Reset button should be type="button" and accessibleAvoid default submit behavior and expose an accessible name.
- <Button + <Button variant="ghost" size="sm" onClick={handleReset} className="h-6 px-2 text-xs" + type="button" + aria-label="Reset parameters" + title="Reset parameters" > - <RotateCcw className="h-3 w-3 mr-1" /> + <RotateCcw className="h-3 w-3 mr-1" aria-hidden /> Reset </Button>
32-36: Tighten types for parameter updatesUse
keyof EffectParametersfor stronger typing and DX.+import type { EffectParameters } from "@/types/effects"; @@ - const handleParameterChange = (parameter: string, value: number[]) => { + const handleParameterChange = (parameter: keyof EffectParameters, value: number[]) => { updateEffectParameters(selectedEffect.id, { [parameter]: value[0], }); };apps/web/src/lib/effects-utils.ts (1)
7-101: Gamma not represented in CSS filter pipeline
parametersToCSSFiltersignoresgamma. CSS has no native gamma filter; if you keepgammain types, either (a) document that it only applies in canvas/WebGL paths, or (b) implement a canvas-based gamma pass. Right now it’s silently ignored at preview time.apps/web/src/components/editor/preview-panel.tsx (2)
336-338: Use the correct resize cursor for each handleCurrently always sets
"nw-resize", which is misleading for other corners.- document.body.style.cursor = dragState.isDragging ? "grabbing" : - resizeState.isResizing ? "nw-resize" : "grab"; + document.body.style.cursor = dragState.isDragging + ? "grabbing" + : resizeState.isResizing + ? `${resizeState.corner}-resize` + : "grab";
350-353: Avoidanyin TSX handlers; use concrete typesReplace
anywithTimelineElement(or a narrowedTextvariant) to satisfy the TS guidelines.- const handleTextMouseDown = ( - e: React.MouseEvent<HTMLDivElement>, - element: any, - track: TimelineTrack - ) => { + const handleTextMouseDown = ( + e: React.MouseEvent<HTMLDivElement | HTMLButtonElement>, + element: TimelineElement, + track: TimelineTrack + ) => { @@ - const handleResizeStart = ( - e: React.MouseEvent<HTMLDivElement>, - element: any, + const handleResizeStart = ( + e: React.MouseEvent<HTMLDivElement | HTMLButtonElement>, + element: TimelineElement, track: TimelineTrack, corner: string ) => { @@ - const handleRotationStart = ( - e: React.MouseEvent<HTMLDivElement>, - element: any, + const handleRotationStart = ( + e: React.MouseEvent<HTMLDivElement | HTMLButtonElement>, + element: TimelineElement, track: TimelineTrack ) => {Also applies to: 376-381, 402-406
apps/web/src/stores/effects-store.ts (2)
376-384: Remove stray console logs in storeAvoid
console.login production code per guidelines.- console.log("Available tracks:", tracks.map(t => ({ type: t.type, elements: t.elements.length }))); - console.log("Media tracks found:", mediaTracks.length); - - // Also check for any media elements that might be video - const allMediaElements = mediaTracks.flatMap(track => track.elements); - console.log("Total media elements:", allMediaElements.length); + // Debug logs removed
395-405: Element time window ignores trims; align with timeline mathUse
duration - trimStart - trimEndto determine visibility, matchinggetTotalDuration.- const elementAtTime = track.elements.find((element) => { - const elementEnd = element.startTime + element.duration; - return currentTime >= element.startTime && currentTime < elementEnd; - }); + const elementAtTime = track.elements.find((element) => { + const visibleEnd = + element.startTime + + (element.duration - element.trimStart - element.trimEnd); + return currentTime >= element.startTime && currentTime < visibleEnd; + }); @@ - const closestElement = track.elements.reduce((closest, element) => { - const elementEnd = element.startTime + element.duration; - const elementMiddle = element.startTime + element.duration / 2; - const closestMiddle = closest.startTime + closest.duration / 2; + const closestElement = track.elements.reduce((closest, element) => { + const elementVisible = element.duration - element.trimStart - element.trimEnd; + const closestVisible = closest.duration - closest.trimStart - closest.trimEnd; + const elementMiddle = element.startTime + elementVisible / 2; + const closestMiddle = closest.startTime + closestVisible / 2;Also applies to: 412-421
| const ip = request.headers.get("x-forwarded-for") ?? "anonymous"; | ||
| const { success } = await baseRateLimit.limit(ip); | ||
|
|
||
| if (!success) { | ||
| return NextResponse.json({ error: "Too many requests" }, { status: 429 }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a trusted client IP and parse X-Forwarded-For safely
Relying solely on x-forwarded-for is spoofable. Prefer request.ip with a safe fallback.
Apply:
- const ip = request.headers.get("x-forwarded-for") ?? "anonymous";
+ const ipHeader = request.ip ?? request.headers.get("x-forwarded-for") ?? "";
+ const ip = ipHeader.split(",")[0].trim() || "anonymous";📝 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.
| const ip = request.headers.get("x-forwarded-for") ?? "anonymous"; | |
| const { success } = await baseRateLimit.limit(ip); | |
| if (!success) { | |
| return NextResponse.json({ error: "Too many requests" }, { status: 429 }); | |
| } | |
| const ipHeader = request.ip ?? request.headers.get("x-forwarded-for") ?? ""; | |
| const ip = ipHeader.split(",")[0].trim() || "anonymous"; | |
| const { success } = await baseRateLimit.limit(ip); | |
| if (!success) { | |
| return NextResponse.json({ error: "Too many requests" }, { status: 429 }); | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/app/api/get-upload-url/route.ts around lines 25-30, you're using
the raw x-forwarded-for header which is spoofable; update to use request.ip when
available and only fall back to a safely parsed X-Forwarded-For value: take the
header, split on commas, trim entries, pick the first non-empty entry that is a
valid/public IP (ignore private/local addresses), and if none found use
"anonymous"; then pass that sanitized IP to baseRateLimit.limit. Ensure trimming
and validation are applied before rate-limiting.
| const params = new URLSearchParams({ | ||
| query: query || "", | ||
| token: env.FREESOUND_API_KEY, | ||
| page: page.toString(), | ||
| page_size: pageSize.toString(), | ||
| sort: sortParam, | ||
| fields: | ||
| "id,name,description,url,previews,download,duration,filesize,type,channels,bitrate,bitdepth,samplerate,username,tags,license,created,num_downloads,avg_rating,num_ratings", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid API key in query; add timeout and use Authorization header
Move the token to the Authorization header to prevent leakage via logs/caches and add a request timeout to avoid hanging.
- const params = new URLSearchParams({
- query: query || "",
- token: env.FREESOUND_API_KEY,
- page: page.toString(),
- page_size: pageSize.toString(),
- sort: sortParam,
+ const params = new URLSearchParams({
+ query: query || "",
+ page: page.toString(),
+ page_size: pageSize.toString(),
+ sort: sortParam,
fields:
"id,name,description,url,previews,download,duration,filesize,type,channels,bitrate,bitdepth,samplerate,username,tags,license,created,num_downloads,avg_rating,num_ratings",
});- const response = await fetch(`${baseUrl}?${params.toString()}`);
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000);
+ const response = await fetch(`${baseUrl}?${params.toString()}`, {
+ headers: { Authorization: `Token ${env.FREESOUND_API_KEY}` },
+ signal: controller.signal,
+ });
+ clearTimeout(timeoutId);Also applies to: 180-189
| if (commercial_only) { | ||
| params.append( | ||
| "filter", | ||
| 'license:("Attribution" OR "Creative Commons 0" OR "Attribution Noncommercial" OR "Attribution Commercial")' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Commercial-only filter includes noncommercial and invalid license
Including "Attribution Noncommercial" defeats the commercial-only intent, and "Attribution Commercial" doesn’t exist in Freesound licenses. Restrict to licenses that allow commercial use.
- params.append(
- "filter",
- 'license:("Attribution" OR "Creative Commons 0" OR "Attribution Noncommercial" OR "Attribution Commercial")'
- );
+ params.append(
+ "filter",
+ 'license:("Attribution" OR "Creative Commons 0")'
+ );📝 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.
| if (commercial_only) { | |
| params.append( | |
| "filter", | |
| 'license:("Attribution" OR "Creative Commons 0" OR "Attribution Noncommercial" OR "Attribution Commercial")' | |
| ); | |
| } | |
| if (commercial_only) { | |
| params.append( | |
| "filter", | |
| 'license:("Attribution" OR "Creative Commons 0")' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/app/api/sounds/search/route.ts around lines 167 to 172, the
commercial_only filter currently includes "Attribution Noncommercial" and an
invalid "Attribution Commercial"; change the filter to only include valid
Freesound licenses that permit commercial use (e.g., "Attribution" and "Creative
Commons 0"), removing any noncommercial or non‑existent license names, and
update the params.append call to use the corrected quoted license list for the
"filter" parameter.
| const transcribeRequestSchema = z.object({ | ||
| filename: z.string().min(1, "Filename is required"), | ||
| language: z.string().optional().default("auto"), | ||
| decryptionKey: z.string().min(1, "Decryption key is required").optional(), | ||
| iv: z.string().min(1, "IV is required").optional(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate decryption fields as an all-or-nothing pair.
Currently, one field can be provided without the other; it silently no-ops. Enforce both-or-neither at the schema level.
-const transcribeRequestSchema = z.object({
+const transcribeRequestSchema = z
+ .object({
filename: z.string().min(1, "Filename is required"),
language: z.string().optional().default("auto"),
decryptionKey: z.string().min(1, "Decryption key is required").optional(),
iv: z.string().min(1, "IV is required").optional(),
-});
+ })
+ .refine(
+ (d) => (!d.decryptionKey && !d.iv) || (Boolean(d.decryptionKey) && Boolean(d.iv)),
+ { message: "Both decryptionKey and iv must be provided together." }
+ );📝 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.
| const transcribeRequestSchema = z.object({ | |
| filename: z.string().min(1, "Filename is required"), | |
| language: z.string().optional().default("auto"), | |
| decryptionKey: z.string().min(1, "Decryption key is required").optional(), | |
| iv: z.string().min(1, "IV is required").optional(), | |
| }); | |
| const transcribeRequestSchema = z | |
| .object({ | |
| filename: z.string().min(1, "Filename is required"), | |
| language: z.string().optional().default("auto"), | |
| decryptionKey: z.string().min(1, "Decryption key is required").optional(), | |
| iv: z.string().min(1, "IV is required").optional(), | |
| }) | |
| .refine( | |
| (d) => (!d.decryptionKey && !d.iv) || (Boolean(d.decryptionKey) && Boolean(d.iv)), | |
| { message: "Both decryptionKey and iv must be provided together." } | |
| ); |
🤖 Prompt for AI Agents
In apps/web/src/app/api/transcribe/route.ts around lines 7 to 12, the Zod schema
allows decryptionKey and iv to be provided independently; update the schema to
enforce an all-or-nothing pair by adding a refine (or superRefine) that checks
both fields are either present (non-empty) or both absent, returning a
validation error if one is set without the other and providing a clear message
like "Both decryptionKey and iv must be provided together or not at all."
| console.error( | ||
| "Missing environment variables:", | ||
| JSON.stringify(transcriptionCheck.missingVars) | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Replace console.* with the project’s structured logger; avoid logging secrets.
Guidelines prohibit console. Use your standard logging util (e.g., logger.error/debug) with redaction. Ensure no PII or secrets are logged.
Do you have a preferred logger (pino/winston/Sentry/Datadog)? I can generate a follow-up patch to swap these calls.
Also applies to: 125-126, 150-151, 168-171, 180-181
🏁 Script executed:
#!/bin/bash
# Find all console.* calls in the transcribe route file
rg -n "console\." apps/web/src/app/api/transcribe/route.ts || true
# Check for existing structured logger usage across TS files
rg -n "logger\." -g "*.ts" || true
# Search for known logging library imports (pino, winston)
rg -n "import .*pino" -g "*.ts" || true
rg -n "import .*winston" -g "*.ts" || trueLength of output: 520
🏁 Script executed:
#!/bin/bash
# Locate any existing logger utility files in the repo
fd -HI logger -t f || true
# Search for exports of a logger instance
rg -n "export .*logger" -g "*.ts" || true
# Search for creation of loggers via common APIs
rg -n "createLogger" -g "*.ts" || trueLength of output: 101
Introduce a structured logger and replace all console.* calls in the transcribe route
The file apps/web/src/app/api/transcribe/route.ts currently uses console.error and console.log at lines 66, 125, 145, 150, 168, and 180. Per project guidelines, replace these with your standard logger (e.g. pino, winston) and ensure any sensitive fields are redacted. No existing logger util was found in the repo—please add or import your preferred logger and update these calls accordingly.
🤖 Prompt for AI Agents
In apps/web/src/app/api/transcribe/route.ts around lines 66-180, replace all
console.error/console.log calls at lines 66, 125, 145, 150, 168, and 180 with a
structured logger: add or import a logger instance (e.g. pino or winston) at the
top of the file, use logger.error/info/debug as appropriate for each call, and
ensure any sensitive fields (API keys, tokens, file contents, PII) are redacted
before logging; keep log messages descriptive and include contextual data via
structured fields rather than string concatenation.
| const missingVars = []; | ||
|
|
||
| if (!env.CLOUDFLARE_ACCOUNT_ID) missingVars.push("CLOUDFLARE_ACCOUNT_ID"); | ||
| if (!env.R2_ACCESS_KEY_ID) missingVars.push("R2_ACCESS_KEY_ID"); | ||
| if (!env.R2_SECRET_ACCESS_KEY) missingVars.push("R2_SECRET_ACCESS_KEY"); | ||
| if (!env.R2_BUCKET_NAME) missingVars.push("R2_BUCKET_NAME"); | ||
| if (!env.MODAL_TRANSCRIPTION_URL) missingVars.push("MODAL_TRANSCRIPTION_URL"); | ||
|
|
||
| return { configured: missingVars.length === 0, missingVars }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid implicit any[] in TS; annotate missingVars.
Current declaration infers any[], violating “Don’t use the any type.” Add explicit string[].
-export function isTranscriptionConfigured() {
- const missingVars = [];
+export function isTranscriptionConfigured() {
+ const missingVars: string[] = [];📝 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.
| const missingVars = []; | |
| if (!env.CLOUDFLARE_ACCOUNT_ID) missingVars.push("CLOUDFLARE_ACCOUNT_ID"); | |
| if (!env.R2_ACCESS_KEY_ID) missingVars.push("R2_ACCESS_KEY_ID"); | |
| if (!env.R2_SECRET_ACCESS_KEY) missingVars.push("R2_SECRET_ACCESS_KEY"); | |
| if (!env.R2_BUCKET_NAME) missingVars.push("R2_BUCKET_NAME"); | |
| if (!env.MODAL_TRANSCRIPTION_URL) missingVars.push("MODAL_TRANSCRIPTION_URL"); | |
| return { configured: missingVars.length === 0, missingVars }; | |
| export function isTranscriptionConfigured() { | |
| const missingVars: string[] = []; | |
| if (!env.CLOUDFLARE_ACCOUNT_ID) missingVars.push("CLOUDFLARE_ACCOUNT_ID"); | |
| if (!env.R2_ACCESS_KEY_ID) missingVars.push("R2_ACCESS_KEY_ID"); | |
| if (!env.R2_SECRET_ACCESS_KEY) missingVars.push("R2_SECRET_ACCESS_KEY"); | |
| if (!env.R2_BUCKET_NAME) missingVars.push("R2_BUCKET_NAME"); | |
| if (!env.MODAL_TRANSCRIPTION_URL) missingVars.push("MODAL_TRANSCRIPTION_URL"); | |
| return { configured: missingVars.length === 0, missingVars }; | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/lib/transcription-utils.ts around lines 4 to 12, the variable
missingVars is implicitly typed as any[]; change its declaration to an explicit
string[] to satisfy the TypeScript no-any rule. Update the declaration to const
missingVars: string[] = []; and leave the rest of the logic unchanged so you
still push string keys and return the configured/missingVars result.
| const mediaItem = useMediaStore | ||
| .getState() | ||
| .mediaFiles.find((item) => item.file === file); | ||
| if (!mediaItem) throw new Error("Failed to create media item"); | ||
|
|
||
| const success = useTimelineStore | ||
| .getState() | ||
| .addElementAtTime(mediaItem, usePlaybackStore.getState().currentTime); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fragile lookup by File identity.
Finding media via item.file === file relies on object identity and may break if the file is cloned/transformed. Prefer capturing the id returned by addMediaFile or generating it in this scope and passing it in.
- await useMediaStore.getState().addMediaFile(activeProject.id, {
+ const id = crypto.randomUUID();
+ await useMediaStore.getState().addMediaFile(activeProject.id, {
name: sound.name,
type: "audio",
file,
duration: sound.duration,
url: URL.createObjectURL(file),
+ id,
});
-
- const mediaItem = useMediaStore
- .getState()
- .mediaFiles.find((item) => item.file === file);
- if (!mediaItem) throw new Error("Failed to create media item");
+ const mediaItem = useMediaStore.getState().mediaFiles.find((m) => m.id === id);
+ if (!mediaItem) throw new Error("Failed to create media item");Note: adjust addMediaFile to respect provided id or return the created id.
📝 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.
| const mediaItem = useMediaStore | |
| .getState() | |
| .mediaFiles.find((item) => item.file === file); | |
| if (!mediaItem) throw new Error("Failed to create media item"); | |
| const success = useTimelineStore | |
| .getState() | |
| .addElementAtTime(mediaItem, usePlaybackStore.getState().currentTime); | |
| // Generate a stable ID instead of relying on object identity | |
| const id = crypto.randomUUID(); | |
| await useMediaStore.getState().addMediaFile(activeProject.id, { | |
| name: sound.name, | |
| type: "audio", | |
| file, | |
| duration: sound.duration, | |
| url: URL.createObjectURL(file), | |
| id, | |
| }); | |
| // Lookup by the generated ID for a more robust match | |
| const mediaItem = useMediaStore.getState().mediaFiles.find((m) => m.id === id); | |
| if (!mediaItem) throw new Error("Failed to create media item"); | |
| const success = useTimelineStore | |
| .getState() | |
| .addElementAtTime(mediaItem, usePlaybackStore.getState().currentTime); |
🤖 Prompt for AI Agents
In apps/web/src/stores/sounds-store.ts around lines 258 to 266, the code looks
up media by object identity (item.file === file) which is fragile; instead
generate or capture a stable id when creating the media file and use that id to
reference the media item when calling addElementAtTime. Change the flow so you
either (a) generate an id in this scope, pass it into addMediaFile (and update
addMediaFile to accept and use a provided id), then find the media item by that
id; or (b) call addMediaFile and have it return the created id, then use that id
to lookup the media item and pass the id into addElementAtTime; ensure
addMediaFile is updated to respect a provided id or to return the created id.
| updateElement: ( | ||
| trackId: string, | ||
| elementId: string, | ||
| updates: Partial<{ | ||
| x: number; | ||
| y: number; | ||
| width: number; | ||
| height: number; | ||
| rotation: number; | ||
| opacity: number; | ||
| }> | ||
| ) => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add optional pushHistory flag to updateElement API (drag/resize friendliness)
Pushing history on every mousemove will bloat undo/redo and degrade performance.
- updateElement: (
- trackId: string,
- elementId: string,
- updates: Partial<{
- x: number;
- y: number;
- width: number;
- height: number;
- rotation: number;
- opacity: number;
- }>
- ) => void;
+ updateElement: (
+ trackId: string,
+ elementId: string,
+ updates: Partial<{
+ x: number;
+ y: number;
+ width: number;
+ height: number;
+ rotation: number;
+ opacity: number;
+ }>,
+ pushHistory?: boolean
+ ) => void;📝 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.
| updateElement: ( | |
| trackId: string, | |
| elementId: string, | |
| updates: Partial<{ | |
| x: number; | |
| y: number; | |
| width: number; | |
| height: number; | |
| rotation: number; | |
| opacity: number; | |
| }> | |
| ) => void; | |
| updateElement: ( | |
| trackId: string, | |
| elementId: string, | |
| updates: Partial<{ | |
| x: number; | |
| y: number; | |
| width: number; | |
| height: number; | |
| rotation: number; | |
| opacity: number; | |
| }>, | |
| pushHistory?: boolean | |
| ) => void; |
🤖 Prompt for AI Agents
In apps/web/src/stores/timeline-store.ts around lines 210 to 221, the
updateElement API should accept an optional pushHistory flag so callers (e.g.
during drag/resize mousemove) can avoid pushing history on every update; change
the type to include pushHistory?: boolean on the updates parameter (or as a
separate optional parameter), update the implementation to default pushHistory
to true, and only perform the history push when pushHistory !== false, and
propagate this flag through any internal calls so mousemove handlers can pass
false to prevent history bloat while preserving normal undo/redo behavior for
discrete actions.
| updateElement: (trackId, elementId, updates) => { | ||
| get().pushHistory(); | ||
| updateTracksAndSave( | ||
| get()._tracks.map((track) => | ||
| track.id === trackId | ||
| ? { | ||
| ...track, | ||
| elements: track.elements.map((element) => | ||
| element.id === elementId | ||
| ? { ...element, ...updates } | ||
| : element | ||
| ), | ||
| } | ||
| : track | ||
| ) | ||
| ); | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t push to history on every update; validate transform inputs
Use an optional flag and clamp/guard values to keep state sane.
- updateElement: (trackId, elementId, updates) => {
- get().pushHistory();
- updateTracksAndSave(
+ updateElement: (trackId, elementId, updates, pushHistory = true) => {
+ if (pushHistory) get().pushHistory();
+ // Basic validation/clamping
+ const safe = { ...updates } as any;
+ if (safe.width !== undefined) safe.width = Math.max(1, Number(safe.width) || 1);
+ if (safe.height !== undefined) safe.height = Math.max(1, Number(safe.height) || 1);
+ if (safe.opacity !== undefined) safe.opacity = Math.min(1, Math.max(0, Number(safe.opacity) || 0));
+ if (safe.rotation !== undefined) safe.rotation = Number(safe.rotation) || 0;
+ updateTracksAndSave(
get()._tracks.map((track) =>
track.id === trackId
? {
...track,
elements: track.elements.map((element) =>
element.id === elementId
- ? { ...element, ...updates }
+ ? { ...element, ...safe }
: element
),
}
: track
)
);
},If UI drag/resize calls this continuously, pass pushHistory=false during move, and true on commit.
🏁 Script executed:
#!/bin/bash
# Locate updateElement definition and context
rg -nP -C3 "updateElement" --type=ts apps/web/src/stores/timeline-store.ts
# Locate all calls to updateElement across the codebase
rg -nR --type=ts "updateElement\s*\(" -g "apps/web/src/**/*.ts"
# Locate pushHistory definition and other uses for comparison
rg -nP "pushHistory" --type=ts -g "apps/web/src/stores/**/*.ts"Length of output: 5322
Make updateElement optionally skip history push & clamp update values
• Change signature to updateElement(trackId, elementId, updates, pushHistory = true) and call get().pushHistory() only when pushHistory is true
• Before merging updates into the element, clamp numeric props: width ≥ 1, height ≥ 1, opacity ∈ [0,1], rotation = Number… to prevent invalid state
🤖 Prompt for AI Agents
In apps/web/src/stores/timeline-store.ts around lines 904 to 920, change
updateElement to accept an optional fourth parameter pushHistory = true and only
call get().pushHistory() when pushHistory is true; before merging updates into
the element, sanitize numeric fields by converting rotation to Number(rotation)
(or 0 on NaN), clamp width and height to a minimum of 1, and clamp opacity to
the [0,1] range (e.g., Math.max(0, Math.min(1, Number(opacity)))) so invalid
values aren’t stored, then merge these sanitized values into the element and
continue with updateTracksAndSave as before.
| x?: number; // Position relative to canvas center | ||
| y?: number; // Position relative to canvas center | ||
| width?: number; // Display width | ||
| height?: number; // Display height | ||
| rotation?: number; // in degrees | ||
| opacity?: number; // 0-1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make transform defaults explicit to avoid undefined at render/export.
MediaElement transform fields are optional while many code paths now rely on them. Either:
- centralize defaults (e.g., width/height/x/y/rotation/opacity) via a helper used on every element creation, or
- make these fields required in MediaElement and supply defaults at creation.
This prevents downstream NaNs/undefined in canvas and CSS.
🤖 Prompt for AI Agents
In apps/web/src/types/timeline.ts around lines 22-27, MediaElement transform
fields are optional which causes downstream NaNs/undefined during render/export;
fix by centralizing explicit defaults (e.g., x:0, y:0, width:100, height:100,
rotation:0, opacity:1) via a small helper that
normalizes/applyDefaultTransform(element) and call it wherever elements are
created or before any render/export path, and optionally update the MediaElement
type to make those transform fields required so callers must supply normalized
values.
| <div | ||
| key={element.id} | ||
| className="absolute cursor-move select-none group" | ||
| style={{ | ||
| left: screenX, | ||
| top: screenY, | ||
| transform: `translate(-50%, -50%) rotate(${rotation}deg)`, | ||
| opacity, | ||
| pointerEvents: dragState.isDragging ? "none" : "auto", | ||
| }} | ||
| onMouseDown={(e) => handleTextMouseDown(e, element, track)} | ||
| > | ||
| <div | ||
| className="px-2 py-1 rounded relative" | ||
| style={{ | ||
| fontSize: `${fontSize}px`, | ||
| fontFamily, | ||
| color, | ||
| backgroundColor, | ||
| textAlign: textAlign as any, | ||
| fontWeight, | ||
| fontStyle, | ||
| textDecoration, | ||
| whiteSpace: "nowrap", | ||
| userSelect: "none", | ||
| cursor: "grab", | ||
| }} | ||
| > | ||
| {content} | ||
|
|
||
| {/* Resize handles for text */} | ||
| <div className="absolute -top-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-nw-resize opacity-0 group-hover:opacity-100 transition-opacity" | ||
| onMouseDown={(e) => handleResizeStart(e, element, track, "nw")} /> | ||
| <div className="absolute -top-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-ne-resize opacity-0 group-hover:opacity-100 transition-opacity" | ||
| onMouseDown={(e) => handleResizeStart(e, element, track, "ne")} /> | ||
| <div className="absolute -bottom-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-sw-resize opacity-0 group-hover:opacity-100 transition-opacity" | ||
| onMouseDown={(e) => handleResizeStart(e, element, track, "sw")} /> | ||
| <div className="absolute -bottom-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-se-resize opacity-0 group-hover:opacity-100 transition-opacity" | ||
| onMouseDown={(e) => handleResizeStart(e, element, track, "se")} /> | ||
|
|
||
| {/* Rotation handle for text */} | ||
| <div className="absolute -top-8 left-1/2 transform -translate-x-1/2 w-6 h-6 bg-primary rounded-full cursor-grab opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center" | ||
| onMouseDown={(e) => handleRotationStart(e, element, track)}> | ||
| <div className="w-4 h-4 text-white text-xs">⟲</div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drag container is a non-interactive div with mouse handlers; use a button for a11y
Per guidelines, avoid event handlers on non-interactive elements. Make the draggable overlay focusable and keyboard-discernible.
- <div
+ <button
key={element.id}
className="absolute cursor-move select-none group"
style={{
left: screenX,
top: screenY,
transform: `translate(-50%, -50%) rotate(${rotation}deg)`,
opacity,
pointerEvents: dragState.isDragging ? "none" : "auto",
}}
onMouseDown={(e) => handleTextMouseDown(e, element, track)}
+ type="button"
+ aria-label="Drag text element"
>
<div
className="px-2 py-1 rounded relative"
style={{
@@
}}
>
{content}
{/* Resize handles for text */}
- <div className="absolute -top-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-nw-resize opacity-0 group-hover:opacity-100 transition-opacity"
- onMouseDown={(e) => handleResizeStart(e, element, track, "nw")} />
- <div className="absolute -top-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-ne-resize opacity-0 group-hover:opacity-100 transition-opacity"
- onMouseDown={(e) => handleResizeStart(e, element, track, "ne")} />
- <div className="absolute -bottom-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-sw-resize opacity-0 group-hover:opacity-100 transition-opacity"
- onMouseDown={(e) => handleResizeStart(e, element, track, "sw")} />
- <div className="absolute -bottom-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-se-resize opacity-0 group-hover:opacity-100 transition-opacity"
- onMouseDown={(e) => handleResizeStart(e, element, track, "se")} />
+ <button type="button" aria-label="Resize from top-left" className="absolute -top-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-nw-resize opacity-0 group-hover:opacity-100 transition-opacity" onMouseDown={(e) => handleResizeStart(e, element, track, "nw")} />
+ <button type="button" aria-label="Resize from top-right" className="absolute -top-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-ne-resize opacity-0 group-hover:opacity-100 transition-opacity" onMouseDown={(e) => handleResizeStart(e, element, track, "ne")} />
+ <button type="button" aria-label="Resize from bottom-left" className="absolute -bottom-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-sw-resize opacity-0 group-hover:opacity-100 transition-opacity" onMouseDown={(e) => handleResizeStart(e, element, track, "sw")} />
+ <button type="button" aria-label="Resize from bottom-right" className="absolute -bottom-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-se-resize opacity-0 group-hover:opacity-100 transition-opacity" onMouseDown={(e) => handleResizeStart(e, element, track, "se")} />
{/* Rotation handle for text */}
- <div className="absolute -top-8 left-1/2 transform -translate-x-1/2 w-6 h-6 bg-primary rounded-full cursor-grab opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center"
- onMouseDown={(e) => handleRotationStart(e, element, track)}>
+ <button type="button" aria-label="Rotate text element" className="absolute -top-8 left-1/2 transform -translate-x-1/2 w-6 h-6 bg-primary rounded-full cursor-grab opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center" onMouseDown={(e) => handleRotationStart(e, element, track)}>
<div className="w-4 h-4 text-white text-xs">⟲</div>
- </div>
+ </button>
</div>
- </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 | |
| key={element.id} | |
| className="absolute cursor-move select-none group" | |
| style={{ | |
| left: screenX, | |
| top: screenY, | |
| transform: `translate(-50%, -50%) rotate(${rotation}deg)`, | |
| opacity, | |
| pointerEvents: dragState.isDragging ? "none" : "auto", | |
| }} | |
| onMouseDown={(e) => handleTextMouseDown(e, element, track)} | |
| > | |
| <div | |
| className="px-2 py-1 rounded relative" | |
| style={{ | |
| fontSize: `${fontSize}px`, | |
| fontFamily, | |
| color, | |
| backgroundColor, | |
| textAlign: textAlign as any, | |
| fontWeight, | |
| fontStyle, | |
| textDecoration, | |
| whiteSpace: "nowrap", | |
| userSelect: "none", | |
| cursor: "grab", | |
| }} | |
| > | |
| {content} | |
| {/* Resize handles for text */} | |
| <div className="absolute -top-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-nw-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "nw")} /> | |
| <div className="absolute -top-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-ne-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "ne")} /> | |
| <div className="absolute -bottom-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-sw-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "sw")} /> | |
| <div className="absolute -bottom-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-se-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "se")} /> | |
| {/* Rotation handle for text */} | |
| <div className="absolute -top-8 left-1/2 transform -translate-x-1/2 w-6 h-6 bg-primary rounded-full cursor-grab opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center" | |
| onMouseDown={(e) => handleRotationStart(e, element, track)}> | |
| <div className="w-4 h-4 text-white text-xs">⟲</div> | |
| </div> | |
| </div> | |
| </div> | |
| ); | |
| <button | |
| key={element.id} | |
| className="absolute cursor-move select-none group" | |
| style={{ | |
| left: screenX, | |
| top: screenY, | |
| transform: `translate(-50%, -50%) rotate(${rotation}deg)`, | |
| opacity, | |
| pointerEvents: dragState.isDragging ? "none" : "auto", | |
| }} | |
| onMouseDown={(e) => handleTextMouseDown(e, element, track)} | |
| type="button" | |
| aria-label="Drag text element" | |
| > | |
| <div | |
| className="px-2 py-1 rounded relative" | |
| style={{ | |
| fontSize: `${fontSize}px`, | |
| fontFamily, | |
| color, | |
| backgroundColor, | |
| textAlign: textAlign as any, | |
| fontWeight, | |
| fontStyle, | |
| textDecoration, | |
| whiteSpace: "nowrap", | |
| userSelect: "none", | |
| cursor: "grab", | |
| }} | |
| > | |
| {content} | |
| {/* Resize handles for text */} | |
| <button | |
| type="button" | |
| aria-label="Resize from top-left" | |
| className="absolute -top-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-nw-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "nw")} | |
| /> | |
| <button | |
| type="button" | |
| aria-label="Resize from top-right" | |
| className="absolute -top-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-ne-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "ne")} | |
| /> | |
| <button | |
| type="button" | |
| aria-label="Resize from bottom-left" | |
| className="absolute -bottom-1 -left-1 w-3 h-3 bg-primary rounded-full cursor-sw-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "sw")} | |
| /> | |
| <button | |
| type="button" | |
| aria-label="Resize from bottom-right" | |
| className="absolute -bottom-1 -right-1 w-3 h-3 bg-primary rounded-full cursor-se-resize opacity-0 group-hover:opacity-100 transition-opacity" | |
| onMouseDown={(e) => handleResizeStart(e, element, track, "se")} | |
| /> | |
| {/* Rotation handle for text */} | |
| <button | |
| type="button" | |
| aria-label="Rotate text element" | |
| className="absolute -top-8 left-1/2 transform -translate-x-1/2 w-6 h-6 bg-primary rounded-full cursor-grab opacity-0 group-hover:opacity-100 transition-opacity flex items-center justify-center" | |
| onMouseDown={(e) => handleRotationStart(e, element, track)} | |
| > | |
| <div className="w-4 h-4 text-white text-xs">⟲</div> | |
| </button> | |
| </div> | |
| </button> |
🤖 Prompt for AI Agents
In apps/web/src/components/editor/preview-panel.tsx around lines 815-862, the
outer draggable container is a non-interactive div with mouse handlers — replace
or augment it to be keyboard-accessible: make it a semantic interactive element
(preferably a <button type="button">) or add role="button" and tabIndex={0},
provide an accessible label (aria-label), and wire onKeyDown to call the same
handler as onMouseDown for Enter/Space to support keyboard activation; preserve
existing styling and prevent default behavior as needed, and ensure nested
interactive controls (resize/rotate handles) still receive pointer events by not
blocking them or by stopping propagation where appropriate.
| /** | ||
| * Validates effect parameters | ||
| */ | ||
| export function validateEffectParameters(parameters: EffectParameters): { | ||
| isValid: boolean; | ||
| errors: string[]; | ||
| } { | ||
| const errors: string[] = []; | ||
|
|
||
| // Check brightness range | ||
| if ( | ||
| parameters.brightness !== undefined && | ||
| (parameters.brightness < -100 || parameters.brightness > 100) | ||
| ) { | ||
| errors.push("Brightness must be between -100 and 100"); | ||
| } | ||
|
|
||
| // Check contrast range | ||
| if ( | ||
| parameters.contrast !== undefined && | ||
| (parameters.contrast < -100 || parameters.contrast > 100) | ||
| ) { | ||
| errors.push("Contrast must be between -100 and 100"); | ||
| } | ||
|
|
||
| // Check saturation range | ||
| if ( | ||
| parameters.saturation !== undefined && | ||
| (parameters.saturation < -100 || parameters.saturation > 100) | ||
| ) { | ||
| errors.push("Saturation must be between -100 and 100"); | ||
| } | ||
|
|
||
| // Check hue range | ||
| if ( | ||
| parameters.hue !== undefined && | ||
| (parameters.hue < -180 || parameters.hue > 180) | ||
| ) { | ||
| errors.push("Hue must be between -180 and 180"); | ||
| } | ||
|
|
||
| // Check blur range | ||
| if ( | ||
| parameters.blur !== undefined && | ||
| (parameters.blur < 0 || parameters.blur > 50) | ||
| ) { | ||
| errors.push("Blur must be between 0 and 50"); | ||
| } | ||
|
|
||
| // Check percentage-based parameters | ||
| const percentageParams = [ | ||
| "sepia", | ||
| "grayscale", | ||
| "invert", | ||
| "vintage", | ||
| "dramatic", | ||
| "warm", | ||
| "cool", | ||
| "cinematic", | ||
| "vignette", | ||
| "grain", | ||
| "sharpen", | ||
| ]; | ||
|
|
||
| percentageParams.forEach((param) => { | ||
| const value = parameters[param as keyof EffectParameters]; | ||
| if (value !== undefined && (value < 0 || value > 100)) { | ||
| errors.push(`${param} must be between 0 and 100`); | ||
| } | ||
| }); | ||
|
|
||
| // Check pixelate range | ||
| if ( | ||
| parameters.pixelate !== undefined && | ||
| (parameters.pixelate < 1 || parameters.pixelate > 50) | ||
| ) { | ||
| errors.push("Pixelate must be between 1 and 50"); | ||
| } | ||
|
|
||
| return { | ||
| isValid: errors.length === 0, | ||
| errors, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate gamma range as defined in types
EffectParameters includes gamma (0.1–3.0), but validation omits it.
@@
// Check hue range
@@
// Check blur range
@@
// Check percentage-based parameters
@@
percentageParams.forEach((param) => {
@@
});
+ // Check gamma range
+ if (
+ parameters.gamma !== undefined &&
+ (parameters.gamma < 0.1 || parameters.gamma > 3.0)
+ ) {
+ errors.push("Gamma must be between 0.1 and 3.0");
+ }
+
// Check pixelate range📝 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.
| /** | |
| * Validates effect parameters | |
| */ | |
| export function validateEffectParameters(parameters: EffectParameters): { | |
| isValid: boolean; | |
| errors: string[]; | |
| } { | |
| const errors: string[] = []; | |
| // Check brightness range | |
| if ( | |
| parameters.brightness !== undefined && | |
| (parameters.brightness < -100 || parameters.brightness > 100) | |
| ) { | |
| errors.push("Brightness must be between -100 and 100"); | |
| } | |
| // Check contrast range | |
| if ( | |
| parameters.contrast !== undefined && | |
| (parameters.contrast < -100 || parameters.contrast > 100) | |
| ) { | |
| errors.push("Contrast must be between -100 and 100"); | |
| } | |
| // Check saturation range | |
| if ( | |
| parameters.saturation !== undefined && | |
| (parameters.saturation < -100 || parameters.saturation > 100) | |
| ) { | |
| errors.push("Saturation must be between -100 and 100"); | |
| } | |
| // Check hue range | |
| if ( | |
| parameters.hue !== undefined && | |
| (parameters.hue < -180 || parameters.hue > 180) | |
| ) { | |
| errors.push("Hue must be between -180 and 180"); | |
| } | |
| // Check blur range | |
| if ( | |
| parameters.blur !== undefined && | |
| (parameters.blur < 0 || parameters.blur > 50) | |
| ) { | |
| errors.push("Blur must be between 0 and 50"); | |
| } | |
| // Check percentage-based parameters | |
| const percentageParams = [ | |
| "sepia", | |
| "grayscale", | |
| "invert", | |
| "vintage", | |
| "dramatic", | |
| "warm", | |
| "cool", | |
| "cinematic", | |
| "vignette", | |
| "grain", | |
| "sharpen", | |
| ]; | |
| percentageParams.forEach((param) => { | |
| const value = parameters[param as keyof EffectParameters]; | |
| if (value !== undefined && (value < 0 || value > 100)) { | |
| errors.push(`${param} must be between 0 and 100`); | |
| } | |
| }); | |
| // Check pixelate range | |
| if ( | |
| parameters.pixelate !== undefined && | |
| (parameters.pixelate < 1 || parameters.pixelate > 50) | |
| ) { | |
| errors.push("Pixelate must be between 1 and 50"); | |
| } | |
| return { | |
| isValid: errors.length === 0, | |
| errors, | |
| }; | |
| } | |
| percentageParams.forEach((param) => { | |
| const value = parameters[param as keyof EffectParameters]; | |
| if (value !== undefined && (value < 0 || value > 100)) { | |
| errors.push(`${param} must be between 0 and 100`); | |
| } | |
| }); | |
| // Check gamma range | |
| if ( | |
| parameters.gamma !== undefined && | |
| (parameters.gamma < 0.1 || parameters.gamma > 3.0) | |
| ) { | |
| errors.push("Gamma must be between 0.1 and 3.0"); | |
| } | |
| // Check pixelate range | |
| if ( | |
| parameters.pixelate !== undefined && | |
| (parameters.pixelate < 1 || parameters.pixelate > 50) | |
| ) { | |
| errors.push("Pixelate must be between 1 and 50"); | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/lib/effects-utils.ts around lines 170 to 253, the validator
omits checking the gamma parameter declared on EffectParameters; add a check
that if parameters.gamma !== undefined and (parameters.gamma < 0.1 ||
parameters.gamma > 3.0) then push an error like "Gamma must be between 0.1 and
3.0". Place this new validation near the other numeric range checks (e.g., after
hue/blur or before percentageParams) so gamma is validated and included in the
returned errors/isValid logic.
| removeEffect: (effectId: string) => void; | ||
| toggleEffect: (effectId: string) => void; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add duplicateEffect to support the UI action
Implement a store-level duplication to pair with the Properties panel.
@@
toggleEffect: (effectId: string) => void;
+ duplicateEffect: (effectId: string) => void;
@@
toggleEffect: (effectId) => {
@@
},
+
+ duplicateEffect: (effectId) => {
+ set((state) => {
+ const original = state.appliedEffects.find((e) => e.id === effectId);
+ if (!original) return {};
+ const copy: EffectInstance = {
+ ...original,
+ id: generateUUID(),
+ name: `${original.name} (Copy)`,
+ createdAt: new Date(),
+ updatedAt: new Date(),
+ };
+ toast.success("Effect duplicated");
+ return {
+ appliedEffects: [...state.appliedEffects, copy],
+ selectedEffect: copy,
+ };
+ });
+ },Also applies to: 352-364
| id: generateUUID(), | ||
| name: preset.name, | ||
| effectType: preset.id as EffectType, | ||
| parameters: { ...preset.parameters }, | ||
| elementId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrong type stored in effectType; map preset id → EffectType instead of unsafe cast
preset.id like "brightness-up" is not an EffectType (e.g., "brightness"). Avoid the cast and introduce a mapping to keep types correct.
+// Map preset ids to canonical effect types
+const PRESET_TO_EFFECT_TYPE: Record<string, EffectType> = {
+ "brightness-up": "brightness",
+ "brightness-down": "brightness",
+ "contrast-up": "contrast",
+ "saturation-up": "saturation",
+ "saturation-down": "saturation",
+ sepia: "sepia",
+ grayscale: "grayscale",
+ invert: "invert",
+ vintage: "vintage",
+ dramatic: "dramatic",
+ warm: "warm",
+ cool: "cool",
+ cinematic: "cinematic",
+ "gaussian-blur": "blur",
+ "motion-blur": "blur",
+ vignette: "vignette",
+ grain: "grain",
+ sharpen: "sharpen",
+ emboss: "emboss",
+ edge: "edge",
+ pixelate: "pixelate",
+};
@@
- effectType: preset.id as EffectType,
+ effectType: PRESET_TO_EFFECT_TYPE[preset.id],📝 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.
| id: generateUUID(), | |
| name: preset.name, | |
| effectType: preset.id as EffectType, | |
| parameters: { ...preset.parameters }, | |
| elementId, | |
| // Map preset ids to canonical effect types | |
| const PRESET_TO_EFFECT_TYPE: Record<string, EffectType> = { | |
| "brightness-up": "brightness", | |
| "brightness-down": "brightness", | |
| "contrast-up": "contrast", | |
| "saturation-up": "saturation", | |
| "saturation-down": "saturation", | |
| sepia: "sepia", | |
| grayscale: "grayscale", | |
| invert: "invert", | |
| vintage: "vintage", | |
| dramatic: "dramatic", | |
| warm: "warm", | |
| cool: "cool", | |
| cinematic: "cinematic", | |
| "gaussian-blur": "blur", | |
| "motion-blur": "blur", | |
| vignette: "vignette", | |
| grain: "grain", | |
| sharpen: "sharpen", | |
| emboss: "emboss", | |
| edge: "edge", | |
| pixelate: "pixelate", | |
| }; | |
| … // other surrounding code … | |
| id: generateUUID(), | |
| name: preset.name, | |
| effectType: PRESET_TO_EFFECT_TYPE[preset.id], | |
| parameters: { ...preset.parameters }, | |
| elementId, | |
| … // other surrounding code … |
🤖 Prompt for AI Agents
In apps/web/src/stores/effects-store.ts around lines 289 to 293, the code
unsafely casts preset.id to EffectType; instead add a mapping from preset.id
values (e.g. "brightness-up", "brightness-down", etc.) to the canonical
EffectType values (e.g. "brightness"), use that lookup to set effectType, and
handle unknown keys with a safe fallback or throw to satisfy typing; update the
creation to assign effectType = presetIdToEffectTypeMap[preset.id] (with
appropriate type guard/validation) rather than using "as EffectType".
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Test A: Video Effects Functionality
Test B: Interactive Text Manipulation
Test Configuration:
Screenshots (if applicable)
Effects Tab
Text Controls on Render Tab
Checklist:
Additional context
This feature significantly enhances OpenCut's video editing capabilities by providing professional-grade effects and intuitive element manipulation. The implementation follows the existing codebase patterns and integrates seamlessly with the current architecture.
Summary by CodeRabbit