Data sources attribution + roaming alert-filter defaults#93
Merged
Conversation
Credit all six external providers, not just Open-Meteo: Open-Meteo, Apple Weather (WeatherKit), NWS/NOAA, Environment and Climate Change Canada, MeteoAlarm (EUMETNET), and Apple Maps. Replaces the single Open-Meteo link in Settings > About with a dedicated screen grouping providers by what they power, each an accessible link. Also fix a stale comment in RegionalWeatherService that referenced Nominatim's rate limit; the code uses Apple CLGeocoder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a "Save Current Filters as Default" button at the bottom of the alert digest page. It stores the current severity filter and hazard type, which are re-applied each time the alert browser opens. The defaults live in AppSettings (raw-value strings, keeping Models decoupled from the service-layer SeverityFilter/HazardType enums), so they persist locally and roam across devices via the existing iCloud KVS sync — no iCloudSyncService changes needed. This is the first setting that lives outside the Settings page. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kellylford
commented
Jul 8, 2026
kellylford
left a comment
Owner
Author
There was a problem hiding this comment.
Review: Data sources attribution + roaming alert-filter defaults
Reviewed the full diff plus surrounding code (Settings.swift, SettingsManager.swift, iCloudSyncService.swift, AlertBrowserService.swift, AlertBrowserView.swift, BrowseCitiesView.swift, SettingsView.swift). This is a clean, well-scoped PR. I could not find any blocking or crash-level defect. Details below by severity.
Verified correct (the things most likely to break, that don't)
- Codable roundtrip in
AppSettings.CodingKeys,init(from:), andencode(to:)are all consistent for both new properties.encodeIfPresent/decodeIfPresentondefaultAlertHazardTypepreserves nil vs. non-nil correctly. Additive fields decoded withdecodeIfPresent+ nocurrentVersionbump = safe both directions for older/newer stored blobs. Not bumpingcurrentVersionis the right call here:applyRemoteSettings()gates onremote.settingsVersion == currentVersion, so a bump would have made pre-existing (v3) blobs stop roaming. - Roaming/iCloud. Confirmed the claim.
saveSettings()→iCloudSyncService.pushSettings()→NSUbiquitousKeyValueStore.set(encodedAppSettings). The two new fields ride inside the encoded blob, so they roam with noiCloudSyncServicechange. Correct. SeverityFilter.extremeOnly = "Extreme"case-name/raw-value mismatch. Storing/reconstructing byrawValueworks —RawRepresentablekeys off the raw value, not the case name, soSeverityFilter(rawValue: "Extreme")reliably yields.extremeOnly. The PR usesrawValuethroughout. No issue.- State seeding. The
didApplyDefaultsonce-guard is correct:onAppearseeds synchronously before the async.taskload renders the filters (no race), and the guard preserves in-session filter changes when navigating into a group detail and back. Good. DataSourceRowaccessibility. Textbook:.accessibilityElement(children: .ignore), explicit label + hint,.isLink, decorative arrow image hidden. Matches CLAUDE.md's non-negotiable a11y pattern.
Should-fix
AlertBrowserView.swift~L277-283 — saved hazard type absent from current region shows a blank Type picker. If a user saves a default hazard type (say.fire) and later opens a region with no fire alerts,presentFamiliesomits.fire, so the menuPickerbound to$hazardTypehas no tag matching the current selection. The picker renders with a blank/empty selected label while the list is silently filtered to empty. The "No alerts match this filter. Try All types." message mitigates it, but the control not reflecting the active filter is confusing and it degrades the very feature being added. Fix: include the selectedhazardTypein the menu even when its count is 0 (e.g. append it topresentFamiliesif missing), or resethazardType = nilinapplySavedDefaultsIfNeeded()when the saved type isn't present in the freshly loaded alerts.
Nits
SettingsView.swift~L543-548 — decorativecheckmark.seal.fillnot hidden. The NavigationLink's explicit.accessibilityLabeloverrides the combined children so VoiceOver still reads correctly, but CLAUDE.md says all decorative images need.accessibilityHidden(true). Add it for consistency.SettingsView.swift~L1099 —.accessibilityAddTraits(.isStaticText)on the introTextis redundant (Text already exposes static-text semantics). Harmless; can drop.SettingsView.swiftDataSourceRowURL(string:)!force-unwrap. Fine for hardcoded literals and matches the pre-existing pattern that was replaced, but a future typo in aurlStringbecomes a crash. Low priority; aguard let/EmptyViewfallback would be safer.AlertBrowserView.swiftL248-251 — thejustSavedDefaultsreset is an unstructuredTask {}that isn't cancelled on disappear. Setting@Stateafter the view is gone is a no-op, so this is harmless, but a.task-scoped timer orTaskcancellation would be tidier.
Summary
Solid PR. No blocking issues. The Codable and roaming mechanics — the highest-risk parts — are correct. One should-fix UX edge case around a saved hazard filter not present in the current region, plus a few minor a11y/style nits.
If a saved default hazard type isn't present in the current region's alerts, keep it in the Type menu (shown as "Name (0)") so the picker reflects the active filter instead of rendering blank. Also mark the Data Sources icon accessibilityHidden and drop a redundant .isStaticText trait, per review nits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related Settings/data improvements, plus a small cleanup.
1. Data Sources & Attribution screen (Settings → About)
The app credited only Open-Meteo, but it pulls from six providers. Replaced the single Open-Meteo link with a dedicated Data Sources & Attribution screen grouping providers by what they power:
Each row is an accessible link (
children: .ignore, explicit label + hint,.isLink).2. Roaming alert-filter defaults
A "Save Current Filters as Default" button at the bottom of the alert digest page saves the current severity filter + hazard type. Re-applied each time the alert browser opens. Stored in
AppSettingsas raw-value strings, so it persists locally and roams across devices via the existing iCloud KVS sync (noiCloudSyncServicechanges). First setting that lives outside the Settings page.3. Cleanup
Fixed a stale comment in
RegionalWeatherServicethat referenced Nominatim's rate limit; the code actually uses AppleCLGeocoder.Version
Bumped to 1.5.8 (3).
Test plan
xcodebuildDebug build for iPhone 17 simulator succeeds🤖 Generated with Claude Code