check#6
Conversation
- Rewrote manageWindowOnDisplayMetricsChange to check window position against ALL connected displays instead of only the primary - Removed stale primaryDisplay closure capture — function now gets fresh display data on each call - Added manageWindowOnDisplayMetricsChange call after position restoration in changePlayerType for both mini and normal modes - Removed unused Display type import This prevents the window from getting stuck off-screen when a monitor is disconnected with the window on it.
…ty (Sandakan#376, Sandakan#436) Bug fixes: - Omit timestamps key when paused so Discord stops counting elapsed time - Replace queue with trailing debounce (1s) — first event fires instantly - Fix sendAudioData.ts RPC payload — was using v1 key names (startTimestamp, endTimestamp, largeImageKey, smallImageKey) silently ignored by SET_ACTIVITY - Use artist artwork (picture_xl → picture_medium → picture_small) for large_image with nora_logo fallback — previously hardcoded in both sendAudioData.ts and hook Crash safety: - Wrap discord.request('SET_ACTIVITY') in .catch() — unhandled rejection could crash Electron main process on RPC disconnect mid-call - await addSongToPlayHistory() — unhandled promise was escaping try-catch - Guard song.artworks?.map() — could throw on undefined - Guard player.duration against NaN/0 in timestamp math Code quality: - Clone data before mutating (instance/type) — no more side effects on caller's objects - Type discord as DiscordRPCClient | null instead of any - Type setDiscordRpcActivity data as Record<string, unknown> instead of any - clearDiscordRpcActivity now flushes debounce state (clears timer + latestData) - Fix debounce race — set debounceTimer = null before reading latestData to prevent data orphaning during callback execution - Guard empty onlineArtworkPaths (truthy {} without picture_small key) - Guard missing preload bridge (window.api?.playerControls?) - Guard getUserSettings() returning null/undefined - Remove dead commented-out code (getRelevantArtistData) - Log in empty catch blocks instead of silent swallow
…most relevant result play button on most-relevant-result was missing bg-transparent!, inheriting base Button's bg-background-color-2/25. combined with rounded-none! this created a visible dark semi-transparent square behind the play_circle icon on album art overlays.
Completes the French translation started in PR Sandakan#410 by guguss-31. Corrects existing translations: musique->chanson for song context, desacivee->desactivee typo, missing variables and () cross-refs, plural adjective agreement for selected*Count keys.
…AlwaysOnTop and toggleAutoLaunch
… validate preloaded song id on skip
feat(Sandakan#364): add Brazilian and Portuguese (pt-BR) translation
…he binary installation.
…automation pipelines
…nt a linting GitHub workflow
complete(Sandakan#410): add complete French translation
fix: discord rpc issues(Sandakan#376, Sandakan#436)
…r-clip fix(Sandakan#480): fullscreen timer bar clips to end when minimized
🧩 Analysis chain🏁 Script executed: #!/bin/bash
curl -s "https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true" | head -c 5000
echo
echo "---status---"
curl -s -o /dev/null -w "%{http_code}\n" "https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true"Length of output: 5525 🏁 Script executed: #!/bin/bash
curl -s "https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true" \
| python3 -c "
import json,sys
data = json.load(sys.stdin)
for i in data['issues']:
comp = i['component'].split(':',1)[-1]
line = i.get('line','-')
print(f\"{i['severity']:8} | {comp}:{line} | {i['rule']} | {i['message']}\")
"Length of output: 3674
Critical
Major
Minor
I'll factor all of these into the ongoing review of the diff (flagging the ones that fall within changed lines, and noting the pre-existing ones for awareness). The Cognitive Complexity issue in Kicking off a fresh review now to incorporate these findings. ✏️ Learnings added
✅ Action performedReview finished.
|
…ing on import - Renumber migration 0006 -> 0005 and fix journal idx to be sequential (0-5). No existing migrations deleted; append-only preserved. - parseSong now calls upsertSongLyrics(songId, path) on import instead of the embedded-only variant, so both embedded and .lrc lyrics are indexed for newly added songs. Prevents stale lyric search for LRC-only tracks.
|
@CodeRabbit hwo long do i have to wait? |
|
Tip For best results, initiate chat on the files or code changes.
(=^･ω･^=) ﾉ hang tight~ |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/search.ts (1)
30-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the "no results" condition to include
lyrics.length.The
availableResultsfallback at lines 80–87 checkssongs/artists/albums/playlists/genresbut notlyrics. When only lyrics results exist, the fallback triggers unnecessarysearchForAvailableResultscalls (extra I/O) and may surface "did you mean..." suggestions despite having results. Addlyrics.length === 0to the condition.🐛 Proposed fix
if ( songs.length === 0 && artists.length === 0 && albums.length === 0 && playlists.length === 0 && - genres.length === 0 + genres.length === 0 && + lyrics.length === 0 ) {🤖 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/search.ts` around lines 30 - 46, The no-results fallback in search.ts currently ignores lyrics, so it can still run searchForAvailableResults even when searchSongsByLyrics returns matches. Update the availableResults check in the search flow to include lyrics.length alongside songs, artists, albums, playlists, and genres, and verify the logic in the search function uses the lyrics array from Promise.all consistently before triggering any “did you mean” suggestions.src/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsx (1)
104-114: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRace condition leaks orphaned interval when queue changes during async fetch.
If the effect cleanup runs while
getSongInfo()is in-flight (e.g., queue changes within the 5s timeout window), the.then()callback still executes and setsintervalRef.current = setInterval(...). The cleanup already ran and cleared the old interval, so this new interval is never cleared —showPopupfires every 40s indefinitely, even after unmount. Each call also sets a 10shideTimerReftimeout, compounding the leak.🔒 Proposed fix: add cancellation flag
useEffect(() => { - let timeoutId: NodeJS.Timeout; + let timeoutId: NodeJS.Timeout | undefined; + let cancelled = false; if (queue.songIds.length > 1 && queue.position !== null) { setUpNextSongData(undefined); const nextSongIndex = queue.songIds[queue.position + 1]; if (nextSongIndex) { timeoutId = setTimeout( () => window.api.audioLibraryControls .getSongInfo([nextSongIndex]) .then((res) => { + if (cancelled) return; if (res && res[0]) { const [nextSongData] = res; upNextSongDataCache.current = nextSongData; intervalRef.current = setInterval(showPopup, 40000); } return undefined; }) .catch((err) => console.error(err)), 5000 ); } } return () => { + cancelled = true; if (timeoutId) clearTimeout(timeoutId); if (intervalRef.current) { clearInterval(intervalRef.current); intervalRef.current = null; } }; }, [queue.position, queue.songIds, showPopup]);🤖 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/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsx` around lines 104 - 114, The UpNextSongPopup effect has an async race where the getSongInfo() promise can resolve after cleanup and still create a new interval, leaving an orphaned setInterval and hide timer running. Add a cancellation/active flag inside the effect around the showPopup/getSongInfo flow, and check it before updating upNextSongDataCache.current, intervalRef.current, or scheduling the 40s interval. Ensure the cleanup for the same effect marks the request as canceled and clears any existing timeout/interval refs so the callback cannot re-arm timers after queue changes or unmount.
🧹 Nitpick comments (9)
src/renderer/src/routes/main-player/albums/$albumId.tsx (1)
43-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the sort persistence pattern into a reusable hook.
The same three-step pattern (read from store → default in
Route.useSearch()→useEffectto persist) is repeated across all six detail-page routes. A focused custom hook would reduce duplication and centralize the persistence logic:♻️ Optional refactor: `usePersistedSortingState` hook
// e.g. src/renderer/src/hooks/usePersistedSortingState.ts import { store } from '`@renderer/store/store`'; import storage from '`@renderer/utils/localStorage`'; import { useStore } from '`@tanstack/react-store`'; import { useEffect } from 'react'; export function usePersistedSortingState( pageKey: SortingStateKeys, defaultSort: SongSortTypes ) { const persistedSort = useStore( store, (state) => state.localStorage.sortingStates?.[pageKey] || defaultSort ); const { sortingOrder = persistedSort } = Route.useSearch(); useEffect(() => { storage.sortingStates.setSortingStates(pageKey, sortingOrder); }, [sortingOrder, pageKey]); return sortingOrder; }Usage in each route would simplify to:
- const albumDetailSortingState = useStore( - store, - (state) => state.localStorage.sortingStates?.albumDetailPage || 'trackNoDescending' - ); - const { scrollTopOffset, sortingOrder = albumDetailSortingState } = Route.useSearch(); - useEffect(() => { - storage.sortingStates.setSortingStates('albumDetailPage', sortingOrder); - }, [sortingOrder]); + const sortingOrder = usePersistedSortingState('albumDetailPage', 'trackNoDescending'); + const { scrollTopOffset } = Route.useSearch();This is purely a DRY improvement — the current code is correct and readable. Defer if preferred.
🤖 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/renderer/src/routes/main-player/albums/`$albumId.tsx around lines 43 - 59, The sort persistence logic in the album detail route is duplicated across multiple detail pages and should be centralized. Extract the read-from-store, Route.useSearch defaulting, and useEffect persistence pattern into a reusable hook such as usePersistedSortingState, then use that hook in this route instead of repeating the persistence code. Keep the hook generic enough to accept the page key and default sort, and ensure it still writes through storage.sortingStates.setSortingStates with the current sortingOrder.src/main/db/queries/lyricsIndex.ts (1)
104-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider parallelizing or batching the backfill for large libraries.
indexAllLyricsprocesses songs sequentially, each involving file I/O (metadata read + LRC file read) and a DB upsert. For libraries with thousands of songs, this could take minutes. A bounded-concurrency approach (e.g., processing chunks of 10–20 songs in parallel) would significantly reduce wall-clock time while keeping DB pressure manageable.♻️ Proposed batched approach
export const indexAllLyrics = async (): Promise<void> => { logger.info('Starting lyrics index backfill.'); const { data: songs } = await getAllSongs({}); let indexed = 0; - for (const song of songs) { - try { - await upsertSongLyrics(song.id, song.path); - indexed += 1; - } catch (error) { - logger.error(`Failed to index lyrics for song ${song.id}`, { error }); - } - } + const BATCH_SIZE = 10; + for (let i = 0; i < songs.length; i += BATCH_SIZE) { + const batch = songs.slice(i, i + BATCH_SIZE); + const results = await Promise.allSettled( + batch.map((song) => upsertSongLyrics(song.id, song.path)) + ); + for (let j = 0; j < results.length; j += 1) { + if (results[j].status === 'fulfilled') indexed += 1; + else logger.error(`Failed to index lyrics for song ${batch[j].id}`, { error: results[j].reason }); + } + } logger.info(`Lyrics index backfill complete. Indexed ${indexed} of ${songs.length} songs.`); };🤖 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/lyricsIndex.ts` around lines 104 - 119, The indexAllLyrics backfill currently runs one song at a time, which will be too slow for large libraries. Update indexAllLyrics to process songs in bounded parallel batches instead of a fully sequential for-of loop, keeping the existing getAllSongs, upsertSongLyrics, and error logging behavior intact. Use a small fixed concurrency level or chunked batches so file I/O and DB upserts run in parallel without overwhelming the database, and preserve the final indexed count and completion log.src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx (3)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport ordering should follow external → internal alias → relative convention.
The
@renderer/store/storeimport (internal alias) appears before external dependencies. As per coding guidelines, useeslint-plugin-simple-import-sort-style import ordering: external dependencies first, then internal path aliases, then relative imports.🤖 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/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx` around lines 1 - 10, The import order in LyricSearchResultsContainer is out of convention because the internal alias import from store is placed before external dependencies. Reorder the imports to follow external first, then internal path aliases like `@renderer/store/store`, then relative imports, keeping the existing symbols in place.Source: Coding guidelines
19-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
HighlightedSnippetis duplicated across two files.
HighlightedSnippetexists in bothAllLyricResults.tsxandLyricSearchResultsContainer.tsxwith near-identical logic (one usesindex % 2 === 1, the other builds aSet<number>). Extract it to a shared component to avoid divergence. Note also that this version addstruncatewhile theAllLyricResultsversion usesmax-w-fullwithout truncation — if intentional, document the difference; if not, unify.Also applies to: 12-27
🤖 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/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx` around lines 19 - 41, HighlightedSnippet is duplicated between LyricSearchResultsContainer and AllLyricResults, so extract the shared snippet-rendering logic into a common component or helper and have both callers reuse it. Keep the highlight parsing behavior in one place (the snippet split, odd-index match handling, and mark/span rendering) so the two files cannot drift. Also reconcile the styling difference around truncate versus the non-truncating variant: either make both use the same class set or document the intentional exception where HighlightedSnippet is used.
66-97: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
searchInputis inuseMemodeps but unused in the body.
searchInputis listed as a dependency at line 96 but is never referenced inside theuseMemocallback, causing unnecessary re-computation when the search input changes without results changing.♻️ Proposed fix
- [handleSongPlayBtnClick, preferences?.isSongIndexingEnabled, noOfVisibleResults, results, searchInput] + [handleSongPlayBtnClick, preferences?.isSongIndexingEnabled, noOfVisibleResults, results]🤖 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/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx` around lines 66 - 97, Remove the unused searchInput dependency from the useMemo in LyricSearchResultsContainer, since the lyricResults callback does not reference it. Keep the dependency list limited to the values actually used in the memoized mapping/filtering logic, such as results, noOfVisibleResults, preferences?.isSongIndexingEnabled, and handleSongPlayBtnClick, to avoid unnecessary recomputation.src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport ordering should follow external → internal alias → relative convention.
The
@renderer/store/storeimport (internal alias) appears before external dependencies (@tanstack/react-store,react,react-i18next). As per coding guidelines, useeslint-plugin-simple-import-sort-style import ordering: external dependencies first, then internal path aliases, then relative imports.♻️ Proposed fix
-import { store } from '`@renderer/store/store`'; import { useStore } from '`@tanstack/react-store`'; import { useMemo } from 'react'; import { useTranslation } from 'react-i18next'; +import { store } from '`@renderer/store/store`'; + import Song from '../../SongsPage/Song';🤖 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/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx` around lines 1 - 6, The import order in AllLyricResults should follow external dependencies first, then internal alias imports, then relative imports. Reorder the imports so `@tanstack/react-store`, `react`, and `react-i18next` come before `@renderer/store/store`, and keep `Song` as the relative import last to match the simple-import-sort convention.Source: Coding guidelines
src/main/other/lastFm/sendFavoritesDataToLastFM.ts (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a helper for the repeated
insertScrobblecall.
insertScrobble({ operationType: method, trackTitle: title, artistNames: artists.join(', ') })is repeated verbatim at lines 55, 88, and 96.scrobbleSong.tsalready uses aqueueScrobbleForRetryhelper for the same pattern — apply the same approach here for consistency and to prevent divergence.♻️ Proposed refactor
+const queueFavoriteForRetry = async (method: Method, title: string, artists: string[]) => { + await insertScrobble({ + operationType: method, + trackTitle: title, + artistNames: artists.join(', ') + }); +}; + const sendFavoritesDataToLastFM = async (method: Method, title: string, artists: string[] = []) => { try { // ... if (!isConnectedToInternet) { - await insertScrobble({ operationType: method, trackTitle: title, artistNames: artists.join(', ') }); + await queueFavoriteForRetry(method, title, artists); return logger.debug('Love/Unlove queued for later - offline', { method, title }); } // ... if (res.status === 200) return logger.debug('Love/Unlove song request accepted.', { method, title, artists }); const json: LastFMLoveUnlovePostResponse = await res.json(); - await insertScrobble({ operationType: method, trackTitle: title, artistNames: artists.join(', ') }); + await queueFavoriteForRetry(method, title, artists); return logger.warn(/* ... */); } catch (error) { - await insertScrobble({ operationType: method, trackTitle: title, artistNames: artists.join(', ') }).catch(() => {}); + await queueFavoriteForRetry(method, title, artists).catch(() => {}); return logger.error(/* ... */); } };Also applies to: 88-88, 96-96
🤖 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/other/lastFm/sendFavoritesDataToLastFM.ts` at line 55, The repeated insertScrobble call in sendFavoritesDataToLastFM should be extracted into a shared helper to keep the three call sites in sync and match the retry pattern used in scrobbleSong.ts. Create a small helper near sendFavoritesDataToLastFM (or a local utility used by this flow) that builds the { operationType, trackTitle, artistNames } payload from method, title, and artists, then replace each direct insertScrobble invocation with that helper. Keep the behavior identical while removing the duplicated payload construction.src/main/other/lastFm/scrobbleSong.ts (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize
LASTFM_BASE_URLto avoid duplication.The constant
LASTFM_BASE_URL = 'https://ws.audioscrobbler.com/2.0/'is defined identically inscrobbleSong.ts,sendFavoritesDataToLastFM.ts, andsendNowPlayingSongDataToLastFM.ts. Move it tolastFmUtils.tsalongsideLASTFM_REQUEST_TIMEOUT_MSand import it, preventing divergence if the URL ever changes.♻️ Proposed refactor
In
lastFmUtils.ts:+export const LASTFM_BASE_URL = 'https://ws.audioscrobbler.com/2.0/';In each consuming file:
-import { LASTFM_REQUEST_TIMEOUT_MS, fetchWithTimeout } from './lastFmUtils'; +import { LASTFM_BASE_URL, LASTFM_REQUEST_TIMEOUT_MS, fetchWithTimeout } from './lastFmUtils'; -const LASTFM_BASE_URL = 'https://ws.audioscrobbler.com/2.0/';🤖 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/other/lastFm/scrobbleSong.ts` at line 13, Centralize the duplicated LASTFM_BASE_URL constant by moving it into lastFmUtils.ts alongside LASTFM_REQUEST_TIMEOUT_MS, then update scrobbleSong, sendFavoritesDataToLastFM, and sendNowPlayingSongDataToLastFM to import and use the shared symbol instead of defining their own copies. Keep the existing constant name LASTFM_BASE_URL so the consuming code can be updated consistently and avoid future divergence.src/main/other/discord.ts (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInitialize
lastPayloador include| undefinedin its type.
lastPayloadis declared without an initializer, so it'sundefinedat runtime until the firstsetDiscordRPCcall. The??on line 47 correctly handles this, but the type{ pid: number; activity: DiscordActivity }doesn't reflect that the variable can beundefined. A future reader could uselastPayloaddirectly without the nullish guard and get a runtimeundefinedwith no TypeScript warning.♻️ Initialize with defaultPayload
-let lastPayload: { pid: number; activity: DiscordActivity }; +let lastPayload: { pid: number; activity: DiscordActivity } = defaultPayload;Alternatively, type it explicitly:
-let lastPayload: { pid: number; activity: DiscordActivity }; +let lastPayload: { pid: number; activity: DiscordActivity } | undefined;🤖 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/other/discord.ts` around lines 34 - 36, `lastPayload` in `discord.ts` is declared as always-present even though it starts out unset, so update its declaration to reflect the initial undefined state or initialize it with the existing default payload used by `setDiscordRPC`. Use the `lastPayload` variable and the `setDiscordRPC` flow as the touchpoints, and ensure any direct access remains type-safe without relying on an implicit runtime 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 @.agents/skills/changelog-entry/SKILL.md:
- Around line 49-56: The comparison link instructions in SKILL.md are hard-coded
to the Sandakan/Nora GitHub slug, which will send users to the wrong repo when
copied. Update the guidance under “Add Comparison Link at the Bottom” to use a
repo-relative compare URL or a placeholder based on the current Nora remote, and
keep the reference to the changelog comparison chain tied to the version-link
pattern used in CHANGELOG.md.
In `@flatpak/app.netlify.noramusic.Nora.metainfo.xml`:
- Around line 80-81: The release entry for the 3.1.0 record has a mismatched
date value; update the release metadata in the metainfo XML so the date matches
the changelog heading for v3.1.0-stable. Locate the release element with version
3.1.0 and change its date attribute from the outdated 2024 value to 2025-03-29.
In `@src/main/core/getSongLyrics.ts`:
- Line 27: The logic in getSongLyrics is stripping the extension with
replaceAll, which removes every matching substring in the full path instead of
only the final file extension. Update the defaultLrcFilePathWithoutExtension
calculation to preserve directory names by using a basename/dirname split or a
single trailing-extension removal, so only the song file’s final extension is
removed before appending .lrc.
In `@src/main/core/sendAudioData.ts`:
- Around line 111-115: The outer catch in sendAudioData is overwriting existing
error codes, so preserve any error that already has a code before wrapping it.
Update the catch block in sendAudioData to detect errors like SONG_NOT_FOUND and
rethrow them unchanged, and only create the SONG_DATA_SEND_FAILED error when the
caught error has no code. Keep the existing logger.error call and ensure the
behavior around usePlaybackErrors remains driven by the original error code.
In `@src/main/ipc.ts`:
- Line 606: Add error containment around the fire-and-forget call in the
ipcMain.on handler by updating the flushScrobbleQueue invocation so any
rejection is handled locally instead of becoming unhandled; keep the existing
synchronous event flow, but chain a .catch() that logs the failure with enough
context for debugging. Use the flushScrobbleQueue symbol and the surrounding
network-status handler in src/main/ipc.ts to locate the change.
In `@src/main/other/lastFm/flushScrobbleQueue.ts`:
- Around line 126-143: The `postToLastFm` helper currently treats any HTTP 200
as success, but Last.fm can still return an API-level failure in the response
body. Update `postToLastFm` to inspect the parsed response payload after
`fetchWithTimeout`, detect Last.fm `error`/`message` fields, and throw when they
indicate a failed request so `markSent` does not remove scrobbles that were not
actually accepted.
In `@src/main/saveLyricsToSong.ts`:
- Around line 114-118: The timestamp cleanup in the lyrics plain-text pipeline
only strips one fractional timestamp and misses common LRC variants, so update
the stripping logic used in saveLyricsToSong to remove all leading timestamp
tags, including multiple tags and optional fractional parts, before
trimming/filtering. Reuse or centralize the shared behavior with
extractPlainTextFromParsed in saveLyricsToLrcFile.ts so both paths stay
consistent and do not diverge.
In `@src/renderer/src/assets/locales/vi/vi.json`:
- Around line 172-173: The locale strings for mostSkipped and leastSkipped are
still in English in the Vietnamese translation file. Update these entries in
vi.json to proper Vietnamese translations, using the existing locale key names
and matching the style of nearby translated labels, so the sort labels read
naturally in Vietnamese.
In
`@src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx`:
- Around line 111-113: The “Lyrics” heading in LyricSearchResultsContainer is
hardcoded and should use localization instead. Update the label in the result
header to use the same i18n key as search/all/index.tsx, namely
t('common.lyric_other'), and keep the text rendering consistent with the
existing translation pattern used in the SearchPage components.
In `@src/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsx`:
- Around line 124-130: The popup effect in UpNextSongPopup.tsx clears timers but
leaves upNextSongDataCache.current populated, so stale song data can be reused
after queue changes. Update the cleanup returned by the effect that manages
showPopup/setUpNextSongData to also reset upNextSongDataCache.current to
undefined (along with hiding the popup) so the next hover or Ctrl double-press
waits for fresh data instead of showing the previous track.
- Around line 34-44: The hide timeout created in showPopup can outlive the
component, so add unmount cleanup for hideTimerRef in UpNextSongPopup to clear
any pending timeout and reset the ref. Use a cleanup effect tied to the
component lifecycle, and keep the existing hideTimerRef logic in showPopup
consistent so the timer is always cleared both before rescheduling and on
unmount.
In `@src/renderer/src/other/player.ts`:
- Around line 550-560: The pending auto-play cleanup in loadSong is removing the
listener from the current targetAudio even when the handler was registered on a
different element. Update the pendingAutoPlayHandler tracking in AudioPlayer so
it stores both the handler and the specific audio element it was attached to,
and remove the listener from that stored element before replacing it. Make sure
the canplay callback still clears only its own registration and does not
interfere with a newer pendingAutoPlayHandler after completeCrossfade or
gaplessSwapToNext swaps activeElement.
---
Outside diff comments:
In `@src/main/search.ts`:
- Around line 30-46: The no-results fallback in search.ts currently ignores
lyrics, so it can still run searchForAvailableResults even when
searchSongsByLyrics returns matches. Update the availableResults check in the
search flow to include lyrics.length alongside songs, artists, albums,
playlists, and genres, and verify the logic in the search function uses the
lyrics array from Promise.all consistently before triggering any “did you mean”
suggestions.
In `@src/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsx`:
- Around line 104-114: The UpNextSongPopup effect has an async race where the
getSongInfo() promise can resolve after cleanup and still create a new interval,
leaving an orphaned setInterval and hide timer running. Add a
cancellation/active flag inside the effect around the showPopup/getSongInfo
flow, and check it before updating upNextSongDataCache.current,
intervalRef.current, or scheduling the 40s interval. Ensure the cleanup for the
same effect marks the request as canceled and clears any existing
timeout/interval refs so the callback cannot re-arm timers after queue changes
or unmount.
---
Nitpick comments:
In `@src/main/db/queries/lyricsIndex.ts`:
- Around line 104-119: The indexAllLyrics backfill currently runs one song at a
time, which will be too slow for large libraries. Update indexAllLyrics to
process songs in bounded parallel batches instead of a fully sequential for-of
loop, keeping the existing getAllSongs, upsertSongLyrics, and error logging
behavior intact. Use a small fixed concurrency level or chunked batches so file
I/O and DB upserts run in parallel without overwhelming the database, and
preserve the final indexed count and completion log.
In `@src/main/other/discord.ts`:
- Around line 34-36: `lastPayload` in `discord.ts` is declared as always-present
even though it starts out unset, so update its declaration to reflect the
initial undefined state or initialize it with the existing default payload used
by `setDiscordRPC`. Use the `lastPayload` variable and the `setDiscordRPC` flow
as the touchpoints, and ensure any direct access remains type-safe without
relying on an implicit runtime undefined.
In `@src/main/other/lastFm/scrobbleSong.ts`:
- Line 13: Centralize the duplicated LASTFM_BASE_URL constant by moving it into
lastFmUtils.ts alongside LASTFM_REQUEST_TIMEOUT_MS, then update scrobbleSong,
sendFavoritesDataToLastFM, and sendNowPlayingSongDataToLastFM to import and use
the shared symbol instead of defining their own copies. Keep the existing
constant name LASTFM_BASE_URL so the consuming code can be updated consistently
and avoid future divergence.
In `@src/main/other/lastFm/sendFavoritesDataToLastFM.ts`:
- Line 55: The repeated insertScrobble call in sendFavoritesDataToLastFM should
be extracted into a shared helper to keep the three call sites in sync and match
the retry pattern used in scrobbleSong.ts. Create a small helper near
sendFavoritesDataToLastFM (or a local utility used by this flow) that builds the
{ operationType, trackTitle, artistNames } payload from method, title, and
artists, then replace each direct insertScrobble invocation with that helper.
Keep the behavior identical while removing the duplicated payload construction.
In
`@src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx`:
- Around line 1-6: The import order in AllLyricResults should follow external
dependencies first, then internal alias imports, then relative imports. Reorder
the imports so `@tanstack/react-store`, `react`, and `react-i18next` come before
`@renderer/store/store`, and keep `Song` as the relative import last to match
the simple-import-sort convention.
In
`@src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx`:
- Around line 1-10: The import order in LyricSearchResultsContainer is out of
convention because the internal alias import from store is placed before
external dependencies. Reorder the imports to follow external first, then
internal path aliases like `@renderer/store/store`, then relative imports, keeping
the existing symbols in place.
- Around line 19-41: HighlightedSnippet is duplicated between
LyricSearchResultsContainer and AllLyricResults, so extract the shared
snippet-rendering logic into a common component or helper and have both callers
reuse it. Keep the highlight parsing behavior in one place (the snippet split,
odd-index match handling, and mark/span rendering) so the two files cannot
drift. Also reconcile the styling difference around truncate versus the
non-truncating variant: either make both use the same class set or document the
intentional exception where HighlightedSnippet is used.
- Around line 66-97: Remove the unused searchInput dependency from the useMemo
in LyricSearchResultsContainer, since the lyricResults callback does not
reference it. Keep the dependency list limited to the values actually used in
the memoized mapping/filtering logic, such as results, noOfVisibleResults,
preferences?.isSongIndexingEnabled, and handleSongPlayBtnClick, to avoid
unnecessary recomputation.
In `@src/renderer/src/routes/main-player/albums/`$albumId.tsx:
- Around line 43-59: The sort persistence logic in the album detail route is
duplicated across multiple detail pages and should be centralized. Extract the
read-from-store, Route.useSearch defaulting, and useEffect persistence pattern
into a reusable hook such as usePersistedSortingState, then use that hook in
this route instead of repeating the persistence code. Keep the hook generic
enough to accept the page key and default sort, and ensure it still writes
through storage.sortingStates.setSortingStates with the current sortingOrder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16b9edc0-bd7b-482a-82d6-d4006e2b9b3c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (113)
.agents/skills/changelog-entry/SKILL.md.github/copilot-instructions.md.github/workflows/build.yml.github/workflows/codeql-analysis.yml.github/workflows/lint.yml.github/workflows/test.ymlCHANGELOG.mdPRIVACY-POLICY.mdREADME.mdchangelog.mdcrowdin.ymlflatpak/app.netlify.noramusic.Nora.metainfo.xmlpackage.jsonrelease-notes-schema.jsonrelease-notes.jsonresources/drizzle/0002_add_zoom_factor_to_user_settings.sqlresources/drizzle/0002_square_greymalkin.sqlresources/drizzle/0003_add_tray_click_setting.sqlresources/drizzle/0003_lowly_komodo.sqlresources/drizzle/0004_talented_master_chief.sqlresources/drizzle/0006_many_nightshade.sqlresources/drizzle/meta/0002_snapshot.jsonresources/drizzle/meta/0003_snapshot.jsonresources/drizzle/meta/0004_talented_master_chief_snapshot.jsonresources/drizzle/meta/0006_snapshot.jsonresources/drizzle/meta/_journal.jsonsrc/main/core/getSongLyrics.tssrc/main/core/saveLyricsToLrcFile.tssrc/main/core/sendAudioData.tssrc/main/core/updateSongListeningData.tssrc/main/db/queries/history.tssrc/main/db/queries/listens.tssrc/main/db/queries/lyricsIndex.tssrc/main/db/queries/scrobble_queue.tssrc/main/db/queries/search.tssrc/main/db/queries/songs.tssrc/main/db/schema.tssrc/main/handleFileProtocol.tssrc/main/ipc.tssrc/main/main.tssrc/main/other/artworks.tssrc/main/other/discord.tssrc/main/other/discordRPC.tssrc/main/other/lastFm/flushScrobbleQueue.tssrc/main/other/lastFm/generateApiRequestBodyForLastFMPostRequests.tssrc/main/other/lastFm/lastFmUtils.tssrc/main/other/lastFm/scrobbleSong.tssrc/main/other/lastFm/sendFavoritesDataToLastFM.tssrc/main/other/lastFm/sendNowPlayingSongDataToLastFM.tssrc/main/parseSong/parseSong.tssrc/main/saveLyricsToSong.tssrc/main/search.tssrc/preload/index.tssrc/renderer/src/App.tsxsrc/renderer/src/assets/locales/as/as.jsonsrc/renderer/src/assets/locales/en/en.jsonsrc/renderer/src/assets/locales/fr/fr.jsonsrc/renderer/src/assets/locales/pl/pl.jsonsrc/renderer/src/assets/locales/pt-br/pt-br.jsonsrc/renderer/src/assets/locales/tr/tr.jsonsrc/renderer/src/assets/locales/vi/vi.jsonsrc/renderer/src/components/Button.tsxsrc/renderer/src/components/ErrorPrompt.tsxsrc/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsxsrc/renderer/src/components/MainContainer.tsxsrc/renderer/src/components/ReleaseNotesPrompt/ReleaseNotesPrompt.tsxsrc/renderer/src/components/ReleaseNotesPrompt/VersionNote.tsxsrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsxsrc/renderer/src/components/SearchPage/MostRelevantResult.tsxsrc/renderer/src/components/SearchPage/NoSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/SearchOptions.tsxsrc/renderer/src/components/SecondaryContainer.tsxsrc/renderer/src/components/SeekBarSlider.tsxsrc/renderer/src/components/SettingsPage/Settings/AboutSettings.tsxsrc/renderer/src/components/SettingsPage/Settings/AudioPlaybackSettings.tsxsrc/renderer/src/components/SettingsPage/SettingsPage.tsxsrc/renderer/src/components/SongsControlsContainer/SongControlsAndSeekbarContainer.tsxsrc/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsxsrc/renderer/src/components/SongsPage/SongOptions.tsxsrc/renderer/src/components/TitleBar/CurrentLocationContainer.tsxsrc/renderer/src/components/TitleBar/NavigationControlsContainer.tsxsrc/renderer/src/components/TitleBar/TitleBar.tsxsrc/renderer/src/contexts/AppUpdateContext.tsxsrc/renderer/src/hooks/useAppLifecycle.tsxsrc/renderer/src/hooks/useAppUpdates.tsxsrc/renderer/src/hooks/useDiscordRpc.tsxsrc/renderer/src/hooks/useListeningData.tsxsrc/renderer/src/hooks/useMediaSession.tsxsrc/renderer/src/hooks/usePlaybackErrors.tsxsrc/renderer/src/hooks/usePlaybackSettings.tsxsrc/renderer/src/i18n.tssrc/renderer/src/other/appReducer.tsxsrc/renderer/src/other/player.tssrc/renderer/src/other/playerQueue.tssrc/renderer/src/routes/main-player/albums/$albumId.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/genres/$genreId.tsxsrc/renderer/src/routes/main-player/lyrics/editor/$songId.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/renderer/src/routes/main-player/queue/index.tsxsrc/renderer/src/routes/main-player/search/all/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/routes/main-player/songs/$songId.edit.tsxsrc/renderer/src/routes/mini-player/index.tsxsrc/renderer/src/utils/parseChangelog.tssrc/types/app.d.tstest/src/main/parseSong/parseSong-concurrency.test.tstest/src/renderer/src/other/playerQueue.test.tstest/src/renderer/src/utils/addMissingPropsToAnObject.test.tstest/src/renderer/src/utils/parseChangelog.test.ts
💤 Files with no reviewable changes (5)
- release-notes-schema.json
- resources/drizzle/0003_add_tray_click_setting.sql
- changelog.md
- resources/drizzle/0002_add_zoom_factor_to_user_settings.sql
- release-notes.json
| ### Step 3: Add Comparison Link at the Bottom | ||
| Go to the bottom of `CHANGELOG.md` and add the comparison link in alphabetical/version order: | ||
|
|
||
| ```markdown | ||
| [3.2.0-stable]: https://github.com/Sandakan/Nora/compare/v3.1.0-stable...v3.2.0-stable | ||
| ``` | ||
|
|
||
| Update the previous version's comparison link to point to the version before it if needed, ensuring the comparison chain is unbroken. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a repo-relative compare URL here.
The hard-coded Sandakan/Nora slug sends copy-pasted instructions to the wrong repository. Make this a placeholder or the current Nora remote instead.
Suggested fix
-[3.2.0-stable]: https://github.com/Sandakan/Nora/compare/v3.1.0-stable...v3.2.0-stable
+[3.2.0-stable]: https://github.com/<owner>/<repo>/compare/v3.1.0-stable...v3.2.0-stable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Step 3: Add Comparison Link at the Bottom | |
| Go to the bottom of `CHANGELOG.md` and add the comparison link in alphabetical/version order: | |
| ```markdown | |
| [3.2.0-stable]: https://github.com/Sandakan/Nora/compare/v3.1.0-stable...v3.2.0-stable | |
| ``` | |
| Update the previous version's comparison link to point to the version before it if needed, ensuring the comparison chain is unbroken. | |
| ### Step 3: Add Comparison Link at the Bottom | |
| Go to the bottom of `CHANGELOG.md` and add the comparison link in alphabetical/version order: | |
🤖 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 @.agents/skills/changelog-entry/SKILL.md around lines 49 - 56, The comparison
link instructions in SKILL.md are hard-coded to the Sandakan/Nora GitHub slug,
which will send users to the wrong repo when copied. Update the guidance under
“Add Comparison Link at the Bottom” to use a repo-relative compare URL or a
placeholder based on the current Nora remote, and keep the reference to the
changelog comparison chain tied to the version-link pattern used in
CHANGELOG.md.
| <release version="3.1.0" date="2024-03-29"> | ||
| <url type="details">https://github.com/Sandakan/Nora/blob/master/changelog.md#v310-stable----29th-of-march-2025-</url> | ||
| <url type="details">https://github.com/Sandakan/Nora/blob/master/CHANGELOG.md#310-stable---2025-03-29</url> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the v3.1.0 heading in CHANGELOG.md to verify the correct date.
rg -n '3\.1\.0' CHANGELOG.md | head -5Repository: Owie6789/Nora
Length of output: 1978
🌐 Web query:
Search the public repository for the CHANGELOG.md heading for version 3.1.0 and the metainfo XML release entry date/url anchor in flatpak/app.netlify.noramusic.Nora.metainfo.xml.
💡 Result:
In the Sandakan/Nora repository, the CHANGELOG.md file contains the heading for version 3.1.0 as: ### v3.1.0-stable - ( 29 th of March 2025) [1][2] Regarding the metainfo XML file for the application, no public record exists for a file named app.netlify.noramusic.Nora.metainfo.xml within the authoritative Sandakan/Nora repository [3][4]. The repository primarily uses the project name Nora, and public identifiers for similar projects (such as those on Flathub) do not match the specific Netlify-based naming convention requested [5]. Consequently, there is no corresponding release entry date or URL anchor for that specific file path [3][4].
Citations:
- 1: https://raw.githubusercontent.com/Sandakan/Nora/master/changelog.md
- 2: https://github.com/Sandakan/Nora/releases/tag/v3.1.0-stable
- 3: https://github.com/Sandakan/Nora
- 4: https://github.com/Sandakan/Oto-Music-for-Desktop
- 5: flathub/codes.nora.gDiceRoller@234aacf
Update the release date to 2025-03-29
flatpak/app.netlify.noramusic.Nora.metainfo.xml: the changelog heading for v3.1.0-stable is dated 29 March 2025, so date="2024-03-29" is the mismatched value here.
🤖 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 `@flatpak/app.netlify.noramusic.Nora.metainfo.xml` around lines 80 - 81, The
release entry for the 3.1.0 record has a mismatched date value; update the
release metadata in the metainfo XML so the date matches the changelog heading
for v3.1.0-stable. Locate the release element with version 3.1.0 and change its
date attribute from the outdated 2024 value to 2025-03-29.
| customLrcFilesSaveLocation?: string | null | ||
| ): string[] => { | ||
| const defaultLrcFilePath = `${songPath}.lrc`; | ||
| const defaultLrcFilePathWithoutExtension = `${songPath.replaceAll(path.extname(songPath), '')}.lrc`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file around the reported lines.
nl -ba src/main/core/getSongLyrics.ts | sed -n '1,180p'
# Probe the exact string behavior on representative paths.
node - <<'JS'
const path = require('path');
const samples = [
'/music/track.mp3',
'/music/album.mp3/track.mp3',
'/music/mp3/track.mp3',
'/music/track',
'/music/.mp3/track.mp3',
'/music/track.final.mp3',
];
for (const songPath of samples) {
const ext = path.extname(songPath);
const candidate = `${songPath.replaceAll(ext, '')}.lrc`;
const basenameStrip = `${path.join(path.dirname(songPath), path.basename(songPath, ext))}.lrc`;
console.log({ songPath, ext, candidate, basenameStrip });
}
JS
# Find other uses of this pattern, if any.
rg -n "replaceAll\(path\.extname\(" srcRepository: Owie6789/Nora
Length of output: 1949
Strip only the trailing extension, not every matching substring
songPath.replaceAll(path.extname(songPath), '') removes every occurrence of the extension text anywhere in the full path, so a directory name like /music/album.mp3/track.mp3 gets mangled before .lrc is appended. Use a basename/dirname split or a single trailing-extension slice instead.
🤖 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/core/getSongLyrics.ts` at line 27, The logic in getSongLyrics is
stripping the extension with replaceAll, which removes every matching substring
in the full path instead of only the final file extension. Update the
defaultLrcFilePathWithoutExtension calculation to preserve directory names by
using a basename/dirname split or a single trailing-extension removal, so only
the song file’s final extension is removed before appending .lrc.
| } catch (error) { | ||
| logger.error(`Failed to send songs data.`, { err: error }); | ||
| throw new Error('SONG_DATA_SEND_FAILED' as ErrorCodes); | ||
| const sendError = new Error('Failed to send song data'); | ||
| (sendError as NodeJS.ErrnoException).code = 'SONG_DATA_SEND_FAILED'; | ||
| throw sendError; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Outer catch overwrites SONG_NOT_FOUND error code with SONG_DATA_SEND_FAILED.
The SONG_NOT_FOUND errors thrown at lines 64–66 (file not found) and 108–110 (no matching song) are caught by this outer catch block and re-thrown as SONG_DATA_SEND_FAILED. The original error code is lost, so callers will never see SONG_NOT_FOUND. This breaks the skip-song-on-error feature — usePlaybackErrors cannot distinguish a missing-file error from a generic failure to show the skip button.
🐛 Proposed fix: preserve errors that already have a code
} catch (error) {
+ // Re-throw errors that already have a specific error code (e.g. SONG_NOT_FOUND)
+ if ((error as NodeJS.ErrnoException).code) {
+ throw error;
+ }
logger.error(`Failed to send songs data.`, { err: error });
const sendError = new Error('Failed to send song data');
(sendError as NodeJS.ErrnoException).code = 'SONG_DATA_SEND_FAILED';
throw sendError;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| logger.error(`Failed to send songs data.`, { err: error }); | |
| throw new Error('SONG_DATA_SEND_FAILED' as ErrorCodes); | |
| const sendError = new Error('Failed to send song data'); | |
| (sendError as NodeJS.ErrnoException).code = 'SONG_DATA_SEND_FAILED'; | |
| throw sendError; | |
| } catch (error) { | |
| // Re-throw errors that already have a specific error code (e.g. SONG_NOT_FOUND) | |
| if ((error as NodeJS.ErrnoException).code) { | |
| throw error; | |
| } | |
| logger.error(`Failed to send songs data.`, { err: error }); | |
| const sendError = new Error('Failed to send song data'); | |
| (sendError as NodeJS.ErrnoException).code = 'SONG_DATA_SEND_FAILED'; | |
| throw sendError; |
🤖 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/core/sendAudioData.ts` around lines 111 - 115, The outer catch in
sendAudioData is overwriting existing error codes, so preserve any error that
already has a code before wrapping it. Update the catch block in sendAudioData
to detect errors like SONG_NOT_FOUND and rethrow them unchanged, and only create
the SONG_DATA_SEND_FAILED error when the caught error has no code. Keep the
existing logger.error call and ensure the behavior around usePlaybackErrors
remains driven by the original error code.
| : `App disconnected from the internet` | ||
| ); | ||
| // isConnectedToInternet = isConnected; | ||
| if (isConnected) flushScrobbleQueue(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add .catch() to flushScrobbleQueue() to prevent unhandled promise rejection.
flushScrobbleQueue() is async but called fire-and-forget inside a synchronous ipcMain.on handler. If the promise rejects (e.g., database error during batch claim/flush), it becomes an unhandled promise rejection that can crash the Electron main process. Add a .catch() to log the failure safely.
As per coding guidelines, ipcMain.on() is correct for fire-and-forget events like app/networkStatusChange, but the async side effect still needs error containment.
🔒️ Proposed fix
- if (isConnected) flushScrobbleQueue();
+ if (isConnected)
+ flushScrobbleQueue().catch((error) =>
+ logger.error('Failed to flush scrobble queue on network reconnect', { error })
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isConnected) flushScrobbleQueue(); | |
| if (isConnected) | |
| flushScrobbleQueue().catch((error) => | |
| logger.error('Failed to flush scrobble queue on network reconnect', { error }) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc.ts` at line 606, Add error containment around the
fire-and-forget call in the ipcMain.on handler by updating the
flushScrobbleQueue invocation so any rejection is handled locally instead of
becoming unhandled; keep the existing synchronous event flow, but chain a
.catch() that logs the failure with enough context for debugging. Use the
flushScrobbleQueue symbol and the surrounding network-status handler in
src/main/ipc.ts to locate the change.
Source: Coding guidelines
| "mostSkipped": "Most skipped (All time)", | ||
| "leastSkipped": "Least skipped (All time)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
mostSkipped and leastSkipped are not translated to Vietnamese.
These sort type labels are left in English ("Most skipped (All time)", "Least skipped (All time)") in the Vietnamese locale file. They should be translated, e.g. "Bỏ qua nhiều nhất (Từ trước đến nay)" and "Bỏ qua ít nhất (Từ trước đến nay)".
🤖 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/renderer/src/assets/locales/vi/vi.json` around lines 172 - 173, The
locale strings for mostSkipped and leastSkipped are still in English in the
Vietnamese translation file. Update these entries in vi.json to proper
Vietnamese translations, using the existing locale key names and matching the
style of nearby translated labels, so the sort labels read naturally in
Vietnamese.
| const hideTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const showPopup = useCallback(() => { | ||
| if (!upNextSongDataCache.current) return; | ||
| if (hideTimerRef.current) clearTimeout(hideTimerRef.current); | ||
| setUpNextSongData(upNextSongDataCache.current); | ||
| setTimeout(() => setUpNextSongData(undefined), 10000); | ||
| hideTimerRef.current = setTimeout(() => { | ||
| setUpNextSongData(undefined); | ||
| hideTimerRef.current = null; | ||
| }, 10000); | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear hideTimerRef on unmount to prevent orphaned hide timeout.
showPopup sets hideTimerRef.current to a 10s timeout, but no effect cleans it up on unmount. The timer fires post-unmount (React 19 silently ignores the state update) but still leaks a timer handle.
🛡️ Proposed fix: add unmount cleanup for hideTimerRef
useEffect(() => {
registerUpNextPopupFn(showPopup);
}, [registerUpNextPopupFn, showPopup]);
+
+ useEffect(() => {
+ return () => {
+ if (hideTimerRef.current) {
+ clearTimeout(hideTimerRef.current);
+ hideTimerRef.current = null;
+ }
+ };
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hideTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const showPopup = useCallback(() => { | |
| if (!upNextSongDataCache.current) return; | |
| if (hideTimerRef.current) clearTimeout(hideTimerRef.current); | |
| setUpNextSongData(upNextSongDataCache.current); | |
| setTimeout(() => setUpNextSongData(undefined), 10000); | |
| hideTimerRef.current = setTimeout(() => { | |
| setUpNextSongData(undefined); | |
| hideTimerRef.current = null; | |
| }, 10000); | |
| }, []); | |
| const hideTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const showPopup = useCallback(() => { | |
| if (!upNextSongDataCache.current) return; | |
| if (hideTimerRef.current) clearTimeout(hideTimerRef.current); | |
| setUpNextSongData(upNextSongDataCache.current); | |
| hideTimerRef.current = setTimeout(() => { | |
| setUpNextSongData(undefined); | |
| hideTimerRef.current = null; | |
| }, 10000); | |
| }, []); | |
| useEffect(() => { | |
| return () => { | |
| if (hideTimerRef.current) { | |
| clearTimeout(hideTimerRef.current); | |
| hideTimerRef.current = null; | |
| } | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsx`
around lines 34 - 44, The hide timeout created in showPopup can outlive the
component, so add unmount cleanup for hideTimerRef in UpNextSongPopup to clear
any pending timeout and reset the ref. Use a cleanup effect tied to the
component lifecycle, and keep the existing hideTimerRef logic in showPopup
consistent so the timer is always cleared both before rescheduling and on
unmount.
| return () => { | ||
| if (timeoutId) clearTimeout(timeoutId); | ||
| if (timeIntervalId) clearInterval(timeIntervalId); | ||
| if (intervalRef.current) { | ||
| clearInterval(intervalRef.current); | ||
| intervalRef.current = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clear upNextSongDataCache on queue change to prevent stale popup data.
The effect hides the popup (setUpNextSongData(undefined)) but never clears upNextSongDataCache.current. If showPopup is triggered (via hover or Ctrl double-press) before the new fetch resolves (~5s), it displays the previous song's data.
🛡️ Proposed fix: clear cache in cleanup
return () => {
cancelled = true;
if (timeoutId) clearTimeout(timeoutId);
if (intervalRef.current) {
clearInterval(intervalRef.current);
intervalRef.current = null;
}
+ upNextSongDataCache.current = null as unknown as SongData;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return () => { | |
| if (timeoutId) clearTimeout(timeoutId); | |
| if (timeIntervalId) clearInterval(timeIntervalId); | |
| if (intervalRef.current) { | |
| clearInterval(intervalRef.current); | |
| intervalRef.current = null; | |
| } | |
| }; | |
| return () => { | |
| cancelled = true; | |
| if (timeoutId) clearTimeout(timeoutId); | |
| if (intervalRef.current) { | |
| clearInterval(intervalRef.current); | |
| intervalRef.current = null; | |
| } | |
| upNextSongDataCache.current = null as unknown as SongData; | |
| }; |
🤖 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/renderer/src/components/SongsControlsContainer/UpNextSongPopup.tsx`
around lines 124 - 130, The popup effect in UpNextSongPopup.tsx clears timers
but leaves upNextSongDataCache.current populated, so stale song data can be
reused after queue changes. Update the cleanup returned by the effect that
manages showPopup/setUpNextSongData to also reset upNextSongDataCache.current to
undefined (along with hiding the popup) so the next hover or Ctrl double-press
waits for fresh data instead of showing the previous track.
| if (this.pendingAutoPlayHandler) { | ||
| targetAudio.removeEventListener('canplay', this.pendingAutoPlayHandler); | ||
| } | ||
| const autoPlayHandler = () => { | ||
| this.pendingAutoPlayHandler = null; | ||
| this.play().catch((err) => | ||
| console.error('[AudioPlayer] Auto-play on canplay failed:', err) | ||
| ); | ||
| targetAudio.removeEventListener('canplay', autoPlayHandler); | ||
| }; | ||
| this.pendingAutoPlayHandler = autoPlayHandler; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
pendingAutoPlayHandler cleanup may target the wrong audio element.
When loadSong removes a prior pendingAutoPlayHandler at line 551, it calls targetAudio.removeEventListener using the current active element. If the active element changed since the handler was registered (e.g., after completeCrossfade or gaplessSwapToNext swapped activeElement), the removal targets the wrong element. The stale handler remains on the old element and could fire unexpectedly — calling this.play() at an unintended time and clearing this.pendingAutoPlayHandler (line 554), which suppresses the new handler.
🛡️ Proposed fix: track the element alongside the handler
- private pendingAutoPlayHandler: (() => void) | null = null;
+ private pendingAutoPlayHandler: (() => void) | null = null;
+ private pendingAutoPlayElement: HTMLAudioElement | null = null;
// ... inside loadSong, lines 550-561:
- if (this.pendingAutoPlayHandler) {
- targetAudio.removeEventListener('canplay', this.pendingAutoPlayHandler);
- }
+ if (this.pendingAutoPlayHandler && this.pendingAutoPlayElement) {
+ this.pendingAutoPlayElement.removeEventListener('canplay', this.pendingAutoPlayHandler);
+ }
const autoPlayHandler = () => {
this.pendingAutoPlayHandler = null;
+ this.pendingAutoPlayElement = null;
this.play().catch((err) =>
console.error('[AudioPlayer] Auto-play on canplay failed:', err)
);
targetAudio.removeEventListener('canplay', autoPlayHandler);
};
this.pendingAutoPlayHandler = autoPlayHandler;
+ this.pendingAutoPlayElement = targetAudio;
targetAudio.addEventListener('canplay', autoPlayHandler);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.pendingAutoPlayHandler) { | |
| targetAudio.removeEventListener('canplay', this.pendingAutoPlayHandler); | |
| } | |
| const autoPlayHandler = () => { | |
| this.pendingAutoPlayHandler = null; | |
| this.play().catch((err) => | |
| console.error('[AudioPlayer] Auto-play on canplay failed:', err) | |
| ); | |
| targetAudio.removeEventListener('canplay', autoPlayHandler); | |
| }; | |
| this.pendingAutoPlayHandler = autoPlayHandler; | |
| if (this.pendingAutoPlayHandler && this.pendingAutoPlayElement) { | |
| this.pendingAutoPlayElement.removeEventListener('canplay', this.pendingAutoPlayHandler); | |
| } | |
| const autoPlayHandler = () => { | |
| this.pendingAutoPlayHandler = null; | |
| this.pendingAutoPlayElement = null; | |
| this.play().catch((err) => | |
| console.error('[AudioPlayer] Auto-play on canplay failed:', err) | |
| ); | |
| targetAudio.removeEventListener('canplay', autoPlayHandler); | |
| }; | |
| this.pendingAutoPlayHandler = autoPlayHandler; | |
| this.pendingAutoPlayElement = targetAudio; |
🤖 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/renderer/src/other/player.ts` around lines 550 - 560, The pending
auto-play cleanup in loadSong is removing the listener from the current
targetAudio even when the handler was registered on a different element. Update
the pendingAutoPlayHandler tracking in AudioPlayer so it stores both the handler
and the specific audio element it was attached to, and remove the listener from
that stored element before replacing it. Make sure the canplay callback still
clears only its own registration and does not interfere with a newer
pendingAutoPlayHandler after completeCrossfade or gaplessSwapToNext swaps
activeElement.
In-scope fixes only (files added/modified by this feature):
search.ts: include lyrics.length in no-results fallback so searchForAvailableResults
is not triggered when lyric matches exist (Major).
lyricsIndex.ts: batch backfill in chunks of 10 (Promise.allSettled) for large
libraries; extract nested ternary into if/else (Sonar Major); use node: prefix
on fs/promises and path imports (Sonar Minor).
LyricSearchResultsContainer + AllLyricResults: extract shared HighlightedSnippet
component (removes duplication); use t('common.lyric_other') instead of hardcoded
'Lyrics' heading; remove unused searchInput useMemo dep; reorder imports to
external -> alias -> relative; stable keys (hl-/tx- prefix) instead of bare index.
Skipped (out of scope / pre-existing, not in this feature's diff):
UpNextSongPopup race, discord.ts lastPayload, lastFm/*, albums sort hook,
getSongLyrics extension strip, sendAudioData, ipc.ts, flushScrobbleQueue,
saveLyricsToSong timestamp, vi.json, player.ts, parseChangelog cognitive
complexity.
|
/agentic_review |
|
@CodeRabbit check the new commits in a scope to make sure everything is visually correct, wiont cause future issues with this lyric search feature, any missing or wrong edge cases, and not well thought out implementation, then also check the sonarcube issues to see i any are still relating in scope |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 2318165 |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Changed files relevant to lyric search/search UI/main search =="
git diff --name-only --diff-filter=ACMR origin/master...HEAD | rg '^(src/main/(db/queries/(lyricsIndex|search)\.ts|search\.ts|parseSong/parseSong\.ts|core/(saveLyricsToLrcFile|getSongLyrics)\.ts|main\.ts)|src/renderer/src/(components/SearchPage|routes/main-player/search|utils/parseChangelog\.ts|components/ReleaseNotesPrompt/VersionNote\.tsx)|src/types/app\.d\.ts|resources/drizzle/(0006_many_nightshade\.sql|meta/_journal\.json)|src/main/db/schema\.ts)$' || true
echo
echo "== SonarCloud issues for PR `#6` =="
curl -s "https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true" \
| python3 - <<'PY'
import json,sys
data=json.load(sys.stdin)
for i in data.get("issues",[]):
comp=i.get("component","").split(":",1)[-1]
line=i.get("line","-")
sev=i.get("severity","")
rule=i.get("rule","")
msg=i.get("message","").replace("\n"," ")
print(f"{sev}\t{comp}:{line}\t{rule}\t{msg}")
PY
echo
echo "== AST outlines =="
ast-grep outline src/main/db/queries/lyricsIndex.ts --view expanded || true
ast-grep outline src/main/db/queries/search.ts --view expanded || true
ast-grep outline src/main/search.ts --view expanded || true
ast-grep outline src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx --view expanded || true
ast-grep outline src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx --view expanded || true
ast-grep outline src/renderer/src/routes/main-player/search/index.tsx --view expanded || true
ast-grep outline src/renderer/src/routes/main-player/search/all/index.tsx --view expanded || true
ast-grep outline src/main/db/schema.ts --match 'songLyrics|isLyricIndexBuilt' --view expanded || true
echo
echo "== Focused file slices =="
for f in \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/search.ts \
src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx \
src/renderer/src/routes/main-player/search/index.tsx \
src/renderer/src/routes/main-player/search/all/index.tsx \
src/main/db/schema.ts \
resources/drizzle/0006_many_nightshade.sql \
src/types/app.d.ts
do
echo
echo "===== $f ====="
nl -ba "$f" | sed -n '1,260p'
doneLength of output: 4102 🏁 Script executed: #!/bin/bash
set -u
echo "== SonarCloud status/body preview =="
status=$(curl -sS -L -A "Mozilla/5.0" \
-o /tmp/sonar_pr6.out \
-w "%{http_code}" \
"https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true")
echo "HTTP $status"
echo "--- first 40 lines ---"
sed -n '1,40p' /tmp/sonar_pr6.out || true
echo "--- content-type guess ---"
file /tmp/sonar_pr6.out || true
echo
echo "== Changed lyric-search related files in renderer/main =="
git diff --name-only --diff-filter=ACMR origin/master...HEAD | rg '(^src/main/.*lyrics|^src/main/search\.ts$|^src/main/db/queries/(search|lyricsIndex)\.ts$|^src/main/db/schema\.ts$|^src/renderer/src/.*(Lyric|Lyrics|Search).*\.(ts|tsx)$|^src/types/app\.d\.ts$)' || true
echo
echo "== Outlines for key files =="
ast-grep outline src/main/db/queries/lyricsIndex.ts --view expanded || true
ast-grep outline src/main/db/queries/search.ts --view expanded || true
ast-grep outline src/main/search.ts --view expanded || true
ast-grep outline src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx --view expanded || true
ast-grep outline src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx --view expanded || true
ast-grep outline src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx --view expanded || true
ast-grep outline src/renderer/src/routes/main-player/search/index.tsx --view expanded || true
ast-grep outline src/renderer/src/routes/main-player/search/all/index.tsx --view expanded || true
ast-grep outline src/renderer/src/components/SearchPage/SearchOptions.tsx --view expanded || true
ast-grep outline src/types/app.d.ts --view expanded || true
echo
echo "== Read focused slices =="
for f in \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/search.ts \
src/main/core/getSongLyrics.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/parseSong/parseSong.ts \
src/main/main.ts \
src/main/db/schema.ts \
src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx \
src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/SearchOptions.tsx \
src/renderer/src/routes/main-player/search/index.tsx \
src/renderer/src/routes/main-player/search/all/index.tsx \
src/types/app.d.ts
do
echo
echo "===== $f ====="
nl -ba "$f" | sed -n '1,260p'
doneLength of output: 52686 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_file() {
f="$1"
echo
echo "===== $f ====="
awk '{printf "%6d %s\n", NR, $0}' "$f" | sed -n '1,260p'
}
echo "== Structural searches =="
rg -n -C3 'phraseto_tsquery|ts_headline|lyrics_vector|matchedLyricSnippet|dangerouslySetInnerHTML|<mark|createQueue\(|playSong\(|isLyricIndexBuilt|indexAllLyrics|upsertSongLyrics|upsertSongLyricsFromText' \
src/main src/renderer/src src/types/app.d.ts || true
echo
echo "== Read focused files =="
for f in \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/search.ts \
src/main/core/getSongLyrics.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/parseSong/parseSong.ts \
src/main/main.ts \
src/main/db/schema.ts \
src/renderer/src/components/SearchPage/HighlightedSnippet.tsx \
src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx \
src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/SearchOptions.tsx \
src/renderer/src/routes/main-player/search/index.tsx \
src/renderer/src/routes/main-player/search/all/index.tsx
do
show_file "$f"
doneLength of output: 156039
Overall: the feature is mostly wired up well end-to-end — schema/indexing/query/UI all connect correctly, and I don’t see an obvious visual/XSS problem in the snippet rendering path. The UI side is safer than it looks because What I’d still flag in scope
SonarCloud issues still relevant in scopeOnly one file from the live Sonar list is still directly related to lyric search:
The other current Sonar issues ( Visual / implementation sanity checkWhat looks good:
Bottom lineIf I were prioritizing fixes for this feature, I’d do them in this order:
✅ Action performedReview finished.
|
… + sonar keys - upsertSongLyrics now returns boolean; when both embedded and LRC sources are empty it deletes the stale song_lyrics row (removeSongLyrics) instead of leaving it searchable. Prevents lyric search returning songs whose lyrics were later removed/cleared. - indexAllLyrics tracks indexed (rows written) vs processed (attempted) and logs both, so the count reflects reality for lyrics-less libraries. - HighlightedSnippet keys use content+index (hl-/tx-<index>-<part>) to satisfy Sonar no-array-index-key rule. Out of scope (noted, not fixed this round): isLyricIndexBuilt is a one-time flag, so changing customLrcFilesSaveLocation later won't rebuild existing songs. Acceptable for v1; a settings-watch rebuild can follow separately.
|
@CodeRabbit Act as a senior code reviewer and QA engineer. Perform a comprehensive, deep-dive analysis of the entire pull request, 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. Think proactively and adversarially. Do not just confirm what looks correct. Hunt for mistakes, edge cases, and silent failures across the full request, including:
CRITICAL CROSS-CHECK: Also review against the 12 open SonarCloud issues for this PR (fetched via the public API for Owie6789_Nora, pullRequest=6, sinceLeakPeriod=true). The relevant in-scope ones are:
Provide a detailed report with:
Assume production environment. Be thorough. Do not approve prematurely. |
|
/agentic_review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx (1)
38-69: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
sliceinstead ofmap+filterfor visible results.The current pattern iterates over all results, returns
undefinedfor items beyondnoOfVisibleResults, then filters them out. Usingslice(0, noOfVisibleResults)beforemapavoids the unnecessary iteration and filter pass, which is cleaner and more efficient for large result sets.♻️ Proposed refactor
() => results.length > 0 - ? results - .map((result, index) => { - if (index < noOfVisibleResults) - return ( - <div key={result.song.songId} className="lyric-result-item mb-2"> - <Song - index={index} - isIndexingSongs={preferences?.isSongIndexingEnabled} - title={result.song.title} - artists={result.song.artists} - album={result.song.album} - artworkPaths={result.song.artworkPaths} - duration={result.song.duration} - songId={result.song.songId} - path={result.song.path} - isAFavorite={result.song.isAFavorite} - year={result.song.year} - isBlacklisted={result.song.isBlacklisted} - onPlayClick={handleSongPlayBtnClick} - /> - <HighlightedSnippet snippet={result.matchedLyricSnippet} /> - </div> - ); - return undefined; - }) - .filter((r) => r !== undefined) + ? results.slice(0, noOfVisibleResults).map((result, index) => ( + <div key={result.song.songId} className="lyric-result-item mb-2"> + <Song + index={index} + isIndexingSongs={preferences?.isSongIndexingEnabled} + title={result.song.title} + artists={result.song.artists} + album={result.song.album} + artworkPaths={result.song.artworkPaths} + duration={result.song.duration} + songId={result.song.songId} + path={result.song.path} + isAFavorite={result.song.isAFavorite} + year={result.song.year} + isBlacklisted={result.song.isBlacklisted} + onPlayClick={handleSongPlayBtnClick} + /> + <HighlightedSnippet snippet={result.matchedLyricSnippet} /> + </div> + )) : [],🤖 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/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx` around lines 38 - 69, The `lyricResults` memo in `LyricSearchResultsContainer` is iterating over every item and then filtering out `undefined` just to enforce `noOfVisibleResults`. Refactor the `results.map(...).filter(...)` chain to first limit the array with `slice(0, noOfVisibleResults)` and then map to the JSX, keeping the existing `Song` and `HighlightedSnippet` rendering logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx`:
- Around line 38-69: The `lyricResults` memo in `LyricSearchResultsContainer` is
iterating over every item and then filtering out `undefined` just to enforce
`noOfVisibleResults`. Refactor the `results.map(...).filter(...)` chain to first
limit the array with `slice(0, noOfVisibleResults)` and then map to the JSX,
keeping the existing `Song` and `HighlightedSnippet` rendering logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a346bc9-81eb-4f02-8ac2-4011c1a6fd9a
📒 Files selected for processing (9)
resources/drizzle/0005_lyrics_index.sqlresources/drizzle/meta/0005_snapshot.jsonresources/drizzle/meta/_journal.jsonsrc/main/db/queries/lyricsIndex.tssrc/main/parseSong/parseSong.tssrc/main/search.tssrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsxsrc/renderer/src/components/SearchPage/HighlightedSnippet.tsxsrc/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
💤 Files with no reviewable changes (1)
- resources/drizzle/0005_lyrics_index.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/parseSong/parseSong.ts
- src/main/search.ts
- src/main/db/queries/lyricsIndex.ts
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "===== lyricsIndex.ts ====="
nl -ba src/main/db/queries/lyricsIndex.ts
echo
echo "===== HighlightedSnippet.tsx ====="
fd -t f HighlightedSnippet
nl -ba $(fd -t f HighlightedSnippet.tsx) 2>/dev/null
echo
echo "===== AllLyricResults.tsx ====="
nl -ba src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
echo
echo "===== LyricSearchResultsContainer.tsx ====="
nl -ba src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
echo
echo "===== search.ts (lyrics query) ====="
nl -ba src/main/db/queries/search.ts | sed -n '1,260p'
echo
echo "===== main.ts backfill section ====="
nl -ba src/main/main.ts | sed -n '1,340p'
echo
echo "===== journal.json ====="
cat resources/drizzle/meta/_journal.json
echo
echo "===== scrobble_queue.ts ====="
nl -ba src/main/db/queries/scrobble_queue.ts
echo
echo "===== flushScrobbleQueue.ts ====="
nl -ba src/main/other/lastFm/flushScrobbleQueue.ts
echo
echo "===== useAppLifecycle.tsx lines 220-320 ====="
nl -ba src/renderer/src/hooks/useAppLifecycle.tsx | sed -n '200,330p'
echo
echo "===== schema.ts ====="
nl -ba src/main/db/schema.ts
echo
echo "===== SonarCloud current open issues ====="
curl -s "https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true" \
| python3 -c "
import json,sys
data=json.load(sys.stdin)
for i in data.get('issues',[]):
comp=i.get('component','').split(':',1)[-1]
line=i.get('line','-')
print(f\"{i.get('severity'):8} | {comp}:{line} | {i.get('rule')} | {i.get('message')}\")
"Length of output: 5682 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "===== drizzle migration files list ====="
fd . resources/drizzle -e sql
echo
echo "===== lyricsIndex.ts ====="
cat -n src/main/db/queries/lyricsIndex.ts
echo
echo "===== HighlightedSnippet.tsx ====="
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
echo
echo "===== AllLyricResults.tsx ====="
cat -n src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
echo
echo "===== LyricSearchResultsContainer.tsx ====="
cat -n src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
echo
echo "===== search.ts (full) ====="
cat -n src/main/db/queries/search.ts
echo
echo "===== main.ts (full) ====="
cat -n src/main/main.ts
echo
echo "===== scrobble_queue.ts ====="
cat -n src/main/db/queries/scrobble_queue.ts
echo
echo "===== flushScrobbleQueue.ts ====="
cat -n src/main/other/lastFm/flushScrobbleQueue.ts
echo
echo "===== useAppLifecycle.tsx (full) ====="
cat -n src/renderer/src/hooks/useAppLifecycle.tsx
echo
echo "===== schema.ts (full) ====="
cat -n src/main/db/schema.ts
echo
echo "===== getSongLyrics.ts ====="
cat -n src/main/core/getSongLyrics.ts
echo
echo "===== saveLyricsToLrcFile.ts ====="
cat -n src/main/core/saveLyricsToLrcFile.ts
echo
echo "===== saveLyricsToSong.ts ====="
cat -n src/main/saveLyricsToSong.ts
echo
echo "===== parseSong.ts (relevant) ====="
cat -n src/main/parseSong/parseSong.tsLength of output: 182719 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "===== getAllSongs signature/defaults ====="
rg -n -B2 -A40 'export const getAllSongs' src/main/db/queries/songs.ts | head -100Length of output: 2882
Overall verdict: Not fully ready — Conditional Ready (Medium-High confidence)The lyric-search feature and the surrounding scrobble-queue/migration/main-process work are structurally sound and I found no security holes or data-corruption bugs. But there's one real functional gap that will cause silent, permanent search gaps for some users, plus a few lower-severity robustness issues worth fixing before wide rollout. Bugs/gaps found (excluding prior feedback)Major
Minor Confirmed working correctly (no action needed)
SonarCloud cross-check — all 4 flagged items are resolved ✅I re-pulled the live SonarCloud issue list (
The 12 issues currently still open on Sonar are all unrelated to the lyric-search scope ( Bottom lineIf you want this shippable with zero known bugs for the lyric-search feature specifically, I'd fix ✅ Action performedReview finished.
|
|
Code review by qodo was updated up to the latest commit 383f2d7 |
… perf, slice
- lyricsIndex.ts: read helpers now return tri-state (string | undefined |
null). undefined = definitively absent, null = read/parse error.
upsertSongLyrics only deletes the song_lyrics row on definitive absence;
on read error it keeps the existing entry and logs (no more wiping a good
index on transient I/O/tag failures).
- parseSong.ts: call upsertSongLyrics(id, path) unconditionally on import
instead of gating on metadata.lyrics, so pre-existing .lrc sidecars added
after the one-time backfill still get indexed.
- indexAllLyrics: use lightweight db.select({id, path}) instead of
getAllSongs({}) to avoid eager relational joins during backfill.
- LyricSearchResultsContainer: slice(0, n).map instead of map+filter(undefined)
per CR nitpick.
|
@CodeRabbit Act as a senior code reviewer and QA engineer. Perform a comprehensive, deep-dive analysis of the entire pull request, 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. Think proactively and adversarially. Do not just confirm what looks correct. Hunt for mistakes, edge cases, and silent failures across the full request, including:
CRITICAL CROSS-CHECK: Also review against the 12 open SonarCloud issues for this PR (fetched via the public API for Owie6789_Nora, pullRequest=6, sinceLeakPeriod=true). The relevant in-scope ones are:
Provide a detailed report with:
Assume production environment. Be thorough. Do not approve prematurely. |
|
/agentic_review |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Quick recap of that report's conclusion:
If you've pushed new commits since that report, let me know (or just push and re-trigger (=^･ᆽ･^=) ﾉ ✅ Action performedReview finished.
|
Owie6789
force-pushed
the
feat/lyrics-search
branch
from
ecbfd99 to
eb61367
Compare
|
|
@CodeRabbit Act as a senior code reviewer and QA engineer. Perform a comprehensive, deep-dive analysis of the entire pull request, 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. Think proactively and adversarially. Do not just confirm what looks correct. Hunt for mistakes, edge cases, and silent failures across the full request, including:
CRITICAL CROSS-CHECK: Also review against the 12 open SonarCloud issues for this PR (fetched via the public API for Owie6789_Nora, pullRequest=6, sinceLeakPeriod=true). The relevant in-scope ones are:
Provide a detailed report with:
Assume production environment. Be thorough. Do not approve prematurely. |
|
/agentic_review |
Qodo is busy workingCheck back in a few minutes. Qodo's code review agents are on it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Latest commits on branch:"
git log --oneline -10
echo
echo "Diff stat vs previous check (if any new commits):"
git diff --stat origin/master...HEAD | tail -5Length of output: 1738 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===== Commit eb61367 changes ====="
git show --stat eb61367
echo
echo "===== parseSong.ts current state ====="
rg -n -B3 -A10 'upsertSongLyrics' src/main/parseSong/parseSong.ts
echo
echo "===== lyricsIndex.ts current full ====="
cat -n src/main/db/queries/lyricsIndex.ts
echo
echo "===== main.ts backfill section ====="
rg -n -B5 -A20 'isLyricIndexBuilt' src/main/main.ts
echo
echo "===== songs.ts getAllSongs relevant slice used by backfill ====="
rg -n -B3 -A5 'export const getAllSongs' src/main/db/queries/songs.ts
echo
echo "===== SonarCloud current open issues ====="
curl -s "https://sonarcloud.io/api/issues/search?componentKeys=Owie6789_Nora&pullRequest=6&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true" \
| python3 -c "
import json,sys
data=json.load(sys.stdin)
for i in data.get('issues',[]):
comp=i.get('component','').split(':',1)[-1]
line=i.get('line','-')
print(f\"{i.get('severity'):8} | {comp}:{line} | {i.get('rule')} | {i.get('message')}\")
"Length of output: 14357 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===== LyricSearchResultsContainer.tsx current ====="
cat -n src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
echo
echo "===== HighlightedSnippet.tsx current ====="
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
echo
echo "===== AllLyricResults.tsx current ====="
cat -n src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
echo
echo "===== getSongByPath usage check (race on concurrent import) ====="
rg -n 'getSongByPath|isSongWithPathAvailable' src/main/db/queries/songs.ts | head -20Length of output: 9914
Updated verdict: Ready for testing (High confidence)What changed since my last report
SonarCloud cross-check — all 4 in-scope issues confirmed resolved, and no lyric-search issues remain openRe-pulled the live SonarCloud data (13 open issues total now). None of them touch the lyric-search code paths:
The 13 issues still open on Sonar are all unrelated to this feature: Remaining non-blocking items (unchanged from before, still Minor/hardening-tier)
None of these are blockers for testing — they're fine as fast-follow hardening. Confirmed working correctly
Nice turnaround on the fixes — this is in solid shape for QA now. 🐰✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation