diff --git a/JanitorialQC.xcodeproj/project.pbxproj b/JanitorialQC.xcodeproj/project.pbxproj index fa0d967..6e96f10 100644 --- a/JanitorialQC.xcodeproj/project.pbxproj +++ b/JanitorialQC.xcodeproj/project.pbxproj @@ -431,7 +431,7 @@ "$(inherited)", "@executable_path/Frameworks", ); - MARKETING_VERSION = 1.9; + MARKETING_VERSION = 1.11; PRODUCT_BUNDLE_IDENTIFIER = com.ltservicesinc.JanitorialQC; PRODUCT_NAME = "$(TARGET_NAME)"; STRING_CATALOG_GENERATE_SYMBOLS = YES; @@ -474,7 +474,7 @@ "$(inherited)", "@executable_path/Frameworks", ); - MARKETING_VERSION = 1.9; + MARKETING_VERSION = 1.11; PRODUCT_BUNDLE_IDENTIFIER = com.ltservicesinc.JanitorialQC; PRODUCT_NAME = "$(TARGET_NAME)"; STRING_CATALOG_GENERATE_SYMBOLS = YES; diff --git a/JanitorialQC/API/APIClient.swift b/JanitorialQC/API/APIClient.swift index c0807b9..2089ec4 100644 --- a/JanitorialQC/API/APIClient.swift +++ b/JanitorialQC/API/APIClient.swift @@ -38,8 +38,35 @@ private struct _Envelope: Decodable, Sendable { private enum CodingKeys: String, CodingKey { case ok, data, error } } -// Free function removed — see refreshAccessToken() which decodes using a -// local JSONDecoder to avoid Swift 6 actor-isolation errors. +// Envelope header only — `ok` and `error`, never the payload. +// +// Split from _Envelope so `decode()` can tell "the server reported a failure" +// apart from "the server succeeded but we could not read the payload". Those +// were indistinguishable while `data` was decoded with `try?`: any schema +// mismatch produced data == nil and surfaced as serverError("Unknown server +// error"), pointing every investigation at the backend. +private struct _EnvelopeMeta: Decodable, Sendable { + let ok: Bool + let error: String? + + nonisolated init(from decoder: any Decoder) throws { + let c = try decoder.container(keyedBy: CodingKeys.self) + ok = try c.decode(Bool.self, forKey: .ok) + error = try? c.decode(String.self, forKey: .error) + } + private enum CodingKeys: String, CodingKey { case ok, error } +} + +// Payload only, decoded STRICTLY so the failure reason propagates. +private struct _EnvelopePayload: Decodable, Sendable { + let data: T + + nonisolated init(from decoder: any Decoder) throws { + let c = try decoder.container(keyedBy: CodingKeys.self) + data = try c.decode(T.self, forKey: .data) // deliberately not `try?` + } + private enum CodingKeys: String, CodingKey { case data } +} // Refresh-only envelope — Sendable so it can cross actor boundaries in Swift 6. // nonisolated init(from:) required on both types: without it the Swift 6 compiler @@ -279,20 +306,41 @@ actor APIClient { // inspection synced in the same triggerSync() pass. if let id = inspectionServerId { body["inspection_id"] = id } if let areaId = issue.areaServerId { body["area_id"] = areaId } - // photo_path = primary photo. Additional photos are sent via a - // separate PATCH call in processIssueQueue after the issue is created, - // because the server create endpoint only stores a single photo_path. + // All evidence photos go in this ONE request. + // + // `photo_path` is the primary; `result_photos` carries the rest and is + // stored server-side in `mobile_photo_paths`, so they display under + // "Photo Evidence" rather than "Resolution Details" + // (app/api/issues.py, create_issue). + // + // These used to be split: create sent photo_path only, then + // processIssueQueue fired a follow-up PATCH for the extras. The extras + // were always known before create — processPhotoQueue fully populates + // photoServerPaths first — so the second call bought nothing and cost a + // window in which the issue was already `synced` while its photos were + // not attached. Sending them together makes attachment atomic with + // creation, and the endpoint's mobile_local_id idempotency covers a + // retry of the whole thing (rule 85). if let first = issue.photoServerPaths.first { body["photo_path"] = first } + let extraPhotos = Array(issue.photoServerPaths.dropFirst()) + if !extraPhotos.isEmpty { body["result_photos"] = extraPhotos } struct R: Decodable, Sendable { let issueId: Int; let duplicate: Bool } let r: R = try await post("/api/v1/issues", body: body) return r.issueId } - // ── Attach additional photos to an existing issue ───────────────────── - // Called after submitIssue when the issue has more than one photo. - // PATCHes /api/v1/issues/{id}/photos with result_photos = [server paths beyond the first]. - // The create endpoint only stores photo_path (single); extras go here. + // ── Attach a LATE evidence photo to an already-created issue ────────── + // + // NOT part of the normal path: submitIssue() sends every evidence photo in + // the create request, and reintroducing a routine post-create call is + // exactly what rule 85 forbids. This exists only for recovery — a photo + // that exhausted its upload attempts, was submitted without, and later + // succeeded via Pending Sync → Retry Failed Items. By then the create + // request is long gone and this is the only way across. + // + // Server-side (app/api/issues.py, update_issue_photos) this merges + // idempotently into mobile_photo_paths, so repeating a path is a no-op. func updateIssuePhotos(issueId: Int, resultPhotos: [String]) async throws { struct R: Decodable, Sendable { let issueId: Int; let resultPhotosCount: Int } let _: R = try await request( @@ -592,7 +640,32 @@ actor APIClient { // ── Token Refresh ───────────────────────────────────────────────────── + /// The refresh currently in flight, if any. + /// + /// `APIClient` being an actor is NOT enough on its own: `refreshAccessToken` + /// suspends at `await`, which releases the actor and lets a second caller + /// enter. Two requests 401-ing at once would then each POST /auth/refresh + /// with the SAME refresh token — the server rotates it on the first, so the + /// second presents an already-spent token, fails, and the user is signed + /// out mid-sync. Easy to hit: pollNotifications and registerDevice both run + /// alongside triggerSync. + /// + /// Coalescing here means concurrent callers await one shared result. The + /// check-and-store below spans no `await`, so it is atomic within the actor. + private var refreshTask: Task? + private func refreshAccessToken() async -> Bool { + if let inFlight = refreshTask { + return await inFlight.value + } + let task = Task { await self.performTokenRefresh() } + refreshTask = task + let result = await task.value + refreshTask = nil + return result + } + + private func performTokenRefresh() async -> Bool { guard let token = KeychainHelper.get(Constants.Keychain.refreshToken), let url = URL(string: ServerConfig.current + "/api/v1/auth/refresh") else { return false } @@ -657,14 +730,50 @@ actor APIClient { } private func decode(_ data: Data) throws -> T { - if let env = try? decoder.decode(_Envelope.self, from: data) { - if env.ok, let result = env.data { return result } - throw APIError.serverError(env.error ?? "Unknown server error") + // Read the envelope header first, so a server-reported failure and an + // unreadable payload cannot be confused for one another. + if let meta = try? decoder.decode(_EnvelopeMeta.self, from: data) { + guard meta.ok else { + throw APIError.serverError(meta.error ?? "Unknown server error") + } + do { + return try decoder.decode(_EnvelopePayload.self, from: data).data + } catch { + // ok == true, so this is OUR problem, not the server's — a + // contract drift between this build and the deployment. + throw APIError.decodingError(Self.describe(error, as: T.self)) + } } + // Not an envelope — a few endpoints return the object bare. do { return try decoder.decode(T.self, from: data) } catch { - throw APIError.decodingError(error.localizedDescription) + throw APIError.decodingError(Self.describe(error, as: T.self)) + } + } + + /// Turn a `DecodingError` into something that names the offending field. + /// + /// `error.localizedDescription` on a DecodingError is always the useless + /// "The data couldn't be read because it isn't in the correct format", + /// which is what the old path surfaced — so a renamed or retyped API field + /// gave no clue which one it was. + private static func describe(_ error: Error, as type: T.Type) -> String { + func path(_ context: DecodingError.Context) -> String { + let keys = context.codingPath.map(\.stringValue).filter { !$0.isEmpty } + return keys.isEmpty ? "\(type)" : "\(type).\(keys.joined(separator: "."))" + } + switch error as? DecodingError { + case .keyNotFound(let key, let ctx): + return "missing field '\(key.stringValue)' in \(path(ctx))" + case .typeMismatch(let expected, let ctx): + return "\(path(ctx)) has the wrong type (expected \(expected))" + case .valueNotFound(let expected, let ctx): + return "\(path(ctx)) was null (expected \(expected))" + case .dataCorrupted(let ctx): + return "\(path(ctx)) is malformed: \(ctx.debugDescription)" + default: + return "could not read \(type): \(error.localizedDescription)" } } } diff --git a/JanitorialQC/Auth/AuthManager.swift b/JanitorialQC/Auth/AuthManager.swift index 4fc25d8..60eb0bf 100644 --- a/JanitorialQC/Auth/AuthManager.swift +++ b/JanitorialQC/Auth/AuthManager.swift @@ -34,6 +34,7 @@ class AuthManager: ObservableObject { do { let response: MeResponseData = try await APIClient.shared.request("/api/v1/auth/me") applyUser(response.user) + reconcileSessionScope(userId: response.user.id) isAuthenticated = true } catch APIError.notAuthenticated { KeychainHelper.clearAll() @@ -57,6 +58,9 @@ class AuthManager: ObservableObject { KeychainHelper.set(response.accessToken, forKey: Constants.Keychain.accessToken) KeychainHelper.set(response.refreshToken, forKey: Constants.Keychain.refreshToken) applyUser(response.user) + // BEFORE isAuthenticated flips, so DashboardView is never rendered + // holding the previous inspector's data. + reconcileSessionScope(userId: response.user.id) isAuthenticated = true // Register device immediately after login — the .task {} and // .onChange(scenePhase) paths both miss this case because they run @@ -97,6 +101,39 @@ class AuthManager: ObservableObject { KeychainHelper.set(user.displayName, forKey: Constants.Keychain.displayName) } + /// Purge local data when this session is scoped to a different + /// `(server, user)` pair than the database currently holds. + /// + /// This is the check that was missing: logout deliberately keeps the cache + /// so the same inspector can work offline after signing back in, but + /// nothing verified that the next sign-in *was* the same inspector. A + /// different one inherited their issues; see `SessionScope` and rule 88. + /// + /// Runs on both `login()` and `restoreSession()` — a session can also be + /// restored into a changed scope after a server switch. + private func reconcileSessionScope(userId: Int) { + let server = ServerConfig.current + let stored = SessionScope.stored + + // No marker: either a genuinely fresh install, or an upgrade from a + // build that predates this. Those are indistinguishable, and purging on + // the guess would delete an in-progress draft belonging to the user + // signing in right now — so adopt the existing data and start tracking + // from here. Every subsequent identity change is then covered. + guard let stored else { + SessionScope.record(userId: userId) + return + } + + guard stored.server != server || stored.userId != userId else { return } + + SyncManager.shared.purgeSessionScopedData( + keepingUserId: userId, + sameServer: stored.server == server + ) + SessionScope.record(userId: userId) + } + private func restoreUserFromKeychain() { currentUserId = Int(KeychainHelper.get(Constants.Keychain.userId) ?? "0") ?? 0 currentUserRole = KeychainHelper.get(Constants.Keychain.userRole) ?? "" diff --git a/JanitorialQC/CLAUDE.md b/JanitorialQC/CLAUDE.md index 323d3b6..56d1cc8 100644 --- a/JanitorialQC/CLAUDE.md +++ b/JanitorialQC/CLAUDE.md @@ -89,7 +89,7 @@ JanitorialQC/ │ ├── API/ │ ├── APIClient.swift # actor — URLSession, JWT inject, 401-retry, photo upload -│ │ # updateIssuePhotos() — PATCH /issues//photos +│ │ # submitIssue() sends photo_path + result_photos (rule 85) │ └── APIModels.swift # All Codable/Sendable response DTOs │ # APIAssignedIssue has photoPath + mobilePhotoPaths + resultPhotos │ @@ -197,7 +197,7 @@ PendingPhoto.self, SyncQueueEntry.self | `LocalIssue` | Issue record | `localId` (UUID, unique), `serverId`, `inspectionLocalId` (`""` for standalone/server-pulled), `facilityServerId`, `severity`, `syncStatus`, `photoLocalPathsJSON`, `photoServerPathsJSON`, **handler fields** (`handlerType`, `handlerLabel`, `facilityHandler*`, `vendor*` — all optional, synced from server, inspector-editable) | | `LocalScheduledInspection` | Read-only cached scheduled/recurring assignment (phase36) | `serverId` (`@Attribute(.unique)`, **no default** — rule 63), `facilityServerId`, `facilityName`, `templateServerId`, `templateName`, `inspectorId`, `frequency`, `frequencyLabel`, `dueDateString` (sort key), `isOverdue`, `nextDue` (computed), `parentInspectionServerId` (`Int?`, phase45 — set when the schedule is a planned follow-up; becomes the run's `parentServerId`). Pulled by `pullScheduledInspections()`; `init(from:)`/`update(from:)` like `LocalFacility` | | `LocalFollowUpRequest` | Read-only cached follow-up request raised on the web (July 2026) | `serverId` (`@Attribute(.unique)`, **no default** — rule 63; this is the *flagged parent* inspection's id and the `parentServerId` the re-inspection links to), `facilityServerId`, `facilityName`, `templateServerId`, `templateName`, `overallScore`, `inspectionDateString` (sort key), `followUpNote`, `note` (computed, trimmed/nil-ed), `inspectedOn` (computed, parses the `yyyy-MM-dd` prefix only — see the file comment), `fulfilledLocally` (`= false`, rule 71), `parentFormDataJSON` (`= "{}"`, the parent's answers cached for re-inspection prefill — rule 79), `parentFormData` (computed). Pulled by `pullFollowUpRequests()` | -| `PendingPhoto` | Photo awaiting upload | `localId`, `localFilePath`, `serverPath`, `uploadStatus`, `entityType` (`"issue"` or `"inspection"`), `fieldId` | +| `PendingPhoto` | Photo awaiting upload | `localId`, `localFilePath`, `serverPath`, `uploadStatus`, `uploadRetryCount` (`Int = 0`, rule 83 — the row stays `"pending"` until it hits 5), `entityType` (`"issue"` or `"inspection"`), `fieldId` | | `SyncQueueEntry` | Outbox entry (informational) | `entityType`, `localId`, `syncStatus`, `payloadJSON` | ### LocalInspection Status Flow @@ -247,7 +247,7 @@ guard isOnline, let context = modelContext, AuthManager.shared.isAuthenticated e 2. **`processInspectionQueue`** — submits completed inspections when all `pendingPhotos` are settled. -3. **`processIssueQueue`** — guards against submitting when parent inspection `syncStatus == "failed"`. After successful submit: sets `syncStatus = "synced"`, **clears `photoLocalPaths = []`** (prevents duplicate photo sections in `IssueDetailView`), then calls `updateIssuePhotos(issueId:resultPhotos:)` for any extra photos beyond the first (`Array(photoServerPaths.dropFirst())`). +3. **`processIssueQueue`** — guards against submitting when parent inspection `syncStatus == "failed"`, **and waits for the issue's own photos to settle** (same rule as inspections — see rule 83). `submitIssue()` sends every evidence photo in the one create request (rule 85). After successful submit: sets `syncStatus = "synced"` and clears `photoLocalPaths = []` **only when every photo uploaded** (rule 84). 4. **`pullReferenceData`** — fetches facilities, areas, templates. **Deduplicates facility response by `id` using `seenFacilityIds = Set()`** before upserting — prevents duplicate buildings in pickers when server returns same facility ID multiple times. @@ -269,13 +269,31 @@ guard isOnline, let context = modelContext, AuthManager.shared.isAuthenticated e Records inserted by `pullAssignedIssues` are identified by: `syncStatus == "synced"` AND `inspectionLocalId == ""`. These are the only records safe to delete during reconciliation. -### clearServerPulledData() — on logout / server switch +### purgeSessionScopedData() — on identity change -Deletes every `LocalIssue` where `serverId != nil`. This covers: -- Server-pulled assigned issues (`inspectionLocalId == ""`, `syncStatus == "synced"`) -- Inspector-created issues that already synced (`inspectionLocalId != ""`, `serverId != nil`) +Replaces `clearServerPulledData()`, which deleted `LocalIssue` only and was called +from the wrong place. See rule 88. -Preserves only truly pending device-created issues (`serverId == nil`, `syncStatus == "pending"`). +**Trigger is a change of `SessionScope` — the `(server, userId)` pair the database is +scoped to — not logout.** `AuthManager.reconcileSessionScope()` compares the incoming +session against the recorded scope on every `login()` and `restoreSession()`, and purges +only when they differ. + +| Model | Kept | +|---|---| +| `LocalFacility` / `LocalArea` / `LocalTemplate` / `LocalScheduledInspection` / `LocalFollowUpRequest` | Nothing — pure caches, re-pulled on the next sync | +| `LocalIssue` | Nothing — the model has no author field, so an unsent issue cannot be attributed and must not be submitted under a different inspector's name | +| `LocalInspection` | Only rows whose `inspectorUserId` matches the incoming user, **and** only when the server is unchanged | +| `PendingPhoto` | Only rows belonging to a kept inspection; the JPEGs of the rest are deleted from disk too | + +A plain logout still purges nothing: the same inspector signing back into the same server +keeps their cache and stays usable offline. That was always the right call — the defect was +that nothing checked whether the next sign-in was the same person. + +**`SessionScope.stored == nil` adopts the existing data rather than purging.** A fresh +install and an upgrade from a build without the marker are indistinguishable, and guessing +"purge" would delete an in-progress draft belonging to the person signing in right then. +Every identity change after that first login is covered. ### Notification polling @@ -324,16 +342,20 @@ All server URLs built as: `ServerConfig.current + endpoint` — **`Constants.bas `decoder.keyDecodingStrategy = .convertFromSnakeCase` — snake_case server fields map to camelCase automatically. Server POST body keys are snake_case (`photo_path`, `result_photos`, `facility_id`, etc.). +**`decode()` reads the envelope header before the payload.** `_EnvelopeMeta` (`ok` + `error`) is decoded first; only if `ok` is true is the payload decoded **strictly** via `_EnvelopePayload`. This separates "the server reported a failure" from "the server succeeded and we could not read it" — previously `data` was decoded with `try?`, so *any* schema drift produced `data == nil` and surfaced as `serverError("Unknown server error")`, sending every investigation to the backend for what was a client-side contract mismatch. Failures now report the offending field (`missing field 'x' in APIFoo.bar`) via `describe(_:as:)`, because `DecodingError.localizedDescription` is always the useless "data couldn't be read" string. + +**Token refresh is coalesced (`refreshTask`).** Being an `actor` is not sufficient: `refreshAccessToken()` suspends at `await`, releasing the actor, so two requests 401-ing at once each POSTed `/auth/refresh` with the *same* refresh token. The server rotates on the first, so the second presented a spent token, failed, and signed the user out mid-sync — reachable because `pollNotifications` and `registerDevice` run alongside `triggerSync`. Concurrent callers now await one shared `Task`. + ### Key methods | Method | Endpoint | Notes | |---|---|---| -| `request` | Any | Generic; 401 auto-refresh once | +| `request` | Any | Generic; 401 auto-refresh once (refresh is coalesced — see above) | | `post` | Any | POST convenience | | `uploadPhoto` | `POST /api/v1/photos/upload` | Multipart form-data; `entity_type="issue"` → `uploads/issue_photos/` | | `submitInspection` | `POST /api/v1/inspections` | Sanitises `local://` paths | | `submitIssue` | `POST /api/v1/issues` | Sends `photo_path` = first server photo only | -| `updateIssuePhotos` | `PATCH /api/v1/issues//photos` | Sends `{ "result_photos": [extra paths] }`; stored server-side in `mobile_photo_paths` | +| `updateIssuePhotos` | `PATCH /api/v1/issues//photos` | **Recovery only** — called from `pushLateIssuePhotoIfNeeded()` for a photo that succeeded *after* its issue was already created. The normal path sends every evidence photo inside `submitIssue`'s create request (rule 85); do not call this from it. Merges idempotently into `mobile_photo_paths`. | | `fetchAssignedIssues` | `GET /api/v1/issues` | Returns issues assigned to OR reported by current user | | `fetchIssueDetail` | `GET /api/v1/issues/` | Fetches current status | | `updateIssueStatus` | `PATCH /api/v1/issues//status` | Inspector updates status | @@ -435,6 +457,8 @@ Flagging that row at submit time exposed a second problem: `ScheduledInspections All start flows are full-screen for consistency (rule 66): draft-resume (dashboard) → `.fullScreenCover` → `ExecuteInspectionView(isModallyPresented: true)` with a leading `Close`; scheduled / new (`+`) / re-inspection → `.fullScreenCover` → `StartInspectionView` (its own Cancel). Pushed presentations (My Inspections row → `ExecuteInspectionView`) keep `isModallyPresented = false` and rely on the nav back button. +**Leaving after submit — `ExecuteInspectionView.onFinished`.** `StartInspectionView` *pushes* the form onto the NavigationStack inside its own cover, so `dismiss()` there only pops: the inspector finished an inspection and landed back on the "New Inspection" form that started it, with Cancel as the only way out. `StartInspectionView` passes its own dismiss as `onFinished` so the whole cover closes. Left nil everywhere popping is correct — the My Inspections row (pushed onto the list's stack) and the dashboard Resume banner (this view *is* the cover root). + ### "Handled By" (issue handler, phase35 → mobile July 2026) `IssueDetailView` (`IssuesView.swift`) shows a "Handled By" section: current handler label + detail, and — for admin/director/PM **and the assigned inspector** — an inline editor (segmented internal/facility/vendor + name/contact/notes) that PATCHes via `updateIssueHandler` and mirrors the result onto `LocalIssue`. The inspector-writable path is a deliberate divergence from the web form (web CLAUDE.md rule 78). @@ -539,24 +563,38 @@ The **Follow-up Requested** card / section (July 2026) is the second trigger, an ### PendingPhoto lifecycle ``` -Created (uploadStatus="pending") +Created (uploadStatus="pending", uploadRetryCount=0) ↓ SyncManager.processPhotoQueue() -Uploaded (uploadStatus="uploaded", serverPath set) - ↓ Parent record updated - Inspection image fields: LocalInspection.formData[fieldId] = serverPath - Issues: LocalIssue.photoServerPaths.append(serverPath) + ├─ success → uploadStatus="uploaded", serverPath set + │ ↓ Parent record updated (attachServerPath) + │ Inspection image fields: LocalInspection.formData[fieldId] = serverPath + │ Issues: LocalIssue.photoServerPaths.append(serverPath) + └─ error → uploadRetryCount += 1, STAYS "pending" (retried next sync) + └─ only at maxPhotoUploadAttempts (5) → uploadStatus="failed" ``` +**`"failed"` is terminal and means "every attempt was used", not "one error happened"** — see rule 83. `SyncStatusView` → Retry Failed Items resets these back to `"pending"`; it is the only thing that does. + ### Multi-photo issue submission sequence ``` 1. processPhotoQueue: uploads all N photos → appends each serverPath to issue.photoServerPaths -2. processIssueQueue: submitIssue(issue) → sends photo_path = photoServerPaths[0] - issue.photoLocalPaths = [] (clear local paths — prevents duplicate sections) - updateIssuePhotos(issueId, photoServerPaths.dropFirst()) - → PATCH /issues//photos with extras + (a shared local file is uploaded ONCE; every PendingPhoto row + pointing at it gets the same serverPath — rule 86) +2. processIssueQueue: waits until all N photos are "uploaded" or "failed" ← rule 83 + submitIssue(issue) → photo_path = photoServerPaths[0] + result_photos = the rest ← rule 85 + (server stores these in mobile_photo_paths) + issue.photoLocalPaths = [] ONLY if all N uploaded ← rule 84 ``` +**One request, not two.** There is no post-create PATCH on the normal path — see rule 85. +Two related calls are NOT exceptions to that: +- `pushLateIssuePhotoIfNeeded()` fires only when `issue.serverId` is already set, i.e. a + photo recovered after the issue was created (rule 83's residual case). +- `PATCH /issues//result_photos` is `IssueDetailView` attaching *resolution* photos, + which genuinely are added after the fact. + ### Photo display in IssueDetailView Gated on `syncStatus`: @@ -600,13 +638,15 @@ While a photo is pending upload, the inspection form field value is `"local://

/photos` for the rest with `try? await`. By then `syncStatus == "synced"`, so `processIssueQueue` never revisited the issue: one failed PATCH silently cost every photo after the first, and the loss became invisible on device too once `pullAssignedIssues` overwrote `photoServerPaths` with the server's copy. The split was never necessary — `POST /api/v1/issues` already accepts `result_photos` and stores it in `mobile_photo_paths` (`app/api/issues.py`, `create_issue`), and `processPhotoQueue` fully populates `photoServerPaths` *before* `processIssueQueue` runs, so the extras were always known at create time. `submitIssue()` now sends `photo_path` + `result_photos` together: attachment is atomic with creation, there is no `synced`-but-unattached window to reconcile, and `mobile_local_id` idempotency covers retrying the whole request. The generalisation holds beyond photos — if a second call is needed after a record is marked synced, either fold it into the first or persist the debt; `try?` there means silent permanent loss. | +| 86 | **Two `PendingPhoto` rows sharing a local file must both receive the uploaded `serverPath`** | The de-dup pass marked the duplicates `"uploaded"` without ever setting `serverPath`, so the same image attached to two form fields submitted the second field blank. `processPhotoQueue` now uploads once and settles every row from a `localFilePath -> serverPath` map (a row whose twin failed stays `"pending"` so both retry together). Uploading once still matters independently: two uploads of one file yield two server filenames and duplicate the photo in the evidence and the PDF. | +| 87 | **`cleanupOrphanedPhotos()` sweeps `JQC/Photos` only, references EVERY surviving `local://` path, and never deletes a file younger than 7 days** | It pointed at `Documents/JQCPhotos`, which no writer has ever used — `contentsOfDirectory` failed, the `guard` returned, and it silently deleted nothing for its entire life while photos accumulated. Correcting the path is only safe alongside rule 83, and only with the reference set widened: a `local://` sentinel surviving on a *submitted* inspection means that photo never reached the server, so the file is the only copy left and is exactly what `PhotoDiagnosticView` reports as recoverable — the old draft-only filter would have deleted it. `JQC/ResultPhotos` is deliberately **not** swept: those files are staged in `IssueDetailView`'s `@State` with no database row, so nothing can prove one is unused. The 7-day age floor covers that flow and the window between writing a JPEG and saving the record that points at it. | +| 88 | **Local data is scoped to a `(server, userId)` pair — purge on an identity CHANGE, never on logout** | Two defects, one cause. (a) Logout deliberately kept the cache so the same inspector could work offline after signing back in — correct — but nothing checked that the next sign-in *was* the same inspector. `pullAssignedIssues`' reconciliation only deletes rows with `inspectionLocalId == ""`, so device-authored synced issues survived indefinitely and a different inspector on the same iPad simply inherited them. (b) The server switch cleared `LocalIssue` alone, leaving `LocalInspection` rows carrying `facilityServerId`/`templateServerId` values that name different rows on the server being switched to — ready to be submitted against it. `SessionScope` (UserDefaults, **not** Keychain — it must outlive `KeychainHelper.clearAll()`) records the pair; `AuthManager.reconcileSessionScope()` compares on every `login()`/`restoreSession()` and calls `SyncManager.purgeSessionScopedData()` only on a mismatch, before `isAuthenticated` flips so no view ever renders the previous user's data. `LocalInspection` is the only model with an author (`inspectorUserId`), so it is the only one whose unsent rows can be handed back; `LocalIssue` has none, and submitting one under a different inspector's credentials would put a false name on a QC record. A nil marker adopts the existing data rather than purging — fresh install and pre-marker upgrade are indistinguishable, and guessing wrong would delete the signing-in user's own draft. | +| 89 | **Never raise a second alert from inside the first one's button action** | Both alerts hang off the same view, so the new presentation is discarded while the first is still tearing down. `ExecuteInspectionView`'s Submit set `showNoGPSAlert = true` from inside the confirm alert's action, and the warning simply never appeared — tapping Submit without a GPS fix did *nothing at all*: no alert, no submission, no feedback. Park the intent in a `@State` flag and act on it from `onChange(of:)` when the first alert's binding flips false, with a short hop so the dismissal animation has finished. Applies to `.sheet`/`.confirmationDialog` chained onto one view too. | +| 90 | **`date` form fields are `"yyyy-MM-dd"`, and an unanswered one must render as unanswered** | Two defects in one widget, both in `CellDatePicker` and `DateFieldView`. (a) They stored `ISO8601DateFormatter().string(...)` — a full `2026-08-18T14:30:00Z` timestamp — into a field the web writes with `` and both the read-only grid and the PDF print verbatim. `FormDateFormat` (UTC + POSIX, `yyyy-MM-dd`) is now the single definition, and parses a leading date out of legacy timestamp values. (b) A `DatePicker` bound to an empty value still displays TODAY, so the field looked answered — but the setter only fires on a *change*, so selecting the already-shown date wrote nothing and `missingRequiredFields()` reported it missing with a date visible on screen. An explicit "Set date" affordance replaces the picker while the value is empty, plus an × to return to unanswered. | --- diff --git a/JanitorialQC/Models/PendingPhoto.swift b/JanitorialQC/Models/PendingPhoto.swift index ee5dbf2..48e02aa 100644 --- a/JanitorialQC/Models/PendingPhoto.swift +++ b/JanitorialQC/Models/PendingPhoto.swift @@ -22,7 +22,20 @@ final class PendingPhoto { /// Populated after successful upload var serverPath: String? /// "pending" | "uploaded" | "failed" + /// + /// "failed" is TERMINAL: it means every upload attempt was used up, and it + /// is what lets processInspectionQueue stop waiting and submit without this + /// photo. A *transient* error must therefore leave the row "pending" — see + /// `uploadRetryCount`. Marking "failed" on the first error is what turned a + /// single dropped connection into a permanently lost evidence photo. var uploadStatus: String + /// Consecutive failed upload attempts. The row stays "pending" — and so + /// keeps blocking its parent's submission — until this reaches + /// `SyncManager.maxPhotoUploadAttempts`. + /// + /// Non-optional with an inline default so SwiftData migrates lightweight + /// (CLAUDE.md rule 8): rows in existing stores read as 0. + var uploadRetryCount: Int = 0 var createdAt: Date // ── Capture metadata (sent to the server, burned into the photo) ─────── @@ -53,6 +66,7 @@ final class PendingPhoto { self.fieldId = fieldId self.serverPath = nil self.uploadStatus = "pending" + self.uploadRetryCount = 0 self.createdAt = Date() // Fall back to now when the caller has no recorded capture moment — // still far better than the server's upload-time default. diff --git a/JanitorialQC/Sync/SyncManager.swift b/JanitorialQC/Sync/SyncManager.swift index 8e20c6d..63f043c 100644 --- a/JanitorialQC/Sync/SyncManager.swift +++ b/JanitorialQC/Sync/SyncManager.swift @@ -299,35 +299,43 @@ class SyncManager: ObservableObject { // ── Outbox: Photos ──────────────────────────────────────────────────── + /// Upload attempts before a PendingPhoto is given up on. + /// + /// Until this is reached the row stays "pending", which does two things: + /// the next sync retries it, AND processInspectionQueue keeps waiting + /// rather than submitting the inspection with a blank photo field. + static let maxPhotoUploadAttempts = 5 + private func processPhotoQueue(context: ModelContext) async { // Fetch all then filter in Swift — #Predicate cannot reference // string literals against PendingPhoto.uploadStatus reliably // when the predicate type is inferred across model boundaries. guard let allPhotos = try? context.fetch(FetchDescriptor()) else { return } - var pending = allPhotos + let pending = allPhotos .filter { $0.uploadStatus == "pending" } .sorted { $0.createdAt < $1.createdAt } - // Defense-in-depth: if two PendingPhoto rows somehow reference the - // exact same local file (e.g. a future call site re-submitting the - // same photo array), only upload it once. The primary fix for photo - // duplication is the re-entrancy guard in triggerSync(), but this - // keeps processPhotoQueue itself safe even if it's ever invoked - // outside that guard. - var seenPaths = Set() + // Two rows can legitimately point at the same file — the same image + // attached to two form fields, or a call site re-submitting a photo + // array. Upload it once, then give EVERY row that references it the + // same server path. + // + // The previous version marked the duplicates "uploaded" up front and + // never set their serverPath, so the second field was submitted blank. + // Settling them from the upload result instead keeps every field + // pointing at real evidence. (Uploading once still matters: two uploads + // of one file produce two different server filenames and duplicate the + // photo in the issue's evidence and its PDF.) + var firstByPath: [String: PendingPhoto] = [:] + var toUpload: [PendingPhoto] = [] var duplicates: [PendingPhoto] = [] - pending = pending.filter { photo in - if seenPaths.contains(photo.localFilePath) { + for photo in pending { + if firstByPath[photo.localFilePath] == nil { + firstByPath[photo.localFilePath] = photo + toUpload.append(photo) + } else { duplicates.append(photo) - return false } - seenPaths.insert(photo.localFilePath) - return true - } - for dup in duplicates { - // Mark the duplicate row as uploaded without re-uploading — the - // first row for this file will populate serverPath/photoServerPaths. - dup.uploadStatus = "uploaded" } // Pre-fetch parent records ONCE before the loop. @@ -340,7 +348,9 @@ class SyncManager: ObservableObject { let allInspections = (try? context.fetch(FetchDescriptor())) ?? [] let allIssues = (try? context.fetch(FetchDescriptor())) ?? [] - for photo in pending { + var uploadedPaths: [String: String] = [:] // localFilePath -> serverPath + + for photo in toUpload { do { // Capture metadata was recorded at the shutter, not now — the // sync may run hours after an offline capture, and the server @@ -352,35 +362,122 @@ class SyncManager: ObservableObject { latitude: photo.captureLatitude, longitude: photo.captureLongitude ) - photo.serverPath = serverPath - photo.uploadStatus = "uploaded" - - // Update parent inspection form field value. - // Pre-fetched before the loop — not repeated per photo. - if photo.entityType == "inspection", let fieldId = photo.fieldId { - let entityId = photo.entityLocalId - allInspections.first(where: { $0.localId == entityId })? - .setValue(serverPath, forFieldId: fieldId) - } - - // Update parent issue photo paths array. - // Pre-fetched before the loop — not repeated per photo. - if photo.entityType == "issue" { - let entityId = photo.entityLocalId - if let issue = allIssues.first(where: { $0.localId == entityId }) { - var paths = issue.photoServerPaths - if !paths.contains(serverPath) { paths.append(serverPath) } - issue.photoServerPaths = paths - } - } + photo.serverPath = serverPath + photo.uploadStatus = "uploaded" + photo.uploadRetryCount = 0 + uploadedPaths[photo.localFilePath] = serverPath + attachServerPath(serverPath, for: photo, + inspections: allInspections, issues: allIssues) + await pushLateIssuePhotoIfNeeded(serverPath, for: photo, + issues: allIssues, context: context) try? context.save() } catch { - photo.uploadStatus = "failed" + // Treat the error as TRANSIENT by default. Leaving the row + // "pending" means the next sync retries it and — critically — + // processInspectionQueue keeps waiting instead of submitting + // the inspection with this field blanked to "" by + // APIClient.submitInspection. + // + // The old code marked "failed" on the very first error, and + // nothing anywhere ever moved a row back off "failed". One + // dropped connection therefore cost the inspection its evidence + // photo permanently, silently, with the inspection still + // reported as successfully synced. + photo.uploadRetryCount += 1 + if photo.uploadRetryCount >= Self.maxPhotoUploadAttempts { + photo.uploadStatus = "failed" + syncError = "A photo failed to upload after " + + "\(Self.maxPhotoUploadAttempts) attempts. " + + "Open Pending Sync → Retry Failed Items to try again." + } try? context.save() } } + + // Settle rows that shared a file with one just uploaded. A row whose + // twin failed is deliberately left "pending" so both retry together. + for dup in duplicates { + guard let serverPath = uploadedPaths[dup.localFilePath] else { continue } + dup.serverPath = serverPath + dup.uploadStatus = "uploaded" + dup.uploadRetryCount = 0 + attachServerPath(serverPath, for: dup, + inspections: allInspections, issues: allIssues) + await pushLateIssuePhotoIfNeeded(serverPath, for: dup, + issues: allIssues, context: context) + } + try? context.save() + } + + /// Carry a photo across to an issue that has ALREADY been created on the + /// server, which the create request therefore could not have included. + /// + /// Only reachable via recovery: the photo exhausted + /// `maxPhotoUploadAttempts`, the issue was submitted without it (rule 83 + /// deliberately lets that happen rather than blocking forever), and the + /// inspector later hit Pending Sync → Retry Failed Items and the upload + /// succeeded. In the normal path processPhotoQueue runs BEFORE + /// processIssueQueue, so `issue.serverId` is still nil here and this does + /// nothing — which is the discriminator, and why this is not a rule 85 + /// violation. + /// + /// Must happen now, in this same pass: `pullAssignedIssues` later overwrites + /// `photoServerPaths` with the server's copy, so a photo left only in local + /// state would be erased before anything else could notice it. + /// + /// Best-effort. A failure here leaves the photo attached locally but not + /// server-side until the next pull overwrites it — the residual limitation + /// noted in rule 83. Reaching this at all takes five consecutive upload + /// failures, and the PATCH merges idempotently, so a repeat is harmless. + private func pushLateIssuePhotoIfNeeded( + _ serverPath: String, + for photo: PendingPhoto, + issues: [LocalIssue], + context: ModelContext + ) async { + guard photo.entityType == "issue" else { return } + let entityId = photo.entityLocalId + guard let issue = issues.first(where: { $0.localId == entityId }), + let issueServerId = issue.serverId + else { return } + + do { + try await APIClient.shared.updateIssuePhotos( + issueId: issueServerId, resultPhotos: [serverPath] + ) + issue.syncErrorMessage = nil + } catch { + issue.syncErrorMessage = + "A recovered photo could not be attached: \(error.localizedDescription)" + } + try? context.save() + } + + /// Write a freshly uploaded server path onto whichever record owns the photo. + /// Both collections are pre-fetched by the caller — see processPhotoQueue. + private func attachServerPath( + _ serverPath: String, + for photo: PendingPhoto, + inspections: [LocalInspection], + issues: [LocalIssue] + ) { + let entityId = photo.entityLocalId + + // Inspection form image field. + if photo.entityType == "inspection", let fieldId = photo.fieldId { + inspections.first(where: { $0.localId == entityId })? + .setValue(serverPath, forFieldId: fieldId) + } + + // Issue evidence photo array. + if photo.entityType == "issue", + let issue = issues.first(where: { $0.localId == entityId }) { + var paths = issue.photoServerPaths + if !paths.contains(serverPath) { paths.append(serverPath) } + issue.photoServerPaths = paths + } } // ── Outbox: Inspections ─────────────────────────────────────────────── @@ -392,6 +489,13 @@ class SyncManager: ObservableObject { .sorted { $0.createdAt < $1.createdAt } for inspection in pending { + // "failed" is only reachable after maxPhotoUploadAttempts, so this + // now means "uploaded, or genuinely unrecoverable" rather than + // "uploaded, or hit one network error". A still-retrying photo + // keeps its row "pending" and holds the inspection back — which is + // the point: submitting first is what blanked the field for good, + // since APIClient.submitInspection rewrites a surviving local:// + // value to "" and the inspection is then marked synced forever. let photosReady = inspection.pendingPhotos.allSatisfy { $0.uploadStatus == "uploaded" || $0.uploadStatus == "failed" } @@ -443,11 +547,30 @@ class SyncManager: ObservableObject { // processInspectionQueue wrote the serverId back, leaving it nil even // when the parent inspection already synced successfully this same pass. let allInspections = (try? context.fetch(FetchDescriptor())) ?? [] + // Issue photos are inserted standalone (FlagIssueView / StandaloneIssueView + // do not append them to any relationship), so they have to be matched by + // entityType + entityLocalId rather than navigated to. + let allPhotos = (try? context.fetch(FetchDescriptor())) ?? [] for issue in pending { let parentLocalId = issue.inspectionLocalId let parent = allInspections.first(where: { $0.localId == parentLocalId }) + // ── Photo readiness ─────────────────────────────────────────── + // Wait for this issue's photos exactly as processInspectionQueue + // waits for an inspection's. There was no guard here at all: the + // issue submitted with photo_path = photoServerPaths.first (nil + // while uploads were still in flight or being retried), and the + // unconditional clear below then dropped the only local reference + // to the files. + let issuePhotos = allPhotos.filter { + $0.entityType == "issue" && $0.entityLocalId == issue.localId + } + let photosSettled = issuePhotos.allSatisfy { + $0.uploadStatus == "uploaded" || $0.uploadStatus == "failed" + } + guard photosSettled else { continue } + // ── Parent inspection status guards ─────────────────────────── if let parent { switch parent.syncStatus { @@ -486,23 +609,25 @@ class SyncManager: ObservableObject { let issueId = try await APIClient.shared.submitIssue(issue, inspectionServerId: inspectionServerId) issue.serverId = issueId issue.syncStatus = "synced" - // Photos are now represented by photoServerPaths on the server. - // Clear the local file paths so IssueDetailView doesn't render - // a duplicate "local photos" section alongside the server section. - issue.photoLocalPaths = [] - try? context.save() - // If there are additional photos beyond the first (which was sent - // as photo_path on create), PATCH them to result_photos now. - // The server create endpoint only stores photo_path; result_photos - // must be set via a separate PATCH call. - let extras = Array(issue.photoServerPaths.dropFirst()) - if !extras.isEmpty { - try? await APIClient.shared.updateIssuePhotos( - issueId: issueId, resultPhotos: extras - ) + // Clear the local file paths ONLY when every photo actually + // reached the server. Clearing unconditionally is what made a + // partial upload unrecoverable: the files stayed on disk but + // nothing referenced them any more, so neither the UI nor + // PhotoDiagnosticView could find them again. Keeping them costs + // a duplicate photo section in IssueDetailView at worst; losing + // them costs the evidence itself. + let allUploaded = issuePhotos.allSatisfy { $0.uploadStatus == "uploaded" } + if allUploaded { + issue.photoLocalPaths = [] } + // No follow-up call: submitIssue() sends every photo in the + // create request (photo_path + result_photos), so attachment is + // atomic with creation — there is no window in which the issue + // is `synced` but its photos are not attached. See rule 85. + try? context.save() + } catch { issue.syncRetryCount += 1 issue.syncErrorMessage = error.localizedDescription @@ -657,6 +782,14 @@ class SyncManager: ObservableObject { private static let lastCleanupKey = "jqc.photoCleanup.lastRunAt" private static let cleanupInterval: TimeInterval = 3600 // 1 hour + /// A file must be at least this old before cleanup will consider it. + /// + /// Resolution photos staged in IssueDetailView live purely in `@State` + /// until they upload — no database row references them — so for that flow + /// age is the only available evidence that a file is not in active use. + /// The floor also covers the window between writing a JPEG and saving the + /// record that points at it. + private static let cleanupMinFileAge: TimeInterval = 7 * 24 * 3600 // 7 days private func cleanupOrphanedPhotos(context: ModelContext) { let last = UserDefaults.standard.object(forKey: Self.lastCleanupKey) as? Date @@ -673,9 +806,17 @@ class SyncManager: ObservableObject { referencedPaths.insert(p.localFilePath) } } - // LocalInspection — draft photos (formData values starting with "local://") + // LocalInspection — EVERY field still holding the local:// sentinel, + // whatever the inspection's status. + // + // Deliberately not limited to drafts. A sentinel surviving on a + // submitted inspection means that photo never reached the server, so + // the file on disk is the only copy in existence — it is exactly the + // material PhotoDiagnosticView reports as "recoverable". Restricting + // this to drafts would have made correcting the directory below destroy + // the evidence this whole fix exists to preserve. if let inspections = try? context.fetch(FetchDescriptor()) { - for insp in inspections where insp.status == "draft" { + for insp in inspections { for val in insp.formData.values { if let s = val as? String, s.hasPrefix("local://") { referencedPaths.insert(String(s.dropFirst("local://".count))) @@ -683,9 +824,12 @@ class SyncManager: ObservableObject { } } } - // LocalIssue — unsync'd issue photos + // LocalIssue — every retained local path, synced or not, for the same + // reason. processIssueQueue now clears these only once all photos have + // actually uploaded, so a path still present means evidence the server + // does not have. if let issues = try? context.fetch(FetchDescriptor()) { - for issue in issues where issue.syncStatus != "synced" { + for issue in issues { for path in issue.photoLocalPaths { referencedPaths.insert(path) } } } @@ -695,20 +839,39 @@ class SyncManager: ObservableObject { // the main thread when JQCPhotos/ contains hundreds of files. Dispatching // here is safe because `referencedPaths` is a value type (Set) // captured by copy — no shared mutable state crosses the boundary. + let minAge = Self.cleanupMinFileAge Task.detached(priority: .utility) { let fm = FileManager.default guard let docsDir = fm.urls(for: .documentDirectory, in: .userDomainMask).first else { return } - let photosDir = docsDir.appendingPathComponent("JQCPhotos") + + // "JQC/Photos" — the directory every writer actually uses + // (FlagIssueView, StandaloneIssueView, CompactImageFieldView, + // ImageFieldView). This previously read "JQCPhotos", which no code + // path has ever written to: contentsOfDirectory failed, the guard + // returned, and this function silently deleted nothing for its + // entire life while photos accumulated indefinitely. + // + // "JQC/ResultPhotos" is deliberately NOT swept. Those files are + // staged in IssueDetailView's @State with no database row, so + // nothing here can prove one is unused; they are cleaned up by + // uploadAndAttachResultPhotos() and removeResultPhoto() instead. + let photosDir = docsDir.appendingPathComponent("JQC/Photos", isDirectory: true) guard let diskFiles = try? fm.contentsOfDirectory( - at: photosDir, includingPropertiesForKeys: nil + at: photosDir, includingPropertiesForKeys: [.contentModificationDateKey] ) else { return } + let cutoff = Date().addingTimeInterval(-minAge) var deletedCount = 0 for fileURL in diskFiles { - if !referencedPaths.contains(fileURL.path) { - try? fm.removeItem(at: fileURL) - deletedCount += 1 - } + if referencedPaths.contains(fileURL.path) { continue } + // Age floor — never touch a file young enough to belong to a + // capture flow that has not yet written its record. + let modified = (try? fileURL.resourceValues( + forKeys: [.contentModificationDateKey] + ))?.contentModificationDate + guard let modified, modified < cutoff else { continue } + try? fm.removeItem(at: fileURL) + deletedCount += 1 } if deletedCount > 0 { print("[JQC] Sync | cleanupOrphanedPhotos | removed \(deletedCount) file(s)") @@ -1025,6 +1188,107 @@ class SyncManager: ObservableObject { } } + // ── Session-scope purge ─────────────────────────────────────────────── + + /// Delete local data belonging to a previous session. + /// + /// Called when the `(server, user)` pair the database is scoped to changes + /// — a different inspector signing in on this iPad, or a server switch. See + /// `SessionScope` for why that combination is the boundary, and rule 88. + /// + /// - Parameters: + /// - keepingUserId: the incoming user. Their own unsent inspections are + /// preserved when `sameServer` is true; pass nil to keep nothing. + /// - sameServer: false when the server changed, in which case NOTHING can + /// be kept — every `serverId`, `facilityServerId` and `templateServerId` + /// names a row in a different database. + /// + /// Deliberately NOT called on a plain logout: the common case is the same + /// inspector signing back into the same server, and wiping the cache there + /// would leave them with an empty app until a full sync succeeds, breaking + /// offline use. That was the correct instinct in the original code — the + /// bug was only that nothing ever checked whether the next login was + /// actually the same person. + func purgeSessionScopedData(keepingUserId: Int?, sameServer: Bool) { + guard let context = modelContext else { return } + + // ── Reference caches ────────────────────────────────────────────── + // Pure server-scoped copies with no local authorship. Cheap to re-pull, + // so there is never a reason to keep one across an identity change. + deleteAll(LocalFacility.self, from: context) // cascades areas + deleteAll(LocalArea.self, from: context) + deleteAll(LocalTemplate.self, from: context) + deleteAll(LocalScheduledInspection.self, from: context) + deleteAll(LocalFollowUpRequest.self, from: context) + + // ── Issues ──────────────────────────────────────────────────────── + // All of them, unconditionally. LocalIssue carries no author field, so + // an unsent one cannot be attributed — and submitting the previous + // inspector's issue under the new inspector's credentials would put a + // false name on a QC record. Server-pulled and already-synced rows are + // the previous user's assignments and stale serverIds respectively. + deleteAll(LocalIssue.self, from: context) + + // ── Inspections ─────────────────────────────────────────────────── + // These DO carry an author (`inspectorUserId`), so the incoming user's + // own unsent work can be handed back to them intact — but only when the + // server is unchanged, since otherwise its facility/template ids point + // into the wrong database. + let inspections = (try? context.fetch(FetchDescriptor())) ?? [] + var keptInspectionIds = Set() + for insp in inspections { + let ownedByIncomingUser = keepingUserId.map { $0 == insp.inspectorUserId } ?? false + if sameServer && ownedByIncomingUser { + keptInspectionIds.insert(insp.localId) + } else { + // Remove the JPEGs too — cleanupOrphanedPhotos would otherwise + // wait out its 7-day age floor holding another user's evidence. + for photo in insp.pendingPhotos { + try? FileManager.default.removeItem(atPath: photo.localFilePath) + } + context.delete(insp) // cascades pendingPhotos + localIssues + } + } + + // ── Photos orphaned by the above ────────────────────────────────── + // Issue photos are inserted standalone rather than through a + // relationship, so no cascade reaches them — and every LocalIssue has + // just been deleted, which is why only kept inspections can retain one. + let photos = (try? context.fetch(FetchDescriptor())) ?? [] + for photo in photos { + let stillOwned = photo.entityType == "inspection" + && keptInspectionIds.contains(photo.entityLocalId) + if !stillOwned { + try? FileManager.default.removeItem(atPath: photo.localFilePath) + context.delete(photo) + } + } + + try? context.save() + + // ── In-memory state from the old session ────────────────────────── + // The poll TASK is left running; only its data is dropped. Stopping it + // here would leave polling dead until the next connectivity change or + // foreground, because nothing on the login path restarts it. + dashboardStats = nil + lastNotificationFetch = nil + unreadNotificationCount = 0 + recentNotifications = [] + syncError = nil + lastSyncAt = nil + updatePendingCount(context: context) + + print("[JQC] Sync | purgeSessionScopedData | kept \(keptInspectionIds.count) " + + "inspection(s) for user \(keepingUserId.map(String.init) ?? "-")") + } + + /// Fetch-all + delete. No #Predicate (CLAUDE.md rule 3), and `try?` + /// parenthesised before `??` (rule 25). + private func deleteAll(_ type: T.Type, from context: ModelContext) { + let rows = (try? context.fetch(FetchDescriptor())) ?? [] + for row in rows { context.delete(row) } + } + // ── Dashboard Stats ─────────────────────────────────────────────────── // Best-effort fetch — a network failure silently leaves dashboardStats nil // so the UI falls back to a placeholder card. Never blocks the sync pipeline. diff --git a/JanitorialQC/Utils/Constants.swift b/JanitorialQC/Utils/Constants.swift index 4270cd7..6c8d549 100644 --- a/JanitorialQC/Utils/Constants.swift +++ b/JanitorialQC/Utils/Constants.swift @@ -59,6 +59,59 @@ nonisolated enum ServerConfig { } } +// MARK: - Session scope + +/// The (server, user) pair the local database is currently scoped to. +/// +/// Almost every local id is meaningful only within one such pair. `serverId` +/// values differ between jqc and jqc1; facility/template ids and inspector +/// facility scope differ per user. So when either half changes, the cached data +/// is not merely stale — it is *wrong*, and silently belongs to someone else. +/// +/// Two concrete failures this exists to close: +/// • A different inspector signing in on the same iPad inherited the previous +/// one's issues. `pullAssignedIssues`' reconciliation only deletes rows with +/// `inspectionLocalId == ""`, so device-authored synced issues survived +/// indefinitely and were simply visible to whoever logged in next. +/// • A server switch cleared `LocalIssue` alone, leaving `LocalInspection` +/// rows carrying facility/template ids from the other server, ready to be +/// submitted against it. +/// +/// Deliberately in UserDefaults, NOT the Keychain: `KeychainHelper.clearAll()` +/// runs on logout, and this marker has to OUTLIVE a logout to be able to notice +/// that the next login is a different person. It is not a secret. +nonisolated enum SessionScope { + + struct Scope: Equatable, Sendable { + let server: String + let userId: Int + } + + private static let userKey = "com.jqc.sessionScope.userId" + private static let serverKey = "com.jqc.sessionScope.server" + + /// The recorded scope, or nil when none has ever been written. + static var stored: Scope? { + let d = UserDefaults.standard + guard let server = d.string(forKey: serverKey), + d.object(forKey: userKey) != nil + else { return nil } + return Scope(server: server, userId: d.integer(forKey: userKey)) + } + + static func record(userId: Int, server: String = ServerConfig.current) { + UserDefaults.standard.set(userId, forKey: userKey) + UserDefaults.standard.set(server, forKey: serverKey) + } + + /// Forget the scope entirely — used on a server switch, where the next + /// login is guaranteed to be against a different data set. + static func clear() { + UserDefaults.standard.removeObject(forKey: userKey) + UserDefaults.standard.removeObject(forKey: serverKey) + } +} + // MARK: - App-wide constants // Explicitly not @MainActor — these constants must be readable from diff --git a/JanitorialQC/Views/Dashboard/ExecuteInspectionView.swift b/JanitorialQC/Views/Dashboard/ExecuteInspectionView.swift index 24ea0bb..d3c9bb8 100644 --- a/JanitorialQC/Views/Dashboard/ExecuteInspectionView.swift +++ b/JanitorialQC/Views/Dashboard/ExecuteInspectionView.swift @@ -29,10 +29,27 @@ struct ExecuteInspectionView: View { /// Work is preserved either way — .onDisappear calls saveDraft(). var isModallyPresented: Bool = false + /// What to do when the inspection has been submitted, instead of the + /// default `dismiss()`. + /// + /// `StartInspectionView` PUSHES this view onto the NavigationStack inside + /// its own `.fullScreenCover`, so a plain `dismiss()` only pops — landing + /// the inspector back on the "New Inspection" form they just started from, + /// with Cancel as the only way out. It passes its own dismiss here so the + /// whole cover closes and they return to the dashboard. + /// + /// Left nil everywhere else, where popping IS correct: the My Inspections + /// row pushes onto the list's stack, and the dashboard's Resume banner + /// presents this view as the cover root. + var onFinished: (() -> Void)? = nil + @State private var formValues: [String: String] = [:] @State private var showFlagIssue = false @State private var showSubmitAlert = false @State private var showNoGPSAlert = false + /// Set when Submit is tapped with no GPS fix; consumed by + /// `onChange(of: showSubmitAlert)` once the confirm alert has dismissed. + @State private var pendingNoGPSPrompt = false @State private var showValidationAlert = false @State private var missingFields: [String] = [] @State private var isSaving = false @@ -101,7 +118,13 @@ struct ExecuteInspectionView: View { switch ftype { case "rating": - if let v = Int(val), v > 0 { earned += v; total += field["max"] as? Int ?? 5 } + // Denominator is a FLAT 5, never the field's `max`. + // `_compute_score_from_form()` in the Flask app (routes/ + // inspections.py) hardcodes `total += 5`, and LocalInspection + // .computeScore() mirrors it — this was the only site reading + // `max`, so a template with max != 5 showed one percentage in + // the toolbar and submitted a different one. + if let v = Int(val), v > 0 { earned += v; total += 5 } case "checkbox": total += 1; if val == "true" { earned += 1 } case "radio": @@ -194,7 +217,16 @@ struct ExecuteInspectionView: View { if locationManager.lastLocation == nil { // No GPS fix yet — warn before proceeding rather than // silently submitting without a location. - showNoGPSAlert = true + // + // Deferred, NOT set here: raising a second alert from + // inside the first one's action, with both attached to the + // same view, is dropped by SwiftUI — the confirm alert is + // still tearing down, so the new presentation is discarded. + // The visible effect was that tapping Submit without a fix + // did nothing at all: no warning, no submission. Handing it + // to onChange(of: showSubmitAlert) below presents it only + // once the first alert has actually gone. + pendingNoGPSPrompt = true } else { Task { await submitInspection() } } @@ -223,7 +255,26 @@ struct ExecuteInspectionView: View { .onChange(of: showSubmitAlert) { _, showing in // Begin acquiring a GPS fix the moment the confirm dialog appears // so a location is likely ready by the time the inspector taps Submit. - if showing { locationManager.requestLocation() } + if showing { + locationManager.requestLocation() + return + } + // Confirm alert has closed. If Submit was tapped without a fix, + // raise the warning now that the presentation slot is free. + guard pendingNoGPSPrompt else { return } + pendingNoGPSPrompt = false + Task { + // One runloop hop. `showing == false` means the binding flipped, + // not that the dismissal animation has finished, and presenting + // into the tail of that animation is unreliable. + try? await Task.sleep(for: .milliseconds(350)) + // Re-check: the fix may have landed while the dialog was up. + if locationManager.lastLocation == nil { + showNoGPSAlert = true + } else { + await submitInspection() + } + } } // Result overlay .overlay(alignment: .top) { @@ -678,9 +729,9 @@ struct ExecuteInspectionView: View { Task { await sync.triggerSync() } } - // Wait 2.5 seconds so inspector reads the result, then dismiss + // Wait 2.5 seconds so inspector reads the result, then leave. try? await Task.sleep(for: .seconds(2.5)) - dismiss() + if let onFinished { onFinished() } else { dismiss() } } /// Find the parent LocalInspection and clear its followUpRequired flag. @@ -1409,15 +1460,83 @@ struct CellDatePicker: View { private var dateBinding: Binding { Binding( - get: { ISO8601DateFormatter().date(from: value) ?? Date() }, - set: { value = ISO8601DateFormatter().string(from: $0) } + get: { FormDateFormat.date(from: value) ?? Date() }, + set: { value = FormDateFormat.string(from: $0) } ) } var body: some View { - DatePicker("", selection: dateBinding, displayedComponents: .date) - .labelsHidden() + // An empty value must LOOK empty until the inspector acts. + // + // The old version bound the DatePicker straight to the value: when it + // was empty the picker still displayed TODAY, so the field looked + // answered — but the setter only fires on a CHANGE, so selecting the + // already-displayed date wrote nothing and missingRequiredFields() + // reported the field missing with a date plainly visible on screen. + // The inspector had to pick a different day and navigate back. This + // explicit step makes unanswered look unanswered and makes today + // selectable in one tap. + if value.isEmpty { + Button { + value = FormDateFormat.string(from: Date()) + } label: { + HStack(spacing: 4) { + Image(systemName: "calendar").font(.system(size: 11)) + Text("Set date").font(.system(size: 12)) + } + .foregroundStyle(Color(.placeholderText)) + .padding(.horizontal, 6).padding(.vertical, 3) + .frame(maxWidth: .infinity, alignment: .leading) + .background(Color(.systemBackground)) + .clipShape(RoundedRectangle(cornerRadius: 5)) + .overlay(RoundedRectangle(cornerRadius: 5) + .stroke(Color(.systemGray4), lineWidth: 1)) + } + .buttonStyle(.plain) + } else { + HStack(spacing: 2) { + DatePicker("", selection: dateBinding, displayedComponents: .date) + .labelsHidden() + Button { value = "" } label: { // back to unanswered + Image(systemName: "xmark.circle.fill") + .font(.system(size: 12)) + .foregroundStyle(.secondary) + } + .buttonStyle(.plain) + } .frame(maxWidth: .infinity, alignment: .leading) + } + } +} + +// MARK: - FormDateFormat +// Wire format for `date` form fields: "yyyy-MM-dd", matching the web's +// (templates/inspections/execute.html), so a value entered +// on the iPad and one entered in a browser are the same string in form_data. +// +// Both date widgets previously used ISO8601DateFormatter, which round-tripped a +// full timestamp ("2026-08-18T14:30:00Z") into a field that the web renders and +// the PDF prints verbatim. +// +// UTC + POSIX locale so the day cannot shift with device timezone or calendar. +// nonisolated for the same reason as PhotoCaptureFormat (rule 82). +nonisolated enum FormDateFormat { + static let formatter: DateFormatter = { + let f = DateFormatter() + f.locale = Locale(identifier: "en_US_POSIX") + f.timeZone = TimeZone(identifier: "UTC") + f.dateFormat = "yyyy-MM-dd" + return f + }() + + static func string(from date: Date) -> String { formatter.string(from: date) } + + /// Parses the canonical form, and tolerates a leading `yyyy-MM-dd` inside a + /// longer timestamp so values written by earlier builds still display. + static func date(from value: String) -> Date? { + if let d = formatter.date(from: value) { return d } + guard value.count >= 10 else { return nil } + return formatter.date(from: String(value.prefix(10))) } } diff --git a/JanitorialQC/Views/Dashboard/FormFieldView.swift b/JanitorialQC/Views/Dashboard/FormFieldView.swift index 5b87e2c..555fdce 100644 --- a/JanitorialQC/Views/Dashboard/FormFieldView.swift +++ b/JanitorialQC/Views/Dashboard/FormFieldView.swift @@ -183,19 +183,45 @@ struct DateFieldView: View { private var dateBinding: Binding { Binding( - get: { - ISO8601DateFormatter().date(from: value) ?? Date() - }, - set: { - value = ISO8601DateFormatter().string(from: $0) - } + get: { FormDateFormat.date(from: value) ?? Date() }, + set: { value = FormDateFormat.string(from: $0) } ) } var body: some View { - DatePicker("", selection: dateBinding, displayedComponents: .date) - .labelsHidden() + // Same two problems as CellDatePicker, same fix — see the comments + // there. Empty must look empty, and the stored format is "yyyy-MM-dd" + // to match the web's , not an ISO 8601 timestamp. + if value.isEmpty { + Button { + value = FormDateFormat.string(from: Date()) + } label: { + HStack(spacing: 6) { + Image(systemName: "calendar") + Text("Set date") + } + .font(.callout) + .foregroundStyle(Color(.placeholderText)) + .padding(10) + .frame(maxWidth: .infinity, alignment: .leading) + .background(Color(.systemBackground)) + .clipShape(RoundedRectangle(cornerRadius: 8)) + .overlay(RoundedRectangle(cornerRadius: 8) + .stroke(Color(.systemGray4), lineWidth: 1)) + } + .buttonStyle(.plain) + } else { + HStack(spacing: 6) { + DatePicker("", selection: dateBinding, displayedComponents: .date) + .labelsHidden() + Button { value = "" } label: { + Image(systemName: "xmark.circle.fill") + .foregroundStyle(.secondary) + } + .buttonStyle(.plain) + } .frame(maxWidth: .infinity, alignment: .leading) + } } } diff --git a/JanitorialQC/Views/Dashboard/SettingsView.swift b/JanitorialQC/Views/Dashboard/SettingsView.swift index f8222c2..26509fc 100644 --- a/JanitorialQC/Views/Dashboard/SettingsView.swift +++ b/JanitorialQC/Views/Dashboard/SettingsView.swift @@ -129,13 +129,17 @@ struct SettingsView: View { Section { Button(role: .destructive) { Task { - // Do NOT clear server-pulled data on a plain logout — - // the user is logging out of the same server, so cached - // facilities, issues, and templates are still valid on - // their next login. Clearing here leaves the issues list - // empty until a full sync succeeds, which breaks offline use. - // Server-pulled data is only cleared when switching servers - // (see the Switch & Log Out alert below). + // Do NOT purge on a plain logout — the same inspector + // signing back into the same server must still find + // their facilities, templates and issues there, or the + // app is unusable offline until a full sync succeeds. + // + // What was missing is not a purge here: it is the check + // that the next sign-in is the SAME person. + // AuthManager.reconcileSessionScope() now does that at + // login and purges only on an identity change, so a + // different inspector no longer inherits this one's + // issues (rule 88). sync.resetNotificationPoller() await auth.logout() } @@ -207,7 +211,14 @@ struct SettingsView: View { settingsServer = chosen pendingServer = nil Task { - clearServerPulledData() + // Purge EVERYTHING, not just issues. The old + // clearServerPulledData() deleted LocalIssue alone, + // leaving LocalInspection rows carrying facility and + // template ids that name different rows on the server + // being switched to — ready to be submitted against it. + // Nothing local survives a server change (rule 88). + sync.purgeSessionScopedData(keepingUserId: nil, sameServer: false) + SessionScope.clear() sync.resetNotificationPoller() await auth.logout() } @@ -218,7 +229,7 @@ struct SettingsView: View { } } message: { if let chosen = pendingServer { - Text("Switching to \(chosen.displayName) will log you out. All cached server data will be cleared. You will need to log in again.") + Text("Switching to \(chosen.displayName) will log you out and erase all local data for this server — including any inspections or issues that have not synced yet. You will need to log in again.") } } } @@ -242,18 +253,4 @@ struct SettingsView: View { } } - /// Delete every LocalIssue that has ever been assigned a serverId. - /// This covers two categories: - /// 1. Server-pulled assigned issues (inspectionLocalId == "", syncStatus == "synced") - /// 2. Inspector-created issues that already synced (inspectionLocalId != "", serverId != nil) - /// — their serverIds are meaningless on a different server, so they must go too. - /// The only records preserved are truly pending device-created issues - /// (serverId == nil, syncStatus == "pending") that have never reached any server. - private func clearServerPulledData() { - let allIssues = (try? context.fetch(FetchDescriptor())) ?? [] - allIssues - .filter { $0.serverId != nil } - .forEach { context.delete($0) } - try? context.save() - } } diff --git a/JanitorialQC/Views/Dashboard/StartInspectionView.swift b/JanitorialQC/Views/Dashboard/StartInspectionView.swift index 03bd4b8..03d0103 100644 --- a/JanitorialQC/Views/Dashboard/StartInspectionView.swift +++ b/JanitorialQC/Views/Dashboard/StartInspectionView.swift @@ -298,7 +298,11 @@ struct StartInspectionView: View { } .navigationDestination(isPresented: $navigateToExecution) { if let inspection = createdInspection { - ExecuteInspectionView(inspection: inspection) + // onFinished closes THIS cover rather than just popping back + // to the form the inspector already finished with — see the + // property's doc comment on ExecuteInspectionView. + ExecuteInspectionView(inspection: inspection, + onFinished: { dismiss() }) } } .onAppear { diff --git a/JanitorialQC/Views/Dashboard/SyncStatusView.swift b/JanitorialQC/Views/Dashboard/SyncStatusView.swift index 500a228..7bc7102 100644 --- a/JanitorialQC/Views/Dashboard/SyncStatusView.swift +++ b/JanitorialQC/Views/Dashboard/SyncStatusView.swift @@ -19,9 +19,20 @@ struct SyncStatusView: View { sort: \LocalIssue.createdAt ) private var pendingIssues: [LocalIssue] + /// Unfiltered — `uploadStatus` is matched in Swift rather than in a + /// #Predicate, per CLAUDE.md rules 3/48. + @Query private var allPendingPhotos: [PendingPhoto] + private var failedInspections: [LocalInspection] { pendingInspections.filter { $0.syncStatus == "failed" } } private var failedIssues: [LocalIssue] { pendingIssues.filter { $0.syncStatus == "failed" } } - private var hasFailedItems: Bool { !failedInspections.isEmpty || !failedIssues.isEmpty } + /// Photos that exhausted `SyncManager.maxPhotoUploadAttempts`. These are the + /// reason an inspection can be submitted with a blank photo field, and + /// nothing else in the app ever moves one off "failed" — so they belong in + /// the retry action too. + private var failedPhotos: [PendingPhoto] { allPendingPhotos.filter { $0.uploadStatus == "failed" } } + private var hasFailedItems: Bool { + !failedInspections.isEmpty || !failedIssues.isEmpty || !failedPhotos.isEmpty + } var body: some View { List { @@ -60,7 +71,7 @@ struct SyncStatusView: View { Button { retryAllFailed() } label: { - Label("Retry Failed Items (\(failedInspections.count + failedIssues.count))", + Label("Retry Failed Items (\(failedInspections.count + failedIssues.count + failedPhotos.count))", systemImage: "exclamationmark.arrow.circlepath") .foregroundStyle(.orange) } @@ -113,6 +124,14 @@ struct SyncStatusView: View { issue.syncRetryCount = 0 issue.syncErrorMessage = nil } + // Photos too. A PendingPhoto only reaches "failed" after every upload + // attempt was used, and that is exactly the state that lets an + // inspection be submitted with its photo field blank — so without this + // the retry button could never actually recover a lost photo. + for photo in failedPhotos { + photo.uploadStatus = "pending" + photo.uploadRetryCount = 0 + } try? context.save() Task { await sync.triggerSync() } } diff --git a/JanitorialQC/Views/Inspection/InspectionHistoryView.swift b/JanitorialQC/Views/Inspection/InspectionHistoryView.swift index 6b3adb9..7103909 100644 --- a/JanitorialQC/Views/Inspection/InspectionHistoryView.swift +++ b/JanitorialQC/Views/Inspection/InspectionHistoryView.swift @@ -162,6 +162,14 @@ struct InspectionHistoryView: View { filterToDate = nil showFilterSheet = false Task { await load(reset: true) } + } onCancel: { + // Discard the draft edits, keep whatever is currently applied, + // and do NOT reload — backing out must change nothing. + draftFromEnabled = filterFromDate != nil + draftToEnabled = filterToDate != nil + if let f = filterFromDate { draftFromDate = f } + if let t = filterToDate { draftToDate = t } + showFilterSheet = false } } .task { @@ -215,6 +223,10 @@ struct DateFilterSheet: View { @Binding var toDate: Date let onApply: () -> Void let onClear: () -> Void + /// Dismiss without touching the active filter. Distinct from `onClear`: + /// Cancel used to call that, so backing out of the sheet silently wiped + /// whatever date range was already applied and reloaded the list. + let onCancel: () -> Void var body: some View { NavigationStack { @@ -243,7 +255,7 @@ struct DateFilterSheet: View { .navigationBarTitleDisplayMode(.inline) .toolbar { ToolbarItem(placement: .cancellationAction) { - Button("Cancel") { onClear() } + Button("Cancel") { onCancel() } } ToolbarItem(placement: .confirmationAction) { Button("Apply", action: onApply)