fix(#463): folder scan and library resync not detecting new files - #490
fix(#463): folder scan and library resync not detecting new files#490Owie6789 wants to merge 19 commits into
Conversation
|
three bugs forming a chain:
fixes: removed the |
e7b42fd to
4ad40a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses library resync and watcher behavior so Nora can detect newly added music files more reliably, including files in nested folders.
Changes:
- Fixes
checkForNewSongs()so all configured folders are scanned. - Adds recursive folder scanning and adjusts parent-folder watcher behavior for new directory events.
- Adds a sidebar resync button and fixes an English locale typo.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/core/checkForNewSongs.ts |
Scans all folders instead of returning after the first one. |
src/main/fs/checkFolderForUnknownContentModifications.ts |
Recursively discovers music files and handles empty DB folder state. |
src/main/fs/addWatchersToParentFolders.ts |
Triggers content scans when new directories appear under watched music folders. |
src/renderer/src/components/Sidebar/Sidebar.tsx |
Adds a visible resync-library button to the sidebar. |
src/renderer/src/assets/locales/en/en.json |
Corrects the resync success message typo. |
| return async (eventType: WatchEventType, filename?: string | null) => { | ||
| if (filename) { | ||
| if (eventType === 'rename') { | ||
| const isADirectory = !fileNameRegex.test(filename); |
| newlyAddedSongPaths, | ||
| folderPath | ||
| }); | ||
| await addNewlyAddedSongsToLibrary(folderPath, newlyAddedSongPaths, abortController.signal); |
| const relevantFolderSongPaths = await getSongPathsRelativeToFolder(folderPath); | ||
|
|
||
| if (relevantFolderSongPaths.length > 0) { | ||
| const dirs = await getFullPathsOfFolderDirs(folderPath); | ||
| const dirs = await getFullPathsOfFolderDirs(folderPath); |
|
@Sandakan , i recommited a version that corret all the lint warnings, mb |
d5bb55b to
f0ee0ac
Compare
| parentFolderPath: string, | ||
| initialFolderPaths: string[] | ||
| ) => { | ||
| const musicFolderPaths = initialFolderPaths; |
| try { | ||
| return await checkFolderForUnknownModifications(folder.path); | ||
| await checkFolderForUnknownModifications(folder.path); | ||
| } catch (error) { |
22dfbc2 to
7bf6db7
Compare
|
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:
📝 WalkthroughWalkthroughChangesLibrary synchronization now scans every top-level music folder, recursively reconciles songs, aggregates failures, and coordinates watcher-triggered scans. IPC requests are serialized and report partial failures, while the sidebar provides a localized resync control. Library resync flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sidebar
participant IPC
participant LibraryScanner
participant FolderScanner
Sidebar->>IPC: request library resync
IPC->>LibraryScanner: start or await resync
LibraryScanner->>FolderScanner: scan each top-level folder
FolderScanner-->>LibraryScanner: return failures and scan status
LibraryScanner-->>IPC: return aggregated result
IPC-->>Sidebar: report successful or partial resync
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 — comprehensive review request. Please do all of the following and report findings as inline review comments on the changed files (not in this thread), grouped by severity. 1. Careful code review
2. UI audit (only if the PR touches UI)
3. Production-readiness verification
4. Code smell sniff
5. Half-baked implementation detection
6. Required output from you
Please be thorough. I will be applying fixes from your review in a follow-up pass. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and diff
gh pr view 490 --repo Sandakan/Nora --json title,body,files,state,additions,deletions
echo "=== PR DIFF ==="
gh pr diff 490 --repo Sandakan/NoraLength of output: 20827 🏁 Script executed: #!/bin/bash
echo "=== checkFolderForUnknownContentModifications.ts (full) ==="
cat src/main/fs/checkFolderForUnknownContentModifications.ts
echo ""
echo "=== addWatchersToParentFolders.ts (full) ==="
cat src/main/fs/addWatchersToParentFolders.ts
echo ""
echo "=== checkForNewSongs.ts (full) ==="
cat src/main/core/checkForNewSongs.ts
echo ""
echo "=== Sidebar.tsx (full) ==="
cat src/renderer/src/components/Sidebar/Sidebar.tsx
echo ""
echo "=== folders DB queries (to check getAllFolderStructures) ==="
fd -e ts "folders" src/main/db/queries/ --exec cat {}
echo ""
echo "=== checkForFolderModifications.ts (full) ==="
cat src/main/fs/checkForFolderModifications.ts
echo ""
echo "=== FolderStructure type definition ==="
rg -n "FolderStructure" src/main/types --include="*.ts" -A 5 -B 2
rg -n "type FolderStructure\|interface FolderStructure" -A 10
echo ""
echo "=== resyncSongsLibrary IPC channel ==="
rg -n "resyncSongsLibrary" -A 3 -B 3
echo ""
echo "=== audioLibraryControls API ==="
rg -n "audioLibraryControls" -A 10 -B 2 src/preload
echo ""
echo "=== saveAbortController ==="
cat src/main/fs/controlAbortControllers.ts 2>/dev/null || rg -n "saveAbortController" -A 5 -B 2
echo ""
echo "=== t('settingsPage.resyncLibrary') in en.json ==="
rg -n "resyncLibrary" src/renderer/src/assets/locales/en/en.json -A 2 -B 2Length of output: 34402 🏁 Script executed: #!/bin/bash
# Post the comprehensive review with inline comments on the PR diff
gh api \
--method POST \
repos/Sandakan/Nora/pulls/490/reviews \
--input - << 'EOF'
{
"body": "## Review Summary\n\nThe three core bug fixes are correct and address the root causes described in `#463`. The overall approach is sound. Several issues below need attention before merge — two blockers, several majors, and a handful of nice-to-haves.\n\n---\n\n### ✅ What is solid\n- **Bug 1 (return removal)** — clean, minimal, correct.\n- **Bug 2 (recursive readdir)** — correctly uses `withFileTypes` + recursion; handles errors gracefully per subdirectory.\n- **Bug 3 (watcher wiring)** — `checkFolderForUnknownModifications` is now correctly called; `stat()`-based directory detection is more robust than the old regex.\n- **Path separator fix** — `fullPath[folderPath.length] === path.sep` correctly prevents `C:\\Music` matching `C:\\MusicExtra`.\n- **`isScanning` debounce** — serialises rapid watcher events for the \"new directory\" branch.\n- **`getClosestFolderId` boundary guard** — `if (parentDir === dir) break` prevents infinite loop at filesystem root.\n- **Typo fix** in `en.json` — long overdue.\n- **IPC stack** — `resyncSongsLibrary` is wired end-to-end (preload → IPC handler → `checkForNewSongs`).\n\n---\n\n### ⚠️ Must change before merge\n\n| # | Severity | File : approx line | Issue |\n|---|----------|--------------------|-------|\n| 1 | **Blocker** | `Sidebar.tsx` : 131 | Fire-and-forget IPC call — errors silently swallowed; user gets no feedback on failure. |\n| 2 | **Blocker** | `checkFolderForUnknownContentModifications.ts` : 14 | Module-level `abortController` is never reset after abort — once aborted, every subsequent scan is permanently dead. |\n| 3 | **Major** | `addWatchersToParentFolders.ts` : 17 | `musicFolderPaths` snapshot is stale after user adds new library folders without restarting; new content won't be watched. |\n| 4 | **Major** | `Sidebar.tsx` : 130 | No loading / disabled state — rapid clicks launch parallel scans that race on the DB. |\n| 5 | **Major** | `Sidebar.tsx` : 132 | `outline-none` removes focus ring; no `focus-visible:` ring; no `aria-label` — keyboard/screen-reader regression. |\n| 6 | **Minor** | `checkFolderForUnknownContentModifications.ts` : 76 | `getAllFolders()` called inside `addNewlyAddedSongsToLibrary` on every invocation — one extra full-table scan per top-level folder. |\n| 7 | **Minor** | `checkFolderForUnknownContentModifications.ts` : 29 | Symlinks to directories are silently skipped (`dirent.isDirectory()` returns false for symlinks). |\n\n---\n\n### 💡 Nice-to-haves\n- Add a warning log when `topLevelFolders` is empty in `checkForNewSongs`.\n- Memoize the sorted copy in `findContainingMusicFolder` — it is recreated on every watcher event.\n- `getAllPathsFromStructures` wraps `sub` in an array unnecessarily; simplify to `getAllPathsFromStructures(sub.subFolders, paths)`.\n- `if (dirs)` guard in `checkFolderForUnknownModifications` is always `true` (catch returns `[]`); remove dead branch.\n- Mislabelled inline comment `// Fix BUG 3` on the blacklist check — BUG 3 is the watcher issue.\n- The five lint-only route / hook changes are out of scope; best moved to a separate chore commit.\n- No unit tests for any of the new logic.",
"event": "COMMENT",
"comments": [
{
"path": "src/main/core/checkForNewSongs.ts",
"position": 14,
"body": "**[Minor] Silent failure when library is empty**\n\nThe original code logged a visible error when `folders.length === 0`. The new version silently exits, making it very hard to diagnose a misconfigured library at startup.\n\n```ts\nif (topLevelFolders.length > 0) {\n // ...\n} else {\n logger.warn('checkForNewSongs: no top-level music folders found — nothing to scan.');\n}\n```"
},
{
"path": "src/main/core/checkForNewSongs.ts",
"position": 5,
"body": "**[Minor] `getTopLevelFolderPaths` scans only root-level folders**\n\n`getAllFolderStructures` returns folders where `parentId IS NULL`. Any subfolder that a user manually added as a standalone root entry is still covered, but this is a subtle contract. A brief doc-comment would help future readers understand why only roots are enumerated (the recursion in `getFullPathsOfFolderDirs` handles descending into children)."
},
{
"path": "src/main/fs/checkFolderForUnknownContentModifications.ts",
"position": 1,
"body": "**[Blocker] Module-level `abortController` is never reset after abort**\n\n```ts\nconst abortController = new AbortController();\nsaveAbortController('checkFolderForUnknownContentModifications', abortController);\n```\n\nThis controller is created once at module load. If it is ever aborted (e.g., via `closeAbortController` or `closeAllAbortControllers` during app shutdown or folder removal), **every future call to `addNewlyAddedSongsToLibrary` or `removeDeletedSongsFromLibrary` will see `abortSignal.aborted === true` and bail out immediately**, permanently breaking all scans for the lifetime of the process.\n\nFix: create a fresh `AbortController` inside `checkFolderForUnknownModifications` on each call, or gate the abort check on a flag that can be reset."
},
{
"path": "src/main/fs/checkFolderForUnknownContentModifications.ts",
"position": 17,
"body": "**[Minor] Symlinks to directories are silently skipped**\n\n`dirent.isDirectory()` returns `false` for symbolic links to directories — only `dirent.isSymbolicLink()` is `true` for those entries. Users who organise their library with symlinked subdirectories (common on macOS/Linux) will find those subtrees never scanned.\n\nIf symlink support is out of scope, add a `logger.debug` when a symlink is encountered so it is at least discoverable. If it should be supported:\n\n```ts\nif (dir.isDirectory() || (dir.isSymbolicLink() && (await stat(fullPath)).isDirectory())) {\n```"
},
{
"path": "src/main/fs/checkFolderForUnknownContentModifications.ts",
"position": 53,
"body": "**[Minor] `getAllFolders()` called on every scan — hoist it**\n\n```ts\nconst allMusicFolders = await getAllFolders();\n```\n\nThis is inside `addNewlyAddedSongsToLibrary`, which is called once per top-level folder. If the user has 10 library roots, this fires 10 full-table scans in sequence. Move this call to `checkFolderForUnknownModifications` and pass the result in as a parameter."
},
{
"path": "src/main/fs/checkFolderForUnknownContentModifications.ts",
"position": 49,
"body": "**[Nit] Mislabelled comment**\n\n```ts\n// Fix BUG 3: Skip if folder is blacklisted — even when DB has 0 songs\n```\n\nBUG 3 (per the PR description) is the parent-folder watcher never calling content scan. This is a blacklist guard — a new piece of logic, not a fix for BUG 3. The misleading label will confuse future git-blame readers."
},
{
"path": "src/main/fs/checkFolderForUnknownContentModifications.ts",
"position": 96,
"body": "**[Nit] `if (dirs)` is always truthy — dead branch**\n\n`getFullPathsOfFolderDirs` always returns `string[]` (the catch block returns `[]`). The `if (dirs)` guard at this line is always `true` and the branch can never be skipped. Remove it for clarity:\n\n```ts\n// dirs is always string[], guaranteed by the catch block\nif (relevantFolderSongPaths.length > 0) { ... }\nif (dirs.length > 0) { ... }\n```"
},
{
"path": "src/main/fs/addWatchersToParentFolders.ts",
"position": 23,
"body": "**[Major] `musicFolderPaths` snapshot goes stale after user adds new library folders**\n\nThe closure captures `initialFolderPaths` at watcher construction time. If the user subsequently adds a new music folder via the UI, `findContainingMusicFolder` will not recognise paths inside it, so the watcher will silently fall through to the plain `checkForFolderModifications(filename)` path instead of calling `checkFolderForUnknownModifications`.\n\nAt minimum, document this limitation with a comment. Ideally, fetch the current folder list lazily inside the handler (or re-create the watcher when the library changes)."
},
{
"path": "src/main/fs/addWatchersToParentFolders.ts",
"position": 19,
"body": "**[Nit] `findContainingMusicFolder` re-sorts on every watcher event**\n\n```ts\nconst sorted = [...musicFolderPaths].sort((a, b) => b.length - a.length);\n```\n\n`musicFolderPaths` never changes after construction, so this allocation + sort runs on every single filesystem event. Compute and cache it once in the enclosing closure:\n\n```ts\nconst sortedMusicFolderPaths = [...initialFolderPaths].sort((a, b) => b.length - a.length);\n\nconst findContainingMusicFolder = (fullPath: string) =>\n sortedMusicFolderPaths.find((folderPath) => { ... });\n```"
},
{
"path": "src/main/fs/addWatchersToParentFolders.ts",
"position": 43,
"body": "**[Minor] Deletion path has no `isScanning` guard — concurrent events possible**\n\nThe `if (containingFolder)` branch is protected by `isScanning`, but the `else if (fullPathStat === null)` deletion path is not. Rapid sequential deletion events (e.g., Syncthing removing a folder tree) will fire multiple concurrent `checkForFolderModifications` calls. This is unlikely to cause data corruption but is worth noting / guarding with the same `isScanning` flag."
},
{
"path": "src/main/fs/addWatchersToParentFolders.ts",
"position": 66,
"body": "**[Nit] Unnecessary wrapping in `getAllPathsFromStructures`**\n\n```ts\nfor (const sub of structure.subFolders) {\n getAllPathsFromStructures([sub], paths); // wraps sub in a new array every iteration\n}\n```\n\nThis creates a throw-away single-element array on every iteration. Cleaner:\n\n```ts\ngetAllPathsFromStructures(structure.subFolders, paths);\n// and remove the inner loop entirely\n```"
},
{
"path": "src/renderer/src/components/Sidebar/Sidebar.tsx",
"position": 8,
"body": "**[Blocker] Fire-and-forget IPC call — errors silently swallowed**\n\n```tsx\nonClick={() => window.api.audioLibraryControls.resyncSongsLibrary()}\n```\n\nThe returned `Promise<true>` is never awaited and no `.catch()` is attached. If the IPC call throws (e.g., the main process handler rejects), the error is lost with no user feedback.\n\nMinimal fix:\n```tsx\nonClick={async () => {\n try {\n await window.api.audioLibraryControls.resyncSongsLibrary();\n } catch (e) {\n // surface to user via existing notification/toast system\n }\n}}\n```\n\nAlso see the loading-state comment below — the two fixes should be applied together."
},
{
"path": "src/renderer/src/components/Sidebar/Sidebar.tsx",
"position": 7,
"body": "**[Major] No loading/disabled state — concurrent resync scans can race**\n\nThe button can be clicked multiple times while a scan is already running. Two parallel calls to `checkForNewSongs` → `checkFolderForUnknownModifications` will race: both read the same DB state, both discover the same \"new\" song paths, and both try to insert them, which can produce duplicates or primary-key conflicts.\n\nAdd a local `isSyncing` state (or lift it to a store if other callers share it) and disable the button while syncing:\n\n```tsx\nconst [isSyncing, setIsSyncing] = useState(false);\n\n<button\n disabled={isSyncing}\n onClick={async () => {\n setIsSyncing(true);\n try { await window.api.audioLibraryControls.resyncSongsLibrary(); }\n finally { setIsSyncing(false); }\n }}\n>\n <span className=\"material-icons-round\">\n {isSyncing ? 'sync' /* add spin CSS */ : 'sync'}\n </span>\n ...\n</button>\n```"
},
{
"path": "src/renderer/src/components/Sidebar/Sidebar.tsx",
"position": 9,
"body": "**[Major] Accessibility regressions: no focus ring, no `aria-label`**\n\n1. `outline-none` removes the browser's default focus indicator. There is no `focus-visible:ring-*` replacement in the className, so keyboard users cannot see which element is focused.\n2. The button has a `title` attribute but no `aria-label`. Screen readers do not consistently expose `title`; `aria-label` should be used for interactive elements.\n3. The button has `hover:` and `active:` states but no `focus:` / `focus-visible:` state.\n\n```tsx\n<button\n aria-label={t('settingsPage.resyncLibrary')}\n className={`... focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-blue-500 ...`}\n // remove outline-none or replace with focus-visible:outline-none\n>\n```"
},
{
"path": "src/renderer/src/components/Sidebar/Sidebar.tsx",
"position": 6,
"body": "**[Minor] Button may be clipped by the `h-full!` list above it**\n\nThe `<ul>` sibling has `h-full!` (i.e., `height: 100% !important`). Inside a flex column `<nav>`, this makes the list claim all available height, potentially pushing the resync `<div>` below the visible area or causing overflow.\n\nCheck across all three sidebar widths (collapsed `lg:w-14`, hovered `lg:hover:w-[30%]`, and `md:hover:w-60`). Consider changing the `<ul>` to `flex-1 overflow-y-auto` and letting the button sit outside the scroll area naturally."
},
{
"path": "src/renderer/src/components/Sidebar/Sidebar.tsx",
"position": 10,
"body": "**[Nit] Template literal without interpolation**\n\n```tsx\nclassName={`flex w-full items-center ...`}\n```\n\nThere are no `${}` expressions here. Use a plain string:\n\n```tsx\nclassName=\"flex w-full items-center ...\"\n```"
}
]
}
EOFLength of output: 14063 🏁 Script executed: #!/bin/bash
gh pr comment 490 --repo Sandakan/Nora --body "$(cat << 'REVIEW_BODY'
## Comprehensive Review — PR `#490`
The three core bug fixes are correct and directly address the root causes in `#463`. The overall approach is sound. I found **2 blockers**, **3 majors**, and several minors/nits that should be resolved before merge.
---
## `src/main/core/checkForNewSongs.ts`
### [Minor] Silent failure when library is empty
The original code logged a visible error when `folders.length === 0`. The new version silently exits — very hard to diagnose at startup.
```ts
if (topLevelFolders.length > 0) {
// ...
} else {
logger.warn('checkForNewSongs: no top-level music folders found — nothing to scan.');
}[Nit] Document the root-only enumeration contract
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | 🔴 Blocker | checkFolderForUnknownContentModifications.ts L14 |
Module-level abortController never resets — once aborted, all future scans permanently dead |
| 2 | 🔴 Blocker | Sidebar.tsx L131 |
Fire-and-forget IPC call; errors silently swallowed |
| 3 | 🟠 Major | addWatchersToParentFolders.ts L17 |
musicFolderPaths snapshot stale after user adds new library folders |
| 4 | 🟠 Major | Sidebar.tsx L130 |
No loading/disabled state; concurrent clicks race on DB |
| 5 | 🟠 Major | Sidebar.tsx L132 |
outline-none + no aria-label — keyboard/screen-reader regression |
| 6 | 🟡 Minor | checkFolderForUnknownContentModifications.ts L29 |
Symlinks to directories silently skipped |
| 7 | 🟡 Minor | checkFolderForUnknownContentModifications.ts L76 |
getAllFolders() called once per folder during scan — hoist it |
| 8 | 🟡 Minor | addWatchersToParentFolders.ts L43 |
Deletion path has no isScanning guard |
| 9 | 🟡 Minor | Sidebar.tsx L126 |
Button may be clipped by h-full! list |
| 10 | 💡 Nit | checkForNewSongs.ts L21 |
Add warning log when folder list is empty |
| 11 | 💡 Nit | addWatchersToParentFolders.ts L19 |
Cache sorted array in findContainingMusicFolder |
| 12 | 💡 Nit | addWatchersToParentFolders.ts L66 |
getAllPathsFromStructures wraps sub in unnecessary array |
| 13 | 💡 Nit | checkFolderForUnknownContentModifications.ts L121 |
if (dirs) always true — dead branch |
| 14 | 💡 Nit | checkFolderForUnknownContentModifications.ts L49 |
Comment "Fix BUG 3" is mislabelled |
| 15 | 💡 Nit | Sidebar.tsx L132 |
Template literal without interpolation |
| 16 | 💡 Nit | 5 unrelated files | Lint-only eslint-disable changes — move to separate chore commit |
| — | ℹ️ Info | Entire PR | No unit tests added for any of the new logic |
| REVIEW_BODY | |||
| )" |
Length of output: 11362
---
</details>
---
`@Owie6789` — I don't have write access to the `Sandakan/Nora` repository, so I can't post inline comments there directly. Here is the full review — copy it into the PR or apply the fixes from it.
---
## Comprehensive Review — PR `#490`
The three core bug fixes are correct and directly address the root causes in `#463`. The overall approach is sound. I found **2 blockers**, **3 majors**, and several minors/nits.
---
## `src/main/core/checkForNewSongs.ts`
### [Minor] Silent failure when library is empty
The original code logged an error when `folders.length === 0`. The new version silently exits with no diagnostic output.
```ts
if (topLevelFolders.length > 0) {
// ...
} else {
logger.warn('checkForNewSongs: no top-level music folders found — nothing to scan.');
}
[Nit] Document the root-only enumeration contract
getAllFolderStructures returns only parentId IS NULL folders. This is intentional (recursion in getFullPathsOfFolderDirs handles the rest), but worth a one-line comment for future readers.
src/main/fs/checkFolderForUnknownContentModifications.ts
🔴 [Blocker] Module-level abortController is never reset after abort
// top of file, executed once at module load
const abortController = new AbortController();
saveAbortController('checkFolderForUnknownContentModifications', abortController);If this controller is ever aborted — e.g., via closeAllAbortControllers during app shutdown or folder removal — every future call to addNewlyAddedSongsToLibrary and removeDeletedSongsFromLibrary will immediately see abortSignal.aborted === true and bail out, permanently breaking all scans for the lifetime of the process. This PR makes it significantly more likely to bite because users can now trigger resync repeatedly from the sidebar.
Fix: Create a fresh AbortController inside checkFolderForUnknownModifications on each call (and register it so it can still be externally cancelled):
const checkFolderForUnknownModifications = async (folderPath: string) => {
const abortController = new AbortController();
saveAbortController('checkFolderForUnknownContentModifications', abortController);
// ... rest of function
};[Minor] Symlinks to directories are silently skipped
dirent.isDirectory() returns false for symbolic links to directories — only dirent.isSymbolicLink() is true for those. Users with symlinked music subdirectories (common on macOS/Linux) will find those subtrees invisible.
// current
if (dir.isDirectory()) { ... }
// with symlink support (import { stat } from 'fs/promises' already available in scope after addWatchersToParentFolders imports it)
if (dir.isDirectory() || (dir.isSymbolicLink() && (await stat(fullPath)).isDirectory())) { ... }At minimum, add a logger.debug when a symlink is encountered so it's discoverable.
[Minor] getAllFolders() called inside addNewlyAddedSongsToLibrary — hoist it
const allMusicFolders = await getAllFolders(); // called once per top-level folderWith 10 library roots this fires 10 full-table reads in sequence. Hoist the call to checkFolderForUnknownModifications and pass the result as a parameter to addNewlyAddedSongsToLibrary.
[Nit] Mislabelled // Fix BUG 3 comment
The comment on the blacklist guard says "Fix BUG 3". BUG 3 (per the PR description) is the watcher never calling content scan. The blacklist check is new defensive logic unrelated to BUG 3. Wrong labels mislead future git blame readers.
[Nit] if (dirs) is always truthy — dead branch
getFullPathsOfFolderDirs always returns string[] (the catch block returns [], never null/undefined). The if (dirs) wrapper is always true and can be removed.
src/main/fs/addWatchersToParentFolders.ts
🟠 [Major] musicFolderPaths snapshot goes stale after user adds new library folders
The closure captures initialFolderPaths at watcher construction time. If the user adds a new music folder via the UI after startup, findContainingMusicFolder won't recognise it and the watcher silently falls through to the deletion-only path — the exact bug this PR fixes, just deferred to the second session.
Fix options (pick one):
- Re-create all watchers whenever the library changes (cleanest).
- Call
getAllFolderStructures()lazily inside the event handler on each fire. - At minimum, add a prominent comment warning of the restart-required limitation.
[Nit] findContainingMusicFolder re-sorts on every watcher event
const sorted = [...musicFolderPaths].sort((a, b) => b.length - a.length);musicFolderPaths is immutable after construction — this allocation + sort runs on every single filesystem event. Cache it once in the outer closure:
const sortedMusicFolderPaths = [...initialFolderPaths].sort((a, b) => b.length - a.length);
const findContainingMusicFolder = (fullPath: string) =>
sortedMusicFolderPaths.find((folderPath) => {
if (!fullPath.startsWith(folderPath)) return false;
if (fullPath.length === folderPath.length) return true;
return fullPath[folderPath.length] === path.sep;
});[Minor] Deletion path has no isScanning guard
The containingFolder branch is protected by isScanning, but the else if (fullPathStat === null) deletion branch is not. Rapid deletion events (e.g., Syncthing removing a folder tree) fire multiple concurrent checkForFolderModifications calls.
[Nit] getAllPathsFromStructures wraps sub in an unnecessary single-element array
// current — allocates a new array on every iteration
for (const sub of structure.subFolders) {
getAllPathsFromStructures([sub], paths);
}
// cleaner
getAllPathsFromStructures(structure.subFolders, paths);
// (remove the for-loop entirely and just recurse on the full array)src/renderer/src/components/Sidebar/Sidebar.tsx
🔴 [Blocker] Fire-and-forget IPC call — errors silently swallowed
onClick={() => window.api.audioLibraryControls.resyncSongsLibrary()}The Promise<true> is never awaited; no .catch() attached. Any IPC rejection is lost with zero user feedback.
onClick={async () => {
try {
await window.api.audioLibraryControls.resyncSongsLibrary();
} catch (e) {
// surface via the existing notification/toast system
}
}}🟠 [Major — UI] No loading/disabled state; concurrent clicks race on the DB
Two parallel checkForNewSongs calls read the same DB state, both discover the same "new" songs, and both attempt to insert them — producing duplicates or constraint violations.
const [isSyncing, setIsSyncing] = useState(false);
<button
disabled={isSyncing}
onClick={async () => {
setIsSyncing(true);
try { await window.api.audioLibraryControls.resyncSongsLibrary(); }
finally { setIsSyncing(false); }
}}
>
<span className={`material-icons-round ${isSyncing ? 'animate-spin' : ''}`}>sync</span>
...
</button>🟠 [Major — UI] Accessibility regression: focus ring removed, no aria-label
outline-noneremoves the browser focus indicator with nofocus-visible:ring-*replacement — keyboard users can't see focus.titleis not reliably exposed by screen readers on interactive elements; usearia-labelinstead.hover:andactive:states present, but nofocus-visible:state.
<button
aria-label={t('settingsPage.resyncLibrary')}
className="... focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-blue-500 ..."
>[Minor] Button may be clipped by the h-full! list above it
<ul className="... h-full! ..."> forces height: 100% !important. In a flex-column <nav> this makes the list claim all available height, potentially pushing the resync <div> below the viewport. Verify at all three sidebar widths (lg:w-14, lg:hover:w-[30%], md:hover:w-60). Suggested fix: replace h-full! on <ul> with flex-1 min-h-0 overflow-y-auto so the list scrolls and the button is always pinned at the bottom.
[Nit] Template literal without interpolation
className={`flex w-full items-center ...`} // no ${} expressions
// use a plain string instead
className="flex w-full items-center ..."Scope creep
useWindowManagement.tsx and four routes/main-player/…/index.tsx files are changed only to add // eslint-disable-next-line react/only-export-components. Unrelated to the bug fixes — best extracted to a separate chore commit.
✅ What is solid
- Bug 1 (return removal) — minimal, clean, correct.
- Bug 2 (recursive
readdir) —withFileTypes+ recursion; per-subdirectory error isolation is good. - Bug 3 (watcher wiring) —
stat()-based directory detection is more robust than the old extension regex;checkFolderForUnknownModificationsnow correctly called. - Path separator guard —
fullPath[folderPath.length] === path.sepcorrectly preventsC:\MusicmatchingC:\MusicExtra. isScanningdebounce — serialises rapid watcher events for the new-directory branch.getClosestFolderIdboundary guard —if (parentDir === dir) breakprevents infinite loop at filesystem root.- IPC stack —
resyncSongsLibraryfully wired: preload → IPC handler →checkForNewSongs. - Typo fix in
en.json✅
Summary table
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | 🔴 Blocker | checkFolderForUnknownContentModifications.ts L14 |
Module-level abortController never resets — once aborted all future scans are dead |
| 2 | 🔴 Blocker | Sidebar.tsx L131 |
Fire-and-forget IPC; errors silently swallowed |
| 3 | 🟠 Major | addWatchersToParentFolders.ts L17 |
musicFolderPaths snapshot goes stale after new folders added |
| 4 | 🟠 Major | Sidebar.tsx L130 |
No loading/disabled state; concurrent clicks race on DB |
| 5 | 🟠 Major | Sidebar.tsx L132 |
outline-none + no aria-label — keyboard/screen-reader regression |
| 6 | 🟡 Minor | checkFolderForUnknownContentModifications.ts L29 |
Symlinks to directories silently skipped |
| 7 | 🟡 Minor | checkFolderForUnknownContentModifications.ts L76 |
getAllFolders() called once per scan iteration — hoist it |
| 8 | 🟡 Minor | addWatchersToParentFolders.ts L43 |
Deletion path has no isScanning guard |
| 9 | 🟡 Minor | Sidebar.tsx L126 |
Button may be clipped by h-full! list |
| 10 | 💡 Nit | checkForNewSongs.ts L21 |
Add warning log when folder list is empty |
| 11 | 💡 Nit | addWatchersToParentFolders.ts L19 |
Cache sorted array in findContainingMusicFolder |
| 12 | 💡 Nit | addWatchersToParentFolders.ts L66 |
getAllPathsFromStructures wraps sub in unnecessary single-element array |
| 13 | 💡 Nit | checkFolderForUnknownContentModifications.ts L121 |
if (dirs) is always truthy — remove dead branch |
| 14 | 💡 Nit | checkFolderForUnknownContentModifications.ts L49 |
Comment "Fix BUG 3" is mislabelled |
| 15 | 💡 Nit | Sidebar.tsx L132 |
Template literal without interpolation |
| 16 | 💡 Nit | 5 unrelated files | Lint-only eslint-disable changes — move to separate chore commit |
| — | ℹ️ Info | Entire PR | No unit tests for any of the new logic |
BLOCKERS: - checkFolderForUnknownContentModifications: create a fresh AbortController per call inside checkFolderForUnknownModifications (the previous module-level controller was never reset after abort, breaking all future scans) - Sidebar: add try/catch + isSyncing loading state to resync button; surface failures to console.error instead of swallowing them silently MAJORS: - addWatchersToParentFolders: fetch music folder paths lazily inside the event handler (the closure-captured initial set went stale after the user added new library folders) - Sidebar: disable the resync button while syncing (prevents concurrent scans racing on the DB) - Sidebar: replace outline-none with focus-visible:outline-* (keyboard focus regression) NITS: - checkFolderForUnknownContentModifications: mislabelled 'Fix BUG 3' comment on the blacklist guard → 'Defensive: skip blacklisted folders' - checkFolderForUnknownContentModifications: remove dead 'if (dirs)' guard (getFullPathsOfFolderDirs always returns string[]) - addWatchersToParentFolders: cache sorted array via lazy fetch (also fixes the re-sort-on-every-event smell) - addWatchersToParentFolders: drop unnecessary single-element array wrap in getAllPathsFromStructures recursion - addWatchersToParentFolders: guard deletion path with isScanning like the rename path - addWatchersToParentFolders: add warn log when no music folders found - Sidebar: change <ul> from h-full! to flex-1 min-h-0 overflow-y-auto (the resync button was being clipped at narrow widths) - Sidebar: add type='button' and aria-label to the resync button
4c88829 to
472d5e7
Compare
|
Applied CodeRabbit review fixes for #490 (commit 472d5e7 on fix/463-folder-scan-resync). Blockers
Majors
Minors / Nits
Intentionally not addressed
Notes:
@coderabbitai please re-review and confirm whether all blockers/majors/minors are now addressed. If any remain, please flag the specific file:line and a minimal fix. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR `#490` latest diff (after commit 472d5e7b) ==="
gh pr diff 490 --repo Sandakan/Nora
echo ""
echo "=== checkFolderForUnknownContentModifications.ts (current) ==="
gh api repos/Sandakan/Nora/contents/src/main/fs/checkFolderForUnknownContentModifications.ts?ref=fix/463-folder-scan-resync \
--jq '.content' | base64 -d
echo ""
echo "=== addWatchersToParentFolders.ts (current) ==="
gh api repos/Sandakan/Nora/contents/src/main/fs/addWatchersToParentFolders.ts?ref=fix/463-folder-scan-resync \
--jq '.content' | base64 -d
echo ""
echo "=== checkForNewSongs.ts (current) ==="
gh api repos/Sandakan/Nora/contents/src/main/core/checkForNewSongs.ts?ref=fix/463-folder-scan-resync \
--jq '.content' | base64 -d
echo ""
echo "=== Sidebar.tsx (current) ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/Sidebar/Sidebar.tsx?ref=fix/463-folder-scan-resync \
--jq '.content' | base64 -dLength of output: 22861
Re-review verdict✅ All blockers resolved
✅ All majors resolved
✅ All minors/nits resolvedDeletion path Three remaining items (low severity, no blockers)1.
|
…canning guard, log when no folders to scan
|
@coderabbitai — final comprehensive re-review pass. Please re-review the latest commit on this PR head branch and answer four specific questions in one response. Do not narrow scope to a checklist. Inspect the entire diff history of this branch (every commit between this branch and What I want you to verify1. Re-review the latest commitReview every new change I just pushed against the full current state of the branch. Surface any new blocker, major, minor, or nit you find. 2. Original issue fulfillmentThe PR's "Closes #N" issue defines the user-visible problem this branch is meant to fix. For each "Closes" issue, walk the issue text and tell me:
3. Deviation from issue fixCompare every commit on this branch to the original issue. Did any commit pull the scope in a direction the issue did not ask for? Examples to look for:
If you find any, name the commit, the file, and the lines, and say whether you think the deviation is justified (e.g., "required for the fix to be sound") or removable. 4. Over-sieveingWalk the whole diff and tell me if any of the changes are over-engineered for the original issue. Examples:
For each one, name it, and say whether it should be kept, narrowed, or removed. What to skip
Output formatReply with four clearly-labeled sections — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR `#490` details ==="
gh pr view 490 --repo Sandakan/Nora --json title,body,commits,files,state,baseRefName,headRefName
echo ""
echo "=== Issue `#463` ==="
gh issue view 463 --repo Sandakan/Nora --json title,body,comments
echo ""
echo "=== Commit log on the branch (vs base) ==="
gh api repos/Sandakan/Nora/pulls/490/commits --jq '.[].sha + " " + .[].commit.message'
echo ""
echo "=== Latest diff ==="
gh pr diff 490 --repo Sandakan/Nora
echo ""
echo "=== checkFolderForUnknownContentModifications.ts (current HEAD) ==="
gh api "repos/Sandakan/Nora/contents/src/main/fs/checkFolderForUnknownContentModifications.ts?ref=fix/463-folder-scan-resync" \
--jq '.content' | base64 -d
echo ""
echo "=== addWatchersToParentFolders.ts (current HEAD) ==="
gh api "repos/Sandakan/Nora/contents/src/main/fs/addWatchersToParentFolders.ts?ref=fix/463-folder-scan-resync" \
--jq '.content' | base64 -d
echo ""
echo "=== checkForNewSongs.ts (current HEAD) ==="
gh api "repos/Sandakan/Nora/contents/src/main/core/checkForNewSongs.ts?ref=fix/463-folder-scan-resync" \
--jq '.content' | base64 -d
echo ""
echo "=== Sidebar.tsx (current HEAD) ==="
gh api "repos/Sandakan/Nora/contents/src/renderer/src/components/Sidebar/Sidebar.tsx?ref=fix/463-folder-scan-resync" \
--jq '.content' | base64 -d
echo ""
echo "=== en.json resync section ==="
gh api "repos/Sandakan/Nora/contents/src/renderer/src/assets/locales/en/en.json?ref=fix/463-folder-scan-resync" \
--jq '.content' | base64 -d | python3 -c "import sys,json; d=json.load(sys.stdin); [print(k,'=',v) for k,v in d.get('settingsPage',{}).items() if 'resync' in k.lower() or 'sync' in k.lower()]"Length of output: 40094 🏁 Script executed: #!/bin/bash
echo "=== Check notifications.songDataUpdateFailed key in en.json ==="
gh api "repos/Sandakan/Nora/contents/src/renderer/src/assets/locales/en/en.json?ref=66082f0726855b2976ead965288038f4dcae5dfa" \
--jq '.content' | base64 -d | python3 -c "
import sys, json
d = json.load(sys.stdin)
n = d.get('notifications', {})
for k,v in n.items():
if 'update' in k.lower() or 'failed' in k.lower() or 'resync' in k.lower() or 'sync' in k.lower():
print(k, '=', v)
print('---all notification keys---')
print(list(n.keys())[:40])
"
echo ""
echo "=== checkForFolderModifications signature (does it take relative or absolute path?) ==="
gh api "repos/Sandakan/Nora/contents/src/main/fs/checkForFolderModifications.ts?ref=66082f0726855b2976ead965288038f4dcae5dfa" \
--jq '.content' | base64 -d | head -30
echo ""
echo "=== tryToParseSong signature ==="
gh api "repos/Sandakan/Nora/contents/src/main/parseSong/parseSong.ts?ref=66082f0726855b2976ead965288038f4dcae5dfa" \
--jq '.content' | base64 -d | head -20Length of output: 3549
1. Re-review of latest commitVerdict: Pass with one minor.
One remaining minor:
One notification wording nit (not a bug): the error path shows 2. Original issue fulfillmentVerdict: Mostly fulfilled. One coverage gap worth documenting. Issue
Gap I cannot confirm by static analysis alone: The watcher in This is acceptable scope for a bug-fix PR but worth a comment in the code or a note in the PR description so the maintainer isn't surprised. 3. Deviation from issue fixVerdict: Two deviations — one justified, one not.
4. Over-engineeringVerdict: One item to consider narrowing; the rest are proportionate.
Keep it — the logic is correct and the edge case is real — but do hoist Everything else ( Bottom line: No blockers. The one actionable item before merge is hoisting 🐇 ✅ |
…fications to avoid N queries per scan, pass folderPathToId and folderId to addNewlyAddedSongsToLibrary
ff2a61b to
8f7df94
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/fs/parseFolderStructuresForSongPaths.ts (2)
123-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnawaited watcher teardown/setup can interleave.
clearAllFolderWatches()(line 123) and both watcher-setup calls are async but never awaited, so the teardown DB read can still be in flight whileaddWatchersToFolders()/addWatchersToParentFolders()register new controllers — the close then targets the fresh controllers. Sequence them.🔧 Proposed fix
- if (resetWatchers) clearAllFolderWatches(); - const result = await saveAllFolderStructures(musicFolders, trx); return result; }); - if (resetWatchers) addWatchersToFolders(); - if (resetWatchers) addWatchersToParentFolders(); + if (resetWatchers) { + await clearAllFolderWatches(); + await addWatchersToFolders(); + await addWatchersToParentFolders(); + } return data;🤖 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/fs/parseFolderStructuresForSongPaths.ts` around lines 123 - 131, Sequence the async watcher lifecycle in the surrounding folder-structure flow: await clearAllFolderWatches() before saveAllFolderStructures, and await addWatchersToFolders() before addWatchersToParentFolders() after the transaction completes. Preserve the existing resetWatchers condition and return behavior.
103-110: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winParent-folder watchers are never closed, so they accumulate on every reset. The abort-controller registry is keyed by both music-folder paths and parent-folder paths, but teardown only enumerates music folders — so each
saveFolderStructures(..., true)adds a new set of recursive parent watchers on top of the old ones (duplicated scans, leaked handles).
src/main/fs/parseFolderStructuresForSongPaths.ts#L103-L110: also close controllers registered for parent folders (e.g. derive them viagetParentFolderPaths(folderPaths.map((f) => f.path)), or iterate the registry's keys) before re-adding watchers.src/main/fs/addWatchersToParentFolders.ts#L139-L139: alternatively track parent-watcher controllers in a dedicated registry and expose aclearAllParentFolderWatches()thataddWatchersToParentFolders()calls before registering new ones.🤖 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/fs/parseFolderStructuresForSongPaths.ts` around lines 103 - 110, Update clearAllFolderWatches in src/main/fs/parseFolderStructuresForSongPaths.ts:103-110 to close abort controllers for both music-folder paths and their derived parent-folder paths before new watchers are registered. Use getParentFolderPaths over the music folder paths, or the registry keys; no direct change is required at src/main/fs/addWatchersToParentFolders.ts:139 if teardown is fixed here.
🧹 Nitpick comments (6)
src/main/core/checkForNewSongs.ts (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor naming/redundancy polish.
sis a generic single-letter name, and thelength > 0guards before spreading are no-ops.♻️ Optional cleanup
const getTopLevelFolderPaths = async (): Promise<string[]> => { const structures = await getAllFolderStructures(); - return structures.map((s) => s.path); + return structures.map((folderStructure) => folderStructure.path); };- if (result.failedSongPaths.length > 0) - failedSongPaths.push(...result.failedSongPaths); - if (result.deletionFailures.length > 0) - deletionFailures.push(...result.deletionFailures); + failedSongPaths.push(...result.failedSongPaths); + deletionFailures.push(...result.deletionFailures);🤖 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/checkForNewSongs.ts` around lines 6 - 9, Update getTopLevelFolderPaths to use a descriptive name instead of the single-letter s when mapping folder structures to paths, and remove any redundant length > 0 guards before spread operations in the surrounding changed code while preserving behavior.Source: Coding guidelines
src/renderer/src/components/Sidebar/Sidebar.tsx (2)
19-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the failure through the app logger instead of
console.error.The renderer has an IPC logging path (
app/getRendererLogs), so main-process logs capture renderer failures.console.errorhere loses that.♻️ Suggested change
} catch (err) { - console.error('Failed to resync songs library.', err); + window.api.log.error?.('Failed to resync songs library.', { err }); addNewNotifications([Adjust to whatever wrapper the codebase exposes for renderer logging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Sidebar/Sidebar.tsx` around lines 19 - 37, Update handleResyncClick’s catch block to route the resync failure through the codebase’s renderer logging wrapper and IPC path instead of console.error, preserving the existing error message and notification behavior.
153-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnounce the syncing state to assistive tech.
disabled+animate-pulseconveys progress visually only. Addaria-busy={isSyncing}and swap the label while syncing so screen-reader users get feedback.♿ Suggested change
onClick={handleResyncClick} disabled={isSyncing} + aria-busy={isSyncing} aria-label={t('settingsPage.resyncLibrary')}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Sidebar/Sidebar.tsx` around lines 153 - 165, Update the resync button in the Sidebar component’s handleResyncClick flow to expose aria-busy={isSyncing}, and replace the accessible label text with a syncing-state translation while isSyncing is true. Preserve the existing resync label when idle and keep the current disabled behavior.src/main/fs/addWatchersToParentFolders.ts (1)
20-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDB round-trip per filesystem event.
findContainingMusicFolderre-reads all folder structures (and allocates + sorts the flattened path list) on everyrenameevent, before debouncing. Bulk copies into a watched tree can fire hundreds of events per second. Move the lookup behind the debounce, or cache the flattened, sorted path list and invalidate it when folders are added/removed.Also,
startsWithcomparison is case-sensitive; on Windows/macOS a differently cased path from the watcher will fail to resolve to its music folder.Also applies to: 63-91
🤖 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/fs/addWatchersToParentFolders.ts` around lines 20 - 30, Update findContainingMusicFolder and the watcher flow so folder structures are not loaded and flattened for every filesystem event: perform the lookup after debouncing or reuse a cached flattened, length-sorted path list, invalidating that cache whenever folders are added or removed. Make the containment comparison filesystem-aware by using case-insensitive matching on Windows/macOS while preserving path-boundary validation so similarly prefixed folders do not match.src/main/ipc.ts (1)
128-128: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftSingle-flight guard is local to this handler only.
resyncPromiseserializes IPC-initiated resyncs, but the startup scan and the watcher-triggeredcheckFolderForUnknownModificationscalls insrc/main/fs/addWatchersToParentFolders.ts/src/main/fs/addWatchersToFolders.tscan run concurrently with it against the same folders, duplicating parse work and racing deletion reconciliation. Consider hoisting the mutex into the scan module (e.g. a sharedrunLibraryScan()insrc/main/core/) so all three entry points share it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc.ts` at line 128, The resyncPromise guard in the IPC handler does not coordinate with startup and watcher scans. Move the single-flight state into a shared scan-layer function such as runLibraryScan, and update the IPC resync flow plus checkFolderForUnknownModifications callers in addWatchersToParentFolders and addWatchersToFolders to use it, ensuring all scans share one mutex and return the existing scan result.src/main/fs/checkFolderForUnknownContentModifications.ts (1)
34-60: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftOne unreadable subdirectory aborts the whole folder's reconciliation.
The recursive call propagates
undefinedup the whole tree, so a single permission-denied subfolder makescheckFolderForUnknownModificationssetscanFailedand skip reconciliation for the entire top-level folder — newly added songs elsewhere in that tree are never picked up. Bailing out is safe with respect to spurious deletions, but consider collecting the failed subpaths and continuing the walk, then suppressing only deletion handling when the inventory is incomplete.Also note that
dir.isDirectory()is false for symlinked directories, so symlinked music subfolders are silently skipped by the new recursive walk — worth confirming that's intended.🤖 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/fs/checkFolderForUnknownContentModifications.ts` around lines 34 - 60, Update getFullPathsOfFolderDirs to continue scanning when a recursive subdirectory read fails, while recording failed subpaths so checkFolderForUnknownModifications can suppress deletion handling only when the inventory is incomplete. Preserve discovered music files from readable siblings and avoid marking the entire top-level scan as failed. Also confirm and handle whether symlinked directories should be traversed according to the intended scan behavior.
🤖 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/fs/addWatchersToParentFolders.ts`:
- Around line 32-47: Update the runScan function so it returns the promise chain
from checkFolderForUnknownModifications, and arrange the rejection handler after
finally (or otherwise ensure the returned chain ends with catch) to satisfy
promise/catch-or-return while preserving the existing logging and dirty-scan
follow-up behavior.
- Around line 49-61: Update the watcher’s shared debounce state around
scheduleScan to replace the single dirtyPath with a Set of pending containing
folders. Add each scheduled folder to the set, then drain and scan every pending
folder sequentially after the debounce delay, rechecking the set after each scan
(including when scanning is already active) so events for different music
folders are not dropped.
In `@src/main/fs/checkFolderForUnknownContentModifications.ts`:
- Around line 185-207: Replace the nested membership checks in the deletion and
addition diffing around relevantFolderSongPaths with Set-based lookups: build
sets for the existing relevant paths and scanned dirs, then use has() while
filtering deletedSongPaths and newlyAddedSongPaths. Preserve the current
deletion handling and result behavior while reducing both comparisons to linear
time.
In `@src/main/ipc.ts`:
- Around line 479-494: Update the resync promise chain in the IPC handler around
checkForNewSongs to catch rejections, log the failure in the main process, and
send an appropriate failure notification to the renderer. Preserve the existing
success/partial messages and ensure the finally block still clears
resyncPromise.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx`:
- Around line 149-152: Add the flex and flex-col classes to the nav wrapper
containing the Sidebar ErrorBoundary, preserving its existing layout classes.
Ensure the wrapper establishes a column flex parent so the ul with flex-1
min-h-0 expands and scrolls while the resync button remains pinned within the
fixed-height sidebar.
---
Outside diff comments:
In `@src/main/fs/parseFolderStructuresForSongPaths.ts`:
- Around line 123-131: Sequence the async watcher lifecycle in the surrounding
folder-structure flow: await clearAllFolderWatches() before
saveAllFolderStructures, and await addWatchersToFolders() before
addWatchersToParentFolders() after the transaction completes. Preserve the
existing resetWatchers condition and return behavior.
- Around line 103-110: Update clearAllFolderWatches in
src/main/fs/parseFolderStructuresForSongPaths.ts:103-110 to close abort
controllers for both music-folder paths and their derived parent-folder paths
before new watchers are registered. Use getParentFolderPaths over the music
folder paths, or the registry keys; no direct change is required at
src/main/fs/addWatchersToParentFolders.ts:139 if teardown is fixed here.
---
Nitpick comments:
In `@src/main/core/checkForNewSongs.ts`:
- Around line 6-9: Update getTopLevelFolderPaths to use a descriptive name
instead of the single-letter s when mapping folder structures to paths, and
remove any redundant length > 0 guards before spread operations in the
surrounding changed code while preserving behavior.
In `@src/main/fs/addWatchersToParentFolders.ts`:
- Around line 20-30: Update findContainingMusicFolder and the watcher flow so
folder structures are not loaded and flattened for every filesystem event:
perform the lookup after debouncing or reuse a cached flattened, length-sorted
path list, invalidating that cache whenever folders are added or removed. Make
the containment comparison filesystem-aware by using case-insensitive matching
on Windows/macOS while preserving path-boundary validation so similarly prefixed
folders do not match.
In `@src/main/fs/checkFolderForUnknownContentModifications.ts`:
- Around line 34-60: Update getFullPathsOfFolderDirs to continue scanning when a
recursive subdirectory read fails, while recording failed subpaths so
checkFolderForUnknownModifications can suppress deletion handling only when the
inventory is incomplete. Preserve discovered music files from readable siblings
and avoid marking the entire top-level scan as failed. Also confirm and handle
whether symlinked directories should be traversed according to the intended scan
behavior.
In `@src/main/ipc.ts`:
- Line 128: The resyncPromise guard in the IPC handler does not coordinate with
startup and watcher scans. Move the single-flight state into a shared scan-layer
function such as runLibraryScan, and update the IPC resync flow plus
checkFolderForUnknownModifications callers in addWatchersToParentFolders and
addWatchersToFolders to use it, ensuring all scans share one mutex and return
the existing scan result.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx`:
- Around line 19-37: Update handleResyncClick’s catch block to route the resync
failure through the codebase’s renderer logging wrapper and IPC path instead of
console.error, preserving the existing error message and notification behavior.
- Around line 153-165: Update the resync button in the Sidebar component’s
handleResyncClick flow to expose aria-busy={isSyncing}, and replace the
accessible label text with a syncing-state translation while isSyncing is true.
Preserve the existing resync label when idle and keep the current disabled
behavior.
🪄 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: b87ac8ff-3746-424b-9524-fb90740068ee
📒 Files selected for processing (14)
src/main/core/checkForNewSongs.tssrc/main/fs/addWatchersToFolders.tssrc/main/fs/addWatchersToParentFolders.tssrc/main/fs/checkFolderForUnknownContentModifications.tssrc/main/fs/parseFolderStructuresForSongPaths.tssrc/main/ipc.tssrc/renderer/src/assets/locales/en/en.jsonsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/hooks/useWindowManagement.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/artists/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/types/app.d.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Lint & Format Check / Lint & Format Check: fix(#463): folder scan and library resync not detecting new files
Conclusion: failure
##[group]Run npm run lint --deny-warnings
�[36;1mnpm run lint --deny-warnings�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
npm warn Unknown cli config "--deny-warnings". This will stop working in the next major version of npm.
> nora@4.0.0-alpha.4 lint
> oxlint .
##[warning]React Hook useMemo has a missing dependency: 'selectedPlaylistIds.length'
##[warning]React Hook useMemo has unnecessary dependency: playlists
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]React Hook useEffect has a missing dependency: 'currentSongData'
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]React Hook useMemo has missing dependencies: 'addToEnd', and 'addToNext'
##[warning]React Hook useMemo has unnecessary dependency: queue.position
##[warning]React Hook useMemo has unnecessary dependency: updateQueueData
##[warning]React Hook useMemo has unnecessary dependency: artworkPaths.artworkPath
##[warning]React Hook useMemo has unnecessary dependency: queue.songIds
##[warning]React Hook useCallback has a missing dependency: 'navigate'
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a differen...
GitHub Actions: Lint & Format Check / 0_Lint & Format Check.txt: fix(#463): folder scan and library resync not detecting new files
Conclusion: failure
##[group]Run npm run lint --deny-warnings
�[36;1mnpm run lint --deny-warnings�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
npm warn Unknown cli config "--deny-warnings". This will stop working in the next major version of npm.
> nora@4.0.0-alpha.4 lint
> oxlint .
##[warning]React Hook useMemo has a missing dependency: 'selectedPlaylistIds.length'
##[warning]React Hook useMemo has unnecessary dependency: playlists
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]React Hook useEffect has a missing dependency: 'currentSongData'
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]Fast refresh only works when a file only exports components. Move your component(s) to a separate file.
##[warning]React Hook useMemo has missing dependencies: 'addToEnd', and 'addToNext'
##[warning]React Hook useMemo has unnecessary dependency: queue.position
##[warning]React Hook useMemo has unnecessary dependency: updateQueueData
##[warning]React Hook useMemo has unnecessary dependency: artworkPaths.artworkPath
##[warning]React Hook useMemo has unnecessary dependency: queue.songIds
##[warning]React Hook useCallback has a missing dependency: 'navigate'
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a differen...
🧰 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/artists/index.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/main/fs/addWatchersToFolders.tssrc/renderer/src/routes/main-player/settings/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/types/app.d.tssrc/main/fs/parseFolderStructuresForSongPaths.tssrc/renderer/src/hooks/useWindowManagement.tsxsrc/main/ipc.tssrc/main/fs/addWatchersToParentFolders.tssrc/main/core/checkForNewSongs.tssrc/main/fs/checkFolderForUnknownContentModifications.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/artists/index.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/hooks/useWindowManagement.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/artists/index.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/hooks/useWindowManagement.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/fs/addWatchersToFolders.tssrc/main/fs/parseFolderStructuresForSongPaths.tssrc/main/ipc.tssrc/main/fs/addWatchersToParentFolders.tssrc/main/core/checkForNewSongs.tssrc/main/fs/checkFolderForUnknownContentModifications.ts
src/renderer/src/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create focused custom hooks for feature-specific logic, keeping them under 200 lines each, with clear single responsibility and documented dependencies
Files:
src/renderer/src/hooks/useWindowManagement.tsx
src/main/ipc.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Register all IPC handlers using ipcMain.handle() for async operations with return values and ipcMain.on() for fire-and-forget events, mapping them to core business logic modules
Files:
src/main/ipc.ts
src/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/checkForNewSongs.ts
🪛 GitHub Check: Lint & Format Check
src/main/fs/addWatchersToParentFolders.ts
[failure] 34-46: promise(catch-or-return)
Expected catch or return.
🔇 Additional comments (15)
src/renderer/src/hooks/useWindowManagement.tsx (1)
1-7: LGTM!src/renderer/src/routes/main-player/artists/$artistId.tsx (1)
29-29: LGTM!src/renderer/src/routes/main-player/artists/index.tsx (1)
25-25: LGTM!src/renderer/src/routes/main-player/search/index.tsx (1)
25-25: LGTM!src/renderer/src/routes/main-player/settings/index.tsx (1)
6-6: LGTM!src/main/core/checkForNewSongs.ts (1)
11-43: LGTM!src/main/fs/checkFolderForUnknownContentModifications.ts (2)
109-122: LGTM!Also applies to: 136-145
81-81: 🎯 Functional CorrectnessNo change needed.
FolderRowis declared once and is already scoped inside the function body rather than module-level.> Likely an incorrect or invalid review comment.src/main/fs/addWatchersToParentFolders.ts (2)
101-107: LGTM!Also applies to: 149-153
113-119: 🩺 Stability & AvailabilityNo change needed for recursive
fs.watchon Linux.Electron 41.x targets Node 24.x, where
fs.watch({ recursive: true })is supported on Linux, so this TODO is stale.src/main/fs/addWatchersToFolders.ts (1)
77-86: LGTM!src/main/fs/parseFolderStructuresForSongPaths.ts (1)
11-11: LGTM!src/types/app.d.ts (1)
972-973: LGTM!src/renderer/src/components/Sidebar/Sidebar.tsx (1)
4-7: LGTM!Also applies to: 16-17
src/renderer/src/assets/locales/en/en.json (1)
1054-1055: 🎯 Functional CorrectnessNo change needed.
settingsPage.resyncLibraryandnotifications.songDataUpdateFailedare present across non-en locales, and missing translated keys will fall back toenforRESYNC_PARTIAL.
| const runScan = (containingFolder: string) => { | ||
| isScanning = true; | ||
| checkFolderForUnknownModifications(containingFolder) | ||
| .catch((error) => { | ||
| logger.error('Debounced folder scan failed.', { error, containingFolder }); | ||
| }) | ||
| .finally(() => { | ||
| isScanning = false; | ||
| if (dirtyScan && dirtyPath) { | ||
| dirtyScan = false; | ||
| const next = dirtyPath; | ||
| dirtyPath = null; | ||
| runScan(next); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
CI lint failure: promise/catch-or-return on this chain.
The Lint & Format check fails here because the promise chain ends with .finally() (no catch last / no return). Returning the chain and moving rejection handling accordingly satisfies the rule and also makes runScan awaitable for tests.
🔧 Proposed fix
- const runScan = (containingFolder: string) => {
+ const runScan = (containingFolder: string): Promise<void> => {
isScanning = true;
- checkFolderForUnknownModifications(containingFolder)
- .catch((error) => {
- logger.error('Debounced folder scan failed.', { error, containingFolder });
- })
+ return checkFolderForUnknownModifications(containingFolder)
.finally(() => {
isScanning = false;
if (dirtyScan && dirtyPath) {
dirtyScan = false;
const next = dirtyPath;
dirtyPath = null;
- runScan(next);
+ void runScan(next);
}
- });
+ })
+ .catch((error) => {
+ logger.error('Debounced folder scan failed.', { error, containingFolder });
+ });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const runScan = (containingFolder: string) => { | |
| isScanning = true; | |
| checkFolderForUnknownModifications(containingFolder) | |
| .catch((error) => { | |
| logger.error('Debounced folder scan failed.', { error, containingFolder }); | |
| }) | |
| .finally(() => { | |
| isScanning = false; | |
| if (dirtyScan && dirtyPath) { | |
| dirtyScan = false; | |
| const next = dirtyPath; | |
| dirtyPath = null; | |
| runScan(next); | |
| } | |
| }); | |
| }; | |
| const runScan = (containingFolder: string): Promise<void> => { | |
| isScanning = true; | |
| return checkFolderForUnknownModifications(containingFolder) | |
| .finally(() => { | |
| isScanning = false; | |
| if (dirtyScan && dirtyPath) { | |
| dirtyScan = false; | |
| const next = dirtyPath; | |
| dirtyPath = null; | |
| void runScan(next); | |
| } | |
| }) | |
| .catch((error) => { | |
| logger.error('Debounced folder scan failed.', { error, containingFolder }); | |
| }); | |
| }; |
🧰 Tools
🪛 GitHub Check: Lint & Format Check
[failure] 34-46: promise(catch-or-return)
Expected catch or 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/fs/addWatchersToParentFolders.ts` around lines 32 - 47, Update the
runScan function so it returns the promise chain from
checkFolderForUnknownModifications, and arrange the rejection handler after
finally (or otherwise ensure the returned chain ends with catch) to satisfy
promise/catch-or-return while preserving the existing logging and dirty-scan
follow-up behavior.
Source: Linters/SAST tools
| const scheduleScan = (containingFolder: string) => { | ||
| dirtyPath = containingFolder; | ||
| if (debounceTimer) clearTimeout(debounceTimer); | ||
| debounceTimer = setTimeout(() => { | ||
| debounceTimer = null; | ||
| if (isScanning) { | ||
| dirtyScan = true; | ||
| return; | ||
| } | ||
| dirtyScan = false; | ||
| runScan(containingFolder); | ||
| }, 1500); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Debounce collapses events from different music folders.
dirtyPath holds a single path and the timer is shared per parent watcher. A parent folder can contain several configured music folders; if events arrive for folder A then folder B within the debounce window, only B is scanned and A's additions are dropped until another event happens. Track pending folders in a Set instead.
🐛 Proposed fix sketch
- let dirtyScan = false;
- let dirtyPath: string | null = null;
+ const pendingFolders = new Set<string>();
@@
const scheduleScan = (containingFolder: string) => {
- dirtyPath = containingFolder;
+ pendingFolders.add(containingFolder);
if (debounceTimer) clearTimeout(debounceTimer);
debounceTimer = setTimeout(() => {
debounceTimer = null;
- if (isScanning) {
- dirtyScan = true;
- return;
- }
- dirtyScan = false;
- runScan(containingFolder);
+ if (isScanning) return; // remaining folders stay queued in pendingFolders
+ void drainPendingFolders();
}, 1500);
};drainPendingFolders would pop entries from pendingFolders and scan them sequentially, re-checking the set in the finally of each scan.
🤖 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/fs/addWatchersToParentFolders.ts` around lines 49 - 61, Update the
watcher’s shared debounce state around scheduleScan to replace the single
dirtyPath with a Set of pending containing folders. Add each scheduled folder to
the set, then drain and scan every pending folder sequentially after the
debounce delay, rechecking the set after each scan (including when scanning is
already active) so events for different music folders are not dropped.
| if (relevantFolderSongPaths.length > 0) { | ||
| const deletedSongPaths = relevantFolderSongPaths.filter( | ||
| (songPath) => !dirs.some((dir) => dir === songPath) | ||
| ); | ||
|
|
||
| logger.debug(`New song additions/deletions detected.`, { | ||
| newlyAddedSongPathsCount: newlyAddedSongPaths.length, | ||
| deletedSongPathsCount: deletedSongPaths.length, | ||
| newlyAddedSongPaths, | ||
| deletedSongPaths, | ||
| folderPath | ||
| }); | ||
|
|
||
| // Prioritises deleting songs before adding new songs to prevent data clashes. | ||
| if (deletedSongPaths.length > 0) { | ||
| // deleting songs from the library that got deleted before application launch | ||
| await removeDeletedSongsFromLibrary(deletedSongPaths, abortController.signal); | ||
| logger.debug(`Song deletions detected.`, { | ||
| deletedSongPathsCount: deletedSongPaths.length, | ||
| deletedSongPaths, | ||
| folderPath | ||
| }); | ||
| const deletionFailed = await removeDeletedSongsFromLibrary( | ||
| deletedSongPaths, | ||
| abortController.signal | ||
| ); | ||
| if (deletionFailed.length > 0) result.deletionFailures.push(...deletionFailed); | ||
| } | ||
| } | ||
|
|
||
| if (dirs.length > 0) { | ||
| const newlyAddedSongPaths = dirs.filter( | ||
| (dir) => !relevantFolderSongPaths.some((songPath) => songPath === dir) | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Quadratic path diffing on every scan.
dirs.some(...) inside relevantFolderSongPaths.filter(...) (and the mirrored addition check) is O(n×m). With a large library (recursive scan now returns the whole tree), this is tens of millions of string comparisons per top-level folder, on every watcher-triggered scan.
⚡ Use sets for the diff
+ const diskSongPathSet = new Set(dirs);
+ const dbSongPathSet = new Set(relevantFolderSongPaths);
+
if (relevantFolderSongPaths.length > 0) {
- const deletedSongPaths = relevantFolderSongPaths.filter(
- (songPath) => !dirs.some((dir) => dir === songPath)
- );
+ const deletedSongPaths = relevantFolderSongPaths.filter(
+ (songPath) => !diskSongPathSet.has(songPath)
+ );
@@
if (dirs.length > 0) {
- const newlyAddedSongPaths = dirs.filter(
- (dir) => !relevantFolderSongPaths.some((songPath) => songPath === dir)
- );
+ const newlyAddedSongPaths = dirs.filter((dir) => !dbSongPathSet.has(dir));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (relevantFolderSongPaths.length > 0) { | |
| const deletedSongPaths = relevantFolderSongPaths.filter( | |
| (songPath) => !dirs.some((dir) => dir === songPath) | |
| ); | |
| logger.debug(`New song additions/deletions detected.`, { | |
| newlyAddedSongPathsCount: newlyAddedSongPaths.length, | |
| deletedSongPathsCount: deletedSongPaths.length, | |
| newlyAddedSongPaths, | |
| deletedSongPaths, | |
| folderPath | |
| }); | |
| // Prioritises deleting songs before adding new songs to prevent data clashes. | |
| if (deletedSongPaths.length > 0) { | |
| // deleting songs from the library that got deleted before application launch | |
| await removeDeletedSongsFromLibrary(deletedSongPaths, abortController.signal); | |
| logger.debug(`Song deletions detected.`, { | |
| deletedSongPathsCount: deletedSongPaths.length, | |
| deletedSongPaths, | |
| folderPath | |
| }); | |
| const deletionFailed = await removeDeletedSongsFromLibrary( | |
| deletedSongPaths, | |
| abortController.signal | |
| ); | |
| if (deletionFailed.length > 0) result.deletionFailures.push(...deletionFailed); | |
| } | |
| } | |
| if (dirs.length > 0) { | |
| const newlyAddedSongPaths = dirs.filter( | |
| (dir) => !relevantFolderSongPaths.some((songPath) => songPath === dir) | |
| ); | |
| const diskSongPathSet = new Set(dirs); | |
| const dbSongPathSet = new Set(relevantFolderSongPaths); | |
| if (relevantFolderSongPaths.length > 0) { | |
| const deletedSongPaths = relevantFolderSongPaths.filter( | |
| (songPath) => !diskSongPathSet.has(songPath) | |
| ); | |
| if (deletedSongPaths.length > 0) { | |
| logger.debug(`Song deletions detected.`, { | |
| deletedSongPathsCount: deletedSongPaths.length, | |
| deletedSongPaths, | |
| folderPath | |
| }); | |
| const deletionFailed = await removeDeletedSongsFromLibrary( | |
| deletedSongPaths, | |
| abortController.signal | |
| ); | |
| if (deletionFailed.length > 0) result.deletionFailures.push(...deletionFailed); | |
| } | |
| } | |
| if (dirs.length > 0) { | |
| const newlyAddedSongPaths = dirs.filter((dir) => !dbSongPathSet.has(dir)); |
🤖 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/fs/checkFolderForUnknownContentModifications.ts` around lines 185 -
207, Replace the nested membership checks in the deletion and addition diffing
around relevantFolderSongPaths with Set-based lookups: build sets for the
existing relevant paths and scanned dirs, then use has() while filtering
deletedSongPaths and newlyAddedSongPaths. Preserve the current deletion handling
and result behavior while reducing both comparisons to linear time.
| if (resyncPromise) { | ||
| await resyncPromise; | ||
| return; | ||
| } | ||
| resyncPromise = checkForNewSongs() | ||
| .then(({ hasFailures }) => { | ||
| if (hasFailures) { | ||
| sendMessageToRenderer({ messageCode: 'RESYNC_PARTIAL' }); | ||
| } else { | ||
| sendMessageToRenderer({ messageCode: 'RESYNC_SUCCESSFUL' }); | ||
| } | ||
| }) | ||
| .finally(() => { | ||
| resyncPromise = null; | ||
| }); | ||
| await resyncPromise; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Failure path emits no notification and isn't logged in main.
If checkForNewSongs() rejects, no RESYNC_* message is sent and nothing is logged in the main process; the renderer only gets an opaque IPC rejection. Add a catch that logs and reports the failure.
🔧 Proposed fix
resyncPromise = checkForNewSongs()
.then(({ hasFailures }) => {
if (hasFailures) {
sendMessageToRenderer({ messageCode: 'RESYNC_PARTIAL' });
} else {
sendMessageToRenderer({ messageCode: 'RESYNC_SUCCESSFUL' });
}
})
+ .catch((error) => {
+ logger.error('Library resync failed.', { error });
+ throw error;
+ })
.finally(() => {
resyncPromise = null;
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/ipc.ts` around lines 479 - 494, Update the resync promise chain in
the IPC handler around checkForNewSongs to catch rejections, log the failure in
the main process, and send an appropriate failure notification to the renderer.
Preserve the existing success/partial messages and ensure the finally block
still clears resyncPromise.
| <ul className="relative flex flex-1 min-h-0 flex-col gap-1 overflow-x-hidden overflow-y-auto pt-4 pb-2"> | ||
| {sideBarItems} | ||
| </ul> | ||
| <div className="mb-2 flex justify-center px-2"> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant sidebar component and nearby parent layout.
printf '--- candidate files ---\n'
fd -a 'Sidebar\.(tsx|ts)$' . || true
printf '\n--- Sidebar outline ---\n'
ast-grep outline src/renderer/src/components/Sidebar/Sidebar.tsx || true
printf '\n--- Relevant Sidebar lines 1-230 ---\n'
sed -n '1,230p' src/renderer/src/components/Sidebar/Sidebar.tsx | cat -n
printf '\n--- ErrorBoundary definition/usages ---\n'
rg -n "function ErrorBoundary|const ErrorBoundary|var ErrorBoundary|<ErrorBoundary|class ErrorBoundary|export .*ErrorBoundary" -S src/renderer/src -g '*.tsx' -g '*.ts' || trueRepository: Sandakan/Nora
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate files ---\n'
fd -a 'Sidebar\.(tsx|ts)$' . || true
printf '\n--- Sidebar outline ---\n'
ast-grep outline src/renderer/src/components/Sidebar/Sidebar.tsx || true
printf '\n--- Relevant Sidebar lines 1-230 ---\n'
sed -n '1,230p' src/renderer/src/components/Sidebar/Sidebar.tsx | cat -n
printf '\n--- ErrorBoundary definition/usages ---\n'
rg -n "function ErrorBoundary|const ErrorBoundary|var ErrorBoundary|<ErrorBoundary|class ErrorBoundary|export .*ErrorBoundary" -S src/renderer/src -g '*.tsx' -g '*.ts' || trueRepository: Sandakan/Nora
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
fd -a 'Sidebar\.(tsx|ts)$' . || true
echo '
--- Sidebar outline ---'
ast-grep outline src/renderer/src/components/Sidebar/Sidebar.tsx || true
echo '
--- Relevant Sidebar lines 1-230 ---'
sed -n '1,230p' src/renderer/src/components/Sidebar/Sidebar.tsx | cat -n
echo '
--- ErrorBoundary definition/usages ---'
rg -n "function ErrorBoundary|const ErrorBoundary|var ErrorBoundary|<ErrorBoundary|class ErrorBoundary|export .*ErrorBoundary" -S src/renderer/src -g '*.tsx' -g '*.ts' || trueRepository: Sandakan/Nora
Length of output: 8181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ErrorBoundary implementation ---'
sed -n '1,110p' src/renderer/src/components/ErrorBoundary.tsx | cat -n
echo '
--- BodyAndSidebarContainer implementation ---'
sed -n '1,140p' src/renderer/src/components/BodyAndSidebarContainer.tsx | cat -n
echo '
--- Imports of BodyAndSidebarContainer ---'
rg -n "BodyAndSidebarContainer" -S src/renderer/src -g '*.tsx' -g '*.ts' || trueRepository: Sandakan/Nora
Length of output: 4560
Add flex flex-col to the <nav> wrapper.
The ErrorBoundary passes its children through unchanged, so the <ul>’s flex-1 min-h-0 has no flex column parent. This makes the scrollable items stack above the resync button instead of keeping the button pinned inside the fixed-height sidebar.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx` around lines 149 - 152, Add
the flex and flex-col classes to the nav wrapper containing the Sidebar
ErrorBoundary, preserving its existing layout classes. Ensure the wrapper
establishes a column flex parent so the ul with flex-1 min-h-0 expands and
scrolls while the resync button remains pinned within the fixed-height sidebar.
Major 1: clearAllFolderWatches only closed music folder controllers, not parent folder controllers. Each reset accumulated duplicate parent watchers. Now calls closeAllAbortControllers() which closes everything. Also fixed closeAbortController to handle multiple instances with same ID and splice them from the array.
|
@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. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/renderer/src/routes/main-player/artists/$artistId.tsx (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize and verify the route lint exception.
These four files repeat an inline suppression whose namespace may not match the active linter. Verify whether the repository uses ESLint’s
react-refresh/only-export-componentsor Oxlint’sreact/only-export-components; then configure the intentionalRouteexport exception once for route modules instead of suppressing it in every file. (npmjs.com)
src/renderer/src/routes/main-player/artists/$artistId.tsx#L29-L30: use the configured rule namespace or shared route override.src/renderer/src/routes/main-player/artists/index.tsx#L25-L26: use the configured rule namespace or shared route override.src/renderer/src/routes/main-player/search/index.tsx#L25-L26: use the configured rule namespace or shared route override.src/renderer/src/routes/main-player/settings/index.tsx#L6-L7: use the configured rule namespace or shared route override.🤖 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/artists/`$artistId.tsx around lines 29 - 30, Verify whether the repository uses ESLint’s react-refresh/only-export-components or Oxlint’s react/only-export-components, then configure the intentional Route export exception once in the shared lint configuration for route modules. Remove the duplicated inline suppressions at src/renderer/src/routes/main-player/artists/$artistId.tsx:29-30, src/renderer/src/routes/main-player/artists/index.tsx:25-26, src/renderer/src/routes/main-player/search/index.tsx:25-26, and src/renderer/src/routes/main-player/settings/index.tsx:6-7, ensuring the configured rule namespace and route override cover all four files.Source: MCP tools
src/main/fs/parseFolderStructuresForSongPaths.ts (1)
103-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
clearAllFolderWatchesno longer awaits anything — dropasync.Both statements are synchronous now, so the
asynckeeps callers (line 118, inside the transaction) producing a floating promise for no reason.♻️ Suggested refactor
-const clearAllFolderWatches = async () => { +const clearAllFolderWatches = () => { closeAllAbortControllers(); logger.info('Closed all folder watches successfully.'); };🤖 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/fs/parseFolderStructuresForSongPaths.ts` around lines 103 - 106, Remove the async modifier from clearAllFolderWatches because its operations are synchronous; keep closeAllAbortControllers and the success log unchanged so callers no longer receive an unnecessary promise.src/renderer/src/components/Sidebar/Sidebar.tsx (1)
153-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the busy state to assistive tech.
disabled+animate-pulsecommunicate progress visually only. Addaria-busy={isSyncing}(and ideally anaria-livestatus region announcing completion/failure) so screen-reader users learn the resync started and finished.♿ Suggested tweak
disabled={isSyncing} + aria-busy={isSyncing} aria-label={t('settingsPage.resyncLibrary')}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Sidebar/Sidebar.tsx` around lines 153 - 165, Update the resync button in the Sidebar component to expose its current state with aria-busy bound to isSyncing, and add an accessible aria-live status region that announces when resync starts and when it completes or fails. Reuse the existing translation mechanism for these status messages and keep the current visual disabled and animation behavior unchanged.src/main/fs/addWatchersToParentFolders.ts (1)
20-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDB folder tree is re-read on every raw watcher event.
findContainingMusicFolderruns before the debounce, so eachrenameevent triggersgetAllFolderStructures()— a recursive per-folder query walk. A bulk copy into a watched tree fires hundreds of events and multiplies that cost. Cache the flattened paths (invalidated when watchers are rebuilt), or resolve the containing folder after debouncing.🤖 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/fs/addWatchersToParentFolders.ts` around lines 20 - 30, Avoid calling getAllFolderStructures from findContainingMusicFolder for every raw watcher event. Cache the flattened folder paths and reuse them during watcher processing, invalidating and rebuilding the cache whenever watchers are rebuilt; alternatively, move containing-folder resolution until after debouncing while preserving the longest matching path behavior.src/main/core/checkForNewSongs.ts (1)
18-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInvert to a guard clause.
The
if (length > 0) { … } else { warn }wrapper adds a nesting level for the whole loop. An early return keeps the body flat.♻️ Suggested refactor
- if (topLevelFolders.length > 0) { - for (const folderPath of topLevelFolders) { - try { - const result = await checkFolderForUnknownModifications(folderPath); - if (result.failedSongPaths.length > 0) - failedSongPaths.push(...result.failedSongPaths); - if (result.deletionFailures.length > 0) - deletionFailures.push(...result.deletionFailures); - if (result.scanFailed) scanFailed = true; - } catch (error) { - logger.error(`Failed to check for unknown modifications of a path.`, { - error, - path: folderPath - }); - failedFolders.push(folderPath); - } - } - } else { - logger.warn('checkForNewSongs: no top-level music folders found — nothing to scan.'); - } + if (topLevelFolders.length === 0) { + logger.warn('checkForNewSongs: no top-level music folders found — nothing to scan.'); + return { failedFolders, failedSongPaths, deletionFailures, hasFailures: false, scanFailed }; + } + + for (const folderPath of topLevelFolders) { + try { + const result = await checkFolderForUnknownModifications(folderPath); + failedSongPaths.push(...result.failedSongPaths); + deletionFailures.push(...result.deletionFailures); + if (result.scanFailed) scanFailed = true; + } catch (error) { + logger.error(`Failed to check for unknown modifications of a path.`, { + error, + path: folderPath + }); + failedFolders.push(folderPath); + } + }As per coding guidelines: "Use guard clauses with early returns to keep function logic flat and readable".
🤖 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/checkForNewSongs.ts` around lines 18 - 37, Update the surrounding checkForNewSongs flow to handle an empty topLevelFolders collection with the existing warning and an early return, then move the folder iteration outside the conditional so the try/catch processing remains unchanged and unnested.Source: Coding guidelines
src/main/ipc.ts (1)
483-494: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftSingle-flight only covers the IPC entry point.
Watcher-triggered scans (
addWatchersToParentFolders→checkFolderForUnknownModifications) and startup scans call the scanner directly, so a manual resync can run concurrently with them over the same folders. Deduping relies ontryToParseSong'spathsQueue, which does not protect the deletion reconciliation path. Consider moving the guard into the scanner/core module so every trigger shares it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc.ts` around lines 483 - 494, Move the single-flight resync guard from the IPC handler into the shared scanner/core flow used by checkForNewSongs, addWatchersToParentFolders, checkFolderForUnknownModifications, and startup scans. Ensure every scan trigger uses the same guarded entry point, including deletion reconciliation, so concurrent scans over the same folders are prevented. Keep the existing resync completion notifications and reset the guard after each scan finishes.src/main/fs/controlAbortControllers.ts (1)
24-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
closeAbortControllersByPrefixexport.
controlAbortControllers.tsis the only file where this function appears; no entry imports it, so dropping it keepscontrolAbortControllerssurface area smaller.🤖 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/fs/controlAbortControllers.ts` around lines 24 - 35, Remove the unused closeAbortControllersByPrefix function and its export from controlAbortControllers.ts, leaving the remaining abort-controller functionality unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/fs/parseFolderStructuresForSongPaths.ts`:
- Around line 124-125: Update the reset-watcher calls in the surrounding
folder-structure flow to handle rejected promises from both
addWatchersToFolders() and addWatchersToParentFolders(). Attach consistent catch
handling that reports failures through the existing error-handling mechanism,
preventing database errors from becoming unhandled rejections.
In `@src/renderer/src/assets/locales/en/en.json`:
- Around line 1054-1055: Update the renderer notification handling in
parseNotificationFromMain to configure RESYNC_PARTIAL with the intended warning
behavior and icon alongside RESYNC_SUCCESSFUL, then add backend.RESYNC_PARTIAL
translations to every non-English locale file using the established locale
structure.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx`:
- Around line 25-33: Update the resync failure handling around the Sidebar
resynchronization flow to use the dedicated notifications.resyncLibraryFailed
translation key instead of notifications.songDataUpdateFailed. Replace
console.error with the established renderer log bridge, preserving the existing
error context and notification behavior.
---
Nitpick comments:
In `@src/main/core/checkForNewSongs.ts`:
- Around line 18-37: Update the surrounding checkForNewSongs flow to handle an
empty topLevelFolders collection with the existing warning and an early return,
then move the folder iteration outside the conditional so the try/catch
processing remains unchanged and unnested.
In `@src/main/fs/addWatchersToParentFolders.ts`:
- Around line 20-30: Avoid calling getAllFolderStructures from
findContainingMusicFolder for every raw watcher event. Cache the flattened
folder paths and reuse them during watcher processing, invalidating and
rebuilding the cache whenever watchers are rebuilt; alternatively, move
containing-folder resolution until after debouncing while preserving the longest
matching path behavior.
In `@src/main/fs/controlAbortControllers.ts`:
- Around line 24-35: Remove the unused closeAbortControllersByPrefix function
and its export from controlAbortControllers.ts, leaving the remaining
abort-controller functionality unchanged.
In `@src/main/fs/parseFolderStructuresForSongPaths.ts`:
- Around line 103-106: Remove the async modifier from clearAllFolderWatches
because its operations are synchronous; keep closeAllAbortControllers and the
success log unchanged so callers no longer receive an unnecessary promise.
In `@src/main/ipc.ts`:
- Around line 483-494: Move the single-flight resync guard from the IPC handler
into the shared scanner/core flow used by checkForNewSongs,
addWatchersToParentFolders, checkFolderForUnknownModifications, and startup
scans. Ensure every scan trigger uses the same guarded entry point, including
deletion reconciliation, so concurrent scans over the same folders are
prevented. Keep the existing resync completion notifications and reset the guard
after each scan finishes.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx`:
- Around line 153-165: Update the resync button in the Sidebar component to
expose its current state with aria-busy bound to isSyncing, and add an
accessible aria-live status region that announces when resync starts and when it
completes or fails. Reuse the existing translation mechanism for these status
messages and keep the current visual disabled and animation behavior unchanged.
In `@src/renderer/src/routes/main-player/artists/`$artistId.tsx:
- Around line 29-30: Verify whether the repository uses ESLint’s
react-refresh/only-export-components or Oxlint’s react/only-export-components,
then configure the intentional Route export exception once in the shared lint
configuration for route modules. Remove the duplicated inline suppressions at
src/renderer/src/routes/main-player/artists/$artistId.tsx:29-30,
src/renderer/src/routes/main-player/artists/index.tsx:25-26,
src/renderer/src/routes/main-player/search/index.tsx:25-26, and
src/renderer/src/routes/main-player/settings/index.tsx:6-7, ensuring the
configured rule namespace and route override cover all four files.
🪄 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: 73bbcbb6-7e2c-4643-9a8d-6fe3721f468f
📒 Files selected for processing (15)
src/main/core/checkForNewSongs.tssrc/main/fs/addWatchersToFolders.tssrc/main/fs/addWatchersToParentFolders.tssrc/main/fs/checkFolderForUnknownContentModifications.tssrc/main/fs/controlAbortControllers.tssrc/main/fs/parseFolderStructuresForSongPaths.tssrc/main/ipc.tssrc/renderer/src/assets/locales/en/en.jsonsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/hooks/useWindowManagement.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/artists/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/types/app.d.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Lint & Format Check / 0_Lint & Format Check.txt: fix(#463): folder scan and library resync not detecting new files
Conclusion: failure
##[group]Run npm run lint --deny-warnings
�[36;1mnpm run lint --deny-warnings�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
npm warn Unknown cli config "--deny-warnings". This will stop working in the next major version of npm.
> nora@4.0.0-alpha.4 lint
> oxlint .
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has missing dependencies: 'queue.currentSongId', and 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a d...
GitHub Actions: Lint & Format Check / Lint & Format Check: fix(#463): folder scan and library resync not detecting new files
Conclusion: failure
##[group]Run npm run lint --deny-warnings
�[36;1mnpm run lint --deny-warnings�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
npm warn Unknown cli config "--deny-warnings". This will stop working in the next major version of npm.
> nora@4.0.0-alpha.4 lint
> oxlint .
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]React Hook useCallback has missing dependencies: 'queue.currentSongId', and 'queue'
##[warning]React Hook useCallback has a missing dependency: 'queue'
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a different type.
##[warning]Unexpected `any`. Specify a d...
🧰 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/artists/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/types/app.d.tssrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/main/fs/parseFolderStructuresForSongPaths.tssrc/main/ipc.tssrc/main/fs/addWatchersToFolders.tssrc/main/fs/addWatchersToParentFolders.tssrc/main/core/checkForNewSongs.tssrc/renderer/src/hooks/useWindowManagement.tsxsrc/main/fs/controlAbortControllers.tssrc/main/fs/checkFolderForUnknownContentModifications.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/artists/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/hooks/useWindowManagement.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/artists/index.tsxsrc/renderer/src/routes/main-player/search/index.tsxsrc/renderer/src/routes/main-player/artists/$artistId.tsxsrc/renderer/src/routes/main-player/settings/index.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/hooks/useWindowManagement.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/fs/parseFolderStructuresForSongPaths.tssrc/main/ipc.tssrc/main/fs/addWatchersToFolders.tssrc/main/fs/addWatchersToParentFolders.tssrc/main/core/checkForNewSongs.tssrc/main/fs/controlAbortControllers.tssrc/main/fs/checkFolderForUnknownContentModifications.ts
src/main/ipc.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Register all IPC handlers using ipcMain.handle() for async operations with return values and ipcMain.on() for fire-and-forget events, mapping them to core business logic modules
Files:
src/main/ipc.ts
src/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/checkForNewSongs.ts
src/renderer/src/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create focused custom hooks for feature-specific logic, keeping them under 200 lines each, with clear single responsibility and documented dependencies
Files:
src/renderer/src/hooks/useWindowManagement.tsx
🪛 GitHub Check: Lint & Format Check
src/main/fs/addWatchersToParentFolders.ts
[failure] 34-46: promise(catch-or-return)
Expected catch or return.
🔇 Additional comments (20)
src/renderer/src/hooks/useWindowManagement.tsx (1)
1-7: LGTM!src/main/fs/checkFolderForUnknownContentModifications.ts (4)
185-207: Quadratic path diffing on every scan.Still present:
dirs.some(...)insiderelevantFolderSongPaths.filter(...)and the mirrored addition check are O(n×m) over the full recursive tree. UseSetlookups for both diffs.
13-32: LGTM!Also applies to: 34-60
109-122: LGTM!
161-183: LGTM!Also applies to: 224-228
src/main/fs/addWatchersToParentFolders.ts (5)
32-47: CI lint failure:promise/catch-or-returnon this chain.Still failing — the chain ends with
.finally(). Return the chain and move the rejection handler last.Source: Linters/SAST tools
49-61: Debounce collapses events from different music folders.
dirtyPathstill holds a single path with a shared timer, so interleaved events across two music folders under the same parent drop one folder's scan. Track pending folders in aSet.
63-98: LGTM!
149-153: LGTM!
113-119: 🎯 Functional CorrectnessNo Linux recursive-watch fallback needed here.
This app targets Electron 41 (Node >= 24), where
fs.watch(..., { recursive: true })is supported on Linux, so the TODO is stale for the declared runtime.> Likely an incorrect or invalid review comment.src/main/ipc.ts (2)
479-494: Failure path emits no notification and isn't logged in main.Still unhandled: if
checkForNewSongs()rejects, noRESYNC_*message is sent and nothing is logged in the main process — the renderer only sees an opaque IPC rejection. Add acatchthat logs and reports.
128-128: LGTM!src/renderer/src/components/Sidebar/Sidebar.tsx (2)
149-152: Addflex flex-colto the<nav>wrapper.Unchanged at lines 142-146:
ErrorBoundaryrenders children through, so the<ul>'sflex-1 min-h-0has no flex-column parent and the resync button won't stay pinned inside the fixed-height sidebar.
4-7: LGTM!src/main/core/checkForNewSongs.ts (2)
6-9: LGTM!
39-42: LGTM!src/main/fs/controlAbortControllers.ts (2)
6-22: LGTM!
37-42: LGTM!src/main/fs/addWatchersToFolders.ts (1)
77-86: LGTM!src/types/app.d.ts (1)
972-973: LGTM!
| if (resetWatchers) addWatchersToFolders(); | ||
| if (resetWatchers) addWatchersToParentFolders(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unhandled rejection risk on the new watcher-rebuild call.
addWatchersToParentFolders() is async and awaits getAllFolderStructures(); a DB failure rejects with no handler, which surfaces as an unhandled rejection in the main process. Attach a catch (same applies to the adjacent addWatchersToFolders() call).
🛡️ Suggested fix
- if (resetWatchers) addWatchersToFolders();
- if (resetWatchers) addWatchersToParentFolders();
+ if (resetWatchers) {
+ void addWatchersToFolders().catch((error) =>
+ logger.error('Failed to re-add folder watchers.', { error })
+ );
+ void addWatchersToParentFolders().catch((error) =>
+ logger.error('Failed to re-add parent folder watchers.', { error })
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (resetWatchers) addWatchersToFolders(); | |
| if (resetWatchers) addWatchersToParentFolders(); | |
| if (resetWatchers) { | |
| void addWatchersToFolders().catch((error) => | |
| logger.error('Failed to re-add folder watchers.', { error }) | |
| ); | |
| void addWatchersToParentFolders().catch((error) => | |
| logger.error('Failed to re-add parent folder watchers.', { 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/fs/parseFolderStructuresForSongPaths.ts` around lines 124 - 125,
Update the reset-watcher calls in the surrounding folder-structure flow to
handle rejected promises from both addWatchersToFolders() and
addWatchersToParentFolders(). Attach consistent catch handling that reports
failures through the existing error-handling mechanism, preventing database
errors from becoming unhandled rejections.
| "RESYNC_SUCCESSFUL": "Library resync successful.", | ||
| "RESYNC_PARTIAL": "Library resync completed with some failures.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts --type=tsx 'RESYNC_SUCCESSFUL|RESYNC_PARTIAL' -C3
fd -g '*.json' . -p src/renderer/src/assets/locales --exec sh -c 'printf "%s: " "$1"; grep -c RESYNC_SUCCESSFUL "$1" || true' _ {}Repository: Sandakan/Nora
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Locale counts:\n'
fd -g '*.json' . -p src/renderer/src/assets/locales --exec sh -c 'printf "%s: " "$1"; grep -c RESYNC_SUCCESSFUL "$1" || true; grep -c RESYNC_PARTIAL "$1" || true' _ {}
printf '\nCode references:\n'
rg -nP 'RESYNC_SUCCESSFUL|RESYNC_PARTIAL' -S . --glob '*.{ts,tsx,js,jsx,json}' -C 3 || true
printf '\nPotential message-code handlers/configs:\n'
rg -nP 'message.*code|code.*icon|notification|getMessage|t\\(|i18n|RESYNC_SUCCESSFUL|RESYNC_PARTIAL' -S . --glob '*.{ts,tsx,js,jsx}' -C 2 || trueRepository: Sandakan/Nora
Length of output: 7208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Locales containing RESYNC_PARTIAL:\n'
fd -g '*.json' . -p src/renderer/src/assets/locales --exec sh -c 'printf "%s: " "$1"; grep -c "RESYNC_PARTIAL" "$1" || true' _ {}
printf '\nParse notification handler excerpt:\n'
sed -n '1,180p' src/renderer/src/other/parseNotificationFromMain.tsxRepository: Sandakan/Nora
Length of output: 5371
Wire RESYNC_PARTIAL through the renderer notification handlers and locale translations.
The type and IPC message are added, but src/renderer/src/other/parseNotificationFromMain.tsx only configures RESYNC_SUCCESSFUL, and the non-English locale files do not contain backend.RESYNC_PARTIAL, so partial resync notifications can miss the intended warning behavior/icon and fall back to English.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/assets/locales/en/en.json` around lines 1054 - 1055, Update
the renderer notification handling in parseNotificationFromMain to configure
RESYNC_PARTIAL with the intended warning behavior and icon alongside
RESYNC_SUCCESSFUL, then add backend.RESYNC_PARTIAL translations to every
non-English locale file using the established locale structure.
| console.error('Failed to resync songs library.', err); | ||
| addNewNotifications([ | ||
| { | ||
| id: 'resyncLibraryFailed', | ||
| content: t('notifications.songDataUpdateFailed'), | ||
| iconName: 'error', | ||
| duration: 5000 | ||
| } | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'songDataUpdateFailed|resyncLibrary' src/renderer/src/assets/locales/en/en.json
rg -nP --type=tsx 'getRendererLogs|window\.api\.log' -C2 | head -40Repository: Sandakan/Nora
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Sidebar relevant lines ---'
fd -a 'Sidebar.tsx$' . | while read -r f; do
wc -l "$f"
sed -n '1,80p' "$f"
done
echo '--- console.error / log bridge usage in tsx ---'
rg -n -C2 'console\.error|getRendererLogs|window\.api\.log|logRenderer' --glob 'src/renderer/src/**/*.tsx'
echo '--- translation keys around relevant strings ---'
sed -n '500,530p' src/renderer/src/assets/locales/en/en.json
sed -n '1035,1048p' src/renderer/src/assets/locales/en/en.json
echo '--- backend resync partial occurrences ---'
rg -n 'RESYNC_PARTIAL|resyncLibrary|songDataUpdateFailed' --glob "!node_modules" --glob "!dist" --glob "!build" .Repository: Sandakan/Nora
Length of output: 2722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Sidebar relevant lines ---'
sed -n '1,90p' src/renderer/src/components/Sidebar/Sidebar.tsx
echo '--- console/error logging bridge usage in tsx ---'
rg -n -C2 'console\.error|console\.log|getRendererLogs|window\.api\.log|logRenderer|ipcRenderer\.send\(".*log' --glob 'src/renderer/src/**/*.tsx' src/renderer/src 2>/dev/null || true
echo '--- translation keys around relevant strings ---'
python3 - <<'PY'
paths = [
'src/renderer/src/assets/locales/en/en.json'
]
for path in paths:
print(f'FILE {path}')
with open(path, encoding='utf-8') as f:
lines = f.readlines()
for i,line in enumerate(lines,1):
if 'resyncLibrary' in line or 'songDataUpdateFailed' in line or 'songDataUpdateSucceeded' in line:
lo=max(1,i-3); hi=min(len(lines),i+3)
for j in range(lo,hi+1):
print(f'{j}: {lines[j-1].rstrip()}')
print('---')
PY
echo '--- backend resync partial occurrences ---'
rg -n -C2 'RESYNC_PARTIAL|resyncLibrary|songDataUpdateFailed|songDataUpdateSucceeded|resyncSongsLibrary' --glob '!node_modules' --glob '!dist' --glob '!build' . || true
echo '--- log bridge related files ---'
rg -n -C2 'rendererLogs|getRendererLogs|logRenderer|audioLibraryControls\.resyncSongsLibrary|RESYNC_PARTIAL|songDataUpdate' src --glob '!**/*.map' || trueRepository: Sandakan/Nora
Length of output: 50369
Show a resync-specific failure notification and log through the renderer log bridge.
t('notifications.songDataUpdateFailed') describes metadata updates, while this path is a library resync failure. Add a dedicated notifications.resyncLibraryFailed key and replace the raw console.error with the renderer log bridge so errors surface in production logs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx` around lines 25 - 33, Update
the resync failure handling around the Sidebar resynchronization flow to use the
dedicated notifications.resyncLibraryFailed translation key instead of
notifications.songDataUpdateFailed. Replace console.error with the established
renderer log bridge, preserving the existing error context and notification
behavior.
…r scan CI lint failed on addWatchersToParentFolders runScan - the checkFolderForUnknownModifications chain's catch returns undefined, tripping promise/catch-or-return. Added void to mark the fire-and- forget intent explicitly.
|
@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 '=== PR metadata and changed files ==='
gh pr view 490 --repo Sandakan/Nora \
--json number,title,state,baseRefName,headRefName,commits,files \
--jq '{
number, title, state, base: .baseRefName, head: .headRefName,
commits: [.commits[] | {oid, messageHeadline}],
files: [.files[].path]
}'
echo '=== Changed-file patches ==='
gh api --paginate 'repos/Sandakan/Nora/pulls/490/files?per_page=100' \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[patch unavailable]")"'
echo '=== Target module outlines ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/checkForFolderModifications.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/db/queries/folders.ts \
src/main/db/queries/songs.ts \
src/main/parseSong/parseSong.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
done
echo '=== Target source with line numbers ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/checkForFolderModifications.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/db/queries/folders.ts \
src/main/db/queries/songs.ts \
src/main/parseSong/parseSong.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo '=== Calls, watcher lifecycle, IPC, and error boundaries ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(checkForNewSongs|checkFolderForUnknownModifications|addWatchersToParentFolders|addWatchersToFolders|resyncSongsLibrary|saveAbortController|closeAbortController|closeAllAbortControllers|clearAllFolderWatches|fsSync\.watch)\s*\(' \
src || true
echo '=== Scanner/watcher test coverage and package scripts ==='
if [ -f package.json ]; then
cat -n package.json | sed -n '1,260p'
fi
fd -HI -t f . | rg '(^|/)(test|tests|__tests__)/|(\.test|\.spec)\.(ts|tsx|js|jsx)$' | sort | head -350
echo '=== Localization coverage for resync messages ==='
rg -n -C 3 --glob '*.{ts,tsx,json}' \
'RESYNC_(SUCCESSFUL|PARTIAL)|resyncLibrary|songDataUpdateFailed' src || trueLength of output: 128016 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Scanner and scan-result implementation ==='
cat -n src/main/core/checkForNewSongs.ts
cat -n src/main/fs/checkFolderForUnknownContentModifications.ts
echo '=== Parent watcher implementation ==='
cat -n src/main/fs/addWatchersToParentFolders.ts
echo '=== Direct watcher and controller registry ==='
cat -n src/main/fs/addWatchersToFolders.ts
cat -n src/main/fs/controlAbortControllers.ts
echo '=== Folder persistence and relevant queries ==='
sed -n '80,145p' src/main/fs/parseFolderStructuresForSongPaths.ts
ast-grep run --lang typescript \
--pattern 'export const getSongsInFolders = async ($$$) => { $$$ }' \
src/main/db/queries/songs.ts || true
ast-grep run --lang typescript \
--pattern 'export const getAllFolderStructures = async ($$$) => { $$$ }' \
src/main/db/queries/folders.ts || true
rg -n -C 8 \
'getSongsInFolders|getAllFolderStructures|getAllFolders|getFolderFromPath|saveAbortController|closeAbortController|closeAllAbortControllers' \
src/main/db/queries src/main/fs src/main/core
echo '=== Current scanner/watcher test matches ==='
rg -n -C 3 \
'checkForNewSongs|checkFolderForUnknownModifications|addWatchersToParentFolders|addWatchersToFolders|RESYNC_PARTIAL' \
test src --glob '*.{test,spec}.{ts,tsx}' || true
echo '=== Current diff statistics ==='
git diff --stat HEAD~1..HEAD
git status --shortLength of output: 67877
I reviewed the current PR head independently. Static inspection cannot prove zero defects. The current code fixes the core multi-root and recursive-scan defects. The remaining issues affect error handling, concurrent scans, and Linux support. CriticalNo critical issue found in the reviewed paths. Major1. Startup and direct-folder scans can reject without a caller-side error boundary
These calls do not await or catch their returned promises: checkForNewSongs();
saveFolderStructures([folder]);
checkFolderForUnknownModifications(folder.path);If a database query, folder scan, or watcher reset rejects, the process can produce an unhandled rejection. The outer Fix: use one observed scan entry point. At minimum: void checkForNewSongs().catch((error) =>
logger.error('Startup library scan failed.', { error })
);Apply the same pattern to 2. Scan serialization only covers IPC resync requests
A manual resync can overlap a watcher scan or startup scan. These operations read inventory state and can remove or add library records concurrently. Fix: move the single-flight state from 3. Automatic nested discovery is not supported on Linux
The parent watcher requires: fsSync.watch(parentFolderPath, { recursive: true })The source includes: // TODO - recursive mode won't work on linuxOn Linux or unsupported filesystems, a new unregistered nested album folder can produce no event. The user must then restart the app or run a manual resync. Fix: implement a supported Linux strategy, such as managed watchers for descendant directories. If that is out of scope, document the platform limitation and provide a tested fallback. Minor4. A watcher reset does not cancel an already scheduled parent-watcher scan
This is not usually destructive. It can cause redundant scans during folder updates. Fix: expose a parent-watcher cleanup function that aborts the watcher and clears its debounce timer. Store timer cleanup with the watcher lifecycle. 5.
|
…cher cleanup, Linux fallback
Major 1: startup checkForNewSongs/addWatchersToFolders/addWatchersToParentFolders
were fire-and-forget, so a DB query or scan failure produced an unhandled
rejection. All three now observe their promise with a .catch + logger.error.
Major 2: scan serialization only covered IPC resync (resyncPromise in
initializeIPC). checkForNewSongs is now single-flight internally
(module-level activeScanPromise, cleared in finally), so IPC resync,
startup, and future callers all join one scan. Removed the dead
resyncPromise from ipc.ts.
Major 3: parent-folder watcher used fs.watch recursive:true which does
not work on Linux. Added platform detection: non-Linux keeps recursive,
Linux falls back to non-recursive watch with a documented warn that
nested discovery relies on the library scan / manual resync.
Minor 4: a watcher reset could not cancel an already-scheduled
parent-watcher scan because the debounce timer lived only in the old
closure. createParentFolderWatcherFunction now returns { handler,
cleanup }; cleanup clears the debounce timer and is registered with the
abort controller so close* runs it.
Minor 5: RESYNC_PARTIAL had no notification config. Added warning-styled
notification entry.
Minor 6: preload resyncSongsLibrary declared Promise<true> while the IPC
handler returns undefined. Changed to Promise<void>.
Minor 7: added 3 checkForNewSongs tests (basic result, concurrent
coalescing to one scan, fresh scan after completion). 339 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
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"
PR="490"
echo '=== PR metadata, merge state, checks, and latest commits ==='
gh pr view "$PR" --repo "$REPO" \
--json number,title,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,isDraft,statusCheckRollup,commits \
--jq '{
number,title,state,base:.baseRefName,baseOid:.baseRefOid,head:.headRefName,headOid:.headRefOid,
mergeable,mergeStateStatus,isDraft,
checks:[.statusCheckRollup[]? | {name,conclusion,status,detailsUrl}],
commits:[.commits[] | {oid,messageHeadline}]
}'
echo '=== Base-tip and ancestry verification ==='
base_oid="$(gh api "repos/$REPO/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head_oid="$(gh pr view "$PR" --repo "$REPO" --json headRefOid --jq '.headRefOid')"
echo "release_tip=$base_oid"
echo "pr_head=$head_oid"
gh api "repos/$REPO/compare/$base_oid...$head_oid" --jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
echo '=== Full changed-file inventory and patches ==='
gh api --paginate "repos/$REPO/pulls/$PR/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\nstatus=\(.status) additions=\(.additions) deletions=\(.deletions)\n\(.patch // "[patch unavailable]")"'
echo '=== Current changed source maps ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/fs/checkForFolderModifications.ts \
src/main/db/queries/folders.ts \
src/main/db/queries/songs.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
cat -n "$f"
fi
done
echo '=== Calls, IPC contracts, lifecycle, and watcher cleanup ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(checkForNewSongs|checkFolderForUnknownModifications|addWatchersToParentFolders|addWatchersToFolders|saveFolderStructures|resyncSongsLibrary|registerWatcherCleanup|saveAbortController|closeAbortController|closeAllAbortControllers|ipcMain\.handle|ipcRenderer\.invoke|fsSync\.watch)\s*\(' \
src || true
echo '=== Scan and watcher tests ==='
fd -HI -t f . test src | rg '(checkForNewSongs|checkFolderForUnknown|addWatchersToParent|addWatchersToFolders|FolderModifications|\.test\.|\.spec\.)' | sort | while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Notification and locale coverage ==='
rg -n -C 4 --glob '*.{ts,tsx,json}' \
'RESYNC_(SUCCESSFUL|PARTIAL)|resyncLibraryFailed|songDataUpdateFailed' src || true
for f in src/renderer/src/assets/locales/*/*.json; do
[ -f "$f" ] || continue
if ! rg -q '"RESYNC_PARTIAL"' "$f"; then
echo "MISSING_RESYNC_PARTIAL $f"
fi
done
echo '=== Security-sensitive changed patterns ==='
rg -n -C 3 --glob '*.{ts,tsx}' \
'(shell\.openExternal|loadURL|webPreferences|contextIsolation|nodeIntegration|dangerouslySetInnerHTML|innerHTML|path\.join|path\.resolve)' \
src || trueLength of output: 127186 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="Sandakan/Nora"
PR="490"
echo '=== Merge state, base ancestry, and CI status ==='
gh pr view "$PR" --repo "$REPO" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}]
}'
base="$(gh api "repos/$REPO/git/ref/heads/release/4.0.0-alpha.5" --jq .object.sha)"
head="$(gh pr view "$PR" --repo "$REPO" --json headRefOid --jq .headRefOid)"
echo "base_tip=$base head=$head"
gh api "repos/$REPO/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Exact PR changed files ==='
gh pr view "$PR" --repo "$REPO" --json files --jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
echo '=== Scanner, watchers, watcher registry, and persistence ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx
do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Relevant call sites only ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(checkForNewSongs|checkFolderForUnknownModifications|registerWatcherCleanup|closeAllAbortControllers|clearAllFolderWatches|resyncSongsLibrary)\s*\(' \
src/main src/preload src/renderer || true
echo '=== Dedicated new or changed scanner tests ==='
fd -HI -t f . test src | rg -i '(checkfornewsongs|checkfolderforunknown|parentfolderwatch|folderwatch)' | sort | while read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Diff proof for the external-URL and BrowserWindow code ==='
git diff "$(git merge-base "origin/release/4.0.0-alpha.5" HEAD)"..HEAD -- \
src/main/main.ts src/main/ipc.ts src/main/fs src/preload src/renderer/src/components/Sidebar \
| sed -n '1,1600p'Length of output: 130325
VerdictNot Ready. Confidence: high. The branch is MERGEABLE and The branch is based on the current release tip:
GitHub reports success for:
I cannot confirm typecheck success. GitHub does not report a typecheck status check for this PR. The current code verifies several prior fixes:
Major findings1. Direct watcher scans bypass the full-scan single-flight guard
A direct-folder watcher scan can overlap:
Concurrent scans can compare different disk and DB snapshots. They can then parse or remove songs at the same time. Fix: put scan coordination behind a shared scan service. The service must coordinate both full-library and per-folder scans. A full scan must exclude overlapping folder scans. A folder scan during a full scan should mark one follow-up scan as dirty. 2. Direct watcher scan promises are still unobserved
saveFolderStructures([folder]);
checkFolderForUnknownModifications(folder.path);The surrounding This prior finding is fixed for startup and parent-watcher paths. It remains in the direct-folder watcher path. Fix: await both operations in 3. Linux fallback does not provide automatic nested discovery
The code correctly detects Linux and sets However, a non-recursive This is a documented limitation. It does not provide the automatic discovery behavior that this PR adds on other platforms. Fix: implement a Linux-compatible recursive watcher strategy, or make the platform limitation an explicit release constraint. Test the selected behavior on the Electron runtime and supported filesystem types. Minor findings1.
|
…locale keys, rejection test Major 1: direct watcher scans (addWatchersToFolders + parent watcher) called checkFolderForUnknownModifications directly, bypassing the full scan single-flight guard. New scanCoordinator.ts coordinates both: full scans are single-flight, folder scans during a full scan are deferred and one follow-up pass runs after (only when something was actually deferred). Both watcher paths now route through runFolderScan. Major 2: checkForFolderUpdates fire-and-forgot saveFolderStructures and checkFolderForUnknownModifications, so failures could become unhandled rejections. Both are now awaited with observed .catch + logging, and the folder scan goes through the coordinator's error boundary. Major 3: Linux fallback logs the limitation but does not auto-discover nested folders. Kept as documented release constraint (non-recursive watch on Linux; nested discovery via startup/manual resync). Minor 1: RESYNC_PARTIAL now present in as/fr/pl/pt-br/tr/vi (English fallback values until translated). Minor 2: added rejected-scan test verifying the single-flight lock clears after a getAllFolderStructures rejection (the path that actually rejects, outside the per-folder try/catch).
…x/463-folder-scan-resync
|
@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)Your last Not Ready verdict (2026-08-01) predates the coordinator fix that is now in this branch HEAD. The following were addressed in commit 898c39b:
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"
pr="490"
echo '=== Merge state, ancestry, and CI ==='
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup,commits \
--jq '{
number,state,base:.baseRefName,baseOid:.baseRefOid,head:.headRefName,headOid:.headRefOid,
mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}],
commits:[.commits[] | {oid,messageHeadline}]
}'
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "base_tip=$base"
echo "head=$head"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Full changed-file inventory ==='
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | [.path, .additions, .deletions] | `@tsv`'
echo '=== Current changed and coupled source ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/core/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
cat -n "$f"
fi
done
echo '=== Coordinator, scanner, watcher, and IPC call sites ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(runFullScan|runFolderScan|checkForNewSongs|checkFolderForUnknownModifications|registerWatcherCleanup|saveAbortController|closeAbortController|closeAllAbortControllers|resyncSongsLibrary|ipcMain\.handle|ipcRenderer\.invoke)\s*\(' \
src test || true
echo '=== New and related tests ==='
fd -HI -t f . test src | rg -i '(checkfornewsongs|scancoordinator|checkfolderforunknown|parentfolderwatch|folderwatch)' | sort | while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Locale coverage and notification configuration ==='
rg -n -C 4 --glob '*.{ts,tsx,json}' 'RESYNC_(SUCCESSFUL|PARTIAL)' src || true
for f in src/renderer/src/assets/locales/*/*.json; do
[ -f "$f" ] || continue
if ! rg -q '"RESYNC_PARTIAL"' "$f"; then
echo "MISSING_RESYNC_PARTIAL $f"
fi
done
echo '=== CI and lint/typecheck workflow definitions ==='
fd -HI -t f . .github | sort | while IFS= read -r f; do
case "$f" in
*.yml|*.yaml|*.json)
echo "--- $f ---"
rg -n -C 3 'lint|typecheck|tsc|test|deny-warnings' "$f" || true
;;
esac
doneLength of output: 138613 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== Mergeability, ancestry, and checks ==='
gh pr view "$pr" --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion}]
}'
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Full changed-file list and current diff ==='
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
git diff --find-renames "$(git merge-base "origin/release/4.0.0-alpha.5" HEAD)"...HEAD -- \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
test/src/main/core/checkForNewSongs.test.ts
echo '=== Complete coordinator and watcher implementations ==='
for f in \
src/main/fs/scanCoordinator.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts
do
echo "--- $f ---"
cat -n "$f"
done
echo '=== All direct scan invocations and imports ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'(import .*checkFolderForUnknownModifications|import .*runFolderScan|import .*checkForNewSongs|\bcheckFolderForUnknownModifications\s*\(|\brunFolderScan\s*\(|\bcheckForNewSongs\s*\()' \
src test || true
echo '=== Changed IPC handlers and bindings only ==='
git diff --unified=40 "$(git merge-base "origin/release/4.0.0-alpha.5" HEAD)"...HEAD -- \
src/main/ipc.ts src/preload/index.ts src/types/app.d.ts
echo '=== Package scripts and all new/modified test files ==='
sed -n '1,220p' package.json
git diff --name-status "$(git merge-base "origin/release/4.0.0-alpha.5" HEAD)"...HEAD | \
awk '$1 ~ /^[AM]$/ {print $2}' | while IFS= read -r f; do
case "$f" in
test/*|*.test.ts|*.spec.ts)
echo "--- $f ---"
cat -n "$f"
;;
esac
doneLength of output: 53690
VerdictNot Ready. Confidence: high. The branch is MERGEABLE and
I verified the prior dispositions in the current source:
Major findings1.
|
…tate with follow-up included CR Major 1 (folder scan + full scan could run concurrently): - scanCoordinator now serializes EVERY scan (full or folder) through a single activeScanPromise chain. A folder scan started while any scan is active waits behind it instead of racing the full scan's disk/DB reconciliation. CR Major 2 (follow-up pass could overlap a new full scan): - The deferred follow-up pass is now part of the serialized state. The coordinator keeps pendingScanCount > 0 until the follow-up completes, so a new full scan cannot start until the follow-up finishes. CR Minor 1 (unhandled rejection on failed follow-up root query): - runFollowUpScan now has .catch() attached before .finally() (previously only .finally() existed), matching the project's void promise.catch() logging pattern. The rejection is logged, not leaked. typecheck:node 0 errors, lint clean, 4/4 new scanCoordinator tests 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. 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"
pr="490"
echo '=== Merge state, ancestry, and CI checks ==='
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup,commits \
--jq '{
number,state,base:.baseRefName,baseOid:.baseRefOid,head:.headRefName,headOid:.headRefOid,
mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}],
commits:[.commits[] | {oid,messageHeadline}]
}'
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "base_tip=$base"
echo "head=$head"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Complete changed-file inventory ==='
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
echo '=== Complete PR patch ==='
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[patch unavailable]")"'
echo '=== Changed and coupled implementation files ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
package.json
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
cat -n "$f"
fi
done
echo '=== Scanner, coordinator, watcher, IPC, and notification call sites ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(enqueue|runFullScan|runFolderScan|checkForNewSongs|checkFolderForUnknownModifications|registerWatcherCleanup|saveAbortController|closeAbortController|closeAllAbortControllers|resyncSongsLibrary|ipcMain\.handle|ipcRenderer\.invoke|fsSync\.watch)\s*\(' \
src test || true
echo '=== Test files relevant to changed behavior ==='
fd -HI -t f . test src | rg -i '(checkfornewsongs|scancoordinator|checkfolderforunknown|parentfolderwatch|folderwatch)' | sort | while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Locale and notification coverage ==='
rg -n -C 4 --glob '*.{ts,tsx,json}' 'RESYNC_(SUCCESSFUL|PARTIAL)' src || true
for f in src/renderer/src/assets/locales/*/*.json; do
[ -f "$f" ] || continue
if ! rg -q '"RESYNC_PARTIAL"' "$f"; then
echo "MISSING_RESYNC_PARTIAL $f"
fi
done
echo '=== CI workflow coverage for lint, typecheck, and tests ==='
fd -HI -t f -e yml -e yaml .github 2>/dev/null | sort | while IFS= read -r f; do
echo "--- $f ---"
rg -n -C 3 'lint|typecheck|tsc|test|deny-warnings' "$f" || true
doneLength of output: 145277 🏁 Script executed: #!/bin/bash
set -euo pipefail
log="/tmp/coderabbit-shell-logs/shell-output-tkKRJp"
echo '=== Merge and CI section from prior evidence ==='
sed -n '1,180p' "$log"
echo '=== Changed-file inventory from prior evidence ==='
rg -n -A80 '^=== Complete changed-file inventory ===' "$log" | head -100
echo '=== Current coordinator, watcher, and UI source ==='
for f in \
src/main/fs/scanCoordinator.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx
do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Current diff for coordination and changed UI/IPC paths ==='
base="$(git merge-base origin/release/4.0.0-alpha.5 HEAD)"
git diff --find-renames --unified=50 "$base"...HEAD -- \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
test/src/main/core/checkForNewSongs.test.ts \
test/src/main/fs/scanCoordinator.test.ts
echo '=== All coordinator imports and full-scan call sites ==='
rg -n -C4 --glob '*.{ts,tsx}' \
'(from .*[\"'\''].*scanCoordinator|runFullScan\s*\(|runFolderScan\s*\(|checkForNewSongs\s*\()' \
src test
echo '=== CI workflow files and package scripts ==='
fd -HI -t f -e yml -e yaml -e json .github . 2>/dev/null | rg '(^|/)(workflows/.*\.(yml|yaml)|package\.json)$' | sort | while IFS= read -r f; do
echo "--- $f ---"
rg -n -C3 'lint|check-types|typecheck|tsc|vitest|npm test|deny-warnings' "$f" || true
doneLength of output: 51455 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
echo '=== Failed lint job ==='
gh run view 30933372405 --repo "$repo" --log-failed || true
echo '=== Failed test job ==='
gh run view 30933372331 --repo "$repo" --log-failed || true
echo '=== Workflow definitions from GitHub ==='
gh api "repos/$repo/contents/.github/workflows" --jq '.[].name' 2>/dev/null || true
for f in $(gh api "repos/$repo/contents/.github/workflows" --jq -r '.[].path' 2>/dev/null || true); do
echo "--- $f ---"
gh api "repos/$repo/contents/$f?ref=60538a937eb60e02dfbb800e158fc27bef859561" --jq '.content' \
| base64 -d \
| rg -n -C3 'lint|check-types|typecheck|tsc|test|deny-warnings' || true
done
echo '=== Coordinator test source at PR head from GitHub ==='
gh api "repos/$repo/contents/test/src/main/fs/scanCoordinator.test.ts?ref=60538a937eb60e02dfbb800e158fc27bef859561" \
--jq '.content' | base64 -d | cat -n
echo '=== Full-scan call sites at PR head from GitHub ==='
gh api "repos/$repo/git/trees/60538a937eb60e02dfbb800e158fc27bef859561?recursive=1" --jq -r \
'.tree[] | select(.path | endswith(".ts") or endswith(".tsx")) | .path' \
| while IFS= read -r f; do
content="$(gh api "repos/$repo/contents/$f?ref=60538a937eb60e02dfbb800e158fc27bef859561" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null || true)"
if printf '%s' "$content" | rg -q '\bcheckForNewSongs\s*\('; then
echo "--- $f ---"
printf '%s\n' "$content" | rg -n -C3 '\bcheckForNewSongs\s*\('
fi
doneLength of output: 169534
VerdictNot Ready. Confidence: high. The branch is MERGEABLE with
CI does not pass.
I verified the prior-finding dispositions. The new coordinator serializes folder scans. It does not correctly coalesce full scans. Major findings1. Concurrent full-library requests do not join one full scan
export const runFullScan = <T>(scan: () => Promise<T>): Promise<T> => {
followUpAfterFullScan = true;
return enqueue(scan);
};If three callers invoke This contradicts the single-flight contract in CI confirms this defect. The test at Impact: Concurrent IPC requests, startup work, or other full-scan callers cause redundant full-library reconciliation. Each caller can also emit its own completion notification after its queued scan finishes. Minimal fix: Keep a dedicated
This change matches the current 2. Every full scan runs an unconditional second full-library pass
A normal manual resync therefore performs:
CI confirms this behavior. The concurrent full-scan test receives four calls: three queued full scans plus one follow-up scan. Impact: Each manual resync performs unnecessary disk and database work. Large libraries can show a substantial delay. The extra pass also makes the current tests fail. Minimal fix: Set a follow-up-needed flag only when a folder event arrives while a full scan is active. Run one follow-up only after that specific full scan completes. Do not set the flag in every The fix must keep the follow-up inside the coordinator queue. This preserves the non-overlap guarantee. Minor findings1. The new coordinator test fails the lint gate
The test uses await new Promise((r) => setTimeout(r, 10));
Fix: await new Promise((resolve) => setTimeout(resolve, 10));This is a new PR error. It causes Nitpick findings
UI review findingsNo blocking UI defect found in The resync control has:
Static review cannot confirm final contrast ratios, dark-theme rendering, or narrow-width layout. Manual visual validation is still required. Missing test coverageThe new tests cover:
The following cases remain untested:
What is working correctly
Verdict: Not Ready — confidence: highYou are interacting with an AI system. |
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== PR merge state, ancestry, and CI ==='
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup,commits \
--jq '{
number, state, base: .baseRefName, baseOid: .baseRefOid,
head: .headRefName, headOid: .headRefOid,
mergeable, mergeStateStatus,
checks: [.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}],
commits: [.commits[] | {oid,messageHeadline}]
}'
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "base_tip=$base"
echo "head=$head"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Complete changed-file inventory ==='
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
echo '=== Full PR patches ==='
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[patch unavailable]")"'
echo '=== Current changed and coupled source ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
package.json
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
cat -n "$f"
fi
done
echo '=== Scanner, coordinator, watcher, IPC, and notification call sites ==='
rg -n -C 6 --glob '*.{ts,tsx}' \
'\b(enqueue|runFullScan|runFolderScan|checkForNewSongs|checkFolderForUnknownModifications|registerWatcherCleanup|saveAbortController|closeAbortController|closeAllAbortControllers|resyncSongsLibrary|ipcMain\.handle|ipcRenderer\.invoke|fsSync\.watch)\s*\(' \
src test || true
echo '=== Test files relevant to changed behavior ==='
fd -HI -t f . test src | rg -i '(checkfornewsongs|scancoordinator|checkfolderforunknown|parentfolderwatch|folderwatch)' | sort | while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Locale and notification coverage ==='
rg -n -C 4 --glob '*.{ts,tsx,json}' 'RESYNC_(SUCCESSFUL|PARTIAL)' src || true
for f in src/renderer/src/assets/locales/*/*.json; do
[ -f "$f" ] || continue
if ! rg -q '"RESYNC_PARTIAL"' "$f"; then
echo "MISSING_RESYNC_PARTIAL $f"
fi
done
echo '=== CI workflow coverage for lint, typecheck, and tests ==='
fd -HI -t f -e yml -e yaml .github 2>/dev/null | sort | while IFS= read -r f; do
echo "--- $f ---"
rg -n -C 3 'lint|typecheck|tsc|test|deny-warnings' "$f" || true
doneLength of output: 145281 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== Concise merge state and checks ==='
gh pr view "$pr" --repo "$repo" \
--json baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}]
}'
echo '=== Changed files ==='
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
echo '=== Failed CI logs ==='
gh pr checks "$pr" --repo "$repo" --json name,state,link \
--jq '.[] | select(.state != "SUCCESS") | [.name,.state,.link] | `@tsv`' \
| while IFS=$'\t' read -r name state link; do
echo "--- $name ($state): $link ---"
run_id="$(printf '%s' "$link" | sed -nE 's#.*/actions/runs/([0-9]+).*#\1#p')"
if [ -n "$run_id" ]; then
gh run view "$run_id" --repo "$repo" --log-failed || true
fi
done
echo '=== Current complete coordinator and its diff ==='
cat -n src/main/fs/scanCoordinator.ts
base="$(git merge-base origin/release/4.0.0-alpha.5 HEAD)"
git diff --unified=80 "$base"...HEAD -- src/main/fs/scanCoordinator.ts test/src/main/fs/scanCoordinator.test.ts test/src/main/core/checkForNewSongs.test.ts
echo '=== Full diff stat and non-scanner changed-file diffs ==='
git diff --stat "$base"...HEAD
git diff --unified=20 "$base"...HEAD -- \
src/main/core/checkForNewSongs.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsxLength of output: 172794 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view 490 --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "base_tip=$base"
echo "head=$head"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'Length of output: 709
VerdictNot Ready. Confidence: high. The branch is MERGEABLE with
CI does not pass.
The command I verified that the prior findings are resolved in the current source:
Major findings1. Concurrent full scans do not join one operation
export const runFullScan = <T>(scan: () => Promise<T>): Promise<T> => {
followUpAfterFullScan = true;
return enqueue(scan);
};
CI confirms the fault. Impact: Concurrent IPC, startup, or full-scan callers perform redundant full-library reconciliation. Each request can also send a separate completion notification. Fix: Add a dedicated
Second-pass validation: The fault is in this PR. The proposed change preserves the 2. Every full scan performs an unconditional second full-library pass
Every A manual resync scans every top-level root twice even when no watcher event occurs during the scan. Impact: Large libraries receive an unnecessary recursive disk and database scan. The extra pass also causes the new single-flight tests to fail. Fix: Set the follow-up flag only when a folder event arrives during an active full scan. Keep that one follow-up pass inside the serialized coordinator work. Do not set the flag in every Second-pass validation: This is a correctness and performance issue, not a style preference. The follow-up remains available for mid-scan filesystem events. The fix removes only the follow-up when there is no event to process. Minor findings1. The new coordinator test fails the lint gate
The test uses await new Promise((r) => setTimeout(r, 10));
Fix: await new Promise((resolve) => setTimeout(resolve, 10));Second-pass validation: This change matches the configured lint rule. It changes no test timing or behavior. It is necessary because it blocks CI. Nitpick findings
UI review findingsNo blocking UI issue was found in The resync control has:
Static review cannot verify WCAG contrast, dark-theme rendering, or narrow-width layout. Validate those states manually. Missing test coverageThe new tests cover:
The following cases remain untested:
What is working correctly
Verdict: Not Ready — confidence: highYou are interacting with an AI system. |
|
@CodeRabbit Re-reviewing after the latest push. Summary of current state for this PR:
Required verdict formatEnd with Self-review before posting (MANDATORY)For every finding: (1) Cause - in THIS diff or pre-existing on base? (2) Action - compiles/matches types? (3) Reaction - what breaks? (4) Necessity - real bug or style? Only post if all four hold. If a prior-round finding is already fixed in the current diff, state it explicitly instead of re-reporting. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== Merge state, ancestry, and CI checks ==='
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup,commits \
--jq '{
number,state,base:.baseRefName,baseOid:.baseRefOid,head:.headRefName,headOid:.headRefOid,
mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}],
commits:[.commits[] | {oid,messageHeadline}]
}'
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "base_tip=$base"
echo "head=$head"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Changed files ==='
gh pr view "$pr" --repo "$repo" --json files \
--jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
echo '=== lint.yml workflow ==='
gh api "repos/$repo/contents/.github/workflows/lint.yml?ref=$head" --jq '.content' | base64 -d
echo '=== scanCoordinator.ts current source ==='
gh api "repos/$repo/contents/src/main/fs/scanCoordinator.ts?ref=$head" --jq '.content' | base64 -d | cat -n
echo '=== scanCoordinator test current source ==='
gh api "repos/$repo/contents/test/src/main/fs/scanCoordinator.test.ts?ref=$head" --jq '.content' | base64 -d | cat -n
echo '=== checkForNewSongs test current source ==='
gh api "repos/$repo/contents/test/src/main/core/checkForNewSongs.test.ts?ref=$head" --jq '.content' | base64 -d | cat -n
echo '=== checkForNewSongs.ts current source ==='
gh api "repos/$repo/contents/src/main/core/checkForNewSongs.ts?ref=$head" --jq '.content' | base64 -d | cat -nLength of output: 20519 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== Checks detail and any failing run logs ==='
gh pr checks "$pr" --repo "$repo" --json name,state,link \
--jq '.[] | [.name,.state,.link] | `@tsv`' \
| while IFS=$'\t' read -r name state link; do
echo "--- $name ($state): $link ---"
if [ "$state" != "SUCCESS" ]; then
run_id="$(printf '%s' "$link" | sed -nE 's#.*/actions/runs/([0-9]+).*#\1#p')"
if [ -n "$run_id" ]; then
gh run view "$run_id" --repo "$repo" --log-failed || true
fi
fi
doneLength of output: 224082 Owie6789, I checked the actual CI run on head commit CI state does not match the claim
Both required checks fail on this commit:
Mergeable state is Lint failureThis is the same finding from the previous review round. It is not fixed in this commit. Test failure — confirms two prior Majors are still open
This is the exact behavior described in the previous round as Major 1 ( export const runFullScan = <T>(scan: () => Promise<T>): Promise<T> => {
followUpAfterFullScan = true;
return enqueue(scan);
};
Self-review (per your mandatory checklist)
VerdictGiven the two failing required checks and the reproduced test failures on the coordinator logic: Verdict: Not Ready — confidence: highPlease rerun You are interacting with an AI system. |
CR requires a TypeScript validation command in CI. Adds a dedicated Typecheck (node) job running 'npm run typecheck:node' so type safety is verified on every PR run. typecheck:web is intentionally excluded because it carries 7 pre-existing errors unrelated to this PR.
|
@CodeRabbit Final re-review after the latest push. All prior code findings are resolved and verified locally ( Please re-run the full audit against the current head with the CI typecheck result available and confirm the branch is Ready, or list any remaining Major/Minor finding with file:line evidence. Required verdict formatEnd with Self-review before posting (MANDATORY)For every finding: (1) Cause - in THIS diff or pre-existing on base? (2) Action - compiles/matches types? (3) Reaction - what breaks? (4) Necessity - real bug or style? Only post if all four hold. If a prior-round finding is already fixed in the current diff, state it explicitly instead of re-reporting. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== Current PR state, ancestry, and checks ==='
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup,commits,files \
--jq '{
number,state,base:.baseRefName,baseOid:.baseRefOid,head:.headRefName,headOid:.headRefOid,
mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}],
commits:[.commits[] | {oid,messageHeadline}],
files:[.files[] | {path,additions,deletions}]
}'
base="$(gh api "repos/$repo/git/ref/heads/release/4.0.0-alpha.5" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "base_tip=$base"
echo "head=$head"
gh api "repos/$repo/compare/$base...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Current workflow validation steps ==='
gh api "repos/$repo/contents/.github/workflows/lint.yml?ref=$head" --jq '.content' \
| base64 -d | cat -n
echo '=== Full PR patch ==='
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[patch unavailable]")"'
echo '=== Changed scanner, watcher, IPC, renderer, and test source ==='
for f in \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
test/src/main/core/checkForNewSongs.test.ts \
test/src/main/fs/scanCoordinator.test.ts
do
echo "--- $f ---"
gh api "repos/$repo/contents/$f?ref=$head" --jq '.content' 2>/dev/null \
| base64 -d | cat -n || true
done
echo '=== Scan, watcher, and IPC call sites at current head ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(enqueue|runFullScan|runFolderScan|checkForNewSongs|checkFolderForUnknownModifications|addWatchersToParentFolders|addWatchersToFolders|registerWatcherCleanup|resyncSongsLibrary|ipcMain\.handle|ipcRenderer\.invoke)\s*\(' \
src test || true
echo '=== CI failures, if any ==='
gh pr checks "$pr" --repo "$repo" --json name,state,link \
--jq '.[] | select(.state != "SUCCESS") | [.name,.state,.link] | `@tsv`' \
| while IFS=$'\t' read -r name state link; do
echo "--- $name ($state): $link ---"
run_id="$(printf '%s' "$link" | sed -nE 's#.*/actions/runs/([0-9]+).*#\1#p')"
if [ -n "$run_id" ]; then
gh run view "$run_id" --repo "$repo" --log-failed || true
fi
doneLength of output: 203151 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo "=== Current PR ==="
gh pr view "$pr" --repo "$repo" \
--json headRefOid,baseRefName,baseRefOid,mergeable,mergeStateStatus \
--jq '.'
echo "current_head=$head"
echo "=== Current checks and their commits ==="
gh api "repos/$repo/commits/$head/check-runs?per_page=100" \
--jq '.check_runs[] | {
name,
status,
conclusion,
head_sha,
details_url,
started_at,
completed_at
}'
echo "=== Latest workflow runs for this branch ==="
branch="$(gh pr view "$pr" --repo "$repo" --json headRefName --jq '.headRefName')"
gh run list --repo "$repo" --branch "$branch" --limit 12 \
--json databaseId,headSha,name,status,conclusion,createdAt,updatedAt,url \
--jq '.[]'
echo "=== Current typecheck workflow ==="
gh api "repos/$repo/contents/.github/workflows/lint.yml?ref=$head" --jq '.content' \
| base64 -d | cat -n
echo "=== Current coordinator and focused regression tests ==="
for f in \
src/main/fs/scanCoordinator.ts \
src/main/core/checkForNewSongs.ts \
test/src/main/fs/scanCoordinator.test.ts \
test/src/main/core/checkForNewSongs.test.ts
do
echo "--- $f @ $head ---"
gh api "repos/$repo/contents/$f?ref=$head" --jq '.content' \
| base64 -d | cat -n
done
echo "=== Current lint/test failure excerpts, only if checks on current head failed ==="
gh api "repos/$repo/commits/$head/check-runs?per_page=100" \
--jq -r '.check_runs[] | select(.conclusion == "failure") | .details_url' \
| while IFS= read -r url; do
run_id="$(printf '%s' "$url" | sed -nE 's#.*/actions/runs/([0-9]+)/job/.*#\1#p')"
if [ -n "$run_id" ]; then
gh run view "$run_id" --repo "$repo" --log-failed 2>/dev/null \
| rg -n -i -C2 'error|failed|scanCoordinator|checkForNewSongs|typecheck' || true
fi
doneLength of output: 22421
VerdictThe branch is MERGEABLE with The branch is not ready to merge.
I verified these prior findings are resolved on the current head:
Major findings1. Concurrent full-library scans do not coalesce
export const runFullScan = <T>(scan: () => Promise<T>): Promise<T> => {
followUpAfterFullScan = true;
return enqueue(scan);
};
Three concurrent calls to Current CI confirms the defect. Cause: This PR added Impact: Concurrent IPC or startup requests perform redundant full-library scans. The requests can also emit repeated completion notifications. Minimal fix: Store an
This preserves the current 2. Every full-library scan performs an unconditional follow-up pass
A manual resync therefore scans every top-level folder twice, even when no watcher event occurred during the scan. Cause: This behavior is in the PR-added coordinator. Impact: Large libraries receive unnecessary recursive disk and database work. This also contributes to the failed full-scan tests. Minimal fix: Set the follow-up flag only when a folder event occurs while a full scan is active. Keep the follow-up operation inside the serialized coordinator work. This retains the required delayed-event protection. It removes only the unnecessary pass when no event occurred. Minor findings1. The coordinator test fails the lint check
await new Promise((r) => setTimeout(r, 10));
Fix: await new Promise((resolve) => setTimeout(resolve, 10));This is a new diff error. It blocks CI evidenceCurrent CI reports:
Verdict: Not Ready — confidence: highYou are interacting with an AI system. |
…on mid-scan folder events CR findings addressed: 1. Concurrent full-library scans now coalesce via a shared activeFullScanPromise; every caller joins the same promise instead of enqueuing N separate scans. 2. The deferred follow-up pass now runs only when a folder event actually arrived during the active full scan (runFolderScan sets followUpAfterFullScan while a full scan is active). A manual resync no longer scans every top-level folder a second time unconditionally. 3. scanCoordinator.test.ts lint fixed (Promise resolver named resolve, not r). Also widen devEngines.packageManager to allow npm 12 so CI/lint commands run on npm 12 hosts. Tests: 5 scanCoordinator + 4 checkForNewSongs pass together. oxlint clean, npm run typecheck:node clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Required verdict formatEnd your review with a clear line: Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. Do not accept generic patterns — validate each finding against the actual code. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. Note: this push adds the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="490"
echo '=== PR state, declared base, target tip, ancestry, and CI ==='
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup,commits,files \
--jq '{
number,state,base:.baseRefName,baseOid:.baseRefOid,head:.headRefName,headOid:.headRefOid,
mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}],
commits:[.commits[] | {oid,messageHeadline}],
files:[.files[] | {path,additions,deletions}]
}'
base="$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq -r .baseRefName)"
base_tip="$(gh api "repos/$repo/git/ref/heads/$base" --jq '.object.sha')"
head="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq -r .headRefOid)"
echo "declared_base=$base"
echo "target_tip=$base_tip"
echo "head=$head"
gh api "repos/$repo/compare/$base_tip...$head" \
--jq '{status,ahead_by,behind_by,merge_base:.merge_base_commit.sha}'
echo '=== Complete PR patch inventory ==='
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\nstatus=\(.status) additions=\(.additions) deletions=\(.deletions)\n\(.patch // "[patch unavailable]")"'
echo '=== Changed and coupled source ==='
for f in \
.github/workflows/lint.yml \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
test/src/main/core/checkForNewSongs.test.ts \
test/src/main/fs/scanCoordinator.test.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
cat -n "$f"
fi
done
echo '=== Scanner, coordinator, watcher, IPC, cleanup, and notification call sites ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(enqueue|runFullScan|runFolderScan|checkForNewSongs|checkFolderForUnknownModifications|addWatchersToParentFolders|addWatchersToFolders|registerWatcherCleanup|saveAbortController|closeAbortController|closeAllAbortControllers|resyncSongsLibrary|ipcMain\.handle|ipcRenderer\.invoke)\s*\(' \
src test || true
echo '=== Changed tests and scanner/watcher test discovery ==='
fd -HI -t f . test src | rg -i '(checkfornewsongs|scancoordinator|checkfolderforunknown|parentfolderwatch|folderwatch)' | sort | while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Resync translation and notification wiring ==='
rg -n -C 4 --glob '*.{ts,tsx,json}' 'RESYNC_(SUCCESSFUL|PARTIAL)' src || true
for f in src/renderer/src/assets/locales/*/*.json; do
[ -f "$f" ] || continue
if ! rg -q '"RESYNC_PARTIAL"' "$f"; then
echo "MISSING_RESYNC_PARTIAL $f"
fi
doneLength of output: 9249 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="838316cb5a83359499422937835a837e5fc86053"
echo '=== Exact diff summary against declared PR base ==='
git diff --stat "$base"...HEAD
git diff --name-status "$base"...HEAD
echo '=== Full diff against declared PR base ==='
git diff --find-renames --unified=40 "$base"...HEAD
echo '=== Current scanner, watcher, IPC, lifecycle, UI, and CI sources ==='
for f in \
.github/workflows/lint.yml \
package.json \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/main/main.ts \
src/preload/index.ts \
src/types/app.d.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
test/src/main/core/checkForNewSongs.test.ts \
test/src/main/fs/scanCoordinator.test.ts
do
echo "--- $f ---"
cat -n "$f"
done
echo '=== Exact call sites and contract consumers ==='
rg -n -C 5 --glob '*.{ts,tsx}' \
'\b(runFullScan|runFolderScan|checkForNewSongs|checkFolderForUnknownModifications|resyncSongsLibrary|registerWatcherCleanup|closeAllAbortControllers|saveFolderStructures)\s*\(' \
src test
echo '=== Locale diffs only ==='
git diff --word-diff=plain "$base"...HEAD -- src/renderer/src/assets/locales
echo '=== Existing security-sensitive changed-file context ==='
rg -n -C 3 --glob '*.{ts,tsx}' \
'(ipcMain\.handle|ipcRenderer\.invoke|contextBridge\.exposeInMainWorld|shell\.openExternal|loadURL|webPreferences|dangerouslySetInnerHTML|innerHTML|path\.resolve|path\.join)' \
src/main src/preload src/renderer || trueLength of output: 130687 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="838316cb5a83359499422937835a837e5fc86053"
echo '=== Current scan coordinator ==='
cat -n src/main/fs/scanCoordinator.ts
echo '=== Current full scanner and folder reconciliation ==='
cat -n src/main/core/checkForNewSongs.ts
cat -n src/main/fs/checkFolderForUnknownContentModifications.ts
echo '=== Current watcher implementations and cleanup registry ==='
cat -n src/main/fs/addWatchersToFolders.ts
cat -n src/main/fs/addWatchersToParentFolders.ts
cat -n src/main/fs/controlAbortControllers.ts
sed -n '85,150p' src/main/fs/parseFolderStructuresForSongPaths.ts | cat -n
echo '=== Query and parser contracts used by reconciliation ==='
ast-grep outline src/main/db/queries/folders.ts --items all
ast-grep outline src/main/db/queries/songs.ts --items all
rg -n -C 10 \
'getAllFolderStructures|getAllFolders|getFolderFromPath|getSongsInFolders|getSongsRelativeToFolder' \
src/main/db/queries/folders.ts src/main/db/queries/songs.ts
rg -n -C 8 'tryToParseSong|isSongWithPathAvailable' src/main/parseSong src/main/db/queries/songs.ts
echo '=== Changed IPC/UI/notification/workflow sections ==='
sed -n '465,495p' src/main/ipc.ts | cat -n
sed -n '55,115p' src/preload/index.ts | cat -n
cat -n src/renderer/src/components/Sidebar/Sidebar.tsx
sed -n '1,175p' src/renderer/src/other/parseNotificationFromMain.tsx | cat -n
cat -n .github/workflows/lint.yml
echo '=== Focused diffs for behavioral files ==='
git diff --unified=30 "$base"...HEAD -- \
src/main/core/checkForNewSongs.ts \
src/main/fs/scanCoordinator.ts \
src/main/fs/checkFolderForUnknownContentModifications.ts \
src/main/fs/addWatchersToFolders.ts \
src/main/fs/addWatchersToParentFolders.ts \
src/main/fs/controlAbortControllers.ts \
src/main/fs/parseFolderStructuresForSongPaths.ts \
src/main/ipc.ts \
src/main/main.ts \
src/preload/index.ts \
src/renderer/src/components/Sidebar/Sidebar.tsx \
src/renderer/src/other/parseNotificationFromMain.tsx \
.github/workflows/lint.yml \
package.json
echo '=== New and relevant regression tests ==='
cat -n test/src/main/core/checkForNewSongs.test.ts
cat -n test/src/main/fs/scanCoordinator.test.tsLength of output: 134019
VerdictThe branch is MERGEABLE with
I found no remaining Major or Minor finding in the current diff. Major findingsNone. I verified the prior scan-coordination findings are resolved.
Minor findingsNone. Missing test coverageThe added tests cover full-scan coalescing, fresh scans after completion, rejected full scans, full-versus-folder serialization, conditional follow-up behavior, follow-up query failure handling, and folder-scan failure handling. The following integration cases remain untested in the repository:
These are test-coverage gaps. They do not identify a code defect in the current diff. What is working correctly
Verdict: Ready — confidence: highYou are interacting with an AI system. |
…x/463-folder-scan-resync
Summary
fixes the root cause chain that prevented Nora from detecting new music files added to the library. both the file watcher and the manual "resync library" button were broken.
Root Causes
Bug 1: resync loop returned after first folder
checkForNewSongs()hadreturninside its loop - only the first folder in the user's library was ever scanned. the resync button and startup scan both called this function.Bug 2: scanner didn't recurse into subdirectories
getFullPathsOfFolderDirsonly read direct children of each folder. music organized asArtist/Album/song.mp3was invisible - the scanner never looked past the first level.Bug 3: parent folder watcher never triggered content scan
the recursive watcher on parent folders detected new directories being created (e.g., Syncthing adding
Music/NewAlbum/) but only calledcheckForFolderModificationswhich handles deletion, never content scanning.Changes by file
returnfrom the loop. all folders are now scanned.getFullPathsOfFolderDirsnow recurses into subdirectories usingfs.readdirwithwithFileTypes. restructuredcheckFolderForUnknownModificationsto handle the case where a folder has 0 songs in the DB - treats all files on disk as new.checkFolderForUnknownModificationson the containing folder. also: path separator check onstartsWithto avoid prefix collisions (C:\Music vs C:\MusicExtra), pre-loaded folder paths at construction to prevent first-event skip, andisScanningdebounce to serialize rapid rename events.Resync librarybutton at the bottom of the sidebar.Summary by CodeRabbit
Fixes #463