Repository navigation
Control remote content and attachment downloads - #259
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary
WalkthroughThe change adds device-local remote-content and attachment-download policies, mailbox overrides, bounded Gmail attachment retrieval, Privacy & Data settings, guarded attachment UI, protected local storage, and policy-driven HTML message rendering. ChangesRemote content and attachment controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MessageHTMLView
participant MessageContentPreferences
participant RemoteMessageContentPresentation
participant RemoteMessageContentNotice
MessageHTMLView->>MessageContentPreferences: resolve effective policy
MessageHTMLView->>RemoteMessageContentPresentation: apply policy and remote-image state
RemoteMessageContentPresentation-->>MessageHTMLView: return presentation state
MessageHTMLView->>RemoteMessageContentNotice: render blocked, consent, loading, or retry state
sequenceDiagram
participant AccountView
participant MailboxConnectionRouter
participant GmailMessageBodyService
participant DownloadedAttachmentStore
AccountView->>MailboxConnectionRouter: request attachment download
MailboxConnectionRouter->>GmailMessageBodyService: load attachment
GmailMessageBodyService-->>AccountView: return validated bytes
AccountView->>DownloadedAttachmentStore: save or reuse downloaded file
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: 1ecea7811e
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swift (1)
133-147: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake
connectionIdan explicit argument.
remoteContentPolicy(for:)uses the global policy whenconnectionIdisnil. A future mailbox call site could therefore bypass a.neveroverride when the global policy is.alwaysLoad. Remove= nil, and passnilexplicitly only when no mailbox connection exists.🤖 Prompt for 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. In `@apps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swift` around lines 133 - 147, Make connectionId a required argument in the MessageHTMLView initializer by removing its default nil value. Update every initializer call site to pass the relevant mailbox connection ID, or pass nil explicitly only when no mailbox connection exists, preserving remoteContentPolicy(for:) behavior.Source: Path instructions
apps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swift (1)
303-315: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClassify
URLError(.cancelled)as cancellation.URLSessionpasses this error tourlSession(_:task:didCompleteWithError:)when it cancels a task. The current checks classify it asRemoteMessageContentError.transferFailed, soloadsets.failed(...)after navigation. MatchURLErrorwithcode == .cancelledand add a regression test.🤖 Prompt for 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. In `@apps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swift` around lines 303 - 315, Update the cancellation classification in the completion handling around state.isCancelled and errorIsCancellation to also recognize URLError values whose code is .cancelled, ensuring they produce CancellationError instead of RemoteMessageContentError.transferFailed. Add a regression test covering a URLSession cancellation error and verify load preserves the cancelled state rather than setting .failed.
🤖 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 @.changeset/control-remote-content-downloads.md:
- Around line 1-4: Update the changeset frontmatter to name the Apple app
package declared in the changeset configuration and specify an appropriate
patch, minor, or major bump, replacing the empty frontmatter while preserving
the existing summary.
In
`@apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift`:
- Line 29: Define the 25 MB attachment limit once as
MailboxMessageAttachment.maximumDownloadByteCount near the model in
MailboxConnection.swift, then update AttachmentDownloadGate.maximumByteCount and
the GmailMessageBodyService initializer default for maximumAttachmentByteCount
to reference that shared constant instead of maintaining independent literals.
- Around line 158-166: Inject a DownloadedAttachmentStore into the view instead
of constructing it inside the .task closure, using the existing
init(fileManager:rootDirectory:) support. Thread the injected store through
MessageAttachmentsView and preserve the existing store behavior for production
callers via an appropriate default. Add focused tests covering policy gating,
existing-download reuse, and retry/failure paths with a temporary store.
- Around line 205-250: Add a removal API to DownloadedAttachmentStore, alongside
save and existingURL, that deletes the stored attachment file and removes empty
parent directories without affecting provider data. Invoke this cleanup from the
existing device-authorization removal, mailbox-connection removal, sign-out, and
Product Account teardown paths; structure the storage key or maintain sufficient
metadata to support per-connection cleanup without relying on an unbounded
full-directory scan.
In
`@apps/unwired-mail/unwired-mail/Features/Settings/PrivacyDataSettingsView.swift`:
- Around line 6-9: Update the AccountView preview setup to inject a
MessageContentPreferences instance alongside SettingsRouter(), matching the
production environment provided by UnwiredMailApp. Ensure the preview’s
PrivacyDataSettingsView path can resolve its non-optional preferences
environment value.
In `@apps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swift`:
- Around line 1668-1669: The privacy and data settings currently receive only
Gmail connections, so overrides for other supported providers cannot be
displayed or cleared. Update the .privacyAndData case to pass the complete
connection list produced by EmailAccountsSettingsView.makeSummaryConnections,
reusing the host’s generic definitions, authorized IDs, and session inputs;
otherwise explicitly document the Gmail-only scope in the section footer.
In `@apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift`:
- Around line 1422-1430: Add loadMessageAttachment to MailboxConnectionRouter
and route it through adapter(for: message.connectionId), forwarding the
attachment, message metadata, and session arguments so provider-specific
implementations handle downloads instead of the default unsupportedProvider
error.
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swift`:
- Around line 94-104: Replace the read-path Dictionary(uniqueKeysWithValues:)
call in MessageContentPreferences.init with the merging initializer
Dictionary(_:uniquingKeysWith:), choosing a deterministic value for duplicate
parsed MailboxConnectionId keys so malformed or legacy UserDefaults data cannot
trap during initialization. Leave the write path unchanged because
MailboxConnectionId.rawValue keys are already unique.
In `@apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift`:
- Around line 248-281: Extend
testReadExposesPresentationScopedDataBackedAttachment to inspect
fixture.cache.serializedPayload after loading the attachment and assert the
serialized body cache contains only the attachment descriptor: presentationData
must be absent and hasPresentationScopedAttachmentData must indicate no cached
presentation bytes, while retaining the existing no-second-request and
returned-data assertions.
In `@apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swift`:
- Around line 1869-1882: Extend the test around apply(policy:hasRemoteImages:)
to cover .alwaysLoad with hasRemoteImages: false, asserting no load starts and
the blocked/display behavior remains correct. After the existing successful
.alwaysLoad load, also assert displayedHTML(originalHTML:) returns the loaded
HTML, while preserving the current assertions for loader input and subsequent
policies.
In `@apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift`:
- Around line 383-412: Extend
testDownloadedAttachmentStoreReusesBoundedLocalFile to assert the saved
attachment has the required file protection level, is excluded from backup, and
resides beneath the store’s dedicated rootDirectory. Use the savedURL and
FileManager resource attributes, preserving the existing path, reuse, and
contents assertions.
- Around line 328-345: Update
testAttachmentDownloadGateEnforcesAutomaticNetworkPolicyAndFailureRetry to
assert the specific error in each loop iteration: for .cellular, require
AttachmentDownloadError.blockedByPolicy and confirm the operation is not
invoked; for .wifi, require propagation of URLError(.cannotConnectToHost).
Preserve the existing requestCount assertion while replacing the empty catch
with network-specific error validation.
---
Outside diff comments:
In `@apps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swift`:
- Around line 133-147: Make connectionId a required argument in the
MessageHTMLView initializer by removing its default nil value. Update every
initializer call site to pass the relevant mailbox connection ID, or pass nil
explicitly only when no mailbox connection exists, preserving
remoteContentPolicy(for:) behavior.
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swift`:
- Around line 303-315: Update the cancellation classification in the completion
handling around state.isCancelled and errorIsCancellation to also recognize
URLError values whose code is .cancelled, ensuring they produce
CancellationError instead of RemoteMessageContentError.transferFailed. Add a
regression test covering a URLSession cancellation error and verify load
preserves the cancelled state rather than setting .failed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68ad1bdb-0ba6-42ca-88a0-f4f31659e429
📒 Files selected for processing (19)
.changeset/control-remote-content-downloads.mdREADME.mdapps/unwired-mail/unwired-mail.xcodeproj/project.pbxprojapps/unwired-mail/unwired-mail/App/UnwiredMailApp.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mail/Features/Account/RemoteMessageContentNotice.swiftapps/unwired-mail/unwired-mail/Features/Settings/PrivacyDataSettingsView.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftdocs/adr/0029-sanitize-html-before-webkit-rendering.mddocs/adr/0030-gate-incoming-attachment-downloads.md
📜 Review details
⚠️ CI failures not shown inline (4)
GitHub Actions: CI / Apple · Unwired Mail: Control remote content and attachment downloads
Conclusion: failure
##[group]Run mise exec -- zsh scripts/check-apple-lint.zsh
�[36;1mmise exec -- zsh scripts/check-apple-lint.zsh�[0m
shell: /bin/bash -e {0}
##[endgroup]
�[2mmise�[0m �[34mnode�[[email protected] [1/3] install
�[2mmise�[0m �[34mpnpm�[[email protected] [1/3] install
�[2mmise�[0m �[34mnode�[[email protected] [1/3] download node-v24.18.1-darwin-arm64.tar.gz
�[2mmise�[0m �[34mnode�[[email protected] [2/3] checksum node-v24.18.1-darwin-arm64.tar.gz
�[2mmise�[0m �[34mnode�[[email protected] [3/3] extract node-v24.18.1-darwin-arm64.tar.gz
�[33mmise�[0m �[33mWARN�[0m GitHub rate limit exceeded. Resets at +00:00
�[2mmise�[0m �[34mnode�[[email protected] [3/3] node -v
�[2mmise�[0m �[34mnode�[[email protected] [3/3] v24.18.1
�[2mmise�[0m �[34mnode�[[email protected] [3/3] npm -v
�[2mmise�[0m �[34mnode�[[email protected] [3/3] 11.16.0
�[2mmise�[0m �[34mnode�[[email protected] �[38;5;10m✓�[0m installed
�[33mmise�[0m �[33mWARN�[0m GitHub API returned a 403 Forbidden error. This is most commonly caused by exceeding the rate limit, though other causes (e.g. insufficient token permissions) are possible.
�[33mmise�[0m �[33mWARN�[0m No GitHub token was found, so mise is making unauthenticated requests to GitHub which have a much lower rate limit.
Create a token at https://github.com/settings/tokens (no scopes required) and set it as GITHUB_TOKEN in your environment.
See https://mise.en.dev/dev-tools/github-tokens.html for all supported token sources (env vars, gh CLI, credential_command, etc.).
�[31mmise�[0m �[31mERROR�[0m Failed to install aqua:pnpm/[email protected]: HTTP status client error (403 Forbidden) for url (https://api.github.com/repos/pnpm/pnpm/releases/tags/v11.5.2)
github ***REDACTED_SECRET_ASSIGNMENT***
github rate limit: 0/60 (core), resets at 1785672558
github response: {"message":"API rate limit exceeded for 13.105.117.142. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.c...
GitHub Actions: CI / Fallow: Control remote content and attachment downloads
Conclusion: failure
##[group]Run if [ "$INPUT_FAIL_ON_ISSUES" != "true" ]; then exit 0; fi
�[36;1mif [ "$INPUT_FAIL_ON_ISSUES" != "true" ]; then exit 0; fi�[0m
�[36;1mif [ "$INPUT_COMMAND" = "audit" ]; then�[0m
�[36;1m # Audit gates on rule severity. Verdict already encodes the gate�[0m
�[36;1m # decision: pass -> no issues, warn -> warn-tier only (do not fail),�[0m
�[36;1m # fail -> error-tier (fail). Counting introduced findings instead�[0m
�[36;1m # would re-introduce the bug issue `#302` was filed to fix.�[0m
�[36;1m if [ "$VERDICT" = "fail" ]; then�[0m
�[36;1m echo "::error::Fallow audit failed (gate: ${INPUT_GATE:-new-only}, ${ISSUES} finding(s) at error severity in changed files)"�[0m
GitHub Actions: CI / 0_Apple · Unwired Mail.txt: Control remote content and attachment downloads
Conclusion: failure
##[group]Run mise exec -- zsh scripts/check-apple-lint.zsh
�[36;1mmise exec -- zsh scripts/check-apple-lint.zsh�[0m
shell: /bin/bash -e {0}
##[endgroup]
�[2mmise�[0m �[34mnode�[[email protected] [1/3] install
�[2mmise�[0m �[34mpnpm�[[email protected] [1/3] install
�[2mmise�[0m �[34mnode�[[email protected] [1/3] download node-v24.18.1-darwin-arm64.tar.gz
�[2mmise�[0m �[34mnode�[[email protected] [2/3] checksum node-v24.18.1-darwin-arm64.tar.gz
�[2mmise�[0m �[34mnode�[[email protected] [3/3] extract node-v24.18.1-darwin-arm64.tar.gz
�[33mmise�[0m �[33mWARN�[0m GitHub rate limit exceeded. Resets at +00:00
�[2mmise�[0m �[34mnode�[[email protected] [3/3] node -v
�[2mmise�[0m �[34mnode�[[email protected] [3/3] v24.18.1
�[2mmise�[0m �[34mnode�[[email protected] [3/3] npm -v
�[2mmise�[0m �[34mnode�[[email protected] [3/3] 11.16.0
�[2mmise�[0m �[34mnode�[[email protected] �[38;5;10m✓�[0m installed
�[33mmise�[0m �[33mWARN�[0m GitHub API returned a 403 Forbidden error. This is most commonly caused by exceeding the rate limit, though other causes (e.g. insufficient token permissions) are possible.
�[33mmise�[0m �[33mWARN�[0m No GitHub token was found, so mise is making unauthenticated requests to GitHub which have a much lower rate limit.
Create a token at https://github.com/settings/tokens (no scopes required) and set it as GITHUB_TOKEN in your environment.
See https://mise.en.dev/dev-tools/github-tokens.html for all supported token sources (env vars, gh CLI, credential_command, etc.).
�[31mmise�[0m �[31mERROR�[0m Failed to install aqua:pnpm/[email protected]: HTTP status client error (403 Forbidden) for url (https://api.github.com/repos/pnpm/pnpm/releases/tags/v11.5.2)
github ***REDACTED_SECRET_ASSIGNMENT***
github rate limit: 0/60 (core), resets at 1785672558
github response: {"message":"API rate limit exceeded for 13.105.117.142. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.c...
GitHub Actions: CI / 2_Fallow.txt: Control remote content and attachment downloads
Conclusion: failure
##[group]Run if [ "$INPUT_FAIL_ON_ISSUES" != "true" ]; then exit 0; fi
�[36;1mif [ "$INPUT_FAIL_ON_ISSUES" != "true" ]; then exit 0; fi�[0m
�[36;1mif [ "$INPUT_COMMAND" = "audit" ]; then�[0m
�[36;1m # Audit gates on rule severity. Verdict already encodes the gate�[0m
�[36;1m # decision: pass -> no issues, warn -> warn-tier only (do not fail),�[0m
�[36;1m # fail -> error-tier (fail). Counting introduced findings instead�[0m
�[36;1m # would re-introduce the bug issue `#302` was filed to fix.�[0m
�[36;1m if [ "$VERDICT" = "fail" ]; then�[0m
�[36;1m echo "::error::Fallow audit failed (gate: ${INPUT_GATE:-new-only}, ${ISSUES} finding(s) at error severity in changed files)"�[0m
🧰 Additional context used
📓 Path-based instructions (5)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Treat docs as product truth, not marketing. Prefer precise domain language
from CONTEXT.md; flag undefined terms, synonym drift, and conflicts with
existing ADRs under docs/adr/. For ADR changes, require a clear decision,
consequences, and privacy or security boundary impact; flag silent
reversals of prior ADRs without an explicit supersession note. For
docs/agents/ and bootstrap docs, verify agent workflow, validation, and
triage guidance stay aligned with AGENTS.md. Flag broken cross-links,
speculative future claims presented as shipped behavior, and docs that
omit the Apple vs TypeScript ownership of a boundary they describe.
Files:
README.mddocs/adr/0029-sanitize-html-before-webkit-rendering.mddocs/adr/0030-gate-incoming-attachment-downloads.md
apps/unwired-mail/**/*.{swift,mm,m,h}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands; use
zsh scripts/check-apple-lint.zshfor formatting and linting and the specifiedxcodebuild testcommand for tests.
Files:
apps/unwired-mail/unwired-mail/Features/Settings/PrivacyDataSettingsView.swiftapps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Features/Account/RemoteMessageContentNotice.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/App/UnwiredMailApp.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode so warnings fail validation; use mise-managed SwiftLint and Xcode's
xcrun swift-formatwhere applicable.
Files:
apps/unwired-mail/unwired-mail/Features/Settings/PrivacyDataSettingsView.swiftapps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Features/Account/RemoteMessageContentNotice.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/App/UnwiredMailApp.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mail/Features/Settings/PrivacyDataSettingsView.swiftapps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Features/Account/RemoteMessageContentNotice.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/App/UnwiredMailApp.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Create a Changeset for runtime behavior changes or exported type/API changes using the package name and an appropriate patch, minor, or major bump; tests-only, internal refactors, docs-only changes, and JSDoc-only maintenance may omit one by maintainer decision.
Files:
.changeset/control-remote-content-downloads.md
🪛 markdownlint-cli2 (0.23.1)
.changeset/control-remote-content-downloads.md
[warning] 4-4: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (27)
README.md (1)
125-125: LGTM!apps/unwired-mail/unwired-mail.xcodeproj/project.pbxproj (1)
73-76: LGTM!Also applies to: 181-184, 245-251, 273-273, 460-466, 488-488
docs/adr/0029-sanitize-html-before-webkit-rendering.md (1)
20-21: LGTM!docs/adr/0030-gate-incoming-attachment-downloads.md (1)
1-11: LGTM!apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (2)
764-808: LGTM!
2792-2838: LGTM!apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift (6)
1-29: LGTM!Also applies to: 928-957, 977-977
1030-1073: LGTM!
1425-1487: LGTM!
1883-1970: LGTM!
2029-2097: LGTM!
1398-1402: 🚀 Performance & ScalabilityNo session leak exists in these call sites.
RemoteMessageContentDataDelegate.finish(_:)callsfinishTasksAndInvalidate()for successful, failed, and cancelled requests.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (1)
190-246: LGTM!Also applies to: 283-315, 317-361, 363-401, 3037-3169
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (3)
1436-1437: LGTM!Also applies to: 3693-3699, 4212-4212, 4257-4262
4312-4350: LGTM!Also applies to: 4362-4412, 6247-6248
4493-4499: 🩺 Stability & AvailabilityNo change needed.
UnwiredMailAppinjects both environment objects into the primary window and DEBUG settings window. The DEBUG settings sheet inherits them fromAccountView.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift (1)
6-26: LGTM!Also applies to: 31-67, 69-95, 97-116
apps/unwired-mail/unwired-mail/App/UnwiredMailApp.swift (1)
6-7: LGTM!Also applies to: 20-21, 37-38, 49-50, 59-60
apps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swift (1)
47-54: LGTM!Also applies to: 107-128
apps/unwired-mail/unwired-mail/Features/Settings/PrivacyDataSettingsView.swift (1)
13-23: LGTM!Also applies to: 25-40, 42-59, 67-80
apps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swift (2)
242-259: LGTM!Also applies to: 1506-1507, 1525-1526
536-541: 🎯 Functional CorrectnessNo production caller emits
SettingsRoute.storage; this change does not expose an active deep-link path.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift (1)
203-203: LGTM!Also applies to: 225-227, 229-295, 297-326, 360-381, 414-456, 631-631, 823-823
apps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swift (1)
118-131: LGTM!Also applies to: 152-156, 179-190
apps/unwired-mail/unwired-mail/Features/Account/RemoteMessageContentNotice.swift (1)
8-35: LGTM!Also applies to: 37-57
apps/unwired-mail/unwired-mail/Services/Mailbox/RemoteMessageContentPresentation.swift (1)
375-380: LGTM!apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swift (1)
1852-1868: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8bfca4f28
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 392218a100
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (1)
431-469: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd cancellation coverage for attachment retrieval.
Cancel an in-flight
loadMessageAttachmentrequest. Assert that it throwsCancellationErrorand does not continue with a successful attachment result. The changed tests cover size failures but not the required cancellation path.As per path instructions: “Verify externally observable behavior, privacy invariants, error paths, cancellation or restart behavior where relevant.”
🤖 Prompt for 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. In `@apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift` around lines 431 - 469, Add cancellation coverage alongside testAttachmentDownloadRejectsDeclaredAndReceivedOversizeResponses by starting an in-flight loadMessageAttachment request, cancelling it before completion, and asserting it throws CancellationError rather than returning an attachment. Use the fixture’s controllable request/session mechanism and verify the request does not continue to a successful result.Source: Path instructions
🤖 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
`@apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift`:
- Around line 219-220: Update the .task(id: taskId) download flow so a
user-initiated trigger remains pending across cancellation and network-state
restarts instead of being consumed at task start. Call requestTracker.finish()
only after the download succeeds or fails with a non-cancellation error; do not
finish on cancellation, and preserve actor isolation and existing retry/offline
behavior.
- Around line 84-113: Update AutomaticAttachmentDownloadCoordinator.acquire to
use a cancellation-aware continuation pattern like AccountView.swift, removing
the cancelled waiter from waiters and completing the awaiting task without
leaving it suspended. Track waiter cancellation state as needed, and update
release to skip and avoid resuming waiters whose tasks are already cancelled
while preserving the maximumConcurrentDownloads accounting and slot handoff
behavior.
In `@apps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swift`:
- Around line 1668-1670: Update the .task closure in the .privacyAndData case to
explicitly discard the Bool returned by gmailViewModel.load() by assigning the
awaited result to _. Preserve the existing asynchronous load behavior.
In `@apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift`:
- Around line 466-504: Add focused coverage for
DownloadedAttachmentStore.clearAll(), saving attachments associated with two
distinct connection IDs, invoking clearAll(), and asserting
existingURL(attachment:messageId:) returns nil for both messages. Preserve the
existing clear(connectionId:) eviction assertions while verifying that full
teardown removes decrypted attachment data across every connection.
---
Outside diff comments:
In `@apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift`:
- Around line 431-469: Add cancellation coverage alongside
testAttachmentDownloadRejectsDeclaredAndReceivedOversizeResponses by starting an
in-flight loadMessageAttachment request, cancelling it before completion, and
asserting it throws CancellationError rather than returning an attachment. Use
the fixture’s controllable request/session mechanism and verify the request does
not continue to a successful result.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a2843b9-5dc9-4568-887a-dcbd5f9721d2
📒 Files selected for processing (14)
README.mdapps/unwired-mail/unwired-mail.xcodeproj/project.pbxprojapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftdocs/adr/0030-gate-incoming-attachment-downloads.md
💤 Files with no reviewable changes (1)
- apps/unwired-mail/unwired-mail.xcodeproj/project.pbxproj
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (4)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Treat docs as product truth, not marketing. Prefer precise domain language
from CONTEXT.md; flag undefined terms, synonym drift, and conflicts with
existing ADRs under docs/adr/. For ADR changes, require a clear decision,
consequences, and privacy or security boundary impact; flag silent
reversals of prior ADRs without an explicit supersession note. For
docs/agents/ and bootstrap docs, verify agent workflow, validation, and
triage guidance stay aligned with AGENTS.md. Flag broken cross-links,
speculative future claims presented as shipped behavior, and docs that
omit the Apple vs TypeScript ownership of a boundary they describe.
Files:
README.mddocs/adr/0030-gate-incoming-attachment-downloads.md
apps/unwired-mail/**/*.{swift,mm,m,h}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands; use
zsh scripts/check-apple-lint.zshfor formatting and linting and the specifiedxcodebuild testcommand for tests.
Files:
apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode so warnings fail validation; use mise-managed SwiftLint and Xcode's
xcrun swift-formatwhere applicable.
Files:
apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
🔇 Additional comments (15)
apps/unwired-mail/unwired-mail/Services/Mailbox/MessageContentPreferences.swift (1)
97-105: LGTM!Also applies to: 119-144
apps/unwired-mail/unwired-mail/Features/Settings/SettingsScene.swift (1)
242-259: LGTM!Also applies to: 536-541, 1506-1507, 1525-1526
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
1436-1437: LGTM!Also applies to: 3704-3710, 6330-6335, 7306-7342, 8330-8330
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift (3)
6-24: LGTM!Also applies to: 43-82, 143-176
273-390: LGTM!
184-186: 🩺 Stability & AvailabilityNo production injection issue.
UnwiredMailApp.swiftinjectsattachmentNetworkMonitorinto all threeWindowGroupscenes. Only theAccountViewpreview omits it.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift (1)
200-202: LGTM!Also applies to: 204-231, 233-291, 293-392, 394-420, 422-464, 506-548, 721-724, 913-916
apps/unwired-mail/unwired-mail/Features/Account/MessageHTMLView.swift (1)
118-147: LGTM!Also applies to: 149-157, 176-202
apps/unwired-mail/unwired-mailTests/MessageHTMLPresentationTests.swift (1)
1852-1889: LGTM!README.md (1)
40-42: LGTM!Also applies to: 85-85, 125-125
docs/adr/0030-gate-incoming-attachment-downloads.md (1)
11-11: LGTM!apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (1)
1453-1455: LGTM!apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift (1)
941-957: LGTM!Also applies to: 977-977, 1030-1073, 1277-1281, 1398-1500, 1719-1754, 2041-2110
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
1980-2045: LGTM!Also applies to: 2257-2267
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (1)
248-383: LGTM!Also applies to: 385-429, 3110-3228
|
@coderabbitai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b8a63775
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0ce7dc0bc
ℹ️ 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift (1)
242-295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFinishing the request tracker on cancellation, and the missing regression test, share one root cause.
requestTracker.finish()is called even when the download task is cancelled, so a network- or policy-triggered restart of the same.task(id:)can downgrade a pending user-initiated download to.automaticand have it silently blocked by policy. No existing test exercises the exact sequence that reproduces this.
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift#L242-L295: remove therequestTracker.finish(requestCount: handledRequestCount)call from thecatch is CancellationErrorbranch so a cancelled-without-a-newer-request task leaves the trigger pending for the restart.apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift#L405-L417: add a case to (or alongside)testCancelledAttachmentDownloadDoesNotConsumeNewerRequestthat callsrequest(),consumeTrigger()(expect.userInitiated), thenfinish(requestCount: tracker.requestCount)with no interveningrequest()call, and asserts the nextconsumeTrigger()still returns.userInitiatedonce the fix above is separately handled at the call-site level (i.e., assert the tracker never loses a request that hasn't actually completed).🤖 Prompt for 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. In `@apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift` around lines 242 - 295, Remove requestTracker.finish(requestCount: handledRequestCount) from the CancellationError branch of the .task(id:) flow in apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift:242-295, leaving cancelled requests pending for restart. In apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift:405-417, extend or add coverage alongside testCancelledAttachmentDownloadDoesNotConsumeNewerRequest to request, consume the user-initiated trigger, finish without another request, and assert the next consumeTrigger() remains user-initiated.apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
81-90: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBound
loadMessageBodyconcurrency or bound gate wait time.
bodyLoadGateis shared by allGmailInboxViewModelinstances for oneproductAccountId. It serializes everyloadMessageBodynetwork and decode operation, including plain-text messages.RemoteMessageContentLoadGate.acquire()has no wait deadline, so queued loads can remain blocked until the active reader finishes or the task is cancelled. Use a bounded concurrency policy or a maximum wait duration, and add a cross-window latency test.🤖 Prompt for 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. In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around lines 81 - 90, Update LoadedMessageImageBudget and the loadMessageBody flow to avoid unbounded serialization and indefinite waits on the shared bodyLoadGate: either permit bounded concurrent body loads or enforce a maximum acquire wait duration with cancellation/timeout handling. Preserve existing loading behavior for successful acquisitions, and add a cross-window latency test covering contention between GmailInboxViewModel instances sharing the same productAccountId.
🤖 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 `@apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift`:
- Around line 1976-1980: Centralize the synthetic attachment ID prefix by
defining a shared constant for "inline-data-" and replacing the duplicated
literal across attachment ID generation, cache filtering, and
retained-presentation filtering. Update the relevant attachment-processing
symbols, including the shown attachments filter, to reference that constant
consistently.
---
Outside diff comments:
In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift`:
- Around line 81-90: Update LoadedMessageImageBudget and the loadMessageBody
flow to avoid unbounded serialization and indefinite waits on the shared
bodyLoadGate: either permit bounded concurrent body loads or enforce a maximum
acquire wait duration with cancellation/timeout handling. Preserve existing
loading behavior for successful acquisitions, and add a cross-window latency
test covering contention between GmailInboxViewModel instances sharing the same
productAccountId.
In
`@apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift`:
- Around line 242-295: Remove requestTracker.finish(requestCount:
handledRequestCount) from the CancellationError branch of the .task(id:) flow in
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift:242-295,
leaving cancelled requests pending for restart. In
apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift:405-417,
extend or add coverage alongside
testCancelledAttachmentDownloadDoesNotConsumeNewerRequest to request, consume
the user-initiated trigger, finish without another request, and assert the next
consumeTrigger() remains user-initiated.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6998cd37-a74c-4750-b1bc-059aa7ab78b9
📒 Files selected for processing (6)
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (3)
apps/unwired-mail/**/*.{swift,mm,m,h}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands; use
zsh scripts/check-apple-lint.zshfor formatting and linting and the specifiedxcodebuild testcommand for tests.
Files:
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode so warnings fail validation; use mise-managed SwiftLint and Xcode's
xcrun swift-formatwhere applicable.
Files:
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
🔇 Additional comments (9)
apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift (3)
14-28: LGTM!Also applies to: 254-259
1887-1934: LGTM!
966-1006: 🎯 Functional CorrectnessConfirm the repeated-fetch policy for cached data-backed attachments.
When
presentationDatais present,decoderemoves theinline-data-*attachment and setsdidResolveAttachmentstofalse.loadMessageBodythen callsloadFreshMessageBodyon every subsequent open. Add a two-open test and confirm this behavior is intentional rather than legacy-cache migration logic.apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (1)
190-469: LGTM!Also applies to: 787-815, 3134-3266
apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift (1)
3320-3363: LGTM!Also applies to: 3566-3619, 4165-4252, 7679-7719
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (2)
6180-6260: LGTM!Also applies to: 6311-6395, 6397-6425
4302-4596: 🗄️ Data Integrity & IntegrationNo change required. Attachment loading, fallback preservation, and message-identifier threading are correctly wired.
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift (1)
13-29: LGTM!apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift (1)
332-369: LGTM!Also applies to: 437-479, 601-666, 694-710
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf104fd480
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ea03a7998
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift (1)
253-309: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not consume the request on cancellation.
When a network change restarts
.task(id: taskId), cancellation cleanup can run before the replacement task callsconsumeTrigger(). The replacement task then sees.automaticand can return.blockedByPolicyunder.wifion cellular. RemoverequestTracker.finishfrom the cancellation branch and add a row-level restart test.🤖 Prompt for 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. In `@apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift` around lines 253 - 309, Update the cancellation branch in the .task(id: taskId) flow to avoid calling requestTracker.finish, preserving the pending request for the replacement task after network-driven restarts. Keep finish calls for successful, blocked, and error paths, and add a row-level test covering restart from Wi-Fi to cellular under the .wifi policy without consuming the automatic request.Source: Path instructions
🤖 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.
Outside diff comments:
In
`@apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift`:
- Around line 253-309: Update the cancellation branch in the .task(id: taskId)
flow to avoid calling requestTracker.finish, preserving the pending request for
the replacement task after network-driven restarts. Keep finish calls for
successful, blocked, and error paths, and add a row-level test covering restart
from Wi-Fi to cellular under the .wifi policy without consuming the automatic
request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccb22647-9ffb-4089-9cd0-fff4e436d7c4
📒 Files selected for processing (7)
.github/workflows/ci.ymlapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (4)
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/**/*.{yml,yaml}: CI must run frozen dependency installation, TypeScript linting, formatting, type checks, tests, Fallow, strict Apple formatting and linting, and the specified Xcode test when the Xcode project exists.
Keep TypeScript, Fallow, and Apple validation in separate CI jobs; usefallow-rs/fallow@v3for Fallow; document why any temporarily non-blocking bootstrap job is non-blocking and what issue will make it required.
Apple validation must use an affected-project matrix with path filters and matching matrix entries; shared Apple tooling changes run all configured projects, while other changes run only affected projects.
Files:
.github/workflows/ci.yml
⚙️ CodeRabbit configuration file
.github/workflows/**/*.{yml,yaml}: Check least-privilege permissions, immutable third-party action pins,
cache safety, secret handling, path filters, and parity with the local
validation commands documented in AGENTS.md.
Files:
.github/workflows/ci.yml
apps/unwired-mail/**/*.{swift,mm,m,h}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands; use
zsh scripts/check-apple-lint.zshfor formatting and linting and the specifiedxcodebuild testcommand for tests.
Files:
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
SwiftLint runs in strict mode so warnings fail validation; use mise-managed SwiftLint and Xcode's
xcrun swift-formatwhere applicable.
Files:
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift
🔇 Additional comments (7)
.github/workflows/ci.yml (1)
141-141: LGTM!apps/unwired-mail/unwired-mailTests/SettingsDestinationRegistryTests.swift (1)
332-349: LGTM!Also applies to: 412-435
apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift (1)
6-8: LGTM!Also applies to: 1899-1990, 2048-2117
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (1)
787-837: LGTM!apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
81-90: LGTM!Also applies to: 6050-6075, 6183-6201, 6311-6394
apps/unwired-mail/unwired-mail/Features/Account/MessageAttachmentDownload.swift (1)
13-29: LGTM!Also applies to: 65-93
apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift (1)
3320-3363: LGTM!Also applies to: 3566-3620, 4166-4253, 7680-7721
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Closes #126
Summary
Validation
mise exec -- zsh scripts/check-apple-lint.zshmise exec -- xcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,id=D6D43A14-3B1A-41B6-B25A-0DFCA1BFB089' -parallel-testing-enabled NO(1,374 tests passed)Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.