From 3fb2c5c2a88317de1c6821d966509fb980da5f8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BE=90=E7=BF=94=E5=AE=87?= Date: Tue, 19 May 2026 17:54:28 +0800 Subject: [PATCH] fix: pomodoro paused-in-break fraction + notif status wired to store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P0 issues from 2026-05-19 code review: 1. PomodoroView progress ring used pomodoroFocusMin*60 as denominator in paused state. If the user paused mid-break, ring rendered 80% done when the underlying break was actually 40% done. Now the store exposes pomodoroPhaseTotalSec which reads pausedPhase to pick the right denominator. 2. WorkerNotificationCenter set its own isAuthorized but never wrote WorkerStore.notificationsAuthorized — the top-bar notif status dot stayed dim even after the user authorized. Refactored into a single applyAuthorized helper that writes both. Co-Authored-By: Claude Opus 4.7 (1M context) --- Sources/engine/NotificationCenter.swift | 19 +++++++++++++---- Sources/engine/WorkerStore.swift | 28 +++++++++++++++++++++++++ Sources/ui/PomodoroView.swift | 10 ++++----- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/Sources/engine/NotificationCenter.swift b/Sources/engine/NotificationCenter.swift index e0d49b8..edaabba 100644 --- a/Sources/engine/NotificationCenter.swift +++ b/Sources/engine/NotificationCenter.swift @@ -31,24 +31,35 @@ final class WorkerNotificationCenter { let status = settings.authorizationStatus switch status { case .authorized, .provisional, .ephemeral: - Task { @MainActor in WorkerNotificationCenter.shared.isAuthorized = true } + Task { @MainActor in Self.applyAuthorized(true) } case .denied: - Task { @MainActor in WorkerNotificationCenter.shared.isAuthorized = false } + Task { @MainActor in Self.applyAuthorized(false) } WorkerDebugLog.write("notifications denied — falling back to in-panel dot") case .notDetermined: UNUserNotificationCenter.current().requestAuthorization(options: [.alert, .sound]) { granted, error in if let error = error { WorkerDebugLog.write("notif auth error: \(error)") } - Task { @MainActor in WorkerNotificationCenter.shared.isAuthorized = granted } + Task { @MainActor in Self.applyAuthorized(granted) } WorkerDebugLog.write("notif auth granted=\(granted)") } @unknown default: - Task { @MainActor in WorkerNotificationCenter.shared.isAuthorized = false } + Task { @MainActor in Self.applyAuthorized(false) } } } } + /// P0 fix (2026-05-19 review): prior code set + /// `WorkerNotificationCenter.shared.isAuthorized` but never + /// propagated to `WorkerStore.notificationsAuthorized`. Result: the + /// top-bar notif status dot stayed dim even after the user authorized. + /// Always write both so observers see consistent state. + @MainActor + private static func applyAuthorized(_ value: Bool) { + WorkerNotificationCenter.shared.isAuthorized = value + WorkerStore.shared.notificationsAuthorized = value + } + /// Fire-and-forget local notification. Returns true if scheduled /// (best effort — auth status may flip between scheduling and firing). @discardableResult diff --git a/Sources/engine/WorkerStore.swift b/Sources/engine/WorkerStore.swift index 5978f8c..c7da2ed 100644 --- a/Sources/engine/WorkerStore.swift +++ b/Sources/engine/WorkerStore.swift @@ -279,6 +279,34 @@ final class WorkerStore: ObservableObject { // MARK: - Pomodoro + /// Total duration (seconds) of the active phase. Used as denominator + /// for the progress ring fraction in PomodoroView. + /// + /// P0 fix (2026-05-19 review): the ring previously used + /// `pomodoroFocusMin * 60` as denominator for the paused state, + /// which is wrong when the user paused mid-break (denominator was + /// 25min while remaining was a break's 5min worth → ring read 80% + /// done when it was actually ~40%). Now `paused` looks at the + /// underlying `pausedPhase` so break/long-break paused renders right. + var pomodoroPhaseTotalSec: Int { + let isLongBreak = pomodoroCycleProgress >= 4 + switch pomodoroPhase { + case .focus: + return pomodoroFocusMin * 60 + case .rest: + return (isLongBreak ? pomodoroLongBreakMin : pomodoroBreakMin) * 60 + case .paused: + switch pausedPhase { + case .rest: + return (isLongBreak ? pomodoroLongBreakMin : pomodoroBreakMin) * 60 + default: + return pomodoroFocusMin * 60 + } + case .idle: + return pomodoroFocusMin * 60 + } + } + func pomodoroStart() { if pomodoroPhase == .paused { pomodoroPhase = pausedPhase diff --git a/Sources/ui/PomodoroView.swift b/Sources/ui/PomodoroView.swift index 1430db5..3c0cd8e 100644 --- a/Sources/ui/PomodoroView.swift +++ b/Sources/ui/PomodoroView.swift @@ -116,12 +116,10 @@ struct PomodoroView: View { } private var progressFraction: CGFloat { - let total: Int - switch store.pomodoroPhase { - case .focus: total = store.pomodoroFocusMin * 60 - case .rest: total = store.pomodoroBreakMin * 60 - case .paused, .idle: total = store.pomodoroFocusMin * 60 - } + // P0 fix (2026-05-19 review): delegate to store so paused-in-break + // gets the right denominator. Inline switch couldn't see the + // private pausedPhase. + let total = store.pomodoroPhaseTotalSec guard total > 0 else { return 0 } let remaining = max(0, store.pomodoroRemaining) return CGFloat(total - remaining) / CGFloat(total)