Skip to content

fix(core): wire AppState via .manage() — all 22 IPC commands now work#34

Merged
mpiton merged 4 commits intomainfrom
feat/fix-appstate-registration
Apr 12, 2026
Merged

fix(core): wire AppState via .manage() — all 22 IPC commands now work#34
mpiton merged 4 commits intomainfrom
feat/fix-appstate-registration

Conversation

@mpiton
Copy link
Copy Markdown
Owner

@mpiton mpiton commented Apr 12, 2026

Summary

  • Fixes [QA] CRITICAL: All IPC commands fail — AppState not registered via .manage() #27 — All 22 Tauri IPC commands were failing with "state not managed" because AppState was never constructed or registered via .manage() in the setup closure
  • Constructs all 12 driven adapters, assembles CommandBus + QueryBus, and registers AppState in the Tauri setup closure
  • Adds NoopCredentialStore and InMemoryStatsRepository stub adapters for the two ports without production implementations yet
  • Adds integration test validating end-to-end wiring with in-memory SQLite

Changes

File Change
src-tauri/src/lib.rs Main fix: complete setup closure with DB init, adapter construction, bus assembly, .manage(), event bridges, plugin watcher
src-tauri/src/adapters/driven/credential/ New NoopCredentialStore stub (returns Ok(None), logs warning on access)
src-tauri/src/adapters/driven/stats/ New InMemoryStatsRepository stub (accumulates in-memory with saturating_add)
src-tauri/src/adapters/driven/mod.rs Register new credential and stats modules
src-tauri/tests/app_state_wiring.rs Integration test: full adapter wiring + query execution on in-memory DB
CHANGELOG.md Document fix in Unreleased section

Test plan

  • cargo build passes (0 errors, 0 warnings)
  • cargo clippy -- -D warnings passes
  • cargo test — 433 passed, 0 failed (includes new integration test)
  • npx vitest run — 297 passed, 0 failed (frontend unaffected)
  • Adversarial code review: 3 HIGH findings fixed (silent watcher failure, duplicate reqwest client, integer overflow)
  • Manual: launch app in dev mode, verify IPC commands respond (settings_get, download_list)

Summary by cubic

Restores all 22 IPC commands by constructing and registering AppState during Tauri setup and entering the tokio runtime; queued downloads now auto-start via the wired QueueManager. Also initializes the DB, wires adapters with a shared reqwest::Client, and adds credential and stats stubs.

  • Bug Fixes

    • Initialize SQLite (WAL) and run migrations at startup
    • Wire driven adapters and assemble CommandBus/QueryBus; share a single reqwest::Client
    • Register AppState with .manage() and enter the runtime so subscribers can tokio::spawn (Fixes [QA] CRITICAL: All IPC commands fail — AppState not registered via .manage() #27)
    • Start QueueManager.start_listening() to execute queued downloads
    • Bridge domain events to the webview and desktop notifications; start plugin hot-reload watcher with failure logging
    • Add stubs: NoopCredentialStore (warns on get/store/delete) and InMemoryStatsRepository (overflow-safe; success_rate is 1.0 when any completed downloads exist, else 0.0)
  • Refactors

    • sqlite::connection::establish_connection now accepts &Path and uses SqliteConnectOptions::filename() for cross-platform, non-UTF-8-safe paths

Written for commit aff7712. Summary will update on new commits.

Summary by CodeRabbit

  • Bug Fixes

    • All Tauri IPC commands are now operational; startup runs DB migrations, initializes adapters, and wires event bridges for reliable behavior.
  • New Features

    • Plugin hot-reload starts at launch and logs failures instead of failing silently.
    • In-memory stats provide consistent metrics with overflow-safe aggregation.
    • A no-op credential store is available as a development stub (no persistent credential writes).
    • Shared HTTP client used for metadata and downloads to reduce duplication.
  • Tests

    • Added an integration test validating full app wiring with in-memory storage.

Construct all driven adapters in the Tauri setup closure, assemble
CommandBus + QueryBus, and register AppState so IPC handlers can
access managed state.

- Establish SQLite connection + run migrations at startup
- Wire 12 driven adapters (event bus, file storage, HTTP client,
  config store, clipboard observer, plugin loader, download engine,
  archive extractor, 4 repositories)
- Share single reqwest::Client between HTTP port and download engine
- Spawn event bridges (Tauri webview + desktop notifications)
- Start plugin hot-reload watcher with failure logging
- Add NoopCredentialStore stub (until keyring-rs integration)
- Add InMemoryStatsRepository stub (until SQLite implementation)
- Add integration test verifying full adapter wiring

Closes #27
@github-actions github-actions bot added documentation Improvements or additions to documentation rust labels Apr 12, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f50882bf-b46e-4464-a38a-d1a9e6edd195

📥 Commits

Reviewing files that changed from the base of the PR and between a790f5e and aff7712.

📒 Files selected for processing (2)
  • src-tauri/src/adapters/driven/sqlite/connection.rs
  • src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs

📝 Walkthrough

Walkthrough

Constructs and registers AppState in Tauri setup, initializes SQLite (WAL) with migrations, assembles driven adapters (including NoopCredentialStore and InMemoryStatsRepository), builds CommandBus/QueryBus, starts event/notification bridges and plugin hot-reload, and adds a wiring integration test.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Added Fixed entry documenting AppState wiring and startup bootstrap (DB init/migrations, adapter assembly, CQRS buses, event bridges, plugin hot-reload, shared HTTP client, stub credential/stats).
Credential Adapter
src-tauri/src/adapters/driven/credential/mod.rs, src-tauri/src/adapters/driven/credential/noop_credential_store.rs
Added NoopCredentialStore and public re-export; implements CredentialStore as a no-op that logs warnings for get/store/delete.
Stats Adapter
src-tauri/src/adapters/driven/stats/mod.rs, src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs
Added InMemoryStatsRepository and public re-export; mutex-protected accumulator, saturating additions, implements StatsRepository (record_completed, get_stats).
Driven Adapter Exports
src-tauri/src/adapters/driven/mod.rs
Registered new credential and stats modules in driven adapter exports.
Core App Wiring
src-tauri/src/lib.rs
Reworked Tauri .setup(...) to perform full bootstrapping: create app dirs, async SQLite connect + migrations, assemble adapters, construct CommandBus/QueryBus, spawn event/notification bridges, start PluginWatcher, and call app.manage(AppState { ... }); added several public re-exports.
Integration Test
src-tauri/tests/app_state_wiring.rs
New test test_appstate_wiring_with_in_memory_db that builds a runtime, wires concrete adapters (in-memory SQLite, stub clipboard) into buses, and asserts basic queries and zeroed stats.
SQLite Connection API
src-tauri/src/adapters/driven/sqlite/connection.rs
Changed establish_connection signature to accept &Path and use SqliteConnectOptions::filename(...); updated callers/tests accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant Setup as Tauri.setup
    participant FS as FileSystem (dirs)
    participant DB as SQLite (WAL + migrations)
    participant Adapters as Driven Adapters
    participant Bus as {CommandBus, QueryBus}
    participant EventBus as Domain Event Bus
    participant Bridges as Tauri/Notification Bridges
    participant Plugin as PluginWatcher

    Setup->>FS: create app/config/plugin directories
    Setup->>DB: open connection (WAL) and run migrations
    Setup->>Adapters: construct driven adapters (http, fs, credential noop, in-memory stats, plugin loader, extractor)
    Adapters->>Bus: provide driven ports to build CommandBus/QueryBus
    Setup->>Bus: instantiate AppState{command_bus, query_bus} and call app.manage(...)
    Bus->>EventBus: connect domain event publishers
    Setup->>Bridges: spawn event & notification bridges
    Setup->>Plugin: start PluginWatcher (log warning on failure)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

dependencies

Poem

🐇 I hopped through folders, wired the state,
Stitched buses, bridges, and a plugin gate.
Stubs hum softly where secrets should be,
In-memory stats keep the devs carefree.
Hooray — IPC calls wake up with me.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly matches the main objective: wiring AppState via .manage() to enable all 22 IPC commands, which is the core fix in this changeset.
Linked Issues check ✅ Passed The PR comprehensively addresses issue #27: constructs AppState from concrete adapters, calls app.manage(state), initializes SQLite with migrations, builds CommandBus/QueryBus, bridges events, and implements NoopCredentialStore and InMemoryStatsRepository stubs.
Out of Scope Changes check ✅ Passed All changes directly support the core objective of wiring AppState and enabling IPC commands; the integration test and adapter implementations are within scope of the bootstrapping requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/fix-appstate-registration

Comment @coderabbitai help to get the list of available commands and usage tips.

The Tauri setup closure runs outside the tokio runtime context.
Capture the Handle during block_on and call .enter() so that
tokio::spawn (used by TokioEventBus::subscribe) finds the reactor.

Fixes startup panic: "there is no reactor running"
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 12, 2026

Greptile Summary

This PR correctly fixes the root cause of #27 by constructing all 12 driven adapters, assembling CommandBus + QueryBus, and registering AppState via .manage() in the Tauri setup closure, along with two new stub adapters (NoopCredentialStore, InMemoryStatsRepository) and a solid integration test.

  • QueueManager is never instantiated or started: StartDownloadCommand emits DownloadCreated "so the queue manager can schedule it" (its own doc comment), and both CancelDownloadCommand and RemoveDownloadCommand leave comments stating "QueueManager's decrement_and_schedule reacts to this event." Without QueueManager::start_listening() being called in setup, download_start returns a success ID but the download stays in Queued state permanently — nothing ever executes.
  • The manual test plan only verifies settings_get and download_list, which would not catch this gap.

Confidence Score: 4/5

Safe to merge for IPC responsiveness, but download execution is broken without QueueManager — needs one fix before production use.

The AppState wiring is correct and the 22 IPC commands will no longer crash with 'state not managed.' However, the QueueManager omission means download_start silently succeeds while no download ever runs, which is a present functional defect on the primary user path. All other findings are P2.

src-tauri/src/lib.rs — QueueManager must be instantiated and start_listening() called in the setup closure.

Important Files Changed

Filename Overview
src-tauri/src/lib.rs Core wiring fix: constructs all adapters and registers AppState via .manage(), but QueueManager is never instantiated or started — downloads will queue but never execute.
src-tauri/src/adapters/driven/credential/noop_credential_store.rs Stub CredentialStore that returns Ok(None) for reads with a warning; store() and delete() silently no-op without logging.
src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs In-memory accumulator using std::sync::Mutex with poison recovery; saturating arithmetic is correct but success_rate is hardcoded 100.0 regardless of failures.
src-tauri/tests/app_state_wiring.rs Integration test wires all adapters and exercises a real query against in-memory SQLite; assertions on Arc::strong_count are trivially true and add no signal.
src-tauri/src/adapters/driven/mod.rs Registers two new modules (credential, stats) correctly alongside existing driven adapters.
CHANGELOG.md Documents the fix and new stub adapters in the Unreleased section; accurate.

Sequence Diagram

sequenceDiagram
    participant FE as Frontend
    participant IPC as Tauri IPC
    participant CB as CommandBus
    participant DB as SqliteDownloadRepo
    participant EB as TokioEventBus
    participant QM as QueueManager ❌ (not started)
    participant DE as SegmentedDownloadEngine

    FE->>IPC: download_start(url)
    IPC->>CB: handle_start_download(cmd)
    CB->>DB: save(download [Queued])
    CB->>EB: publish(DownloadCreated)
    EB-->>QM: (no subscriber — QueueManager never started)
    Note over QM,DE: Download stays Queued forever,<br/>engine.start() is never called
    CB-->>IPC: Ok(download_id)
    IPC-->>FE: download_id ✓ (but download never runs)
Loading

Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix(core): enter tokio runtime context f..." | Re-trigger Greptile

Comment on lines +140 to +166
4,
));

// ── CQRS buses ──────────────────────────────────────────
let command_bus = Arc::new(CommandBus::new(
download_repo,
download_engine,
event_bus.clone(),
file_storage,
http_client,
plugin_loader,
config_store,
credential_store,
clipboard_observer,
archive_extractor.clone(),
));

let query_bus = Arc::new(QueryBus::new(
download_read_repo,
history_repo,
stats_repo,
plugin_read_repo,
archive_extractor,
));

// ── Register AppState ───────────────────────────────────
app.manage(AppState {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 QueueManager never instantiated or started

QueueManager is exported from lib.rs and the StartDownloadCommand handler explicitly states it "emits DownloadCreated so the queue manager can schedule it" (see start_download.rs line 5). Comments in cancel_download.rs and remove_download.rs also read: "QueueManager's decrement_and_schedule reacts to this event." Without QueueManager::start_listening(), every download started via download_start stays stuck in Queued state and never executes — the IPC returns success but nothing actually downloads.

// After assembling the buses, add:
let queue_manager = Arc::new(QueueManager::new(
    download_repo_arc.clone(),   // needs an Arc of the write repo
    download_engine.clone(),
    event_bus.clone(),
    4, // max_concurrent — could come from config
));
queue_manager.clone().start_listening();
app.manage(queue_manager);

(Adapters that are cloned into both the bus and the queue manager need to be kept as Arc before moving into the bus constructor.)

Fix in Claude Code

Comment on lines +27 to +33
fn store(&self, _service: &str, _credential: &Credential) -> Result<(), DomainError> {
Ok(())
}

fn delete(&self, _service: &str) -> Result<(), DomainError> {
Ok(())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent no-ops on store and delete without logging

get() correctly logs a warning when credentials are accessed, but store() and delete() silently return Ok(()) with no trace output. Any feature that calls store() expecting durability (e.g., plugin auth tokens) will silently lose data with no observable signal in logs, making it hard to diagnose issues.

Suggested change
fn store(&self, _service: &str, _credential: &Credential) -> Result<(), DomainError> {
Ok(())
}
fn delete(&self, _service: &str) -> Result<(), DomainError> {
Ok(())
}
fn store(&self, service: &str, _credential: &Credential) -> Result<(), DomainError> {
tracing::warn!(
service,
"credential write ignored: keyring store is not yet implemented"
);
Ok(())
}
fn delete(&self, service: &str) -> Result<(), DomainError> {
tracing::warn!(
service,
"credential delete ignored: keyring store is not yet implemented"
);
Ok(())
}

Fix in Claude Code

Comment on lines +74 to +83
Ok(StatsView {
total_downloaded_bytes: accumulator.total_bytes,
total_files: accumulator.total_files,
avg_speed,
peak_speed: accumulator.peak_speed,
success_rate: 100.0,
daily_volumes: vec![],
top_hosts: vec![],
})
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 success_rate hardcoded to 100.0 regardless of failures

The accumulator tracks total_bytes, total_files, and total_speed_sum, but has no concept of failures. The returned StatsView always reports success_rate: 100.0 and empty daily_volumes/top_hosts. If this stub view is rendered in the stats UI, users will always see 100% success rate even after repeated failures — silently misleading. Consider returning f64::NAN or 0.0 as a sentinel, or at least documenting the TODO inline on the field.

Fix in Claude Code

Comment on lines +101 to +102
assert!(Arc::strong_count(&command_bus) >= 1);
assert!(Arc::strong_count(&query_bus) >= 1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Trivially-true assertions don't verify wiring

Arc::strong_count(&command_bus) >= 1 is always true and cannot catch a wiring regression. A more meaningful assertion would verify the exact count (e.g., == 1 to confirm no unexpected holders) or, better, exercise a command through the bus rather than just constructing it. The query already exercised below is a stronger signal — the strong-count assertions add no value and could be removed.

Fix in Claude Code

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 8 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/credential/noop_credential_store.rs">

<violation number="1" location="src-tauri/src/adapters/driven/credential/noop_credential_store.rs:27">
P2: `store` reports success even though this adapter does not persist credentials, which can cause silent credential loss. Return an explicit error (or at least a warning + error) for unimplemented writes.</violation>
</file>

<file name="src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs">

<violation number="1" location="src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs:79">
P3: `success_rate` is returned as `100.0`, but the rest of the codebase treats success rate as a 0–1 fraction (e.g. 0.95 or 1.0). This will over-report success by 100×. Use `1.0` (or compute a fraction) instead.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/adapters/driven/credential/noop_credential_store.rs`:
- Around line 27-32: The Noop credential store's store() and delete() functions
currently return Ok(()) even though they don't persist anything; update
noop_credential_store.rs so store(&self, _service, _credential) and
delete(&self, _service) return an Err(DomainError) to fail fast (e.g.
DomainError::new("credential store not implemented") or a suitable existing
variant like StorageUnavailable/NotImplemented) instead of Ok, or at minimum log
a warning via the same logging facility used elsewhere before returning Err;
ensure you reference and use the existing DomainError constructors/variants used
across the codebase to keep error types consistent.

In `@src-tauri/tests/app_state_wiring.rs`:
- Around line 100-102: The current test uses Arc::strong_count(&command_bus) >=
1 which is always true for a live Arc and does not exercise the real
registration path; update the test to register state via app.manage(AppState {
... }) (or refactor the setup into a helper that returns an AppState instance
used by both src-tauri/src/lib.rs and this test) so the buses are created and
registered through the real state-registration path, then retrieve the managed
AppState (or inspect its command_bus/query_bus fields) and assert their Arc
counts or behavior; reference the command_bus, query_bus, AppState, and
app.manage symbols when making the change.
🪄 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: 68a41e92-6a0d-45b5-87cc-067a3a697cc7

📥 Commits

Reviewing files that changed from the base of the PR and between d6a8f9f and 76a16d9.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • src-tauri/src/adapters/driven/credential/mod.rs
  • src-tauri/src/adapters/driven/credential/noop_credential_store.rs
  • src-tauri/src/adapters/driven/mod.rs
  • src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs
  • src-tauri/src/adapters/driven/stats/mod.rs
  • src-tauri/src/lib.rs
  • src-tauri/tests/app_state_wiring.rs

Comment on lines +100 to +102
// Verify buses are accessible
assert!(Arc::strong_count(&command_bus) >= 1);
assert!(Arc::strong_count(&query_bus) >= 1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

These assertions won't catch issue #27 coming back.

Arc::strong_count(..) >= 1 is always true for a live Arc, and this test still builds the buses directly instead of going through app.manage(AppState { .. }) in src-tauri/src/lib.rs. Please exercise the real state-registration path, or factor the setup logic into a helper that returns AppState so this regression is actually covered.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/tests/app_state_wiring.rs` around lines 100 - 102, The current test
uses Arc::strong_count(&command_bus) >= 1 which is always true for a live Arc
and does not exercise the real registration path; update the test to register
state via app.manage(AppState { ... }) (or refactor the setup into a helper that
returns an AppState instance used by both src-tauri/src/lib.rs and this test) so
the buses are created and registered through the real state-registration path,
then retrieve the managed AppState (or inspect its command_bus/query_bus fields)
and assert their Arc counts or behavior; reference the command_bus, query_bus,
AppState, and app.manage symbols when making the change.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/lib.rs`:
- Around line 109-110: The bootstrap currently wires NoopCredentialStore into
run(), causing credential-backed flows to never persist; replace the
NoopCredentialStore instantiation with a real persisted credential store that
implements CredentialStore (for example a FileCredentialStore::new(config_path)
or PersistentCredentialStore::new(config_path)) and construct it the same way
TomlConfigStore::new(config_path) is created so credentials survive restarts;
keep NoopCredentialStore only in test/dev wiring and ensure the variable
signature remains let credential_store: Arc<dyn CredentialStore> =
Arc::new(...).
- Around line 85-89: Change connection::establish_connection to accept a Path
(or PathBuf) instead of &str and move any UTF-8 conversion/validation inside
that function so startup no longer fails when db_path is non-UTF-8; update the
call site here (where db_path is passed to connection::establish_connection) to
pass the Path (&db_path or db_path.as_path()) and ensure establish_connection
returns/propagates a clear error (with context) if it cannot convert the Path to
a UTF-8 string for APIs that require &str or otherwise handle non-UTF-8 paths by
using OsStr/OsString APIs and opening SQLite directly from the Path.
🪄 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: 0a3843d2-749b-4d84-831c-16ef117ad9d5

📥 Commits

Reviewing files that changed from the base of the PR and between 76a16d9 and f5908f9.

📒 Files selected for processing (1)
  • src-tauri/src/lib.rs

Comment on lines +109 to +110
let config_store: Arc<dyn ConfigStore> = Arc::new(TomlConfigStore::new(config_path));
let credential_store: Arc<dyn CredentialStore> = Arc::new(NoopCredentialStore);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't ship NoopCredentialStore from the real app bootstrap.

Line 110 wires a stub that always behaves like “no credentials configured”, so any credential-backed flow can never round-trip persisted state across launches. This belongs in test/dev wiring, not run().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/lib.rs` around lines 109 - 110, The bootstrap currently wires
NoopCredentialStore into run(), causing credential-backed flows to never
persist; replace the NoopCredentialStore instantiation with a real persisted
credential store that implements CredentialStore (for example a
FileCredentialStore::new(config_path) or
PersistentCredentialStore::new(config_path)) and construct it the same way
TomlConfigStore::new(config_path) is created so credentials survive restarts;
keep NoopCredentialStore only in test/dev wiring and ensure the variable
signature remains let credential_store: Arc<dyn CredentialStore> =
Arc::new(...).

- Wire QueueManager with start_listening so queued downloads execute
- Add tracing::warn to NoopCredentialStore store/delete (not just get)
- Change success_rate from 100.0 to 0.0 sentinel (0-1 fraction scale)
- Remove trivially-true Arc::strong_count assertions from test
- Refactor establish_connection to accept &Path instead of &str
- Add TODO comments for NoopCredentialStore and max_concurrent config
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src-tauri/src/lib.rs (1)

106-107: ⚠️ Potential issue | 🟠 Major

Don't ship the noop credential store from the real app bootstrap.

Line 107 means every credential read behaves like “not configured” and writes/deletes are dropped, so credential-backed flows cannot persist across restarts in production. Keep this stub in test/dev wiring only and use a persisted implementation in run().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/lib.rs` around lines 106 - 107, The bootstrap currently
constructs a NoopCredentialStore (let credential_store: Arc<dyn CredentialStore>
= Arc::new(NoopCredentialStore)) which prevents any credentials from being
persisted; replace this stub in the real app wiring by constructing and using
the persisted credential store implementation (e.g., the keyring-backed
implementation you plan to expose from keyring-rs or a
KeyringCredentialStore::new() factory) inside run(), and keep
NoopCredentialStore only for dev/test wiring; update the code that passes
credential_store into downstream functions so production builds use the
persisted store and tests/dev can still inject the noop.
🤖 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/connection.rs`:
- Around line 12-18: The code currently converts db_path to a UTF-8 string and
builds SqliteConnectOptions via from_str, which breaks on Windows paths and
special chars; instead call SqliteConnectOptions::filename(db_path) (passing the
&Path directly) to construct sqlite_opts, then apply your per-connection
PRAGMA/mode settings on that options object (e.g., set mode=rwc via the options
API or using SqliteConnectOptions methods) and remove the to_str() / custom
UTF-8 error branch; update establish_connection to use the new sqlite_opts when
creating the SeaORM DatabaseConnection so paths with backslashes or URI
characters are handled correctly.

In `@src-tauri/src/lib.rs`:
- Around line 119-120: The bootstrap currently wires
Arc::new(InMemoryStatsRepository::new()) into the production StatsRepository,
causing stats to reset on each relaunch; replace this with the SQLite-backed
implementation (e.g., SqliteStatsRepo or SqliteStatsRepository) so the Arc<dyn
StatsRepository> is constructed from the persistent SQLite repo initialized
earlier (and pass any DB connection/Pool used for migrations), or if you need an
in-memory adapter only for tests/dev, guard the use of InMemoryStatsRepository
behind the same environment/config flag used for test/dev so production uses the
SQLite-backed repo instead.

---

Duplicate comments:
In `@src-tauri/src/lib.rs`:
- Around line 106-107: The bootstrap currently constructs a NoopCredentialStore
(let credential_store: Arc<dyn CredentialStore> = Arc::new(NoopCredentialStore))
which prevents any credentials from being persisted; replace this stub in the
real app wiring by constructing and using the persisted credential store
implementation (e.g., the keyring-backed implementation you plan to expose from
keyring-rs or a KeyringCredentialStore::new() factory) inside run(), and keep
NoopCredentialStore only for dev/test wiring; update the code that passes
credential_store into downstream functions so production builds use the
persisted store and tests/dev can still inject the noop.
🪄 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: b79e5a64-61df-47ff-aeec-ca38f9387d99

📥 Commits

Reviewing files that changed from the base of the PR and between f5908f9 and a790f5e.

📒 Files selected for processing (5)
  • src-tauri/src/adapters/driven/credential/noop_credential_store.rs
  • src-tauri/src/adapters/driven/sqlite/connection.rs
  • src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs
  • src-tauri/src/lib.rs
  • src-tauri/tests/app_state_wiring.rs
✅ Files skipped from review due to trivial changes (1)
  • src-tauri/src/adapters/driven/credential/noop_credential_store.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src-tauri/tests/app_state_wiring.rs
  • src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs

Comment on lines +119 to +120
// TODO: replace with SqliteStatsRepo once implemented
let stats_repo: Arc<dyn StatsRepository> = Arc::new(InMemoryStatsRepository::new());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Stats will reset on every relaunch with the in-memory repo.

Line 120 wires InMemoryStatsRepository into the production bootstrap even though startup initializes SQLite and runs migrations. That makes stats_* IPC state process-local, so accumulated totals disappear on restart and can diverge from the persisted download/history data. Use a SQLite-backed stats repository here, or gate the in-memory adapter to test/dev only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/lib.rs` around lines 119 - 120, The bootstrap currently wires
Arc::new(InMemoryStatsRepository::new()) into the production StatsRepository,
causing stats to reset on each relaunch; replace this with the SQLite-backed
implementation (e.g., SqliteStatsRepo or SqliteStatsRepository) so the Arc<dyn
StatsRepository> is constructed from the persistent SQLite repo initialized
earlier (and pass any DB connection/Pool used for migrations), or if you need an
in-memory adapter only for tests/dev, guard the use of InMemoryStatsRepository
behind the same environment/config flag used for test/dev so production uses the
SQLite-backed repo instead.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 5 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/adapters/driven/stats/in_memory_stats_repo.rs">

<violation number="1" location="src-tauri/src/adapters/driven/stats/in_memory_stats_repo.rs:80">
P2: `success_rate` is hard-coded to `0.0`, so this adapter reports 0% success even after recorded completed downloads.</violation>
</file>

<file name="src-tauri/src/adapters/driven/sqlite/connection.rs">

<violation number="1" location="src-tauri/src/adapters/driven/sqlite/connection.rs:13">
P1: Avoid constructing the SQLite connection string by interpolating a filesystem path into `sqlite://...`. This can fail for Windows absolute paths and filenames with URI-reserved characters; configure the connection with `SqliteConnectOptions::filename(db_path)` (plus `create_if_missing`) so path handling is cross-platform and path-safe.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

- Use SqliteConnectOptions::filename() instead of URL interpolation
  for cross-platform path safety (Windows, non-UTF-8, URI chars)
- Fix success_rate: return 1.0 when completed downloads exist, 0.0
  when empty (record_completed only tracks successes)
@mpiton mpiton merged commit 65db42a into main Apr 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation rust

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[QA] CRITICAL: All IPC commands fail — AppState not registered via .manage()

1 participant