feat: pro blocks and setup - #159
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (23)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (22)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis PR adds Ambient Player and Immersive Player pro blocks with registry, showcase, and documentation wiring. It replaces external clipboard hooks with a local hook, centralizes API URLs, and updates player styling, playback, and caption behavior. ChangesPro Blocks Feature
Copy-to-Clipboard Hook Migration
API Base URL Centralization
Player Styling and Playback Behavior Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant User
participant BlockViewerToolbar
participant getRegistryInstallCommand
participant useCopyToClipboard
User->>BlockViewerToolbar: Click install button
BlockViewerToolbar->>getRegistryInstallCommand: item
getRegistryInstallCommand-->>BlockViewerToolbar: installCommand
BlockViewerToolbar->>useCopyToClipboard: copyToClipboard(installCommand)
useCopyToClipboard-->>BlockViewerToolbar: isCopied = true
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/www/lib/constants.ts">
<violation number="1" location="apps/www/lib/constants.ts:11">
P2: `LIMEPLAY_API_BASE_URL` is hardcoded to a production Workers URL with no environment-variable override. In local development this will hit the production API, and there is no way to point to a local or preview API instance. Consider following the pattern used by `PROD_BASE_HOST` and allowing an `process.env.LIMEPLAY_API_BASE_URL` override.</violation>
</file>
<file name="apps/www/components/blocks/block-showcase.tsx">
<violation number="1" location="apps/www/components/blocks/block-showcase.tsx:22">
P2: the ambient-player and immersive-player showcase entries are structurally identical, and all three video-player entries share the same wrapper layout — consider extracting a shared helper like `renderVideoBlock(Component)` to reduce duplication and keep future block additions consistent</violation>
</file>
<file name="apps/www/hooks/use-copy-to-clipboard.ts">
<violation number="1" location="apps/www/hooks/use-copy-to-clipboard.ts:31">
P1: Rapid successive copies within the timeout window cause premature `isCopied` reset. Each `copyToClipboard` call schedules a new timeout without clearing the previous one: if the user copies at t=0 and t=500ms (with default 2000ms timeout), the first timeout fires at t=2000ms and flips `isCopied` to false, even though the second copy's timeout should keep it true until t=2500ms. Track the timeout ID in a ref and clear it before scheduling a new one.</violation>
</file>
<file name="apps/www/components/block-viewer.tsx">
<violation number="1" location="apps/www/components/block-viewer.tsx:489">
P0: The `?token=<token>` in the pro-block install URL is a placeholder that never gets resolved — users will literally copy `?token=<token>` to their clipboard, and running that command will fail with an auth error. This blocks installation of all pro blocks (ambient-player, immersive-player). Either wire in an actual token (from env, config, or user input) or remove the placeholder logic until auth is ready.</violation>
</file>
<file name="apps/www/content/docs/blocks/ambient-player.mdx">
<violation number="1" location="apps/www/content/docs/blocks/ambient-player.mdx:5">
P1: The `preview: ambient-player` frontmatter references a preview component whose source file doesn't exist yet. The block-showcase tries to import from `@/registry/pro/blocks/ambient-player/player`, but `registry/pro/blocks/` has no files. Either create the source component as part of this PR or remove/skip the preview field until the component is ready to avoid a broken doc page.</violation>
</file>
<file name="apps/www/scripts/registry-dev.mts">
<violation number="1" location="apps/www/scripts/registry-dev.mts:435">
P2: The generated `proRegistryItems` lookup can include a bogus `registry` entry because this loop imports every `.json` file from the shadcn output, including the tier-level `registry.json` that `validateTierRegistries()` expects in `registry/.generated/pro`. Skipping `registry.json` here would keep the runtime module limited to actual pro item JSON files.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| function getRegistryInstallCommand(item: RegistryItem) { | ||
| const itemName = item.categories?.includes("pro") | ||
| ? `"https://limeplay.winoffrg.dev/r/pro/${item.name}.json?token=<token>"` |
There was a problem hiding this comment.
P0: The ?token=<token> in the pro-block install URL is a placeholder that never gets resolved — users will literally copy ?token=<token> to their clipboard, and running that command will fail with an auth error. This blocks installation of all pro blocks (ambient-player, immersive-player). Either wire in an actual token (from env, config, or user input) or remove the placeholder logic until auth is ready.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/components/block-viewer.tsx, line 489:
<comment>The `?token=<token>` in the pro-block install URL is a placeholder that never gets resolved — users will literally copy `?token=<token>` to their clipboard, and running that command will fail with an auth error. This blocks installation of all pro blocks (ambient-player, immersive-player). Either wire in an actual token (from env, config, or user input) or remove the placeholder logic until auth is ready.</comment>
<file context>
@@ -483,6 +484,14 @@ function BlockViewerView() {
+function getRegistryInstallCommand(item: RegistryItem) {
+ const itemName = item.categories?.includes("pro")
+ ? `"https://limeplay.winoffrg.dev/r/pro/${item.name}.json?token=<token>"`
+ : `@limeplay/${item.name}`
+
</file context>
| onCopy() | ||
| } | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
P1: Rapid successive copies within the timeout window cause premature isCopied reset. Each copyToClipboard call schedules a new timeout without clearing the previous one: if the user copies at t=0 and t=500ms (with default 2000ms timeout), the first timeout fires at t=2000ms and flips isCopied to false, even though the second copy's timeout should keep it true until t=2500ms. Track the timeout ID in a ref and clear it before scheduling a new one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/hooks/use-copy-to-clipboard.ts, line 31:
<comment>Rapid successive copies within the timeout window cause premature `isCopied` reset. Each `copyToClipboard` call schedules a new timeout without clearing the previous one: if the user copies at t=0 and t=500ms (with default 2000ms timeout), the first timeout fires at t=2000ms and flips `isCopied` to false, even though the second copy's timeout should keep it true until t=2500ms. Track the timeout ID in a ref and clear it before scheduling a new one.</comment>
<file context>
@@ -0,0 +1,38 @@
+ onCopy()
+ }
+
+ setTimeout(() => {
+ setIsCopied(false)
+ }, timeout)
</file context>
| title: Ambient Player | ||
| description: A pro video player block that layers YouTube-style cinematic ambient lighting behind the standard Limeplay video player. | ||
| component: ambient-player | ||
| preview: ambient-player |
There was a problem hiding this comment.
P1: The preview: ambient-player frontmatter references a preview component whose source file doesn't exist yet. The block-showcase tries to import from @/registry/pro/blocks/ambient-player/player, but registry/pro/blocks/ has no files. Either create the source component as part of this PR or remove/skip the preview field until the component is ready to avoid a broken doc page.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/content/docs/blocks/ambient-player.mdx, line 5:
<comment>The `preview: ambient-player` frontmatter references a preview component whose source file doesn't exist yet. The block-showcase tries to import from `@/registry/pro/blocks/ambient-player/player`, but `registry/pro/blocks/` has no files. Either create the source component as part of this PR or remove/skip the preview field until the component is ready to avoid a broken doc page.</comment>
<file context>
@@ -0,0 +1,66 @@
+title: Ambient Player
+description: A pro video player block that layers YouTube-style cinematic ambient lighting behind the standard Limeplay video player.
+component: ambient-player
+preview: ambient-player
+registry: pro
+status: pro
</file context>
| process.env.VERCEL_ENV === "preview" | ||
| ? `https://${process.env.VERCEL_URL ?? "limeplay.winoffrg.dev"}` | ||
| : "https://limeplay.winoffrg.dev" | ||
| export const LIMEPLAY_API_BASE_URL = "https://limeplay.winoffrg.workers.dev" |
There was a problem hiding this comment.
P2: LIMEPLAY_API_BASE_URL is hardcoded to a production Workers URL with no environment-variable override. In local development this will hit the production API, and there is no way to point to a local or preview API instance. Consider following the pattern used by PROD_BASE_HOST and allowing an process.env.LIMEPLAY_API_BASE_URL override.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/lib/constants.ts, line 11:
<comment>`LIMEPLAY_API_BASE_URL` is hardcoded to a production Workers URL with no environment-variable override. In local development this will hit the production API, and there is no way to point to a local or preview API instance. Consider following the pattern used by `PROD_BASE_HOST` and allowing an `process.env.LIMEPLAY_API_BASE_URL` override.</comment>
<file context>
@@ -8,3 +8,4 @@ export const PROD_BASE_HOST =
process.env.VERCEL_ENV === "preview"
? `https://${process.env.VERCEL_URL ?? "limeplay.winoffrg.dev"}`
: "https://limeplay.winoffrg.dev"
+export const LIMEPLAY_API_BASE_URL = "https://limeplay.winoffrg.workers.dev"
</file context>
| export const LIMEPLAY_API_BASE_URL = "https://limeplay.winoffrg.workers.dev" | |
| export const LIMEPLAY_API_BASE_URL = process.env.LIMEPLAY_API_BASE_URL ?? "https://limeplay.winoffrg.workers.dev" |
| @@ -2,6 +2,8 @@ import type { ReactNode } from "react" | |||
|
|
|||
There was a problem hiding this comment.
P2: the ambient-player and immersive-player showcase entries are structurally identical, and all three video-player entries share the same wrapper layout — consider extracting a shared helper like renderVideoBlock(Component) to reduce duplication and keep future block additions consistent
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/components/blocks/block-showcase.tsx, line 22:
<comment>the ambient-player and immersive-player showcase entries are structurally identical, and all three video-player entries share the same wrapper layout — consider extracting a shared helper like `renderVideoBlock(Component)` to reduce duplication and keep future block additions consistent</comment>
<file context>
@@ -12,6 +14,19 @@ type BlockShowcaseDefinition = {
+ <BlockPreviewWithToolbar>
+ <div className="flex size-full">
+ <BlockPreviewPane>
+ <AmbientPlayer layout="fill">
+ <BlockStreamSync playerType="video" />
+ </AmbientPlayer>
</file context>
| if (outputDir) { | ||
| const entries = await fs.readdir(outputDir) | ||
| for (const entry of entries) { | ||
| if (!entry.endsWith(".json")) continue |
There was a problem hiding this comment.
P2: The generated proRegistryItems lookup can include a bogus registry entry because this loop imports every .json file from the shadcn output, including the tier-level registry.json that validateTierRegistries() expects in registry/.generated/pro. Skipping registry.json here would keep the runtime module limited to actual pro item JSON files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/scripts/registry-dev.mts, line 435:
<comment>The generated `proRegistryItems` lookup can include a bogus `registry` entry because this loop imports every `.json` file from the shadcn output, including the tier-level `registry.json` that `validateTierRegistries()` expects in `registry/.generated/pro`. Skipping `registry.json` here would keep the runtime module limited to actual pro item JSON files.</comment>
<file context>
@@ -401,6 +418,47 @@ function startWatcher() {
+ if (outputDir) {
+ const entries = await fs.readdir(outputDir)
+ for (const entry of entries) {
+ if (!entry.endsWith(".json")) continue
+
+ const filePath = path.join(outputDir, entry)
</file context>
| if (!entry.endsWith(".json")) continue | |
| if (!entry.endsWith(".json") || entry === "registry.json") continue |
![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
🧹 Nitpick comments (1)
apps/www/registry/default/hooks/use-playback.ts (1)
347-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove commented-out dead code; keep a concise TODO instead.
The full
stalledHandlerimplementation is commented out along with its registration and cleanup. Dead code in comments degrades maintainability and can silently rot. Replace the block with a brief TODO referencing the Safari issue and the URL, then delete the implementation.♻️ Proposed cleanup
- /** - * - TODO: Breaks on Safari, stalled is not handled correctly - `@ref`: https://medium.com/@nathan5x/event-lifecycle-of-html-video-element-part-1-f63373c981d3#:~:text=Lifecycle%20in%20Safari-,Safari,-%2C%20in%20general%2C%20is - - const stalledHandler = () => { - const prevStatus = store.getState().playback.status - - store.setState(({ playback }) => { - playback.status = "buffering" - }) - - events.emit("buffering", { isBuffering: true }) - events.emit("statuschange", { prevStatus, status: "buffering" }) - } - */ + // TODO: "stalled" handler disabled — breaks on Safari due to stalled lifecycle issues. + // Ref: https://medium.com/@nathan5x/event-lifecycle-of-html-video-element-part-1-f63373c981d3#:~:text=Lifecycle%20in%20Safari-,Safari,-%2C%20in%20general%2C%20isAnd remove the commented-out subscription/cleanup lines:
- // on(media, "stalled", stalledHandler)- // off(media, "stalled", stalledHandler)Also applies to: 378-378, 394-394
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/registry/default/hooks/use-playback.ts` around lines 347 - 362, Remove the commented-out stalled handling dead code around use-playback’s Safari TODO, including the stalledHandler implementation and its commented registration/cleanup, and replace it with a short TODO note referencing the Safari stalled-event issue and the linked URL. Keep the reference in use-playback’s playback event handling area so the existing stalled logic can be restored later without leaving commented implementation blocks behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/www/hooks/use-copy-to-clipboard.ts`:
- Around line 31-33: The copy-state reset in useCopyToClipboard is using an
untracked setTimeout, which can race and leak. Update the useCopyToClipboard
hook to store the timeout ID in a ref, clear any existing timer before
scheduling a new one when copying, and add cleanup on unmount to cancel the
pending timer. Keep the fix localized around the setTimeout/setIsCopied(false)
logic in useCopyToClipboard so repeated copies and component teardown are
handled safely.
---
Nitpick comments:
In `@apps/www/registry/default/hooks/use-playback.ts`:
- Around line 347-362: Remove the commented-out stalled handling dead code
around use-playback’s Safari TODO, including the stalledHandler implementation
and its commented registration/cleanup, and replace it with a short TODO note
referencing the Safari stalled-event issue and the linked URL. Keep the
reference in use-playback’s playback event handling area so the existing stalled
logic can be restored later without leaving commented implementation blocks
behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f009cff-f2c6-4f94-ba09-913446cf4046
⛔ Files ignored due to path filters (6)
.gitignoreis excluded by none and included by noneapps/www/package.jsonis excluded by none and included by noneapps/www/scripts/registry-dev.mtsis excluded by none and included by noneapps/www/scripts/validate-registries.tsis excluded by none and included by noneapps/www/vercel.jsonis excluded by none and included by nonebun.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (21)
apps/www/components/block-viewer.tsxapps/www/components/blocks/block-showcase.tsxapps/www/components/copy-button.tsxapps/www/components/hero-buttons.tsxapps/www/components/mdx-components.tsxapps/www/components/mdx/pro-installation-note.tsxapps/www/components/players/audio-player/demo.tsapps/www/content/docs/blocks/ambient-player.mdxapps/www/content/docs/blocks/immersive-player.mdxapps/www/content/docs/blocks/meta.jsonapps/www/hooks/use-copy-to-clipboard.tsapps/www/lib/catalogs/apple-music.tsapps/www/lib/catalogs/blender-open-films.tsapps/www/lib/constants.tsapps/www/registry/collection/registry-blocks.tsapps/www/registry/collection/registry-ui.tsapps/www/registry/default/blocks/audio-player/components/fixed-timeline-control.tsxapps/www/registry/default/blocks/audio-player/components/volume-group-control.tsxapps/www/registry/default/blocks/video-player/player.tsxapps/www/registry/default/hooks/use-playback.tsapps/www/registry/pro
| setTimeout(() => { | ||
| setIsCopied(false) | ||
| }, timeout) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear previous timeout to prevent race condition and timer leak.
The setTimeout is never tracked or cleared. If a user copies again before the timeout fires, the earlier timeout will prematurely reset isCopied to false, causing a visual flicker. Additionally, if the component unmounts, the orphaned timer leaks. Store the timeout ID in a ref, clear any existing timer before setting a new one, and clean up on unmount.
🛡️ Proposed fix
export function useCopyToClipboard({
onCopy,
timeout = 2000,
}: {
onCopy?: () => void
timeout?: number
} = {}) {
const [isCopied, setIsCopied] = React.useState(false)
+ const timeoutRef = React.useRef<ReturnType<typeof setTimeout> | null>(null)
+
+ React.useEffect(() => {
+ return () => {
+ if (timeoutRef.current) clearTimeout(timeoutRef.current)
+ }
+ }, [])
const copyToClipboard = (value: string) => {
if (typeof window === "undefined") {
return
}
const clipboard = "clipboard" in navigator ? navigator.clipboard : null
if (!clipboard) return
if (!value) return
clipboard.writeText(value).then(() => {
setIsCopied(true)
if (onCopy) {
onCopy()
}
+ if (timeoutRef.current) clearTimeout(timeoutRef.current)
setTimeout(() => {
setIsCopied(false)
+ timeoutRef.current = null
}, timeout)
}, console.error)
}
return { copyToClipboard, isCopied }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/www/hooks/use-copy-to-clipboard.ts` around lines 31 - 33, The copy-state
reset in useCopyToClipboard is using an untracked setTimeout, which can race and
leak. Update the useCopyToClipboard hook to store the timeout ID in a ref, clear
any existing timer before scheduling a new one when copying, and add cleanup on
unmount to cancel the pending timer. Keep the fix localized around the
setTimeout/setIsCopied(false) logic in useCopyToClipboard so repeated copies and
component teardown are handled safely.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/www/package.json">
<violation number="1" location="apps/www/package.json:72">
P2: The dependency change still leaves installs on `shaka-player@5.1.13`: `^5.1.12` permits 5.1.13, and `bun.lock` is still resolved to 5.1.13. If this change is meant to roll back Shaka to 5.1.12, it would be safer to pin `"shaka-player": "5.1.12"` and regenerate/commit `bun.lock` so CI and deployments actually use that version.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "remark-gfm": "^4.0.1", | ||
| "remark-rehype": "^11.1.2", | ||
| "shaka-player": "^4.16.34", | ||
| "shaka-player": "^5.1.12", |
There was a problem hiding this comment.
P2: The dependency change still leaves installs on shaka-player@5.1.13: ^5.1.12 permits 5.1.13, and bun.lock is still resolved to 5.1.13. If this change is meant to roll back Shaka to 5.1.12, it would be safer to pin "shaka-player": "5.1.12" and regenerate/commit bun.lock so CI and deployments actually use that version.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/package.json, line 72:
<comment>The dependency change still leaves installs on `shaka-player@5.1.13`: `^5.1.12` permits 5.1.13, and `bun.lock` is still resolved to 5.1.13. If this change is meant to roll back Shaka to 5.1.12, it would be safer to pin `"shaka-player": "5.1.12"` and regenerate/commit `bun.lock` so CI and deployments actually use that version.</comment>
<file context>
@@ -69,7 +69,7 @@
"remark-gfm": "^4.0.1",
"remark-rehype": "^11.1.2",
- "shaka-player": "^5.1.13",
+ "shaka-player": "^5.1.12",
"three": "^0.184.0",
"ts-morph": "27.0.2",
</file context>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/www/registry/default/hooks/use-captions.ts (1)
59-75: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the toggle logic to prevent always hiding the captions.
The current implementation unconditionally calls
player.selectTextTrack(null)at the end of the function, effectively overriding any attempt to turn the captions on. The nullification must be wrapped in anelseblock to ensure it only turns off the captions if they are already active.🐛 Proposed fix for the toggle logic
if (!captions.activeTrack) { const defaultTrack = findDefaultTrack(captions.tracks) if (defaultTrack) { player.selectTextTrack(defaultTrack) const activeTrack = player .getTextTracks() .find((track: shaka.extern.TextTrack) => track.active) set(({ captions }) => { captions.activeTrack = activeTrack ?? null }) } - } - - player.selectTextTrack(null) + } else { + player.selectTextTrack(null) + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/registry/default/hooks/use-captions.ts` around lines 59 - 75, Update the caption toggle logic so player.selectTextTrack(null) executes only in the else branch when captions.activeTrack is already active; keep the existing default-track selection and activeTrack state update in the if (!captions.activeTrack) branch.
🧹 Nitpick comments (1)
apps/www/registry/default/hooks/use-captions.ts (1)
114-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the debug artifact.
This
console.logstatement was likely left behind during development and should be removed.♻️ Proposed fix
- console.log("Active track changed:", activeTrack)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/registry/default/hooks/use-captions.ts` at line 114, Remove the development-only console.log statement from the active track change handling in use-captions.ts, leaving the surrounding activeTrack logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/www/registry/default/hooks/use-captions.ts`:
- Around line 59-75: Update the caption toggle logic so
player.selectTextTrack(null) executes only in the else branch when
captions.activeTrack is already active; keep the existing default-track
selection and activeTrack state update in the if (!captions.activeTrack) branch.
---
Nitpick comments:
In `@apps/www/registry/default/hooks/use-captions.ts`:
- Line 114: Remove the development-only console.log statement from the active
track change handling in use-captions.ts, leaving the surrounding activeTrack
logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7f0b854-cb85-4c35-a8d6-f936c5bbd93f
📒 Files selected for processing (2)
apps/www/registry/default/examples/captions-state-control-demo.tsxapps/www/registry/default/hooks/use-captions.ts
💤 Files with no reviewable changes (1)
- apps/www/registry/default/examples/captions-state-control-demo.tsx
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/www/registry/default/hooks/use-captions.ts">
<violation number="1" location="apps/www/registry/default/hooks/use-captions.ts:114">
P3: Caption changes emit a production console message, exposing track metadata and creating noisy logs during normal playback. Remove this debugging statement.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| .getTextTracks() | ||
| .find((track: shaka.extern.TextTrack) => track.active) | ||
|
|
||
| console.log("Active track changed:", activeTrack) |
There was a problem hiding this comment.
P3: Caption changes emit a production console message, exposing track metadata and creating noisy logs during normal playback. Remove this debugging statement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/registry/default/hooks/use-captions.ts, line 114:
<comment>Caption changes emit a production console message, exposing track metadata and creating noisy logs during normal playback. Remove this debugging statement.</comment>
<file context>
@@ -111,8 +111,16 @@ function CaptionsSetup() {
.getTextTracks()
.find((track: shaka.extern.TextTrack) => track.active)
+ console.log("Active track changed:", activeTrack)
+
store.setState(({ captions }) => {
</file context>
ea45063 to
25ee01c
Compare
3471312 to
55d79a3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
55d79a3 to
99e6ccd
Compare
|
Note The previously reviewed commits are no longer reachable (likely due to a force-push or rebase), so CodeRabbit is performing a full review instead of an incremental one. This review may take a little longer. |
There was a problem hiding this comment.
2 existing issues remain and 6 new issues found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/www/registry/default/hooks/use-playback.ts">
<violation number="1" location="apps/www/registry/default/hooks/use-playback.ts:378">
P1: When the media element emits `stalled` without `waiting`, playback stays in `playing`/`paused` and buffering UI/events never update. Re-enable stalled handling instead of removing the listener so stalled network interruptions transition status to `buffering` consistently.</violation>
</file>
<file name="apps/www/content/docs/blocks/ambient-player.mdx">
<violation number="1" location="apps/www/content/docs/blocks/ambient-player.mdx:60">
P2: The AutoTypeTable path `./registry/pro/blocks/ambient-player/player.tsx` resolves relative to this doc, but `apps/www/registry/pro` is empty, so the referenced source file does not exist and the API reference will not render. Point it at the real source location (or ship the player source in this PR), matching `video-player.mdx`, which references a file that actually exists under `./registry/default/blocks/...`.</violation>
</file>
<file name="apps/www/components/copy-button.tsx">
<violation number="1" location="apps/www/components/copy-button.tsx:87">
P2: The new `useCopyToClipboard` hook's `copyToClipboard` (apps/www/hooks/use-copy-to-clipboard.ts) returns `void`, never throws, and swallows failures via `.then(..., console.error)`. In `handleCopy` the call is not awaited and sits in a `try/catch` that expects a throwing/Promise call, then unconditionally flips status to "copied" after 100ms. So when `navigator.clipboard.writeText` fails (permissions denied, non-secure context), the button never runs the catch and still shows the success checkmark, misreporting a failed copy as successful. The refactor removed the ability to await the write (react-use returned a rejectable Promise); return a promise from `copyToClipboard` and `await copyToClipboard(...)` in `handleCopy`, moving `setStatus("copied")` into the success path.</violation>
</file>
<file name="apps/www/registry/default/hooks/use-captions.ts">
<violation number="1" location="apps/www/registry/default/hooks/use-captions.ts:74">
P1: On the first captions-control click with no active track, the handler selects the default track and immediately unloads it, so captions never turn on. Keep the final call as `player.setTextTrackVisibility(!captions.visible)` to toggle rendering without clearing the selected track.</violation>
</file>
<file name="apps/www/scripts/registry-dev.mts">
<violation number="1" location="apps/www/scripts/registry-dev.mts:361">
P2: Pro dependencies are now emitted as `@limeplay/pro/media-ambient`, but shadcn documents namespaced dependencies as `@namespace/item-name` (two segments). Verify shadcn v4 resolves the extra `pro/` path segment to `<registry-url>/r/pro/media-ambient.json`; if it only parses the first segment after `@`, pro dependency resolution breaks. If unverified, emit the explicit URL instead.</violation>
<violation number="2" location="apps/www/scripts/registry-dev.mts:450">
P3: `writeProRuntimeModule` generates `registry/__pro__.ts`, but nothing in the repo imports `proRegistryItems`; the docs site already gets pro items from the `pro` entry in `registry/__index__.tsx`. Either wire up the consumer or drop the module until it is used.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
| on(media, "ended", endedHandler) | ||
| on(media, "waiting", waitingHandler) | ||
| on(media, "stalled", stalledHandler) | ||
| // on(media, "stalled", stalledHandler) |
There was a problem hiding this comment.
P1: When the media element emits stalled without waiting, playback stays in playing/paused and buffering UI/events never update. Re-enable stalled handling instead of removing the listener so stalled network interruptions transition status to buffering consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/registry/default/hooks/use-playback.ts, line 378:
<comment>When the media element emits `stalled` without `waiting`, playback stays in `playing`/`paused` and buffering UI/events never update. Re-enable stalled handling instead of removing the listener so stalled network interruptions transition status to `buffering` consistently.</comment>
<file context>
@@ -369,7 +375,7 @@ function PlaybackSetup() {
on(media, "ended", endedHandler)
on(media, "waiting", waitingHandler)
- on(media, "stalled", stalledHandler)
+ // on(media, "stalled", stalledHandler)
on(media, "error", errorHandler)
</file context>
| } | ||
|
|
||
| player.setTextTrackVisibility(!captions.visible) | ||
| player.selectTextTrack(null) |
There was a problem hiding this comment.
P1: On the first captions-control click with no active track, the handler selects the default track and immediately unloads it, so captions never turn on. Keep the final call as player.setTextTrackVisibility(!captions.visible) to toggle rendering without clearing the selected track.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/registry/default/hooks/use-captions.ts, line 74:
<comment>On the first captions-control click with no active track, the handler selects the default track and immediately unloads it, so captions never turn on. Keep the final call as `player.setTextTrackVisibility(!captions.visible)` to toggle rendering without clearing the selected track.</comment>
<file context>
@@ -71,7 +71,7 @@ export function captionsFeature(): MediaFeature<
}
- player.setTextTrackVisibility(!captions.visible)
+ player.selectTextTrack(null)
},
tracks: undefined,
</file context>
| player.selectTextTrack(null) | |
| player.setTextTrackVisibility(!captions.visible) |
| ## API Reference | ||
|
|
||
| <AutoTypeTable | ||
| path="./registry/pro/blocks/ambient-player/player.tsx" |
There was a problem hiding this comment.
P2: The AutoTypeTable path ./registry/pro/blocks/ambient-player/player.tsx resolves relative to this doc, but apps/www/registry/pro is empty, so the referenced source file does not exist and the API reference will not render. Point it at the real source location (or ship the player source in this PR), matching video-player.mdx, which references a file that actually exists under ./registry/default/blocks/....
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/content/docs/blocks/ambient-player.mdx, line 60:
<comment>The AutoTypeTable path `./registry/pro/blocks/ambient-player/player.tsx` resolves relative to this doc, but `apps/www/registry/pro` is empty, so the referenced source file does not exist and the API reference will not render. Point it at the real source location (or ship the player source in this PR), matching `video-player.mdx`, which references a file that actually exists under `./registry/default/blocks/...`.</comment>
<file context>
@@ -0,0 +1,66 @@
+## API Reference
+
+<AutoTypeTable
+ path="./registry/pro/blocks/ambient-player/player.tsx"
+ name="AmbientPlayerProps"
+/>
</file context>
| const [leaveDirection, setLeaveDirection] = useState({ x: 0, y: 0 }) | ||
| const buttonRef = useRef<HTMLButtonElement>(null) | ||
| const [, copyToClipboard] = useCopyToClipboard() | ||
| const { copyToClipboard } = useCopyToClipboard() |
There was a problem hiding this comment.
P2: The new useCopyToClipboard hook's copyToClipboard (apps/www/hooks/use-copy-to-clipboard.ts) returns void, never throws, and swallows failures via .then(..., console.error). In handleCopy the call is not awaited and sits in a try/catch that expects a throwing/Promise call, then unconditionally flips status to "copied" after 100ms. So when navigator.clipboard.writeText fails (permissions denied, non-secure context), the button never runs the catch and still shows the success checkmark, misreporting a failed copy as successful. The refactor removed the ability to await the write (react-use returned a rejectable Promise); return a promise from copyToClipboard and await copyToClipboard(...) in handleCopy, moving setStatus("copied") into the success path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/components/copy-button.tsx, line 87:
<comment>The new `useCopyToClipboard` hook's `copyToClipboard` (apps/www/hooks/use-copy-to-clipboard.ts) returns `void`, never throws, and swallows failures via `.then(..., console.error)`. In `handleCopy` the call is not awaited and sits in a `try/catch` that expects a throwing/Promise call, then unconditionally flips status to "copied" after 100ms. So when `navigator.clipboard.writeText` fails (permissions denied, non-secure context), the button never runs the catch and still shows the success checkmark, misreporting a failed copy as successful. The refactor removed the ability to await the write (react-use returned a rejectable Promise); return a promise from `copyToClipboard` and `await copyToClipboard(...)` in `handleCopy`, moving `setStatus("copied")` into the success path.</comment>
<file context>
@@ -84,7 +84,7 @@ export const CopyButton: React.FC<CopyButtonProps> = ({
const [leaveDirection, setLeaveDirection] = useState({ x: 0, y: 0 })
const buttonRef = useRef<HTMLButtonElement>(null)
- const [, copyToClipboard] = useCopyToClipboard()
+ const { copyToClipboard } = useCopyToClipboard()
const handleCopy = async () => {
</file context>
| const item = registryItemsMap.get(dependency)! | ||
| if (isProItem(item)) { | ||
| return `${PRO_BASE_URL}/${dependency}.json` | ||
| return `${LIMEPLAY_REGISTRY_NAME}/pro/${dependency}` |
There was a problem hiding this comment.
P2: Pro dependencies are now emitted as @limeplay/pro/media-ambient, but shadcn documents namespaced dependencies as @namespace/item-name (two segments). Verify shadcn v4 resolves the extra pro/ path segment to <registry-url>/r/pro/media-ambient.json; if it only parses the first segment after @, pro dependency resolution breaks. If unverified, emit the explicit URL instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/scripts/registry-dev.mts, line 361:
<comment>Pro dependencies are now emitted as `@limeplay/pro/media-ambient`, but shadcn documents namespaced dependencies as `@namespace/item-name` (two segments). Verify shadcn v4 resolves the extra `pro/` path segment to `<registry-url>/r/pro/media-ambient.json`; if it only parses the first segment after `@`, pro dependency resolution breaks. If unverified, emit the explicit URL instead.</comment>
<file context>
@@ -333,17 +358,13 @@ function resolveRegistryDependency(dependency: string): string {
const item = registryItemsMap.get(dependency)!
if (isProItem(item)) {
- return `${PRO_BASE_URL}/${dependency}.json`
+ return `${LIMEPLAY_REGISTRY_NAME}/pro/${dependency}`
}
return `${BASE_URL}/${dependency}.json`
</file context>
| return `${LIMEPLAY_REGISTRY_NAME}/pro/${dependency}` | |
| return `${REGISTRY_HOST}/r/pro/${dependency}.json` |
| // This file is autogenerated by scripts/registry-dev.mts | ||
| // Do not edit this file directly. | ||
|
|
||
| export const proRegistryItems: Record<string, unknown> = ${JSON.stringify( |
There was a problem hiding this comment.
P3: writeProRuntimeModule generates registry/__pro__.ts, but nothing in the repo imports proRegistryItems; the docs site already gets pro items from the pro entry in registry/__index__.tsx. Either wire up the consumer or drop the module until it is used.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/www/scripts/registry-dev.mts, line 450:
<comment>`writeProRuntimeModule` generates `registry/__pro__.ts`, but nothing in the repo imports `proRegistryItems`; the docs site already gets pro items from the `pro` entry in `registry/__index__.tsx`. Either wire up the consumer or drop the module until it is used.</comment>
<file context>
@@ -401,6 +418,47 @@ function startWatcher() {
+// This file is autogenerated by scripts/registry-dev.mts
+// Do not edit this file directly.
+
+export const proRegistryItems: Record<string, unknown> = ${JSON.stringify(
+ items,
+ null,
</file context>
1 participant
Summary by CodeRabbit