Repository navigation
Promote Pins from messages to Threads - #388
Conversation
|
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
WalkthroughPins now use stable mailbox-scoped thread identities. Synchronization migrates legacy message pins, repairs rethreaded anchors, and deduplicates records. Mailbox projection, UI actions, and body prefetch now operate at thread scope. ChangesThread-scoped pin synchronization
Pinned-thread body prefetch
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MailShell
participant PinViewModel
participant PinSyncService
participant MailboxConnection
participant GmailMessageBodyService
MailShell->>PinViewModel: togglePin(threadId, anchorMessageId)
PinViewModel->>PinSyncService: setPinned(threadId, anchorMessageId)
PinSyncService->>MailboxConnection: reconcile thread and anchor identities
MailShell->>MailboxConnection: prefetchMessageBodies(pinnedThreadIds)
MailboxConnection->>GmailMessageBodyService: pass provider thread IDs
GmailMessageBodyService->>GmailMessageBodyService: select eligible messages and reconcile cache
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
gipity-bot
Bot
added
codex
codex-automation
human needed
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 0eefd19652
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/AccountView.swift`:
- Around line 5529-5552: Extract the shared pinned-thread merge logic from
reconcile and load into one private method that accepts the loaded thread-ID set
and changedThreadIds, then updates pinnedThreadIds by preserving locally
updating or changed IDs while applying the loaded set for the rest. Replace both
inline implementations with calls to this method, keeping each caller’s existing
identity-set source unchanged.
- Around line 1783-1790: Update reconcilePins to store the reconciliation Task
in a property, cancel any previous task before starting a new one, and guard
result application so only the current task can update PinViewModel.reconcile
state. Cancel and clear pinReconcileTask in the existing onDisappear lifecycle
handler, preserving actor isolation and allowing cancellation during offline or
retry transitions.
In `@apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift`:
- Around line 1146-1155: Move the pinned-message eligibility calculation into
GmailMessageBodyPrefetchPlan by exposing the unfiltered pinned message ID set
from its initializer. Update the call site near the plan construction to use
that plan property instead of repeating the isExcludedFromBodyPrefetch and
pinnedThreadIds predicate, while preserving pinnedMessages’ existing subtraction
of recently selected messages.
In `@apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift`:
- Around line 256-290: Move construction of the proposed payload into the
records.update closure, after validating the current record and advancing the
change clock from current.changedAtMilliseconds. Use that freshly built payload
for the isNewer comparison and write result, while preserving the existing
behavior when no current record exists.
- Around line 292-312: Update loadThreadRecords and loadLegacyRecords to use a
tolerant listing path that skips individual decode failures and
PinSyncError.invalidPayload while continuing to load valid records. Preserve
cancellation and transport errors, and add coverage for one valid plus one
invalid record in each family.
In `@apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift`:
- Around line 685-701: Update
testLegacyMessageWithoutReliableLinkageMigratesToSingletonThread so its input
message has an empty or unreliable providerThreadId and rfcMessageId: nil,
rather than precomputing and supplying the expected singleton identity as
providerThreadId. Keep the expected singletonThreadId assertion and legacy pin
migration assertion to verify reconcilePins synthesizes the fallback identity.
- Around line 722-748: Extend
testPinnedThreadRepairsWhenAnchorMessageIsRethreaded to call loadPinnedThreadIds
after both reconciliation operations and assert that only repairedThreadId is
loaded. Verify the superseded Self.threadId record is excluded, while preserving
the existing reconcile result and payload count assertions.
- Around line 424-460: Expose the production legacy identifier derivation used
by PinSyncService through a suitable internal helper, then update the test’s
legacyIdentifier(for:) to call it instead of duplicating the SHA256
construction. Keep LegacyPinTestPayload.schemaVersion aligned with the
production schema while removing only the duplicated identifier logic.
- Around line 994-996: Update payloadCount to replace the unavailable
payloads.keys.count(where:) usage with the Swift 5-compatible
payloads.keys.filter { $0.hasPrefix(prefix) }.count expression, preserving the
existing prefix-matching behavior.
In `@apps/unwired-mail/unwired-mailTests/SwipePreferenceSyncServiceTests.swift`:
- Around line 192-205: Update testPinSwipeReflectsTheThreadPinAcrossMessages to
create a second message with a distinct providerMessageId but the same
providerThreadId as older. Set the second message’s threadIdentity as
pinTargetThreadId while retaining older.threadIdentity in pinnedThreadIds, then
keep the expected “Unpin” assertion to verify cross-message thread pin
recognition.
🪄 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: b76a2f50-d7aa-4a9c-9157-13c11976b91d
📒 Files selected for processing (17)
.changeset/promote-thread-pins.mdapps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Features/Account/SwipeActions.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/MicrosoftGraphMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/SwipePreferenceSyncServiceTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Apple performance · Unwired Mail
- GitHub Check: Apple Debug · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (4)
.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/promote-thread-pins.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/MicrosoftGraphMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Features/Account/SwipeActions.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.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-mailTests/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/MicrosoftGraphMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Features/Account/SwipeActions.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.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-mailTests/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/MicrosoftGraphMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Features/Account/SwipeActions.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.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-mailTests/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/MicrosoftGraphMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift
🪛 markdownlint-cli2 (0.23.2)
.changeset/promote-thread-pins.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (28)
apps/unwired-mail/unwired-mailTests/SwipePreferenceSyncServiceTests.swift (1)
137-138: LGTM!Also applies to: 153-154, 174-175
apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (1)
42-52: LGTM!Also applies to: 1041-1052, 1411-1411, 2979-2996
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift (1)
4-32: LGTM!Also applies to: 50-116, 154-190, 204-254, 314-392, 406-419, 437-469
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
1250-1259: LGTM!Also applies to: 1392-1397, 1778-1781, 2312-2334, 2370-2379, 2410-2410, 2910-2916, 2937-2950, 3657-3662, 3693-3694, 3710-3714, 3737-3737, 4345-4345, 4366-4367, 4429-4429, 4666-4670, 4726-4726, 4853-4853, 5485-5490, 5507-5519, 5554-5583, 5592-5597, 6867-6867, 7311-7323, 7406-7406, 7626-7627, 7655-7660, 7669-7669, 7685-7693, 7707-7707, 7736-7736, 7779-7779, 7930-7930, 7953-7954, 7984-7984, 8017-8017, 8026-8026, 8095-8095, 8200-8200
apps/unwired-mail/unwired-mail/Features/Account/SwipeActions.swift (1)
39-40: LGTM!Also applies to: 94-94
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
1231-1231: LGTM!Also applies to: 1261-1266, 1369-1369, 1390-1390, 1399-1404, 1848-1848, 1860-1860, 2350-2356
apps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift (1)
4684-4684: LGTM!apps/unwired-mail/unwired-mailTests/IMAPMailboxConnectionAdapterTests.swift (1)
1736-1736: LGTM!apps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift (4)
2153-2153: LGTM!Also applies to: 2200-2200, 2586-2586, 2640-2640, 2695-2695
2846-2846: LGTM!Also applies to: 2920-2923, 5186-5186, 5495-5503
4590-4590: LGTM!Also applies to: 5043-5043, 5383-5383, 5407-5407, 5413-5413, 5419-5419, 5441-5441, 5446-5446, 5477-5477, 5483-5483, 5532-5532
8121-8135: LGTM!Also applies to: 9111-9111, 9296-9296, 9343-9343
apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift (6)
1-1: LGTM!Also applies to: 26-49
52-93: LGTM!Also applies to: 95-172
174-247: LGTM!Also applies to: 287-299
465-524: LGTM!Also applies to: 526-583, 585-625
974-982: LGTM!Also applies to: 998-1000, 1050-1065
334-345: 🗄️ Data Integrity & IntegrationNo cache-policy change is needed
.authoritativereads the record from the transport. It does not use the ciphertext cache, solegacyPinStateobserves post-migration state.> Likely an incorrect or invalid review comment..changeset/promote-thread-pins.md (1)
1-5: LGTM!apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift (2)
46-76: LGTM!
264-269: LGTM!Also applies to: 293-298
apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift (1)
3629-3667: LGTM!apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift (1)
3377-3402: LGTM!apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (2)
93-93: LGTM!Also applies to: 117-117, 123-183, 196-196
519-519: LGTM!Also applies to: 563-563, 594-594, 628-639, 668-668, 704-704, 731-731, 760-773, 2013-2013
apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift (2)
1403-1428: LGTM!Also applies to: 1430-1471, 1473-1513
3196-3217: LGTM!Also applies to: 3269-3291, 3311-3329, 3349-3364, 7411-7441
apps/unwired-mail/unwired-mailTests/MicrosoftGraphMailboxConnectionAdapterTests.swift (1)
2392-2392: LGTM!
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-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
1026-1084: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTwo
MailboxMessageMetadatareconstruction sites silently drophasAttachments. Both sites reconstructMailboxMessageMetadataafter this PR addedcategoryIdshandling, but neither forwards the pre-existinghasAttachmentsfield. SincehasAttachmentsdefaults tofalseon the struct, Swift's memberwise initializer silently resets it whenever the parameter is omitted, discarding the real attachment state computed just before the reconstruction.
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift#L1026-L1084: inmergedMetadata, addhasAttachments: metadata.hasAttachmentsto the reconstructedMailboxMessageMetadata(...)call. This function backsload,sync,continueBackfill,refreshNewestPages, andoverrideCategory, so every IMAP message currently reportshasAttachments == falseregardless of its real state.apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift#L4205-L4225: inassigningCategory, addhasAttachments: hasAttachmentsto the reconstructedMailboxMessageMetadata(...)call. Every Microsoft Graph message loses its real attachment status after a category override.🐛 Proposed fix for both sites
--- a/apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift +++ b/apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift @@ mergedMetadata categoryIds: Array( Set( appearances.flatMap { appearance in [appearance.categoryId].compactMap { $0 } + (appearance.categoryIds ?? []) }) - ).sorted() + ).sorted(), + hasAttachments: metadata.hasAttachments )--- a/apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift +++ b/apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift @@ assigningCategory subject: subject, categoryIds: [categoryId], - bccRecipients: bccRecipients + bccRecipients: bccRecipients, + hasAttachments: hasAttachments )As per path instructions: "Prioritize functional correctness, data protection, and Apple platform behavior over style already enforced by swift-format and SwiftLint... Require focused tests for changed behavior and failure paths." Add a regression test asserting
hasAttachmentssurvivesmergedMetadata/overrideCategory(IMAP) andassigningCategory(Microsoft Graph) for a message that actually has attachments.🤖 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 1026 - 1084, The reconstructed MailboxMessageMetadata values omit hasAttachments and reset it to false. In apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift:1026-1084, update mergedMetadata to forward metadata.hasAttachments; in apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift:4205-4225, update assigningCategory to forward hasAttachments. Add focused regression coverage proving attachment state survives IMAP mergedMetadata/overrideCategory and Microsoft Graph assigningCategory.Source: Path instructions
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
1401-1407: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCoalesce and cancel pin reconciliation (still unresolved).
reconcilePinsstill starts a new unstructuredTaskon every change ofinboxViewModel.navigationSnapshot.messagesByConnection. That property changes after each connection sync,reloadLocalcall, and backfill step, so several reconcile runs can run at the same time.Two problems remain:
- A slower, earlier run can finish after a newer run and overwrite
pinViewModel.pinnedThreadIdswith a stale result.- The Task is never stored, so
AccountViewcannot cancel it on teardown.Store the task in a
@Stateproperty, cancel the previous one before starting a new one, and cancel it in the existing.onDisappearhandler.♻️ Proposed direction
+ `@State` private var pinReconcileTask: Task<Void, Never>? + private func reconcilePins( with messagesByConnection: [MailboxConnectionId: [MailboxMessageMetadata]] ) { let messages = messagesByConnection.values.flatMap { $0 } - Task { + pinReconcileTask?.cancel() + pinReconcileTask = Task { + guard !Task.isCancelled else { return } await pinViewModel.reconcile(with: messages) } }As per path instructions: "Check actor isolation, cancellation, lifecycle transitions, persistence, retry and offline behavior".
Also applies to: 1805-1812
🤖 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 1401 - 1407, Update AccountView’s pin reconciliation flow around pinReconciledMailShell and reconcilePins by storing the active reconciliation Task in a `@State` property, cancelling it before starting a replacement, and checking cancellation so stale results cannot update pinViewModel.pinnedThreadIds. Cancel and clear the stored task in the existing .onDisappear handler, preserving correct MainActor/lifecycle behavior.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/AccountView.swift`:
- Around line 1401-1407: Update AccountView’s pin reconciliation flow around
pinReconciledMailShell and reconcilePins by storing the active reconciliation
Task in a `@State` property, cancelling it before starting a replacement, and
checking cancellation so stale results cannot update
pinViewModel.pinnedThreadIds. Cancel and clear the stored task in the existing
.onDisappear handler, preserving correct MainActor/lifecycle behavior.
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift`:
- Around line 1026-1084: The reconstructed MailboxMessageMetadata values omit
hasAttachments and reset it to false. In
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift:1026-1084,
update mergedMetadata to forward metadata.hasAttachments; in
apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift:4205-4225,
update assigningCategory to forward hasAttachments. Add focused regression
coverage proving attachment state survives IMAP mergedMetadata/overrideCategory
and Microsoft Graph assigningCategory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78dff05b-c2f2-4d5e-9ee7-c39f3c2649ae
📒 Files selected for processing (6)
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Apple Debug · Unwired Mail
- GitHub Check: Apple performance · Unwired Mail
🧰 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/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/MicrosoftGraphMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/MailboxConnectionAdapterTests.swift
🔇 Additional comments (7)
apps/unwired-mail/unwired-mail/Services/Mailbox/MailboxConnection.swift (2)
843-889: LGTM!Also applies to: 892-953, 1044-1074, 1094-1139
2984-3013: 🗄️ Data Integrity & IntegrationNo change required: Gmail excludes
DRAFT,SPAM, andTRASHmessages during pinned-thread prefetch.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
1267-1273: LGTM!Also applies to: 1796-1803, 3705-3711, 3723-3748, 3754-3762, 3780-3793, 4395-4417, 4488-4497, 4740-4745, 4793-4809, 4913-4936
apps/unwired-mail/unwired-mailTests/MailboxConnectionAdapterTests.swift (1)
2150-2157: LGTM!Also applies to: 2197-2204, 2583-2590, 2637-2644, 2692-2699, 2826-2982, 4580-4592, 5041-5050, 5172-5200, 5380-5391, 5402-5454, 5456-5544
apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift (1)
1573-1691: LGTM!Also applies to: 3633-3705
apps/unwired-mail/unwired-mail/Services/Mailbox/IMAPMailboxConnectionAdapter.swift (1)
31-88: LGTM!apps/unwired-mail/unwired-mail/Services/Mailbox/MicrosoftGraphMailboxConnectionAdapter.swift (1)
371-489: LGTM!Also applies to: 3381-3422
There was a problem hiding this comment.
Actionable comments posted: 2
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/PinSyncServiceTests.swift (1)
776-782: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the loaded pin set directly after repair.
The test proves repair convergence only indirectly. Lines 790-791 assert an empty pin set after the unpin, which implies the superseded
thread-001record is not pinned. Add a direct assertion between the repair and the unpin so a regression in tombstoning fails at the repair step instead of at the unpin step.💚 Proposed assertion
`#expect`(result == [repairedThreadId]) `#expect`(repeatedResult == [repairedThreadId]) + `#expect`( + try await services.firstDevice.loadPinnedThreadIds(session: firstDeviceSession) + == [repairedThreadId]) `#expect`( await services.transport.payloadCount(prefix: PinSyncService.payloadIdentifierPrefix) == 2)🤖 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/PinSyncServiceTests.swift` around lines 776 - 782, In the repair test around PinSyncService, add a direct assertion immediately after the repair results and before the unpin operation that the loaded pin set excludes the superseded thread-001 record and contains the repaired thread as expected. Keep the existing result and payload-count assertions unchanged.Source: Path instructions
♻️ Duplicate comments (2)
apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift (1)
689-705: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe singleton-fallback test still supplies the expected identity as input.
Line 693 builds
singletonThreadIdfrom the literal"message:<connectionId>:<providerMessageId>"format. Line 699 passes that same string as the messageproviderThreadId.threadIdentitytherefore returns the provider thread id directly. No fallback synthesis runs, and the assertion at Line 703 restates the input.Issue
#158requires a singleton fallback when reliable conversation identity and verified RFC reply linkage are unavailable. Supply a message with an emptyproviderThreadIdandrfcMessageId: nil, then assert that the service produces the singleton identity.This matches the path instruction to flag tests that duplicate the implementation instead of exercising it.
🤖 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/PinSyncServiceTests.swift` around lines 689 - 705, Update testLegacyMessageWithoutReliableLinkageMigratesToSingletonThread to construct the message with an empty providerThreadId and rfcMessageId: nil, while retaining the expected singletonThreadId assertion. Ensure the test no longer passes the synthesized singleton identity as input and instead verifies that reconcilePins produces it through the fallback path.Source: Path instructions
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift (1)
342-352: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe proposed payload is still built before the change clock observes the current record.
setPinnedcallsnextChangeAtMilliseconds()at Line 344 and buildsproposedat Line 345.writeThreadPinadvances the clock from the current record at Line 407, afterproposedis fixed. If a remote device wrote the record with a clock ahead of this device,proposed.isNewer(than:)is false,requiringMatchingStateis true, and the toggle throwsPinSyncError.concurrentModification. Every internal retry reuses the same staleproposed.Build the payload inside the
writeThreadPinupdate closure, afteradvanceChangeClock, so each attempt carries a strictly newer timestamp.🤖 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/ProductSync/PinSyncService.swift` around lines 342 - 352, Move proposed payload construction out of setPinned and into the writeThreadPin update closure, after advanceChangeClock has observed the current record. Generate changedAtMilliseconds and the payload inside that closure on every attempt, preserving the resolved thread ID and pin fields so retries always use a strictly newer timestamp.
🤖 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/ProductSync/PinSyncService.swift`:
- Around line 387-396: Update loadRedirectRecords to use the tolerant
record-listing path already used for records and legacyRecords, skipping invalid
redirect payloads while continuing to load valid entries. Preserve cancellation
and transport errors, and keep change-clock updates and result insertion for
accepted records unchanged.
- Around line 303-311: The redirect repair flow around writeRedirect and
resolveRedirect must handle A→B→A flapping without failing the account: refuse
to persist a redirect when its target already resolves back to the former
thread, and make resolveRedirect return the last thread identity on cycle
detection instead of throwing invalidPayload. In
apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift:776-782, add
coverage that reconciles the anchor to thread-repaired, then back to thread-001,
and verifies loadPinnedThreadIds and setPinned still succeed.
---
Outside diff comments:
In `@apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift`:
- Around line 776-782: In the repair test around PinSyncService, add a direct
assertion immediately after the repair results and before the unpin operation
that the loaded pin set excludes the superseded thread-001 record and contains
the repaired thread as expected. Keep the existing result and payload-count
assertions unchanged.
---
Duplicate comments:
In `@apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift`:
- Around line 342-352: Move proposed payload construction out of setPinned and
into the writeThreadPin update closure, after advanceChangeClock has observed
the current record. Generate changedAtMilliseconds and the payload inside that
closure on every attempt, preserving the resolved thread ID and pin fields so
retries always use a strictly newer timestamp.
In `@apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift`:
- Around line 689-705: Update
testLegacyMessageWithoutReliableLinkageMigratesToSingletonThread to construct
the message with an empty providerThreadId and rfcMessageId: nil, while
retaining the expected singletonThreadId assertion. Ensure the test no longer
passes the synthesized singleton identity as input and instead verifies that
reconcilePins produces it through the fallback path.
🪄 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: 6a523129-6ae9-4771-84a3-d584ba3f97af
📒 Files selected for processing (2)
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Apple performance · Unwired Mail
- GitHub Check: Apple Debug · Unwired Mail
🧰 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/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.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/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.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/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.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/PinSyncServiceTests.swift
🔇 Additional comments (7)
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift (5)
153-200: LGTM!
241-250: LGTM!
277-296: LGTM!
398-439: LGTM!
484-506: LGTM!Also applies to: 568-577, 625-634
apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift (2)
29-48: LGTM!Also applies to: 63-94, 104-130, 136-141, 172-173, 183-194, 210-216, 244-248, 297-299, 474-528, 538-585, 595-628
707-751: LGTM!
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-mailTests/PinSyncServiceTests.swift (1)
680-710: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTombstone the legacy pins after successful migration.
reconcilePinshas completed durable thread-pin synchronization when this test observes one thread payload. The test then expects both legacy message pins to remain active. This conflicts with the required migration order: preserve legacy pins on a failed thread-pin write, then remove them after a successful durable write.Change the successful-migration assertions to require inactive legacy records. Keep
testLegacyPinRemainsUntilThreadPinWriteSucceedsas the failure-path coverage.Proposed test update
- try await services.legacyPinState(Self.messageId, session: firstDeviceSession) == true) + try await services.legacyPinState(Self.messageId, session: firstDeviceSession) == false) @@ - try await services.legacyPinState(secondMessageId, session: firstDeviceSession) == true) + try await services.legacyPinState(secondMessageId, session: firstDeviceSession) == false)As per path instructions, “Verify externally observable behavior.”
🤖 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/PinSyncServiceTests.swift` around lines 680 - 710, Update testLegacyMessagePinsRemainActiveAndDeduplicateByThreadDuringRollout so both legacyPinState assertions expect inactive records after successful reconcilePins and durable payload delivery. Leave testLegacyPinRemainsUntilThreadPinWriteSucceeds unchanged to cover retention when synchronization fails.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/AccountView.swift`:
- Around line 5694-5696: Update the error-handling path around
PinSyncing.reconcilePins so the generic catch checks whether pinReconcileTask
was cancelled before assigning errorMessage. Ignore errors caused by
cancellation, including cancellation observed after the task is cancelled, while
preserving errorMessage assignment for genuine failures.
---
Outside diff comments:
In `@apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift`:
- Around line 680-710: Update
testLegacyMessagePinsRemainActiveAndDeduplicateByThreadDuringRollout so both
legacyPinState assertions expect inactive records after successful reconcilePins
and durable payload delivery. Leave
testLegacyPinRemainsUntilThreadPinWriteSucceeds unchanged to cover retention
when synchronization fails.
🪄 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: b3fe44ea-bbe0-41ee-af65-1c049f7563b3
📒 Files selected for processing (5)
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/SwipePreferenceSyncServiceTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Apple Debug · Unwired Mail
- GitHub Check: Apple performance · Unwired Mail
🧰 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-mailTests/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Features/Account/AccountView.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/SwipePreferenceSyncServiceTests.swiftapps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift
🔇 Additional comments (7)
apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift (3)
713-729: The singleton fallback is still not covered.Line 724 supplies the expected
message:identity asproviderThreadId. The reconciliation path can reuse that value instead of synthesizing the singleton identity. Use an empty or unreliable provider thread ID with no RFC linkage.
428-438: The test still duplicates the legacy identifier contract.
legacyIdentifier(for:)duplicates the production SHA-256 identifier derivation. Reuse an internal production helper from the test target.
303-350: LGTM!Also applies to: 467-678, 778-823, 914-963, 1176-1190
apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)
1083-1083: LGTM!Also applies to: 1266-1267, 1404-1408, 1812-1815, 5686-5693, 5700-5713
apps/unwired-mail/unwired-mailTests/SwipePreferenceSyncServiceTests.swift (1)
137-138: LGTM!Also applies to: 153-154, 174-175, 192-207
apps/unwired-mail/unwired-mail/Services/Gmail/GmailMessageBodyService.swift (1)
39-79: LGTM!Also applies to: 1140-1188
apps/unwired-mail/unwired-mailTests/GmailMessageBodyServiceTests.swift (1)
93-93: LGTM!Also applies to: 117-117, 134-187, 201-201, 524-524, 568-568, 599-599, 635-641, 673-673, 709-709, 736-736, 767-775, 2018-2018
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ProductSync/PinSyncService.swift`:
- Around line 303-310: Make concurrent reverse redirects converge
deterministically: update the redirect-write flow around resolveRedirect and
writeRedirect in
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift#L303-L310
to prevent or collapse A→B/B→A races; update cycle handling at
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swift#L517-L518
so every cycle member resolves to one canonical thread identity; add the
two-device race coverage in
apps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift#L825-L860,
verifying later load, pin, and unpin operations use the same identity.
🪄 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: 99847af1-22ef-4eaa-8c64-01e078bb9090
📒 Files selected for processing (2)
apps/unwired-mail/unwired-mail/Services/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Apple performance · Unwired Mail
- GitHub Check: Apple Debug · Unwired Mail
🧰 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/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.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/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.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/ProductSync/PinSyncService.swiftapps/unwired-mail/unwired-mailTests/PinSyncServiceTests.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/PinSyncServiceTests.swift
1 participant
Closes #158
Summary
Validation
mise exec -- zsh scripts/check-apple-lint.zsh— passed (strict formatting and SwiftLint)PinSyncServiceTests— passedtestEditingOutboxReplyOnSameConnectionPreservesProviderReplyMetadata(requestsReadReceipt:)failed forrequestsReadReceipt == truebecause the replacement retainedfalse; theMailboxConnectionAdapterTestssuite rerun reproduced the same failure (178 tests, one failure)TESTING CI_PERFORMANCE_BUDGET— passed (1 test)Documentation
CONTEXT.md, ADR-0012, and ADR-0023 already describe Thread-scoped Pins, migration ordering, Stable Thread Identity, and bounded pinned-Thread body-cache behavior, so no additional documentation edit was required.Remaining human action
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.