feat(ios): offer to trash the original when removing an offline copy - #229
Merged
Conversation
Removing a download now confirms through a dialog whose default keeps the put.io original; a separately worded destructive choice asks put.io for it after the local copy is gone. Remote failures are reported per file with a retry and never resurrect the local copy. The dialog copy follows the mode captured with the targets, so a preference refresh cannot make the text disagree with the action. Refs #106
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c168fad12d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds an explicit “remove download vs remove download + delete/move original” flow for offline items, including retryable remote-delete outcomes and updated harness/journey artifacts.
Changes:
- Introduces offline-removal models/copy and queue support for “remove locally, then delete original” with durable failure reporting + retry.
- Updates the iOS downloads UI to present a confirmation dialog with distinct accessible actions and a follow-up failure alert.
- Extends harness/journey contracts + tests to cover the new dialog, screenshot, and seeded transient delete failure.
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 |
|---|---|
| docs/HARNESS.md | Updates journey description and required screenshots to include the new removal dialog capture. |
| Tools/PutioHarness/Tests/PutioHarnessKitTests/ModelsTests.swift | Updates expected downloads attachment list to include runtime-downloads-remove. |
| Tools/PutioHarness/Sources/PutioHarnessKit/Models.swift | Extends BrowserJourneyContract attachment names with the new screenshot. |
| Tests/iOSUITests/Sources/DownloadsJourneyTests.swift | Updates UI journey to validate new confirmation dialog, failure report, and retry behavior. |
| Tests/iOS/Sources/OfflineDownloadsTests.swift | Adds extensive unit coverage for local-before-remote deletion, failure merging, retry handling, and copy. |
| Apps/iOS/Sources/PutioApp.swift | Wires new downloads view inputs and queue deleteOriginal runtime hook. |
| Apps/iOS/Sources/OfflineRemoval.swift | Adds new removal target/outcome types and user-facing copy helpers. |
| Apps/iOS/Sources/OfflineDownloadsView.swift | Replaces alert with confirmation dialog and adds remote-failure retry alert + accessibility identifiers. |
| Apps/iOS/Sources/OfflineDownloads.swift | Adds queue APIs for deleting originals and tracking merged remote failures. |
| Apps/Shared/Sources/HarnessSeededAPI.swift | Seeds a transient first-delete failure for the root video to prove retry UX. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+612
to
+633
| func deleteOriginals(_ targets: [PutioOfflineRemovalTarget]) async | ||
| -> PutioOfflineOriginalOutcome | ||
| { | ||
| var outcome = PutioOfflineOriginalOutcome() | ||
| for target in targets { | ||
| do { | ||
| try await deleteOriginal(target.id) | ||
| outcome.deleted.append(target) | ||
| } catch PutioRuntimeError.notFound { | ||
| // Already gone, possibly from a delete whose response was lost. | ||
| outcome.deleted.append(target) | ||
| } catch { | ||
| outcome.failures.append(.init(target: target, reason: .init(error))) | ||
| } | ||
| } | ||
| var merged = originalFailure ?? PutioOfflineOriginalOutcome() | ||
| let touched = Set(targets.map(\.id)) | ||
| merged.failures.removeAll { touched.contains($0.target.id) } | ||
| merged.failures.append(contentsOf: outcome.failures) | ||
| originalFailure = merged.failures.isEmpty ? nil : merged | ||
| return outcome | ||
| } |
Comment on lines
+101
to
+109
| let removeLocal = app.buttons["downloads.remove-local"].firstMatch | ||
| XCTAssertTrue(removeLocal.waitForExistence(timeout: 5)) | ||
| XCTAssertTrue(app.staticTexts["Remove “Root Movie.mkv”?"].exists, "the title names the file") | ||
| XCTAssertTrue( | ||
| app.staticTexts.element( | ||
| matching: NSPredicate(format: "label CONTAINS %@", "every device signed in to your account") | ||
| ).exists, "the confirmation does not explain that other devices are affected") | ||
| let removeOriginal = app.buttons["downloads.remove-original"].firstMatch | ||
| XCTAssertTrue(removeOriginal.exists) |
Comment on lines
+118
to
+119
| let failure = app.alerts["Could not move original to Trash"] | ||
| XCTAssertTrue(failure.waitForExistence(timeout: 10)) |
| app.buttons["downloads.remove-original-retry"].firstMatch.tap() | ||
| XCTAssertTrue(failure.waitForNonExistence(timeout: 10)) | ||
| XCTAssertTrue(app.staticTexts["No downloads"].waitForExistence(timeout: 5)) | ||
| XCTAssertFalse(app.alerts.firstMatch.exists, "retry must not report a second failure") |
This was referenced Sep 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Removing an offline download only cleared the local copy; there was no way to also let go of the put.io original. Any such action must stay explicit, follow the account's Trash setting, and never report success the server did not confirm.
Refs #106. First of three stacked PRs; the next two add durable retries across relaunch and Trash-setting confirmation.
Solution
PutioRuntime.deleteFilerefuses those requests.PutioOfflineQueue.removeDeletingOriginalsremoves the local copies first, then asks put.io for each original. A remote failure never resurrects the local copy: the queue keeps a per-file failure report with Try again; anotFoundanswer counts as done. The delete request refuses to run under another account after a sign-out.runtime-downloads-remove.png.Proof
mise run verify,mise run harness -- test --platform ios, andmise run harness -- journey --platform ios --scenario files-browserpassed locally on this commit.OfflineDownloadsTestscover local-before-remote ordering, plain removal never touching the original, partial remote failure with retry, reports acting only on the failures they showed, merged failures across concurrent requests, error-reason mapping, and Trash-aware copy.