Add configurable timer ring duration and repeat reminders - #861
Marcus2626 wants to merge 5 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe built-in timer supports configurable alert ring durations and repeat intervals. Alerts alternate between ringing and silence until stopped. Timer settings, timer lifecycle handling, localized text, regression checks, and CI are updated. ChangesBuilt-in timer alerts
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant TimerManager
participant TimerAlertController
participant SoundPlayer
TimerManager->>TimerAlertController: Update alert with configured duration and interval
TimerAlertController-->>TimerManager: Return ringing state when a phase changes
TimerManager->>SoundPlayer: Play sound while ringing
TimerManager->>SoundPlayer: Stop sound while silent
TimerManager->>TimerAlertController: Stop alert on timer lifecycle changes
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Configurable repeating alerts have bounded settings and lifecycle handling for stopping, pausing, replacement, and sleep. No actionable merge-blocking risk is established; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is confined to local timer alerts, with bounded settings and safeguards against stale reminders. No introduced security issue was established. Remaining uncertainty concerns concurrent lifecycle callers and actual audio behavior during interruption and recovery. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 6 files. (3 skipped: 3 unsupported.)
Comment |
this pic was useless
|
Simplified the timer reminder implementation to fit the existing timer structure more closely.
The existing timer method signatures and external Clock integration remain unchanged. Configurable ring duration, repeat intervals, live setting changes, pause/resume behavior, and sleep/wake handling are preserved. Alert transitions are now checked once per second instead of every 0.25 seconds; delayed ticks advance only one phase and do not replay missed reminders. Against the same Validation:
|
|
@coderabbitai full review Please review the simplified timer reminder implementation, especially timer reuse, stale callback protection, pause/resume behavior, and sleep/wake handling. |
✅ Action performedFull review finished. |
Closes #859
Summary
Atoll's built-in timer currently keeps ringing after it expires until the timer is stopped. This change lets users set how long each ring lasts and how many minutes to wait before the next reminder. The timer stays in overtime between rings and continues reminding until it is stopped.
Root cause
On expiry,
TimerManagerstarted anAVAudioPlayerwith unlimited looping. It had no ringing/silent phases or saved settings for an automatic stop and later reminder.What changed
TimerAlertControllerto alternate ringing and silence. The silent interval begins when playback actually stops; changing settings also affects an active reminder.Code scope and function changes
The runtime changes are confined to
DynamicIsland/managers/TimerManager.swift(timer lifecycle), the newDynamicIsland/managers/TimerAlertController.swift(ring/silence scheduling), andDynamicIsland/components/Settings/SettingsView.swift(Timer settings).DynamicIsland/models/Constants.swiftadds the 30-second and 5-minute defaults;DynamicIsland/Localizable.xcstringsadds English and Simplified Chinese text. The remaining changes are the Swift regression test, its runner and CI entry, the.gitignoreexception for that runner, and a settings screenshot.Existing code changed
TimerManager.init()TimerManager.startTimer()TimerManager.stopTimer(),forceStopTimer(), andadoptExternalTimer()TimerManager.pauseTimer()andresumeTimer()TimerManager.beginTimerSession()andendTimerSession()TimerManager.playTimerSound()TimerSettingsinSettingsView.swiftNew code added
TimerManager.scheduleCountdown()TimerAlertController.init(...)anddeinitTimerAlertController.start()andstop()stop()also silences playback.TimerAlertController.update()TimerAlertController.prepareForSleep()TimerAlertController.duration(minutes:seconds:)andclamp(_:to:)TimerSettings.timerAlertSectionandTimerAlertConfiguration0:00and durations over one hour.TimerAlertRegression.main()and itsadvance(_:)helpertests/run_timer_alert_regression.shbuilds and runs this test in CI.At expiry,
scheduleCountdown()changes the timer to overtime and callsTimerAlertController.start(). The controller loops the selected sound, stops it when the ring duration elapses, and records that stop time as the start of the silent interval. When that interval elapses, it starts another ring. Stopping or replacing the timer callsstop(), which cancels the cycle. The existing countdown continues to display overtime throughout the silent intervals.Related short-timer UI check
In the earlier separate-window version, the overlap was reproducible with a 1-second timer: hover over the Dynamic Island, then move the pointer away just before expiry. At expiry, the old X cancel button and the square Stop button could appear together on the right while the timer title scrolled on the left; the controls obscured the overtime digits. Hovering over the island and moving away again refreshed the layout, leaving one square Stop button and readable overtime digits.
The same short-timer sequence did not reproduce the overlap on this
dev-based branch.devhad already replaced the separate control window with inline controls in56ff960, before this branch was created. This PR contains no Dynamic Island layout change; the result is recorded here as a related UI regression check, not as a fix made by this patch.Testing done
Tested on macOS 26.7 / Apple Silicon with Xcode 26.6:
codesign --verify --deep --strictpassed.bash tests/run_timer_alert_regression.shpassed. Checks minute/second conversion and bounds; exact ring/silence boundaries; cancellation; changes to an active alert; replacement; simulated sleep/wake; delayed updates; RunLoop scheduling; and cleanup. Audio callbacks are simulated in this test.python3 -m unittest discover -s tests -vpassed 7/7: six privacy-configuration checks and the existing timer lifecycle regression.git diff --checkpassed; the working tree is clean.Settings screenshot
Summary by CodeRabbit