feat(domain): package commands CRUD + cascade actions (task 27)#131
Merged
feat(domain): package commands CRUD + cascade actions (task 27)#131
Conversation
Nine command handlers wired through CommandBus: - create_package / update_package / delete_package (CRUD) - set_package_password (keyring-only secrets) - set_package_priority (cascade DownloadPrioritySet per child) - move_package_to_folder (re-uses task 13 change_directory) - toggle_package_auto_extract - add_download_to_package / remove_download_from_package PackageRepository trait gains attach_download/detach_download for the FK singleton on downloads.package_id; SqlitePackageRepo implements both via raw UPDATE so semantics match the existing ON DELETE SET NULL migration. DomainEvent gains PackageUpdated and PackageDeleted (forwarded to the frontend as package-updated and package-deleted). Nine Tauri IPC commands registered in invoke_handler. 43 new application-layer tests against in-memory mocks plus 5 SQLite-level tests; per-handler line coverage ≥94%, well above the 85% acceptance bar.
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds end-to-end package management: new DomainEvents, PackageRepository operations and SQLite adapter, CommandBus handlers and Tauri IPC for package CRUD and member operations, event forwarding to the frontend, and extensive unit and SQLite tests. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (IPC)
participant IPC as Tauri IPC Handler
participant CB as CommandBus
participant Repo as PackageRepository
participant DB as SQLite
participant EB as Event Bus
participant TB as Tauri Event Bridge
FE->>IPC: package_create(name, folder_path)
IPC->>CB: handle_create_package(cmd)
CB->>CB: validate & generate PackageId
CB->>Repo: save(package)
Repo->>DB: INSERT packages
Repo-->>CB: Ok
CB->>EB: publish PackageCreated{id, name}
EB->>TB: forward event
TB->>FE: emit("package-created", {id, name})
CB-->>IPC: Ok(PackageId)
IPC-->>FE: PackageId
sequenceDiagram
participant FE as Frontend (IPC)
participant IPC as Tauri IPC Handler
participant CB as CommandBus
participant Repo as PackageRepository
participant DownloadRepo as DownloadRepository
participant DB as SQLite
participant EB as Event Bus
participant TB as Tauri Event Bridge
FE->>IPC: package_add_download(package_id, download_id)
IPC->>CB: handle_add_download_to_package(cmd)
CB->>Repo: load_package(package_id)
Repo->>DB: SELECT packages
alt package found
CB->>DownloadRepo: load(download_id)
alt download found
CB->>Repo: attach_download(package_id, download_id)
Repo->>DB: UPDATE downloads SET package_id=?
alt download row affected
Repo-->>CB: Ok
CB->>EB: publish PackageUpdated{id}
EB->>TB: forward event
TB->>FE: emit("package-updated", {id})
CB-->>IPC: Ok(())
else
Repo-->>CB: NotFound
CB-->>IPC: NotFound Error
end
else
DownloadRepo-->>CB: NotFound
CB-->>IPC: NotFound Error
end
else
Repo-->>CB: NotFound
CB-->>IPC: NotFound Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 4/5 reviews remaining, refill in 12 minutes. Comment |
![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: 4b192ddb77
ℹ️ 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 (4)
src-tauri/src/application/commands/toggle_package_auto_extract.rs (1)
54-102: Consider asserting the publish side effect in the toggle test.The test proves persistence and the returned boolean, but it never verifies that
PackageUpdatedis emitted, so an event regression could still slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/toggle_package_auto_extract.rs` around lines 54 - 102, The test test_toggle_auto_extract_flips_value_and_returns_new_state currently asserts persistence and return values but doesn’t verify that a PackageUpdated event was published; update the test to inspect the CapturingEventBus (used when building the bus) after each handle_toggle_package_auto_extract call and assert that a PackageUpdated event was emitted with the expected PackageId and the new auto_extract value (or at least that one PackageUpdated event exists and its payload matches id and auto_extract state), and optionally assert event count increments between the first and second toggles to ensure both updates were published.src-tauri/src/domain/ports/driven/tests.rs (1)
814-845: Mirror the newattach_downloadfailure path more closely.This mock updates membership correctly, but it still cannot surface
DomainError::NotFoundfor a missing download id because it has no download lookup. That leaves the new contract's error branch unexercised in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/ports/driven/tests.rs` around lines 814 - 845, The mock's attach_download currently never returns DomainError::NotFound because it doesn't verify the download exists; update the mock (e.g., add a downloads set or reference to the download lookup used in tests) and change attach_download to first check that the given DownloadId exists and return Err(DomainError::NotFound) when it does not, while preserving the existing detach_download and membership-retention behavior (use the existing members mutex and ensure attach_download still detaches the id from other packages before inserting).src-tauri/src/application/commands/tests_support.rs (2)
311-315: Align in-memory package ordering with production ordering.
InMemoryPackageRepo::snapshotcurrently sorts only by id. Production package listing iscreated_atthenid, so this fixture can mask order-sensitive regressions.Suggested diff
- let mut packages: Vec<Package> = self.store.lock().unwrap().values().cloned().collect(); - packages.sort_by(|a, b| a.id().as_str().cmp(b.id().as_str())); + let mut packages: Vec<Package> = self.store.lock().unwrap().values().cloned().collect(); + packages.sort_by(|a, b| { + a.created_at() + .cmp(&b.created_at()) + .then_with(|| a.id().as_str().cmp(b.id().as_str())) + }); packagesAlso applies to: 343-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/tests_support.rs` around lines 311 - 315, In InMemoryPackageRepo::snapshot change the sort from only id to first compare package created_at then id so the in-memory fixture matches production ordering (i.e., sort by (created_at, id) instead of only id); implement the comparison using the Package getters available (compare created_at timestamps, fall back to id.as_str()) and apply the same fix to the other snapshot implementation around lines 343-345 to ensure both fixtures use created_at then id ordering.
363-377: Improve fixture fidelity by rejecting attach on missing package.
attach_downloadcurrently creates membership entries even when the package does not exist. Adding an existence guard would make tests closer to real persistence constraints and catch missed upstream validation earlier.Suggested diff
fn attach_download( &self, package_id: &PackageId, download_id: DownloadId, ) -> Result<(), DomainError> { + if !self.store.lock().unwrap().contains_key(package_id) { + return Err(DomainError::NotFound(format!( + "package {} not found", + package_id.as_str() + ))); + } let mut guard = self.members.lock().unwrap(); for entries in guard.values_mut() { entries.retain(|d| d != &download_id); } let bucket = guard.entry(package_id.clone()).or_default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/tests_support.rs` around lines 363 - 377, attach_download currently creates a new membership bucket via guard.entry(...).or_default() even when the package isn't present; change attach_download to check existence with guard.get_mut(package_id) (or guard.contains_key) and if the package key is missing return an appropriate DomainError (for example DomainError::NotFound(package_id.clone())), otherwise retain existing logic to deduplicate and push the download_id into the existing Vec; remove the entry(...).or_default() creation so missing packages are rejected instead of created.
🤖 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/driving/tauri_ipc.rs`:
- Around line 3010-3024: The package_move_to_folder handler currently only
rejects empty new_folder values but allows relative paths; update the
package_move_to_folder function to validate the incoming new_folder by
converting it to a PathBuf and rejecting any path where !is_absolute(),
returning an Err(String) like the empty-path case; keep creating the
MovePackageToFolderCommand with PackageId::new(id) and PathBuf::from(new_folder)
only after the new_folder passes both non-empty and is_absolute() checks so
relative paths (e.g., "../" or "./") are refused before filesystem operations
are invoked.
In `@src-tauri/src/application/commands/delete_package.rs`:
- Around line 37-66: The package delete currently performs child detach/remove,
repo.delete, credential removal, and event publish sequentially, which can leave
a half-deleted package on errors; make the cascade atomic or explicitly report
partial failure: if the repo supports transactions, wrap the loop that calls
handle_remove_download (or repo.detach_download) and repo.delete in a
transaction (begin/commit/rollback) so failures rollback all detach/removes; if
no transaction support, first attempt all child removals/ detaches but collect
errors (treat NotFound from handle_remove_download as non-fatal), and if any
child operation fails revert successful child changes where possible or return a
composed error describing which child ids failed and do NOT call
repo.delete/credential_store().delete/event_bus().publish; ensure
credential_store().delete and event_bus().publish are only executed after a
fully successful package delete and include detailed error reporting if those
later steps fail (reference functions: handle_remove_download,
repo.detach_download, repo.delete, package_credential_service_key,
credential_store().delete, event_bus().publish, DomainEvent::PackageDeleted).
In `@src-tauri/src/application/commands/set_package_password.rs`:
- Around line 38-65: The current flow mutates the credential store via
credentials.store() / credentials.delete() before persisting the package marker
with repo.save(), causing possible divergence if repo.save() fails; change the
order or add an explicit transaction so database state is durable before
modifying the keyring: write the updated Package (Package::reconstruct ->
repo.save) first and only then call credentials.store() or credentials.delete(),
and if the subsequent keyring operation fails attempt to roll back the DB change
(e.g., restore the previous Package state via repo.save(previous)) or wrap both
in a DB transaction if the repo supports it; note that credentials.delete() is
idempotent so clearing can be done safely when rolling back.
In `@src-tauri/src/application/commands/set_package_priority.rs`:
- Around line 38-73: Start a DB transaction before calling Package::reconstruct
and repo.save so the package update plus all cascade updates to members
(repo.list_downloads, download_repo().save for each download) happen atomically;
perform all download updates inside the same transaction and commit only if
every save succeeds, rolling back on any error. Also defer publishing
DomainEvent::DownloadPrioritySet and DomainEvent::PackageUpdated until after the
transaction commits (or publish as part of a transactional outbox) so events are
not emitted for rolled-back changes. Locate the logic around
Package::reconstruct, repo.save(&updated), repo.list_downloads(&cmd.id),
download_repo().save(&next) and event_bus().publish(...) and wrap them in your
repository/connection transaction API, ensuring proper rollback on error.
---
Nitpick comments:
In `@src-tauri/src/application/commands/tests_support.rs`:
- Around line 311-315: In InMemoryPackageRepo::snapshot change the sort from
only id to first compare package created_at then id so the in-memory fixture
matches production ordering (i.e., sort by (created_at, id) instead of only id);
implement the comparison using the Package getters available (compare created_at
timestamps, fall back to id.as_str()) and apply the same fix to the other
snapshot implementation around lines 343-345 to ensure both fixtures use
created_at then id ordering.
- Around line 363-377: attach_download currently creates a new membership bucket
via guard.entry(...).or_default() even when the package isn't present; change
attach_download to check existence with guard.get_mut(package_id) (or
guard.contains_key) and if the package key is missing return an appropriate
DomainError (for example DomainError::NotFound(package_id.clone())), otherwise
retain existing logic to deduplicate and push the download_id into the existing
Vec; remove the entry(...).or_default() creation so missing packages are
rejected instead of created.
In `@src-tauri/src/application/commands/toggle_package_auto_extract.rs`:
- Around line 54-102: The test
test_toggle_auto_extract_flips_value_and_returns_new_state currently asserts
persistence and return values but doesn’t verify that a PackageUpdated event was
published; update the test to inspect the CapturingEventBus (used when building
the bus) after each handle_toggle_package_auto_extract call and assert that a
PackageUpdated event was emitted with the expected PackageId and the new
auto_extract value (or at least that one PackageUpdated event exists and its
payload matches id and auto_extract state), and optionally assert event count
increments between the first and second toggles to ensure both updates were
published.
In `@src-tauri/src/domain/ports/driven/tests.rs`:
- Around line 814-845: The mock's attach_download currently never returns
DomainError::NotFound because it doesn't verify the download exists; update the
mock (e.g., add a downloads set or reference to the download lookup used in
tests) and change attach_download to first check that the given DownloadId
exists and return Err(DomainError::NotFound) when it does not, while preserving
the existing detach_download and membership-retention behavior (use the existing
members mutex and ensure attach_download still detaches the id from other
packages before inserting).
🪄 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: 1244975a-c0d9-4c21-9d7c-189efbcc7ea6
📒 Files selected for processing (21)
CHANGELOG.mdsrc-tauri/src/adapters/driven/event/tauri_bridge.rssrc-tauri/src/adapters/driven/logging/download_log_bridge.rssrc-tauri/src/adapters/driven/sqlite/package_repo.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/command_bus.rssrc-tauri/src/application/commands/add_download_to_package.rssrc-tauri/src/application/commands/create_package.rssrc-tauri/src/application/commands/delete_package.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/move_package_to_folder.rssrc-tauri/src/application/commands/remove_download_from_package.rssrc-tauri/src/application/commands/set_package_password.rssrc-tauri/src/application/commands/set_package_priority.rssrc-tauri/src/application/commands/tests_support.rssrc-tauri/src/application/commands/toggle_package_auto_extract.rssrc-tauri/src/application/commands/update_package.rssrc-tauri/src/domain/event.rssrc-tauri/src/domain/ports/driven/package_repository.rssrc-tauri/src/domain/ports/driven/tests.rssrc-tauri/src/lib.rs
Address inline review feedback on PR #131: - remove_download_from_package: validate FK ownership before detach so a stale package_id paired with a real download_id no longer strips the download from a different package (codex P1). - add_download_to_package: capture previous owner and emit PackageUpdated for the source on hand-off so consumers refresh both buckets, not just the destination (codex P2). - move_package_to_folder: reject relative destination paths so a crafted IPC payload (e.g. \"../escape\") cannot walk outside the working directory before per-download moves run (CodeRabbit major). - set_package_password: persist the marker before touching the keyring so a keyring failure no longer leaves an orphan secret pointing at no DB row; both store and delete remain idempotent so retries converge (CodeRabbit major). PackageRepository gains find_package_of_download for the FK lookup; the SQLite adapter and the in-memory mocks both implement it. Eight new tests cover cross-package detach rejection, source/destination event emission on reassignment, relative-path validation, and the FK lookup contract (returns None for loose or missing rows).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0a5c697e2
ℹ️ 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".
Comment on lines
+64
to
+68
| repo.save(&updated)?; | ||
|
|
||
| match cmd.password.as_deref() { | ||
| Some(secret) => credentials.store(&key, &Credential::new(String::new(), secret))?, | ||
| None => credentials.delete(&key)?, |
There was a problem hiding this comment.
If credentials.store/credentials.delete fails (for example, keychain access denied or unavailable), this handler returns an error after repo.save(&updated) has already committed the new password marker state. That leaves persisted package metadata changed even though the command failed, so callers can see a package pointing to a missing keyring secret (or cleared marker while old secret still exists) and retries become non-idempotent. This should mirror the account handlers’ rollback pattern so DB and keyring stay consistent on failure paths.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
If credentials.store/credentials.delete fails (...), this handler returns an error after repo.save(&updated) has already committed the new password marker state.
Fixed in 04c365f. The handler now rolls the marker row back to the previous value when the keyring write fails, mirroring update_account's recovery path. Best-effort credentials.delete(&key) cleanup wipes any partial keyring entry so the row never points at a half-written secret. Two regression tests (test_set_package_password_keyring_failure_rolls_back_marker_on_set and ..._on_clear) drive the path via an InMemoryCredentialStore fail-mode flag and assert both DB rollback and that no PackageUpdated event leaks for the failed command.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src-tauri/src/application/commands/set_package_password.rs (1)
49-69:⚠️ Potential issue | 🟠 MajorRestore the previous marker if the keyring step fails.
This still leaves the package row and keyring out of sync on credential errors: a failed
storeleaves a marker pointing at no secret, and a faileddeleteleaves a live secret after the marker was cleared. Sinceexistingis already loaded, please best-effort save it back when Lines 67-68 fail before returning the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/set_package_password.rs` around lines 49 - 69, When saving the new marker with Package::reconstruct and repo.save you must roll back the DB marker if the subsequent keyring operation (credentials.store or credentials.delete) fails; change the match on cmd.password.as_deref() so that on Err(e) you best-effort call repo.save(&existing) to restore the previous package row before returning the original error, and ensure you propagate the original credentials error (not the rollback result) while logging or ignoring any rollback error.
🤖 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/tests_support.rs`:
- Around line 363-376: The helper attach_download currently removes download_id
from every bucket (via guard.values_mut().retain) before checking the
destination, so re-attaching the same DownloadId to the same PackageId pushes it
to the end and changes order; change the loop to only remove download_id from
buckets whose key != package_id (e.g., iterate guard.iter_mut() or guard.entries
and call retain only when the entry's PackageId != package_id) so the
destination bucket's existing order is preserved, then proceed to get bucket =
guard.entry(package_id.clone()).or_default() and push only if
!bucket.contains(&download_id).
In `@src-tauri/src/domain/ports/driven/tests.rs`:
- Around line 814-835: The mock's attach_download currently removes the download
from all packages before checking for an existing entry in the target package,
causing a same-package reattach to change queue_position; change the logic in
attach_download so it first checks the target package bucket for an existing
download (using the bucket from guard.entry(package_id.clone()).or_default() and
bucket.iter().any(...)) and returns Ok(()) if present, and only then detach the
download from other packages (iterate guard entries but skip the target package
key when calling retain). This preserves no-op behavior for same-package
reattaches and matches the SQL adapter semantics.
---
Duplicate comments:
In `@src-tauri/src/application/commands/set_package_password.rs`:
- Around line 49-69: When saving the new marker with Package::reconstruct and
repo.save you must roll back the DB marker if the subsequent keyring operation
(credentials.store or credentials.delete) fails; change the match on
cmd.password.as_deref() so that on Err(e) you best-effort call
repo.save(&existing) to restore the previous package row before returning the
original error, and ensure you propagate the original credentials error (not the
rollback result) while logging or ignoring any rollback error.
🪄 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: ecbe14da-1860-498d-8b62-c1ad92922625
📒 Files selected for processing (8)
src-tauri/src/adapters/driven/sqlite/package_repo.rssrc-tauri/src/application/commands/add_download_to_package.rssrc-tauri/src/application/commands/move_package_to_folder.rssrc-tauri/src/application/commands/remove_download_from_package.rssrc-tauri/src/application/commands/set_package_password.rssrc-tauri/src/application/commands/tests_support.rssrc-tauri/src/domain/ports/driven/package_repository.rssrc-tauri/src/domain/ports/driven/tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/src/domain/ports/driven/package_repository.rs
- src-tauri/src/application/commands/remove_download_from_package.rs
…ch noop Round two of PR #131 review feedback: - set_package_password: roll the marker row back to the previous value when `credentials.store`/`delete` fails, mirroring the recovery path in `update_account`. Best-effort cleanup wipes any partially-written keyring entry so callers never observe a row pointing at a missing secret. Two new tests cover both rollback paths via an `InMemoryCredentialStore` fail-mode flag (codex P1). - InMemoryPackageRepo / InMemoryPackageRepository: detach foreign-bucket entries first, then bail when the download already lives in the target bucket. Previous shape blew the queue position away on a same-package reattach and let port tests pass behaviour SQLite never produces (CodeRabbit major, both fixtures). Two regression tests assert order survives a same-package reattach; 1269 tests pass (was 1265).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04c365f4d8
ℹ️ 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".
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/application/commands/set_package_password.rs`:
- Around line 71-96: Before performing repo.save(&existing), read and stash the
current keyring value for the package key (the value retrieved via
credentials.get or equivalent for the same key used with
credentials.store/credentials.delete). If the keyring_op returns Err(e) and you
roll back the DB with repo.save(&existing), restore the previously stashed
credential back into the keyring using credentials.store(&key,
&stashed_credential) instead of unconditionally calling
credentials.delete(&key); only delete on explicit user removal. Update the
error-handling block around keyring_op, repo.save(&existing), and
credentials.delete(&key) to use the stashed value (and log any restore errors)
so transient keyring failures don’t erase an existing secret.
🪄 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: e53efc43-7f2c-4b5c-a5f4-7648e2fe3dc5
📒 Files selected for processing (4)
src-tauri/src/application/commands/add_download_to_package.rssrc-tauri/src/application/commands/set_package_password.rssrc-tauri/src/application/commands/tests_support.rssrc-tauri/src/domain/ports/driven/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/application/commands/add_download_to_package.rs
PR #131 round 3: codex P1 / CodeRabbit major. The cleanup branch in `set_package_password` issued an unconditional `credentials.delete(&key)` after marker rollback. Because the keyring service key is stable per package, that wiped a previously valid secret whenever a transient `store` error happened mid-rotation — turning a recoverable failure into permanent data loss. Capture the existing credential before mutating state, then on keyring failure restore it (`store(&key, prev)` if there was one, `delete(&key)` if the package was unconfigured). Mirrors the snapshot-and-restore pattern from `update_account`. Regression test `test_set_package_password_failed_rotation_preserves_previous_secret` seeds a working secret, forces the next `store` to fail, and asserts both the marker rolls back and the original secret survives intact.
5 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
Implements Task 27: nine command handlers for package management (create, update, delete, set password, set priority, move folder, toggle auto-extract, add/remove downloads). All handlers wired through CommandBus and exposed via Tauri IPC. Package FK management via new
attach_download/detach_downloadport methods. Domain events emitted for updates and deletes. Unblocks Task 28 (Queries) and Task 29 (Package UI).Why
Complete CRUD foundation for packages. Cascade operations (priority → children, move → children) ensure consistency. Keyring-only password storage prevents secrets in SQLite. FK management (attach/detach) replaces manual patching with atomic repository semantics.
Changes
Testing
43 application tests + 5 SQLite tests = ≥94% per-handler coverage (target: 85%)
Related Issues
Notes for Reviewer
Large commit (2510 insertions, 21 files) — review by handler is suggested:
Checklist
Summary by cubic
Implements full package management for Task 27 with nine commands, robust keyring-only passwords (rollback and previous-secret restore on failure), cascades for priority and folder moves, and stricter invariants for download attach/detach. Adds repo support, IPC endpoints, and events so the UI can manage packages and stay in sync.
New Features
attach_download/detach_downloadandfind_package_of_download; SQLite adapter implements all.Some(Some),Some(None),None); emitspackage-created,package-updated,package-deleted.PackageUpdatedfor both source and destination; keyring failures roll back the marker and restore the previous secret; same-package reattach is a no-op that preserves order; relative folder paths are rejected.Migration
package-updatedandpackage-deletedin the UI.set_package_priority;update_packagedoes not cascade.Written for commit 7f0bf44. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Documentation