feat: pro blocks and setup#159
Open
WINOFFRG wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds Ambient Player and Immersive Player pro blocks with registry, showcase, and documentation wiring; replaces ChangesPro Blocks Feature
Copy-to-Clipboard Hook Migration
API Base URL Centralization
Player Styling and Playback Behavior Fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes 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
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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>
Suggested change
| 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>
Suggested change
| 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
Comment on lines
+31
to
+33
| 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.
2 issues 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:74">
P1: Turning captions on immediately deselects the default track because this call runs after `player.selectTextTrack(defaultTrack)`. Make deselection the active-track branch instead, so the toggle can enable captions.</violation>
<violation number="2" 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
| } | ||
|
|
||
| player.setTextTrackVisibility(!captions.visible) | ||
| player.selectTextTrack(null) |
There was a problem hiding this comment.
P1: Turning captions on immediately deselects the default track because this call runs after player.selectTextTrack(defaultTrack). Make deselection the active-track branch instead, so the toggle can enable captions.
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>Turning captions on immediately deselects the default track because this call runs after `player.selectTextTrack(defaultTrack)`. Make deselection the active-track branch instead, so the toggle can enable captions.</comment>
<file context>
@@ -71,7 +71,7 @@ export function captionsFeature(): MediaFeature<
}
- player.setTextTrackVisibility(!captions.visible)
+ player.selectTextTrack(null)
},
tracks: undefined,
</file context>
| .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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit