Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors Limeplay's media asset loading from callback-based configuration to session-scoped, event-driven orchestration. Core changes include: ChangesSource-Driven Asset Loading and Recovery Architecture
Sequence Diagram(s)sequenceDiagram
participant Block as AudioPlayer/VideoPlayer
participant PSC as PlaybackSourceController
participant UA as useAsset
participant AF as assetFeature
participant Player as ShakaPlayer
participant Events as useMediaEvents
Block->>PSC: source, loading, sourceKey
PSC->>UA: loadSource(normalizedSource, session)
UA->>AF: loadAsset/loadPlaylist/loadSource
AF->>AF: createAssetSession
AF->>Events: emit(assetloadstart)
AF->>Player: createDefaultLoad (Shaka config)
Player-->>AF: player ready
AF->>Player: player.load(src)
alt load succeeds
AF->>Events: emit(assetloaded)
else load fails
AF->>AF: recover.loadError → Retry/Skip/Stop
AF->>Events: emit(assetloaderror)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR standardizes Limeplay’s source-loading contract across blocks and hooks, shifts asset loading configuration to session-scoped loading options, and updates/expands documentation (including a new Usage guide and Mermaid diagrams) to match the new API.
Changes:
- Refactor
useAsset/usePlaybackSource/blocks to use a unifiedsource,loading, andsourceKeycontract with session-local loading configuration. - Update player and asset event payloads (notably
playerreadyandplaybackerror) and move lifecycle notifications to the media event system. - Docs overhaul: add a new Usage page, update quick start + hook/block docs, add Mermaid rendering support, and adjust docs navigation.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| bun.lock | Adds beautiful-mermaid and bumps shaka-player lock entries. |
| apps/www/registry/default/hooks/use-player.ts | Updates player event payloads (playerready, playbackerror) and emission sites. |
| apps/www/registry/default/hooks/use-playback-source.ts | Moves controller input to source + loading and calls useAsset().loadSource. |
| apps/www/registry/default/hooks/use-asset.ts | Major refactor: session-scoped loading, recovery policy API, asset lifecycle events, and loadSource. |
| apps/www/registry/default/blocks/video-player/page.tsx | Updates block usage to pass source instead of mediaProps.src. |
| apps/www/registry/default/blocks/video-player/components/playlist-navigation-controls.tsx | Adds a “Next” control for playlist navigation. |
| apps/www/registry/default/blocks/video-player/components/media-player.tsx | Updates block props to the shared source/loading API; adjusts mediaProps handling. |
| apps/www/registry/default/blocks/video-player/components/bottom-controls.tsx | Wires in the new playlist next control. |
| apps/www/registry/default/blocks/audio-player/hooks/use-playlist-asset.ts | Derives playlist items from useAsset() state instead of audio source context. |
| apps/www/registry/default/blocks/audio-player/components/track-info.tsx | Centralizes display metadata via getAudioAssetMetadata and improves empty-art fallback. |
| apps/www/registry/default/blocks/audio-player/components/playlist.tsx | Uses normalized playlist items and shared metadata; improves poster fallback. |
| apps/www/registry/default/blocks/audio-player/components/playback-mode-controls.tsx | Makes repeat-mode UI variant-aware (asset vs playlist). |
| apps/www/registry/default/blocks/audio-player/components/playback-controls.tsx | Adds showNavigation to conditionally render prev/next controls. |
| apps/www/registry/default/blocks/audio-player/components/media-player.tsx | Updates audio block props to source/loading; moves autoplay/className usage into mediaProps. |
| apps/www/registry/default/blocks/audio-player/components/controls.tsx | Renders controls conditionally based on playlist vs single-asset mode. |
| apps/www/registry/default/blocks/audio-player/components/audio-source.tsx | Replaces source context with loading defaults and adds getAudioAssetMetadata. |
| apps/www/package.json | Adds docs:generate, stops running fumadocs-mdx on postinstall, adds beautiful-mermaid, bumps shaka-player. |
| apps/www/content/docs/usage.mdx | New Usage guide covering blocks, source shapes, loading, and recovery. |
| apps/www/content/docs/quick-start.mdx | Updates quick start to include asset/playlist features and PlaybackSourceController. |
| apps/www/content/docs/hooks/use-player.mdx | Aligns docs with new event payloads and positions usePlayer as low-level. |
| apps/www/content/docs/hooks/use-playback.mdx | Updates event-system link target. |
| apps/www/content/docs/hooks/use-playback-source.mdx | Updates docs to the new source + loading model and loadSource behavior. |
| apps/www/content/docs/hooks/use-asset.mdx | Updates to optionless useAsset() and session-local loading + recovery policies. |
| apps/www/content/docs/events.mdx | Removes the standalone Events reference page. |
| apps/www/content/docs/concepts.mdx | Reworks concepts content and introduces Mermaid diagrams for architecture explanations. |
| apps/www/content/docs/components/media-provider.mdx | Updates docs link to Concepts for the event model. |
| apps/www/content/docs/blocks/video-player.mdx | Updates usage to source, expands docs around playlists/loading. |
| apps/www/content/docs/blocks/meta.json | Updates Blocks section metadata and adds explicit pages list. |
| apps/www/content/docs/blocks/audio-player.mdx | Updates usage to source, expands docs around single-track + loading. |
| apps/www/components/stream-panel/use-stream-panel-sync.ts | Migrates stream panel orchestration to asset events and session-local loading via loadSource. |
| apps/www/components/players/audio-player/demo-player.tsx | Updates demo to pass source instead of playlist. |
| apps/www/components/mdx/mermaid.tsx | Adds server-side Mermaid SVG rendering via beautiful-mermaid. |
| apps/www/components/mdx-components.tsx | Registers the Mermaid MDX component. |
| apps/www/app/layout.config.tsx | Updates docs nav links/icons and adds a “Usage” top-level entry. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 34 files
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/www/content/docs/hooks/use-player.mdx (1)
45-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove deleted
on*callback fields from theusePlayerexample.Lines [50-56] still document
onLoad/onError, which conflicts with the updated event-driven contract and can mislead users into using removed APIs.As per coding guidelines, "Never reference deleted APIs: ...
on*callbacks."Suggested patch
-const { load, preload, player } = usePlayer({ - onLoad: async (asset, player, media) => { - await player.load(asset.src) - }, - onError: (error, asset) => { - console.error("Load failed:", error) - }, -}) +const { load, preload, player } = usePlayer()🤖 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/content/docs/hooks/use-player.mdx` around lines 45 - 57, The example still passes removed onLoad/onError callback props to usePlayer; remove the onLoad and onError fields from the usePlayer call in the example (leave the const { load, preload, player } = usePlayer(...) pattern) and update the example body to demonstrate the event-driven contract by invoking player.load or load directly where appropriate (i.e., call player.load(asset.src) from an event handler or effect rather than providing onLoad/onError callbacks).Source: Coding guidelines
apps/www/components/stream-panel/use-stream-panel-sync.ts (1)
76-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an abort guard after
loadDefault().After the await on Lines 94-99, the code immediately continues into caption setup. If that load was superseded while
loadDefault()was running,addBlenderCaptions()can still mutate the stale player/session.As per coding guidelines, "Use abort/generation guards after each await in async load functions to prevent race conditions."
Suggested fix
await context.loadDefault( context.preloadManager ?? { config: context.asset.config, src: stream.playback.hls, } ) + context.signal.throwIfAborted() try { await addBlenderCaptions(context.player, stream) } catch (error) {🤖 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/components/stream-panel/use-stream-panel-sync.ts` around lines 76 - 133, The loader.load implementation calls await context.loadDefault(...) then unconditionally calls addBlenderCaptions(context.player, stream), which can race if the load was superseded; after the await context.loadDefault(...) add an abort/generation guard (e.g., if (context.signal.aborted) return; or context.signal.throwIfAborted()) before invoking addBlenderCaptions so captions are only added for the current load operation. Update the loader.load function (inside the assetOptions useMemo) to perform this check immediately after the loadDefault await and before the try/catch that calls addBlenderCaptions.Source: Coding guidelines
🤖 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/app/layout.config.tsx`:
- Around line 53-57: Update the incorrect block links that use
"/blocks/video-player" to the docs namespace "/docs/blocks/video-player": in
apps/www/app/layout.config.tsx (lines 53-57) change the navigation item's url
value for the "Blocks" entry from "/blocks/video-player" to
"/docs/blocks/video-player"; in apps/www/content/docs/quick-start.mdx (line 26)
update the inline link target from "/blocks/video-player" to
"/docs/blocks/video-player".
In `@apps/www/content/docs/concepts.mdx`:
- Around line 45-47: The example selector for useVolumeStore is using the wrong
field — change the selector from useVolumeStore((state) => state.volume) to
useVolumeStore((state) => state.volume.level) (and if desired rename the local
variable from volume to volumeLevel for clarity) so the example matches the
page’s volume.level model.
In
`@apps/www/registry/default/blocks/audio-player/components/playback-mode-controls.tsx`:
- Around line 34-35: The code currently calls setRepeatMode via usePlaylist()
which subscribes the whole slice; switch to a selector-based action access by
replacing the usePlaylist() call with usePlaylistStore(s => s.setRepeatMode) and
keep the existing repeatMode selector (usePlaylistStore(state =>
state.repeatMode)) so updates remain scoped and avoid extra re-renders; update
any references to setRepeatMode to use that selector-returned function.
In
`@apps/www/registry/default/blocks/video-player/components/playlist-navigation-controls.tsx`:
- Around line 12-13: Replace the selector-anti-pattern in PlaylistNextControl by
removing "const { hasNext, next } = usePlaylist()" and instead read "next"
directly from the store with usePlaylistStore(s => s.next) and compute "hasNext"
with a derived selector that uses s.repeatMode, s.queue.length, and
s.getNextIndex() (e.g. usePlaylistStore(s => { const nextIndex =
s.getNextIndex(); return s.repeatMode !== 'off' || s.queue.length > 0 &&
nextIndex != null; }) ), leaving useHasPlaylist() as is; this ensures
selector-first access and prevents reading the whole playlist object via
usePlaylist().
In `@apps/www/registry/default/hooks/use-asset.ts`:
- Around line 609-617: getSourceTypeForItems() collapses single-item arrays to
Asset, so in places that are intended to accept playlist-shaped sources (e.g.,
the loadPlaylist handler and the other spots that call getSourceTypeForItems
before createAssetSession/normalizeAssets) detect if the incoming source is an
Array and, if so, set resolvedSourceType = AssetSourceType.Playlist (instead of
the value from getSourceTypeForItems); update the code paths that call
createAssetSession and normalizeAssets (the loadPlaylist implementation and the
other similar handlers that use resolvedSourceType) to use this forced Playlist
value for array inputs so one-item arrays preserve playlist mode downstream.
- Around line 923-965: The playback-error handler uses a closed-over currentItem
and unconditionally acts after awaiting recover.playbackError, so it can
reload/skip a stale session; capture a short-lived guard before awaiting (e.g.,
const sessionId = api.getState().asset.activeSession?.id and/or const
playlistId/index), then after each await re-read api.getState() and verify the
sessionId and currentItem still match (or abort if changed). Replace uses of the
closed-over currentItem with fresh values from api.getState() after the await,
and only call asset.loadAsset or playlist.next if the guard passes; also set
asset.previousError via api.setState only after confirming the session is
unchanged. Reference symbols: events.on("playbackerror") handler,
options.recover.playbackError, currentItem, api.getState(),
asset.activeSession?.id, AssetRecoveryAction.Reload/Skip, asset.loadAsset,
playlist.next, and getHasNext.
In `@apps/www/registry/default/hooks/use-playback-source.ts`:
- Around line 57-59: The effect currently only dedupes by resolvedSourceKey so
changes to other request options (like initialIndex) don't trigger reloads;
update the logic in use-playback-source so loadSource is keyed and re-run when
the full request options change: include initialIndex (and any other request
params used to build resolvedSourceKey) in the dedupe key and in the useEffect
dependency array, and reset loadedSourceKeyRef.current when those request
options change; adjust the comparison that prevents reloads (the place using
resolvedSourceKey and loadedSourceKeyRef) to compare against the combined key so
loadSource(...) will run for different initialIndex values. Ensure you reference
and update the symbols resolvedSourceKey, loadSource, initialIndex, and
loadedSourceKeyRef in that change.
---
Outside diff comments:
In `@apps/www/components/stream-panel/use-stream-panel-sync.ts`:
- Around line 76-133: The loader.load implementation calls await
context.loadDefault(...) then unconditionally calls
addBlenderCaptions(context.player, stream), which can race if the load was
superseded; after the await context.loadDefault(...) add an abort/generation
guard (e.g., if (context.signal.aborted) return; or
context.signal.throwIfAborted()) before invoking addBlenderCaptions so captions
are only added for the current load operation. Update the loader.load function
(inside the assetOptions useMemo) to perform this check immediately after the
loadDefault await and before the try/catch that calls addBlenderCaptions.
In `@apps/www/content/docs/hooks/use-player.mdx`:
- Around line 45-57: The example still passes removed onLoad/onError callback
props to usePlayer; remove the onLoad and onError fields from the usePlayer call
in the example (leave the const { load, preload, player } = usePlayer(...)
pattern) and update the example body to demonstrate the event-driven contract by
invoking player.load or load directly where appropriate (i.e., call
player.load(asset.src) from an event handler or effect rather than providing
onLoad/onError callbacks).
🪄 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: 1ae0f141-1777-4c38-ab09-97cf58130597
⛔ Files ignored due to path filters (2)
apps/www/package.jsonis excluded by none and included by nonebun.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (32)
apps/www/app/layout.config.tsxapps/www/components/mdx-components.tsxapps/www/components/mdx/mermaid.tsxapps/www/components/players/audio-player/demo-player.tsxapps/www/components/stream-panel/use-stream-panel-sync.tsapps/www/content/docs/blocks/audio-player.mdxapps/www/content/docs/blocks/meta.jsonapps/www/content/docs/blocks/video-player.mdxapps/www/content/docs/components/media-provider.mdxapps/www/content/docs/concepts.mdxapps/www/content/docs/events.mdxapps/www/content/docs/hooks/use-asset.mdxapps/www/content/docs/hooks/use-playback-source.mdxapps/www/content/docs/hooks/use-playback.mdxapps/www/content/docs/hooks/use-player.mdxapps/www/content/docs/quick-start.mdxapps/www/content/docs/usage.mdxapps/www/registry/default/blocks/audio-player/components/audio-source.tsxapps/www/registry/default/blocks/audio-player/components/controls.tsxapps/www/registry/default/blocks/audio-player/components/media-player.tsxapps/www/registry/default/blocks/audio-player/components/playback-controls.tsxapps/www/registry/default/blocks/audio-player/components/playback-mode-controls.tsxapps/www/registry/default/blocks/audio-player/components/playlist.tsxapps/www/registry/default/blocks/audio-player/components/track-info.tsxapps/www/registry/default/blocks/audio-player/hooks/use-playlist-asset.tsapps/www/registry/default/blocks/video-player/components/bottom-controls.tsxapps/www/registry/default/blocks/video-player/components/media-player.tsxapps/www/registry/default/blocks/video-player/components/playlist-navigation-controls.tsxapps/www/registry/default/blocks/video-player/page.tsxapps/www/registry/default/hooks/use-asset.tsapps/www/registry/default/hooks/use-playback-source.tsapps/www/registry/default/hooks/use-player.ts
💤 Files with no reviewable changes (1)
- apps/www/content/docs/events.mdx
Comment on lines
+53
to
+57
| icon: <IconBlocks />, | ||
| text: "Blocks", | ||
| type: "main", | ||
| url: "/docs/events", | ||
| url: "/blocks/video-player", | ||
| }, |
There was a problem hiding this comment.
Block documentation links are using the wrong base path.
Both sites use /blocks/video-player instead of the docs namespace, so navigation can break for readers.
apps/www/app/layout.config.tsx#L53-L57: change the Blocks nav URL to/docs/blocks/video-player.apps/www/content/docs/quick-start.mdx#L26-L26: update the inline link target to/docs/blocks/video-player.
📍 Affects 2 files
apps/www/app/layout.config.tsx#L53-L57(this comment)apps/www/content/docs/quick-start.mdx#L26-L26
🤖 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/app/layout.config.tsx` around lines 53 - 57, Update the incorrect
block links that use "/blocks/video-player" to the docs namespace
"/docs/blocks/video-player": in apps/www/app/layout.config.tsx (lines 53-57)
change the navigation item's url value for the "Blocks" entry from
"/blocks/video-player" to "/docs/blocks/video-player"; in
apps/www/content/docs/quick-start.mdx (line 26) update the inline link target
from "/blocks/video-player" to "/docs/blocks/video-player".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/www/registry/default/hooks/use-playback-source.ts (1)
83-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
loadingin the reload invalidation path.This still skips
loadSource(...)when the session config changes but the source key does not.requestKeyonly tracksresolvedSourceKey,sourceType, andinitialIndex, so a newloadingobject re-runs the effect but immediately returns on the old cached key. That keeps the previousgetAssetId/ loader / recovery config pinned to the new request.Minimal fix
useEffect(() => { loadedSourceKeyRef.current = null - }, [requestKey]) + }, [requestKey, loading])Also applies to: 96-104
🤖 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-source.ts` around lines 83 - 90, The requestKey useMemo only tracks resolvedSourceKey, sourceType, and initialIndex, but doesn't account for changes to the loading object containing session config. When loading changes but these tracked values don't, the useEffect that resets loadedSourceKeyRef.current doesn't properly trigger, causing loadSource to be skipped and keeping stale cached configuration. Add loading to both the requestKey useMemo calculation and the useEffect dependency array (at 83-90 and also at 96-104 per the consolidated comment) so that session config changes properly invalidate the cached key and re-run the load logic.
🤖 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.
Duplicate comments:
In `@apps/www/registry/default/hooks/use-playback-source.ts`:
- Around line 83-90: The requestKey useMemo only tracks resolvedSourceKey,
sourceType, and initialIndex, but doesn't account for changes to the loading
object containing session config. When loading changes but these tracked values
don't, the useEffect that resets loadedSourceKeyRef.current doesn't properly
trigger, causing loadSource to be skipped and keeping stale cached
configuration. Add loading to both the requestKey useMemo calculation and the
useEffect dependency array (at 83-90 and also at 96-104 per the consolidated
comment) so that session config changes properly invalidate the cached key and
re-run the load logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36b3071d-e2f0-4636-bd0c-be3baa4758d9
📒 Files selected for processing (10)
apps/www/components/stream-panel/use-stream-panel-sync.tsapps/www/content/docs/blocks/audio-player.mdxapps/www/content/docs/concepts.mdxapps/www/content/docs/usage.mdxapps/www/registry/collection/registry-blocks.tsapps/www/registry/default/blocks/audio-player/components/playback-mode-controls.tsxapps/www/registry/default/blocks/video-player/components/playlist-navigation-controls.tsxapps/www/registry/default/blocks/video-player/lib/media-kit.tsapps/www/registry/default/hooks/use-asset.tsapps/www/registry/default/hooks/use-playback-source.ts
✅ Files skipped from review due to trivial changes (3)
- apps/www/registry/default/blocks/video-player/lib/media-kit.ts
- apps/www/content/docs/concepts.mdx
- apps/www/content/docs/usage.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/www/content/docs/blocks/audio-player.mdx
- apps/www/registry/default/blocks/audio-player/components/playback-mode-controls.tsx
- apps/www/components/stream-panel/use-stream-panel-sync.ts
- apps/www/registry/default/hooks/use-asset.ts
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
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
Release Notes
New Features
Documentation
source/loadingcontract and revised usage/minimal examples.loadSource-centered loading model) and adjusted event guidance.Changes
source, with refined playlist-mode UI and updated event payloads.