Repository navigation
Add encrypted Mail Profile foundation - #448
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 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 (5)
📝 WalkthroughSummary
Migration and privacy
Affected areas
Validation
WalkthroughThe PR adds encrypted Mail Profile models and Product Sync storage. It migrates legacy mailbox connections into a deterministic Default Profile, adds profile-scoped queries and edits, supports field-level conflict handling, and documents and tests the behavior. ChangesMail Profile foundation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MailboxConnectionSyncService
participant ProductSync
participant EncryptedProfileRecord
Client->>MailboxConnectionSyncService: loadProfileSnapshot(session)
MailboxConnectionSyncService->>ProductSync: load active connections
ProductSync-->>MailboxConnectionSyncService: return connection snapshot
MailboxConnectionSyncService->>EncryptedProfileRecord: load and migrate profile payload
EncryptedProfileRecord-->>MailboxConnectionSyncService: return synchronized profiles
MailboxConnectionSyncService-->>Client: return profile snapshot and scoped connections
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
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
`@apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swift`:
- Around line 301-319: The reconciliation logic around activeIds and
retainedAssignments must preserve assignments for connections absent from the
current payload, removing them only when an explicit connection-removal
tombstone exists; continue assigning default profiles to newly active,
unassigned connections. In
apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swift#L301-L319,
update the filtering logic accordingly. In
docs/adr/0048-separate-encrypted-mail-profile-ownership.md#L17-L17, document
this retention rule in the Decision and the resulting ownership consequence in
Consequences.
- Around line 285-300: Update the profile-repair logic before the guard in the
validation flow so a non-nil defaultProfileId that is absent from profiles
re-adds defaultProfile, resets defaultProfileId to defaultProfile.id, and marks
changed. Keep the repair deterministic and idempotent, then allow the existing
guard to validate the repaired profile set.
In
`@apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncService.swift`:
- Around line 568-574: Update the profile-name validation in createProfile and
validateProfilePayload to reject duplicate names and names exceeding 40
characters with a dedicated MailProfileSyncError case, rather than
concurrentModification or generic invalidProfileState. Ensure the new error
communicates invalid or duplicate input and is used consistently for both
validation paths.
- Around line 774-786: Update MailProfileFieldValue.set(_:for:) to return Bool,
returning true only when the field and value cases match and are assigned, and
false for mismatches. In resolveProfileConflict, check this result before
removing the conflict or persisting the profile, and reject the resolution when
it is false so mismatched synchronized payloads cannot be silently discarded.
- Around line 681-694: Update updateProfiles so
Self.validateProfilePayload(payload) runs only when mutation returns true and
the payload will be written; allow the changed == false read path to return
.acceptAuthoritative without validating legacy or mixed-version values. Preserve
sorting and resolvedPayload assignment for both paths.
In `@apps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swift`:
- Around line 1127-1149: Add tests alongside
testDefaultProfileMigrationIsIdempotentAndClaimsConnectionsAddedByOlderClients
covering three gaps: retain a non-default Profile assignment when its connection
is removed from and later restored to the payload; accept and return a seeded
mail-profiles-primary payload containing a Profile name longer than 40
characters; and verify a Profile operation throws
MailProfileSyncError.missingProductSyncKeyMaterial when key material is absent
while another product-sync payload exists. Reuse the existing services, payload,
and error-test setup patterns.
In `@CONTEXT.md`:
- Around line 395-406: Add relationship bullets in the Relationships section
covering that each Mailbox Connection belongs to exactly one Mail Profile,
Profile-scoped queries require an explicit Mail Profile, and provider
credentials remain device-local and outside Profile Record Scope. Align the
terminology with Mail Profile, Mailbox Connection, and Profile Record Scope as
defined in the diff and ADR 0048.
In `@docs/adr/0048-separate-encrypted-mail-profile-ownership.md`:
- Line 17: Update the ADR’s reconciliation decision, including the text around
the default-profile behavior, to explicitly document what happens when an absent
connection later returns: state that its prior Profile ownership is removed and
it is reassigned to the Default Profile, or document the retention rule that
prevents this. Include the resulting user-visible consequence and any relevant
privacy or security boundary impact.
🪄 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: e1a716ad-cff0-4ae6-8f7e-d5adb7a4f034
📒 Files selected for processing (6)
.changeset/encrypted-mail-profile-foundation.mdCONTEXT.mdapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncService.swiftapps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swiftdocs/adr/0048-separate-encrypted-mail-profile-ownership.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
.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/encrypted-mail-profile-foundation.md
docs/adr/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Record durable architecture decisions and privacy-boundary decisions in
docs/adr/.
Files:
docs/adr/0048-separate-encrypted-mail-profile-ownership.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/0048-separate-encrypted-mail-profile-ownership.mdCONTEXT.md
CONTEXT.md
📄 CodeRabbit inference engine (AGENTS.md)
Update
CONTEXT.mdwhen product language, domain terms, or resolved ambiguity changes.
Files:
CONTEXT.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/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncService.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/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncService.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/MailboxConnectionSyncServiceTests.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swiftapps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncService.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/MailboxConnectionSyncServiceTests.swift
🪛 markdownlint-cli2 (0.23.2)
.changeset/encrypted-mail-profile-foundation.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (8)
.changeset/encrypted-mail-profile-foundation.md (1)
1-3: LGTM!Also applies to: 5-5
apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncModels.swift (2)
1-1: LGTM!Also applies to: 170-260
324-331: LGTM!Also applies to: 334-350, 352-370
docs/adr/0048-separate-encrypted-mail-profile-ownership.md (1)
1-15: LGTM!Also applies to: 19-26, 28-29
apps/unwired-mail/unwired-mail/Services/ProductSync/MailboxConnectionSyncService.swift (3)
75-75: LGTM!Also applies to: 104-109
525-550: LGTM!
588-637: LGTM!Also applies to: 639-667, 713-746, 748-759, 762-772
apps/unwired-mail/unwired-mailTests/MailboxConnectionSyncServiceTests.swift (1)
1089-1125: LGTM!Also applies to: 1151-1175, 1177-1226, 1228-1250, 1416-1418
Closes #390
Summary
Validation
mise exec -- pnpm install --frozen-lockfilemise exec -- pnpm lintmise exec -- pnpm formatmise exec -- pnpm turbo run check-typesmise exec -- pnpm testmise exec -- pnpm fallow(zero issues)mise exec -- zsh scripts/check-apple-lint.zsh(122 files, zero violations)MailboxConnectionSyncServiceTestson iPhone 17 Simulator UDIDD6D43A14-3B1A-41B6-B25A-0DFCA1BFB089(37 tests passed)Remaining human actions
Profile switching, lifecycle UI, lock behavior, and complete Settings integration remain tracked by the existing child issues and are intentionally outside #390.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.