Fix double rest-timer alert on native (OS notification + client beep firing together) - #78
Merged
Merged
Conversation
…ication) fireTimerNotification() was unconditionally playing a client-side haptic + Web Audio beep on native, on top of the OS local notification that scheduleBackgroundTimerNotification() already scheduled for the same expiry (with its own sound file and system alert vibration). notifyTimerExpired() only cancels that scheduled notification *after* detecting expiry client-side (via the 250ms poll or the visibilitychange reconciler) — i.e. at or after the exact instant the OS notification is due to fire — so the cancel essentially never won the race. Every rest timer completion on the native app produced two audible/haptic alerts. Fix: before playing the client-side cue, check whether the OS notification will actually alert (LocalNotifications.checkPermissions()). If it will, skip the redundant haptic/beep and let the OS notification be the sole alert. If notification permission isn't granted, keep the client-side fallback since nothing else will cue the user. fireTimerNotification() is now async to support the permission check; notifyTimerExpired() and its four call sites await it accordingly. Adds a regression test in native-platform.spec.ts asserting no vibrate/haptic and no oscillator (beep) calls when the OS will alert; verified it fails without the fix and passes with it.
… fix Follow-up to the previous commit's native double-alert fix. Five rounds of adversarial code review surfaced further issues in that fix and in the surrounding pre-existing expiry-detection code; this addresses all confirmed findings, each backed by a regression test that fails without its fix and passes with it. notifications.ts: - fireTimerNotification()'s "will the OS alert" check now queries the OS's own state (getPending/getDeliveredNotifications) instead of an in-memory "did schedule() succeed" flag, which reset on every page reload even though the OS-scheduled notification itself survives one. - scheduleBackgroundTimerNotification() clears stale delivered notifications before scheduling a new one — the fixed notification id meant a previous timer's delivered entry never expired, permanently masking a later schedule() failure. - fireTimerNotification() now returns whether cancellation is still warranted, and skips cancelling when the OS is trusted to alert (pending or already delivered) — unconditionally cancelling a still- pending notification we'd just decided to trust would silently prevent it from ever firing. - cancelBackgroundTimerNotification() is now properly awaitable instead of fire-and-forget, closing (not just narrowing) the race where a fast next rest timer's schedule() call, reusing the same fixed notification id, could be wiped out by a late-arriving cancel meant for the previous timer. - Promise.all -> Promise.allSettled when checking pending/delivered state, so one call rejecting doesn't discard a result the other call already provided. workout.ts: - The four expiry-detection call sites now share one handleTimerExpiry helper instead of a duplicated guard+cleanup sequence, with a try/catch that resets the guard on failure so a transient IndexedDB write error can't permanently block every future expiry. - timerExpiryHandled (the guard closing the race between the poll, visibilitychange, and recovery-interval detectors) is scoped per renderWorkout() call, not module-level — a module-level guard let a stale render's detector (left running when the workout screen is left via a raw route change instead of the in-app Back button) win the race and permanently block the *live* render's own detector from updating the visible page. - The resumed-timer recovery interval now clears any pre-existing timerInterval before overwriting it — otherwise a stale render's interval keeps running unowned, and when it later notices the same expiry and clears "timerInterval" itself, it cancels the live render's interval instead (same shared module variable). Verified via bun run typecheck, eslint, and the full Playwright suite (275 tests) on chromium, plus repeated runs of every new/changed regression test to rule out flakiness in either direction.
…leanup CI (iphone-webkit shard) caught a real regression from the previous commit's stale-delivered-notification fix: awaiting removeAllDeliveredNotifications() before schedule() delayed the moment a new timer's notification actually became pending/cancellable. A cancelBackgroundTimerNotification() call fired immediately after (e.g. skip-timer-btn, or the "cancelling before the fire time prevents the notification" test) could now complete before schedule() had even run, cancelling nothing — leaving the notification to fire anyway. The cleanup doesn't need to land before schedule() call, only sometime before this timer's own eventual fireTimerNotification() check (90+ seconds later) — so make it non-blocking (fire-and-forget, run in parallel) instead of awaited ahead of schedule(). Restores the original timing between schedule() and an immediately-following cancel() call. Verified via bun run typecheck, eslint, and the full Playwright suite on chromium (this environment has no WebKit available to reproduce the iphone-webkit-only failure directly, but the fix directly addresses the root cause CI's log identified: schedule() no longer waits on the cleanup call).
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Problem
On the native iOS app, completing a rest timer produced two separate alerts: the OS-scheduled local notification (its own sound + system alert vibration) and the app's own client-side haptic + 3-tone beep, essentially every time — not just as a rare race.
The scheduled native notification is driven by the OS's own clock and fires independent of whether the app's JS is even running.
notifyTimerExpired()only tried to cancel it after detecting expiry client-side (via a 250ms poll or a visibilitychange reconciler) — i.e. at or after the same instant the OS notification was due — so that cancel essentially never won the race.Fix
fireTimerNotification()now checks whether the OS will actually alert (queryingLocalNotificationsstate directly —getPending()/getDeliveredNotifications(), not an in-memory flag, since that survives reload but a flag wouldn't) before playing the client-side haptic/beep, skipping it when the OS already has it covered.scheduleBackgroundTimerNotification()clears stale delivered notifications before scheduling a new one, so a leftover delivered entry from a previous timer (the app reuses one fixed notification id) can't permanently mask a laterschedule()failure.fireTimerNotification()now reports whether cancellation is still warranted, and skips it when the OS is trusted to alert — unconditionally cancelling a still-pending notification we just decided to trust would silently prevent it from ever firing.cancelBackgroundTimerNotification()is properly awaitable now instead of fire-and-forget, closing the race where a fast next rest timer'sschedule()call (same reused id) could be wiped out by a late-arriving cancel meant for the previous timer.workout.tsthat can independently notice a timer expired (the poll, the visibilitychange reconciler, the resumed-timer recovery interval, and the already-expired-on-mount check) now share onehandleTimerExpiryhelper with a single guard, instead of a duplicated sequence that could drift out of sync.renderWorkout()call, not module-level — a module-level guard let a stale render's detector (left running if the workout screen is left via a raw route change instead of the in-app Back button) win the race and permanently block the live render's own detector from updating the visible page.timerIntervalbefore overwriting it, so a stale render's interval can't later clobber the live render's interval through the shared module variable.Process
This went through five rounds of adversarial code review (via the
code-reviewskill atmaxeffort), each round's confirmed findings fixed and regression-tested before the next pass — including a couple of bugs the review surfaced in my own earlier fixes for this same issue (an in-memory tracking flag that didn't survive reload, a stale-notification-masking bug, and the module-level-guard regression above).Testing
bun run typecheckandeslint— clean🤖 Generated with Claude Code
Generated by Claude Code