Fix video duration metadata #917
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughConsolidates video metadata handling across desktop, web, and backend: introduces structured S3VideoMeta (durationInSecs, width, height, fps), refactors upload flows to typed presigned requests with per-part presign in multipart, adds upgrade gating by duration, surfaces duration as top-level DB fields, and updates UI to read numeric durations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant DesktopApp
participant FFmpeg as build_video_meta
participant API as /api/desktop/create
participant DB
User->>DesktopApp: Finish recording / Export
DesktopApp->>FFmpeg: Analyze file (duration, width, height, fps)
FFmpeg-->>DesktopApp: S3VideoMeta
DesktopApp->>API: GET /create?durationInSecs&width&height&fps
API->>DB: Insert video (duration, width, height, fps)
DB-->>API: Video id + upload config
API-->>DesktopApp: S3 upload config
sequenceDiagram
autonumber
participant DesktopApp
participant API as /api/upload/signed
participant S3
participant API2 as /api/upload/multipart/complete
participant DB
rect rgb(240,248,255)
note over DesktopApp: Multipart upload
loop For each part
DesktopApp->>API: Presign { videoId, subpath, method: Put }
API-->>DesktopApp: Presigned URL
DesktopApp->>S3: PUT part (Content-MD5)
S3-->>DesktopApp: ETag
end
end
DesktopApp->>API2: Complete { parts[], meta? }
API2->>DB: Update duration/width/height/fps (if provided)
DB-->>API2: OK
API2-->>DesktopApp: Upload complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (5)
✨ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/desktop/src/utils/createPresets.ts (2)
33-35: Off-by-one: default index is set past the end after push.
store.presets.lengthafter the push is one past the last valid index. This will persist an out-of-range default.Apply this diff:
- store.default = preset.default ? store.presets.length : store.default; + store.default = preset.default ? store.presets.length - 1 : store.default;
39-44: Deleting a preset can leavedefaultout-of-range or misaligned.
- If you delete an item before the current default, the default should decrement to keep pointing at the same logical item.
- If you delete the last item or the current default, clamp or select a sensible fallback.
- Current logic only reacts to “deleting the last item” via the removed index, not the current
default, and can leavedefault> max index.Apply this diff for robust handling:
- store.presets.splice(index, 1); - store.default = - index > store.presets.length - 1 - ? store.presets.length - 1 - : store.default; + store.presets.splice(index, 1); + if (store.presets.length === 0) { + store.default = null; + } else if (store.default != null) { + if (store.default === index) { + // deleted the default; pick the next item if available, otherwise clamp + store.default = Math.min(index, store.presets.length - 1); + } else if (store.default > index) { + // shift left to keep pointing at the same logical item + store.default = store.default - 1; + } else if (store.default > store.presets.length - 1) { + // just in case, clamp to last index + store.default = store.presets.length - 1; + } + }apps/web/components/VideoThumbnail.tsx (1)
19-37: Fix rounding edge case: “60 secs” for ~59.6 sec clipsUsing
Math.ceil(momentDuration.asSeconds() % 60)can yield 60 seconds when the duration is just under the next minute (e.g., 59.6 sec → “60 secs”). Compute using rounded total seconds, then derive h/m/s to avoid “60 secs” outputs.Also, drop the console.log to avoid noisy renders.
Apply:
-const formatDuration = (durationSecs: number) => { - console.log("FORMAT DURATION", durationSecs); - - const momentDuration = moment.duration(durationSecs, "seconds"); - - const totalHours = Math.floor(momentDuration.asHours()); - const totalMinutes = Math.floor(momentDuration.asMinutes()); - const remainingSeconds = Math.ceil(momentDuration.asSeconds() % 60); // Use ceil to avoid 0 secs - - if (totalHours > 0) { - return `${totalHours} hr${totalHours > 1 ? "s" : ""}`; - } else if (totalMinutes > 0) { - return `${totalMinutes} min${totalMinutes > 1 ? "s" : ""}`; - } else if (remainingSeconds > 0) { - return `${remainingSeconds} sec${remainingSeconds !== 1 ? "s" : ""}`; - } else { - return "< 1 sec"; // For very short durations - } -}; +const formatDuration = (durationSecs: number) => { + const totalSeconds = Math.max(0, Math.round(durationSecs)); + const hours = Math.floor(totalSeconds / 3600); + const minutes = Math.floor((totalSeconds % 3600) / 60); + const seconds = totalSeconds % 60; + + if (hours > 0) return `${hours} hr${hours > 1 ? "s" : ""}`; + if (minutes > 0) return `${minutes} min${minutes > 1 ? "s" : ""}`; + if (seconds > 0) return `${seconds} sec${seconds !== 1 ? "s" : ""}`; + return "< 1 sec"; +};If
momentis no longer used elsewhere in this file, you can safely remove the import to reduce bundle size.apps/desktop/src-tauri/src/upload.rs (1)
657-676: Possible out-of-bounds access on uploaded_parts[0] during header re-uploadIf the first chunk was never uploaded (edge timing), indexing
uploaded_parts[0]will panic. Add a guard.- if realtime_is_done.unwrap_or(false) { + if realtime_is_done.unwrap_or(false) && !uploaded_parts.is_empty() { info!("realtime video done, uploading header chunk"); let part = Self::upload_chunk( &app, &client, &file_path, s3_config.id(), &upload_id, &mut 1, &mut 0, uploaded_parts[0].size as u64, )apps/web/lib/folder.ts (1)
180-186: Missing videos.duration in GROUP BY can break under ONLY_FULL_GROUP_BYYou select
videos.durationalongside aggregates but do not group by it. In MySQL with ONLY_FULL_GROUP_BY (default), this will error..groupBy( videos.id, videos.ownerId, videos.name, videos.createdAt, videos.metadata, + videos.duration, users.name, )
🧹 Nitpick comments (20)
apps/desktop/src/utils/createPresets.ts (3)
29-31: Prefer stripping the property (no ts suppression) over assigning undefined.Assigning
undefinedkeeps the key present on the object; if downstream code checks key presence, this can still be truthy via"timeline" in config. You can drop the property at runtime and eliminate the type suppression by excluding it during destructuring.Apply this diff:
- const config = { ...preset.config }; - // @ts-expect-error we reeeally don't want the timeline in the preset - config.timeline = undefined; + const { timeline: _ignoredTimeline, ...config } = preset.config as ProjectConfiguration;
45-48: Optional: GuardsetDefaultagainst out-of-bounds indices.A quick bounds check avoids persisting invalid state and aligns with the deletion safeguards above.
Example:
- updatePresets((store) => { - store.default = index; - }), + updatePresets((store) => { + if (index >= 0 && index < store.presets.length) { + store.default = index; + } + }),
49-52: Optional: GuardrenamePresetagainst out-of-bounds indices.Prevents silent no-ops or runtime errors if called with an invalid index.
Example:
- updatePresets((store) => { - store.presets[index].name = name; - }), + updatePresets((store) => { + if (index >= 0 && index < store.presets.length) { + store.presets[index].name = name; + } + }),apps/desktop/src/routes/editor/ShareButton.tsx (1)
118-118: Console logging is fine; add context for easier triageAdding
console.error(error)improves debugging. Consider adding context to the log message so it’s searchable in aggregated logs.Apply:
- console.error(error); + console.error("[ShareButton] upload failed", error);Optionally, guard noisy logs to dev builds only.
apps/desktop/src/routes/editor/ExportDialog.tsx (1)
363-363: Mirror logging improvement: add context to error logsSame feedback as ShareButton: include a short prefix to make logs searchable.
Apply:
- console.error(error); + console.error("[ExportDialog] upload failed", error);apps/web/components/VideoThumbnail.tsx (1)
112-116: Confirm desired behavior: hides “< 1 sec” badgesThe condition
{videoDuration && (...)}suppresses the badge for0seconds. If you intend to display “< 1 sec” for very short clips, change the guard to show non-negative numbers:- {videoDuration && ( + {(typeof videoDuration === "number") && videoDuration >= 0 && (If hiding zero-length clips is intentional, ignore this.
apps/desktop/src-tauri/src/upload.rs (4)
148-163: Progress event lags by one chunk; increment before emittingYou emit using the previous accumulated size, so UI progress lags one chunk. Increment first, then emit.
- let progress_stream = reader_stream.inspect({ + let progress_stream = reader_stream.inspect({ let app = app.clone(); move |chunk| { - if bytes_uploaded > 0 { - let _ = UploadProgress { - progress: bytes_uploaded as f64 / total_size as f64, - } - .emit(&app); - } - if let Ok(chunk) = chunk { bytes_uploaded += chunk.len(); } + + if bytes_uploaded > 0 { + let _ = UploadProgress { + progress: bytes_uploaded as f64 / total_size as f64, + } + .emit(&app); + } } });
301-308: Avoid manual query-string concatenation for meta; use RequestBuilder.query to handle URL encoding and float formattingBuilding the URL by string concatenation risks encoding issues (e.g., locales, precision, reserved characters). Prefer passing meta as query parameters on the request builder.
For example (outside this block), build params and call query(...) instead of pushing to the URL string:
let mut params: Vec<(&str, String)> = Vec::new(); if let Some(name) = name { params.push(("name", name)); } if let Some(m) = meta { params.push(("durationInSecs", m.duration_in_secs.to_string())); params.push(("width", m.width.to_string())); params.push(("height", m.height.to_string())); if let Some(fps) = m.fps { params.push(("fps", fps.to_string())); } } let response = app .authed_api_request(s3_config_url, |client, url| client.get(url).query(¶ms)) .await .map_err(|e| format!("Failed to send request to Next.js handler: {e}"))?;
392-399: Handle unknown/invalid duration values from ffmpeg more defensively
input.duration()can be negative/unknown. Consider falling back to stream-based duration (using stream.duration and time_base) or 0.0 when unknown to avoid propagating bogus values. Also consider rounding to a reasonable precision to avoid long float strings over the wire.Would you like me to provide a robust fallback implementation that derives duration from the stream time base when
input.duration()is invalid?
994-998: Avoid nested Option for fps in completion payload
metadata.as_ref().map(|m| m.fps)yieldsOption<Option<f32>>. Useand_thento flatten.- "fps": metadata.as_ref().map(|m| m.fps), + "fps": metadata.as_ref().and_then(|m| m.fps),apps/desktop/src-tauri/src/lib.rs (2)
1136-1138: Minor: avoid to_string within format!
erris already a String; the extra to_string is redundant.- let metadata = build_video_meta(&output_path) - .map_err(|err| format!("Error getting output video meta: {}", err.to_string()))?; + let metadata = build_video_meta(&output_path) + .map_err(|err| format!("Error getting output video meta: {err}"))?;
1139-1142: Magic number 300.0 — consider extracting a named constantImproves readability and centralizes plan gating behavior.
Example:
const FREE_MAX_DURATION_SECS: f64 = 300.0; if !auth.is_upgraded() && metadata.duration_in_secs > FREE_MAX_DURATION_SECS { return Ok(UploadResult::UpgradeRequired); }crates/project/src/configuration.rs (1)
438-441: Public accessor is good; add a guard for zero/negative timescaleA debug guard prevents accidental divide-by-zero from malformed configs. Optional but cheap.
- pub fn duration(&self) -> f64 { - (self.end - self.start) / self.timescale - } + pub fn duration(&self) -> f64 { + debug_assert!(self.timescale > 0.0, "timescale must be > 0"); + (self.end - self.start) / self.timescale + }apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (2)
39-64: Allow null duration to match DB nullability
videos.durationis nullable at the DB level; making this prop acceptnullavoids accidental narrowing and future runtime/type friction.- duration?: number; + duration?: number | null;
433-433: Null-safe forwarding of duration to VideoThumbnailForward
undefinedwhendurationisnullto match typicalnumber | undefinedprop shapes downstream.- videoDuration={cap.duration} + videoDuration={cap.duration ?? undefined}packages/database/schema.ts (1)
238-243: Consider float fps and unsigned dimensions
- Many sources produce fractional frame rates (23.976, 29.97, 59.94). Storing fps as
intloses fidelity.- Width/height can’t be negative; marking them
unsignedtightens constraints.- duration: float("duration"), - width: int("width"), - height: int("height"), - fps: int("fps"), + duration: float("duration"), + width: int("width", { unsigned: true }), + height: int("height", { unsigned: true }), + fps: float("fps"),apps/web/utils/zod.ts (1)
3-7: Optional: provide an integer variant for width/height/fpsWidth, height, and fps are integer DB columns. Consider an int-focused helper to avoid fractional writes:
export const intStringOrNumberOptional = z .union([z.number(), z.string().trim().min(1)]) .transform((v) => (typeof v === "string" ? Number(v) : v)) .refine( (v) => v === undefined || (Number.isFinite(v) && Number.isInteger(v)), { message: "Must be an integer" }, ) .optional();Then use this for width/height/fps.
apps/desktop/src-tauri/src/export.rs (1)
96-104: Avoid panic: replace unwrap() with proper error propagationRecordingMeta::load_for_project(&path).unwrap() will crash the command on read/parse errors. Align with the surrounding error handling and propagate as String.
- let meta = RecordingMeta::load_for_project(&path).unwrap(); + let meta = RecordingMeta::load_for_project(&path).map_err(|e| e.to_string())?;apps/web/app/api/desktop/[...route]/video.ts (2)
49-53: Upgrade gate: guard is fine; ensure coercion can’t bypassWith the helper fix (trim + finite), values like " " won’t coerce to 0 and bypass gating. Without the fix, whitespace could incorrectly pass.
No code change here if you adopt the zod fix.
120-124: Consider integer validation for width/height/fpsThese are integer columns; using a dedicated int helper avoids fractional inputs:
- width: intStringOrNumberOptional
- height: intStringOrNumberOptional
- fps: intStringOrNumberOptional
This prevents truncation by the DB.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
apps/desktop/src-tauri/src/export.rs(1 hunks)apps/desktop/src-tauri/src/lib.rs(4 hunks)apps/desktop/src-tauri/src/recording.rs(1 hunks)apps/desktop/src-tauri/src/upload.rs(11 hunks)apps/desktop/src/routes/editor/ExportDialog.tsx(1 hunks)apps/desktop/src/routes/editor/ShareButton.tsx(1 hunks)apps/desktop/src/utils/createPresets.ts(1 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(2 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(1 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx(2 hunks)apps/web/app/api/desktop/[...route]/video.ts(3 hunks)apps/web/app/api/upload/[...route]/multipart.ts(3 hunks)apps/web/app/api/upload/[...route]/signed.ts(6 hunks)apps/web/components/VideoThumbnail.tsx(2 hunks)apps/web/lib/folder.ts(1 hunks)apps/web/utils/zod.ts(1 hunks)crates/project/src/configuration.rs(1 hunks)packages/database/helpers.ts(1 hunks)packages/database/index.ts(2 hunks)packages/database/schema.ts(1 hunks)packages/database/types/metadata.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
apps/desktop/src-tauri/src/export.rs (2)
apps/desktop/src-tauri/src/lib.rs (1)
get_video_metadata(942-1009)crates/project/src/meta.rs (5)
path(91-93)path(251-253)path(284-286)load_for_project(94-100)project_config(109-132)
apps/web/lib/folder.ts (1)
packages/database/schema.ts (1)
videos(231-277)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/upload.rs (1)
build_video_meta(380-400)
apps/web/app/(org)/dashboard/caps/page.tsx (1)
packages/database/schema.ts (1)
videos(231-277)
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx (1)
packages/database/schema.ts (1)
videos(231-277)
apps/web/app/api/upload/[...route]/multipart.ts (3)
apps/web/utils/zod.ts (1)
stringOrNumberOptional(4-7)packages/database/index.ts (2)
db(30-35)updateIfDefined(38-39)packages/database/schema.ts (1)
videos(231-277)
apps/web/app/api/upload/[...route]/signed.ts (3)
apps/web/utils/zod.ts (1)
stringOrNumberOptional(4-7)packages/database/index.ts (2)
db(30-35)updateIfDefined(38-39)packages/database/schema.ts (1)
videos(231-277)
apps/web/app/api/desktop/[...route]/video.ts (1)
apps/web/utils/zod.ts (1)
stringOrNumberOptional(4-7)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/routes/editor/context.ts (1)
meta(308-310)crates/editor/src/editor_instance.rs (1)
meta(106-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
packages/database/helpers.ts (2)
1-1: LGTM: Import prepares for Drizzle SQL helpersThe new
sqlimport aligns with the PR’s SQL-expression updates. No issues here.
22-26: No duplicateVideoMetadatatype found – ignore removal/rename suggestionOur searches confirm that the only
VideoMetadatadeclaration lives in packages/database/helpers.ts (lines 22–26). There is noVideoMetadatatype (alias or interface) in packages/database/types/metadata.ts or anywhere else, so there’s no collision or drift risk.Likely an incorrect or invalid review comment.
apps/web/components/VideoThumbnail.tsx (1)
16-16: Prop type change to number looks goodSwitching
videoDurationtonumberaligns with the new schema and upstream changes (e.g., CapCard). No issues here.packages/database/types/metadata.ts (2)
37-37: Good hardening: disallow extra keys on Space metadata
[key: string]: neveris an effective way to prevent accidental writes of arbitrary keys to Space metadata at compile time.
44-45: Good hardening: disallow extra keys on User metadataSame here—this prevents shape drift in user metadata and enforces exactness on the type.
apps/desktop/src-tauri/src/upload.rs (1)
79-88: S3VideoMeta shape and serde config look correct
- duration_in_secs correctly renamed to durationInSecs for API compatibility
- fps is optional and skipped when None
Looks good.
apps/desktop/src-tauri/src/recording.rs (1)
766-775: Passing full video metadata to upload_video is correctSwitching from duration-only to full metadata here is aligned with the new API and avoids duplicate server-side probes.
apps/desktop/src-tauri/src/lib.rs (1)
1167-1168: End-to-end meta propagation looks goodPassing
Some(metadata.clone())to create_or_get_video andSome(metadata)to upload_video matches the new typed flow. Good consistency.Also applies to: 1181-1182
apps/web/lib/folder.ts (1)
148-149: Good: select duration at the top levelIncluding
duration: videos.durationaligns with the schema change and unblocks UI components that need a numeric duration.apps/web/app/(org)/dashboard/caps/page.tsx (1)
156-156: LGTM: duration surfaced correctly in the projectionSelecting
videos.durationhere aligns with the new column and is functionally dependent onvideos.id(present in GROUP BY), so no GROUP BY changes are required on MySQL 8/PlanetScale.apps/desktop/src-tauri/src/export.rs (1)
96-104: LGTM: unified metadata source and accurate duration fallbackUsing get_video_metadata() and summing timeline segment durations via segment.duration() improves correctness and removes the raw_duration fallback. Looks good.
apps/web/app/api/upload/[...route]/multipart.ts (1)
292-307: LGTM: revalidation scoped by videoId derived from fileKeyTriggering page revalidation by videoIdFromFileKey keeps UI consistent after upload completion. Looks good.
apps/web/app/api/upload/[...route]/signed.ts (1)
120-123: LGTM: presigned metadata simplified and consistentIncluding a single duration metadata key for both POST and PUT flows is a good simplification and matches the new database fields. Looks good.
Also applies to: 133-134
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/utils/zod.ts (1)
5-12: “Optional” schema still coerces blanks/whitespace to 0; make it truly optional and safeWith the current preprocess, "" or " " becomes 0 (Number(" ") === 0), slipping through as a valid number and defeating validation/gating. Also, NaN/Infinity aren’t rejected explicitly.
Harden the schema to:
- accept numbers or trimmed non-empty strings
- convert strings to numbers only after trimming
- reject NaN/Infinity
- be truly optional
-export const stringOrNumberOptional = z.preprocess((val) => { - if (val === null || val === undefined) return val; - if (typeof val === "string") { - const n = Number(val); - return Number.isNaN(n) ? val : n; // let z.number() reject non-numeric strings - } - return val; // numbers pass through -}, z.number().optional()); +export const stringOrNumberOptional = z + .union([z.number(), z.string().trim().min(1)]) + .transform((v) => (typeof v === "string" ? Number(v) : v)) + .refine((v) => Number.isFinite(v), { message: "Must be a finite number" }) + .optional();Follow-up: this changes semantics to truly optional and rejects blanks. Ensure API callers aren’t relying on blank → 0 coercion.
Run this to find callsites and double-check expectations:
#!/bin/bash set -euo pipefail # Show usages of stringOrNumberOptional (with some context) rg -n --type=ts -C3 '\bstringOrNumberOptional\b' # Find places that immediately rely on numeric comparisons (may assume >0) rg -n --type=ts -C2 '\bstringOrNumberOptional\b.*\n.*(duration|width|height|fps)'apps/desktop/src-tauri/src/upload.rs (1)
392-399: Potential panic: unwrap() on decoder; return a proper error insteadvideo_codec.decoder().video().unwrap() will panic on unsupported/malformed streams. Map the error and propagate it.
- let video = video_codec.decoder().video().unwrap(); + let video = video_codec + .decoder() + .video() + .map_err(|e| format!("Failed to create video decoder: {e}"))?;
🧹 Nitpick comments (3)
apps/web/components/VideoThumbnail.tsx (2)
19-35: Avoid “60 secs” rounding glitch; add basic input validation in formatterUsing Math.ceil on seconds will render 59.1s as “60 secs”. Also guard against negative/non-finite inputs to prevent odd labels.
Apply this diff:
-const formatDuration = (durationSecs: number) => { - const momentDuration = moment.duration(durationSecs, "seconds"); - - const totalHours = Math.floor(momentDuration.asHours()); - const totalMinutes = Math.floor(momentDuration.asMinutes()); - const remainingSeconds = Math.ceil(momentDuration.asSeconds() % 60); // Use ceil to avoid 0 secs - - if (totalHours > 0) { - return `${totalHours} hr${totalHours > 1 ? "s" : ""}`; - } else if (totalMinutes > 0) { - return `${totalMinutes} min${totalMinutes > 1 ? "s" : ""}`; - } else if (remainingSeconds > 0) { - return `${remainingSeconds} sec${remainingSeconds !== 1 ? "s" : ""}`; - } else { - return "< 1 sec"; // For very short durations - } -}; +const formatDuration = (durationSecs: number) => { + if (!Number.isFinite(durationSecs) || durationSecs < 0) return ""; + const d = moment.duration(durationSecs, "seconds"); + + const hours = Math.floor(d.asHours()); + if (hours >= 1) return `${hours} hr${hours !== 1 ? "s" : ""}`; + + const minutes = Math.floor(d.asMinutes()); + if (minutes >= 1) return `${minutes} min${minutes !== 1 ? "s" : ""}`; + + const seconds = Math.floor(d.asSeconds() % 60); + if (seconds >= 1) return `${seconds} sec${seconds !== 1 ? "s" : ""}`; + return "< 1 sec"; +};
110-114: Don’t render duration badge for negative/NaN durationsCurrently, any non-zero number (including negatives) renders the badge. Restore a stricter guard.
- {videoDuration && ( + {typeof videoDuration === "number" && Number.isFinite(videoDuration) && videoDuration > 0 && ( <p className="text-white leading-0 px-2 left-3 rounded-full backdrop-blur-sm absolute z-10 bottom-3 bg-black/50 text-[11px]"> {formatDuration(videoDuration)} </p> )}apps/desktop/src-tauri/src/upload.rs (1)
124-133: Compute video metadata once and reuse; avoid double ffmpeg parseYou pass meta to create_or_get_video, then compute fresh metadata again for presign. Parsing twice is unnecessary and slower on large files. Reuse the same metadata if provided; otherwise, compute once and use for both calls.
- None => create_or_get_video(app, false, Some(video_id.clone()), None, meta).await?, + None => create_or_get_video(app, false, Some(video_id.clone()), None, meta.clone()).await?, @@ - let presigned_put = presigned_s3_put( - app, - PresignedS3PutRequest { - video_id: video_id.clone(), - subpath: "result.mp4".to_string(), - method: PresignedS3PutRequestMethod::Put, - meta: Some(build_video_meta(&file_path)?), - }, - ) + // Reuse provided meta, otherwise compute once. + let meta_for_put = match meta { + Some(ref m) => Some(m.clone()), + None => Some(build_video_meta(&file_path)?), + }; + let presigned_put = presigned_s3_put( + app, + PresignedS3PutRequest { + video_id: video_id.clone(), + subpath: "result.mp4".to_string(), + method: PresignedS3PutRequestMethod::Put, + meta: meta_for_put, + }, + ) .await?;Also applies to: 121-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/upload.rs(12 hunks)apps/web/components/VideoThumbnail.tsx(2 hunks)apps/web/utils/zod.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx (1)
213-221: Fix ONLY_FULL_GROUP_BY: add videos.duration to GROUP BY in fetchSpaceVideosYou select
duration: videos.duration(Line 200) but don’t includevideos.durationin.groupBy(...). This will error on MySQL with ONLY_FULL_GROUP_BY enabled. Add it alongside the other grouped columns.Apply this diff:
.groupBy( videos.id, videos.ownerId, videos.name, videos.createdAt, videos.metadata, - users.name, + users.name, + videos.duration, )apps/web/app/api/upload/[...route]/multipart.ts (1)
6-6: Fix missing import and drop unused symbol from drizzle-orm
andis used later in the.where(...)clause (Line 291) but is not imported. Conversely,sqlis imported but unused. This will fail at runtime/compile.Apply this diff:
-import { eq, sql } from "drizzle-orm"; +import { and, eq } from "drizzle-orm";
🧹 Nitpick comments (6)
apps/web/app/api/upload/[...route]/signed.ts (2)
10-10: Remove unusedsqlimport
sqlisn’t referenced in this module. Keeping it will trigger lint warnings.Apply:
-import { and, eq, sql } from "drizzle-orm"; +import { and, eq } from "drizzle-orm";
120-123: Don’t drop legitimate zero values when emitting S3 metadataUsing a truthiness check will treat
0as “missing,” writing an empty string instead of "0". Switch to an undefined check.Apply:
- "x-amz-meta-duration": durationInSecs - ? durationInSecs.toString() - : "", + "x-amz-meta-duration": + durationInSecs !== undefined ? durationInSecs.toString() : "",And for PUT metadata:
- duration: durationInSecs ? durationInSecs.toString() : "", + duration: + durationInSecs !== undefined ? durationInSecs.toString() : "",Also applies to: 133-134
apps/desktop/src-tauri/src/upload.rs (4)
79-88: S3VideoMeta shape looks good; remove redundant serde rename on duration.You already have rename_all = "camelCase", so the explicit rename on duration_in_secs is unnecessary.
#[derive(Serialize, Debug, Clone)] #[serde(rename_all = "camelCase")] pub struct S3VideoMeta { - #[serde(rename = "durationInSecs")] - pub duration_in_secs: f64, + pub duration_in_secs: f64, pub width: u32, pub height: u32, #[serde(skip_serializing_if = "Option::is_none")] pub fps: Option<f32>, }
114-115: Avoid re-parsing video metadata; compute once and reuse.build_video_meta is invoked again for presign even if the caller provided meta. Derive it once (or use the provided one) and reuse for both create_or_get_video and presigned_s3_put.
pub async fn upload_video( app: &AppHandle, video_id: String, file_path: PathBuf, existing_config: Option<S3UploadMeta>, screenshot_path: Option<PathBuf>, - meta: Option<S3VideoMeta>, + meta: Option<S3VideoMeta>, ) -> Result<UploadedVideo, String> { println!("Uploading video {video_id}..."); let client = reqwest::Client::new(); - let s3_config = match existing_config { - Some(config) => config, - None => create_or_get_video(app, false, Some(video_id.clone()), None, meta).await?, - }; + // Compute metadata once (or reuse what was provided) + let final_meta: Option<S3VideoMeta> = match meta { + Some(m) => Some(m), + None => Some(build_video_meta(&file_path)?), + }; + let s3_config = match existing_config { + Some(config) => config, + None => { + create_or_get_video(app, false, Some(video_id.clone()), None, final_meta.clone()) + .await? + } + }; - let presigned_put = presigned_s3_put( - app, - PresignedS3PutRequest { - video_id: video_id.clone(), - subpath: "result.mp4".to_string(), - method: PresignedS3PutRequestMethod::Put, - meta: Some(build_video_meta(&file_path)?), - }, - ) - .await?; + let presigned_put = presigned_s3_put( + app, + PresignedS3PutRequest { + video_id: video_id.clone(), + subpath: "result.mp4".to_string(), + method: PresignedS3PutRequestMethod::Put, + meta: final_meta, + }, + ) + .await?;Also applies to: 121-121, 124-134
301-308: Pushing float metadata via query params: consider stability and future-proofing.Numbers in query params are fine here; just a heads-up that large precision floats may serialize with many decimals. If this ever becomes noisy server-side, consider rounding or switching this endpoint to accept a JSON body (POST) to align with presigned_s3_put.
349-378: Handle non-2xx responses before JSON deserialization in presigned_s3_put.If the server returns an error with a non-JSON body, response.json() produces a generic “deserialize” error and hides the actual server message. Check status and plumb the body for better diagnostics.
let response = app .authed_api_request("/api/upload/signed", |client, url| { client.post(url).json(&body) }) .await .map_err(|e| format!("Failed to send request to Next.js handler: {e}"))?; if response.status() == StatusCode::UNAUTHORIZED { return Err("Failed to authenticate request; please log in again".into()); } + if !response.status().is_success() { + let status = response.status(); + let body = response + .text() + .await + .unwrap_or_else(|_| "<no response body>".to_string()); + return Err(format!( + "Failed to get presigned URL. Status: {status}. Body: {body}" + )); + } + let Wrapper { presigned_put_data } = response .json::<Wrapper>() .await .map_err(|e| format!("Failed to deserialize server response: {e}"))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/desktop/src-tauri/src/upload.rs(12 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx(3 hunks)apps/web/app/api/upload/[...route]/multipart.ts(3 hunks)apps/web/app/api/upload/[...route]/signed.ts(6 hunks)packages/database/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/database/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/web/app/api/upload/[...route]/signed.ts (3)
apps/web/utils/zod.ts (1)
stringOrNumberOptional(5-12)packages/database/index.ts (2)
db(30-35)updateIfDefined(38-39)packages/database/schema.ts (1)
videos(231-277)
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx (1)
packages/database/schema.ts (1)
videos(231-277)
apps/web/app/api/upload/[...route]/multipart.ts (3)
apps/web/utils/zod.ts (1)
stringOrNumberOptional(5-12)packages/database/index.ts (2)
db(30-35)updateIfDefined(38-39)packages/database/schema.ts (1)
videos(231-277)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (11)
apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx (2)
306-306: LGTM: selecting duration for org queryExposing
duration: videos.durationhere is correct and aligns with the new metadata shape.
323-331: Good catch adding duration to GROUP BY in org queryIncluding
videos.durationin.groupBy(...)here resolves ONLY_FULL_GROUP_BY issues for the organization query. Mirror this in the space query (comment above).apps/web/app/api/upload/[...route]/multipart.ts (1)
277-293: Good: owner-scoped update with partial set semanticsDeriving
videoIdToUsewith a fallback and scoping the update withand(eq(videos.id, videoIdToUse), eq(videos.ownerId, user.id))is the right call, andupdateIfDefined(...)ensures partial updates.Note: ensure the import of
andis fixed per the import comment so this compiles.apps/web/app/api/upload/[...route]/signed.ts (2)
27-31: Schema looks correct with the optional helperUsing
stringOrNumberOptionalhere is consistent and, with the hardened helper, aligns validation with partial update behavior.
152-157: Good: partial metadata update with ownership guardUsing
updateIfDefinedfor duration/width/height/fps and scoping by both video id and owner id is solid and consistent with the multipart flow.apps/desktop/src-tauri/src/upload.rs (6)
236-246: Typed presign for image upload is clear and safe.Using the unified PresignedS3PutRequest with method=Put and no meta for screenshots keeps the flow consistent.
331-340: PresignedS3PutRequest shape is solid; flattening meta keeps payload tidy.The flattened meta in the presign payload matches the API design and avoids nested objects.
341-347: Enum for method improves clarity.Explicit PresignedS3PutRequestMethod is preferable to ad-hoc strings.
390-394: Good fix: removed unwrap() on decoder and properly propagate errors.This eliminates a potential panic on malformed/unsupported streams and addresses the earlier concern.
6-6: Duration and FPS extraction logic looks correct.Using AV_TIME_BASE for container duration and mapping frame_rate() to Option is appropriate. Width/height sourced from the decoder context is standard.
Also applies to: 396-402
492-500: Multipart completion payload with flattened meta is well-structured.The new MultipartCompleteResponse<'a> with parts by reference and optional meta is clean and minimizes copies. finalize_upload integration looks correct.
Also applies to: 1001-1008
|
Needs planetscale migration before merging |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Access Control