feat(account): SQLite Accounts repository (task 20)#124
Merged
Conversation
Add `accounts` table migration with UNIQUE(service_name, username),
sea-orm entity + converters, `AccountRepository` driven port and
`SqliteAccountRepo` adapter. Refactor domain `Account` to a `String`-backed
`AccountId` matching the spec's TEXT PRIMARY KEY and add the
`traffic_total` / `last_validated` / `created_at` fields required by
PRD-v2 §8 P1. Credentials stay out of SQLite -- exposed via
`Account::credential_ref()` returning `keyring://{service}/{username}`.
Unblocks tasks 21-25, 38, 51-56, 75-76 (Accounts commands/queries/UI,
Debrid/premium hoster plugins).
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)
📝 WalkthroughWalkthroughThis PR adds SQLite-backed account persistence: a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Port as AccountRepository
participant Adapter as SqliteAccountRepo
participant DB as SQLite
participant Keyring as OS_Keyring
App->>Port: save(account)
Port->>Adapter: save(account)
Adapter->>DB: INSERT ... ON CONFLICT(...) DO UPDATE ...
DB-->>Adapter: OK / UNIQUE VIOLATION
alt UNIQUE VIOLATION
Adapter-->>Port: Err(DomainError::AlreadyExists)
Port-->>App: Err(DomainError::AlreadyExists)
else Success
Adapter-->>Port: Ok(())
Port-->>App: Ok(())
end
App->>Port: find_by_id(id)
Port->>Adapter: find_by_id(id)
Adapter->>DB: SELECT ... WHERE id=?
DB-->>Adapter: row / none
alt row
Adapter->>Port: Some(Account)
Port-->>App: Some(Account)
else none
Port-->>App: None
end
App->>Account: credential_ref()
Account-->>App: "keyring://{service}/{username}"
App->>Keyring: lookup("keyring://...")
Keyring-->>App: credentials
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src-tauri/src/adapters/driven/sqlite/connection.rs (1)
167-176: Assert duplicate failure reason, not only failure presence.Line 175 currently accepts any insert error. Tightening this to validate uniqueness-related failure will prevent false positives.
Proposed test hardening
- let dup = db + let dup = db .execute(Statement::from_string( sea_orm::DatabaseBackend::Sqlite, format!( "INSERT INTO accounts (id, service_name, username, account_type, enabled, created_at) VALUES ('a2', 'real-debrid', 'alice', 'debrid', 1, {now})" ), )) .await; - assert!(dup.is_err(), "UNIQUE(service_name, username) must reject"); + let dup_err = dup.expect_err("UNIQUE(service_name, username) must reject"); + assert!( + dup_err.to_string().to_ascii_lowercase().contains("unique"), + "expected UNIQUE constraint error, got: {dup_err}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/sqlite/connection.rs` around lines 167 - 176, The test currently only asserts that db.execute(...) (captured as dup) is an Err; change it to inspect the DbErr returned by db.execute/Statement::from_string and assert it indicates a uniqueness constraint violation (e.g., match DbErr::Exec or the underlying Sqlx/DatabaseError and check the error message contains "UNIQUE" or "UNIQUE constraint failed"). Locate the db.execute call and the assert!(dup.is_err(), ...) and replace the broad assertion with a specific match/assert that dup.unwrap_err() is the uniqueness-related error from SeaORM/Sqlite.src-tauri/src/domain/ports/driven/tests.rs (1)
674-690: Add a secondary sort key for deterministic ties.If multiple accounts share the same
created_at, current ordering can vary because source iteration is fromHashMap.Deterministic ordering tweak
fn list(&self) -> Result<Vec<Account>, DomainError> { let mut accounts: Vec<Account> = self.store.lock().unwrap().values().cloned().collect(); - accounts.sort_by_key(|a| a.created_at()); + accounts.sort_by(|a, b| { + a.created_at() + .cmp(&b.created_at()) + .then_with(|| a.id().as_str().cmp(b.id().as_str())) + }); Ok(accounts) } fn list_by_service(&self, service_name: &str) -> Result<Vec<Account>, DomainError> { let mut accounts: Vec<Account> = self .store .lock() .unwrap() .values() .filter(|a| a.service_name() == service_name) .cloned() .collect(); - accounts.sort_by_key(|a| a.created_at()); + accounts.sort_by(|a, b| { + a.created_at() + .cmp(&b.created_at()) + .then_with(|| a.id().as_str().cmp(b.id().as_str())) + }); Ok(accounts) }🤖 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 674 - 690, The current sort in list and list_by_service uses only created_at, which yields non-deterministic order for ties; update both functions (list and list_by_service) to sort with a secondary key (e.g., account id or another stable field on Account) so tied created_at values are ordered deterministically—modify the sort_by_key call to use a tuple or comparator that includes created_at and the stable field (reference Account::created_at and the stable identifier accessor such as Account::id or Account::service_name).src-tauri/src/adapters/driven/sqlite/account_repo.rs (1)
48-60: Keepcreated_atinsert-only on the upsert path.Updating
CreatedAthere means any later save can rewrite the original creation timestamp and change list ordering. I'd leave it out ofupdate_columns(...)so it remains stable after the first insert.Diff
account::Column::TrafficTotal, account::Column::ValidUntil, account::Column::LastValidated, - account::Column::CreatedAt, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/sqlite/account_repo.rs` around lines 48 - 60, The upsert currently includes account::Column::CreatedAt in the update_columns array which allows later saves to overwrite creation timestamps; remove account::Column::CreatedAt from the OnConflict::column(account::Column::Id).update_columns(...) list so created_at is only set on insert and not updated on conflict, leaving the other columns (ServiceName, Username, AccountType, Enabled, TrafficLeft, TrafficTotal, ValidUntil, LastValidated) unchanged in behavior.
🤖 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/account.rs`:
- Around line 47-63: The ActiveModel::from_domain function currently uses
unchecked `as i64` casts for u64 fields which can overflow; replace those casts
for traffic_left, traffic_total, valid_until, last_validated, and created_at
with checked conversions (i64::try_from(...)) and surface failures instead of
silently producing negative values—change from_domain to return a Result<Self,
E> (or propagate a conversion error type) and use
i64::try_from(account.traffic_left().map(|b| b as
u64).unwrap_or_default())-style calls (or map each Option<u64> with .map(|v|
i64::try_from(v)?)) so each Set(...) receives a validated i64 or the function
returns an error; ensure you update callers to handle the Result.
In `@src-tauri/src/domain/model/account.rs`:
- Around line 163-166: The credential_ref() method builds a fragile URI from
service_name and username; percent-encode each path segment (service_name and
username) before formatting to avoid collisions when they contain reserved
characters (or alternatively validate and reject unsupported chars). Update
credential_ref() to percent-encode self.service_name and self.username (e.g.,
using the percent-encoding / url crate’s component encoding for path segments)
and then return format!("keyring://{}/{}", encoded_service, encoded_username) so
stored refs are unambiguous.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/sqlite/account_repo.rs`:
- Around line 48-60: The upsert currently includes account::Column::CreatedAt in
the update_columns array which allows later saves to overwrite creation
timestamps; remove account::Column::CreatedAt from the
OnConflict::column(account::Column::Id).update_columns(...) list so created_at
is only set on insert and not updated on conflict, leaving the other columns
(ServiceName, Username, AccountType, Enabled, TrafficLeft, TrafficTotal,
ValidUntil, LastValidated) unchanged in behavior.
In `@src-tauri/src/adapters/driven/sqlite/connection.rs`:
- Around line 167-176: The test currently only asserts that db.execute(...)
(captured as dup) is an Err; change it to inspect the DbErr returned by
db.execute/Statement::from_string and assert it indicates a uniqueness
constraint violation (e.g., match DbErr::Exec or the underlying
Sqlx/DatabaseError and check the error message contains "UNIQUE" or "UNIQUE
constraint failed"). Locate the db.execute call and the assert!(dup.is_err(),
...) and replace the broad assertion with a specific match/assert that
dup.unwrap_err() is the uniqueness-related error from SeaORM/Sqlite.
In `@src-tauri/src/domain/ports/driven/tests.rs`:
- Around line 674-690: The current sort in list and list_by_service uses only
created_at, which yields non-deterministic order for ties; update both functions
(list and list_by_service) to sort with a secondary key (e.g., account id or
another stable field on Account) so tied created_at values are ordered
deterministically—modify the sort_by_key call to use a tuple or comparator that
includes created_at and the stable field (reference Account::created_at and the
stable identifier accessor such as Account::id or Account::service_name).
🪄 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: ae9c04ff-8fd9-4adf-bdb8-1ffe577dc7cc
📒 Files selected for processing (14)
CHANGELOG.mdsrc-tauri/src/adapters/driven/sqlite/account_repo.rssrc-tauri/src/adapters/driven/sqlite/connection.rssrc-tauri/src/adapters/driven/sqlite/entities/account.rssrc-tauri/src/adapters/driven/sqlite/entities/mod.rssrc-tauri/src/adapters/driven/sqlite/migrations/m20260428_000006_create_accounts.rssrc-tauri/src/adapters/driven/sqlite/migrations/mod.rssrc-tauri/src/adapters/driven/sqlite/mod.rssrc-tauri/src/domain/model/account.rssrc-tauri/src/domain/model/mod.rssrc-tauri/src/domain/ports/driven/account_repository.rssrc-tauri/src/domain/ports/driven/mod.rssrc-tauri/src/domain/ports/driven/tests.rssrc-tauri/src/lib.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 14 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/adapters/driven/sqlite/account_repo.rs">
<violation number="1" location="src-tauri/src/adapters/driven/sqlite/account_repo.rs:59">
P2: `created_at` is being updated during upsert. This should remain immutable after initial insert, otherwise subsequent saves can rewrite creation time and reorder list results.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Replace unchecked `as i64` casts with `i64::try_from(...)` in `ActiveModel::from_domain` so u64 values above i64::MAX surface as `ValidationError` instead of silently flipping to negatives. - Percent-encode service_name and username in `Account::credential_ref()` so reserved characters (`/`, `@`, unicode...) cannot produce ambiguous keyring refs that point at the wrong stored credential. - Drop `CreatedAt` from the upsert `update_columns` list so the original insertion timestamp stays stable across subsequent saves, preventing list-order drift. - Tighten the connection-level UNIQUE test to assert the error message actually mentions \"unique\" rather than accepting any failure. - Sort `InMemoryAccountRepository::list*` deterministically by (created_at, id) to match the SQLite adapter and avoid HashMap iteration order leaking through ties.
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/domain/ports/driven/tests.rs`:
- Around line 655-671: The mock save method currently overwrites an existing
Account (in save) including its created_at, causing behavior divergence from the
SQLite adapter; change save so that when guard.get(account.id()) finds an
existing entry you preserve its created_at value and copy or set that timestamp
onto the Account being stored (or construct a new Account merging
existing.created_at with the incoming account's other fields) before inserting
into guard, keeping the collision check logic and using the same AccountId,
mutex guard and Account methods (service_name, username, id, created_at) to
locate and preserve the original creation time.
🪄 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: e096de42-7ef0-4832-9241-a97b8117b052
📒 Files selected for processing (5)
src-tauri/src/adapters/driven/sqlite/account_repo.rssrc-tauri/src/adapters/driven/sqlite/connection.rssrc-tauri/src/adapters/driven/sqlite/entities/account.rssrc-tauri/src/domain/model/account.rssrc-tauri/src/domain/ports/driven/tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/src/adapters/driven/sqlite/connection.rs
- src-tauri/src/adapters/driven/sqlite/entities/account.rs
Re-saving the same id in the in-memory mock previously overwrote `created_at`, which diverged from the SQLite adapter's insert-only semantics. The mock now mirrors production: same-id saves keep the existing timestamp and only update mutable fields.
This was referenced Apr 28, 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
• Add
accountsSQLite table migration with UNIQUE(service_name, username) constraint• Refactor
Accountdomain model: id → AccountId(String) newtype matching TEXT PRIMARY KEY spec• Add missing fields:
traffic_total,last_validated,created_at• Sea-orm entity with
from_domain/into_domainconverters handling type conversions•
AccountRepositorydriven port withfind_by_id,save,list,list_by_service,delete•
SqliteAccountRepoadapter implementing the port with proper UNIQUE violation error handling• Credentials stored in keyring only — SQLite refs via
Account::credential_ref()method• Comprehensive test coverage: domain 99.70%, adapter 95.81%, migrations + UNIQUE constraint tests
Type
feat (new persistence layer)
Verification
Unblocks
Tasks #21-25, #38, #51-56, #75-76 (Accounts commands/queries/UI, hoster plugins)
Summary by cubic
Adds SQLite-backed Accounts persistence and a driven
AccountRepositoryto implement task 20. RefactorsAccountto useAccountId(String), addstraffic_total,last_validated,created_at, and hardens credential refs and value validation; credentials remain in the OS keyring.New Features
accountstable with UNIQUE(service_name, username)and deterministic ordering bycreated_at(upsert preservescreated_at).AccountRepositoryport andSqliteAccountRepoadapter withfind_by_id,save(upsert),list,list_by_service,delete; UNIQUE violations map toAlreadyExists.sea-ormentity withfrom_domain/into_domainconverters; i64 conversions are checked (ValidationErrorif abovei64::MAX).AccountId(String), new fields, andcredential_ref()→ percent-encodedkeyring://{service}/{username}(no credentials in SQLite).Migration
accountsand indexes; existing data is preserved.AccountIdand providecreated_atwhere needed.Written for commit 6734ee6. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests