Add rename playlist to the playlist manager#239
Open
abdimed wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces playlist renaming capability to the music player. The feature adds a provider interface contract, backend implementation with validation, UI state tracking, input handling via the ChangesPlaylist Rename Feature
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 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 `@external/local/provider.go`:
- Around line 363-366: The code checks os.Stat(newPath) but ignores errors other
than "nil" and then calls os.Rename without wrapping errors; change the logic so
that after calling os.Stat(newPath) you return a wrapped error if stat failed
for any reason other than os.IsNotExist(err) (e.g. return fmt.Errorf("stat %q:
%w", newPath, err)); keep the existing check that returns fmt.Errorf("playlist
%q already exists", newName) when stat == nil; then call os.Rename(oldPath,
newPath) and if it returns an error wrap and return it (e.g. return
fmt.Errorf("rename %q -> %q: %w", oldPath, newPath, err)). Ensure you reference
newPath, newName, oldPath, os.Stat and os.Rename when making the changes.
In `@ui/model/keys.go`:
- Around line 1761-1764: After a successful rename in the else branch, update
the active playlist reference so future name-based ops work: set
m.loadedPlaylist to the new name (use newName) before calling
m.plMgrRefreshList(); ensure you only overwrite m.loadedPlaylist when the
renamed playlist was the currently loaded one (compare to
m.plManager.renameOldName if necessary) so other state isn't clobbered.
- Around line 1474-1481: In the "r" key handler branch, prevent entering rename
mode for the reserved history playlist by checking the real playlist before
setting rename state: after computing realIdx with
m.plMgrPlaylistRealIndex(m.plManager.cursor) and retrieving pl :=
m.plManager.playlists[realIdx], return/do nothing if that playlist is the
reserved history (e.g. pl.IsReserved || pl.Type == PlaylistTypeHistory ||
pl.Name == reservedHistoryName depending on how the playlist marks it); only set
m.plManager.renameOldName, m.plManager.renameName and m.plManager.screen =
plMgrScreenRename when the playlist is not the reserved history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01b8846f-118a-490a-b861-f4caeb23e026
📒 Files selected for processing (9)
docs/keybindings.mdexternal/local/provider.goprovider/interfaces.gosite/index.htmlui/model/keys.goui/model/model.goui/model/overlays.goui/model/state.goui/model/view_overlays.go
Comment on lines
+363
to
+366
| if _, err := os.Stat(newPath); err == nil { | ||
| return fmt.Errorf("playlist %q already exists", newName) | ||
| } | ||
| return os.Rename(oldPath, newPath) |
There was a problem hiding this comment.
Handle unexpected destination stat errors and wrap rename failures.
If os.Stat(newPath) fails for reasons other than non-existence, the code currently continues and can surface a misleading error later. Also, os.Rename should be wrapped with context.
Proposed fix
if _, err := os.Stat(newPath); err == nil {
return fmt.Errorf("playlist %q already exists", newName)
+ } else if !errors.Is(err, os.ErrNotExist) {
+ return fmt.Errorf("stat destination playlist %q: %w", newName, err)
}
- return os.Rename(oldPath, newPath)
+ if err := os.Rename(oldPath, newPath); err != nil {
+ return fmt.Errorf("rename playlist %q to %q: %w", oldName, newName, err)
+ }
+ return nil
}As per coding guidelines, "Error handling must wrap errors with fmt.Errorf("context: %w", err)."
📝 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
| if _, err := os.Stat(newPath); err == nil { | |
| return fmt.Errorf("playlist %q already exists", newName) | |
| } | |
| return os.Rename(oldPath, newPath) | |
| if _, err := os.Stat(newPath); err == nil { | |
| return fmt.Errorf("playlist %q already exists", newName) | |
| } else if !errors.Is(err, os.ErrNotExist) { | |
| return fmt.Errorf("stat destination playlist %q: %w", newName, err) | |
| } | |
| if err := os.Rename(oldPath, newPath); err != nil { | |
| return fmt.Errorf("rename playlist %q to %q: %w", oldName, newName, err) | |
| } | |
| return nil |
🤖 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 `@external/local/provider.go` around lines 363 - 366, The code checks
os.Stat(newPath) but ignores errors other than "nil" and then calls os.Rename
without wrapping errors; change the logic so that after calling os.Stat(newPath)
you return a wrapped error if stat failed for any reason other than
os.IsNotExist(err) (e.g. return fmt.Errorf("stat %q: %w", newPath, err)); keep
the existing check that returns fmt.Errorf("playlist %q already exists",
newName) when stat == nil; then call os.Rename(oldPath, newPath) and if it
returns an error wrap and return it (e.g. return fmt.Errorf("rename %q -> %q:
%w", oldPath, newPath, err)). Ensure you reference newPath, newName, oldPath,
os.Stat and os.Rename when making the changes.
Comment on lines
+1474
to
+1481
| case "r": | ||
| realIdx := m.plMgrPlaylistRealIndex(m.plManager.cursor) | ||
| if realIdx >= 0 { | ||
| name := m.plManager.playlists[realIdx].Name | ||
| m.plManager.renameOldName = name | ||
| m.plManager.renameName = name | ||
| m.plManager.screen = plMgrScreenRename | ||
| } |
There was a problem hiding this comment.
Prevent entering rename mode for the reserved history playlist.
The current flow opens the rename screen for the reserved playlist and only fails on submit. This should be blocked at key handling time to match the feature contract.
Proposed fix
case "r":
realIdx := m.plMgrPlaylistRealIndex(m.plManager.cursor)
if realIdx >= 0 {
name := m.plManager.playlists[realIdx].Name
+ if name == history.PlaylistName {
+ m.status.Show("Recently Played cannot be renamed", statusTTLDefault)
+ return nil
+ }
m.plManager.renameOldName = name
m.plManager.renameName = name
m.plManager.screen = plMgrScreenRename
}📝 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
| case "r": | |
| realIdx := m.plMgrPlaylistRealIndex(m.plManager.cursor) | |
| if realIdx >= 0 { | |
| name := m.plManager.playlists[realIdx].Name | |
| m.plManager.renameOldName = name | |
| m.plManager.renameName = name | |
| m.plManager.screen = plMgrScreenRename | |
| } | |
| case "r": | |
| realIdx := m.plMgrPlaylistRealIndex(m.plManager.cursor) | |
| if realIdx >= 0 { | |
| name := m.plManager.playlists[realIdx].Name | |
| if name == history.PlaylistName { | |
| m.status.Show("Recently Played cannot be renamed", statusTTLDefault) | |
| return nil | |
| } | |
| m.plManager.renameOldName = name | |
| m.plManager.renameName = name | |
| m.plManager.screen = plMgrScreenRename | |
| } |
🤖 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 `@ui/model/keys.go` around lines 1474 - 1481, In the "r" key handler branch,
prevent entering rename mode for the reserved history playlist by checking the
real playlist before setting rename state: after computing realIdx with
m.plMgrPlaylistRealIndex(m.plManager.cursor) and retrieving pl :=
m.plManager.playlists[realIdx], return/do nothing if that playlist is the
reserved history (e.g. pl.IsReserved || pl.Type == PlaylistTypeHistory ||
pl.Name == reservedHistoryName depending on how the playlist marks it); only set
m.plManager.renameOldName, m.plManager.renameName and m.plManager.screen =
plMgrScreenRename when the playlist is not the reserved history.
Comment on lines
+1761
to
+1764
| } else { | ||
| m.status.Showf(statusTTLDefault, "Renamed %q to %q", m.plManager.renameOldName, newName) | ||
| m.plMgrRefreshList() | ||
| } |
There was a problem hiding this comment.
Update loadedPlaylist after a successful rename.
After renaming, m.loadedPlaylist can still point to the old name, which breaks later name-based operations for the active local playlist.
Proposed fix
} else {
m.status.Showf(statusTTLDefault, "Renamed %q to %q", m.plManager.renameOldName, newName)
+ if m.loadedPlaylist == m.plManager.renameOldName {
+ m.loadedPlaylist = newName
+ }
m.plMgrRefreshList()
}📝 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
| } else { | |
| m.status.Showf(statusTTLDefault, "Renamed %q to %q", m.plManager.renameOldName, newName) | |
| m.plMgrRefreshList() | |
| } | |
| } else { | |
| m.status.Showf(statusTTLDefault, "Renamed %q to %q", m.plManager.renameOldName, newName) | |
| if m.loadedPlaylist == m.plManager.renameOldName { | |
| m.loadedPlaylist = newName | |
| } | |
| m.plMgrRefreshList() | |
| } |
🤖 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 `@ui/model/keys.go` around lines 1761 - 1764, After a successful rename in the
else branch, update the active playlist reference so future name-based ops work:
set m.loadedPlaylist to the new name (use newName) before calling
m.plMgrRefreshList(); ensure you only overwrite m.loadedPlaylist when the
renamed playlist was the currently loaded one (compare to
m.plManager.renameOldName if necessary) so other state isn't clobbered.
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
Adds a rename playlist action to the TUI playlist manager overlay. Press
ron any playlist in the manager to rename it. The input is pre-populated with the current name.Entercommits,Esccancels. The reserved "Recently Played" history playlist cannot be renamed.Screenshots / video
https://github.com/user-attachments/assets/2efe4a6c-bb0d-4750-ab5a-9b468666599f
How to test
pra rename screen appears pre-filled with the current nameChecklist
make checkpassesdocs/andsite/index.htmlupdated for user-facing changesSummary by CodeRabbit
New Features
Documentation