Cellular Data Usage: Download status fixes#4071
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a cellular-download TOCTOU issue by explicitly tracking whether the user approved a cellular download at queue time, and then using that persisted status (instead of runtime network checks) to choose the correct URLSession for the download.
Changes:
- Extend
NetworkUtils.downloadEpisodeRequestedto return both “queue for later” and “explicit cellular approval” signals, and propagate that intoDownloadManager.addToQueue. - Add
AutoDownloadStatus.userApprovedCellularand updateDownloadManager.performDownloadsession selection to rely on this status when the feature flag is enabled. - Add unit tests validating session selection behavior for the new status and regression coverage for the TOCTOU scenario.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| podcasts/PlaybackActionHelper.swift | Records explicit cellular approval from the prompt and passes an appropriate autoDownloadStatus into the download queue. |
| podcasts/ListeningHistoryViewController+Table.swift | Updates “Retry” flow to propagate explicit cellular approval into queued downloads. |
| podcasts/Extensions/NetworkUtils+Helpers.swift | Updates download permission prompt helper to return (later, approvedCellular) and documents the behavior. |
| podcasts/DownloadsViewController+Table.swift | Updates per-episode retry flow to pass .userApprovedCellular when the user confirms cellular. |
| podcasts/DownloadsViewController.swift | Updates “Retry All Failed” flow to use the new prompt callback shape and propagate approval status. |
| podcasts/DownloadManager.swift | Changes session selection logic to use .userApprovedCellular (feature-flagged) instead of inferring approval from current network state. |
| PocketCastsTests/Tests/Utilities/DownloadManagerCellularSessionTests.swift | Adds tests validating session selection for .userApprovedCellular, .notSpecified, and .autoDownloaded. |
| Modules/DataModel/Sources/PocketCastsDataModel/Public/Enums.swift | Adds the new AutoDownloadStatus.userApprovedCellular = 5 enum case. |
You can also share your feedback on Copilot code review. Take the survey.
# Conflicts: # Modules/Utils/Sources/PocketCastsUtils/Feature Flags/FeatureFlag.swift
62e0036 to
dce652f
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a cellular-download TOCTOU issue by explicitly tracking whether the user approved downloading over cellular at the time downloads are queued, rather than inferring approval from the network state at download start time.
Changes:
- Adds a new
AutoDownloadStatus.userApprovedCellularvalue and propagates it through multiple “download” entry points (single-episode actions and bulk downloads). - Updates
DownloadManagersession selection to use explicit approval (userApprovedCellular) when thecellularDownloadStatusFixfeature flag is enabled. - Adds a new unit test suite validating session selection behavior and introduces a new feature flag to gate the change.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| podcasts/Podcasts/Podcast Page/PodcastViewController.swift | Bulk “Download All” now forwards a userApprovedCellular signal into queueing. |
| podcasts/Podcasts/Podcast Page/EpisodeListSearchController.swift | Bulk download confirmation now passes a cellular-approval boolean into the delegate call. |
| podcasts/PlaylistViewController.swift | Bulk playlist download confirmation/queueing now passes a cellular-approval boolean and uses it to set autoDownloadStatus. |
| podcasts/PlaybackActionHelper.swift | Single-episode download flows now receive explicit cellular approval from NetworkUtils and queue with the correct status. |
| podcasts/ListeningHistoryViewController+Table.swift | Retry-download flow now records explicit cellular approval when queueing. |
| podcasts/Extensions/NetworkUtils+Helpers.swift | Extends downloadEpisodeRequested callback to include an approvedCellular flag when the prompt is shown/accepted. |
| podcasts/DownloadsViewController+Table.swift | Retry-download flow now records explicit cellular approval when queueing. |
| podcasts/DownloadsViewController.swift | “Retry all failed” now queues with explicit cellular approval status when applicable. |
| podcasts/DownloadManager.swift | Session selection logic updated to rely on explicit approval status under the feature flag; queued-startup now preserves stored status. |
| podcasts/Common Components/MultiSelect/MultiSelectHelper.swift | Multi-select download flow now records whether the user approved cellular for the bulk action. |
| PocketCastsTests/Tests/Utilities/DownloadManagerCellularSessionTests.swift | New tests validating session selection for various AutoDownloadStatus/settings combinations. |
| Modules/Utils/Sources/PocketCastsUtils/Feature Flags/FeatureFlag.swift | Adds cellularDownloadStatusFix feature flag. |
| Modules/DataModel/Sources/PocketCastsDataModel/Public/Enums.swift | Adds AutoDownloadStatus.userApprovedCellular = 5. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
SergioEstevao
left a comment
There was a problem hiding this comment.
Working correctly.
![]()
Generated by 🚫 Danger |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| let tempFilePath = tempPathForEpisode(episode) | ||
| let mobileDataAllowed = autoDownloadStatus == .autoDownloaded ? Settings.autoDownloadMobileDataAllowed() : Settings.mobileDataAllowed() | ||
| let useCellularSession = (mobileDataAllowed || (!NetworkUtils.shared.isConnectedToUnexpensiveConnection() && autoDownloadStatus != .autoDownloaded)) // allow cellular downloads if not on WiFi and not auto downloaded, because it means the user said yes to a confirmation prompt | ||
| let useCellularSession: Bool | ||
| if FeatureFlag.cellularDownloadStatusFix.enabled { | ||
| // Allow cellular downloads if mobile data is allowed by settings, if the user explicitly approved cellular download at queue time, or for episodes downloaded by the player for streaming | ||
| useCellularSession = mobileDataAllowed || autoDownloadStatus == .userApprovedCellular || autoDownloadStatus == .playerDownloadedForStreaming | ||
| } else { | ||
| // allow cellular downloads if not on WiFi and not auto downloaded, because it means the user said yes to a confirmation prompt | ||
| useCellularSession = (mobileDataAllowed || (!NetworkUtils.shared.isConnectedToUnexpensiveConnection() && autoDownloadStatus != .autoDownloaded)) | ||
| } |
There was a problem hiding this comment.
With cellularDownloadStatusFix enabled, useCellularSession is now driven purely by mobileDataAllowed or autoDownloadStatus == .userApprovedCellular (plus streaming). Any UI flow that shows a “Not on Wi‑Fi” confirmation but still queues episodes with .notSpecified will now create a Wi‑Fi-only task that won’t run on cellular. For example, podcasts/New Detail/PlaylistDetailViewController+EditActions.swift still enqueues bulk downloads with .notSpecified even when the user explicitly confirms on cellular. Please update those call sites to pass .userApprovedCellular when the user confirms cellular download (and keep .notSpecified for Wi‑Fi / queue-for-later).
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
Fixes cellular downloads by explicitly tracking when a user approves cellular download via the prompt, rather than inferring it from network state at download time.
Previously, the app inferred cellular approval by checking
!isConnectedToUnexpensiveConnection()at download time. This was unreliable because:The theoretical pattern goes like this:
autoDownloadStatus = .notSpecifiedperformDownloadruns!onWifiis now true, and status is.notSpecified(not.autoDownloaded)useCellularSession = truedownload uses cellular even though the user never approved itNOTE This is a change in behavior and it may make sense for queued episodes to continue to download over cellular once the user has initiated the download.
To test
Checklist
CHANGELOG.mdif necessary.