Skip to content

Improvements to critical alarm handling#38

Open
ETolboom wants to merge 6 commits intoLoopKit:mainfrom
ETolboom:improved-critical-alerts
Open

Improvements to critical alarm handling#38
ETolboom wants to merge 6 commits intoLoopKit:mainfrom
ETolboom:improved-critical-alerts

Conversation

@ETolboom
Copy link
Copy Markdown

@ETolboom ETolboom commented Apr 21, 2026

This commit adds several improvements to the current handling of critical alerts:

  • A "Do Not Disturb" override toggle has been added to each individual schedule.
  • A banner for requesting critical alert permissions is shown if shouldOverrideDoNotDisturb is explictly set to true by the user in NotificationHelperOverride
  • Critical Alert Volume has been moved from its own sub-menu into the Alarm Settings view

Disclaimer: I have made use of A.I. in order to implement this feature as I am not primarily a Swift developer (primarily Golang). I have critically reviewed every line of code to ensure that the solution is minimal and correct.

I've also ensured that everything related to settings and state is handled in NotifcationHelper and nothing is mutated from the UI side.

I was unsure how to approach Localization as I see Lokalise is used.

Screenshots in app 01_CGM 02_NO_ENTITLEMENT 03_ENTITLED 04_PERMISSION 05_SCHEDULE_1 06_CRITICAL_ALERT_VOLUME

Add Do Not Disturb override toggle to each schedule
Add banner for requesting critical alert permissions if shouldOverrideDoNotDisturb is true
Move Critical Alert Volume into Alarm Settings view
Copilot AI review requested due to automatic review settings April 21, 2026 23:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors and extends “critical alert” support by moving the critical volume control into Alarm Settings, adding a per-schedule Do Not Disturb override, and introducing a permission-request banner (gated by NotificationHelperOverride).

Changes:

  • Add per-schedule overrideDoNotDisturb and propagate it through persistence/state.
  • Add a Critical Alerts permission banner + request flow, and a conditional critical volume section in Alarm Settings.
  • Update notification sending so glucose alarms are only “critical” when the active schedule requests DND override.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
LibreTransmitterUI/Views/Settings/SettingsView.swift Removes the standalone navigation entry for critical volume (moving it into Alarm Settings).
LibreTransmitterUI/Views/Settings/AlarmSettings/CriticalAlarmsVolumeView.swift Replaces the old view with reusable sections: permission banner + critical volume slider section.
LibreTransmitterUI/Views/Settings/AlarmSettings/AlarmSettingsView.swift Adds schedule-level DND override UI/state and conditionally shows the critical volume section.
LibreTransmitter/NotificationHelperOverride.swift Exposes override flag publicly for UI usage.
LibreTransmitter/NotificationHelper.swift Adds critical-alert permission/status helpers; makes glucose notifications critical only when schedule override is enabled.
Common/Settings/GlucoseSchedules.swift Adds persisted overrideDoNotDisturb to schedules and logic to determine when it should apply.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread LibreTransmitterUI/Views/Settings/AlarmSettings/CriticalAlarmsVolumeView.swift Outdated
Comment thread LibreTransmitterUI/Views/Settings/AlarmSettings/CriticalAlarmsVolumeView.swift Outdated
Comment thread LibreTransmitterUI/Views/Settings/AlarmSettings/CriticalAlarmsVolumeView.swift Outdated
Comment thread LibreTransmitterUI/Views/Settings/AlarmSettings/CriticalAlarmsVolumeView.swift Outdated
Comment thread LibreTransmitter/NotificationHelperOverride.swift Outdated
Comment on lines 389 to +392
AlarmHighRow(schedule: schedule, glucoseUnit: glucoseUnit, glucoseUnitDesc: glucoseUnitDesc, errorReporter: errorReporter)
if criticalAlertsEnabled {
OverrideDoNotDisturbRow(schedule: schedule)
}
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The per-schedule "Override Do Not Disturb" toggle is only rendered when criticalAlertsEnabled is true. This means users can’t preconfigure overrides before enabling the permission (and it also partially contradicts the PR description that the toggle is added to each schedule). Consider always showing the row but disabling it (or showing explanatory text) when Critical Alerts aren’t enabled.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think this is still proper.

If the user does not have the permission, the toggles nor the volume slider have any effect. Only after the user has set shouldOverrideDoNotDisturb to true and given critical alert permissions should the functionality be available.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Override Do Not Disturb" might be confusing to have enabled on a per schedule basis, it should ideally be set for all schedules at once. I might be misunderstanding the intention or use-case though. My intention is that a user will rather use the "silence/snooze" feature for situations where no audible alarms are required.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Override Do Not Disturb" might be confusing to have enabled on a per schedule basis, it should ideally be set for all schedules at once. I might be misunderstanding the intention or use-case though. My intention is that a user will rather use the "silence/snooze" feature for situations where no audible alarms are required.

Hi,

I had envisioned it like this:
Schedule 1 and schedule 2 would represent day time and night time schedules, respectively.

Schedule 1 will present HIGH and LOW alarms as a normal notification.

Schedule 2 will present the same but then with audible alarms through the use of critical alerts.

My reasoning was that for overnight lows its critical I hear them. Critical alerts allow a minimum volume and can bypass any focus profiles. During the day I'm usually on my phone enough (or apple watch) such that the notifications will tell me enough. Furthemore, I often have classes and meetings during the day so that requires my phone to be on silent.

}

public static func requestCriticalAlertPermission(completion: @escaping (Bool) -> Void) {
UNUserNotificationCenter.current().requestAuthorization(options: [.badge, .sound, .alert, .criticalAlert]) { _, _ in
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requestCriticalAlertPermission discards the granted and error values returned by requestAuthorization. Using those values would allow the UI to show a more accurate message (e.g. denied vs. missing entitlement) and avoid an extra round-trip in some cases.

Suggested change
UNUserNotificationCenter.current().requestAuthorization(options: [.badge, .sound, .alert, .criticalAlert]) { _, _ in
UNUserNotificationCenter.current().requestAuthorization(options: [.badge, .sound, .alert, .criticalAlert]) { granted, error in
if let error {
logger.debug("\(#function) failed to request critical alert permission: \(error.localizedDescription)")
criticalAlarmsEnabled = false
DispatchQueue.main.async {
completion(false)
}
return
}
guard granted else {
logger.debug("\(#function) critical alert permission denied")
criticalAlarmsEnabled = false
DispatchQueue.main.async {
completion(false)
}
return
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is meaningful.

requestCriticalAlertPermission is called from the CriticalAlertsBannerSection which already shows a proper message in case the result is not .enabled

If the user denied it then it is clear what the reason is. Otherwise there is only possible reason left: the app was not built with the correct entitlements.

Comment thread LibreTransmitterUI/Views/Settings/AlarmSettings/AlarmSettingsView.swift Outdated
@ETolboom
Copy link
Copy Markdown
Author

Will review the Copilot suggestions tomorrow

ETolboom and others added 5 commits April 22, 2026 10:58
…VolumeView.swift

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…calAlertsBannerSection) into AlarmSettingsView
…dules changes not being detected

Always show volume section if critical alerts permission been granted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants