Repository navigation
Feat/v3.11 - #127
Merged
Merged
Feat/v3.11#127
Conversation
- Insert Play Options as settings item 5 (History/Updates/About shift to 6/7/8) - Toggle ContinueOnNavigate, ConfirmStop, ShowMetadata - Adjust DefaultVolume ±5%; Reset to Defaults - Load/save via LoadPlayOptionsConfigFromUnified / SavePlayOptionsConfigToUnified
…rminates the entire process tree — both the shim and any child processes it spawned (the real mpv). Process.Kill() is then called afterward to clean up Go's internal process handle. The old "last resort" fallback that lacked /T is replaced entirely by this upfront approach.
📝 WalkthroughWalkthroughAdds Continue-on-Navigate playback handoff with an app-level Now Playing bar, Play Options config (volume, confirm-stop, metadata, last-used), per-player MPV instance isolation and killed-flag to avoid Stop/Play races, async UUID station resolution, pervasive UI wiring, and many unit/regression tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as UI Screen
participant Player as MPVPlayer
participant App
rect rgba(100, 150, 200, 0.5)
Note over User,App: Continue on Navigate (handoff)
User->>Screen: Select station
Screen->>Player: PlayWithVolume(station, vol)
Player->>Player: Start playback
User->>Screen: Navigate away
alt ContinueOnNavigate enabled
Screen->>App: handoffPlaybackMsg(player, station, context)
App->>App: activePlayer = player
App->>AllScreens: nowPlayingBar available
else
Screen->>Player: Stop()
end
end
sequenceDiagram
participant User
participant Screen as TopRated/Search
participant API as API Client
participant Player as MPVPlayer
participant Storage as Unified Config
rect rgba(100, 150, 200, 0.5)
Note over Screen,Player: UUID resolution + play options
Storage->>Screen: LoadPlayOptionsConfig()
User->>Screen: Select station (no URL)
Screen->>API: GetByUUID(uuid)
API-->>Screen: Station (URLResolved)
Screen->>Screen: compute startVol (playOptsCfg + station)
Screen->>Player: PlayWithVolume(station, startVol)
end
sequenceDiagram
participant User
participant App
participant Player as MPVPlayer
rect rgba(100, 150, 200, 0.5)
Note over Player,App: Windows process termination path
User->>App: Stop or close app
App->>Player: Stop()
Player->>Player: set killed=true
alt Windows
Player->>OS: taskkill /F /T /PID {pid}
Player->>Player: Process.Kill() attempt
else
Player->>OS: Process.Kill()
end
Player-->>App: stopped
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/ui/search.go (1)
532-553:⚠️ Potential issue | 🟠 Major
ConfirmStopis still bypassed on Search playback exits.The
0andEscbranches now navigate immediately, but they never gate onm.playOptsCfg.ConfirmStop. That leaves the new setting ineffective on this screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/search.go` around lines 532 - 553, The 0 and "esc" branches navigate/stop immediately without honoring m.playOptsCfg.ConfirmStop; modify both branches to first check m.playOptsCfg.ConfirmStop and invoke the same confirmation flow used elsewhere (the function that prompts the user and returns whether to proceed) before navigating or handing off the player: if ConfirmStop is true, call that confirmation routine and only continue to call m.navigateToMainCmd(), m.handOffPlayer(), or m.player.Stop() when the user confirms; otherwise abort and stay in the current selection state. Ensure you still nil out m.selectedStation and set m.state appropriately only after confirmation succeeds.internal/ui/lucky.go (1)
206-223:⚠️ Potential issue | 🔴 CriticalMissing handler for
luckyStateConfirmStopstate.The
Updatemethod'stea.KeyMsgswitch (lines 206-223) routes key messages to state-specific handlers, but there's no case forluckyStateConfirmStop. When the user is in the confirm-stop prompt, key presses will fall through to the default text input handler at line 541-544, making it impossible to confirm or cancel stopping.🐛 Proposed fix: add handler for luckyStateConfirmStop
case luckyStateManageTags: return m.updateManageTags(msg) + case luckyStateConfirmStop: + return m.updateConfirmStop(msg) }Then add the handler method:
// updateConfirmStop handles the confirm-stop prompt (Phase 5). func (m LuckyModel) updateConfirmStop(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.String() { case "y", "Y", "1": // Stop playback and return to input if m.player != nil { _ = m.player.Stop() } m.state = luckyStateInput m.inputMode = true m.textInput.Focus() m.selectedStation = nil m.reloadSearchHistory() m.rebuildMenuWithHistory() return m, nil case "n", "N", "2", "esc": // Cancel - return to playing m.state = luckyStatePlaying return m, nil } return m, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/lucky.go` around lines 206 - 223, Switch in LuckyModel's Update is missing a case for luckyStateConfirmStop, so key presses during the confirm-stop prompt fall through to the default input handler; add a case in the state switch to call a new handler (e.g., m.updateConfirmStop(msg)) and implement updateConfirmStop on LuckyModel to handle "y"/"Y"/"1" by stopping playback (if m.player != nil call m.player.Stop()), set m.state = luckyStateInput, m.inputMode = true, focus m.textInput, clear m.selectedStation = nil, call m.reloadSearchHistory() and m.rebuildMenuWithHistory(), and handle "n"/"N"/"2"/"esc" by setting m.state = luckyStatePlaying and returning; ensure the method returns (tea.Model, tea.Cmd) like the other update* handlers.internal/ui/app.go (1)
302-355:⚠️ Potential issue | 🟠 MajorVolume retrieval in Cleanup() occurs after activePlayer is stopped.
At line 303,
activePlayer.Stop()is called and the player is set to nil at line 304. Then at lines 344-355, the code attempts to get the volume fromactivePlayer(which is now nil) orquickFavPlayer. TheactivePlayerbranch at lines 346-347 will never execute becauseactivePlayerwas just set to nil.🐛 Proposed fix: retrieve volume before stopping
func (a *App) Cleanup() { a.cleanupOnce.Do(func() { if a.sleepTimer != nil { a.sleepTimer.Cancel() a.sleepTimer = nil } + + // Phase 5: Save LastUsedVolume before stopping players + if a.playOptsCfg.StartVolumeMode == "last_used" { + var lastVol int + if a.activePlayer != nil { + lastVol = a.activePlayer.GetVolume() + } else if a.quickFavPlayer != nil { + lastVol = a.quickFavPlayer.GetVolume() + } + if lastVol > 0 { + a.playOptsCfg.LastUsedVolume = lastVol + _ = storage.SavePlayOptionsConfigToUnified(a.playOptsCfg) + } + } + if a.activePlayer != nil { _ = a.activePlayer.Stop() a.activePlayer = nil } if a.quickFavPlayer != nil { _ = a.quickFavPlayer.Stop() } // ... rest of cleanup - - // Phase 5: Save LastUsedVolume if needed - if a.playOptsCfg.StartVolumeMode == "last_used" { - var lastVol int - if a.activePlayer != nil { - lastVol = a.activePlayer.GetVolume() - } else if a.quickFavPlayer != nil { - lastVol = a.quickFavPlayer.GetVolume() - } - if lastVol > 0 { - a.playOptsCfg.LastUsedVolume = lastVol - _ = storage.SavePlayOptionsConfigToUnified(a.playOptsCfg) - } - } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 302 - 355, The code reads a player's volume after Stop() and nil assignment so activePlayer is always nil; in Cleanup() capture the volume before calling Stop()/nil-ing the player: check a.activePlayer.GetVolume() first (if non-nil) and fallback to a.quickFavPlayer.GetVolume(), store that into a.playOptsCfg.LastUsedVolume when StartVolumeMode == "last_used", then call Stop()/set nil for activePlayer and stop other players and finally call storage.SavePlayOptionsConfigToUnified(a.playOptsCfg); update references to activePlayer, quickFavPlayer, playOptsCfg.StartVolumeMode, playOptsCfg.LastUsedVolume, and storage.SavePlayOptionsConfigToUnified accordingly.
🧹 Nitpick comments (4)
internal/player/mpv_test.go (1)
70-72: Minor note:returnaftert.Fatalis redundant.
t.Fatalalready stops test execution viaruntime.Goexit(). Thereturndoesn't cause harm but is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/player/mpv_test.go` around lines 70 - 72, Remove the redundant return immediately following the t.Fatal call in the test (the t.Fatal invocation already stops execution via runtime.Goexit()); locate the failing assertion in internal/player/mpv_test.go where t.Fatal("Expected current station to be set") is used and delete the subsequent `return` statement so the test is cleaner.internal/storage/config.go (1)
176-186: Extract a shared unified-config updater.This is another copy of the load → mutate one section → save flow already used by the appearance, connection, blocklist, shuffle, and play-history adapters above. A small helper would keep the validation/error-handling path and the non-atomic-update caveat in one place instead of repeating it for each new section.
♻️ Possible extraction
+func updateUnifiedConfig(update func(*config.Config)) error { + cfg, err := config.Load() + if err != nil { + return err + } + update(cfg) + return config.Save(cfg) +} + func SavePlayOptionsConfigToUnified(po config.PlayOptionsConfig) error { - cfg, err := config.Load() - if err != nil { - return err - } - cfg.PlayOptions = po - return config.Save(cfg) + return updateUnifiedConfig(func(cfg *config.Config) { + cfg.PlayOptions = po + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/config.go` around lines 176 - 186, Extract a shared updater function (e.g., UpdateUnifiedConfig(func(cfg *config.UnifiedConfig) error) error) that encapsulates the load → mutate → save flow, preserves the non-atomic update caveat and error handling, and accepts a mutator callback that updates a section; then refactor SavePlayOptionsConfigToUnified (and the appearance, connection, blocklist, shuffle, and play-history adapter functions) to call this new UpdateUnifiedConfig and perform only the section mutation (cfg.PlayOptions = po) inside the callback, returning any validation errors from the mutator so all load/save/error logic is centralized.internal/ui/searchmodel_extra.go (2)
47-50: Init() returns nil instead of a command to start the ticker.Unlike other UI models that return
tickEverySecond()or similar fromInit(), this returnsnil. The ticker is started conditionally in response to messages (e.g., lines 128, 177, 211), which means status messages won't count down until the first relevant event occurs.This is acceptable since the countdown is triggered when needed, but it differs from some other models that start the ticker unconditionally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/searchmodel_extra.go` around lines 47 - 50, Init currently returns nil so the per-second ticker never starts until a message triggers it; change SearchModel.Init to return the ticker command (e.g., return tickEverySecond()) so the status countdown starts unconditionally like other UI models—update the Init() method in the SearchModel type to call and return tickEverySecond() (or the shared ticker helper used by other models) instead of nil.
316-388: Number buffer logic differs from other screens - single digits are buffered.In
handleMenuKey, single digits 1-9 are buffered (line 369) and require either a second digit or Enter to execute. This differs from the main menu inapp.gowhere single digits that cannot start a valid QF number execute immediately.The comment at lines 366-367 explains this is intentional for two-digit history shortcuts. However, this means pressing "1" alone doesn't immediately select the first search type—the user must press Enter. This is a UX inconsistency but may be intentional for this screen's workflow.
Consider adding a brief timeout or documenting the Enter requirement more prominently if this behavior is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/searchmodel_extra.go` around lines 316 - 388, The number-buffer UX inconsistency is that SearchModel.handleMenuKey buffers single digits (numberBuffer) and requires Enter, unlike app.go's immediate single-digit selection; implement a brief auto-execute timeout to match user expectations: when you append a single digit to numberBuffer in handleMenuKey, dispatch a tea.Cmd that sends a new numberTimeoutMsg after ~700ms; add a case in SearchModel.Update (or handleMenuKey) to handle numberTimeoutMsg by parsing numberBuffer and calling selectByNumber (clearing numberBuffer), and ensure Enter and the existing two-digit logic still work and that the special "0" handling and menuList update behavior remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/README.md`:
- Line 22: Update the "Continue on Navigate" bullet to match current UI by
changing "Now Playing bar showing on every page" to "Now Playing bar showing on
every non-player screen" and change the Settings pointer from "3. Play Options"
to "5. Play Options"; also make the same two textual edits where this content
repeats (the other occurrences referenced around lines 482-485) so all docs
consistently reflect the UI behavior and the new Settings menu index.
In `@internal/storage/config_test.go`:
- Line 12: The test currently only calls t.Setenv("HOME", t.TempDir()) which is
insufficient to fully isolate config paths; update the test(s) (referencing the
call to t.Setenv("HOME", ...) in internal/storage/config_test.go) to also set
XDG_CONFIG_HOME and APPDATA to the same temp dir (e.g., call
t.Setenv("XDG_CONFIG_HOME", tempDir) and t.Setenv("APPDATA", tempDir)), or add a
small helper (e.g., setConfigEnvForTest) that takes t and a temp dir and sets
HOME, XDG_CONFIG_HOME, and APPDATA; apply this change to both occurrences noted
in the file.
In `@internal/ui/app_test.go`:
- Around line 644-674: The test currently only calls nowPlayingBar() and
re-checks isPlayerScreen() locally, so it doesn't exercise the render path;
update TestNowPlayingBar_HiddenOnPlayerScreens to call app.View() for each
player screen (screenTopRated, screenMostPlayed, screenPlay) and assert that the
returned view string does NOT contain the nowPlayingBar() output (or the
specific banner substring) when app.isPlayerScreen() is true; keep the initial
skip if nowPlayingBar() returns empty, but when not skipped use app.View() (not
just re-deriving appended = !app.isPlayerScreen()) to verify the bar is actually
suppressed in the rendered output.
In `@internal/ui/app.go`:
- Around line 761-764: The searchScreen.sleepCountdown field is never updated
because the assignment of the parsed countdown string was commented out; inspect
the code path where the local variable countdown (string) is computed and either
convert it to an int and assign it to searchScreen.sleepCountdown (e.g., parse
remaining seconds with strconv.Atoi or compute seconds from a duration) or
change searchScreen.sleepCountdown's type to string if it should display the
formatted countdown; update the assignment at the spot referencing countdown so
searchScreen.sleepCountdown reflects the actual remaining time and keep the
fall-through routing behavior intact.
- Around line 1262-1282: The inner conditional that checks a.activePlayer != nil
&& a.activePlayer != a.quickFavPlayer is dead code because the outer if already
requires a.activePlayer == nil; remove that inner if and its Stop() call and
directly perform the handoff: set a.activePlayer = a.quickFavPlayer,
a.activeStation = a.playingStation, a.activeContextLabel = "Quick Play", create
a fresh a.quickFavPlayer via player.NewMPVPlayer(), set its metadata manager if
a.metadataManager != nil, and call a.broadcastNowPlayingBar(); leave the else
branch that stops a.quickFavPlayer intact.
In `@internal/ui/browse_tags.go`:
- Around line 343-386: The handoff currently returns a handoffPlaybackMsg
containing the live m.player, which leaves BrowseTagsModel still holding that
same pointer and allows subsequent playSelected() to reuse/overlap the
handed-off player; fix both navigateBackCmd() and navigateToMainCmd() to perform
a safe handoff by capturing the current player into a local variable (e.g.
oldPlayer := m.player), then immediately release ownership on the model (set
m.player = nil or mark it as handed off) before returning the handoffPlaybackMsg
so the model no longer references the live player; apply the same safe-handoff
change to the analogous code at the other occurrence (lines ~536-556) so App
becomes the sole owner after handoff.
- Around line 393-409: The branches checking m.playOptsCfg.ConfirmStop
mistakenly set m.state = browseTagsStateConfirmStop (an unhandled state) causing
a blank UI; instead, remove the assignment to browseTagsStateConfirmStop and
perform the same stop/hand-off flow as the non-confirm path: call cmd :=
m.navigateBackCmd(), set m.state = browseTagsStateDetail, set m.selectedStation
= nil, call m.loadDetailStations(), and return m, cmd if cmd != nil (i.e., treat
ConfirmStop true the same as false here or implement a proper confirm UI handler
if intended later); update both the "esc" and "0" cases accordingly.
In `@internal/ui/most_played_test.go`:
- Around line 16-17: Tests use hardcoded "/tmp" paths when constructing
NewMostPlayedModel and blocklist.NewManager; replace those with paths derived
from t.TempDir() (e.g., join t.TempDir() with "favorites" and "blocklist.json"
using filepath.Join) and keep the existing playOptsCfg assignment
(config.PlayOptionsConfig{ConfirmStop: true}); update all occurrences that
construct NewMostPlayedModel and blocklist.NewManager in this file (including
the other test cases referenced) to use t.TempDir() so tests are OS-independent.
In `@internal/ui/most_played.go`:
- Around line 936-952: The confirm-stop flow currently always calls
navigateBackCmd() in MostPlayedModel.handleConfirmStopInput which loses the
origin (whether prompt came from pressing "0" or Escape) and causes wrong
navigation; add a boolean field on MostPlayedModel (e.g.,
confirmStopOpenedByZero or confirmStopFromMain) that you set when the
confirm-stop prompt is opened, then update handleConfirmStopInput to check that
flag: if the prompt was opened by "0" (the new flag true) perform the main-menu
return behavior (set m.state appropriately, clear m.selectedStation, and do not
call navigateBackCmd), otherwise call navigateBackCmd() as before; clear/reset
the flag after handling so subsequent prompts behave correctly.
- Around line 443-486: The model keeps the old player pointer after handing off
playback, causing MostPlayed to later reuse the app-owned player; to fix, make
the navigateBackCmd and navigateToMainCmd methods use a pointer receiver (change
MostPlayedModel to *MostPlayedModel) and clear m.player (set m.player = nil)
when you create/return the handoffPlaybackMsg so the model no longer holds the
app-owned player; update both functions (navigateBackCmd, navigateToMainCmd) to
mutate the model and nil out m.player before returning the handoff command (or
include a cmd that nils m.player immediately prior to returning
handoffPlaybackMsg).
In `@internal/ui/play.go`:
- Around line 1365-1382: Wrap the call to RenderStationDetailsWithRating so it
respects m.playOptsCfg.ShowMetadata: only call
RenderStationDetailsWithRating(*m.selectedStation, hasVoted, metadata, rating,
m.starRenderer) when ShowMetadata is true, and when false render the compact
fallback used by the other play screens (e.g., call the compact renderer used
elsewhere such as RenderStationDetailsCompact or RenderStationHeaderCompact with
the minimal args like *m.selectedStation and m.starRenderer). Also remove the
unconditional RenderStationDetailsWithRating call at the top of the snippet and
ensure the rest of the playback-status block remains inside the same
ShowMetadata conditional.
- Around line 1668-1686: The ConfirmStop handler always sends users to
playStateStationSelection; preserve the original opener (Esc vs 0) by adding a
PlayModel field (e.g. confirmStopFromZero bool or confirmStopOrigin enum) and
use it in updateConfirmStop to choose the destination: if confirmStopFromZero
then set m.state = playStateMainMenu else set m.state =
playStateStationSelection; only clear selectedStation/trackHistory when
navigating to station selection, reset confirmStopFromZero after handling, and
ensure the code that opens the confirm-stop prompt sets this new field
appropriately.
In `@internal/ui/search_test.go`:
- Around line 486-490: Delete the empty placeholder test
TestQuickFavoritesLoading (or replace it with a meaningful regression) because
it contains no assertions and references removed behavior
quickFavoritesLoadedMsg; either remove the TestQuickFavoritesLoading function
from internal/ui/search_test.go or implement a new test that asserts the current
behavior that replaced quickFavoritesLoadedMsg (identify and call the new
loader/notification API used by the code under test, exercise the load path, and
assert expected state/side-effects instead of leaving the test body empty).
In `@internal/ui/search.go`:
- Around line 105-110: The placeholder loadAvailableLists() must be implemented
to return a tea.Cmd that reads the persisted lists from the app storage and
updates the SearchModel; replace the stub with logic that calls your storage
layer (the same API used by handleSelectList and handleNewListInput flows) to
fetch available lists, then dispatches a Msg (e.g., AvailableListsLoaded or an
error msg) to update m.AvailableLists and clear loading state. Also implement
the complementary save command(s) invoked by handleSelectList() and
handleNewListInput() to persist a station into the chosen list (use the storage
client method used elsewhere), and return a tea.Cmd that emits a
SaveToListResult (success or failure) so the UI can show success/failure and
refresh available lists by calling loadAvailableLists(). Ensure you reference
loadAvailableLists, handleSelectList, and handleNewListInput when wiring these
commands and messages.
In `@internal/ui/styles.go`:
- Around line 327-349: RenderPageWithBottomHelp currently reserves a single line
for layout.NowPlaying but injects the full banner which can wrap; before writing
layout.NowPlaying (or when computing nowPlayingLineCount) truncate or sanitize
it to fit a single rendered line by measuring terminal width and trimming with
an ellipsis (or strip/control chars) so it cannot wrap, or alternatively compute
the wrapped line count and use that to set nowPlayingLineCount; modify the code
paths that reference layout.NowPlaying and nowPlayingLineCount to use the
truncated/sanitized banner (or the actual wrapped height) so the help footer
cannot be pushed off-screen.
In `@internal/ui/tag_playlists.go`:
- Around line 600-627: updatePlaying currently sets m.state =
tagPlaylistsStateConfirmStop but the code never handles that state, leaving the
UI non-interactive; implement handling for tagPlaylistsStateConfirmStop inside
the tag playlists update loop so the confirm prompt is actionable: add a branch
(or case) that when m.state == tagPlaylistsStateConfirmStop accepts confirm keys
(e.g., Enter to perform the action by calling navigateBackCmd() or
navigateToMainCmd() as appropriate and clearing
selectedStation/selectedPlaylist) and cancel keys (e.g., Esc to reset m.state =
tagPlaylistsStateDetail and call loadDetailStations() if needed); use the
existing helper methods navigateBackCmd(), navigateToMainCmd(), and
loadDetailStations() and ensure state transitions and returned cmds are wired so
the confirm state is actually dispatched and rendered.
- Around line 555-598: The navigateBackCmd and navigateToMainCmd currently hand
off m.player directly (via handoffPlaybackMsg) which can leave two players
active; replace these direct handoffs with the safe handoff/startup pattern used
elsewhere: when ContinueOnNavigate && m.selectedStation, return a tea.Cmd (or
tea.Batch) that performs the two-step safe handoff used by startPlayback — i.e.,
send a handoff message that lets App take ownership using the station (not
blindly reuse m.player) and ensure the local model stops or nils out its
m.player after handing off so the app can stop/replace its player safely; mirror
the exact sequence and message ordering from the other safe-handoff
implementation referenced in this PR (use handoffPlaybackMsg,
backToMainMsg/startup sequence, and clear/stop m.player locally).
In `@internal/ui/top_rated.go`:
- Around line 605-632: The confirm-stop state (topRatedStateConfirmStop) is set
by handlePlayingInput but View() never renders a prompt, so add a rendering path
in the TopRatedModel.View() to detect topRatedStateConfirmStop and display a
clear confirmation prompt (e.g., "Stop playback? (y/n)") together with relevant
context (station title from m.selectedStation or current play info) and any key
hints; ensure the prompt is focused/visible so y/n key handling (already in
handlePlayingInput) is effective and that returning to other states preserves
selection and UI layout.
- Around line 384-414: playStation currently hands m.player to the App
immediately (via handoffPlaybackMsg) while TopRatedModel continues to use the
same pointer, causing shared ownership bugs; fix by relinquishing local
ownership before handing off: when ContinueOnNavigate is true, set m.player =
nil (or otherwise mark it as handed off) and ensure subsequent code paths in
TopRatedModel do not call methods on m.player after the handoff, then return the
handoffPlaybackMsg (and keep selectedStation/state updates) so App becomes the
sole owner; also ensure stopCmd still signals stopActivePlaybackMsg for the App
instead of calling m.player directly.
In `@README.md`:
- Line 23: Update the "Continue on Navigate" bullet text so it matches the
current UI: change the phrase "Now Playing bar showing on every page" to "Now
Playing bar showing on every non-player screen" and update the settings
reference from "3. Play Options" to "5. Play Options"; apply the same two edits
wherever the "Continue on Navigate" / "Now Playing bar" description appears
(including the other occurrence referenced) so the README mentions "Continue on
Navigate", "Now Playing bar", and "5. Play Options" consistently.
---
Outside diff comments:
In `@internal/ui/app.go`:
- Around line 302-355: The code reads a player's volume after Stop() and nil
assignment so activePlayer is always nil; in Cleanup() capture the volume before
calling Stop()/nil-ing the player: check a.activePlayer.GetVolume() first (if
non-nil) and fallback to a.quickFavPlayer.GetVolume(), store that into
a.playOptsCfg.LastUsedVolume when StartVolumeMode == "last_used", then call
Stop()/set nil for activePlayer and stop other players and finally call
storage.SavePlayOptionsConfigToUnified(a.playOptsCfg); update references to
activePlayer, quickFavPlayer, playOptsCfg.StartVolumeMode,
playOptsCfg.LastUsedVolume, and storage.SavePlayOptionsConfigToUnified
accordingly.
In `@internal/ui/lucky.go`:
- Around line 206-223: Switch in LuckyModel's Update is missing a case for
luckyStateConfirmStop, so key presses during the confirm-stop prompt fall
through to the default input handler; add a case in the state switch to call a
new handler (e.g., m.updateConfirmStop(msg)) and implement updateConfirmStop on
LuckyModel to handle "y"/"Y"/"1" by stopping playback (if m.player != nil call
m.player.Stop()), set m.state = luckyStateInput, m.inputMode = true, focus
m.textInput, clear m.selectedStation = nil, call m.reloadSearchHistory() and
m.rebuildMenuWithHistory(), and handle "n"/"N"/"2"/"esc" by setting m.state =
luckyStatePlaying and returning; ensure the method returns (tea.Model, tea.Cmd)
like the other update* handlers.
In `@internal/ui/search.go`:
- Around line 532-553: The 0 and "esc" branches navigate/stop immediately
without honoring m.playOptsCfg.ConfirmStop; modify both branches to first check
m.playOptsCfg.ConfirmStop and invoke the same confirmation flow used elsewhere
(the function that prompts the user and returns whether to proceed) before
navigating or handing off the player: if ConfirmStop is true, call that
confirmation routine and only continue to call m.navigateToMainCmd(),
m.handOffPlayer(), or m.player.Stop() when the user confirms; otherwise abort
and stay in the current selection state. Ensure you still nil out
m.selectedStation and set m.state appropriately only after confirmation
succeeds.
---
Nitpick comments:
In `@internal/player/mpv_test.go`:
- Around line 70-72: Remove the redundant return immediately following the
t.Fatal call in the test (the t.Fatal invocation already stops execution via
runtime.Goexit()); locate the failing assertion in internal/player/mpv_test.go
where t.Fatal("Expected current station to be set") is used and delete the
subsequent `return` statement so the test is cleaner.
In `@internal/storage/config.go`:
- Around line 176-186: Extract a shared updater function (e.g.,
UpdateUnifiedConfig(func(cfg *config.UnifiedConfig) error) error) that
encapsulates the load → mutate → save flow, preserves the non-atomic update
caveat and error handling, and accepts a mutator callback that updates a
section; then refactor SavePlayOptionsConfigToUnified (and the appearance,
connection, blocklist, shuffle, and play-history adapter functions) to call this
new UpdateUnifiedConfig and perform only the section mutation (cfg.PlayOptions =
po) inside the callback, returning any validation errors from the mutator so all
load/save/error logic is centralized.
In `@internal/ui/searchmodel_extra.go`:
- Around line 47-50: Init currently returns nil so the per-second ticker never
starts until a message triggers it; change SearchModel.Init to return the ticker
command (e.g., return tickEverySecond()) so the status countdown starts
unconditionally like other UI models—update the Init() method in the SearchModel
type to call and return tickEverySecond() (or the shared ticker helper used by
other models) instead of nil.
- Around line 316-388: The number-buffer UX inconsistency is that
SearchModel.handleMenuKey buffers single digits (numberBuffer) and requires
Enter, unlike app.go's immediate single-digit selection; implement a brief
auto-execute timeout to match user expectations: when you append a single digit
to numberBuffer in handleMenuKey, dispatch a tea.Cmd that sends a new
numberTimeoutMsg after ~700ms; add a case in SearchModel.Update (or
handleMenuKey) to handle numberTimeoutMsg by parsing numberBuffer and calling
selectByNumber (clearing numberBuffer), and ensure Enter and the existing
two-digit logic still work and that the special "0" handling and menuList update
behavior remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eba024a2-8b73-440b-8557-8946552367de
📒 Files selected for processing (40)
README.mddocs/README.mdinternal/api/client.gointernal/blocklist/manager_test.gointernal/config/config.gointernal/config/config_test.gointernal/player/mpv.gointernal/player/mpv_test.gointernal/storage/config.gointernal/storage/config_test.gointernal/storage/station_metadata_test.gointernal/storage/station_ratings_test.gointernal/storage/station_tags_test.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/appearance_settings.gointernal/ui/blocklist.gointernal/ui/browse_tags.gointernal/ui/components/stars_test.gointernal/ui/connection_settings.gointernal/ui/gist.gointernal/ui/helpers.gointernal/ui/list.gointernal/ui/lucky.gointernal/ui/messages.gointernal/ui/most_played.gointernal/ui/most_played_test.gointernal/ui/play.gointernal/ui/play_station_test.gointernal/ui/play_test.gointernal/ui/search.gointernal/ui/search_test.gointernal/ui/searchmodel_extra.gointernal/ui/settings.gointernal/ui/settings_test.gointernal/ui/shuffle_settings.gointernal/ui/sleep_summary.gointernal/ui/styles.gointernal/ui/tag_playlists.gointernal/ui/top_rated.go
Comment on lines
+644
to
+674
| // TestNowPlayingBar_HiddenOnPlayerScreens verifies that the Now Playing bar | ||
| // is not appended to the view when the current screen is a player screen. | ||
| func TestNowPlayingBar_HiddenOnPlayerScreens(t *testing.T) { | ||
| app := newContinueOnNavigateApp() | ||
| app.activeStation = newTestStation("uuid-top", "Top Rated FM") | ||
| app.activeContextLabel = "Top Rated" | ||
| app.activePlayer = app.quickFavPlayer | ||
| app.width = 80 | ||
| app.height = 24 | ||
|
|
||
| // nowPlayingBar() returns a non-empty string when activeStation is set. | ||
| if app.nowPlayingBar() == "" { | ||
| t.Skip("nowPlayingBar returned empty — player may need volume; skipping rendering check") | ||
| } | ||
|
|
||
| for _, s := range []Screen{screenTopRated, screenMostPlayed, screenPlay} { | ||
| app.screen = s | ||
| if !app.isPlayerScreen() { | ||
| t.Errorf("precondition: %v should be a player screen", s) | ||
| } | ||
| // The bar must be suppressed on this screen. | ||
| bar := app.nowPlayingBar() | ||
| if bar != "" && app.isPlayerScreen() { | ||
| // Simulate what View() does: only append when !isPlayerScreen(). | ||
| appended := !app.isPlayerScreen() | ||
| if appended { | ||
| t.Errorf("Now Playing bar should not be appended on player screen %v", s) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This regression test doesn't exercise the render path.
TestNowPlayingBar_HiddenOnPlayerScreens can skip before asserting anything, and when it does run it only re-derives !app.isPlayerScreen() locally instead of checking App.View() or the rendered output. That means it will still pass if player screens start rendering the banner again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ui/app_test.go` around lines 644 - 674, The test currently only
calls nowPlayingBar() and re-checks isPlayerScreen() locally, so it doesn't
exercise the render path; update TestNowPlayingBar_HiddenOnPlayerScreens to call
app.View() for each player screen (screenTopRated, screenMostPlayed, screenPlay)
and assert that the returned view string does NOT contain the nowPlayingBar()
output (or the specific banner substring) when app.isPlayerScreen() is true;
keep the initial skip if nowPlayingBar() returns empty, but when not skipped use
app.View() (not just re-deriving appended = !app.isPlayerScreen()) to verify the
bar is actually suppressed in the rendered output.
…order, refactor updateUnifiedConfig
…u going off-screen
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/lucky.go (1)
1707-1735:⚠️ Potential issue | 🟠 MajorKeep shuffle exit handling aligned with normal playback.
esc/0here bypassConfirmStop, and the0branch clearsm.selectedStationbeforenavigateToMainCmd(), soContinueOnNavigatecan never hand off from shuffle. Gate these exits the same way asupdatePlaying(), and build the main-menu command before clearing the station.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/lucky.go` around lines 1707 - 1735, The esc and "0" handlers bypass the same stop/hand-off gating used in updatePlaying(), clearing m.selectedStation before building navigation commands so ContinueOnNavigate can never hand off from shuffle; change both branches to mirror updatePlaying(): call/await the same ConfirmStop gating (use ConfirmStop/ContinueOnNavigate flow), build the navigation command (navigateBackCmd() or navigateToMainCmd()) before clearing m.selectedStation and before nil-ing m.shuffleManager, and only clear state (m.selectedStation, m.shuffleEnabled, m.shuffleManager) after the navigation command has been created/handed off so ContinueOnNavigate can perform the handoff.
♻️ Duplicate comments (2)
internal/ui/search.go (1)
105-110:⚠️ Potential issue | 🟠 MajorSave-to-list flow is still stubbed and non-functional.
loadAvailableLists()andsaveToList()both returnnil, but they are wired into active UI paths (s, list select, new list enter). This leaves the feature partially wired with no persistence behavior.Also applies to: 238-242, 281-283, 305-306, 573-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/search.go` around lines 105 - 110, The save-to-list flow is stubbed: implement loadAvailableLists() and saveToList() so the UI triggers ('s', list select, new list enter) actually load available lists and persist items; specifically, have loadAvailableLists() fetch list metadata from your storage/service, update the SearchModel's list fields (e.g., availableLists, listLoading flags) and return a tea.Cmd to set the model, and have saveToList(listID, item) call the persistence API (or storage layer), update model state (e.g., selectedList, saved status, error handling), and return a tea.Cmd that updates the UI on success or logs/errors on failure; ensure both functions surface errors and update model fields used by the UI so the wired keybindings work end-to-end.internal/ui/top_rated.go (1)
387-417:⚠️ Potential issue | 🟠 MajorStill handing App a player that
TopRatedModelkeeps using.
playStation()emitshandoffPlaybackMsg{player: m.player}but keeps the same pointer locally. That leaves App and Top Rated sharing oneMPVPlayer, so later local stop/replay paths can kill or reuse the app-owned stream. Follow the fresh-player handoff pattern used inPlayModel.handOffPlayer()here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/top_rated.go` around lines 387 - 417, In playStation, you hand off the same m.player pointer via handoffPlaybackMsg while TopRatedModel continues to use it—this shares a single MPVPlayer between App and TopRatedModel and causes stop/replay races; update playStation to follow the PlayModel.handOffPlayer() pattern: create a fresh player instance to hand to the app (or clone/replace m.player), send handoffPlaybackMsg with that new player and station/contextLabel "Top Rated", and ensure TopRatedModel replaces its local m.player with a new local-only player (or reinitialize its player) so App and TopRatedModel no longer share the same MPVPlayer; locate symbols playStation, m.player, handoffPlaybackMsg and PlayModel.handOffPlayer to implement the same fresh-player handoff flow.
🧹 Nitpick comments (1)
internal/player/mpv_test.go (1)
126-128: Don’t ignoreStop()errors in test setup.Swallowing the error makes failures harder to diagnose if setup behavior changes. Assert it explicitly.
Suggested patch
- _ = p.Stop() + if err := p.Stop(); err != nil { + t.Fatalf("Stop() should not fail while preparing killed state: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/player/mpv_test.go` around lines 126 - 128, The test currently ignores the error returned by p.Stop(); change the test setup to assert that p.Stop() returns nil (or handle the error explicitly) instead of discarding it. Locate the call to p.Stop() in mpv_test.go (the variable p in the test) and replace the ignored result with an assertion (e.g., require.NoError / t.Fatalf / t.Assert) so any Stop() failure fails the test and surfaces diagnostic information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/lucky.go`:
- Around line 853-888: When entering luckyStateConfirmStop (in both the "esc"
and "0" handlers) record who opened the prompt (e.g., set a field like
m.confirmStopTarget = "esc" or "0" / or a small enum) so the confirm handler can
route correctly; then update the confirmation branch to check
m.confirmStopTarget and, if it was opened from an opener that should hand off
(the Esc/ContinueOnNavigate path), call navigateBackCmd() / navigateToMainCmd()
and send the handoffPlaybackMsg (reusing the existing handoff logic that uses
m.selectedStation), otherwise perform the local stop (m.player.Stop()), clear
selectedStation, reload history and rebuild the menu and return to
luckyStateInput; make the change in both places noted (current block and the
duplicate around 2145-2162) and clear m.confirmStopTarget after handling.
In `@internal/ui/most_played_test.go`:
- Around line 47-68: Extend TestMostPlayedConfirmStopTriggered to exercise the
full confirm flow by sending a '0' key (to enter confirm) then a 'y' key (to
confirm) using m.Update twice; after the second Update assert the model is no
longer in mostPlayedStateConfirmStop and that the confirmStopTarget field has
been cleared (or the model transitioned to the expected post-confirm state),
ensuring the confirm path from the main-menu resolves properly. Use the existing
loop for keys and add a subcase that sends msg for '0' then msg for 'y',
checking result.state and m.confirmStopTarget after the confirmation.
In `@internal/ui/most_played.go`:
- Around line 939-954: In handleConfirmStopInput, ensure you reset the modal
state and clear confirmStopTarget before returning from both branches: when
m.confirmStopTarget == "main" (before returning the navigateToMainCmd()) and in
the else branch (before calling navigateBackCmd()/setting m.state =
mostPlayedStateList); set m.state = mostPlayedStateList (or
mostPlayedStateConfirmStop -> mostPlayedStateList) and m.confirmStopTarget = ""
and clear m.selectedStation as needed, then return m with the chosen cmd to
avoid leaving the stale confirm dialog when reopening MostPlayedModel.
In `@internal/ui/play.go`:
- Around line 1675-1694: The "main" branch in updateConfirmStop leaves m.state
as playStateConfirmStop and only stops the player, causing the stale confirm
prompt to persist when the model is reused; change that branch to follow the
same main-menu exit path as updatePlaying(): stop the player if non-nil, reset
m.state back to the main play state (the same identifier used by updatePlaying()
for returning to the main menu), clear m.selectedStation and m.trackHistory, and
then return the backToMainMsg so the model fully transitions to the main menu.
In `@internal/ui/search.go`:
- Around line 122-123: The new state searchStateConfirmStop is set in the state
machine but View()'s switch lacks a case to render it, causing ConfirmStop
transitions to fall through to the "Unknown state" branch; add a case for
searchStateConfirmStop inside the View() switch (the same switch that handles
searchStateSavePrompt and others) and implement rendering/handlers equivalent to
the intended confirm-stop prompt (reuse the save/confirm prompt logic or the
same UI code path used for searchStateSavePrompt) so ConfirmStop transitions
display the correct confirmation UI instead of "Unknown state".
In `@internal/ui/tag_playlists.go`:
- Around line 609-627: The code shows branches that unconditionally trigger the
stop-confirm flow when m.playOptsCfg.ConfirmStop is true; change those branches
to skip the confirm dialog when navigation should keep playback running by
checking m.playOptsCfg.ContinueOnNavigate (or equivalent) before setting
m.confirmStopTarget and m.state to tagPlaylistsStateConfirmStop. Specifically,
update the sections around the navigate-back and "0" cases (where
navigateBackCmd(), m.confirmStopTarget, tagPlaylistsStateConfirmStop,
selectedPlaylist, and loadDetailStations() are used) to: if ConfirmStop &&
!ContinueOnNavigate then set confirmStopTarget/state and return, else perform
the navigation (call navigateBackCmd(), set state/detail fields,
loadDetailStations(), and return cmd) so the dialog is not shown when continuing
playback; apply the same change to the similar block at lines ~697-706.
- Around line 810-813: The code currently ignores a persisted LastUsedVolume of
0 due to the `> 0` guard; update the logic in the start volume selection so that
when m.playOptsCfg.StartVolumeMode == "last_used" you assign startVol =
m.playOptsCfg.LastUsedVolume regardless of it being zero (i.e., remove the `> 0`
check), ensuring m.playOptsCfg.DefaultVolume remains the fallback only when
StartVolumeMode is not "last_used" or LastUsedVolume is truly unset by your
config semantics.
- Around line 715-724: When handling the confirm-stop back-path (the block that
checks m.confirmStopTarget and sets cmd = m.navigateBackCmd()), ensure you run
the same detail-list refresh that the direct Esc path does so detailStations
(membership/counts) are up to date; locate the Esc branch that refreshes
detailStations and call that same function or inline logic here (before setting
m.state = tagPlaylistsStateDetail and before returning) so the playlist view is
not stale after confirming back.
- Around line 1154-1160: The Now Playing bar should be injected based on the
model's persistent UI state rather than the transient mpv instance; in
renderPageWithBottomHelp (TagPlaylistsModel.renderPageWithBottomHelp) replace
the current condition that checks m.player (m.player == nil ||
!m.player.IsPlaying()) with a check against the model state (e.g., use
m.state.IsPlaying() or m.state.PlayerIsActive / m.state.ShowNowPlaying flag
provided by the state object) so the decision uses the app-level playback state
instead of the live mpv instance; update the condition to consult that state
accessor/flag and leave the remaining logic returning
RenderPageWithBottomHelp(layout, height) unchanged.
In `@internal/ui/top_rated.go`:
- Around line 261-272: Responses for topRatedResolvedMsg currently
unconditionally dereference GetRating(...).Rating and call playStation, which
can panic or hijack playback if the user changed selection while the lookup was
in flight; modify the handler to first verify the response corresponds to the
current pending request (e.g., compare msg.station.StationUUID to a
pendingTopRatedUUID field on m), then call m.ratingsManager.GetRating(...) and
check the returned rating is non-nil before reading .Rating and before updating
the cache or invoking m.playStation(*msg.station); clear the pendingTopRatedUUID
after handling. Apply the same guard and nil-check changes to the analogous
block at the other location (lines referenced in the review).
- Around line 661-676: In handleConfirmStopInput, when handling the "y"/"1"
branch you must reset the confirm-stop state before returning: set m.state =
topRatedStateList and clear m.confirmStopTarget (e.g., m.confirmStopTarget = "")
for both the confirmStopTarget == "main" branch (before returning the
navigateToMainCmd()) and the else branch (before calling navigateBackCmd());
keep clearing m.selectedStation as already done so the model is fully reset
prior to returning the command.
---
Outside diff comments:
In `@internal/ui/lucky.go`:
- Around line 1707-1735: The esc and "0" handlers bypass the same stop/hand-off
gating used in updatePlaying(), clearing m.selectedStation before building
navigation commands so ContinueOnNavigate can never hand off from shuffle;
change both branches to mirror updatePlaying(): call/await the same ConfirmStop
gating (use ConfirmStop/ContinueOnNavigate flow), build the navigation command
(navigateBackCmd() or navigateToMainCmd()) before clearing m.selectedStation and
before nil-ing m.shuffleManager, and only clear state (m.selectedStation,
m.shuffleEnabled, m.shuffleManager) after the navigation command has been
created/handed off so ContinueOnNavigate can perform the handoff.
---
Duplicate comments:
In `@internal/ui/search.go`:
- Around line 105-110: The save-to-list flow is stubbed: implement
loadAvailableLists() and saveToList() so the UI triggers ('s', list select, new
list enter) actually load available lists and persist items; specifically, have
loadAvailableLists() fetch list metadata from your storage/service, update the
SearchModel's list fields (e.g., availableLists, listLoading flags) and return a
tea.Cmd to set the model, and have saveToList(listID, item) call the persistence
API (or storage layer), update model state (e.g., selectedList, saved status,
error handling), and return a tea.Cmd that updates the UI on success or
logs/errors on failure; ensure both functions surface errors and update model
fields used by the UI so the wired keybindings work end-to-end.
In `@internal/ui/top_rated.go`:
- Around line 387-417: In playStation, you hand off the same m.player pointer
via handoffPlaybackMsg while TopRatedModel continues to use it—this shares a
single MPVPlayer between App and TopRatedModel and causes stop/replay races;
update playStation to follow the PlayModel.handOffPlayer() pattern: create a
fresh player instance to hand to the app (or clone/replace m.player), send
handoffPlaybackMsg with that new player and station/contextLabel "Top Rated",
and ensure TopRatedModel replaces its local m.player with a new local-only
player (or reinitialize its player) so App and TopRatedModel no longer share the
same MPVPlayer; locate symbols playStation, m.player, handoffPlaybackMsg and
PlayModel.handOffPlayer to implement the same fresh-player handoff flow.
---
Nitpick comments:
In `@internal/player/mpv_test.go`:
- Around line 126-128: The test currently ignores the error returned by
p.Stop(); change the test setup to assert that p.Stop() returns nil (or handle
the error explicitly) instead of discarding it. Locate the call to p.Stop() in
mpv_test.go (the variable p in the test) and replace the ignored result with an
assertion (e.g., require.NoError / t.Fatalf / t.Assert) so any Stop() failure
fails the test and surfaces diagnostic information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eb090b8f-90c6-4978-bbec-d02cac484d63
📒 Files selected for processing (15)
README.mddocs/README.mdinternal/player/mpv_test.gointernal/storage/config.gointernal/storage/config_test.gointernal/ui/app.gointernal/ui/browse_tags.gointernal/ui/lucky.gointernal/ui/most_played.gointernal/ui/most_played_test.gointernal/ui/play.gointernal/ui/search.gointernal/ui/search_test.gointernal/ui/tag_playlists.gointernal/ui/top_rated.go
✅ Files skipped from review due to trivial changes (4)
- README.md
- docs/README.md
- internal/storage/config_test.go
- internal/ui/browse_tags.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/storage/config.go
- internal/ui/search_test.go
- internal/ui/app.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/app.go (1)
467-503:⚠️ Potential issue | 🟠 MajorInitialize Search's favorites snapshot before using it for dedupe.
SearchModel.handlePlaybackStopped()andsaveToQuickFavorites()dedupe againstm.quickFavorites, but this setup path never seeds that slice. Search therefore treats My-favorites as empty on first use and can append duplicates immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 467 - 503, SearchModel's quickFavorites slice is never seeded in the search screen setup so dedupe in SearchModel.handlePlaybackStopped and saveToQuickFavorites fails; after creating a.searchScreen (via NewSearchModel) populate its quickFavorites with the current "My-favorites" snapshot (either by calling the SearchModel method that builds the snapshot or by loading the favorites from storage and assigning to a.searchScreen.quickFavorites) so subsequent calls to handlePlaybackStopped/saveToQuickFavorites correctly dedupe against existing entries.
♻️ Duplicate comments (5)
internal/ui/top_rated.go (2)
247-250:⚠️ Potential issue | 🟠 MajorCarry the requested UUID through
topRatedResolvedMsg.A failed lookup currently clears
pendingResolveUUIDunconditionally. If request A fails after request B has started, A wipes B's pending UUID and B's later success gets discarded as stale. Include the requested UUID in the message and ignore mismatched failures the same way you already ignore mismatched successes.Also applies to: 262-268, 476-481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/top_rated.go` around lines 247 - 250, The topRatedResolvedMsg struct must carry the requested UUID so failure messages don't clear another request's pendingResolveUUID; add a uuid field to topRatedResolvedMsg, populate it wherever a resolve response is sent (the code paths that create topRatedResolvedMsg), and in the message handler compare msg.uuid to pendingResolveUUID before clearing or acting on errors—treat mismatched failures the same as you already treat mismatched successes. Update the other occurrences of topRatedResolvedMsg creation/handling noted (the similar blocks around the other ranges) to set and check the uuid consistently.
418-425:⚠️ Potential issue | 🟠 MajorDon't swap
m.playerout while Top Rated is still in the playing state.With
ContinueOnNavigateenabled,playStation()hands the live player to App immediately and installs a fresh idle player, but the screen stays intopRatedStatePlaying. From there, back/main navigation, stop/status rendering, and now-playing injection are all operating on the replacement player instead of the stream that just started.Also applies to: 584-623
internal/ui/app.go (1)
758-769:⚠️ Potential issue | 🟡 MinorAssign the computed countdown to
searchScreen.sleepCountdown.Search's view reads the integer
sleepCountdownfield, but this branch only updatesplayScreen.sleepCountdownand leaves Search at0. The sleep-timer banner on the Search now-playing screen therefore never advances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 758 - 769, In the tickMsg branch you compute a string countdown and only set a.playScreen.sleepCountdown, leaving a.searchScreen.sleepCountdown stale and mismatched (search expects an int). Convert the computed remaining seconds into an integer (e.g. totalSeconds from a.sleepTimer.Remaining()), set a.searchScreen.sleepCountdown = totalSeconds, and keep/update a.playScreen.sleepCountdown (formatted string) as before; use the same a.sleepTimer.Remaining() call and the tickMsg handler to locate the code to change.internal/ui/tag_playlists.go (1)
561-600:⚠️ Potential issue | 🟠 MajorUse the safe handoff/startup sequence here too.
navigateBackCmd()/navigateToMainCmd()hand Appm.player, butstartPlayback()later reuses that same pointer and never stops an existingactivePlayer. Starting from Tag Playlists can therefore either overlap with a stream from another screen or retune the app-owned background stream without updating App state.Also applies to: 811-829
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/tag_playlists.go` around lines 561 - 600, navigateBackCmd and navigateToMainCmd are directly handing off m.player (via handoffPlaybackMsg) while startPlayback expects App to manage activePlayer, causing potential overlapping streams or lost state; change both functions to use the same safe handoff/startup sequence as other screens: instead of sending m.player directly, ensure you stop local player and send a handoff that triggers App's controlled startup (or send a message that instructs App to perform the handoff/start sequence), i.e., update navigateBackCmd and navigateToMainCmd to stop m.player when appropriate (or null it) and use the established startup/handoff helper flow used elsewhere (referencing navigateBackCmd, navigateToMainCmd, handoffPlaybackMsg, startPlayback, activePlayer, player, selectedStation, playOptsCfg) so App always creates/attaches the activePlayer rather than reusing a screen-local pointer.internal/ui/most_played.go (1)
447-487:⚠️ Potential issue | 🟠 MajorRelinquish
m.playerwhen handing playback to App.These helpers give App the current player, but the model keeps the same pointer when it drops back to the list. The next
Enterreuses the app-owned player and the follow-upstopActivePlaybackMsgstops the new stream; the list view also suppresses the app now-playing bar becausem.player.IsPlaying()stays true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/most_played.go` around lines 447 - 487, In both navigateBackCmd and navigateToMainCmd, relinquish ownership of m.player when creating a handoffPlaybackMsg: capture the current player into a local variable (e.g., p := m.player), then in the returned cmd closure set m.player = nil and return handoffPlaybackMsg{player: p, station: station, contextLabel: "Most Played"} (for navigateToMainCmd do the same inside the first Batch func). This ensures the model no longer retains the player pointer after handing playback to App.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/app.go`:
- Around line 800-813: The handler for handoffPlaybackMsg should not set
playingFromMain (or playingStation) because that causes main-menu controls to
target the idle quickFavPlayer; instead, stop setting a.playingFromMain = true
in the handoffPlaybackMsg case and ensure main-menu control logic reads from
a.activePlayer when non-nil (update the control handlers that currently
reference quickFavPlayer to prefer a.activePlayer, or add a small accessor that
returns activePlayer if present). Keep the existing Stop()/assignment of
a.activePlayer, a.activeStation, and a.activeContextLabel but remove routing
through playingFromMain so pause/volume/Esc operate on a.activePlayer rather
than quickFavPlayer.
In `@internal/ui/searchmodel_extra.go`:
- Around line 152-160: In the saveSuccessMsg branch, you are updating
m.saveMessage but calling m.reloadSearchHistory(), which leaves m.quickFavorites
stale; replace that call with the routine that refreshes the Quick Favorites
cache (e.g., call m.reloadQuickFavorites() if it exists) or add/rename a
function that rebuilds m.quickFavorites and invoke it here so duplicate checks
see the newly saved station (refer to the saveSuccessMsg case,
m.saveMessage/m.saveMessageTime, and m.quickFavorites).
---
Outside diff comments:
In `@internal/ui/app.go`:
- Around line 467-503: SearchModel's quickFavorites slice is never seeded in the
search screen setup so dedupe in SearchModel.handlePlaybackStopped and
saveToQuickFavorites fails; after creating a.searchScreen (via NewSearchModel)
populate its quickFavorites with the current "My-favorites" snapshot (either by
calling the SearchModel method that builds the snapshot or by loading the
favorites from storage and assigning to a.searchScreen.quickFavorites) so
subsequent calls to handlePlaybackStopped/saveToQuickFavorites correctly dedupe
against existing entries.
---
Duplicate comments:
In `@internal/ui/app.go`:
- Around line 758-769: In the tickMsg branch you compute a string countdown and
only set a.playScreen.sleepCountdown, leaving a.searchScreen.sleepCountdown
stale and mismatched (search expects an int). Convert the computed remaining
seconds into an integer (e.g. totalSeconds from a.sleepTimer.Remaining()), set
a.searchScreen.sleepCountdown = totalSeconds, and keep/update
a.playScreen.sleepCountdown (formatted string) as before; use the same
a.sleepTimer.Remaining() call and the tickMsg handler to locate the code to
change.
In `@internal/ui/most_played.go`:
- Around line 447-487: In both navigateBackCmd and navigateToMainCmd, relinquish
ownership of m.player when creating a handoffPlaybackMsg: capture the current
player into a local variable (e.g., p := m.player), then in the returned cmd
closure set m.player = nil and return handoffPlaybackMsg{player: p, station:
station, contextLabel: "Most Played"} (for navigateToMainCmd do the same inside
the first Batch func). This ensures the model no longer retains the player
pointer after handing playback to App.
In `@internal/ui/tag_playlists.go`:
- Around line 561-600: navigateBackCmd and navigateToMainCmd are directly
handing off m.player (via handoffPlaybackMsg) while startPlayback expects App to
manage activePlayer, causing potential overlapping streams or lost state; change
both functions to use the same safe handoff/startup sequence as other screens:
instead of sending m.player directly, ensure you stop local player and send a
handoff that triggers App's controlled startup (or send a message that instructs
App to perform the handoff/start sequence), i.e., update navigateBackCmd and
navigateToMainCmd to stop m.player when appropriate (or null it) and use the
established startup/handoff helper flow used elsewhere (referencing
navigateBackCmd, navigateToMainCmd, handoffPlaybackMsg, startPlayback,
activePlayer, player, selectedStation, playOptsCfg) so App always
creates/attaches the activePlayer rather than reusing a screen-local pointer.
In `@internal/ui/top_rated.go`:
- Around line 247-250: The topRatedResolvedMsg struct must carry the requested
UUID so failure messages don't clear another request's pendingResolveUUID; add a
uuid field to topRatedResolvedMsg, populate it wherever a resolve response is
sent (the code paths that create topRatedResolvedMsg), and in the message
handler compare msg.uuid to pendingResolveUUID before clearing or acting on
errors—treat mismatched failures the same as you already treat mismatched
successes. Update the other occurrences of topRatedResolvedMsg creation/handling
noted (the similar blocks around the other ranges) to set and check the uuid
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a5a59844-a699-420f-953e-8aba67ffe05d
📒 Files selected for processing (10)
internal/player/mpv_test.gointernal/ui/app.gointernal/ui/lucky.gointernal/ui/most_played.gointernal/ui/most_played_test.gointernal/ui/play.gointernal/ui/search.gointernal/ui/searchmodel_extra.gointernal/ui/tag_playlists.gointernal/ui/top_rated.go
✅ Files skipped from review due to trivial changes (2)
- internal/ui/play.go
- internal/ui/most_played_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/player/mpv_test.go
- internal/ui/lucky.go
Comment on lines
+800
to
+813
| case handoffPlaybackMsg: | ||
| // A play screen is navigating away with ContinueOnNavigate=true. | ||
| // Stop any previously handed-off player before accepting the new one. | ||
| if a.activePlayer != nil && a.activePlayer != msg.player { | ||
| _ = a.activePlayer.Stop() | ||
| } | ||
| a.activePlayer = msg.player | ||
| a.activeStation = msg.station | ||
| a.activeContextLabel = msg.contextLabel | ||
| // Set these so Now Playing bar and controls work from main menu | ||
| a.playingFromMain = true | ||
| if msg.station != nil { | ||
| a.playingStation = msg.station | ||
| } |
There was a problem hiding this comment.
Don't route screen handoffs through playingFromMain.
The main-menu playback controls still operate on quickFavPlayer, not activePlayer. Setting playingFromMain here makes the main menu advertise pause/volume/Esc controls for a handed-off stream, but those keys hit the idle Quick Play player and leave the handed-off audio running.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ui/app.go` around lines 800 - 813, The handler for
handoffPlaybackMsg should not set playingFromMain (or playingStation) because
that causes main-menu controls to target the idle quickFavPlayer; instead, stop
setting a.playingFromMain = true in the handoffPlaybackMsg case and ensure
main-menu control logic reads from a.activePlayer when non-nil (update the
control handlers that currently reference quickFavPlayer to prefer
a.activePlayer, or add a small accessor that returns activePlayer if present).
Keep the existing Stop()/assignment of a.activePlayer, a.activeStation, and
a.activeContextLabel but remove routing through playingFromMain so
pause/volume/Esc operate on a.activePlayer rather than quickFavPlayer.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/ui/app.go (1)
305-319: Consider validatinglastVolrange before persisting.The code saves
lastVolwhen> 0, butGetVolume()can return values up to 100. While this is likely fine, consider also capping at 100 or validating against the expected range to ensure the persisted value is always valid.💡 Optional: Add range validation
if lastVol > 0 { + if lastVol > 100 { + lastVol = 100 + } a.playOptsCfg.LastUsedVolume = lastVol _ = storage.SavePlayOptionsConfigToUnified(a.playOptsCfg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 305 - 319, Validate and clamp the volume read from activePlayer.GetVolume() or quickFavPlayer.GetVolume() before persisting: after computing lastVol, ensure it falls within the expected 0–100 range (e.g., if lastVol < 0 set to 0; if lastVol > 100 set to 100) and only then assign a.playOptsCfg.LastUsedVolume and call storage.SavePlayOptionsConfigToUnified; update the code around the StartVolumeMode == "last_used" block to perform this range check/clamp prior to saving.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/app.go`:
- Around line 1565-1589: stopScreenPlayers is stopping the newly created
quickFavPlayer (a.quickFavPlayer) which is idle and causes its Stop() to mark it
killed so subsequent Play() calls fail; change stopScreenPlayers to either skip
a.quickFavPlayer entirely (treat it as app-level, not screen-owned) or only call
Stop() on quickFavPlayer when it IsPlaying() (use p.IsPlaying())—mirror the
behavior used in stopAllPlayback for screen players and ensure you still skip
any player equal to a.activePlayer.
In `@internal/ui/searchmodel_extra.go`:
- Around line 452-460: In the Enter key handler (the "enter" case in
searchmodel_extra.go) stop any handed-off player before starting the new stream
by emitting a stopActivePlaybackMsg prior to calling m.playStation(station);
update the return to combine the stop message and the play command (e.g., return
m, tea.Batch(stopActivePlaybackMsg{}, m.playStation(station)) or otherwise
sequence the stop command before the play command) while keeping
m.selectedStation and m.state = searchStatePlaying unchanged.
---
Nitpick comments:
In `@internal/ui/app.go`:
- Around line 305-319: Validate and clamp the volume read from
activePlayer.GetVolume() or quickFavPlayer.GetVolume() before persisting: after
computing lastVol, ensure it falls within the expected 0–100 range (e.g., if
lastVol < 0 set to 0; if lastVol > 100 set to 100) and only then assign
a.playOptsCfg.LastUsedVolume and call storage.SavePlayOptionsConfigToUnified;
update the code around the StartVolumeMode == "last_used" block to perform this
range check/clamp prior to saving.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b61df7c7-35da-42b5-ac1c-13f0302ef3e4
📒 Files selected for processing (7)
internal/ui/app.gointernal/ui/app_test.gointernal/ui/most_played.gointernal/ui/search.gointernal/ui/searchmodel_extra.gointernal/ui/tag_playlists.gointernal/ui/top_rated.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/ui/top_rated.go
- internal/ui/tag_playlists.go
- internal/ui/search.go
- internal/ui/app_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/ui/searchmodel_extra.go (1)
453-460:⚠️ Potential issue | 🟠 MajorStop handed-off playback before starting a new result stream.
At Line 459, playback starts directly. If app-level handed-off playback is still active, this can leave previous audio running. Emit
stopActivePlaybackMsgbeforem.playStation(station).💡 Proposed fix
case "enter": if item, ok := m.resultsList.SelectedItem().(stationListItem); ok { station := item.station m.selectedStation = &station m.state = searchStatePlaying if m.player != nil { - return m, m.playStation(station) + return m, tea.Batch( + func() tea.Msg { return stopActivePlaybackMsg{} }, + m.playStation(station), + ) } return m, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/searchmodel_extra.go` around lines 453 - 460, When handling the "enter" case where you extract station := item.station and call m.playStation(station), stop any app-level handed-off playback first by emitting stopActivePlaybackMsg before invoking m.playStation; update the branch that checks if m.player != nil (and the path that falls through to playStation) to send stopActivePlaybackMsg (e.g., via returning m, stopActivePlaybackMsg{} or enqueuing the message) immediately prior to calling m.playStation(station) so the previous audio is stopped before starting the new stream.
🧹 Nitpick comments (1)
internal/ui/searchmodel_extra.go (1)
127-131: Avoid an always-on 1s ticker when no countdown is active.Line 302 reschedules
tickEverySecond()unconditionally, so once started it keeps running forever. Gate rescheduling to active countdown state only.♻️ Proposed adjustment
case tickMsg: if m.saveMessageTime > 0 { m.saveMessageTime-- if m.saveMessageTime == 0 { m.saveMessage = "" + return m, nil } + return m, tickEverySecond() } - return m, tickEverySecond() + return m, nilBased on learnings: Ensure the message timer mechanism for BlocklistModel and similar UI models (PlayModel, SearchModel, LuckyModel) decrements on every incoming message instead of relying on a periodic tick. Apply this pattern broadly to all Go files under
internal/uito keep timer semantics consistent across UI models.Also applies to: 295-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/searchmodel_extra.go` around lines 127 - 131, The playbackStartedMsg branch currently always reschedules tickEverySecond() causing a perpetual 1s ticker; change the logic in the playbackStartedMsg case so you only call tickEverySecond() when the countdown is active (i.e. m.saveMessageTime > 0), and otherwise return nil to avoid starting the ticker; mirror this pattern (use incoming-message-driven decrement and only start the per-second ticker when an active saveMessageTime/countdown exists) across the UI models (e.g. BlocklistModel, PlayModel, SearchModel, LuckyModel) so all internal/ui files use the same guarded tick scheduling and decrement timers on incoming messages instead of unconditionally starting a periodic ticker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/ui/searchmodel_extra.go`:
- Around line 453-460: When handling the "enter" case where you extract station
:= item.station and call m.playStation(station), stop any app-level handed-off
playback first by emitting stopActivePlaybackMsg before invoking m.playStation;
update the branch that checks if m.player != nil (and the path that falls
through to playStation) to send stopActivePlaybackMsg (e.g., via returning m,
stopActivePlaybackMsg{} or enqueuing the message) immediately prior to calling
m.playStation(station) so the previous audio is stopped before starting the new
stream.
---
Nitpick comments:
In `@internal/ui/searchmodel_extra.go`:
- Around line 127-131: The playbackStartedMsg branch currently always
reschedules tickEverySecond() causing a perpetual 1s ticker; change the logic in
the playbackStartedMsg case so you only call tickEverySecond() when the
countdown is active (i.e. m.saveMessageTime > 0), and otherwise return nil to
avoid starting the ticker; mirror this pattern (use incoming-message-driven
decrement and only start the per-second ticker when an active
saveMessageTime/countdown exists) across the UI models (e.g. BlocklistModel,
PlayModel, SearchModel, LuckyModel) so all internal/ui files use the same
guarded tick scheduling and decrement timers on incoming messages instead of
unconditionally starting a periodic ticker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9ebe0ece-943a-4837-9182-6e47adca2e53
📒 Files selected for processing (2)
internal/ui/app.gointernal/ui/searchmodel_extra.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ui/app.go
Only re-schedule the tick when saveMessageTime > 0 or sleepTimerActive. Fix all startTick guards to use <= 0 && !sleepTimerActive, covering the persistent message state (-1). Emit stopActivePlaybackMsg before starting a new search result stream.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/ui/search.go (1)
1037-1043: Verify"✓"prefix covers all success cases in the playing view.The success message classification changed from checking multiple substrings to primarily checking for
"✓". Ensure all success messages in this flow use the"✓"prefix; otherwise they'll fall through toerrorStyle().Looking at the codebase, messages like
"Volume: %d%%"don't contain"✓"or"Already", so they would render witherrorStyle(). However, this appears intentional since volume messages are handled separately insearchStateResults(line 971-977) which has different styling logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/search.go` around lines 1037 - 1043, The current rendering logic for save messages in the playing view checks only for a "✓" substring or "Already" to choose successStyle() or infoStyle(), otherwise using errorStyle(); update this by either (A) ensuring all success messages set m.saveMessage with a leading "✓" before reaching the rendering code (standardize message creators that set m.saveMessage), or (B) broaden the condition in the rendering block that checks m.saveMessage (the code using successStyle(), infoStyle(), errorStyle()) to also detect other known success prefixes/markers used elsewhere (e.g., "Volume:", "Saved", etc.) or delegate to the same classification used by searchStateResults so volume/status messages don't mistakenly render as errors. Ensure you reference and update the producers of m.saveMessage or the rendering condition so all intended success messages get successStyle().internal/ui/searchmodel_extra.go (1)
324-336: Timer decrement relies on tick, not every message.The
tickMsghandler decrementssaveMessageTimeonly on tick events. Based on the retrieved learning, the timer mechanism should decrement on every incoming message for consistency with other UI models (BlocklistModel, PlayModel, LuckyModel).However, since
tickEverySecond()fires reliably every second, this implementation provides more predictable timeout behavior (exactly N seconds) rather than being dependent on user activity. This trade-off may be intentional for user experience consistency.If the codebase convention is to decrement on every message, consider aligning this handler with that pattern. Otherwise, the current tick-based approach is functionally correct.
Based on learnings: "Ensure the message timer mechanism for BlocklistModel and similar UI models (PlayModel, SearchModel, LuckyModel) decrements on every incoming message instead of relying on a periodic tick."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/searchmodel_extra.go` around lines 324 - 336, The tickMsg case currently decrements m.saveMessageTime only on tick events; align with the project convention by moving the decrement-and-clear logic to run on every incoming message (i.e., in the general message-handling paths of the SearchModel Update) rather than only in tickMsg. Specifically, ensure m.saveMessageTime is decremented and when it reaches 0 clear m.saveMessage and stop the timer, update all message-handling branches to perform the decrement check, and leave tickMsg (and tickEverySecond()) to only drive periodic ticks (or keep tick-based fallback if desired) so behavior matches BlocklistModel / PlayModel / LuckyModel conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/play.go`:
- Around line 1407-1424: RenderStationDetailsWithRating is currently called
unconditionally; move that call inside the ShowMetadata guard so station details
follow the documented ShowMetadata behavior. Specifically, wrap or relocate the
call to RenderStationDetailsWithRating(*m.selectedStation, hasVoted, metadata,
rating, m.starRenderer) so it executes only when m.playOptsCfg.ShowMetadata is
true (same pattern as in most_played.go), preserving the existing
playback-status rendering (m.player.IsPlaying(), m.player.GetCachedTrack(),
IsValidTrackMetadata, etc.) and using m.selectedStation for context.
---
Nitpick comments:
In `@internal/ui/search.go`:
- Around line 1037-1043: The current rendering logic for save messages in the
playing view checks only for a "✓" substring or "Already" to choose
successStyle() or infoStyle(), otherwise using errorStyle(); update this by
either (A) ensuring all success messages set m.saveMessage with a leading "✓"
before reaching the rendering code (standardize message creators that set
m.saveMessage), or (B) broaden the condition in the rendering block that checks
m.saveMessage (the code using successStyle(), infoStyle(), errorStyle()) to also
detect other known success prefixes/markers used elsewhere (e.g., "Volume:",
"Saved", etc.) or delegate to the same classification used by searchStateResults
so volume/status messages don't mistakenly render as errors. Ensure you
reference and update the producers of m.saveMessage or the rendering condition
so all intended success messages get successStyle().
In `@internal/ui/searchmodel_extra.go`:
- Around line 324-336: The tickMsg case currently decrements m.saveMessageTime
only on tick events; align with the project convention by moving the
decrement-and-clear logic to run on every incoming message (i.e., in the general
message-handling paths of the SearchModel Update) rather than only in tickMsg.
Specifically, ensure m.saveMessageTime is decremented and when it reaches 0
clear m.saveMessage and stop the timer, update all message-handling branches to
perform the decrement check, and leave tickMsg (and tickEverySecond()) to only
drive periodic ticks (or keep tick-based fallback if desired) so behavior
matches BlocklistModel / PlayModel / LuckyModel conventions.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 8cfb3a25-974c-48fc-9cfc-533530e1d372
📒 Files selected for processing (4)
internal/ui/app.gointernal/ui/play.gointernal/ui/search.gointernal/ui/searchmodel_extra.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ui/app.go
…onDetailsWithRating on ShowMetadata in play view
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/search.go`:
- Around line 393-396: The code that sets startVol ignores legitimately saved
mute (0) because it checks m.playOptsCfg.LastUsedVolume > 0; change the
condition to accept 0 (e.g., if m.playOptsCfg.StartVolumeMode == "last_used" &&
m.playOptsCfg.LastUsedVolume >= 0) so a persisted 0 is applied, or alternatively
introduce a sentinel (e.g., LastUsedVolume == -1 means unset) and update the
check accordingly; adjust any serialization/initialization of
playOptsCfg.LastUsedVolume to use the chosen sentinel if you go that route and
update uses of startVol, DefaultVolume, StartVolumeMode and LastUsedVolume to
match.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: d6fbea15-5e7e-414a-9cf3-d28e5708385c
📒 Files selected for processing (2)
internal/ui/play.gointernal/ui/search.go
✅ Files skipped from review due to trivial changes (1)
- internal/ui/play.go
Comment on lines
+393
to
+396
| startVol := m.playOptsCfg.DefaultVolume | ||
| if m.playOptsCfg.StartVolumeMode == "last_used" && m.playOptsCfg.LastUsedVolume > 0 { | ||
| startVol = m.playOptsCfg.LastUsedVolume | ||
| } |
There was a problem hiding this comment.
Volume of 0 is ignored when StartVolumeMode is "last_used".
The condition m.playOptsCfg.LastUsedVolume > 0 prevents a legitimately saved volume of 0 (muted) from being applied. If a user sets volume to 0 and expects it to persist via last_used mode, the default volume will be used instead.
Consider using a sentinel value (e.g., -1) to indicate "unset" or changing the condition to >= 0 if 0 is a valid persisted volume:
💡 Suggested fix if 0 should be a valid last-used volume
- if m.playOptsCfg.StartVolumeMode == "last_used" && m.playOptsCfg.LastUsedVolume > 0 {
+ if m.playOptsCfg.StartVolumeMode == "last_used" && m.playOptsCfg.LastUsedVolume >= 0 {
startVol = m.playOptsCfg.LastUsedVolume
}📝 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.
Suggested change
| startVol := m.playOptsCfg.DefaultVolume | |
| if m.playOptsCfg.StartVolumeMode == "last_used" && m.playOptsCfg.LastUsedVolume > 0 { | |
| startVol = m.playOptsCfg.LastUsedVolume | |
| } | |
| startVol := m.playOptsCfg.DefaultVolume | |
| if m.playOptsCfg.StartVolumeMode == "last_used" && m.playOptsCfg.LastUsedVolume >= 0 { | |
| startVol = m.playOptsCfg.LastUsedVolume | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ui/search.go` around lines 393 - 396, The code that sets startVol
ignores legitimately saved mute (0) because it checks
m.playOptsCfg.LastUsedVolume > 0; change the condition to accept 0 (e.g., if
m.playOptsCfg.StartVolumeMode == "last_used" && m.playOptsCfg.LastUsedVolume >=
0) so a persisted 0 is applied, or alternatively introduce a sentinel (e.g.,
LastUsedVolume == -1 means unset) and update the check accordingly; adjust any
serialization/initialization of playOptsCfg.LastUsedVolume to use the chosen
sentinel if you go that route and update uses of startVol, DefaultVolume,
StartVolumeMode and LastUsedVolume to match.
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
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.
Closes #126 #120
📑 Description
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
x), and troubleshooting guidance.