Skip to content

Playlists: move a playlist or folder between folders - #42

Merged
cole-hackman merged 1 commit into
mainfrom
claude/lexicon-playlist-tree
Aug 8, 2026
Merged

cole-hackman merged 1 commit into
mainfrom
claude/lexicon-playlist-tree

Conversation

@cole-hackman

Copy link
Copy Markdown
Owner

Folder-drop and drag-between were listed as two gaps. They are one missing change kind. Playlists tree moves to done.

PlaylistReorder already writes djmdPlaylist.Seq, and its SQL puts the parent in the WHERE clause specifically so a reorder cannot move anything between folders. That was the right call — a reorder that restructured the tree would be a nasty surprise — and it means moving needs its own verb.

Two refusals, because djmdPlaylist enforces neither

  • The destination must be a folder. Rekordbox nests under folders only; a playlist parented to a playlist is a shape nothing renders, so it would simply vanish from the sidebar.
  • A folder cannot go inside itself or a descendant. This is the one that actually worried me: it does not error, it loses no data, it just detaches that entire subtree from the root. The playlists still exist in djmdPlaylist, and there is no path to them from the tree, ever. Silent and total.

Three smaller decisions

  • The UI mirrors both refusals rather than letting the applier be the only guard. Duplication I would normally argue against — but without it the drop looks like it worked, and the rejection surfaces only when the user opens the review table, by which point they have made several more drops on a tree that was lying to them. A folder highlights only when the drop would actually be accepted.
  • The ancestor walk goes upward from the destination, not downward from the dragged folder: a playlist tree is far wider than it is deep, so the upward walk is bounded by depth while the downward one visits the whole subtree. Both the Rust and TypeScript versions keep a seen set — a database that already contains a cycle must not hang the sync or the render, and the move is not what created that problem, so it reports "not a descendant" and lets the write through.
  • old_parent_id rides on the change. Without it changes::undo blocks the inverse and the tree cannot be put back — and for a drag, the easiest edit in the app to make by accident, that is exactly where undo matters.

The rules live in lib/playlist-tree.ts rather than inside the drop handler, for the same reason reorder.ts exists: jsdom does not run drag events, so a rule that lives in a handler is a rule nothing tests.

The move is staged, not written — the tree redraws from master.db, so the row does not appear to move until Sync applies it. Honest behaviour for a change that has not happened yet.

Verification

cargo fmt --all -- --check          clean
cargo test --workspace              54 test binaries, 0 failures (changes +11)
cargo clippy --all-targets -D warn  clean
pnpm test                           807 tests, 70 files
pnpm typecheck / pnpm lint          clean
pnpm e2e                            59 passed

Matrix moves to 59 done / 21 partial / 14 missing / 2 blocked / 16 deferred; Library & browser is now 16 done / 1 partial, the remaining one being the track table's missing drag source.


Generated by Claude Code

Folder-drop and drag-between were listed as two gaps; they are one missing
change kind. `PlaylistReorder` writes `djmdPlaylist.Seq` and puts the parent in
its `WHERE` clause specifically so a reorder cannot restructure the tree — a
good decision that means moving needs its own verb.

`PlaylistMove` carries two refusals, because `djmdPlaylist` enforces neither
and both corrupt the tree:

- **The destination must be a folder.** Rekordbox nests under folders only; a
  playlist parented to a playlist is a shape nothing renders, so it would
  simply vanish from the sidebar.
- **A folder cannot go inside itself or a descendant.** This is the one that
  matters: it does not error and it loses no data, it just detaches that entire
  subtree from the root. The playlists still exist in `djmdPlaylist` and there
  is no path to them from the tree, ever.

Three smaller decisions:

- **The UI mirrors both refusals** rather than letting the applier be the only
  guard. Duplication I would normally argue against, but without it the drop
  looks like it worked and the rejection surfaces only when the user opens the
  review table — by which point they have made several more drops on a tree
  that was lying to them. A folder highlights only when the drop is acceptable.
- **The ancestor walk goes upward from the destination**, not downward from the
  dragged folder: a tree is far wider than it is deep. Both the Rust and the
  TypeScript version keep a `seen` set, because a database that already
  contains a cycle must not hang the sync or the render — and the move is not
  what created that problem, so it reports "not a descendant" and proceeds.
- **`old_parent_id` rides on the change.** Without it `changes::undo` blocks
  the inverse and the tree cannot be put back, which for a drag — the easiest
  edit in the app to make by accident — is where undo actually matters.

The rules live in `lib/playlist-tree.ts` rather than inside the drop handler,
for the same reason `reorder.ts` exists: jsdom does not run drag events, so a
rule that lives in a handler is a rule nothing tests.

The move is staged, not written: the tree redraws from `master.db`, so the row
does not appear to move until Sync applies it.

Per docs/lexicon/02-library.md §Playlists.
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@cole-hackman, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0cbd6875-adfd-4a73-8381-762eaf351647

📥 Commits

Reviewing files that changed from the base of the PR and between 6224e20 and a78ba01.

📒 Files selected for processing (13)
  • apps/desktop/src-tauri/src/lib.rs
  • apps/desktop/src-tauri/src/playlist_tools.rs
  • apps/desktop/src/components/PlaylistPanel.tsx
  • apps/desktop/src/ipc.ts
  • apps/desktop/src/lib/playlist-tree.test.ts
  • apps/desktop/src/lib/playlist-tree.ts
  • crates/changes/src/applier.rs
  • crates/changes/src/applier/playlists.rs
  • crates/changes/src/lib.rs
  • crates/changes/src/undo.rs
  • docs/JOURNAL.md
  • docs/STATUS.md
  • docs/lexicon/PARITY.md

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cole-hackman
cole-hackman marked this pull request as ready for review August 8, 2026 06:59
@cole-hackman
cole-hackman merged commit c076e1b into main Aug 8, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants