feat(desktop): add resumable Last.fm importer - #40
Conversation
![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: 7be700a94e5c
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 | 2 |
architecture:seams (2 findings)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:787
Retune's single-owner/source-of-truth rule is violated because every mutation clones
session, drops its mutex, writes the full file, and only then reacquires the mutex to replace live state. The runner and concurrent Tauri commands can therefore overwrite unrelated matches/options/decisions; adverse interleaving can even leave memory containing mutation A whilelastfm-import.jsoncontains mutation B, changing state after relaunch. Centralize importer mutations behind one serialized read-modify-persist-commit transaction (or revision/CAS with rebase for long operations) so the durable file and live session advance together.
Major - apps/desktop/src-tauri/src/lastfm_import.rs:964
The account-bound lifecycle transition is not guarded.
run_matchingvalidates the account only once before processing the entire history, whileset_matchesaccepts results in any phase andfinish_matchingunconditionally changes the phase toReview. If an account change suspends the session while a Spotify search is in flight, that runner can keep checkpointing results under the replacement credentials and finally clearSuspended; the final apply rechecks the account, but the durable safety state and matches have already been corrupted. Give each runner an account/session generation and revalidate it plusphase == Matchingbefore every checkpoint and before the final transition, aborting without holding the membership gate across searches.
frontend:view-state (1 finding)
Major - apps/desktop/test/ui.test.ts:81
This test calls only the synchronous
nextRemainingImportQueuehelper, so it does not cover the claimed stale-result protection. InLastFmImporter.openQueueItem, two rapid queue selections can resolve out of order: the olderlastfm_import_pageresponse still callssetPage, replacing the page for the newer selected album. A user can then review/apply the wrong album. Add a deterministic deferred-invoke regression test for A→B selection with A resolving last, and make the view ignore/abort responses whose requested artist/album is no longer current.
rust:implementation-tests (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:787
Service::savecalls synchronous JSON serialization, fsync, and rename directly from Tauri async commands and the importer async worker. Large resumable sessions can therefore block a Tokio worker (and all commands scheduled on it) for filesystem latency, contrary to the persistence boundary. Move the store write intospawn_blocking/a dedicated persistence worker, and add a testable serialization boundary that preserves the atomic-write error behavior.
tauri:config-ipc (2 findings)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:2031
lastfm_import_state,lastfm_import_queue, andlastfm_import_pagereturn the persisted session without verifying its Last.fm/Spotify account binding. The importer calls all three as soon as its window opens, so after accounts change it displays the previous account's history and review data until a later mutating command happens to suspend it. Gate snapshot reads (or window opening) on the same account comparison, suspend on mismatch, and return only the suspended state with no queue/page data.
Minor - apps/desktop/src-tauri/capabilities/default.json:7
Adding
lastfm-importerto the main window's default capability grants the importerdialog:defaultandopener:default, even though this dedicated review surface does not need the main window's broad plugin grants. This widens the impact of a renderer compromise in the importer. Give it a separate capability with only its required core/window permissions and, if the attribution link requires it, the narrow opener permission rather than the dialog and opener defaults.
Reviewer Coverage
architecture:seams— complete (broad); inspected 17 assigned files (21 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/dialogViews.tsx,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/lastfmImporter.css,apps/desktop/src/main.tsx,apps/desktop/src/types.ts,crates/retune-spotify/src/client.rs,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Narrow architecture review only; implementation, frontend behavior, Tauri IPC, credential security, and live Spotify/Last.fm validation were outside scope.frontend:view-state— complete (constrained); inspected 1 assigned file (21 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI test file; inspected related importer state/component code only to validate test coverage.rust:implementation-tests— complete (broad); inspected 6 assigned files (21 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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,crates/retune-spotify/src/client.rs; skipped: none; constraints: Focused Rust test command could not run because the RTK wrapper could not spawn cargo in this review environment.security:credential-boundary— complete (constrained); inspected 5 assigned files (21 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Review was limited to credential, account-binding, and sensitive-state boundaries. Focused Rust tests could not run because Cargo was unavailable in this environment.tauri:config-ipc— complete (constrained); inspected 7 assigned files (21 inspected across reviewers):apps/desktop/src-tauri/capabilities/default.json,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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri configuration and IPC review; no native app or connected-account validation was run.
Inspected files (21)
ARCHITECTURE.mdapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/provider.rsapps/desktop/src-tauri/src/spotify_commands.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/dialogViews.tsxapps/desktop/src/lastfmImportState.tsapps/desktop/src/lastfmImporter.cssapps/desktop/src/main.tsxapps/desktop/src/types.tsapps/desktop/test/ui.test.tscrates/retune-spotify/src/client.rsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 58s | 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 58s wall · 10m 39s compute |
| Cost | unavailable |
| Tokens | 3.2M in / 30.5k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 17.7k
- Out: 747
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 17s
architecture:seams— gpt-5.6-sol- In: 1.1M
- Out: 9.2k
- Cache read: 951.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 3m 16s
frontend:view-state— gpt-5.6-terra- In: 184.5k
- Out: 2.3k
- Cache read: 141.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 50s
rust:implementation-tests— gpt-5.6-terra- In: 450.3k
- Out: 4.1k
- Cache read: 380.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 28s
security:credential-boundary— gpt-5.6-terra- In: 663.9k
- Out: 5.4k
- Cache read: 572.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 59s
tauri:config-ipc— gpt-5.6-terra- In: 731.1k
- Out: 7.4k
- Cache read: 623.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 31s
orchestrator-rollup— gpt-5.6-terra- In: 40.2k
- Out: 1.2k
- Cache read: 28.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 14s
| "windows": [ | ||
| "main" | ||
| "main", | ||
| "lastfm-importer" |
There was a problem hiding this comment.
Adding lastfm-importer to the main window's default capability grants the importer dialog:default and opener:default, even though this dedicated review surface does not need the main window's broad plugin grants. This widens the impact of a renderer compromise in the importer. Give it a separate capability with only its required core/window permissions and, if the attribution link requires it, the narrow opener permission rather than the dialog and opener defaults.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in d5360f1. The main default capability now covers only main and retains the dialog/opener permissions it uses. Added capabilities/lastfm-importer.json scoped to lastfm-importer with core/window-title and opener permissions only—no dialog permission. Frontend typecheck/build, Rust clippy, and workspace tests pass.
There was a problem hiding this comment.
Thanks—this addresses the capability-scope concern by isolating the importer and removing its dialog permission.
Summary:
Resolved: the importer now has a dedicated, least-privilege capability without dialog access; the main window retains its existing broader permissions.
There was a problem hiding this comment.
Non-actionable in pass 2: the capability scope remains fixed in d5360f1. The importer capability is separate, has core/window-title and opener only, and has no dialog permission; no additional code change was needed.
There was a problem hiding this comment.
Resolved: the importer remains isolated in a dedicated least-privilege capability with no dialog permission; no further change is needed.
| } | ||
|
|
||
| #[tauri::command] | ||
| pub(crate) async fn lastfm_import_state( |
There was a problem hiding this comment.
lastfm_import_state, lastfm_import_queue, and lastfm_import_page return the persisted session without verifying its Last.fm/Spotify account binding. The importer calls all three as soon as its window opens, so after accounts change it displays the previous account's history and review data until a later mutating command happens to suspend it. Gate snapshot reads (or window opening) on the same account comparison, suspend on mismatch, and return only the suspended state with no queue/page data.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in d5360f1. State/queue/page reads now verify the current Last.fm identity and Spotify connection, using the exact in-memory Spotify library account when available and the shortest /me check otherwise. Sign-out/mismatch suspends the session; suspended state is redacted, queue is empty, and page is None. Regression: suspended_reads_are_redacted_and_empty.
There was a problem hiding this comment.
Thanks—gating all snapshot reads and redacting suspended responses addresses the cross-account disclosure path.
Summary:
Resolved: importer state, queue, and page reads now verify both account bindings; mismatch or sign-out suspends and returns no history, queue, or page data, with regression coverage.
There was a problem hiding this comment.
Non-actionable in pass 2: the read-boundary fix remains in d5360f1. State, queue, and page reads verify the bound accounts and return redacted Suspended/empty data on mismatch; the existing suspended-read regression and full workspace tests pass.
There was a problem hiding this comment.
Resolved: account-bound snapshot reads remain guarded, and mismatch or sign-out yields only redacted suspended state with empty queue/page data.
| self.session.lock().await.clone() | ||
| } | ||
|
|
||
| async fn save(&self, session: LastFmImportSessionV1) -> Result<(), String> { |
There was a problem hiding this comment.
Retune's single-owner/source-of-truth rule is violated because every mutation clones session, drops its mutex, writes the full file, and only then reacquires the mutex to replace live state. The runner and concurrent Tauri commands can therefore overwrite unrelated matches/options/decisions; adverse interleaving can even leave memory containing mutation A while lastfm-import.json contains mutation B, changing state after relaunch. Centralize importer mutations behind one serialized read-modify-persist-commit transaction (or revision/CAS with rebase for long operations) so the durable file and live session advance together.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in d5360f1. All Service read-modify-write mutations now use one mutate_session boundary that holds the session mutex from the source snapshot through blocking durable persistence and replaces in-memory state only after the atomic write succeeds. Added overlapping_mutations_preserve_memory_and_disk_changes, which runs concurrent search-term/count-mode mutations and verifies both memory and a reloaded session. This reply addresses both duplicate reports 1 and 7.
There was a problem hiding this comment.
Thanks—the serialized mutation boundary and concurrent reload regression address the lost-update and memory/disk divergence race.
Summary:
Resolved: importer mutations now serialize snapshot, atomic persistence, and in-memory commit in one boundary; concurrent updates are verified to survive both live state and reload.
There was a problem hiding this comment.
Non-actionable in pass 2: the serialized mutation boundary remains correct in d5360f1. Snapshot, blocking atomic persistence, and in-memory replacement stay under one mutex; the overlapping mutation regression and workspace clippy/tests pass.
There was a problem hiding this comment.
Resolved: mutations serialize snapshot, atomic persistence, and in-memory commit under one boundary, preventing lost updates and live/disk divergence.
| let Some(mut session) = self.snapshot().await else { | ||
| return Ok(()); | ||
| }; | ||
| session.phase = ImportPhase::Review; |
There was a problem hiding this comment.
The account-bound lifecycle transition is not guarded. run_matching validates the account only once before processing the entire history, while set_matches accepts results in any phase and finish_matching unconditionally changes the phase to Review. If an account change suspends the session while a Spotify search is in flight, that runner can keep checkpointing results under the replacement credentials and finally clear Suspended; the final apply rechecks the account, but the durable safety state and matches have already been corrupted. Give each runner an account/session generation and revalidate it plus phase == Matching before every checkpoint and before the final transition, aborting without holding the membership gate across searches.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in d5360f1. Background matching now revalidates the connected account before every durable group checkpoint, set_matches_during_matching rejects phase/account changes without writing, and finalization rechecks the account before entering Review. Suspended sessions also reject retry writes. Regression: matching_checkpoint_and_finalization_cannot_write_through_suspension.
There was a problem hiding this comment.
Thanks—checkpoint and finalization guards now prevent an in-flight matcher from writing through suspension or an account change.
Summary:
Resolved: matching revalidates account ownership before durable checkpoints and finalization, rejects phase/account-invalid writes, and includes regression coverage for suspension during matching.
There was a problem hiding this comment.
Non-actionable in pass 2: matching checkpoints and finalization still revalidate phase and account before durable writes. The suspension regression remains green in the 27-test importer suite and full workspace tests.
There was a problem hiding this comment.
Resolved: matching validates account ownership and phase before each durable checkpoint and finalization, preventing writes through suspension or account changes.
| self.session.lock().await.clone() | ||
| } | ||
|
|
||
| async fn save(&self, session: LastFmImportSessionV1) -> Result<(), String> { |
There was a problem hiding this comment.
Service::save calls synchronous JSON serialization, fsync, and rename directly from Tauri async commands and the importer async worker. Large resumable sessions can therefore block a Tokio worker (and all commands scheduled on it) for filesystem latency, contrary to the persistence boundary. Move the store write into spawn_blocking/a dedicated persistence worker, and add a testable serialization boundary that preserves the atomic-write error behavior.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in d5360f1. Session JSON serialization and atomic fsync/rename now run inside tauri::async_runtime::spawn_blocking, while the serialized mutation boundary remains held to preserve ordering. overlapping_mutations_preserve_memory_and_disk_changes verifies concurrent changes survive both the live session and a disk reload.
There was a problem hiding this comment.
spawn_blocking addresses the Tokio-worker blocking concern, and the concurrent-mutation test covers ordering. Could you also add coverage for an atomic-write/serialization failure propagating through the new blocking boundary without committing the live mutation? That was the remaining requested error-behavior guarantee.
There was a problem hiding this comment.
Accepted and fixed in 6cbafa3. Added failed_blocking_persistence_does_not_commit_live_mutation: it forces the atomic rename to fail through the spawn_blocking boundary and verifies the error plus unchanged live and reloaded disk session. Importer tests: 27/27; workspace tests: 414 passed, 1 ignored.
There was a problem hiding this comment.
Thanks—the new failure-injection regression covers the missing atomic-write error path through the blocking boundary.
Summary:
Resolved: blocking persistence now propagates atomic-write failure without committing the live mutation or changing the persisted session, with regression coverage.
| assert.equal(validImportIntent(false, false), false) | ||
| }) | ||
|
|
||
| test('Last.fm per-target fuzzy strategies and stale-free queue advancement are deterministic', () => { |
There was a problem hiding this comment.
This test calls only the synchronous nextRemainingImportQueue helper, so it does not cover the claimed stale-result protection. In LastFmImporter.openQueueItem, two rapid queue selections can resolve out of order: the older lastfm_import_page response still calls setPage, replacing the page for the newer selected album. A user can then review/apply the wrong album. Add a deterministic deferred-invoke regression test for A→B selection with A resolving last, and make the view ignore/abort responses whose requested artist/album is no longer current.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in d5360f1. Importer page loads now use a monotonic pageRequestGeneration ref; refresh and queue selection apply a response only when its generation is current, so an older promise cannot replace a newer selection. Added deterministic isCurrentImportPageResponse coverage in the frontend test suite.
There was a problem hiding this comment.
The generation guard is the right implementation direction. Could you add the requested deferred-invoke A→B regression that resolves A last and asserts it cannot replace B’s rendered page? Predicate-level isCurrentImportPageResponse coverage alone does not exercise the asynchronous selection path.
There was a problem hiding this comment.
Accepted and fixed in 6cbafa3. Added applyCurrentImportPageResponse, used by refresh and openQueueItem, plus a deferred A→B test that resolves A last and asserts only B is applied. Frontend tests: 43/43; TypeScript and production build pass.
There was a problem hiding this comment.
Thanks—the shared response guard and deferred A→B regression now exercise the actual out-of-order selection path.
Summary:
Resolved: refresh and queue selection ignore stale page responses, with a deterministic A→B test proving that a late A response cannot replace B.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: d5360f184b58
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 1 |
| frontend:view-state | 1 |
| rust:implementation-tests | 1 |
| security:credential-boundary | 0 |
| tauri:config-ipc | 0 |
architecture:seams (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1234
The account-bound lifecycle still is not enforced at the mutation boundary.
review_actionand the sibling options/count-mode/search-term commands mutate a session without validating its owner or rejectingSuspended;update_review_phasecan consequently turn a suspended session intoDone, making the previously redacted history visible under the replacement account. Manual track/album rematches also check the account only before their Spotify search and then commit through owner-agnosticset_match(es), so an account switch during the request can persist results from the new credentials into the old session. Require the expected Last.fm/Spotify binding and allowed phase in the serialized mutation itself for every command, revalidate after asynchronous searches, and make suspension terminal except through the owner-validated resume transition.
frontend:view-state (1 finding)
Major - apps/desktop/test/ui.test.ts:93
These assertions test only the equality predicate, not the asynchronous queue-selection flow it is meant to protect. The required regression is an A→B queue selection with deferred
lastfm_import_pagecalls, resolving A last and asserting that B remains the rendered page. Without that integration-level fixture, removing/misapplying the generation check inopenQueueItemwould still leave this test green, so the stale-page user regression is not covered.
rust:implementation-tests (1 finding)
Minor - apps/desktop/src-tauri/src/lastfm_import.rs:792
The new
spawn_blockingpersistence boundary has no regression test for a serialization/atomic-write failure. The concurrent test proves successful ordering, but not the important invariant here: a failed blocking write must return its error while leaving the live session and its on-disk predecessor unchanged. Add a hermetic failure injection (for example, an unwritable/replaced store path) around a mutation and assert both state snapshots remain at the prior value.
Reviewer Coverage
architecture:seams— complete (constrained); inspected 18 assigned files (22 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/capabilities/lastfm-importer.json,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/dialogViews.tsx,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/lastfmImporter.css,apps/desktop/src/main.tsx,apps/desktop/src/types.ts,crates/retune-spotify/src/client.rs,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Narrow architecture review only; Rust implementation/test defects, Tauri IPC details, credential handling, frontend behavior, and live provider validation were outside scope.frontend:view-state— complete (constrained); inspected 1 assigned file (22 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI test file; inspected related importer code only to validate the claimed regression coverage.rust:implementation-tests— complete (constrained); inspected 6 assigned files (22 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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,crates/retune-spotify/src/client.rs; skipped: none; constraints: nonesecurity:credential-boundary— complete (constrained); inspected 5 assigned files (22 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Read-only review; automated tests were not run.tauri:config-ipc— complete (constrained); inspected 7 assigned files (22 inspected across reviewers):apps/desktop/src-tauri/capabilities/default.json,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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri configuration and IPC review; no native app or connected-account validation was run.
Inspected files (22)
ARCHITECTURE.mdapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src-tauri/capabilities/lastfm-importer.jsonapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/provider.rsapps/desktop/src-tauri/src/spotify_commands.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/dialogViews.tsxapps/desktop/src/lastfmImportState.tsapps/desktop/src/lastfmImporter.cssapps/desktop/src/main.tsxapps/desktop/src/types.tsapps/desktop/test/ui.test.tscrates/retune-spotify/src/client.rsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
6 PR discussion threads considered. 4 summarized; 4 resolved.
Completed in 4m 06s | 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 06s wall · 7m 03s compute |
| Cost | unavailable |
| Tokens | 7.3M in / 49.7k out |
Per-workstream usage
architecture:seams— gpt-5.6-sol- In: 2.8M
- Out: 16.0k
- Cache read: 2.6M
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 36s
frontend:view-state— gpt-5.6-terra- In: 450.3k
- Out: 3.7k
- Cache read: 385.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 33s
rust:implementation-tests— gpt-5.6-terra- In: 820.1k
- Out: 6.0k
- Cache read: 722.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 42s
security:credential-boundary— gpt-5.6-terra- In: 1.4M
- Out: 9.4k
- Cache read: 1.3M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 22s
tauri:config-ipc— gpt-5.6-terra- In: 1.6M
- Out: 11.9k
- Cache read: 1.4M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 34s
orchestrator-rollup— gpt-5.6-terra- In: 253.1k
- Out: 2.7k
- Cache read: 219.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 13s
| self.session.lock().await.clone() | ||
| } | ||
|
|
||
| async fn persist(&self, session: LastFmImportSessionV1) -> Result<(), String> { |
There was a problem hiding this comment.
The new spawn_blocking persistence boundary has no regression test for a serialization/atomic-write failure. The concurrent test proves successful ordering, but not the important invariant here: a failed blocking write must return its error while leaving the live session and its on-disk predecessor unchanged. Add a hermetic failure injection (for example, an unwritable/replaced store path) around a mutation and assert both state snapshots remain at the prior value.
Reply inline to this comment.
| assert.equal(remainingImportCount(state), 1) | ||
| const done = { ...importQueue()[0], remaining: false, status: 'done' as const } | ||
| assert.equal(nextRemainingImportQueue([done, importQueue()[1]], done, 'plays')?.artist, 'Alpha') | ||
| assert.equal(isCurrentImportPageResponse(1, 2), false) |
There was a problem hiding this comment.
These assertions test only the equality predicate, not the asynchronous queue-selection flow it is meant to protect. The required regression is an A→B queue selection with deferred lastfm_import_page calls, resolving A last and asserting that B remains the rendered page. Without that integration-level fixture, removing/misapplying the generation check in openQueueItem would still leave this test green, so the stale-page user regression is not covered.
Reply inline to this comment.
| artist: &str, | ||
| album: &str, | ||
| ) -> Result<(), String> { | ||
| self.mutate_session(|session| { |
There was a problem hiding this comment.
The account-bound lifecycle still is not enforced at the mutation boundary. review_action and the sibling options/count-mode/search-term commands mutate a session without validating its owner or rejecting Suspended; update_review_phase can consequently turn a suspended session into Done, making the previously redacted history visible under the replacement account. Manual track/album rematches also check the account only before their Spotify search and then commit through owner-agnostic set_match(es), so an account switch during the request can persist results from the new credentials into the old session. Require the expected Last.fm/Spotify binding and allowed phase in the serialized mutation itself for every command, revalidate after asynchronous searches, and make suspension terminal except through the owner-validated resume transition.
Reply inline to this comment.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6cbafa3aa621
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 1 |
| frontend:view-state | 0 |
| rust:implementation-tests | 0 |
| security:credential-boundary | 0 |
| tauri:config-ipc | 0 |
architecture:seams (1 finding)
Major - apps/desktop/src-tauri/src/lastfm_import.rs:1234
The account-bound lifecycle still lacks one guarded mutation boundary.
review_actionand the options/count/search-term mutations can modify a suspended session without validating its Last.fm/Spotify owner;update_review_phasecan consequently changeSuspendedtoDone, making previously redacted prior-account data readable. Manual rematches also validate only before the Spotify search and then commit through owner-agnostic setters, so an account switch during the request can write stale results. This violates the single-owner lifecycle rule. Pass the expected account binding and allowed phase into every serialized mutation, treat suspension as terminal except for an owner-validated resume, and revalidate after network work before committing.
Reviewer Coverage
architecture:seams— complete (constrained); inspected 18 assigned files (22 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/capabilities/lastfm-importer.json,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/dialogViews.tsx,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/lastfmImporter.css,apps/desktop/src/main.tsx,apps/desktop/src/types.ts,crates/retune-spotify/src/client.rs,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Narrow architecture review; Rust implementation/test defects, Tauri boundary details, credential handling, and React behavior were excluded. Static review only; no live Spotify, Last.fm, or native-app validation was performed.frontend:view-state— complete (constrained); inspected 1 assigned file (22 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI test file; inspected related importer state/component code to validate the deferred-response coverage.rust:implementation-tests— complete (constrained); inspected 6 assigned files (22 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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,crates/retune-spotify/src/client.rs; skipped: none; constraints: nonesecurity:credential-boundary— complete (constrained); inspected 5 assigned files (22 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Read-only review; automated tests were not run.tauri:config-ipc— complete (constrained); inspected 7 assigned files (22 inspected across reviewers):apps/desktop/src-tauri/capabilities/default.json,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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri configuration and IPC review; no native app or connected-account validation was run.
Inspected files (22)
ARCHITECTURE.mdapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src-tauri/capabilities/lastfm-importer.jsonapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/provider.rsapps/desktop/src-tauri/src/spotify_commands.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/dialogViews.tsxapps/desktop/src/lastfmImportState.tsapps/desktop/src/lastfmImporter.cssapps/desktop/src/main.tsxapps/desktop/src/types.tsapps/desktop/test/ui.test.tscrates/retune-spotify/src/client.rsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
6 PR discussion threads considered. 6 summarized; 6 resolved.
Completed in 2m 52s | 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 52s wall · 3m 34s compute |
| Cost | unavailable |
| Tokens | 9.7M in / 57.3k out |
Per-workstream usage
architecture:seams— gpt-5.6-sol- In: 3.3M
- Out: 17.5k
- Cache read: 3.1M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 19s
frontend:view-state— gpt-5.6-terra- In: 783.3k
- Out: 4.8k
- Cache read: 698.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 29s
rust:implementation-tests— gpt-5.6-terra- In: 1.3M
- Out: 7.5k
- Cache read: 1.2M
- Cache create: unavailable
- Cost: unavailable
- Duration: 37s
security:credential-boundary— gpt-5.6-terra- In: 1.7M
- Out: 10.3k
- Cache read: 1.5M
- Cache create: unavailable
- Cost: unavailable
- Duration: 25s
tauri:config-ipc— gpt-5.6-terra- In: 2.0M
- Out: 13.4k
- Cache read: 1.8M
- Cache create: unavailable
- Cost: unavailable
- Duration: 33s
orchestrator-rollup— gpt-5.6-terra- In: 594.7k
- Out: 3.7k
- Cache read: 539.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 8s
| artist: &str, | ||
| album: &str, | ||
| ) -> Result<(), String> { | ||
| self.mutate_session(|session| { |
There was a problem hiding this comment.
The account-bound lifecycle still lacks one guarded mutation boundary. review_action and the options/count/search-term mutations can modify a suspended session without validating its Last.fm/Spotify owner; update_review_phase can consequently change Suspended to Done, making previously redacted prior-account data readable. Manual rematches also validate only before the Spotify search and then commit through owner-agnostic setters, so an account switch during the request can write stale results. This violates the single-owner lifecycle rule. Pass the expected account binding and allowed phase into every serialized mutation, treat suspension as terminal except for an owner-validated resume, and revalidate after network work before committing.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in commit a1e4966b83b3c28c1158bdb0dc5f044c30f5b4d1. All review/options/count/search-term/rematch writes now pass through the serialized mutate_owned_session boundary, which validates the persisted Last.fm username, Spotify account, and allowed phase before replacing memory or disk. Suspended sessions are terminal for reads/mutations; matching checkpoints/finalization and apply revalidate ownership after work before committing. Added owned_review_mutations_reject_mismatch_and_suspension; focused importer tests pass 28/28, workspace clippy passes with -D warnings, and the escalated full workspace test passes 416 tests with 1 ignored.
There was a problem hiding this comment.
Thanks—the owner- and phase-validated mutation boundary closes the suspended-session and post-search rematch commit paths.
Summary:
Resolved: review, preference, and rematch mutations now validate bound accounts and allowed phase before persistence; suspension is terminal and ownership is revalidated after asynchronous work.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: a1e4966b83b3
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 0 |
| frontend:view-state | 1 |
| rust:implementation-tests | 0 |
| security:credential-boundary | 0 |
| tauri:config-ipc | 0 |
frontend:view-state (1 finding)
Major - apps/desktop/test/ui.test.ts:97
This is still a unit test for
applyCurrentImportPageResponse, not an A→B queue-selection test. It manually supplies generations and callbacks, so it remains green ifopenQueueItemstops incrementingpageRequestGeneration, fails to use the helper, or wires the response to the wrong page state—the actual user-visible stale-page regression. Add a deterministic test harness for the selection/load controller (or render the importer with mocked deferredlastfm_import_pagecalls) that selects A then B, resolves B then A, and asserts B stays rendered.
Reviewer Coverage
architecture:seams— complete (constrained); inspected 18 assigned files (22 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/capabilities/lastfm-importer.json,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/dialogViews.tsx,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/lastfmImporter.css,apps/desktop/src/main.tsx,apps/desktop/src/types.ts,crates/retune-spotify/src/client.rs,docs/architecture/library.md,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Narrow static architecture review; Rust implementation/test defects, Tauri boundary details, credential handling, and React behavior were excluded. No live Spotify, Last.fm, or native-app validation was performed.frontend:view-state— complete (constrained); inspected 1 assigned file (22 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI test file; inspected related importer state/component code to validate stale-response coverage.rust:implementation-tests— complete (constrained); inspected 6 assigned files (22 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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,crates/retune-spotify/src/client.rs; skipped: none; constraints: nonesecurity:credential-boundary— complete (constrained); inspected 5 assigned files (22 inspected across reviewers):apps/desktop/src-tauri/src/lib.rs,apps/desktop/src-tauri/src/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,docs/architecture/persistence.md,docs/architecture/spotify.md; skipped: none; constraints: Read-only review; automated tests were not run.tauri:config-ipc— complete (constrained); inspected 7 assigned files (22 inspected across reviewers):apps/desktop/src-tauri/capabilities/default.json,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/provider.rs,apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri configuration and IPC review; no native app or connected-account validation was run.
Inspected files (22)
ARCHITECTURE.mdapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src-tauri/capabilities/lastfm-importer.jsonapps/desktop/src-tauri/src/lastfm.rsapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/provider.rsapps/desktop/src-tauri/src/spotify_commands.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/dialogViews.tsxapps/desktop/src/lastfmImportState.tsapps/desktop/src/lastfmImporter.cssapps/desktop/src/main.tsxapps/desktop/src/types.tsapps/desktop/test/ui.test.tscrates/retune-spotify/src/client.rsdocs/architecture/library.mddocs/architecture/persistence.mddocs/architecture/spotify.md
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 3m 00s | 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 00s wall · 5m 13s compute |
| Cost | unavailable |
| Tokens | 13.3M in / 70.6k out |
Per-workstream usage
architecture:seams— gpt-5.6-sol- In: 4.1M
- Out: 23.6k
- Cache read: 3.8M
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 16s
frontend:view-state— gpt-5.6-terra- In: 1.1M
- Out: 6.0k
- Cache read: 982.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 28s
rust:implementation-tests— gpt-5.6-terra- In: 1.8M
- Out: 9.1k
- Cache read: 1.7M
- Cache create: unavailable
- Cost: unavailable
- Duration: 38s
security:credential-boundary— gpt-5.6-terra- In: 2.8M
- Out: 12.8k
- Cache read: 2.6M
- Cache create: unavailable
- Cost: unavailable
- Duration: 59s
tauri:config-ipc— gpt-5.6-terra- In: 2.8M
- Out: 15.3k
- Cache read: 2.6M
- Cache create: unavailable
- Cost: unavailable
- Duration: 46s
orchestrator-rollup— gpt-5.6-terra- In: 713.6k
- Out: 3.9k
- Cache read: 653.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 5s
| assert.equal(isCurrentImportPageResponse(2, 2), true) | ||
| }) | ||
|
|
||
| test('Last.fm deferred page selection keeps the newer response', async () => { |
There was a problem hiding this comment.
This is still a unit test for applyCurrentImportPageResponse, not an A→B queue-selection test. It manually supplies generations and callbacks, so it remains green if openQueueItem stops incrementing pageRequestGeneration, fails to use the helper, or wires the response to the wrong page state—the actual user-visible stale-page regression. Add a deterministic test harness for the selection/load controller (or render the importer with mocked deferred lastfm_import_page calls) that selects A then B, resolves B then A, and asserts B stays rendered.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in commit a8081a2b0ef88fa993af4c647bd898bfcc507139. loadSelectedImportPage is now the production loader used by openQueueItem; it owns generation increment, queue selection, page invocation, and current-response application. Replaced the helper-only test with a deterministic A→B deferred queue-selection test: B resolves first, A resolves last, and only B is applied while selection records A then B. npm test passes 43/43, lint exits 0, TypeScript has no errors, and the frontend build passes.
1 participant
Summary
Implements issue #39: a resumable, account-bound native “Last.fm importer” window that snapshots complete Last.fm history page-by-page, matches it to Spotify, and supports durable review before applying content, historical counts, metadata, timestamps, exclusions, fuzzy resolution, and partial/Accept All progress.
Highlights:
user.getRecentTrackssnapshot boundary, sequential checkpointing, compact source variants, retry/resume, account binding, quarantine, atomic persistence, and 100 MB guard.The approved plan was posted to issue #39. Closes #39.
Architecture and safety
apps/desktop/src-tauri/src/lastfm_import.rs.lastfm-import.json, owner-only on Unix, bound to both Last.fm username and Spotify account ID, and excluded from normal backup/restore.Automated evidence
cargo fmt --all -- --check: passed.cargo clippy --workspace --all-targets -- -D warnings: passed.cargo test --workspace: passed; desktop 314 passed/1 ignored, Spotify 55 passed, retune-core 27 passed, audio suites 4 + 6 + 3 passed, doc tests 0.npm test: 42 passed, 0 failed.npm run lint: exit 0; seven existing exhaustive-deps warnings outside the importer.npx tsc -b: passed with no errors.npm run build: passed.git diff --check: passed.Pre-merge empirical gate
No native app was launched and no connected Spotify or Last.fm state was mutated in this phase. Root owns the remaining live gate: exhaustive control-by-control comparison with the always-open Edge design mock; app-data/export backup and restore checks; account and per-URI membership baseline; real single-track and whole-album tests; live Accept All/cancel verification; cleanup and restoration; and confirmation that Last.fm scrobbles were not polluted.