@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

• Walk every changed file end-to-end. For each function/hook/component, describe what it does, what it depends on, and what can break it.
• Verify the diff actually implements the PR title and the linked issue. Flag scope creep and unrelated drive-by edits.
• Check that all new state, events, IPC channels, persistence keys, and settings keys are wired through the full stack (UI → context/store → IPC → main process → disk).
• Confirm the linked issue's acceptance criteria are met. If a criterion is not met, say so explicitly.

2. UI audit (only if the PR touches UI)

• Render correctness: layout, alignment, overflow, clipping, hit targets, focus rings, dark/light theme, HiDPI.
• Accessibility: keyboard nav, ARIA roles/labels, contrast, motion-reduce, screen reader announcements for state changes.
• Interaction states: default / hover / focus / active / disabled / loading / error / empty. Any state missing is a bug.
• Performance: unnecessary re-renders, missing memoization on heavy lists, large images not resized, layout thrash, debounce/throttle on scroll/resize.
• Empty/zero/error states: what does the user see when there is no data, network is down, permission is denied, or the file is corrupt.
• Cross-platform: Windows, macOS, Linux. Electron-specific quirks (frameless regions, title bar drag, safe-area on multi-monitor with different DPI).

3. Production-readiness verification

• No console.log / debugger / commented-out code left in.
• No hardcoded paths, secrets, dev URLs, or fake TODO placeholders.
• Error handling: every async path has a real catch that surfaces something to the user or to telemetry. No swallowed rejections.
• Migrations / schema changes: if a persisted shape changed, is there a migration that does not lose user data?
• Backwards compatibility with existing user data and settings. If something is intentionally breaking, is it called out in the PR description?
• Memory leaks: listeners / observers / timers / intervals / AbortControllers are cleaned up on unmount.
• Race conditions: concurrent clicks, double-submits, stale closures, optimistic UI rollback.
• IPC contract: channel names match the renderer/main handshake, payloads are validated, and the main process guards against malformed messages.

4. Code smell sniff

• Duplicated logic that should be a shared util / hook / component.
• God components / functions over ~150 lines that should be split.
• Boolean traps, magic numbers, stringly-typed APIs, leaky abstractions.
• useEffect that should be a derived value, event handler, or ref.
• Inline styles or className soup where a token or class would do.
• Inconsistent naming or file/folder structure vs the rest of the repo.
• Overly defensive code, dead code, as any / // @ts-ignore without a written justification.

5. Half-baked implementation detection

• Feature appears to work in the happy path screenshot but lacks:
  • error path, empty path, permission-denied path, network-down path
  • loading / skeleton state
  • persistence (settings do not survive restart)
  • undo / cancel / back navigation
  • a11y (keyboard + screen reader)
  • i18n / RTL consideration if the rest of the app is localized
  • tests (unit + integration) for the new logic
• Looks like a "first commit" version that ships a flag and stops — flag any feature gated behind an env var or if (false) branch.
• "I'll add tests later" code with no test files added in this PR.

6. Required output from you

• Inline review comments on the diff, file by file.
• A short summary at the end listing: ✅ what is solid, ⚠️ what must change before merge, 💡 nice-to-haves.
• For each ⚠️ item: severity (blocker / major / minor), the file:line, and the smallest possible fix direction.
• If you find a blocker in the UI, also call it out in the summary so it is impossible to miss.

Please be thorough. I will be applying fixes from your review in a follow-up pass.

#!/bin/bash
# Get PR details and changed files
gh pr view 496 --repo Sandakan/Nora --json title,body,files,additions,deletions,changedFiles

echo "=== FILES CHANGED ==="
gh pr diff 496 --repo Sandakan/Nora

#!/bin/bash
# Get head SHA and check existing patterns in LyricsSettings for how other settings propagate to store
gh pr view 496 --repo Sandakan/Nora --json headRefOid,headRefName

echo "=== Check how other settings in LyricsSettings.tsx propagate to the store ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/SettingsPage/Settings/LyricsSettings.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d

echo "=== Check appReducer for UPDATE_PREFERENCES action or similar ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/other/appReducer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | grep -n 'showLyricsWithSongInfo\|UPDATE_PREFERENCES\|localStorage\|preferences\|setPreferences' | head -60

echo "=== Check storage.preferences.setPreferences to understand if it triggers store updates ==="
gh api repos/Sandakan/Nora/git/trees/refs/pull/496/head?recursive=1 --jq '.tree[].path' | grep -i 'storage\|preferences' | grep -v node_modules | head -30

#!/bin/bash
# Get the PR head commit SHA for review comments
gh api repos/Sandakan/Nora/pulls/496 --jq '{sha: .head.sha, number: .number}'

#!/bin/bash
echo "=== Check localStorage utility setPreferences ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/utils/localStorage.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | grep -n 'setPreferences\|dispatch\|store\|UPDATE_LOCAL' | head -40

echo "=== Full setPreferences impl ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/utils/localStorage.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | grep -n -A 20 'setPreferences'

echo "=== Check if storage.setPreferences dispatches to store ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/utils/localStorage.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | head -80

#!/bin/bash
# Get accurate line numbers from the PR head for each changed file
echo "=== FullScreenPlayer.tsx line numbers ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | head -120

echo "=== LyricsContainer.tsx line numbers ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | grep -n '' | sed -n '140,200p'

echo "=== LyricsSettings.tsx line numbers (new section) ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/SettingsPage/Settings/LyricsSettings.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | sed -n '65,100p'

echo "=== ipc.ts relevant lines ==="
gh api repos/Sandakan/Nora/contents/src/main/ipc.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | sed -n '84,95p'

#!/bin/bash
# Post comprehensive inline review via GitHub API
gh api repos/Sandakan/Nora/pulls/496/reviews \
  --method POST \
  --field commit_id="b8d731abc27760ac68057a7f45863935f9ff7112" \
  --field event="COMMENT" \
  --field body="## Review Summary

### ✅ What is solid
- **Type safety**: `showLyricsWithSongInfo: boolean` correctly added to `app.d.ts` and `LOCAL_STORAGE_DEFAULT_TEMPLATE` — new users and fresh installs get a safe default.
- **Store persistence**: `storage.preferences.setPreferences` dispatches \`UPDATE_LOCAL_STORAGE_PREFERENCES\` to the Zustand store immediately (confirmed in \`localStorage.ts:221\`), so the setting propagates live without a restart.
- **i18n coverage**: Both description and label keys added to \`en.json\` — consistent with the rest of the settings page.
- **Dead code removal**: \`isArtistBackgroundsEnabled = false\` and the dead conditional around \`isArtistBackgroundsEnabled\` removed — good cleanup.
- **Backwards compatibility**: Default is \`false\`, so existing users see no change.
- **Lyrics toggle hidden in side-by-side mode**: Correctly suppressed when the setting is on.

---

### ⚠️ Must change before merge

| # | Severity | File : Line | Issue |
|---|----------|-------------|-------|
| 1 | **Blocker** | \`FullScreenPlayer.tsx:99\` | \`SeekBarSlider\` is a sibling of the side-by-side wrapper inside a \`flex-row\` container — it gets pushed off-screen (see inline) |
| 2 | **Blocker** | \`LyricsContainer.tsx:177\` | \`!lyrics\` catches \`null\` (initial/loading state), not just \`undefined\` (not found) — causes a false \"No lyrics\" flash on every song load (see inline) |
| 3 | **Major** | \`FullScreenPlayer.tsx:21\` | \`useState(isShowLyricsWithSongInfo)\` captures the preference only at mount — live preference changes while the player is open are ignored (see inline) |
| 4 | **Major** | \`ipc.ts:88-89\` | \`toggleAutoLaunch\` and \`toggleMiniPlayerAlwaysOnTop\` imports are unrelated scope creep — should be a separate PR (see inline) |
| 5 | **Minor** | \`FullScreenPlayer.tsx:55\` | Both branches of the blur ternary are identical: \`blur-[2rem]!\` — dead conditional (see inline) |
| 6 | **Minor** | \`LyricsContainer.tsx:153\` | No \`overflow-auto\` on \`LyricsContainer\` itself in side-by-side mode — only the parent has it; \`h-full\` child prevents scroll propagation (see inline) |

---

### 💡 Nice-to-haves
- **Tests**: No unit/integration tests added for the new layout branch or the new preference key.
- **RTL layout**: \`flex-row\` is not reversed for RTL locales — if the app ever targets RTL languages, the side-by-side order will be wrong. Consider \`flex-row\` + \`rtl:flex-row-reverse\` or using \`dir\` attribute.
- **Prop drilling**: \`isShowLyricsWithSongInfo\` is now threaded into both \`LyricsContainer\` and \`SongInfoContainer\`. These components already read from the store internally (via \`useStore\`), so reading the preference directly inside each child would eliminate the prop entirely.
- **Variable shadowing** in \`LyricsSettings.tsx:74\`: \`const showLyricsWithSongInfo = ...\` inside the \`useEffect\` shadows the outer state variable. This is the same pattern as \`autoTranslateLyrics\` (pre-existing), but worth naming differently (e.g. \`storedValue\`) to avoid confusion.
- **JSX duplication**: \`SongInfoContainer\` and \`LyricsContainer\` are fully duplicated across the two render branches. A single render with conditional class/prop overrides would be cleaner.
- **\`imgPath\` memo** (\`FullScreenPlayer.tsx:39-41\`): \`useMemo\` with a single expression that just returns a property is not meaningful — a plain variable would suffice." \
  --field "comments[][path]=src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx" \
  --field "comments[][line]=99" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**🔴 Blocker — SeekBarSlider rendered outside side-by-side layout, breaks in flex-row mode**

In side-by-side mode the outer container is \`flex flex-row justify-end\`. The \`SeekBarSlider\` is a sibling of the \`<div className=\"flex h-full w-full flex-row overflow-hidden\">\` wrapper, which itself declares \`w-full\`. That leaves zero horizontal space for the seek bar — it either overflows off-screen to the right or is squeezed to zero width.

The normal-mode layout works because the outer container is \`flex-col\`, which stacks children vertically. In \`flex-row\`, the slider needs to live *below* the two-column block, not beside it.

**Minimal fix** — wrap both the side-by-side columns and the seek bar in a shared \`flex-col\` so the slider always sits at the bottom:

\`\`\`tsx
{isShowLyricsWithSongInfo ? (
  <div className=\"flex h-full w-full flex-col\">   {/* ← flex-col wrapper */}
    <div className=\"flex min-h-0 flex-1 flex-row overflow-hidden\">
      <div className=\"flex w-1/2 items-end\">
        <SongInfoContainer ... />
      </div>
      <div className=\"w-1/2 overflow-auto\">
        <LyricsContainer ... />
      </div>
    </div>
    <SeekBarSlider ... />   {/* ← moves here, inside the wrapper */}
  </div>
) : (
  <>
    <LyricsContainer ... />
    <SongInfoContainer ... />
    <SeekBarSlider ... />
  </>
)}
\`\`\`

Also remove or change the outer \`flex-row\` class on line 61 — it is now redundant and misleading." \
  --field "comments[][path]=src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx" \
  --field "comments[][line]=177" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**🔴 Blocker — \`!lyrics\` incorrectly matches the loading/null state, causing a premature \"No lyrics\" flash**

The original code tested \`lyrics === undefined\` to mean \"lyrics were looked up and definitively not found\". \`null\` is the initial state (fetch not yet complete). By changing to \`!lyrics\`, you now show the \"No lyrics available\" error panel while the fetch is still in flight (when \`lyrics\` is \`null\`), then hide it once lyrics arrive — creating a visible flash for every song that has lyrics.

**Fix** — restore the three-way distinction:

\`\`\`tsx
{/* lyrics === null  → still loading, show nothing */}
{/* lyrics === undefined → fetch done, no lyrics found */}
{(isShowLyricsWithSongInfo || isLyricsVisible) && lyrics === undefined && (
  <div className=\"text-font-color-highlight flex h-full w-full flex-col justify-center text-2xl opacity-50\">
    <span className=\"material-icons-round-outlined mb-2 text-5xl\">brightness_alert</span>
    <p>{t('lyricsPage.noLyrics')}</p>
    <p className=\"mt-4 text-base\">{t('lyricsPage.noLyricsDescription')}</p>
  </div>
)}
\`\`\`" \
  --field "comments[][path]=src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx" \
  --field "comments[][line]=21" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**🟠 Major — stale \`isLyricsVisible\` initial value when preference changes while component is mounted**

\`useState(isShowLyricsWithSongInfo)\` captures the preference value *once at mount*. If the user opens Settings in another window and toggles \"Show lyrics with song details\" while \`FullScreenPlayer\` is already rendered, \`isLyricsVisible\` will not update — the player will still behave as if the old setting is active until remount.

Add a sync effect:

\`\`\`tsx
useEffect(() => {
  if (isShowLyricsWithSongInfo) {
    setIsLyricsVisible(true);
  }
}, [isShowLyricsWithSongInfo]);
\`\`\`

This also makes the intent explicit: side-by-side mode always forces lyrics visible." \
  --field "comments[][path]=src/main/ipc.ts" \
  --field "comments[][line]=89" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**🟠 Major — unrelated scope creep: \`toggleAutoLaunch\` and \`toggleMiniPlayerAlwaysOnTop\` imports**

These two imports have nothing to do with the lyrics/song-details side-by-side feature (PR `#496` / issue `#461`). They appear to be drive-by fixes for previously missing imports in \`ipc.ts\`.

- If they were causing a build or runtime error, they deserve their own focused commit/PR with an explanation.
- If they are unused here, they should be removed — unused imports in \`ipc.ts\` can mask dead IPC handlers.

Please move them to a separate PR or add a comment explaining why they are included here." \
  --field "comments[][path]=src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx" \
  --field "comments[][line]=55" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**🟡 Minor — both branches of the blur ternary are identical (dead conditional)**

\`\`\`tsx
className={\`... \${isLyricsVisible ? 'blur-[2rem]!' : 'blur-[2rem]!'}\`}
\`\`\`

Both the truthy and falsy branches produce the exact same class string \`blur-[2rem]!\`. The conditional has no effect. Either this was meant to vary the blur intensity between lyrics-visible and lyrics-hidden states (in which case the second branch is wrong) or it should simply be a static class:

\`\`\`tsx
className=\"... blur-[2rem]!\"
\`\`\`" \
  --field "comments[][path]=src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx" \
  --field "comments[][line]=153" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**🟡 Minor — lyrics content not scrollable in side-by-side mode**

In side-by-side mode the \`LyricsContainer\` div has \`h-full\` but no \`overflow-auto\`. The parent wrapper in \`FullScreenPlayer.tsx\` (\`<div className=\"w-1/2 overflow-auto\">\`) technically has \`overflow-auto\`, but because the child declares \`h-full\` it will try to fill the parent's height and block scroll propagation — long lyrics will be clipped with no scrollbar.

**Fix** — add \`overflow-auto\` to the side-by-side class string and remove it from the parent (or keep both and let the child own its scroll):

\`\`\`tsx
isShowLyricsWithSongInfo
  ? 'relative flex h-full w-full flex-col items-start overflow-auto p-4'
  : 'absolute top-0 ...'
\`\`\`

Also, \`py-4 pr-4 pl-4\` can be written as \`p-4\` (identical result, cleaner)."

@Owie6789 — The GitHub integration doesn't have write access to post inline diff comments on this repo, so I'm delivering the full review here with exact file:line references. All findings are verified against the actual PR head (b8d731a).

Review Summary

✅ What is solid

• Type safety: showLyricsWithSongInfo: boolean correctly added to app.d.ts and LOCAL_STORAGE_DEFAULT_TEMPLATE — safe default for new installs.
• Store persistence: storage.preferences.setPreferences dispatches UPDATE_LOCAL_STORAGE_PREFERENCES to the Zustand store immediately (verified in localStorage.ts:221) — the setting propagates live without a restart. ✔
• i18n coverage: Both showLyricsWithSongInfoDescription and showLyricsWithSongInfo keys added to en.json — consistent with the rest of the settings page.
• Dead code removal: isArtistBackgroundsEnabled = false constant and its dead conditional removed — good housekeeping.
• Backwards compatibility: Default is false, so existing users see no change.
• Lyrics toggle hidden in side-by-side mode: Correctly suppressed via {!isShowLyricsWithSongInfo && <Button ... />}.

⚠️ Must change before merge

🔴 Blocker 1 — SeekBarSlider pushed off-screen in side-by-side mode

FullScreenPlayer.tsx Line 61 + Line 99

In side-by-side mode, the outer wrapper is flex flex-row justify-end (Line 61). The SeekBarSlider (Line 99) is a direct sibling of the inner <div className="flex h-full w-full flex-row overflow-hidden"> wrapper. Since that wrapper is w-full, there is zero horizontal space left for the seek bar — it will overflow or be squashed to nothing.

The normal layout works because flex-col stacks children vertically. In flex-row the slider needs to live below the two-column block. Minimal fix:

{isShowLyricsWithSongInfo ? (
  // ← outer flex-col so SeekBarSlider always sits below the columns
  <div className="flex h-full w-full flex-col">
    <div className="flex min-h-0 flex-1 flex-row overflow-hidden">
      <div className="flex w-1/2 items-end">
        <SongInfoContainer ... isShowLyricsWithSongInfo={isShowLyricsWithSongInfo} />
      </div>
      <div className="w-1/2 overflow-auto">
        <LyricsContainer ... isShowLyricsWithSongInfo={isShowLyricsWithSongInfo} />
      </div>
    </div>
    <SeekBarSlider ... />  {/* ← moved inside the wrapper */}
  </div>
) : (
  <>
    <LyricsContainer ... />
    <SongInfoContainer ... />
    <SeekBarSlider ... />
  </>
)}

Also simplify Line 61's outer div — the flex-row/flex-col toggle is now redundant since the inner wrapper handles the axis:

className={`flex max-w-full flex-col justify-end ${isMouseActive && 'group/fullScreenPlayer'}`}

🔴 Blocker 2 — !lyrics catches the loading state (null), causing a false "No lyrics" flash

LyricsContainer.tsx Line 177

The original code used lyrics === undefined (fetch completed, nothing found). null is the initial state before the fetch resolves. The changed condition !lyrics is truthy for both null and undefined, so on every song that has lyrics, users will see the "No lyrics available" error panel while the fetch is still in flight, then it disappears once lyrics arrive.

Fix — restore the three-way distinction:

// null  → fetch not yet complete → show nothing
// undefined → fetch complete, no lyrics found → show error
{(isShowLyricsWithSongInfo || isLyricsVisible) && lyrics === undefined && (
  <div className="text-font-color-highlight flex h-full w-full flex-col justify-center text-2xl opacity-50">
    <span className="material-icons-round-outlined mb-2 text-5xl">brightness_alert</span>
    <p>{t('lyricsPage.noLyrics')}</p>
    <p className="mt-4 text-base">{t('lyricsPage.noLyricsDescription')}</p>
  </div>
)}

🟠 Major — Stale isLyricsVisible initial value when preference changes while player is mounted

FullScreenPlayer.tsx Line 21

const [isLyricsVisible, setIsLyricsVisible] = useState(isShowLyricsWithSongInfo);

useState only uses its argument as the initial value at mount. If the user navigates to Settings and toggles "Show lyrics with song details" while FullScreenPlayer is already open, isLyricsVisible stays stale — the layout won't switch until the component remounts. Add a sync effect:

useEffect(() => {
  if (isShowLyricsWithSongInfo) setIsLyricsVisible(true);
}, [isShowLyricsWithSongInfo]);

This also makes the intent explicit: enabling side-by-side always forces lyrics visible.

🟠 Major — Unrelated scope creep: toggleAutoLaunch + toggleMiniPlayerAlwaysOnTop imports

src/main/ipc.ts Lines 88–89

These two imports have nothing to do with the lyrics/song-details side-by-side feature. They appear to be drive-by fixes for previously missing imports in ipc.ts.

• If they were causing a build/runtime error, they deserve their own focused commit or PR with an explanation.
• Unrelated changes in the same PR make review, bisect, and revert harder.

Please either move them to a separate PR or add an inline comment explaining why they're included here.

🟡 Minor — Dead ternary: both branches produce identical blur-[2rem]!

FullScreenPlayer.tsx Line 55

className={`... ${isLyricsVisible ? 'blur-[2rem]!' : 'blur-[2rem]!'}`}

Both branches are the same string — the conditional has no effect. Replace with a static class or fix to produce two distinct values if the intent was to differentiate:

className="... blur-[2rem]!"

🟡 Minor — Lyrics not scrollable in side-by-side mode

LyricsContainer.tsx Line 153

In side-by-side mode the LyricsContainer div has h-full but no overflow-auto. The parent in FullScreenPlayer.tsx has overflow-auto, but because the child fills the parent's full height with h-full, scroll is blocked. Long lyrics will be clipped with no scrollbar.

Fix — move overflow-auto onto the container itself and remove it from the parent:

isShowLyricsWithSongInfo
  ? 'relative flex h-full w-full flex-col items-start overflow-auto p-4'
  : 'absolute top-0 ...'

Side note: py-4 pr-4 pl-4 is equivalent to p-4 — cleaner to write.

💡 Nice-to-haves

• Tests: No unit/integration tests for the new layout branch or preference key.
• RTL layout: flex-row is not reversed for RTL locales. If the app ever targets RTL languages, consider rtl:flex-row-reverse or a dir-aware wrapper.
• Prop drilling: isShowLyricsWithSongInfo is threaded into both LyricsContainer and SongInfoContainer, but both already call useStore internally. Reading the preference directly inside each child would eliminate the prop entirely.
• Variable shadowing (LyricsSettings.tsx Line 74): const showLyricsWithSongInfo = storage.preferences.getPreferences(...) inside the effect shadows the outer state variable. Pre-existing pattern, but rename the inner variable (e.g. storedValue) for clarity.
• JSX duplication: SongInfoContainer and LyricsContainer are fully duplicated across both render branches. A single render with conditional class/prop differences would be cleaner and easier to maintain.
• imgPath memo (FullScreenPlayer.tsx Lines 39–41): useMemo wrapping a single property access adds overhead with no benefit. A plain const imgPath = currentSongData.artworkPath; suffices.

@coderabbitai review

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@coderabbitai

Addressed the major finding on src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx.

Pushed 0f88a242 to Owie6789:fix/461-lyrics-songdetails-sidebyside:

1. New query lyricsQuery.fullScreenPlayer in src/renderer/src/queries/lyrics.ts — wraps the entire getSongLyrics + getTranslatedLyrics + convertLyricsToPinyin/romanizeLyrics/convertLyricsToRomaja flow in one queryFn. It honors preferences.autoTranslateLyrics and preferences.autoConvertLyrics, and the current i18n.language. Caches via TanStack Query and dedupes across renders.
2. Safe defaults — the queryFn returns null (rather than throwing) on any rejection, so the existing !lyrics no-lyrics UI still renders. Errors are logged once via console.error inside the queryFn instead of being silently swallowed at the call site.
3. LyricsContainer rewrite — replaced the 49-line useEffect with a useQuery(lyricsQuery.fullScreenPlayer(...), { enabled: isLyricsVisible }) and a tiny useEffect that mirrors lyrics?.lyrics?.isSynced back to setIsLyricsAvailable. Dropped the useState<SongLyrics | null | undefined>(null) (the query owns the data now) and the unused useTranslation import stays only for the no-synced-lyrics copy.

I went with useQuery rather than useSuspenseQuery because the isLyricsVisible toggle wants a clean "not loaded" state when the lyrics panel is hidden, which useSuspenseQuery doesn't express well. The existing lyrics/index.tsx page follows the same useQuery({ ...lyricsQuery.single(...), staleTime: Infinity }) shape, so this stays consistent with the rest of the app.

Skipped (out of scope / per "keep changes minimal"):

• LyricsSettings.tsx L71-77 nitpick (💤 Low value) — the four useState + useEffect pairs are duplicative but harmless; extracting a usePreference hook is a stylistic refactor that belongs in a separate PR.

Could you re-review? If anything else surfaces (e.g. a missing enabled: isLyricsVisible interaction or a cache-key collision with lyricsQuery.single), happy to address.

#!/bin/bash
echo "=== Commit 0f88a242 diff ==="
gh api repos/Sandakan/Nora/commits/0f88a242 --jq '.files[] | {filename: .filename, patch: .patch}'

echo "=== New queries/lyrics.ts ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/queries/lyrics.ts?ref=0f88a242 \
  --jq '.content' | base64 -d

echo "=== Updated LyricsContainer.tsx ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx?ref=0f88a242 \
  --jq '.content' | base64 -d | cat -n

@Owie6789 — Good refactor overall. Inspected the new commit. The useQuery approach is clean, the queryFn is well-guarded, and there is no cache key collision with lyricsQuery.single (the factory namespaces them as ['lyrics', 'fullScreenPlayer', ...] vs ['lyrics', 'single', ...], so the fsp- prefix is harmless redundancy rather than a requirement). Two issues remain, one of which is the flash blocker in a slightly new form:

🔴 Blocker — enabled: isLyricsVisible never fires in side-by-side mode

LyricsContainer.tsx Line 40

enabled: isLyricsVisible   // ← isShowLyricsWithSongInfo is not considered

In side-by-side mode isShowLyricsWithSongInfo is true and lyrics should always load, but isLyricsVisible can be false (it's the useState that was only initialized from the preference at mount — the major finding still open). The query will never be triggered in side-by-side mode and the container will silently stay empty.

Fix:

enabled: isLyricsVisible || !!isShowLyricsWithSongInfo

🔴 Blocker — "No lyrics" flash is still present (sentinel shifted from null to undefined)

LyricsContainer.tsx Line 148

{(isShowLyricsWithSongInfo || isLyricsVisible) && !lyrics && (
  /* No lyrics available panel */
)}

With useQuery, data starts as undefined while the fetch is in flight (React Query's initial value before the queryFn resolves), then becomes null once the queryFn returns null (no lyrics / error). !lyrics is truthy for both states, so the "No lyrics" panel flashes for every song that has lyrics — same root problem as before, just with undefined as the loading sentinel instead of null.

Fix — destructure isPending and gate on it:

const { data: lyrics, isPending } = useQuery({
  ...lyricsQuery.fullScreenPlayer({ ... }),
  enabled: isLyricsVisible || !!isShowLyricsWithSongInfo
});

// …

{(isShowLyricsWithSongInfo || isLyricsVisible) && !isPending && !lyrics && (
  <div className="text-font-color-highlight …">
    {/* No lyrics available */}
  </div>
)}

isPending is true only while the query is actively fetching (and enabled is true), so this correctly suppresses the panel during the load and shows it only when the fetch is genuinely complete and returned nothing.

🟡 Minor — staleTime not set (unnecessary refetch on window focus)

LyricsContainer.tsx Line 27

The existing lyricsQuery.single usage elsewhere in the app passes staleTime: Infinity at the call site. Without it, TanStack Query's default staleTime: 0 marks the data stale immediately, so focusing the app window will trigger a refetch of the entire translate/convert pipeline. Lyrics don't change mid-song, so:

const { data: lyrics, isPending } = useQuery({
  ...lyricsQuery.fullScreenPlayer({ ... }),
  enabled: isLyricsVisible || !!isShowLyricsWithSongInfo,
  staleTime: Infinity
});

✅ Confirmed resolved by this commit

• Cache key isolation: no collision with lyricsQuery.single — confirmed.
• queryFn error handling: all async paths return null on failure; error is logged once — clean.
• Translation/conversion pipeline: null-guarded at each step, isReset flag checked before chaining — correct.
• setIsLyricsAvailable effect: correctly minimal, runs only when lyrics changes — correct.
• useSkipLyricsLines(lyrics ?? null): the ?? null coercion correctly passes null when data is undefined — correct.

@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 upstream/release/4.0.0-alpha.5) and walk the code in context.

What I want you to verify

1. Re-review the latest commit

Review 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 fulfillment

The 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:

• The exact behavior the issue describes
• The exact behavior the PR's final code produces
• Whether the user-visible problem is actually solved (e.g., the bug is gone, the feature works in the documented happy path and the obvious edge cases)
• If you can't be sure, say so explicitly and say what you would need to confirm

3. Deviation from issue fix

Compare 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:

• New abstractions, helpers, or files that the issue did not ask for
• Refactors of unrelated code that ride along with the fix
• Configuration changes the issue did not justify
• Re-shaped public APIs or component contracts the issue did not require
• Localization keys or i18n additions the issue did not require (these often balloon the diff)

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-sieveing

Walk the whole diff and tell me if any of the changes are over-engineered for the original issue. Examples:

• Helpers and utilities that are only used once inside this PR
• Defensive null checks / type guards for values that the actual call sites can never produce
• Generic abstractions over a single concrete case
• Excessive error-handling layering for an error path that the existing module already covers
• "While I'm here" cleanups of code adjacent to the fix

For each one, name it, and say whether it should be kept, narrowed, or removed.

What to skip

• I am not asking you to re-verify the comments / formatting / naming-convention items you already approved in earlier passes
• I am not asking you to re-litigate the very-nits you raised on the previous re-review for which I just pushed a fix in this round; only call them out if the new push regresses them
• I am not asking for a fresh exhaustive lint pass

Output format

Reply with four clearly-labeled sections — ## 1. Re-review of latest commit, ## 2. Original issue fulfillment, ## 3. Deviation from issue fix, ## 4. Over-sieveing. In each, lead with the verdict (Pass / Concern), then a concise list of named items (file:line), then a short rationale. Keep the whole reply under ~120 lines so I can act on it.

#!/bin/bash
echo "=== Current PR head ==="
gh api repos/Sandakan/Nora/pulls/496 --jq '{sha: .head.sha, branch: .head.ref, base: .base.ref}'

echo "=== All commits on branch ==="
gh api "repos/Sandakan/Nora/pulls/496/commits" --jq '.[].sha + " " + .[].commit.message'

echo "=== Issue `#461` body ==="
gh api repos/Sandakan/Nora/issues/461 --jq '.body'

echo "=== Full PR diff (latest) ==="
gh pr diff 496 --repo Sandakan/Nora

@Owie6789 — Analysis complete against HEAD c089c7e0. Four sections below, each verdict-first.

1. Re-review of latest commit (c089c7e0)

Verdict: One blocker still open; the three fixes from my last pass are correct.

✅ Fixed by this commit

• enabled: isLyricsVisible || !!isShowLyricsWithSongInfo — query now fires in side-by-side mode regardless of the isLyricsVisible toggle state.
• !isPending && !lyrics — "No lyrics" panel is suppressed while the fetch is in flight; flash is gone.
• staleTime: Infinity — no spurious refetches on window focus; consistent with lyricsQuery.single usage.

🔴 Blocker — SeekBarSlider layout not fixed (FullScreenPlayer.tsx Lines 61 + 99)

The outer container is still conditionally flex-row when isShowLyricsWithSongInfo is true:

className={`flex max-w-full ${isShowLyricsWithSongInfo ? 'flex-row' : 'flex-col'} justify-end ...`}

SeekBarSlider (Line 99) is a sibling of the <div className="flex h-full w-full flex-row overflow-hidden"> two-column wrapper. That wrapper declares w-full, which in a flex-row parent consumes 100% of horizontal space — the seek bar has zero room and is pushed off-screen or squashed to nothing.

Additionally, the inner <div className="flex h-full w-full flex-row overflow-hidden"> declares h-full, but its parent (the outer wrapper) has no h-full, so the height-fill chain is broken: columns won't expand to fill the viewport either.

Minimal fix (same as previously recommended — still applies):

{isShowLyricsWithSongInfo ? (
  <div className="flex h-full w-full flex-col">        {/* ← flex-col, not flex-row */}
    <div className="flex min-h-0 flex-1 flex-row overflow-hidden">
      <div className="flex w-1/2 items-end">
        <SongInfoContainer ... />
      </div>
      <div className="w-1/2 overflow-auto">
        <LyricsContainer ... />
      </div>
    </div>
    <SeekBarSlider ... />    {/* ← lives inside the wrapper, below columns */}
  </div>
) : (
  <>
    <LyricsContainer ... />
    <SongInfoContainer ... />
    <SeekBarSlider ... />
  </>
)}

And simplify the outer div to always flex-col (it no longer needs to toggle its axis):

className={`flex max-w-full flex-col justify-end ${isMouseActive && 'group/fullScreenPlayer'}`}

🟡 Nit — py-4 pr-4 pl-4 → p-4 (LyricsContainer.tsx Line ~122)

py-4 pr-4 pl-4 is identical to p-4. Not blocking, just noise.

2. Original issue fulfillment

Verdict: Conditionally passes — happy path works; seek-bar blocker prevents it from being fully usable.

What issue #461 (point 2) describes:

"In fullscreen or 'Now Playing' mode, opening lyrics currently replaces the song details entirely. It would improve usability if both lyrics and song information (title, artist, artwork) could be displayed simultaneously."

What the PR's final code produces:

• Adds an opt-in preference showLyricsWithSongInfo (off by default, persisted to localStorage).
• When enabled: fullscreen switches to a 50/50 flex-row split — SongInfoContainer left, LyricsContainer right.
• SongInfoContainer stays visible (no opacity-0/invisible hiding), lyrics toggle is hidden.
• Lyrics are fetched automatically (no manual toggle needed).
• Setting is accessible at Settings → Lyrics.

Is the problem solved?
The core ask — seeing both at once — is implemented and the preference survives restarts. However, with the SeekBarSlider blocker unresolved, the seek bar (and potentially playback controls) would be invisible or inaccessible in side-by-side mode, making this mode barely usable in practice. I can't confirm it fully passes until that layout bug is fixed.

3. Deviation from issue fix

Verdict: Two deviations; one unjustified, one borderline.

4. Over-engineering

Verdict: Two items worth noting; neither is a blocker.

1. String-concatenated query keys in lyricsQuery.fullScreenPlayer (queries/lyrics.ts Lines 54–63)

   queryKey: [
     `fsp-title=${title}`,
     `fsp-artists=${artists.join(',')}`,
     ...
   ]

   TanStack Query serializes nested objects natively (e.g. ['lyrics', 'fullScreenPlayer', { title, artists, ... }]). The fsp- prefix strings are a manual collision-avoidance pattern that createQueryKeys already handles by namespacing. The fsp-artists=a,b,c string also loses array identity (an artist named "a,b" collides with ["a","b"]). Recommendation: narrow — use a structured object key instead:

   queryKey: [{ title, artists, album, path, duration, language, autoTranslate, autoConvert }]

2. lyricsQuery.fullScreenPlayer as a single-use factory (queries/lyrics.ts Lines 47–118, used only in LyricsContainer.tsx)
   The translate → convert pipeline it contains duplicates logic that the original useEffect (now removed) and presumably lyricsQuery.single both implement. As a single-use abstraction it adds ~70 lines of new code for one call site. Recommendation: keep — the caching/dedup/retry benefits are real, and the error-handling is cleaner than the original useEffect. But consider whether lyricsQuery.single could be extended to accept autoTranslate/autoConvert params instead of maintaining two parallel pipelines. That consolidation is a follow-up, not a blocker.

@CodeRabbit, sorry i forgot to commit all my changes before asking that of you reapply and rereview that prompt i sad previously now that i have properly commited

#!/bin/bash
echo "=== Current PR head ==="
gh api repos/Sandakan/Nora/pulls/496 --jq '{sha: .head.sha, branch: .head.ref, base: .base.ref}'

echo "=== All commits on branch (newest first) ==="
gh api "repos/Sandakan/Nora/pulls/496/commits" --jq '.[].sha + " " + .[].commit.message'

echo "=== Full PR diff ==="
gh pr diff 496 --repo Sandakan/Nora

echo "=== Issue `#461` body ==="
gh api repos/Sandakan/Nora/issues/461 --jq '.body'

@Owie6789 — No worries! Here is the full four-section review against HEAD 41612701.

1. Re-review of latest commit

Verdict: Two blockers remain; two new unrelated changes introduced.

✅ Addressed since last pass

• useEffect sync of isLyricsVisible with isShowLyricsWithSongInfo — correctly added with an !== undefined guard. ✅
• enabled: isLyricsVisible || !!isShowLyricsWithSongInfo — query now fires in side-by-side mode. ✅
• staleTime: Infinity — no spurious refetches. ✅
• lyrics !== null guard on synced/unsynced lyrics blocks — prevents content flash during load. ✅

🔴 Blocker 1 — SeekBarSlider still off-screen in side-by-side mode

FullScreenPlayer.tsx Lines 67 + 108

The outer container still toggles flex-row when isShowLyricsWithSongInfo is true:

className={`flex max-w-full ${isShowLyricsWithSongInfo ? 'flex-row' : 'flex-col'} justify-end ...`}

The SeekBarSlider (Line 108) is a direct sibling of the inner <div className="flex h-full w-full flex-row overflow-hidden">, which has w-full. Inside a flex-row parent, that consumes 100% of horizontal space — the seek bar has zero room and is pushed off-screen or squashed to nothing. This fix was recommended in both previous passes and is still missing.

Minimal fix — wrap the two-column block and the seek bar together in a flex-col div:

{isShowLyricsWithSongInfo ? (
  <div className="flex h-full w-full flex-col">
    <div className="flex min-h-0 flex-1 flex-row overflow-hidden">
      <div className="flex w-1/2 items-end">
        <SongInfoContainer ... isShowLyricsWithSongInfo={isShowLyricsWithSongInfo} />
      </div>
      <div className="w-1/2 overflow-auto">
        <LyricsContainer ... isShowLyricsWithSongInfo={isShowLyricsWithSongInfo} />
      </div>
    </div>
    <SeekBarSlider ... />   {/* ← inside the wrapper, below columns */}
  </div>
) : (
  <>
    <LyricsContainer ... />
    <SongInfoContainer ... />
    <SeekBarSlider ... />
  </>
)}

And change the outer div to always flex-col (the inner wrapper owns the axis now):

className={`flex max-w-full flex-col justify-end ${isMouseActive && 'group/fullScreenPlayer'}`}

🔴 Blocker 2 — "No lyrics" flash still present; sentinel is wrong

LyricsContainer.tsx Line 148 (no-lyrics panel condition)

{(isShowLyricsWithSongInfo || isLyricsVisible) && lyrics === undefined && (
  /* No lyrics panel */
)}

With TanStack Query, data is undefined during the pending/loading state (before the queryFn resolves). The queryFn returns null when lyrics are definitively not found. So lyrics === undefined fires while the fetch is still in flight — producing the same flash as before, just with undefined as the loading sentinel instead of null.

isPending is already destructured on the same line but never used in the render block (TypeScript will warn about the unused variable).

Fix — either gate on isPending or change the sentinel:

{(isShowLyricsWithSongInfo || isLyricsVisible) && !isPending && lyrics === null && (
  <div ...>no lyrics</div>
)}

lyrics === null is only true after the queryFn completes and definitively finds no lyrics. lyrics === undefined means "not fetched yet."

🟠 New: Unrelated scope creep — tabIndex changes

MainContainer.tsx Line 62 · SecondaryContainer.tsx Line 22

Both change tabIndex={focusable ? 1 : undefined} → tabIndex={focusable ? 0 : undefined}. The fix is correct (tabIndex > 0 breaks natural tab order and is a known a11y anti-pattern), but it has nothing to do with lyrics side-by-side. Please move to a separate PR, or keep if you are batching a11y fixes intentionally — but call it out in the PR description.

2. Original issue fulfillment

Verdict: Conditionally passes — the concept is correct; SeekBar blocker and no-lyrics flash prevent a fully usable release.

What issue #461 point 2 describes:

"In fullscreen or 'Now Playing' mode, opening lyrics currently replaces the song details entirely. It would improve usability if both lyrics and song information (title, artist, artwork) could be displayed simultaneously."

What the PR produces (when both blockers are fixed):

• Opt-in showLyricsWithSongInfo preference, off by default, persisted to localStorage and dispatched to the Zustand store immediately ✅
• When on: fullscreen renders a 50/50 flex-row split with SongInfoContainer left and LyricsContainer right ✅
• SongInfoContainer stays visible at full opacity; lyrics toggle button is hidden ✅
• Lyrics are fetched automatically via useQuery without the user pressing the toggle ✅
• Setting is accessible at Settings → Lyrics ✅

The core user ask is addressed. The two blockers must be fixed before this is usable — in side-by-side mode the seek bar (and playback control) would be unreachable, and every song briefly shows "No lyrics available" during load.

3. Deviation from issue fix

Verdict: Four deviations found; two unjustified.

4. Over-engineering

Verdict: Two items; one should be narrowed, one is borderline keep.

1. String-concatenated query keys (queries/lyrics.ts Lines 55–65) — narrow

   queryKey: [`fsp-title=${title}`, `fsp-artists=${artists.join(',')}`, ...]

   TanStack Query serializes nested objects natively. The fsp- prefix strings also introduce an artist-name collision bug: an artist named "a,b" produces the same key segment as ["a", "b"]. Use a structured object instead:

   queryKey: [{ scope: 'fullScreenPlayer', title, artists, album, path, duration, language, autoTranslate, autoConvert }]

2. lyricsQuery.fullScreenPlayer as a single-use factory (queries/lyrics.ts Lines 47–118, called only in LyricsContainer.tsx) — keep (borderline)
   The fetch + translate + convert pipeline here duplicates logic from the old useEffect, but the caching, dedup, and retry are genuine wins. Worth noting as a follow-up: consider whether lyricsQuery.single could be extended to accept autoTranslate/autoConvert params to avoid two parallel pipelines long-term.

TL;DR for your next fix pass: The two items that must be resolved before merge are the SeekBarSlider layout (flex-row outer container + seek bar sibling) and the lyrics === undefined → !isPending && lyrics === null sentinel swap in the no-lyrics panel condition.

( ˶ᵔᵕᵔ˶ ) 🐇

@coderabbitai rereview

Merge conflict with upstream/release/4.0.0-alpha.5 resolved:

• Conflict: src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx — the upstream stacked LyricsContainer/SongInfoContainer layout conflicted with the fix branch's new conditional side-by-side layout (isShowLyricsWithSongInfo).
• Resolution: Kept the fix branch's side-by-side layout structure (the purpose of this PR) while incorporating the upstream's before:max-w-full CSS addition to the seekbar slider in both layout branches.
• SHA: 931a4a6b

#!/bin/bash
echo "=== Current PR head ==="
gh api repos/Sandakan/Nora/pulls/496 --jq '{sha: .head.sha, branch: .head.ref}'

echo "=== All commits ==="
gh api "repos/Sandakan/Nora/pulls/496/commits" --jq '.[].sha + " " + .[].commit.message'

echo "=== Full PR diff ==="
gh pr diff 496 --repo Sandakan/Nora

echo "=== FullScreenPlayer.tsx at HEAD ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n

echo "=== LyricsContainer.tsx at HEAD ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n

@Owie6789 — Good work resolving the merge conflict. The before:max-w-full is correctly applied to both SeekBarSlider instances. Here is the re-review against HEAD 931a4a6b.

✅ All previous blockers are resolved

⚠️ Still open before merge

🟠 Major — src/main/ipc.ts Lines 88–89 (unresolved since commit 6f8476ee)

toggleAutoLaunch and toggleMiniPlayerAlwaysOnTop imports are unrelated to this feature. Still present in this PR. Please move to a separate PR or add a clear inline comment explaining the dependency.

🟡 Minor / Nits (non-blocking)

• FullScreenPlayer.tsx Line 61 — Dead ternary still present: ${isLyricsVisible ? 'blur-[2rem]!' : 'blur-[2rem]!'}. Both branches are identical; simplify to a static class blur-[2rem]!.
• LyricsContainer.tsx Line 125 — py-4 pr-4 pl-4 → p-4 (same output, cleaner).
• FullScreenPlayer.tsx Lines 45–47 — useMemo(() => currentSongData.artworkPath, [...]) wraps a single property access; a plain const is sufficient.

TL;DR: No blockers remain. The one item requiring attention before merge is the unrelated ipc.ts imports. The rest are nits you can address now or in a follow-up.

( ˶ᵔᵕᵔ˶ ) 🐇

@coderabbitai rereview

#!/bin/bash
echo "=== Current PR head ==="
gh api repos/Sandakan/Nora/pulls/496 --jq '{sha: .head.sha, branch: .head.ref}'

echo "=== All commits ==="
gh api "repos/Sandakan/Nora/pulls/496/commits" --jq '.[].sha + " " + .[].commit.message'

echo "=== Full PR diff ==="
gh pr diff 496 --repo Sandakan/Nora

echo "=== FullScreenPlayer.tsx at HEAD ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n

echo "=== LyricsContainer.tsx at HEAD ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n

echo "=== ipc.ts lines 85-95 ==="
gh api repos/Sandakan/Nora/contents/src/main/ipc.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | sed -n '84,96p'

@Owie6789 — No new commits have landed since my last review: the PR head is still 931a4a6b, which is the same SHA I reviewed in the previous pass.

The current standing hasn't changed:

It looks like your intended commits may not have been pushed yet. Once you push, tag me again and I'll pick up the new SHA immediately.

( ˶ᵔᵕᵔ˶ ) 🐇

@coderabbitai review my last commit and see if it satisfies what you asked and the original issue

#!/bin/bash
echo "=== Current PR head ==="
gh api repos/Sandakan/Nora/pulls/496 --jq '{sha: .head.sha, branch: .head.ref}'

echo "=== All commits ==="
gh api "repos/Sandakan/Nora/pulls/496/commits" --jq '.[].sha + " " + .[].commit.message'

echo "=== Full PR diff ==="
gh pr diff 496 --repo Sandakan/Nora

echo "=== FullScreenPlayer.tsx at HEAD ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/FullScreenPlayer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n

echo "=== LyricsContainer.tsx at HEAD ==="
gh api repos/Sandakan/Nora/contents/src/renderer/src/components/FullScreenPlayer/containers/LyricsContainer.tsx?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n

echo "=== ipc.ts lines 84-96 ==="
gh api repos/Sandakan/Nora/contents/src/main/ipc.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | sed -n '84,96p'

@Owie6789 — Reviewed HEAD f967cc65. Here's the full accounting:

✅ Everything previously flagged is resolved

⚠️ One item still open

🟠 src/main/ipc.ts Lines 88–89 — toggleAutoLaunch and toggleMiniPlayerAlwaysOnTop imports are still present and unrelated to this feature. This is the sole remaining item before the PR is clean. Please either move it to a separate PR or add a brief inline comment explaining the dependency (e.g., these were missing and caused a runtime error that blocked this feature).

Original issue fulfillment

✅ Passes — Issue #461's request (lyrics and song details visible simultaneously in fullscreen/Now Playing mode) is fully addressed:

• Opt-in showLyricsWithSongInfo preference, off by default, persisted to localStorage and dispatched live to the Zustand store ✅
• 50/50 side-by-side layout with SongInfoContainer left and scrollable LyricsContainer right ✅
• SongInfoContainer stays fully visible; lyrics toggle hidden in this mode ✅
• Lyrics fetched automatically; no manual toggle required ✅
• Setting accessible at Settings → Lyrics with i18n labels ✅

Fix the ipc.ts imports and this is ready for merge. 🐇

@coderabbitai the ipc.ts has zero diff from release/4.0.0-alpha.5 — the imports in question already exist in the base branch. the commit 6f8476e that added them was redundant (already in base). can you recheck the diff between this PR's head and the base?

#!/bin/bash
echo "=== PR diff filtered to ipc.ts only ==="
gh pr diff 496 --repo Sandakan/Nora -- src/main/ipc.ts

echo "=== ipc.ts on base branch (release/4.0.0-alpha.5) lines 84-96 ==="
gh api repos/Sandakan/Nora/contents/src/main/ipc.ts?ref=release/4.0.0-alpha.5 \
  --jq '.content' | base64 -d | cat -n | sed -n '84,96p'

echo "=== ipc.ts on PR head lines 84-96 ==="
gh api repos/Sandakan/Nora/contents/src/main/ipc.ts?ref=refs/pull/496/head \
  --jq '.content' | base64 -d | cat -n | sed -n '84,96p'

@Owie6789 — You are completely right. The diff confirms it: toggleAutoLaunch and toggleMiniPlayerAlwaysOnTop are present at Lines 88–89 in the base branch (release/4.0.0-alpha.5) and the PR head is byte-for-byte identical. ipc.ts has no diff in this PR.

I was wrong to keep flagging that as scope creep across multiple review passes — commit 6f8476ee's changes had already been incorporated into the base branch before this PR was opened, so nothing in ipc.ts originates from this PR.

With that corrected, this PR has no remaining open issues. All previous blockers and majors have been resolved, and the ipc.ts concern is withdrawn. The PR is clean for merge.

( ˶ᵔᵕᵔ˶ ) 🐇

Learnt from: Owie6789
URL: https://github.com/Sandakan/Nora/pull/496

Timestamp: 2026-06-12T15:52:22.894Z
Learning: In the Nora repository (GitHub: Sandakan/Nora), when reviewing PR `#496` (branch fix/461-lyrics-songdetails-sidebyside targeting release/4.0.0-alpha.5), the imports `toggleAutoLaunch` and `toggleMiniPlayerAlwaysOnTop` in `src/main/ipc.ts` Lines 88-89 already exist in the base branch `release/4.0.0-alpha.5`. These are NOT scope creep introduced by this PR. Do not flag them as unrelated changes in any future review of this file or PR.

@Sandakan ready to merge. CR verdict: clean (latest commit f967cc6 + rebase pass). All flagged items resolved incl. the ipc.ts false-positive? Branch is MERGEABLE / CLEAN.