Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
Note
|
| Cohort / File(s) | Summary |
|---|---|
Registry Updates apps/www/registry/collection/registry-blocks.ts, apps/www/registry/collection/registry-ui.ts |
Added new "youtube-music" block entry with full configuration including metadata, dependencies, and file definitions. Removed the "pip-control" UI registry entry. |
Timeline Control Components apps/www/registry/default/ui/timeline-control.tsx, apps/www/registry/default/blocks/linear-player/components/timeline-slider-control.tsx |
Simplified Root className composition with DEV notes indicating unopinionated migration. Updated Progress component styling and applied rounded-s-full class to TimelineSlider.Progress. |
Asset Hook apps/www/registry/default/hooks/use-asset.ts |
Disabled automatic playlist advancement on error by commenting out playlist.next() call in the onError handler. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
- fix: base ui dependency in select #129 — Modifies registry collection entries for block dependencies, sharing the same registry configuration pattern with this PR's block additions and removals.
Poem
🐰 Hoppy tunes now play,
pip-control hops away,
Timeline rounded bright,
Registry takes flight,
Code made cleaner today! 🎵
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The title 'feat: pro components handling' is vague and does not clearly describe the specific changes made in this PR. | Consider using a more specific title that describes the actual changes, such as 'feat: add youtube-music block and update pro components' or 'feat: reorganize registry blocks and ui components'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/pro-blocks
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/www/registry/default/blocks/linear-player/components/timeline-slider-control.tsx (1)
1-1:⚠️ Potential issue | 🔴 CriticalFix styling violations: use CSS variables instead of hardcoded colors.
Line 35:
bg-whiteshould use an approved CSS variable instead of a hardcoded color.Line 95:
bg-red-600should use an approved CSS variable instead of a Tailwind color.Line 41:
top-auto!uses a forced property override; confirm this is intentional and cannot be solved through proper component composition.
🤖 Fix all issues with AI agents
In `@apps/www/registry/collection/registry-blocks.ts`:
- Around line 169-264: The registry entry for the "youtube-music" block declares
many files under blocks/youtube-music (e.g.,
blocks/youtube-music/components/media-player.tsx and
blocks/youtube-music/lib/create-media-store.ts) but those files and the
apps/www/registry/pro/blocks/ directory are missing, causing imports like
pro-youtube-music-player.tsx ->
"@/registry/pro/blocks/youtube-music/components/media-player" to fail; fix by
either (A) creating the declared files and matching directory
(blocks/youtube-music/*) with the exported components and a
lib/create-media-store implementation and ensure pro-youtube-music-player.tsx
imports the correct module paths, or (B) remove or disable the youtube-music
entry from the registry block list and update pro-youtube-music-player.tsx to
stop importing the missing module (or point it to an existing replacement), then
run the build to verify no unresolved imports remain.
There was a problem hiding this comment.
Pull request overview
Adds “pro” tier handling to the registry and begins wiring a new pro “YouTube Music” player into the site UI.
Changes:
- Update pro-registry detection logic to look for any TS/TSX files under
registry/pro(recursive). - Introduce a new
youtube-musicpro block entry in the registry collection and remove an existing pro UI entry. - Adjust several UI pieces (timeline slider + toggle-group) and add a new home-page embed for the pro player.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/www/scripts/build-registry.ts | Updates pro-submodule detection to determine whether to build the pro registry tier. |
| apps/www/registry/default/ui/timeline-control.tsx | Tweaks timeline slider styling (currently with commented/TODO styles). |
| apps/www/registry/default/hooks/use-asset.ts | Changes playlist error behavior by disabling auto-advance. |
| apps/www/registry/default/blocks/linear-player/components/timeline-slider-control.tsx | Adjusts progress rounding via a passed className. |
| apps/www/registry/collection/registry-ui.ts | Removes a pro UI item (pip-control) from the collection. |
| apps/www/registry/collection/registry-blocks.ts | Adds a new pro block definition for youtube-music. |
| apps/www/components/ui/toggle-group.tsx | Extends toggle-group API with orientation/spacing and updates styling. |
| apps/www/components/pro-youtube-music-player.tsx | Adds a client component that dynamically imports the pro YouTube Music player. |
| apps/www/app/(home)/page.tsx | Renders the pro YouTube Music player on the home page. |
| className | ||
| )} | ||
| // DEV: Opinidated animations and border without variable | ||
| // TODO: Uncomment and migrate to unopininated before PR |
There was a problem hiding this comment.
Typo in comment: "unopininated" should be "unopinionated".
| // TODO: Uncomment and migrate to unopininated before PR | |
| // TODO: Uncomment and migrate to unopinionated before PR |
| console.error("[useAsset] Playlist error:", item.id, error) | ||
|
|
||
| playlist.next() | ||
| // playlist.next() |
There was a problem hiding this comment.
Commenting out playlist.next() in onError changes behavior so a failing item will no longer be skipped, which can stall playback/queue progression. If the intent is to stop auto-advancing on errors, replace this with explicit error state handling; otherwise restore the playlist.next() call.
| // playlist.next() | |
| if (playlist.hasNext()) { | |
| playlist.next() | |
| } |
| group/toggle-group flex w-fit flex-row items-center gap-[--spacing(var(--gap))] rounded-lg | ||
| data-[orientation=vertical]:flex-col data-[orientation=vertical]:items-stretch | ||
| data-[size=sm]:rounded-[min(var(--radius-md),10px)] |
There was a problem hiding this comment.
gap-[--spacing(var(--gap))] is not a valid Tailwind arbitrary value expression (the --spacing() token is used with numeric literals elsewhere, e.g. (--spacing(4))). As written, this is likely to generate invalid CSS and the spacing prop won’t apply. Consider using a supported pattern (e.g. gap-[var(--gap)] with a length value, or a Tailwind spacing token) and ensure --gap includes units if it’s meant to be a length.
| data-orientation={orientation} | ||
| data-size={size} | ||
| data-slot="toggle-group" | ||
| data-spacing={spacing} | ||
| data-variant={variant} |
There was a problem hiding this comment.
The new orientation prop is only reflected via data-orientation, but it is not passed to ToggleGroupPrimitive.Root as orientation={orientation}. Radix uses orientation for correct keyboard navigation/roving focus behavior, so this should be forwarded (or rename the prop if it’s styling-only).
| group-data-horizontal/toggle-group:data-[spacing=0]:first:rounded-l-lg | ||
| group-data-vertical/toggle-group:data-[spacing=0]:first:rounded-t-lg | ||
| group-data-horizontal/toggle-group:data-[spacing=0]:last:rounded-r-lg | ||
| group-data-vertical/toggle-group:data-[spacing=0]:last:rounded-b-lg | ||
| group-data-horizontal/toggle-group:data-[spacing=0]:data-[variant=outline]:border-l-0 | ||
| group-data-vertical/toggle-group:data-[spacing=0]:data-[variant=outline]:border-t-0 | ||
| group-data-horizontal/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-l | ||
| group-data-vertical/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-t |
There was a problem hiding this comment.
These variants (group-data-horizontal/... and group-data-vertical/...) don’t correspond to any data-* attribute set on the group, so they won’t ever match. Elsewhere in the codebase, the established pattern is group-data-[attr=value]/... (e.g. group-data-[collapsible=icon] in components/ui/sidebar.tsx). Consider switching these to group-data-[orientation=horizontal]/toggle-group: and group-data-[orientation=vertical]/toggle-group: (and apply the same fix to the subsequent border variants).
| group-data-horizontal/toggle-group:data-[spacing=0]:first:rounded-l-lg | |
| group-data-vertical/toggle-group:data-[spacing=0]:first:rounded-t-lg | |
| group-data-horizontal/toggle-group:data-[spacing=0]:last:rounded-r-lg | |
| group-data-vertical/toggle-group:data-[spacing=0]:last:rounded-b-lg | |
| group-data-horizontal/toggle-group:data-[spacing=0]:data-[variant=outline]:border-l-0 | |
| group-data-vertical/toggle-group:data-[spacing=0]:data-[variant=outline]:border-t-0 | |
| group-data-horizontal/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-l | |
| group-data-vertical/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-t | |
| group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:first:rounded-l-lg | |
| group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:first:rounded-t-lg | |
| group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:last:rounded-r-lg | |
| group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:last:rounded-b-lg | |
| group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:data-[variant=outline]:border-l-0 | |
| group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:data-[variant=outline]:border-t-0 | |
| group-data-[orientation=horizontal]/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-l | |
| group-data-[orientation=vertical]/toggle-group:data-[spacing=0]:data-[variant=outline]:first:border-t |
| import { ProYouTubeMusicPlayer } from "@/components/pro-youtube-music-player" | ||
| import { ScrollIndicator } from "@/components/scroll-indicator" |
There was a problem hiding this comment.
Importing ProYouTubeMusicPlayer here will break builds where the apps/www/registry/pro submodule isn’t checked out, because the component currently references @/registry/pro/... modules that won’t resolve at compile time. Please gate this behind a mechanism that doesn’t require the pro module to exist (or remove it from the default home page).
| // DEV: Opinidated animations and border without variable | ||
| // TODO: Uncomment and migrate to unopininated before PR | ||
| // className={cn( | ||
| // ` | ||
| // relative h-1 rounded-full transition-[height] duration-150 ease-out-quad | ||
| // data-[orientation=horizontal]:h-(--lp-timeline-track-height) | ||
| // active:data-[orientation=horizontal]:h-(--lp-timeline-track-height-active) | ||
| // `, | ||
| // className | ||
| // )} |
There was a problem hiding this comment.
This leaves temporary DEV/TODO comments and a large commented-out className block in a shared UI component. Please remove the commented code/TODO before merging and commit the finalized styling (either restore the previous behavior or implement the intended variable-driven styles).
| // DEV: Opinidated animations and border without variable | |
| // TODO: Uncomment and migrate to unopininated before PR | |
| // className={cn( | |
| // ` | |
| // relative h-1 rounded-full transition-[height] duration-150 ease-out-quad | |
| // data-[orientation=horizontal]:h-(--lp-timeline-track-height) | |
| // active:data-[orientation=horizontal]:h-(--lp-timeline-track-height-active) | |
| // `, | |
| // className | |
| // )} |
| // DEV: Opinidated animations and border without variable | ||
| // TODO: Uncomment and migrate to unopininated before PR |
There was a problem hiding this comment.
Typo in comment: "Opinidated" should be "Opinionated".
| // DEV: Opinidated animations and border without variable | |
| // TODO: Uncomment and migrate to unopininated before PR | |
| // DEV: Opinionated animations and border without variable | |
| // TODO: Uncomment and migrate to unopinionated before PR |
| import("@/registry/pro/blocks/youtube-music/components/media-player") | ||
| .then((mod) => mod.YouTubeMusicPlayer) |
There was a problem hiding this comment.
This dynamic import points at @/registry/pro/..., but apps/www/registry/pro is a git submodule and is empty/unavailable in CI and many checkouts (and import() specifiers must still resolve at build time). As a result, Next.js will fail to compile with "module not found" even though there’s a .catch(). Consider importing through a stable wrapper that always exists (e.g. the autogenerated registry/__index__.tsx which only includes pro entries when the submodule is present), or provide a stub module in this repo so builds succeed without initializing the pro submodule.
| import("@/registry/pro/blocks/youtube-music/components/media-player") | |
| .then((mod) => mod.YouTubeMusicPlayer) | |
| import("@/registry/__index__") | |
| .then((mod) => { | |
| return mod.YouTubeMusicPlayer ?? (() => null) | |
| }) |
| files: [ | ||
| { | ||
| path: "blocks/youtube-music/lib/create-media-store.ts", | ||
| type: "registry:lib", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/media-player.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/controls.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/playback-controls.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/action-controls.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/playback-mode-controls.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/volume-group-control.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/fixed-timeline-control.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/track-info.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/playlist.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/player-hooks.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/components/icons.tsx", | ||
| type: "registry:component", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/ui/button.tsx", | ||
| type: "registry:ui", | ||
| }, | ||
| { | ||
| path: "blocks/youtube-music/youtube-music.module.css", | ||
| target: "components/youtube-music/youtube-music.module.css", | ||
| type: "registry:style", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
This new pro block entry references a set of blocks/youtube-music/... files that are not present in the current repo checkout (and apps/www/registry/pro is a submodule). If these files are expected to live in the pro submodule, the submodule commit pointer needs to be updated alongside this metadata; otherwise, enabling the pro registry build will fail when shadcn build tries to read missing files.
| files: [ | |
| { | |
| path: "blocks/youtube-music/lib/create-media-store.ts", | |
| type: "registry:lib", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/media-player.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/controls.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/playback-controls.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/action-controls.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/playback-mode-controls.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/volume-group-control.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/fixed-timeline-control.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/track-info.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/playlist.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/player-hooks.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/components/icons.tsx", | |
| type: "registry:component", | |
| }, | |
| { | |
| path: "blocks/youtube-music/ui/button.tsx", | |
| type: "registry:ui", | |
| }, | |
| { | |
| path: "blocks/youtube-music/youtube-music.module.css", | |
| target: "components/youtube-music/youtube-music.module.css", | |
| type: "registry:style", | |
| }, | |
| ], | |
| files: [], |
1 participant
Summary by CodeRabbit
New Features
Removals
Improvements