Cache stops to SQLite database

[diveshpatil9104]

Closes #62

What

Persists transit stops to a local SQLite database (via GRDB.swift) as the user browses the map. On API failure (no network, timeout), previously visited areas serve stops from cache instead of showing a blank map.

How

• API returns stops → displayed on map → saved to SQLite in the background
• API fails → cache queried by bounding-box → cached stops displayed
• API stops always take priority for display (they carry full Route objects for labels/icons)
• Cached stops use routes: [] to prevent nil crash on Stop.routes! — shows default bus icon offline

Schema

Single cachedStop table, composite primary key (regionId, id), indexed on (regionId, lat, lon) for spatial queries and (regionId, lastUpdated) for purging.

New files

• OBAKitCore/Persistence/StopCacheDatabase.swift — DB setup + migrations
• OBAKitCore/Persistence/CachedStop.swift — GRDB record with Stop ↔ cache conversion
• OBAKitCore/Persistence/StopCacheRepository.swift — Read, upsert, purge, clear
• OBAKitTests/Persistence/StopCacheRepositoryTests.swift — 14 tests

Modified files

• Apps/Shared/app_shared.yml — Added GRDB.swift package
• OBAKitCore/project.yml — Added GRDB dependency
• OBAKitCore/Orchestration/CoreApplication.swift — Lazy stopCacheDatabase / stopCacheRepository (nil if DB init fails → app works as before)
• OBAKit/Mapping/MapRegionManager.swift — Save after display, serve from cache on error

ScreenRec

Before (Bug)	After (Fixed)
issue.mp4	fix.mp4
Internet off → blank map, no stops visible	Internet off → cached stops appear for previously visited areas

Verification

• Airplane Mode → cached stops appear for visited areas
• Route labels render correctly online (API stops used, not cached)
• All unit tests pass

[aaronbrethorst]

This is an ambitious and well-structured PR, Divesh — offline stop caching is a feature users have been wanting for a long time (issue #62!), and the architecture here is solid: clean separation between database, repository, and record layers, good use of GRDB's migration system, and thorough test coverage of the happy paths. That said, there are a few issues that need to be addressed before this can be merged.

Critical Issues

1. CancellationError is caught and swallowed in the cache fallback path (MapRegionManager.swift:284-305): The catch block catches all errors, including CancellationError. If the user navigates away and the task is cancelled, this code will still query the cache and set stale stops on the map for a view that may no longer be relevant. Add an early check:

   } catch {
       if error is CancellationError { return }
       // ... rest of cache fallback logic
   }

2. No cache eviction is wired up (StopCacheRepository.swift): deleteStopsOlderThan() and clearCache() are implemented and tested, but never called in production code. The cache will grow unboundedly as users browse different areas. Over months of use, this could accumulate tens of thousands of rows. Please wire up eviction — for example, call deleteStopsOlderThan(Date().addingTimeInterval(-30 * 24 * 3600), regionId:) on app launch or region change.

3. API error is completely swallowed when cache has data (MapRegionManager.swift:299-303): When the API fails and cached stops exist, the return on line 303 bypasses both logging and displayError(). The user sees stops on the map with no indication they're viewing stale cached data (and the stops will lack route labels/icons since cached stops use routes: []). At minimum:

   • Log the API error before attempting the cache fallback
   • Consider a subtle banner or toast indicating offline/cached data is being shown

Important Issues

1. Missing test: routes is non-nil after cache round-trip (StopCacheRepositoryTests.swift): Stop.routes is [Route]! (implicitly unwrapped). The toStop() method correctly injects "routes": [] to prevent a crash, but there's no test verifying this. If that line is accidentally removed, every offline user will get a force-unwrap crash when the map tries to render stop annotations. Please add a test:

   func test_cachedStop_routesIsNotNil_afterRoundTrip() {
       let stop = makeStop(id: "1_100")
       repository.saveStops([stop], regionId: 1)
       let results = repository.stopsInRegion(minLat: 47.0, maxLat: 48.0, minLon: -123.0, maxLon: -122.0, regionId: 1)
       XCTAssertNotNil(results[0].routes)
       // This accesses routeTypes -> routes.map, would crash if routes is nil:
       XCTAssertEqual(results[0].prioritizedRouteTypeForDisplay, .unknown)
   }

2. Missing test: toStop() failure with corrupted data (StopCacheRepositoryTests.swift): There's no test verifying that stopsInRegion gracefully handles records where toStop() returns nil (e.g., corrupted routeIDs JSON). A test inserting a row with invalid JSON and verifying it's silently filtered out would protect against crashes from database corruption.

3. saveStops() is a synchronous blocking call in an async context (MapRegionManager.swift:282): repository.saveStops() calls database.dbQueue.write synchronously, which blocks a thread from Swift's cooperative thread pool. This runs on every successful map pan. For small batches this is fast, but consider dispatching the save to a dedicated DispatchQueue to avoid starving other async tasks.

Fit-and-Finish

1. toStop() JSON roundtrip is fragile (CachedStop.swift:77-98): The manual dictionary with hardcoded keys ("lat", "lon", "routeIds", etc.) must exactly match Stop.CodingKeys. If a contributor renames a key or adds a required field, this will silently break and return nil for all cached stops. Consider storing the encoded Stop JSON as a blob column instead — you'd still keep latitude, longitude, and regionId as indexed columns for spatial queries, but the full stop data round-trips through JSONEncoder/JSONDecoder with compile-time safety.

2. routeIDs encoding failure saves a corrupted record (CachedStop.swift:49-55): If JSONEncoder().encode(stop.routeIDs) fails, the catch block sets routeIDs = "[]" and continues saving. This persists a stop with no route information. Consider making CachedStop.init failable and skipping the stop instead of saving corrupted data.

3. Indentation: The mapRegionManagerShowZoomInStatus method in MapRegionManager.swift is not modified by this PR but is worth noting — it has an indentation issue from a separate PR (#1063). No action needed here.

Strengths

• The three-layer architecture (Database, Repository, Record) is clean and well-separated.
• GRDB's DatabaseMigrator gives a solid foundation for future schema evolution.
• The composite primary key (regionId, id) correctly handles multi-region scenarios.
• Test coverage is thorough for the happy paths: 14 tests covering round-trips, upserts, bounding-box queries, direction handling, region scoping, and purge operations.
• Graceful degradation: if the database fails to initialize, the app falls back to API-only behavior with no crash.
• The routes: [] workaround for Stop.routes! is well-documented and correctly prevents a force-unwrap crash.

Recommended Action

Please address the three critical issues (CancellationError handling, cache eviction, and API error visibility), then this is in good shape. The important issues around test coverage would also strengthen the PR significantly.

[diveshpatil9104]

Thanks Aaron for the review! All critical and important items are addressed:

Critical fixes:

• Added CancellationError early return at the top of the catch block — prevents stale cache writes for cancelled tasks
• Wired up cache eviction: purgeStaleStopCache() runs inside refreshServices() (app launch + region change), deletes stops older than 30 days scoped to current region
• API error is now logged with Logger.error() before attempting cache fallback

Important fixes:

• Added test_cachedStop_routesIsNotNil_afterRoundTrip — verifies routes is non-nil and prioritizedRouteTypeForDisplay does not crash
• Added test_stopsInRegion_gracefullyHandlesCorruptedRouteIDs — inserts invalid JSON via raw SQL, verifies it is silently filtered out by compactMap
• Regarding saveStops blocking: the write is a single transaction for small batches (typically 20-50 stops per map pan) and completes in milliseconds on SQLite. Happy to move to a dedicated DispatchQueue if you did prefer, but wanted to flag the tradeoff of added complexity vs. actual impact.

Fit-and-finish (noted for follow-up):

• The toStop() JSON dictionary fragility and failable CachedStop.init are good improvements — I will address them in a follow-up PR to keep this one focused.

[aaronbrethorst]

Nice work on this feature, Divesh — offline stop caching is a long-requested improvement, and your approach with GRDB + a clean repository pattern is solid. The test suite is thorough and well-designed (especially the makeStop helper going through real Codable paths), and the cache integration into MapRegionManager reads clearly. I also appreciate how you addressed all the critical items from the first review round.

I found one critical issue that needs fixing before merge, and a handful of important items worth addressing.

Critical

1. Branch needs rebase onto main — currently reverts PRs #1052, #1054, #1055

The branch diverged from main before three recent PRs were merged. Because some of the same files are touched, merging this PR as-is would revert those previously accepted changes:

• PR #1052: Reverts the SelectedTab(rawValue:) force-unwrap crash fix back to SelectedTab(rawValue: raw)!
• PR #1054: Reverts the debug-build Crashlytics/Analytics disable
• PR #1055: Reverts the bookmarks sync fix (groupsController.updateModelState() and reloadFormFromStore() calls removed)

Fix: Rebase onto current main:

git fetch origin
git rebase origin/main

2. nil as Any in toStop() dictionary can silently break deserialization for stops without direction

CachedStop.swift:77-89 — When direction or wheelchairBoarding is nil, the expression direction as Any wraps Optional<String>.none into Any. JSONSerialization can produce NSNull for this, which then causes JSONDecoder to fail when decoding Stop (since Stop uses decodeIfPresent and expects the key to be absent, not null). The stop is silently dropped by compactMap.

Stops without direction data are common in production, so this is a real data-loss path.

Fix — build the dictionary without nil keys:

var stopDict: [String: Any] = [
    "id": id,
    "code": code,
    "name": name,
    "lat": latitude,
    "lon": longitude,
    "locationType": locationType,
    "routeIds": decodedRouteIDs,
    "regionIdentifier": regionId,
    "routes": [] as [[String: Any]]
]
if let direction = direction {
    stopDict["direction"] = direction
}
if let wheelchairBoarding = wheelchairBoarding {
    stopDict["wheelchairBoarding"] = wheelchairBoarding
}

And add a test for stops with nil direction/wheelchairBoarding round-tripping correctly through the cache.

Important

3. Add corruption recovery to StopCacheDatabase

StopCacheDatabase.swift:23-39 — If the SQLite file becomes corrupted (power loss during write, iOS killing the process), the initializer throws, and the lazy Optional in CoreApplication permanently disables caching for the session. Since this is a cache (not primary data), the right behavior is to delete the corrupted file and start fresh.

public init(databasePath: String? = nil) throws {
    let path = // ... existing path logic ...
    do {
        dbQueue = try DatabaseQueue(path: path)
        try runMigrations()
    } catch {
        Logger.error("Cache database corrupted, recreating: \(error)")
        try? FileManager.default.removeItem(atPath: path)
        try? FileManager.default.removeItem(atPath: path + "-wal")
        try? FileManager.default.removeItem(atPath: path + "-shm")
        dbQueue = try DatabaseQueue(path: path)
        try runMigrations()
    }
}

4. Removed doc comments on findUserPin and setUserAnnotation are unrelated to this PR

MapRegionManager.swift — The diff removes parameter-level documentation from findUserPin(for:) and setUserAnnotation(coordinate:title:subtitle:). These are unrelated changes that muddy the PR scope. Please restore them.

5. Two test gaps worth covering

The test suite is strong overall, but two gaps stand out:

• Empty routeIDs: No test creates a stop with routeIDs: []. Since Stop.routes is [Route]!, a round-trip failure here would be a crash. Add a test verifying makeStop(id: "no_routes", routeIDs: []) survives the cache round-trip.

• clearCache cross-region isolation: test_deleteStopsOlderThan_scopedToRegion correctly verifies purge is region-scoped, but test_clearCache_removesAllStopsForRegion doesn't verify that other regions are preserved. Add a matching cross-region assertion.

Fit and finish (optional, noted for follow-up)

• Sendable conformance: StopCacheDatabase and StopCacheRepository are used across async boundaries but lack Sendable conformance. Consider marking both final and adding @unchecked Sendable for Swift 6 readiness.

• purgeStaleStopCache() runs synchronously during init: Called from refreshServices() in CoreApplication.init(). On first launch it's trivial (empty DB), but on subsequent launches it blocks init on a DB write. Consider wrapping in Task.detached(priority: .utility).

• toStop() dictionary fragility: Already acknowledged for a follow-up PR. The as Any fix in Critical #2 is the most urgent part of this.****

[diveshpatil9104]

Thanks for the review Aaron! All items addressed:

Critical:

• Rebased onto latest main
• Fixed nil as Any in toStop() — dictionary now omits nil keys with if let instead of inserting them. Added test_stopWithNilDirectionAndWheelchairBoarding_roundTripsCorrectly

Important:

• Added corruption recovery to StopCacheDatabase.init — deletes .sqlite/-wal/-shm and recreates on failure
• Restored doc comments on findUserPin(for:) and setUserAnnotation(coordinate:title:subtitle:)
• Added test_cachedStop_emptyRouteIDs_roundTripsCorrectly for empty routeIDs
• test_clearCache now verifies cross-region isolation

[aaronbrethorst]

Hey Divesh, excellent work iterating on this PR. You've addressed every single item from both review rounds thoroughly and correctly: the CancellationError guard, cache eviction wiring, API error logging, the rebase, the nil as Any fix, corruption recovery, restored doc comments, and the five additional tests. The diff is clean (728 additions, 0 deletions against main), the architecture is solid, and the test suite is comprehensive. This feature is going to make a real difference for users in spotty coverage areas.

There's nothing left to change--this PR is ready to merge. Offline stop caching has been a long-requested feature (#62!), and this implementation handles it gracefully: clean fallback when the API is down, transparent behavior when it's up, and no impact if the cache itself fails to initialize. Well done.