Mike Wichers
e18a1080de
- Full static analysis of all 33 Swift source files - Identified 2 Blockers in IYmtgTests.swift (ScannerViewModel init mismatch, missing property forwards) - Identified 1 Critical issue: IYmtg_Builder_Mac is empty, cards.json cannot be generated - Documented 4 Major issues: deprecated onChange API, missing FirebaseCore import, Firebase delete data leak, dead batchUpdatePrices function - Updated claude_review_summary.md with complete findings by severity - Added Project Audit section to README.md with link to summary Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
224 lines
12 KiB
Markdown
224 lines
12 KiB
Markdown
# Claude Review Summary
|
||
**Review Date:** 2026-03-05 (Revision 2 — Full Compilation Readiness Audit)
|
||
**Blueprint:** `ai_blueprint.md` — Full Project Readiness Audit
|
||
|
||
---
|
||
|
||
## Executive Summary
|
||
|
||
The source code is architecturally complete and well-structured. Two **Blocker** issues will prevent the test target from compiling. One **Critical** issue (missing `cards.json` generator) means the scanner cannot function at all at runtime. Four **Major** issues represent deprecation warnings and a Firebase data-leak bug. The app target itself will compile once Firebase imports are confirmed.
|
||
|
||
---
|
||
|
||
## 1. Dependency Verification
|
||
|
||
### Package Manager
|
||
No `Package.swift`, `Podfile`, or `Cartfile` exists in the repository. Swift Package Manager dependencies are managed through the Xcode `.xcodeproj` file, which lives on the developer's Mac (not in this repo). Dependency versions cannot be verified from source alone.
|
||
|
||
**Required SPM packages (must be added in Xcode):**
|
||
|
||
| Package | Required Modules | Min Version |
|
||
|---|---|---|
|
||
| `firebase-ios-sdk` | FirebaseCore, FirebaseAuth, FirebaseFirestore, FirebaseStorage | ≥ 10.x |
|
||
| SwiftData | Built-in (iOS 17+) | iOS 17 SDK |
|
||
|
||
**Minimum Deployment Target:** iOS 17 is required by `SwiftData`, `@ModelActor`, and `ModelContainer`. This must be set in Xcode → Build Settings.
|
||
|
||
---
|
||
|
||
## 2. File & Resource Integrity
|
||
|
||
### Missing Runtime Files (excluded by `.gitignore` — must be added before shipping)
|
||
|
||
| File | Referenced In | Status |
|
||
|---|---|---|
|
||
| `cards.json` | `ScannerViewModel.swift:96` | ❌ **CRITICAL** — Generator (`IYmtg_Builder_Mac/`) is empty |
|
||
| `GoogleService-Info.plist` | `IYmtgApp.swift:9` | ⚠️ Expected missing — handled gracefully with local-mode fallback |
|
||
| `IYmtgFoilClassifier.mlmodel` | `FoilEngine.swift:12` | ⚠️ Expected missing — app degrades gracefully (foil = "None") |
|
||
| `IYmtgConditionClassifier.mlmodel` | `ConditionEngine.swift:17` | ⚠️ Expected missing — app degrades gracefully (grade = "Ungraded") |
|
||
| `IYmtgStampClassifier.mlmodel` | `StampDetector.swift:8` | ⚠️ Expected missing — app degrades gracefully |
|
||
| `IYmtgSetClassifier.mlmodel` | `SetSymbolEngine.swift:8` | ⚠️ Expected missing — app degrades gracefully |
|
||
|
||
### Missing Image Assets (must exist in `Assets.xcassets` inside Xcode project)
|
||
|
||
| Asset Name | Referenced In | Notes |
|
||
|---|---|---|
|
||
| `scanner_frame` | `ContentView.swift:114` | Required — scanner overlay frame |
|
||
| `logo_header` | `ContentView.swift:147` | Required — header logo |
|
||
| `empty_library` | `ContentView.swift:230` | Required — empty state illustration |
|
||
| `share_watermark` | `ContentView.swift:334` | Optional — has fallback text if missing |
|
||
|
||
### Placeholder / Orphan Files
|
||
|
||
| File | Issue |
|
||
|---|---|
|
||
| `Data/Persistence/PersistenceActor.swift` | Empty placeholder — must be **removed from Xcode project navigator** (note inside the file confirms this) |
|
||
| `Features/CardDetail/` | Directory is empty — `CardDetailView` is defined in `ContentView.swift` instead |
|
||
|
||
---
|
||
|
||
## 3. Static Code Analysis
|
||
|
||
### BLOCKER Issues (prevent compilation)
|
||
|
||
#### B1 — `IYmtgTests.swift:113` — `ScannerViewModel()` no-arg init does not exist
|
||
**Severity:** Blocker | **File:** `IYmtg_App_iOS/IYmtgTests.swift` | **Lines:** 113, 137
|
||
|
||
`ScannerViewModel` only exposes `init(collectionVM: CollectionViewModel)`. The test file calls `ScannerViewModel()` with no arguments in two test functions (`testViewModelFiltering` and `testPortfolioCalculation`). The Swift compiler will reject this.
|
||
|
||
```swift
|
||
// Current (BROKEN):
|
||
let vm = ScannerViewModel()
|
||
|
||
// Fix:
|
||
let colVM = CollectionViewModel()
|
||
let vm = ScannerViewModel(collectionVM: colVM)
|
||
```
|
||
|
||
#### B2 — `IYmtgTests.swift:118-148` — Accessing non-existent properties on `ScannerViewModel`
|
||
**Severity:** Blocker | **File:** `IYmtg_App_iOS/IYmtgTests.swift` | **Lines:** 118, 121, 126, 137, 143
|
||
|
||
The test directly assigns/reads `vm.scannedList`, `vm.librarySearchText`, `vm.filteredList`, and `vm.portfolioValue` on a `ScannerViewModel` instance. None of these are forwarded computed properties on `ScannerViewModel` — they belong to `CollectionViewModel`. The compiler will produce unresolved identifier errors.
|
||
|
||
**Fix:** Obtain these properties from `vm.collectionVM` instead:
|
||
```swift
|
||
vm.collectionVM.scannedList = [card1, card2]
|
||
vm.collectionVM.librarySearchText = "Alpha"
|
||
// etc.
|
||
```
|
||
|
||
---
|
||
|
||
### CRITICAL Issues (app non-functional at runtime)
|
||
|
||
#### C1 — `cards.json` generator is missing (`IYmtg_Builder_Mac/` is empty)
|
||
**Severity:** Critical | **File:** `IYmtg_Builder_Mac/` (directory) | **Line:** N/A
|
||
|
||
`ScannerViewModel.init` attempts to load `cards.json` from the app bundle. If absent, the app immediately shows a "Database Missing" alert and the scanner is permanently non-functional. The `IYmtg_Builder_Mac/` directory — which should contain the Swift CLI tool that generates this file — is completely empty. This is the single most important missing piece before any developer can test the scanner.
|
||
|
||
---
|
||
|
||
### MAJOR Issues (warnings or significant functional gaps)
|
||
|
||
#### M1 — Deprecated `.onChange(of:)` API (iOS 17 deprecation warnings)
|
||
**Severity:** Major | **File:** `IYmtg_App_iOS/ContentView.swift` | **Lines:** 172, 237
|
||
|
||
The single-parameter closure form of `.onChange(of:)` was deprecated in iOS 17. Since the app targets iOS 17+, Xcode will emit deprecation warnings on every build.
|
||
|
||
```swift
|
||
// Line 172 — DEPRECATED:
|
||
.onChange(of: scenePhase) { newPhase in ... }
|
||
|
||
// Fix:
|
||
.onChange(of: scenePhase) { _, newPhase in ... }
|
||
|
||
// Line 237 — DEPRECATED:
|
||
.onChange(of: vm.selectedCurrency) { _ in vm.refreshPrices(force: true) }
|
||
|
||
// Fix:
|
||
.onChange(of: vm.selectedCurrency) { _, _ in vm.refreshPrices(force: true) }
|
||
```
|
||
|
||
#### M2 — `ModelManager.swift` missing `import FirebaseCore`
|
||
**Severity:** Major | **File:** `IYmtg_App_iOS/Services/CoreML/ModelManager.swift` | **Lines:** 1–3, 35
|
||
|
||
`ModelManager.swift` uses `FirebaseApp.app()` (a `FirebaseCore` type) but only imports `FirebaseStorage`. In Swift, each file must explicitly import the module it uses. This may compile in some Xcode/SPM configurations where `FirebaseStorage` transitively links `FirebaseCore`, but it is not guaranteed and should be made explicit.
|
||
|
||
```swift
|
||
// Add to ModelManager.swift imports:
|
||
import FirebaseCore
|
||
```
|
||
|
||
#### M3 — `CloudEngine.delete(card:)` never called — Firebase data leak on card deletion
|
||
**Severity:** Major | **File:** `IYmtg_App_iOS/Features/Collection/CollectionViewModel.swift` | **Line:** 271–278
|
||
|
||
`CollectionViewModel.deleteCard(_:)` removes the card from SwiftData (via `saveCollectionAsync()`) and deletes the local image, but never calls `CloudEngine.delete(card:)`. Any cards that were previously backed up to Firebase Firestore will remain in the user's Firestore inventory forever after deletion. The method to fix this already exists — it just needs to be called.
|
||
|
||
```swift
|
||
// In CollectionViewModel.deleteCard(_:), after removing from scannedList:
|
||
Task { await CloudEngine.delete(card: card) }
|
||
```
|
||
|
||
#### M4 — `CloudEngine.batchUpdatePrices()` is dead code
|
||
**Severity:** Major | **File:** `IYmtg_App_iOS/Services/Cloud/CloudEngine.swift` | **Lines:** 49–69
|
||
|
||
`CloudEngine.batchUpdatePrices(cards:)` is fully implemented but never called anywhere in the codebase. Price updates are reflected in SwiftData + CloudKit automatically, making this function either redundant or an incomplete feature. It should either be wired into the price-refresh flow or removed.
|
||
|
||
---
|
||
|
||
### MINOR Issues (low impact, housekeeping)
|
||
|
||
#### m1 — `Features/CardDetail/` directory is empty
|
||
**Severity:** Minor | **File:** `IYmtg_App_iOS/Features/CardDetail/` (directory)
|
||
|
||
`CardDetailView`, `DashboardView`, `ScannerView`, `WelcomeView`, and `FeatureRow` are all defined in `ContentView.swift`, making it a 469-line monolith. The `CardDetail/` directory exists but holds no files. This violates the project's modular structure intent but does not affect compilation.
|
||
|
||
#### m2 — `PersistenceActor.swift` placeholder in Xcode project
|
||
**Severity:** Minor | **File:** `IYmtg_App_iOS/Data/Persistence/PersistenceActor.swift`
|
||
|
||
This file contains only a migration comment and no executable code. It must be manually removed from the Xcode project navigator to avoid confusion (the file itself documents this).
|
||
|
||
#### m3 — `AppConfig.swift` not configured for production
|
||
**Severity:** Minor | **File:** `IYmtg_App_iOS/AppConfig.swift` | **Lines:** 24, 27
|
||
|
||
| Setting | Current Value | Required |
|
||
|---|---|---|
|
||
| `contactEmail` | `"support@iymtg.com"` | Real developer email (Scryfall policy) |
|
||
| `tipJarProductIDs` | `[]` | Real App Store Connect product IDs |
|
||
| `buildNumber` | `"2"` | Increment with each App Store submission |
|
||
|
||
The `validate()` function checks for `"yourdomain.com"` but not for `"iymtg.com"`, so the fatalError guard will not fire in DEBUG with the current placeholder.
|
||
|
||
#### m4 — OTA model updates require app restart (undocumented)
|
||
**Severity:** Minor | **File:** `IYmtg_App_iOS/Services/CoreML/ModelManager.swift` | **Line:** 16
|
||
|
||
`FoilEngine.model`, `ConditionEngine.model`, `StampDetector.model`, and `SetSymbolEngine.model` are all `static var` properties evaluated once at class-load time. When `ModelManager.checkForUpdates()` downloads a new `.mlmodelc` to Documents, the running app will not use it until restarted. This is expected behavior but should be documented in the README or in-app release notes.
|
||
|
||
---
|
||
|
||
## 4. Feature Completeness Review
|
||
|
||
| Feature | Status | Notes |
|
||
|---|---|---|
|
||
| Card Scanner (camera, frame capture) | ✅ Complete | `ScannerViewModel` — full pipeline |
|
||
| Card Identification (fingerprint + OCR + heuristics) | ✅ Complete | `AnalysisActor`, `FeatureMatcher`, `OCREngine`, heuristics |
|
||
| Foil Detection | ✅ Complete | `FoilEngine` — requires trained model |
|
||
| Condition Grading | ✅ Complete | `ConditionEngine` — requires trained model |
|
||
| Stamp Detection | ✅ Complete | `StampDetector` — requires trained model |
|
||
| Set Symbol Detection | ✅ Complete | `SetSymbolEngine` — requires trained model |
|
||
| Collection Management | ✅ Complete | `CollectionViewModel` |
|
||
| SwiftData + CloudKit Persistence | ✅ Complete | `PersistenceController`, `BackgroundPersistenceActor` |
|
||
| JSON → SwiftData Migration | ✅ Complete | `migrateFromJSONIfNeeded()` |
|
||
| Scryfall Price Refresh | ✅ Complete | `ScryfallAPI.updateTrends()` |
|
||
| PDF Insurance Export | ✅ Complete | `ExportEngine.generatePDF()` |
|
||
| CSV / Arena / MTGO Export | ✅ Complete | `ExportEngine` |
|
||
| Firebase Backup (manual) | ✅ Complete | `CloudEngine` + `CollectionViewModel.backupAllToFirebase()` |
|
||
| OTA Model Updates | ✅ Complete | `ModelManager.checkForUpdates()` |
|
||
| Training Image Upload | ✅ Complete | `TrainingUploader` |
|
||
| In-App Purchase (Tip Jar) | ✅ Complete | `StoreEngine` — IAP IDs not configured |
|
||
| App Review Prompt | ✅ Complete | `ReviewEngine` |
|
||
| Training Guide (in-app) | ✅ Complete | `TrainingGuideView` |
|
||
| Welcome Screen | ✅ Complete | `WelcomeView` |
|
||
| Dashboard (Portfolio stats) | ✅ Complete | `DashboardView` |
|
||
| **IYmtg_Builder_Mac (cards.json generator)** | ❌ **MISSING** | Directory exists but is completely empty |
|
||
| Unit Tests | ⚠️ Present but broken | 2 Blocker compilation errors (see B1, B2) |
|
||
|
||
---
|
||
|
||
## 5. Summary Scorecard
|
||
|
||
| Category | Status | Priority |
|
||
|---|---|---|
|
||
| Test target compilation | ❌ 2 Blockers | **Blocker** — fix before any CI |
|
||
| Runtime: `cards.json` missing | ❌ Scanner non-functional | **Critical** |
|
||
| Deprecated API warnings | ⚠️ 2 occurrences | **Major** |
|
||
| Firebase import missing | ⚠️ Potential compile error | **Major** |
|
||
| Firebase delete data leak | ⚠️ Silent bug | **Major** |
|
||
| Dead code (`batchUpdatePrices`) | ⚠️ Minor bloat | **Major** |
|
||
| `CardDetail/` directory empty | ℹ️ Structural inconsistency | Minor |
|
||
| `PersistenceActor.swift` placeholder | ℹ️ Xcode cleanup needed | Minor |
|
||
| AppConfig production values | ℹ️ Not release-ready | Minor |
|
||
| OTA restart undocumented | ℹ️ Developer note needed | Minor |
|
||
| SPM deps (no manifest in repo) | ℹ️ Expected (Xcode-managed) | Informational |
|
||
| App source code (all other files) | ✅ Compiles, well-structured | — |
|