diff --git a/apps/expo/features/guides/screens/GuidesListScreen.tsx b/apps/expo/features/guides/screens/GuidesListScreen.tsx index bb61859536..65b420e7d6 100644 --- a/apps/expo/features/guides/screens/GuidesListScreen.tsx +++ b/apps/expo/features/guides/screens/GuidesListScreen.tsx @@ -1,4 +1,5 @@ import { getAppBarOptions } from '@packrat/ui/src/app-bar'; +import { Button } from '@packrat/ui/src/button'; import { IosTransparentHeaderOverlapFix } from '@packrat/ui/src/ios-transparent-header-overlap-fix'; import { SearchOverlay } from '@packrat/ui/src/search-overlay'; import { Text } from '@packrat/ui/src/text'; @@ -29,6 +30,8 @@ export const GuidesListScreen = () => { const { data: guidesData, isLoading: isLoadingGuides, + isError: isErrorGuides, + error: guidesError, refetch: refetchGuides, fetchNextPage: fetchNextPageGuides, hasNextPage: hasNextPageGuides, @@ -43,6 +46,8 @@ export const GuidesListScreen = () => { const { data: searchData, isLoading: isSearching, + isError: isErrorSearch, + error: searchError, refetch: refetchSearch, fetchNextPage: fetchNextPageSearch, hasNextPage: hasNextPageSearch, @@ -58,6 +63,8 @@ export const GuidesListScreen = () => { const isSearchMode = searchQuery.length > 0; const data = isSearchMode ? searchData : guidesData; const isLoading = isSearchMode ? isSearching : isLoadingGuides; + const isError = isSearchMode ? isErrorSearch : isErrorGuides; + const error = isSearchMode ? searchError : guidesError; const refetch = isSearchMode ? refetchSearch : refetchGuides; const fetchNextPage = isSearchMode ? fetchNextPageSearch : fetchNextPageGuides; const hasNextPage = isSearchMode ? hasNextPageSearch : hasNextPageGuides; @@ -102,17 +109,40 @@ export const GuidesListScreen = () => { }; const renderEmpty = () => { - return ( - - {isLoading ? ( + if (isLoading) { + return ( + - ) : ( - - {isSearchMode - ? t('guides.noGuidesFound', { query: searchQuery }) - : t('guides.noGuidesAvailable')} + + ); + } + + // A failed request leaves `guides` empty too. Without this branch the list + // renders "No guides available", which reads as "there is no content" when + // the real problem is that the fetch failed (offline, expired session, 5xx). + if (isError) { + return ( + + + {t('guides.failedToLoadGuides')} - )} + + {error?.message || t('guides.pleaseTryAgain')} + + + + ); + } + + return ( + + + {isSearchMode + ? t('guides.noGuidesFound', { query: searchQuery }) + : t('guides.noGuidesAvailable')} + ); }; @@ -152,13 +182,7 @@ export const GuidesListScreen = () => { ) : null } - ListEmptyComponent={ - - - {t('guides.noGuidesFound', { query: searchQuery })} - - - } + ListEmptyComponent={renderEmpty()} ListFooterComponent={ isFetchingNextPageSearch ? ( diff --git a/apps/expo/lib/i18n/locales/en.json b/apps/expo/lib/i18n/locales/en.json index bd4d5ca8f1..9f2a2cb294 100644 --- a/apps/expo/lib/i18n/locales/en.json +++ b/apps/expo/lib/i18n/locales/en.json @@ -1059,6 +1059,9 @@ "noGuidesFound": "No guides found for \"{{query}}\"", "noGuidesAvailable": "No guides available", "failedToLoad": "Failed to load guide", + "failedToLoadGuides": "Failed to load guides", + "pleaseTryAgain": "Please check your connection and try again.", + "tryAgain": "Try Again", "viewAll": "View all", "browseGuides": "Browse helpful guides and tutorials", "by": "By", diff --git a/apps/landing/app/support/page.tsx b/apps/landing/app/support/page.tsx new file mode 100644 index 0000000000..671b9d68c5 --- /dev/null +++ b/apps/landing/app/support/page.tsx @@ -0,0 +1,138 @@ +import { siteConfig } from 'landing-app/config/site'; +import Link from 'next/link'; + +export const metadata = { + title: 'Support | PackRat', + description: + 'Get help with PackRat — the mobile app, your account, and the Claude and ChatGPT connectors.', +}; + +export default function SupportPage() { + return ( +
+
+
+

Support

+

+ Need a hand with PackRat? Here's how to reach us and where to find answers. +

+
+ +
+

Contact us

+ +
+
+

Email

+

+ For any question, bug report, or feedback, email{' '} + + {siteConfig.support.email} + + . We aim to reply within two business days. +

+
+ +
+

What to include

+

To help us resolve things on the first reply, please tell us:

+
    +
  • What you were trying to do, and what happened instead
    • +
  • Where it happened — the iOS app, the web app, or a connector
    • +
  • The email address on your PackRat account
    • +
  • A screenshot, if the problem is something you can see
    • +
+
+
+
+ +
+

Connectors

+

+ PackRat works inside Claude and ChatGPT, so you can plan trips, build packing lists, and + search gear without leaving the conversation. +

+
    +
  • + + Connector setup and documentation + {' '} + — how to connect, what it can do, and the tools it exposes +
    • +
  • + Connecting requires a free PackRat account. If sign-in fails, first confirm the email + and password work in the app itself, then email us. +
    • +
  • + A connector only ever reads and writes your own PackRat data plus the public gear + catalog. Nothing is shared with other users. +
    • +
+
+ +
+

Common questions

+ +
+
+

I forgot my password

+

+ Use Forgot password on the sign-in screen to get a reset link. If + it doesn't arrive, check your spam folder before contacting us. +

+
+ +
+

My packs aren't syncing

+

+ PackRat saves changes locally first and syncs when you're back online. Confirm + you have a connection and that you're signed in to the same account on both + devices. If a pack is still missing after that, email us and we'll investigate. +

+
+ +
+

Weather looks wrong or missing

+

+ Forecasts cover the near term, so dates far in the future won't return a + forecast. For a trip months out, expect seasonal guidance rather than a daily + forecast. +

+
+ +
+

A gear item is wrong or missing

+

+ The catalog is large and sourced from manufacturers and retailers, so specs can + drift. Email us the product name and what's incorrect and we'll get it + fixed. +

+
+
+
+ +
+

Account and privacy

+
    +
  • + + Delete your account + {' '} + — steps and what happens to your data +
    • +
  • + + Privacy policy + +
    • +
  • + + Terms of service + +
    • +
+
+
+
+ ); +} diff --git a/apps/landing/config/site.ts b/apps/landing/config/site.ts index 3216b8ac2d..a8a188619b 100644 --- a/apps/landing/config/site.ts +++ b/apps/landing/config/site.ts @@ -366,8 +366,9 @@ export const siteConfig = { ], }, - // Support contact — surfaced from MCP /health, the login page, and the connector listing. - // Email is the canonical channel; we don't run a separate support web page yet. + // Support contact — surfaced from MCP /health, the login page, the connector + // listing, and the /support page. Email is the canonical channel; render + // these values rather than hardcoding an address. support: { email: 'hello@packratai.com', mailto: 'mailto:hello@packratai.com', diff --git a/apps/landing/public/_headers b/apps/landing/public/_headers index 2d74e7653b..24c30d926f 100644 --- a/apps/landing/public/_headers +++ b/apps/landing/public/_headers @@ -3,3 +3,11 @@ X-Content-Type-Options: nosniff Referrer-Policy: strict-origin-when-cross-origin Permissions-Policy: camera=(), microphone=(), geolocation=() + +# Demo recordings linked from the Claude / OpenAI connector submissions. +# Serve inline so a reviewer can play the video in-browser instead of +# downloading it, and cache hard since the filenames are versioned by content. +/demo/* + Content-Type: video/mp4 + Content-Disposition: inline + Cache-Control: public, max-age=31536000, immutable diff --git a/apps/landing/public/demo/packrat-chatgpt-demo.mp4 b/apps/landing/public/demo/packrat-chatgpt-demo.mp4 new file mode 100644 index 0000000000..f5154c0ece Binary files /dev/null and b/apps/landing/public/demo/packrat-chatgpt-demo.mp4 differ diff --git a/apps/swift/Sources/PackRat/Features/Catalog/CatalogView.swift b/apps/swift/Sources/PackRat/Features/Catalog/CatalogView.swift index 38315f6bf3..7b7b5b7ccc 100644 --- a/apps/swift/Sources/PackRat/Features/Catalog/CatalogView.swift +++ b/apps/swift/Sources/PackRat/Features/Catalog/CatalogView.swift @@ -1,5 +1,6 @@ import SwiftUI import NukeUI +import SwiftData struct CatalogView: View { @Environment(AppState.self) private var appState @@ -254,6 +255,7 @@ struct AddCatalogItemToPackSheet: View { let item: CatalogItem let packsViewModel: PacksViewModel @Environment(\.dismiss) private var dismiss + @Environment(\.modelContext) private var modelContext @State private var selectedPackId: String? @State private var quantity = 1 @@ -325,7 +327,8 @@ struct AddCatalogItemToPackSheet: View { category: item.categories?.first, consumable: false, worn: false, - notes: nil + notes: nil, + context: modelContext ) success = true Task { diff --git a/apps/swift/Sources/PackRat/Features/Guides/GuidesView.swift b/apps/swift/Sources/PackRat/Features/Guides/GuidesView.swift index fb40b92b35..372d5facf1 100644 --- a/apps/swift/Sources/PackRat/Features/Guides/GuidesView.swift +++ b/apps/swift/Sources/PackRat/Features/Guides/GuidesView.swift @@ -7,18 +7,34 @@ struct Guide: Codable, Identifiable, Sendable { let id: String let title: String let content: String? - let excerpt: String? + /// The API calls this `description`; the UI shows it as the row excerpt. + let description: String? + /// Coarse bucket — currently `"general"` for every guide. let category: String? - let imageUrl: String? + /// The tags the /guides/categories endpoint actually returns; this is what + /// the category filter must match against, not `category`. + let categories: [String]? + let author: String? + let difficulty: String? let createdAt: String? + + var excerpt: String? { description } } +/// Mirrors `GuidesResponseSchema` in packages/schemas/src/guides.ts: +/// `{ items, totalCount, page, limit, totalPages }`. struct GuidesResponse: Codable { - let guides: [Guide]? - let data: [Guide]? - let total: Int? + let items: [Guide] + let totalCount: Int? + let page: Int? + let limit: Int? + let totalPages: Int? +} - var items: [Guide] { guides ?? data ?? [] } +/// Mirrors `GuideCategoriesResponseSchema`: `{ categories, count }`. +struct GuideCategoriesResponse: Codable { + let categories: [String] + let count: Int? } // MARK: - Service @@ -33,10 +49,9 @@ final class GuidesService: Sendable { var query: [String: String] = ["page": "\(page)", "limit": "\(limit)"] if let cat = category { query["category"] = cat } let endpoint = Endpoint(.get, "/api/guides", query: query) - if let wrapped = try? await api.send(endpoint, as: GuidesResponse.self) { - return wrapped.items - } - return try await api.send(endpoint) + // Errors propagate: a failed fetch must surface as an error state, not + // as an empty list indistinguishable from "there are no guides". + return try await api.send(endpoint, as: GuidesResponse.self).items } func getGuide(_ id: String) async throws -> Guide { @@ -46,8 +61,7 @@ final class GuidesService: Sendable { func categories() async throws -> [String] { let endpoint = Endpoint(.get, "/api/guides/categories") - if let arr = try? await api.send(endpoint, as: [String].self) { return arr } - return [] + return try await api.send(endpoint, as: GuideCategoriesResponse.self).categories } } @@ -67,7 +81,9 @@ final class GuidesViewModel { var filteredGuides: [Guide] { var result = guides - if let cat = selectedCategory { result = result.filter { $0.category == cat } } + if let cat = selectedCategory { + result = result.filter { $0.categories?.contains(cat) == true || $0.category == cat } + } if !searchText.isEmpty { result = result.filter { $0.title.localizedCaseInsensitiveContains(searchText) || @@ -97,13 +113,18 @@ final class GuidesViewModel { isLoading = true error = nil defer { isLoading = false } + + // The category filter is a convenience; losing it must not blank the + // list, so its failure is kept separate from the guides request. + async let fetchedCategories = try? service.categories() + do { - async let g = service.listGuides() - async let c = service.categories() - (guides, categories) = try await (g, c) + guides = try await service.listGuides() } catch { self.error = error.localizedDescription } + + categories = await fetchedCategories ?? [] } func loadMore() async { @@ -157,24 +178,18 @@ struct GuidesView: View { } private var categoryBar: some View { - HStack { - Picker("Category", selection: $viewModel.selectedCategory) { - Label("All", systemImage: "line.3.horizontal.decrease.circle") - .tag(nil as String?) - ForEach(viewModel.categories, id: \.self) { cat in - Label(cat.capitalized, systemImage: "tag") - .tag(Optional(cat)) - } + // A .menu Picker already renders its own label and the selected value, + // so no trailing Text — that duplicated the selection a third time. + Picker("Category", selection: $viewModel.selectedCategory) { + Label("All", systemImage: "line.3.horizontal.decrease.circle") + .tag(nil as String?) + ForEach(viewModel.categories, id: \.self) { cat in + Label(cat.capitalized, systemImage: "tag") + .tag(Optional(cat)) } - .pickerStyle(.menu) - .accessibilityIdentifier("guides_category_filter") - - Spacer() - - Text(viewModel.selectedCategory?.capitalized ?? "All") - .font(.subheadline) - .foregroundStyle(.secondary) } + .pickerStyle(.menu) + .accessibilityIdentifier("guides_category_filter") .padding(.vertical, 2) } @@ -183,7 +198,7 @@ struct GuidesView: View { if !viewModel.categories.isEmpty { Section { categoryBar - .listRowInsets(EdgeInsets(top: 8, leading: 0, bottom: 8, trailing: 0)) + .listRowInsets(EdgeInsets(top: 8, leading: 16, bottom: 8, trailing: 16)) .listRowSeparator(.hidden) } } diff --git a/apps/swift/Sources/PackRat/Features/Packs/PackDetailView.swift b/apps/swift/Sources/PackRat/Features/Packs/PackDetailView.swift index bd07985c52..c8b203ad93 100644 --- a/apps/swift/Sources/PackRat/Features/Packs/PackDetailView.swift +++ b/apps/swift/Sources/PackRat/Features/Packs/PackDetailView.swift @@ -226,7 +226,8 @@ struct PackDetailView: View { category: category == "Uncategorized" ? nil : category, consumable: item.consumable, worn: item.worn, - notes: item.notes + notes: item.notes, + context: modelContext ) } catch { self.error = error.localizedDescription diff --git a/apps/swift/Sources/PackRat/Features/Packs/PackFormView.swift b/apps/swift/Sources/PackRat/Features/Packs/PackFormView.swift index 5ded3bc9e3..919569bf84 100644 --- a/apps/swift/Sources/PackRat/Features/Packs/PackFormView.swift +++ b/apps/swift/Sources/PackRat/Features/Packs/PackFormView.swift @@ -108,7 +108,8 @@ struct PackFormView: View { context: modelContext ) } else { - try await viewModel.createPack( + // Create never throws — offline writes queue for replay. + await viewModel.createPack( name: name.trimmingCharacters(in: .whitespaces), description: description.isEmpty ? nil : description, category: category.isEmpty ? nil : category, diff --git a/apps/swift/Sources/PackRat/Features/Packs/PacksListView.swift b/apps/swift/Sources/PackRat/Features/Packs/PacksListView.swift index 3aacc7bffa..5c205db29a 100644 --- a/apps/swift/Sources/PackRat/Features/Packs/PacksListView.swift +++ b/apps/swift/Sources/PackRat/Features/Packs/PacksListView.swift @@ -13,7 +13,6 @@ struct PacksListView: View { @State private var isLoadingPublic = false @State private var packPendingDeletion: Pack? @State private var showingDeleteConfirmation = false - @State private var deleteError: String? @Environment(\.modelContext) private var modelContext #if os(iOS) @Environment(\.horizontalSizeClass) private var horizontalSizeClass @@ -97,17 +96,6 @@ struct PacksListView: View { .sheet(isPresented: $showingCreateSheet) { PackFormView(viewModel: viewModel) } - .alert( - "Couldn't Delete Pack", - isPresented: Binding( - get: { deleteError != nil }, - set: { if !$0 { deleteError = nil } } - ) - ) { - Button("OK", role: .cancel) { deleteError = nil } - } message: { - Text(deleteError ?? "") - } .navigationDestination(isPresented: $showingRecentPacks) { RecentPacksView(packs: viewModel.packs) } @@ -235,12 +223,10 @@ struct PacksListView: View { private func deletePack(_ pack: Pack) { packPendingDeletion = nil Task { - do { - try await viewModel.deletePack(pack.id, context: modelContext) - if selectedId == pack.id { selectedId = nil } - } catch { - deleteError = error.localizedDescription - } + // Deleting never fails at the call site — an unreachable server queues the + // delete for replay. Failures surface via the pending-writes banner. + await viewModel.deletePack(pack.id, context: modelContext) + if selectedId == pack.id { selectedId = nil } } } diff --git a/apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift b/apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift index feedbb65b8..314f4407b7 100644 --- a/apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift +++ b/apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift @@ -12,9 +12,11 @@ final class PacksViewModel { var searchText = "" let service: PackService + private let outbox: OutboxService - init(service: PackService = .shared) { + init(service: PackService = .shared, outbox: OutboxService = .shared) { self.service = service + self.outbox = outbox } var currentPage = 1 @@ -48,6 +50,9 @@ final class PacksViewModel { } if let context, !isCacheLoaded { + // Clear rows from the retired `local-` id scheme before reading the cache, + // so they never reach the UI or the outbox. + LegacyLocalIDMigration.runIfNeeded(context: context) let cached = (try? context.fetch(FetchDescriptor( sortBy: [SortDescriptor(\.cachedAt, order: .reverse)] ))) ?? [] @@ -123,27 +128,47 @@ final class PacksViewModel { try? context.save() } + /// Never throws: an unreachable server queues the create for replay instead of + /// failing at the call site. Surfaced failures come from `OutboxService.failedCount`. func createPack( name: String, description: String?, category: String?, isPublic: Bool, context: ModelContext? = nil - ) async throws { + ) async { let localPack = makeLocalPack(name: name, description: description, category: category, isPublic: isPublic) + let payload = PackMutationPayload( + name: name, description: description, category: category, isPublic: isPublic + ) + + func queueCreate() { + outbox.enqueue( + entityType: .pack, + entityId: localPack.id, + operation: .create, + payload: OutboxService.encode(payload), + context: context + ) + } + guard canUseRemotePersonalStore else { packs.insert(localPack, at: 0) upsertCachedPack(localPack, context: context) + queueCreate() return } let pack: Pack do { + // Send the local id so a retry can't create a duplicate record. pack = try await service.createPack( - name: name, description: description, category: category, isPublic: isPublic + id: localPack.id, name: name, description: description, + category: category, isPublic: isPublic ) } catch { pack = localPack + queueCreate() } packs.insert(pack, at: 0) upsertCachedPack(pack, context: context) @@ -167,6 +192,19 @@ final class PacksViewModel { updatedAt: Date.iso8601Now() ) + let payload = PackMutationPayload( + name: name, description: description, category: category, isPublic: isPublic + ) + func queueUpdate() { + outbox.enqueue( + entityType: .pack, + entityId: packId, + operation: .update, + payload: OutboxService.encode(payload), + context: context + ) + } + let updated: Pack if canUseRemotePersonalStore { do { @@ -175,9 +213,11 @@ final class PacksViewModel { ) } catch { updated = localUpdated + queueUpdate() } } else { updated = localUpdated + queueUpdate() } if let idx = packs.firstIndex(where: { $0.id == packId }) { packs[idx] = updated @@ -185,19 +225,31 @@ final class PacksViewModel { upsertCachedPack(updated, context: context) } - // Optimistic delete: remove immediately, restore on error - func deletePack(_ packId: String, context: ModelContext? = nil) async throws { + /// Optimistic delete. The removal always sticks locally — an unreachable server + /// queues the delete for replay rather than resurrecting the pack, so delete now + /// behaves like create and update. Never throws, for the same reason. + func deletePack(_ packId: String, context: ModelContext? = nil) async { guard let idx = packs.firstIndex(where: { $0.id == packId }) else { return } - let removed = packs.remove(at: idx) + packs.remove(at: idx) deleteCachedPack(packId, context: context) - guard !packId.hasPrefix("local-") else { return } - guard canUseRemotePersonalStore else { return } + + func queueDelete() { + outbox.enqueue( + entityType: .pack, + entityId: packId, + operation: .delete, + context: context + ) + } + + guard canUseRemotePersonalStore else { + queueDelete() + return + } do { try await service.deletePack(packId) } catch { - packs.insert(removed, at: idx) - upsertCachedPack(removed, context: context) - throw error + queueDelete() } } @@ -208,18 +260,35 @@ final class PacksViewModel { packId: packId, name: name, weight: weight, weightUnit: weightUnit, quantity: quantity, category: category, consumable: consumable, worn: worn, notes: notes ) + let payload = PackItemMutationPayload( + name: name, weight: weight, weightUnit: weightUnit, quantity: quantity, + category: category, consumable: consumable, worn: worn, notes: notes + ) + func queueCreate() { + outbox.enqueue( + entityType: .packItem, + entityId: localItem.id, + operation: .create, + parentId: packId, + payload: OutboxService.encode(payload), + context: context + ) + } + let item: PackItem if canUseRemotePersonalStore { do { item = try await service.addItem( - to: packId, name: name, weight: weight, weightUnit: weightUnit, + to: packId, id: localItem.id, name: name, weight: weight, weightUnit: weightUnit, quantity: quantity, category: category, consumable: consumable, worn: worn, notes: notes ) } catch { item = localItem + queueCreate() } } else { item = localItem + queueCreate() } if let idx = packs.firstIndex(where: { $0.id == packId }) { var items = packs[idx].items ?? [] @@ -248,6 +317,27 @@ final class PacksViewModel { worn: worn, notes: notes ?? current?.notes ) + let payload = PackItemMutationPayload( + name: name, + weight: weight ?? current?.weight, + weightUnit: weightUnit ?? current?.weightUnit.rawValue, + quantity: quantity ?? current?.quantity, + category: category ?? current?.category, + consumable: consumable, + worn: worn, + notes: notes ?? current?.notes + ) + func queueUpdate() { + outbox.enqueue( + entityType: .packItem, + entityId: itemId, + operation: .update, + parentId: packId, + payload: OutboxService.encode(payload), + context: context + ) + } + let updated: PackItem if canUseRemotePersonalStore { do { @@ -257,9 +347,11 @@ final class PacksViewModel { ) } catch { updated = localUpdated + queueUpdate() } } else { updated = localUpdated + queueUpdate() } var items = packs[packIdx].items ?? [] items[itemIdx] = updated @@ -268,32 +360,43 @@ final class PacksViewModel { } } - // Optimistic item delete + /// Optimistic item delete. Like `deletePack`, a failed or offline delete stays + /// deleted locally and is queued for replay instead of being rolled back. func deleteItem(_ itemId: String, from packId: String, context: ModelContext? = nil) async throws { guard let packIdx = packs.firstIndex(where: { $0.id == packId }), let itemIdx = packs[packIdx].items?.firstIndex(where: { $0.id == itemId }) else { return } var items = packs[packIdx].items ?? [] - let removed = items.remove(at: itemIdx) + items.remove(at: itemIdx) packs[packIdx] = rebuildPack(packs[packIdx], items: items) upsertCachedPack(packs[packIdx], context: context) - guard canUseRemotePersonalStore else { return } + + func queueDelete() { + outbox.enqueue( + entityType: .packItem, + entityId: itemId, + operation: .delete, + parentId: packId, + context: context + ) + } + + guard canUseRemotePersonalStore else { + queueDelete() + return + } do { try await service.deleteItem(itemId, from: packId) } catch { - var restored = packs[packIdx].items ?? [] - restored.insert(removed, at: itemIdx) - if let idx = packs.firstIndex(where: { $0.id == packId }) { - packs[idx] = rebuildPack(packs[idx], items: restored) - upsertCachedPack(packs[idx], context: context) - } - throw error + queueDelete() } } private func makeLocalPack(name: String, description: String?, category: String?, isPublic: Bool) -> Pack { let now = Date.iso8601Now() + // A plain client UUID — the same scheme the server accepts on create — so an + // offline pack is syncable as-is. No `local-` prefix to reconcile later. return Pack( - id: "local-\(UUID().uuidString)", userId: nil, name: name, + id: UUID().uuidString.lowercased(), userId: nil, name: name, description: description, category: PackCategory(rawValue: category ?? ""), isPublic: isPublic, image: nil, tags: nil, templateId: nil, deleted: false, isAIGenerated: false, items: [], @@ -303,7 +406,7 @@ final class PacksViewModel { } private func makeLocalItem( - id: String = "local-item-\(UUID().uuidString)", + id: String = UUID().uuidString.lowercased(), packId: String, name: String, weight: Double?, diff --git a/apps/swift/Sources/PackRat/Features/Trips/TripFormView.swift b/apps/swift/Sources/PackRat/Features/Trips/TripFormView.swift index b43d76fe17..f577a2914c 100644 --- a/apps/swift/Sources/PackRat/Features/Trips/TripFormView.swift +++ b/apps/swift/Sources/PackRat/Features/Trips/TripFormView.swift @@ -202,7 +202,8 @@ struct TripFormView: View { context: modelContext ) } else { - try await viewModel.createTrip( + // Create never throws — offline writes queue for replay. + await viewModel.createTrip( name: name, description: description.isEmpty ? nil : description, startDate: hasDates ? startDate : nil, endDate: hasDates ? endDate : nil, diff --git a/apps/swift/Sources/PackRat/Features/Trips/TripsListView.swift b/apps/swift/Sources/PackRat/Features/Trips/TripsListView.swift index 0f06762fe9..c589972769 100644 --- a/apps/swift/Sources/PackRat/Features/Trips/TripsListView.swift +++ b/apps/swift/Sources/PackRat/Features/Trips/TripsListView.swift @@ -114,12 +114,12 @@ struct TripsListView: View { Divider() #endif Button("Delete", systemImage: "trash", role: .destructive) { - Task { try? await viewModel.deleteTrip(trip.id) } + Task { await viewModel.deleteTrip(trip.id, context: modelContext) } } } .swipeActions(edge: .trailing) { Button(role: .destructive) { - Task { try? await viewModel.deleteTrip(trip.id) } + Task { await viewModel.deleteTrip(trip.id, context: modelContext) } } label: { Label("Delete", systemImage: "trash") } diff --git a/apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift b/apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift index 20edd2ed26..db78ae2ed2 100644 --- a/apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift +++ b/apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift @@ -12,9 +12,11 @@ final class TripsViewModel { var searchText = "" private let service: TripService + private let outbox: OutboxService - init(service: TripService = .shared) { + init(service: TripService = .shared, outbox: OutboxService = .shared) { self.service = service + self.outbox = outbox } var currentPage = 1 @@ -60,6 +62,9 @@ final class TripsViewModel { } if let context, !isCacheLoaded { + // Clear rows from the retired `local-` id scheme before reading the cache, + // so they never reach the UI or the outbox. + LegacyLocalIDMigration.runIfNeeded(context: context) let cached = (try? context.fetch(FetchDescriptor( sortBy: [SortDescriptor(\.cachedAt, order: .reverse)] ))) ?? [] @@ -132,25 +137,52 @@ final class TripsViewModel { try? context.save() } + /// Never throws: an unreachable server queues the create for replay instead of + /// failing at the call site. Surfaced failures come from `OutboxService.failedCount`. func createTrip(name: String, description: String?, startDate: Date?, endDate: Date?, location: TripLocationBody?, notes: String?, packId: String?, - context: ModelContext? = nil) async throws { + context: ModelContext? = nil) async { let localTrip = makeLocalTrip( name: name, description: description, startDate: startDate, endDate: endDate, location: location, notes: notes, packId: packId ) + let payload = TripMutationPayload( + name: name, + description: description, + startDate: startDate?.iso8601String(), + endDate: endDate?.iso8601String(), + latitude: location?.latitude, + longitude: location?.longitude, + locationName: location?.name, + notes: notes, + packId: packId + ) + func queueCreate() { + outbox.enqueue( + entityType: .trip, + entityId: localTrip.id, + operation: .create, + payload: OutboxService.encode(payload), + context: context + ) + } + let trip: Trip if canUseRemotePersonalStore { do { + // Send the local id so a retry can't create a duplicate record. trip = try await service.createTrip( + id: localTrip.id, name: name, description: description, startDate: startDate, endDate: endDate, location: location, notes: notes, packId: packId ) } catch { trip = localTrip + queueCreate() } } else { trip = localTrip + queueCreate() } trips.insert(trip, at: 0) upsertCachedTrip(trip, context: context) @@ -171,6 +203,27 @@ final class TripsViewModel { packId: packId, updatedAt: Date.iso8601Now() ) + let payload = TripMutationPayload( + name: name, + description: description, + startDate: startDate?.iso8601String(), + endDate: endDate?.iso8601String(), + latitude: location?.latitude, + longitude: location?.longitude, + locationName: location?.name, + notes: notes, + packId: packId + ) + func queueUpdate() { + outbox.enqueue( + entityType: .trip, + entityId: tripId, + operation: .update, + payload: OutboxService.encode(payload), + context: context + ) + } + let updated: Trip if canUseRemotePersonalStore { do { @@ -180,9 +233,11 @@ final class TripsViewModel { ) } catch { updated = localUpdated + queueUpdate() } } else { updated = localUpdated + queueUpdate() } if let idx = trips.firstIndex(where: { $0.id == tripId }) { trips[idx] = updated @@ -190,19 +245,30 @@ final class TripsViewModel { upsertCachedTrip(updated, context: context) } - // Optimistic delete - func deleteTrip(_ tripId: String, context: ModelContext? = nil) async throws { + /// Optimistic delete. An unreachable server queues the delete for replay rather + /// than resurrecting the trip, matching create/update behaviour. Never throws. + func deleteTrip(_ tripId: String, context: ModelContext? = nil) async { guard let idx = trips.firstIndex(where: { $0.id == tripId }) else { return } - let removed = trips.remove(at: idx) + trips.remove(at: idx) deleteCachedTrip(tripId, context: context) - guard !tripId.hasPrefix("local-") else { return } - guard canUseRemotePersonalStore else { return } + + func queueDelete() { + outbox.enqueue( + entityType: .trip, + entityId: tripId, + operation: .delete, + context: context + ) + } + + guard canUseRemotePersonalStore else { + queueDelete() + return + } do { try await service.deleteTrip(tripId) } catch { - trips.insert(removed, at: idx) - upsertCachedTrip(removed, context: context) - throw error + queueDelete() } } @@ -216,8 +282,10 @@ final class TripsViewModel { packId: String? ) -> Trip { let now = Date.iso8601Now() + // A plain client UUID — the scheme the server accepts on create — so an + // offline trip is syncable as-is. No `local-` prefix to reconcile later. return Trip( - id: "local-\(UUID().uuidString)", + id: UUID().uuidString.lowercased(), name: name, description: description, notes: notes, diff --git a/apps/swift/Sources/PackRat/Network/APIClient.swift b/apps/swift/Sources/PackRat/Network/APIClient.swift index f24cd24c0c..df776aeff4 100644 --- a/apps/swift/Sources/PackRat/Network/APIClient.swift +++ b/apps/swift/Sources/PackRat/Network/APIClient.swift @@ -253,16 +253,16 @@ actor APIClient { private func validateStatus(_ response: URLResponse, data: Data) throws { guard let http = response as? HTTPURLResponse else { throw PackRatError.unknown } + if (200...299).contains(http.statusCode) { return } + + let message = APIErrorBody.decodeMessage(from: data) + switch http.statusCode { - case 200...299: return - case 401: - if let message = APIErrorBody.decodeMessage(from: data), !message.isEmpty { - throw PackRatError.httpError(statusCode: http.statusCode, message: message) - } - throw PackRatError.unauthorized - case 404: throw PackRatError.notFound + // A 401 on a sign-in attempt means "wrong credentials", not "your + // session expired" — keep the server's wording when it gave us one. + case 401 where message == nil: throw PackRatError.unauthorized + case 404 where message == nil: throw PackRatError.notFound default: - let message = APIErrorBody.decodeMessage(from: data) throw PackRatError.httpError(statusCode: http.statusCode, message: message) } } @@ -282,16 +282,34 @@ actor APIClient { } } -private struct APIErrorBody: Decodable { +/// Error bodies come in two shapes: +/// - Better Auth (`/api/auth/**`): `{"message": "...", "code": "..."}` +/// - Elysia routes: `{"error": "..."}` +/// Decoding only `error` silently dropped every Better Auth message, so auth +/// failures surfaced as a generic "could not be loaded" banner. +struct APIErrorBody: Decodable { let error: String? let message: String? let code: String? - static func decodeMessage(from data: Data) -> String? { - guard let body = try? JSONDecoder().decode(Self.self, from: data) else { - return nil - } + /// Nil unless `value` has non-whitespace content. A blank string is as absent as + /// a missing key: `InlineErrorView` trims before rendering, so `{"message":" "}` + /// would otherwise suppress the 401/404 fallback and show the generic banner. + private static func nonBlank(_ value: String?) -> String? { + guard let value else { return nil } + let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) + return trimmed.isEmpty ? nil : trimmed + } + + /// Prefer the human-readable text; fall back to the machine code so an + /// unrecognised body still says something specific. Blank strings are + /// treated as absent so `{"message":""}` falls through rather than + /// surfacing an empty banner. + var displayMessage: String? { + Self.nonBlank(message) ?? Self.nonBlank(error) ?? Self.nonBlank(code) + } - return body.message ?? body.error ?? body.code + static func decodeMessage(from data: Data) -> String? { + (try? JSONDecoder().decode(Self.self, from: data))?.displayMessage } } diff --git a/apps/swift/Sources/PackRat/PackRatApp.swift b/apps/swift/Sources/PackRat/PackRatApp.swift index 2eff0b7e87..1841b4cdb0 100644 --- a/apps/swift/Sources/PackRat/PackRatApp.swift +++ b/apps/swift/Sources/PackRat/PackRatApp.swift @@ -28,6 +28,7 @@ struct PackRatApp: App { WindowGroup { AuthGateView() .environment(authManager) + .flushesPendingWrites() } .modelContainer(PersistenceController.shared.container) #if os(macOS) @@ -45,10 +46,14 @@ struct PackRatApp: App { .environment(authManager) } + // These windows host the same detail views as the main window, so they + // can queue writes on their own. Flush from here too — a standalone + // window may be the only one the user has open. WindowGroup("Pack", id: "pack", for: String.self) { $packId in if let id = packId { PackWindowView(packId: id) .environment(authManager) + .flushesPendingWrites() } } .modelContainer(PersistenceController.shared.container) @@ -58,6 +63,7 @@ struct PackRatApp: App { if let id = tripId { TripWindowView(tripId: id) .environment(authManager) + .flushesPendingWrites() } } .modelContainer(PersistenceController.shared.container) diff --git a/apps/swift/Sources/PackRat/Persistence/LegacyLocalIDMigration.swift b/apps/swift/Sources/PackRat/Persistence/LegacyLocalIDMigration.swift new file mode 100644 index 0000000000..ac6b4d33bb --- /dev/null +++ b/apps/swift/Sources/PackRat/Persistence/LegacyLocalIDMigration.swift @@ -0,0 +1,94 @@ +import Foundation +import OSLog +import SwiftData + +private let logger = Logger(subsystem: "com.packrat.app", category: "migration") + +/// One-time cleanup of cached entities created by builds that minted local ids as +/// `local-` (packs, trips) and `local-item-` (pack items). +/// +/// Those ids were never valid server ids. Offline writes now use a plain lowercase +/// UUID — the same scheme the server accepts on create — so a queued create lands +/// as-is. Rows left on disk from the older scheme have no such path: they were never +/// uploaded, and any edit or delete would queue a mutation keyed on an id the server +/// answers 404 for, which `OutboxService.classify` marks terminal. The user would see +/// permanent, unfixable sync failures with no way to clear them. +/// +/// These rows are cache entries for entities the server has never seen, and nothing +/// references them by id, so dropping them is safe and leaves the store consistent. +/// Re-minting a fresh UUID and enqueuing a create was the alternative, but that would +/// resurrect content the user may have deleted on another device and can't be +/// reconciled without server state. Dropping is the conservative choice. +enum LegacyLocalIDMigration { + /// Ids from the retired scheme all carry this prefix (`local-item-` included). + static let legacyPrefix = "local-" + + /// True for an id minted by the retired local-id scheme. + static func isLegacy(_ id: String) -> Bool { + id.hasPrefix(legacyPrefix) + } + + private static let defaultsKey = "legacyLocalIDMigrationCompleted" + + /// Drops every cached pack, pack item, and trip still keyed on a `local-` id, + /// along with any queued mutation that targets one. Runs at most once per install. + /// + /// The flag is only set after a scan that completed — a failed fetch leaves it + /// clear so the next launch tries again. Setting it unconditionally would strand + /// the legacy rows on disk forever, which is the exact state this removes. + /// + /// Idempotent regardless of the flag — a second run simply finds nothing. + static func runIfNeeded(context: ModelContext?, defaults: UserDefaults = .standard) { + guard let context, !defaults.bool(forKey: defaultsKey) else { return } + if run(context: context) != nil { + defaults.set(true, forKey: defaultsKey) + } + } + + /// The migration itself, without the run-once gate. Exposed for tests. + /// + /// Returns the number of rows removed, or `nil` if any fetch failed — in which + /// case the scan is incomplete and must not be recorded as done. + @discardableResult + static func run(context: ModelContext) -> Int? { + guard let packs = try? context.fetch(FetchDescriptor()), + let trips = try? context.fetch(FetchDescriptor()), + let mutations = try? context.fetch(FetchDescriptor()) + else { + logger.error("Legacy local- id scan failed to read the store; will retry next launch") + return nil + } + + var removed = 0 + + for pack in packs where isLegacy(pack.id) { + context.delete(pack) + removed += 1 + } + + // Pack items are serialized inside `CachedPack.jsonData` rather than stored as + // their own rows, so legacy `local-item-` items go with their parent pack + // above. Items with legacy ids under a server-side pack are unreachable + // without re-encoding every cached pack; the next successful `load` overwrites + // that pack's cache from the server anyway. + + for trip in trips where isLegacy(trip.id) { + context.delete(trip) + removed += 1 + } + + // Queued writes against a legacy id can only ever fail. Drop them too, so the + // user isn't left with failed pending writes they can't act on. + for mutation in mutations + where isLegacy(mutation.entityId) || mutation.parentId.map(isLegacy) == true { + context.delete(mutation) + removed += 1 + } + + if removed > 0 { + try? context.save() + logger.info("Dropped \(removed, privacy: .public) cached rows using retired local- ids") + } + return removed + } +} diff --git a/apps/swift/Sources/PackRat/Persistence/PendingMutation.swift b/apps/swift/Sources/PackRat/Persistence/PendingMutation.swift new file mode 100644 index 0000000000..2394c3acaa --- /dev/null +++ b/apps/swift/Sources/PackRat/Persistence/PendingMutation.swift @@ -0,0 +1,120 @@ +import Foundation +import SwiftData + +/// The kind of entity a queued write applies to. +enum OutboxEntityType: String, Codable, Sendable { + case pack + case packItem + case trip +} + +/// The write being replayed. `create` and `update` carry a payload; `delete` does not. +enum OutboxOperation: String, Codable, Sendable { + case create + case update + case delete +} + +/// A durable record of a write that has not yet reached the server. +/// +/// Every offline (or transport-failed) write enqueues one of these. `OutboxService` +/// drains the queue in `createdAt` order on reconnect and on app foreground, so a +/// create-then-update on the same entity replays in the order the user made it. +@Model +final class PendingMutation { + @Attribute(.unique) var id: String + /// Raw value of `OutboxEntityType` — SwiftData predicates can't filter on enums. + var entityTypeRaw: String + /// Client-generated UUID of the target entity. Stable across sync, so child + /// items keep valid foreign keys once the parent reaches the server. + var entityId: String + /// Raw value of `OutboxOperation`. + var operationRaw: String + /// For pack items, the owning pack's id. Nil for packs and trips. + var parentId: String? + /// JSON-encoded operation payload. Nil for deletes. + var payload: Data? + var createdAt: Date + var attemptCount: Int + var lastError: String? + /// Set once the mutation exhausts its retries or hits a non-retryable server + /// response. Failed mutations stay in the store for the UI to surface rather + /// than being dropped silently. + var failed: Bool + /// Earliest time this mutation may be replayed again. + /// + /// `flush` runs on launch, on every connectivity change, and on every foreground — + /// events a user can trigger several times a second. Without a persisted floor, a + /// brief server outage would burn the whole retry budget in seconds and mark the + /// write permanently failed. Defaults to `.distantPast` so a freshly queued + /// mutation is eligible immediately, and so rows written by earlier builds + /// (which lack the column) migrate in as ready rather than never-eligible. + var nextAttemptAt: Date = Date.distantPast + + init( + id: String = UUID().uuidString, + entityType: OutboxEntityType, + entityId: String, + operation: OutboxOperation, + parentId: String? = nil, + payload: Data? = nil, + createdAt: Date = Date() + ) { + self.id = id + self.entityTypeRaw = entityType.rawValue + self.entityId = entityId + self.operationRaw = operation.rawValue + self.parentId = parentId + self.payload = payload + self.createdAt = createdAt + self.attemptCount = 0 + self.lastError = nil + self.failed = false + self.nextAttemptAt = .distantPast + } + + var entityType: OutboxEntityType? { OutboxEntityType(rawValue: entityTypeRaw) } + var operation: OutboxOperation? { OutboxOperation(rawValue: operationRaw) } +} + +// MARK: - Payloads + +/// Field set for a queued pack create/update. Optionals are absent-means-unchanged +/// on update, and carry the full record on create. +struct PackMutationPayload: Codable, Sendable { + var name: String + var description: String? + var category: String? + var isPublic: Bool +} + +/// Field set for a queued pack-item create/update. +struct PackItemMutationPayload: Codable, Sendable { + var name: String + var weight: Double? + var weightUnit: String? + var quantity: Int? + var category: String? + var consumable: Bool + var worn: Bool + var notes: String? +} + +/// Field set for a queued trip create/update. Dates are ISO-8601 strings so the +/// payload round-trips through JSON without timezone drift. +struct TripMutationPayload: Codable, Sendable { + var name: String + var description: String? + var startDate: String? + var endDate: String? + var latitude: Double? + var longitude: Double? + var locationName: String? + var notes: String? + var packId: String? + + var location: TripLocationBody? { + guard let latitude, let longitude else { return nil } + return TripLocationBody(latitude: latitude, longitude: longitude, name: locationName) + } +} diff --git a/apps/swift/Sources/PackRat/Persistence/PersistenceController.swift b/apps/swift/Sources/PackRat/Persistence/PersistenceController.swift index d8181c7eef..a9f6134cc3 100644 --- a/apps/swift/Sources/PackRat/Persistence/PersistenceController.swift +++ b/apps/swift/Sources/PackRat/Persistence/PersistenceController.swift @@ -8,7 +8,7 @@ final class PersistenceController { let container: ModelContainer private init() { - let schema = Schema([CachedPack.self, CachedTrip.self, ShoppingItem.self]) + let schema = Schema([CachedPack.self, CachedTrip.self, ShoppingItem.self, PendingMutation.self]) let config = ModelConfiguration("PackRat", schema: schema) do { try FileManager.default.createDirectory( diff --git a/apps/swift/Sources/PackRat/Services/OutboxService.swift b/apps/swift/Sources/PackRat/Services/OutboxService.swift new file mode 100644 index 0000000000..0f9512ceee --- /dev/null +++ b/apps/swift/Sources/PackRat/Services/OutboxService.swift @@ -0,0 +1,439 @@ +import Foundation +import Observation +import OSLog +import SwiftData + +private let logger = Logger(subsystem: "com.packrat.app", category: "outbox") + +/// Drains queued offline writes to the server. +/// +/// Writes made while offline (or that fail with a transport error) are recorded as +/// `PendingMutation` rows instead of being stranded on-device. This service replays +/// them in `createdAt` order — serially, so a create-then-update on the same entity +/// lands in the order the user made it — whenever connectivity returns or the app +/// comes to the foreground. +/// +/// Retry policy: transport failures are retried up to `maxAttempts`. A 4xx from the +/// server is terminal — the payload is wrong, so retrying forever would never help; +/// the mutation is marked `failed` and left in the store for the UI to surface. A +/// 404 on delete, and a 409 on create, are treated as success: the server already +/// agrees with the local state. +@Observable +@MainActor +final class OutboxService { + static let shared = OutboxService() + + /// Number of mutations still waiting to reach the server. + private(set) var pendingCount: Int = 0 + /// Number of mutations that gave up and need user attention. + private(set) var failedCount: Int = 0 + private(set) var isFlushing = false + + /// Transport failures beyond this count mark the mutation `failed` rather than + /// retrying indefinitely. + static let maxAttempts = 5 + + private let packService: PackService + private let tripService: TripService + + init(packService: PackService = .shared, tripService: TripService = .shared) { + self.packService = packService + self.tripService = tripService + } + + // MARK: - Enqueue + + /// Records a write that could not reach the server. + /// + /// Collapses redundant work before inserting: a delete of a never-synced entity + /// drops its queued create (and any updates) and never contacts the server, and + /// consecutive updates to the same entity collapse to the latest payload. + func enqueue( + entityType: OutboxEntityType, + entityId: String, + operation: OutboxOperation, + parentId: String? = nil, + payload: Data? = nil, + context: ModelContext? + ) { + guard let context else { return } + + // A create or update with no payload can never replay — `decode` would throw + // `missingPayload` on flush and the write would be marked failed. That only + // happens if `encode` failed, which is already logged; refuse the row here so + // the queue never carries a mutation that is guaranteed to fail. + if operation != .delete, payload == nil { + logger.error( + "Refusing to queue \(operation.rawValue, privacy: .public) for \(entityType.rawValue, privacy: .public) with no payload" + ) + return + } + + let existing = pendingMutations(for: entityId, context: context) + + switch operation { + case .delete: + // A create still queued means the server has never seen this entity — + // create + delete cancel out entirely. + if existing.contains(where: { $0.operation == .create }) { + for mutation in existing { context.delete(mutation) } + // The parent never reached the server, so its queued children can + // never land either — their creates would replay against an id the + // server has no record of and be marked terminal. Drop them with the + // parent instead of surfacing failures for a pack the user deleted. + for child in childMutations(of: entityId, context: context) { + context.delete(child) + } + saveAndRefresh(context) + return + } + // Otherwise the delete supersedes any queued updates. + for mutation in existing where mutation.operation == .update { + context.delete(mutation) + } + + case .update: + // Fold into the queued create so the entity is created with its final + // values in a single request. + if let create = existing.first(where: { $0.operation == .create }) { + create.payload = payload + saveAndRefresh(context) + return + } + // Collapse consecutive updates to the last one. + for mutation in existing where mutation.operation == .update { + context.delete(mutation) + } + + case .create: + break + } + + context.insert(PendingMutation( + entityType: entityType, + entityId: entityId, + operation: operation, + parentId: parentId, + payload: payload + )) + saveAndRefresh(context) + } + + // MARK: - Flush + + /// Replays every queued write, oldest first. Safe to call repeatedly — it + /// no-ops while a flush is already running or while offline. + @discardableResult + func flush(context: ModelContext?) async -> Bool { + guard let context, !isFlushing else { return false } + guard NetworkMonitor.shared.isConnected, KeychainService.shared.sessionToken != nil else { + return false + } + + isFlushing = true + defer { + isFlushing = false + refreshCounts(context) + } + + let now = Date() + var descriptor = FetchDescriptor( + predicate: #Predicate { !$0.failed && $0.nextAttemptAt <= now } + ) + descriptor.sortBy = [SortDescriptor(\.createdAt, order: .forward)] + let queued = (try? context.fetch(descriptor)) ?? [] + guard !queued.isEmpty else { return false } + + // Entity ids whose queued create has not landed this pass. A child sent before + // its parent exists server-side gets a 404, which `classify` marks terminal — + // so a transient parent failure would permanently fail the child. + var blockedParents = Set() + + var didSync = false + for mutation in queued { + // Connectivity can drop mid-drain; stop and keep the rest queued. + guard NetworkMonitor.shared.isConnected else { break } + + if Self.shouldDefer(mutation, blockedParents: blockedParents) { + // Hold the child until the parent create succeeds. Match the parent's + // schedule rather than charging the child an attempt for someone + // else's failure. + if let parent = queued.first(where: { + $0.entityId == mutation.parentId && $0.operation == .create + }) { + mutation.nextAttemptAt = parent.nextAttemptAt + } + try? context.save() + continue + } + + let outcome = await apply(mutation) + switch outcome { + case .success: + context.delete(mutation) + didSync = true + case .retry(let message, let chargesAttempt): + mutation.lastError = message + // An expired session isn't the write's fault, so it doesn't spend the + // budget — otherwise a few foregrounds during a signed-out spell would + // permanently fail a write that only needed a re-auth. + if chargesAttempt { + mutation.attemptCount += 1 + if mutation.attemptCount >= Self.maxAttempts { + mutation.failed = true + } + } + mutation.nextAttemptAt = Self.backoffDate( + attemptCount: mutation.attemptCount, from: Date() + ) + if mutation.operation == .create { blockedParents.insert(mutation.entityId) } + case .terminal(let message): + mutation.attemptCount += 1 + mutation.lastError = message + mutation.failed = true + if mutation.operation == .create { blockedParents.insert(mutation.entityId) } + } + try? context.save() + } + return didSync + } + + /// Whether `mutation` must wait because the parent it belongs to has a queued + /// create that hasn't reached the server in this pass. + /// + /// Sending a child first draws a 404, which `classify` marks terminal — so a + /// transient parent failure would otherwise permanently fail the child. + static func shouldDefer(_ mutation: PendingMutation, blockedParents: Set) -> Bool { + guard let parentId = mutation.parentId else { return false } + return blockedParents.contains(parentId) + } + + /// When a retried mutation becomes eligible again: roughly 2s, 4s, 8s, 16s, 32s. + /// + /// Keyed off `attemptCount`, so an auth retry (which doesn't spend an attempt) + /// still waits its current tier instead of spinning on every foreground. + /// + /// Up to 25% jitter is added on top. Without it every write failed by the same + /// outage becomes eligible at the same instant, and the next flush replays the + /// whole queue in one burst against a server that was just struggling. + static func backoffDate(attemptCount: Int, from date: Date) -> Date { + let exponent = min(max(attemptCount, 1), maxAttempts) + let base = pow(2.0, Double(exponent)) + return date.addingTimeInterval(base + Double.random(in: 0...(base * 0.25))) + } + + /// Clears mutations that gave up, after the user has acknowledged them. + func discardFailed(context: ModelContext?) { + guard let context else { return } + let failed = (try? context.fetch(FetchDescriptor( + predicate: #Predicate { $0.failed } + ))) ?? [] + for mutation in failed { context.delete(mutation) } + saveAndRefresh(context) + } + + /// Recomputes `pendingCount` / `failedCount` from the store. + func refreshCounts(_ context: ModelContext?) { + guard let context else { return } + let all = (try? context.fetch(FetchDescriptor())) ?? [] + pendingCount = all.filter { !$0.failed }.count + failedCount = all.filter(\.failed).count + } + + // MARK: - Replay + + enum Outcome: Equatable { + case success + /// Transport-level failure — worth another attempt later. `chargesAttempt` is + /// false when the failure says nothing about the write itself (an expired + /// session), so it shouldn't consume the retry budget. + case retry(String, chargesAttempt: Bool = true) + /// Server rejected the payload; retrying can't fix it. + case terminal(String) + } + + private func apply(_ mutation: PendingMutation) async -> Outcome { + guard let entityType = mutation.entityType, let operation = mutation.operation else { + return .terminal("Unrecognised queued mutation") + } + do { + switch (entityType, operation) { + case (.pack, .create): + let payload: PackMutationPayload = try decode(mutation.payload) + _ = try await packService.createPack( + id: mutation.entityId, + name: payload.name, + description: payload.description, + category: payload.category, + isPublic: payload.isPublic + ) + case (.pack, .update): + let payload: PackMutationPayload = try decode(mutation.payload) + _ = try await packService.updatePack( + mutation.entityId, + name: payload.name, + description: payload.description, + category: payload.category, + isPublic: payload.isPublic + ) + case (.pack, .delete): + try await packService.deletePack(mutation.entityId) + + case (.packItem, .create): + let payload: PackItemMutationPayload = try decode(mutation.payload) + guard let packId = mutation.parentId else { + return .terminal("Queued pack item has no pack") + } + _ = try await packService.addItem( + to: packId, + id: mutation.entityId, + name: payload.name, + weight: payload.weight, + weightUnit: payload.weightUnit, + quantity: payload.quantity, + category: payload.category, + consumable: payload.consumable, + worn: payload.worn, + notes: payload.notes + ) + case (.packItem, .update): + let payload: PackItemMutationPayload = try decode(mutation.payload) + guard let packId = mutation.parentId else { + return .terminal("Queued pack item has no pack") + } + _ = try await packService.updateItem( + mutation.entityId, + in: packId, + name: payload.name, + weight: payload.weight, + weightUnit: payload.weightUnit, + quantity: payload.quantity, + category: payload.category, + consumable: payload.consumable, + worn: payload.worn, + notes: payload.notes + ) + case (.packItem, .delete): + guard let packId = mutation.parentId else { + return .terminal("Queued pack item has no pack") + } + try await packService.deleteItem(mutation.entityId, from: packId) + + case (.trip, .create): + let payload: TripMutationPayload = try decode(mutation.payload) + _ = try await tripService.createTrip( + id: mutation.entityId, + name: payload.name, + description: payload.description, + startDate: payload.startDate?.toDate(), + endDate: payload.endDate?.toDate(), + location: payload.location, + notes: payload.notes, + packId: payload.packId + ) + case (.trip, .update): + let payload: TripMutationPayload = try decode(mutation.payload) + _ = try await tripService.updateTrip( + mutation.entityId, + name: payload.name, + description: payload.description, + startDate: payload.startDate?.toDate(), + endDate: payload.endDate?.toDate(), + location: payload.location, + notes: payload.notes, + packId: payload.packId + ) + case (.trip, .delete): + try await tripService.deleteTrip(mutation.entityId) + } + return .success + } catch { + return classify(error, operation: operation) + } + } + + /// Decides whether a replay failure is worth retrying. + 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 } + // 401 can arrive as a raw status rather than `.unauthorized` when the body + // carries a message. Either way it's a session problem, not a bad payload. + if statusCode == 401 { + return .retry(message ?? "Sign-in required to sync", chargesAttempt: false) + } + // Rate limiting and request timeout are transient despite being 4xx — + // the payload is fine, the server just wants us to come back later. + if statusCode == 429 || statusCode == 408 { + return .retry(message ?? "Server is busy (\(statusCode))") + } + // Any other 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, and waiting on the user + // to sign back in must not spend the retry budget. + return .retry("Sign-in required to sync", chargesAttempt: false) + case PackRatError.decodingError: + return .terminal("Could not read the server response") + default: + return .retry(error.localizedDescription) + } + } + + private func decode(_ data: Data?) throws -> T { + guard let data else { throw PackRatError.decodingError(OutboxError.missingPayload) } + return try JSONDecoder().decode(T.self, from: data) + } + + /// Queued mutations belonging to `parentId` — pack items under their pack. + private func childMutations(of parentId: String, context: ModelContext) -> [PendingMutation] { + let descriptor = FetchDescriptor( + predicate: #Predicate { $0.parentId == parentId && !$0.failed } + ) + return (try? context.fetch(descriptor)) ?? [] + } + + private func pendingMutations(for entityId: String, context: ModelContext) -> [PendingMutation] { + var descriptor = FetchDescriptor( + predicate: #Predicate { $0.entityId == entityId && !$0.failed } + ) + descriptor.sortBy = [SortDescriptor(\.createdAt, order: .forward)] + return (try? context.fetch(descriptor)) ?? [] + } + + private func saveAndRefresh(_ context: ModelContext) { + try? context.save() + refreshCounts(context) + } +} + +enum OutboxError: Error { + case missingPayload +} + +extension OutboxService { + /// Convenience for encoding a payload at the call site. + /// + /// The payload types are plain `Codable` structs, so this should never fail. If it + /// somehow does, log it rather than returning a silent `nil` — `enqueue` refuses a + /// payload-less create/update, so a swallowed error here would otherwise drop the + /// user's write with nothing to diagnose. + static func encode(_ value: T) -> Data? { + do { + return try JSONEncoder().encode(value) + } catch { + logger.error( + "Failed to encode outbox payload for \(String(describing: T.self), privacy: .public): \(error.localizedDescription, privacy: .public)" + ) + return nil + } + } +} diff --git a/apps/swift/Sources/PackRat/Services/PackService.swift b/apps/swift/Sources/PackRat/Services/PackService.swift index 80c707753f..fbc1e2e692 100644 --- a/apps/swift/Sources/PackRat/Services/PackService.swift +++ b/apps/swift/Sources/PackRat/Services/PackService.swift @@ -14,10 +14,12 @@ final class PackService: Sendable { return try await api.send(endpoint) } - func createPack(name: String, description: String? = nil, category: String? = nil, isPublic: Bool = false) async throws -> Pack { + /// `id` is caller-supplied so an offline-created pack keeps the same identity + /// when the outbox replays its create against the server. + func createPack(id: String = UUID().uuidString.lowercased(), name: String, description: String? = nil, category: String? = nil, isPublic: Bool = false) async throws -> Pack { let now = Date.iso8601Now() let body = CreatePackRequest( - id: UUID().uuidString.lowercased(), + id: id, name: name, description: description, category: category, @@ -46,9 +48,9 @@ final class PackService: Sendable { try await api.sendDiscarding(endpoint) } - func addItem(to packId: String, name: String, weight: Double? = nil, weightUnit: String? = nil, quantity: Int? = nil, category: String? = nil, consumable: Bool? = nil, worn: Bool? = nil, notes: String? = nil) async throws -> PackItem { + func addItem(to packId: String, id: String = UUID().uuidString.lowercased(), name: String, weight: Double? = nil, weightUnit: String? = nil, quantity: Int? = nil, category: String? = nil, consumable: Bool? = nil, worn: Bool? = nil, notes: String? = nil) async throws -> PackItem { let body = CreatePackItemRequest( - id: UUID().uuidString.lowercased(), + id: id, name: name, weight: weight, weightUnit: weightUnit, diff --git a/apps/swift/Sources/PackRat/Services/TripService.swift b/apps/swift/Sources/PackRat/Services/TripService.swift index 340d6d3926..e232c77d02 100644 --- a/apps/swift/Sources/PackRat/Services/TripService.swift +++ b/apps/swift/Sources/PackRat/Services/TripService.swift @@ -11,7 +11,10 @@ final class TripService: Sendable { return try await api.send(endpoint) } + /// `id` is caller-supplied so an offline-created trip keeps the same identity + /// when the outbox replays its create against the server. func createTrip( + id: String = UUID().uuidString.lowercased(), name: String, description: String? = nil, startDate: Date? = nil, @@ -22,7 +25,7 @@ final class TripService: Sendable { ) async throws -> Trip { let now = Date.iso8601Now() let body = CreateTripRequest( - id: UUID().uuidString.lowercased(), + id: id, name: name, description: description, location: location, diff --git a/apps/swift/Sources/PackRat/Shared/ErrorView.swift b/apps/swift/Sources/PackRat/Shared/ErrorView.swift index 0779853f3d..c854ea07f2 100644 --- a/apps/swift/Sources/PackRat/Shared/ErrorView.swift +++ b/apps/swift/Sources/PackRat/Shared/ErrorView.swift @@ -25,7 +25,7 @@ struct InlineErrorView: View { HStack(spacing: 6) { Image(systemName: presentation.inlineSystemImage) .foregroundStyle(presentation.inlineColor) - Text(presentation.description) + Text(presentation.inlineDescription(forRawMessage: message)) .font(.caption) .foregroundStyle(.secondary) } @@ -191,6 +191,49 @@ struct FriendlyErrorPresentation { } } + /// Inline copy for a raw error string. + /// + /// The keyword buckets above are meant for infrastructure failures, where a + /// reassuring canned sentence beats a raw `NSURLErrorDomain` dump. But an + /// actionable server message ("User already exists. Use another email.") + /// matches no bucket and used to fall through to the generic + /// `temporarilyUnavailable` copy — telling users the service was broken when + /// they simply needed to pick a different email. Show those verbatim. + func inlineDescription(forRawMessage rawMessage: String) -> String { + guard isGenericFallback else { return description } + + let trimmed = rawMessage.trimmingCharacters(in: .whitespacesAndNewlines) + return FriendlyErrorPresentation.isPresentableToUser(trimmed) ? trimmed : description + } + + /// True when the keyword matcher found no specific bucket for the message. + private var isGenericFallback: Bool { + accessibilityIdentifier == FriendlyErrorPresentation.temporarilyUnavailable.accessibilityIdentifier + } + + /// A message is safe to show verbatim when it reads like a sentence written + /// for a person, rather than a decoding dump or an opaque error domain. + private static func isPresentableToUser(_ message: String) -> Bool { + guard !message.isEmpty, message.count <= 160 else { return false } + + let lowered = message.lowercased() + let leakyMarkers = [ + "error domain", + "codingkey", + "debugdescription", + "keynotfound", + "typemismatch", + "valuenotfound", + "datacorrupted", + "the operation couldn’t be completed", + "the operation couldn't be completed", + "", + "{", + "}", + ] + return !leakyMarkers.contains { lowered.contains($0) } + } + private init( title: String, description: String, diff --git a/apps/swift/Sources/PackRat/Shared/OutboxFlushModifier.swift b/apps/swift/Sources/PackRat/Shared/OutboxFlushModifier.swift new file mode 100644 index 0000000000..3fd7f099b3 --- /dev/null +++ b/apps/swift/Sources/PackRat/Shared/OutboxFlushModifier.swift @@ -0,0 +1,42 @@ +import SwiftData +import SwiftUI + +/// Drains the offline write queue whenever it can plausibly succeed: at launch, +/// when connectivity returns, and when the app comes back to the foreground. +private struct OutboxFlushModifier: ViewModifier { + @Environment(\.modelContext) private var modelContext + @Environment(\.scenePhase) private var scenePhase + + private var outbox: OutboxService { .shared } + private var isConnected: Bool { NetworkMonitor.shared.isConnected } + + func body(content: Content) -> some View { + VStack(spacing: 0) { + // Queued and failed writes are otherwise invisible: an offline write + // succeeds locally and replays silently, so a server rejection would + // never reach the user. + PendingWritesBanner() + content + } + .task { + outbox.refreshCounts(modelContext) + await outbox.flush(context: modelContext) + } + // NetworkMonitor is @Observable, so this fires on every connectivity change. + .onChange(of: isConnected) { _, connected in + guard connected else { return } + Task { await outbox.flush(context: modelContext) } + } + .onChange(of: scenePhase) { _, phase in + guard phase == .active else { return } + Task { await outbox.flush(context: modelContext) } + } + } +} + +extension View { + /// Attach once, above the app's content, to keep queued offline writes moving. + func flushesPendingWrites() -> some View { + modifier(OutboxFlushModifier()) + } +} diff --git a/apps/swift/Sources/PackRat/Shared/PendingWritesBanner.swift b/apps/swift/Sources/PackRat/Shared/PendingWritesBanner.swift new file mode 100644 index 0000000000..3fc5ec9d11 --- /dev/null +++ b/apps/swift/Sources/PackRat/Shared/PendingWritesBanner.swift @@ -0,0 +1,70 @@ +import SwiftData +import SwiftUI + +/// Surfaces the outbox counters. +/// +/// Offline writes succeed locally and replay in the background, so without this the +/// only signal that a write never reached the server was `OutboxService.failedCount`, +/// which nothing rendered. A write that the server rejected would disappear silently. +/// +/// Reads the counters directly in `body` so `@Observable` tracking picks up changes. +struct PendingWritesBanner: View { + @Environment(\.modelContext) private var modelContext + @State private var showingDiscardConfirmation = false + + private var outbox: OutboxService { .shared } + + var body: some View { + let failed = outbox.failedCount + let pending = outbox.pendingCount + + if failed > 0 { + HStack(spacing: 8) { + Image(systemName: "exclamationmark.triangle.fill") + .font(.callout) + Text(failed == 1 + ? "1 change couldn't be saved to the server" + : "\(failed) changes couldn't be saved to the server") + .font(.callout) + Spacer(minLength: 8) + // Discarding drops the writes for good, leaving local state + // permanently diverged from the server — so the label says what it + // does and the action is confirmed rather than one stray tap. + Button("Discard") { showingDiscardConfirmation = true } + .font(.callout.weight(.semibold)) + .buttonStyle(.plain) + } + .foregroundStyle(.white) + .padding(.horizontal, 16) + .padding(.vertical, 10) + .frame(maxWidth: .infinity) + .background(.red.gradient) + .transition(.move(edge: .top).combined(with: .opacity)) + .alert("Discard unsaved changes?", isPresented: $showingDiscardConfirmation) { + Button("Discard", role: .destructive) { + outbox.discardFailed(context: modelContext) + } + Button("Keep", role: .cancel) { } + } message: { + Text(failed == 1 + ? "1 change never reached the server. Discarding removes it for good — this device and the server will stay out of sync." + : "\(failed) changes never reached the server. Discarding removes them for good — this device and the server will stay out of sync.") + } + } else if pending > 0 { + HStack(spacing: 8) { + Image(systemName: "arrow.triangle.2.circlepath") + .font(.callout) + Text(pending == 1 + ? "1 change waiting to sync" + : "\(pending) changes waiting to sync") + .font(.callout) + } + .foregroundStyle(.white) + .padding(.horizontal, 16) + .padding(.vertical, 10) + .frame(maxWidth: .infinity) + .background(.gray.gradient) + .transition(.move(edge: .top).combined(with: .opacity)) + } + } +} diff --git a/apps/swift/Sources/PackRat/Shared/VisualSampleData.swift b/apps/swift/Sources/PackRat/Shared/VisualSampleData.swift index 3dd553c630..977c1798ab 100644 --- a/apps/swift/Sources/PackRat/Shared/VisualSampleData.swift +++ b/apps/swift/Sources/PackRat/Shared/VisualSampleData.swift @@ -30,9 +30,11 @@ enum VisualSampleData { - Water treatment - First aid and repair kit """, - excerpt: "A practical packing order for shoulder-season overnight trips.", - category: "backpacking", - imageUrl: nil, + description: "A practical packing order for shoulder-season overnight trips.", + category: "general", + categories: ["gear", "planning"], + author: nil, + difficulty: "Beginner", createdAt: Date.iso8601Now() ), Guide( @@ -43,9 +45,11 @@ enum VisualSampleData { Desert routes change quickly with heat, wind, and road access. Confirm water sources, carry a reserve, and leave dry campsites with enough margin for the next exposed section. """, - excerpt: "How to set a reliable water margin for hot, exposed routes.", - category: "safety", - imageUrl: nil, + description: "How to set a reliable water margin for hot, exposed routes.", + category: "general", + categories: ["safety", "planning"], + author: nil, + difficulty: "Intermediate", createdAt: Date.iso8601Now() ), Guide( @@ -56,16 +60,19 @@ enum VisualSampleData { Pack active insulation separately from camp warmth. A waterproof liner, dry socks, and an accessible shell prevent small weather shifts from becoming trip problems. """, - excerpt: "Simple layer choices for cold starts, wind, and afternoon rain.", - category: "skills", - imageUrl: nil, + description: "Simple layer choices for cold starts, wind, and afternoon rain.", + category: "general", + categories: ["skills", "gear"], + author: nil, + difficulty: "Intermediate", createdAt: Date.iso8601Now() ), ] } static var guideCategories: [String] { - Array(Set(guides.compactMap(\.category))).sorted() + // Mirrors the API: the filter lists the `categories` tags, not `category`. + Array(Set(guides.flatMap { $0.categories ?? [] })).sorted() } static func seasonSuggestions(location: String) -> SeasonSuggestionsResponse { diff --git a/apps/swift/Tests/PackRatTests/ErrorPresentationTests.swift b/apps/swift/Tests/PackRatTests/ErrorPresentationTests.swift new file mode 100644 index 0000000000..72bb144458 --- /dev/null +++ b/apps/swift/Tests/PackRatTests/ErrorPresentationTests.swift @@ -0,0 +1,133 @@ +import Testing +import Foundation +@testable import PackRat + +// Regression coverage for a TestFlight report: signing up with an +// already-registered email showed "This content could not be loaded right now." +// Two independent defects combined to hide the real reason: +// 1. APIErrorBody decoded only `error`, but Better Auth returns `message`. +// 2. InlineErrorView rendered the matched bucket's canned copy, so any +// message that matched no bucket became the generic fallback. + +// MARK: - Error body decoding + +@Suite("APIErrorBody") +struct APIErrorBodyTests { + private func decode(_ json: String) throws -> APIErrorBody { + try JSONDecoder().decode(APIErrorBody.self, from: Data(json.utf8)) + } + + @Test("reads Better Auth's `message` field") + func betterAuthMessage() throws { + // Verbatim body from POST /api/auth/sign-up/email with a taken email. + let body = try decode( + #"{"message":"User already exists. Use another email.","code":"USER_ALREADY_EXISTS_USE_ANOTHER_EMAIL"}"# + ) + #expect(body.displayMessage == "User already exists. Use another email.") + } + + @Test("reads Elysia's `error` field") + func elysiaError() throws { + let body = try decode(#"{"error":"Pack not found"}"#) + #expect(body.displayMessage == "Pack not found") + } + + @Test("prefers `message` when both fields are present") + func prefersMessage() throws { + let body = try decode(#"{"error":"generic","message":"Password too short"}"#) + #expect(body.displayMessage == "Password too short") + } + + @Test("falls back to `code` when no prose is supplied") + func fallsBackToCode() throws { + let body = try decode(#"{"code":"PASSWORD_TOO_SHORT"}"#) + #expect(body.displayMessage == "PASSWORD_TOO_SHORT") + } + + @Test("returns nil for a body with no recognised fields") + func unrecognisedBody() throws { + let body = try decode(#"{"unexpected":"shape"}"#) + #expect(body.displayMessage == nil) + } + + @Test("treats empty strings as absent") + func emptyStringsIgnored() throws { + let body = try decode(#"{"message":"","error":"","code":"RATE_LIMITED"}"#) + #expect(body.displayMessage == "RATE_LIMITED") + } + + @Test("treats whitespace-only strings as absent") + func whitespaceOnlyIgnored() throws { + // A blank message used to win over the later fields, which suppressed the + // 401/404 fallback in validateStatus and rendered an empty banner. + let body = try decode(#"{"message":" ","error":"\n\t","code":"RATE_LIMITED"}"#) + #expect(body.displayMessage == "RATE_LIMITED") + } + + @Test("returns nil when every field is blank") + func allBlankFieldsYieldNil() throws { + let body = try decode(#"{"message":" ","error":"","code":" "}"#) + #expect(body.displayMessage == nil) + } + + @Test("trims surrounding whitespace from a real message") + func trimsRealMessage() throws { + let body = try decode(#"{"message":" Password too short "}"#) + #expect(body.displayMessage == "Password too short") + } +} + +// MARK: - Inline error copy + +@Suite("FriendlyErrorPresentation inline copy") +struct FriendlyErrorPresentationTests { + @Test("shows an actionable signup message verbatim") + func showsActionableMessageVerbatim() { + let raw = "User already exists. Use another email." + let presentation = FriendlyErrorPresentation(raw) + #expect(presentation.inlineDescription(forRawMessage: raw) == raw) + } + + @Test("shows other validation messages verbatim") + func showsValidationMessagesVerbatim() { + for raw in ["Password too short", "Invalid email or password"] { + let presentation = FriendlyErrorPresentation(raw) + #expect(presentation.inlineDescription(forRawMessage: raw) == raw) + } + } + + @Test("keeps friendly copy for offline failures") + func keepsFriendlyCopyWhenOffline() { + let raw = "The Internet connection appears to be offline." + let presentation = FriendlyErrorPresentation(raw) + #expect(presentation.inlineDescription(forRawMessage: raw) == presentation.description) + #expect(presentation.description != raw) + } + + @Test("keeps friendly copy for auth failures") + func keepsFriendlyCopyForAuthFailures() { + let raw = "401 unauthorized" + let presentation = FriendlyErrorPresentation(raw) + #expect(presentation.inlineDescription(forRawMessage: raw) == presentation.description) + } + + @Test("hides leaky decoding dumps behind the friendly fallback") + func hidesLeakyDumps() { + let leaky = [ + #"keyNotFound(CodingKeys(stringValue: "id"), context)"#, + "Error Domain=NSCocoaErrorDomain Code=4865", + #"{"raw":"payload"}"#, + ] + for raw in leaky { + let presentation = FriendlyErrorPresentation(raw) + #expect(presentation.inlineDescription(forRawMessage: raw) == presentation.description) + } + } + + @Test("hides over-long messages that would break the banner layout") + func hidesOverLongMessages() { + let raw = String(repeating: "a", count: 200) + let presentation = FriendlyErrorPresentation(raw) + #expect(presentation.inlineDescription(forRawMessage: raw) == presentation.description) + } +} diff --git a/apps/swift/Tests/PackRatTests/OutboxTests.swift b/apps/swift/Tests/PackRatTests/OutboxTests.swift new file mode 100644 index 0000000000..ac0462260c --- /dev/null +++ b/apps/swift/Tests/PackRatTests/OutboxTests.swift @@ -0,0 +1,449 @@ +import Foundation +import SwiftData +import Testing +@testable import PackRat + +// Regression coverage for the offline write outbox. Each suite pins behaviour that +// was wrong before: retries burning their budget without delay, an expired session +// permanently failing a write, 429 treated as a bad payload, child item mutations +// stranded by a cancelled parent create, and cached rows from the retired `local-` +// id scheme queueing writes the server can only reject. + +// MARK: - Retry classification + +@Suite("OutboxService.classify") +@MainActor +struct OutboxClassifyTests { + private let outbox = OutboxService() + + @Test("429 retries rather than failing the write") + func rateLimitRetries() { + let outcome = outbox.classify( + PackRatError.httpError(statusCode: 429, message: nil), operation: .create + ) + #expect(outcome == .retry("Server is busy (429)", chargesAttempt: true)) + } + + @Test("408 retries rather than failing the write") + func requestTimeoutRetries() { + let outcome = outbox.classify( + PackRatError.httpError(statusCode: 408, message: nil), operation: .update + ) + #expect(outcome == .retry("Server is busy (408)", chargesAttempt: true)) + } + + @Test("401 retries without spending an attempt") + func unauthorizedStatusIsAttemptFree() { + let outcome = outbox.classify( + PackRatError.httpError(statusCode: 401, message: nil), operation: .create + ) + #expect(outcome == .retry("Sign-in required to sync", chargesAttempt: false)) + } + + @Test("PackRatError.unauthorized retries without spending an attempt") + func unauthorizedErrorIsAttemptFree() { + let outcome = outbox.classify(PackRatError.unauthorized, operation: .create) + #expect(outcome == .retry("Sign-in required to sync", chargesAttempt: false)) + } + + @Test("other 4xx stays terminal") + func badRequestIsTerminal() { + let outcome = outbox.classify( + PackRatError.httpError(statusCode: 400, message: "Bad name"), operation: .create + ) + #expect(outcome == .terminal("Bad name")) + } + + @Test("5xx retries and spends an attempt") + func serverErrorRetries() { + let outcome = outbox.classify( + PackRatError.httpError(statusCode: 503, message: nil), operation: .create + ) + #expect(outcome == .retry("Server error (503)", chargesAttempt: true)) + } + + @Test("the server already agreeing counts as success") + func idempotentResponsesSucceed() { + #expect(outbox.classify( + PackRatError.httpError(statusCode: 404, message: nil), operation: .delete + ) == .success) + #expect(outbox.classify( + PackRatError.httpError(statusCode: 409, message: nil), operation: .create + ) == .success) + } +} + +// MARK: - Backoff + +@Suite("OutboxService backoff") +@MainActor +struct OutboxBackoffTests { + // Delays carry up to 25% jitter, so each tier spans [base, base * 1.25]. + private func expectedRange(base: Double) -> ClosedRange { + base...(base * 1.25) + } + + @Test("delay grows with each attempt") + func delayGrows() { + let now = Date(timeIntervalSince1970: 1_000_000) + let first = OutboxService.backoffDate(attemptCount: 1, from: now).timeIntervalSince(now) + let second = OutboxService.backoffDate(attemptCount: 2, from: now).timeIntervalSince(now) + let third = OutboxService.backoffDate(attemptCount: 3, from: now).timeIntervalSince(now) + + #expect(expectedRange(base: 2).contains(first)) + #expect(expectedRange(base: 4).contains(second)) + #expect(expectedRange(base: 8).contains(third)) + // Tiers stay ordered despite jitter: 2 * 1.25 < 4, 4 * 1.25 < 8. + #expect(first < second) + #expect(second < third) + } + + @Test("a zero attempt count still waits, so an auth retry can't spin") + func attemptFreeRetryStillWaits() { + // An auth failure doesn't increment attemptCount. Without a floor the mutation + // would be eligible again on the very next foreground. + let now = Date(timeIntervalSince1970: 1_000_000) + let delay = OutboxService.backoffDate(attemptCount: 0, from: now).timeIntervalSince(now) + #expect(expectedRange(base: 2).contains(delay)) + } + + @Test("delay is capped at the retry ceiling") + func delayIsCapped() { + let now = Date(timeIntervalSince1970: 1_000_000) + let ceiling = pow(2.0, Double(OutboxService.maxAttempts)) + // Beyond maxAttempts the tier stops growing, jitter aside. + let capped = OutboxService.backoffDate(attemptCount: 99, from: now).timeIntervalSince(now) + #expect(expectedRange(base: ceiling).contains(capped)) + } + + @Test("jitter spreads a burst of same-tier retries") + func jitterSpreadsRetries() { + // Without jitter every write failed by one outage becomes eligible at the same + // instant and the next flush replays the whole queue at once. + let now = Date(timeIntervalSince1970: 1_000_000) + let delays = Set((0..<50).map { + _ in OutboxService.backoffDate(attemptCount: 3, from: now).timeIntervalSince(now) + }) + #expect(delays.count > 1) + } + + @Test("a new mutation is eligible immediately") + func freshMutationIsEligible() { + let mutation = PendingMutation(entityType: .pack, entityId: "p1", operation: .delete) + #expect(mutation.nextAttemptAt == .distantPast) + } +} + +// MARK: - Parent/child ordering + +@Suite("OutboxService child deferral") +@MainActor +struct OutboxChildDeferralTests { + @Test("a child waits while its parent create is unsent") + func childDefersToBlockedParent() { + // Sending the item before the pack exists server-side draws a 404, which + // classify marks terminal — a transient parent failure would otherwise + // permanently fail the child. + let child = PendingMutation( + entityType: .packItem, entityId: "item-1", operation: .create, + parentId: "pack-1" + ) + #expect(OutboxService.shouldDefer(child, blockedParents: ["pack-1"])) + } + + @Test("a child proceeds once its parent is no longer blocked") + func childProceedsWhenParentLanded() { + let child = PendingMutation( + entityType: .packItem, entityId: "item-1", operation: .create, + parentId: "pack-1" + ) + #expect(!OutboxService.shouldDefer(child, blockedParents: [])) + #expect(!OutboxService.shouldDefer(child, blockedParents: ["pack-2"])) + } + + @Test("a parentless mutation is never deferred") + func parentlessNeverDefers() { + let pack = PendingMutation(entityType: .pack, entityId: "pack-1", operation: .create) + #expect(!OutboxService.shouldDefer(pack, blockedParents: ["pack-1"])) + } +} + +// MARK: - Enqueue collapsing + +@Suite("OutboxService.enqueue") +@MainActor +struct OutboxEnqueueTests { + /// In-memory store so each test gets a clean queue. + private func makeContext() throws -> ModelContext { + let container = try ModelContainer( + for: PendingMutation.self, CachedPack.self, CachedTrip.self, + configurations: ModelConfiguration(isStoredInMemoryOnly: true) + ) + return ModelContext(container) + } + + private func mutations(_ context: ModelContext) -> [PendingMutation] { + (try? context.fetch(FetchDescriptor())) ?? [] + } + + private func payload() -> Data? { + OutboxService.encode(PackMutationPayload( + name: "Trip pack", description: nil, category: nil, isPublic: false + )) + } + + @Test("deleting a never-synced pack drops its queued child items") + func parentCancellationCascades() throws { + let context = try makeContext() + let outbox = OutboxService() + + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .create, + payload: payload(), context: context + ) + outbox.enqueue( + entityType: .packItem, entityId: "item-1", operation: .create, + parentId: "pack-1", + payload: OutboxService.encode(PackItemMutationPayload( + name: "Tent", weight: nil, weightUnit: nil, quantity: nil, + category: nil, consumable: false, worn: false, notes: nil + )), + context: context + ) + #expect(mutations(context).count == 2) + + // The pack was never created server-side, so the item create can never land. + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .delete, context: context + ) + + #expect(mutations(context).isEmpty) + #expect(outbox.pendingCount == 0) + } + + @Test("cancelling one pack leaves another pack's items alone") + func cascadeIsScopedToTheParent() throws { + let context = try makeContext() + let outbox = OutboxService() + + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .create, + payload: payload(), context: context + ) + outbox.enqueue( + entityType: .packItem, entityId: "item-2", operation: .delete, + parentId: "pack-2", context: context + ) + + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .delete, context: context + ) + + let remaining = mutations(context) + #expect(remaining.count == 1) + #expect(remaining.first?.entityId == "item-2") + } + + @Test("a create/update with no payload is refused rather than queued to fail") + func payloadlessWriteIsRefused() throws { + let context = try makeContext() + let outbox = OutboxService() + + // Only reachable if encoding failed. Such a row could never replay — decode + // would throw missingPayload and the write would be marked failed. + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .create, + payload: nil, context: context + ) + outbox.enqueue( + entityType: .pack, entityId: "pack-2", operation: .update, + payload: nil, context: context + ) + + #expect(mutations(context).isEmpty) + } + + @Test("a delete still queues without a payload") + func deleteNeedsNoPayload() throws { + let context = try makeContext() + let outbox = OutboxService() + + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .delete, context: context + ) + + #expect(mutations(context).count == 1) + } + + @Test("consecutive updates collapse to the latest payload") + func updatesCollapse() throws { + let context = try makeContext() + let outbox = OutboxService() + + let first = OutboxService.encode(PackMutationPayload( + name: "First", description: nil, category: nil, isPublic: false + )) + let second = OutboxService.encode(PackMutationPayload( + name: "Second", description: nil, category: nil, isPublic: false + )) + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .update, + payload: first, context: context + ) + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .update, + payload: second, context: context + ) + + let remaining = mutations(context) + #expect(remaining.count == 1) + let decoded = remaining.first?.payload.flatMap { + try? JSONDecoder().decode(PackMutationPayload.self, from: $0) + } + #expect(decoded?.name == "Second") + } + + @Test("an update folds into a queued create") + func updateFoldsIntoCreate() throws { + let context = try makeContext() + let outbox = OutboxService() + + // Decides whether an offline-created entity reaches the server with its final + // values in one request, rather than as a create followed by an update. + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .create, + payload: payload(), context: context + ) + outbox.enqueue( + entityType: .pack, entityId: "pack-1", operation: .update, + payload: OutboxService.encode(PackMutationPayload( + name: "Renamed", description: nil, category: nil, isPublic: false + )), + context: context + ) + + let remaining = mutations(context) + #expect(remaining.count == 1) + #expect(remaining.first?.operation == .create) + let decoded = remaining.first?.payload.flatMap { + try? JSONDecoder().decode(PackMutationPayload.self, from: $0) + } + #expect(decoded?.name == "Renamed") + } +} + +// MARK: - Legacy local id migration + +@Suite("LegacyLocalIDMigration") +@MainActor +struct LegacyLocalIDMigrationTests { + private func makeContext() throws -> ModelContext { + let container = try ModelContainer( + for: PendingMutation.self, CachedPack.self, CachedTrip.self, + configurations: ModelConfiguration(isStoredInMemoryOnly: true) + ) + return ModelContext(container) + } + + private func makePack(id: String) -> Pack { + Pack( + id: id, userId: nil, name: "Pack", description: nil, category: nil, + isPublic: false, image: nil, tags: nil, templateId: nil, + deleted: false, isAIGenerated: false, items: [], + totalWeight: 0, baseWeight: 0, wornWeight: 0, consumableWeight: 0, + createdAt: Date.iso8601Now(), updatedAt: Date.iso8601Now() + ) + } + + @Test("recognises ids from the retired scheme") + func detectsLegacyIds() { + #expect(LegacyLocalIDMigration.isLegacy("local-ABC")) + #expect(LegacyLocalIDMigration.isLegacy("local-item-ABC")) + #expect(!LegacyLocalIDMigration.isLegacy(UUID().uuidString.lowercased())) + } + + @Test("drops cached packs and their queued writes") + func dropsLegacyPacks() throws { + let context = try makeContext() + context.insert(CachedPack(from: makePack(id: "local-old-pack"))) + context.insert(CachedPack(from: makePack(id: "11111111-2222-3333-4444-555555555555"))) + context.insert(PendingMutation( + entityType: .pack, entityId: "local-old-pack", operation: .delete + )) + // A child item under the legacy pack — its parentId is just as unusable. + context.insert(PendingMutation( + entityType: .packItem, entityId: "local-item-9", operation: .delete, + parentId: "local-old-pack" + )) + try context.save() + + LegacyLocalIDMigration.run(context: context) + + let packs = (try? context.fetch(FetchDescriptor())) ?? [] + #expect(packs.count == 1) + #expect(packs.first?.id == "11111111-2222-3333-4444-555555555555") + #expect(((try? context.fetch(FetchDescriptor())) ?? []).isEmpty) + } + + @Test("leaves a clean store untouched") + func noOpOnCleanStore() throws { + let context = try makeContext() + context.insert(CachedPack(from: makePack(id: "11111111-2222-3333-4444-555555555555"))) + try context.save() + + #expect(LegacyLocalIDMigration.run(context: context) == 0) + #expect(((try? context.fetch(FetchDescriptor())) ?? []).count == 1) + } + + @Test("is idempotent") + func isIdempotent() throws { + let context = try makeContext() + context.insert(CachedPack(from: makePack(id: "local-old-pack"))) + try context.save() + + #expect(LegacyLocalIDMigration.run(context: context) == 1) + #expect(LegacyLocalIDMigration.run(context: context) == 0) + } + + @Test("runs only once per install") + func runsOnceViaFlag() throws { + let context = try makeContext() + let defaults = UserDefaults(suiteName: "LegacyLocalIDMigrationTests-\(UUID().uuidString)")! + context.insert(CachedPack(from: makePack(id: "local-old-pack"))) + try context.save() + + LegacyLocalIDMigration.runIfNeeded(context: context, defaults: defaults) + #expect(((try? context.fetch(FetchDescriptor())) ?? []).isEmpty) + + // A legacy row written after the flag is set is not re-scanned — the retired + // scheme can't produce new ids, so one pass is enough. + context.insert(CachedPack(from: makePack(id: "local-another"))) + try context.save() + LegacyLocalIDMigration.runIfNeeded(context: context, defaults: defaults) + #expect(((try? context.fetch(FetchDescriptor())) ?? []).count == 1) + } + + @Test("a completed scan reports a count rather than nil") + func completedScanReportsCount() throws { + // runIfNeeded gates the completion flag on this being non-nil, so a scan that + // read the store must be distinguishable from one that couldn't. + let context = try makeContext() + context.insert(CachedPack(from: makePack(id: "local-old-pack"))) + try context.save() + + #expect(LegacyLocalIDMigration.run(context: context) != nil) + } + + @Test("the flag is not set when nothing was scanned yet") + func flagUnsetBeforeFirstRun() throws { + let context = try makeContext() + let defaults = UserDefaults(suiteName: "LegacyLocalIDMigrationTests-\(UUID().uuidString)")! + context.insert(CachedPack(from: makePack(id: "local-old-pack"))) + try context.save() + + #expect(!defaults.bool(forKey: "legacyLocalIDMigrationCompleted")) + LegacyLocalIDMigration.runIfNeeded(context: context, defaults: defaults) + // Set only after the scan completed, so a failed read retries next launch. + #expect(defaults.bool(forKey: "legacyLocalIDMigrationCompleted")) + } +} diff --git a/packages/mcp/src/output-schemas.ts b/packages/mcp/src/output-schemas.ts index 98c39ede76..4cac4ef2e2 100644 --- a/packages/mcp/src/output-schemas.ts +++ b/packages/mcp/src/output-schemas.ts @@ -31,23 +31,34 @@ * packrat_admin_stats → AdminStatsSchema * packrat_admin_analytics_* → schemas-package analytics shapes * - * Tier 2 deferral list — tools whose API response shape is loosely typed - * by Eden Treaty / not currently modeled in `@packrat/schemas`. These - * tools emit text-only output today and are tracked in - * `docs/mcp/runbook.md` under "U8 output envelopes → Tier 2 deferral": + * Tier 2 lift (this change): these tools now declare an `outputSchema`, + * reusing shapes already modeled in `@packrat/schemas`: * - * - all of `packs.items.*` create/update/delete payloads - * - catalog vector-search responses - * - feed/trail-conditions/guides/knowledge handlers - * - admin list endpoints whose Treaty inferred type loses the array - * element shape after the response coercion + * packrat_search_gear_catalog → CatalogItemsResponseSchema + * packrat_get_catalog_item → CatalogItemSchema + * packrat_semantic_gear_search → catalog rows + `similarity` + * packrat_similar_catalog_items → catalog rows + `similarity` + * packrat_list_pack_items → list-of-PackItem with nextOffset + * packrat_get_pack_item → PackItemSchema + * packrat_list_guides → GuidesResponseSchema + * packrat_search_guides → GuideSearchResponseSchema + * packrat_get_guide → GuideDetailSchema + * packrat_list_guide_categories → GuideCategoriesResponseSchema * - * The intent is that a follow-up unit derives the missing schemas from - * the API route definitions and lifts those tools to Tier 1. + * Still text-only (shape not modeled, or Treaty loses the element type): + * pack/template/trip write payloads, feed, trail-conditions, knowledge, + * season suggestions. */ import { AdminStatsSchema } from '@packrat/schemas/admin'; -import { PackSchema, PackWithItemsSchema } from '@packrat/schemas/packs'; +import { CatalogItemSchema, CatalogItemsResponseSchema } from '@packrat/schemas/catalog'; +import { + GuideCategoriesResponseSchema, + GuideDetailSchema, + GuideSearchResponseSchema, + GuidesResponseSchema, +} from '@packrat/schemas/guides'; +import { PackItemSchema, PackSchema, PackWithItemsSchema } from '@packrat/schemas/packs'; import { TripSchema } from '@packrat/schemas/trips'; import { UserSchema } from '@packrat/schemas/users'; import { z } from 'zod'; @@ -134,6 +145,52 @@ export const GetWeatherOutputSchema = z }) .passthrough(); +// ── Tier 2 → Tier 1 lift ───────────────────────────────────────────────────── +// The tools below were previously text-only (see the "Tier 2 deferral list" +// in the module docstring). Their API response shapes ARE modeled in +// `@packrat/schemas`, so per this file's reuse policy we re-export those +// rather than re-deriving them here. + +/** `packrat_search_gear_catalog` — paginated catalog page. */ +export const SearchGearCatalogOutputSchema = CatalogItemsResponseSchema; + +/** `packrat_get_catalog_item` — a single catalog row. */ +export const GetCatalogItemOutputSchema = CatalogItemSchema; + +/** + * `packrat_semantic_gear_search` / `packrat_similar_catalog_items` — vector + * search returns catalog rows with a cosine `similarity` score attached, and + * the service drops the 1536-dim `embedding` column from the projection. + * `.passthrough()` keeps validation from failing if the service adds a field. + */ +export const CatalogSimilarityOutputSchema = z + .object({ + items: z.array(CatalogItemSchema.extend({ similarity: z.number() }).passthrough()), + total: z.number().int().optional(), + limit: z.number().int().optional(), + offset: z.number().int().optional(), + nextOffset: z.number().int().nullable().optional(), + }) + .passthrough(); + +/** `packrat_list_pack_items` — the API returns a bare array; we wrap it. */ +export const ListPackItemsOutputSchema = paginatedWithNextOffset(PackItemSchema); + +/** `packrat_get_pack_item` — a single pack item row. */ +export const GetPackItemOutputSchema = PackItemSchema; + +/** `packrat_list_guides` — paginated guides page. */ +export const ListGuidesOutputSchema = GuidesResponseSchema; + +/** `packrat_search_guides` — same page shape plus the echoed query. */ +export const SearchGuidesOutputSchema = GuideSearchResponseSchema; + +/** `packrat_get_guide` — a single guide including its MDX/Markdown body. */ +export const GetGuideOutputSchema = GuideDetailSchema; + +/** `packrat_list_guide_categories` — the category list plus a count. */ +export const ListGuideCategoriesOutputSchema = GuideCategoriesResponseSchema; + /** `packrat_admin_stats` — re-export of the API's admin stats schema. */ export const AdminStatsOutputSchema = AdminStatsSchema; diff --git a/packages/mcp/src/tools/catalog.ts b/packages/mcp/src/tools/catalog.ts index 80096f42a5..c687765562 100644 --- a/packages/mcp/src/tools/catalog.ts +++ b/packages/mcp/src/tools/catalog.ts @@ -1,6 +1,11 @@ import { z } from 'zod'; import { call, clampLimit, PAGINATION_LIMIT_MAX } from '../client'; import { CatalogSortField, SortOrder } from '../enums'; +import { + CatalogSimilarityOutputSchema, + GetCatalogItemOutputSchema, + SearchGearCatalogOutputSchema, +} from '../output-schemas'; import { tool } from '../registerTool'; import type { AgentContext } from '../types'; @@ -44,6 +49,7 @@ export function registerCatalogTools(agent: AgentContext): void { sort_by: z.nativeEnum(CatalogSortField).optional(), sort_order: z.nativeEnum(SortOrder).default(SortOrder.Asc), }, + outputSchema: SearchGearCatalogOutputSchema.shape, annotations: { title: 'Search Gear Catalog', readOnlyHint: true, @@ -64,6 +70,7 @@ export function registerCatalogTools(agent: AgentContext): void { }, }), action: 'search catalog', + structured: true, }), ); @@ -80,6 +87,7 @@ export function registerCatalogTools(agent: AgentContext): void { query: z.string().min(3), limit: z.number().int().min(1).max(30).default(8), }, + outputSchema: CatalogSimilarityOutputSchema.shape, annotations: { title: 'Semantic Gear Search', readOnlyHint: true, @@ -92,6 +100,7 @@ export function registerCatalogTools(agent: AgentContext): void { call({ promise: agent.api.user.catalog['vector-search'].get({ query: { q: query, limit } }), action: 'semantic catalog search', + structured: true, }), ); @@ -107,6 +116,7 @@ export function registerCatalogTools(agent: AgentContext): void { inputSchema: { item_id: z.number().int().describe('The catalog item ID'), }, + outputSchema: GetCatalogItemOutputSchema.shape, annotations: { title: 'Get Catalog Item', readOnlyHint: true, @@ -120,6 +130,7 @@ export function registerCatalogTools(agent: AgentContext): void { promise: agent.api.user.catalog({ id: String(item_id) }).get(), action: 'get catalog item', resourceHint: `catalog item ${item_id}`, + structured: true, }), ); @@ -136,6 +147,7 @@ export function registerCatalogTools(agent: AgentContext): void { limit: z.number().int().min(1).max(50).default(10), threshold: z.number().min(0).max(1).optional(), }, + outputSchema: CatalogSimilarityOutputSchema.shape, annotations: { title: 'Find Similar Catalog Items', readOnlyHint: true, @@ -154,6 +166,7 @@ export function registerCatalogTools(agent: AgentContext): void { }), action: 'find similar catalog items', resourceHint: `catalog item ${item_id}`, + structured: true, }), ); diff --git a/packages/mcp/src/tools/guides.ts b/packages/mcp/src/tools/guides.ts index 51bba2d5a0..4092a773a5 100644 --- a/packages/mcp/src/tools/guides.ts +++ b/packages/mcp/src/tools/guides.ts @@ -1,5 +1,11 @@ import { z } from 'zod'; import { call } from '../client'; +import { + GetGuideOutputSchema, + ListGuideCategoriesOutputSchema, + ListGuidesOutputSchema, + SearchGuidesOutputSchema, +} from '../output-schemas'; import { tool } from '../registerTool'; import type { AgentContext } from '../types'; @@ -23,6 +29,7 @@ export function registerGuidesTools(agent: AgentContext): void { sort_field: z.enum(['title', 'category', 'createdAt', 'updatedAt']).optional(), sort_order: z.enum(['asc', 'desc']).optional(), }, + outputSchema: ListGuidesOutputSchema.shape, annotations: { title: 'List Outdoor Guides', readOnlyHint: true, @@ -42,6 +49,7 @@ export function registerGuidesTools(agent: AgentContext): void { }, }), action: 'list guides', + structured: true, }), ); @@ -52,6 +60,7 @@ export function registerGuidesTools(agent: AgentContext): void { title: 'List Guide Categories', description: 'List all guide categories.', inputSchema: {}, + outputSchema: ListGuideCategoriesOutputSchema.shape, annotations: { title: 'List Guide Categories', readOnlyHint: true, @@ -64,6 +73,7 @@ export function registerGuidesTools(agent: AgentContext): void { call({ promise: agent.api.user.guides.categories.get(), action: 'list guide categories', + structured: true, }), ); @@ -84,6 +94,7 @@ export function registerGuidesTools(agent: AgentContext): void { limit: z.number().int().min(1).max(50).default(20), category: z.string().optional(), }, + outputSchema: SearchGuidesOutputSchema.shape, annotations: { title: 'Search Outdoor Guides', readOnlyHint: true, @@ -96,6 +107,7 @@ export function registerGuidesTools(agent: AgentContext): void { call({ promise: agent.api.user.guides.search.get({ query: { q: query, page, limit, category } }), action: 'search guides', + structured: true, }), ); @@ -106,6 +118,7 @@ export function registerGuidesTools(agent: AgentContext): void { title: 'Get Guide', description: 'Get a specific guide by ID. Returns MDX/Markdown content.', inputSchema: { guide_id: z.string() }, + outputSchema: GetGuideOutputSchema.shape, annotations: { title: 'Get Guide', readOnlyHint: true, @@ -118,6 +131,7 @@ export function registerGuidesTools(agent: AgentContext): void { call({ promise: agent.api.user.guides({ id: guide_id }).get(), action: 'get guide', + structured: true, resourceHint: `guide ${guide_id}`, }), ); diff --git a/packages/mcp/src/tools/packs.ts b/packages/mcp/src/tools/packs.ts index b04821fde6..131adb8226 100644 --- a/packages/mcp/src/tools/packs.ts +++ b/packages/mcp/src/tools/packs.ts @@ -1,7 +1,12 @@ import { z } from 'zod'; import { call, clampLimit, nowIso, ok, PAGINATION_LIMIT_MAX, withNextOffset } from '../client'; import { ItemCategory, PackCategory } from '../enums'; -import { GetPackOutputSchema, ListPacksOutputSchema } from '../output-schemas'; +import { + GetPackItemOutputSchema, + GetPackOutputSchema, + ListPackItemsOutputSchema, + ListPacksOutputSchema, +} from '../output-schemas'; import { tool } from '../registerTool'; import type { AgentContext } from '../types'; @@ -231,6 +236,7 @@ export function registerPackTools(agent: AgentContext): void { title: 'List Pack Items', description: 'List all items in a pack.', inputSchema: { pack_id: z.string().describe('The pack ID') }, + outputSchema: ListPackItemsOutputSchema.shape, annotations: { title: 'List Pack Items', readOnlyHint: true, @@ -239,12 +245,31 @@ export function registerPackTools(agent: AgentContext): void { openWorldHint: false, }, }, - async ({ pack_id }) => - call({ - promise: agent.api.user.packs({ packId: pack_id }).items.get(), - action: 'list pack items', - resourceHint: `pack ${pack_id}`, - }), + async ({ pack_id }) => { + const result = await agent.api.user.packs({ packId: pack_id }).items.get(); + if (result.error || result.data == null) { + // Defer to the standard error envelope for failure consistency. + return call({ + promise: Promise.resolve(result), + action: 'list pack items', + resourceHint: `pack ${pack_id}`, + }); + } + // The API returns a bare array; normalise into the `{ data, nextOffset }` + // envelope the declared outputSchema expects. + // + // `nextOffset` is always null: this endpoint takes only `pack_id` and + // returns every item in one response, so there is never a next page. + // Don't route this through `withNextOffset` — passing `limit: + // items.length` would make its `items.length >= limit` check true for + // every response (including an empty pack), advertising a bogus + // continuation offset that a consumer could follow into a loop. + const items = Array.isArray(result.data) ? result.data : []; + return ok({ + data: { data: items, nextOffset: null }, + structured: true, + }); + }, ); // ── Get a single pack item ──────────────────────────────────────────────── @@ -256,6 +281,7 @@ export function registerPackTools(agent: AgentContext): void { title: 'Get Pack Item', description: 'Get full details of a single pack item.', inputSchema: { item_id: z.string().describe('The pack item ID') }, + outputSchema: GetPackItemOutputSchema.shape, annotations: { title: 'Get Pack Item', readOnlyHint: true, @@ -269,6 +295,7 @@ export function registerPackTools(agent: AgentContext): void { promise: agent.api.user.packs.items({ itemId: item_id }).get(), action: 'get pack item', resourceHint: `item ${item_id}`, + structured: true, }), );