fix(queue): manual additions stack in user queue, not at absolute end#158
Conversation
…lute end Adding a track from the context menu's "Add to queue" called `queue::append`, which dropped the new row at the bottom of the entire queue — behind every remaining album / playlist / smart-fill track. Watching what you actually queued meant scrolling past 100+ context tracks, and the user picks fired in the wrong order relative to "Play next" (which already inserted at current+1). Carve out a real "user queue" block, Spotify-style: the contiguous 'manual'-sourced rows sitting between the current track and the context tail. - New `queue::append_to_user_queue` finds the first non-'manual' position past the cursor (= start of the context tail) and inserts the picks immediately before it, pushing the tail down. Degenerates into `append` when the entire tail is already manual (queue all user picks, or current at the last slot) and into `fill_queue` when the queue is empty so the first "Add to queue" click on a fresh session still starts playback. - `player_add_to_queue` switched to call the new helper. - "Play next" semantics unchanged (`insert_after_current` already pushes to the top of the user queue at current+1). End state: Now Playing → user queue (Play-next stacked on top, Add-to-queue stacked at the bottom) → context tail. Album resumes where it left off after the user queue drains.
InstaZDLL
added
scope: backend
📝 WalkthroughWalkthroughCette PR ajoute ChangesUser queue placement strategy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/queue.rs`:
- Around line 338-359: The boundary/length/current_index calculations must run
inside the same DB transaction as the write to avoid race conditions; start a
transaction with pool.begin() before calling queue_length, read_setting_i64, and
the SELECT MIN(position) query (the boundary computation), then perform
fill_queue or append using that same transaction/connection (use the &mut
sqlx::SqliteConnection transaction type and update calls to fill_queue, append
and any upsert helpers to accept and use the transaction connection instead of
pool) so the decision and insert are atomic and respect SQLite single-writer
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fcbb4eed-87e8-4cb0-8ac2-6a635e64f438
📒 Files selected for processing (3)
docs/features/playback.mdsrc-tauri/src/commands/player.rssrc-tauri/src/queue.rs
The boundary computation (queue_length + current_index + MIN(position) WHERE source_type != 'manual') was running outside the writer transaction. SQLite WAL allows many readers and one writer, so an auto-advance / fill_queue / shuffle firing between our reads and our pool.begin() could invalidate every input we'd already decided on — the insert then lands at the wrong slot relative to the new cursor or the new queue contents. Wrap the whole decision-then-write in a single `pool.begin()` transaction. Every SQL now runs against `&mut *tx`: - COUNT(*) for the length check - SELECT value FROM profile_setting for the cursor - SELECT MIN(position) for the boundary - the park-shift INSERT block - the preshuffle invalidation Empty-queue and all-manual-tail fallbacks are inlined (their previous delegation to fill_queue / append would have opened their own nested txs, defeating the atomicity guarantee). Same SQL shape, no new helpers — keeps the public queue.rs API surface unchanged. Note: `insert_after_current` has the same pre-existing structural race but is out of scope for this PR's review.
|
Addressed in 8dbfc26. Finding valid. Confirmed against current code: Fix: open
Both fallback branches (empty queue → fill, all-manual tail → append) had to be inlined — delegating to Out of scope, noted: Tests: 111/111 passed, cargo check clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/queue.rs`:
- Around line 338-382: The UPDATE on profile_setting for key
'queue.current_index' may do nothing for new profiles; replace that UPDATE with
an INSERT ... ON CONFLICT DO UPDATE pattern so the setting is created if missing
(same pattern used elsewhere, e.g. fill_queue). Locate the sqlx::query that
updates profile_setting for 'queue.current_index' and change it to insert the
key, value ("0") and updated_at (now) and on conflict(key) update value and
updated_at, binding the same parameters (value and now) as needed; leave the
surrounding transaction, DELETE for 'queue.preshuffle', and tx.commit()
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eef9f440-21e5-4844-a4e3-fb269c3a14d1
📒 Files selected for processing (1)
src-tauri/src/queue.rs
|
@coderabbitai Skipping per «verify each finding» — the stated failure mode (fresh profile lacks the row → UPDATE no-ops → cursor undefined) is invalid:
```sql So every profile — fresh or migrated — has the row at boot. The UPDATE affects 1 row deterministically. This is exactly why The broader CLAUDE.md note ("Pattern: INSERT … ON CONFLICT DO UPDATE") applies to keys that aren't seeded — e.g. |
|
✏️ Learnings added
✅ Actions performedComments resolved and changes approved. |
1 participant
Summary
User flagged that "Add to queue" was dropping tracks at the bottom of the entire queue — behind every remaining album / playlist track. Watching what you actually queued meant scrolling past 100+ context tracks, and the picks fired in the wrong order relative to "Play next" (which already inserts at current+1).
Carve out a real "user queue" block, Spotify-style: the contiguous
source_type = 'manual'rows sitting between the current track and the context tail.How
The
queue_itemtable already carries asource_typecolumn populated by every queue-building path ('album'from Play album,'playlist'from Play playlist,'manual'from the two context-menu actions). New helperqueue::append_to_user_queuewalks that flag:Result = start of the context tail. Insert the picks immediately before it (push the tail down by N, park-shift detour for SQLite's
UNIQUE(position)). When the query returns NULL — entire tail is already manual, or current is at the last slot — degenerate intoappend. When the queue is empty, degenerate intofill_queueso the first "Add to queue" on a fresh session still starts playback.player_add_to_queueswitched to the new helper. "Play next" semantics unchanged —insert_after_currentalready pushes to current+1, i.e. the top of the user queue.End state
Test plan
cargo check --all-targetscleancargo test --lib queue→ 2/2 passed (only pure-fn coverage exists for queue.rs, matches the rest of the codebase's no-SQL-integration-tests convention)track 3 → Z → X → Y → track 4 → track 5 → …Summary by CodeRabbit