chore: cleanup some comments & improve seek code #1094
Conversation
WalkthroughRefines comment timestamp handling to use nullish coalescing. Reworks video seeking in Share.tsx to clamp and perform a single, listener-once readiness flow with timeout-based cleanup. Removes inline comments in page.tsx and Notification.ts without changing behavior. No exported/public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Share as Share.tsx
participant Video as HTMLVideoElement
User->>Share: Request share with time
Share->>Video: Resolve v (playerRef or document.querySelector)
alt Video found
alt readyState ≥ 1
Share->>Video: seekOnce(clampedTime)
else Not ready yet
rect rgb(240,248,255)
note right of Share: Attach once-only listeners
Share->>Video: addEventListener('canplay', handleReady, {once:true})
Share->>Video: addEventListener('loadedmetadata', handleReady, {once:true})
Share->>Share: start 3s cleanup timeout
end
Video-->>Share: canplay or loadedmetadata
Share->>Video: seekOnce(clampedTime)
Share->>Share: remove listeners & clear timeout
opt No event within 3s
Share->>Share: cleanup listeners (timeout)
end
end
else No video
Share->>Share: warn and return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/actions/videos/new-comment.ts(1 hunks)apps/web/app/s/[videoId]/Share.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(0 hunks)apps/web/lib/Notification.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/web/lib/Notification.ts
- apps/web/app/s/[videoId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (7)
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/app/s/[videoId]/Share.tsxapps/web/actions/videos/new-comment.ts
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]/Share.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/app/s/[videoId]/Share.tsxapps/web/actions/videos/new-comment.ts
**/*.{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/app/s/[videoId]/Share.tsxapps/web/actions/videos/new-comment.ts
**/*.{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/app/s/[videoId]/Share.tsxapps/web/actions/videos/new-comment.ts
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/app/s/[videoId]/Share.tsxapps/web/actions/videos/new-comment.ts
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
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
| timeoutId = setTimeout(() => { | ||
| v.removeEventListener("canplay", handleReady); | ||
| v.removeEventListener("loadedmetadata", handleReady); | ||
| }, 3000); |
There was a problem hiding this comment.
Do not drop readiness listeners after 3 s
If the video metadata takes longer than 3 seconds to arrive (slow network, large asset, cold CDN), this timeout removes both readiness listeners before they ever fire. Once that happens there is no remaining path to call seekOnce, so the user’s chapter/time jump silently fails. That’s a regression from the previous behavior that waited indefinitely for readiness. Please keep the listeners until they run, or at least only remove them after we actually succeed in seeking.
- timeoutId = setTimeout(() => {
- v.removeEventListener("canplay", handleReady);
- v.removeEventListener("loadedmetadata", handleReady);
- }, 3000);
+ timeoutId = setTimeout(() => {
+ if (v.readyState >= 1) {
+ seekOnce(time);
+ v.removeEventListener("canplay", handleReady);
+ v.removeEventListener("loadedmetadata", handleReady);
+ timeoutId = null;
+ }
+ }, 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.
| timeoutId = setTimeout(() => { | |
| v.removeEventListener("canplay", handleReady); | |
| v.removeEventListener("loadedmetadata", handleReady); | |
| }, 3000); | |
| timeoutId = setTimeout(() => { | |
| if (v.readyState >= 1) { | |
| seekOnce(time); | |
| v.removeEventListener("canplay", handleReady); | |
| v.removeEventListener("loadedmetadata", handleReady); | |
| timeoutId = null; | |
| } | |
| }, 3000); |
🤖 Prompt for AI Agents
In apps/web/app/s/[videoId]/Share.tsx around lines 247 to 250, the code
currently drops the "canplay" and "loadedmetadata" readiness listeners after a
3s timeout which can prevent seekOnce from ever being called on slow networks;
remove that timeout logic and instead keep the listeners attached until they
fire, or alternatively only remove each listener after its handler successfully
runs (and only clear any timeout if/when the seek succeeds), ensuring there
remains a path to call seekOnce even when metadata arrives later than 3s.
Summary by CodeRabbit
Bug Fixes
Chores