Repository navigation
Add Attachment Preview to standards-based mail - #431
gipity-bot[bot] wants to merge 6 commits into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 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 (2)
📝 WalkthroughSummary
Affected areas
Scope
Validation
WalkthroughChangesIMAP attachment preview
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MailboxClient
participant IMAPMailboxConnectionAdapter
participant IMAPMessageBodyService
participant SystemIMAPMailboxClient
participant SwiftMail
MailboxClient->>IMAPMailboxConnectionAdapter: request message attachment
IMAPMailboxConnectionAdapter->>IMAPMessageBodyService: authorize and gate download
IMAPMessageBodyService->>SystemIMAPMailboxClient: fetch matching MIME section
SystemIMAPMailboxClient->>SwiftMail: retrieve and decode attachment
SwiftMail-->>SystemIMAPMailboxClient: return attachment bytes
SystemIMAPMailboxClient-->>IMAPMessageBodyService: return validated data
IMAPMessageBodyService-->>IMAPMailboxConnectionAdapter: return attachment data
IMAPMailboxConnectionAdapter-->>MailboxClient: provide attachment data
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@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: 8
🤖 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/preview-imap-attachments.md:
- Line 5: Update .changeset/preview-imap-attachments.md line 5 to use
“Attachment Preview,” “Standards-Based Mailbox Connections,” and protected
“Downloaded Attachment” storage. Update
docs/adr/0030-gate-incoming-attachment-downloads.md line 9 to use “Inline
Image,” “Bounded Encrypted Body Cache,” “Attachment Preview,” and “Downloaded
Attachment,” explicitly stating that downloaded bytes remain device-local.
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift`:
- Around line 1445-1460: Update the cached-body decoding flow around
IMAPCachedMessageBodyPayload so a successfully decoded payload with a version
other than currentVersion is treated as unsupported and does not enter the
legacy UTF-8 fallback. Preserve the legacy branch only for data that cannot
decode as the payload type, and ensure the version mismatch propagates through
the existing catch/removal and refresh path.
- Around line 1288-1291: Update the catch block in the body-loading method to
rethrow CancellationError before checking the cached body, while preserving the
cache fallback for other provider errors. Keep cancellation behavior consistent
with prefetchMessageBodies.
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swift`:
- Around line 128-139: Update the fetch-bound calculation in the attachment
download flow around attachmentPart and maximumRequestedByteCount so a declared
byteCount of zero is resolved as the unknown-size case before constructing the
BODY.PEEK range. Ensure zero requests the full
MailboxMessageAttachmentPolicy.maximumByteCount allowance, while positive
declared sizes retain their existing byteCount + 1 limit.
- Around line 314-318: Update the response-reading loop around
trailingLiteralLength and response.append so it tracks the total accumulated
response size, including literals and other appended line data. Compare that
cumulative size against maximumLiteralByteCount and throw
MailboxMessageAttachmentError.invalidResponse as soon as the bound is exceeded,
while preserving the existing per-literal validation.
- Around line 895-905: Update the attachment parsing flow associated with
isAttachment so MailboxMessageAttachment.byteCount uses the decoded payload size
rather than the encoded BODYSTRUCTURE size in values[6]. Retain the encoded size
for BODY.PEEK read bounds, and add a BASE64 boundary test covering a decoded
payload at the maximum allowed byte count.
In `@apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift`:
- Around line 972-1034: Add coverage for the two untested paths: extend
RecordingIMAPClient with a beforeAttachmentReturn suspension hook and test that
cancelling loadMessageAttachment maps to CancellationError, then add a test that
runs prefetchMessageBodies before loadMessageBody for an attachment-bearing
message and verifies a second body request refreshes attachment descriptors.
Reuse the existing adapter, cache, and recording-client test helpers while
asserting the externally visible request and descriptor behavior.
In `@docs/adr/0030-gate-incoming-attachment-downloads.md`:
- Line 9: Update the ADR paragraph to explicitly identify
SystemIMAPMailboxClient and IMAPMailboxConnectionAdapter as the Apple Swift
owners of the provider-neutral attachment descriptor and download boundary, and
state that no TypeScript layer participates in this boundary.
🪄 Autofix
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: 4bb8b741-2b6b-42fb-81a0-52e0670284f5
📒 Files selected for processing (5)
.changeset/preview-imap-attachments.mdapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftdocs/adr/0030-gate-incoming-attachment-downloads.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple Debug · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (6)
docs/adr/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Record durable architecture decisions and privacy-boundary decisions in
docs/adr/.
Files:
docs/adr/0030-gate-incoming-attachment-downloads.md
**/*.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:
docs/adr/0030-gate-incoming-attachment-downloads.md
.changeset/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Create a Changeset for runtime behavior changes or exported type/API changes; tests-only, internal refactors, documentation-only changes, and JSDoc-only maintenance may omit one at maintainer discretion.
Files:
.changeset/preview-imap-attachments.md
apps/unwired-mail/**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands. Run
zsh scripts/check-apple-lint.zshfor format and lint, and use the documentedxcodebuild testcommand for tests; SwiftLint runs in strict mode.
Files:
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
Apple validation must use SwiftLint strict mode and Swift Format strict linting; if Apple tooling is unavailable, report that clearly in the final handoff.
Files:
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.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/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.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/IMAPMailboxConnectionAdapterTests.swift
🪛 markdownlint-cli2 (0.23.2)
.changeset/preview-imap-attachments.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (3)
apps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swift (1)
77-111: LGTM!Also applies to: 861-863, 989-1014
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
194-223: LGTM!Also applies to: 1198-1224, 1264-1273, 1304-1327, 2042-2064
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift (1)
1278-1337: LGTM!Also applies to: 1339-1377, 1379-1417, 1711-1743
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 (2)
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift (2)
1769-1793: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicate
hasAttachmentsparameter. Swift cannot compileimapMessagewithhasAttachmentsdeclared at both lines 1763 and 1769.🤖 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/IMAPMailboxConnectionAdapterTests.swift` around lines 1769 - 1793, Remove the duplicate hasAttachments declaration from the imapMessage helper, retaining a single parameter and its existing propagation into IMAPProviderMessage. Ensure the helper continues to initialize message.hasAttachments consistently.
1500-1540: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerify the attachment descriptor for the GreenMail fixture.
This test only verifies
hasAttachments. It does not verify thatloadMessageBodyexposes the expected attachment descriptor. A regression can show an attachment indicator but provide no preview.Load the fixture body and assert the part ID, filename, MIME type, and byte count. As per path instructions: "Verify externally observable behavior, privacy invariants, error paths, cancellation or restart behavior where relevant, and deterministic isolation."
🤖 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/IMAPMailboxConnectionAdapterTests.swift` around lines 1500 - 1540, Extend testSystemClientRecognizesGreenMailAttachmentBodyStructure to load the fixture body after metadata and verify the exposed attachment descriptor, including part ID, filename, MIME type, and byte count. Use the existing fixture message and authorization flow, and retain the hasAttachments assertion while checking the externally observable attachment details.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-mailTests/IMAPMailboxConnectionAdapterTests.swift`:
- Around line 1769-1793: Remove the duplicate hasAttachments declaration from
the imapMessage helper, retaining a single parameter and its existing
propagation into IMAPProviderMessage. Ensure the helper continues to initialize
message.hasAttachments consistently.
- Around line 1500-1540: Extend
testSystemClientRecognizesGreenMailAttachmentBodyStructure to load the fixture
body after metadata and verify the exposed attachment descriptor, including part
ID, filename, MIME type, and byte count. Use the existing fixture message and
authorization flow, and retain the hasAttachments assertion while checking the
externally observable attachment details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 333d39cd-497a-4a37-91a9-25e22a4c2c40
📒 Files selected for processing (2)
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
apps/unwired-mail/**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands. Run
zsh scripts/check-apple-lint.zshfor format and lint, and use the documentedxcodebuild testcommand for tests; SwiftLint runs in strict mode.
Files:
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
Apple validation must use SwiftLint strict mode and Swift Format strict linting; if Apple tooling is unavailable, report that clearly in the final handoff.
Files:
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.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/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.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/IMAPMailboxConnectionAdapterTests.swift
🔇 Additional comments (6)
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (5)
1289-1291: PropagateCancellationErrorbefore the cache fallback.This catch block still returns a cached body after cancellation. The existing review comment covers this unchanged issue.
1104-1105: LGTM!
1265-1274: LGTM!
1305-1327: LGTM!
2043-2065: LGTM!apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift (1)
660-680: LGTM!Also applies to: 980-1040, 1286-1423
|
@codex review |
![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: 41bc0ab2ba
ℹ️ 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 (1)
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
2067-2079: 🚀 Performance & Scalability | 🔵 TrivialConsider the sync-gate hold time for attachment downloads.
The attachment download runs inside
syncGate.withLock(connection.id). An attachment can reach 25 MB, so one download can hold the per-connection lock for the whole transfer. During that timesyncInbox,continueHistoricalBackfill,prefetchMessageBodies, and other body loads on the same Mailbox Connection wait.The lock is also what guarantees that cleanup cannot interleave with provider access, so do not remove it without preserving that invariant. One option is to acquire the gate only for
authorizationForProviderAccessand to hold a separate download-scoped guard for the transfer, with an explicit re-check of the authorization generation after the download completes.🤖 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/IMAPMailboxConnectionAdapter.swift` around lines 2067 - 2079, Reduce the duration of the per-connection sync gate in the attachment-loading flow around authorizationForProviderAccess and bodyReader.loadMessageAttachment: do not hold syncGate.withLock through the full download. Preserve cleanup/provider-access serialization by introducing the appropriate download-scoped guard, and re-check the authorization generation after the transfer completes before returning.
🤖 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/Mailbox/SystemIMAPMailboxClient.swift`:
- Around line 189-196: Update the attachment lookup in the guard within the
mailbox attachment download flow to match the selected part by attachment.id
rather than full MailboxMessageAttachment equality. Keep the existing
encodedByteCount extraction and bounds validation unchanged after selecting the
matching part.
---
Outside diff comments:
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift`:
- Around line 2067-2079: Reduce the duration of the per-connection sync gate in
the attachment-loading flow around authorizationForProviderAccess and
bodyReader.loadMessageAttachment: do not hold syncGate.withLock through the full
download. Preserve cleanup/provider-access serialization by introducing the
appropriate download-scoped guard, and re-check the authorization generation
after the transfer completes before returning.
🪄 Autofix
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: a6e65031-8d33-4b46-a80f-d1803a10a874
📒 Files selected for processing (5)
.changeset/preview-imap-attachments.mdapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftdocs/adr/0030-gate-incoming-attachment-downloads.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
.changeset/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Create a Changeset for runtime behavior changes or exported type/API changes; tests-only, internal refactors, documentation-only changes, and JSDoc-only maintenance may omit one at maintainer discretion.
Files:
.changeset/preview-imap-attachments.md
docs/adr/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Record durable architecture decisions and privacy-boundary decisions in
docs/adr/.
Files:
docs/adr/0030-gate-incoming-attachment-downloads.md
**/*.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:
docs/adr/0030-gate-incoming-attachment-downloads.md
apps/unwired-mail/**/*.{swift,m,mm}
📄 CodeRabbit inference engine (AGENTS.md)
The Apple app must provide formatter, linter, and test commands. Run
zsh scripts/check-apple-lint.zshfor format and lint, and use the documentedxcodebuild testcommand for tests; SwiftLint runs in strict mode.
Files:
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
Apple validation must use SwiftLint strict mode and Swift Format strict linting; if Apple tooling is unavailable, report that clearly in the final handoff.
Files:
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.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/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.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/IMAPMailboxConnectionAdapterTests.swift
🪛 markdownlint-cli2 (0.23.2)
.changeset/preview-imap-attachments.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (7)
apps/unwired-mail/unwired-mail/Services/Mailbox/SystemIMAPMailboxClient.swift (2)
177-217: LGTM!Also applies to: 266-292, 294-310, 312-349
167-172: 📐 Maintainability & Code QualitySwiftMail API usage is valid. SwiftMail 1.10.0 exposes all referenced symbols, and
fetchPartreturns raw transfer-encoded bytes.decoded(for:)performs the single required decoding step.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
1289-1294: LGTM!Also applies to: 1307-1330, 1448-1473, 1475-1485, 2005-2014, 2058-2066, 2513-2520
apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift (2)
1110-1119: 📐 Maintainability & Code Quality | ⚡ Quick winAssert that the prefetch entry exists before the refresh assertion.
This test targets the refresh branch in
IMAPMessageBodyService.loadMessageBody, which refetches when the cached payload reportsincludesAttachmentMetadata == false. The only assertion about request counts isclient.bodyRequestCount == 2.That count also holds if
prefetchMessageBodiesstored nothing and the second load was a plain cache miss. The test would then pass without exercising the branch. Capture the cache in the test and assert that an entry exists after the prefetch.As per path instructions: "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."
♻️ Proposed assertion for the prefetch entry
+ let cache = RecordingIMAPBodyCache() let adapter = try makeAdapter( authorizationStore: authorizationStore, + cache: cache, client: client, definitions: [definition], keyStore: keyStore )try await adapter.prefetchMessageBodies( connection: connection, pinnedThreadIds: [message.threadIdentity], referenceDate: Date(timeIntervalSince1970: 1_781_200_100), session: session ) + `#expect`( + try cache.loadMessageBody( + productAccountId: session.productAccountId, + stableProviderMessageId: message.stableProviderMessageId + ) != nil) let body = try await adapter.loadMessageBody(message: message, session: session)Source: Path instructions
979-1012: LGTM!Also applies to: 1122-1169, 1379-1471, 1980-1980, 2046-2058
.changeset/preview-imap-attachments.md (1)
1-5: LGTM!docs/adr/0030-gate-incoming-attachment-downloads.md (1)
9-11: LGTM!
1 participant
Summary
BODYSTRUCTUREresponses while excluding readable body parts and inline Content-ID imagesValidation
mise exec -- pnpm lint— passedmise exec -- pnpm format— passedmise exec -- pnpm turbo run check-types— passedmise exec -- pnpm test— passedmise exec -- pnpm fallow— passedmise exec -- zsh scripts/check-apple-lint.zsh— passed (121 Swift files, 0 violations)IMAPMailboxConnectionAdapterTests— passed (37 tests)Human actions
None beyond normal review.
Closes #323
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.