Fix stale timer cleanup hiding restarted timers - #644
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughTimer sessions now scope and cancel delayed smooth-close work. External cleanup validates session identity before mutating timer state. New Swift and Python regression tests compile and execute the lifecycle check in macOS CI. ChangesTimer session safety
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DynamicIsland/managers/SystemTimerBridge.swift`:
- Around line 822-835: Update completeExternalTimer() to persist the completed
external timer session ID before scheduling the main-queue block, rather than
reading TimerManager.shared.activeSessionID inside that block. Use the stored
completion token for delayed cleanup, and clear or replace it when a new timer
run is observed so a replacement Clock session cannot be ended by stale cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb3203b9-3fde-4112-8150-3a532089db60
📒 Files selected for processing (7)
.github/workflows/ci.yml.gitignoreDynamicIsland/managers/SystemTimerBridge.swiftDynamicIsland/managers/TimerLifecycle.swiftDynamicIsland/managers/TimerManager.swifttests/TimerLifecycleRegression.swifttests/test_timer_lifecycle.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Summary
Root cause
TimerManager.scheduleSmoothClose()scheduled an unconditional presentation update three seconds after a timer completed. If the user stopped that timer and immediately started another timer with the same duration, the old closure setisTimerActivetofalsefor the replacement run. The countdown itself continued, so its alarm still fired without an Island stop control.SystemTimerBridgealso scheduled a 3.2-second external-timer cleanup that checked only whether some external timer was active. It could therefore clear a replacement Clock timer.Fix
Each manual or external timer run now receives a unique session token. Starting or resetting a timer cancels pending smooth-close work, and delayed cleanup verifies that its captured token still belongs to the current session. A positive external update after completion starts a new session even if Clock reused the previous timer ID and duration.
Validation
python3 -m unittest tests.test_privacy_configuration tests.test_timer_lifecycleSummary by CodeRabbit
Bug Fixes
Tests