Conversation
…, UI filter, and XSS-safe snippets
Feature:
- New song_lyrics table with GIN tsvector index (phraseto_tsquery search)
- indexAllLyrics backfill on startup (one-time isLyricIndexBuilt flag), now
lightweight db.select({id, path}) instead of heavy getAllSongs join
- parseSong indexes both embedded + .lrc on import (unconditional, not just
embedded)
- searchSongsByLyrics wired into search flow; no-results fallback includes
lyrics.length
- UI: LyricSearchResultsContainer + AllLyricResults with XSS-safe
HighlightedSnippet (text nodes + <mark>, no dangerouslySetInnerHTML)
- i18n key common.lyric_other for headings
- Migration 0005_lyrics_index.sql, sequential append-only journal (idx 0-5)
Review fixes (CR + SonarCloud, all in-scope):
- upsertSongLyrics returns boolean; tri-state read helpers (string | undefined
| null) so transient I/O errors keep the existing index row instead of
wiping it. removeSongLyrics only on definitive absence.
- Backfill counter tracks indexed (wrote) vs processed (attempted) accurately
- Extracted HighlightedSnippet to shared component; stable keys (no array index)
- Import reorder (external -> alias -> relative); node: import prefix
- slice(0, n).map instead of map+filter(undefined)
Closes Sandakan#527
…ar dep at load time - Changed parseSong.ts static import of lyricsIndex to dynamic import() so it doesn't trigger circular dependency on app startup - Fixed main.ts import path from relative to @main alias for consistency
|
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:
📝 WalkthroughWalkthroughAdds database-backed lyric indexing and phrase search, integrates indexing into startup and song-save flows, and renders lyric results with snippets in search views. It also adds crossfade playback typing and settings translations. ChangesLyrics search feature
Crossfade metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SearchModal
participant MainSearch
participant LyricsQuery
participant LyricsDatabase
participant SearchResults
SearchModal->>MainSearch: submit keyword
MainSearch->>LyricsQuery: searchSongsByLyrics(options)
LyricsQuery->>LyricsDatabase: execute phrase query and rank matches
LyricsDatabase-->>LyricsQuery: songs and highlighted snippets
LyricsQuery-->>MainSearch: lyrics results
MainSearch-->>SearchResults: render lyric result collection
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
…Line type Removes the local ParsedLine type alias and uses the canonical LyricLine from app.d.ts. Removes redundant casts at call sites and in extractPlainText body.
|
@CodeRabbit with an aggressive profile, audit for performance in very large libraries and come back w a proper report to reduce any form of performance loss while being able to fetch lyrics match in sub milliseconds and fast app loading time, with all the best practices, with no code regression |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== drizzle migration ==="
fd -a '0005_lyrics_index.sql' | xargs -I{} cat -n {}
echo "=== schema.ts (songLyrics section) ==="
fd -a 'schema.ts' -p src/main/db | xargs -I{} sh -c 'grep -n "songLyrics\|tsvector\|isLyricIndexBuilt\|is_lyric_index_built" -n {} -A5 -B5'
echo "=== lyricsIndex.ts ==="
fd -a 'lyricsIndex.ts' | xargs -I{} cat -n {}
echo "=== search.ts (queries) ==="
fd -a 'search.ts' -p src/main/db/queries | xargs -I{} cat -n {}
echo "=== main/search.ts ==="
fd -a 'search.ts' -p src/main | xargs -I{} cat -n {}Length of output: 36558 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== main.ts indexAllLyrics trigger ==="
rg -n "indexAllLyrics|isLyricIndexBuilt" src/main/main.ts -B5 -A15
echo "=== parseSong.ts lyrics indexing ==="
rg -n "lyricsIndex|upsertSongLyrics" src/main/parseSong/parseSong.ts -B5 -A10
echo "=== saveLyricsToLrcFile.ts ==="
fd -a 'saveLyricsToLrcFile.ts' | xargs -I{} cat -n {}
echo "=== saveLyricsToSong.ts ==="
fd -a 'saveLyricsToSong.ts' | xargs -I{} cat -n {}
echo "=== db.ts pool config ==="
fd -a 'db.ts' -p src/main/db | xargs -I{} cat -n {}
echo "=== getSongLyrics.ts getLrcFilePaths ==="
rg -n "getLrcFilePaths" src/main/core/getSongLyrics.ts -A20Length of output: 21232 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== search debounce / keyword length gating in renderer ==="
rg -n "debounce|search\(" src/renderer/src/routes/main-player/search/index.tsx -B3 -A10 | head -100
echo "=== check where search() is invoked from main/ipc ==="
rg -n "search\(" src/main -g '!*.test.ts' | grep -v "search.ts:" | head -30
echo "=== withFileHandle implementation (sync/async, native binding) ==="
fd -a 'withFileHandle.ts' | xargs -I{} cat -n {}
echo "=== check any min length gate for search keyword ==="
rg -n "keyword.length|MIN_.*LENGTH|minLength" src/renderer/src -iLength of output: 954 🏁 Script executed: #!/bin/bash
set +e
echo "=== search debounce / keyword length gating in renderer ==="
rg -n "debounce|search\(" src/renderer/src/routes/main-player/search/index.tsx -B3 -A10 | head -100
echo "=== check where search() default import is invoked ==="
rg -n "import search|search(" src/main/ipc.ts 2>/dev/null | head -20
fd -a 'ipc.ts' -p src/main | xargs -I{} rg -n "search" {} -B2 -A8
echo "=== withFileHandle implementation ==="
fd -a 'withFileHandle.ts' | xargs -I{} cat -n {}
echo "=== min length gate for search keyword ==="
rg -n "keyword.length|MIN_.*LENGTH|minLength" src/renderer/src -iLength of output: 5217 🤖 web_search executed:
Length of output: 10251 🏁 Script executed: #!/bin/bash
set +e
echo "=== getUserSettings implementation ==="
fd -a 'settings.ts' -p src/main/db/queries | xargs -I{} sed -n '1,60p' {}Length of output: 783
🔴 Critical — N+1 DB round-trip in the backfill (biggest win)
const readLrcLyrics = async (songPath: string): Promise<LyricReadResult> => {
const { customLrcFilesSaveLocation } = await getUserSettings(); // <-- DB hit, every call
...This is called once per song inside This matters especially here because PGlite runs in Postgres single-user mode = a single connection. I confirmed via web search: concurrent calls from the same PGlite instance are queued/serialized, not parallelized (https://electric.ax/blog/2026/03/25/announcing-pglite-v04, electric-sql/pglite#324). So every one of those 20,000 calls is fully serial — there's no overlap to hide the latency. Fix: hoist 🔴 Critical — backfill does per-row inserts instead of batched upserts
Fix: wrap each batch in one 🟠 High —
|
…ed name Rename 0005_lyrics_index.sql back to 0005_many_nightshade.sql (drizzle-kit random name) and update journal tag accordingly. Descriptive names are not drizzle-kit convention and would be flagged in review.
…h + fix snapshot naming - Fix N+1 DB round-trip: getUserSettings() was called per-song in readLrcLyrics, now hoisted to indexAllLyrics and threaded through upsertSongLyrics. Removes ~10k redundant SELECT queries for a 10k-song library. - Rename snapshot back to 0005_snapshot.json (drizzle-kit convention, not descriptive names).
|
@CodeRabbit full review of this PR. focus on correctness, edge cases, and whether anything will break at runtime. skip anything already flagged in prior reviews. context on decisions i made (correct me if wrong):
check: types, imports, i18n, XSS safety of snippet renderer, drizzle schema validity, migration naming (0005_many_nightshade.sql is drizzle-kit generated random name), and any runtime bugs. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR diff summary ==="
git diff --stat HEAD^ HEAD
git diff --name-status HEAD^ HEAD
echo "=== Changed TypeScript/TSX outlines ==="
for f in \
src/main/db/schema.ts \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/search.ts \
src/main/parseSong/parseSong.ts \
src/main/main.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.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 \
src/types/app.d.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
done
echo "=== Full PR patch ==="
git diff --find-renames --find-copies HEAD^ HEAD -- \
resources/drizzle src/main src/renderer/src src/types/app.d.ts
echo "=== Drizzle migrations and journal ==="
fd -a . resources/drizzle | sort
fd -a 'journal.json' resources/drizzle -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}
echo "=== Search IPC/type call sites and existing search UI contracts ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(LyricsSearchResult|searchSongsByLyrics|SearchFilters|search.*lyrics|lyrics.*search|SearchOptions)' \
src types
echo "=== Lyrics helpers and save/import call sites ==="
rg -n -C4 --glob '*.ts' \
'(upsertSongLyrics|indexAllLyrics|readLrcLyrics|readEmbeddedLyrics|getSongLyrics|saveLyricsToLrcFile|saveLyricsToSong)' \
src/main
echo "=== Relevant tests ==="
fd -a -e ts -e tsx . | rg -i '(search|lyric|parseSong|saveLyrics|schema|migration|db).*(test|spec)|((test|spec).*\.(ts|tsx))'Length of output: 62707 🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref=""
for candidate in origin/main origin/master main master; do
if git rev-parse --verify -q "$candidate" >/dev/null; then
base_ref="$candidate"
break
fi
done
if [ -z "$base_ref" ]; then
echo "Could not determine base branch"; git branch -a; exit 0
fi
merge_base="$(git merge-base HEAD "$base_ref")"
echo "=== Base ref: $base_ref; merge base: $merge_base ==="
git diff --stat "$merge_base" HEAD
git diff --name-status "$merge_base" HEAD
echo "=== Complete PR diff ==="
git diff --find-renames "$merge_base" HEAD -- \
resources/drizzle src/main src/renderer/src src/types/app.d.ts src/preload
echo "=== Current focused implementations ==="
for spec in \
"src/main/db/queries/lyricsIndex.ts:1,230" \
"src/main/db/queries/search.ts:215,310" \
"src/main/search.ts:1,140" \
"src/main/parseSong/parseSong.ts:80,260" \
"src/main/main.ts:1,70" \
"src/main/main.ts:350,470" \
"src/main/core/saveLyricsToLrcFile.ts:115,220" \
"src/main/saveLyricsToSong.ts:1,130" \
"src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,160" \
"src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,220" \
"src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,180" \
"src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,120" \
"src/renderer/src/components/SearchPage/SearchOptions.tsx:1,100" \
"src/renderer/src/routes/main-player/search/index.tsx:50,270" \
"src/renderer/src/routes/main-player/search/all/index.tsx:1,145" \
"src/types/app.d.ts:840,880" \
"src/preload/index.ts:185,210" \
"src/renderer/src/queries/search.ts:1,80" \
"src/renderer/src/utils/zod/searchPageSchema.ts:1,80"
do
file="${spec%%:*}"; range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file ($range) ---"
sed -n "$range" "$file"
fi
done
echo "=== Migration, schema, config, and migration execution ==="
cat -n resources/drizzle/0005_many_nightshade.sql
sed -n '1,80p' resources/drizzle/meta/0005_snapshot.json
rg -n -C4 --glob '*.{ts,js,json}' \
'(drizzle|migrate|_journal|resources/drizzle|migration)' \
drizzle.config.* package.json src resources
echo "=== Locale keys and analogous result containers ==="
rg -n -C2 '"lyric_other"|lyric_other|search.*lyrics' src/renderer/src/assets/locales
for f in \
src/renderer/src/components/SearchPage/Result_Containers/SongSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllSongResults.tsx
do
[ -f "$f" ] && { echo "--- $f ---"; sed -n '1,220p' "$f"; }
done
echo "=== Callers of the changed function signatures ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'\b(upsertSongLyrics|upsertSongLyricsFromText|removeSongLyrics|indexAllLyrics)\s*\(' \
src
echo "=== Existing test configuration and relevant tests ==="
fd -a -e ts -e tsx . | rg -i '(^|/)(test|tests|__tests__|spec)(/|$)|\.(test|spec)\.(ts|tsx)$'
rg -n -C2 --glob '*.{json,ts,js}' '(vitest|jest|test:|lint|typecheck)' package.json .github 2>/dev/null || trueLength of output: 119647 🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref="$(git rev-parse --verify -q origin/main || git rev-parse --verify -q main || git rev-parse --verify -q origin/master || git rev-parse --verify master)"
merge_base="$(git merge-base HEAD "$base_ref")"
echo "=== Changed file inventory (complete PR) ==="
git diff --name-status "$merge_base" HEAD | sed -n '1,500p'
echo "=== Patch whitespace/conflict-marker checks ==="
git diff --check "$merge_base" HEAD || true
rg -n --glob '!node_modules/**' '^(<<<<<<<|=======|>>>>>>>)' . || true
print_range() {
local file="$1" range="$2"
echo "--- $file ($range) ---"
sed -n "${range}p" "$file"
}
print_range src/main/db/queries/lyricsIndex.ts '1,210'
print_range src/main/db/queries/search.ts '220,300'
print_range src/main/search.ts '1,130'
print_range src/main/parseSong/parseSong.ts '200,300'
print_range src/main/main.ts '380,460'
print_range src/main/core/saveLyricsToLrcFile.ts '130,210'
print_range src/main/saveLyricsToSong.ts '1,120'
print_range src/main/db/schema.ts '1,45'
print_range src/main/db/schema.ts '400,515'
print_range resources/drizzle/0005_many_nightshade.sql '1,220'
print_range resources/drizzle/meta/_journal.json '35,55'
print_range resources/drizzle/meta/0005_snapshot.json '1,60'
echo "=== Every lyrics-index call/import with context ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'(upsertSongLyrics|upsertSongLyricsFromText|removeSongLyrics|indexAllLyrics)' src
echo "=== Search renderer implementation ==="
print_range src/renderer/src/components/SearchPage/HighlightedSnippet.tsx '1,160'
print_range src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx '1,240'
print_range src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx '1,220'
print_range src/renderer/src/components/SearchPage/SearchOptions.tsx '1,100'
print_range src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx '1,130'
print_range src/renderer/src/routes/main-player/search/index.tsx '55,250'
print_range src/renderer/src/routes/main-player/search/all/index.tsx '35,135'
print_range src/renderer/src/queries/search.ts '1,90'
print_range src/renderer/src/utils/zod/searchPageSchema.ts '1,90'
echo "=== Locale validity and required keys across loaded locales ==="
python3 - <<'PY'
import json, pathlib
files = sorted(pathlib.Path("src/renderer/src/assets/locales").glob("*/*.json"))
for f in files:
try:
data=json.loads(f.read_text())
common=data.get("common", {})
print(f"{f}: valid; common.lyric_other={common.get('lyric_other')!r}")
except Exception as e:
print(f"{f}: INVALID JSON: {e}")
PY
echo "=== Drizzle config and migration executor ==="
fd -a -E node_modules 'drizzle.config.*' . -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh {}
rg -n -C4 --glob '*.{ts,js,json}' \
'(migrate\(|drizzle-orm.*migrat|resources/drizzle|migrationsFolder)' \
src package.json electron.vite.config.* drizzle.config.* 2>/dev/null || true
echo "=== Imports and route creation from newly changed TS/TSX files ==="
git diff --name-only --diff-filter=AM "$merge_base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r f; do
echo "--- $f ---"
sed -n '1,45p' "$f"
doneLength of output: 105871 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== lyricsIndex.ts ==="
cat -n src/main/db/queries/lyricsIndex.ts
echo "=== lyric search query and search orchestration ==="
sed -n '215,290p' src/main/db/queries/search.ts | cat -n
sed -n '1,115p' src/main/search.ts | cat -n
echo "=== all direct lyrics index callers (with full local context) ==="
rg -n -C8 --glob '*.{ts,tsx}' '\b(upsertSongLyrics|upsertSongLyricsFromText|removeSongLyrics|indexAllLyrics)\s*\(' src
echo "=== index lifecycle placement ==="
rg -n -C10 --glob '*.ts' 'indexAllLyrics|isLyricIndexBuilt|is_lyric_index_built' src/main
echo "=== schema and migration lyrics/settings declarations ==="
sed -n '400,510p' src/main/db/schema.ts | cat -n
cat -n resources/drizzle/0005_many_nightshade.sql
echo "--- journal entry ---"
sed -n '38,50p' resources/drizzle/meta/_journal.json | cat -n
echo "--- prior snapshot naming ---"
fd -a '_snapshot.json' resources/drizzle/meta | sort
echo "=== lyrics UI components in full ==="
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
cat -n src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
cat -n src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
sed -n '1,130p' src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx | cat -n
sed -n '90,250p' src/renderer/src/routes/main-player/search/index.tsx | cat -n
sed -n '35,135p' src/renderer/src/routes/main-player/search/all/index.tsx | cat -n
echo "=== search filter and schema ==="
cat -n src/renderer/src/components/SearchPage/SearchOptions.tsx
cat -n src/renderer/src/utils/zod/searchPageSchema.ts
cat -n src/renderer/src/queries/search.ts
echo "=== relevant helper contracts ==="
rg -n -C8 --glob '*.ts' \
'(export const getLrcFilePaths|const getLrcFilePaths|export default.*withFileHandle|export const withFileHandle|function withFileHandle|saveLyricsToLRCFile)' \
src/main
echo "=== locale key status ==="
for f in src/renderer/src/assets/locales/*/*.json; do
printf '%s: ' "$f"
jq -r '.common.lyric_other // "<missing>"' "$f"
done
echo "=== changed-code diagnostics: suspicious positionals/undefined fields ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(new URL\(songData\.path\)|changePlayerType\(|registerUpNextPopupFn|showUpNextSongPopup|resetErrorCount|MEDIA_ERR_SRC_NOT_SUPPORTED)' \
src/renderer/src src/mainLength of output: 89984 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Filter behavior in analogous search result containers ==="
for f in \
src/renderer/src/components/SearchPage/Result_Containers/SongSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/Result_Containers/ArtistsSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/Result_Containers/AlbumSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/Result_Containers/MostRelevantSearchResultsContainer.tsx \
src/renderer/src/components/SearchPage/SearchResultsFilter.tsx
do
echo "--- $f ---"
sed -n '1,260p' "$f"
done
echo "=== Full index completion and save-lyrics paths ==="
sed -n '270,305p' src/main/main.ts
sed -n '1,145p' src/main/saveLyricsToSong.ts
sed -n '115,180p' src/main/core/saveLyricsToLrcFile.ts
echo "=== Existing automatic backfill/reset mechanisms ==="
rg -n -C5 --glob '*.ts' \
'(isLyricIndexBuilt|is_lyric_index_built|removeSongLyrics|lyrics_text|song_lyrics)' \
src test
echo "=== Taglib declaration/package evidence ==="
rg -n -C3 --glob '*.{json,d.ts,ts}' \
'(node-taglib-sharp|createFromPath)' package.json src test | sed -n '1,240p'Length of output: 58668
Findings
Validated items
✏️ Learnings added
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
Plus validation that XSS handling in If you'd like, I can open follow-up issues for findings 1–3 so they're tracked separately from this PR — just say which ones. Otherwise, let me know if there's anything specific beyond that report you want re-checked. (=^･ω･^=) already on it! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/main/db/queries/search.ts (1)
232-242: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDeduplicate
phraseto_tsqueryevaluation.
phraseto_tsquery('simple', ${keyword})is evaluated three times per query (select, where, orderBy). While PostgreSQL may cache it within a single statement, using a CTE or lateral join to compute it once is cleaner and avoids redundant parsing.🤖 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/search.ts` around lines 232 - 242, Update the query around the results selection to compute phraseto_tsquery('simple', keyword) once via a CTE or lateral join, then reuse that computed value in the snippet, where, and orderBy expressions. Preserve the existing search filtering, ranking order, and result limit.
🤖 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/core/getSongLyrics.ts`:
- Line 27: The LRC path builders in getSongLyrics must remove only the song
file’s trailing extension, not matching text elsewhere in the path. Update the
default path construction at src/main/core/getSongLyrics.ts:27 to use
suffix-based removal, and update the custom path construction at
src/main/core/getSongLyrics.ts:34 to use path.basename with the extension
argument.
In `@src/main/core/saveLyricsToLrcFile.ts`:
- Around line 158-167: Update the lyrics indexing flow in saveLyricsToLrcFile to
use upsertSongLyrics instead of upsertSongLyricsFromText. Re-read the song’s
existing LRC and embedded lyrics, aggregate both sources while preserving
whichever source is not being updated, then upsert the combined lyrics so
indexing one source cannot remove the other.
In `@src/main/db/queries/lyricsIndex.ts`:
- Around line 144-156: The indexAllLyrics backfill currently logs per-song
failures but still allows callers to mark the index as built. Update
indexAllLyrics to return a success signal or failure count based on rejected
results, then update its caller in main.ts to persist isLyricIndexBuilt only
when every song indexes successfully.
In `@src/main/db/queries/search.ts`:
- Around line 225-251: Wrap the database query and result mapping in
searchSongsByLyrics with error handling so FTS failures do not reject the
surrounding Promise.all. Log the failure using the existing project logging
approach and return an empty result list as the safe fallback, while preserving
timing and successful-result behavior.
In `@src/main/main.ts`:
- Around line 293-297: The lyrics backfill flow currently marks
isLyricIndexBuilt complete whenever indexAllLyrics resolves, even when
individual indexing operations fail. Update indexAllLyrics to return a result
containing failure information, then change the main.ts promise chain to call
saveUserSettings({ isLyricIndexBuilt: true }) only when no failures occurred;
otherwise retain the unset flag and log an appropriate warning.
In `@src/main/parseSong/parseSong.ts`:
- Around line 272-276: Update the lyrics indexing call in parseSong to pass the
configured customLrcFilesSaveLocation to upsertSongLyrics, and import and use
getUserSettings to obtain that setting before invoking it. Preserve the existing
song ID, path, and error logging behavior.
In `@src/main/saveLyricsToSong.ts`:
- Line 119: Update the save flow around upsertSongLyricsFromText so embedded
lyrics do not replace existing LRC-indexed content. Preserve both sources by
applying the chosen aggregation approach consistently with the song_lyrics
schema: either support separate rows keyed by songId and source, or retrieve and
merge existing lyrics text before updating the single songId row while retaining
source information.
---
Nitpick comments:
In `@src/main/db/queries/search.ts`:
- Around line 232-242: Update the query around the results selection to compute
phraseto_tsquery('simple', keyword) once via a CTE or lateral join, then reuse
that computed value in the snippet, where, and orderBy expressions. Preserve the
existing search filtering, ranking order, and result limit.
🪄 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: cbdf9f75-ef8b-4872-8f7d-694bcf71e5cd
📒 Files selected for processing (21)
resources/drizzle/0005_many_nightshade.sqlresources/drizzle/meta/0005_snapshot.jsonresources/drizzle/meta/_journal.jsonsrc/main/core/getSongLyrics.tssrc/main/core/saveLyricsToLrcFile.tssrc/main/db/queries/lyricsIndex.tssrc/main/db/queries/search.tssrc/main/db/schema.tssrc/main/main.tssrc/main/parseSong/parseSong.tssrc/main/saveLyricsToSong.tssrc/main/search.tssrc/renderer/src/assets/locales/en/en.jsonsrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsxsrc/renderer/src/components/SearchPage/HighlightedSnippet.tsxsrc/renderer/src/components/SearchPage/NoSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/SearchOptions.tsxsrc/renderer/src/routes/main-player/search/all/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/types/app.d.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/renderer/src/routes/main-player/search/index.tsxsrc/main/parseSong/parseSong.tssrc/renderer/src/components/SearchPage/HighlightedSnippet.tsxsrc/renderer/src/components/SearchPage/SearchOptions.tsxsrc/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsxsrc/types/app.d.tssrc/main/db/schema.tssrc/main/saveLyricsToSong.tssrc/main/core/getSongLyrics.tssrc/main/main.tssrc/renderer/src/components/SearchPage/NoSearchResultsContainer.tsxsrc/renderer/src/routes/main-player/search/all/index.tsxsrc/main/db/queries/search.tssrc/main/search.tssrc/main/db/queries/lyricsIndex.tssrc/main/core/saveLyricsToLrcFile.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/search/index.tsxsrc/renderer/src/components/SearchPage/HighlightedSnippet.tsxsrc/renderer/src/components/SearchPage/SearchOptions.tsxsrc/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsxsrc/renderer/src/components/SearchPage/NoSearchResultsContainer.tsxsrc/renderer/src/routes/main-player/search/all/index.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/search/index.tsxsrc/renderer/src/components/SearchPage/HighlightedSnippet.tsxsrc/renderer/src/components/SearchPage/SearchOptions.tsxsrc/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsxsrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsxsrc/renderer/src/components/SearchPage/NoSearchResultsContainer.tsxsrc/renderer/src/routes/main-player/search/all/index.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/parseSong/parseSong.tssrc/main/db/schema.tssrc/main/saveLyricsToSong.tssrc/main/core/getSongLyrics.tssrc/main/main.tssrc/main/db/queries/search.tssrc/main/search.tssrc/main/db/queries/lyricsIndex.tssrc/main/core/saveLyricsToLrcFile.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/core/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Every core function must call dataUpdateEvent() after modifying database state to notify the renderer of changes via IPC
Files:
src/main/core/getSongLyrics.tssrc/main/core/saveLyricsToLrcFile.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/search.tssrc/main/db/queries/lyricsIndex.ts
🔇 Additional comments (18)
src/types/app.d.ts (2)
556-556: 🩺 Stability & AvailabilityVerify
crossfadeDurationhas a default for existing persistedPlaybackstate.
crossfadeDuration: numberis non-optional, but existing users' persisted playback state likely lacks this field. Runtime access would returnundefineddespite the type claimingnumber, potentially causing NaN in crossfade calculations. Verify that a default value is applied when loading persisted settings.
855-872: LGTM!src/main/search.ts (1)
30-46: LGTM!Also applies to: 86-87, 110-110
src/renderer/src/assets/locales/en/en.json (2)
13-14: LGTM!
388-392: 🎯 Functional CorrectnessConfirm the minimum crossfade value
crossfadeSecondsneeds a_onevariant if the crossfade setting can reach 1 second; otherwise the current key is fine. The range needs to be checked before changing the locale entry.src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx (1)
1-53: LGTM!src/renderer/src/components/SearchPage/HighlightedSnippet.tsx (1)
1-33: LGTM!src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx (1)
18-19: LGTM!Also applies to: 39-45, 47-55
src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx (1)
1-122: LGTM!src/renderer/src/components/SearchPage/SearchOptions.tsx (1)
11-12: LGTM!Also applies to: 25-26
src/renderer/src/routes/main-player/search/all/index.tsx (1)
6-6: LGTM!Also applies to: 77-89, 122-122
src/renderer/src/routes/main-player/search/index.tsx (1)
10-10: LGTM!Also applies to: 227-232
resources/drizzle/0005_many_nightshade.sql (1)
1-12: LGTM!resources/drizzle/meta/_journal.json (1)
40-46: LGTM!src/main/core/getSongLyrics.ts (1)
95-106: LGTM!src/main/saveLyricsToSong.ts (1)
110-122: LGTM on the error handling pattern — catchinggetSongByPathandupsertSongLyricsFromTextfailures independently prevents indexing errors from disrupting the save flow.resources/drizzle/meta/0005_snapshot.json (1)
3025-3109: LGTM!src/main/db/schema.ts (1)
471-503: LGTM!
| 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
replaceAll(path.extname(...)) corrupts paths containing the extension string outside the suffix. Both the default and custom LRC path builders use the same flawed pattern to strip the file extension.
src/main/core/getSongLyrics.ts#L27-L27: ReplacesongPath.replaceAll(path.extname(songPath), '')with suffix-based removal:songPath.slice(0, songPath.length - path.extname(songPath).length).src/main/core/getSongLyrics.ts#L34-L34: Replacepath.basename(songPath.replaceAll(path.extname(songPath), ''))withpath.basename(songPath, path.extname(songPath)).
📍 Affects 1 file
src/main/core/getSongLyrics.ts#L27-L27(this comment)src/main/core/getSongLyrics.ts#L34-L34
🤖 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 LRC path builders in
getSongLyrics must remove only the song file’s trailing extension, not matching
text elsewhere in the path. Update the default path construction at
src/main/core/getSongLyrics.ts:27 to use suffix-based removal, and update the
custom path construction at src/main/core/getSongLyrics.ts:34 to use
path.basename with the extension argument.
| const plainText = extractPlainTextFromParsed(songLyrics.lyrics.parsedLyrics); | ||
| if (plainText) { | ||
| const song = await getSongByPath(songPathWithoutProtocol).catch(() => undefined); | ||
| if (song) { | ||
| const { upsertSongLyricsFromText } = await import('@main/db/queries/lyricsIndex'); | ||
| await upsertSongLyricsFromText(song.id, plainText, 'LRC').catch((error) => | ||
| logger.error('Failed to index LRC lyrics', { error, songId: song.id }) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'upsertSongLyricsFromText|upsertSongLyrics' src/main/saveLyricsToSong.tsRepository: Sandakan/Nora
Length of output: 863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## saveLyricsToLrcFile.ts\n'
ast-grep outline src/main/core/saveLyricsToLrcFile.ts --view expanded || true
sed -n '1,260p' src/main/core/saveLyricsToLrcFile.ts | cat -n
printf '\n## lyricsIndex query\n'
ast-grep outline src/main/db/queries/lyricsIndex.ts --view expanded || true
sed -n '1,260p' src/main/db/queries/lyricsIndex.ts | cat -n
printf '\n## saveLyricsToSong.ts\n'
ast-grep outline src/main/saveLyricsToSong.ts --view expanded || true
sed -n '1,220p' src/main/saveLyricsToSong.ts | cat -nRepository: Sandakan/Nora
Length of output: 20848
Reuse upsertSongLyrics for lyrics indexing. upsertSongLyricsFromText overwrites the existing row with only the latest source, so saving an LRC file or embedded lyrics can drop the other source from the search index. Re-read and aggregate both sources here 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/saveLyricsToLrcFile.ts` around lines 158 - 167, Update the
lyrics indexing flow in saveLyricsToLrcFile to use upsertSongLyrics instead of
upsertSongLyricsFromText. Re-read the song’s existing LRC and embedded lyrics,
aggregate both sources while preserving whichever source is not being updated,
then upsert the combined lyrics so indexing one source cannot remove the other.
| for (let j = 0; j < results.length; j += 1) { | ||
| const result = results[j]; | ||
| if (result.status === 'fulfilled') { | ||
| processed += 1; | ||
| if (result.value) indexed += 1; | ||
| } else logger.error(`Failed to index lyrics for song ${batch[j].id}`, { error: result.reason }); | ||
| } | ||
| } | ||
|
|
||
| logger.info( | ||
| `Lyrics index backfill complete. Indexed ${indexed} songs, processed ${processed} of ${songs.length}.` | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'indexAllLyrics|isLyricIndexBuilt' src/mainRepository: Sandakan/Nora
Length of output: 2377
Backfill needs to gate isLyricIndexBuilt on full success. indexAllLyrics can finish with per-song failures still logged, so src/main/main.ts will persist isLyricIndexBuilt = true and those songs will never be retried. Return a success signal or failure count, and only save the flag when the backfill completes cleanly.
🤖 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 144 - 156, The
indexAllLyrics backfill currently logs per-song failures but still allows
callers to mark the index as built. Update indexAllLyrics to return a success
signal or failure count based on rejected results, then update its caller in
main.ts to persist isLyricIndexBuilt only when every song indexes successfully.
| export const searchSongsByLyrics = async ( | ||
| options: SearchOptions, | ||
| trx: DB | DBTransaction = db | ||
| ) => { | ||
| const { keyword } = options; | ||
| const timer = timeStart(); | ||
|
|
||
| const results = await trx | ||
| .select({ | ||
| song: songs, | ||
| snippet: sql<string>`ts_headline('simple', ${songLyrics.lyricsText}, phraseto_tsquery('simple', ${keyword}), 'MaxWords=12, MinWords=4, ShortWord=2')`, | ||
| source: songLyrics.source | ||
| }) | ||
| .from(songLyrics) | ||
| .innerJoin(songs, eq(songLyrics.songId, songs.id)) | ||
| .where(sql`${songLyrics.lyricsVector} @@ phraseto_tsquery('simple', ${keyword})`) | ||
| .orderBy(sql`ts_rank(${songLyrics.lyricsVector}, phraseto_tsquery('simple', ${keyword})) DESC`) | ||
| .limit(100); | ||
|
|
||
| timeEnd(timer, 'Search Songs By Lyrics'); | ||
|
|
||
| return results.map((result) => ({ | ||
| song: convertToSongData(result.song as GetAllSongsReturnType[number]), | ||
| matchedLyricSnippet: result.snippet, | ||
| source: result.source as 'LRC' | 'EMBEDDED' | 'BOTH' | ||
| })); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add error handling to prevent lyrics search failures from breaking all searches.
searchSongsByLyrics has no try/catch and is called inside Promise.all in search.ts. If the FTS infrastructure fails (e.g., missing extension, migration issue), the entire search rejects and users get zero results. As per coding guidelines, data fetching errors should be handled within queryFn, returning safe defaults instead of throwing.
🛡️ Proposed fix
export const searchSongsByLyrics = async (
options: SearchOptions,
trx: DB | DBTransaction = db
) => {
const { keyword } = options;
const timer = timeStart();
+ try {
const results = await trx
.select({
song: songs,
snippet: sql<string>`ts_headline('simple', ${songLyrics.lyricsText}, phraseto_tsquery('simple', ${keyword}), 'MaxWords=12, MinWords=4, ShortWord=2')`,
source: songLyrics.source
})
.from(songLyrics)
.innerJoin(songs, eq(songLyrics.songId, songs.id))
.where(sql`${songLyrics.lyricsVector} @@ phraseto_tsquery('simple', ${keyword})`)
.orderBy(sql`ts_rank(${songLyrics.lyricsVector}, phraseto_tsquery('simple', ${keyword})) DESC`)
.limit(100);
timeEnd(timer, 'Search Songs By Lyrics');
return results.map((result) => ({
song: convertToSongData(result.song as GetAllSongsReturnType[number]),
matchedLyricSnippet: result.snippet,
source: result.source as 'LRC' | 'EMBEDDED' | 'BOTH'
}));
+ } catch (error) {
+ timeEnd(timer, 'Search Songs By Lyrics');
+ logger.error('Failed to search songs by lyrics', { error, keyword });
+ return [];
+ }
};📝 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.
| export const searchSongsByLyrics = async ( | |
| options: SearchOptions, | |
| trx: DB | DBTransaction = db | |
| ) => { | |
| const { keyword } = options; | |
| const timer = timeStart(); | |
| const results = await trx | |
| .select({ | |
| song: songs, | |
| snippet: sql<string>`ts_headline('simple', ${songLyrics.lyricsText}, phraseto_tsquery('simple', ${keyword}), 'MaxWords=12, MinWords=4, ShortWord=2')`, | |
| source: songLyrics.source | |
| }) | |
| .from(songLyrics) | |
| .innerJoin(songs, eq(songLyrics.songId, songs.id)) | |
| .where(sql`${songLyrics.lyricsVector} @@ phraseto_tsquery('simple', ${keyword})`) | |
| .orderBy(sql`ts_rank(${songLyrics.lyricsVector}, phraseto_tsquery('simple', ${keyword})) DESC`) | |
| .limit(100); | |
| timeEnd(timer, 'Search Songs By Lyrics'); | |
| return results.map((result) => ({ | |
| song: convertToSongData(result.song as GetAllSongsReturnType[number]), | |
| matchedLyricSnippet: result.snippet, | |
| source: result.source as 'LRC' | 'EMBEDDED' | 'BOTH' | |
| })); | |
| }; | |
| export const searchSongsByLyrics = async ( | |
| options: SearchOptions, | |
| trx: DB | DBTransaction = db | |
| ) => { | |
| const { keyword } = options; | |
| const timer = timeStart(); | |
| try { | |
| const results = await trx | |
| .select({ | |
| song: songs, | |
| snippet: sql<string>`ts_headline('simple', ${songLyrics.lyricsText}, phraseto_tsquery('simple', ${keyword}), 'MaxWords=12, MinWords=4, ShortWord=2')`, | |
| source: songLyrics.source | |
| }) | |
| .from(songLyrics) | |
| .innerJoin(songs, eq(songLyrics.songId, songs.id)) | |
| .where(sql`${songLyrics.lyricsVector} @@ phraseto_tsquery('simple', ${keyword})`) | |
| .orderBy(sql`ts_rank(${songLyrics.lyricsVector}, phraseto_tsquery('simple', ${keyword})) DESC`) | |
| .limit(100); | |
| timeEnd(timer, 'Search Songs By Lyrics'); | |
| return results.map((result) => ({ | |
| song: convertToSongData(result.song as GetAllSongsReturnType[number]), | |
| matchedLyricSnippet: result.snippet, | |
| source: result.source as 'LRC' | 'EMBEDDED' | 'BOTH' | |
| })); | |
| } catch (error) { | |
| timeEnd(timer, 'Search Songs By Lyrics'); | |
| logger.error('Failed to search songs by lyrics', { error, keyword }); | |
| return []; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/db/queries/search.ts` around lines 225 - 251, Wrap the database
query and result mapping in searchSongsByLyrics with error handling so FTS
failures do not reject the surrounding Promise.all. Log the failure using the
existing project logging approach and return an empty result list as the safe
fallback, while preserving timing and successful-result behavior.
Source: Coding guidelines
| if (!isLyricIndexBuilt) { | ||
| import('@main/db/queries/lyricsIndex') | ||
| .then(({ indexAllLyrics }) => indexAllLyrics()) | ||
| .then(() => saveUserSettings({ isLyricIndexBuilt: true })) | ||
| .catch((error) => logger.error('Lyrics index backfill failed', { error })); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
isLyricIndexBuilt is set to true even when individual songs fail to index, permanently preventing retries.
indexAllLyrics() uses Promise.allSettled and logs per-song failures without throwing. Since it always resolves, the .then(() => saveUserSettings({ isLyricIndexBuilt: true })) chain unconditionally marks the backfill complete. Songs that failed indexing will never be retried, leaving the search index permanently incomplete.
🔧 Proposed fix: track failures and conditionally set the flag
In indexAllLyrics (lyricsIndex.ts), return a result object so the caller can decide:
-export const indexAllLyrics = async (): Promise<void> => {
+export const indexAllLyrics = async (): Promise<{ failed: number; total: number }> => {
// ... existing batch logic ...
+ const failed = songs.length - processed;
logger.info(
`Lyrics index backfill complete. Indexed ${indexed} songs, processed ${processed} of ${songs.length}.`
);
+ return { failed, total: songs.length };
};Then in main.ts, only set the flag when there are zero failures (or accept partial success with a logged warning):
if (!isLyricIndexBuilt) {
import('`@main/db/queries/lyricsIndex`')
- .then(({ indexAllLyrics }) => indexAllLyrics())
- .then(() => saveUserSettings({ isLyricIndexBuilt: true }))
+ .then(({ indexAllLyrics }) => indexAllLyrics())
+ .then(({ failed, total }) => {
+ if (failed === 0) {
+ return saveUserSettings({ isLyricIndexBuilt: true });
+ }
+ logger.warn(`Lyrics backfill incomplete: ${failed}/${total} songs failed. Will retry next startup.`);
+ })
.catch((error) => logger.error('Lyrics index backfill failed', { 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/main.ts` around lines 293 - 297, The lyrics backfill flow currently
marks isLyricIndexBuilt complete whenever indexAllLyrics resolves, even when
individual indexing operations fail. Update indexAllLyrics to return a result
containing failure information, then change the main.ts promise chain to call
saveUserSettings({ isLyricIndexBuilt: true }) only when no failures occurred;
otherwise retain the unset flag and log an appropriate warning.
| import('@main/db/queries/lyricsIndex') | ||
| .then(({ upsertSongLyrics }) => upsertSongLyrics(res.songData.id, res.songData.path)) | ||
| .catch((error) => | ||
| logger.error('Failed to index lyrics on import', { error, songId: res.songData.id }) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the configured custom LRC directory to indexing.
upsertSongLyrics(res.songData.id, res.songData.path) omits customLrcFilesSaveLocation, so readLrcLyrics only scans the default location. Newly imported songs whose LRC files live in the user's custom directory won't have those lyrics indexed (matches the correctness issue noted in the PR).
🛠️ Proposed fix
import('`@main/db/queries/lyricsIndex`')
- .then(({ upsertSongLyrics }) => upsertSongLyrics(res.songData.id, res.songData.path))
+ .then(async ({ upsertSongLyrics }) => {
+ const { customLrcFilesSaveLocation } = await getUserSettings();
+ return upsertSongLyrics(res.songData.id, res.songData.path, customLrcFilesSaveLocation);
+ })
.catch((error) =>
logger.error('Failed to index lyrics on import', { error, songId: res.songData.id })
);Ensure getUserSettings is imported in this module.
📝 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.
| import('@main/db/queries/lyricsIndex') | |
| .then(({ upsertSongLyrics }) => upsertSongLyrics(res.songData.id, res.songData.path)) | |
| .catch((error) => | |
| logger.error('Failed to index lyrics on import', { error, songId: res.songData.id }) | |
| ); | |
| import('`@main/db/queries/lyricsIndex`') | |
| .then(async ({ upsertSongLyrics }) => { | |
| const { customLrcFilesSaveLocation } = await getUserSettings(); | |
| return upsertSongLyrics(res.songData.id, res.songData.path, customLrcFilesSaveLocation); | |
| }) | |
| .catch((error) => | |
| logger.error('Failed to index lyrics on import', { error, songId: res.songData.id }) | |
| ); |
🤖 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/parseSong/parseSong.ts` around lines 272 - 276, Update the lyrics
indexing call in parseSong to pass the configured customLrcFilesSaveLocation to
upsertSongLyrics, and import and use getUserSettings to obtain that setting
before invoking it. Preserve the existing song ID, path, and error logging
behavior.
| .map((line) => line.replace(/^\[\d+:\d+\.\d+\]/, '').trim()) | ||
| .filter((line) => line.length > 0) | ||
| .join('\n'); | ||
| await upsertSongLyricsFromText(song.id, plainText, 'EMBEDDED').catch((err) => |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Saving embedded lyrics overwrites the entire lyrics index row, losing previously indexed LRC lyrics.
upsertSongLyricsFromText performs onConflictDoUpdate that replaces lyricsText, source, and updatedAt for the existing songId row. If the startup backfill indexed lyrics from an LRC file (source: 'LRC'), saving embedded lyrics here replaces that row entirely with source: 'EMBEDDED'. The LRC content is lost from the search index, and since song_lyrics has a single row per song (PK = songId), there's no way to retain both sources.
🔧 Proposed fix: aggregate lyrics from multiple sources
Option A — Change the PK to (songId, source) so multiple sources coexist:
// schema.ts
- songId: integer('song_id').primaryKey()
+ songId: integer('song_id')
// ...
+ source: varchar('source', { length: 20 }).notNull(),
+ // composite PK in table-level config
+ // (t) => [primaryKey({ columns: [t.songId, t.source] }), index('idx_song_lyrics_vector').using('gin', t.lyricsVector)]Option B — Concatenate new lyrics into existing text instead of replacing:
export const upsertSongLyricsFromText = async (
songId: number,
lyricsText: string,
source: LyricsSource,
trx: typeof db = db
): Promise<void> => {
if (!lyricsText || lyricsText.trim().length === 0) return;
const existing = await trx.query.songLyrics.findFirst({ where: eq(songLyrics.songId, songId) });
if (existing && existing.source !== source) {
// Merge both sources into the indexed text
const mergedText = `${existing.lyricsText}\n${lyricsText}`;
await trx.update(songLyrics).set({ lyricsText: mergedText, updatedAt: new Date() }).where(eq(songLyrics.songId, songId));
} else {
await trx
.insert(songLyrics)
.values({ songId, lyricsText, source })
.onConflictDoUpdate({
target: songLyrics.songId,
set: { lyricsText, source, updatedAt: new Date() }
});
}
};🤖 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/saveLyricsToSong.ts` at line 119, Update the save flow around
upsertSongLyricsFromText so embedded lyrics do not replace existing LRC-indexed
content. Preserve both sources by applying the chosen aggregation approach
consistently with the song_lyrics schema: either support separate rows keyed by
songId and source, or retrieve and merge existing lyrics text before updating
the single songId row while retaining source information.
…ge, search error handling
- indexAllLyrics returns { allSucceeded } so main.ts only sets isLyricIndexBuilt on full success; partial backfills retry next launch
- parseSong passes customLrcFilesSaveLocation to upsertSongLyrics (was missing, custom LRC dirs not indexed on import)
- saveLyricsToLrcFile and saveLyricsToSong now call upsertSongLyrics (re-reads both sources) instead of upsertSongLyricsFromText (overwrites with one source)
- searchSongsByLyrics wrapped in try/catch, returns [] on FTS failure so Promise.all in search.ts doesn't reject
- Removed unused extractPlainTextFromParsed from saveLyricsToLrcFile
|
@CodeRabbit re-review after fixes. all 3 findings addressed:
verify these fixes are correct and check for any remaining issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== lyricsIndex.ts full ==="
cat -n src/main/db/queries/lyricsIndex.ts
echo "=== main.ts backfill block ==="
rg -n "indexAllLyrics|isLyricIndexBuilt|allSucceeded" src/main/main.ts -B5 -A20
echo "=== parseSong.ts lyrics call ==="
rg -n "upsertSongLyrics|customLrcFilesSaveLocation|getUserSettings" src/main/parseSong/parseSong.ts -B5 -A10
echo "=== saveLyricsToLrcFile.ts ==="
cat -n src/main/core/saveLyricsToLrcFile.ts
echo "=== saveLyricsToSong.ts ==="
cat -n src/main/saveLyricsToSong.ts
echo "=== search.ts searchSongsByLyrics ==="
rg -n "searchSongsByLyrics" src/main/db/queries/search.ts -B10 -A40
echo "=== main/search.ts Promise.all ==="
sed -n '1,80p' src/main/search.tsLength of output: 26009 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== getLrcFilePaths ==="
rg -n "getLrcFilePaths" -A25 src/main/core/getSongLyrics.tsLength of output: 2052 This confirms the gap I suspected. Ready to reply.
1. Partial backfill retry — ✅ correct 2. Custom LRC directory on import — ✅ correct 3. Source-preserving re-index on save —
Net effect: when a user has a custom LRC save directory configured, Fix: in 4. Lyrics search failure isolation — ✅ correct Summary
✏️ Learnings added
|
…in save paths CR catch: saveLyricsToLrcFile and savePendingSongLyrics were calling upsertSongLyrics without customLrcFilesSaveLocation, so songs with custom LRC directories would lose their LRC index after save. Both now fetch and pass the setting.
|
@CodeRabbit re-review. saveLyricsToLrcFile and savePendingSongLyrics now pass customLrcFilesSaveLocation to upsertSongLyrics. both settings fetches confirmed at correct scope. verify complete. |
|
@CodeRabbit re-review HEAD. saveLyricsToLrcFile.ts now fetches customLrcFilesSaveLocation via getUserSettings and passes it to upsertSongLyrics at line 148. saveLyricsToSong.ts savePendingSongLyrics now fetches customLrcFilesSaveLocation and passes it at line 115. both confirmed in latest push e232e81. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== saveLyricsToLrcFile.ts full ==="
cat -n src/main/core/saveLyricsToLrcFile.ts
echo "=== saveLyricsToSong.ts full ==="
cat -n src/main/saveLyricsToSong.tsLength of output: 12427
Both call sites now resolve the LRC path consistently with where the file was actually written, closing out finding 3 completely. One minor, non-blocking observation: in All four findings from the correctness review are now fully resolved. No remaining runtime/data-correctness issues found in this PR. (=^･ω･^=) all clear! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== saveLyricsToLrcFile.ts ==="
cat -n src/main/core/saveLyricsToLrcFile.ts
echo "=== saveLyricsToSong.ts ==="
cat -n src/main/saveLyricsToSong.tsLength of output: 12407
This closes the gap from the previous review — all three original findings, plus the follow-up regression on finding 3, are now fully addressed:
One minor, non-blocking observation: No remaining runtime/correctness issues found. This PR is in good shape from my end. ᕦ(ò_óˇ)ᕤ nailed it [resolve] |
…ings in LRC save CR inline finding (minor, getSongLyrics.ts + saveLyricsToLrcFile.ts): replaceAll(path.extname(x), '') corrupts any path containing the extension string outside the suffix (e.g. '/music/rock.mp3/song.mp3' became '/music/rock/song.lrc'). Replaced with suffix-based removal: slice(0, len - extname.len) and basename(path, extname). CR optional observation (saveLyricsToLrcFile.ts): getLrcFileSaveDirectory fetched getUserSettings internally while saveLyricsToLRCFile fetched it again for the re-index call. getLrcFileSaveDirectory is now a pure sync function taking the setting as a parameter, eliminating the redundant DB round-trip per LRC save. Both fixes verified against edge cases (extension in dirname, no extension, custom LRC dir). typecheck clean, lint 0 errors, 336 tests pass.
|
@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
base_ref=""
for candidate in origin/main origin/master main master; do
if git rev-parse --verify -q "$candidate" >/dev/null; then
base_ref="$candidate"
break
fi
done
if [ -z "$base_ref" ]; then
echo "ERROR: No base branch found"
git branch -a
exit 0
fi
merge_base="$(git merge-base HEAD "$base_ref")"
printf 'BASE=%s\nMERGE_BASE=%s\nHEAD=%s\n' "$base_ref" "$merge_base" "$(git rev-parse HEAD)"
echo "=== Complete PR inventory ==="
git diff --name-status "$merge_base" HEAD
echo "=== Diff stat ==="
git diff --stat "$merge_base" HEAD
echo "=== Patch integrity ==="
git diff --check "$merge_base" HEAD || true
echo "=== Repository top-level files ==="
fd -a -d 2 -t f -E node_modules | sort | sed -n '1,260p'
echo "=== Project scripts and dependencies ==="
for f in package.json pnpm-lock.yaml yarn.lock package-lock.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,260p' "$f"
fi
done
echo "=== Changed TypeScript / TSX outlines ==="
git diff --name-only --diff-filter=ACMR "$merge_base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r f; do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo "=== Test, lint, typecheck configuration ==="
fd -a -t f -E node_modules | rg '(^|/)(vitest|jest|playwright|eslint|biome|tsconfig|electron\.vite|vite)\..*|(^|/)(test|tests|__tests__|e2e)(/|$)|\.(test|spec)\.(ts|tsx)$' | sort | sed -n '1,320p'
echo "=== Changed code: IPC, input validation, unsafe rendering, and error handlers ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(ipcMain|ipcRenderer|contextBridge|z\.object|safeParse|parse\(|dangerouslySetInnerHTML|innerHTML|eval\(|new Function|child_process|exec\(|spawn\(|fs\.(writeFile|unlink|rm|rename)|try\s*\{|catch\s*\(|Promise\.(all|allSettled|race)|setTimeout|setInterval)' \
src | sed -n '1,900p'
echo "=== Current PR patch ==="
git diff --find-renames "$merge_base" HEAD -- \
resources/drizzle src/main src/preload src/renderer/src src/types \
| sed -n '1,2200p'Length of output: 85525 🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref=""
for candidate in origin/main origin/master main master; do
if git rev-parse --verify -q "$candidate" >/dev/null; then base_ref="$candidate"; break; fi
done
merge_base="$(git merge-base HEAD "$base_ref")"
echo "=== Changed-file inventory ==="
git diff --name-status "$merge_base" HEAD | cat
echo "=== Migration files and journal: base ==="
git ls-tree -r --name-only "$merge_base" resources/drizzle | sort
echo "--- base journal ---"
git show "$merge_base:resources/drizzle/meta/_journal.json" 2>/dev/null | cat -n || true
echo "=== Migration files and journal: HEAD ==="
fd -a -t f . resources/drizzle | sort
echo "--- HEAD journal ---"
cat -n resources/drizzle/meta/_journal.json
echo "=== Migration SQL and metadata diff, excluding large snapshot bodies ==="
git diff --find-renames "$merge_base" HEAD -- resources/drizzle \
':(exclude)resources/drizzle/meta/*_snapshot.json' | sed -n '1,1000p'
echo "=== Migration executor and Drizzle configuration ==="
fd -a -t f -E node_modules . | rg '(^|/)(drizzle\.config\..*|.*migrat.*\.(ts|js)|db\.(ts|js))$' |
while IFS= read -r f; do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
rg -n -C4 '(migrate\(|migrationsFolder|resources/drizzle|drizzle-orm.*migrat|PGlite)' "$f" || true
done
echo "=== Focused lyrics feature code ==="
for f in \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/search.ts \
src/main/main.ts \
src/main/parseSong/parseSong.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts \
src/main/core/getSongLyrics.ts \
src/main/db/schema.ts \
src/preload/index.ts \
src/renderer/src/queries/search.ts \
src/renderer/src/utils/zod/searchPageSchema.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/routes/main-player/search/index.tsx \
src/renderer/src/routes/main-player/search/all/index.tsx \
src/types/app.d.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo "=== Changed-feature tests and test references ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(lyricsIndex|searchSongsByLyrics|saveLyricsToLRCFile|savePendingSongLyrics|HighlightedSnippet|LyricsSearchResult|getLrcFilePaths)' \
test tests src 2>/dev/null || true
echo "=== Search input entry point and IPC handler ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'(searchSongsByLyrics|search\(|searchSong|SEARCH|searchPageSchema|ipcMain\.handle.*search|ipcMain\.on.*search)' \
src/main src/preload src/renderer/src | sed -n '1,1000p'Length of output: 119555 🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref=""
for candidate in origin/main origin/master main master; do
if git rev-parse --verify -q "$candidate" >/dev/null; then base_ref="$candidate"; break; fi
done
merge_base="$(git merge-base HEAD "$base_ref")"
echo "=== Base and HEAD migration journals ==="
echo "--- BASE ---"
git show "$merge_base:resources/drizzle/meta/_journal.json" 2>/dev/null | jq .
echo "--- HEAD ---"
jq . resources/drizzle/meta/_journal.json
echo "=== SQL migration contents: base and HEAD ==="
echo "--- BASE migration list ---"
git ls-tree -r --name-only "$merge_base" resources/drizzle | rg '\.sql$' | sort
echo "--- BASE 0002 ---"
git show "$merge_base:resources/drizzle/0002_add_zoom_factor_to_user_settings.sql" 2>/dev/null | cat -n || true
echo "--- HEAD migration list ---"
fd -a -e sql . resources/drizzle | sort
for f in $(fd -a -e sql . resources/drizzle | sort); do
echo "--- $f ---"
cat -n "$f"
done
echo "=== Migration runner and DB setup ==="
for f in src/main/db/db.ts src/main/db/migrate.ts drizzle.config.ts drizzle.config.js; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
rg -n -C5 --glob '*.{ts,js}' \
'(migrate\(|migrationsFolder|resources/drizzle|__drizzle_migrations|migration)' \
src drizzle.config.* package.json
echo "=== Lyrics write, index, and search implementation ==="
for spec in \
'src/main/saveLyricsToSong.ts:1,145' \
'src/main/core/saveLyricsToLrcFile.ts:1,180' \
'src/main/db/queries/lyricsIndex.ts:1,230' \
'src/main/db/queries/search.ts:210,285' \
'src/main/search.ts:1,120' \
'src/main/parseSong/parseSong.ts:245,295' \
'src/main/core/getSongLyrics.ts:1,145' \
'src/main/main.ts:275,315' \
'src/main/ipc.ts:280,310' \
'src/preload/index.ts:190,215' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,155' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,80'
do
file="${spec%%:*}"; range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file [$range] ---"
sed -n "$range" "$file" | cat -n
fi
done
echo "=== Current and base schema deltas for affected tables ==="
git diff --unified=80 "$merge_base" HEAD -- src/main/db/schema.ts src/types/app.d.ts | sed -n '1,1500p'
echo "=== Changed test files and test commands ==="
echo "--- changed tests ---"
git diff --name-only "$merge_base" HEAD | rg '(\.test|\.spec|/test/|/tests/|/__tests__/)' || true
echo "--- package scripts ---"
jq '.scripts' package.json
echo "--- relevant existing tests ---"
fd -a -t f -E node_modules | rg '(lyrics|search).*\.(test|spec)\.(ts|tsx)$|(^|/)(test|tests|__tests__)/' | sort | sed -n '1,300p'
echo "=== Locale-key coverage ==="
python3 - <<'PY'
import json
from pathlib import Path
for p in sorted(Path("src/renderer/src/assets/locales").glob("*/*.json")):
try:
d=json.loads(p.read_text())
print(f"{p}: lyric_other={d.get('common', {}).get('lyric_other')!r}")
except Exception as exc:
print(f"{p}: INVALID {exc}")
PYLength of output: 49748 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Installed package versions ==="
jq '{dependencies, devDependencies} | with_entries(.value |= with_entries(select(.key | test("drizzle|pglite|electron|zod")))' package.json
echo "=== Focused changed source ==="
for item in \
'src/main/saveLyricsToSong.ts 1 145' \
'src/main/core/saveLyricsToLrcFile.ts 1 180' \
'src/main/db/queries/lyricsIndex.ts 1 240' \
'src/main/db/queries/search.ts 210 295' \
'src/main/search.ts 1 125' \
'src/main/parseSong/parseSong.ts 245 300' \
'src/main/core/getSongLyrics.ts 1 150' \
'src/main/main.ts 275 315' \
'src/main/ipc.ts 285 305' \
'src/preload/index.ts 190 215' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx 1 145' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx 1 80' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx 1 120' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx 1 100'
do
set -- $item
file="$1"; start="$2"; end="$3"
if [ -f "$file" ]; then
echo "--- $file, lines $start-$end ---"
sed -n "${start},${end}p" "$file" | cat -n
fi
done
echo "=== All direct call sites and unawaited LRC saves ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'(saveLyricsToLRCFile\(|upsertSongLyrics\(|upsertSongLyricsFromText\(|indexAllLyrics\()' \
src
echo "=== Migration timestamp changes and exact schema additions ==="
git diff --unified=25 "$(git merge-base HEAD "$(git rev-parse --verify -q origin/main || git rev-parse --verify -q main || git rev-parse --verify -q origin/master || git rev-parse --verify master)")" HEAD -- \
resources/drizzle/meta/_journal.json \
resources/drizzle/0002_add_zoom_factor_to_user_settings.sql \
resources/drizzle/0002_square_greymalkin.sql \
resources/drizzle/0003_lowly_komodo.sql \
resources/drizzle/0004_talented_master_chief.sql \
resources/drizzle/0005_many_nightshade.sql \
src/main/db/schema.ts
echo "=== Changed files with no direct test coverage ==="
git diff --name-only "$(git merge-base HEAD "$(git rev-parse --verify -q origin/main || git rev-parse --verify -q main || git rev-parse --verify -q origin/master || git rev-parse --verify master)")" HEAD \
| sed -n '1,300p'
echo "--- test file names ---"
fd -a -t f -E node_modules | rg '\.(test|spec)\.(ts|tsx)$' | sort | sed -n '1,300p'Length of output: 3079 🤖 web_search executed:
Length of output: 15581 🏁 Script executed: #!/bin/bash
set -u
echo "=== Dependency versions ==="
grep -nE '"(drizzle-orm|drizzle-kit|`@electric-sql/pglite`|electron|zod)"' package.json || true
print_file() {
file="$1"
start="$2"
end="$3"
if [ -f "$file" ]; then
echo "--- $file: lines $start-$end ---"
sed -n "${start},${end}p" "$file" | nl -ba
fi
}
print_file src/main/saveLyricsToSong.ts 1 145
print_file src/main/core/saveLyricsToLrcFile.ts 1 180
print_file src/main/db/queries/lyricsIndex.ts 1 240
print_file src/main/db/queries/search.ts 220 290
print_file src/main/search.ts 1 120
print_file src/main/parseSong/parseSong.ts 255 295
print_file src/main/main.ts 285 315
print_file src/main/ipc.ts 285 305
print_file src/preload/index.ts 190 215
print_file src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx 1 145
print_file src/renderer/src/components/SearchPage/HighlightedSnippet.tsx 1 80
print_file src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx 1 120
echo "=== Call-site context ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(saveLyricsToLRCFile\(|upsertSongLyrics\(|indexAllLyrics\(|searchSongsByLyrics\()' src || true
echo "=== Test inventory ==="
echo "--- changed test files ---"
base_ref="$(git rev-parse --verify -q origin/main || git rev-parse --verify -q main || git rev-parse --verify -q origin/master || git rev-parse --verify master)"
merge_base="$(git merge-base HEAD "$base_ref")"
git diff --name-only "$merge_base" HEAD | rg '(\.test|\.spec|/test/|/tests/|/__tests__/)' || true
echo "--- all relevant tests ---"
fd -a -t f -E node_modules | rg -i '(lyrics|search).*\.(test|spec)\.(ts|tsx)$' || true
echo "--- package scripts ---"
grep -nA35 '"scripts"' package.json || trueLength of output: 13159
Readiness verdictNot Ready Confidence: High (90%) A production upgrade can fail during database startup. This finding is independent of the prior lyrics-index findings. I reviewed the current PR diff and relevant integration paths. I excluded the issues already covered in this thread:
I did not run the application, build, or test suite. The sandbox does not permit repository execution. Static review cannot prove zero defects. FindingsCritical — Rewritten historical Drizzle migration can prevent startup for upgraded usersLocations
Evidence The base branch contains this journal entry: {
"idx": 2,
"when": 1775380800000,
"tag": "0002_add_zoom_factor_to_user_settings"
}The PR replaces it with: {
"idx": 2,
"when": 1780036880379,
"tag": "0002_square_greymalkin"
}The deleted migration already creates The replacement migration also creates ALTER TABLE "user_settings"
ADD COLUMN "tray_single_click_toggles_window" boolean DEFAULT false NOT NULL;
ALTER TABLE "user_settings"
ADD COLUMN "zoom_factor" double precision DEFAULT 0.8 NOT NULL;
await migrate(db, { migrationsFolder });on every application startup. Drizzle migration processing reads the migration journal and uses the journal timestamp as the migration watermark. The replacement entry has a later timestamp than the released migration. An existing user database can therefore treat the replacement That migration then executes: ALTER TABLE "user_settings" ADD COLUMN "zoom_factor" ...The column already exists. PostgreSQL rejects the statement. The database initialization fails. The application cannot start. Drizzle documents journal-based migration metadata and tracks applied migration hashes and timestamps. See the Drizzle migration metadata documentation and the migration reader implementation. Impact
Required fix Do not modify or delete released migrations or their journal entries. Restore both of these unchanged:
Create new append-only migrations instead:
Major — Unawaited LRC write can lose error propagation and produce stale search stateLocation
if (shouldSaveLrcFile) saveLyricsToLRCFile(songPath, songLyrics);Evidence
The caller neither awaits the promise nor attaches a rejection handler. If the LRC write fails because of permissions, a removed directory, a disconnected volume, or an invalid custom destination, the caller does not receive the failure. The save flow can continue to save embedded lyrics or report later state changes while the LRC write failed. This also creates concurrent operations on the same song:
The final index usually converges because the re-index reads persisted sources. It does not provide ordering or atomicity. A failure can remain unobserved by the initiating save path. Impact
Suggested fix Await the operation when LRC persistence is part of the requested save: if (shouldSaveLrcFile) {
await saveLyricsToLRCFile(songPath, songLyrics);
}Make
If concurrent persistence is intentional, use Minor — The feature has no focused automated test coverageAffected areas
Evidence The PR changes several existing tests, but none cover lyrics indexing or lyrics search. The only matching existing test found is Impact The critical migration defect would likely have been detected by an upgrade migration test. The following runtime behavior has no regression protection:
Suggested fix Add focused tests before release:
Validated controlsThe following areas are correctly designed based on static review. Search query safetyThe lyrics query uses Drizzle SQL interpolation for the search keyword. The keyword is passed as a query value. It is not concatenated into executable SQL. This reduces SQL injection risk in Renderer XSS handling
It splits the Search route validation
z.enum(searchFilterTypes)The renderer also enables search only when the trimmed keyword has content. FTS failure containment
Backfill retry behaviorThe current code returns an Custom LRC directory propagationThe import path and both lyrics save paths pass Source preservationThe save paths re-read sources with Required release gateDo not merge or ship until the migration history is restored and an upgrade migration test passes. After that fix, run these checks in CI or a release candidate environment: Also run a manual upgrade test with an existing production-format PGlite database. The database must contain the original You are interacting with an AI system. |
…cFilePaths tests CR Major: saveLyricsToSong fire-and-forgot saveLyricsToLRCFile, so an LRC write failure became an unhandled rejection while the save flow reported success. The call is now awaited with a try/catch that logs and sends LYRICS_SAVE_FAILED to the renderer. Added the message code to the notification trigger map, the MessageCodes union, and en.json. CR Minor: added 5 focused getLrcFilePaths tests covering the default/ extension-stripped paths, the replaceAll path-corruption regression (extension string inside a directory), custom-dir basename stripping, no-extension files, and absence of custom paths without a custom dir. Note: CR's Critical finding (rewritten historical migration 0002_add_zoom_factor_to_user_settings) is a false positive. The base commit 6cb9580 and release/4.0.0-alpha.5 already carry 0002_square_greymalkin (renamed in a120e1b, 2026-05-29, an ancestor of both release and this PR). This PR's migration diff is append-only (0003/0004/0005 added, idx 0-2 untouched). CR appears to have compared against the master/v5 line which uses the old migration name.
|
@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. Prior-finding disposition (please verify, do not re-report)Your previous round flagged a Critical: "Rewritten historical Drizzle migration 0002_add_zoom_factor_to_user_settings can prevent startup." That finding is a false positive against this PR's actual base. Evidence:
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. 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
target="release/4.0.0-alpha.5"
echo "=== Repository and target resolution ==="
git status --short
git rev-parse --verify HEAD
git rev-parse --verify "$target^{commit}" || git rev-parse --verify "origin/$target^{commit}"
target_ref="$target"
if ! git rev-parse --verify -q "$target_ref^{commit}" >/dev/null; then
target_ref="origin/$target"
fi
base="$(git merge-base HEAD "$target_ref")"
printf 'TARGET_REF=%s\nTARGET=%s\nMERGE_BASE=%s\nHEAD=%s\n' \
"$target_ref" "$(git rev-parse "$target_ref")" "$base" "$(git rev-parse HEAD)"
echo "=== Target ancestry and latest-tip check ==="
git merge-base --is-ancestor "$target_ref" HEAD && echo "HEAD contains target tip: yes" || echo "HEAD contains target tip: no"
git merge-base --is-ancestor "$base" "$target_ref" && echo "merge base is target ancestor: yes"
git log --oneline -1 "$target_ref"
git log --oneline -1 HEAD
echo "--- target..HEAD commits ---"
git log --oneline "$target_ref..HEAD" | sed -n '1,160p'
echo "--- merge-base..target commits ---"
git log --oneline "$base..$target_ref" | sed -n '1,160p'
echo "=== Merge simulation (read-only temporary index) ==="
tmp_index="$(mktemp)"
rm -f "$tmp_index"
export GIT_INDEX_FILE="$tmp_index"
git read-tree "$target_ref"
if git merge-tree --write-tree "$target_ref" HEAD >/tmp/merge-tree.out 2>/tmp/merge-tree.err; then
echo "merge-tree: clean"
else
echo "merge-tree: conflicts or failed"
cat /tmp/merge-tree.out /tmp/merge-tree.err
fi
rm -f "$tmp_index"
unset GIT_INDEX_FILE
echo "=== Target-relative changed-file inventory and patch checks ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
git diff --check "$base" HEAD || true
echo "=== Target-relative migration chain ==="
echo "--- target migrations ---"
git ls-tree -r --name-only "$target_ref" resources/drizzle | sort
echo "--- HEAD migrations ---"
fd -a -t f . resources/drizzle | sort
echo "--- journal diff ---"
git diff --unified=30 "$base" HEAD -- resources/drizzle/meta/_journal.json
echo "--- SQL diff ---"
git diff --find-renames --unified=20 "$base" HEAD -- resources/drizzle '*.sql'
echo "--- target historical 0002 ---"
git show "$target_ref:resources/drizzle/0002_square_greymalkin.sql" 2>/dev/null || true
echo "--- head journal ---"
cat -n resources/drizzle/meta/_journal.json
echo "=== Changed source outlines ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r f; do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo "=== Full target-relative patch, source and configuration ==="
git diff --find-renames "$base" HEAD -- \
src resources/drizzle package.json pnpm-lock.yaml electron.vite.config.* \
| sed -n '1,3600p'
echo "=== IPC/preload/search boundary and new symbol call sites ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge|searchSongsByLyrics|LyricsSearchResult|upsertSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchPageSchema)' \
src
echo "=== Focused modified runtime files ==="
for f in \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/search.ts \
src/main/main.ts \
src/main/parseSong/parseSong.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts \
src/main/core/getSongLyrics.ts \
src/main/db/schema.ts \
src/preload/index.ts \
src/renderer/src/queries/search.ts \
src/renderer/src/utils/zod/searchPageSchema.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/routes/main-player/search/index.tsx \
src/renderer/src/routes/main-player/search/all/index.tsx \
src/types/app.d.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo "=== Test inventory and target-relative test changes ==="
git diff --name-only "$base" HEAD | rg '(^|/)(test|tests|__tests__)/|\.(test|spec)\.(ts|tsx)$' || true
fd -a -t f -E node_modules | rg '(^|/)(test|tests|__tests__)/|\.(test|spec)\.(ts|tsx)$' | sort | sed -n '1,360p'
rg -n -C2 --glob '*.{ts,tsx}' \
'(lyricsIndex|searchSongsByLyrics|HighlightedSnippet|saveLyricsToLRCFile|savePendingSongLyrics|getLrcFilePaths)' \
test tests src 2>/dev/null || true
echo "=== CI/check state for PR `#528` ==="
gh pr view 528 --repo Sandakan/Nora --json baseRefName,headRefName,mergeable,mergeStateStatus,statusCheckRollup,commits \
--jq '{baseRefName,headRefName,mergeable,mergeStateStatus,statusCheckRollup:[.statusCheckRollup[]|{name,conclusion,status,detailsUrl}],headSha:(.commits[-1].oid)}' \
|| trueLength of output: 5199 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target="release/4.0.0-alpha.5"
echo "=== Available local and remote refs ==="
git branch -a --no-color
git remote -v
echo "=== Pull request base and head metadata ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName,
baseRefOid,
headRefName,
headRefOid,
mergeable,
mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
echo "=== Resolve target branch from GitHub ==="
gh api "repos/$repo/git/ref/heads/$target" --jq '.object.sha'
echo "=== Fetch only the target ref when it is absent locally ==="
if ! git rev-parse --verify -q "refs/remotes/origin/$target^{commit}" >/dev/null; then
git fetch --no-tags origin \
"refs/heads/$target:refs/remotes/origin/$target"
fi
echo "=== Resolved target and merge base ==="
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf 'HEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD \
&& echo "HEAD contains the current target tip: yes" \
|| echo "HEAD contains the current target tip: no"
echo "--- commits present on target after merge base ---"
git log --oneline "$base..$target_ref" | sed -n '1,100p'
echo "--- commits in PR after merge base ---"
git log --oneline "$base..HEAD" | sed -n '1,160p'Length of output: 4190 🏁 Script executed: #!/bin/bash
set -euo pipefail
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== Complete target-relative changed-file inventory ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration verification against actual target ==="
echo "--- migration files on target ---"
git ls-tree -r --name-only "$target_ref" resources/drizzle | sort
echo "--- migration files at HEAD ---"
fd -a -t f . resources/drizzle | sort
echo "--- journal diff ---"
git diff --unified=40 "$base" HEAD -- resources/drizzle/meta/_journal.json
echo "--- migration SQL diff ---"
git diff --find-renames --unified=25 "$base" HEAD -- resources/drizzle '*.sql'
echo "--- current journal entries ---"
jq '.entries' resources/drizzle/meta/_journal.json
echo "--- target idx 0-2 entries ---"
git show "$target_ref:resources/drizzle/meta/_journal.json" | jq '.entries[:3]'
echo "=== Complete source patch ==="
git diff --find-renames --unified=30 "$base" HEAD -- \
src resources/drizzle package.json electron.vite.config.* \
| sed -n '1,4200p'
echo "=== Changed TypeScript/TSX outline ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r f; do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo "=== Runtime implementations and adjacent contracts ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,260' \
'src/main/db/queries/search.ts:1,320' \
'src/main/search.ts:1,160' \
'src/main/main.ts:260,330' \
'src/main/parseSong/parseSong.ts:240,310' \
'src/main/core/getSongLyrics.ts:1,190' \
'src/main/core/saveLyricsToLrcFile.ts:1,230' \
'src/main/saveLyricsToSong.ts:1,180' \
'src/main/db/schema.ts:390,530' \
'src/main/db/db.ts:1,120' \
'src/main/ipc.ts:260,325' \
'src/preload/index.ts:175,225' \
'src/renderer/src/queries/search.ts:1,100' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,120' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,120' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,210' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,160' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,150' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx:1,120' \
'src/renderer/src/routes/main-player/search/index.tsx:1,290' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,180' \
'src/types/app.d.ts:820,900'
do
file="${spec%%:*}"
range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file [$range] ---"
sed -n "$range" "$file" | nl -ba
fi
done
echo "=== All direct callers and relevant IPC/preload contracts ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(upsertSongLyrics|upsertSongLyricsFromText|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics)\s*\(' \
src
echo "--- IPC handlers and preload bindings related to search ---"
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.handle|ipcMain\.on|contextBridge\.exposeInMainWorld|searchPage|search\b)' \
src/main src/preload src/renderer/src | sed -n '1,1400p'
echo "=== Test files and exact lyrics-related tests ==="
git diff --name-status "$base" HEAD -- \
':!resources/drizzle/meta/0005_snapshot.json' | rg '(test|spec)|^' | sed -n '1,320p'
fd -a -t f -E node_modules | rg '(lyrics|search).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo "=== Typecheck scripts and check details ==="
jq '.scripts' package.json
gh run view 30699737936 --repo Sandakan/Nora --json conclusion,jobs \
--jq '{conclusion, jobs:[.jobs[]|{name,conclusion,steps:[.steps[]|{name,conclusion}]}]}' || true
gh run view 30699737932 --repo Sandakan/Nora --json conclusion,jobs \
--jq '{conclusion, jobs:[.jobs[]|{name,conclusion,steps:[.steps[]|{name,conclusion}]}]}' || trueLength of output: 94850 🏁 Script executed: #!/bin/bash
set -euo pipefail
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== Target-relative changed files, excluding generated snapshot ==="
git diff --name-status "$base" HEAD -- ':!resources/drizzle/meta/0005_snapshot.json'
echo "=== SQL migrations introduced by this PR ==="
for f in $(git diff --name-only --diff-filter=A "$base" HEAD -- 'resources/drizzle/*.sql'); do
echo "--- $f ---"
cat -n "$f"
done
echo "=== Migration file additions and modifications ==="
git diff --name-status "$base" HEAD -- resources/drizzle | rg -v '0005_snapshot.json'
echo "=== src/main/db/queries/lyricsIndex.ts ==="
cat -n src/main/db/queries/lyricsIndex.ts
echo "=== src/main/db/queries/search.ts lines 210-300 ==="
sed -n '210,300p' src/main/db/queries/search.ts | cat -n
echo "=== src/main/search.ts ==="
cat -n src/main/search.ts
echo "=== src/main/main.ts relevant startup code ==="
sed -n '270,320p' src/main/main.ts | cat -n
echo "=== src/main/parseSong/parseSong.ts relevant code ==="
sed -n '245,305p' src/main/parseSong/parseSong.ts | cat -n
echo "=== src/main/core/getSongLyrics.ts lines 15-135 ==="
sed -n '15,135p' src/main/core/getSongLyrics.ts | cat -n
echo "=== src/main/core/saveLyricsToLrcFile.ts ==="
cat -n src/main/core/saveLyricsToLrcFile.ts
echo "=== src/main/saveLyricsToSong.ts ==="
cat -n src/main/saveLyricsToSong.ts
echo "=== Renderer search implementations ==="
for f in \
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 \
src/renderer/src/queries/search.ts \
src/renderer/src/utils/zod/searchPageSchema.ts
do
echo "--- $f ---"
cat -n "$f"
done
echo "=== New and related test contents ==="
for f in \
test/src/main/core/getSongLyrics.test.ts \
test/src/common/parseLyrics.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo "=== New-line additions in changed source, with current line numbers ==="
git diff --unified=0 "$base" HEAD -- '*.ts' '*.tsx' |
rg -n '^\+\s*(export |const |function |await |return |if |try|catch|use|<|[A-Za-z_])' | sed -n '1,1200p'
echo "=== Package scripts and successful CI job steps ==="
jq '.scripts' package.json
gh run view 30699737936 --repo Sandakan/Nora --json conclusion,jobs \
--jq '{conclusion, jobs:[.jobs[]|{name,conclusion,steps:[.steps[]|{name,conclusion}]}]}' || true
gh run view 30699737932 --repo Sandakan/Nora --json conclusion,jobs \
--jq '{conclusion, jobs:[.jobs[]|{name,conclusion,steps:[.steps[]|{name,conclusion}]}]}' || trueLength of output: 115180 🏁 Script executed: #!/bin/bash
set -euo pipefail
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== Remaining changed-file diffs ==="
git diff --unified=50 "$base" HEAD -- \
src/renderer/src/assets/locales/en/en.json \
src/renderer/src/other/parseNotificationFromMain.tsx \
src/main/db/schema.ts \
src/types/app.d.ts \
resources/drizzle/meta/0005_snapshot.json \
| sed -n '1,2400p'
echo "=== Migration snapshot consistency for new objects ==="
jq -r '
{
songLyrics: .tables["public.song_lyrics"],
isLyricIndexBuilt: .tables["public.user_settings"].columns.is_lyric_index_built
}' resources/drizzle/meta/0005_snapshot.json
echo "--- schema declarations ---"
sed -n '400,520p' src/main/db/schema.ts | cat -n
echo "=== Search IPC path and declared API types ==="
rg -n -C8 --glob '*.{ts,tsx}' \
'(search:\s*\{|search\([^)]*filter|search\([^)]*keyword|searchPage|IpcChannels|SEARCH)' \
src/main/ipc.ts src/preload/index.ts src/types/app.d.ts src/renderer/src/queries/search.ts
echo "=== Comparable Song result behavior ==="
for f in \
src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllSongResults.tsx \
src/renderer/src/components/SearchPage/Result_Containers/SongSearchResultsContainer.tsx \
src/renderer/src/components/SongsPage/Song.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,280p' "$f" | cat -n
fi
done
echo "=== Workflows and typecheck references ==="
fd -a -t f -E node_modules .github |
while IFS= read -r f; do
echo "--- $f ---"
rg -n -C3 '(typecheck|check-types|npm run test|vitest|lint|format)' "$f" || true
done
echo "=== All test files added or modified by target-relative PR ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.spec.ts'
echo "=== Existing direct coverage of new main and renderer modules ==="
for term in lyricsIndex searchSongsByLyrics HighlightedSnippet LyricSearchResultsContainer AllLyricResults saveLyricsToLRCFile savePendingSongLyrics; do
echo "--- $term ---"
rg -n --glob '*.{test,spec}.{ts,tsx}' "\\b${term}\\b" test src 2>/dev/null || true
done
echo "=== New-file import resolution and type-sensitive diagnostics ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(LYRICS_SAVE_FAILED|LYRICS_SAVED_IN_LRC_FILE|PENDING_LYRICS_SAVED|isLyricIndexBuilt|LyricsSearchResult)' \
src typesLength of output: 107500
VerdictNot Ready — high confidence. The branch is MERGEABLE with CI status:
The prior migration Critical is invalid for this target branch. The migration chain is append-only:
Major findingsMajor — Backfill can still mark a failed index as completeLocations
Root cause
if (embedded === null || lrc === null) {
logger.warn(...);
return false;
}
if (result.status === 'fulfilled') {
processed += 1;
if (result.value) indexed += 1;
} else {
failed += 1;
}Therefore, a read or parse error produces a fulfilled saveUserSettings({ isLyricIndexBuilt: true })The affected song will not retry on the next launch. Impact A temporary file-system error, unreadable LRC file, or tag parsing error can leave one or more lyric rows missing or stale permanently. Fix Return a result that separates these states. For example: type LyricsIndexResult = 'indexed' | 'absent' | 'read-error';Count Do not count an Minor findingsMinor — The all-results Lyrics view does not construct a playback queueLocations
Root cause
Impact Playing a song from the full Lyrics result view does not use the lyric-result queue. It uses the existing queue instead. This behavior differs from the full Songs result view and the compact Lyrics result view. Fix Add the same queue construction behavior used by Missing test coverageThe PR adds The following new behavior has no direct automated coverage:
What is working correctly
Resolve the backfill completion condition and add a test for it before release. Verdict: Not Ready — high confidenceYou are interacting with an AI system. |
…s results CR Major: upsertSongLyrics returned false both for read-errors AND for absent lyrics, but indexAllLyrics only counted rejected promises as failures. A read/parse error produced a fulfilled false -> failed stayed 0 -> allSucceeded true -> isLyricIndexBuilt persisted true, so the song never retried. Now returns 'indexed' | 'absent' | 'read-error'; countBackfillResults counts read-error as a failure; isLyricIndexBuilt is only set when no read-errors or rejections occur. CR Minor: AllLyricResults rendered Song without onPlayClick, so playing from the full Lyrics view used the existing queue instead of the lyric-result queue. Added the same blacklist-filtered queue construction as AllSongResults. Tests: 4 countBackfillResults unit tests (read-error failure, absent not failure, rejected promise, empty batch). 345 total pass.
|
@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. Prior-finding disposition (verify, do not re-report)
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
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"
target="release/4.0.0-alpha.5"
echo "=== Resolve PR and target ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
if ! git rev-parse --verify -q "refs/remotes/origin/$target^{commit}" >/dev/null; then
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target"
fi
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf '\nHEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD \
&& echo "HEAD contains target tip: yes" \
|| echo "HEAD contains target tip: no"
echo "=== Read-only merge simulation ==="
if git merge-tree --write-tree "$target_ref" HEAD >/tmp/pr528-merge.out 2>/tmp/pr528-merge.err; then
echo "merge-tree: clean"
else
echo "merge-tree: failed/conflicted"
cat /tmp/pr528-merge.out /tmp/pr528-merge.err
fi
echo "=== Complete target-relative inventory ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "--- patch check ---"
git diff --check "$base" HEAD || true
echo "=== Migration chain and journal ==="
echo "--- target entries ---"
git show "$target_ref:resources/drizzle/meta/_journal.json" | jq '.entries'
echo "--- HEAD entries ---"
jq '.entries' resources/drizzle/meta/_journal.json
echo "--- target-relative migration diff ---"
git diff --find-renames --unified=30 "$base" HEAD -- resources/drizzle
echo "=== Changed source outlines ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r f; do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo "=== Full changed source/configuration patch ==="
git diff --find-renames --unified=35 "$base" HEAD -- \
src resources/drizzle package.json electron.vite.config.* \
| sed -n '1,5200p'
echo "=== Focused current implementations ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,300' \
'src/main/db/queries/search.ts:200,320' \
'src/main/search.ts:1,160' \
'src/main/main.ts:260,335' \
'src/main/parseSong/parseSong.ts:240,315' \
'src/main/core/getSongLyrics.ts:1,200' \
'src/main/core/saveLyricsToLrcFile.ts:1,240' \
'src/main/saveLyricsToSong.ts:1,190' \
'src/main/db/schema.ts:390,540' \
'src/main/ipc.ts:250,335' \
'src/preload/index.ts:165,235' \
'src/renderer/src/queries/search.ts:1,110' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,130' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,130' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,240' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,220' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,160' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx:1,130' \
'src/renderer/src/routes/main-player/search/index.tsx:1,310' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,200' \
'src/types/app.d.ts:820,910'
do
file="${spec%%:*}"; range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file [$range] ---"
sed -n "$range" "$file" | nl -ba
fi
done
echo "=== All affected call sites and boundary contracts ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(countBackfillResults|upsertSongLyrics|upsertSongLyricsFromText|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics)\s*\(' \
src test 2>/dev/null || true
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|LyricsSearchResult|SearchFilters)' \
src test 2>/dev/null || true
echo "=== Tests and scripts ==="
echo "--- target-relative test changes ---"
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
echo "--- relevant tests ---"
fd -a -t f -E node_modules | rg '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo "--- package scripts ---"
jq '.scripts' package.jsonLength of output: 82318 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== PR merge state and CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion}]
}'
printf 'HEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD \
&& echo "target-tip-contained=yes" \
|| echo "target-tip-contained=no"
if git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1; then
echo "merge-tree=clean"
else
echo "merge-tree=conflict-or-error"
fi
echo "=== Target-relative changed files ==="
git diff --name-status "$base" HEAD | grep -v 'resources/drizzle/meta/0005_snapshot.json'
echo "=== Patch whitespace check ==="
git diff --check "$base" HEAD || true
echo "=== Journal and SQL migration changes ==="
git diff --unified=20 "$base" HEAD -- resources/drizzle/meta/_journal.json 'resources/drizzle/*.sql'
echo "=== Journal prefix equality (idx 0-4) ==="
python3 - <<'PY'
import json, subprocess
target = json.loads(subprocess.check_output(
["git", "show", "refs/remotes/origin/release/4.0.0-alpha.5:resources/drizzle/meta/_journal.json"],
text=True
))["entries"]
head = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("target_entries=", len(target), "head_entries=", len(head))
print("prefix_unchanged=", target == head[:len(target)])
print("added_entries=", head[len(target):])
PY
echo "=== Current lyrics index implementation ==="
cat -n src/main/db/queries/lyricsIndex.ts
echo "=== Current save/import/startup implementations ==="
sed -n '270,320p' src/main/main.ts | cat -n
sed -n '250,310p' src/main/parseSong/parseSong.ts | cat -n
cat -n src/main/core/saveLyricsToLrcFile.ts
cat -n src/main/saveLyricsToSong.ts
echo "=== Current search implementation ==="
sed -n '195,290p' src/main/db/queries/search.ts | cat -n
cat -n src/main/search.ts
echo "=== Current renderer implementations ==="
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
cat -n src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
sed -n '1,190p' src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx | cat -n
sed -n '1,150p' src/renderer/src/routes/main-player/search/all/index.tsx | cat -n
sed -n '1,250p' src/renderer/src/routes/main-player/search/index.tsx | cat -n
echo "=== IPC/preload/type and route contracts ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on).*search|search:\s*\(|contextBridge|searchPageSchema|SearchFilters|LyricsSearchResult)' \
src/main src/preload src/renderer/src src/types
echo "=== Changed IPC/preload files only ==="
git diff --name-only "$base" HEAD -- src/main src/preload | sort
echo "=== Changed tests and complete relevant test files ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
fd -a -t f -E node_modules | rg -i '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo "=== Project scripts ==="
jq '.scripts' package.jsonLength of output: 126565
VerdictNot Ready — high confidence. The branch is mergeable with The branch is not based on the latest target tip:
CI does not pass at this time:
The previous migration finding is withdrawn. The migration chain is append-only against the correct target branch. Entries Major findingsMajor — The PR is behind the current release target and required CI checks failLocation: PR merge state and CI checks. The PR does not contain the current Both required visible checks fail. The failure details were not available in this audit output. Impact: The PR can merge cleanly, but it can omit target-branch changes. Failed lint and test checks block a production release. Fix:
Major — A
|
|
@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
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target="release/4.0.0-alpha.5"
echo "=== PR metadata and current CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name,status,conclusion,detailsUrl}]
}'
echo "=== Resolve authoritative upstream target ==="
target_sha="$(gh api "repos/$repo/git/ref/heads/$target" --jq '.object.sha')"
echo "upstream_target_sha=$target_sha"
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf 'head=%s\ntarget=%s\nmerge_base=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD \
&& echo "head_contains_target=yes" \
|| echo "head_contains_target=no"
echo "=== Read-only merge check ==="
if git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1; then
echo "merge_tree=clean"
else
echo "merge_tree=conflicted_or_failed"
fi
echo "=== Complete target-relative changed-file inventory ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration chain verification ==="
python3 - <<'PY'
import json, subprocess
target_ref = "refs/remotes/origin/release/4.0.0-alpha.5"
target = json.loads(subprocess.check_output(
["git", "show", f"{target_ref}:resources/drizzle/meta/_journal.json"], text=True
))["entries"]
with open("resources/drizzle/meta/_journal.json") as f:
head = json.load(f)["entries"]
print("target_entry_count=", len(target))
print("head_entry_count=", len(head))
print("target_prefix_unchanged=", target == head[:len(target)])
print("added_entries=", head[len(target):])
PY
git diff --find-renames --unified=30 "$base" HEAD -- resources/drizzle
echo "=== Changed source outlines ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo "=== Current feature code and direct callers ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,320' \
'src/main/db/queries/search.ts:190,330' \
'src/main/search.ts:1,180' \
'src/main/main.ts:250,340' \
'src/main/parseSong/parseSong.ts:230,325' \
'src/main/core/getSongLyrics.ts:1,220' \
'src/main/core/saveLyricsToLrcFile.ts:1,260' \
'src/main/saveLyricsToSong.ts:1,210' \
'src/main/db/schema.ts:380,550' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,150' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,240' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,240' \
'src/renderer/src/routes/main-player/search/index.tsx:1,320' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,210' \
'src/renderer/src/queries/search.ts:1,130' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,150' \
'src/types/app.d.ts:810,920'
do
file="${spec%%:*}"; range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file [$range] ---"
sed -n "$range" "$file" | nl -ba
fi
done
echo "=== Changed IPC and preload files, plus search boundary ==="
git diff --name-only "$base" HEAD -- src/main src/preload | sort
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|LyricsSearchResult|SearchFilters)' \
src/main src/preload src/renderer/src src/types
echo "=== All index/save/search call sites ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(countBackfillResults|upsertSongLyrics|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics)\s*\(' \
src test 2>/dev/null || true
echo "=== Tests and scripts ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
fd -a -t f -E node_modules | rg -i '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r file; do
echo "--- $file ---"
cat -n "$file"
done
jq '.scripts' package.jsonLength of output: 162628 🏁 Script executed: #!/bin/bash
set -euo pipefail
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
show_range() {
file="$1"
start="$2"
end="$3"
echo "--- $file: Lines $start-$end ---"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo "=== Full target-relative source diff, excluding generated snapshot ==="
git diff --find-renames --unified=50 "$base" HEAD -- \
src resources/drizzle \
':(exclude)resources/drizzle/meta/0005_snapshot.json' \
| sed -n '1,5000p'
echo "=== Main lyrics indexing and lifecycle ==="
show_range src/main/db/queries/lyricsIndex.ts 1 260
show_range src/main/main.ts 275 320
show_range src/main/parseSong/parseSong.ts 250 305
show_range src/main/core/saveLyricsToLrcFile.ts 115 190
show_range src/main/saveLyricsToSong.ts 1 150
show_range src/main/core/getSongLyrics.ts 15 135
echo "=== Search data flow ==="
show_range src/main/db/queries/search.ts 210 290
show_range src/main/search.ts 1 130
show_range src/main/ipc.ts 270 325
show_range src/preload/index.ts 175 225
show_range src/renderer/src/queries/search.ts 1 100
show_range src/renderer/src/utils/zod/searchPageSchema.ts 1 100
echo "=== Renderer results, playback, filters, notifications ==="
show_range src/renderer/src/components/SearchPage/HighlightedSnippet.tsx 1 100
show_range src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx 1 180
show_range src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx 1 200
show_range src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx 1 150
show_range src/renderer/src/components/SearchPage/SearchOptions.tsx 1 100
show_range src/renderer/src/routes/main-player/search/index.tsx 55 285
show_range src/renderer/src/routes/main-player/search/all/index.tsx 20 165
show_range src/renderer/src/other/parseNotificationFromMain.tsx 20 100
show_range src/types/app.d.ts 845 875
echo "=== Comparable queue implementations ==="
for file in \
src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllSongResults.tsx \
src/renderer/src/components/SearchPage/Result_Containers/SongSearchResultsContainer.tsx \
src/renderer/src/components/SongsPage/Song.tsx
do
echo "--- $file ---"
sed -n '1,260p' "$file"
done
echo "=== All affected call sites ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(countBackfillResults|upsertSongLyrics|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics)\s*\(' \
src test
echo "=== Changed IPC and preload files ==="
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
rg -n -C4 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on).*search|contextBridge.*search|search:\s*async|search:\s*\()' \
src/main/ipc.ts src/preload/index.ts
echo "=== Focused tests ==="
for file in test/src/main/core/getSongLyrics.test.ts test/src/main/db/queries/lyricsIndex.test.ts; do
echo "--- $file ---"
cat "$file"
done
echo "=== CI workflows and scripts ==="
jq '.scripts' package.json
fd -a -t f .github | while IFS= read -r file; do
echo "--- $file ---"
sed -n '1,260p' "$file"
doneLength of output: 219285
VerdictNot Ready — high confidence. The branch is MERGEABLE with
The prior migration finding does not apply to this target. The migration chain is append-only. The target journal entries Major findings
|
…t delimiters + notification styling + nitpicks Major 1: parseSong's import path discarded the upsertSongLyrics result, so a transient read-error left the song permanently out of lyric search once isLyricIndexBuilt was true. On 'read-error' it now logs and persists isLyricIndexBuilt: false so the next startup backfill retries. Major 2: updateSongId3Tags had 3 unawaited saveLyricsToLRCFile callers (pending metadata save, unknown-source update, known-source update) whose rejections became unhandled and left the LRC file + search index out of sync. All 3 now await with try/catch + logged song path. Minor 1: ts_headline defaulted to <b>/</b> markers; literal <b> in lyric text was rendered as highlight markup. Configured StartSel=[NRABEG], StopSel=[NRAEND] and HighlightedSnippet splits on those tokens. Minor 2: LYRICS_SAVE_FAILED was in the success notification list with a 'done' icon. Moved to the failure config (error icon). Nitpicks: totalResults now includes genres+lyrics counts; 0005 SQL gets a trailing newline; BATCH_SIZE hoisted to module scope as LYRICS_BACKFILL_BATCH_SIZE; saveLyricsToLrcFile warns when getSongByPath returns no song after a successful LRC write. 345 tests pass, typecheck clean. Renderer-component test for the snippet delimiter change deferred: the suite has no jsdom/RTL infra.
|
@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)
Known gap (not code)
Required verdict formatEnd with Report structure
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target="release/4.0.0-alpha.5"
echo "=== PR state and CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf '\nHEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
if git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1; then
echo "merge_tree=clean"
else
echo "merge_tree=conflict_or_error"
fi
echo "=== Complete target-relative changed files ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration journal and SQL changes ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
old = json.loads(subprocess.check_output(
["git", "show", f"{target}:resources/drizzle/meta/_journal.json"], text=True
))["entries"]
with open("resources/drizzle/meta/_journal.json") as f:
new = json.load(f)["entries"]
print("target_prefix_unchanged=", old == new[:len(old)])
print("added_entries=", new[len(old):])
PY
git diff --find-renames --unified=30 "$base" HEAD -- resources/drizzle
echo "=== Changed source outlines ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo "=== Full target-relative patch excluding generated snapshot ==="
git diff --find-renames --unified=40 "$base" HEAD -- \
src resources/drizzle \
':(exclude)resources/drizzle/meta/0005_snapshot.json' | sed -n '1,6500p'
echo "=== Current changed implementations and cross-cutting callers ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,340' \
'src/main/db/queries/search.ts:190,330' \
'src/main/search.ts:1,170' \
'src/main/main.ts:260,340' \
'src/main/parseSong/parseSong.ts:235,325' \
'src/main/core/getSongLyrics.ts:1,220' \
'src/main/core/saveLyricsToLrcFile.ts:1,260' \
'src/main/saveLyricsToSong.ts:1,210' \
'src/main/updateSong/updateSongId3Tags.ts:100,180' \
'src/main/updateSong/updateSongId3Tags.ts:740,810' \
'src/main/updateSong/updateSongId3Tags.ts:1070,1140' \
'src/main/db/schema.ts:380,550' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,140' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,230' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,240' \
'src/renderer/src/other/parseNotificationFromMain.tsx:1,130' \
'src/renderer/src/routes/main-player/search/index.tsx:1,310' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,200' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx:1,130' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,160' \
'src/renderer/src/queries/search.ts:1,120' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,140' \
'src/types/app.d.ts:810,920'
do
file="${spec%%:*}"; range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file [$range] ---"
sed -n "$range" "$file" | nl -ba
fi
done
echo "=== Lyrics/save/search callers, IPC, and preload changes ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(countBackfillResults|upsertSongLyrics|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics)\s*\(' \
src test 2>/dev/null || true
echo "--- changed IPC and preload files ---"
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|LyricsSearchResult|SearchFilters)' \
src/main src/preload src/renderer/src src/types
echo "=== Tests, scripts, and direct new-feature coverage ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
fd -a -t f -E node_modules | rg -i '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r file; do
echo "--- $file ---"
cat -n "$file"
done
echo "--- scripts ---"
jq '.scripts' package.jsonLength of output: 100530 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== PR state and CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion}]}' || true
printf 'HEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 && echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
echo "=== Target-relative changed files ==="
git diff --name-status "$base" HEAD
echo "=== Migration journal prefix ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
old = json.loads(subprocess.check_output(["git", "show", f"{target}:resources/drizzle/meta/_journal.json"], text=True))["entries"]
new = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("prefix_unchanged=", old == new[:len(old)])
print("added=", new[len(old):])
PY
show () {
echo "=== $1 ==="
cat -n "$1"
}
show src/main/db/queries/lyricsIndex.ts
show src/main/parseSong/parseSong.ts
show src/main/core/saveLyricsToLrcFile.ts
show src/main/saveLyricsToSong.ts
show src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
show src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
show src/renderer/src/other/parseNotificationFromMain.tsx
echo "=== updateSongId3Tags LRC callers ==="
rg -n -C6 --glob '*.ts' '\b(saveLyricsToLRCFile|upsertSongLyrics)\s*\(' src/main/updateSong/updateSongId3Tags.ts src/main/parseSong/parseSong.ts src/main/saveLyricsToSong.ts
echo "=== Search query and orchestration ==="
sed -n '200,290p' src/main/db/queries/search.ts | cat -n
sed -n '1,135p' src/main/search.ts | cat -n
echo "=== Renderer search/filter/routes ==="
for f in \
src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx \
src/renderer/src/routes/main-player/search/all/index.tsx \
src/renderer/src/routes/main-player/search/index.tsx \
src/renderer/src/components/SearchPage/SearchOptions.tsx \
src/renderer/src/utils/zod/searchPageSchema.ts \
src/renderer/src/queries/search.ts \
src/types/app.d.ts
do
echo "--- $f ---"
cat -n "$f"
done
echo "=== Changed IPC/preload files ==="
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
echo "=== New-feature tests ==="
for f in test/src/main/core/getSongLyrics.test.ts test/src/main/db/queries/lyricsIndex.test.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo "=== Test references ==="
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'(indexAllLyrics|upsertSongLyrics|searchSongsByLyrics|HighlightedSnippet|saveLyricsToLRCFile|savePendingSongLyrics|AllLyricResults|LyricSearchResultsContainer)' \
test src 2>/dev/null || true
echo "=== Scripts ==="
jq '.scripts' package.jsonLength of output: 150864 🏁 Script executed: #!/bin/bash
set -euo pipefail
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== Startup backfill and settings write semantics ==="
sed -n '275,325p' src/main/main.ts | cat -n
fd -a 'settings.ts' src/main/db/queries -x sh -c 'echo "--- $1 ---"; cat -n "$1"' sh
rg -n -C5 --glob '*.ts' '\bisLyricIndexBuilt\b|is_lyric_index_built' src test
echo "=== Remaining changed feature files ==="
for f in \
src/main/core/getSongLyrics.ts \
src/main/db/schema.ts \
resources/drizzle/0005_many_nightshade.sql \
src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx \
src/renderer/src/assets/locales/en/en.json
do
echo "--- $f ---"
cat -n "$f"
done
echo "=== Full diff for files not previously printed in detail ==="
git diff --unified=25 "$base" HEAD -- \
src/main/core/getSongLyrics.ts \
src/main/db/schema.ts \
resources/drizzle/0005_many_nightshade.sql \
src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx \
src/renderer/src/assets/locales/en/en.json \
src/types/app.d.ts \
| sed -n '1,2800p'
echo "=== Save and indexing calls that may overlap ==="
rg -n -C5 --glob '*.ts' \
'\b(saveUserSettings|upsertSongLyrics|indexAllLyrics|saveLyricsToLRCFile)\s*\(' \
src/main
echo "=== Change scope safety checks ==="
git diff --check "$base" HEAD
git diff --name-only "$base" HEAD -- src/main/ipc.ts src/preload/index.tsLength of output: 188568
VerdictNot Ready — high confidence. The branch is MERGEABLE with
The previous findings are fixed in the current code:
Major findings
|
…retry, control-char snippet delimiters Major 1 (concurrent write race): add a lyricsIndexGeneration counter. indexAllLyrics captures the generation at start and reports allSucceeded only if the generation is unchanged at the end. Import read-errors and post-save read-errors call invalidateLyricsIndex() so a late concurrent failure cannot be overwritten by an older successful backfill. Major 2 (post-save read-errors): saveLyricsToLrcFile and savePendingSongLyrics now check the fulfilled upsertSongLyrics result; on read-error they invalidate the index + log, so next startup retries. Minor (literal delimiter collision): swap [NRABEG]/[NRAEND] for control chars SOH/STX (u0001/u0002), impossible in lyric text. HighlightedSnippet splits on those. Nitpick (getSongLyrics): extract stripExtension() named helper instead of the long inline slice expression. tsquery-once CTE nitpick retained as-is: the DB|DBTransaction union type rejects the overload; CR rated it not-a-correctness-problem and the current form uses bound params. 345 tests pass, 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. 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
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target="release/4.0.0-alpha.5"
echo "=== PR state and checks ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]}' || true
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf 'HEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 && echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
echo "=== Complete target-relative change inventory ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration journal and SQL diff ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
old = json.loads(subprocess.check_output(
["git", "show", f"{target}:resources/drizzle/meta/_journal.json"], text=True
))["entries"]
new = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("target_prefix_unchanged=", old == new[:len(old)])
print("added_entries=", new[len(old):])
PY
git diff --find-renames --unified=30 "$base" HEAD -- resources/drizzle
echo "=== Changed code outlines ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo "=== Full target-relative source diff, excluding generated snapshot ==="
git diff --find-renames --unified=40 "$base" HEAD -- \
src resources/drizzle \
':(exclude)resources/drizzle/meta/0005_snapshot.json' | sed -n '1,7000p'
echo "=== Current indexing, save, import, and search implementations ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,360' \
'src/main/db/queries/search.ts:180,340' \
'src/main/search.ts:1,180' \
'src/main/main.ts:250,350' \
'src/main/parseSong/parseSong.ts:230,335' \
'src/main/core/getSongLyrics.ts:1,240' \
'src/main/core/saveLyricsToLrcFile.ts:1,280' \
'src/main/saveLyricsToSong.ts:1,220' \
'src/main/updateSong/updateSongId3Tags.ts:90,190' \
'src/main/updateSong/updateSongId3Tags.ts:730,830' \
'src/main/updateSong/updateSongId3Tags.ts:1060,1160' \
'src/main/db/schema.ts:370,560' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,160' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,250' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,250' \
'src/renderer/src/other/parseNotificationFromMain.tsx:1,150' \
'src/renderer/src/routes/main-player/search/index.tsx:1,330' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,210' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx:1,150' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,170' \
'src/renderer/src/queries/search.ts:1,130' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,150' \
'src/types/app.d.ts:800,930'
do
file="${spec%%:*}"; range="${spec#*:}"
[ -f "$file" ] && { echo "--- $file [$range] ---"; sed -n "$range" "$file" | nl -ba; }
done
echo "=== All affected callers and state writes ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(invalidateLyricsIndex|lyricsIndexGeneration|countBackfillResults|upsertSongLyrics|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics|saveUserSettings)\s*\(' \
src test 2>/dev/null || true
echo "=== IPC and preload changes/contracts ==="
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|LyricsSearchResult|SearchFilters)' \
src/main src/preload src/renderer/src src/types
echo "=== Tests and scripts ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
fd -a -t f -E node_modules | rg -i '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r file; do
echo "--- $file ---"
cat -n "$file"
done
jq '.scripts' package.jsonLength of output: 100706 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
echo "=== Current PR state ==="
gh pr view 528 --repo "$repo" \
--json mergeable,mergeStateStatus,headRefOid,baseRefOid,statusCheckRollup \
--jq '{mergeable,mergeStateStatus,headRefOid,baseRefOid,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion}]}' || true
base="$(git merge-base HEAD "$target_ref")"
printf 'HEAD=%s TARGET=%s BASE=%s\n' "$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 && echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
echo "=== Target-relative changed files ==="
git diff --name-status "$base" HEAD
echo "=== Lyrics invalidation, upsert outcomes, and persisted setting writes ==="
rg -n -C5 --glob '*.ts' \
'\b(invalidateLyricsIndex|lyricsIndexGeneration|isLyricIndexBuilt|saveUserSettings|upsertSongLyrics)\b' \
src/main/db/queries/lyricsIndex.ts \
src/main/main.ts \
src/main/parseSong/parseSong.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts
echo "=== Exact post-save implementations ==="
sed -n '120,210p' src/main/core/saveLyricsToLrcFile.ts
sed -n '80,155p' src/main/saveLyricsToSong.ts
echo "=== Startup and import implementations ==="
sed -n '280,325p' src/main/main.ts
sed -n '265,315p' src/main/parseSong/parseSong.ts
echo "=== Generation/backfill implementation ==="
sed -n '55,225p' src/main/db/queries/lyricsIndex.ts
echo "=== Current snippet/search implementation ==="
sed -n '205,275p' src/main/db/queries/search.ts
sed -n '1,80p' src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
echo "=== Tests ==="
for f in test/src/main/db/queries/lyricsIndex.test.ts test/src/main/core/getSongLyrics.test.ts; do
[ -f "$f" ] && { echo "--- $f ---"; sed -n '1,280p' "$f"; }
done
echo "=== Migration prefix and patch integrity ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
old = json.loads(subprocess.check_output(["git","show",f"{target}:resources/drizzle/meta/_journal.json"],text=True))["entries"]
new = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("journal_prefix_unchanged=", old == new[:len(old)])
print("new_entries=", new[len(old):])
PY
git diff --check "$base" HEAD || trueLength of output: 44318
VerdictNot Ready — high confidence. The branch is MERGEABLE with
The migration chain is append-only. The target journal prefix is unchanged. This PR adds only Major findings
|
…ve retry, lint-clean snippet split Major 1 (marker protocol inconsistent): the CTE revert in the prior round restored [NRABEG]/[NRAEND] in searchSongsByLyrics while HighlightedSnippet split on SOH/STX. Restored StartSel=\u0001, StopSel=\u0002 so both boundaries use one protocol. Major 2 (post-save retry not persisted): saveLyricsToLrcFile and savePendingSongLyrics now persist isLyricIndexBuilt: false alongside invalidateLyricsIndex(), so a read-error survives restart even when no backfill is in flight. Major 3 (CI lint fails): HighlightedSnippet's control-char regex violated eslint no-control-regex. Replaced the regex split with a char-code splitter (SOH/STX markers), keeping the protocol and passing lint. Nitpicks: renamed the generation comment to describe in-process invalidation + persisted setting; backfill completion log now notes when generation changed. 345 tests pass, typecheck clean, feature files 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. 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
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target="release/4.0.0-alpha.5"
echo "=== PR state and CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf '\nHEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD \
&& echo "target_tip_contained=yes" \
|| echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 \
&& echo "merge_tree=clean" \
|| echo "merge_tree=conflict_or_error"
echo "=== Complete target-relative changed files ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration history ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
old = json.loads(subprocess.check_output(
["git", "show", f"{target}:resources/drizzle/meta/_journal.json"], text=True
))["entries"]
with open("resources/drizzle/meta/_journal.json") as f:
new = json.load(f)["entries"]
print("target_prefix_unchanged=", old == new[:len(old)])
print("added_entries=", new[len(old):])
PY
git diff --find-renames --unified=35 "$base" HEAD -- resources/drizzle
echo "=== Changed-code structure ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo "=== Full target-relative source diff, excluding generated snapshot ==="
git diff --find-renames --unified=40 "$base" HEAD -- \
src resources/drizzle \
':(exclude)resources/drizzle/meta/0005_snapshot.json' | sed -n '1,7500p'
echo "=== Current feature implementations ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,380' \
'src/main/db/queries/search.ts:180,340' \
'src/main/search.ts:1,180' \
'src/main/main.ts:250,350' \
'src/main/parseSong/parseSong.ts:230,340' \
'src/main/core/getSongLyrics.ts:1,250' \
'src/main/core/saveLyricsToLrcFile.ts:1,290' \
'src/main/saveLyricsToSong.ts:1,230' \
'src/main/updateSong/updateSongId3Tags.ts:90,190' \
'src/main/updateSong/updateSongId3Tags.ts:730,830' \
'src/main/updateSong/updateSongId3Tags.ts:1060,1160' \
'src/main/db/schema.ts:370,560' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,180' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,260' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,260' \
'src/renderer/src/other/parseNotificationFromMain.tsx:1,150' \
'src/renderer/src/routes/main-player/search/index.tsx:1,340' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,220' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx:1,160' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,180' \
'src/renderer/src/queries/search.ts:1,140' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,160' \
'src/types/app.d.ts:800,940'
do
file="${spec%%:*}"; range="${spec#*:}"
[ -f "$file" ] && { echo "--- $file [$range] ---"; sed -n "$range" "$file" | nl -ba; }
done
echo "=== Callers, state writes, IPC, and preload ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(invalidateLyricsIndex|lyricsIndexGeneration|countBackfillResults|upsertSongLyrics|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics|saveUserSettings)\s*\(' \
src test 2>/dev/null || true
echo "--- IPC/preload files changed by this PR ---"
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|LyricsSearchResult|SearchFilters)' \
src/main src/preload src/renderer/src src/types
echo "=== Tests and available project scripts ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
fd -a -t f -E node_modules | rg -i '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r file; do
echo "--- $file ---"
cat -n "$file"
done
jq '.scripts' package.jsonLength of output: 100728 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== PR merge state and CI ==="
gh pr view 528 --repo "$repo" \
--json mergeable,mergeStateStatus,headRefOid,baseRefOid,statusCheckRollup \
--jq '{mergeable,mergeStateStatus,headRefOid,baseRefOid,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]}' || true
printf 'HEAD=%s TARGET=%s BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 && echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
echo "=== Changed files ==="
git diff --name-status "$base" HEAD
echo "=== Patch check ==="
git diff --check "$base" HEAD || true
echo "=== Journal append-only check ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
old = json.loads(subprocess.check_output(["git","show",f"{target}:resources/drizzle/meta/_journal.json"], text=True))["entries"]
new = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("prefix_unchanged=", old == new[:len(old)])
print("added_entries=", new[len(old):])
PY
echo "=== Exact marker protocol ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'StartSel|StopSel|ts_headline|NRABEG|NRAEND|codePointAt|0x0001|0x0002|u0001|u0002' \
src/main/db/queries/search.ts \
src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
echo "=== Exact invalidation and persistence paths ==="
rg -n -C7 --glob '*.ts' \
'invalidateLyricsIndex|isLyricIndexBuilt|saveUserSettings|read-error|upsertSongLyrics' \
src/main/db/queries/lyricsIndex.ts \
src/main/main.ts \
src/main/parseSong/parseSong.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts \
src/main/updateSong/updateSongId3Tags.ts
echo "=== SaveLyricsToLRCFile callers ==="
rg -n -C6 --glob '*.ts' '\bsaveLyricsToLRCFile\s*\(' src/main
echo "=== Full changed code excluding snapshot ==="
git diff --find-renames --unified=20 "$base" HEAD -- \
src resources/drizzle \
':(exclude)resources/drizzle/meta/0005_snapshot.json' | sed -n '1,5000p'
echo "=== Current focused file contents ==="
for file in \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts \
src/main/parseSong/parseSong.ts \
src/renderer/src/components/SearchPage/HighlightedSnippet.tsx \
test/src/main/db/queries/lyricsIndex.test.ts \
test/src/main/core/getSongLyrics.test.ts
do
if [ -f "$file" ]; then
echo "--- $file ---"
cat -n "$file"
fi
done
echo "=== IPC/preload changes and search contracts ==="
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
rg -n -C4 --glob '*.{ts,tsx}' \
'ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|SearchFilters|LyricsSearchResult' \
src/main src/preload src/renderer/src src/types
echo "=== Scripts ==="
jq '.scripts' package.jsonLength of output: 221967
VerdictNot Ready — high confidence. The branch is MERGEABLE with
The migration chain is append-only. The target journal prefix is unchanged. This PR adds only migration Major findings
|
…yric index retry on save errors Major 1: ts_headline StartSel/StopSel now emit SOH (0x01)/STX (0x02) control chars matching HighlightedSnippet's parser (was emitting visible [NRABEG]/[NRAEND] brackets the renderer never split on). Marked words now render in <mark>. Major 2: saveLyricsToLrcFile and saveLyricsToSong now persist isLyricIndexBuilt:false via saveUserSettings after a post-save 'read-error', so a handled re-index failure retries on next startup instead of leaving a stale completed index. Nitpick: removed unused isHighlight toggle in HighlightedSnippet. Added HighlightedSnippet render test (marker parsing -> <mark>, plain span, trailing-opening-marker case). 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. 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"
target="release/4.0.0-alpha.5"
echo "=== PR merge state and CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
statusCheckRollup: [.statusCheckRollup[] | {name,status,conclusion,detailsUrl}]
}' || true
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf '\nHEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 \
&& echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
echo "=== Full target-relative inventory ==="
git diff --name-status "$base" HEAD
git diff --stat "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration journal and SQL delta ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
before = json.loads(subprocess.check_output(
["git", "show", f"{target}:resources/drizzle/meta/_journal.json"], text=True
))["entries"]
with open("resources/drizzle/meta/_journal.json") as f:
after = json.load(f)["entries"]
print("journal_prefix_unchanged=", before == after[:len(before)])
print("added_entries=", after[len(before):])
PY
git diff --find-renames --unified=30 "$base" HEAD -- resources/drizzle
echo "=== Changed TypeScript/TSX structure ==="
git diff --name-only --diff-filter=ACMR "$base" HEAD -- '*.ts' '*.tsx' |
while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all || true
done
echo "=== Full source patch, excluding generated schema snapshot ==="
git diff --find-renames --unified=35 "$base" HEAD -- \
src resources/drizzle \
':(exclude)resources/drizzle/meta/0005_snapshot.json' | sed -n '1,8000p'
echo "=== Current feature code ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:1,400' \
'src/main/db/queries/search.ts:180,350' \
'src/main/search.ts:1,190' \
'src/main/main.ts:250,360' \
'src/main/parseSong/parseSong.ts:225,350' \
'src/main/core/getSongLyrics.ts:1,270' \
'src/main/core/saveLyricsToLrcFile.ts:1,310' \
'src/main/saveLyricsToSong.ts:1,250' \
'src/main/updateSong/updateSongId3Tags.ts:80,1180' \
'src/main/db/schema.ts:360,580' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,200' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,280' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,280' \
'src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx:1,190' \
'src/renderer/src/components/SearchPage/SearchOptions.tsx:1,160' \
'src/renderer/src/routes/main-player/search/index.tsx:1,360' \
'src/renderer/src/routes/main-player/search/all/index.tsx:1,240' \
'src/renderer/src/other/parseNotificationFromMain.tsx:1,170' \
'src/renderer/src/queries/search.ts:1,150' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,170' \
'src/types/app.d.ts:790,960'
do
file="${spec%%:*}"; range="${spec#*:}"
if [ -f "$file" ]; then
echo "--- $file [$range] ---"
sed -n "$range" "$file" | nl -ba
fi
done
echo "=== Callers, state writes, IPC/preload contracts ==="
rg -n -C6 --glob '*.{ts,tsx}' \
'\b(invalidateLyricsIndex|lyricsIndexGeneration|countBackfillResults|upsertSongLyrics|removeSongLyrics|indexAllLyrics|saveLyricsToLRCFile|savePendingSongLyrics|searchSongsByLyrics|saveUserSettings)\s*\(' \
src test 2>/dev/null || true
echo "--- changed IPC/preload files ---"
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
rg -n -C5 --glob '*.{ts,tsx}' \
'(ipcMain\.(handle|on)|contextBridge\.exposeInMainWorld|searchPageSchema|LyricsSearchResult|SearchFilters)' \
src/main src/preload src/renderer/src src/types
echo "=== Tests and scripts ==="
git diff --name-status "$base" HEAD -- 'test/**' '**/*.test.ts' '**/*.test.tsx' '**/*.spec.ts' '**/*.spec.tsx'
fd -a -t f -E node_modules | rg -i '(lyrics|search|backfill).*\.(test|spec)\.(ts|tsx)$|getSongLyrics.*\.(test|spec)\.(ts|tsx)$' | sort |
while IFS= read -r file; do
echo "--- $file ---"
cat -n "$file"
done
echo "--- package scripts ---"
jq '.scripts' package.jsonLength of output: 100289 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
echo "=== PR state and target ancestry ==="
gh pr view 528 --repo "$repo" \
--json mergeable,mergeStateStatus,headRefOid,baseRefOid,statusCheckRollup \
--jq '{mergeable,mergeStateStatus,headRefOid,baseRefOid,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]}' || true
printf 'HEAD=%s\nTARGET=%s\nBASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 && echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
git diff --check "$base" HEAD || true
echo "=== Changed file inventory ==="
git diff --name-status "$base" HEAD
echo "=== Exact marker protocol ==="
rg -n -C5 --glob '*.{ts,tsx}' \
"ts_headline|StartSel|StopSel|codePointAt|0x0001|0x0002|\\\\x01|\\\\x02" \
src/main/db/queries/search.ts \
src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
echo "=== Retry persistence and generation lifecycle ==="
for spec in \
'src/main/db/queries/lyricsIndex.ts:70,235' \
'src/main/main.ts:280,320' \
'src/main/parseSong/parseSong.ts:265,315' \
'src/main/core/saveLyricsToLrcFile.ts:125,205' \
'src/main/saveLyricsToSong.ts:85,165'
do
file="${spec%%:*}"; range="${spec#*:}"
echo "--- $file [$range] ---"
sed -n "$range" "$file" | cat -n
done
echo "=== Every LRC save caller ==="
rg -n -C8 --glob '*.ts' '\bsaveLyricsToLRCFile\s*\(' src/main
echo "=== Search, route, and renderer contracts ==="
for spec in \
'src/main/search.ts:1,145' \
'src/renderer/src/components/SearchPage/HighlightedSnippet.tsx:1,130' \
'src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx:1,150' \
'src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx:1,180' \
'src/renderer/src/routes/main-player/search/index.tsx:100,270' \
'src/renderer/src/routes/main-player/search/all/index.tsx:35,145' \
'src/renderer/src/utils/zod/searchPageSchema.ts:1,100' \
'src/renderer/src/queries/search.ts:1,90'
do
file="${spec%%:*}"; range="${spec#*:}"
echo "--- $file [$range] ---"
sed -n "$range" "$file" | cat -n
done
echo "=== Current test coverage ==="
for file in test/src/main/db/queries/lyricsIndex.test.ts test/src/main/core/getSongLyrics.test.ts; do
if [ -f "$file" ]; then
echo "--- $file ---"
cat -n "$file"
fi
done
echo "--- all direct feature test references ---"
rg -n --glob '*.{test,spec}.{ts,tsx}' \
'\b(indexAllLyrics|upsertSongLyrics|searchSongsByLyrics|HighlightedSnippet|saveLyricsToLRCFile|savePendingSongLyrics|AllLyricResults|LyricSearchResultsContainer)\b' \
test src 2>/dev/null || true
echo "=== Migration and boundary changes ==="
git diff --unified=20 "$base" HEAD -- resources/drizzle src/main/ipc.ts src/preload/index.ts
echo "--- scripts ---"
jq '.scripts' package.jsonLength of output: 8290 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target_ref="refs/remotes/origin/release/4.0.0-alpha.5"
base="$(git merge-base HEAD "$target_ref")"
show_range() {
file="$1"
start="$2"
end="$3"
echo "--- $file: Lines $start-$end ---"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo "=== Current PR state and CI ==="
gh pr view 528 --repo "$repo" \
--json mergeable,mergeStateStatus,statusCheckRollup \
--jq '{mergeable,mergeStateStatus,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion}]}' || true
echo "=== Migration prefix and SQL ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.5"
base_entries = json.loads(subprocess.check_output(
["git", "show", f"{target}:resources/drizzle/meta/_journal.json"], text=True
))["entries"]
head_entries = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("prefix_unchanged=", base_entries == head_entries[:len(base_entries)])
print("appended=", head_entries[len(base_entries):])
PY
cat -n resources/drizzle/0005_many_nightshade.sql
echo "=== Index lifecycle and persistence ==="
show_range src/main/db/queries/lyricsIndex.ts 1 250
show_range src/main/main.ts 280 320
show_range src/main/parseSong/parseSong.ts 265 315
show_range src/main/core/saveLyricsToLrcFile.ts 115 210
show_range src/main/saveLyricsToSong.ts 80 170
echo "=== All saveLyricsToLRCFile call sites ==="
rg -n -C7 --glob '*.ts' '\bsaveLyricsToLRCFile\s*\(' src/main
echo "=== Search and renderer data flow ==="
show_range src/main/db/queries/search.ts 215 270
show_range src/main/search.ts 1 135
show_range src/renderer/src/components/SearchPage/HighlightedSnippet.tsx 1 115
show_range src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx 1 130
show_range src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx 1 155
show_range src/renderer/src/routes/main-player/search/all/index.tsx 35 140
show_range src/renderer/src/routes/main-player/search/index.tsx 160 245
show_range src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx 1 115
show_range src/renderer/src/components/SearchPage/SearchOptions.tsx 1 90
show_range src/renderer/src/utils/zod/searchPageSchema.ts 1 90
show_range src/renderer/src/queries/search.ts 1 80
echo "=== IPC/preload delta ==="
git diff --name-status "$base" HEAD -- src/main/ipc.ts src/preload/index.ts
echo "=== Test contents ==="
for file in \
test/src/main/core/getSongLyrics.test.ts \
test/src/main/db/queries/lyricsIndex.test.ts \
test/src/renderer/src/components/SearchPage/HighlightedSnippet.test.ts
do
echo "--- $file ---"
cat -n "$file"
done
echo "=== UI and security-related changes ==="
git diff --unified=25 "$base" HEAD -- \
src/renderer/src/assets/locales/en/en.json \
src/renderer/src/other/parseNotificationFromMain.tsx \
src/types/app.d.ts \
src/main/core/getSongLyrics.ts \
src/main/db/schema.tsLength of output: 93914
VerdictReady — medium confidence. The branch is MERGEABLE with
I performed a second pass on each possible finding. I did not find a defect that survived the cause, action, reaction, and necessity checks. Major findingsNone. Minor findingsNone. Nitpick findingsNone. UI review findingsNo blocking UI issue found.
Missing test coverageThe PR adds focused tests for:
The following behavior remains without direct automated coverage:
These are coverage gaps. They are not confirmed runtime defects. What is working correctlyMerge and migration safety
IPC boundary
Lifecycle and persistence
Search and security
Renderer behavior
Verdict: Ready — medium confidenceYou are interacting with an AI system. |
|
@CodeRabbit the search feature doesn't work deeply investigate why |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== IPC search handler ==="
rg -n -B5 -A40 "ipcMain\.(handle|on)\(['\"].*search" src/main/ipc.ts src/main/main.ts 2>/dev/null || true
rg -n -C10 "search" src/main/ipc.ts | head -150
echo "=== preload search binding ==="
rg -n -C10 "search" src/preload/index.ts | head -150
echo "=== renderer queries/search.ts ==="
cat -n src/renderer/src/queries/search.ts
echo "=== main/search.ts full ==="
cat -n src/main/search.ts
echo "=== db/queries/search.ts full ==="
cat -n src/main/db/queries/search.ts
echo "=== search route index.tsx full ==="
cat -n src/renderer/src/routes/main-player/search/index.tsxLength of output: 40111 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== searchPageSchema.ts ==="
cat -n src/renderer/src/utils/zod/searchPageSchema.ts
echo "=== SearchOptions.tsx ==="
cat -n src/renderer/src/components/SearchPage/SearchOptions.tsx
echo "=== convertToSongData / convertToGenre relevant snippet ==="
rg -n -A20 "export const convertToSongData" src/main/utils/convert.ts | head -60
rg -n -A15 "export const convertToGenre" src/main/utils/convert.ts
echo "=== db schema relations for songLyrics ==="
rg -n -B3 -A20 "songLyrics" src/main/db/schema.ts
echo "=== db/db.ts (migration execution on startup) ==="
cat -n src/main/db/db.ts
echo "=== Recent commits touching search files ==="
git log --oneline -15 -- src/main/search.ts src/main/db/queries/search.ts src/renderer/src/routes/main-player/search/index.tsx src/renderer/src/queries/search.ts
echo "=== MostRelevantSearchResultsContainer (uses searchResults) ==="
fd -a 'MostRelevantSearchResultsContainer.tsx' -x cat -n {}
echo "=== NoSearchResultsContainer.tsx ==="
cat -n src/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx
echo "=== SearchStartPlaceholder.tsx ==="
fd -a 'SearchStartPlaceholder.tsx' -x cat -n {}Length of output: 35359 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Check node/npm availability and try installing pglite standalone for query test ==="
node --version || true
npm --version || true
mkdir -p /tmp/pglite-test && cd /tmp/pglite-test
npm init -y >/dev/null 2>&1 || true
npm install `@electric-sql/pglite` --no-audit --no-fund 2>&1 | tail -20 || true
cat > test.mjs << 'EOF'
import { PGlite } from '`@electric-sql/pglite`';
const db = new PGlite();
await db.exec(`
CREATE TABLE songs (id serial primary key, title text);
CREATE TABLE song_lyrics (
song_id integer primary key references songs(id),
lyrics_text text not null,
source varchar(20) not null,
lyrics_vector tsvector generated always as (to_tsvector('simple', lyrics_text)) stored,
created_at timestamp default now() not null,
updated_at timestamp default now() not null
);
INSERT INTO songs (title) VALUES ('Test Song');
INSERT INTO song_lyrics (song_id, lyrics_text, source) VALUES (1, 'never gonna give you up never gonna let you down', 'LRC');
`);
const keyword = 'never gonna';
try {
const res = await db.query(
`select s.*,
ts_headline('simple', sl.lyrics_text, phraseto_tsquery('simple', $1), 'StartSel=' || E'\\x01' || ', StopSel=' || E'\\x02' || ', MaxWords=12, MinWords=4, ShortWord=2') as snippet,
sl.source
from song_lyrics sl
inner join songs s on sl.song_id = s.id
where sl.lyrics_vector @@ phraseto_tsquery('simple', $2)
order by ts_rank(sl.lyrics_vector, phraseto_tsquery('simple', $3)) desc
limit 100`,
[keyword, keyword, keyword]
);
console.log('SUCCESS:', JSON.stringify(res.rows, null, 2));
console.log('snippet bytes:', [...res.rows[0].snippet].map(c => c.codePointAt(0)));
} catch (err) {
console.error('QUERY FAILED:', err);
}
EOF
node test.mjsLength of output: 2670 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== HighlightedSnippet.tsx current content ==="
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
echo "=== LyricSearchResultsContainer.tsx current content ==="
cat -n src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
echo "=== AllLyricResults.tsx current content ==="
cat -n src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
echo "=== check types alignment app.d.ts LyricsSearchResult ==="
rg -n -B2 -A15 "interface LyricsSearchResult" src/types/app.d.ts
echo "=== check convertToSongData return shape used by lyrics search vs GetAllSongsReturnType typing mismatch ==="
rg -n "GetAllSongsReturnType" src/main/db/queries/songs.ts | head -5
rg -n -B3 -A25 "export type GetAllSongsReturnType" src/main/db/queries/songs.tsLength of output: 13022 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Full convertToSongData function ==="
rg -n -B2 -A60 "export const convertToSongData" src/main/utils/convert.ts
echo "=== check other convertTo* to see artwork handling pattern ==="
rg -n "\.artworks" src/main/utils/convert.ts
echo "=== Simulate: what columns does the lyrics search select({song: songs}) actually produce vs full songs.findMany with relations ==="
rg -n -B3 -A15 "export const songs = pgTable" src/main/db/schema.tsLength of output: 4376 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Check for existing helper to get single/many songs with full relations (artworks/artists/albums/genres) ==="
rg -n "with:\s*\{" -B5 src/main/db/queries/songs.ts | head -80
echo "=== getSongById or similar helper ==="
rg -n "export const getSong" src/main/db/queries/*.ts
echo "=== Check if songs.findMany with 'where: inArray(songs.id, ...)' pattern exists elsewhere for reuse ==="
rg -n "inArray(songs.id" src/main -A5 -B5Length of output: 4396 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== getSongsInPathList implementation ==="
sed -n '560,680p' src/main/db/queries/songs.ts
echo "=== getSongsByNames implementation ==="
sed -n '430,500p' src/main/db/queries/songs.tsLength of output: 5423
Root causeFile: The query selects only the raw const results = await trx
.select({
song: songs,
snippet: sql<string>`ts_headline(...)`,
source: songLyrics.source
})
.from(songLyrics)
.innerJoin(songs, eq(songLyrics.songId, songs.id))
...This does not join The code then casts this flat row to the shape used by all other search paths: return results.map((result) => ({
song: convertToSongData(result.song as GetAllSongsReturnType[number]),
...
}));
const artworks = song.artworks.map((a) => a.artwork);
What happens at run time
VerificationI reproduced this with a live PGlite instance. The SQL query itself is correct. It returns the song row and the highlighted snippet with the correct SOH/STX markers. The failure happens only in the post-query JavaScript conversion step, not in the database. ImpactEvery search that matches a lyric fails silently. The Lyrics tab always shows zero results, even for exact phrase matches, because the error is caught and swallowed. Only a This explains "the search feature doesn't work": the Lyrics feature added by this PR cannot return a single result under normal use. Fix
Option A — two-step query, reuse existing relation shape: export const searchSongsByLyrics = async (options: SearchOptions, trx: DB | DBTransaction = db) => {
const { keyword } = options;
const timer = timeStart();
try {
const matches = await trx
.select({
songId: songLyrics.songId,
snippet: sql<string>`ts_headline(...)`,
source: songLyrics.source,
rank: sql<number>`ts_rank(${songLyrics.lyricsVector}, phraseto_tsquery('simple', ${keyword}))`
})
.from(songLyrics)
.where(sql`${songLyrics.lyricsVector} @@ phraseto_tsquery('simple', ${keyword})`)
.orderBy(sql`ts_rank(${songLyrics.lyricsVector}, phraseto_tsquery('simple', ${keyword})) DESC`)
.limit(100);
if (matches.length === 0) return [];
const songRows = await trx.query.songs.findMany({
where: inArray(songs.id, matches.map((m) => m.songId)),
with: {
artists: { with: { artist: { columns: { id: true, name: true } } } },
albums: { with: { album: { columns: { id: true, title: true }, with: { artists: { with: { artist: { columns: { id: true, name: true } } } } } } } },
genres: { with: { genre: { columns: { id: true, name: true } } } },
artworks: { with: { artwork: { with: { palette: { columns: { id: true }, with: { swatches: {} } } } } } },
playlists: { with: { playlist: { columns: { id: true, name: true } } } }
}
});
const songById = new Map(songRows.map((s) => [s.id, s]));
timeEnd(timer, 'Search Songs By Lyrics');
return matches
.filter((m) => songById.has(m.songId))
.map((m) => ({
song: convertToSongData(songById.get(m.songId)! as GetAllSongsReturnType[number]),
matchedLyricSnippet: m.snippet,
source: m.source as 'LRC' | 'EMBEDDED' | 'BOTH'
}));
} catch (error) {
logger.error('Lyrics search failed', { error, keyword });
timeEnd(timer, 'Search Songs By Lyrics (failed)');
return [];
}
};This preserves rank order from the first query while attaching full relation data from the second. Option B (defensive, smaller diff, but does not fix missing artwork/artist/album/genre data in the UI): add Recommendation: apply Option A. Also add a regression test that inserts a song with lyrics, calls I withdraw my previous "Ready" verdict. This PR is not ready until this fix lands, since it is a full-feature-breaking regression in the exact feature the PR delivers. (=｀ω´=) tracked it down ✏️ Learnings added
You are interacting with an AI system. |
searchSongsByLyrics() was selecting only a flat songs row (no artist/album/ genre/artwork/playlist relations) and casting it to GetAllSongsReturnType before convertToSongData(). convertToSongData() accesses song.artworks.map() without optional chaining, so the undefined artworks field threw inside the try block and lyrics search always returned []. Rewrite as a two-step query: the tsvector match query returns songId + snippet + source, then a trx.query.songs.findMany with the full relation shape (same as getAllSongs) supplies complete song data, mapped back in rank order. Also harden convertToSongData() to guard song.artworks with optional chaining, matching the existing pattern for artists/albums/genres, so any future caller with a non-relational row degrades gracefully instead of throwing. Adds regression tests covering populated relations, empty results, and rank-order preservation.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch Context: prior finding already fixedYour Critical finding (lyrics search always returns [] because
ScopeReview the FULL diff of this PR against 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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
target="release/4.0.0-alpha.6"
echo "=== PR state and CI ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]}' || true
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf '\nHEAD=%s\nTARGET=%s\nMERGE_BASE=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git merge-base --is-ancestor "$target_ref" HEAD && echo "target_tip_contained=yes" || echo "target_tip_contained=no"
git merge-tree --write-tree "$target_ref" HEAD >/dev/null 2>&1 && echo "merge_tree=clean" || echo "merge_tree=conflict_or_error"
echo "=== Changed files ==="
git diff --name-status "$base" HEAD
echo "=== Patch integrity ==="
git diff --check "$base" HEAD || true
echo "=== Migration journal prefix ==="
python3 - <<'PY'
import json, subprocess
target = "refs/remotes/origin/release/4.0.0-alpha.6"
old = json.loads(subprocess.check_output(["git","show",f"{target}:resources/drizzle/meta/_journal.json"], text=True))["entries"]
new = json.load(open("resources/drizzle/meta/_journal.json"))["entries"]
print("prefix_unchanged=", old == new[:len(old)])
print("added_entries=", new[len(old):])
PY
echo "=== searchSongsByLyrics current implementation ==="
sed -n '1,300p' src/main/db/queries/search.ts | cat -n
echo "=== convert.ts convertToSongData ==="
rg -n -B3 -A40 "export const convertToSongData" src/main/utils/convert.ts
echo "=== getAllSongs relation shape for comparison ==="
rg -n -B3 -A40 "export const getAllSongs" src/main/db/queries/songs.ts
echo "=== new regression test ==="
fd -a 'searchSongsByLyrics.test.ts' -x cat -n {}Length of output: 25133 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '300,340p' src/main/db/queries/search.ts | cat -n
echo "=== HighlightedSnippet.tsx test ==="
cat -n test/src/renderer/src/components/SearchPage/HighlightedSnippet.test.ts
echo "=== HighlightedSnippet.tsx ==="
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsxLength of output: 5688
VerdictReady — high confidence. The branch is MERGEABLE with
Verification of the prior Critical findingI read the current code at
I traced the call sites and consumers. I ran the second-pass self-review (cause, action, reaction, necessity) against this diff. I found no new defect that survives all four checks. Major findingsNone. Minor findingsNone. Missing test coverageThe following behaviors remain without direct automated coverage. None of these are confirmed defects; they are coverage gaps.
What is working correctly
(=^･ω･^=) tracked and closed [resolve] You are interacting with an AI system. |
|
you need to talk back and forth and reason with ai agents about this implementation to iron out details. For example, i feel like its pointless to hold onto lyrics in the database just for search. |
thought as much, will look deeper into implementation |
|
@CodeRabbit intelligently fix the merge conflict |
|
This command requires write access to the repository. Only users with write or admin permissions can trigger CodeRabbit to commit or create pull requests. |
@CodeRabbit look deeper into the architecture , it can be better |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
target="release/4.0.0-alpha.6"
repo="Sandakan/Nora"
echo "=== PR and target state ==="
gh pr view 528 --repo "$repo" \
--json baseRefName,baseRefOid,headRefOid,mergeable,mergeStateStatus \
--jq '{baseRefName,baseRefOid,headRefOid,mergeable,mergeStateStatus}' || true
git fetch --no-tags origin "refs/heads/$target:refs/remotes/origin/$target" >/dev/null 2>&1 || true
target_ref="refs/remotes/origin/$target"
base="$(git merge-base HEAD "$target_ref")"
printf 'head=%s\ntarget=%s\nmerge_base=%s\n' \
"$(git rev-parse HEAD)" "$(git rev-parse "$target_ref")" "$base"
git diff --name-status "$base" HEAD
echo "=== Changed architecture map ==="
for file in \
src/main/db/schema.ts \
resources/drizzle/0005_many_nightshade.sql \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/core/getSongLyrics.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts \
src/main/parseSong/parseSong.ts \
src/main/main.ts \
src/main/search.ts \
src/types/app.d.ts
do
if [ -f "$file" ]; then
echo "--- outline: $file ---"
ast-grep outline "$file" --items all || true
fi
done
echo "=== Schema, migration, index, and search query ==="
sed -n '380,540p' src/main/db/schema.ts | nl -ba
cat -n resources/drizzle/0005_many_nightshade.sql
sed -n '200,335p' src/main/db/queries/search.ts | nl -ba
echo "=== Indexing and source lifecycle ==="
cat -n src/main/db/queries/lyricsIndex.ts
sed -n '1,220p' src/main/core/getSongLyrics.ts | nl -ba
sed -n '110,220p' src/main/core/saveLyricsToLrcFile.ts | nl -ba
sed -n '1,190p' src/main/saveLyricsToSong.ts | nl -ba
sed -n '250,325p' src/main/parseSong/parseSong.ts | nl -ba
sed -n '275,330p' src/main/main.ts | nl -ba
echo "=== All lyric consumers and source readers ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'\b(songLyrics|lyricsText|lyricsVector|matchedLyricSnippet|fetchLyricsFromLRCFile|getSongLyrics|readEmbeddedLyrics|readLrcLyrics|upsertSongLyrics|searchSongsByLyrics)\b' \
src test
echo "=== Existing song relation and search conventions ==="
ast-grep outline src/main/db/queries/songs.ts --items all || true
rg -n -C5 --glob '*.ts' \
'\b(getAllSongs|searchSongsByName|convertToSongData|ts_headline|to_tsvector|phraseto_tsquery)\b' \
src/main
echo "=== Tests that establish current contracts ==="
for file in \
test/src/main/db/queries/searchSongsByLyrics.test.ts \
test/src/main/db/queries/lyricsIndex.test.ts \
test/src/main/core/getSongLyrics.test.ts \
test/src/renderer/src/components/SearchPage/HighlightedSnippet.test.ts
do
if [ -f "$file" ]; then
echo "--- $file ---"
cat -n "$file"
fi
done
echo "=== Target-relative patch for architecture-relevant files ==="
git diff --find-renames --unified=35 "$base" HEAD -- \
src/main/db/schema.ts \
resources/drizzle \
src/main/db/queries/lyricsIndex.ts \
src/main/db/queries/search.ts \
src/main/core/getSongLyrics.ts \
src/main/core/saveLyricsToLrcFile.ts \
src/main/saveLyricsToSong.ts \
src/main/parseSong/parseSong.ts \
src/main/main.ts \
src/main/search.ts \
src/types/app.d.ts \
test/src/main/db/queries/searchSongsByLyrics.test.ts \
test/src/main/db/queries/lyricsIndex.test.ts \
test/src/main/core/getSongLyrics.test.ts \
test/src/renderer/src/components/SearchPage/HighlightedSnippet.test.tsLength of output: 38449 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Schema and migration ==="
sed -n '400,525p' src/main/db/schema.ts | cat -n
cat -n resources/drizzle/0005_many_nightshade.sql
echo "=== Lyrics search implementation ==="
sed -n '210,340p' src/main/db/queries/search.ts | cat -n
sed -n '1,145p' src/main/search.ts | cat -n
echo "=== Indexing implementation ==="
cat -n src/main/db/queries/lyricsIndex.ts
echo "=== Canonical lyric source read/write paths ==="
sed -n '15,145p' src/main/core/getSongLyrics.ts | cat -n
sed -n '115,220p' src/main/core/saveLyricsToLrcFile.ts | cat -n
sed -n '1,180p' src/main/saveLyricsToSong.ts | cat -n
sed -n '250,325p' src/main/parseSong/parseSong.ts | cat -n
sed -n '275,330p' src/main/main.ts | cat -n
rg -n -C4 --glob '*.ts' \
'\b(synchronizedLyrics|unsynchronizedLyrics|saveLyricsToLRCFile|fetchLyricsFromAudioSource|fetchLyricsFromLRCFile|readEmbeddedLyrics|readLrcLyrics)\b' \
src/main
echo "=== Song query relation contract and conversion ==="
rg -n -A85 -B5 'export const getAllSongs|export type GetAllSongsReturnType' \
src/main/db/queries/songs.ts
rg -n -A65 -B5 'export const convertToSongData' src/main/utils/convert.ts
echo "=== Search result and renderer contract ==="
sed -n '845,880p' src/types/app.d.ts | cat -n
cat -n src/renderer/src/components/SearchPage/HighlightedSnippet.tsx
cat -n src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx
cat -n src/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx
echo "=== Tests ==="
for file in \
test/src/main/db/queries/searchSongsByLyrics.test.ts \
test/src/main/db/queries/lyricsIndex.test.ts \
test/src/main/core/getSongLyrics.test.ts \
test/src/renderer/src/components/SearchPage/HighlightedSnippet.test.ts
do
if [ -f "$file" ]; then
echo "--- $file ---"
cat -n "$file"
fi
done
echo "=== Size/retention and deletion paths ==="
rg -n -C5 --glob '*.{ts,sql}' \
'\b(removeSongLyrics|delete\(songLyrics\)|delete\(songs\)|onDelete.*cascade|lyrics_text|song_lyrics)\b' \
src resources/drizzle testLength of output: 113020
Your concern is valid. The database must not become an accidental second canonical lyrics store. Current architectureThe current design has three layers:
The application does not use Is retaining
|
This branch has not been deployed
2 participants
Summary
Adds a lyrics search feature that lets users find songs by searching for lyric text. Results appear in a dedicated Lyrics tab within the search modal, with matched phrases highlighted inline.
Architecture
Database-backed full-text search using PostgreSQL tsvector + GIN index for sub-millisecond queries, even on 10,000+ song libraries. The approach avoids runtime file scanning by indexing lyrics at import time and via a one-time startup backfill.
Changes by file
Database
resources/drizzle/0005_many_nightshade.sql- New song_lyrics table with generated tsvector column, GIN index, and is_lyric_index_built user setting (drizzle-kit generated)src/main/db/schema.ts- songLyrics table definition with custom tsvector typesrc/main/db/queries/lyricsIndex.ts- Core indexing logic: reads embedded + LRC lyrics, upserts to song_lyrics, batch backfill with Promise.allSettled, tri-state read helpers (string/undefined/null) to preserve index rows on transient I/O errors. getUserSettings hoisted out of per-song path to avoid N+1 DB round-trips.Search
src/main/db/queries/search.ts- searchSongsByLyrics using phraseto_tsquery + ts_rank + ts_headline for snippet generationsrc/main/search.ts- Wired lyrics into the existing Promise.all search flow, added lyrics to the no-results fallback checkImport/save indexing
src/main/parseSong/parseSong.ts- Dynamic import of lyricsIndex on song import to avoid circular dependencysrc/main/main.ts- Background indexAllLyrics() backfill triggered on startup (one-time, guarded by is_lyric_index_built)src/main/core/saveLyricsToLrcFile.ts- Re-indexes lyrics in DB after saving to LRC filesrc/main/saveLyricsToSong.ts- Re-indexes lyrics after embedding into audio fileFrontend
src/renderer/src/components/SearchPage/HighlightedSnippet.tsx- XSS-safe snippet renderer (splits on b tags from ts_headline, renders with mark)src/renderer/src/components/SearchPage/Result_Containers/LyricSearchResultsContainer.tsx- Lyrics results section in search pagesrc/renderer/src/components/SearchPage/All_Search_Result_Containers/AllLyricResults.tsx- Full lyrics results viewsrc/renderer/src/components/SearchPage/NoSearchResultsContainer.tsx- Added lyrics to no-results conditionsrc/renderer/src/components/SearchPage/SearchOptions.tsx- Added Lyrics filter typesrc/renderer/src/routes/main-player/search/index.tsx- Added lyrics section in search pagesrc/renderer/src/routes/main-player/search/all/index.tsx- Added lyrics rendering in all-results viewTypes and i18n
src/types/app.d.ts- LyricsSearchResult interface, SearchFilters union extended with Lyricssrc/renderer/src/assets/locales/en/en.json- common.lyric_other keyCloses #527
Summary by CodeRabbit
Fixes #527