feat(download): queue drag-reorder + priority rescheduling (task 12)#105
Merged
feat(download): queue drag-reorder + priority rescheduling (task 12)#105
Conversation
Implements PRD-v2 P0.12: - Commands `download_move_to_top`, `download_move_to_bottom`, `download_reorder_queue` + `queue_position` column persisted in SQLite (migration `m20260425_000004_add_queue_position`). - `QueueManager` sort now uses priority desc → queue_position asc → created_at asc, and subscribes to new `DownloadPrioritySet` / `QueueReordered` events so set_priority triggers immediate rescheduling — a high-priority item starts as soon as a slot is free. - Default `download_list` sort is `queue_position ASC → created_at DESC` so fresh downloads (position 0) stay newest-first while manually reordered rows stick across reloads. - Frontend wires `@dnd-kit/core` + `@dnd-kit/sortable` into `DownloadsTable` with a drag handle column (only enabled for Queued/Retry/Waiting rows) and Move to top / Move to bottom menu items. `computeReorderedIds` helper filters non-reorderable IDs from the new order before invoking `download_reorder_queue`. - Covered by new unit tests in the queue_manager, move_queue, and set_priority modules (Rust) and new Vitest cases in DownloadsTable.test.tsx (drag handler + move-to-top/bottom IPC).
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent per-download Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend (DownloadsTable)
participant Tauri as Tauri IPC
participant CmdBus as CommandBus
participant Repo as DownloadRepo
participant DB as SQLite
participant EventBus as EventBus
participant QueueMgr as QueueManager
User->>Frontend: Drag item or click move action
Frontend->>Frontend: computeReorderedIds() / prepare orderedIds
Frontend->>Tauri: download_reorder_queue(orderedIds) or download_move_to_top/bottom
Tauri->>CmdBus: ReorderQueueCommand / MoveToTopCommand
CmdBus->>CmdBus: lock_queue_positions()
CmdBus->>Repo: load reorderable items (SELECT queue_position...)
Repo->>DB: query
DB-->>Repo: rows
CmdBus->>Repo: save_batch(updated downloads)
Repo->>DB: BEGIN / UPDATE queue_position... / COMMIT
DB-->>Repo: OK
CmdBus->>EventBus: publish(QueueReordered / DownloadPrioritySet)
EventBus-->>QueueMgr: deliver event
QueueMgr->>QueueMgr: on_slot_freed() -> sort by priority, queue_position, created_at
EventBus-->>Tauri: bridge emits "queue-reordered"/"download-priority-set"
Tauri-->>Frontend: frontend receives event and refreshes UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR completes PRD-v2 P0.12 (task 12) by adding drag-and-drop queue reordering with a persistent Confidence Score: 5/5Safe to merge; all previously flagged P1s are resolved, and the single remaining finding is a P2 UX refinement. No P0 or P1 issues found. The only finding is a P2 style/UX concern: sortableIds includes non-reorderable rows, causing unwanted visual transforms on Downloading/Completed rows during drag. Backend logic is solid — state guards, atomic batch transactions, position-lock, and correct queue_position assignment for new/redownloaded items are all in place. 782 Rust tests and 546 TS tests pass. src/views/DownloadsView/DownloadsTable.tsx — the sortableIds memo should exclude non-reorderable rows to avoid visual transform artefacts and confusing over-target behaviour.
|
| Filename | Overview |
|---|---|
| src/views/DownloadsView/DownloadsTable.tsx | Adds DndContext/SortableContext/SortableRow for drag-and-drop reordering. Hooks are correctly placed before early returns (previous issue addressed). computeReorderedIds and handleDragEnd are well-structured. Minor issue: sortableIds includes non-reorderable rows, causing unwanted visual transforms and confusing over-target behaviour during drag. |
| src-tauri/src/application/commands/move_queue.rs | New move_to_top, move_to_bottom, and reorder_queue handlers with state guards, POSITION_STRIDE, duplicate-ID rejection, and atomic batch persistence. Previously flagged issues (missing state guard, stride-of-1 starvation) are fully addressed. Well-tested with 10+ unit tests. |
| src-tauri/src/application/commands/start_download.rs | Acquires queue_position_lock before allocating position via next_queue_position(), so new downloads append to the back of the queue rather than landing at position 0 and jumping ahead of reordered items. Previous regression is resolved. |
| src-tauri/src/application/services/queue_manager.rs | QueueManager now subscribes to DownloadPrioritySet and QueueReordered events and calls on_slot_freed() for both. Scheduling sort updated to priority DESC → queue_position ASC → created_at ASC. Two new tests verify the tie-breaking behaviour. |
| src-tauri/src/adapters/driven/sqlite/download_repo.rs | Refactored save logic into persist_download helper; added save_batch with proper SQLite transaction for atomic reordering; find_by_state now returns deterministic order (queue_position ASC, created_at ASC, id ASC); QueuePosition added to upsert update_columns. |
| src-tauri/src/adapters/driven/sqlite/migrations/m20260425_000004_add_queue_position.rs | Clean migration adding queue_position BIGINT NOT NULL DEFAULT 0 with index and reversible down() migration. Existing rows default to 0, preserving created_at DESC secondary sort for un-reordered items. |
Sequence Diagram
sequenceDiagram
participant UI as DownloadsTable (React)
participant IPC as Tauri IPC
participant CB as CommandBus
participant Repo as SqliteDownloadRepo
participant EB as EventBus
participant QM as QueueManager
Note over UI: User drags row / clicks Move to Top
UI->>UI: computeReorderedIds()
UI->>IPC: download_reorder_queue / download_move_to_top
IPC->>CB: handle_reorder_queue / handle_move_to_top
CB->>CB: lock_queue_positions()
CB->>Repo: load_reorderable_pool()
CB->>Repo: save_batch() [atomic transaction]
CB->>EB: publish(QueueReordered)
EB-->>QM: QueueReordered event
QM->>QM: on_slot_freed()
QM->>Repo: find_by_state(Queued/Retry)
QM->>QM: sort by priority DESC → queue_position ASC → created_at ASC
QM->>Repo: save(Downloading)
QM->>IPC: DownloadStarted event → UI invalidates cache
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/views/DownloadsView/DownloadsTable.tsx
Line: 663-664
Comment:
**Non-reorderable rows included as DnD collision targets and receive visual transforms**
`sortableIds` is built from all rows (`rows.map((r) => r.original.id)`), including Downloading, Completed, Paused, etc. Because these IDs are passed to `SortableContext.items`, dnd-kit will:
1. Use non-reorderable rows as valid `over` targets during a drag — so `event.over?.id` can be a Downloading row's ID, and `computeReorderedIds` silently produces the same order (the dragged reorderable item didn't cross any reorderable boundary), causing the drag to appear to "snap back" with no visible change.
2. Apply CSS transforms to non-reorderable rows (they call `useSortable` and the strategy shifts them to make room), so Downloading/Completed rows will visually slide around while the user is dragging a Queued item — which looks wrong.
Limit `sortableIds` to reorderable rows only:
```suggestion
const sortableIds = useMemo(
() => rows.filter((r) => isReorderable(r.original.state)).map((r) => r.original.id),
[rows],
);
```
With this fix, non-reorderable rows no longer receive transforms or act as collision targets, and `computeReorderedIds` will get an `overId` that is always a reorderable item's ID.
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "fix(download): SQL ORDER BY on find_by_s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (4)
src-tauri/src/domain/event.rs (1)
112-124: Prefer domain type over primitive for event priority payload.Line 117 currently uses
u8; consider emitting the validated domain type (Priority) to keep invariants enforced across publishers/subscribers.♻️ Suggested refactor
+use crate::domain::model::queue::Priority; ... DownloadPrioritySet { id: DownloadId, - priority: u8, + priority: Priority, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/event.rs` around lines 112 - 124, The DownloadPrioritySet event currently carries a primitive u8 for priority which bypasses domain validation; change its payload to use the domain type Priority instead (i.e., DownloadPrioritySet { id: DownloadId, priority: Priority }) so publishers/subscribers preserve invariants, and update all constructors/emitters and match arms handling DownloadPrioritySet to accept and propagate Priority rather than u8 (ensure any conversions/validation occur at the boundary where raw values enter the system).src-tauri/src/adapters/driven/event/tauri_bridge.rs (1)
60-61: Add explicit bridge-contract tests for the new queue events.The mapping is good, but it’s worth pinning
event_name+ payload shape forDownloadPrioritySetandQueueReorderedto prevent silent frontend contract regressions.✅ Suggested test additions
#[test] +fn test_event_name_and_payload_queue_management_variants() { + let priority_evt = DomainEvent::DownloadPrioritySet { + id: DownloadId(11), + priority: 9, + }; + let (priority_name, priority_payload) = to_tauri_event(&priority_evt); + assert_eq!(priority_name, "download-priority-set"); + assert_eq!(priority_payload["id"], 11); + assert_eq!(priority_payload["priority"], 9); + + let reorder_evt = DomainEvent::QueueReordered { + affected_ids: vec![DownloadId(2), DownloadId(5), DownloadId(8)], + }; + let (reorder_name, reorder_payload) = to_tauri_event(&reorder_evt); + assert_eq!(reorder_name, "queue-reordered"); + assert_eq!(reorder_payload["affectedIds"], serde_json::json!([2, 5, 8])); +}Also applies to: 139-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs` around lines 60 - 61, Add explicit bridge-contract tests that assert the emitted event name and payload shape for the new DomainEvent variants DomainEvent::DownloadPrioritySet and DomainEvent::QueueReordered as mapped in the event_name logic inside tauri_bridge.rs; create tests that construct each DomainEvent variant, run it through the same serialization/mapping path used by the bridge, and assert the resulting tuple (event_name string and JSON payload keys/types) exactly matches the expected contract (e.g., "download-priority-set" and "queue-reordered" and their required payload fields), mirroring the approach used for existing events so any frontend contract regression fails the test.src-tauri/src/adapters/driven/sqlite/migrations/m20260425_000004_add_queue_position.rs (1)
24-29: Consider a composite queue-order index for default list sorting.Given the default ordering is queue-position then creation time, indexing both fields usually performs better than indexing
queue_positionalone once many rows share the same position (e.g., default0).📈 Optional migration adjustment
.create_index( Index::create() - .name("idx_downloads_queue_position") + .name("idx_downloads_queue_position_created_at") .table(Downloads::Table) .col(Downloads::QueuePosition) + .col(Downloads::CreatedAt) .to_owned(), ) #[derive(DeriveIden)] enum Downloads { Table, QueuePosition, + CreatedAt, }Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/sqlite/migrations/m20260425_000004_add_queue_position.rs` around lines 24 - 29, The created index idx_downloads_queue_position only covers Downloads::QueuePosition; change it to a composite index on (Downloads::QueuePosition, Downloads::CreatedAt) so default sorting by queue position then creation time uses the index — update the Index::create() chain to add .col(Downloads::CreatedAt) and rename the index (e.g., idx_downloads_queue_position_created_at) accordingly; apply the same change to the other identical index creation block referenced (the one around lines 55-59).src/views/DownloadsView/__tests__/DownloadsTable.test.tsx (1)
280-283: Minor: Inconsistent indentation.Lines 281-282 appear to have less indentation than the surrounding object properties, though this may be a display artifact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/DownloadsView/__tests__/DownloadsTable.test.tsx` around lines 280 - 283, Align the inconsistent indentation in the test object literal in DownloadsTable.test.tsx: ensure the properties priority, queuePosition, createdAt and the spread ...overrides are indented uniformly (matching surrounding object properties) so the object is consistently formatted; locate the object that sets priority: 5, queuePosition: 0, createdAt: Date.now(), ...overrides and fix the spacing to match the other properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/application/commands/move_queue.rs`:
- Around line 99-107: The loop in move_queue.rs mutates each Download via
download.with_queue_position(...) and calls self.download_repo().save(...) per
item, which can leave the queue partially reordered if a later save fails; wrap
the whole reorder in a repository-level transaction or batch operation so all
saves are committed atomically (e.g., use a transaction API on
self.download_repo() or implement a batch_save/update_positions that accepts
cmd.ordered_ids and their new positions), perform all find_by_id and prepare
moved entities inside the transaction, then persist them together and rollback
on error to ensure atomicity.
- Around line 91-113: handle_reorder_queue currently renumbers only
cmd.ordered_ids starting at 1, which can produce duplicate queue_position values
for omitted (filtered-out) items; update the logic to fetch the full set of
queueable downloads from download_repo (e.g., all items with
queued/retry/waiting states), merge cmd.ordered_ids into that full list
preserving the relative order of omitted items, then iterate over the combined
ordered list assigning monotonic queue_position values (1..N) and saving each
via download_repo().save; keep publishing DomainEvent::QueueReordered with the
affected_ids derived from the final reordered list.
In `@src/views/DownloadsView/DownloadsTable.tsx`:
- Around line 683-705: The hooks that set up DnD (sortableIds via useMemo,
sensors via useSensors/useSensor, and handleDragEnd via useCallback) are
declared after early returns for loading/empty states which can change the hook
order and break React; move the declarations of sortableIds, sensors, and
handleDragEnd so they are executed before any early return paths (isLoading or
filteredDownloads.length === 0) so the hook order remains stable across renders,
keeping their current implementations and dependencies ([filteredDownloads,
reorderMut], etc.).
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs`:
- Around line 60-61: Add explicit bridge-contract tests that assert the emitted
event name and payload shape for the new DomainEvent variants
DomainEvent::DownloadPrioritySet and DomainEvent::QueueReordered as mapped in
the event_name logic inside tauri_bridge.rs; create tests that construct each
DomainEvent variant, run it through the same serialization/mapping path used by
the bridge, and assert the resulting tuple (event_name string and JSON payload
keys/types) exactly matches the expected contract (e.g., "download-priority-set"
and "queue-reordered" and their required payload fields), mirroring the approach
used for existing events so any frontend contract regression fails the test.
In
`@src-tauri/src/adapters/driven/sqlite/migrations/m20260425_000004_add_queue_position.rs`:
- Around line 24-29: The created index idx_downloads_queue_position only covers
Downloads::QueuePosition; change it to a composite index on
(Downloads::QueuePosition, Downloads::CreatedAt) so default sorting by queue
position then creation time uses the index — update the Index::create() chain to
add .col(Downloads::CreatedAt) and rename the index (e.g.,
idx_downloads_queue_position_created_at) accordingly; apply the same change to
the other identical index creation block referenced (the one around lines
55-59).
In `@src-tauri/src/domain/event.rs`:
- Around line 112-124: The DownloadPrioritySet event currently carries a
primitive u8 for priority which bypasses domain validation; change its payload
to use the domain type Priority instead (i.e., DownloadPrioritySet { id:
DownloadId, priority: Priority }) so publishers/subscribers preserve invariants,
and update all constructors/emitters and match arms handling DownloadPrioritySet
to accept and propagate Priority rather than u8 (ensure any
conversions/validation occur at the boundary where raw values enter the system).
In `@src/views/DownloadsView/__tests__/DownloadsTable.test.tsx`:
- Around line 280-283: Align the inconsistent indentation in the test object
literal in DownloadsTable.test.tsx: ensure the properties priority,
queuePosition, createdAt and the spread ...overrides are indented uniformly
(matching surrounding object properties) so the object is consistently
formatted; locate the object that sets priority: 5, queuePosition: 0, createdAt:
Date.now(), ...overrides and fix the spacing to match the other properties.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1caa002c-80c7-45a7-81ef-22e8ef449e70
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
CHANGELOG.mdpackage.jsonsrc-tauri/src/adapters/driven/event/tauri_bridge.rssrc-tauri/src/adapters/driven/logging/download_log_bridge.rssrc-tauri/src/adapters/driven/sqlite/download_read_repo.rssrc-tauri/src/adapters/driven/sqlite/download_repo.rssrc-tauri/src/adapters/driven/sqlite/entities/download.rssrc-tauri/src/adapters/driven/sqlite/migrations/m20260425_000004_add_queue_position.rssrc-tauri/src/adapters/driven/sqlite/migrations/mod.rssrc-tauri/src/adapters/driven/sqlite/progress_bridge.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/move_queue.rssrc-tauri/src/application/commands/set_priority.rssrc-tauri/src/application/queries/get_downloads.rssrc-tauri/src/application/read_models/download_view.rssrc-tauri/src/application/services/queue_manager.rssrc-tauri/src/domain/event.rssrc-tauri/src/domain/model/download.rssrc-tauri/src/domain/model/views.rssrc-tauri/src/lib.rssrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/types/download.tssrc/views/DownloadsView/DownloadsTable.tsxsrc/views/DownloadsView/__tests__/DownloadsTable.test.tsxsrc/views/DownloadsView/__tests__/DownloadsView.test.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/application/commands/move_queue.rs">
<violation number="1" location="src-tauri/src/application/commands/move_queue.rs:100">
P1: `handle_reorder_queue` can partially persist position changes before failing on a later missing ID, leaving queue order in an inconsistent intermediate state.</violation>
<violation number="2" location="src-tauri/src/application/commands/move_queue.rs:104">
P1: Reordering renumbers only the submitted IDs to `1..N`, which can collide with `queue_position` values on omitted queued/retry/waiting downloads. Reindex against the full reorderable set (or require callers to submit the complete set) to preserve a consistent global queue order.</violation>
</file>
<file name="src/views/DownloadsView/DownloadsTable.tsx">
<violation number="1" location="src/views/DownloadsView/DownloadsTable.tsx:395">
P1: Depend on `sortable.setNodeRef` instead of the full `sortable` object to prevent constant ref recreation during drag.</violation>
<violation number="2" location="src/views/DownloadsView/DownloadsTable.tsx:684">
P1: Map `sortableIds` from the visually sorted `rows` instead of `filteredDownloads` to ensure dnd-kit's sorting strategies calculate correctly when column sorts are active.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Move dnd hooks before early returns in DownloadsTable to satisfy rules of hooks - Build sortableIds from visually sorted rows (not filteredDownloads) so dnd-kit collision matches the rendered order - Drop full sortable object from setRef deps; depend on setNodeRef only - Reject move_to_top/move_to_bottom on non-reorderable states - Use POSITION_STRIDE=1024 for top/bottom moves to avoid early i64 saturation - Reorder: validate all submitted ids, merge omitted reorderable items into the renumbering so a filtered submission cannot leave duplicate queue_position values - Add save_batch to DownloadRepository (default sequential impl) and override on SQLite with a transaction so reorder commits atomically
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/application/commands/move_queue.rs">
<violation number="1" location="src-tauri/src/application/commands/move_queue.rs:162">
P2: Reject duplicate IDs in `ordered_ids`; currently duplicates are accepted and can produce inconsistent queue positions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -183,6 +184,7 @@ impl Download { | |||
| downloaded_bytes: 0, | |||
There was a problem hiding this comment.
Every download is created with queue_position: 0. After a user performs any reorder operation, existing reorderable items are assigned positions 1..N. A newly added download retains the default position 0, which sorts before all reordered items (0 < 1). This causes the new download to jump to the front of the scheduling queue regardless of the user's explicitly established order, violating the user's intent.
The QueueManager scheduling sort (b.priority()...then_with(|| a.queue_position()...)) will consistently pick the new 0-position item over all reordered items of equal priority.
Fix: when creating a download, compute and assign the max(current_queue_positions) + 1 (or an appropriate stride value), similar to handle_move_to_bottom, so it appends to the back of the current queue rather than hijacking position 0.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src-tauri/src/domain/model/download.rs
Line: 184
Comment:
**New downloads skip reordered queue**
Every download is created with `queue_position: 0`. After a user performs any reorder operation, existing reorderable items are assigned positions `1..N`. A newly added download retains the default position `0`, which sorts *before* all reordered items (`0 < 1`). This causes the new download to jump to the front of the scheduling queue regardless of the user's explicitly established order, violating the user's intent.
The `QueueManager` scheduling sort (`b.priority()...then_with(|| a.queue_position()...)`) will consistently pick the new 0-position item over all reordered items of equal priority.
Fix: when creating a download, compute and assign the `max(current_queue_positions) + 1` (or an appropriate stride value), similar to `handle_move_to_bottom`, so it appends to the back of the current queue rather than hijacking position 0.
How can I resolve this? If you propose a fix, please make it concise.
- Reject duplicate ids in reorder_queue ordered_ids - Assign back-of-queue position to new downloads (start_download, redownload) so they don't jump in front of items the user has explicitly reordered
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/application/commands/move_queue.rs`:
- Around line 48-54: The current next_queue_position (and similar blocks at
lines 71-84 and 107-120) reads the current extremum then computes and persists a
new queue_position outside a transaction, which allows races; fix by moving the
extremum lookup + write into a single repository-level atomic operation or lock.
Add a new method on the DownloadRepository trait such as
allocate_next_queue_position(&self) -> Result<i64, AppError> (or a transactional
helper like with_transaction/execute_atomic that takes a closure) and implement
it so the repository performs the max(queue_position) query and inserts/returns
the new saturating_add(POSITION_STRIDE) value inside one DB transaction/lock;
then replace callers of next_queue_position (and the analogous
prev/insert-at-extreme logic in the other blocks) to call this new atomic
repository method instead of doing load_reorderable_pool() -> compute -> save
outside a transaction.
- Around line 176-180: The current sort uses (d.queue_position(), d.id().0)
which reorders items that legitimately share a queue_position; preserve the
existing omitted order by capturing the iteration order as a stable secondary
key instead of using id(). Replace the collection of omitted with an enumerated
collect (e.g., capture the original index from pool_index.values().enumerate()),
then sort_by_key using (d.queue_position(), original_index) and finally map back
to Vec<&Download>, referencing the existing symbols omitted, pool_index,
submitted, Download and the method queue_position() to locate where to change
the sort.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c61768f-7c1e-4a75-a196-7c04f192e069
📒 Files selected for processing (3)
src-tauri/src/application/commands/move_queue.rssrc-tauri/src/application/commands/redownload.rssrc-tauri/src/application/commands/start_download.rs
Comment on lines
+48
to
+54
| pub(crate) fn next_queue_position(repo: &dyn DownloadRepository) -> Result<i64, AppError> { | ||
| let pool = load_reorderable_pool(repo)?; | ||
| Ok(match pool.iter().map(|d| d.queue_position()).max() { | ||
| Some(m) => m.saturating_add(POSITION_STRIDE), | ||
| None => 0, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Make extreme-position allocation atomic.
These paths all do read current min/max -> derive new queue_position -> save outside a transaction. Two concurrent create/move commands can therefore persist the same extreme value, so “append to back” / “move to top” / “move to bottom” stops being deterministic. Please move the extremum lookup and write behind one repository-level atomic operation or lock.
Also applies to: 71-84, 107-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/application/commands/move_queue.rs` around lines 48 - 54, The
current next_queue_position (and similar blocks at lines 71-84 and 107-120)
reads the current extremum then computes and persists a new queue_position
outside a transaction, which allows races; fix by moving the extremum lookup +
write into a single repository-level atomic operation or lock. Add a new method
on the DownloadRepository trait such as allocate_next_queue_position(&self) ->
Result<i64, AppError> (or a transactional helper like
with_transaction/execute_atomic that takes a closure) and implement it so the
repository performs the max(queue_position) query and inserts/returns the new
saturating_add(POSITION_STRIDE) value inside one DB transaction/lock; then
replace callers of next_queue_position (and the analogous prev/insert-at-extreme
logic in the other blocks) to call this new atomic repository method instead of
doing load_reorderable_pool() -> compute -> save outside a transaction.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/application/commands/move_queue.rs">
<violation number="1" location="src-tauri/src/application/commands/move_queue.rs:49">
P1: Compute-and-assign of the next queue position should be atomic with persistence; otherwise concurrent creates can allocate the same `queue_position` and break deterministic queue ordering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Add CommandBus.queue_position_lock (tokio Mutex) so move_to_top, move_to_bottom, reorder_queue, start_download and redownload serialize their min/max read + persist; concurrent calls cannot allocate the same position any more - Reorder: when omitted items share a queue_position, use the pool's original iteration order as the tie-breaker instead of id, so a filtered submit cannot silently reshuffle hidden downloads
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src-tauri/src/application/commands/start_download.rs (1)
561-564: Consider referencing the stride constant in the test assertion.The assertion hardcodes
5 + 1024which creates implicit coupling toPOSITION_STRIDE. If the stride changes, this test will fail without obvious cause.♻️ Suggested improvement
+ use crate::application::commands::move_queue::POSITION_STRIDE; assert_eq!( saved.queue_position(), - 5 + 1024, + 5 + POSITION_STRIDE, "new download must append after the highest existing reorderable position" );Note: This requires making
POSITION_STRIDEpub(crate)or exposing a test helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/start_download.rs` around lines 561 - 564, The test currently hardcodes the expected queue position as `5 + 1024`; change it to reference the stride constant instead (e.g. `5 + POSITION_STRIDE`) so the assertion uses the source-of-truth `POSITION_STRIDE` when checking `saved.queue_position()`; to enable this, make `POSITION_STRIDE` reachable to the test (e.g. change its visibility to `pub(crate)` or expose a small test-only helper) and update the assertion to compute the expected value using that symbol.src-tauri/src/application/commands/move_queue.rs (1)
607-648: Test coverage note: multi-item tie-breaker scenario not fully exercised.This test has only one omitted item (
d2), so the tie-breaker between multiple omitted items sharingqueue_positionisn't validated. Consider adding a test with 2+ omitted items at the same position to confirm relative order preservation.🧪 Additional test case
#[tokio::test] async fn test_reorder_queue_preserves_multiple_omitted_items_order_on_tie() { // d1, d2, d3 all share queue_position 0. Submit only d1. // d2 and d3 must retain their relative insertion order. let repo = MockRepo::new() .with(make_download(1, 0)) // inserted first .with(make_download(2, 0)) // inserted second .with(make_download(3, 0)); // inserted third let events = Arc::new(RecordingBus::new()); let bus = make_bus(repo, events); bus.handle_reorder_queue(ReorderQueueCommand { ordered_ids: vec![DownloadId(1)], }) .await .unwrap(); let d1 = bus.download_repo().find_by_id(DownloadId(1)).unwrap().unwrap(); let d2 = bus.download_repo().find_by_id(DownloadId(2)).unwrap().unwrap(); let d3 = bus.download_repo().find_by_id(DownloadId(3)).unwrap().unwrap(); assert_eq!(d1.queue_position(), 1, "submitted item first"); // d2 and d3 should follow in their original insertion order assert!(d2.queue_position() < d3.queue_position(), "omitted items must preserve relative order"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/move_queue.rs` around lines 607 - 648, The current test test_reorder_queue_preserves_omitted_order_on_position_tie only covers one omitted item; add a second test (e.g., test_reorder_queue_preserves_multiple_omitted_items_order_on_tie) that creates three MockRepo entries via make_download with the same queue_position (0), calls bus.handle_reorder_queue(ReorderQueueCommand { ordered_ids: vec![DownloadId(1)] }), then fetches items via bus.download_repo().find_by_id and asserts the submitted item has the lowest queue_position and that the two omitted items preserve their original relative order (e.g., assert!(d2.queue_position() < d3.queue_position())). Use the same helpers (MockRepo, make_download, RecordingBus, make_bus, DownloadId) so the test exercises the multi-omitted-item tie-breaker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/application/commands/move_queue.rs`:
- Around line 607-648: The current test
test_reorder_queue_preserves_omitted_order_on_position_tie only covers one
omitted item; add a second test (e.g.,
test_reorder_queue_preserves_multiple_omitted_items_order_on_tie) that creates
three MockRepo entries via make_download with the same queue_position (0), calls
bus.handle_reorder_queue(ReorderQueueCommand { ordered_ids: vec![DownloadId(1)]
}), then fetches items via bus.download_repo().find_by_id and asserts the
submitted item has the lowest queue_position and that the two omitted items
preserve their original relative order (e.g., assert!(d2.queue_position() <
d3.queue_position())). Use the same helpers (MockRepo, make_download,
RecordingBus, make_bus, DownloadId) so the test exercises the multi-omitted-item
tie-breaker.
In `@src-tauri/src/application/commands/start_download.rs`:
- Around line 561-564: The test currently hardcodes the expected queue position
as `5 + 1024`; change it to reference the stride constant instead (e.g. `5 +
POSITION_STRIDE`) so the assertion uses the source-of-truth `POSITION_STRIDE`
when checking `saved.queue_position()`; to enable this, make `POSITION_STRIDE`
reachable to the test (e.g. change its visibility to `pub(crate)` or expose a
small test-only helper) and update the assertion to compute the expected value
using that symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d79e8932-3f90-431b-9b50-10da8db67c03
📒 Files selected for processing (4)
src-tauri/src/application/command_bus.rssrc-tauri/src/application/commands/move_queue.rssrc-tauri/src/application/commands/redownload.rssrc-tauri/src/application/commands/start_download.rs
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/application/commands/move_queue.rs">
<violation number="1" location="src-tauri/src/application/commands/move_queue.rs:192">
P2: Tie-breaking omitted items by pool index is nondeterministic because `find_by_state` does not enforce SQL ordering. Equal `queue_position` items can be reshuffled unpredictably.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .enumerate() | ||
| .filter(|(_, d)| !submitted.contains(&d.id())) | ||
| .collect(); | ||
| omitted.sort_by_key(|(idx, d)| (d.queue_position(), *idx)); |
There was a problem hiding this comment.
P2: Tie-breaking omitted items by pool index is nondeterministic because find_by_state does not enforce SQL ordering. Equal queue_position items can be reshuffled unpredictably.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/src/application/commands/move_queue.rs, line 192:
<comment>Tie-breaking omitted items by pool index is nondeterministic because `find_by_state` does not enforce SQL ordering. Equal `queue_position` items can be reshuffled unpredictably.</comment>
<file context>
@@ -170,22 +180,24 @@ impl CommandBus {
+ .filter(|(_, d)| !submitted.contains(&d.id()))
.collect();
- omitted.sort_by_key(|d| (d.queue_position(), d.id().0));
+ omitted.sort_by_key(|(idx, d)| (d.queue_position(), *idx));
- let mut final_order: Vec<&Download> = Vec::with_capacity(pool_index.len());
</file context>
…eaker PR #105 review pointed out the reorder tie-breaker relied on `find_by_state` iteration order, which had no SQL ORDER BY clause. Add `(queue_position ASC, created_at ASC, id ASC)` to the SQLite query so the pool order is stable across processes; mirror the same ordering in the in-memory MockRepo used by command tests.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src-tauri/src/application/commands/move_queue.rs (2)
173-178: Minor: Redundant reorderability check for items already in the reorderable pool.Items in
pool_indexcome fromload_reorderable_poolwhich only loadsREORDERABLE_STATES, sois_reorderable(download.state())at line 173 will always returntrue. This check is defensive but could be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/move_queue.rs` around lines 173 - 178, The check guarding is_reorderable(download.state()) is redundant because items in pool_index are populated by load_reorderable_pool which already filters to REORDERABLE_STATES; remove the entire if-block that returns AppError in move_queue.rs (the block referencing is_reorderable(download.state()) and AppError::Validation) and either rely on the existing pool invariants or replace it with a debug_assert!(is_reorderable(download.state())) for defensive debug-time validation; ensure you update any surrounding logic that assumed the early Err return is possible.
849-871: Consider adding a symmetric test formove_to_bottomsaturating ati64::MAX.There's a test for
move_to_topsaturating ati64::MIN, but no corresponding test formove_to_bottomati64::MAX. While thesaturating_addat line 120 handles this correctly, a test would document the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/move_queue.rs` around lines 849 - 871, Add a new async #[tokio::test] named something like test_move_to_bottom_saturates_at_max_i64 that mirrors test_move_to_top_saturates_at_min_i64: create a MockRepo with make_download(1, i64::MAX) and make_download(2, 0), create events/ bus via RecordingBus::new() and make_bus, call bus.handle_move_to_bottom(MoveToBottomCommand { id: DownloadId(2) }).await.unwrap(), then fetch the moved entry via bus.download_repo().find_by_id(DownloadId(2)).unwrap().unwrap() and assert that moved.queue_position() == i64::MAX to verify saturation without overflow; reference MoveToBottomCommand, DownloadId, make_download, MockRepo, RecordingBus, make_bus, and queue_position() when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/application/commands/move_queue.rs`:
- Around line 173-178: The check guarding is_reorderable(download.state()) is
redundant because items in pool_index are populated by load_reorderable_pool
which already filters to REORDERABLE_STATES; remove the entire if-block that
returns AppError in move_queue.rs (the block referencing
is_reorderable(download.state()) and AppError::Validation) and either rely on
the existing pool invariants or replace it with a
debug_assert!(is_reorderable(download.state())) for defensive debug-time
validation; ensure you update any surrounding logic that assumed the early Err
return is possible.
- Around line 849-871: Add a new async #[tokio::test] named something like
test_move_to_bottom_saturates_at_max_i64 that mirrors
test_move_to_top_saturates_at_min_i64: create a MockRepo with make_download(1,
i64::MAX) and make_download(2, 0), create events/ bus via RecordingBus::new()
and make_bus, call bus.handle_move_to_bottom(MoveToBottomCommand { id:
DownloadId(2) }).await.unwrap(), then fetch the moved entry via
bus.download_repo().find_by_id(DownloadId(2)).unwrap().unwrap() and assert that
moved.queue_position() == i64::MAX to verify saturation without overflow;
reference MoveToBottomCommand, DownloadId, make_download, MockRepo,
RecordingBus, make_bus, and queue_position() when adding the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58c78350-cd15-4f5e-96d4-e56494002da4
📒 Files selected for processing (2)
src-tauri/src/adapters/driven/sqlite/download_repo.rssrc-tauri/src/application/commands/move_queue.rs
7 tasks
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
• Implement drag-and-drop queue reordering with dnd-kit and persistent
queue_positioncolumn• Add move-to-top/bottom commands with automatic position assignment
• Subscribe QueueManager to priority+reorder events for immediate rescheduling
• Extend sorting to three-level (priority → queue_position → created_at)
Changes
move_to_top,move_to_bottom,reorder_queuecommands; SQLite migration; event publishingqueue_position BIGINTcolumn with default sortqueue_position ASC → created_at DESCcomputeReorderedIds()helper, move actions in context menuTest Coverage
Summary by cubic
Adds drag-and-drop queue reordering with persistent positions and instant rescheduling when priority or order changes. New downloads now append to the back of the manual queue, with a global queue lock to prevent race conditions. Completes PRD‑v2 P0.12 (task 12).
New Features
@dnd-kit/core+@dnd-kit/sortable+@dnd-kit/utilities; drag handle on Queued/Retry/Waiting rows; menu adds Move to top/Move to bottom.download_move_to_top,download_move_to_bottom,download_reorder_queue; emitsDownloadPrioritySetandQueueReordered.queue_position; default list sort is queue ASC → created_at DESC;queue/queuePositionsort fields supported.download_start,download_redownload) get a back‑of‑queue position, so they don’t jump ahead of manually ordered items.ORDER BY queue_position ASC, created_at ASC, id ASC) ensures stable tie‑breakers across processes.Migration
queue_position BIGINT NOT NULL DEFAULT 0with an index (migrationm20260425_000004_add_queue_position). Run DB migrations before using queue reorder.Written for commit d91a2e9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
i18n