feat(ui): merged immersive view with tabbed control panel + native fullscreen (#328)#333
Merged
Merged
Conversation
…llscreen (#328) Merge the now-playing and lyrics fullscreens into one optional immersive view with a tabbed control panel (Lyrics / Queue) on the right, and add opt-in native OS fullscreen. Both are per-profile profile_setting toggles defaulting OFF, so the out-of-the-box experience is unchanged. - useTrackLyrics: shared lyrics fetch/parse/active-line hook consumed by both LyricsPanel and the immersive view (no double-fetch, all race guards preserved); removes the createPortal coupling from LyricsPanel - ImmersiveView orchestrator + ImmersiveNowPlaying / ImmersiveSidePanel / ImmersiveLyricsColumn / ImmersiveQueueTab / ImmersiveShareButton - classic interface (merged OFF or narrow window) keeps the pre-#328 now-playing <-> lyrics Mic2 toggle intact - PlayerContext: fullscreenOverlay enum -> immersiveOpen bool, old open/close names kept as aliases - native fullscreen capabilities added to capabilities/default.json - ImmersiveViewCard (Settings -> Appearance) + onboarding done-step tip - i18n 17 locales, docs synced Removes FullscreenNowPlaying + FullscreenLyrics.
InstaZDLL
added
scope: frontend
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLa vue plein écran Now Playing/Lyrics devient une vue immersive unifiée à deux colonnes. L’état player, les préférences, le hook de paroles, le rendu immersif, l’intégration dans le player et les textes associés sont mis à jour autour de ce nouveau flux. ChangesVue immersive fusionnée
Estimation de l'effort de revue🎯 4 (Complex) | ⏱️ ~60 minutes PRs potentiellement liées
Poème
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@docs/features/ui.md`:
- Line 165: The `immersive` description in the UI docs overstates the behavior
of `ui.cover_action`; update the wording to match `ImmersiveView` more
accurately. Clarify that the merged immersive view is only used when
`merged_lyrics` is enabled and the window is wide enough, and that it falls back
to the classic view otherwise. Keep the change in the `immersive` entry so the
docs reflect the actual behavior without promising a guaranteed fullscreen
merged view.
In `@src/components/player/ImmersiveQueueTab.tsx`:
- Around line 31-38: The fetch error handling in ImmersiveQueueTab should not
clear the last known queue snapshot when playerGetQueue() fails. Update the
promise chain in the ImmersiveQueueTab component so the catch path only logs the
refresh failure and leaves setSnapshot unchanged, while still guarding updates
with fetchSeqRef.current and seq to avoid stale results overwriting newer data.
- Around line 41-53: The listener setup in ImmersiveQueueTab’s useEffect can
leak if the component unmounts before listen("player:queue-changed", doFetch)
resolves, because unlisten is still null when cleanup runs. Update the effect to
track whether it has been disposed and, after listen resolves, immediately call
the returned unlisten if the component already unmounted; keep the existing
cleanup path in useEffect so the subscription is always removed.
In `@src/components/player/ImmersiveShareButton.tsx`:
- Around line 102-118: The action buttons in ImmersiveShareButton are missing an
explicit button type, so they can act as submit buttons inside a form; update
each menu action button in the component to use type="button" while keeping the
existing onClick handlers for handleSave and handleCopy.
In `@src/components/player/ImmersiveView.tsx`:
- Around line 109-147: The fullscreen enter/exit effect in ImmersiveView can
race on a fast open→close, leaving the window fullscreen after unmount because
the cleanup skips restoration while win.setFullscreen(true) is still in flight.
In the useEffect around enteredFullscreenRef and priorMaximizedRef, track an
explicit “enter in progress” state or cancellation flag that is checked again
after the await, and if the component has already been cleaned up, immediately
undo the fullscreen change and restore the prior maximized state before setting
enteredFullscreenRef.current.
In `@src/components/player/PlayerBar.tsx`:
- Around line 409-412: The `ImmersiveView` rendering in `PlayerBar` is
incorrectly guarded by `currentTrack`, which lets the modal unmount without
running `closeImmersive` when the track disappears. Remove the `currentTrack`
condition from the `immersiveOpen` check so `ImmersiveView` stays mounted while
open, and rely on `ImmersiveView` itself to handle missing track state; keep
`closeImmersive` as the only way to clear `immersiveOpen`.
In `@src/contexts/PlayerContext.tsx`:
- Around line 200-203: Reset the immersive UI state in PlayerContext whenever
the active profile/track is cleared, because immersiveOpen can stay true after
currentTrack becomes null and reopen ImmersiveView on the next track. Update the
PlayerContext state handling around immersiveOpen, setImmersiveOpen, and
currentTrack so the immersive view is explicitly closed when the track/profile
reset happens, and keep immersiveInitialTab in sync if needed.
In `@src/hooks/useImmersivePrefs.ts`:
- Around line 74-104: Le hook useImmersivePrefs peut réappliquer des préférences
périmées car refresh ne se relance qu’au mount/événement custom et les réponses
async peuvent arriver hors ordre. Faites dépendre le useEffect d’un signal de
changement de profil (ou équivalent) et ajoutez une garde de
séquence/cancellation dans refresh pour ignorer toute lecture obsolète avant
setPrefs, afin que ImmersiveViewCard reflète toujours le bon profil.
In `@src/hooks/useTrackLyrics.ts`:
- Around line 294-296: The applyPayload callback in useTrackLyrics only updates
the payload and leaves any previous error state visible, which can mask newly
applied external lyrics. Update applyPayload so that when a non-null
LyricsPayload is applied it also clears the existing error state, matching the
cleanup already done in importLyrics; use the applyPayload and importLyrics
callbacks in useTrackLyrics as the main reference points.
- Around line 109-115: The `useTrackLyrics` effect branch for `trackId == null`
currently resets payload and error but leaves `isFetching` unchanged, so the
spinner can stay stuck after a canceled request. Update the `trackId == null`
handling in `useTrackLyrics` to explicitly clear `isFetching` along with
`setPayload(null)` and `setError(null)`, keeping `prevIsRadioRef` in sync before
returning.
- Around line 277-285: Le callback clear() dans useTrackLyrics peut appliquer
setPayload(null) après un changement de piste, ce qui risque d’effacer l’état de
la nouvelle piste. Ajoute le même garde que dans importLyrics et refetch en
comparant requestedTrackId à trackIdRef.current juste après await
clearLyrics(trackId) et avant setPayload(null). Si la piste a changé
entre-temps, quitte sans modifier l’état; conserve aussi la gestion d’erreur
existante.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef0d070b-be6b-4bda-b53c-884e43190a10
📒 Files selected for processing (38)
CLAUDE.mddocs/features/integrations.mddocs/features/ui.mdsrc-tauri/crates/app/capabilities/default.jsonsrc/components/common/OnboardingModal.tsxsrc/components/layout/LyricsPanel.tsxsrc/components/player/FullscreenLyrics.tsxsrc/components/player/FullscreenNowPlaying.tsxsrc/components/player/ImmersiveLyricsColumn.tsxsrc/components/player/ImmersiveNowPlaying.tsxsrc/components/player/ImmersiveQueueTab.tsxsrc/components/player/ImmersiveShareButton.tsxsrc/components/player/ImmersiveSidePanel.tsxsrc/components/player/ImmersiveView.tsxsrc/components/player/PlayerBar.tsxsrc/components/views/SettingsView.tsxsrc/components/views/settings/ImmersiveViewCard.tsxsrc/contexts/PlayerContext.tsxsrc/hooks/useImmersivePrefs.tssrc/hooks/usePlayer.tssrc/hooks/useTrackLyrics.tssrc/i18n/locales/ar.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pt-BR.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/zh-CN.jsonsrc/i18n/locales/zh-TW.json
💤 Files with no reviewable changes (2)
- src/components/player/FullscreenLyrics.tsx
- src/components/player/FullscreenNowPlaying.tsx
- useTrackLyrics: clear isFetching on track->null, guard clear() against track switch, clear error in applyPayload (match import/refetch) - useImmersivePrefs: re-read on profile switch + sequence guard vs out-of-order async reads - ImmersiveView: undo fullscreen if cleanup raced setFullscreen(true) - ImmersiveQueueTab: keep last queue on transient fetch error, fix listener leak when unmounted before listen() resolves - ImmersiveShareButton: type="button" on menu actions - PlayerBar/PlayerContext: drop currentTrack mount guard, close immersive on profile reset so it can't reopen on the next track - docs: cover_action `immersive` wording matches actual opt-in behaviour
Owner
Author
|
@coderabbitai thanks — all findings were valid, fixed in 37767a7:
typecheck / lint / build all green. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✅ |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/hooks/useImmersivePrefs.ts (1)
75-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRéinitialise
loadedpendant le changement de profil.
prefsconserve les valeurs du profil précédent jusqu’à la fin de la nouvelle lecture. Pendant cette fenêtre,ImmersiveViewpeut appliquer l’ancienmergedLyrics/useNativeFullscreenau nouveau profil.Correctif proposé
useEffect(() => { let cancelled = false; + // eslint-disable-next-line react-hooks/set-state-in-effect + setPrefs({ ...DEFAULTS, loaded: false }); const refresh = async () => {🤖 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/hooks/useImmersivePrefs.ts` around lines 75 - 115, `useImmersivePrefs` keeps the previous profile’s values during a profile switch because `loaded` stays true until `refresh` finishes. Update the `useEffect` in `useImmersivePrefs` so that when `activeProfileId` changes it immediately resets `prefs` back to `DEFAULTS` (with `loaded` false) before starting the async read, then let `refresh` replace it with the new profile’s values once `getProfileSetting` resolves.src/hooks/useTrackLyrics.ts (1)
280-289: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEfface aussi l’erreur après un
clear()réussi.Après
setPayload(null), une erreur précédente reste dans l’état et peut continuer à masquer l’état vide attendu.Correctif proposé
- await clearLyrics(trackId); + await clearLyrics(requestedTrackId); if (requestedTrackId !== trackIdRef.current) return; setPayload(null); + setError(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/hooks/useTrackLyrics.ts` around lines 280 - 289, The clear path in useTrackLyrics’s clear callback leaves a previous error in state even after a successful clear, so the empty state can still be masked. Update the clear() flow after clearLyrics(trackId) succeeds and the staleness guard passes to also reset the error state alongside setPayload(null), using the existing state setter in useTrackLyrics so a successful clear fully reflects the empty/no-error state.
🤖 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.
Duplicate comments:
In `@src/hooks/useImmersivePrefs.ts`:
- Around line 75-115: `useImmersivePrefs` keeps the previous profile’s values
during a profile switch because `loaded` stays true until `refresh` finishes.
Update the `useEffect` in `useImmersivePrefs` so that when `activeProfileId`
changes it immediately resets `prefs` back to `DEFAULTS` (with `loaded` false)
before starting the async read, then let `refresh` replace it with the new
profile’s values once `getProfileSetting` resolves.
In `@src/hooks/useTrackLyrics.ts`:
- Around line 280-289: The clear path in useTrackLyrics’s clear callback leaves
a previous error in state even after a successful clear, so the empty state can
still be masked. Update the clear() flow after clearLyrics(trackId) succeeds and
the staleness guard passes to also reset the error state alongside
setPayload(null), using the existing state setter in useTrackLyrics so a
successful clear fully reflects the empty/no-error state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab8175cd-ecfc-4717-9ada-cde7f47b1400
📒 Files selected for processing (8)
docs/features/ui.mdsrc/components/player/ImmersiveQueueTab.tsxsrc/components/player/ImmersiveShareButton.tsxsrc/components/player/ImmersiveView.tsxsrc/components/player/PlayerBar.tsxsrc/contexts/PlayerContext.tsxsrc/hooks/useImmersivePrefs.tssrc/hooks/useTrackLyrics.ts
Owner
Author
|
@coderabbitai both valid, fixed in 3d804be:
typecheck / lint green. |
|
Tip For best results, initiate chat on the files or code changes.
In particular:
Thanks for tightening this up, and for confirming typecheck/lint are green. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/useImmersivePrefs.ts`:
- Around line 83-90: The `useImmersivePrefs` refresh flow resets state to
`loaded:false`, but the error path never publishes a დასრულებული state, so
callers can stay blocked after a Tauri read failure. Update the `refresh` logic
in `useImmersivePrefs` so the `catch` path still sets a final prefs state based
on `DEFAULTS` for the current refresh, with `loaded` true, and keep the existing
`setPrefs({ ...DEFAULTS, loaded: false })` only as the temporary pre-read reset.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94811dec-a95e-40ea-987d-c318d1b49a18
📒 Files selected for processing (2)
src/hooks/useImmersivePrefs.tssrc/hooks/useTrackLyrics.ts
Owner
Author
|
@coderabbitai valid, fixed in 98e3b8c — the |
|
Tip For best results, initiate chat on the files or code changes.
Publishing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/player/ImmersiveLyricsColumn.tsx`:
- Around line 84-90: The cover button in ImmersiveLyricsColumn uses an
accessibility label that describes the wrong action. Update the button’s
aria-label and title to a dedicated “return to classic lyrics/view” label
instead of playerBar.openFullscreen, since ImmersiveView passes
onCoverClick={toggleClassicLyrics} and the click does not open fullscreen. Keep
the change localized to the cover-click button and use the existing i18n pattern
so screen readers and the tooltip announce the actual 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 972357fa-e278-4d62-acc1-90e15e569678
📒 Files selected for processing (2)
src/components/player/ImmersiveLyricsColumn.tsxsrc/components/player/ImmersiveView.tsx
Owner
Author
|
@coderabbitai valid, fixed in the latest push — the cover-click button now uses |
|
Tip For best results, initiate chat on the files or code changes.
Thanks for tightening it up and confirming typecheck + lint are green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #328. Reworks the immersive view to address the reporter's two points (not actually fullscreen; now-playing and lyrics are mutually-exclusive overlays with dead side space) — but ships them as opt-in, per-profile options so the out-of-the-box experience is unchanged.
Both toggles live in Settings → Appearance (
ImmersiveViewCard), persist inprofile_setting, and default OFF:immersive.merged_lyrics)PanelRighttoggle to hide/showimmersive.use_native_fullscreen)setFullscreen(true)), restoring the prior maximized state on closeNarrow windows (< 900 px) force the classic interface even when merged is on.
Architecture
useTrackLyrics— extracts the full lyrics lifecycle (three-tier fetch + parse + active-line/word tracking + import/refetch/clear, with every mid-flight staleness guard) fromLyricsPanelinto a shared hook. BothLyricsPaneland the immersive view consume it, so there's no second fetch path and the oldcreatePortal-from-panel coupling is gone.ImmersiveView(orchestrator + reversible native-fullscreen effect + narrow fallback) →ImmersiveNowPlaying,ImmersiveSidePanel(tabs) →ImmersiveLyricsColumn+ImmersiveQueueTab,ImmersiveShareButton.ImmersiveQueueTabreusesplayer_get_queue/player_jump_to_index(white-on-dark styled); the dockedQueuePanelkeeps its drag-reorder.PlayerContext:fullscreenOverlayenum →immersiveOpenbool +immersiveInitialTab; oldopenFullscreenNowPlaying/openFullscreenLyrics/close*kept as aliases.core:window:allow-set-fullscreen/allow-is-fullscreen/allow-is-maximized/allow-maximize— added tocapabilities/default.json(compiled into the binary, so it needs a Rust rebuild).FullscreenNowPlaying+FullscreenLyrics.Notes
docs/features/ui.md,integrations.md) synced.Validation
bun run typecheck✅ ·bun run lint✅ ·bun run build✅Summary by CodeRabbit