[CI] Fix set sleep timer via shortcut/siri when locked (#4812) - #4979
[CI] Fix set sleep timer via shortcut/siri when locked (#4812)#4979kean wants to merge 8 commits into
Conversation
|
Claude finished @kean's task in 5m 37s —— View job Claude Code Review
The approach is right: moving to Five things below, one of which I'd treat as blocking. 🔴 The legacy SiriKit handler is still registered
🟠 "Minutes" parameter actually holds seconds
🟠 No clamping to the app's own limits
🟡 Always reports success
🟡 Translations bypass GlotPress
Minor / nits
I couldn't build or run the tests here (no Xcode toolchain on this Linux runner), so the |
kean
added
the
[Type] Other
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @Parameter( | ||
| title: LocalizedStringResource( | ||
| "siri_shortcut_migrated_sleep_timer_minutes_title", | ||
| defaultValue: "Minutes", | ||
| table: "AppIntents" | ||
| ) | ||
| ) | ||
| var minutes: Int? | ||
|
|
||
| static var parameterSummary: some ParameterSummary { | ||
| Summary("Set sleep timer", table: "AppIntents") | ||
| } |
There was a problem hiding this comment.
The parameter is labeled "Minutes" but the value is treated as seconds.
Interpreting the legacy value as seconds is correct — SiriShortcutsManager.setSleepTimerIntent() populates it with Settings.customSleepTime(), which is a TimeInterval in seconds (podcasts/Settings.swift:397), and the old handler passed it straight into setSleepTimerInterval. So migratedValue(_:defaultDuration:) preserves behaviour.
The problem is the new user-visible title: existing shortcuts hold values like 1800, and a user who opens the action in Shortcuts sees a field labeled Minutes containing 1800. If they "correct" it to 30, they get a 30-second timer — well below the app's own 5-minute floor (Constants.Limits.minSleepTime).
Two related points:
- Retitle the parameter to reflect reality (the Swift property name must stay
minutesfor the migration to bind, but the display title doesn't have to). parameterSummarydoesn't reference\(\.$minutes), so the parameter isn't surfaced in the Shortcuts action at all — migrated shortcuts become uneditable. If that's deliberate (to avoid exposing the seconds field), a comment explaining why would help; otherwise include it in the summary.
| func setSleepTimer(duration: TimeInterval) -> Bool { | ||
| AnalyticsHelper.siriSleeptimer() | ||
| guard let timeInterval = TimeInterval(exactly: newTime) else { return false } | ||
| PlaybackManager.shared.setSleepTimerInterval(timeInterval) | ||
| guard duration > 0 else { return false } | ||
| PlaybackManager.shared.setSleepTimerInterval(duration) | ||
| return true | ||
| } |
There was a problem hiding this comment.
Consider clamping to the app's own sleep-timer limits.
Constants.Limits.minSleepTime (5 min) and maxSleepTime (5 h) bound every in-app path, and PlaybackManager.setSleepTimerInterval does no clamping of its own. With the new intent, "set a sleep timer for 3 seconds" or "for 40 hours" now goes straight through. > 0 is a much weaker invariant than the rest of the app assumes.
func setSleepTimer(duration: TimeInterval) -> Bool {
AnalyticsHelper.siriSleeptimer()
guard duration > 0 else { return false }
let clamped = min(max(duration, Constants.Limits.minSleepTime), Constants.Limits.maxSleepTime)
PlaybackManager.shared.setSleepTimerInterval(clamped)
return true
}Note this also changes the legacy path (sleepTimer(newTime:)), which is arguably a fix too — a migrated shortcut holding a small value currently produces a sub-minute timer.
| @MainActor | ||
| func perform() async throws -> some IntentResult { | ||
| let duration = SleepTimerIntentDuration.resolvedValue( | ||
| duration, | ||
| defaultDuration: Settings.customSleepTime() | ||
| ) | ||
| _ = SiriShortcutsManager.shared.setSleepTimer(duration: duration) | ||
|
|
||
| return .result() | ||
| } |
There was a problem hiding this comment.
The intent always reports success, even when nothing happens.
Two things get swallowed here:
_ =discards theBoolfromsetSleepTimer(duration:).- More importantly,
PlaybackManager.setSleepTimerIntervalonly assignssleepTimeRemainingand posts a notification — the countdown is driven by the playback update loop. If nothing is playing when Siri runs this from the lock screen, the timer is recorded (and.playerSleepTimerEnabledis tracked) but nothing is actually scheduled, while Siri reports success.
The old .continueInApp flow at least put the user in front of the player. Now it's silent. Worth returning a dialog and failing loudly when there's no active playback:
func perform() async throws -> some IntentResult & ProvidesDialog {
guard PlaybackManager.shared.currentEpisode() != nil else {
throw AppIntentError... // or .result(dialog: "Nothing is playing")
}
...
return .result(dialog: ...)
}At minimum, please confirm the no-playback case is acceptable — it's the most likely way a user hits this intent from a locked device.
| struct MigratedSleepTimerIntent: AudioPlaybackIntent, CustomIntentMigratedAppIntent { | ||
| static let intentClassName = "SJSleepTimerIntent" |
There was a problem hiding this comment.
Blocking question: the legacy SiriKit handler is still registered, which may mean the bug isn't actually fixed for existing shortcuts.
PodcastsIntents/Info.plist still lists SJSleepTimerIntent under IntentsSupported, IntentHandler.swift:9 still routes it to SleepTimerIntentHandler, and that handler still returns .continueInApp (PodcastsIntents/SleepTimerIntentHandler.swift:21) — which is exactly the code path that fails on a locked device.
Apple's migration guidance for CustomIntentMigratedAppIntent is to remove the migrated intent from the extension's IntentsSupported array and delete its handler, so the system routes to the App Intent instead. Leaving both in place is at best dead code, at worst the legacy extension keeps winning for shortcuts created before this build — i.e. the exact users this PR is meant to fix.
podcasts/AppDelegate+SiriShortcuts.swift:105-109 also still handles the intent in-app and would be dead too.
Please verify on-device with a shortcut created on the old build, run while locked, before merging. If the App Intent does take precedence, the leftover handler should still be removed in this PR (or a follow-up) to avoid two sources of truth.
| @@ -0,0 +1,132 @@ | |||
| { | |||
There was a problem hiding this comment.
These translations live outside the GlotPress pipeline.
fastlane/Fastfile (generate_strings_file_for_glotpress, download_localized_strings_from_glotpress) only handles *.lproj/Localizable.strings, and CLAUDE.md/AGENTS.md document that as the way to add strings. String catalogs are genuinely the right (and effectively only) mechanism for App Intents metadata, since the system extracts it at build time — but the consequence is that the 15 locales committed here will never be reviewed by translators or refreshed when the source strings change, and they'll silently drift.
Worth flagging to whoever owns i18n before merge, and ideally leaving a note in the file (or a follow-up issue) so the next person adding an App Intent string knows what the process is. Also: were these translations human-reviewed? A few read a bit machine-generated (e.g. the fr phrase mixes ${applicationName}: with the ${applicationName} : spacing convention used in the sibling string).
|
Claude finished @kean's task in 2m 29s —— View job Claude Code Review (re-run after
|
| Finding | Status | |
|---|---|---|
| 🔴 | SJSleepTimerIntent still in PodcastsIntents/Info.plist IntentsSupported + IntentHandler.swift:9 + SleepTimerIntentHandler returning .continueInApp |
open — verified still present |
| 🟠 | minutes parameter titled "Minutes" but holds seconds; parameterSummary omits \(\.$minutes) |
open |
| 🟠 | No clamping to Constants.Limits.minSleepTime / maxSleepTime |
open |
| 🟡 | Intent always returns .result(); Bool discarded |
open |
| 🟡 | AppIntents.xcstrings / AppShortcuts.xcstrings bypass GlotPress |
open |
New this pass
The trunk merge added a Live Activity to the code path this PR now runs in the background.
PlaybackManager.setSleepTimerInterval (PlaybackManager.swift:1964) now calls startSleepTimerLiveActivity, which reaches Activity.request(...) (SleepTimerLiveActivityController.swift:31). So this intent will attempt to start an ActivityKit activity from a background App Intent on a locked device — an interaction neither PR was tested against. It degrades safely (the catch at line 32 logs and moves on), so it isn't a crash risk, but it is worth an on-device check: if the request fails while locked, the user gets a sleep timer with no Lock Screen countdown, which is precisely where they'd look for it. Gated behind FeatureFlag.sleepTimerLiveActivity, so this may be moot depending on rollout state.
This also sharpens the clamping finding. Settings.customSleepTime() clamps to minSleepTime itself (Settings.swift:399), so the no-argument path is safe — the invariant is already established in the codebase, and only the explicit-duration path through setSleepTimer(duration:) breaks it.
And it corroborates the "always reports success" finding. The comment at PlaybackManager.swift:2001-2003 states it outright: "The timer only counts down while playback is running." With nothing playing, setSleepTimerInterval records sleepTimeRemaining, fires .playerSleepTimerEnabled, and Siri says it worked — but nothing counts down.
Two new inline comments:
- No CHANGELOG entry for this PR's own fix — the file change only reorders the Live Activity note. (details)
- Duplicate string in
AppIntents.xcstrings—Summary("Set sleep timer", table: "AppIntents")keys an entry by its own English text, duplicatingsiri_shortcut_set_sleep_timer_title. Editing the copy later drops all 15 translations. (details)
Verified as fine
IPHONEOS_DEPLOYMENT_TARGET = 17.0, soAudioPlaybackIntentandAppShortcutsProviderneed no@availableguard; the@available(iOS 26.0, *) supportedModesmatchesPlayEpisodeIntent.swift:22exactly.PocketCastsAppShortcutsProvideris the onlyAppShortcutsProviderin the project — no conflict with an existing one.- Both App Shortcut phrases contain
${applicationName}in every one of the 15 locales, which the system requires. sleepTimer(newTime:)still has a live caller (AppDelegate+SiriShortcuts.swift:108), so the refactor isn't leaving a dangling API. It also now rejects0and negatives, which the oldTimeInterval(exactly:)guard let through.- The
nancase inresolvedValueis covered byisFinite;-.infinityis tested explicitly.
I still can't build or run tests here (no Xcode toolchain on this Linux runner), so the @Parameter(title:description:defaultUnit:supportsNegativeNumbers:) overload on Measurement<UnitDuration>? and the Summary(_:table:) overload remain unverified by me — CI covers both.
• branch lparry/fix-siri-sleep-timer-lock
| @@ -1,12 +1,12 @@ | |||
| 8.20 | |||
| ----- | |||
| - Add a Sleep Timer Live Activity so users can see the remaining sleep timer countdown from the Lock Screen and Dynamic Island. [#4949](https://github.com/Automattic/pocket-casts-ios/pull/4949) | |||
There was a problem hiding this comment.
This PR doesn't add a CHANGELOG entry for its own fix.
The only change here moves the pre-existing Live Activity note from 8.19 to 8.20 (fallout from the trunk merge, and correct). But "sleep timer shortcuts now work while the device is locked" is a user-facing bug fix, and every other entry in this file carries one with a PR link.
| - Add a Sleep Timer Live Activity so users can see the remaining sleep timer countdown from the Lock Screen and Dynamic Island. [#4949](https://github.com/Automattic/pocket-casts-ios/pull/4949) | |
| - Fix setting the sleep timer via Siri or a Shortcut failing while the device is locked [#4812](https://github.com/Automattic/pocket-casts-ios/pull/4812) |
| { | ||
| "sourceLanguage" : "en", | ||
| "strings" : { | ||
| "Set sleep timer" : { |
There was a problem hiding this comment.
Two keys hold the same English string, and one of them is the string itself.
"Set sleep timer" (this entry) exists only because MigratedSleepTimerIntent.parameterSummary passes a bare literal:
Summary("Set sleep timer", table: "AppIntents")which makes the source text the key. siri_shortcut_set_sleep_timer_title (line 109) is the same English string under a proper key, and it's already the title of both intents plus the App Shortcut's shortTitle.
The practical cost: if the copy ever changes, whoever edits it has to know to update both entries and that one of them is keyed by its own value — otherwise the summary and the title drift apart, and the literal-keyed entry silently loses all 15 translations because the key no longer exists. Worth pointing the Summary at siri_shortcut_set_sleep_timer_title and deleting this entry.
2 participants
CI mirror of #4812 by @lparry (
lparry:codex/fix-siri-sleep-timer-lock), opened from this repo so the CI workflows run.Please review and discuss on the original PR: #4812
The six commits are copied verbatim from that branch. On top of them this PR also has:
trunk, to bring the branch up to date