[feat] add a new skeleton loading screen for playlist to replace the s…#11
[feat] add a new skeleton loading screen for playlist to replace the s…#11CupOfMakiato wants to merge 7 commits into
Conversation
…pinner loading overlay
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a skeleton-screen overlay and loader API, swaps app imports to use it, computes playlist-aware overlay bounds, and integrates artwork/duration caching, artwork hydration utilities, metadata IPC/options, session playback-position persistence, cursor utilities, and player properties UI across playlist/player/recent/library/queue flows. ChangesSkeleton Loading Screen Component
Artwork, Metadata, IPC & UI Integrations
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
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.
Actionable comments posted: 1
🤖 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 `@ui/components/skeleton-screen/skeleton.css`:
- Around line 18-27: The `#loadingOverlay` rule has duplicate display declarations
(display: flex and display: none); remove the unintended one by deleting the
first display: flex declaration so only display: none remains (or if overlay
should be visible by default, delete display: none instead) — update the CSS in
skeleton.css at the `#loadingOverlay` block to keep a single display property
(reference display: flex and display: none).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fec55eb0-1f93-44b0-a384-4d9b95728105
📒 Files selected for processing (5)
ui/components/skeleton-screen/skeleton.cssui/components/skeleton-screen/skeleton.jsui/index.htmlui/pages/playlist/playlist.jsui/script.js
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ui/components/bottom-player/player.js (1)
130-142: ⚡ Quick winUse valid definition-list structure for metadata rows.
createMetadataRow()wraps<dt>/<dd>in a<div>, then appends that wrapper into a<dl>. This is invalid list semantics and can reduce assistive-tech quality. Prefer either direct<dt>/<dd>children of<dl>, or switch the container/children to non-dlelements consistently.Also applies to: 162-167
🤖 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 `@ui/components/bottom-player/player.js` around lines 130 - 142, createMetadataRow currently creates a wrapper <div> and nests <dt>/<dd> inside it, which produces invalid definition-list structure when those rows are appended into a <dl>; update createMetadataRow to produce valid children for the DL by either returning/appending the <dt> and <dd> directly (e.g., have createMetadataRow return an array or append both elements to the caller's <dl>) or stop using <dt>/<dd> and use non-<dl> markup consistently (e.g., create a <div> with <span class="label"> and <span class="value">). Apply the same fix to the other similar helper(s) referenced in the diff so the DOM always uses valid <dl> children or consistently non-<dl> elements.ui/pages/playlist/playlist.js (1)
27-27: ⚡ Quick winConsider making the skeleton delay configurable or removing for production.
PLAYLIST_SKELETON_TEST_DELAY_MS = 250adds a 250ms artificial delay to every playlist load. If this is only for testing/demo purposes, consider:
- Setting it to
0for production builds- Making it configurable via environment/feature flag
- Adding a comment clarifying its purpose
Currently, users experience an unnecessary 250ms wait on every playlist navigation.
🤖 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 `@ui/pages/playlist/playlist.js` at line 27, PLAYLIST_SKELETON_TEST_DELAY_MS currently injects a 250ms artificial delay into playlist loading; change it so production builds don't impose this delay by reading a configurable value (e.g., from process.env or a feature flag) and defaulting to 0 in prod, or set it to 0 for production and keep a non-zero default only for tests/dev, and add a short comment above PLAYLIST_SKELETON_TEST_DELAY_MS explaining it's a test/demo hook so future editors know why it exists.
🤖 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 `@ui/components/bottom-player/player.js`:
- Around line 411-417: The UI currently wires only `#trackPropertiesBtn`
(showCurrentTrackProperties) leaving `#trackEqualizerBtn` and `#trackTestBtn` as
dead controls; either add event listeners for trackEqualizerBtn and trackTestBtn
that call the appropriate handlers (e.g., showTrackEqualizer() and
runTrackTest() or other existing functions) with proper .catch logging mirroring
the trackPropertiesBtn pattern, or remove those button elements from the
DOM/template if no feature exists; update player.js to
querySelector('`#trackEqualizerBtn`') and querySelector('`#trackTestBtn`') and
attach click handlers or remove their markup to keep the UI and code consistent.
In `@ui/pages/playlist/playlist.js`:
- Around line 1051-1054: Currently applyNowPlayingHighlight() is invoked on
every playerState subscription tick which causes unnecessary DOM work; modify
the playerState.subscribe callback (where playbackSnapshot is set) to only call
applyNowPlayingHighlight() when the data that affects highlighting changes
(e.g., currentTrackIndex or playlist) by comparing relevant fields between the
new snapshot and the previous one (store previousSnapshot or
lastHighlightedIndex), or debounce the call if rapid progress updates occur;
specifically update the subscription around playerState.subscribe /
playbackSnapshot / applyNowPlayingHighlight to perform a shallow/deep compare of
snapshot.currentTrackIndex and snapshot.playlist (or track lastHighlightedIndex)
before invoking applyNowPlayingHighlight, or wrap the call in a short debounce
to reduce frequency.
---
Nitpick comments:
In `@ui/components/bottom-player/player.js`:
- Around line 130-142: createMetadataRow currently creates a wrapper <div> and
nests <dt>/<dd> inside it, which produces invalid definition-list structure when
those rows are appended into a <dl>; update createMetadataRow to produce valid
children for the DL by either returning/appending the <dt> and <dd> directly
(e.g., have createMetadataRow return an array or append both elements to the
caller's <dl>) or stop using <dt>/<dd> and use non-<dl> markup consistently
(e.g., create a <div> with <span class="label"> and <span class="value">). Apply
the same fix to the other similar helper(s) referenced in the diff so the DOM
always uses valid <dl> children or consistently non-<dl> elements.
In `@ui/pages/playlist/playlist.js`:
- Line 27: PLAYLIST_SKELETON_TEST_DELAY_MS currently injects a 250ms artificial
delay into playlist loading; change it so production builds don't impose this
delay by reading a configurable value (e.g., from process.env or a feature flag)
and defaulting to 0 in prod, or set it to 0 for production and keep a non-zero
default only for tests/dev, and add a short comment above
PLAYLIST_SKELETON_TEST_DELAY_MS explaining it's a test/demo hook so future
editors know why it exists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebe41d21-c88e-4825-9394-43b3d647c389
📒 Files selected for processing (7)
ui/components/bottom-player/player.cssui/components/bottom-player/player.htmlui/components/bottom-player/player.jsui/components/skeleton-screen/skeleton.cssui/components/skeleton-screen/skeleton.jsui/pages/playlist/playlist.cssui/pages/playlist/playlist.js
✅ Files skipped from review due to trivial changes (1)
- ui/pages/playlist/playlist.css
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/components/skeleton-screen/skeleton.js
…mance optimize, resolve lagginess by loading fix size image once per session
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
ui/pages/library/library.js (1)
188-191: ⚡ Quick winAvoid repeated linear lookup when hydrating card artwork.
Using
playlists.find(...)per card makes this render path O(n²). Build aMaponce and do O(1) lookups.Refactor sketch
+const playlistsById = new Map(playlists.map((playlist) => [playlist.id, playlist])) container.querySelectorAll('.libraryPlaylistCard').forEach((card) => { const playlistId = card.getAttribute('data-playlist-id') - const playlist = playlists.find((item) => item.id === playlistId) + const playlist = playlistsById.get(playlistId) const imageElement = card.querySelector('img') if (!playlist || !imageElement) { return }🤖 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 `@ui/pages/library/library.js` around lines 188 - 191, The code currently calls playlists.find(...) for every element returned by container.querySelectorAll('.libraryPlaylistCard'), causing O(n²) behavior; fix this by creating a Map from playlist id to playlist once (e.g., build a map from playlists using each playlist.id) and then inside the loop use map.get(playlistId) to fetch the playlist in O(1) before accessing imageElement; update any references to playlist (from the previous find) to use the mapped value and handle missing entries the same way as before.
🤖 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 `@main.js`:
- Around line 640-660: The handler for 'playlist:savePlaybackPosition' is
persisting the raw UI index instead of the remapped index used when saving the
playlist; change the logic that computes approvedCurrentTrackIndex to resolve
the renderer's current track into the same index in
settingsStore.get('recentPlaylist') used by playlist:save (e.g., take the
renderer-provided track identifier/path from the IPC payload, find its
occurrence in the retrieved playlist array and use that index; if no identifier
is provided, fall back to the existing bounds check), then persist that resolved
index via settingsStore.set('recentPlaylistIndex') and persist normalized
playback position only when the resolved index >= 0. Ensure this mapping code
lives inside the 'playlist:savePlaybackPosition' handler so it mirrors the
remapping behavior of savePlaylist/save.
In `@ui/components/recent-music/recent-metadata.js`:
- Around line 32-58: The metadata cache currently keys only by filePath causing
mixed results when callers request different options (e.g.,
{includeImage:true}); update getCachedMetadataPromise and
setCachedMetadataPromise to use a composite cache key that includes serialized
metadata options (for example stringify or join includeImage flag) so the stored
promise is unique per filePath+options, and update all callers (the places that
call these functions when requesting metadata with/without includeImage) to pass
the same options used to build the key so a cached promise always matches the
requested option set.
In `@ui/components/recent-music/recent-music.js`:
- Around line 405-411: The async .then block that sets recentTracks[index].image
can write the artwork to the wrong item if recentTracks has been reordered or
re-rendered; capture a stable identifier before the async call (e.g., const
trackId = recentTracks[index]?.id) and in the .then verify the track is still
the same before mutating (either by checking recentTracks[index]?.id === trackId
or locating the current index via recentTracks.findIndex(t => t.id ===
trackId)), and perform the update immutably via your state updater instead of
directly writing recentTracks[index]; this ensures artwork is assigned only to
the matching track.
In `@ui/pages/playlist/playlist.js`:
- Around line 681-697: The hydrate function rememberActivePlaylistTrackArtwork
can write artwork into whatever playlist is currently active when the async
artwork resolves; to fix it, capture a stable identifier or reference for the
playlist at the top (e.g., const originalPlaylistId = activePlaylist.id or const
originalPlaylistRef = activePlaylist) and before mutating ensure the playlist
still matches (re-fetch with getActivePlaylist() and compare id or reference)
and that the target track at trackIndex still corresponds (validate
Array.isArray(tracks) and the same filePath via normalizeTrackRecord) before
assigning activePlaylist.tracks[trackIndex] = {...}; update the checks in
rememberActivePlaylistTrackArtwork (and the other similar block at 699-723) to
only write when the playlist identity and track match.
In `@ui/services/audio-service.js`:
- Around line 188-193: The current early return when includeImage is true
returns artwork with buildFallbackTrackData(filePath) even if metadataCache no
longer has the real metadata, causing later lookups to show fallback text;
change the logic in the branch that references
includeImage/artworkCache/metadataCache so you only short-circuit when both
artworkCache.has(filePath) AND metadataCache.has(filePath); otherwise fetch
metadata via metadataCache.get(filePath) || buildFallbackTrackData(filePath)
first and then attach artworkCache.get(filePath) if present (using the same
filePath and image key) so text metadata is preserved when available.
In `@ui/services/session-service.js`:
- Around line 15-22: compactStorageValue is performing an asynchronous delayed
write which can stomp newer changes; change it to either perform compaction
synchronously during load or implement a stale-write guard: capture the raw
string that was normalized (pass an originalRaw parameter into
compactStorageValue), and before calling localStorage.setItem(key, ...), read
window.localStorage.getItem(key) and only proceed if it still equals the
captured originalRaw; apply the same guard to the other delayed compaction sites
(the other compact storage call sites referenced).
In `@ui/state/player-state.js`:
- Around line 21-29: createStateSnapshot currently returns the live
state.playlist reference which allows consumers to mutate store state directly;
update the createStateSnapshot function to return a cloned playlist (e.g., use
[...state.playlist] or state.playlist.slice()) instead of state.playlist so
snapshots contain an independent array while keeping existing clones for
progress and currentTrack.
- Around line 127-138: The reset() function currently omits restoring
state.volume so the store doesn't match its declared initial state; update
reset() to set state.volume back to the initial value (e.g. state.volume =
initialState.volume or state.volume = 1 if initialState isn't accessible)
alongside the other fields before calling notify(), referencing the reset()
function and state.volume to locate the change.
- Around line 84-91: The equality check in setCurrentTrack currently compares
filePath, title, artist, and image but omits album, causing album-only updates
to be ignored; update the conditional inside setCurrentTrack to also compare
state.currentTrack.album === nextTrack.album (and ensure nextTrack is
constructed as currently done) so album differences will let the branch treat
the track as changed and notify subscribers.
In `@ui/utils/artwork.js`:
- Around line 23-27: The function currently returns metadata?.image raw from
audioService.resolveTrackMetadata which can bypass image sanitization and leak
invalid values into imageElement.src via hydrators; update the code to pass
metadata.image through the application's image sanitizer/normalizer (import the
existing sanitizer used elsewhere) before returning, ensure you return a
sanitized string or '' for invalid/null values, and keep references to
audioService.resolveTrackMetadata, metadata?.image, and imageElement.src so
reviewers can locate the change.
---
Nitpick comments:
In `@ui/pages/library/library.js`:
- Around line 188-191: The code currently calls playlists.find(...) for every
element returned by container.querySelectorAll('.libraryPlaylistCard'), causing
O(n²) behavior; fix this by creating a Map from playlist id to playlist once
(e.g., build a map from playlists using each playlist.id) and then inside the
loop use map.get(playlistId) to fetch the playlist in O(1) before accessing
imageElement; update any references to playlist (from the previous find) to use
the mapped value and handle missing entries the same way as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 664aa4e1-0506-4716-93b1-89f506792406
⛔ Files ignored due to path filters (5)
ui/assets/cursor-auto.pngis excluded by!**/*.pngui/assets/cursor-drag-clicked.pngis excluded by!**/*.pngui/assets/cursor-drag.pngis excluded by!**/*.pngui/assets/cursor-pointer-clicked.pngis excluded by!**/*.pngui/assets/cursor-text.pngis excluded by!**/*.png
📒 Files selected for processing (21)
main.jspreload.jsui/components/bottom-player/player.jsui/components/loading/loading.cssui/components/recent-music/recent-metadata.jsui/components/recent-music/recent-music.cssui/components/recent-music/recent-music.jsui/pages/about/about-layout.htmlui/pages/library/library.jsui/pages/playlist/playlist-sortable.jsui/pages/playlist/playlist.cssui/pages/playlist/playlist.jsui/pages/queue/queue.jsui/services/audio-service.jsui/services/session-service.jsui/state/player-state.jsui/ui.cssui/utils/artwork.jsui/utils/cursor-interaction.jsui/utils/playlist-media.jsui/utils/track-record.js
✅ Files skipped from review due to trivial changes (2)
- ui/components/recent-music/recent-music.css
- ui/pages/about/about-layout.html
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/pages/playlist/playlist-header.js (1)
222-230: 💤 Low value
document.execCommandis deprecated but acceptable here.
execCommand('insertText', ...)is deprecated but remains the most reliable cross-browser method for plain-text paste handling incontentEditableelements. Modern alternatives likedocument.execCommandreplacements or Clipboard API insertions have inconsistent contentEditable support. This is acceptable for now, but be aware it may need updating in the future.🤖 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 `@ui/pages/playlist/playlist-header.js` around lines 222 - 230, The paste handler onTitlePaste currently uses the deprecated document.execCommand('insertText', ...) but that's intentional for reliable cross-browser contentEditable behavior; add an inline comment next to the onTitlePaste implementation and the document.execCommand call that states why execCommand is being used (deprecated but most reliable for contentEditable plain-text paste), note the Clipboard API alternative is inconsistent for contentEditable, and mark a TODO to revisit when browser support improves so reviewers and future maintainers understand the rationale.
🤖 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.
Nitpick comments:
In `@ui/pages/playlist/playlist-header.js`:
- Around line 222-230: The paste handler onTitlePaste currently uses the
deprecated document.execCommand('insertText', ...) but that's intentional for
reliable cross-browser contentEditable behavior; add an inline comment next to
the onTitlePaste implementation and the document.execCommand call that states
why execCommand is being used (deprecated but most reliable for contentEditable
plain-text paste), note the Clipboard API alternative is inconsistent for
contentEditable, and mark a TODO to revisit when browser support improves so
reviewers and future maintainers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feb87a7a-b921-4aa7-9792-153d8ea9ae67
📒 Files selected for processing (14)
main.jspreload.jsui/components/recent-music/recent-metadata.jsui/components/skeleton-screen/skeleton.cssui/pages/playlist/playlist-actions.jsui/pages/playlist/playlist-artwork.jsui/pages/playlist/playlist-duration.jsui/pages/playlist/playlist-header.jsui/pages/playlist/playlist-scroll.jsui/pages/playlist/playlist.cssui/pages/playlist/playlist.jsui/services/audio-service.jsui/services/session-service.jsui/utils/route.js
✅ Files skipped from review due to trivial changes (1)
- ui/utils/route.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/components/recent-music/recent-metadata.js
- ui/services/audio-service.js
- ui/components/skeleton-screen/skeleton.css
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/pages/playlist/playlist-header.js (1)
222-230: 💤 Low value
document.execCommandis deprecated but acceptable here.The
execCommand('insertText', ...)API is deprecated, but for contentEditable paste handling it remains the most reliable cross-browser approach. Modern alternatives using the Clipboard API and programmatic Selection manipulation are significantly more complex.No action required unless browser support issues arise in the future.
🤖 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 `@ui/pages/playlist/playlist-header.js` around lines 222 - 230, onTitlePaste uses the deprecated document.execCommand('insertText', ...) but that's acceptable; leave the implementation as-is, but if you want a non-blocking fallback when document.execCommand is missing, detect its existence in onTitlePaste and otherwise read clipboard text via navigator.clipboard.readText() and insert it at the caret using document.getSelection()/Range manipulation (use title.isContentEditable to guard and keep event.preventDefault()). Reference: onTitlePaste, document.execCommand, title.isContentEditable.
🤖 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 `@ui/pages/playlist/playlist-artwork.js`:
- Around line 39-88: hydrateTrackArtworkRows currently captures activePlaylist
early but uses getActivePlaylist() again when the artwork promise resolves,
allowing race conditions that write artwork into a different active playlist;
fix by capturing the playlist identifier (e.g., playlistId = activePlaylist?.id)
and the filePath at the start of each row iteration, then before calling
rememberActivePlaylistTrackArtwork (and before schedulePlaylistArtworkPersist)
verify that getActivePlaylist()?.id === playlistId and the resolved artwork's
filePath still matches the captured filePath; only then call
rememberActivePlaylistTrackArtwork(trackIndex, image) and persist — otherwise
discard the result and clear resolve promises.
---
Nitpick comments:
In `@ui/pages/playlist/playlist-header.js`:
- Around line 222-230: onTitlePaste uses the deprecated
document.execCommand('insertText', ...) but that's acceptable; leave the
implementation as-is, but if you want a non-blocking fallback when
document.execCommand is missing, detect its existence in onTitlePaste and
otherwise read clipboard text via navigator.clipboard.readText() and insert it
at the caret using document.getSelection()/Range manipulation (use
title.isContentEditable to guard and keep event.preventDefault()). Reference:
onTitlePaste, document.execCommand, title.isContentEditable.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f157eac2-4569-4039-9752-9c2dfa66f2d2
📒 Files selected for processing (14)
main.jspreload.jsui/components/recent-music/recent-metadata.jsui/components/skeleton-screen/skeleton.cssui/pages/playlist/playlist-actions.jsui/pages/playlist/playlist-artwork.jsui/pages/playlist/playlist-duration.jsui/pages/playlist/playlist-header.jsui/pages/playlist/playlist-scroll.jsui/pages/playlist/playlist.cssui/pages/playlist/playlist.jsui/services/audio-service.jsui/services/session-service.jsui/utils/route.js
✅ Files skipped from review due to trivial changes (1)
- ui/utils/route.js
🚧 Files skipped from review as they are similar to previous changes (7)
- preload.js
- ui/pages/playlist/playlist.css
- ui/components/recent-music/recent-metadata.js
- ui/services/audio-service.js
- ui/components/skeleton-screen/skeleton.css
- main.js
- ui/services/session-service.js
Summary by CodeRabbit
New Features
Bug Fixes / Improvements