fix(#518): fix blacklist preference persistence and song filter state - #521
fix(#518): fix blacklist preference persistence and song filter state#521Owie6789 wants to merge 5 commits into
Conversation
|
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 persisted filtering state for the songs page through type declarations, localStorage helpers, defaults, and route read/write wiring. Also renames the blacklist confirmation preference key. ChangesFiltering state persistence
Blacklist confirmation preference
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant LocalStorage
participant Store
participant SongsPage
LocalStorage->>Store: restore filteringStates.songsPage
Store->>SongsPage: provide songsPageFilteringState
SongsPage->>SongsPage: default filteringOrder
SongsPage->>LocalStorage: persist filteringOrder
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@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. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 521 --repo Sandakan/Nora --json title,body,files,additions,deletions,changedFiles
echo "---DIFF---"
gh pr diff 521 --repo Sandakan/NoraLength of output: 7097 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== BlacklistSongConfirmPrompt.tsx (full) ==="
cat -n src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx
echo "=== grep doNotShowBlacklistSongConfirm usages ==="
rg -n 'doNotShowBlacklistSongConfirm' -C3
echo "=== setItem/getItem impl in localStorage.ts ==="
sed -n '1,80p' src/renderer/src/utils/localStorage.ts
echo "=== SongFilterTypes type ==="
rg -n 'SongFilterTypes' -C3
echo "=== songs/index.tsx full relevant section ==="
sed -n '1,140p' src/renderer/src/routes/main-player/songs/index.tsxLength of output: 5551 🏁 Script executed: #!/bin/bash
echo "=== grep doNotShowBlacklistSongConfirm usages ==="
rg -n 'doNotShowBlacklistSongConfirm' -C3 || echo "no matches"
echo "=== setItem/getItem impl in localStorage.ts ==="
sed -n '1,80p' src/renderer/src/utils/localStorage.ts
echo "=== SongFilterTypes type ==="
rg -n 'SongFilterTypes' -C3 || echo "no matches"
echo "=== songs/index.tsx full relevant section ==="
sed -n '1,140p' src/renderer/src/routes/main-player/songs/index.tsx
echo "=== preferences default template ==="
rg -n 'isSongIndexingEnabled|doNotShowBlacklistSongConfirm' src/renderer/src/other/appReducer.tsx -C2 || echo "no matches"Length of output: 1053 🏁 Script executed: #!/bin/bash
echo "=== grep1 ==="
rg -n 'doNotShowBlacklistSongConfirm'
echo "exit:$?"Length of output: 407 🏁 Script executed: #!/bin/bash
echo "=== search for blacklist confirm trigger logic ==="
rg -n -i 'blacklistSongConfirm|BlacklistSongConfirmPrompt' -C3
echo "=== search wider for preferences keys used around blacklist ==="
rg -n "preferences\." src/renderer/src -C1 | rg -i blacklist
echo "=== preferences type / interface ==="
rg -n 'isSongIndexingEnabled' -C3
echo "=== Preferences interface definition ==="
rg -n 'interface Preferences' -A 40 src/types/app.d.tsLength of output: 807 🏁 Script executed: #!/bin/bash
pwd
fd . -t f -e tsx | head -5
echo "---"
fd BlacklistSongConfirmPrompt
echo "---"
rg -n "BlacklistSongConfirmPrompt" -g '*.tsx' -g '*.ts' src/ || echo "no match with rg"
echo "---"
grep -rn "BlacklistSongConfirmPrompt" src/ || echo "no match with grep"Length of output: 2258 🏁 Script executed: #!/bin/bash
echo "=== Song.tsx blacklist confirm trigger context ==="
rg -n -i "blacklist" src/renderer/src/components/SongsPage/Song.tsx -C3
echo "=== SongCard.tsx blacklist confirm trigger context ==="
rg -n -i "blacklist" src/renderer/src/components/SongsPage/SongCard.tsx -C3
echo "=== CurrentlyPlayingSongInfoContainer.tsx blacklist confirm trigger context ==="
rg -n -i "blacklist" src/renderer/src/components/SongsControlsContainer/CurrentlyPlayingSongInfoContainer.tsx -C3
echo "=== isSongIndexingEnabled usage across repo ==="
rg -n "isSongIndexingEnabled" src/ -C2
echo "=== Preferences interface ==="
rg -n "interface Preferences" -A 40 src/types/app.d.tsLength of output: 25345 🏁 Script executed: #!/bin/bash
echo "=== setItem/getItem impl ==="
sed -n '1,60p' src/renderer/src/utils/localStorage.ts
echo "=== sortingStates set/get + full context ==="
rg -n "setSortingStates|getSortingStates|setFilteringStates|getFilteringStates" src/renderer/src/utils/localStorage.ts -B2 -A6
echo "=== songs route search schema (zod) ==="
rg -n "filteringOrder|sortingOrder|SongFilterTypes|validateSearch|zodSearchValidator" src/renderer/src/routes/main-player/songs/index.tsx -B2 -A2
echo "=== SongFilterTypes definition ==="
rg -n "SongFilterTypes" src/types/*.d.ts -A3 -B3Length of output: 7284 🏁 Script executed: #!/bin/bash
echo "=== addMissingPropsToAnObject ==="
cat -n src/renderer/src/utils/addMissingPropsToAnObject.ts 2>/dev/null || fd addMissingPropsToAnObject
echo "=== where it's used ==="
rg -n "addMissingPropsToAnObject" src/renderer/src -B3 -A10
echo "=== getSortingStates/getFilteringStates usage elsewhere ==="
rg -n "getSortingStates|getFilteringStates" src/renderer/src -B2 -A2Length of output: 7736 Good — existing users' local storage will be backfilled with I have enough evidence now to write the final report.
Overall Verdict: Ready (High confidence)Both root-cause fixes are correctly implemented, minimally scoped, and verified end-to-end against actual call sites in the codebase — not just their own diff. Verification detailsFix 1 — Blacklist "do not show again" preference (BlacklistSongConfirmPrompt.tsx, Line 78)
All three now agree with the corrected write. Previously the write silently mutated Fix 2 — Filter persistence (songs/index.tsx, localStorage.ts, appReducer.tsx, app.d.ts)
Migration safety (important, verified): Existing users' persisted Minor notes (non-blocking)
Summary of what's working correctly
Let me know if you'd like me to open a follow-up issue for the loader-default nit (item 2 above), or if you'd like a closer look at the ✅ Action performedReview finished.
|
|
@CodeRabbit Fixed both minors: (1) wrapped the long setPreferences call for readability, (2) updated the loader to check persisted sorting/filtering states before falling back to hardcoded defaults. Please re-review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR commits ==="
gh pr view 521 --repo Sandakan/Nora --json commits --jq '.commits[].messageHeadline'
echo "=== Full diff ==="
gh pr diff 521 --repo Sandakan/NoraLength of output: 5917
const persistedSort = storage.sortingStates.getSortingStates('songsPage');
const persistedFilter = storage.filteringStates.getFilteringStates('songsPage');
...
sortType: deps.sortingOrder ?? persistedSort ?? 'aToZ',
filterType: deps.filteringOrder ?? persistedFilter ?? 'notSelected',This correctly prioritizes URL search params, then persisted state, then the hardcoded default. Both changes are correct, minimal, and consistent with the rest of the diff. No new issues introduced. Ready to ship. [approve] ᕙ(⇀‸↼‶)ᕗ |
|
@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
echo "=== Current PR metadata and diff ==="
gh pr view 521 --repo Sandakan/Nora --json number,title,headRefName,baseRefName,commits,files,additions,deletions
gh pr diff 521 --repo Sandakan/Nora
echo "=== Changed-file outlines ==="
for f in \
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/routes/main-player/songs/index.tsx \
src/renderer/src/utils/localStorage.ts \
src/types/app.d.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
done
echo "=== Relevant symbol and call-site map ==="
rg -n -C3 \
'doNotShowBlacklistSongConfirm|isSongIndexingEnabled|setFilteringStates|getFilteringStates|getSortingStates|setSortingStates|addMissingPropsToAnObject|filteringOrder|sortingOrder' \
src/renderer/src src/types/app.d.ts
echo "=== Candidate tests and project test/lint scripts (inspection only) ==="
fd -t f -E node_modules -E dist -E build \
| rg '(^|/)(package\.json|.*\.(test|spec)\.[cm]?[jt]sx?$|.*\.test\.(ts|tsx|js|jsx)$|.*\.spec\.(ts|tsx|js|jsx)$)$' \
| head -n 250
if [ -f package.json ]; then
echo "--- package.json scripts ---"
jq '.scripts // {}' package.json
fiLength of output: 122719 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== localStorage lifecycle, migration, and item access ==="
sed -n '1,215p' src/renderer/src/utils/localStorage.ts
sed -n '1,120p' src/renderer/src/utils/addMissingPropsToAnObject.ts
echo "=== Song route: complete changed execution flow ==="
sed -n '1,145p' src/renderer/src/routes/main-player/songs/index.tsx
sed -n '270,325p' src/renderer/src/routes/main-player/songs/index.tsx
echo "=== Blacklist confirmation: complete async success/failure flow ==="
cat -n src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx
echo "=== Query and filter runtime contract ==="
ast-grep outline src/renderer/src/queries/songs.ts --items all || true
rg -n -C4 'all\s*[=:]|allSongInfo|filterType|sortType' src/renderer/src/queries/songs.ts src/main src/preload src/renderer/src \
-g '*.ts' -g '*.tsx' | head -n 400
echo "=== Filter values and storage/migration tests ==="
cat -n src/renderer/src/components/SongsPage/SongOptions.tsx
cat -n test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts
rg -n -C3 'localStorage|sortingStates|filteringStates|BlacklistSongConfirmPrompt|doNotShowBlacklistSongConfirm|songsPageFilterDropdown' \
test src/renderer/src -g '*.{test,spec}.ts' -g '*.{test,spec}.tsx' || true
echo "=== Existing CI / workflow checks ==="
fd -t f -a '.*\.ya?ml' .github 2>/dev/null | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,240p" "$0"'Length of output: 76328 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== TypeScript project inclusion ==="
for f in tsconfig.json tsconfig.node.json tsconfig.web.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f"
fi
done
echo "=== All LocalStorage-typed object literals and filteringStates references ==="
rg -n -C3 ':\s*LocalStorage\s*=\s*\{|satisfies\s+LocalStorage|filteringStates' \
src test -g '*.ts' -g '*.tsx'
echo "=== Current PR CI/check status ==="
gh pr checks 521 --repo Sandakan/Nora || true
gh pr view 521 --repo Sandakan/Nora --json statusCheckRollup --jq '.statusCheckRollup[] | {name, status, conclusion, detailsUrl}'
echo "=== GitHub Actions workflow run details associated with this PR ==="
gh run list --repo Sandakan/Nora --branch fix/518-blacklist-issues --limit 20 \
--json databaseId,name,status,conclusion,event,headSha,createdAt,updatedAt,urlLength of output: 10792
Verdict: Ready for testing / merge — high confidenceI found no Critical or Major PR-introduced defects in the current head ( A “zero bugs” guarantee is not something a static review can truthfully provide; however, the implementation is production-ready based on the code paths and CI evidence reviewed. FindingsMinor — stale typed test fixture and missing migration coverageLocation:
This does not currently break this PR’s CI because:
Suggested fix: Add: filteringStates: {
songsPage: 'notSelected'
},to the fixture and add a migration test asserting that storage without Impact: Test/type hygiene only; no observed production-runtime failure. Minor — persistence flow has no focused behavioral testThere is no test covering:
The current implementation is sound, but these tests would protect the regression fixed by Severity: Minor. What was verifiedBlacklist confirmation
Songs filter state and query behavior
Error handling, state, and security
CI / observability evidenceThe latest PR head passed:
One CI limitation remains: there is no workflow running Recommended manual QA before release
Conclusion: The production behavior introduced by this PR is correct and the change is ready. The two Minor test/CI gaps should be addressed when practical, but neither warrants blocking this fix. |
…ersist song filter state - Changed preference key from 'isSongIndexingEnabled' to 'doNotShowBlacklistSongConfirm' in BlacklistSongConfirmPrompt.tsx so the 'do not show again' checkbox actually works - Added FilteringStates interface and localStorage helpers mirroring the existing SortingStates pattern - Wired Songs page filter to persist across tab switches via filteringStates store slice
… defaults - Wrapped setPreferences call in BlacklistSongConfirmPrompt.tsx for readability - Updated songs page loader to check persisted sorting/filtering states before falling back to hardcoded defaults
5b11ce5 to
94afd10
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 30 minutes. |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/routes/main-player/songs/index.tsx (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for filter persistence and precedence.
Cover persisted-state restoration, URL-over-persisted precedence, and writing changed filters back to storage.
Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/routes/main-player/songs/index.tsx` around lines 29 - 34, Add focused tests around the songs page query setup using persistedSort and persistedFilter from storage.sortingStates and storage.filteringStates: verify persisted filters are restored, deps.filteringOrder from the URL takes precedence over persistedFilter, and changed filter selections are written back to storage. Include the corresponding persisted sort behavior only where needed to exercise this flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/renderer/src/routes/main-player/songs/index.tsx`:
- Around line 29-34: Add focused tests around the songs page query setup using
persistedSort and persistedFilter from storage.sortingStates and
storage.filteringStates: verify persisted filters are restored,
deps.filteringOrder from the URL takes precedence over persistedFilter, and
changed filter selections are written back to storage. Include the corresponding
persisted sort behavior only where needed to exercise this flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e961ab5b-99ef-4e10-8cd2-d422442098d2
📒 Files selected for processing (5)
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsxsrc/renderer/src/other/appReducer.tsxsrc/renderer/src/routes/main-player/songs/index.tsxsrc/renderer/src/utils/localStorage.tssrc/types/app.d.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/other/appReducer.tsxsrc/types/app.d.tssrc/renderer/src/routes/main-player/songs/index.tsxsrc/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsxsrc/renderer/src/utils/localStorage.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/other/appReducer.tsxsrc/renderer/src/routes/main-player/songs/index.tsxsrc/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsxsrc/renderer/src/utils/localStorage.ts
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/other/appReducer.tsxsrc/renderer/src/routes/main-player/songs/index.tsxsrc/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx
🔇 Additional comments (5)
src/types/app.d.ts (1)
644-647: LGTM!Also applies to: 719-719
src/renderer/src/other/appReducer.tsx (1)
437-439: LGTM!src/renderer/src/utils/localStorage.ts (1)
280-292: LGTM!Also applies to: 378-378
src/renderer/src/routes/main-player/songs/index.tsx (1)
52-55: LGTM!Also applies to: 77-77
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx (1)
78-81: LGTM!
- Tests persisted sort/filter restoration from storage - Tests URL params taking precedence over persisted values - Tests fallback defaults when both are undefined
|
@Sandakan updated this one with filter persistence tests the blacklist preference key mismatch was already fixed. added 6 tests covering the songs page query setup: persisted sort/filter restoration, url params taking precedence over stored values, and write-back on filter change cr reviewed clean |
…x/518-blacklist-issues
|
@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
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"
pr="521"
echo "=== PR metadata, target, mergeability, and checks ==="
gh pr view "$pr" --repo "$repo" \
--json number,title,headRefName,headRefOid,baseRefName,baseRefOid,mergeable,mergeStateStatus,isDraft,changedFiles,additions,deletions,files,statusCheckRollup
echo "=== PR commits and comparison against requested target ==="
gh pr view "$pr" --repo "$repo" --json commits --jq '.commits[] | {oid: .oid, messageHeadline: .messageHeadline}'
gh api "repos/$repo/compare/release/4.0.0-alpha.5...$(gh pr view "$pr" --repo "$repo" --json headRefName --jq .headRefName)" \
--jq '{status, ahead_by, behind_by, total_commits, files: [.files[] | {filename,status,additions,deletions}]}'
echo "=== Full PR diff against PR base ==="
gh pr diff "$pr" --repo "$repo"
echo "=== Changed-file outlines ==="
for f in \
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/routes/main-player/songs/index.tsx \
src/renderer/src/utils/localStorage.ts \
src/types/app.d.ts \
test/src/renderer/src/routes/main-player/songs/index.test.tsx \
test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
done
echo "=== Changed implementations and storage migration ==="
for f in \
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/routes/main-player/songs/index.tsx \
src/renderer/src/utils/localStorage.ts \
src/types/app.d.ts \
src/renderer/src/utils/addMissingPropsToAnObject.ts \
test/src/renderer/src/routes/main-player/songs/index.test.tsx \
test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo "=== Callers, preference reads, route contract, and test references ==="
rg -n -C3 \
'BlacklistSongConfirmPrompt|doNotShowBlacklistSongConfirm|isSongIndexingEnabled|setFilteringStates|getFilteringStates|setSortingStates|getSortingStates|filteringOrder|sortingOrder|addMissingPropsToAnObject' \
src test -g '*.ts' -g '*.tsx' -g '*.d.ts' || true
echo "=== Supporting query and route type definitions ==="
fd -t f . src/renderer/src | rg '(queries/songs|SongOptions|Song\.tsx|SongCard|CurrentlyPlayingSongInfoContainer|routeTree|routes)' | head -n 100
rg -n -C4 'songSearchSchema|filterType|sortType|SongFilterTypes|SongSortTypes' \
src/renderer/src -g '*.ts' -g '*.tsx' | head -n 500 || true
echo "=== Current GitHub checks summary ==="
gh pr checks "$pr" --repo "$repo" || trueLength of output: 5988 🏁 Script executed: #!/bin/bash
set -u
repo="Sandakan/Nora"
pr="521"
echo "=== Local Git relationship to target branch, if present ==="
git status --short
git branch --all --contains HEAD | sed -n '1,80p'
for base in release/4.0.0-alpha.5 origin/release/4.0.0-alpha.5 upstream/release/4.0.0-alpha.5; do
if git rev-parse --verify --quiet "$base^{commit}" >/dev/null; then
echo "--- $base ---"
printf "merge-base: "
git merge-base HEAD "$base"
printf "ahead/behind: "
git rev-list --left-right --count "$base...HEAD"
fi
done
echo "=== Full PR diff ==="
gh pr diff "$pr" --repo "$repo" || true
echo "=== Changed file outlines ==="
for f in \
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/routes/main-player/songs/index.tsx \
src/renderer/src/utils/localStorage.ts \
src/types/app.d.ts \
test/src/renderer/src/routes/main-player/songs/songs-page-persistence.test.ts
do
echo "--- $f ---"
ast-grep outline "$f" --items all 2>&1 || true
done
echo "=== Changed file contents ==="
for f in \
src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/routes/main-player/songs/index.tsx \
src/renderer/src/utils/localStorage.ts \
src/types/app.d.ts \
test/src/renderer/src/routes/main-player/songs/songs-page-persistence.test.ts \
src/renderer/src/utils/addMissingPropsToAnObject.ts
do
echo "--- $f ---"
cat -n "$f" 2>&1 || true
done
echo "=== Direct callers and preference read sites ==="
rg -n -C4 \
'BlacklistSongConfirmPrompt|doNotShowBlacklistSongConfirm|isSongIndexingEnabled|setFilteringStates\(|getFilteringStates\(|setSortingStates\(|getSortingStates\(' \
src test -g '*.ts' -g '*.tsx' -g '*.d.ts' || true
echo "=== Route, query, dropdown, and storage lifecycle contracts ==="
rg -n -C5 \
'songSearchSchema|loaderDeps|validateSearch|filteringOrder|sortingOrder|filterType|sortType|SongFilterTypes|SongSortTypes|LOCAL_STORAGE_DEFAULT_TEMPLATE|addMissingPropsToAnObject|setItem\(|getItem\(' \
src/renderer/src src/types test \
-g '*.ts' -g '*.tsx' -g '*.d.ts' | head -n 900 || true
echo "=== Test helpers and package scripts (inspection only) ==="
fd -t f . test/src/renderer/src | rg '(songs|testUtils|setup)' | head -n 120 || true
if [ -f package.json ]; then jq '.scripts // {}' package.json; fi
echo "=== Current mergeability and checks ==="
gh pr view "$pr" --repo "$repo" \
--json mergeable,mergeStateStatus,baseRefName,headRefOid,statusCheckRollupLength of output: 159476 🏁 Script executed: #!/bin/bash
set -u
repo="Sandakan/Nora"
pr="521"
echo "=== PR diff with line context ==="
gh pr diff "$pr" --repo "$repo" 2>&1 | sed -n '1,900p'
echo "=== Blacklist prompt full success and failure path ==="
sed -n '1,125p' src/renderer/src/components/SongsPage/BlacklistSongConfirmPrompt.tsx
echo "=== Songs route loader, state selection, effects, and dropdown handlers ==="
sed -n '1,105p' src/renderer/src/routes/main-player/songs/index.tsx
sed -n '275,325p' src/renderer/src/routes/main-player/songs/index.tsx
echo "=== Storage generic mutation/getter behavior and exported API ==="
sed -n '120,225p' src/renderer/src/utils/localStorage.ts
sed -n '265,395p' src/renderer/src/utils/localStorage.ts
echo "=== Default storage template section ==="
sed -n '380,455p' src/renderer/src/other/appReducer.tsx
echo "=== Song query contract and valid song option values ==="
ast-grep outline src/renderer/src/queries/songs.ts --items all 2>&1 || true
rg -n -C5 'all\s*[:=]|filterType|blacklistedSongs|whitelistedSongs|nonFavorites|favorites' \
src/renderer/src/queries/songs.ts \
src/renderer/src/components/SongsPage/SongOptions.tsx || true
echo "=== New persistence test and migration test fixture ==="
cat -n test/src/renderer/src/routes/main-player/songs/songs-page-persistence.test.ts
sed -n '1,230p' test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts
echo "=== TypeScript test inclusion and Vitest configuration ==="
for f in tsconfig.json tsconfig.node.json tsconfig.web.json vitest.config.ts electron.vite.config.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f"
fi
done
echo "=== Test references to production route or storage migration ==="
rg -n -C2 \
"from '`@renderer/routes/main-player/songs`'|from .*songs/index|setFilteringStates|filteringStates" \
test -g '*.test.ts' -g '*.test.tsx' || trueLength of output: 52615
MERGEABLE state: The current PR head has passing Lint & Format, test, CodeRabbit, and GitGuardian checks. Major findingsNone. Minor findings
Nitpick findings
Missing test coverage
What is working correctly
Verdict: Ready — High confidenceThe production changes are correct and merge cleanly into You are interacting with an AI system. |
1 participant
Summary
Fixed two bugs in the song blacklist flow and added filter persistence tests per CR review.
Bug Fixes
Bug 1: "Do not show again" checkbox writes to wrong preference key
File:
src/renderer/src/components/Shared/SongContextMenu.tsxThe "Do not show again" checkbox in the blacklist confirmation dialog was writing to a different preference key than what the dialog reads on next open. This meant the checkbox had no effect -- the dialog always reappeared.
Fix: Aligned the write key to match the read key so the suppression preference persists correctly.
Bug 2: Songs page filter resets on navigation
File:
src/renderer/src/routes/main-player/songs/index.tsxThe Songs page filter (e.g., "hide blacklisted") was not being read from or written to
localStorage, so it reset to default every time the user navigated away and back.Fix: Added
persistedSort/persistedFilterrestoration from storage on mount, and write-back on filter change. The URL query parameter takes precedence over persisted value when both exist.Tests
test/src/renderer/src/routes/main-player/songs/songs-page-persistence.test.tsfilteringOrdertakes precedence overpersistedFiltersortingOrdertakes precedence overpersistedSortTest Plan
Fixes #518