feat(swift): add offline write outbox so local writes reach the server - #2673
Conversation
WalkthroughThe Swift app now persists offline pack, item, and trip mutations in SwiftData. ChangesOffline mutation persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ViewModel
participant SwiftData
participant OutboxService
participant PackTripService
participant Connectivity
ViewModel->>PackTripService: Submit mutation with local UUID
PackTripService-->>ViewModel: Return success or failure
ViewModel->>OutboxService: Enqueue failed or offline mutation
OutboxService->>SwiftData: Persist PendingMutation
Connectivity-->>OutboxService: Report restored connectivity
OutboxService->>SwiftData: Fetch queued mutations
OutboxService->>PackTripService: Replay mutation with preserved UUID
PackTripService-->>OutboxService: Return replay result
OutboxService->>SwiftData: Delete successful mutation or record failure
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Coverage Report for packages/utils (./packages/utils)
File CoverageNo changed files found. |
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Coverage Report for apps/expo (./apps/expo)
File CoverageNo changed files found. |
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Coverage Report for packages/api (./packages/api)
File CoverageNo changed files found. |
Deploying packrat-guides with
|
| Latest commit: |
26d8009
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://716cc8c2.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://worktree-issue-2672-offline.packrat-guides-6gq.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 26d8009 | Commit Preview URL Branch Preview URL |
Aug 09 2026, 02:49 PM |
Deploying packrat-landing with
|
| Latest commit: |
26d8009
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://145f493c.packrat-landing.pages.dev |
| Branch Preview URL: | https://worktree-issue-2672-offline.packrat-landing.pages.dev |
Writes made while offline were stranded on-device permanently: there was no outbox, no retry, and no path by which a locally-created pack or trip ever reached the server. Every reference to the `local-` id prefix was a guard that skipped the server rather than a push. Add a durable outbox: - `PendingMutation` SwiftData model — entity type, entity id, operation, optional parent id, JSON payload, attempt count and terminal-failure flag. - `OutboxService` drains the queue serially in `createdAt` order on reconnect, on foreground, and at launch, so create-then-update replays in the order the user made it. - Retire the `local-` prefix in Packs/Trips in favour of a plain client UUID. The API already accepts client-supplied ids on create, so an offline record is syncable as-is — no id reconciliation, no dangling child foreign keys. - Thread that id through `createPack`/`addItem`/`createTrip` so a replayed create can't produce a duplicate record. - Collapse redundant mutations on enqueue: create+delete of a never-synced entity cancels out, updates fold into a pending create, and consecutive updates collapse to the latest. - Terminal-failure policy: 4xx is not retried (the payload is wrong), 5xx and transport errors retry up to 5 attempts, then mark the mutation failed and keep it for the UI rather than dropping it. A 404 on delete and 409 on create count as success — the server already agrees. - Make delete consistent with create/update: a failed delete now stays deleted locally and flushes later instead of resurrecting the record. Refs #2672
…tions
Device testing on iPhone 17 found the outbox silently dropped every delete: a
pack deleted offline vanished from the UI but no PendingMutation row was ever
written, so the delete never reached the server on reconnect.
`OutboxService.enqueue` starts with `guard let context else { return }`, and the
view-model write methods take `context: ModelContext? = nil`. The create/update
form views passed a context, but the delete paths and two item paths relied on
the default nil — so exactly the writes that needed queuing were discarded.
Pass `modelContext` at the call sites that were missing it:
- `TripsListView` deleteTrip (context menu and swipe action)
- `PackDetailView` deleteItem and updateItem
- `CatalogView` addItem in AddCatalogItemToPackSheet
Verified end-to-end against a local API with a real signed-in account: a pack
created offline reaches the server on reconnect, and a pack deleted offline
enqueues (`pack|delete|48d5961a…`), flushes on reconnect, and leaves the queue
empty with no failed rows.
Refs #2672
26d8009 to
97b8bb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/swift/Sources/PackRat/Services/OutboxService.swift`:
- Around line 126-146: Update OutboxService replay at
apps/swift/Sources/PackRat/Services/OutboxService.swift#L126-L146 to track
mutation entity IDs returning .retry or .terminal and skip later queued
mutations whose parentId matches. Update the .delete branch at
apps/swift/Sources/PackRat/Services/OutboxService.swift#L60-L89 to fetch and
delete child PendingMutation rows with parentId equal to the deleted entityId,
ensuring dependent work is not replayed after cancellation.
- Around line 293-295: Update the outbox flush loop around the
PackRatError.unauthorized handling so authentication failures use a non-counting
outcome that preserves the queued mutation without incrementing attemptCount,
then stop draining because subsequent mutations will encounter the same expired
session. Keep the existing retry behavior for other transient failures and
retain the re-authentication message.
- Around line 279-301: Update classify(_:operation:) to cast the Error
existential to PackRatError before pattern matching, then switch on the typed
enum. Preserve all existing outcome logic for httpError, notFound, unauthorized,
decodingError, and the default retry path.
- Around line 296-306: Update decode<T> and its callers so missing or unreadable
stored payloads produce the appropriate terminal outcome directly at the decode
site, rather than being passed to classify’s server-response message. Preserve
retry classification for unrelated errors and ensure lastError uses local
payload wording.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03472f3a-ff62-49ed-bb8c-f78d9b2a3557
📒 Files selected for processing (12)
apps/swift/Sources/PackRat/Features/Catalog/CatalogView.swiftapps/swift/Sources/PackRat/Features/Packs/PackDetailView.swiftapps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swiftapps/swift/Sources/PackRat/Features/Trips/TripsListView.swiftapps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swiftapps/swift/Sources/PackRat/PackRatApp.swiftapps/swift/Sources/PackRat/Persistence/PendingMutation.swiftapps/swift/Sources/PackRat/Persistence/PersistenceController.swiftapps/swift/Sources/PackRat/Services/OutboxService.swiftapps/swift/Sources/PackRat/Services/PackService.swiftapps/swift/Sources/PackRat/Services/TripService.swiftapps/swift/Sources/PackRat/Shared/OutboxFlushModifier.swift
| for mutation in queued { | ||
| // Connectivity can drop mid-drain; stop and keep the rest queued. | ||
| guard NetworkMonitor.shared.isConnected else { break } | ||
| let outcome = await apply(mutation) | ||
| switch outcome { | ||
| case .success: | ||
| context.delete(mutation) | ||
| didSync = true | ||
| case .retry(let message): | ||
| mutation.attemptCount += 1 | ||
| mutation.lastError = message | ||
| if mutation.attemptCount >= Self.maxAttempts { | ||
| mutation.failed = true | ||
| } | ||
| case .terminal(let message): | ||
| mutation.attemptCount += 1 | ||
| mutation.lastError = message | ||
| mutation.failed = true | ||
| } | ||
| try? context.save() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The outbox ignores the parentId dependency between mutations. PendingMutation records parentId, but neither the cancel path nor the replay path uses it. Queued rows are treated as independent, so a pack item can outlive its pack or replay before its pack exists. Both sites below produce permanent failed rows for work the user expects to sync.
apps/swift/Sources/PackRat/Services/OutboxService.swift#L126-L146: track entities whose mutation returned.retryor.terminalin this pass, and skip any later mutation whoseparentIdis in that set, so a deferred pack create does not force its item creates into a terminal 404.apps/swift/Sources/PackRat/Services/OutboxService.swift#L60-L89: in the.deletebranch, also fetch and delete rows whoseparentIdequals the deletedentityId, so removing a pack cancels its queued item mutations instead of replaying them against a pack that never reached the server.
📍 Affects 1 file
apps/swift/Sources/PackRat/Services/OutboxService.swift#L126-L146(this comment)apps/swift/Sources/PackRat/Services/OutboxService.swift#L60-L89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/swift/Sources/PackRat/Services/OutboxService.swift` around lines 126 -
146, Update OutboxService replay at
apps/swift/Sources/PackRat/Services/OutboxService.swift#L126-L146 to track
mutation entity IDs returning .retry or .terminal and skip later queued
mutations whose parentId matches. Update the .delete branch at
apps/swift/Sources/PackRat/Services/OutboxService.swift#L60-L89 to fetch and
delete child PendingMutation rows with parentId equal to the deleted entityId,
ensuring dependent work is not replayed after cancellation.
| private func classify(_ error: Error, operation: OutboxOperation) -> Outcome { | ||
| switch error { | ||
| case PackRatError.httpError(let statusCode, let message): | ||
| // The server already agrees with our intent. | ||
| if operation == .delete, statusCode == 404 { return .success } | ||
| if operation == .create, statusCode == 409 { return .success } | ||
| // 4xx means the request itself is wrong — retrying can't fix it. | ||
| if (400...499).contains(statusCode) { | ||
| return .terminal(message ?? "Server rejected the change (\(statusCode))") | ||
| } | ||
| // 5xx is transient. | ||
| return .retry(message ?? "Server error (\(statusCode))") | ||
| case PackRatError.notFound: | ||
| return operation == .delete ? .success : .terminal("The item no longer exists on the server") | ||
| case PackRatError.unauthorized: | ||
| // Keep queued: the write should survive a re-auth. | ||
| return .retry("Sign-in required to sync") | ||
| case PackRatError.decodingError: | ||
| return .terminal("Could not read the server response") | ||
| default: | ||
| return .retry(error.localizedDescription) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the PackRatError declaration and look for precedent of switching over an existential Error.
fd -i 'packraterror|apierror|error' apps/swift/Sources --extension swift --exec cat -n {} \; | head -200
rg -n --type=swift -B2 -A6 'switch error' apps/swift/SourcesRepository: PackRat-AI/PackRat
Length of output: 9764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Swift files mentioning PackRatError =="
rg -n --type=swift 'enum PackRatError|typealias PackRatError|struct PackRatError|class PackRatError|import.*Error' apps/swift/Sources || true
echo
echo "== Switch/catch patterns with PackRatError =="
rg -n --type=swift -B3 -A8 'switch .*error|catch (PackRatError|[^,{}]+Error)|case PackRatError\.' apps/swift/Sources || true
echo
echo "== OutboxService classify snippet with surrounding context =="
sed -n '240,320p' apps/swift/Sources/PackRat/Services/OutboxService.swift
echo
echo "== PackRatError references == "
rg -n --type=swift 'PackRatError' apps/swift/SourcesRepository: PackRat-AI/PackRat
Length of output: 15169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== APIError.swift =="
cat -n apps/swift/Sources/PackRat/Models/APIError.swift
echo
echo "== Swift availability/tools =="
command -v swift || true
swift --version 2>/dev/null || true
echo
echo "== Swift switch enum-case pattern probe if swift is available =="
if command -v swift >/dev/null 2>&1; then
tmp="$(mktemp -d)"
cat > "$tmp/swiftswitch.swift" <<'EOF'
enum PackRatError: Error {
case httpError(statusCode: Int, message: String?)
case notFound
}
func classify(_ error: Error) -> String {
switch error {
case PackRatError.httpError(let statusCode, let message):
return "http \(statusCode) \(message ?? "")"
case PackRatError.notFound:
return "not found"
default:
return error.localizedDescription
}
}
EOF
swift "$tmp/swiftswitch.swift" 2>&1 || true
else
echo "swift compiler not available"
fiRepository: PackRat-AI/PackRat
Length of output: 1273
🌐 Web query:
Swift switch case enum pattern matching switch Error existential pattern matched directly Swift enum Error switch pattern case
💡 Result:
In Swift, you cannot directly use an enumeration case pattern to match against an existential type (like any Error) [1][2]. Because an existential type erases the concrete type information at compile time, the compiler cannot verify if the underlying value matches a specific enumeration case without a dynamic cast [1][3]. When you attempt to switch over an any Error value using an enum case pattern, the compiler will emit an error because enum case patterns require the value to be of the specific enum type [4][1]. To work around this, you must use a type-casting pattern [4]. There are two common approaches: 1. Type Casting in a Switch Case: You can cast the existential to a specific enum type within a case statement [5][6]. switch error { case let myError as MySpecificErrorEnum: // Handle specific enum cases here switch myError { case.someCase:... } case let otherError: // Handle other errors } 2. Casting in a Catch Block: When catching errors, you can use the same pattern-matching syntax to cast the error to your expected enum type [7][8]. do { try performAction } catch let error as MySpecificErrorEnum { // 'error' is now typed as MySpecificErrorEnum switch error { case.someCase:... } } catch { // Handle other errors } If you find yourself frequently switching over error existentials, you may want to consider using Swift's typed throws (introduced in later versions) if your API design allows it [9][10]. Typed throws (e.g., throws(MyError)) eliminate the need for existential boxing, allowing the compiler to know the exact error type and enabling exhaustive switch statements directly on the thrown error [9][10].
Citations:
- 1: https://forums.swift.org/t/can-the-compiler-be-more-helpful-when-we-need-to-explicitly-dynamically-specialize-existentials/60021
- 2: https://forums.swift.org/t/switching-on-protocols/82002
- 3: https://github.com/swiftlang/swift/blob/d06e05fc6d0f6827066e05d5c2ac40a6f23852d1/userdocs/diagnostics/existential-type.md
- 4: https://web.archive.org/web/20220612012016/https:/docs.swift.org/swift-book/ReferenceManual/Patterns.html
- 5: https://forums.swift.org/t/pattern-matching-and-binding-simultaneously/60574
- 6: [SR-7001] Error considered to conform to CustomStringConvertible in switch swiftlang/swift#49549
- 7: https://github.com/swiftlang/swift/blob/main/docs/ErrorHandlingRationale.md
- 8: https://forums.swift.org/t/idea-add-forced-conversion-for-error-catching-pattern-matching/1882
- 9: https://swiftcrafted.dev/article/swift-typed-throws-complete-guide-type-safe-error-handling
- 10: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0413-typed-throws.md
Cast error before case-matching PackRatError.
PackRatError is an Error existential here, so switch error { case PackRatError.httpError(...) } is not a valid switch pattern. Use a cast in the first case and then switch on the typed enum.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/swift/Sources/PackRat/Services/OutboxService.swift` around lines 279 -
301, Update classify(_:operation:) to cast the Error existential to PackRatError
before pattern matching, then switch on the typed enum. Preserve all existing
outcome logic for httpError, notFound, unauthorized, decodingError, and the
default retry path.
| case PackRatError.unauthorized: | ||
| // Keep queued: the write should survive a re-auth. | ||
| return .retry("Sign-in required to sync") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
An expired session burns the retry budget and discards offline work.
.unauthorized returns .retry, and every .retry increments attemptCount. Foreground activation, reconnect, and launch each trigger a flush. Five of those with an expired token mark the mutation failed, even though nothing is wrong with the payload. The comment states the write should survive a re-auth, but it does not.
Do not charge auth failures against the attempt budget.
🐛 Proposed fix: add a non-counting outcome
private enum Outcome {
case success
/// Transport-level failure — worth another attempt later.
case retry(String)
+ /// Blocked by something outside the payload (for example a missing
+ /// session). Requeue without consuming an attempt.
+ case deferred(String)
/// Server rejected the payload; retrying can't fix it.
case terminal(String)
} case PackRatError.unauthorized:
// Keep queued: the write should survive a re-auth.
- return .retry("Sign-in required to sync")
+ return .deferred("Sign-in required to sync")Handle it in the flush loop, and stop the drain because every later mutation will hit the same wall:
case .retry(let message):
mutation.attemptCount += 1
mutation.lastError = message
if mutation.attemptCount >= Self.maxAttempts {
mutation.failed = true
}
+ case .deferred(let message):
+ mutation.lastError = message
+ try? context.save()
+ return didSync🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/swift/Sources/PackRat/Services/OutboxService.swift` around lines 293 -
295, Update the outbox flush loop around the PackRatError.unauthorized handling
so authentication failures use a non-counting outcome that preserves the queued
mutation without incrementing attemptCount, then stop draining because
subsequent mutations will encounter the same expired session. Keep the existing
retry behavior for other transient failures and retain the re-authentication
message.
| case PackRatError.decodingError: | ||
| return .terminal("Could not read the server response") | ||
| default: | ||
| return .retry(error.localizedDescription) | ||
| } | ||
| } | ||
|
|
||
| private func decode<T: Decodable>(_ data: Data?) throws -> T { | ||
| guard let data else { throw PackRatError.decodingError(OutboxError.missingPayload) } | ||
| return try JSONDecoder().decode(T.self, from: data) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The error text blames the server for a local payload problem.
decode throws PackRatError.decodingError when the stored payload is missing or unreadable. classify then reports "Could not read the server response". That text is shown to the user through lastError, and it points at the wrong cause.
Return the terminal outcome at the decode site instead of routing a local failure through the server-response branch.
🐛 Proposed fix
case PackRatError.decodingError:
return .terminal("Could not read the server response")
+ case OutboxError.missingPayload:
+ return .terminal("The queued change is missing its data") private func decode<T: Decodable>(_ data: Data?) throws -> T {
- guard let data else { throw PackRatError.decodingError(OutboxError.missingPayload) }
+ guard let data else { throw OutboxError.missingPayload }
return try JSONDecoder().decode(T.self, from: data)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case PackRatError.decodingError: | |
| return .terminal("Could not read the server response") | |
| default: | |
| return .retry(error.localizedDescription) | |
| } | |
| } | |
| private func decode<T: Decodable>(_ data: Data?) throws -> T { | |
| guard let data else { throw PackRatError.decodingError(OutboxError.missingPayload) } | |
| return try JSONDecoder().decode(T.self, from: data) | |
| } | |
| case PackRatError.decodingError: | |
| return .terminal("Could not read the server response") | |
| case OutboxError.missingPayload: | |
| return .terminal("The queued change is missing its data") | |
| default: | |
| return .retry(error.localizedDescription) | |
| } | |
| } | |
| private func decode<T: Decodable>(_ data: Data?) throws -> T { | |
| guard let data else { throw OutboxError.missingPayload } | |
| return try JSONDecoder().decode(T.self, from: data) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/swift/Sources/PackRat/Services/OutboxService.swift` around lines 296 -
306, Update decode<T> and its callers so missing or unreadable stored payloads
produce the appropriate terminal outcome directly at the decode site, rather
than being passed to classify’s server-response message. Preserve retry
classification for unrelated errors and ensure lastError uses local payload
wording.
Fixes #2672.
Problem
The Swift app is offline-first only on the read path. Writes made while offline were stranded on-device permanently — no outbox, no retry, and no path by which a locally-created pack or trip ever reached the server. Every reference to the
local-id prefix was a guard that skipped the server rather than a push.Delete was also inconsistent with create/update in the same file:
deletePackrestored the removed pack and rethrew ("Couldn't Delete Pack"), whilecreatePackunder identical conditions silently succeeded locally.Approach
The API already accepts client-supplied ids on create (
packages/api/src/routes/packs/index.tsinsertsdata.id), so the only reason offline records were unsyncable is that the offline path minted a deliberately-poisonedlocal-id instead of reusing the same client UUID scheme. Dropping the prefix removes the id-reconciliation problem entirely.Changes
PendingMutation(new SwiftData model) — entity type, entity id, operation, optional parent id, JSON payload,attemptCount,lastError,failed. Registered in the container, so queued writes survive kill/relaunch.OutboxService(new) — drains the queue serially increatedAtorder, so create-then-update on the same entity replays in the order the user made it.local-prefix in Packs/Trips for a plain client UUID, and threaded that id throughcreatePack/addItem/createTripso a replayed create can't produce a duplicate record.failedand keep it for the UI rather than dropping it silently. 404-on-delete and 409-on-create count as success — the server already agrees with local state.deletePackanddeleteItem..flushesPendingWrites()on the root view flushes at launch, onNetworkMonitor.isConnectedchange, and on foreground.Acceptance criteria
deletePackandcreatePackbehave consistently when the network is unavailableDecisions on the issue's open questions
Out of scope
The
local-guards inPackTemplatesViewModelandTrailConditionsViewModelare left alone — they mint and guard consistently within their own files and are outside this issue's Packs/Trips scope. Extending caching to the remaining 18 features is likewise separate.Test plan
Launch with
--force-offline, create/edit/delete a pack and a trip, force-quit and relaunch, then remove the flag and confirm all writes land server-side on reconnect.Summary by CodeRabbit