improvement: Comment & reactions timestamps #1091
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds timestamp support to comment creation and optimistic updates; introduces in-player comment markers with seek handling and hover tooltips; propagates onSeek/mainControlsVisible through player components; gates navbar custom-domain link by verification; validates notification video/owner fetches; minor page keying and logging tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant T as Toolbar/Comments UI
participant A as Action newComment
participant DB as DB
participant N as Notification
U->>T: Submit comment (text/emoji)
T->>T: Read video.currentTime → timestamp
T->>A: newComment({ content, videoId, type, parentCommentId, timestamp })
A->>DB: Create comment with timestamp
alt video & owner present
A->>N: Create notification
else missing owner
A->>N: Skip notification (warn)
end
A-->>T: Result
T-->>U: Optimistic update reconciled
sequenceDiagram
autonumber
participant U as User
participant CS as CommentStamp
participant P as CapVideoPlayer
participant S as Share/ShareVideo
participant V as <video> element
U->>CS: Click marker
CS->>P: onSeek(timestamp)
P->>S: onSeek(timestamp)
S->>V: Seek when ready (immediate or via canplay/loadedmetadata)
V-->>S: Seek applied
sequenceDiagram
autonumber
participant MPC as MediaPlayerControls
participant P as CapVideoPlayer
MPC->>MPC: controlsVisible changes
MPC-->>P: mainControlsVisible(controlsVisible)
Note right of P: Adjust UI (e.g., cue positioning)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx (1)
95-205: Don't hide pending custom domainsWhen an org has entered a custom domain but verification hasn’t finished (
customDomaintruthy,domainVerifiedfalse), the new logic flipsisDomainSetupVerifiedto false and we now render “No custom domain set.” That’s inaccurate and removes the only on-screen confirmation of the domain they just configured, likely confusing owners who are still waiting on DNS propagation. Please keep showing the entered domain (optionally mark it as pending) while still routing them back to Settings until verification passes.Suggested adjustment:
- const isDomainSetupVerified = - activeOrg?.organization.customDomain && - activeOrg?.organization.domainVerified; + const customDomain = activeOrg?.organization.customDomain; + const isDomainSetupVerified = Boolean( + customDomain && activeOrg?.organization.domainVerified, + );- href={ - isDomainSetupVerified - ? `https://${activeOrg.organization.customDomain}` - : "/dashboard/settings/organization" - } + href={ + isDomainSetupVerified + ? `https://${customDomain}` + : "/dashboard/settings/organization" + }- <p className="w-full text-[11px] flex-1 duration-200 truncate leading-0 text-gray-11"> - {isDomainSetupVerified - ? activeOrg?.organization.customDomain - : "No custom domain set"} - </p> + <p className="w-full text-[11px] flex-1 duration-200 truncate leading-0 text-gray-11"> + {customDomain + ? isDomainSetupVerified + ? customDomain + : `${customDomain} (verification pending)` + : "No custom domain set"} + </p>This preserves the verification check without regressing the UX for domains that are still pending.
apps/web/lib/Notification.ts (1)
127-134: Guard orgId for reply notifications and skip self-repliesIn the reply path you insert with
orgId: recipientUser.activeOrganizationIdwithout verifying it exists; unlike the non-reply path, this can insert a null orgId. Also skip notifying when replying to your own comment.Apply this diff:
const notificationId = nanoId(); - await db().insert(notifications).values({ + // Skip if recipient has no active org + if (!recipientUser.activeOrganizationId) { + console.warn( + `Reply recipient ${recipientId} has no active organization, skipping notification`, + ); + return; + } + // Skip self-replies + if (recipientId === notification.authorId) return; + + await db().insert(notifications).values({ id: notificationId, orgId: recipientUser.activeOrganizationId, recipientId, type, data, });apps/web/app/s/[videoId]/Share.tsx (3)
79-140: Switch to EffectRuntime’s useEffectQuery (repo rule).Client-side server state must use useEffectQuery from @/lib/EffectRuntime, not useQuery. Replace the import and call to align with apps/web guidelines.
Apply:
-import { useQuery } from "@tanstack/react-query"; +import { useEffectQuery } from "@/lib/EffectRuntime"; @@ - return useQuery({ + return useEffectQuery({ queryKey: ["videoStatus", videoId], queryFn: async (): Promise<VideoStatusResult> => { const res = await getVideoStatus(videoId); if ("success" in res && res.success === false) throw new Error("Failed to fetch video status"); return res as VideoStatusResult; },
1-427: Remove inline comments per repo guideline.Multiple inline comments exist in this client module. Repo rule forbids inline/block/doc comments in TS/TSX. Remove them (your handleSeek diff above already addresses the main block).
323-336: AddplayerReftoSidebarProps. Sidebar is already wrapped withforwardRef; update the props interface in
apps/web/app/s/[videoId]/_components/Sidebar.tsx:interface SidebarProps { // existing props… + playerRef?: React.RefObject<HTMLVideoElement | null>; }apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (2)
376-381: Duplicate loadedmetadata listener attached.loadedmetadata is added twice; handler will fire twice. Remove the duplicate to avoid double work and inconsistent state.
Apply:
- video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); - video.addEventListener("load", handleLoad); + video.addEventListener("load", handleLoad); @@ - video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks);
1-1: Rename CapVideoPlayer.tsx to kebab-case
Renameapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx→apps/web/app/s/[videoId]/_components/cap-video-player.tsxto comply with the repo’s kebab-case filename convention.
🧹 Nitpick comments (16)
apps/web/actions/videos/new-comment.ts (2)
28-29: Validate and normalize timestamp; avoid storing invalid values and replies with timestampsGuard against NaN/negative inputs and ensure replies don’t carry timestamps.
Apply this diff:
- const timestamp = data.timestamp; + const timestamp = + Number.isFinite(data.timestamp) && data.timestamp >= 0 + ? Math.floor(data.timestamp) + : null;- timestamp: timestamp ?? null, + timestamp: parentCommentId ? null : timestamp,Also applies to: 46-46
73-73: Prefer targeted client cache updates over broad route revalidationRevalidating
/s/${videoId}is heavy. When invoked from the client, favor TanStack Query v5’ssetQueryData/setQueriesDatafor the comments list and avoid broad invalidations.apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx (1)
97-101: Make timestamp rendering robust; avoid deprecated substr and guard for finite valuesEnsure the timestamp is finite/non-negative, preserve 0s, and avoid
substr.Apply this diff:
- {comment.timestamp !== null && ( + {Number.isFinite(Number(comment.timestamp)) && + Number(comment.timestamp) >= 0 && ( <button type="button" - onClick={() => { - onSeek?.(Number(comment.timestamp)); - }} + onClick={() => onSeek?.(Math.floor(Number(comment.timestamp)))} className="text-xs text-blue-500 cursor-pointer hover:text-blue-700" > - {new Date(comment.timestamp * 1000) - .toISOString() - .substr(11, 8)} + {new Date(Math.floor(Number(comment.timestamp)) * 1000) + .toISOString() + .slice(11, 19)} </button> )}Also applies to: 102-120, 122-122
apps/web/app/s/[videoId]/_components/tabs/Activity/index.tsx (1)
6-12: onSeek propagation LGTM; drop unused importsForwarding
onSeekis correct. IfRefObjectisn’t used here, remove it to keep imports lean.Also applies to: 73-74
apps/web/app/s/[videoId]/_components/video/media-player.tsx (1)
897-903: Remove inline comment to comply with repo guidelines.Inline comment violates “no comments in TS/TSX” rule.
Apply this diff:
- // Call the callback whenever controlsVisible changes useEffect(() => { if (typeof mainControlsVisible === "function") { mainControlsVisible(controlsVisible); } }, [mainControlsVisible, controlsVisible]);As per coding guidelines.
apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx (2)
77-80: Avoid querying the DOM repeatedly; centralize currentTime retrieval.DRY and reduce null checks by using a tiny helper.
Apply this diff:
+ const getVideoCurrentTime = () => { + const el = document.querySelector("video") as HTMLVideoElement | null; + return el?.currentTime ?? 0; + }; ... - // Get current video time from the video element - const videoElement = document.querySelector("video") as HTMLVideoElement; - const currentTime = videoElement?.currentTime || 0; + const currentTime = getVideoCurrentTime(); ... - const videoElement = document.querySelector("video") as HTMLVideoElement; - const currentTime = videoElement?.currentTime || 0; + const currentTime = getVideoCurrentTime();Also applies to: 115-117
77-77: Remove inline comment to meet repo standards.Comments in TSX aren’t allowed in this codebase.
- // Get current video time from the video elementAs per coding guidelines.
apps/web/app/s/[videoId]/_components/CommentStamp.tsx (3)
30-30: Don’t set key inside the component.Parent supplies keys; local key is redundant and can confuse React.
- key={comment.id}
41-41: Remove inline JSX comments to match repo policy.Comments in TSX are disallowed here.
- {/* Comment marker */} ... - {/* Arrow pointing down to marker */} ... - {/* User avatar/initial */} ... - {/* Comment content */}As per coding guidelines.
Also applies to: 59-61, 64-69, 70-77
1-86: File naming: prefer kebab-case.Please rename to comment-stamp.tsx and update imports.
As per coding guidelines.
apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)
31-33: Consolidate currentTime reads.Use a small helper to avoid repeating querySelector logic and potential null checks.
+ const getVideoCurrentTime = () => { + const el = document.querySelector("video") as HTMLVideoElement | null; + return el?.currentTime ?? 0; + }; ... - const videoElement = document.querySelector("video") as HTMLVideoElement; - const currentTime = videoElement?.currentTime || 0; + const currentTime = getVideoCurrentTime(); ... - const videoElement = document.querySelector("video") as HTMLVideoElement; - const currentTime = videoElement?.currentTime || 0; + const currentTime = getVideoCurrentTime(); ... - const videoElement = document.querySelector( - "video", - ) as HTMLVideoElement; + const videoElement = document.querySelector("video") as HTMLVideoElement | null; ... - {(() => { - const videoElement = document.querySelector( - "video", - ) as HTMLVideoElement; - return videoElement && videoElement.currentTime > 0 - ? `Comment at ${videoElement.currentTime.toFixed(2)}` - : "Comment"; - })()} + {(() => { + const t = getVideoCurrentTime(); + return t > 0 ? `Comment at ${t.toFixed(2)}` : "Comment"; + })()}Also applies to: 72-74, 143-146, 219-226
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (2)
45-48: Props mismatch: type declares aiProcessing but implementation drops it.Keep the public surface consistent; safely accept and ignore the prop.
->(({ data, comments, chapters = [] }, ref) => { +>(({ data, comments, chapters = [], aiProcessing: _aiProcessing }, ref) => {
61-69: Remove inline comments per repo policy.Replace narrative comments with self-explanatory names; comments are not allowed.
- // Handle comments data useEffect(() => { ... - // Handle seek functionality const handleSeek = (time: number) => {As per coding guidelines.
Also applies to: 71-77
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (2)
234-249: Avoid inline comments; keep effect scoped to ref changes.Remove comments per repo rule. Optionally include videoRef in deps if ref identity can change.
Apply:
-// Track video duration for comment markers useEffect(() => { const video = videoRef.current; if (!video) return; - - const handleLoadedMetadata = () => { + const handleLoadedMetadata = () => { setDuration(video.duration); }; - video.addEventListener("loadedmetadata", handleLoadedMetadata); - return () => { video.removeEventListener("loadedmetadata", handleLoadedMetadata); }; -}, [urlResolved]); +}, [urlResolved]);
606-629: Clamp marker positions and guard against invalid timestamps.Prevents NaN/overflow when incoming timestamp is out of range; keeps markers within bounds.
Apply:
- .map((comment) => { - const position = (Number(comment.timestamp) / duration) * 100; + .map((comment) => { + const ts = Math.max( + 0, + Math.min(duration - 0.001, Number(comment.timestamp)), + ); + const position = (ts / duration) * 100; const containerPadding = 20; const availableWidth = `calc(100% - ${containerPadding * 2}px)`; const adjustedPosition = `calc(${containerPadding}px + (${position}% * ${availableWidth} / 100%))`;apps/web/app/s/[videoId]/Share.tsx (1)
1-1: Rename Share.tsx → share.tsx
git mv apps/web/app/s/[videoId]/Share.tsx apps/web/app/s/[videoId]/share.tsx
Update import paths to referenceshare.tsx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/actions/videos/new-comment.ts(3 hunks)apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx(2 hunks)apps/web/app/s/[videoId]/Share.tsx(2 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(8 hunks)apps/web/app/s/[videoId]/_components/CommentStamp.tsx(1 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(2 hunks)apps/web/app/s/[videoId]/_components/Toolbar.tsx(10 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx(1 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx(7 hunks)apps/web/app/s/[videoId]/_components/tabs/Activity/index.tsx(2 hunks)apps/web/app/s/[videoId]/_components/video/media-player.tsx(3 hunks)apps/web/app/s/[videoId]/page.tsx(3 hunks)apps/web/lib/Notification.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must be implemented in Next.js Server Actions under apps/web/actions; do not place AI calls elsewhere
Files:
apps/web/actions/videos/new-comment.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/actions/videos/new-comment.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/CommentStamp.tsxapps/web/lib/Notification.tsapps/web/app/s/[videoId]/_components/tabs/Activity/index.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/s/[videoId]/_components/video/media-player.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/actions/videos/new-comment.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/CommentStamp.tsxapps/web/lib/Notification.tsapps/web/app/s/[videoId]/_components/tabs/Activity/index.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/s/[videoId]/_components/video/media-player.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/actions/videos/new-comment.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/CommentStamp.tsxapps/web/lib/Notification.tsapps/web/app/s/[videoId]/_components/tabs/Activity/index.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/s/[videoId]/_components/video/media-player.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/actions/videos/new-comment.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/CommentStamp.tsxapps/web/lib/Notification.tsapps/web/app/s/[videoId]/_components/tabs/Activity/index.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/s/[videoId]/_components/video/media-player.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/actions/videos/new-comment.tsapps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/CommentStamp.tsxapps/web/lib/Notification.tsapps/web/app/s/[videoId]/_components/tabs/Activity/index.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/s/[videoId]/_components/video/media-player.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsx
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/s/[videoId]/_components/ShareVideo.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/CommentStamp.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/index.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/Share.tsxapps/web/app/s/[videoId]/_components/video/media-player.tsxapps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsxapps/web/app/s/[videoId]/_components/Toolbar.tsx
🧬 Code graph analysis (5)
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (3)
apps/web/app/s/[videoId]/_components/utils/transcript-utils.ts (1)
TranscriptEntry(3-8)apps/web/hooks/use-transcript.ts (1)
useTranscript(5-27)packages/database/schema.ts (1)
comments(312-332)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
packages/database/schema.ts (1)
comments(312-332)
apps/web/lib/Notification.ts (2)
packages/database/index.ts (1)
db(29-34)packages/database/schema.ts (2)
videos(240-288)users(48-96)
apps/web/app/s/[videoId]/_components/tabs/Activity/Comment.tsx (1)
apps/web/app/s/[videoId]/_components/tabs/Activity/utils.ts (2)
formatTimestamp(1-10)formatTimeAgo(12-32)
apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx (1)
apps/web/actions/videos/new-comment.ts (1)
newComment(11-76)
🪛 GitHub Check: Typecheck
apps/web/app/s/[videoId]/Share.tsx
[failure] 323-323:
Type '{ data: { createdAt: Date; transcriptionStatus: "PROCESSING" | "COMPLETE" | "ERROR" | null; metadata: VideoMetadata | null; name: string; id: string & Brand<"VideoId">; ... 25 more ...; ownerIsPro?: boolean | undefined; }; ... 12 more ...; ref: RefObject<...>; }' is not assignable to type 'IntrinsicAttributes & SidebarProps & RefAttributes<{ scrollToBottom: () => void; }>'.
⏰ 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). (4)
- GitHub Check: Vercel Agent Review
- 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/s/[videoId]/page.tsx (1)
320-325: Verify necessity of key={videoId} on top-level wrappersAdding keys on these non-list roots will force remounts when videoId changes. If the intent is to reset local client state (e.g., player/optimistic UI) between videos, keep it; otherwise, consider removing to preserve state across param updates.
Also applies to: 330-333
apps/web/app/s/[videoId]/_components/video/media-player.tsx (2)
877-891: New mainControlsVisible callback is wired correctly.Prop type and destructuring look good; callback exposure from Controls is appropriate.
2343-2344: Volume slider expand/collapse transition—LGTM.ClassName tweak improves affordance without changing behavior.
apps/web/app/s/[videoId]/_components/tabs/Activity/Comments.tsx (2)
59-60: Scroll behavior dependency update—LGTM.Including commentParams/replyParams prevents unintended auto-scroll after deep links.
70-71: API propagation—LGTM.Exposing scrollToBottom via ref and passing onSeek to Comment is correct.
Also applies to: 213-214
apps/web/app/s/[videoId]/_components/Toolbar.tsx (1)
90-101: Optimistic + server action flow—LGTM.startTransition usage and timestamp plumbing align with the new API.
Also applies to: 219-227
apps/web/app/s/[videoId]/_components/ShareVideo.tsx (1)
178-186: comments prop normalization—LGTM.Mapping to the player’s simple shape and wiring onSeek is correct.
apps/web/app/s/[videoId]/Share.tsx (2)
271-274: startTransition usage for optimistic UI looks good.
156-165: React and React-DOM are consistently 19.1.1 across packages; use() is supported.apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (2)
536-545: Tracks conditional rendering LGTM.
633-633: mainControlsVisible callback integration LGTM.
| content: content, | ||
| videoId: videoId, | ||
| timestamp: null, | ||
| timestamp: timestamp || null, |
There was a problem hiding this comment.
Bug: 0-second timestamps are coerced to null
Using timestamp || null drops valid 0 into null. Use nullish coalescing.
Apply this diff:
- timestamp: timestamp || null,
+ timestamp: timestamp ?? null,📝 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.
| timestamp: timestamp || null, | |
| timestamp: timestamp ?? null, |
🤖 Prompt for AI Agents
In apps/web/actions/videos/new-comment.ts around line 46, the assignment
`timestamp: timestamp || null` incorrectly converts a valid 0 timestamp to null;
replace the logical OR with nullish coalescing so that only undefined or null
become null (i.e., use timestamp ?? null) and ensure the updated expression
preserves falsy numeric 0 values.
| const data = await newComment({ | ||
| content, | ||
| videoId: props.videoId, | ||
| parentCommentId: "", | ||
| type: "text", | ||
| timestamp: currentTime, | ||
| }); | ||
| handleCommentSuccess(data); |
There was a problem hiding this comment.
0-second timestamps are being saved as null (server action bug).
new-comment.ts uses timestamp || null, which coerces 0 to null. This breaks exact-start markers.
Apply this diff in apps/web/actions/videos/new-comment.ts:
- const timestamp = data.timestamp;
...
- timestamp: timestamp || null,
+ const timestamp = data.timestamp;
+ // Preserve 0s; only null/undefined become null in DB
+ timestamp: timestamp ?? null,Based on relevant code snippet.
Also applies to: 142-148
| console.warn("Failed to create view notification:", error); | ||
| // Don't throw the error, just log it since this is not critical for the page to function | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Log level change is good; remove inline comment per repo guidelines
console.warn for a non-critical failure is appropriate. However, inline comments are disallowed in TS/TSX per coding guidelines. Please drop the added comment line.
Apply this diff:
- // Don't throw the error, just log it since this is not critical for the page to function📝 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.
| console.warn("Failed to create view notification:", error); | |
| // Don't throw the error, just log it since this is not critical for the page to function | |
| } | |
| console.warn("Failed to create view notification:", error); | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/page.tsx around lines 374 to 376, you added a
console.warn followed by an inline comment; remove the inline comment line "//
Don't throw the error, just log it since this is not critical for the page to
function" so the file contains only the console.warn call (or equivalent
logging) and no inline comment, complying with TS/TSX repo guidelines.
| const handleSeek = useCallback((time: number) => { | ||
| let video = playerRef.current; | ||
|
|
||
| // Fallback to DOM query if ref is not available | ||
| if (!video) { | ||
| video = document.querySelector("video") as HTMLVideoElement; | ||
| if (!video) { | ||
| console.warn("Video player not ready"); | ||
| return; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Try to seek immediately if video has metadata | ||
| if (video.readyState >= 1) { | ||
| // HAVE_METADATA | ||
| try { | ||
| video.currentTime = time; | ||
| return; | ||
| } catch (error) { | ||
| console.error("Failed to seek video:", error); | ||
| } | ||
| } | ||
|
|
||
| // If video isn't ready, wait for it to be ready | ||
| const handleCanPlay = () => { | ||
| try { | ||
| video.currentTime = time; | ||
| } catch (error) { | ||
| console.error("Failed to seek video after canplay:", error); | ||
| } | ||
| video.removeEventListener("canplay", handleCanPlay); | ||
| }; | ||
|
|
||
| const handleLoadedMetadata = () => { | ||
| try { | ||
| video.currentTime = time; | ||
| } catch (error) { | ||
| console.error("Failed to seek video after loadedmetadata:", error); | ||
| } | ||
| video.removeEventListener("loadedmetadata", handleLoadedMetadata); | ||
| }; | ||
|
|
||
| // Listen for multiple events to ensure we catch when the video is ready | ||
| video.addEventListener("canplay", handleCanPlay); | ||
| video.addEventListener("loadedmetadata", handleLoadedMetadata); | ||
|
|
||
| // Cleanup after 3 seconds if events don't fire | ||
| setTimeout(() => { | ||
| video.removeEventListener("canplay", handleCanPlay); | ||
| video.removeEventListener("loadedmetadata", handleLoadedMetadata); | ||
| }, 3000); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Make seeking idempotent; use once listeners, clamp time, and remove inline comments.
Avoid duplicate seeks and dangling listeners; clamp against duration when known; remove comments per repo rule.
Apply:
-const handleSeek = useCallback((time: number) => {
- let video = playerRef.current;
-
- // Fallback to DOM query if ref is not available
- if (!video) {
- video = document.querySelector("video") as HTMLVideoElement;
- if (!video) {
- console.warn("Video player not ready");
- return;
- }
- }
-
- // Try to seek immediately if video has metadata
- if (video.readyState >= 1) {
- // HAVE_METADATA
- try {
- video.currentTime = time;
- return;
- } catch (error) {
- console.error("Failed to seek video:", error);
- }
- }
-
- // If video isn't ready, wait for it to be ready
- const handleCanPlay = () => {
- try {
- video.currentTime = time;
- } catch (error) {
- console.error("Failed to seek video after canplay:", error);
- }
- video.removeEventListener("canplay", handleCanPlay);
- };
-
- const handleLoadedMetadata = () => {
- try {
- video.currentTime = time;
- } catch (error) {
- console.error("Failed to seek video after loadedmetadata:", error);
- }
- video.removeEventListener("loadedmetadata", handleLoadedMetadata);
- };
-
- // Listen for multiple events to ensure we catch when the video is ready
- video.addEventListener("canplay", handleCanPlay);
- video.addEventListener("loadedmetadata", handleLoadedMetadata);
-
- // Cleanup after 3 seconds if events don't fire
- setTimeout(() => {
- video.removeEventListener("canplay", handleCanPlay);
- video.removeEventListener("loadedmetadata", handleLoadedMetadata);
- }, 3000);
-}, []);
+const handleSeek = useCallback((time: number) => {
+ const v =
+ playerRef.current ?? (document.querySelector("video") as HTMLVideoElement | null);
+ if (!v) {
+ console.warn("Video player not ready");
+ return;
+ }
+ const seekOnce = (t: number) => {
+ const dur = Number.isFinite(v.duration) && v.duration > 0 ? v.duration : null;
+ const clamped = dur ? Math.max(0, Math.min(dur - 0.001, t)) : t;
+ try {
+ v.currentTime = clamped;
+ } catch (e) {
+ console.error("Failed to seek video:", e);
+ }
+ };
+ if (v.readyState >= 1) {
+ seekOnce(time);
+ return;
+ }
+ let timeoutId: ReturnType<typeof setTimeout> | null = null;
+ const handleReady = () => {
+ seekOnce(time);
+ v.removeEventListener("canplay", handleReady);
+ v.removeEventListener("loadedmetadata", handleReady);
+ if (timeoutId) clearTimeout(timeoutId);
+ };
+ v.addEventListener("canplay", handleReady, { once: true });
+ v.addEventListener("loadedmetadata", handleReady, { once: true });
+ timeoutId = setTimeout(() => {
+ v.removeEventListener("canplay", handleReady);
+ v.removeEventListener("loadedmetadata", handleReady);
+ }, 3000);
+}, []);📝 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 handleSeek = useCallback((time: number) => { | |
| let video = playerRef.current; | |
| // Fallback to DOM query if ref is not available | |
| if (!video) { | |
| video = document.querySelector("video") as HTMLVideoElement; | |
| if (!video) { | |
| console.warn("Video player not ready"); | |
| return; | |
| } | |
| } | |
| }; | |
| // Try to seek immediately if video has metadata | |
| if (video.readyState >= 1) { | |
| // HAVE_METADATA | |
| try { | |
| video.currentTime = time; | |
| return; | |
| } catch (error) { | |
| console.error("Failed to seek video:", error); | |
| } | |
| } | |
| // If video isn't ready, wait for it to be ready | |
| const handleCanPlay = () => { | |
| try { | |
| video.currentTime = time; | |
| } catch (error) { | |
| console.error("Failed to seek video after canplay:", error); | |
| } | |
| video.removeEventListener("canplay", handleCanPlay); | |
| }; | |
| const handleLoadedMetadata = () => { | |
| try { | |
| video.currentTime = time; | |
| } catch (error) { | |
| console.error("Failed to seek video after loadedmetadata:", error); | |
| } | |
| video.removeEventListener("loadedmetadata", handleLoadedMetadata); | |
| }; | |
| // Listen for multiple events to ensure we catch when the video is ready | |
| video.addEventListener("canplay", handleCanPlay); | |
| video.addEventListener("loadedmetadata", handleLoadedMetadata); | |
| // Cleanup after 3 seconds if events don't fire | |
| setTimeout(() => { | |
| video.removeEventListener("canplay", handleCanPlay); | |
| video.removeEventListener("loadedmetadata", handleLoadedMetadata); | |
| }, 3000); | |
| }, []); | |
| const handleSeek = useCallback((time: number) => { | |
| const v = | |
| playerRef.current ?? (document.querySelector("video") as HTMLVideoElement | null); | |
| if (!v) { | |
| console.warn("Video player not ready"); | |
| return; | |
| } | |
| const seekOnce = (t: number) => { | |
| const dur = Number.isFinite(v.duration) && v.duration > 0 ? v.duration : null; | |
| const clamped = dur ? Math.max(0, Math.min(dur - 0.001, t)) : t; | |
| try { | |
| v.currentTime = clamped; | |
| } catch (e) { | |
| console.error("Failed to seek video:", e); | |
| } | |
| }; | |
| if (v.readyState >= 1) { | |
| seekOnce(time); | |
| return; | |
| } | |
| let timeoutId: ReturnType<typeof setTimeout> | null = null; | |
| const handleReady = () => { | |
| seekOnce(time); | |
| v.removeEventListener("canplay", handleReady); | |
| v.removeEventListener("loadedmetadata", handleReady); | |
| if (timeoutId) clearTimeout(timeoutId); | |
| }; | |
| v.addEventListener("canplay", handleReady, { once: true }); | |
| v.addEventListener("loadedmetadata", handleReady, { once: true }); | |
| timeoutId = setTimeout(() => { | |
| v.removeEventListener("canplay", handleReady); | |
| v.removeEventListener("loadedmetadata", handleReady); | |
| }, 3000); | |
| }, []); |
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/Share.tsx around lines 216-268, the seek handler can
perform duplicate seeks and leave dangling listeners; update handleSeek to clamp
requested time to [0, video.duration] when duration is finite, only set
video.currentTime if the difference from currentTime is greater than a small
epsilon (to make seeking idempotent), attach event listeners with the { once:
true } option (or remove them immediately after firing) instead of manual
removal, and keep the handler cleanup timeout but ensure it clears listeners
only if still present; also remove inline comments per repo style.
| // First, check if the video exists | ||
| const [videoExists] = await db() | ||
| .select({ id: videos.id, ownerId: videos.ownerId }) | ||
| .from(videos) | ||
| .where(eq(videos.id, Video.VideoId.make(notification.videoId))) | ||
| .limit(1); | ||
|
|
||
| if (!videoExists) { | ||
| console.error("Video not found for videoId:", notification.videoId); | ||
| throw new Error(`Video not found for videoId: ${notification.videoId}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Good early validations; remove newly added inline comments to meet repo policy
The two-step video/owner checks are solid. However, inline comments were added in these hunks; comments are disallowed in TS/TSX here. Please remove them.
Example diff (remove only comment lines):
- // First, check if the video exists
...
- // Then get the owner data
...
- // Don't throw an error, just skip notification creation
- // This handles cases where the video exists but the owner was deleted
...
- // Also applies to: 47-56, 58-69, 71-77
🤖 Prompt for AI Agents
In apps/web/lib/Notification.ts around lines 35-46 (and also apply the same
change to ranges 47-56, 58-69, 71-77), remove the newly added inline comment
lines so the TypeScript/TSX file contains no inline comments in these hunks;
keep the existing validation code and error handling intact but delete only the
comment lines to comply with repo policy.
Summary by CodeRabbit
New Features
Bug Fixes