Decouple and complete the resumable Last.fm import pipeline - #42
Conversation
Replace the V1 importer with an account-bound resumable V2 snapshot, lazy Spotify matching, and review flow. Validate cutoff, cache, and account ownership, keep aggregation responsive, and cover the updated UI and persistence behavior. Closes #41
TDD assessment — #41 / PR #42Major
Empirical validationStill required by the approved plan: perform the controlled real Last.fm import, interrupt/relaunch it, verify exact resume and no Spotify request before opening a review batch, and do not apply content. This assessment ran |
Validate cursor-implied acknowledged pages before resume or aggregation, and recheck suspended source caches. Add production seams and coverage for source planning, ownership-safe lazy matching, cached revisits, and sequential Accept All preparation. Closes #41
Validate completed suspended snapshots before resuming aggregation and correct the cache-validation fixtures for metadata and size rejection.
|
TDD assessment disposition:
Follow-up commits: db26a89 and aac67a8. Full workspace tests passed after the main follow-up; the final edge correction passed 51 importer tests, all-target desktop check, rustfmt, and diff-check. |
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: aac67a857fd8
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 2 |
| frontend:view-state | 1 |
| rust:implementation-tests | 1 |
| security:credential-boundary | 0 |
| tauri:config-ipc | 1 |
architecture:seams (2 findings)
Major - apps/desktop/src/LastFmImporter.tsx:297
Retune's architecture assigns long-lived orchestration to the desktop shell and only transient interaction state to React, but this component effect is the sole mechanism that restarts a persisted Downloading/Aggregating session. Because
LastFmImportermounts only in the separately opened importer window, relaunching the main application leaves the durable source runner idle indefinitely until the user opens that window, contrary to the documented relaunch-resume lifecycle. Move persisted-phase inspection and runner claiming/spawning into Tauri startup (or a shell-owned service hydration hook); React should only observe progress and issue an explicit first-start action.
Major - apps/desktop/src-tauri/src/lastfm_import.rs:2056
The account-isolation rule requires Spotify-derived session state to remain bound to its owning Spotify account, but the common readability guard used by
lastfm_import_stateandlastfm_import_queuenow checks only Last.fm identity. After switching Spotify accounts, a bound Review/Done session can therefore return the previousspotifyAccountIdand queue data derived from cached matches beforelastfm_import_pageeventually performs Spotify validation; additionally,start_or_resumecan clear Suspended based solely on Last.fm identity. Split source-phase readability from provider-derived review readability and require exact cached identity or/mevalidation before unredacting state/queue or clearing a Spotify-originated suspension, while keeping Downloading/Aggregating Spotify-free.
frontend:view-state (1 finding)
Major - apps/desktop/test/ui.test.ts:4
This race test only verifies that the stale request does not call the
applycallback; it does not cover invalidating or disabling the already-rendered review page when B is selected. In the component, selecting B leaves page A mounted and actionable while B loads (pageLoadingdisables queue rows but notImportPagecontrols), so a user can accept/skip/change matches for A while the queue highlights B. That violates the new-selection stale-result invariant. Clear the current page (or make its controls inert) before starting the B request, and extend this regression test to assert that no action can target A until B's exact page resolves.
rust:implementation-tests (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1348
checkpoint_pageperformswrite_pagesynchronously from the async import runner. That path reads the manifest, creates directories, and atomically writes a parsed page and manifest; a permitted page may be nearly 100 MiB. Slow app-data storage will therefore block an async runtime worker during normal importing, contrary to the importer’s responsive/background-I/O boundary. Move the cache write intospawn_blockingusing cloned session/page data, then retain the existing cursor recheck inmutate_sessionbefore acknowledging it.
tauri:config-ipc (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1
lastfm_import_pagehas no page/batch size bound. It returns every row in an artist/album group (plus full fuzzy disclosures), and the new lazy path then searches every unmatched row in that group before responding. A valid Last.fm history can place thousands of distinct no-album tracks under one artist, producing an oversized IPC DTO and a long serial Spotify request run that freezes or fails the WebView on opening that review item. Use a stable batch ID with validated offset/limit (and paginate fuzzy disclosures), and cap matching to that bounded batch.
Reviewer Coverage
architecture:seams— complete (broad); inspected 11 assigned files (13 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/types.ts,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: No live Last.fm or Spotify accounts were exercised; findings are based on the changed code, architecture documents, and call-flow inspection. Review limited to ownership, dependency direction, lifecycle, provider isolation, persistence seams, and proportionate design.frontend:view-state— complete (constrained); inspected 1 assigned file (13 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI regression test; traced the importer component and state helpers only to validate the exercised transition.cd apps/desktop && npm run test -- --runpasses (46 tests).rust:implementation-tests— complete (broad); inspected 4 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lastfm.rs,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs; skipped: none; constraints: Targeted Rust test execution could not start because the required RTK command proxy could not spawn cargo in this environment.security:credential-boundary— complete (constrained); inspected 4 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Credential-boundary review only; no live Last.fm or Spotify service calls were performed.tauri:config-ipc— complete (constrained); inspected 5 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lastfm.rs,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review focused on Tauri IPC/DTO and desktop-boundary behavior; no live-service or native-window run was performed.
Inspected files (13)
ARCHITECTURE.mdapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/store.rsapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/lastfmImportState.tsapps/desktop/src/types.tsapps/desktop/test/ui.test.tsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 4m 41s | gpt-5.6-sol, gpt-5.6-terra | cr 0.10.288
| Field | Value |
|---|---|
| Model | gpt-5.6-sol, gpt-5.6-terra |
| Reviewers | architecture:seams, frontend:view-state, rust:implementation-tests, security:credential-boundary, tauri:config-ipc |
| Engine | codex_cli · gpt-5.6-sol, gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 4m 41s wall · 12m 56s compute |
| Cost | unavailable |
| Tokens | 5.1M in / 36.8k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 16.9k
- Out: 607
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 14s
architecture:seams— gpt-5.6-sol- In: 2.1M
- Out: 10.4k
- Cache read: 1.9M
- Cache create: unavailable
- Cost: unavailable
- Duration: 3m 54s
frontend:view-state— gpt-5.6-terra- In: 378.1k
- Out: 4.1k
- Cache read: 316.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 24s
rust:implementation-tests— gpt-5.6-terra- In: 1.1M
- Out: 5.8k
- Cache read: 963.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 04s
security:credential-boundary— gpt-5.6-terra- In: 583.7k
- Out: 5.5k
- Cache read: 494.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 55s
tauri:config-ipc— gpt-5.6-terra- In: 962.0k
- Out: 9.4k
- Cache read: 847.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 3m 06s
orchestrator-rollup— gpt-5.6-terra- In: 61.8k
- Out: 1.0k
- Cache read: 47.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s
Harden account ownership and shell resume, bound review batches, and move cache I/O off the async runtime. Add focused regressions and update the importer architecture docs.
Keep default selections local to each persisted batch and apply album review actions across all sibling batches.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: aae48827eb30
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 0 |
| frontend:view-state | 1 |
| rust:implementation-tests | 1 |
| security:credential-boundary | 1 |
| tauri:config-ipc | 1 |
frontend:view-state (1 finding)
Major - apps/desktop/test/ui.test.ts:132
The new status coverage omits
downloadAction('aggregating', { retryable: false }). That state is reachable when aggregation fails terminally: the runner stops but leaves the persisted session inaggregating; the current helper returns disabled “Preparing review…”, so the user has no in-window way to resume/retry and remains stuck on a misleading progress screen. Add this case and make a non-retryable aggregation error expose an enabled retry/resume action (as the downloading case already does).
rust:implementation-tests (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1357
The resumed-source cache validation still runs synchronously on the async command worker.
validate_cachewalks every acknowledged page and reads/deserializes them, which can be up to the 100 MiB cache limit; resuming a suspended import can therefore block the executor despite the checkpoint write having been moved tospawn_blocking. Run this validation (and the invalidation/quarantine it triggers) in a blocking task, then reacquire/recheck the session before changing it.
security:credential-boundary (1 finding)
Major - apps/desktop/src-tauri/src/lib.rs:2487
Automatic resume starts a persisted Aggregating snapshot without first validating that its Last.fm username still matches the live session. Aggregation performs no Last.fm request, so an account-A snapshot can become Review and the runner's final event exposes its unredacted state to a newly connected account B before any command-level readability check suspends it. Validate the live Last.fm owner before claiming the runner; on mismatch durably suspend first and emit only the redacted state.
tauri:config-ipc (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1
lastfm_import_queuestill returns the entire review queue on every refresh, including every batch'ssource_ids(Service::queuecopies them at line 1758). The 100-row batch cap only bounds each item; a large but valid 100 MiB session can contain thousands of batches, and each state-change refresh—especially Accept All's per-batch loop—serializes the full queue again. This defeats the responsive/bounded IPC boundary and can stall the WebView. Return paginated queue summaries with a validated cursor/limit, and replacesource_idswithsource_count(the frontend only uses its length).
Reviewer Coverage
architecture:seams— complete (constrained); inspected 11 assigned files (13 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/types.ts,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: No live Last.fm or Spotify accounts were exercised; review used the changed code, architecture documents, and recorded validation evidence. Review limited to ownership, dependency direction, lifecycle, provider isolation, persistence seams, and proportionate design.frontend:view-state— complete (constrained); inspected 1 assigned file (13 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI regression test, with importer/state-helper tracing for context.cd apps/desktop && npm run test -- --runpasses (46 tests).rust:implementation-tests— complete (constrained); inspected 4 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lastfm.rs,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs; skipped: none; constraints: nonesecurity:credential-boundary— complete (constrained); inspected 4 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Credential-boundary review only; no live Last.fm or Spotify service calls were performed.tauri:config-ipc— complete (constrained); inspected 5 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lastfm.rs,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri IPC/DTO review; no live-service or native-window run was performed.
Inspected files (13)
ARCHITECTURE.mdapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/store.rsapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/lastfmImportState.tsapps/desktop/src/types.tsapps/desktop/test/ui.test.tsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 29s | gpt-5.6-sol, gpt-5.6-terra | cr 0.10.288
| Field | Value |
|---|---|
| Model | gpt-5.6-sol, gpt-5.6-terra |
| Reviewers | architecture:seams, frontend:view-state, rust:implementation-tests, security:credential-boundary, tauri:config-ipc |
| Engine | codex_cli · gpt-5.6-sol, gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 2m 29s wall · 6m 58s compute |
| Cost | unavailable |
| Tokens | 10.0M in / 55.5k out |
Per-workstream usage
architecture:seams— gpt-5.6-sol- In: 3.9M
- Out: 16.3k
- Cache read: 3.6M
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 07s
frontend:view-state— gpt-5.6-terra- In: 820.3k
- Out: 6.8k
- Cache read: 718.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 59s
rust:implementation-tests— gpt-5.6-terra- In: 2.0M
- Out: 8.5k
- Cache read: 1.8M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 04s
security:credential-boundary— gpt-5.6-terra- In: 1.2M
- Out: 8.1k
- Cache read: 1.0M
- Cache create: unavailable
- Cost: unavailable
- Duration: 54s
tauri:config-ipc— gpt-5.6-terra- In: 2.0M
- Out: 14.7k
- Cache read: 1.8M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 46s
orchestrator-rollup— gpt-5.6-terra- In: 90.1k
- Out: 1.2k
- Cache read: 70.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
Paginate queue summaries without source IDs, move resumed cache validation off the async runtime, and verify the live Last.fm identity before resuming aggregation. Keep terminal aggregation recoverable and Accept All refreshes bounded.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 1dc44881d4ef
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 0 |
| frontend:view-state | 0 |
| rust:implementation-tests | 1 |
| security:credential-boundary | 1 |
| tauri:config-ipc | 1 |
rust:implementation-tests (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1814
queue_pagefirst builds the entire queue withself.queue()and only then slices it. The frontend requests every cursor to hydrate its queue, so an import with many 100-row batches repeatedly rebuilds all queue items—quadratic total work and repeated large allocations on the async worker. This defeats the stated bounded queue-page contract and can make a large review unresponsive. Derive only the requested batch range from the session (or compute/cache a queue projection once per session revision), rather than materializing the full queue for every cursor.
security:credential-boundary (1 finding)
Major - apps/desktop/src-tauri/src/lib.rs:2487
The startup check is only a point-in-time Last.fm ownership check. If account A passes it, then the Last.fm session changes while the Aggregating runner is doing its blocking work, aggregation can still commit A's snapshot to Review and emit its unredacted state to account B. Serialize Last.fm account changes against the runner, or recheck and suspend the expected owner immediately before the aggregation commit/event so an account switch cannot publish the prior account's session.
tauri:config-ipc (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1894
The batch response is still not bounded: it attaches the entire session-wide
count_modesmap andlocked_count_modesset to every 100-rowImportPageView. As a large import progresses, the locked set grows with every completed Spotify target, so opening one batch repeatedly serializes unrelated session state and can again create an oversized IPC response. Project both collections to the target URIs present in this page'sfuzzy_groups(the only values the frontend reads), while keeping the complete maps persisted in the session.
Reviewer Coverage
architecture:seams— complete (constrained); inspected 11 assigned files (13 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/types.ts,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: No connected-service or live app-data validation was performed; assessment is based on the checked-out head and repository architecture contracts. Review was limited to ownership, dependency direction, lifecycle, provider isolation, and proportional abstraction; implementation, IPC, credential, and React behavior defects were out of scope.frontend:view-state— complete (constrained); inspected 1 assigned file (13 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI regression test, with importer/state-helper tracing for context.cd apps/desktop && npm run test -- --runpasses (46 tests).rust:implementation-tests— complete (constrained); inspected 4 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lastfm.rs,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs; skipped: none; constraints: nonesecurity:credential-boundary— complete (constrained); inspected 4 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Credential-boundary review only; no live Last.fm or Spotify service calls were performed.tauri:config-ipc— complete (constrained); inspected 5 assigned files (13 inspected across reviewers):apps/desktop/src-tauri/src/lastfm.rs,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/store.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri IPC/DTO review; no live-service or native-window run was performed.
Inspected files (13)
ARCHITECTURE.mdapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/store.rsapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/lastfmImportState.tsapps/desktop/src/types.tsapps/desktop/test/ui.test.tsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 21s | gpt-5.6-sol, gpt-5.6-terra | cr 0.10.288
| Field | Value |
|---|---|
| Model | gpt-5.6-sol, gpt-5.6-terra |
| Reviewers | architecture:seams, frontend:view-state, rust:implementation-tests, security:credential-boundary, tauri:config-ipc |
| Engine | codex_cli · gpt-5.6-sol, gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 21s wall · 6m 09s compute |
| Cost | unavailable |
| Tokens | 13.4M in / 70.1k out |
Per-workstream usage
architecture:seams— gpt-5.6-sol- In: 5.0M
- Out: 22.4k
- Cache read: 4.6M
- Cache create: unavailable
- Cost: unavailable
- Duration: 3m 00s
frontend:view-state— gpt-5.6-terra- In: 1.3M
- Out: 9.0k
- Cache read: 1.2M
- Cache create: unavailable
- Cost: unavailable
- Duration: 48s
rust:implementation-tests— gpt-5.6-terra- In: 2.8M
- Out: 10.5k
- Cache read: 2.6M
- Cache create: unavailable
- Cost: unavailable
- Duration: 43s
security:credential-boundary— gpt-5.6-terra- In: 1.6M
- Out: 10.3k
- Cache read: 1.4M
- Cache create: unavailable
- Cost: unavailable
- Duration: 48s
tauri:config-ipc— gpt-5.6-terra- In: 2.6M
- Out: 16.6k
- Cache read: 2.4M
- Cache create: unavailable
- Cost: unavailable
- Duration: 41s
orchestrator-rollup— gpt-5.6-terra- In: 121.2k
- Out: 1.3k
- Cache read: 98.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 5s
Project review count modes to visible fuzzy targets and derive queue pages directly from the session snapshot. Serialize aggregation commits and importer events with the connected Last.fm owner.
1 participant
Summary
Validation
cargo test --workspace— passed (339 desktop, 56 Spotify; 1 hardware test ignored)cargo check -p retune-desktop --all-targets— passedcargo test -p retune-desktop lastfm_import::tests— 44 passednpm run test -- --run— 46 passednpm exec -- tsc --noEmit -p tsconfig.app.json— passednpm run lint— passed with 7 pre-existing React hooks warningsnode scripts/check-docs.mjs, Rustfmt, andgit diff --check— passedNo connected-service/live proof or live app-data mutation was performed.
Closes #41