Conversation
WalkthroughIntroduces gateway battery notification support by creating a new Changes
Sequence DiagramsequenceDiagram
participant StationAlertsManager
participant DeviceDetails
participant StationNotificationsUseCase
participant AlertNotification
StationAlertsManager->>DeviceDetails: Check gatewayBatteryState
alt Gateway Battery is Low
StationAlertsManager->>StationNotificationsUseCase: isNotificationEnabled(.gwBattery, deviceId)
StationNotificationsUseCase-->>StationAlertsManager: true/false
alt Notification Enabled & Not Sent Today
StationAlertsManager->>AlertNotification: Create gwBattery alert
AlertNotification-->>StationAlertsManager: Alert created
StationAlertsManager->>StationAlertsManager: Append to alerts array
end
end
Note over StationAlertsManager: Mirrors existing low-battery<br/>notification flow for gateway
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The changes involve consistent refactoring across multiple layers (presentation, domain, API) with a clear pattern: renaming Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
PresentationLayer/UIComponents/Screens/WeatherStations/StationDetails/Notifications/StationNotificationsTypes.swift (1)
11-44: LGTM! Extension updated to use the new switch options type.The refactoring from
StationNotificationsTypestoStationNotificationsSwitchOptionsis consistent. The device-specific logic (non-Helium devices exclude firmware update notifications) is preserved.Consider renaming this file to
StationNotificationsSwitchOptions+Extensions.swiftto better reflect that it now extendsStationNotificationsSwitchOptionsrather thanStationNotificationsTypes. This would improve code navigation and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
PresentationLayer/Extensions/DomainExtensions/StationAlert+.swift(2 hunks)PresentationLayer/UIComponents/Screens/WeatherStations/StationDetails/Notifications/StationNotificationsTypes.swift(2 hunks)PresentationLayer/UIComponents/Screens/WeatherStations/StationDetails/Notifications/StationNotificationsViewModel.swift(3 hunks)PresentationLayer/Utils/StationAlertsManager.swift(2 hunks)wxm-ios/DomainLayer/DomainLayer/UseCases/StationNotificationsUseCase.swift(4 hunks)wxm-ios/DomainLayer/DomainLayer/UseCases/UseCaseApi/StationNotificationsUseCaseApi.swift(1 hunks)wxm-ios/Resources/Localizable/Localizable.xcstrings(7 hunks)wxm-ios/Resources/Localizable/LocalizableString+Errors.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
PresentationLayer/Utils/StationAlertsManager.swift (3)
wxm-ios/DomainLayer/DomainLayer/UseCases/StationNotificationsUseCase.swift (1)
isNotificationEnabled(53-56)wxm-ios/DomainLayer/DomainLayer/UseCases/MeUseCase.swift (1)
lastNotificationAlertSent(174-179)WeatherXMTests/PresentationLayer/MockUseCases/MockMeUseCase.swift (1)
lastNotificationAlertSent(192-194)
PresentationLayer/UIComponents/Screens/WeatherStations/StationDetails/Notifications/StationNotificationsViewModel.swift (1)
PresentationLayer/UIComponents/Screens/WeatherStations/StationDetails/Notifications/StationNotificationsTypes.swift (1)
casesForDevice(38-44)
wxm-ios/DomainLayer/DomainLayer/UseCases/UseCaseApi/StationNotificationsUseCaseApi.swift (2)
wxm-ios/DomainLayer/DomainLayer/UseCases/StationNotificationsUseCase.swift (4)
areNotificationsEnabledForDevice(23-30)setNotificationsForDevice(32-36)setNotificationEnabled(38-51)isNotificationEnabled(53-56)WeatherXMTests/DomainLayer/UseCases/StationNotificationsUseCaseTests.swift (1)
setNotificationsForDevice(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WeatherXM | Unit tests | wxm-ios - iOS
🔇 Additional comments (7)
PresentationLayer/Utils/StationAlertsManager.swift (1)
112-124: LGTM! Gateway battery notification logic is correct.The implementation correctly mirrors the existing
shouldSendLowBatteryNotificationpattern. Note that checking.batteryon line 114 (rather than a non-existent.gwBatteryswitch option) is intentional—gateway battery shares the same UI switch as regular battery but sends a distinct.gwBatteryalert type.wxm-ios/DomainLayer/DomainLayer/UseCases/StationNotificationsUseCase.swift (2)
10-10: LGTM! Typealias updated to reflect new switch options type.The change from
StationNotificationsTypestoStationNotificationsSwitchOptionscorrectly reflects the architectural separation between UI switch options (4 cases) and alert types (5 cases includinggwBattery).
38-56: LGTM! Public API updated consistently.Both methods correctly use
StationNotificationsSwitchOptionsfor the type parameter. The default behavior (missing options default totrueon line 55) is preserved.PresentationLayer/Extensions/DomainExtensions/StationAlert+.swift (1)
21-22: LGTM! Gateway battery notification strings wired correctly.The new
gwBatterycase is properly integrated into bothnotificationTitleandnotificationDescription(for:), referencing the newly added localization keys.Also applies to: 36-37
wxm-ios/DomainLayer/DomainLayer/UseCases/UseCaseApi/StationNotificationsUseCaseApi.swift (1)
10-23: LGTM! Clean separation between switch options and alert types.The introduction of
StationNotificationsSwitchOptions(UI switches, 4 cases) alongside the expandedStationNotificationsTypes(alert types, 5 cases) is well-designed. Gateway battery notifications share the battery switch but send a distinctgwBatteryalert type, which is logical and maintains UI simplicity.wxm-ios/Resources/Localizable/LocalizableString+Errors.swift (1)
80-81: LGTM! Gateway battery localization strings added correctly.The new
notificationGwBatteryTitleandnotificationGwBatteryDescriptioncases follow the existing pattern for notification localization, including proper string interpolation support for the station name.Also applies to: 108-108, 256-259
PresentationLayer/UIComponents/Screens/WeatherStations/StationDetails/Notifications/StationNotificationsViewModel.swift (1)
19-22: LGTM! View model consistently updated to use switch options type.All properties, methods, and helpers are correctly updated from
StationNotificationsTypestoStationNotificationsSwitchOptions. The analytics tracking and option persistence logic are preserved.Also applies to: 34-45, 76-87
| "state" : "translated", | ||
| "value" : "Low GW battery" | ||
| "value" : "Low Gateway battery" | ||
| } |
There was a problem hiding this comment.
Restore title casing for consistency.
Line 5366 currently reads “Low Gateway battery”, but every other alert title keeps Title Case (“Low Gateway Battery”). Please align the casing:
- "value" : "Low Gateway battery"
+ "value" : "Low Gateway Battery"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "state" : "translated", | |
| "value" : "Low GW battery" | |
| "value" : "Low Gateway battery" | |
| } | |
| "state" : "translated", | |
| "value" : "Low Gateway Battery" | |
| } |
🤖 Prompt for AI Agents
In wxm-ios/Resources/Localizable/Localizable.xcstrings around lines 5365 to
5367, the alert title uses inconsistent casing ("Low Gateway battery"); change
the value string to Title Case ("Low Gateway Battery") to match other alert
titles and keep localization consistency.
| "state" : "translated", | ||
| "value" : "To avoid data interruptions, please make sure the Gateway of %@ is properly exposed to sunlight." | ||
| } |
There was a problem hiding this comment.
Tighten the new gateway notification copy.
“Gateway of %@” feels awkward in English. Switching to the possessive keeps the voice consistent with the rest of the file:
- "value" : "To avoid data interruptions, please make sure the Gateway of %@ is properly exposed to sunlight."
+ "value" : "To avoid data interruptions, please make sure %@’s gateway is properly exposed to sunlight."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "state" : "translated", | |
| "value" : "To avoid data interruptions, please make sure the Gateway of %@ is properly exposed to sunlight." | |
| } | |
| "state" : "translated", | |
| "value" : "To avoid data interruptions, please make sure %@'s gateway is properly exposed to sunlight." | |
| } |
🤖 Prompt for AI Agents
In wxm-ios/Resources/Localizable/Localizable.xcstrings around lines 6751 to
6753, the string "To avoid data interruptions, please make sure the Gateway of
%@ is properly exposed to sunlight." uses an awkward "Gateway of %@" phrasing;
change it to the possessive form to match the file's voice—e.g. "To avoid data
interruptions, please make sure %@'s Gateway is properly exposed to
sunlight."—or alternatively "To avoid data interruptions, please make sure the
%@ Gateway is properly exposed to sunlight.", and update the localized value
accordingly.
Code Coverage Summary
|
pantelisss
deleted the
feature/fe-2028-update-low-battery-messages-for-pulse
branch
3 participants
Why?
Update texts for pulse station alerts
How?
Testing
Run the app with mock scheme and ensure the warnings in the station alerts screen are the expected
It's a little tricky. Run the mock scheme on a real device. Pause the debugger (screenshot) and then run the following command and ensure the texts on each notification are the expected.
If it's not easy to run it on a real device, check it codewise
Screenshots (if applicable)
Additional context
fixes fe-2028
Summary by CodeRabbit
New Features
Documentation