Split WhereSession into a coordinator + scope-tiered child models#53
Conversation
WhereSession had grown into an ~850-line observable that mixed always-on concerns with UI mirrors and lived for the whole logged-in lifetime, so its store-change subscription drove report/data-issue refreshes even on a headless background relaunch nothing renders. Decompose it by lifetime scope: - WhereSession stays as a slim always-on coordinator: tracking/authorization, launch-time reminder/summary application (reading WherePreferences), widget republish, and erase. No presentation state, no store-change subscription. - ReportModel (scene-scoped, @State in a new MainTabs container) owns the report/selectedYear/ranking/missing-days, the day-write intents, the drift threshold, and the Resolve badge count. It owns the store-change subscription, started on scene .active and cancelled on background — closing the leak. - ResolveModel (view-scoped in ResolutionView) owns the data-issue list + dismiss; BackupModel and RemindersSettingsModel (view-scoped in SettingsView) own backup progress and the reminder/summary editing surface. Injection is hybrid: ambient app state (WhereModel, the WhereSession coordinator) flows through the environment and is read non-optional so broken wiring is a loud crash; scoped models are passed explicitly via initializers so their wiring is compile-checked. Launch drops the loadYear step and no longer wires the subscription in syncAuth (the report loads with the scene). Tests move 1:1 with their concern (Backup/Reminders/Resolve/Report + a coordinator WhereSessionTests), and Where/AGENTS.md + the root AGENTS.md are updated to describe the new tiers and injection rule. Co-authored-by: Cursor <cursoragent@cursor.com>
After the WhereSession split, changing the drift threshold in Settings only recomputed the Resolve badge count (on ReportModel), not the issue list: the view-scoped ResolveModel re-scanned only via .task(id: report.report), which a threshold change doesn't mutate. The badge and list could then visibly disagree until the next committed write or year switch. WherePreferences isn't observable by design, so reading the threshold through it can't drive a body re-run. Mirror it in an observable stored property on ReportModel (as RemindersSettingsModel already does for its preferences), and key the Resolve list's scan .task on a single DataIssueScanInputs value (year + report + threshold). The list and the badge now recompute on the same triggers by construction, and the scanner cache keyed by (year, threshold) makes the two concurrent scans converge regardless of order. Co-authored-by: Cursor <cursoragent@cursor.com>
Moving the store-change subscription onto the scene-scoped ReportModel means a backgrounded scene stops refreshing. ReportModelTests covered observeDataChanges() refreshing on a write, but nothing asserted the pause/resume contract, so a future edit could silently reintroduce the leak and stay green. Add three tests: activate() subscribes and pulls (and stays live for a later committed write); deactivate() stops refreshing on a write — proven against a live probe on the same store, which makes the negative assertion deterministic (once the probe reacts to the shared ping, the paused model has demonstrably seen and ignored it); and a subsequent activate() re-subscribes and pulls the gap accumulated while backgrounded. Co-authored-by: Cursor <cursoragent@cursor.com>
RootView keyed MainTabs (and thus the scene-scoped ReportModel) on ObjectIdentifier(session), which is only unique among live objects: a session freed and a new one allocated at the same address could collide, leaving the scene un-rebuilt across a reset. Unlikely in practice — the old session is retained via the environment until the new tree replaces it — but the guarantee shouldn't rest on allocator behavior. Give WhereSession a process-unique, monotonically increasing SessionID minted at init and key MainTabs on session.id. A monotonic token is never reused within the process, so a rebuilt session always forces a fresh scene. Co-authored-by: Cursor <cursoragent@cursor.com>
RemindersSettingsModelTests covered the preferences round-trip but not that a setter actually drives services.reminders.configure / services.summary.configure down to the scheduler. The coordinator's launch/foreground apply path is tested in WhereSessionTests, but the live edit on the Settings surface wasn't — so a setter could stop reconciling and stay green. Inject spy schedulers and assert each setter reaches the reconciler: enabling prompts for authorization and reconciles on, disabling reconciles off without a prompt, and a time edit re-reconciles with the new time — for both the reminder and the daily-summary schedule. Co-authored-by: Cursor <cursoragent@cursor.com>
| /// threading the coordinator through. | ||
| @MainActor | ||
| @Observable | ||
| public final class ReportModel { |
There was a problem hiding this comment.
I don't think this name is very clear given this comment, should this be public final class YearReportModel or similar?
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Renamed ReportModel → YearReportModel across the module (source, tests, previews/fixtures, and Where/AGENTS.md). Commit 164685f.
The scene-scoped ReportModel scans for the tab badge on activate(), but the view-scoped ResolveModel only scans from ResolutionView's .task after the first render — so a populated Resolve tab could flash the "all clear" empty state for a frame while the badge already showed a non-zero count. Track hasLoaded on ResolveModel (set once the first scan lands, or when a fixture is seeded) and hold a spinner in ResolutionView until then, rather than the empty state. It stays true afterward, so later re-scans update the list in place with no spinner flicker. Co-authored-by: Cursor <cursoragent@cursor.com>
refreshDataIssueCount read the drift threshold straight from preferences.driftThresholdMeters, while the Resolve list scans on dataIssueScanInputs, which reads the observable driftThresholdStorage mirror. The two are kept in lockstep by the sole setter today, so this was already consistent — but reading the mirror in both places makes the badge/list agreement hold by construction rather than resting on a shared invariant, and it survives a future second writer of the preference. Co-authored-by: Cursor <cursoragent@cursor.com>
ReportModel.locations(in:) and representativeCoordinates() (migrated verbatim from WhereSession) used try? and returned []/[:] on failure, discarding the error — against the repo's "never silently swallow" rule, where an empty result reads as "no locations" indistinguishable from success. Replace with do/catch that logs a warning and returns empty, so the Elsewhere drill-in still renders nothing on failure but the failure is observable in the log rather than passing silently. Co-authored-by: Cursor <cursoragent@cursor.com>
| /// `services` / `preferences` / `now` / `calendar` are exposed so the | ||
| /// view-scoped models a tab builds (`BackupModel`, `RemindersSettingsModel`, | ||
| /// `ResolveModel`) can be constructed from the injected `report` without also | ||
| /// threading the coordinator through. |
There was a problem hiding this comment.
I have my eye on you!!! Let's work towards fixing this; we should thread it through so the compiler checks it. Let's add it to the TODOs.
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Left the @Environment lookup as-is for this PR and filed the compiler-checked follow-up as a P2 in Where/TODOs.md: thread the coordinator through initializers (like the scoped models already are) or use a non-defaulting typed EnvironmentKey so a broken wiring fails to build. Commit 07a88c8.
|
|
||
| /// Start-of-day keys for days that still need logging in the loaded year. | ||
| public var missingDayKeys: Set<Date> { | ||
| guard let report, isViewingCurrentYear else { return [] } |
There was a problem hiding this comment.
Hnnm this indicates to me we should still further split this type apart (nil type checks), lets file this away for further dealing.
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Filed a P2 in Where/TODOs.md to split YearReportModel further — separating the read-only presentation state from the write-intent (setManualDay(s), overrideDay, clear*) and Elsewhere drill-in (days(in:), locations(in:), representativeCoordinates()) surface. Commit 07a88c8.
The data-change observer re-pulls the report and then the badge count in two sequential awaits. The test waited only for the report to repopulate before asserting dataIssueCount > 0, so it could read the stale 0 in the window between the two awaits — fast enough to pass locally, but it lost the race on the loaded CI runner. Wait for the final state (report populated AND count > 0) instead of the intermediate one, so the assertion can't observe the gap. Co-authored-by: Cursor <cursoragent@cursor.com>
| case idle | ||
| case loading | ||
| case loaded | ||
| case failed(String) |
There was a problem hiding this comment.
Should this hold an error instead? Thoughts?
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Agreed. LoadState.failed(String) is now failed(LoadError), a typed enum with reportUnavailable(message:) / clearFailed(message:). It keeps the user-presentable text as a String (so LoadState stays Equatable for the refresh guards and tests) while naming which operation failed, so a caller can branch on it. Commit fff422c.
| /// Unlogged days this year (Jan 1 through today), collapsed into ranges, for | ||
| /// the warning banner and the backfill flow. Empty unless viewing the | ||
| /// current year, since past years can't gain "today" coverage. | ||
| public var missingDays: [MissingDayRange] { |
There was a problem hiding this comment.
I feel like the majority of the logic in this method should be moved into the report itself so we're not doing date math in the view model.
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Moved it down. The derivation now lives on YearReport in WhereCore (missingDayRanges(asOf:calendar:)), which handles the "current year only" rule and the today-exclusion; the view-model missingDays is now just report?.missingDayRanges(asOf: now(), calendar: calendar) ?? []. Commit c75f5cb.
| } | ||
|
|
||
| /// Start-of-day keys for days that still need logging in the loaded year. | ||
| public var missingDayKeys: Set<Date> { |
There was a problem hiding this comment.
Same comment as above, can we move this into report so this becomes a one liner w/ no logic?
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Same treatment — now report?.missingDayKeys(asOf: now(), calendar: calendar) ?? [], with the logic on YearReport in WhereCore. Commit c75f5cb.
| day: 15, | ||
| )), | ||
| let range = calendar.range(of: .day, in: .year, for: midYear) | ||
| else { return 365 } |
There was a problem hiding this comment.
Don't hardcode, pull from the calendar.
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Done. daysInSelectedYear now reads calendar.dayCount(ofYear: selectedYear) — a new Calendar extension in WhereCore that derives 365/366 from the calendar's own range(of: .day, in: .year, for:), so leap years come out right instead of a hardcoded literal. Commit c75f5cb.
|
|
||
| /// Number of calendar days in the selected year (365, or 366 in a leap | ||
| /// year). Region cards scale their ambient progress bar against this. | ||
| public var daysInSelectedYear: Int { |
There was a problem hiding this comment.
Same as above; can we move most of this down onto report?
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Pushed down to the same Calendar.dayCount(ofYear:) extension in WhereCore (with its own tests); the property is now a one-liner. Commit c75f5cb.
| public func activate() async { | ||
| observeDataChanges() | ||
| await refresh() | ||
| await refreshDataIssueCount(force: false) |
There was a problem hiding this comment.
Any reason for refresh and refreshDataIssueCount to be separate? I see we call them next to each other frequently.
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Added refreshAll(forceDataIssueCount:) to name the paired call the shared sites (activate(), select(year:), the store-change observer) use. Kept refresh() and refreshDataIssueCount(force:) separately callable on purpose: the drift-threshold setter recomputes only the badge count with no report re-pull, so folding them would force a redundant report fetch there. Commit 6ac4e08.
| #if DEBUG | ||
| /// Preview/test seam: inject a `ResolveModel` seeded via | ||
| /// `@_spi(Testing) setDataIssues` so the list renders without raw samples. | ||
| init(report: ReportModel, resolve: ResolveModel) { |
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Can't apply @_spi(Testing) here — the compiler only allows it on public/open declarations, and ResolutionView (and this DEBUG init) is internal ("internal initializer cannot be declared '@_spi'"). The seam stays gated behind #if DEBUG and is reached from tests via @testable import WhereUI, which is how the module's other internal test seams work. The one public cross-module seam in this area — ResolveModel.setDataIssues(_:) — is @_spi(Testing).
| /// Set by the `@_spi(Testing)` seeder so `load(...)` doesn't clobber | ||
| /// preview/test fixtures with an empty scan of a store that has no raw | ||
| /// samples. Never compiled into release. | ||
| private var isSeeded = false |
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
The actual cross-module test seam here — setDataIssues(_:) — is already @_spi(Testing) (public, in the DEBUG extension below). This line is the private var isSeeded flag it sets, and @_spi can't go on a private (or any non-public) declaration, so it stays internal to the #if DEBUG seam.
| /// tunes the border-drift relabel suggestions. | ||
| public func load(year: Int, primaryRegions: [Region]) async { | ||
| #if DEBUG | ||
| if isSeeded { return } |
There was a problem hiding this comment.
Hmm seeing this now; I don't love this short circuit since it means our tests aren't exercising this path. Any way to actually trigger this instead of adding the isSeeded override?
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Good call. Reworked dismissWritesToStoreAndRemovesRow to seed two calendar-adjacent manual days with disjoint regions (CA then NY), which makes the scanner return a genuine dismissible abrupt-change issue — so dismiss now runs the real load(...) scan instead of a setDataIssues fixture, and the isSeeded short-circuit is no longer on the tested path. The seam keeps its own narrow test (seedingMarksTheModelLoaded) and stays preview-only. Commit 7050bd6.
| /// outlive the screen that drives it. | ||
| @MainActor | ||
| @Observable | ||
| public final class BackupModel { |
There was a problem hiding this comment.
This still feels like a "fat" view model to me, lots of percent math, etc. Can we push more of this down into WhereCore?
There was a problem hiding this comment.
Posted by an AI agent on kve's behalf.
Pushed the export-file lifecycle down into BackupCoordinator (WhereCore): it now tracks the previous temp export directory and purges it on the next export, with a test (exportPurgesThePreviousExportDirectory). BackupModel no longer owns that previousExportDirectory state or the cleanup logic. Commit 2697578.
The scene-scoped model is specifically the selected *year*'s report and everything derived from it, so `YearReportModel` reads more clearly than the generic `ReportModel`. Pure rename (type, file, preview factories, and docs) — no behavior change. Addresses PR review: "should this be `YearReportModel` or similar?" Co-authored-by: Cursor <cursoragent@cursor.com>
The missing-day derivation (present-day set + backlog cutoff, current-year
only) now lives on `YearReport.missingDay{Keys,Ranges,Count}(asOf:calendar:)`,
and the year-length math on `Calendar.dayCount(ofYear:)`. `YearReportModel`
just supplies "now" + the calendar, so its `missingDays` / `missingDayKeys` /
`missingDayCount` / `daysInSelectedYear` are one-liners with no date math.
`daysInSelectedYear` also drops the hardcoded `365`: the count is derived from
the calendar's day-of-year range, with the literal now only an asserted release
fallback for a calendar that can't resolve a date.
Addresses PR review: move the date math onto `report`; don't hardcode 365.
Co-authored-by: Cursor <cursoragent@cursor.com>
The export staging-directory bookkeeping — track the last export and delete it when the next one starts — moves from `BackupModel` into the `BackupCoordinator` actor, which already owns export. This also fixes a silently-swallowed `try?` on the cleanup (now logged) and makes the cleanup survive the Settings view being torn down, since the coordinator lives for the app. `BackupModel` keeps only the export/import UI state. Addresses PR review: push more of this "fat" view model down into WhereCore. Co-authored-by: Cursor <cursoragent@cursor.com>
`LoadState.failed` now carries a typed, `Equatable` `LoadError` (`reportUnavailable` / `clearFailed`) instead of a bare `String`, so the failing operation is named in the type and tests can match the case. The error screens read `error.message`; the value stays a `String` (not `any Error`) so `LoadState` remains `Equatable` for the refresh guards. Adds a `failSamples()` hook to `TestStore` and a test that a failed load parks in `.reportUnavailable`. Addresses PR review: "should this hold an error instead?" Co-authored-by: Cursor <cursoragent@cursor.com>
`activate()`, `select(year:)`, and the store-change observer all pulled a fresh report and recounted the Resolve badge back to back; give that pairing a name (`refreshAll(forceDataIssueCount:)`). `refresh()` and `refreshDataIssueCount(force:)` stay separately callable — the drift-threshold setter recomputes only the count, with no report re-pull. Addresses PR review: "any reason refresh and refreshDataIssueCount are separate?" Co-authored-by: Cursor <cursoragent@cursor.com>
`dismissWritesToStoreAndRemovesRow` seeded a fixture via `setDataIssues` and then dismissed it, so the `isSeeded` short-circuit meant the test never ran the actual `load(...)` scan. Seed the store with two calendar-adjacent, disjoint manual days instead — the scanner returns a genuine dismissible abrupt-change issue, and dismiss now exercises the production path. The seam keeps its own narrow test (`seedingMarksTheModelLoaded`) and stays preview-only. Addresses PR review: "tests aren't exercising this path — any way to trigger it instead of the isSeeded override?" Co-authored-by: Cursor <cursoragent@cursor.com>
Two review threads flagged follow-up work that's out of scope for this PR: compiler-checked coordinator wiring (vs the silent `@Environment` lookup) and a further split of `YearReportModel` (presentation state vs write-intent/drill-in surface). Record both under P2 so they're tracked rather than lost in the PR. Co-authored-by: Cursor <cursoragent@cursor.com>
…-scope-split Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # Where/AGENTS.md
1 participant
Summary
WhereSessionhad grown into an ~850-line observable that mixed always-on concerns with UI mirrors and lived for the whole logged-in lifetime — so its store-change subscription drove report/data-issue refreshes even on a headless background relaunch that renders nothing. This splits it by lifetime scope and fixes the leak.WhereSession→ slim always-on coordinator. Tracking/authorization, launch-time reminder/summary application (readingWherePreferences), widget republish, and erase. No presentation state, no store-change subscription.YearReportModel(scene-scoped,@Statein a newMainTabs). Owns the report / selected year / ranking / missing days, the day-write intents, the drift threshold, and the Resolve badge count. It owns the store-change subscription, started on scene.activeand cancelled on.background— closing the leak.ResolveModel(the data-issue list + dismiss, inResolutionView);BackupModelandRemindersSettingsModel(backup progress and the reminder/summary editing surface, inSettingsView).WhereModel, theWhereSessioncoordinator) flows through the environment and is read non-optional so broken wiring is a loud crash; scoped models are passed via initializers so their wiring is compile-checked.loadYearstep and no longer wires the subscription insyncAuth— the report loads with the scene.Backup/Reminders/Resolve/YearReport+ a coordinatorWhereSessionTests);Where/AGENTS.md+ rootAGENTS.mddescribe the new tiers and injection rule.Code-review follow-ups (round 1, separate commits)
YearReportModeland key the Resolve scan.taskon a singleDataIssueScanInputs(year + report + threshold), so list and badge recompute on the same triggers.deactivate()stops refreshing on a committed write (proven against a live probe on the same store) and a lateractivate()re-subscribes and pulls the gap — so the leak can't silently return.RootViewkeyedMainTabsonObjectIdentifier(session)(unique only among live objects).WhereSessionnow mints a process-uniqueSessionID; a rebuilt session can't collide with a freed one at the same address.RemindersSettingsModelsetter drivesconfigure(...)down to the scheduler (enable prompts + reconciles on, disable reconciles off without a prompt, time edit re-reconciles).Code-review follow-ups (round 2, separate commits)
ReportModel→YearReportModel. The old name didn't convey that it's scoped to the selected year; renamed across sources, tests, previews/fixtures, andAGENTS.md.WhereCore. Missing-day derivation moved ontoYearReport(missingDayRanges/Keys/Count(asOf:calendar:)) and a newCalendar.dayCount(ofYear:)(derives 365/366 from the calendar, no hardcoded literal). The view-model properties are now one-liners.WhereCore.BackupCoordinatornow tracks and purges the previous temp export directory (survives the Settings screen being torn down);BackupModelno longer holds that state or percent math.LoadState.failed.failed(String)→failed(LoadError)(reportUnavailable/clearFailed), so callers can branch on which operation failed whileLoadStatestaysEquatable.refreshAll(forceDataIssueCount:)helper names the paired report + badge refresh the shared sites use;refresh()andrefreshDataIssueCount(force:)stay separately callable (the drift-threshold setter recounts only the badge).ResolveModeldismiss test through a real scan. It now seeds two calendar-adjacent disjoint manual days so the scanner returns a genuine dismissible issue, exercising the productionload(...)path instead of theisSeededfixture short-circuit (which keeps its own narrow test and stays preview-only).Where/TODOs.md: compiler-checked coordinator wiring (vs the@Environmentlookup) and a further split ofYearReportModel.Merged
mainin (picks up the new Foreman targets + the slimmedAGENTS.mdfiles); resolved theWhere/AGENTS.mdconflict by keeping the slim structure and correcting only the sentences the split made stale.Test plan
./swiftformat --lintcleantuist test WhereUITestsgreen locally (incl.YearReportModelTestspause/resume +RemindersSettingsModelTestsreconcile + the reworkedResolveModelTestsdismiss test)tuist test WhereCoreTestsgreen locally (incl. newCalendar+DayCountTests,YearReport+MissingDaysTests, and theBackupCoordinatorpurge test)macos-26):tuist test --no-selective-testingacross all schemesMade with Cursor