diff --git a/architecture/node-editing.md b/architecture/node-editing.md new file mode 100644 index 0000000..b0ea195 --- /dev/null +++ b/architecture/node-editing.md @@ -0,0 +1,201 @@ +# Editable and deletable library nodes + +## Context and problem statement + +The search feature (`architecture/search.md`) introduced *creatable* nodes: +`%` under `/tidal/search` turns a typed term into a tree node holding search +results. Those nodes are currently immutable — a typo'd term can only be +abandoned, and stale terms accumulate for the lifetime of the server process. + +This feature makes such nodes *modifiable*: in the library pane, `e` renames +the selected node (for a search term: re-runs the search under the new term) +and `d` deletes it. Modifiability is a per-node capability advertised by the +provider, exactly like `is_creatable` — the TUI never hardcodes which paths +support what. + +Run autonomously per standing user instruction; every decision below records +the options considered and the rationale. + +## Assumptions + +- "Nodes that got created via `%`" are today exactly the search-term nodes + under `/tidal/search`; the design must not special-case them (playlists are + the obvious future candidate), but they are the only provider implementation + in this iteration. +- Editing means **renaming** (the title is the only user-supplied property a + node has). For a search term, the title *is* the query, so a rename re-runs + the search. +- `e`/`d` act on the **selected item in the library list** (the same selection + model as queueing), not on the currently-open node. +- Proto changes must stay wire-compatible (additive fields/rpcs only), as with + the search feature. + +## Decisions + +### D1 — Two capability flags, on the child only + +Options considered: + +1. One `is_modifiable` flag implying both rename and delete. +2. Two flags `is_editable` / `is_deletable`. +3. Flags on both `LibraryNodeChild` and `LibraryNode` (mirroring + `is_creatable`). + +**Decision: (2), child-only** — `LibraryNodeChild.is_editable = 5`, +`LibraryNodeChild.is_deletable = 6`. Future node kinds plausibly support only +one of the two (a favorites entry may be deletable but not renamable), and an +extra bool costs nothing on the wire. Unlike `is_creatable` (consumed for the +*open* node: pane hint + `%` target), edit/delete are only ever checked +against the selected **child**, so node-level copies would have no consumer — +they are left out until something reads them. + +### D2 — RPC shapes + +Options considered: + +1. One generic `UpdateLibraryNode` with optional fields + a separate delete. +2. `RenameLibraryNode(path, new_title) → node` and + `DeleteLibraryNode(path) → parent node`. + +**Decision: (2).** Rename is the only edit that exists; a generic update +message would be speculative surface area. Both responses carry the node the +TUI should display next, following `CreateLibraryNode`: + +- **Rename returns the renamed node** and the TUI navigates into it — the + same "show me the result" behavior as `%` create. The path changes on + rename (term is percent-encoded into the path), so returning the node is + also what tells the client the new path. +- **Delete returns the refreshed parent node** — the user is looking at the + parent listing when they press `d`; returning it saves a follow-up + `GetLibraryNode` and can never serve a stale cached listing. + +### D3 — Provider semantics (tidaldy) + +- `rename_lib_node(path, new_title)`: `path` must parse to + `TidalPath::SearchTerm`, else `NotSupported`. Title is trimmed; empty → + `InvalidInput`. The old term is **replaced in place** (keeps its position in + the terms list). Renaming to an already-existing term merges: the old entry + is removed, the existing one wins — the list never holds duplicates. + Renaming a term the server doesn't know (stale client cache, restart) + registers the new term — same forgiveness as `get_lib_node` on unknown + terms. Returns `get_lib_node(new_path)`. +- `delete_lib_node(path)`: `path` must parse to `TidalPath::SearchTerm`, else + `NotSupported`. Removing an unknown term succeeds silently — delete is + idempotent. Returns the refreshed parent node (`get_lib_node(/tidal/search)`). +- Both reuse the `search_terms` `RwLock` discipline: poison-tolerant, never + held across an await. +- **Queued search tracks survive rename/delete**: a queue entry + `/tidal/search//` resolves URLs/metadata from the + embedded track id alone; term registration is irrelevant to playback. + +### D4 — No delete confirmation (for now) + +**Options**: confirm prompt (`y`/`n` mini-mode) vs immediate delete. + +**Decision: immediate.** The only deletable nodes are search terms, which are +free to recreate (`%` + retype); a confirmation mode adds a third input state +for no protected value. **Open question**: when higher-value nodes (user +playlists) become deletable, a confirmation step must be revisited — noted +here so the decision is rediscovered. + +### D5 — TUI input overlay grows a purpose + +`InputState` today hardcodes creation (`parent_path` + buffer). It becomes + +```rust +InputState { purpose: InputPurpose, buffer: String } +enum InputPurpose { Create { parent_path }, Rename { path } } +``` + +- `e` opens the overlay **prefilled with the current title** (append-only + editing as before: chars push, Backspace pops); Enter sends + `MessageFromUi::RenameNode`, Esc cancels. Overlay label: `rename: ▏` + vs `new node: ▏`. +- Prefilling means "rename" degrades gracefully to "retype" — no cursor + movement is introduced in this iteration (matches the existing overlay). +- Submitting an unchanged title is sent anyway; the provider treats it as a + no-op rename and returns the node (harmless refresh). +- `d` sends `MessageFromUi::DeleteNode { path }` directly (D4). +- Both keys are silently ignored when the selected item lacks the flag, + mirroring `%` on non-creatable nodes. +- List marker: editable/deletable children render a `[e]`, `[d]` or `[ed]` + suffix in `COLOR_SECONDARY`, alongside the existing `[%]` for creatable + ones. The bindings table gains `e`/`d` in the Library scope (plain `e` and + `d` are unbound there today; `d` only exists in Queue scope), so the help + modal picks them up automatically. + +### D6 — Client cache handling + +`RpcClient` keeps `library_node_cache`. On rename: evict the **old path** and +the **parent**, insert the returned node under its new path. On delete: evict +the deleted path and the parent, insert the returned parent. (Same reasoning +as create's eviction — a stale `/tidal/search` listing would resurrect the +old term in the UI.) + +### D7 — Server plumbing + +Two new `ProviderCommand`s (`RenameLibraryNode`, `DeleteLibraryNode`) with the +established bounded(1) reply rendezvous; orchestrator routes `/tidal`-prefixed +paths to the tidal client and answers anything else `NotSupported`. gRPC error +mapping is identical to create: `NotSupported` → `failed_precondition`, +`InvalidInput` → `invalid_argument`, rest → `internal`; no internals leak into +`Status` messages. + +## Flows + +```d2 +shape: sequence_diagram +user: { shape: person } +tui: cbd-tui +server: crabidy-server +tidal: tidaldy + +user -> tui: "e on selected [ed] node" +tui -> tui: open overlay prefilled with title +user -> tui: edit text, Enter +tui -> server: RenameLibraryNode(path, new_title) +server -> tidal: rename_lib_node +tidal -> tidal: replace term in list (merge on collision) +tidal -> server: node at new path (fresh search) +server -> tui: renamed node +tui -> tui: evict old path + parent, show renamed node + +user -> tui: "d on selected [ed] node" +tui -> server: DeleteLibraryNode(path) +server -> tidal: delete_lib_node +tidal -> tidal: remove term (idempotent) +tidal -> server: refreshed parent node +server -> tui: parent node +tui -> tui: evict path + parent, show parent listing +``` + +Capability flags travel with every listing: + +```d2 +direction: right +tidaldy: { + search_arm: "get_lib_node(/tidal/search)" +} +proto: "LibraryNodeChild { is_editable=5, is_deletable=6 }" +tui: { + list: "library list: title [ed]" + keys: "e -> rename overlay\nd -> DeleteNode" +} +tidaldy.search_arm -> proto: term children flagged +proto -> tui.list: render marker +proto -> tui.keys: gate actions +``` + +## Boundaries and risks + +- **Proto**: additive only — two rpcs, two child fields (5, 6). Old clients + ignore the flags and never call the rpcs; old servers reject unknown rpcs + with `unimplemented` (tonic default), which the TUI logs without crashing. +- **Rename-to-collision** merges terms; the response node is the *existing* + term's node. The user sees the results they asked for either way. +- **Concurrent clients**: two TUIs editing the same term list race benignly — + the list is a `RwLock`-guarded Vec, every operation is atomic under the + write lock, and stale views self-heal on the next listing fetch. +- **Not in scope**: editing anything but the title; deleting non-search + nodes; confirmation UX (D4); persistent search terms (still per-process, + as shipped by the search feature). diff --git a/cbd-tui/src/app/bindings.rs b/cbd-tui/src/app/bindings.rs index 8bd99a3..86bf079 100644 --- a/cbd-tui/src/app/bindings.rs +++ b/cbd-tui/src/app/bindings.rs @@ -60,6 +60,14 @@ pub enum Action { /// /tidal/search). While the overlay is open, keys bypass this table /// entirely (see `App::handle_input_key`). LibraryCreateNode, + /// Open the input overlay prefilled with the selected item's title to + /// rename it. No-op unless the selection `is_editable` (e.g. a search + /// term, whose rename re-runs the search). + LibraryEditNode, + /// Delete the selected item. No-op unless the selection `is_deletable`. + /// Deliberately unconfirmed: today's deletable nodes (search terms) are + /// free to recreate (architecture/node-editing.md, D4). + LibraryDeleteNode, // Queue pane QueueInsertHere, QueueFirst, @@ -251,6 +259,20 @@ pub const BINDINGS: &[Binding] = &[ action: Action::LibraryCreateNode, description: "Create node here (e.g. search term)", }, + Binding { + scope: Scope::Library, + mods: KeyModifiers::NONE, + code: KeyCode::Char('e'), + action: Action::LibraryEditNode, + description: "Rename selected node (e.g. search term)", + }, + Binding { + scope: Scope::Library, + mods: KeyModifiers::NONE, + code: KeyCode::Char('d'), + action: Action::LibraryDeleteNode, + description: "Delete selected node", + }, Binding { scope: Scope::Library, mods: KeyModifiers::NONE, @@ -483,20 +505,20 @@ mod tests { ), None ); - // 'd' removes a track in the queue but is unbound in the library. + // 'c' clears the queue there but is unbound in the library. assert_eq!( lookup( UiFocus::Queue, false, - key(KeyCode::Char('d'), KeyModifiers::NONE) + key(KeyCode::Char('c'), KeyModifiers::NONE) ), - Some(Action::QueueRemoveTrack) + Some(Action::QueueClearKeepCurrent) ); assert_eq!( lookup( UiFocus::Library, false, - key(KeyCode::Char('d'), KeyModifiers::NONE) + key(KeyCode::Char('c'), KeyModifiers::NONE) ), None ); @@ -600,6 +622,44 @@ mod tests { ); } + #[test] + fn edit_and_delete_bind_in_the_library_scope_only() { + assert_eq!( + lookup( + UiFocus::Library, + false, + key(KeyCode::Char('e'), KeyModifiers::NONE) + ), + Some(Action::LibraryEditNode) + ); + // Plain 'd' renames per scope: library delete vs queue remove-track; + // Ctrl+d stays the jump (see control_must_match_exactly). + assert_eq!( + lookup( + UiFocus::Library, + false, + key(KeyCode::Char('d'), KeyModifiers::NONE) + ), + Some(Action::LibraryDeleteNode) + ); + assert_eq!( + lookup( + UiFocus::Queue, + false, + key(KeyCode::Char('d'), KeyModifiers::NONE) + ), + Some(Action::QueueRemoveTrack) + ); + assert_eq!( + lookup( + UiFocus::Queue, + false, + key(KeyCode::Char('e'), KeyModifiers::NONE) + ), + None + ); + } + #[test] fn open_help_swallows_everything_but_close() { for focus in [UiFocus::Library, UiFocus::Queue] { diff --git a/cbd-tui/src/app/library.rs b/cbd-tui/src/app/library.rs index 15fa275..8a4718c 100644 --- a/cbd-tui/src/app/library.rs +++ b/cbd-tui/src/app/library.rs @@ -53,6 +53,19 @@ impl Library { pub fn is_creatable(&self) -> bool { self.is_creatable } + /// Path and current title of the selected item, if it may be renamed + /// (`e`). `None` when nothing is selected or the item is not editable. + pub fn selected_editable(&self) -> Option<(String, String)> { + let item = self.list.get(self.list_state.selected()?)?; + item.is_editable + .then(|| (item.path.clone(), item.title.clone())) + } + /// Path of the selected item, if it may be deleted (`d`). `None` when + /// nothing is selected or the item is not deletable. + pub fn selected_deletable(&self) -> Option { + let item = self.list.get(self.list_state.selected()?)?; + item.is_deletable.then(|| item.path.clone()) + } pub fn get_selected(&self) -> Option> { if self.list.iter().any(|i| i.marked) { return Some( @@ -163,6 +176,8 @@ impl Library { marked: false, is_queable: true, is_creatable: false, + is_editable: false, + is_deletable: false, }) .chain(node.children.iter().map(|c| UiItem { path: c.path.clone(), @@ -171,6 +186,8 @@ impl Library { marked: false, is_queable: c.is_queable, is_creatable: c.is_creatable, + is_editable: c.is_editable, + is_deletable: c.is_deletable, })) .collect(); @@ -190,11 +207,22 @@ impl Library { if i.is_creatable { text.push_str(" [%]"); } + // Modifiable items advertise their keys: [e], [d] or [ed]. + if i.is_editable || i.is_deletable { + text.push_str(" ["); + if i.is_editable { + text.push('e'); + } + if i.is_deletable { + text.push('d'); + } + text.push(']'); + } let style = if i.marked { Style::default() .fg(COLOR_GREEN) .add_modifier(Modifier::BOLD) - } else if i.is_creatable { + } else if i.is_creatable || i.is_editable || i.is_deletable { Style::default().fg(COLOR_SECONDARY) } else { Style::default() diff --git a/cbd-tui/src/app/mod.rs b/cbd-tui/src/app/mod.rs index e116801..9777726 100644 --- a/cbd-tui/src/app/mod.rs +++ b/cbd-tui/src/app/mod.rs @@ -44,6 +44,10 @@ struct UiItem { is_queable: bool, /// Children may be created under this item — rendered with a `%` marker. is_creatable: bool, + /// This item may be renamed (`e`) — part of the `[ed]` marker. + is_editable: bool, + /// This item may be deleted (`d`) — part of the `[ed]` marker. + is_deletable: bool, } pub const COLOR_PRIMARY: Color = Color::Rgb(129, 161, 193); @@ -71,6 +75,17 @@ pub enum MessageFromUi { parent_path: String, title: String, }, + /// Rename an editable node; the orchestrator navigates the library into + /// the renamed node (its path changes with the title). + RenameNode { + path: String, + new_title: String, + }, + /// Delete a deletable node; the orchestrator shows the refreshed parent + /// listing the server returns. + DeleteNode { + path: String, + }, AppendTracks(Vec), QueueTracks(Vec), InsertTracks(Vec, usize), @@ -99,13 +114,23 @@ pub enum DispatchResult { Quit, } -/// State of the one-line text input overlay (currently only node creation). +/// Why the input overlay is open — decides the submit message and the +/// overlay label. +pub enum InputPurpose { + /// `%`: create a child under the creatable node at `parent_path`. + Create { parent_path: String }, + /// `e`: rename the node at `path`. The buffer starts prefilled with the + /// current title, so "rename" degrades to "retype" (append-only editing, + /// no cursor movement). + Rename { path: String }, +} + +/// State of the one-line text input overlay (node creation and rename). /// /// While this is `Some` on [`App`], key events bypass `bindings::lookup` /// entirely and go to [`App::handle_input_key`]. pub struct InputState { - /// Creatable node the new child will be created under. - pub parent_path: String, + pub purpose: InputPurpose, /// Text typed so far. Append-only editing: chars push, Backspace pops. pub buffer: String, } @@ -142,10 +167,10 @@ impl App { /// Handles one key while the input overlay is open (`input.is_some()`). /// - /// `Esc` cancels, `Enter` submits a non-empty trimmed buffer as - /// `MessageFromUi::CreateNode` (empty submits just close), `Backspace` - /// pops, printable chars append. Everything else is ignored — the - /// bindings table is never consulted while the overlay is open. + /// `Esc` cancels, `Enter` submits a non-empty trimmed buffer as the + /// message matching [`InputPurpose`] (empty submits just close), + /// `Backspace` pops, printable chars append. Everything else is ignored — + /// the bindings table is never consulted while the overlay is open. pub fn handle_input_key(&mut self, key: crossterm::event::KeyEvent) { use crossterm::event::KeyCode; let Some(input) = self.input.as_mut() else { @@ -158,10 +183,20 @@ impl App { KeyCode::Enter => { let title = input.buffer.trim().to_string(); if !title.is_empty() { - let _ = self.tx.send(MessageFromUi::CreateNode { - parent_path: input.parent_path.clone(), - title, - }); + match &input.purpose { + InputPurpose::Create { parent_path } => { + let _ = self.tx.send(MessageFromUi::CreateNode { + parent_path: parent_path.clone(), + title, + }); + } + InputPurpose::Rename { path } => { + let _ = self.tx.send(MessageFromUi::RenameNode { + path: path.clone(), + new_title: title, + }); + } + } } self.input = None; } @@ -225,11 +260,30 @@ impl App { // creatable; silently ignored otherwise. if self.library.is_creatable() { self.input = Some(InputState { - parent_path: self.library.path().to_string(), + purpose: InputPurpose::Create { + parent_path: self.library.path().to_string(), + }, buffer: String::new(), }); } } + Action::LibraryEditNode => { + // Opens the overlay prefilled with the current title when the + // selected item is editable; silently ignored otherwise. + if let Some((path, title)) = self.library.selected_editable() { + self.input = Some(InputState { + purpose: InputPurpose::Rename { path }, + buffer: title, + }); + } + } + Action::LibraryDeleteNode => { + // Unconfirmed by design: today's deletable nodes are search + // terms, free to recreate (architecture/node-editing.md, D4). + if let Some(path) = self.library.selected_deletable() { + let _ = self.tx.send(MessageFromUi::DeleteNode { path }); + } + } Action::QueueInsertHere => { if let Some(selected) = self.queue.selected() { self.library.queue_insert(selected); @@ -283,15 +337,19 @@ impl App { self.queue.render(f, right_side[0], queue_focused); self.now_playing.render(f, right_side[1]); - // The node-creation input: one line inside the bottom of the + // The node-creation/rename input: one line inside the bottom of the // library pane, drawn over the list while open. if let Some(input) = &self.input { let area = main[0]; if area.height >= 3 && area.width >= 4 { + let label = match &input.purpose { + InputPurpose::Create { .. } => "new node", + InputPurpose::Rename { .. } => "rename", + }; let line = Rect::new(area.x + 1, area.y + area.height - 2, area.width - 2, 1); f.render_widget(Clear, line); f.render_widget( - Paragraph::new(format!("new node: {}▏", input.buffer)) + Paragraph::new(format!("{label}: {}▏", input.buffer)) .style(Style::default().fg(COLOR_SECONDARY)), line, ); @@ -415,7 +473,9 @@ mod tests { app.library.update(creatable_node("/tidal/search")); let _ = app.dispatch(Action::LibraryCreateNode); let input = app.input.as_ref().expect("input overlay open"); - assert_eq!(input.parent_path, "/tidal/search"); + assert!( + matches!(&input.purpose, InputPurpose::Create { parent_path } if parent_path == "/tidal/search") + ); assert_eq!(input.buffer, ""); } @@ -497,6 +557,126 @@ mod tests { assert!(rx.try_recv().is_err(), "whitespace-only must not create"); } + /// A /tidal/search listing with one term child, modifiable or not. + fn search_listing(modifiable: bool) -> LibraryNode { + use crabidy_core::proto::crabidy::LibraryNodeChild; + LibraryNode { + children: vec![LibraryNodeChild { + is_editable: modifiable, + is_deletable: modifiable, + ..LibraryNodeChild::new("/tidal/search/abba".to_string(), "abba".to_string(), false) + }], + ..creatable_node("/tidal/search") + } + } + + #[test] + fn edit_opens_the_overlay_prefilled_only_on_editable_selections() { + let (mut app, _rx) = app(); + // Nothing loaded: 'e' must be a no-op. + assert_eq!( + app.dispatch(Action::LibraryEditNode), + DispatchResult::Continue + ); + assert!(app.input.is_none()); + + app.library.update(search_listing(false)); + let _ = app.dispatch(Action::LibraryEditNode); + assert!(app.input.is_none(), "non-editable selection: no overlay"); + + app.library.update(search_listing(true)); + let _ = app.dispatch(Action::LibraryEditNode); + let input = app.input.as_ref().expect("rename overlay open"); + assert!( + matches!(&input.purpose, InputPurpose::Rename { path } if path == "/tidal/search/abba") + ); + assert_eq!(input.buffer, "abba", "prefilled with the current title"); + } + + #[test] + fn rename_submit_sends_the_trimmed_new_title() { + let (mut app, rx) = app(); + app.library.update(search_listing(true)); + let _ = app.dispatch(Action::LibraryEditNode); + type_str(&mut app, " tribute "); + app.handle_input_key(key(crossterm::event::KeyCode::Enter)); + assert!(app.input.is_none()); + match rx.try_recv() { + Ok(MessageFromUi::RenameNode { path, new_title }) => { + assert_eq!(path, "/tidal/search/abba"); + assert_eq!(new_title, "abba tribute"); + } + other => panic!("expected RenameNode, got {:?}", other.is_ok()), + } + } + + #[test] + fn rename_cancels_and_emptied_buffers_send_nothing() { + let (mut app, rx) = app(); + app.library.update(search_listing(true)); + + let _ = app.dispatch(Action::LibraryEditNode); + app.handle_input_key(key(crossterm::event::KeyCode::Esc)); + assert!(app.input.is_none()); + assert!(rx.try_recv().is_err(), "Esc must not rename"); + + let _ = app.dispatch(Action::LibraryEditNode); + for _ in 0.."abba".len() { + app.handle_input_key(key(crossterm::event::KeyCode::Backspace)); + } + app.handle_input_key(key(crossterm::event::KeyCode::Enter)); + assert!(app.input.is_none()); + assert!(rx.try_recv().is_err(), "emptied buffer must not rename"); + } + + #[test] + fn delete_sends_only_for_deletable_selections() { + let (mut app, rx) = app(); + let _ = app.dispatch(Action::LibraryDeleteNode); + assert!(rx.try_recv().is_err(), "no selection: no delete"); + + app.library.update(search_listing(false)); + let _ = app.dispatch(Action::LibraryDeleteNode); + assert!(rx.try_recv().is_err(), "non-deletable selection: no delete"); + + app.library.update(search_listing(true)); + let _ = app.dispatch(Action::LibraryDeleteNode); + match rx.try_recv() { + Ok(MessageFromUi::DeleteNode { path }) => { + assert_eq!(path, "/tidal/search/abba"); + } + other => panic!("expected DeleteNode, got {:?}", other.is_ok()), + } + } + + #[test] + fn modifiable_items_are_marked_and_the_rename_overlay_is_labelled() { + let (mut app, _rx) = app(); + app.library.update(search_listing(true)); + + let draw = |app: &mut App| { + let backend = ratatui::backend::TestBackend::new(80, 24); + let mut terminal = ratatui::Terminal::new(backend).expect("test terminal"); + terminal.draw(|f| app.render(f)).expect("draw app"); + let buf = terminal.backend().buffer(); + let mut text = String::new(); + for y in 0..buf.area.height { + for x in 0..buf.area.width { + text.push_str(buf[(x, y)].symbol()); + } + text.push('\n'); + } + text + }; + + let text = draw(&mut app); + assert!(text.contains("abba [ed]"), "modifiable marker: {text}"); + + let _ = app.dispatch(Action::LibraryEditNode); + let text = draw(&mut app); + assert!(text.contains("rename: abba"), "rename overlay label"); + } + #[test] fn queue_clear_actions_carry_the_keep_current_flag() { let (mut app, rx) = app(); diff --git a/cbd-tui/src/app/queue.rs b/cbd-tui/src/app/queue.rs index 5334851..9fc1c84 100644 --- a/cbd-tui/src/app/queue.rs +++ b/cbd-tui/src/app/queue.rs @@ -64,6 +64,8 @@ impl Queue { marked: false, is_queable: false, is_creatable: false, + is_editable: false, + is_deletable: false, }) .collect(); diff --git a/cbd-tui/src/main.rs b/cbd-tui/src/main.rs index 6e0e7ec..a30cc5d 100644 --- a/cbd-tui/src/main.rs +++ b/cbd-tui/src/main.rs @@ -127,6 +127,30 @@ async fn poll( } } }, + MessageFromUi::RenameNode { path, new_title } => { + // Navigates the library into the renamed node on success; + // on failure the library stays where it is. + match rpc_client.rename_library_node(&path, &new_title).await { + Ok(node) => { + let _ = tx.send(MessageToUi::ReplaceLibraryNode(node.clone())); + } + Err(err) => { + error!(path, new_title, "failed to rename node: {err}"); + } + } + }, + MessageFromUi::DeleteNode { path } => { + // Shows the refreshed parent listing on success; on + // failure the library stays where it is. + match rpc_client.delete_library_node(&path).await { + Ok(parent) => { + let _ = tx.send(MessageToUi::ReplaceLibraryNode(parent.clone())); + } + Err(err) => { + error!(path, "failed to delete node: {err}"); + } + } + }, MessageFromUi::AppendTracks(uuids) => { rpc_client.append_tracks(uuids).await? } diff --git a/cbd-tui/src/rpc.rs b/cbd-tui/src/rpc.rs index 5e06596..86a93ea 100644 --- a/cbd-tui/src/rpc.rs +++ b/cbd-tui/src/rpc.rs @@ -1,10 +1,10 @@ use crabidy_core::proto::crabidy::{ crabidy_service_client::CrabidyServiceClient, AppendRequest, ChangeVolumeRequest, - ClearQueueRequest, CreateLibraryNodeRequest, GetLibraryNodeRequest, GetUpdateStreamRequest, - GetUpdateStreamResponse, InitRequest, InitResponse, InsertRequest, LibraryNode, NextRequest, - PrevRequest, QueueRequest, RemoveRequest, ReplaceRequest, RestartTrackRequest, - SetCurrentRequest, ToggleMuteRequest, TogglePlayRequest, ToggleRepeatRequest, - ToggleShuffleRequest, + ClearQueueRequest, CreateLibraryNodeRequest, DeleteLibraryNodeRequest, GetLibraryNodeRequest, + GetUpdateStreamRequest, GetUpdateStreamResponse, InitRequest, InitResponse, InsertRequest, + LibraryNode, NextRequest, PrevRequest, QueueRequest, RemoveRequest, RenameLibraryNodeRequest, + ReplaceRequest, RestartTrackRequest, SetCurrentRequest, ToggleMuteRequest, TogglePlayRequest, + ToggleRepeatRequest, ToggleShuffleRequest, }; use std::{collections::HashMap, error::Error, fmt, time::Duration}; @@ -123,6 +123,58 @@ impl RpcClient { Ok(&self.library_node_cache[&path]) } + /// Renames an editable node and returns it at its (changed) path. + /// + /// Cache contract: the old path's entry and the parent's entry are + /// evicted (the parent's child listing changed; the old path is dead), + /// and the renamed node is inserted under its new path. + pub async fn rename_library_node( + &mut self, + path: &str, + new_title: &str, + ) -> Result<&LibraryNode, Box> { + let request = Request::new(RenameLibraryNodeRequest { + path: path.to_string(), + new_title: new_title.to_string(), + }); + let response = self.client.rename_library_node(request).await?; + let Some(node) = response.into_inner().node else { + return Err(Box::new(RpcClientError::NotFound)); + }; + // The old path is dead and the parent's child listing changed; + // cached copies would resurrect the old term. + self.library_node_cache.remove(path); + if let Some(parent) = crabidy_core::parent_path(path) { + self.library_node_cache.remove(parent); + } + let new_path = node.path.clone(); + self.library_node_cache.insert(new_path.clone(), node); + Ok(&self.library_node_cache[&new_path]) + } + + /// Deletes a node and returns the refreshed parent listing. + /// + /// Cache contract: the deleted path's entry and the parent's stale entry + /// are evicted, and the returned parent node is inserted fresh. + pub async fn delete_library_node( + &mut self, + path: &str, + ) -> Result<&LibraryNode, Box> { + let request = Request::new(DeleteLibraryNodeRequest { + path: path.to_string(), + }); + let response = self.client.delete_library_node(request).await?; + let Some(parent) = response.into_inner().parent else { + return Err(Box::new(RpcClientError::NotFound)); + }; + // Drop the deleted node and the stale parent listing; the response + // carries the fresh parent to cache instead. + self.library_node_cache.remove(path); + let parent_path = parent.path.clone(); + self.library_node_cache.insert(parent_path.clone(), parent); + Ok(&self.library_node_cache[&parent_path]) + } + pub async fn append_tracks(&mut self, paths: Vec) -> Result<(), Box> { let append_request = Request::new(AppendRequest { paths }); self.client.append(append_request).await?; diff --git a/crabidy-core/crabidy/v1/crabidy.proto b/crabidy-core/crabidy/v1/crabidy.proto index eeadeab..9d1386b 100644 --- a/crabidy-core/crabidy/v1/crabidy.proto +++ b/crabidy-core/crabidy/v1/crabidy.proto @@ -12,6 +12,14 @@ service CrabidyService { // a search term and the created node holds its results. Idempotent: an // existing title returns the existing node. rpc CreateLibraryNode(CreateLibraryNodeRequest) returns (CreateLibraryNodeResponse); + // Renames a node whose listing entry sets is_editable. For a search term + // the title is the query, so a rename re-runs the search; the node's path + // changes with the title. Renaming onto an existing sibling title merges + // with it. Returns the renamed node at its new path. + rpc RenameLibraryNode(RenameLibraryNodeRequest) returns (RenameLibraryNodeResponse); + // Deletes a node whose listing entry sets is_deletable. Idempotent: + // deleting an already-gone node succeeds. Returns the refreshed parent. + rpc DeleteLibraryNode(DeleteLibraryNodeRequest) returns (DeleteLibraryNodeResponse); // Queue rpc Queue(QueueRequest) returns (QueueResponse); @@ -67,6 +75,27 @@ message CreateLibraryNodeResponse { LibraryNode node = 1; } +message RenameLibraryNodeRequest { + // Path of the node to rename. + string path = 1; + // New human-entered title; the provider derives the new path segment. + string new_title = 2; +} +message RenameLibraryNodeResponse { + // The renamed node, at its (possibly changed) path. + LibraryNode node = 1; +} + +message DeleteLibraryNodeRequest { + // Path of the node to delete. + string path = 1; +} +message DeleteLibraryNodeResponse { + // The parent node with the deleted child gone — what a client should + // display after the delete, without a follow-up GetLibraryNode. + LibraryNode parent = 1; +} + // Queue message QueueRequest { repeated string paths = 1; @@ -160,6 +189,10 @@ message LibraryNodeChild { bool is_queable = 3; // Children may be created under this node (see CreateLibraryNode). bool is_creatable = 4; + // This node may be renamed (see RenameLibraryNode). + bool is_editable = 5; + // This node may be deleted (see DeleteLibraryNode). + bool is_deletable = 6; } message QueueModifiers { diff --git a/crabidy-core/src/lib.rs b/crabidy-core/src/lib.rs index a94d640..07cf751 100644 --- a/crabidy-core/src/lib.rs +++ b/crabidy-core/src/lib.rs @@ -41,6 +41,25 @@ pub trait ProviderClient: std::fmt::Debug + Send + Sync { parent_path: &str, title: &str, ) -> Result; + /// Renames a node advertised as editable (`LibraryNodeChild.is_editable`). + /// + /// For a search term the title is the query, so a rename re-runs the + /// search; the node's path changes with the title. Renaming onto an + /// existing sibling title merges with it (that node is returned). + /// Errors: [`ProviderError::NotSupported`] when the path is not + /// editable, [`ProviderError::InvalidInput`] when the new title is empty + /// or whitespace-only. + async fn rename_lib_node( + &self, + path: &str, + new_title: &str, + ) -> Result; + /// Deletes a node advertised as deletable (`LibraryNodeChild.is_deletable`). + /// + /// Idempotent: deleting an already-gone node succeeds. Returns the + /// refreshed parent node (what a client should display next). Errors: + /// [`ProviderError::NotSupported`] when the path is not deletable. + async fn delete_lib_node(&self, path: &str) -> Result; } #[derive(Clone, Debug, Hash, PartialEq, Eq)] @@ -137,14 +156,17 @@ impl LibraryNode { } impl LibraryNodeChild { - /// A regular, non-creatable child. Creatable children (e.g. the search - /// node) set `is_creatable` explicitly via struct update. + /// A regular, non-creatable, immutable child. Creatable/editable/ + /// deletable children (e.g. the search node and its terms) set the + /// capability flags explicitly via struct update. pub fn new(path: String, title: String, is_queable: bool) -> Self { Self { path, title, is_queable, is_creatable: false, + is_editable: false, + is_deletable: false, } } } @@ -248,4 +270,14 @@ mod tests { let _ = decode_segment(bad); } } + + #[test] + fn child_new_defaults_all_capability_flags_off() { + // Wire contract: plain children are immutable; providers opt into + // capabilities explicitly via struct update. + let child = LibraryNodeChild::new("/tidal/x".to_string(), "x".to_string(), true); + assert!(!child.is_creatable); + assert!(!child.is_editable); + assert!(!child.is_deletable); + } } diff --git a/crabidy-server/src/main.rs b/crabidy-server/src/main.rs index 578f0ef..77dde95 100644 --- a/crabidy-server/src/main.rs +++ b/crabidy-server/src/main.rs @@ -163,6 +163,19 @@ pub enum ProviderCommand { title: String, result_tx: flume::Sender>, }, + /// Renames an editable node (see `ProviderClient::rename_lib_node`); + /// replies with the renamed node at its new path. + RenameLibraryNode { + path: String, + new_title: String, + result_tx: flume::Sender>, + }, + /// Deletes a deletable node (see `ProviderClient::delete_lib_node`); + /// replies with the refreshed parent node. + DeleteLibraryNode { + path: String, + result_tx: flume::Sender>, + }, } impl ProviderCommand { @@ -172,6 +185,8 @@ impl ProviderCommand { Self::GetTrackUrls { .. } => "get_track_urls", Self::ResolveTracks { .. } => "resolve_tracks", Self::CreateLibraryNode { .. } => "create_library_node", + Self::RenameLibraryNode { .. } => "rename_library_node", + Self::DeleteLibraryNode { .. } => "delete_library_node", } } } diff --git a/crabidy-server/src/provider.rs b/crabidy-server/src/provider.rs index c272c1b..c124040 100644 --- a/crabidy-server/src/provider.rs +++ b/crabidy-server/src/provider.rs @@ -56,6 +56,22 @@ impl ProviderOrchestrator { error!("failed to send create_library_node result: {err}"); } } + ProviderCommand::RenameLibraryNode { + path, + new_title, + result_tx, + } => { + let result = self.rename_lib_node(&path, &new_title).await; + if let Err(err) = result_tx.send_async(result).await { + error!("failed to send rename_library_node result: {err}"); + } + } + ProviderCommand::DeleteLibraryNode { path, result_tx } => { + let result = self.delete_lib_node(&path).await; + if let Err(err) = result_tx.send_async(result).await { + error!("failed to send delete_library_node result: {err}"); + } + } } } @@ -194,4 +210,30 @@ impl ProviderClient for ProviderOrchestrator { warn!(parent_path, "no provider supports creating nodes here"); Err(ProviderError::NotSupported) } + + /// Routes to the provider that owns the path. The synthetic root's own + /// children are fixed and never editable. + #[instrument(skip(self))] + async fn rename_lib_node( + &self, + path: &str, + new_title: &str, + ) -> Result { + if path == tidaldy::PROVIDER_ROOT || path.starts_with("/tidal/") { + return self.tidal_client.rename_lib_node(path, new_title).await; + } + warn!(path, "no provider supports renaming this node"); + Err(ProviderError::NotSupported) + } + + /// Routes to the provider that owns the path. The synthetic root's own + /// children are fixed and never deletable. + #[instrument(skip(self))] + async fn delete_lib_node(&self, path: &str) -> Result { + if path == tidaldy::PROVIDER_ROOT || path.starts_with("/tidal/") { + return self.tidal_client.delete_lib_node(path).await; + } + warn!(path, "no provider supports deleting this node"); + Err(ProviderError::NotSupported) + } } diff --git a/crabidy-server/src/rpc.rs b/crabidy-server/src/rpc.rs index 7645add..4f724cc 100644 --- a/crabidy-server/src/rpc.rs +++ b/crabidy-server/src/rpc.rs @@ -2,14 +2,16 @@ use crate::{PlaybackCommand, PlaybackMessage, ProviderCommand, ProviderMessage}; use crabidy_core::proto::crabidy::{ crabidy_service_server::CrabidyService, get_update_stream_response::Update as StreamUpdate, AppendRequest, AppendResponse, ChangeVolumeRequest, ChangeVolumeResponse, ClearQueueRequest, - ClearQueueResponse, CreateLibraryNodeRequest, CreateLibraryNodeResponse, GetLibraryNodeRequest, + ClearQueueResponse, CreateLibraryNodeRequest, CreateLibraryNodeResponse, + DeleteLibraryNodeRequest, DeleteLibraryNodeResponse, GetLibraryNodeRequest, GetLibraryNodeResponse, GetUpdateStreamRequest, GetUpdateStreamResponse, InitRequest, InitResponse, InsertRequest, InsertResponse, NextRequest, NextResponse, PrevRequest, - PrevResponse, QueueRequest, QueueResponse, RemoveRequest, RemoveResponse, ReplaceRequest, - ReplaceResponse, RestartTrackRequest, RestartTrackResponse, SaveQueueRequest, - SaveQueueResponse, SetCurrentRequest, SetCurrentResponse, StopRequest, StopResponse, - ToggleMuteRequest, ToggleMuteResponse, TogglePlayRequest, TogglePlayResponse, - ToggleRepeatRequest, ToggleRepeatResponse, ToggleShuffleRequest, ToggleShuffleResponse, + PrevResponse, QueueRequest, QueueResponse, RemoveRequest, RemoveResponse, + RenameLibraryNodeRequest, RenameLibraryNodeResponse, ReplaceRequest, ReplaceResponse, + RestartTrackRequest, RestartTrackResponse, SaveQueueRequest, SaveQueueResponse, + SetCurrentRequest, SetCurrentResponse, StopRequest, StopResponse, ToggleMuteRequest, + ToggleMuteResponse, TogglePlayRequest, TogglePlayResponse, ToggleRepeatRequest, + ToggleRepeatResponse, ToggleShuffleRequest, ToggleShuffleResponse, }; use crabidy_core::ProviderError; use std::pin::Pin; @@ -146,6 +148,91 @@ impl CrabidyService for RpcService { } } + /// Renames an editable node via the provider loop. Same error mapping as + /// `create_library_node`: `NotSupported` → `failed_precondition`, + /// `InvalidInput` → `invalid_argument`, everything else `internal`. + #[instrument(skip(self, request), fields(path, new_title))] + async fn rename_library_node( + &self, + request: Request, + ) -> Result, Status> { + let RenameLibraryNodeRequest { path, new_title } = request.into_inner(); + tracing::Span::current().record("path", path.as_str()); + tracing::Span::current().record("new_title", new_title.as_str()); + debug!("received rename_library_node request"); + let (result_tx, result_rx) = flume::bounded(1); + self.provider_tx + .send_async(ProviderMessage::new(ProviderCommand::RenameLibraryNode { + path, + new_title, + result_tx, + })) + .await + .map_err(|err| { + error!("provider channel closed: {err}"); + Status::internal("provider unavailable") + })?; + let result = result_rx.recv_async().await.map_err(|err| { + error!("no reply from provider: {err}"); + Status::internal("provider did not reply") + })?; + match result { + Ok(node) => Ok(Response::new(RenameLibraryNodeResponse { + node: Some(node), + })), + Err(ProviderError::NotSupported) => { + Err(Status::failed_precondition("this node cannot be renamed")) + } + Err(ProviderError::InvalidInput) => Err(Status::invalid_argument("invalid node title")), + Err(err) => { + error!("rename_library_node failed: {err}"); + Err(Status::internal(err.to_string())) + } + } + } + + /// Deletes a deletable node via the provider loop and returns the + /// refreshed parent. Same error mapping as `create_library_node`. + #[instrument(skip(self, request), fields(path))] + async fn delete_library_node( + &self, + request: Request, + ) -> Result, Status> { + let DeleteLibraryNodeRequest { path } = request.into_inner(); + tracing::Span::current().record("path", path.as_str()); + debug!("received delete_library_node request"); + let (result_tx, result_rx) = flume::bounded(1); + self.provider_tx + .send_async(ProviderMessage::new(ProviderCommand::DeleteLibraryNode { + path, + result_tx, + })) + .await + .map_err(|err| { + error!("provider channel closed: {err}"); + Status::internal("provider unavailable") + })?; + let result = result_rx.recv_async().await.map_err(|err| { + error!("no reply from provider: {err}"); + Status::internal("provider did not reply") + })?; + match result { + Ok(parent) => Ok(Response::new(DeleteLibraryNodeResponse { + parent: Some(parent), + })), + Err(ProviderError::NotSupported) => { + Err(Status::failed_precondition("this node cannot be deleted")) + } + // No provider raises this for delete today; mapped anyway so the + // contract stays uniform across the node-mutation rpcs. + Err(ProviderError::InvalidInput) => Err(Status::invalid_argument("invalid node path")), + Err(err) => { + error!("delete_library_node failed: {err}"); + Err(Status::internal(err.to_string())) + } + } + } + #[instrument(skip(self, request), fields(paths))] async fn queue( &self, diff --git a/plan/node-editing.md b/plan/node-editing.md new file mode 100644 index 0000000..8f3f3fe --- /dev/null +++ b/plan/node-editing.md @@ -0,0 +1,83 @@ +# Plan — editable and deletable nodes + +Ordered tasks for the `implement` stage. Inputs: `architecture/node-editing.md`, +stubs across the workspace, gates in `quality/node-editing.md`. Tests: +`devenv shell -- cargo test --workspace` (10 failing at plan time = the +target: 5 in tidaldy, 5 in cbd-tui). Use the session-local `CARGO_TARGET_DIR` +if `target/` contains artifacts owned by the repo owner. + +## 1. Term-list mutations (tidaldy) + +- [x] Implement `rename_search_term(old, new)`: under one write lock, if + `new` already exists remove the `old` slot (merge); else if `old` + exists replace it in place; else push `new` (stale-client + forgiveness). Implement `remove_search_term(term)`: `retain` + everything but `term`. Both poison-tolerant like + `register_search_term`, no await under the lock. **Verify**: + `rename_replaces_in_place_and_merges_duplicates` passes; gate "lock + discipline". + +## 2. Provider rename/delete (tidaldy) + +- [x] `rename_lib_node`: trim new title → `InvalidInput` if empty; + `parse_path(path)?` must be `TidalPath::SearchTerm(old)` else + `NotSupported`; decode old term, `rename_search_term`, return + `get_lib_node(join_path(parent, encode_segment(new)))`. **Verify**: + `rename_rejects_empty_titles_and_foreign_paths` passes (validation + before any network call). +- [x] `delete_lib_node`: `parse_path(path)?` must be `SearchTerm` else + `NotSupported`; decode + `remove_search_term` (idempotent); return + `get_lib_node(/tidal/search)`. **Verify**: `delete_rejects_foreign_paths` + and `delete_removes_terms_idempotently_and_returns_the_parent` pass. +- [x] Flag term children in the `Search` arm of `get_lib_node`: + `is_editable: true, is_deletable: true` via struct update. **Verify**: + `search_node_lists_created_terms` passes (extended assertion). + +## 3. Server handlers (crabidy-server) + +- [x] Replace the `todo!()` bodies of `rpc.rs + rename_library_node`/`delete_library_node`: record span fields, + bounded(1) rendezvous with the matching `ProviderCommand`, map errors + exactly like `create_library_node` (`NotSupported` → + `failed_precondition` with an operation-specific message, + `InvalidInput` → `invalid_argument`, rest → `internal`). **Verify**: + gates "error mapping" + "bounded rendezvous"; `cargo check`. + (Orchestrator routing and the command-loop arms were finalized at + stub time — re-read them against the gate rather than re-doing them.) + +## 4. TUI library accessors + marker (cbd-tui) + +- [x] Implement `Library::selected_editable` / `selected_deletable` from + `list_state.selected()` + the `UiItem` flags. **Verify**: + `edit_opens_the_overlay_prefilled_only_on_editable_selections`, + `delete_sends_only_for_deletable_selections`, + `rename_submit_sends_the_trimmed_new_title`, + `rename_cancels_and_emptied_buffers_send_nothing` pass (dispatch and + overlay logic already landed with the stubs). +- [x] Render the `[e]`/`[d]`/`[ed]` suffix for modifiable items in + `Library::render`, `COLOR_SECONDARY` like `[%]`. **Verify**: + `modifiable_items_are_marked_and_the_rename_overlay_is_labelled` + passes; gate "marked in UI". + +## 5. TUI ↔ server wiring (cbd-tui) + +- [x] `RpcClient::rename_library_node`: send the rpc, evict the old path and + the parent (`crabidy_core::parent_path`), insert + return the renamed + node. `delete_library_node`: send the rpc, evict the deleted path and + the parent, insert + return the returned parent node. **Verify**: gate + "cache eviction" (read; the cache is private, as with create). +- [x] `main.rs poll`: replace the `todo!()` arms — `RenameNode` calls + `rename_library_node`, `DeleteNode` calls `delete_library_node`; on + success `ReplaceLibraryNode(node)`, on failure `error!` and leave the + UI unchanged. **Verify**: gate "failures never panic". + +## 6. End-to-end + gates sweep + +- [x] Exercise the full path against the live API if the local tidal config + is available (create a term, rename it, delete it, confirm the parent + listing and that a queued search track still resolves a URL); note the + outcome in the summary. If unavailable, rely on the offline tests and + say so. +- [x] Sweep: no `todo!()` left (workspace grep), fmt + clippy + tests green, + every `quality/node-editing.md` box checked, docs updated where + behavior shifted. Append the outcome + deviations to `plan/summary.md`. diff --git a/plan/summary.md b/plan/summary.md index 3ce103d..f74cb74 100644 --- a/plan/summary.md +++ b/plan/summary.md @@ -1,5 +1,44 @@ # Implementation summaries +## node-editing (2026-07-20) + +Built per `plan/node-editing.md`: search-term nodes (created via `%`) are now +modifiable — `e` opens the input overlay prefilled with the current title and +renames (re-running the search; merge on title collision), `d` deletes +without confirmation (documented decision, architecture/node-editing.md D4). +Capabilities travel as `LibraryNodeChild.is_editable`/`is_deletable` (fields +5/6, child-only — no consumer for node-level copies), surfaced as a `[ed]` +marker; two new rpcs `RenameLibraryNode` (returns the renamed node, TUI +navigates into it) and `DeleteLibraryNode` (returns the refreshed parent). +All 59 workspace tests green; every gate in `quality/node-editing.md` +checked. Verified against the live Tidal API with a temporary ignored probe +(removed after passing): create `beatles` → rename to `rolling stones` +(in-place, 20 tracks / 40 children) → a track queued under the old term +still resolved a stream URL → delete emptied the listing. + +The whole feature ran autonomously per standing instruction; decisions are +recorded in `architecture/node-editing.md` (options + rationale per topic). + +### Deviations from plan / architecture (node-editing) + +- **Self-rename bug caught by the gate tests**: the first + `rename_search_term` implementation deleted a term renamed to itself (the + merge branch removed the "old" slot). Fixed with an explicit `old != new` + guard; the architecture text ("merge on collision") now implicitly means + *distinct* titles. +- **`pane_bindings_only_match_their_own_pane` (help-modal suite) updated**: + it asserted plain `d` is unbound in the library — now it is + `LibraryDeleteNode` by design; the test's queue-only example key moved to + `c`. +- **`delete_library_node` also maps `InvalidInput` → `invalid_argument`** + although no provider raises it for delete today — keeps the error contract + uniform across the three node-mutation rpcs. +- **End-to-end check ran at the provider layer** (as with search): no + interactive terminal/audio device in this environment; the gRPC handler + and TUI layers above it are covered by unit tests and review. +- **Environment note**: builds/tests again ran with a session-local + `CARGO_TARGET_DIR` (owner-built artifacts in `target/`); no repo change. + ## search (2026-07-20) Built per `plan/search.md`: `%` inside `/tidal/search` opens a one-line input; diff --git a/quality/node-editing.md b/quality/node-editing.md new file mode 100644 index 0000000..6cd4560 --- /dev/null +++ b/quality/node-editing.md @@ -0,0 +1,91 @@ +# Quality gates — editable and deletable nodes + +Checklist for the `implement` stage. Automatic tests live in +`crabidy-core/src/lib.rs`, `tidaldy/src/lib.rs`, and +`cbd-tui/src/app/{bindings,mod}.rs` test modules; run with +`devenv shell -- cargo test --workspace` (network-dependent tidaldy tests +stay `#[ignore]`). 10 tests fail at gate-writing time — they define the +target. + +## Contract & wire + +- [x] Proto changes are additive only: existing field numbers untouched; + `LibraryNodeChild.is_editable = 5`, `is_deletable = 6`; + `RenameLibraryNode` and `DeleteLibraryNode` rpcs present. Old clients + keep working against the new server. +- [x] `rpc.rs` (server) maps errors per the documented contract for **both** + new rpcs: `NotSupported` → `failed_precondition`, `InvalidInput` → + `invalid_argument`, everything else → `internal`. No `color-eyre` + report or debug formatting of internals leaks into `Status` messages. +- [x] `ProviderOrchestrator::{rename,delete}_lib_node` route `/tidal`-prefixed + paths to the Tidal client and answer anything else (including the + synthetic root `/`) with `NotSupported` — same prefix discipline as + `create_lib_node`. +- [x] Both new commands follow the existing bounded(1)-reply rendezvous + pattern; no new unbounded channels anywhere in the feature. + +## Provider semantics (tidaldy) + +- [x] `rename_lib_node` trims the new title; empty/whitespace → + `InvalidInput`; any path that is not a `SearchTerm` (including + `/tidal/search` itself) → `NotSupported`; validation happens before any + network call. +- [x] Rename replaces the term **in place** (keeps its list position); + renaming onto an existing term merges (the old slot is removed, no + duplicates ever); renaming an unknown term registers the new one + (stale-client forgiveness). Returns `get_lib_node(new_path)` — the + node at its new percent-encoded path. +- [x] `delete_lib_node` accepts only `SearchTerm` paths (`NotSupported` + otherwise), removes the term idempotently (unknown term → success), + and returns the refreshed `/tidal/search` parent node. +- [x] `rename_search_term` / `remove_search_term` follow the established + lock discipline: poison-tolerant, the `search_terms` lock is never + held across an `await`. +- [x] Term children returned by the `Search` arm set + `is_editable: true, is_deletable: true`; nothing else in the provider + sets either flag. +- [x] Queued search tracks keep playing after a rename/delete of their term: + `get_urls_for_track` / `get_metadata_for_track` resolve from the track + id embedded in the path, independent of term registration. + +## No panics on user input (hard rule) + +- [x] All `todo!()` stubs from api-design are gone (grep the workspace). +- [x] Rename/delete failures (network, auth, malformed paths from stale + clients) surface as `ProviderError`/`Status`, never a panic; the TUI's + rename and delete paths handle an error reply without crashing the + orchestrate task (log + stay put). + +## TUI behavior + +- [x] `e` opens the overlay only when the **selected** item `is_editable`, + prefilled with the current title; `d` sends `DeleteNode` only when the + selected item `is_deletable`; both are silent no-ops otherwise + (including when nothing is selected or the list is empty). +- [x] The overlay carries its purpose: submit sends `CreateNode` for + `InputPurpose::Create` and `RenameNode` for `InputPurpose::Rename`; + the rendered label distinguishes them (`new node:` vs `rename:`). + While the overlay is open the bindings table stays unreachable + (existing input-mode bypass, unchanged). +- [x] Delete is deliberately unconfirmed (architecture/node-editing.md D4); + the open question about confirmation for higher-value nodes is + preserved in the architecture doc, not silently dropped. +- [x] Modifiable children are visibly marked in the library list (`[e]`, + `[d]` or `[ed]` suffix, `COLOR_SECONDARY`), coexisting with the `[%]` + marker for creatable ones. +- [x] `RpcClient::rename_library_node` evicts the old path **and** the + parent entry, then caches the node under its new path; + `delete_library_node` evicts the deleted path and the parent, then + caches the returned parent. No stale `/tidal/search` listing can + resurrect an old term. +- [x] On successful rename the library navigates into the renamed node; on + successful delete it shows the refreshed parent; on failure the + library stays where it was. + +## Code quality + +- [x] Public items added in all five crates have doc comments matching final + behavior; the `e`/`d` bindings appear in the help modal automatically. +- [x] No new dependencies. +- [x] `devenv shell -- cargo fmt --check`, `cargo clippy --workspace` + (no new warnings), `cargo test --workspace` all pass. diff --git a/tidaldy/src/lib.rs b/tidaldy/src/lib.rs index 3fca51e..f5462b1 100644 --- a/tidaldy/src/lib.rs +++ b/tidaldy/src/lib.rs @@ -260,11 +260,17 @@ impl crabidy_core::ProviderClient for Client { .search_terms_snapshot() .iter() .map(|term| { - crabidy_core::proto::crabidy::LibraryNodeChild::new( - crabidy_core::join_path(path, &crabidy_core::encode_segment(term)), - term.clone(), - false, - ) + // Term nodes are the modifiable nodes: renamable + // (`e`) and deletable (`d`). + crabidy_core::proto::crabidy::LibraryNodeChild { + is_editable: true, + is_deletable: true, + ..crabidy_core::proto::crabidy::LibraryNodeChild::new( + crabidy_core::join_path(path, &crabidy_core::encode_segment(term)), + term.clone(), + false, + ) + } }) .collect(), is_queable: false, @@ -306,6 +312,48 @@ impl crabidy_core::ProviderClient for Client { let term_path = crabidy_core::join_path(parent_path, &crabidy_core::encode_segment(term)); self.get_lib_node(&term_path).await } + + #[instrument(skip(self))] + async fn rename_lib_node( + &self, + path: &str, + new_title: &str, + ) -> Result { + let new_term = new_title.trim(); + if new_term.is_empty() { + return Err(crabidy_core::ProviderError::InvalidInput); + } + // Only search terms are editable. + let TidalPath::SearchTerm(encoded_old) = parse_path(path)? else { + warn!(path, "renaming not supported here"); + return Err(crabidy_core::ProviderError::NotSupported); + }; + let old_term = crabidy_core::decode_segment(encoded_old); + // In-place replace keeps the list position; renaming onto an + // existing term merges with it. + self.rename_search_term(&old_term, new_term); + // A SearchTerm path always has the search node as its parent. + let parent = crabidy_core::parent_path(path).unwrap_or(PROVIDER_ROOT); + let new_path = crabidy_core::join_path(parent, &crabidy_core::encode_segment(new_term)); + self.get_lib_node(&new_path).await + } + + #[instrument(skip(self))] + async fn delete_lib_node( + &self, + path: &str, + ) -> Result { + // Only search terms are deletable. + let TidalPath::SearchTerm(encoded) = parse_path(path)? else { + warn!(path, "deleting not supported here"); + return Err(crabidy_core::ProviderError::NotSupported); + }; + // Idempotent: removing an already-gone term is a success. + self.remove_search_term(&crabidy_core::decode_segment(encoded)); + // A SearchTerm path always has the search node as its parent. + let parent = crabidy_core::parent_path(path).unwrap_or(PROVIDER_ROOT); + self.get_lib_node(parent).await + } } /// The root of this provider in the global library tree. @@ -645,6 +693,39 @@ impl Client { } } + /// Renames `old` to `new` in place, keeping its position. If `new` + /// already exists elsewhere the entries merge (the `old` slot is + /// removed) — the list never holds duplicates. An unknown `old` behaves + /// like registration of `new` (stale-client forgiveness). The lock is + /// released before any await point. + fn rename_search_term(&self, old: &str, new: &str) { + let mut terms = match self.search_terms.write() { + Ok(terms) => terms, + Err(poisoned) => poisoned.into_inner(), + }; + if terms.iter().any(|t| t == new) { + // Merge — but a self-rename must not delete the very term it + // is supposed to keep. + if old != new { + terms.retain(|t| t != old); + } + } else if let Some(slot) = terms.iter_mut().find(|t| *t == old) { + *slot = new.to_string(); + } else { + terms.push(new.to_string()); + } + } + + /// Removes a term. Unknown terms are a no-op — delete is idempotent. + /// The lock is released before any await point. + fn remove_search_term(&self, term: &str) { + let mut terms = match self.search_terms.write() { + Ok(terms) => terms, + Err(poisoned) => poisoned.into_inner(), + }; + terms.retain(|t| t != term); + } + /// Builds the node for one search term: its `tracks` are the track /// results (queueable individually, paths under this node), its children /// are artist and album results pointing at **canonical** @@ -1171,6 +1252,96 @@ mod tests { assert_eq!(titles, vec!["AC/DC", "abba"]); assert_eq!(node.children[0].path, "/tidal/search/AC%2FDC"); assert!(node.children.iter().all(|c| !c.is_queable)); + // Term nodes are the modifiable nodes: renamable and deletable. + assert!(node + .children + .iter() + .all(|c| c.is_editable && c.is_deletable)); + } + + #[tokio::test] + async fn rename_rejects_empty_titles_and_foreign_paths() { + use crabidy_core::ProviderError; + let client = offline_client(); + // Validation happens before any network call, so this works offline. + assert_eq!( + client.rename_lib_node("/tidal/search/abba", " ").await, + Err(ProviderError::InvalidInput) + ); + assert_eq!( + client + .rename_lib_node("/tidal/playlists/xyz", "queen") + .await, + Err(ProviderError::NotSupported) + ); + // The search node itself is creatable, not editable. + assert_eq!( + client.rename_lib_node("/tidal/search", "queen").await, + Err(ProviderError::NotSupported) + ); + assert_eq!( + client.rename_lib_node("/nope", "queen").await, + Err(ProviderError::MalformedPath) + ); + } + + #[tokio::test] + async fn delete_rejects_foreign_paths() { + use crabidy_core::ProviderError; + let client = offline_client(); + assert_eq!( + client.delete_lib_node("/tidal/playlists/xyz").await, + Err(ProviderError::NotSupported) + ); + // The search node itself is not deletable, only its terms are. + assert_eq!( + client.delete_lib_node("/tidal/search").await, + Err(ProviderError::NotSupported) + ); + assert_eq!( + client.delete_lib_node("/nope").await, + Err(ProviderError::MalformedPath) + ); + } + + #[tokio::test] + async fn delete_removes_terms_idempotently_and_returns_the_parent() { + let client = offline_client(); + client.register_search_term("abba"); + client.register_search_term("queen"); + let parent = client + .delete_lib_node("/tidal/search/abba") + .await + .expect("refreshed parent"); + assert_eq!(parent.path, "/tidal/search"); + let titles: Vec<_> = parent.children.iter().map(|c| c.title.as_str()).collect(); + assert_eq!(titles, vec!["queen"]); + // Deleting an already-gone term succeeds — delete is idempotent. + let parent = client + .delete_lib_node("/tidal/search/abba") + .await + .expect("idempotent delete"); + assert_eq!(parent.children.len(), 1); + } + + #[test] + fn rename_replaces_in_place_and_merges_duplicates() { + let client = offline_client(); + client.register_search_term("abba"); + client.register_search_term("queen"); + client.register_search_term("kiss"); + // In-place: the renamed term keeps its list position. + client.rename_search_term("queen", "wham"); + assert_eq!(client.search_terms_snapshot(), vec!["abba", "wham", "kiss"]); + // Merge: renaming onto an existing term drops the old slot. + client.rename_search_term("abba", "kiss"); + assert_eq!(client.search_terms_snapshot(), vec!["wham", "kiss"]); + // An unknown old term registers the new one (stale-client forgiveness). + client.rename_search_term("ghost", "toto"); + assert_eq!(client.search_terms_snapshot(), vec!["wham", "kiss", "toto"]); + // Renaming a term to itself changes nothing. + client.rename_search_term("kiss", "kiss"); + assert_eq!(client.search_terms_snapshot(), vec!["wham", "kiss", "toto"]); } #[test]