feat(domain): SQLite repo for Package aggregate (task 26)#130
Merged
Conversation
Adds the persistence layer for packages per PRD-v2 §P1.7 / PRD §6.3: - Migration `m20260429_000007` recreates the `packages` table with the full schema (TEXT id, source_type, folder_path, password, auto_extract, priority, created_at) and adds `downloads.package_id` as a FK with ON DELETE SET NULL plus the supporting index. The legacy stub table from migration 1 is dropped (never wired to a repo or query). - New `PackageRepository` driven port with save/find_by_id/list/delete/ list_downloads. `SqlitePackageRepo` adapter upserts via sea-orm with a stable `created_at` and orders `list_downloads` by queue_position. - Domain `Package` aggregate gains `PackageId(String)` typed wrapper, `PackageSourceType` enum, plus the new persisted fields. The in-memory `download_ids` collection is unchanged (FK is the on-disk source of truth). - `DomainEvent::PackageCreated.id` switches from u64 to PackageId. Twenty-one new unit tests cover the four acceptance criteria (fresh + existing-DB migration, FK SET NULL semantics, full-field round-trip, ≥85% adapter coverage), plus error paths, source-type round-trip per variant, and the `InMemoryPackageRepository` mock for upcoming command/query handlers. Unblocks tasks 27, 28, 30, 31.
📝 WalkthroughWalkthroughAdds full Package persistence: new SQLite migration, SeaORM entity, Package domain model with typed Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Domain as DomainService
participant Repo as SqlitePackageRepo
participant DB as SQLite
participant Tauri as TauriBridge
Client->>Domain: create Package(PackageId, source, created_at)
Domain->>Repo: save(package)
Repo->>DB: UPSERT INTO packages (...) ON CONFLICT(id) DO UPDATE ...
DB-->>Repo: OK / Row data
Repo-->>Domain: saved
Domain->>Tauri: emit DomainEvent::PackageCreated(PackageId,...)
Tauri->>Client: to_tauri_event(payload{id: "pkg-1", name: "..."})
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Review rate limit: 3/5 reviews remaining, refill in 13 minutes and 23 seconds. Comment |
github-actions
Bot
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6be4c6ae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/event/tauri_bridge.rs (1)
393-395: Assert thePackageCreatedpayload shape here too.This change mostly affects the frontend contract:
idis now a string. The test only checks the event name, so the bridge could regress back to a numeric payload unnoticed.Suggested assertion
assert_eq!( event_name(&DomainEvent::PackageCreated { id: crate::domain::model::package::PackageId::new("pkg-1"), name: "pkg".into() }), "package-created" ); + let (_, payload) = to_tauri_event(&DomainEvent::PackageCreated { + id: crate::domain::model::package::PackageId::new("pkg-1"), + name: "pkg".into(), + }); + assert_eq!(payload["id"], "pkg-1"); + assert_eq!(payload["name"], "pkg");🤖 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 393 - 395, The test currently only asserts the event name when constructing DomainEvent::PackageCreated; update the test to also assert the event payload shape by extracting/deserializing the emitted event payload and verifying that the PackageCreated payload contains id as a string (e.g., "pkg-1") and name "pkg"; locate the test that uses event_name(&DomainEvent::PackageCreated { ... }) and add assertions against the payload (or use the existing helper that returns the full event object) to check DomainEvent::PackageCreated.id is a string and DomainEvent::PackageCreated.name == "pkg".
🤖 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/adapters/driven/sqlite/entities/package.rs`:
- Around line 27-44: The converter method into_domain currently coerces
self.auto_extract via self.auto_extract != 0 which silently treats invalid
values (e.g., 2 or -1) as true; change into_domain to explicitly validate
auto_extract so only 0 maps to false and 1 maps to true and return a
DomainError::ValidationError for any other value (include package id and the
invalid auto_extract value in the error message); update the call to
Package::reconstruct to pass the validated bool instead of self.auto_extract !=
0.
In `@src-tauri/src/domain/model/package.rs`:
- Around line 29-67: The enum PackageSourceType currently documents that values
are persisted as lower-snake-case but the Display/FromStr implementations use
"split-archive" for PackageSourceType::SplitArchive; update the
formatting/parsing to use "split_archive" to match the documented
lower_snake_case contract: change the fmt::Display match arm for
PackageSourceType::SplitArchive to return "split_archive" and change the FromStr
arm to accept "split_archive" (and keep other arms as-is) so Display and FromStr
round-trip consistently.
- Around line 70-71: The Package aggregate must enforce the 1..=10 priority
invariant at creation and mutation: update the reconstruct function and
set_priority method (and any constructor/associated fn that accepts a priority)
to validate the incoming u8 and reject values outside 1..=10 rather than
accepting them; return a Result (or domain error) on invalid input so the
aggregate cannot hold an invalid priority (use DEFAULT_PACKAGE_PRIORITY only as
a default when no explicit value is provided), and ensure error type/message
clearly indicates "priority must be between 1 and 10" while referencing the
Package::reconstruct and Package::set_priority code paths.
In `@src-tauri/src/domain/ports/driven/tests.rs`:
- Around line 746-753: The mock's attach_download currently appends downloads
but must preserve the ordering contract used by list_downloads/SqlitePackageRepo
(ordered by queue_position); change attach_download (and the similar helper
around the other occurrence) to insert each DownloadId into the package's Vec in
queue_position order instead of push-only — e.g., grab the Vec via
members.lock().unwrap().entry(...).or_default(), find the insertion index by
comparing download.queue_position with existing entries (binary_search_by or
linear scan) and insert at that index so list_downloads will return members
sorted by queue_position.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs`:
- Around line 393-395: The test currently only asserts the event name when
constructing DomainEvent::PackageCreated; update the test to also assert the
event payload shape by extracting/deserializing the emitted event payload and
verifying that the PackageCreated payload contains id as a string (e.g.,
"pkg-1") and name "pkg"; locate the test that uses
event_name(&DomainEvent::PackageCreated { ... }) and add assertions against the
payload (or use the existing helper that returns the full event object) to check
DomainEvent::PackageCreated.id is a string and DomainEvent::PackageCreated.name
== "pkg".
🪄 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: 48440ed0-f718-40e2-b5a1-755625ec63e2
📒 Files selected for processing (16)
CHANGELOG.mdsrc-tauri/src/adapters/driven/event/tauri_bridge.rssrc-tauri/src/adapters/driven/sqlite/connection.rssrc-tauri/src/adapters/driven/sqlite/entities/mod.rssrc-tauri/src/adapters/driven/sqlite/entities/package.rssrc-tauri/src/adapters/driven/sqlite/migrations/m20260429_000007_create_packages.rssrc-tauri/src/adapters/driven/sqlite/migrations/mod.rssrc-tauri/src/adapters/driven/sqlite/mod.rssrc-tauri/src/adapters/driven/sqlite/package_repo.rssrc-tauri/src/domain/event.rssrc-tauri/src/domain/model/mod.rssrc-tauri/src/domain/model/package.rssrc-tauri/src/domain/ports/driven/mod.rssrc-tauri/src/domain/ports/driven/package_repository.rssrc-tauri/src/domain/ports/driven/tests.rssrc-tauri/src/lib.rs
- Migration m20260429_000007 down() now restores the legacy packages
table (BIGINT id, name, created_at) so rolling back lands on the
same shape migration 6 had pre-007 instead of leaving migration
state 6 with no packages table at all.
- entities/package.rs::Model::into_domain now rejects auto_extract
values outside {0,1} via DomainError::ValidationError, matching the
fail-fast behaviour already in place for source_type / priority /
created_at.
- PackageSourceType::SplitArchive now serialises as "split_archive"
(lower_snake_case) instead of "split-archive", matching the doc
contract on the enum.
- Package::reconstruct + Package::set_priority enforce the documented
1..=10 priority invariant inside the aggregate via a new
validate_package_priority helper. reconstruct now returns
Result<Self, DomainError>; set_priority returns Result<(), _>.
All call sites updated.
- InMemoryPackageRepository mock now stores members as
(queue_position, DownloadId) tuples and list_downloads sorts by
queue_position, mirroring the SqlitePackageRepo contract so
port-level tests cannot diverge from production.
- tauri_bridge PackageCreated test asserts the payload shape (id as
string, name) in addition to the event name, so a regression to the
numeric id payload would be caught.
Owner
Author
|
Addressed the nitpick on |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adapters/driven/sqlite/entities/package.rs`:
- Around line 27-54: The converter into_domain currently silently normalizes
negative created_at via safe_u64(self.created_at); change it to validate
created_at first: if self.created_at is negative return
Err(DomainError::ValidationError(...)) with a clear message including self.id
and the invalid value, otherwise convert to u64 (e.g., call safe_u64 or cast)
and pass that into Package::reconstruct; update the into_domain function to
perform this check before constructing the Package so negative timestamps are
rejected like source_type, auto_extract, and priority.
🪄 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: e68c0beb-e3ec-47eb-a5a8-2e550c2b3296
📒 Files selected for processing (7)
CHANGELOG.mdsrc-tauri/src/adapters/driven/event/tauri_bridge.rssrc-tauri/src/adapters/driven/sqlite/entities/package.rssrc-tauri/src/adapters/driven/sqlite/migrations/m20260429_000007_create_packages.rssrc-tauri/src/adapters/driven/sqlite/package_repo.rssrc-tauri/src/domain/model/package.rssrc-tauri/src/domain/ports/driven/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/adapters/driven/sqlite/migrations/m20260429_000007_create_packages.rs
Replace safe_u64(self.created_at) with strict u64::try_from on Model::into_domain so a corrupt row carrying a negative timestamp surfaces as DomainError::ValidationError instead of being silently coerced to 0 and jumping to the front of the (created_at asc, id asc) ordering used by find_by_id / list. Matches the fail-fast contract already in place for source_type, auto_extract, and priority. Drops the now-unused safe_u64 import. New regression test test_find_by_id_rejects_negative_created_at seeds a row with created_at = -1 and asserts the validation path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adapters/driven/sqlite/package_repo.rs`:
- Around line 101-104: The SQL string passed to Statement::from_sql_and_values
with sea_orm::DatabaseBackend::Sqlite uses a PostgreSQL-style placeholder `$1`,
which SQLite does not accept; update the query in the call to query_all /
Statement::from_sql_and_values to use a `?` placeholder ("SELECT id FROM
downloads WHERE package_id = ? ORDER BY queue_position ASC, id ASC") and leave
the parameter list ([id_value.into()]) unchanged so the binding still works.
🪄 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: 5d23d20f-86cf-4277-b40d-7d18086c920b
📒 Files selected for processing (2)
src-tauri/src/adapters/driven/sqlite/entities/package.rssrc-tauri/src/adapters/driven/sqlite/package_repo.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/adapters/driven/sqlite/entities/package.rs
Replace PostgreSQL-style `$1` placeholder with SQLite-native `?` in the raw SQL passed to Statement::from_sql_and_values with DatabaseBackend::Sqlite. Per SeaORM's raw SQL docs, SQLite expects `?` and `$N` is reserved for the PostgreSQL backend. The query behaviour was incidentally correct in tests but relied on SQLx's compatibility layer; switching to `?` matches the documented contract and the convention already used elsewhere in the sqlite/ module.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/sqlite/package_repo.rs (1)
136-155: Parameterize the download seed helper instead of formattingpackage_idinto SQL.Now that
PackageIdis aString, this helper breaks on ids containing quotes and can hide repository failures behind malformed test SQL. ReuseStatement::from_sql_and_valueshere too; while you’re touching it, the doc comment above should stop claiming the helper returns an id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/sqlite/package_repo.rs` around lines 136 - 155, The helper insert_download_in_package currently formats package_id directly into the SQL string which breaks on quotes; change it to use a parameterized statement via Statement::from_sql_and_values, binding download_id, queue_position and package_id (use NULL when package_id is None) instead of interpolating pkg, and call db.execute with that statement; also update the doc comment above insert_download_in_package to remove the incorrect claim that it returns an id.
🤖 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/adapters/driven/sqlite/package_repo.rs`:
- Around line 136-155: The helper insert_download_in_package currently formats
package_id directly into the SQL string which breaks on quotes; change it to use
a parameterized statement via Statement::from_sql_and_values, binding
download_id, queue_position and package_id (use NULL when package_id is None)
instead of interpolating pkg, and call db.execute with that statement; also
update the doc comment above insert_download_in_package to remove the incorrect
claim that it returns an id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 939320d6-0181-4dbc-8be1-9c7973e29e27
📒 Files selected for processing (1)
src-tauri/src/adapters/driven/sqlite/package_repo.rs
This was referenced Apr 29, 2026
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
Persists the
Packageaggregate to SQLite with full CRUD operations and proper foreign key handling. Implements the repository pattern per PRD-v2 §P1.7, unblocking downstream package management tasks (27–31).Why
Packages (containers, playlists, manual groups, split archives) were in-memory only. Persistence enables user workflows: install container → convert to package → redownload members, or group related downloads manually. Foreign key
ON DELETE SET NULLpreserves download records even if their package is deleted.Changes
Packageaggregate with typedPackageId(String, not u64) and newPackageSourceTypeenum (Container, Playlist, Manual, SplitArchive). Added setters for folder_path, password, auto_extract, priority. Kept in-memorydownload_idsvec separate from persisted FK inverse.PackageRepositorytrait with methodsfind_by_id,save,list,delete,list_downloads. Contracts match hexagonal architecture (domain never knows about ORM).m20260429_000007_create_packagescreates table with TEXT primary key, source_type enum, optional folder_path/password, defaults for auto_extract (1) and priority (5). Adds package_id FK to downloads with SET NULL cascade. Creates idx_downloads_package.SqlitePackageRepoimplements trait via sea-orm upsert (insert…on_conflict). Preserves created_at timestamp on update. Uses raw SQL for list_downloads since download entity FK not yet exposed (future task).into_domain()andfrom_domain()converters. Handles type conversions: source_type string ↔ enum, i64 timestamp → u64, validation on unknown enum variants.Testing
package.rscovering creation, setters, source-type round-trip, reconstruction.package_repo.rscovering save/find/list/delete, upsert with created_at preservation, FK SET NULL validation, list_downloads ordering.ports/driven/tests.rsin-memory mock implementation.```bash
cargo test --workspace -- --nocapture
cargo llvm-cov --workspace --fail-under-lines 85
```
All 1204 tests pass (1201 existing + 3 new error-path).
Related Issues
Notes for Reviewer
package_idtype changed from u64 to String (TEXT primary key in SQL). Required touchingDomainEvent::PackageCreatedto usePackageIdwrapper instead of raw u64.list_downloads()uses raw SQL parameterized query because download entity hasn't yet exposed the new package_id FK field (that wiring happens in future tasks). This keeps the repo self-contained without blocking on downstream changes.save()explicitly excludescreated_atfrom the upsert update columns via sea-orm'son_conflict().do_update().col()chain. This prevents timestamp mutation on subsequent saves.Checklist
.unwrap()outside testsSummary by cubic
Adds SQLite persistence for packages with a new
PackageRepository+SqlitePackageRepo, full CRUD, and strict validation. Implements Task-26 (PRD-v2 §P1.7) and updates events to use stringPackageId.New Features
find_by_id,save(upsert, preservescreated_at),list(ordered by(created_at, id)),delete,list_downloads(ordered byqueue_position; raw SQL now uses?placeholder for SQLite).PackageId(String),PackageSourceType(serializes assplit_archive), setters; priority 1..=10 enforced;auto_extractis 0/1;created_atmust be non-negative;DomainEvent::PackageCreated.idisPackageId.Migration
packagestable; adddownloads.package_id TEXT REFERENCES packages(id) ON DELETE SET NULLandidx_downloads_package.packagestable (BIGINT id, name, created_at).Written for commit f13c1b2. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests