Replace SelectedTab force unwrap with safe fallback#1052
Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Divesh, thanks for taking the time to make this fix to UserDataStore.lastSelectedView. You're right that force-unwrapping a failable initializer on data coming from UserDefaults is a crash waiting to happen, and your instinct to use nil-coalescing with .map is consistent with how the rest of the codebase handles this (e.g., MKMapType(rawValue:) ?? .mutedStandard in MapRegionManager, RouteType(rawValue:) ?? .unknown in Route, etc.). Nice catch. Before we can merge this, I will need you to make a couple changes:
Important
-
Add a log message when the fallback is used. The change as written silently discards the invalid raw value, which makes it impossible to diagnose tab preference issues in production. This file already establishes the convention of logging before falling back — see line 557 (
Logger.error("Unable to decode recent map items: \(error)")) and line 777 (Logger.error("Unable to decode \(key): \(error)")). Please follow the same pattern here. Something like:guard let tab = SelectedTab(rawValue: raw) else { Logger.warn("Invalid SelectedTab raw value \(raw) in UserDefaults. Falling back to .map.") return .map } return tab
Fit and Finish
-
Add a test for the invalid raw value fallback. The existing tests cover the default and the happy path. Please add a test that writes an out-of-range integer directly to UserDefaults and asserts the fallback to
.map:func test_selectedTabIndex_invalidRawValueFallsBackToMap() { userDefaults.set(999, forKey: "UserDataStore.lastSelectedView") expect(self.userDefaultsStore.lastSelectedView) == SelectedTab.map }
Thanks again, and I look forward to merging this change.
There was a problem hiding this comment.
Hey Divesh — both items from my earlier review are addressed exactly as requested. The guard let with Logger.warn matches the existing error-logging convention in UserDefaultsStore, and the test for the invalid raw value fallback covers the edge case cleanly. This looks good to merge. Thanks for the quick turnaround!
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Divesh — both items from my earlier review are addressed exactly as requested. The guard let with Logger.warn matches the existing error-logging convention in UserDefaultsStore, and the test for the invalid raw value fallback covers the edge case cleanly. This looks good to merge. Thanks for the quick turnaround!
Summary
Replace
SelectedTab(rawValue: raw)!withSelectedTab(rawValue: raw) ?? .mapon line 667 ofUserDataStore.swift.Problem
SelectedTab(rawValue:)is a failable initializer — it returnsnilfor any raw value that doesn't match a case. While theguardon line 663 reduces the risk, the force unwrap is still unsafe if:SelectedTabenum cases are reordered or removed in a future updateUsing
?? .mapis consistent with the existing fallback on line 664, which already defaults to.mapwhen the key is absent.Fix