improve: duration formatting #912
Conversation
WalkthroughAdds dynamic confirmLabel to ConfirmationDialog usages to reflect pending states across multiple delete/remove flows. Refactors cap duration display: removes on-card overlay in CapCard and introduces a videoDuration prop to VideoThumbnail, which now renders a formatted duration badge. Also removes an unused comment in a TypeScript declarations file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Page/Component
participant Dialog as ConfirmationDialog
participant Mut as Mutation
User->>UI: Initiates delete/remove
UI->>Dialog: open={true}, loading={pending}, confirmLabel={pending ? "...ing..." : "Delete/Remove"}
User->>Dialog: Click Confirm
Dialog->>Mut: onConfirm()
Mut-->>Dialog: pending=true
Dialog->>UI: propagate loading + label update
Mut-->>Dialog: success/error, pending=false
Dialog->>UI: close (on success or handled error)
sequenceDiagram
autonumber
participant Card as CapCard
participant Thumb as VideoThumbnail
participant Helper as formatDuration
Card->>Thumb: videoDuration=cap.metadata?.duration
alt videoDuration provided and > 0
Thumb->>Helper: formatDuration(ms as string)
Helper-->>Thumb: "X sec/min/hr"
Thumb-->>Card: Render image + duration badge
else
Thumb-->>Card: Render image without badge
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(org)/dashboard/settings/organization/components/MembersCard.tsx (1)
115-132: Bug: Dialog description uses hardcoded test data instead of pendingMemberThe description references pendingMemberTest instead of the actual selected member. This will show stale/wrong info in production.
Apply this diff to remove the test stub and use pendingMember, also fix capitalization:
- const pendingMemberTest = { - id: "1", - name: "John Doe", - email: "john.doe@example.com", - }; + // Removed test stub; use `pendingMember` state for runtime values. @@ <ConfirmationDialog open={confirmOpen} icon={<FontAwesomeIcon icon={faUser} />} title="Remove member" description={ - pendingMemberTest - ? `Are you sure you want to remove ${pendingMemberTest.name} - from your organization? this action cannot be undone.` + pendingMember + ? `Are you sure you want to remove ${pendingMember.name} from your organization? This action cannot be undone.` : "" } confirmLabel={removing ? "Removing..." : "Remove"}
🧹 Nitpick comments (3)
apps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx (1)
336-353: Consider preventing dialog dismissal while deletingIf not already handled inside ConfirmationDialog, prevent closing the dialog via Escape/backdrop click while loading is true to avoid accidental dismissal mid-request.
Would you like me to add a prop (e.g., disableCloseWhileLoading) to ConfirmationDialog and wire it here?
apps/web/components/VideoThumbnail.tsx (2)
4-4: Replace moment with a tiny formatter and fix “< 1 sec”/“60 secs” edge cases
- Moment is heavy for this use. A tiny pure-TS formatter trims bundle cost.
- Current logic makes "< 1 sec" unreachable for sub-second values due to ceil; also can display “60 secs” instead of “1 min”.
- Recommend accepting number | string, avoiding parse/format churn upstream.
Apply these diffs:
Remove the moment import:
-import moment from "moment";Broaden the prop type to accept number | string:
- videoDuration?: string; + videoDuration?: number | string;Replace formatDuration with a lightweight, edge-case-safe version:
-const formatDuration = (duration: string) => { - const durationMs = parseFloat(duration); - const momentDuration = moment.duration(durationMs, "milliseconds"); - - 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 = (duration: string | number): string => { + const ms = typeof duration === "number" ? duration : Number.parseFloat(duration); + if (!Number.isFinite(ms) || ms < 0) return "< 1 sec"; + + // Use integer seconds; sub-second shows "< 1 sec" + const totalSeconds = Math.floor(ms / 1000); + if (totalSeconds < 1) return "< 1 sec"; + + 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" : ""}`; + return `${seconds} sec${seconds !== 1 ? "s" : ""}`; +};Also applies to: 16-16, 19-36
111-115: Tailwind leading-0 may not exist (use leading-none)If you’re on default Tailwind, leading-0 isn’t defined. Suggest leading-none for consistent line-height.
- <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]"> + <p className="text-white leading-none px-2 left-3 rounded-full backdrop-blur-sm absolute z-10 bottom-3 bg-black/50 text-[11px]">
📜 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 (9)
apps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(2 hunks)apps/web/app/(org)/dashboard/caps/components/Folder.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/SelectedCapsBar.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/MembersCard.tsx(1 hunks)apps/web/app/(org)/dashboard/spaces/browse/page.tsx(1 hunks)apps/web/components/VideoThumbnail.tsx(4 hunks)packages/ui-solid/src/auto-imports.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/ui-solid/src/auto-imports.d.ts
⏰ 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 (8)
apps/web/app/(org)/dashboard/_components/Navbar/SpacesList.tsx (2)
336-353: Dynamic confirm label improves perceived responsivenessUsing confirmLabel={removing ? "Deleting..." : "Delete"} aligns the CTA text with the in-flight state. Looks good and consistent with loading={removing}.
336-353: ConfirmationDialog’s confirmLabel is correctly typed and defaulted
TheConfirmationDialogcomponent declaresconfirmLabel?: stringand assigns it a default of"Confirm", so existing usages that omit it will fall back to “Confirm.”apps/web/app/(org)/dashboard/caps/components/SelectedCapsBar.tsx (1)
79-93: Good UX: confirm label reflects deletion stateconfirmLabel={isDeleting ? "Deleting..." : "Delete"} is consistent with loading={isDeleting} and the disabled/spinner states on the trigger button. All good.
apps/web/app/(org)/dashboard/spaces/browse/page.tsx (1)
226-242: LGTM – dynamic label matches loading stateconfirmLabel toggles with removing and mirrors loading={removing}. This improves clarity during the async delete.
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (1)
69-79: LGTM – confirm label tied to mutation stateconfirmLabel uses removeDomainMutation.isPending and aligns with loading. Nice consistency with other dialogs in the PR.
apps/web/app/(org)/dashboard/settings/organization/components/MembersCard.tsx (1)
123-141: LGTM – removal label toggles with stateconfirmLabel={removing ? "Removing..." : "Remove"} is consistent with loading={removing}. Good.
apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
377-386: ConfirmationDialog.confirmLabel prop verifiedThe
ConfirmationDialogcomponent’s props include the optionalconfirmLabelstring (with a default of"Confirm"), so your dynamic label override (deleteFolder.isPending ? "Deleting..." : "Delete") will be applied correctly. Nice UX enhancement to reflect the pending state directly on the button.apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
380-387: Dynamic confirmLabel on ConfirmationDialog: good consistency with pending stateMatches the pattern used elsewhere in the PR and improves UX while the delete mutation is pending.
| <VideoThumbnail | ||
| videoDuration={cap.metadata?.duration} | ||
| imageClass={clsx( | ||
| anyCapSelected |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify duration type/units passed to VideoThumbnail
You’re passing cap.metadata?.duration into videoDuration (typed as string in VideoThumbnail). Please confirm:
- Type is string or update to number | string (recommended in my VideoThumbnail comment).
- Units are milliseconds (the formatter assumes ms; if it’s seconds, it will undercount by 1000x).
Run this to find the VideoMetadata definition and the duration field’s type and docstring/usages:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Searching for VideoMetadata definition...'
rg -n -C3 -P '(interface|type)\s+VideoMetadata\b' --type=ts || true
echo 'Searching for duration field within VideoMetadata...'
rg -n -C3 -P 'VideoMetadata|duration\s*:\s*' --type=ts | sed 's/^/• /' || true
echo 'Searching for where metadata.duration is set (to infer units)...'
rg -n -C3 -P '\bmetadata\b[^;]*\bduration\b' --type=ts -- apps || trueLength of output: 95791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== VideoMetadata interface snippet (first 50 lines) ==="
sed -n '1,50p' packages/database/types/metadata.ts || true
echo
echo "=== Searching for 'duration' property in VideoMetadata ==="
rg -n '^\s*duration\s*:' -C2 packages/database/types/metadata.ts || echo "No duration property found"Length of output: 1423
Align VideoMetadata.duration (seconds) with VideoThumbnail (milliseconds)
We discovered that VideoMetadata.duration is defined as string | number in seconds:
/** Duration of the video in seconds */
duration?: string | number;However, VideoThumbnail expects a string representing milliseconds:
interface VideoThumbnailProps {
videoDuration?: string;
}
const formatDuration = (duration: string) => {
const durationMs = parseFloat(duration);
return moment.duration(durationMs, "milliseconds")…
};Passing raw seconds into a formatter that treats its input as milliseconds will undercount by 1 000×. Please address this mismatch:
• In CapCard.tsx (lines 431–434)
- Convert seconds → milliseconds at the call site, e.g.:
- <VideoThumbnail - videoDuration={cap.metadata?.duration} - … - /> + <VideoThumbnail + videoDuration={ + cap.metadata?.duration != null + ? String(Number(cap.metadata.duration) * 1000) + : undefined + } + … + />
• In VideoThumbnail.tsx
- Update prop type to accept both string | number:
- interface VideoThumbnailProps { videoDuration?: string } + interface VideoThumbnailProps { videoDuration?: string | number }
- Enhance
formatDurationto detect numbers or adjust the unit (e.g., use"seconds"when appropriate) or normalize all inputs to milliseconds.
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx around lines
431–434, the video duration (seconds) is being passed directly to VideoThumbnail
which expects milliseconds; convert the value to milliseconds at the call site
(e.g., multiply numeric seconds by 1000 and ensure a string is passed). Also
update VideoThumbnail.tsx to accept videoDuration?: string | number and
normalize inside (accept number or string, parse/convert seconds → milliseconds
when needed or treat numeric input as milliseconds consistently), and make
formatDuration resilient by parsing numbers and ensuring it always calls
moment.duration with milliseconds.
This PR: improves duration formatting + confirmation dialog label messages on loading.
Summary by CodeRabbit
New Features
Chores