feat(#461): add smart playlists with rule engine, lastfm api, and criteria editor - #498
feat(#461): add smart playlists with rule engine, lastfm api, and criteria editor#498Owie6789 wants to merge 38 commits into
Conversation
|
root cause n/a, new feature. smart playlists work by storing rules as json in the criteria column. the rule engine (playlist-rules.ts) compiles those into drizzle where clauses on the songs table. refreshSmartPlaylist clears and re-inserts playlists_songs inside a transaction. saveSmartPlaylistCriteria writes the json + calls refresh so the ui updates immediately. the 3 lastfm endpoints follow the existing get-request pattern using ipcRenderer.invoke + ipcMain.handle. -edit |
|
@CodeRabbit — comprehensive review request. Please do all of the following and report findings as inline review comments on the changed files (not in this thread), grouped by severity. 1. Careful code review
2. UI audit (only if the PR touches UI)
3. Production-readiness verification
4. Code smell sniff
5. Half-baked implementation detection
6. Required output from you
Please be thorough. I will be applying fixes from your review in a follow-up pass. |
|
The self-reported post-hoc fixes (Boolean coercion, Perform a comprehensive review of every changed file in this PR as described by the requester. Cover all six areas:
✅ Action performedFull review finished. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds smart-playlist storage, criteria validation, SQL evaluation, Last.fm retrieval and synchronization, IPC and preload APIs, renderer controls, localized messages, playlist mutation guards, and a scrobble queue migration. ChangesSmart Playlists and Last.fm Integration
Scrobble Queue Migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant PreloadAPI
participant IPC
participant LastFm
participant SmartPlaylistSync
participant Database
Renderer->>PreloadAPI: request Last.fm tracks
PreloadAPI->>IPC: invoke track retrieval
IPC->>LastFm: fetch normalized track data
LastFm-->>IPC: return tracks
IPC-->>Renderer: return tracks
Renderer->>PreloadAPI: synchronize matched songs
PreloadAPI->>IPC: invoke smart playlist sync
IPC->>SmartPlaylistSync: validate source and song IDs
SmartPlaylistSync->>Database: replace memberships and save source
Database-->>SmartPlaylistSync: return synchronized count
SmartPlaylistSync-->>Renderer: return synchronization result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
src/main/db/queries/playlist-rules.ts (1)
6-116: 🏗️ Heavy liftSplit
buildConditioninto smaller field-specific helpers.
buildConditioncurrently combines parsing, operator mapping, and all field branches in one large function. Breaking it into focused helpers will make this rule engine easier to validate and extend safely.As per coding guidelines "Keep functions small, aiming for 30-50 lines maximum per function; extract complex logic into separate helper functions for single responsibility".
🤖 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 `@src/main/db/queries/playlist-rules.ts` around lines 6 - 116, buildCondition is too large and mixes parsing, operator mapping, and field branches; extract focused helpers and delegate each case to them. Create helpers like numericCondition(rule) (replacing numeric), stringCondition(rule) (replacing str), booleanCondition(rule) (replacing bool), existsRelationCondition(rule, relationColumn, joinTable) for genre/artist/album branches, countCondition(rule, eventsTable) for playCount/skipCount, and lastPlayedCondition(rule) for the lastPlayed logic, then have buildCondition just switch on field and call the appropriate helper (e.g., buildCondition -> numericCondition, stringCondition, booleanCondition, existsRelationCondition for 'genre'/'artist'/'album', countCondition for 'playCount'/'skipCount', lastPlayedCondition for 'lastPlayed'); ensure each helper parses value once and returns SQL | undefined.
🤖 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 `@src/main/db/queries/playlist-rules.ts`:
- Around line 75-96: The playCount, skipCount (and the lastPlayed branch around
lines 103-109) switch statements drop SmartPlaylistRuleOperator 'neq' silently;
update each switch in the song rule builder (the playCount, skipCount cases and
the lastPlayed case) to add a case 'neq' that returns the appropriate SQL
expression using a not-equal comparison (e.g., count(...) <> ${n} for
playCount/skipCount and a <> (or IS DISTINCT FROM if nullable timestamps are
involved) comparison for lastPlayed) instead of falling through to undefined so
'neq' rules are honored.
- Around line 9-20: The numeric helper currently coerces value to Number without
guarding for NaN/Infinity, which can emit invalid SQL; modify the numeric
function (and the other numeric coercion helpers in this file) to compute const
n = Number(value) and immediately return undefined if !Number.isFinite(n) before
the switch on operator so malformed inputs are ignored and no SQL condition is
emitted for non-finite numbers.
In `@src/main/db/schema.ts`:
- Around line 303-304: The playlists table in schema.ts is missing the index
declared in src/resources/drizzle/0000_add_smart_playlist_columns.sql
(idx_playlists_is_smart), causing schema drift; open the playlists table
definition in src/main/db/schema.ts and add a Drizzle index named
idx_playlists_is_smart targeting the isSmart column (the same column declared as
isSmart: boolean('is_smart').notNull().default(false)), then run npm run
db:generate to regenerate migrations so the Drizzle schema and SQL index stay in
sync.
In `@src/main/ipc.ts`:
- Around line 340-358: The Last.fm IPC handlers ('app/lastfm/getUserTopTracks',
'app/lastfm/getUserRecentTracks', 'app/lastfm/getUserLovedTracks') accept
renderer-controlled inputs without validation; update each ipcMain.handle
callback to trim and validate the username (reject or return an error for
empty/invalid usernames), clamp/normalize limit to a safe range (e.g., min 1,
max 100) and coerce types, and for getUserTopTracks also validate the period
against the allowed set ('overall','7day','1month','3month','6month','12month')
before calling getUserTopTracks/getUserRecentTracks/getUserLovedTracks so
network helpers only receive cleaned, bounded inputs.
- Around line 481-493: Extract the orchestration inside the ipcMain.handle
handlers into a core business API (e.g., create functions like
saveAndRefreshSmartPlaylist(playlistId, criteria) and
refreshSmartPlaylistById(playlistId)) in src/main/core; move logic that calls
updatePlaylistCriteria, refreshSmartPlaylist and getPlaylistById (and the
JSON.parse of playlist.criteria / SmartPlaylistCriteria handling) into those
core functions, export them, then simplify the ipc handlers in ipc.ts to just
call these core functions (ipcMain.handle('app/saveSmartPlaylistCriteria' ->
call saveAndRefreshSmartPlaylist; ipcMain.handle('app/refreshSmartPlaylist' ->
call refreshSmartPlaylistById) and return their { songIds } results). Ensure
type usage (SmartPlaylistCriteria) is preserved and core functions are
unit-testable.
- Around line 487-492: Wrap the JSON.parse of playlist.criteria inside a
try/catch within the ipcMain.handle('app/refreshSmartPlaylist') handler: if
getPlaylistById returns a playlist but parsing fails, catch the error, log it
(use the existing logger used elsewhere in ipc.ts), and return the safe fallback
{ songIds: [] } instead of letting the IPC call reject; on successful parse
continue to call refreshSmartPlaylist(playlistId, criteria) and return its
result.
In `@src/main/other/lastFm/getUserLovedTracks.ts`:
- Around line 25-46: In getUserLovedTracks, change the URL construction to use
'https://ws.audioscrobbler.com/2.0/' instead of 'http://...', and wrap the
fetch(url) call with an AbortController-based timeout (create AbortController,
pass controller.signal to fetch, set a timer to controller.abort() after a
reasonable ms and clear it on success) to bound the request; additionally, when
res.ok is false, read and log the response status and body (or at least
status/text) and return undefined (or throw) so failures are visible—refer to
symbols url, LAST_FM_API_KEY, fetch(...), AbortController, res and
getUserLovedTracks when applying the fix.
In `@src/main/other/lastFm/getUserRecentTracks.ts`:
- Around line 26-48: Update the request in getUserRecentTracks to use HTTPS for
the Last.fm endpoint, add an AbortController-based timeout around the fetch call
(create controller, pass controller.signal to fetch, and clear/abort on
timeout), and change the non-OK response handling so it throws a descriptive
Error (including HTTP status and statusText or body) instead of returning
undefined; adjust the fetch/response logic around the existing url construction,
the fetch(url) call, and the res.ok branch so that errors always throw and the
caller/catch will log them.
In `@src/main/other/lastFm/getUserTopTracks.ts`:
- Around line 27-50: In getUserTopTracks update the URL to use HTTPS and make
the fetch call fail-fast by using an AbortController with a short timeout (e.g.,
a few seconds) and cancelling the request on timeout; if the response is not OK,
throw an Error containing the HTTP status and statusText instead of returning
undefined, and include any JSON error details when available (references:
getUserTopTracks, url, fetch call, LAST_FM_API_KEY, and the data.error
handling).
In `@src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx`:
- Around line 180-181: In SmartPlaylistCriteriaEditor, the input elements
currently use the hardcoded placeholder "value" (placeholder="value" at the JSX
for the inputs around the shown diff and similarly at the other occurrence);
replace those with the localized string from your app i18n helper (e.g. call the
existing translation function/hook such as t('smartPlaylist.criteria.value') or
useI18n().t(...) and pass that result to the placeholder prop) so both input
placeholders are localized; ensure you import/use the same translation hook
already used elsewhere in the component and pick a descriptive translation key
like "smartPlaylist.criteria.value".
- Around line 125-143: The save path calling
window.api.playlistsData.saveSmartPlaylistCriteria can reject and currently
leaves an unhandled rejection; wrap the await in a try/catch inside the async
callback (the useCallback around the save logic in SmartPlaylistCriteriaEditor)
so that on error you call addNewNotifications with the 'smartPlaylistSaveFailed'
payload and avoid assuming success, and only call changePromptMenuData(false)
after a confirmed successful save (result true). Ensure you reference
saveSmartPlaylistCriteria, result, addNewNotifications and changePromptMenuData
when making the changes.
- Around line 38-47: The JSON.parse result used in useMemo for initialCriteria
may produce an object that doesn't match SmartPlaylistCriteria (missing/invalid
rules) so add a validation guard: after parsing playlist.criteria in the
initialCriteria useMemo, verify the parsed value satisfies the
SmartPlaylistCriteria shape (e.g., has a rules array and required fields) — if
it fails validation, return defaultCriteria(); implement or call a small
type-guard helper (e.g., isValidSmartPlaylistCriteria(parsed)) and use that in
the initialCriteria block to ensure rendering won't break.
---
Nitpick comments:
In `@src/main/db/queries/playlist-rules.ts`:
- Around line 6-116: buildCondition is too large and mixes parsing, operator
mapping, and field branches; extract focused helpers and delegate each case to
them. Create helpers like numericCondition(rule) (replacing numeric),
stringCondition(rule) (replacing str), booleanCondition(rule) (replacing bool),
existsRelationCondition(rule, relationColumn, joinTable) for genre/artist/album
branches, countCondition(rule, eventsTable) for playCount/skipCount, and
lastPlayedCondition(rule) for the lastPlayed logic, then have buildCondition
just switch on field and call the appropriate helper (e.g., buildCondition ->
numericCondition, stringCondition, booleanCondition, existsRelationCondition for
'genre'/'artist'/'album', countCondition for 'playCount'/'skipCount',
lastPlayedCondition for 'lastPlayed'); ensure each helper parses value once and
returns SQL | undefined.
🪄 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 Plus
Run ID: 7ed705ed-2a12-4234-96f5-45b771071ceb
📒 Files selected for processing (19)
src/main/db/queries/playlist-rules.tssrc/main/db/queries/playlists.tssrc/main/db/schema.tssrc/main/ipc.tssrc/main/other/lastFm/getUserLovedTracks.tssrc/main/other/lastFm/getUserRecentTracks.tssrc/main/other/lastFm/getUserTopTracks.tssrc/main/utils/convert.tssrc/preload/index.tssrc/renderer/src/assets/locales/en/en.jsonsrc/renderer/src/components/PlaylistsPage/Playlist.tsxsrc/renderer/src/components/SearchPage/Result_Containers/PlaylistSearchResultsContainer.tsxsrc/renderer/src/components/SmartPlaylistCriteriaEditor.tsxsrc/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsxsrc/renderer/src/routes/main-player/playlists/$playlistId.tsxsrc/renderer/src/routes/main-player/playlists/favorites.tsxsrc/renderer/src/routes/main-player/playlists/history.tsxsrc/resources/drizzle/0000_add_smart_playlist_columns.sqlsrc/types/app.d.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use module-level initialization for singleton services (AudioPlayer, PlayerQueue) accessed by intervals, timers, or event handlers, NOT useRef-based singletons, to avoid stale closure issues
Always return cleanup functions in useEffect hooks that register IPC event listeners, and ensure corresponding removal functions are called to prevent memory leaks
Use descriptive, context-rich variable and function names; avoid generic names like 'data', 'user', 'info', 'temp'. Functions should start with verbs (calculateDuration, fetchSongData, validatePlaylist)
Use guard clauses with early returns to keep function logic flat and readable, avoiding deep nesting and improving error handling clarity
Use eslint-plugin-simple-import-sort for automatic import organization: external dependencies, internal path aliases (@renderer,@main,@common), then relative imports
All data fetching errors should be handled within queryFn, returning safe defaults (empty arrays, null, default objects) instead of throwing exceptions
Keep functions small, aiming for 30-50 lines maximum per function; extract complex logic into separate helper functions for single responsibility
Files:
src/main/db/schema.tssrc/main/db/queries/playlists.tssrc/renderer/src/routes/main-player/playlists/favorites.tsxsrc/main/utils/convert.tssrc/main/other/lastFm/getUserTopTracks.tssrc/renderer/src/components/SearchPage/Result_Containers/PlaylistSearchResultsContainer.tsxsrc/main/db/queries/playlist-rules.tssrc/main/other/lastFm/getUserLovedTracks.tssrc/renderer/src/components/PlaylistsPage/Playlist.tsxsrc/types/app.d.tssrc/renderer/src/routes/main-player/playlists/history.tsxsrc/main/other/lastFm/getUserRecentTracks.tssrc/main/ipc.tssrc/preload/index.tssrc/renderer/src/routes/main-player/playlists/$playlistId.tsxsrc/renderer/src/components/SmartPlaylistCriteriaEditor.tsxsrc/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx
src/main/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All business logic should be in src/main/core/ modules with single responsibility, following the pattern of taking data as parameters and calling database queries or external APIs
Files:
src/main/db/schema.tssrc/main/db/queries/playlists.tssrc/main/utils/convert.tssrc/main/other/lastFm/getUserTopTracks.tssrc/main/db/queries/playlist-rules.tssrc/main/other/lastFm/getUserLovedTracks.tssrc/main/other/lastFm/getUserRecentTracks.tssrc/main/ipc.ts
src/main/db/schema.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All database schema changes must be made here using Drizzle table definitions, then run npm run db:generate to create migrations automatically
Files:
src/main/db/schema.ts
src/main/db/queries/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All database queries must be defined in dedicated query modules following the pattern: one file per entity (songs.ts, artists.ts, playlists.ts, etc.) using Drizzle ORM methods
Files:
src/main/db/queries/playlists.tssrc/main/db/queries/playlist-rules.ts
src/renderer/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/renderer/src/**/*.{ts,tsx}: Use dispatch() function from store.ts for all state updates in the renderer, never modify store state directly
Extract all data fetching logic into centralized query modules using createQueryKeys factory pattern from@lukemorales/query-key-factoryin src/renderer/src/queries/, never inline fetch logic in components
Use useSuspenseQuery() for data fetching in components with TanStack Router loaders for pre-fetching, not custom fetch hooks or useQuery without suspense
Use TanStack Router's , useNavigate(), and useRouter() for navigation, not deprecated changeCurrentActivePage() or updatePageHistoryIndex() functions
Files:
src/renderer/src/routes/main-player/playlists/favorites.tsxsrc/renderer/src/components/SearchPage/Result_Containers/PlaylistSearchResultsContainer.tsxsrc/renderer/src/components/PlaylistsPage/Playlist.tsxsrc/renderer/src/routes/main-player/playlists/history.tsxsrc/renderer/src/routes/main-player/playlists/$playlistId.tsxsrc/renderer/src/components/SmartPlaylistCriteriaEditor.tsxsrc/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx
src/renderer/src/**/*.tsx
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All custom hooks that integrate features into App.tsx must be called in App.tsx with no return value (they manage state via dispatch and event listeners internally)
Files:
src/renderer/src/routes/main-player/playlists/favorites.tsxsrc/renderer/src/components/SearchPage/Result_Containers/PlaylistSearchResultsContainer.tsxsrc/renderer/src/components/PlaylistsPage/Playlist.tsxsrc/renderer/src/routes/main-player/playlists/history.tsxsrc/renderer/src/routes/main-player/playlists/$playlistId.tsxsrc/renderer/src/components/SmartPlaylistCriteriaEditor.tsxsrc/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx
src/main/ipc.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Register all IPC handlers using ipcMain.handle() for async operations with return values and ipcMain.on() for fire-and-forget events, mapping them to core business logic modules
Files:
src/main/ipc.ts
src/preload/index.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All IPC communication to main process must be exposed through window.api with categorized namespaces (playerControls, audioLibraryControls, settingsHelpers, etc.) for type safety
Files:
src/preload/index.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to src/main/db/queries/**/*.ts : All database queries must be defined in dedicated query modules following the pattern: one file per entity (songs.ts, artists.ts, playlists.ts, etc.) using Drizzle ORM methods
Applied to files:
src/main/db/queries/playlists.ts
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to **/*.{ts,tsx} : Use descriptive, context-rich variable and function names; avoid generic names like 'data', 'user', 'info', 'temp'. Functions should start with verbs (calculateDuration, fetchSongData, validatePlaylist)
Applied to files:
src/main/ipc.ts
🔇 Additional comments (12)
src/types/app.d.ts (1)
73-78: LGTM!Also applies to: 700-727, 738-739
src/main/utils/convert.ts (1)
105-107: LGTM!src/resources/drizzle/0000_add_smart_playlist_columns.sql (1)
1-4: LGTM!src/main/db/queries/playlists.ts (1)
259-268: LGTM!src/renderer/src/assets/locales/en/en.json (1)
76-76: LGTM!Also applies to: 269-302
src/renderer/src/components/PlaylistsPage/Playlist.tsx (1)
18-18: LGTM!Also applies to: 213-220, 425-430
src/renderer/src/components/SearchPage/Result_Containers/PlaylistSearchResultsContainer.tsx (1)
49-50: LGTM!src/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx (1)
123-125: LGTM!Also applies to: 134-135
src/renderer/src/routes/main-player/playlists/$playlistId.tsx (1)
24-26: LGTM!Also applies to: 150-177, 219-230, 279-279
src/renderer/src/routes/main-player/playlists/favorites.tsx (1)
37-39: LGTM!src/renderer/src/routes/main-player/playlists/history.tsx (1)
35-37: LGTM!src/preload/index.ts (1)
430-448: LGTM!Also applies to: 494-497, 648-648
| ipcMain.handle('app/saveSmartPlaylistCriteria', async (_, playlistId: number, criteria: SmartPlaylistCriteria) => { | ||
| await updatePlaylistCriteria(playlistId, criteria); | ||
| const songIds = await refreshSmartPlaylist(playlistId, criteria); | ||
| return { songIds }; | ||
| }); | ||
|
|
||
| ipcMain.handle('app/refreshSmartPlaylist', async (_, playlistId: number) => { | ||
| const playlist = await getPlaylistById(playlistId); | ||
| if (!playlist?.criteria) return { songIds: [] }; | ||
| const criteria: SmartPlaylistCriteria = JSON.parse(playlist.criteria); | ||
| const songIds = await refreshSmartPlaylist(playlistId, criteria); | ||
| return { songIds }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move smart-playlist orchestration out of ipc.ts into a core module.
initializeIPC now contains write/parse/refresh business flow. Keep IPC handlers as transport adapters and delegate this flow to src/main/core/* for single-responsibility and testability.
As per coding guidelines src/main/ipc.ts: “Register all IPC handlers … mapping them to core business logic modules”, and src/main/**/*.ts: “All business logic should be in src/main/core/ modules…”.
🤖 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 `@src/main/ipc.ts` around lines 481 - 493, Extract the orchestration inside the
ipcMain.handle handlers into a core business API (e.g., create functions like
saveAndRefreshSmartPlaylist(playlistId, criteria) and
refreshSmartPlaylistById(playlistId)) in src/main/core; move logic that calls
updatePlaylistCriteria, refreshSmartPlaylist and getPlaylistById (and the
JSON.parse of playlist.criteria / SmartPlaylistCriteria handling) into those
core functions, export them, then simplify the ipc handlers in ipc.ts to just
call these core functions (ipcMain.handle('app/saveSmartPlaylistCriteria' ->
call saveAndRefreshSmartPlaylist; ipcMain.handle('app/refreshSmartPlaylist' ->
call refreshSmartPlaylistById) and return their { songIds } results). Ensure
type usage (SmartPlaylistCriteria) is preserved and core functions are
unit-testable.
…s + lastfm)
playlist-rules.ts
- numeric helper now returns undefined for !Number.isFinite(n), guarding
against NaN/Infinity values from malformed rule inputs
- playCount / skipCount switches now handle 'neq' operator (was silently
dropped before)
- lastPlayed switch now handles 'neq' (via not-between on the same window)
- All three branches guard against !Number.isFinite(value) before the switch
getUserLovedTracks.ts / getUserRecentTracks.ts / getUserTopTracks.ts
- URL changed http -> https
- Added AbortController-based 10s timeout around fetch
- Non-OK response now throws Error with status+statusText instead of
returning undefined (errors are now visible to callers)
- Catch block re-throws so the IPC layer can surface them
ipc.ts
- Last.fm IPC handlers now validate inputs at the boundary:
trim username, reject empty, clamp limit to [1,100], validate period
against allowed set
- app/refreshSmartPlaylist JSON.parse is now wrapped in try/catch with
a safe { songIds: [] } fallback and logger.error
schema.ts
- Added idx_playlists_is_smart Drizzle index to match the
0000_add_smart_playlist_columns.sql migration (closes schema drift)
SmartPlaylistCriteriaEditor.tsx
- Added isValidCriteriaShape type guard: parsed JSON must have
matchType ALL/ANY and rules[] with string field/operator
- placeholder='value' replaced with t('playlist.criteriaValuePlaceholder')
- saveCriteria now wraps the IPC call in try/catch and notifies on error
(previously unhandled rejection)
en.json
- Added playlist.criteriaValuePlaceholder = 'value'
|
Applied CodeRabbit review fixes for #498 (commit dc7fefc on fix/461-smart-playlists-lastfm). playlist-rules.ts
getUserLovedTracks / getUserRecentTracks / getUserTopTracks
ipc.ts
schema.ts
SmartPlaylistCriteriaEditor.tsx
en.json
Intentionally not applied (out of scope for this fix PR):
@CodeRabbit please re-review and confirm whether all the inline findings are now addressed. If the 'neq'\ cases for playCount/skipCount/lastPlayed need different semantics (e.g., IS DISTINCT FROM for nullable timestamps), flag the specific file:line. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== playlist-rules.ts ==="
cat src/main/db/queries/playlist-rules.ts
echo ""
echo "=== getUserLovedTracks.ts ==="
cat src/main/other/lastFm/getUserLovedTracks.ts
echo ""
echo "=== getUserRecentTracks.ts ==="
cat src/main/other/lastFm/getUserRecentTracks.ts
echo ""
echo "=== getUserTopTracks.ts ==="
cat src/main/other/lastFm/getUserTopTracks.ts
echo ""
echo "=== ipc.ts (lastfm + smart playlist sections) ==="
grep -n "lastfm\|smartPlaylist\|refreshSmartPlaylist\|saveSmartPlaylist\|getUserTop\|getUserRecent\|getUserLoved\|JSON.parse\|username\|limit\|period\|clamp\|trim\|songIds" src/main/ipc.ts | head -100
echo ""
echo "=== schema.ts (playlists table) ==="
grep -n -A 30 "playlists\b" src/main/db/schema.ts | head -60
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx ==="
cat src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx
echo ""
echo "=== en.json playlist section ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
print(json.dumps(d.get('playlist', {}), indent=2))
print('---common---')
print(json.dumps(d.get('common', {}), indent=2))
"Length of output: 38681
✅ Confirmed Fixed
|
| Operator | NULL behaviour | Correct? |
|---|---|---|
gt / gte |
NULL excluded — never-played songs don't match "recently played" | ✅ Intentionally correct |
lt / lte |
NULL excluded — never-played songs silently dropped from "played long ago" | lt X days |
eq |
NULL excluded | ✅ Correct (NULL ≠ any date) |
neq |
NULL NOT BETWEEN x AND y → NULL → row excluded |
❌ Never-played songs disappear from "not played in this window", which is almost certainly wrong |
Minimal fix for neq:
case 'neq': return sql`${lastPlayed} IS NULL OR ${lastPlayed} NOT BETWEEN ${cutoff}::timestamp AND ${new Date(cutoff.getTime() + 86400000)}::timestamp`;Optional (but consistent) fix for lt / lte:
case 'lt': return sql`${lastPlayed} IS NULL OR ${lastPlayed} < ${cutoff}::timestamp`;
case 'lte': return sql`${lastPlayed} IS NULL OR ${lastPlayed} <= ${cutoff}::timestamp`;2. Double notification on save failure — minor (src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx)
The .catch() handler already calls addNewNotifications([{ id: 'smartPlaylistSaveFailed', ... }]) and returns false. Then the code falls into the else branch which fires the same notification again — the user sees the banner twice.
Fix: use a flag or restructure:
const result = await window.api.playlistsData
.saveSmartPlaylistCriteria(playlist.playlistId, cleaned)
.catch((error: unknown) => {
void error;
return false as const;
});
if (result) {
addNewNotifications([{ id: 'smartPlaylistCriteriaSaved', ... }]);
changePromptMenuData(false);
} else {
addNewNotifications([{ id: 'smartPlaylistSaveFailed', ... }]);
// ↑ one notification, whether the error came from catch or a falsy result
}💡 Still Out-of-Scope (acknowledged, no action needed this PR)
buildConditionrefactor into focused helpers- Smart-playlist orchestration extracted out of
ipc.ts
Would you like me to open follow-up issues for these two items?
…e-failed notification once
|
@coderabbitai — final comprehensive re-review pass. Please re-review the latest commit on this PR head branch and answer four specific questions in one response. Do not narrow scope to a checklist. Inspect the entire diff history of this branch (every commit between this branch and What I want you to verify1. Re-review the latest commitReview every new change I just pushed against the full current state of the branch. Surface any new blocker, major, minor, or nit you find. 2. Original issue fulfillmentThe PR's "Closes #N" issue defines the user-visible problem this branch is meant to fix. For each "Closes" issue, walk the issue text and tell me:
3. Deviation from issue fixCompare every commit on this branch to the original issue. Did any commit pull the scope in a direction the issue did not ask for? Examples to look for:
If you find any, name the commit, the file, and the lines, and say whether you think the deviation is justified (e.g., "required for the fix to be sound") or removable. 4. Over-sieveingWalk the whole diff and tell me if any of the changes are over-engineered for the original issue. Examples:
For each one, name it, and say whether it should be kept, narrowed, or removed. What to skip
Output formatReply with four clearly-labeled sections — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Issue `#461` ==="
gh issue view 461 --repo Sandakan/Nora
echo ""
echo "=== Commit log on this branch vs base ==="
git log --oneline release/4.0.0-alpha.5..HEAD 2>/dev/null || git log --oneline origin/release/4.0.0-alpha.5..HEAD 2>/dev/null || git log --oneline -20
echo ""
echo "=== playlist-rules.ts full ==="
cat src/main/db/queries/playlist-rules.ts
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx full ==="
cat src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx
echo ""
echo "=== ipc.ts lastfm + smart playlist sections ==="
grep -n "lastfm\|smartPlaylist\|refreshSmartPlaylist\|saveSmartPlaylist\|getUserTop\|getUserRecent\|getUserLoved\|JSON\.parse\|username\|clamp\|period\|songIds\|logger" src/main/ipc.ts | head -120
echo ""
echo "=== ipc.ts relevant blocks with context ==="
grep -n -A 20 "saveSmartPlaylist\|refreshSmartPlaylist\|getUserTop\|getUserRecent\|getUserLoved" src/main/ipc.ts | head -150
echo ""
echo "=== getUserLovedTracks.ts ==="
cat src/main/other/lastFm/getUserLovedTracks.ts
echo ""
echo "=== getUserRecentTracks.ts ==="
cat src/main/other/lastFm/getUserRecentTracks.ts
echo ""
echo "=== getUserTopTracks.ts ==="
cat src/main/other/lastFm/getUserTopTracks.ts
echo ""
echo "=== schema.ts playlists ==="
grep -n -A 20 "playlists\b" src/main/db/schema.ts | head -80
echo ""
echo "=== migration sql ==="
cat src/resources/drizzle/0000_add_smart_playlist_columns.sql
echo ""
echo "=== playlists.ts queries ==="
cat src/main/db/queries/playlists.ts | grep -A 20 "updatePlaylistCriteria\|isSmart\|criteria"
echo ""
echo "=== en.json playlist + common sections ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
print(json.dumps(d.get('playlist', {}), indent=2))
print('---common---')
print(json.dumps(d.get('common', {}), indent=2))
"
echo ""
echo "=== convert.ts relevant ==="
grep -n -A 10 "isSmart\|criteria" src/main/utils/convert.ts
echo ""
echo "=== app.d.ts smart playlist types ==="
grep -n -A 5 "SmartPlaylist\|isSmart\|criteria" src/types/app.d.ts | head -80
echo ""
echo "=== $playlistId.tsx smart playlist UI ==="
grep -n "isSmart\|refreshSmart\|openCriteria\|SmartPlaylist" "src/renderer/src/routes/main-player/playlists/\$playlistId.tsx" | head -50
echo ""
echo "=== Playlist.tsx smart overlay ==="
grep -n "isSmart\|auto_awesome\|SmartPlaylist\|editCriteria" src/renderer/src/components/PlaylistsPage/Playlist.tsx | head -40Length of output: 55083 🏁 Script executed: #!/bin/bash
echo "=== All table names in schema.ts ==="
grep -n "pgTable\|'play_" src/main/db/schema.ts | head -60
echo ""
echo "=== Check for play_history anywhere in codebase ==="
rg -n "play_history" --type ts | head -30
echo ""
echo "=== Check for play_history in SQL files ==="
rg -rn "play_history" --include="*.sql" | head -20
echo ""
echo "=== saveSmartPlaylistCriteria full handler ==="
grep -n -A 10 "saveSmartPlaylistCriteria" src/main/ipc.ts
echo ""
echo "=== Check lastFmUserData usage in renderer ==="
rg -rn "lastFmUserData\|getUserTopTracks\|getUserRecentTracks\|getUserLovedTracks" src/renderer/ | head -30
echo ""
echo "=== preload index.ts lastFmUserData section ==="
grep -n -A 15 "lastFmUserData" src/preload/index.ts | head -50Length of output: 4785
1. Re-review of latest commitVerdict: Pass with two actionable items ✅ IS NULL guards in New findings:
2. Original issue fulfillmentVerdict: Partial — manual rule engine complete, Last.fm-driven automation is wired but disconnected Issue What item 6 asks for:
What the PR produces:
The Last.fm data cannot reach the playlist engine yet. The issue asked for playlists built from Last.fm listening data; what ships is the fetch infrastructure alongside an independent manual-rules feature. Whether this is intentional ("manual rules first; Last.fm seeding in the next PR") should be stated explicitly in the PR description, because right now the implementation advertises Last.fm integration without delivering the user-visible half of it. 3. Deviation from issue fixVerdict: Pass — no unrelated drive-bys; one justifiable but forward-only addition
No unrelated refactors, no config changes the issue didn't require, no reshaped unrelated component contracts. 4. Over-engineeringVerdict: Mostly proportionate — one pattern to tighten
Suggested follow-up issues (if you want me to open them):
|
|
@coderabbitai rereview fix(#461): merge upstream/release/4.0.0-alpha.5 into smart playlists with Last.fm integration Changes:
Please review: smart playlist creation/editing, Last.fm tag integration, song filtering logic |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== git log (recent commits) ==="
git log --oneline -15
echo ""
echo "=== playlist-rules.ts full ==="
cat src/main/db/queries/playlist-rules.ts
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria + refreshSmartPlaylist + lastfm handlers ==="
grep -n -A 25 "saveSmartPlaylistCriteria\|refreshSmartPlaylist\|getUserTop\|getUserRecent\|getUserLoved" src/main/ipc.ts | head -180
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx full ==="
cat src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx
echo ""
echo "=== getUserLovedTracks.ts ==="
cat src/main/other/lastFm/getUserLovedTracks.ts
echo ""
echo "=== getUserRecentTracks.ts ==="
cat src/main/other/lastFm/getUserRecentTracks.ts
echo ""
echo "=== getUserTopTracks.ts ==="
cat src/main/other/lastFm/getUserTopTracks.ts
echo ""
echo "=== schema.ts playlists table ==="
grep -n -A 30 "export const playlists" src/main/db/schema.ts | head -50
echo ""
echo "=== playlists.ts updatePlaylistCriteria ==="
grep -n -A 15 "updatePlaylistCriteria" src/main/db/queries/playlists.ts
echo ""
echo "=== preload index.ts lastFmUserData + smart playlist ==="
grep -n -A 10 "lastFmUserData\|saveSmartPlaylist\|refreshSmartPlaylist" src/preload/index.ts | head -80
echo ""
echo "=== Check if lastFmUserData is consumed in renderer ==="
rg -rn "lastFmUserData\|getUserTopTracks\|getUserRecentTracks\|getUserLovedTracks" src/renderer/ | head -30
echo ""
echo "=== Check for merge conflict markers ==="
rg -rn "<<<<<<|=======|>>>>>>>" src/ | head -20
echo ""
echo "=== app.d.ts smart playlist types + lastfm types ==="
grep -n -A 6 "SmartPlaylist\|isSmart\|criteria\|lastFm\|LastFm" src/types/app.d.ts | head -100
echo ""
echo "=== migration SQL files ==="
ls src/resources/drizzle/
cat src/resources/drizzle/0000_add_smart_playlist_columns.sql
echo ""
echo "=== en.json playlist section ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
print(json.dumps(d.get('playlist', {}), indent=2))
print('---common---')
print(json.dumps(d.get('common', {}), indent=2))
"
echo ""
echo "=== $playlistId.tsx smart playlist actions ==="
grep -n -A 5 "isSmart\|refreshSmart\|openCriteria\|SmartPlaylist" "src/renderer/src/routes/main-player/playlists/\$playlistId.tsx" | head -80
echo ""
echo "=== AddSongsToPlaylistsPrompt.tsx isSmart filter ==="
grep -n -A 5 "isSmart\|criteria" src/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx | head -30
echo ""
echo "=== convert.ts isSmart + criteria ==="
grep -n -A 5 "isSmart\|criteria" src/main/utils/convert.ts | head -30Length of output: 51788 🏁 Script executed: #!/bin/bash
echo "=== All table names in schema.ts ==="
grep -n "pgTable\b" src/main/db/schema.ts | head -40
echo ""
echo "=== play_history references anywhere ==="
rg -rn "play_history" --type ts
rg -rn "play_history" --include="*.sql"
echo ""
echo "=== play_events references ==="
rg -rn "play_events" --type ts | head -20
echo ""
echo "=== skip_events references ==="
rg -rn "skip_events" --type ts | head -20
echo ""
echo "=== saveSmartPlaylistCriteria handler full context ==="
grep -n -A 8 "saveSmartPlaylistCriteria" src/main/ipc.ts
echo ""
echo "=== refreshSmartPlaylist full handler context ==="
grep -n -A 15 "'app/refreshSmartPlaylist'" src/main/ipc.tsLength of output: 6327
Merge qualityPass. No actual git conflict markers exist in the real source files. The
Carry-forward items still open (not addressed in this push)
|
| Status | Item | Severity |
|---|---|---|
| ✅ | Upstream merge clean — no conflicts, no regressions | — |
| ✅ | play_history table confirmed correct |
— |
| ✅ | All prior code-correctness fixes verified intact | — |
saveSmartPlaylistCriteria non-atomic (ipc.ts L509) |
Major | |
Missing shape guard in refreshSmartPlaylist handler (ipc.ts L520) |
Minor | |
key={idx} on removable rule rows |
Minor | |
| 💡 | Boolean option labels not translated | Nit |
|
@coderabbitai rereview |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== git log (recent) ==="
git log --oneline -10
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria + refreshSmartPlaylist full handlers ==="
grep -n -A 30 "saveSmartPlaylistCriteria\|'app/refreshSmartPlaylist'" src/main/ipc.ts | head -120
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx rules map / key usage ==="
grep -n "key={" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx defaultRule ==="
grep -n -A 8 "defaultRule\|defaultCriteria\|isValidCriteriaShape" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -60
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx boolean option labels ==="
grep -n "option value" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -20
echo ""
echo "=== en.json playlist section ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
print(json.dumps(d.get('playlist', {}), indent=2))
"
echo ""
echo "=== playlist-rules.ts lastPlayed neq + lt/lte IS NULL guards ==="
grep -n -A 6 "neq\|'lt'\|'lte'\|IS NULL\|null\|cutoff" src/main/db/queries/playlist-rules.ts | head -80
echo ""
echo "=== isValidCriteriaShape usage in ipc.ts ==="
grep -n "isValidCriteriaShape\|validCriteria\|shape" src/main/ipc.ts | head -20
echo ""
echo "=== db.transaction in ipc.ts ==="
grep -n "transaction\|trx" src/main/ipc.ts | head -20
echo ""
echo "=== Check if isValidCriteriaShape exported/imported ==="
rg -n "isValidCriteriaShape" --type ts | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx saveCriteria function ==="
grep -n -A 30 "saveCriteria\|saveSmartPlaylist" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -60Length of output: 13767
✅ Confirmed Fixed Since Last Pass
|
…riteria Replace hardcoded 'true'/'false' strings with i18n keys playlist.valueTrue and playlist.valueFalse
|
@coderabbitai review my last commit and see if it satisfies what you asked and the original issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Latest commits ==="
git log --oneline -8
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria full handler ==="
grep -n -A 25 "saveSmartPlaylistCriteria" src/main/ipc.ts | head -60
echo ""
echo "=== ipc.ts refreshSmartPlaylist full handler ==="
grep -n -A 25 "'app/refreshSmartPlaylist'" src/main/ipc.ts | head -60
echo ""
echo "=== db.transaction / trx usage in ipc.ts ==="
grep -n "transaction\|trx\|isValidCriteria\|validCriteria" src/main/ipc.ts | head -30
echo ""
echo "=== isValidCriteriaShape - where it lives now ==="
rg -rn "isValidCriteriaShape" --type ts | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - key usage on rule rows ==="
grep -n "key={" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - defaultRule function ==="
grep -n -A 8 "defaultRule\|defaultCriteria\|randomUUID\|crypto" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -50
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - boolean option labels ==="
grep -n "option value\|valueTrue\|valueFalse\|playlist\." src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -30
echo ""
echo "=== en.json playlist.valueTrue / valueFalse ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
playlist = d.get('playlist', {})
for k, v in playlist.items():
print(f' {k}: {v}')
"
echo ""
echo "=== playlist-rules.ts isValidCriteriaShape (if moved there) ==="
grep -n "isValidCriteriaShape\|validCriteria\|shape" src/main/db/queries/playlist-rules.ts | head -20
echo ""
echo "=== Issue `#461` body ==="
gh issue view 461 --repo Sandakan/Nora 2>/dev/null || echo "Could not fetch issue"Length of output: 14201
✅ Fixed in this commit
|
Three items CodeRabbit flagged:
- saveSmartPlaylistCriteria handler now wraps updatePlaylistCriteria +
refreshSmartPlaylist in a db.transaction so partial failure can't
leave the DB with criteria without refreshed songs (silent corruption).
- refreshSmartPlaylist handler validates parsed JSON shape before passing
to the rule engine, preventing an empty playlist when stale criteria
return undefined rules.
- defaultRule() returns a stable crypto.randomUUID id and the list uses
key={rule.id ?? idx} so removing a middle rule doesn't misalign React
DOM nodes (focus ring / select value drift).
|
@coderabbitai review my last commit and see if it satisfies what you asked and the original issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Latest commits ==="
git log --oneline -8
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria full handler ==="
grep -n -A 30 "saveSmartPlaylistCriteria" src/main/ipc.ts | head -70
echo ""
echo "=== ipc.ts refreshSmartPlaylist full handler ==="
grep -n -A 30 "'app/refreshSmartPlaylist'" src/main/ipc.ts | head -70
echo ""
echo "=== db.transaction / trx references in ipc.ts ==="
grep -n "transaction\|trx\|isValidCriteria" src/main/ipc.ts | head -30
echo ""
echo "=== isValidCriteriaShape - where it lives now ==="
rg -rn "isValidCriteriaShape" --type ts | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - key usage on rule rows ==="
grep -n "key={" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - defaultRule function ==="
grep -n -A 10 "defaultRule\|randomUUID\|crypto\." src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -60
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - boolean option labels ==="
grep -n "option value\|valueTrue\|valueFalse" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -20
echo ""
echo "=== playlist-rules.ts lastPlayed / neq / IS NULL guards ==="
grep -n -A 8 "neq\|'lt'\|'lte'\|IS NULL\|null.*cutoff\|cutoff.*null" src/main/db/queries/playlist-rules.ts | head -80
echo ""
echo "=== Check for any new shared util for criteria shape ==="
fd -e ts "smartPlaylist" src/ 2>/dev/null || echo "no dedicated file found"
rg -rn "isValidCriteriaShape" src/ | head -20
echo ""
echo "=== Check if lastFmUserData consumed in renderer ==="
rg -rn "lastFmUserData\|getUserTopTracks\|getUserRecentTracks\|getUserLovedTracks" src/renderer/ | head -20
echo ""
echo "=== en.json valueTrue / valueFalse ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
playlist = d.get('playlist', {})
for k, v in playlist.items():
print(f' {k}: {v}')
"Length of output: 12766
✅ All Previously Open Items: Fixed
One Remaining Nit (non-blocking)The inline shape guard in if (
!parsed ||
typeof parsed !== 'object' ||
!Array.isArray(parsed.rules) ||
(parsed.matchType !== 'ALL' && parsed.matchType !== 'ANY') // add this
) { ... }Original Issue
|
…atchType check The inline shape guard in the refreshSmartPlaylist IPC handler only checked rules was an array, leaving matchType unvalidated. A persisted criteria blob with matchType: 'BOGUS' would slip through and silently return an empty playlist via evaluateSmartPlaylist's default branch. Parity with the editor's isValidCriteriaShape: now requires matchType === 'ALL' || matchType === 'ANY'. Closes the non-blocking nit CR flagged after my last commit.
|
@coderabbitai review my last commit and see if it satisfies what you asked and the original issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Latest commits ==="
git log --oneline -8
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria full handler ==="
grep -n -A 35 "saveSmartPlaylistCriteria" src/main/ipc.ts | head -80
echo ""
echo "=== ipc.ts refreshSmartPlaylist full handler ==="
grep -n -A 35 "'app/refreshSmartPlaylist'" src/main/ipc.ts | head -80
echo ""
echo "=== matchType validation in ipc.ts ==="
grep -n "matchType\|ALL\|ANY" src/main/ipc.ts | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - defaultRule + key usage ==="
grep -n "defaultRule\|randomUUID\|crypto\.\|key={" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -30
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - boolean option labels ==="
grep -n "option value\|valueTrue\|valueFalse" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -10
echo ""
echo "=== playlist-rules.ts - matchType handling ==="
grep -n "matchType\|ALL\|ANY\|and\|or" src/main/db/queries/playlist-rules.ts | head -20
echo ""
echo "=== en.json playlist section (all keys) ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
playlist = d.get('playlist', {})
for k, v in playlist.items():
print(f' {k}: {v}')
"
echo ""
echo "=== Check if lastFmUserData consumed in renderer ==="
rg -rn "lastFmUserData\|getUserTopTracks\|getUserRecentTracks\|getUserLovedTracks" src/renderer/ | head -20
echo ""
echo "=== Check for any new files added in latest commit ==="
git diff --name-only HEAD~1 HEAD 2>/dev/null | head -30Length of output: 9725
✅ Full Status — All Items Resolved
No regressions introduced. Original Issue
|
|
@coderabbitai please review my latest commit on this PR and provide a final merge verdict. If everything from your previous review is satisfied, explicitly say 'Branch is clean and ready to merge' and explain why Sandakan should approve. If any blockers remain, list them. Specifically check:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Latest commits ==="
git log --oneline -10
echo ""
echo "=== Files changed in latest commit ==="
git diff --name-only HEAD~1 HEAD 2>/dev/null | head -30
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria full handler ==="
grep -n -A 40 "saveSmartPlaylistCriteria" src/main/ipc.ts | head -90
echo ""
echo "=== ipc.ts refreshSmartPlaylist full handler ==="
grep -n -A 40 "'app/refreshSmartPlaylist'" src/main/ipc.ts | head -90
echo ""
echo "=== matchType validation in ipc.ts ==="
grep -n "matchType\|ALL\|ANY" src/main/ipc.ts | head -20
echo ""
echo "=== db.transaction / trx in ipc.ts ==="
grep -n "transaction\|trx" src/main/ipc.ts | head -20
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - key + defaultRule + bool options ==="
grep -n "key={\|defaultRule\|randomUUID\|crypto\.\|valueTrue\|valueFalse\|option value" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -30
echo ""
echo "=== All migration SQL files ==="
ls src/resources/drizzle/
echo ""
echo "=== Migration 0003 content ==="
find src/resources/drizzle -name "0003_*" -exec cat {} \;
echo ""
echo "=== Migration 0004 content ==="
find src/resources/drizzle -name "0004_*" -exec cat {} \;
echo ""
echo "=== Migration 0000 smart playlist content ==="
cat src/resources/drizzle/0000_add_smart_playlist_columns.sql 2>/dev/null || find src/resources/drizzle -name "0000_*" -exec cat {} \;
echo ""
echo "=== schema.ts playlists table ==="
grep -n -A 35 "export const playlists" src/main/db/schema.ts | head -50
echo ""
echo "=== Check for merge conflicts ==="
rg -rn "<<<<<<|=======|>>>>>>>" src/ | head -10
echo ""
echo "=== Check for console.log / debugger in new files ==="
rg -n "console\.log\|debugger\b" src/main/db/queries/playlist-rules.ts src/main/other/lastFm/ src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx 2>/dev/null | head -20
echo ""
echo "=== lastFmUserData renderer usage ==="
rg -rn "lastFmUserData\|getUserTopTracks\|getUserRecentTracks\|getUserLovedTracks" src/renderer/ | head -20
echo ""
echo "=== en.json playlist section all keys ==="
python3 -c "
import json
with open('src/renderer/src/assets/locales/en/en.json') as f:
d = json.load(f)
playlist = d.get('playlist', {})
for k, v in playlist.items():
print(f' {k}: {v}')
print('---common.save---')
print(d.get('common', {}).get('save', 'MISSING'))
"
echo ""
echo "=== drizzle meta journal (migration ordering) ==="
find src/resources/drizzle -name "*.json" -exec cat {} \; 2>/dev/null | head -80
echo ""
echo "=== git diff vs upstream base (file count) ==="
git log --oneline origin/release/4.0.0-alpha.5..HEAD 2>/dev/null | head -20 || echo "Cannot reach upstream"Length of output: 13274 🏁 Script executed: #!/bin/bash
echo "=== Find all SQL files in project ==="
fd -e sql | sort | head -40
echo ""
echo "=== Find all drizzle config files ==="
fd "drizzle.config" | head -10
echo ""
echo "=== drizzle config contents ==="
fd "drizzle.config" -e ts -e js | xargs cat 2>/dev/null | head -30
echo ""
echo "=== Find all migration directories ==="
fd -t d "migrations\|drizzle" | head -20
echo ""
echo "=== Full tree of drizzle-related dirs ==="
fd -e sql | sort
echo ""
echo "=== Meta journal files ==="
fd "journal.json\|_journal" | xargs cat 2>/dev/null | head -60
echo ""
echo "=== Check if 0003/0004 files exist with different path ==="
fd "0003\|0004" --type f | head -20
echo ""
echo "=== schema.ts skip_count column ==="
grep -n "skip\|skipCount\|skip_count" src/main/db/schema.ts | head -20
echo ""
echo "=== playlist-rules.ts skip_count references ==="
grep -n "skip\|skipCount\|skip_count" src/main/db/queries/playlist-rules.ts | head -20Length of output: 3411 🏁 Script executed: #!/bin/bash
echo "=== 0003_lowly_komodo.sql content ==="
cat resources/drizzle/0003_lowly_komodo.sql
echo ""
echo "=== 0002_square_greymalkin.sql content ==="
cat resources/drizzle/0002_square_greymalkin.sql
echo ""
echo "=== 0000_init.sql first 40 lines ==="
head -40 resources/drizzle/0000_init.sql
echo ""
echo "=== How migrations are applied — search for migrate/runMigrations in main process ==="
rg -rn "migrate\b\|runMigrations\|drizzle\|resources/drizzle\|sql.*file\|readFileSync.*sql\|glob.*sql" --type ts src/main/ | grep -v "schema\|query\|playlist-rules\|playlists\.ts\|convert" | head -40
echo ""
echo "=== Search for any migration runner referencing the resources path ==="
rg -rn "0000_add_smart\|src/resources/drizzle\|resources/drizzle" --type ts | head -20
echo ""
echo "=== Check if there is a migration loader that reads all SQL files ==="
fd -e ts "migrate\|migration\|db" src/main/ | head -20
echo ""
echo "=== src/main/db/ directory contents ==="
fd . src/main/db/ | sort | head -30Length of output: 4674 🏁 Script executed: #!/bin/bash
echo "=== db.ts full content ==="
cat src/main/db/db.ts
echo ""
echo "=== Does anything reference src/resources/drizzle at runtime? ==="
rg -rn "src/resources/drizzle\|__resources\|app\.getPath\|migrate" --type ts src/main/ | head -30
echo ""
echo "=== electron-builder or vite config — how src/resources is bundled ==="
fd "electron.vite\|vite.config\|electron-builder" -e ts -e js -e json | head -10
fd "electron.vite\|vite.config\|electron-builder" -e ts -e js -e json | xargs grep -l "resources/drizzle\|extraResources\|asarUnpack" 2>/dev/null | head -5Length of output: 4169
Code-Level Items (all prior raises)
❌ BLOCKER — Missing Drizzle Migration (
|
…ct directory The schema migration for adding is_smart and criteria columns to the playlists table was committed to src/resources/drizzle/ but db.ts resolves migrations from resources/drizzle/ at the repo root. So the migration would never run end-to-end: any user upgrading from a prior version would hit an error the moment a smart playlist operation tried to read/write is_smart or criteria. Resolution: - npm run db:generate regenerated the migration into the correct path -> resources/drizzle/0004_mixed_adam_destine.sql (and matching snapshot) - Deleted the misplaced src/resources/drizzle/0000_add_smart_playlist_columns.sql - Confirmed schema and migrations are coherent: another db:generate run reports 'No schema changes, nothing to migrate' - Journal sequence now: 0003_lowly_komodo (upstream) -> 0004_mixed_adam_destine
|
@coderabbitai review my last commit and see if it satisfies what you asked and the original issue. The migration blocker is fixed — the misplaced file in src/resources/drizzle/ has been deleted, npm run db:generate now correctly outputs to resources/drizzle/0004_mixed_adam_destine.sql, and a second db:generate run reports 'No schema changes, nothing to migrate'. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Latest commits ==="
git log --oneline -8
echo ""
echo "=== Files changed in latest commit ==="
git diff --name-only HEAD~1 HEAD 2>/dev/null | head -30
echo ""
echo "=== resources/drizzle/ directory listing ==="
ls -1 resources/drizzle/
echo ""
echo "=== 0004_mixed_adam_destine.sql content ==="
cat resources/drizzle/0004_mixed_adam_destine.sql 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== Check misplaced file is gone ==="
ls src/resources/drizzle/ 2>/dev/null || echo "src/resources/drizzle/ does NOT exist (correct)"
echo ""
echo "=== db.ts migration path ==="
grep -n "migrationsFolder\|resources/drizzle\|migrate" src/main/db/db.ts | head -20
echo ""
echo "=== drizzle meta journal (migration ordering) ==="
find resources/drizzle -name "*.json" | xargs cat 2>/dev/null | head -80
echo ""
echo "=== ipc.ts saveSmartPlaylistCriteria full handler (transaction check) ==="
grep -n -A 35 "saveSmartPlaylistCriteria" src/main/ipc.ts | head -80
echo ""
echo "=== ipc.ts refreshSmartPlaylist shape guard + matchType check ==="
grep -n -A 35 "'app/refreshSmartPlaylist'" src/main/ipc.ts | head -80
echo ""
echo "=== SmartPlaylistCriteriaEditor.tsx - key + randomUUID + bool options ==="
grep -n "key={\|randomUUID\|crypto\.\|valueTrue\|valueFalse\|option value" src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -20
echo ""
echo "=== No console.log / debugger in new files ==="
rg -n "console\.log\|debugger\b" src/main/db/queries/playlist-rules.ts src/main/other/lastFm/ src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx 2>/dev/null | head -20
echo ""
echo "=== No merge conflict markers ==="
rg -rn "^<<<<<<|^=======$|^>>>>>>" src/ | head -10Length of output: 9404
Final Status
All blockers, majors, minors, and nits from the full review pass are resolved. Sandakan can approve: the smart playlist rule engine, persistence, IPC wiring, UI, migration, and safety guards are all correct and production-ready. 🐇 ✨ |
…n, songIds bound, console.error cleanup Major 1: refreshSmartPlaylist now validates persisted lastFmSource shape (username/type/period/limit) before returning skipped. Malformed stored source returns invalid-lastfm-source instead of silently skipping rule refresh. Major 2: syncLastFmToSmartPlaylist IPC now rejects songIds arrays over MAX_LASTFM_MATCH_IDS (100) before acquiring the playlist lock. Major 3: added 10 validateLastFmSource tests (34 total in file). Minor 6: replaced remaining console.error in changed renderer paths (playlist artwork picker, song history clear) with log(). Minor 4/5 (cancellation IPC, serial matching) retained per CR fix direction: stale-write protection + pending state cover the overlapping-operation risk; true cancellation needs a main-process batch matcher (follow-up).
|
@CodeRabbit Act as a senior code reviewer and QA engineer. Perform a comprehensive, deep-dive analysis of the entire codebase, excluding any prior feedback or issues I have raised. Your goal is to determine if the code is fully ready for testing and will ship with zero bugs. For every action, function, method, and state change, verify that there is a correct, equal, and opposite reaction or handling (e.g. error handling, state rollback, resource cleanup, input validation, edge case coverage). Ensure nothing slips through or behaves incorrectly. Specifically, check: all possible execution paths and branches, input validation and sanitization, error handling and exception propagation, resource management, state consistency across async operations, boundary conditions and edge cases, side effects and their reversibility, concurrency and race conditions, security vulnerabilities, logging and observability. Provide a detailed report with: overall readiness verdict (Ready/Not Ready) with confidence level, list of all potential bugs or gaps with severity (Critical/Major/Minor), specific code locations and suggested fixes, summary of what is working correctly. Assume production environment. Be thorough. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== PR metadata and comparison range ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable \
2>&1 || true
base="$(git merge-base HEAD origin/release/4.0.0-alpha.5 2>/dev/null || \
git merge-base HEAD release/4.0.0-alpha.5 2>/dev/null || \
git rev-parse HEAD~1)"
printf 'BASE=%s\n' "$base"
echo '=== Commits and changed files ==='
git log --oneline --decorate "${base}..HEAD"
git diff --name-status "${base}..HEAD"
echo '=== Changed TypeScript and TSX structure ==='
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
[ -f "$file" ] || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
echo '=== Smart-playlist, Last.fm, IPC, and persistence definitions/call sites ==='
rg -n -C 8 --glob '*.{ts,tsx}' \
'\b(SmartPlaylistResult|SmartPlaylistCriteria|LastFmSource|validateSmartPlaylistCriteria|saveSmartPlaylistCriteria|refreshSmartPlaylist|evaluateSmartPlaylist|syncLastFmToSmartPlaylist|replaceSmartPlaylistMembership|setLastFmSource|smartPlaylistLocks|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist)\b' \
src || true
echo '=== Primary feature implementations ==='
for file in \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/core/replaceSmartPlaylistMembership.ts \
src/main/core/setLastFmSource.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
if [ -f "$file" ]; then
echo "--- $file"
cat -n "$file"
fi
done
echo '=== Migration journal and snapshot lineage ==='
find resources/drizzle -maxdepth 2 -type f | sort
echo '--- journal ---'
cat resources/drizzle/meta/_journal.json 2>/dev/null || true
echo '--- migration SQL ---'
find resources/drizzle -maxdepth 1 -name '*.sql' \
-exec sh -c 'echo "--- $1"; cat -n "$1"' _ {} \;
echo '--- snapshot lineage ---'
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("resources/drizzle/meta").glob("*_snapshot.json")):
try:
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
except Exception as exc:
print(f"{path.name}: invalid JSON: {exc}")
PY
echo '=== Test inventory and changed-code hygiene ==='
echo '--- PR-added tests ---'
git diff --name-only "${base}..HEAD" |
rg -i '(^|/)(test|tests|__tests__|spec)(/|\.|$)' || true
echo '--- relevant tests ---'
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|replaceSmartPlaylistMembership)' \
test src 2>/dev/null || true
echo '--- suspicious changed-code artifacts ---'
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
xargs -r rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any' || trueLength of output: 121465 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Validator and its tests ==='
cat -n src/main/db/queries/validateSmartPlaylistCriteria.ts
cat -n test/src/main/db/queries/validateSmartPlaylistCriteria.test.ts
echo '=== Main IPC: imports, lock helper, and smart-playlist handlers ==='
sed -n '1,180p;500,700p' src/main/ipc.ts | cat -n
echo '=== Preload and shared type contracts ==='
sed -n '420,545p' src/preload/index.ts | cat -n
rg -n -A60 -B10 \
'(SmartPlaylistResult|SmartPlaylistFailureReason|LastFmSource|SmartPlaylistCriteria|IpcChannels)' \
src/types/app.d.ts
echo '=== Rule evaluator and refresh implementation ==='
cat -n src/main/db/queries/playlist-rules.ts
echo '=== Last.fm synchronization and legacy mutation paths ==='
for file in \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/core/replaceSmartPlaylistMembership.ts \
src/main/core/setLastFmSource.ts; do
if [ -f "$file" ]; then
echo "--- $file"
cat -n "$file"
fi
done
echo '=== Playlist persistence helpers and all relevant mutation call sites ==='
rg -n -A35 -B8 \
'export const (getPlaylistById|updatePlaylistCriteria|createPlaylist)\b' \
src/main/db/queries/playlists.ts
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(syncLastFmToSmartPlaylist|replaceSmartPlaylistMembership|setLastFmSource|saveSmartPlaylistCriteria|refreshSmartPlaylist|evaluateSmartPlaylist)\s*\(' \
src test || true
echo '=== Last.fm API handler payload validation ==='
rg -n -C 16 \
"'app/(lastfm/getUserTopTracks|lastfm/getUserRecentTracks|lastfm/getUserLovedTracks|syncLastFmToSmartPlaylist)'" \
src/main/ipc.ts
echo '=== Direct changed-path error reporting and UI pending-state checks ==='
rg -n -C 3 --glob '*.{ts,tsx}' \
'console\.(error|log)\s*\(|isSaving|isSyncing|AbortController|addNewNotifications' \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx'Length of output: 102980
Overall verdict: Not ReadyConfidence: High I completed an independent static review of the current PR state. I inspected the smart-playlist rule engine, stored criteria handling, IPC handlers, preload contracts, Last.fm synchronization, renderer behavior, migrations, and tests. I did not run the application, build, test suite, or migration upgrade. Static review cannot prove zero runtime defects. Major findings1.
|
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR. Cross-check every changed file against its callers. Include nitpick-level findings too. Prior-finding disposition (verify, do not re-report)
Required verdict formatEnd with Report structure
Cite file paths and line numbers. If a prior finding is already fixed, say so explicitly. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base_ref="origin/release/4.0.0-alpha.5"
echo '=== PR metadata ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus \
2>&1 || true
echo '=== Resolve comparison base ==='
git fetch origin release/4.0.0-alpha.5 --quiet 2>&1 || true
base="$(git merge-base HEAD "$base_ref" 2>/dev/null || git merge-base HEAD release/4.0.0-alpha.5 2>/dev/null)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$(git rev-parse HEAD)"
printf 'behind_upstream='
git rev-list --count "HEAD..$base_ref" 2>/dev/null || true
printf 'ahead_upstream='
git rev-list --count "$base_ref..HEAD" 2>/dev/null || true
echo '=== Full PR commit and changed-file inventory ==='
git log --oneline --decorate "${base}..HEAD"
git diff --name-status "${base}..HEAD"
git diff --stat "${base}..HEAD"
echo '=== AST map of every changed TypeScript/TSX file ==='
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
echo '=== Complete contents of changed feature source files ==='
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
cat -n "$file"
done
echo '=== Feature definitions and caller cross-reference ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|replaceSmartPlaylistMembership|setLastFmSource|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
src test || true
echo '=== Full diff for non-source changes ==='
git diff --find-renames --unified=30 "${base}..HEAD" -- \
resources/drizzle \
'src/renderer/src/assets/locales/**' \
'test/**' \
'package.json' \
'pnpm-lock.yaml' \
'package-lock.json' \
'yarn.lock' \
| sed -n '1,8000p'
echo '=== Migration journal and snapshot lineage ==='
find resources/drizzle -maxdepth 2 -type f | sort
cat resources/drizzle/meta/_journal.json 2>/dev/null || true
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("resources/drizzle/meta").glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Tests and static hygiene ==='
git diff --name-only "${base}..HEAD" | rg -i '(^|/)(test|tests|__tests__|spec)(/|\.|$)' || true
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
test src 2>/dev/null || true
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
xargs -r rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any' || trueLength of output: 3735 🏁 Script executed: #!/bin/bash
set -uo pipefail
base="838316cb5a83359499422937835a837e5fc86053"
head="f22fdf9b57abd818dd2e43a9b43a902cf5e4d4d9"
echo '=== Local Git availability ==='
git rev-parse HEAD 2>&1 || true
git branch -a 2>&1 | sed -n '1,160p'
git cat-file -e "${base}^{commit}" 2>/dev/null || \
git fetch origin "$base" --quiet 2>&1 || true
git cat-file -e "${head}^{commit}" 2>/dev/null || \
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>&1 || true
git cat-file -e "${base}^{commit}" 2>&1 || true
git cat-file -e "${head}^{commit}" 2>&1 || true
actual_head="$(git rev-parse HEAD 2>/dev/null || printf '%s' "$head")"
if git cat-file -e "${base}^{commit}" 2>/dev/null && git cat-file -e "${actual_head}^{commit}" 2>/dev/null; then
range="${base}..${actual_head}"
echo "RANGE=$range"
echo '=== Full PR inventory ==='
git log --oneline --reverse "$range"
git diff --name-status "$range"
git diff --stat "$range"
echo '=== Changed source AST maps ==='
git diff --name-only "$range" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
echo '=== Smart playlist, Last.fm, and caller implementations ==='
for file in \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$file" || continue
echo "--- $file"
cat -n "$file"
done
echo '=== Definitions and all callers ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
src test || true
echo '=== Full changed-file diff ==='
git diff --find-renames --unified=25 "$range" -- \
'src/**' 'resources/drizzle/**' 'test/**' \
| sed -n '1,10000p'
echo '=== Migration metadata ==='
find resources/drizzle -maxdepth 2 -type f | sort
cat resources/drizzle/meta/_journal.json 2>/dev/null || true
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("resources/drizzle/meta").glob("*_snapshot.json")):
d = json.loads(path.read_text())
print(f"{path.name}: id={d.get('id')} prevId={d.get('prevId')}")
PY
echo '=== Tests and hygiene ==='
git diff --name-only "$range" | rg -i '(^|/)(test|tests|__tests__|spec)(/|\.|$)' || true
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
test src || true
git diff --name-only "$range" -- 'src/**/*.ts' 'src/**/*.tsx' |
xargs -r rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any' || true
else
echo '=== Fallback: GitHub changed-files API ==='
gh api --paginate "repos/Sandakan/Nora/pulls/498/files?per_page=100" \
--jq '.[] | [.filename, .status, .additions, .deletions] | `@tsv`' 2>&1 || true
fiLength of output: 106483 🏁 Script executed: #!/bin/bash
set -euo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-5cwWZb"
echo '=== Inventory and heading locations in the completed audit log ==='
rg -n '^===|^--- src/' "$log" | sed -n '1,500p'
echo '=== Extract current implementations from the audit log ==='
for name in \
'src/main/db/queries/validateSmartPlaylistCriteria.ts' \
'src/main/db/queries/playlist-rules.ts' \
'src/main/db/queries/playlists.ts' \
'src/main/core/syncLastFmToSmartPlaylist.ts' \
'src/main/ipc.ts' \
'src/preload/index.ts' \
'src/types/app.d.ts' \
'src/main/other/lastFm/getUserTopTracks.ts' \
'src/main/other/lastFm/getUserRecentTracks.ts' \
'src/main/other/lastFm/getUserLovedTracks.ts' \
'src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx' \
'src/renderer/src/hooks/useLastFmConsumer.ts' \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx'; do
echo "--- $name"
line="$(grep -n -m1 -- "--- $name" "$log" | cut -d: -f1 || true)"
if test -n "$line"; then
sed -n "${line},$((line + 460))p" "$log" | sed -n '/^--- /{ /--- '"$(printf '%s' "$name" | sed 's/[.[\*^$()+?{|]/\\&/g')"'/!q; }; p'
else
echo 'NOT FOUND'
fi
done
echo '=== Current working-tree targeted evidence ==='
for file in \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$file" || continue
echo "--- $file"
rg -n -C 5 \
'(validateLastFmSource|validateSmartPlaylistCriteria|MAX_LASTFM_MATCH_IDS|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|lastFmSource|evaluateSmartPlaylist|refreshSmartPlaylist|ipcMain\.handle|return \{ success|Number\.isInteger|period)' \
"$file" || true
done
echo '=== Caller and bridge contract checks ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'(syncLastFmToSmartPlaylist|setLastFmSource|replaceSmartPlaylistMembership|saveSmartPlaylistCriteria|refreshSmartPlaylist|lastFmSource)' \
src test || trueLength of output: 5260 🏁 Script executed: #!/bin/bash
set -uo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-5cwWZb"
echo '=== Validator ==='
sed -n '871,1031p' "$log"
echo '=== Rule evaluator and playlist queries ==='
sed -n '1032,1525p' "$log"
echo '=== Atomic Last.fm sync core ==='
sed -n '1526,1640p' "$log"
echo '=== Smart-playlist and Last.fm IPC handlers ==='
sed -n '1809,2637p' "$log" | rg -n -C 10 \
'(MAX_LASTFM_MATCH_IDS|smartPlaylistLocks|withSmartPlaylistLock|lastfm|getUserTopTracks|getUserRecentTracks|getUserLovedTracks|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|setLastFmSource|ipcMain\.handle)' || true
echo '=== Preload and global API types ==='
sed -n '2638,4728p' "$log" | rg -n -C 8 \
'(lastFmUserData|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|SmartPlaylistResult|LastFmSource|Window)' || true
echo '=== Renderer async flows ==='
sed -n '4729,5798p' "$log" | rg -n -C 8 \
'(isCreating|isSaving|isSyncing|saveSmartPlaylistCriteria|refreshSmartPlaylist|syncLastFmToSmartPlaylist|AbortController|searchSongsByName|addNewNotifications)' || true
echo '=== Last.fm helpers ==='
sed -n '5799,5997p' "$log"
echo '=== Full call-site results from initial audit ==='
sed -n '5998,6787p' "$log"
echo '=== Changed-file list ==='
sed -n '8,104p' "$log"Length of output: 178857
VerdictMERGEABLE state: I reviewed the full diff against The prior fixes are present and were not re-reported:
This was a static audit. I did not run the build, test suite, or migration upgrade. Major findings
|
…Id, shared constants, IPC reuse, nits Major (period validation): validateLastFmSource now rejects any defined period that is not an allowed string (numeric/boolean/object/null all rejected), not just non-allowed strings. Minor (playlistId validation): - saveSmartPlaylistCriteria uses Number.isSafeInteger instead of isFinite so fractional ids are rejected before the lock is created. - refreshSmartPlaylist validates playlistId before creating the lock. Minor (shared constants): VALID_PERIODS and MAX_LIMIT now live in src/main/db/queries/smartPlaylistConstants.ts, imported by both validateSmartPlaylistCriteria and syncLastFmToSmartPlaylist to prevent validation drift. Nit (IPC duplication): syncLastFmToSmartPlaylist IPC handler reuses validateLastFmSource instead of inline duplicate validation. Nit (IpcChannels union): app/syncLastFmToSmartPlaylist added to the IpcChannels type union. Nit (SmartPlaylistCriteriaEditor): save uses try/catch instead of .catch; Cancel button is disabled while a save is pending. Tests: added 4 cases for non-string period values in validateLastFmSource. 374 tests pass (38 in validateSmartPlaylistCriteria), typecheck clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. Do not accept generic patterns — validate each finding against the actual code. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== PR metadata and merge state ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus \
2>&1 || true
base="$(git merge-base HEAD origin/release/4.0.0-alpha.5 2>/dev/null || \
git merge-base HEAD release/4.0.0-alpha.5 2>/dev/null || \
git rev-parse HEAD~1)"
printf 'BASE=%s\n' "$base"
printf 'ahead='
git rev-list --count "${base}..HEAD"
printf 'behind='
git rev-list --count "HEAD..${base}"
echo '=== Changed files ==='
git diff --name-status "${base}..HEAD"
echo '=== Key files: validator, ipc, syncLastFm, playlist-rules, useLastFmConsumer ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/ipc.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/db/queries/playlist-rules.ts \
src/renderer/src/hooks/useLastFmConsumer.ts \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/types/app.d.ts; do
if [ -f "$file" ]; then
echo "--- $file"
cat -n "$file"
fi
doneLength of output: 131061 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== All usages of MAX_LASTFM_MATCH_IDS ==='
rg -n 'MAX_LASTFM_MATCH_IDS' src test
echo '=== Confirm ipc.ts does not import smartPlaylistConstants ==='
rg -n "from './db/queries/smartPlaylistConstants'|from '`@main/db/queries/smartPlaylistConstants`'" src/main/ipc.ts || echo "NOT IMPORTED"
echo '=== Check syncLastFmToSmartPlaylist core for any array-length bound ==='
rg -n 'songIds\.length' src/main/core/syncLastFmToSmartPlaylist.ts
echo '=== Check preload types for lastFmUserData / smart playlist methods, and Playlist type for limit UI ==='
rg -n -A5 -B2 'lastFmUserData|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist' src/preload/index.ts
echo '=== Check htmlFor/id usage in SmartPlaylistCriteriaEditor and other label patterns in codebase for convention ==='
rg -n 'htmlFor|id=' src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx || true
rg -n 'htmlFor' src/renderer/src/components -g '*.tsx' | head -20Length of output: 6558
VerdictMERGEABLE state: I checked this diff against the full coverage checklist. I did not run lint, typecheck, or tests. I cannot confirm CI pass or fail from static review alone. Both prior major findings are fixed. I confirm this instead of repeating them:
No new major or blocking defect survived my four-point self-review this round. Major findingsNone found in this pass. Minor findings
|
…load undefined, a11y labels, limit UI
Minors:
- ipc.ts imports MAX_LASTFM_MATCH_IDS from smartPlaylistConstants instead of redeclaring it.
- syncLastFmToSmartPlaylist now enforces the MAX_LASTFM_MATCH_IDS bound on songIds inside the core function (defense-in-depth), and replaces inline username/type checks with validateLastFmSource (which now returns the normalized source on success).
- preload declares the three lastfm user-data methods as Promise<...> | undefined to match the main handler returning undefined for empty username.
Nitpicks:
- smartPlaylistConstants MAX_LASTFM_MATCH_IDS is now imported where used (no unused export).
- inline username/type checks removed (validateLastFmSource is the single source of truth).
UI:
- SmartPlaylistCriteriaEditor field/operator/value labels now use htmlFor paired with id on each control (a11y).
- Added a Result limit input (criteria.limit) with validation (1..MAX_LIMIT), i18n keys added to en.json.
validateLastFmSource now returns { success: true, source } with the normalized source; tests updated accordingly. typecheck:node 0 errors, lint clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. Do not accept generic patterns — validate each finding against the actual code. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base_ref="origin/release/4.0.0-alpha.5"
echo '=== PR metadata and merge state ==='
gh pr view 498 --repo "$repo" \
--json number,state,title,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
2>&1 || true
echo '=== Resolve base and branch distance ==='
git fetch origin release/4.0.0-alpha.5 --quiet 2>&1 || true
base="$(git merge-base HEAD "$base_ref" 2>/dev/null || git merge-base HEAD release/4.0.0-alpha.5)"
head="$(git rev-parse HEAD)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
printf 'behind_base='
git rev-list --count "HEAD..$base_ref" 2>/dev/null || true
printf 'ahead_base='
git rev-list --count "$base_ref..HEAD" 2>/dev/null || true
echo '=== Full diff inventory ==='
git log --oneline --reverse "${base}..HEAD"
git diff --name-status "${base}..HEAD"
git diff --stat "${base}..HEAD"
echo '=== Changed source AST map ==='
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
echo '=== Current smart-playlist, Last.fm, IPC, and renderer implementations ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$file" || continue
echo "--- $file"
cat -n "$file"
done
echo '=== Changed-file callers and IPC/preload contract cross-check ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|SmartPlaylistResult|LastFmSource|MAX_LASTFM_MATCH_IDS|MAX_LIMIT)\b' \
src test || true
echo '=== Full changed non-source diff: migrations, locales, tests, and config ==='
git diff --find-renames --unified=25 "${base}..HEAD" -- \
resources/drizzle \
'src/renderer/src/assets/locales/**' \
test \
package.json \
pnpm-lock.yaml \
package-lock.json \
yarn.lock \
| sed -n '1,10000p'
echo '=== Migration metadata ==='
find resources/drizzle -maxdepth 2 -type f | sort
echo '--- journal ---'
cat resources/drizzle/meta/_journal.json 2>/dev/null || true
echo '--- snapshots ---'
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("resources/drizzle/meta").glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Tests, scripts, and static hygiene ==='
fd -t f -E node_modules -E dist 'package.json|eslint.config.*|biome.json*|tsconfig*.json|vitest.config.*' . |
sort | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,260p" "$0"'
echo '--- feature tests ---'
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
test src 2>/dev/null || true
echo '--- changed-code hygiene ---'
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
xargs -r rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 5451 🏁 Script executed: #!/bin/bash
set -uo pipefail
repo="Sandakan/Nora"
base_oid="838316cb5a83359499422937835a837e5fc86053"
head_oid="5e0fb84f7bb097d996a7507b0ab8833acb8adebd"
echo '=== Local refs and checked-out revision ==='
git rev-parse HEAD 2>&1 || true
git branch -a 2>&1 | sed -n '1,180p'
git status --short 2>&1 || true
echo '=== Fetch exact PR commits without assuming a local base branch ==='
git fetch origin "$base_oid" --quiet 2>&1 || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>&1 || true
git cat-file -e "${base_oid}^{commit}" 2>&1 || true
git cat-file -e "${head_oid}^{commit}" 2>&1 || true
current_head="$(git rev-parse HEAD 2>/dev/null || true)"
printf 'checked_out_head=%s\nexpected_pr_head=%s\n' "$current_head" "$head_oid"
if git cat-file -e "${base_oid}^{commit}" 2>/dev/null && \
git cat-file -e "${head_oid}^{commit}" 2>/dev/null; then
range="${base_oid}..${head_oid}"
echo "=== Exact PR range: $range ==="
git log --oneline --reverse "$range"
git diff --name-status "$range"
git diff --stat "$range"
echo '=== Changed source AST map ==='
git diff --name-only "$range" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
else
echo '=== GitHub changed-file fallback ==='
gh api --paginate "repos/${repo}/pulls/498/files?per_page=100" \
--jq '.[] | [.filename, .status, .additions, .deletions] | `@tsv`' 2>&1 || true
fi
echo '=== GitHub current check details ==='
gh pr checks 498 --repo "$repo" 2>&1 || true
echo '=== Current feature files and direct callers ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx'; do
test -f "$file" || continue
echo "--- $file"
cat -n "$file"
done
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|SmartPlaylistResult|LastFmSource|MAX_LASTFM_MATCH_IDS|MAX_LIMIT)\b' \
src test 2>/dev/null || true
echo '=== Feature tests and lint/type configuration ==='
fd -t f -E node_modules -E dist 'package.json|eslint.config.*|biome.json*|tsconfig*.json|vitest.config.*' . |
sort | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,240p" "$0"'
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
test src 2>/dev/null || trueLength of output: 134171 🏁 Script executed: #!/bin/bash
set -euo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-mAB8t5"
repo="Sandakan/Nora"
echo '=== PR files and current checks ==='
gh api --paginate "repos/${repo}/pulls/498/files?per_page=100" \
--jq '.[] | [.filename, .status, .additions, .deletions] | `@tsv`' 2>&1 || true
gh pr checks 498 --repo "$repo" 2>&1 || true
gh pr view 498 --repo "$repo" \
--json baseRefOid,headRefOid,mergeable,mergeStateStatus,statusCheckRollup 2>&1 || true
echo '=== Audit-log inventory and range markers ==='
rg -n '^===|^--- src/|^--- resources/|^--- test/' "$log" | sed -n '1,500p'
echo '=== Exact changed-file inventory and diff header from log ==='
sed -n '1,280p' "$log"
echo '=== IPC, preload, type, and sync-core sections ==='
for pattern in \
'^--- src/main/ipc.ts$' \
'^--- src/preload/index.ts$' \
'^--- src/types/app.d.ts$' \
'^--- src/main/core/syncLastFmToSmartPlaylist.ts$' \
'^--- src/main/db/queries/validateSmartPlaylistCriteria.ts$' \
'^--- src/main/db/queries/playlist-rules.ts$' \
'^--- src/main/db/queries/playlists.ts$' \
'^--- src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx$' \
'^--- src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx$' \
'^--- src/renderer/src/hooks/useLastFmConsumer.ts$' \
'^--- src/main/other/lastFm/getUserTopTracks.ts$' \
'^--- src/main/other/lastFm/getUserRecentTracks.ts$' \
'^--- src/main/other/lastFm/getUserLovedTracks.ts$'; do
line="$(rg -n -m1 "$pattern" "$log" | cut -d: -f1 || true)"
echo "### $pattern at ${line:-missing}"
if test -n "$line"; then
sed -n "${line},$((line + 460))p" "$log"
fi
done
echo '=== Current source: async error paths, visible controls, and imports ==='
rg -n -C 8 --glob '*.{ts,tsx}' \
'(saveCriteria|setIsSaving|syncToSmartPlaylist|fetchAndMatchTracks|addNewPlaylist|isCreating|lastFmUserData|ipcMain\.handle|saveSmartPlaylistCriteria|refreshSmartPlaylist)' \
src/main/ipc.ts src/preload/index.ts src/renderer/src 2>/dev/null || true
echo '=== Locale keys and CSS/transition usage in changed renderer files ==='
rg -n -C 2 \
'"(limit|limitPlaceholder|lastFmFetchFailed|lastFmNoMatches|lastFmSyncFailed|lastFmSyncSuccess|criteriaSaveFailed)"' \
src/renderer/src/assets/locales 2>/dev/null || true
rg -n --glob '*.{tsx,css}' 'transition-all|focus:outline|aria-|htmlFor|dark:' \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' 2>/dev/null || trueLength of output: 159011 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== Current PR merge and CI status ==='
gh pr view 498 --repo "$repo" \
--json baseRefOid,headRefOid,mergeable,mergeStateStatus,statusCheckRollup 2>&1 || true
gh pr checks 498 --repo "$repo" 2>&1 || true
echo '=== Exact Last.fm IPC handlers ==='
sed -n '360,398p' src/main/ipc.ts | cat -n
echo '=== Exact playlist creation IPC and core path ==='
sed -n '485,501p' src/main/ipc.ts | cat -n
ast-grep outline src/main/core/addNewPlaylist.ts --items all
cat -n src/main/core/addNewPlaylist.ts
rg -n -C 5 '\bcreatePlaylist\s*\(' src/main/db/queries/playlists.ts src/main/core/addNewPlaylist.ts
echo '=== Exact criteria-save UI error path and Button invocation model ==='
sed -n '124,188p;385,405p' src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | cat -n
ast-grep outline src/renderer/src/components/Button.tsx --items all
rg -n -C 8 '(clickHandler|onClick)' src/renderer/src/components/Button.tsx
echo '=== Changed renderer component accessibility and responsive snippets ==='
sed -n '92,152p' src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx | cat -n
sed -n '238,405p' src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | cat -n
echo '=== Verify direct Last.fm endpoint call sites and API key URL construction ==='
rg -n -C 6 '\bgetUser(Top|Recent|Loved)Tracks\s*\(' src test
for file in \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
echo "--- $file"
cat -n "$file"
doneLength of output: 39185
VerdictMerge state: CI state:
The repository does not define an I checked the full PR file set and the direct callers for the smart-playlist, Last.fm, IPC, preload, database, migration, and renderer paths. I applied the required cause, action, reaction, and necessity review to each finding. The listed prior findings are fixed and are not re-reported:
Major findingsNone. Minor findings
|
…dedupe constants, locale + responsive fixes Minor 1: SmartPlaylistCriteriaEditor.saveCriteria now has a catch block that logs the error and shows playlist.criteriaSaveFailed when the IPC invoke rejects (Button does not await clickHandler promise). Minor 2: the three Last.fm user-data IPC handlers now validate username at runtime (typeof === 'string' ? trim : '') so a number/object/array payload no longer throws on .trim(). Minor 3: app/addNewPlaylist validates isSmart at the IPC boundary (typeof === 'boolean' ? value : undefined), rejecting malformed non-boolean payloads. Nitpick 1: ipc.ts reuses VALID_PERIODS and MAX_LIMIT from smartPlaylistConstants instead of local duplicates. Nitpick 2: .tsx uses the playlist.syncFromLastFm locale key without a hardcoded English fallback. Nitpick 3: NewPlaylistPrompt name input uses w-full min-w-0 max-w-[75%] so it shrinks on narrow windows instead of overflowing at min-w-[400px]. typecheck:node 0 errors, lint clean (pre-existing warnings untouched).
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base_ref="origin/release/4.0.0-alpha.5"
echo '=== PR merge state and CI ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
2>&1 || true
gh pr checks 498 --repo "$repo" 2>&1 || true
echo '=== Exact comparison range ==='
git fetch origin release/4.0.0-alpha.5 --quiet 2>&1 || true
base="$(git merge-base HEAD "$base_ref" 2>/dev/null || git merge-base HEAD release/4.0.0-alpha.5)"
head="$(git rev-parse HEAD)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
printf 'behind_target='
git rev-list --count "HEAD..$base_ref" 2>/dev/null || true
printf 'ahead_target='
git rev-list --count "$base_ref..HEAD" 2>/dev/null || true
echo '=== Full changed-file inventory ==='
git log --oneline --reverse "${base}..HEAD"
git diff --name-status "${base}..HEAD"
git diff --stat "${base}..HEAD"
echo '=== AST maps for every changed TypeScript/TSX file ==='
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
echo '=== Current source for all feature and direct integration paths ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$file" || continue
echo "--- $file"
cat -n "$file"
done
echo '=== Definitions, IPC handlers, preload bindings, and consumers ==='
rg -n -C 7 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
src test || true
echo '=== Full non-source diff: migrations, locales, tests, and configuration ==='
git diff --find-renames --unified=25 "${base}..HEAD" -- \
resources/drizzle \
'src/renderer/src/assets/locales/**' \
test \
package.json \
pnpm-lock.yaml \
package-lock.json \
yarn.lock \
| sed -n '1,10000p'
echo '=== Migration journal and snapshot lineage ==='
find resources/drizzle -maxdepth 2 -type f | sort
echo '--- journal ---'
cat resources/drizzle/meta/_journal.json 2>/dev/null || true
echo '--- snapshots ---'
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("resources/drizzle/meta").glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Test, lint, typecheck, and static-hygiene inventory ==='
fd -t f -E node_modules -E dist 'package.json|eslint.config.*|biome.json*|oxlint*|tsconfig*.json|vitest.config.*' . |
sort |
xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,280p" "$0"'
echo '--- feature tests ---'
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
test src 2>/dev/null || true
echo '--- changed-code artifacts ---'
git diff --name-only "${base}..HEAD" -- 'src/**/*.ts' 'src/**/*.tsx' |
xargs -r rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 5847 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base="838316cb5a83359499422937835a837e5fc86053"
head="d24d651249b735b42bd4151bb6475a4a03c7742a"
echo '=== Fetch and verify exact PR commits ==='
git fetch origin "$base" --quiet 2>&1 || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>&1 || true
git cat-file -e "${base}^{commit}"
git cat-file -e "${head}^{commit}" 2>/dev/null || head="$(git rev-parse origin/pr-498)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
echo '=== Full PR inventory ==='
git log --oneline --reverse "${base}..${head}"
git diff --name-status "${base}..${head}"
git diff --stat "${base}..${head}"
echo '=== AST maps for changed TypeScript and TSX files at PR head ==='
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
git cat-file -e "${head}:${file}" 2>/dev/null || continue
echo "--- $file"
git show "${head}:${file}" > "/tmp/$(basename "$file")"
ast-grep outline "/tmp/$(basename "$file")" --items all 2>&1 || true
done
echo '=== Full current source for feature paths ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
git cat-file -e "${head}:${file}" 2>/dev/null || continue
echo "--- $file"
git show "${head}:${file}" | cat -n
done
echo '=== Full changed source diff ==='
git diff --find-renames --unified=25 "${base}..${head}" -- 'src/**' | sed -n '1,14000p'
echo '=== Non-source diff, migrations, locales, and tests ==='
git diff --find-renames --unified=25 "${base}..${head}" -- \
resources/drizzle \
'src/renderer/src/assets/locales/**' \
test \
package.json \
pnpm-lock.yaml \
package-lock.json \
yarn.lock \
| sed -n '1,14000p'
echo '=== Cross-reference every feature symbol at PR head ==='
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
rg -n -C 7 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
"$worktree/src" "$worktree/test" 2>/dev/null || true
echo '=== Migration lineage at PR head ==='
find "$worktree/resources/drizzle" -maxdepth 2 -type f | sort
cat "$worktree/resources/drizzle/meta/_journal.json" 2>/dev/null || true
python3 - "$worktree" <<'PY'
import json, sys
from pathlib import Path
root = Path(sys.argv[1]) / "resources/drizzle/meta"
for path in sorted(root.glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Test and static-hygiene inventory at PR head ==='
find "$worktree" -path '*/node_modules' -prune -o -type f \( -name 'package.json' -o -name 'tsconfig*.json' -o -name 'vitest.config.*' -o -name 'oxlint*' \) -print |
sort |
while IFS= read -r file; do echo "--- ${file#"$worktree"/}"; sed -n '1,280p' "$file"; done
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
"$worktree/test" "$worktree/src" 2>/dev/null || true
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do git show "${head}:${file}"; done |
rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 165924 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base="838316cb5a83359499422937835a837e5fc86053"
head="d24d651249b735b42bd4151bb6475a4a03c7742a"
echo '=== Current merge and CI state ==='
gh pr view 498 --repo "$repo" \
--json mergeable,mergeStateStatus,statusCheckRollup,baseRefOid,headRefOid 2>&1 || true
gh pr checks 498 --repo "$repo" 2>&1 || true
echo '=== Changed-file list, excluding generated snapshots ==='
git diff --name-status "${base}..${head}" |
rg -v 'resources/drizzle/meta/.*snapshot\.json' || true
echo '=== Exact smart-playlist IPC handlers ==='
git show "${head}:src/main/ipc.ts" |
sed -n '350,410p;485,705p' | cat -n
echo '=== Exact smart-playlist core and query behavior ==='
for file in \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlists.ts \
src/main/db/queries/smartPlaylistConstants.ts; do
echo "--- $file"
git show "${head}:${file}" | cat -n
done
echo '=== Exact preload and shared type contracts ==='
for file in src/preload/index.ts src/types/app.d.ts; do
echo "--- $file"
git show "${head}:${file}" |
rg -n -C 8 \
'(lastFmUserData|addNewPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|SmartPlaylistResult|SmartPlaylistCriteria|LastFmSource|IpcChannels)' || true
done
echo '=== Exact renderer UI and lifecycle paths ==='
for file in \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx'; do
echo "--- $file"
git show "${head}:${file}" | cat -n
done
echo '=== Exact Last.fm request helpers ==='
for file in \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
echo "--- $file"
git show "${head}:${file}" | cat -n
done
echo '=== All changed source files: imports, exported declarations, and suspicious paths ==='
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
echo "--- $file"
rg -n \
'^(import|export )|ipcMain\.(handle|on)|useEffect|useCallback|useState|return |throw |catch|finally|fetch\(|JSON\.parse|transaction\(' \
"$worktree/$file" || true
done
echo '=== Focused test inventory and previous-base comparison for candidate code ==='
git diff --name-only "${base}..${head}" -- test
git diff --unified=12 "${base}..${head}" -- src/main/ipc.ts src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts | sed -n '1,5000p'Length of output: 202130
VerdictMerge state: CI state: GitHub reports successful I reviewed the full PR diff at head The listed prior fixes are present. I did not re-report them:
Major findingsNone. Minor findings
|
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base_ref="origin/release/4.0.0-alpha.5"
echo '=== PR merge state and checks ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup 2>&1 || true
gh pr checks 498 --repo "$repo" 2>&1 || true
echo '=== Resolve exact comparison range ==='
git fetch origin release/4.0.0-alpha.5 --quiet 2>&1 || true
base="$(git merge-base HEAD "$base_ref" 2>/dev/null || git merge-base HEAD release/4.0.0-alpha.5)"
head="$(git rev-parse HEAD)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
printf 'behind_target='
git rev-list --count "HEAD..$base_ref" 2>/dev/null || true
printf 'ahead_target='
git rev-list --count "$base_ref..HEAD" 2>/dev/null || true
echo '=== Full PR inventory ==='
git log --oneline --reverse "${base}..${head}"
git diff --name-status "${base}..${head}"
git diff --stat "${base}..${head}"
echo '=== AST outlines of changed TypeScript/TSX files ==='
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$file" || continue
echo "--- $file"
ast-grep outline "$file" --items all 2>&1 || true
done
echo '=== Feature source and full-stack call sites ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$file" || continue
echo "--- $file"
cat -n "$file"
done
echo '=== All relevant definitions and consumers ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|SmartPlaylistResult|LastFmSource)\b' \
src test || true
echo '=== Migrations, locale changes, tests, and configuration ==='
git diff --find-renames --unified=20 "${base}..${head}" -- \
resources/drizzle 'src/renderer/src/assets/locales/**' test \
package.json pnpm-lock.yaml package-lock.json yarn.lock | sed -n '1,12000p'
echo '=== Migration lineage ==='
cat resources/drizzle/meta/_journal.json 2>/dev/null || true
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("resources/drizzle/meta").glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Tests, lint configuration, and changed-code hygiene ==='
fd -t f -E node_modules -E dist 'package.json|oxlint*|eslint.config.*|biome.json*|tsconfig*.json|vitest.config.*' . |
sort | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,260p" "$0"'
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
test src 2>/dev/null || true
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
xargs -r rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 5921 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base="838316cb5a83359499422937835a837e5fc86053"
head="d24d651249b735b42bd4151bb6475a4a03c7742a"
echo '=== Fetch exact commits ==='
git fetch origin "$base" --quiet 2>&1 || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>&1 || true
git cat-file -e "${base}^{commit}"
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
git cat-file -e "${head}^{commit}"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
echo '=== Full PR inventory ==='
git log --oneline --reverse "${base}..${head}"
git diff --name-status "${base}..${head}"
git diff --stat "${base}..${head}"
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
echo '=== Outlines of every changed TypeScript/TSX file ==='
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" || continue
echo "--- $file"
ast-grep outline "$worktree/$file" --items all 2>&1 || true
done
echo '=== Current implementations: source, bridge, renderer, and integrations ==='
for file in \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$worktree/$file" || continue
echo "--- $file"
cat -n "$worktree/$file"
done
echo '=== Definitions and all callers at PR head ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|SmartPlaylistResult|LastFmSource)\b' \
"$worktree/src" "$worktree/test" 2>/dev/null || true
echo '=== Migrations, locales, and tests diff ==='
git diff --find-renames --unified=20 "${base}..${head}" -- \
resources/drizzle 'src/renderer/src/assets/locales/**' test \
package.json pnpm-lock.yaml package-lock.json yarn.lock | sed -n '1,12000p'
echo '=== Migration lineage at PR head ==='
cat "$worktree/resources/drizzle/meta/_journal.json" 2>/dev/null || true
python3 - "$worktree" <<'PY'
import json, sys
from pathlib import Path
root = Path(sys.argv[1]) / "resources/drizzle/meta"
for path in sorted(root.glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Test, lint configuration, and hygiene ==='
fd -t f -E node_modules -E dist 'package.json|oxlint*|eslint.config.*|biome.json*|tsconfig*.json|vitest.config.*' "$worktree" |
sort | xargs -r -n1 sh -c 'echo "--- ${0#*/}"; sed -n "1,260p" "$0"'
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
"$worktree/test" "$worktree/src" 2>/dev/null || true
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" && cat "$worktree/$file"
done | rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 88024 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="838316cb5a83359499422937835a837e5fc86053"
head="d24d651249b735b42bd4151bb6475a4a03c7742a"
for ref in "$base" "$head"; do
git cat-file -e "${ref}^{commit}" 2>/dev/null || true
done
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
show_range() {
local file="$1"
local ranges="$2"
echo "--- $file"
git show "${head}:${file}" | sed -n "$ranges" | cat -n
}
echo '=== Exact IPC handlers, lock helper, imports, and playlist creation ==='
show_range src/main/ipc.ts '1,160p;340,420p;470,710p'
echo '=== Exact preload bindings and shared API types ==='
show_range src/preload/index.ts '410,535p'
show_range src/types/app.d.ts '1,115p;700,800p'
echo '=== Exact validator, constants, rule engine, and Last.fm sync core ==='
show_range src/main/db/queries/smartPlaylistConstants.ts '1,120p'
show_range src/main/db/queries/validateSmartPlaylistCriteria.ts '1,230p'
show_range src/main/db/queries/playlist-rules.ts '1,245p'
show_range src/main/core/syncLastFmToSmartPlaylist.ts '1,190p'
show_range src/main/db/queries/playlists.ts '45,120p;235,290p'
echo '=== Exact renderer mutation, editor, and lifecycle paths ==='
show_range src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx '1,175p'
show_range src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx '1,450p'
show_range src/renderer/src/hooks/useLastFmConsumer.ts '1,235p'
show_range 'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' '100,340p'
echo '=== Last.fm request implementation ==='
show_range src/main/other/lastFm/getUserTopTracks.ts '1,160p'
show_range src/main/other/lastFm/getUserRecentTracks.ts '1,180p'
show_range src/main/other/lastFm/getUserLovedTracks.ts '1,160p'
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
echo '=== Exact callers of exposed smart and Last.fm APIs ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'(saveSmartPlaylistCriteria|refreshSmartPlaylist|syncLastFmToSmartPlaylist|getUserTopTracks|getUserRecentTracks|getUserLovedTracks|addNewPlaylist)\s*\(' \
"$worktree/src" "$worktree/test" || true
echo '=== Diff for prior candidate areas ==='
git diff --unified=15 "${base}..${head}" -- \
src/main/ipc.ts \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/preload/index.ts \
src/types/app.d.ts | sed -n '1,10000p'
echo '=== Relevant tests beyond validation ==='
find "$worktree/test" -type f -name '*.test.ts' -print | sort
rg -n -C 3 --glob '*.test.ts' \
'(evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|SmartPlaylistCriteriaEditor|useLastFmConsumer|ipc)' \
"$worktree/test" || trueLength of output: 180983
VerdictMerge state: CI state:
I reviewed the full PR range from base The prior findings are resolved and are not re-reported:
Major findingsNone. Minor findings
|
…ue on field change
CR Minor 1 (invalid songIds logging can throw):
- syncLastFmToSmartPlaylist IPC error log now uses Array.isArray(songIds) ? songIds.length
: undefined, so a malformed (null/object) payload no longer throws while building the
log object. The documented { success: false, count: 0 } result is still returned.
CR Minor 2 (field change leaves invisible invalid rule value):
- The criteria rule field-change handler now resets rule.value to a type-appropriate
default (true for boolean fields isFavorite/isBlacklisted, '' otherwise) so a stale
value from the previous field is not silently dropped by saveCriteria's empty-filter.
typecheck:node clean, lint clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== PR merge state and CI ==='
gh pr view 498 --repo "$repo" \
--json number,mergeable,mergeStateStatus,statusCheckRollup,baseRefOid,headRefOid 2>&1 || true
gh pr checks 498 --repo "$repo" 2>&1 || true
echo '=== Verify prior minor findings ==='
echo '--- songIds array guard in ipc.ts ---'
rg -n -B5 -A5 'songIds\.length' src/main/ipc.ts || true
echo '--- boolean field reset in SmartPlaylistCriteriaEditor.tsx ---'
rg -n -B10 -A25 'handleFieldChange|onChange.*field|updateRule' src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -150
echo '--- 20-rule add limit in editor ---'
rg -n -B5 -A10 'Add rule|addRule|rules.length' src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx | head -80
echo '=== Full current diff stat vs release base ==='
base="$(git merge-base HEAD origin/release/4.0.0-alpha.5 2>/dev/null || git merge-base HEAD release/4.0.0-alpha.5 2>/dev/null)"
echo "BASE=$base HEAD=$(git rev-parse HEAD)"
git diff --stat "${base}..HEAD" | tail -5Length of output: 12115 VerdictMERGEABLE state: Base commit: I verified the two minor findings from the prior review pass. Confirmed fixes
|
|
@CodeRabbit Re-reviewing after the latest push. Summary of current state for this PR:
Required verdict formatEnd with Self-review before posting (MANDATORY)For every finding: (1) Cause - in THIS diff or pre-existing on base? (2) Action - compiles/matches types? (3) Reaction - what breaks? (4) Necessity - real bug or style? Only post if all four hold. If a prior-round finding is already fixed in the current diff, state it explicitly instead of re-reporting. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== Pull request metadata, merge state, and CI checks ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup
gh pr checks 498 --repo "$repo" || true
echo '=== Resolve exact PR base and head ==='
base_oid="$(gh pr view 498 --repo "$repo" --json baseRefOid --jq '.baseRefOid')"
head_oid="$(gh pr view 498 --repo "$repo" --json headRefOid --jq '.headRefOid')"
git fetch origin "$base_oid" --quiet 2>/dev/null || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>/dev/null || true
git cat-file -e "${base_oid}^{commit}"
if ! git cat-file -e "${head_oid}^{commit}" 2>/dev/null; then
head_oid="$(git rev-parse origin/pr-498)"
fi
git cat-file -e "${head_oid}^{commit}"
printf 'BASE=%s\nHEAD=%s\n' "$base_oid" "$head_oid"
printf 'behind_target='
git rev-list --count "${head_oid}..${base_oid}" || true
printf 'ahead_target='
git rev-list --count "${base_oid}..${head_oid}" || true
echo '=== Full PR inventory ==='
git log --oneline --reverse "${base_oid}..${head_oid}"
git diff --name-status "${base_oid}..${head_oid}"
git diff --stat "${base_oid}..${head_oid}"
worktree="$(mktemp -d)"
git archive "$head_oid" | tar -x -C "$worktree"
echo '=== AST outlines for all changed TypeScript and TSX files ==='
git diff --name-only "${base_oid}..${head_oid}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" || continue
echo "--- $file"
ast-grep outline "$worktree/$file" --items all || true
done
echo '=== Current feature files and direct integration paths ==='
for file in \
.github/workflows/lint.yml \
package.json \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$worktree/$file" || continue
echo "--- $file"
cat -n "$worktree/$file"
done
echo '=== Definitions and all relevant callers at the PR head ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
"$worktree/src" "$worktree/test" || true
echo '=== Migrations, locales, tests, and workflow diff ==='
git diff --find-renames --unified=20 "${base_oid}..${head_oid}" -- \
.github/workflows \
resources/drizzle \
'src/renderer/src/assets/locales/**' \
test \
package.json \
| sed -n '1,14000p'
echo '=== Migration lineage ==='
cat "$worktree/resources/drizzle/meta/_journal.json" 2>/dev/null || true
python3 - "$worktree" <<'PY'
import json, sys
from pathlib import Path
root = Path(sys.argv[1]) / "resources/drizzle/meta"
for path in sorted(root.glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Test inventory and static hygiene ==='
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
"$worktree/test" "$worktree/src" || true
git diff --name-only "${base_oid}..${head_oid}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" && cat "$worktree/$file"
done | rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 87041 🏁 Script executed: #!/bin/bash
set -euo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-uACyK7"
echo '=== Audit log headings and exact PR metadata ==='
rg -n '^=== Pull request|^=== Resolve|^=== Full PR|^=== Current feature|^=== Definitions|^=== Migrations|^=== Test inventory|^\{' "$log" | sed -n '1,260p'
sed -n '1,320p' "$log"
echo '=== Extract key current-file sections from audit log ==='
for file in \
.github/workflows/lint.yml \
package.json \
src/main/ipc.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx; do
echo "--- $file"
line="$(rg -n -m1 -- "--- $file$" "$log" | cut -d: -f1 || true)"
if test -n "$line"; then
sed -n "${line},$((line + 520))p" "$log"
else
echo "NOT FOUND"
fi
done
echo '=== Current repository evidence: CI workflow and focused paths ==='
echo '--- .github/workflows/lint.yml ---'
cat -n .github/workflows/lint.yml
echo '--- package scripts ---'
rg -n -A35 -B3 '"scripts"' package.json
echo '--- smart-playlist handler locations ---'
rg -n -C 8 \
"'app/(lastfm/getUserTopTracks|lastfm/getUserRecentTracks|lastfm/getUserLovedTracks|addNewPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist)'" \
src/main/ipc.ts
echo '--- editor mutation and rule-limit locations ---'
rg -n -C 8 \
'(saveCriteria|addRule|handleFieldChange|setIsSaving|rules\.length|disabled=)' \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx
echo '--- all focused feature tests ---'
fd -t f -E node_modules -E dist . test src | rg '(\.test\.|\.spec\.|__tests__)' | sortLength of output: 126830 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== Current PR identity and checks ==='
gh pr view 498 --repo "$repo" \
--json baseRefOid,headRefOid,mergeable,mergeStateStatus,statusCheckRollup
gh pr checks 498 --repo "$repo" || true
base="$(gh pr view 498 --repo "$repo" --json baseRefOid --jq '.baseRefOid')"
head="$(gh pr view 498 --repo "$repo" --json headRefOid --jq '.headRefOid')"
git fetch origin "$base" --quiet 2>/dev/null || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>/dev/null || true
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
echo '=== CI workflow files at the exact PR head ==='
git ls-tree -r --name-only "$head" .github/workflows
for file in $(git ls-tree -r --name-only "$head" .github/workflows); do
echo "--- $file"
git show "${head}:${file}" | cat -n
done
echo '=== Result-limit validator and editor controls at the exact PR head ==='
echo '--- validator ---'
git show "${head}:src/main/db/queries/validateSmartPlaylistCriteria.ts" |
nl -ba | sed -n '1,190p'
echo '--- criteria editor relevant ranges ---'
git show "${head}:src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx" |
nl -ba | sed -n '105,215p;300,425p'
echo '=== PR diff for the candidate files ==='
git diff --unified=20 "${base}..${head}" -- \
.github/workflows \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/main/db/queries/validateSmartPlaylistCriteria.tsLength of output: 11784
VerdictMerge state: Current GitHub CI state:
The current head is I rechecked the prior smart-playlist, Last.fm, IPC, transaction, migration, and UI findings. The earlier fixes remain present. This includes strict criteria and Last.fm source validation, safe-integer validation, bounded sync IDs, shared constants, lock cleanup, transaction-scoped refresh, atomic Last.fm sync, and corrected editor field transitions. Major findingsNone. Minor findings
|
CR requires a TypeScript validation command in CI. Adds a dedicated Typecheck (node) job running 'npm run typecheck:node' so type safety is verified on every PR run. typecheck:web is intentionally excluded because it carries 7 pre-existing errors unrelated to this PR.
|
@CodeRabbit Final re-review after the latest push. All prior code findings are resolved and verified locally ( Please re-run the full audit against the current head with the CI typecheck result available and confirm the branch is Ready, or list any remaining Major/Minor finding with file:line evidence. Required verdict formatEnd with Self-review before posting (MANDATORY)For every finding: (1) Cause - in THIS diff or pre-existing on base? (2) Action - compiles/matches types? (3) Reaction - what breaks? (4) Necessity - real bug or style? Only post if all four hold. If a prior-round finding is already fixed in the current diff, state it explicitly instead of re-reporting. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== PR metadata and current checks ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup
gh pr checks 498 --repo "$repo" || true
base="$(gh pr view 498 --repo "$repo" --json baseRefOid --jq '.baseRefOid')"
head="$(gh pr view 498 --repo "$repo" --json headRefOid --jq '.headRefOid')"
git fetch origin "$base" --quiet 2>/dev/null || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>/dev/null || true
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
git cat-file -e "${base}^{commit}"
git cat-file -e "${head}^{commit}"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
printf 'behind_target='
git rev-list --count "${head}..${base}" || true
printf 'ahead_target='
git rev-list --count "${base}..${head}" || true
echo '=== Full PR inventory ==='
git log --oneline --reverse "${base}..${head}"
git diff --name-status "${base}..${head}"
git diff --stat "${base}..${head}"
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
echo '=== Changed source outlines ==='
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" || continue
echo "--- $file"
ast-grep outline "$worktree/$file" --items all || true
done
echo '=== Current changed feature paths ==='
for file in \
.github/workflows/lint.yml \
package.json \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$worktree/$file" || continue
echo "--- $file"
cat -n "$worktree/$file"
done
echo '=== Relevant definitions and callers ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
"$worktree/src" "$worktree/test" || true
echo '=== Workflows, migrations, locales, and tests diff ==='
git diff --find-renames --unified=20 "${base}..${head}" -- \
.github/workflows \
resources/drizzle \
'src/renderer/src/assets/locales/**' \
test \
package.json | sed -n '1,14000p'
echo '=== Migration lineage ==='
cat "$worktree/resources/drizzle/meta/_journal.json" 2>/dev/null || true
python3 - "$worktree" <<'PY'
import json, sys
from pathlib import Path
root = Path(sys.argv[1]) / "resources/drizzle/meta"
for path in sorted(root.glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Feature test inventory and changed-code hygiene ==='
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria)' \
"$worktree/test" "$worktree/src" || true
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" && cat "$worktree/$file"
done | rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 86827 🏁 Script executed: #!/bin/bash
set -euo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-M3l0wv"
repo="Sandakan/Nora"
echo '=== Current PR mergeability and CI ==='
gh pr view 498 --repo "$repo" \
--json baseRefOid,headRefOid,mergeable,mergeStateStatus,statusCheckRollup
gh pr checks 498 --repo "$repo" || true
echo '=== Head identifiers and source workflow ==='
git rev-parse HEAD
cat -n .github/workflows/lint.yml
rg -n -A30 -B3 '"scripts"' package.json
echo '=== Exact latest-fix locations ==='
echo '--- 20-rule editor bound ---'
rg -n -C 12 '(addRule|rules\.length|Add rule|disabled=)' \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx
echo '--- save lifecycle and field-change behavior ---'
rg -n -C 10 '(saveCriteria|setIsSaving|handleFieldChange|defaultValue|updateRule)' \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx
echo '--- smart-playlist IPC boundaries and lock lifecycle ---'
rg -n -C 12 \
"'app/(lastfm/getUserTopTracks|lastfm/getUserRecentTracks|lastfm/getUserLovedTracks|addNewPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist)'|withSmartPlaylistLock|smartPlaylistLocks" \
src/main/ipc.ts
echo '--- atomic sync and rule refresh ---'
cat -n src/main/core/syncLastFmToSmartPlaylist.ts
cat -n src/main/db/queries/playlist-rules.ts
echo '=== Current bridge contracts and consumers ==='
rg -n -C 6 \
'(lastFmUserData|addNewPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|SmartPlaylistResult)' \
src/preload/index.ts src/types/app.d.ts \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx'
echo '=== New-feature diagnostics only ==='
rg -n -C 3 --glob '*.{ts,tsx}' \
'console\.(error|log)\s*\(|debugger\b|TODO|FIXME|transition-all' \
src/main/ipc.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/db/queries/playlist-rules.ts \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx || true
echo '=== Relevant test inventory ==='
fd -t f -E node_modules -E dist . test src | rg '(\.test\.|\.spec\.|__tests__)' | sortLength of output: 56644
VerdictMerge state: CI state: All reported checks pass at head
I verified that Major findingsNone. Minor findings
|
…idator limit The validator rejects criteria with more than 20 rules (MAX_RULES), but the editor let users add unlimited rules and then receive a generic save failure. - Export MAX_RULES from validateSmartPlaylistCriteria and reuse it in the editor. - addRule() now returns the previous state unchanged once rules.length >= MAX_RULES. - The Add rule button is disabled at the limit, matching the validator. oxlint clean, npm run typecheck:node clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Required verdict formatEnd your review with a clear line: Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. Do not accept generic patterns — validate each finding against the actual code. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. Note: this push adds the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== PR metadata, mergeability, and checks ==='
gh pr view 498 --repo "$repo" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup
gh pr checks 498 --repo "$repo" || true
base="$(gh pr view 498 --repo "$repo" --json baseRefOid --jq '.baseRefOid')"
head="$(gh pr view 498 --repo "$repo" --json headRefOid --jq '.headRefOid')"
git fetch origin "$base" --quiet 2>/dev/null || true
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>/dev/null || true
git cat-file -e "${base}^{commit}"
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
git cat-file -e "${head}^{commit}"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
printf 'behind_declared_base='
git rev-list --count "${head}..${base}"
printf 'ahead_declared_base='
git rev-list --count "${base}..${head}"
echo '=== Full PR changed-file inventory ==='
git log --oneline --reverse "${base}..${head}"
git diff --name-status "${base}..${head}"
git diff --stat "${base}..${head}"
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
echo '=== Changed TypeScript and TSX AST outlines ==='
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" || continue
echo "--- $file"
ast-grep outline "$worktree/$file" --items all || true
done
echo '=== Current feature implementations and direct integration files ==='
for file in \
.github/workflows/lint.yml \
package.json \
src/main/db/queries/smartPlaylistConstants.ts \
src/main/db/queries/validateSmartPlaylistCriteria.ts \
src/main/db/queries/playlist-rules.ts \
src/main/db/queries/playlists.ts \
src/main/core/addNewPlaylist.ts \
src/main/core/addSongsToPlaylist.ts \
src/main/core/removeSongFromPlaylist.ts \
src/main/core/syncLastFmToSmartPlaylist.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx \
src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx \
src/renderer/src/hooks/useLastFmConsumer.ts \
'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' \
src/main/other/lastFm/getUserTopTracks.ts \
src/main/other/lastFm/getUserRecentTracks.ts \
src/main/other/lastFm/getUserLovedTracks.ts; do
test -f "$worktree/$file" || continue
echo "--- $file"
cat -n "$worktree/$file"
done
echo '=== Symbols and all callers/consumers at PR head ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(validateSmartPlaylistCriteria|validateLastFmSource|VALID_PERIODS|MAX_LIMIT|MAX_LASTFM_MATCH_IDS|evaluateSmartPlaylist|refreshSmartPlaylist|syncLastFmToSmartPlaylist|saveSmartPlaylistCriteria|smartPlaylistLocks|withSmartPlaylistLock|lastFmUserData|useLastFmConsumer|addNewPlaylist|addSongsToPlaylist|createPlaylist|SmartPlaylistResult|LastFmSource)\b' \
"$worktree/src" "$worktree/test" || true
echo '=== Changed workflows, migrations, locales, tests, and package scripts ==='
git diff --find-renames --unified=20 "${base}..${head}" -- \
.github/workflows resources/drizzle 'src/renderer/src/assets/locales/**' test package.json |
sed -n '1,16000p'
echo '=== Migration lineage ==='
cat "$worktree/resources/drizzle/meta/_journal.json" 2>/dev/null || true
python3 - "$worktree" <<'PY'
import json, sys
from pathlib import Path
root = Path(sys.argv[1]) / "resources/drizzle/meta"
for path in sorted(root.glob("*_snapshot.json")):
data = json.loads(path.read_text())
print(f"{path.name}: id={data.get('id')} prevId={data.get('prevId')}")
PY
echo '=== Feature-test inventory and changed-code hygiene ==='
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(SmartPlaylist|playlist-rules|LastFm|syncLastFmToSmartPlaylist|validateSmartPlaylistCriteria|addSongsToPlaylist|scan)' \
"$worktree/test" "$worktree/src" || true
git diff --name-only "${base}..${head}" -- 'src/**/*.ts' 'src/**/*.tsx' |
while IFS= read -r file; do
test -f "$worktree/$file" && cat "$worktree/$file"
done | rg -n 'console\.(log|error)|debugger\b|TODO|FIXME|`@ts-ignore`|as any|transition-all' || trueLength of output: 86954 🏁 Script executed: #!/bin/bash
set -euo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-fwscxN"
echo '=== Audit-log map ==='
rg -n '^===|^--- ' "$log" | sed -n '1,650p'
echo '=== PR metadata, base/head, and CI output ==='
sed -n '1,260p' "$log"
echo '=== Changed-file inventory and commit range ==='
start="$(rg -n -m1 '^=== Full PR changed-file inventory ===' "$log" | cut -d: -f1)"
end="$(rg -n -m1 '^=== Changed TypeScript and TSX AST outlines ===' "$log" | cut -d: -f1)"
sed -n "${start},$((end - 1))p" "$log"
echo '=== All changed files not in the initial feature-file list ==='
rg -n '^--- (src|test|\.github|resources)/' "$log" |
rg -i '(discord|scan|symlink|folder|addsongs|playlist|ipc|preload|workflow|test)' |
sed -n '1,500p'
echo '=== Current workflow and package scripts ==='
for pattern in '^--- \.github/workflows/lint\.yml$' '^--- package\.json$'; do
line="$(rg -n -m1 "$pattern" "$log" | cut -d: -f1 || true)"
echo "### $pattern"
test -n "$line" && sed -n "${line},$((line + 300))p" "$log"
done
echo '=== Current source around the recent-fix areas ==='
for keyword in \
'discord' \
'scan' \
'symlink' \
'addSongsToPlaylist' \
'addSongsToPlaylistsPrompt' \
'bulk'; do
echo "### Keyword: $keyword"
rg -n -i -C 8 "$keyword" "$log" | sed -n '1,700p' || true
done
echo '=== Test files added or changed in the PR ==='
rg -n -A3 -B3 '^diff --git a/test/' "$log" | sed -n '1,2000p'
echo '=== Source code lines from the exact current head for selected new fixes ==='
for candidate in \
src/main/core/addSongsToPlaylist.ts \
src/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx \
src/main/core/scanMusicFolders.ts \
src/main/core/scanMusicFolder.ts \
src/main/other/discord/index.ts \
src/main/ipc.ts \
.github/workflows/lint.yml; do
line="$(rg -n -m1 -- "^--- $candidate$" "$log" | cut -d: -f1 || true)"
echo "### $candidate"
test -n "$line" && sed -n "${line},$((line + 420))p" "$log"
doneLength of output: 192024 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
head="$(gh pr view 498 --repo "$repo" --json headRefOid --jq '.headRefOid')"
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>/dev/null || true
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
show() {
local file="$1"
local range="$2"
echo "--- $file"
git show "${head}:${file}" | nl -ba | sed -n "$range"
}
echo "HEAD=$head"
echo '=== Core bulk add/remove and renderer consumer ==='
show src/main/core/addSongsToPlaylist.ts '1,150p'
show src/main/core/removeSongFromPlaylist.ts '1,130p'
show src/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx '70,190p'
echo '=== Smart-playlist IPC boundaries and mutation lock ==='
show src/main/ipc.ts '120,150p;360,410p;475,680p'
show src/preload/index.ts '425,530p'
show src/types/app.d.ts '15,90p;720,780p'
echo '=== Criteria editor state and UI controls ==='
show src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx '20,220p;300,440p'
echo '=== Rule evaluation and atomic synchronization ==='
show src/main/db/queries/playlist-rules.ts '1,245p'
show src/main/core/syncLastFmToSmartPlaylist.ts '1,180p'
show src/main/db/queries/validateSmartPlaylistCriteria.ts '1,210p'
echo '=== Last.fm lifecycle and playlist page consumers ==='
show src/renderer/src/hooks/useLastFmConsumer.ts '1,230p'
show 'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' '120,330p'
show src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx '20,170p'
echo '=== Full call-site contexts for changed IPC API symbols ==='
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
rg -n -C 5 --glob '*.{ts,tsx}' \
'\.(addSongsToPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|syncLastFmToSmartPlaylist|getUserTopTracks|getUserRecentTracks|getUserLovedTracks|addNewPlaylist)\s*\(' \
"$worktree/src" "$worktree/test" || true
echo '=== Changed test files and explicit test names ==='
git diff --name-only "$(gh pr view 498 --repo "$repo" --json baseRefOid --jq '.baseRefOid')".."$head" -- test |
while IFS= read -r file; do
echo "--- $file"
git show "${head}:${file}" | rg -n '^\s*(test|it|describe)\(' || true
doneLength of output: 2719 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
head="$(gh pr view 498 --repo "$repo" --json headRefOid --jq '.headRefOid')"
git fetch origin "pull/498/head:refs/remotes/origin/pr-498" --quiet 2>/dev/null || true
if ! git cat-file -e "${head}^{commit}" 2>/dev/null; then
head="$(git rev-parse origin/pr-498)"
fi
show() {
local file="$1"
local range="$2"
echo "--- $file"
git show "${head}:${file}" | cat -n | sed -n "$range"
}
echo "HEAD=$head"
echo '=== Core bulk add/remove and renderer consumer ==='
show src/main/core/addSongsToPlaylist.ts '1,150p'
show src/main/core/removeSongFromPlaylist.ts '1,130p'
show src/renderer/src/components/SongsPage/AddSongsToPlaylistsPrompt.tsx '70,190p'
echo '=== Smart-playlist IPC boundaries and mutation lock ==='
show src/main/ipc.ts '120,150p;360,410p;475,680p'
show src/preload/index.ts '425,530p'
show src/types/app.d.ts '15,90p;720,780p'
echo '=== Criteria editor state and UI controls ==='
show src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx '20,220p;300,440p'
echo '=== Rule evaluation and atomic synchronization ==='
show src/main/db/queries/playlist-rules.ts '1,245p'
show src/main/core/syncLastFmToSmartPlaylist.ts '1,180p'
show src/main/db/queries/validateSmartPlaylistCriteria.ts '1,210p'
echo '=== Last.fm lifecycle and playlist page consumers ==='
show src/renderer/src/hooks/useLastFmConsumer.ts '1,230p'
show 'src/renderer/src/routes/main-player/playlists/$playlistId.tsx' '120,330p'
show src/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx '20,170p'
echo '=== Full call-site contexts for changed IPC API symbols ==='
worktree="$(mktemp -d)"
git archive "$head" | tar -x -C "$worktree"
rg -n -C 5 --glob '*.{ts,tsx}' \
'\.(addSongsToPlaylist|saveSmartPlaylistCriteria|refreshSmartPlaylist|syncLastFmToSmartPlaylist|getUserTopTracks|getUserRecentTracks|getUserLovedTracks|addNewPlaylist)\s*\(' \
"$worktree/src" "$worktree/test" || true
echo '=== Changed test files and explicit test names ==='
git diff --name-only "$(gh pr view 498 --repo "$repo" --json baseRefOid --jq '.baseRefOid')".."$head" -- test |
while IFS= read -r file; do
echo "--- $file"
git show "${head}:${file}" | rg -n '^\s*(test|it|describe)\(' || true
doneLength of output: 101238
VerdictMerge state: Comparison range: Base CI state: Passed.
The workflow runs Major findingsNone. Minor findingsNone. I completed a second pass on each candidate issue before this result. I checked cause, fix safety, caller effects, and necessity. I did not retain style-only findings. Missing test coverageThe PR adds focused validation coverage in The current PR range does not add focused tests for these behavior paths:
These are test coverage gaps. I found no current code defect that requires a Major or Minor finding. What is working correctly
The current PR diff does not contain Discord, scan, or symlink implementation files. I therefore did not treat those claimed fixes as evidence within this PR audit. Verdict: Ready — medium confidenceYou are interacting with an AI system. |
…x/461-smart-playlists-lastfm
1 participant
Summary
Adds smart playlist support (rule-based auto-populated playlists) and Last.fm integration: fetch user top/recent/loved tracks and sync matching local songs into a smart playlist.
Motivation
Issue #461 requests automatic playlists based on listening habits via Last.fm, including late-night, workout/high-energy, recently played, and frequently played tracks. This PR delivers the rule-based playlist engine, the creation flow, the criteria editor, and the Last.fm fetch + sync pipeline.
Changes by file
Schema and Migrations
resources/drizzle/0004_mixed_adam_destine.sql— addsis_smart,criteriacolumns to playlists table with index onis_smartresources/drizzle/0005_talented_master_chief.sql— scrobble_queue migration, renumbered into the journal chainsrc/main/db/schema.ts— playlist schema additionsCore Smart Playlist Engine
src/main/db/queries/playlist-rules.ts— rule compiler for 11 fields (year, duration, bitRate, isFavorite, isBlacklisted, genre, artist, album, playCount, skipCount, lastPlayed) with operator support per field;evaluateSmartPlaylist()builds SQL with AND/OR matching inside the transaction;refreshSmartPlaylist()atomically replaces membership; case-insensitive ILIKE with wildcard escaping; correct NULL handling for nullable columns and untagged songs; zero-condition safety returns empty setsrc/main/db/queries/validateSmartPlaylistCriteria.ts— shared main-process validator for per-rule field/operator/value/limit checks, used by both save and refreshsrc/main/db/queries/playlists.ts—updatePlaylistCriteria()for persisting criteria JSONSmart Playlist Creation
src/main/core/addNewPlaylist.ts— acceptsisSmartflag, creation flow through IPCsrc/renderer/src/components/PlaylistsPage/NewPlaylistPrompt.tsx— smart playlist toggle, trim validation, pending state, duplicate-submit guardsrc/renderer/src/components/SmartPlaylistCriteriaEditor.tsx— visual rule editor with field/operator selection, accessible controls, stable rule IDs, empty numeric input preserved, Last.fm source cleared on rule saveLast.fm Integration
src/main/other/lastFm/getUserTopTracks.ts,getUserRecentTracks.ts,getUserLovedTracks.ts— fetch APIs with HTTPS, URLSearchParams, 10s timeout, HTTP/API error checkssrc/main/core/syncLastFmToSmartPlaylist.ts— atomic membership replacement + Last.fm source persistence in one transaction, input validation at IPC boundarysrc/renderer/src/hooks/useLastFmConsumer.ts— fetch-and-match pipeline with per-request abort, pending state, i18n notificationssrc/renderer/src/routes/main-player/playlists/$playlistId.tsx— Sync from Last.fm action, disabled while syncingIPC and Preload
src/main/ipc.ts— save/refresh/sync handlers with per-playlist lock serialization, discriminated results,.finally()lock cleanup, input validationsrc/preload/index.ts— typed preload bindings matching the discriminated result contractsSafety
src/main/core/addSongsToPlaylist.ts/removeSongFromPlaylist.ts— smart playlist mutation guards at main-process boundary with renderer notification, discriminated return valuesCANNOT_MODIFY_SMART_PLAYLISTwired into notification config with locale stringTests
test/src/main/db/queries/validateSmartPlaylistCriteria.test.ts— 23 tests covering valid/invalid rules, null entries, field/operator/value validation, limitsTest Plan
%,_, and\wildcardsSummary by CodeRabbit
New Features
Bug Fixes
Fixes #461