fix(core): record completed downloads into history via event bus bridge#120
Merged
fix(core): record completed downloads into history via event bus bridge#120
Conversation
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughAdds an event-driven history recorder bridge that projects Changes
Sequence DiagramsequenceDiagram
participant EB as EventBus
participant Bridge as History Recorder Bridge
participant HR as HistoryRepository
participant DR as DownloadRepository (backfill)
EB->>Bridge: Publish DownloadCompletedPersisted { id, snapshot }
Bridge->>Bridge: snapshot.to_history_entry() (choose bytes, duration→s floor 1, avg_speed)
Bridge->>HR: record(HistoryEntry)
HR-->>Bridge: Ok / Err
Note right of Bridge: on Err -> tracing::warn! (swallow)
Note over DR,Bridge: Startup backfill task
DR->>DR: find_by_state(Completed)
DR-->>Bridge: list of Completed downloads
Bridge->>HR: for each download: if no history -> record(synthesized HistoryEntry)
HR-->>Bridge: Ok / Err (Err logged, continue)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: The combined changelog entry merges two distinct changes into one
bullet; split them into two separate '-' bullets so the history-recorder note
(mentioning spawn_history_recorder_bridge, DownloadCompletedPersisted,
HistoryRepository::record, HistoryEntry projection rules) is one item and the
accessibility skip-link change (mentioning AppLayout, id="main-content",
tabIndex={-1}, sr-only reveal) is its own item; preserve the existing wording
for each but insert a newline and leading '-' before the skip-link paragraph to
make it a separate changelog bullet.
In `@src-tauri/src/adapters/driven/event/history_recorder_bridge.rs`:
- Around line 30-62: The subscriber record_for_event currently only reads id
from DomainEvent::DownloadCompletedPersisted and then re-queries download_repo
(using download_repo.find_by_id) which can race and produce missing or stale
data; update the fix by changing the event payload to carry the downloaded-row
projection (e.g., include the Download snapshot fields used by
derive_history_entry on DomainEvent::DownloadCompletedPersisted) or,
alternatively, move the history_repo.record call into the same completion/write
path that persists the Download so it happens atomically; update the event
construction site to populate the new fields and then change record_for_event to
use the provided snapshot instead of calling find_by_id, keeping references to
DomainEvent::DownloadCompletedPersisted, record_for_event, derive_history_entry,
and history_repo.record to locate and modify the code.
In `@src-tauri/src/lib.rs`:
- Around line 430-438: The startup currently only subscribes future
DownloadCompletedPersisted events via spawn_history_recorder_bridge, so
already-completed downloads lack history rows; add a one-time backfill run at
startup that queries the download repository for downloads with status Completed
(using download_repo_for_history_bridge or the same download repo API), for each
completed download check for an existing history row in history_repo_for_bridge
and insert a corresponding history record (or emit/handle a synthetic
DownloadCompletedPersisted into the same recorder logic) for any missing
entries; place this backfill invocation just before or after
spawn_history_recorder_bridge to ensure the same data-shaping/insert code is
reused and avoid duplicating logic.
🪄 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: cfb1f3ee-38c2-4d35-a60d-031dda339471
📒 Files selected for processing (4)
CHANGELOG.mdsrc-tauri/src/adapters/driven/event/history_recorder_bridge.rssrc-tauri/src/adapters/driven/event/mod.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 4 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="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:12">
P3: This changelog item accidentally includes an unrelated skip-link paragraph after the history bridge fix, creating a duplicated/misfiled entry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ### Fixed | ||
|
|
||
| - Keyboard accessibility: the app now renders a `SkipLink` ("Skip to main content" / "Aller au contenu principal") as the first focusable element in `AppLayout`, paired with `id="main-content"` + `tabIndex={-1}` on the `<main>` landmark. The link stays visually hidden via `sr-only` until focused, then reveals itself in the top-left corner; activating it programmatically focuses the main content (with `event.preventDefault()` so the URL hash stays clean for SPA routing). This addresses the WCAG 2.1 "bypass blocks" success criterion and gives keyboard users a single-Tab shortcut over the 10-item Sidebar. Note: the QA report (#116) reproduced via `tauri-pilot press Tab` is a known false positive — synthetic `KeyboardEvent` dispatched by the pilot does not trigger the webview's native focus-traversal behaviour, so the issue title's "first Tab keypress leaves focus on BODY" is a measurement artefact rather than a real-user bug. The skip-link is shipped anyway because it is a genuine a11y improvement (issue #116). | ||
| - History view stayed empty even after dozens of completed downloads: `HistoryRepository::record` had no production caller, so the `history` table was never written. The Statistics view (which reads the `statistics` table via `spawn_stats_recorder_bridge`) showed the right numbers, but every history-driven feature — the History view's group/filter/search/export, `download_redownload {sourceKind:"history"}` (P0.9), and the `history_purge_older_than` worker (P0.14) — had nothing to operate on. A new `spawn_history_recorder_bridge` mirrors the stats recorder pattern: it subscribes to `DownloadCompletedPersisted` on the event bus, looks up the persisted `Download` aggregate via the write repo, projects it into a `HistoryEntry` (`total_bytes` prefers `file_size` and falls back to `downloaded_bytes`; `updated_at - created_at` is converted from milliseconds to seconds with a `1`-second floor so an instant completion does not divide by zero in `avg_speed`; `completed_at` is the `updated_at` epoch in seconds, matching the Unix-seconds contract documented on `HistoryFilter::date_from`/`date_to`) and calls `record`. The bridge swallows download-repo and history-repo failures with `tracing::warn!` so a write glitch never propagates back into the queue/UI flow. Wired in `lib.rs` alongside the existing tauri/notification/download-log/progress/stats bridges (issue: vortex-qa BUG #1). ("Skip to main content" / "Aller au contenu principal") as the first focusable element in `AppLayout`, paired with `id="main-content"` + `tabIndex={-1}` on the `<main>` landmark. The link stays visually hidden via `sr-only` until focused, then reveals itself in the top-left corner; activating it programmatically focuses the main content (with `event.preventDefault()` so the URL hash stays clean for SPA routing). This addresses the WCAG 2.1 "bypass blocks" success criterion and gives keyboard users a single-Tab shortcut over the 10-item Sidebar. Note: the QA report (#116) reproduced via `tauri-pilot press Tab` is a known false positive — synthetic `KeyboardEvent` dispatched by the pilot does not trigger the webview's native focus-traversal behaviour, so the issue title's "first Tab keypress leaves focus on BODY" is a measurement artefact rather than a real-user bug. The skip-link is shipped anyway because it is a genuine a11y improvement (issue #116). |
There was a problem hiding this comment.
P3: This changelog item accidentally includes an unrelated skip-link paragraph after the history bridge fix, creating a duplicated/misfiled entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CHANGELOG.md, line 12:
<comment>This changelog item accidentally includes an unrelated skip-link paragraph after the history bridge fix, creating a duplicated/misfiled entry.</comment>
<file context>
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
-- Keyboard accessibility: the app now renders a `SkipLink` ("Skip to main content" / "Aller au contenu principal") as the first focusable element in `AppLayout`, paired with `id="main-content"` + `tabIndex={-1}` on the `<main>` landmark. The link stays visually hidden via `sr-only` until focused, then reveals itself in the top-left corner; activating it programmatically focuses the main content (with `event.preventDefault()` so the URL hash stays clean for SPA routing). This addresses the WCAG 2.1 "bypass blocks" success criterion and gives keyboard users a single-Tab shortcut over the 10-item Sidebar. Note: the QA report (#116) reproduced via `tauri-pilot press Tab` is a known false positive — synthetic `KeyboardEvent` dispatched by the pilot does not trigger the webview's native focus-traversal behaviour, so the issue title's "first Tab keypress leaves focus on BODY" is a measurement artefact rather than a real-user bug. The skip-link is shipped anyway because it is a genuine a11y improvement (issue #116).
+- History view stayed empty even after dozens of completed downloads: `HistoryRepository::record` had no production caller, so the `history` table was never written. The Statistics view (which reads the `statistics` table via `spawn_stats_recorder_bridge`) showed the right numbers, but every history-driven feature — the History view's group/filter/search/export, `download_redownload {sourceKind:"history"}` (P0.9), and the `history_purge_older_than` worker (P0.14) — had nothing to operate on. A new `spawn_history_recorder_bridge` mirrors the stats recorder pattern: it subscribes to `DownloadCompletedPersisted` on the event bus, looks up the persisted `Download` aggregate via the write repo, projects it into a `HistoryEntry` (`total_bytes` prefers `file_size` and falls back to `downloaded_bytes`; `updated_at - created_at` is converted from milliseconds to seconds with a `1`-second floor so an instant completion does not divide by zero in `avg_speed`; `completed_at` is the `updated_at` epoch in seconds, matching the Unix-seconds contract documented on `HistoryFilter::date_from`/`date_to`) and calls `record`. The bridge swallows download-repo and history-repo failures with `tracing::warn!` so a write glitch never propagates back into the queue/UI flow. Wired in `lib.rs` alongside the existing tauri/notification/download-log/progress/stats bridges (issue: vortex-qa BUG #1). ("Skip to main content" / "Aller au contenu principal") as the first focusable element in `AppLayout`, paired with `id="main-content"` + `tabIndex={-1}` on the `<main>` landmark. The link stays visually hidden via `sr-only` until focused, then reveals itself in the top-left corner; activating it programmatically focuses the main content (with `event.preventDefault()` so the URL hash stays clean for SPA routing). This addresses the WCAG 2.1 "bypass blocks" success criterion and gives keyboard users a single-Tab shortcut over the 10-item Sidebar. Note: the QA report (#116) reproduced via `tauri-pilot press Tab` is a known false positive — synthetic `KeyboardEvent` dispatched by the pilot does not trigger the webview's native focus-traversal behaviour, so the issue title's "first Tab keypress leaves focus on BODY" is a measurement artefact rather than a real-user bug. The skip-link is shipped anyway because it is a genuine a11y improvement (issue #116).
- `plugin_report_broken` errored on every official plugin because their `plugin.toml` manifests do not declare a `repository` field, while the handler in `application/commands/report_broken_plugin.rs` requires `manifest.repository_url()` to be `Some` to build the GitHub issue URL. The handler now falls back to the local plugin store cache (`PluginStoreEntry.repository`, populated from `registry.toml` via `plugin_store_refresh`) when the loaded manifest is missing the field, so the feature works for the four official plugins without requiring a re-publish. The validation message now points at the actual TOML field (`[plugin].repository` in `plugin.toml`) instead of the internal Rust field name `repository_url`, which only exists on `PluginInfo` (issue #115).
- Statistics view KPIs derived from the `statistics` table (total files, daily volume, average and peak speed) stayed at zero even when completed downloads existed: `StatsRepository::record_completed` had no production caller, so the daily rollup table was never written. A new `spawn_stats_recorder_bridge` subscribes to `DownloadCompletedPersisted` on the event bus, looks up the persisted `Download` aggregate, derives `(bytes, avg_speed)` (`file_size` falling back to `downloaded_bytes`; `updated_at - created_at` is converted from milliseconds to seconds with a `1`-second floor so an instant completion does not divide by zero) and calls `record_completed`. The bridge swallows repo and stats failures with `tracing::warn!` so a stats glitch never propagates back into the queue/UI flow. Wired alongside the existing `tauri`/`notification`/`download_log`/`progress` bridges in `lib.rs`. KPIs sourced from `downloads` (`success_rate`, `top_hosts`) keep their existing read path so no migration is needed (issue #114).
</file context>
Suggested change
| - History view stayed empty even after dozens of completed downloads: `HistoryRepository::record` had no production caller, so the `history` table was never written. The Statistics view (which reads the `statistics` table via `spawn_stats_recorder_bridge`) showed the right numbers, but every history-driven feature — the History view's group/filter/search/export, `download_redownload {sourceKind:"history"}` (P0.9), and the `history_purge_older_than` worker (P0.14) — had nothing to operate on. A new `spawn_history_recorder_bridge` mirrors the stats recorder pattern: it subscribes to `DownloadCompletedPersisted` on the event bus, looks up the persisted `Download` aggregate via the write repo, projects it into a `HistoryEntry` (`total_bytes` prefers `file_size` and falls back to `downloaded_bytes`; `updated_at - created_at` is converted from milliseconds to seconds with a `1`-second floor so an instant completion does not divide by zero in `avg_speed`; `completed_at` is the `updated_at` epoch in seconds, matching the Unix-seconds contract documented on `HistoryFilter::date_from`/`date_to`) and calls `record`. The bridge swallows download-repo and history-repo failures with `tracing::warn!` so a write glitch never propagates back into the queue/UI flow. Wired in `lib.rs` alongside the existing tauri/notification/download-log/progress/stats bridges (issue: vortex-qa BUG #1). ("Skip to main content" / "Aller au contenu principal") as the first focusable element in `AppLayout`, paired with `id="main-content"` + `tabIndex={-1}` on the `<main>` landmark. The link stays visually hidden via `sr-only` until focused, then reveals itself in the top-left corner; activating it programmatically focuses the main content (with `event.preventDefault()` so the URL hash stays clean for SPA routing). This addresses the WCAG 2.1 "bypass blocks" success criterion and gives keyboard users a single-Tab shortcut over the 10-item Sidebar. Note: the QA report (#116) reproduced via `tauri-pilot press Tab` is a known false positive — synthetic `KeyboardEvent` dispatched by the pilot does not trigger the webview's native focus-traversal behaviour, so the issue title's "first Tab keypress leaves focus on BODY" is a measurement artefact rather than a real-user bug. The skip-link is shipped anyway because it is a genuine a11y improvement (issue #116). | |
| - History view stayed empty even after dozens of completed downloads: `HistoryRepository::record` had no production caller, so the `history` table was never written. The Statistics view (which reads the `statistics` table via `spawn_stats_recorder_bridge`) showed the right numbers, but every history-driven feature — the History view's group/filter/search/export, `download_redownload {sourceKind:"history"}` (P0.9), and the `history_purge_older_than` worker (P0.14) — had nothing to operate on. A new `spawn_history_recorder_bridge` mirrors the stats recorder pattern: it subscribes to `DownloadCompletedPersisted` on the event bus, looks up the persisted `Download` aggregate via the write repo, projects it into a `HistoryEntry` (`total_bytes` prefers `file_size` and falls back to `downloaded_bytes`; `updated_at - created_at` is converted from milliseconds to seconds with a `1`-second floor so an instant completion does not divide by zero in `avg_speed`; `completed_at` is the `updated_at` epoch in seconds, matching the Unix-seconds contract documented on `HistoryFilter::date_from`/`date_to`) and calls `record`. The bridge swallows download-repo and history-repo failures with `tracing::warn!` so a write glitch never propagates back into the queue/UI flow. Wired in `lib.rs` alongside the existing tauri/notification/download-log/progress/stats bridges (issue: vortex-qa BUG #1). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/domain/event.rs (1)
118-120: Make the snapshot the single source of truth for the download ID.
DownloadCompletedPersistednow carriesidtwice: once as the outer field and again insidesnapshot.id. The Tauri bridge reads the outer field, while the history/stats bridges use the snapshot, so a mismatched constructor would send different parts of the system to different downloads. I'd drop the outeridor derive it fromsnapshoteverywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/event.rs` around lines 118 - 120, DownloadCompletedPersisted currently duplicates the download ID (outer id: DownloadId and snapshot.id: DownloadCompletedSnapshot.id); remove the outer id field from the DownloadCompletedPersisted enum/struct and update all constructors and matchers to only store the snapshot (DownloadCompletedSnapshot), then update consumers (the Tauri bridge and history/stats bridges) to derive the DownloadId from snapshot.id (or add a small accessor on DownloadCompletedSnapshot if helpful) so every code path reads the ID from the snapshot single source of truth.
🤖 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/services/queue_manager.rs`:
- Around line 284-287: The code unconditionally republishes
DownloadCompletedPersisted when download.state() == DownloadState::Completed;
change the guard to ensure at-most-once emission by checking a persisted flag or
DB marker before publishing: e.g., call a new or existing accessor like
download.is_completion_persisted() (or query the downloads repository by id) and
only call self.event_bus.publish(DomainEvent::DownloadCompletedPersisted { id,
snapshot }) if that check is false, then immediately mark the download as
completion-persisted (e.g., download.mark_completion_persisted() or
repository.update_completion_persisted(id, true)) after publishing; keep using
build_completed_snapshot for the event payload and adjust any model/repo methods
accordingly so downstream history_recorder_bridge and stats_recorder_bridge
receive the event exactly once.
---
Nitpick comments:
In `@src-tauri/src/domain/event.rs`:
- Around line 118-120: DownloadCompletedPersisted currently duplicates the
download ID (outer id: DownloadId and snapshot.id:
DownloadCompletedSnapshot.id); remove the outer id field from the
DownloadCompletedPersisted enum/struct and update all constructors and matchers
to only store the snapshot (DownloadCompletedSnapshot), then update consumers
(the Tauri bridge and history/stats bridges) to derive the DownloadId from
snapshot.id (or add a small accessor on DownloadCompletedSnapshot if helpful) so
every code path reads the ID from the snapshot single source of truth.
🪄 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: 808c3a8b-57c3-42f1-99eb-96debdb482d2
📒 Files selected for processing (10)
CHANGELOG.mdsrc-tauri/src/adapters/driven/event/history_recorder_bridge.rssrc-tauri/src/adapters/driven/event/stats_recorder_bridge.rssrc-tauri/src/adapters/driven/event/tauri_bridge.rssrc-tauri/src/adapters/driven/tray/activity_tracker.rssrc-tauri/src/application/services/history_backfill.rssrc-tauri/src/application/services/mod.rssrc-tauri/src/application/services/queue_manager.rssrc-tauri/src/domain/event.rssrc-tauri/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- src-tauri/src/application/services/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/src/lib.rs
- CHANGELOG.md
`HistoryRepository::record` had no production caller, leaving the `history` table empty after every completed download. The History view, redownload-from-history (P0.9), and the retention purge worker (P0.14) all silently no-op as a result. Mirror the existing `spawn_stats_recorder_bridge` pattern: subscribe to `DownloadCompletedPersisted`, load the `Download` aggregate, derive a `HistoryEntry` (file_size with downloaded_bytes fallback, ms→s conversion with 1s floor for avg_speed) and call `HistoryRepository::record`. Swallow repo errors with warn-level tracing so a history write failure never propagates into the queue. Tests: - 7 unit tests covering the projection, fallback, ms→s conversion, duration floor, non-completed-event filtering, missing-download path, and error swallowing on both sides. - 2 tokio integration tests (one against `RecordingHistoryRepo` via `TokioEventBus`, one against `SqliteHistoryRepo` via `setup_test_db`) to lock down the regression end-to-end. Refs: vortex-qa BUG #1
mpiton
force-pushed
the
fix/qa-history-recorder-bridge
branch
from
a52330e to
a91d5a3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/src/application/services/queue_manager.rs (1)
284-287:⚠️ Potential issue | 🟠 Major
DownloadCompletedPersistedstill needs at-most-once semantics.Line 284 only checks whether the row is currently
Completed, so a secondDownloadCompletedfor the same download republishesDownloadCompletedPersisted. The history bridge inserts unconditionally and the stats bridge increments unconditionally, so this duplicates history rows and double-counts stats. Please make this publish path idempotent, or make the downstream writes idempotent bydownload_id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/services/queue_manager.rs` around lines 284 - 287, The current code publishes DomainEvent::DownloadCompletedPersisted whenever download.state() == DownloadState::Completed, causing duplicate persisted events; change the logic in the block that calls build_completed_snapshot(&download) and self.event_bus.publish(...) so it only publishes once per download: either (a) detect the completion transition by checking the previous state (publish only when state just changed to DownloadState::Completed), or (b) add and check an explicit persisted flag on the Download (e.g., download.is_completed_persisted() / download.mark_completed_persisted()) and only publish when that flag is false, setting it atomically before/after publish; alternatively ensure downstream idempotency by including download_id as an idempotency key in DomainEvent::DownloadCompletedPersisted and performing upsert-by-download_id in the history and stats handlers—use the symbols DownloadState::Completed, build_completed_snapshot, self.event_bus.publish, and DomainEvent::DownloadCompletedPersisted to locate and update the code.
🧹 Nitpick comments (1)
src-tauri/src/domain/event.rs (1)
118-120: Use one source of truth for the download ID.
DownloadCompletedPersistednow carries bothidandsnapshot.id.src-tauri/src/adapters/driven/event/tauri_bridge.rsemits the outerid, whilesrc-tauri/src/adapters/driven/event/history_recorder_bridge.rsandsrc-tauri/src/adapters/driven/event/stats_recorder_bridge.rsusesnapshot.id. One mismatched publish would refresh one download in the UI and record a different one in history/stats. Prefer removing the outeridand readingsnapshot.ideverywhere, or enforce equality at construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/event.rs` around lines 118 - 120, The DownloadCompletedPersisted variant currently carries both an outer id and snapshot.id which can diverge; fix by removing the outer id and using snapshot.id as the single source of truth: update the enum variant DownloadCompletedPersisted to only hold snapshot: DownloadCompletedSnapshot (remove DownloadId), then change all constructors and pattern matches (e.g., the code in tauri_bridge.rs that emits the outer id, and history_recorder_bridge.rs / stats_recorder_bridge.rs that read snapshot.id) to stop passing/expecting the outer id and instead read the id from snapshot.id; alternatively if you prefer to keep both, add a single constructor/new function that enforces id == snapshot.id and use that everywhere to validate equality (return Result or panic) so they never diverge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src-tauri/src/application/services/queue_manager.rs`:
- Around line 284-287: The current code publishes
DomainEvent::DownloadCompletedPersisted whenever download.state() ==
DownloadState::Completed, causing duplicate persisted events; change the logic
in the block that calls build_completed_snapshot(&download) and
self.event_bus.publish(...) so it only publishes once per download: either (a)
detect the completion transition by checking the previous state (publish only
when state just changed to DownloadState::Completed), or (b) add and check an
explicit persisted flag on the Download (e.g., download.is_completed_persisted()
/ download.mark_completed_persisted()) and only publish when that flag is false,
setting it atomically before/after publish; alternatively ensure downstream
idempotency by including download_id as an idempotency key in
DomainEvent::DownloadCompletedPersisted and performing upsert-by-download_id in
the history and stats handlers—use the symbols DownloadState::Completed,
build_completed_snapshot, self.event_bus.publish, and
DomainEvent::DownloadCompletedPersisted to locate and update the code.
---
Nitpick comments:
In `@src-tauri/src/domain/event.rs`:
- Around line 118-120: The DownloadCompletedPersisted variant currently carries
both an outer id and snapshot.id which can diverge; fix by removing the outer id
and using snapshot.id as the single source of truth: update the enum variant
DownloadCompletedPersisted to only hold snapshot: DownloadCompletedSnapshot
(remove DownloadId), then change all constructors and pattern matches (e.g., the
code in tauri_bridge.rs that emits the outer id, and history_recorder_bridge.rs
/ stats_recorder_bridge.rs that read snapshot.id) to stop passing/expecting the
outer id and instead read the id from snapshot.id; alternatively if you prefer
to keep both, add a single constructor/new function that enforces id ==
snapshot.id and use that everywhere to validate equality (return Result or
panic) so they never diverge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a91ebb2-2939-453d-b67a-e1ce21ab167f
📒 Files selected for processing (11)
CHANGELOG.mdsrc-tauri/src/adapters/driven/event/history_recorder_bridge.rssrc-tauri/src/adapters/driven/event/mod.rssrc-tauri/src/adapters/driven/event/stats_recorder_bridge.rssrc-tauri/src/adapters/driven/event/tauri_bridge.rssrc-tauri/src/adapters/driven/tray/activity_tracker.rssrc-tauri/src/application/services/history_backfill.rssrc-tauri/src/application/services/mod.rssrc-tauri/src/application/services/queue_manager.rssrc-tauri/src/domain/event.rssrc-tauri/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- src-tauri/src/application/services/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/src/adapters/driven/event/mod.rs
- src-tauri/src/adapters/driven/tray/activity_tracker.rs
- CHANGELOG.md
- src-tauri/src/application/services/history_backfill.rs
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
• History table remained empty after every completed download; History view, re-download-from-history (P0.9), and retention purge (P0.14) all silently no-op
• Implemented event bridge pattern (mirrors existing stats_recorder_bridge): subscribe to DownloadCompletedPersisted, project Download to HistoryEntry, persist to history table
• Timestamp conversion: ms→s with 1s floor for avg_speed; file_size with downloaded_bytes fallback
• 9 tests: projection, fallback, duration floor, filtering, missing-download, error swallowing; 2 tokio integration tests against real SQLite
Type
fix
Summary by cubic
Fixes empty History by recording completed downloads into the
historytable via an event-bus bridge and a one-time startup backfill. Unblocks the History view, redownload from history (P0.9), and retention purge (P0.14); fixes vortex-qa BUG #1.history_recorder_bridgeand exportedspawn_history_recorder_bridge; wired inlib.rs.Completeddownloads, skips ones already inhistory, and writes missing rows.DownloadCompletedPersistednow carries aDownloadCompletedSnapshot; both history and stats recorders project from it (no repo re-reads or races). Simplifiedspawn_stats_recorder_bridgeto drop the download-repo dependency.avg_speed;total_bytesprefersfile_sizewithdownloaded_bytesfallback;completed_atfromupdated_at(seconds).tauriand tray bridges to the new event shape; repo errors are logged withwarnand never block the queue/UI.Written for commit a91d5a3. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
New Features