Parallelize Last.fm history imports - #44
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: 4bd016534abd
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 0 |
| frontend:view-state | 1 |
| rust:implementation-tests | 0 |
| tauri:config-ipc | 0 |
| security:credential-boundary | 0 |
frontend:view-state (1 finding)
Major - apps/desktop/test/ui.test.ts:231
This source-text test does not exercise the changed async UI state. In particular, it misses the A→B race where A settles after B:
openQueueItemunconditionally clearspageLoadingin A'sfinally, even though B is still loading. The queue becomes enabled and the UI can replace the matching status with “No review page selected” before B resolves. Add a deterministic deferred-promise regression test that tracks loading through overlapping requests and asserts only the newest request may clear it; then guard the production cleanup by the request generation.
Reviewer Coverage
architecture:seams— complete (broad); inspected 11 assigned files (12 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/lastfmImporter.css,apps/desktop/src/types.ts,docs/architecture/library.md,docs/architecture/persistence.md; skipped: none; constraints: Review was limited to ownership, dependency direction, lifecycle boundaries, provider isolation, persistence seams, and proportionate abstraction; implementation, Tauri IPC, credential, and React behavior defects were out of scope.frontend:view-state— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI test file; production UI was consulted only to assess whether the added regression coverage exercises its changed behavior.rust:implementation-tests— complete (broad); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/src-tauri/src/lastfm_import.rs; skipped: none; constraints: Focused Rust test execution could not be run because the cargo executable is unavailable in this review environment.tauri:config-ipc— complete (constrained); inspected 2 assigned files (12 inspected across reviewers):apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Focused Rust test command could not run because the required RTK proxy could not spawn cargo in this environment. Narrow Tauri IPC/configuration review only; general Rust and frontend-state concerns were out of scope.security:credential-boundary— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):docs/architecture/persistence.md; skipped: none; constraints: Review scope was limited to the assigned changed architecture document; implementation files were not inspected.
Inspected files (12)
ARCHITECTURE.mdapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/lastfmImportState.tsapps/desktop/src/lastfmImporter.cssapps/desktop/src/types.tsapps/desktop/test/ui.test.tsdocs/architecture/library.mddocs/architecture/persistence.md
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 24s | 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, tauri:config-ipc, security:credential-boundary |
| Engine | codex_cli · gpt-5.6-sol, gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 24s wall · 9m 04s compute |
| Cost | unavailable |
| Tokens | 2.5M in / 23.3k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 18.3k
- Out: 462
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
architecture:seams— gpt-5.6-sol- In: 912.7k
- Out: 6.5k
- Cache read: 807.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 57s
frontend:view-state— gpt-5.6-terra- In: 262.2k
- Out: 4.6k
- Cache read: 201.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 33s
rust:implementation-tests— gpt-5.6-terra- In: 521.1k
- Out: 4.8k
- Cache read: 460.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 44s
tauri:config-ipc— gpt-5.6-terra- In: 685.2k
- Out: 5.1k
- Cache read: 596.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 52s
security:credential-boundary— gpt-5.6-terra- In: 107.9k
- Out: 1.2k
- Cache read: 71.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 38s
orchestrator-rollup— gpt-5.6-terra- In: 38.9k
- Out: 557
- Cache read: 28.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
| assert.match(appCss, /\.modal-backdrop \{[^}]*z-index: 10;[^}]*background: rgb\(0 0 0 \/ \.38\)/s) | ||
| }) | ||
|
|
||
| test('Last.fm queue selection keeps refresh stable and controls retain native button semantics', () => { |
There was a problem hiding this comment.
This source-text test does not exercise the changed async UI state. In particular, it misses the A→B race where A settles after B: openQueueItem unconditionally clears pageLoading in A's finally, even though B is still loading. The queue becomes enabled and the UI can replace the matching status with “No review page selected” before B resolves. Add a deterministic deferred-promise regression test that tracks loading through overlapping requests and asserts only the newest request may clear it; then guard the production cleanup by the request generation.
Reply inline to this comment.
There was a problem hiding this comment.
Acknowledged — fixed in commit 91417de.
Root cause: loadSelectedImportPage owned request generations, but LastFmImporter.openQueueItem unconditionally cleared pageLoading in its outer finally. A stale request could therefore clear loading while a newer B request was still pending.
The helper now owns current-generation-only completion in a finally path, including errors. openQueueItem passes setPageLoading(false) to that completion callback, while the existing current-generation error check remains unchanged.
Regression coverage:
- The existing deterministic deferred A→B test now starts A and B, resolves A while B is pending, and asserts loading remains true.
- Resolving B asserts loading becomes false and B remains selected/applied.
- The test was red before the fix and green after it.
Checks:
npm test: 51 passed, 0 failed.npx tsc --noEmit -p tsconfig.app.json: no errors.npm run lint: passed; 7 pre-existing exhaustive-deps warnings.npm run build: passed.git diff --check: passed.
There was a problem hiding this comment.
Thanks—the generation-scoped completion path and deferred A→B regression cover the reported race.
Summary:
Fixed stale-request loading cleanup: only the current request generation may clear page loading, with deterministic overlapping-request regression coverage.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 91417de4189b
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 0 |
| frontend:view-state | 0 |
| rust:implementation-tests | 0 |
| tauri:config-ipc | 0 |
| security:credential-boundary | 0 |
Reviewer Coverage
architecture:seams— complete (constrained); inspected 11 assigned files (12 inspected across reviewers):ARCHITECTURE.md,apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/lastfmImportState.ts,apps/desktop/src/lastfmImporter.css,apps/desktop/src/types.ts,docs/architecture/library.md,docs/architecture/persistence.md; skipped: none; constraints: Review was limited to ownership, dependency direction, lifecycle boundaries, provider isolation, persistence seams, and proportionate abstraction; implementation, Tauri IPC, credential, and React behavior defects were out of scope. Review was read-only; verification relied on source inspection and the supplied automated-check results.frontend:view-state— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow review limited to the assigned UI test file.rust:implementation-tests— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/src-tauri/src/lastfm_import.rs; skipped: none; constraints: nonetauri:config-ipc— complete (constrained); inspected 2 assigned files (12 inspected across reviewers):apps/desktop/src-tauri/src/lastfm_import.rs,apps/desktop/test/ui.test.ts; skipped: none; constraints: Narrow Tauri IPC/configuration review; the follow-up change is confined to frontend test coverage.security:credential-boundary— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):docs/architecture/persistence.md; skipped: none; constraints: Review scope was limited to the assigned changed architecture document; implementation files were not inspected.
Inspected files (12)
ARCHITECTURE.mdapps/desktop/src-tauri/src/lastfm_import.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/lastfmImportState.tsapps/desktop/src/lastfmImporter.cssapps/desktop/src/types.tsapps/desktop/test/ui.test.tsdocs/architecture/library.mddocs/architecture/persistence.md
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 2m 15s | 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, tauri:config-ipc, security:credential-boundary |
| Engine | codex_cli · gpt-5.6-sol, gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 2m 15s wall · 3m 09s compute |
| Cost | unavailable |
| Tokens | 4.3M in / 29.3k out |
Per-workstream usage
architecture:seams— gpt-5.6-sol- In: 1.9M
- Out: 9.6k
- Cache read: 1.7M
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 39s
frontend:view-state— gpt-5.6-terra- In: 510.2k
- Out: 5.9k
- Cache read: 425.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 30s
rust:implementation-tests— gpt-5.6-terra- In: 673.4k
- Out: 5.4k
- Cache read: 595.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 16s
tauri:config-ipc— gpt-5.6-terra- In: 898.5k
- Out: 5.8k
- Cache read: 792.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 20s
security:credential-boundary— gpt-5.6-terra- In: 199.8k
- Out: 1.9k
- Cache read: 148.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 17s
orchestrator-rollup— gpt-5.6-terra- In: 87.4k
- Out: 725
- Cache read: 70.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
1 participant
Summary
Automated checks
cargo fmt --all --check— passed.cargo test -p retune-desktop lastfm_import— 66 passed, 0 failed.cargo clippy --workspace --all-targets -- -D warnings— passed; the existingblock v0.1.6future-incompatibility warning remains.cargo test --workspace— passed; desktop 363 passed with 1 ignored, Spotify 56 passed, and the remaining workspace suites passed.node scripts/check-docs.mjs— passed.npm test— 51 passed.npm run lint— passed with 7 pre-existing exhaustive-deps warnings.npx tsc --noEmit -p tsconfig.app.json— passed.npm run build— passed.git diff --check— passed.Empirical native validation
This PR is intentionally draft for root review; it is not merged or marked ready.