fix(ios): restore profile dedup + remove dead reconnect path#210
Conversation
PR #209's rebase resolution kept the dict-based saved-profile API but dropped the recency-aware dedup that pre-existed on main. Restore it: - shouldPreferProfile picks the newer/Tailscale-preferring/has-last- successful-address profile when two map to the same storage key. - loadSavedProfilesRaw uses it during legacy [HostConnectionProfile] array migration so collisions don't fall to last-write-wins. - saveSavedProfiles runs a paranoia dedup pass before encoding. - reconnectToSavedHost (and its only helper profile(forSavedHost:)) removed; both were unreachable post-rebase, flagged by Greptile.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated saved host profile persistence to deduplicate entries by storage key using a preference function, and removed the public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Here's the review. No blocking issues — changes are correct and well-scoped. Observations below:
Tiebreaker is not a total order Removed functions — confirmed clean Cosmetic nit Overall: logic is sound, deletion is safe, dedup priority order (newer > tailscale > lastSuccessful) is reasonable. The string-timestamp comparison is the only item I'd keep an eye on long-term. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 1479-1490: deduplicatedProfiles currently only collapses entries
with identical profileStorageKey values so it misses pairs where an older
profile used an older storage key; change deduplicatedProfiles to compute a
canonical identity key (e.g., prefer profile.hostIdentity or
profile.lastHostDeviceId when present, falling back to profileStorageKey or
address) for each profile, use that canonical key to dedupe, and keep selection
logic using shouldPreferProfile(existing, new) so the stronger/newer profile
replaces stale ones; update references to profileStorageKey only as a fallback
and ensure returned dictionary uses the latest profile values for each canonical
identity.
- Around line 1426-1434: The tie-breaker in shouldPreferProfile(_:over:) only
checks candidate.tailscaleAddress != nil but must treat any tailnet route in
lastSuccessfulAddress or savedAddressCandidates as "Tailscale presence"; add a
small helper (e.g. isTailscalePresent(_: HostConnectionProfile) -> Bool) that
returns true if profile.tailscaleAddress != nil OR profile.lastSuccessfulAddress
is a tailnet route OR profile.savedAddressCandidates contains a tailnet route,
then replace the tie-break condition that currently checks
candidate.tailscaleAddress != nil && existing.tailscaleAddress == nil with a
comparison using isTailscalePresent(candidate) && !isTailscalePresent(existing)
so equal-updatedAt profiles prefer the one with any tailscale route encoded in
any of those fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1d5d756-a64c-4834-908c-a9c785a81bd1
📒 Files selected for processing (1)
apps/ios/ADE/Services/SyncService.swift
Addresses two CodeRabbit findings on #210: - profileHasTailnetRoute checks tailscaleAddress, lastSuccessfulAddress (via syncIsTailscaleRoute), and savedAddressCandidates so the shouldPreferProfile tie-break treats tailnet routes encoded in any field as equivalent to a populated tailscaleAddress. - deduplicatedProfiles now matches on hostIdentity, lastHostDeviceId, hostName:port, and lastSuccessfulAddress:port simultaneously, so profiles whose storage key evolved (e.g., older entry written before hostIdentity was learned) collapse with the newer stronger-keyed entry instead of leaving both in savedReconnectHosts.
Summary
Follow-up to #209. The rebase resolution there kept the dict-based saved-profile API but dropped the recency-aware dedup that pre-existed on main. This restores it and cleans up dead code Greptile flagged in the PR review.
shouldPreferProfile(_:over:)picks the better profile when two map to the same storage key (newerupdatedAt, then Tailscale presence, then last-successful-address presence).loadSavedProfilesRaw()uses it during legacy[HostConnectionProfile]array migration so collisions don't fall to last-write-wins.saveSavedProfiles(_:)runs a paranoia dedup pass before encoding.reconnectToSavedHost(_:preferTailnet:)and its only helperprofile(forSavedHost:)removed — both unreachable post-rebase, flagged by Greptile P2-A in Context Doc Generation #209.Net diff: +29 / -33 in
apps/ios/ADE/Services/SyncService.swift. Single file.Test plan
:keychain key.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Greptile Summary
This PR restores the recency-aware profile deduplication logic (
shouldPreferProfile,deduplicatedProfiles) that was dropped in the #209 rebase, and removes the unreachablereconnectToSavedHost/profile(forSavedHost:)pair. The dedup pass runs both during legacy array migration and as a paranoia pass insaveSavedProfiles.The two P2 findings are edge cases in
deduplicatedProfiles: (1) when a bridging profile spans two pre-existing canonical slots, only the first-matched slot is merged and the second leaks through; (2) stale identity-key pointers remain inkeyToIndexafter a replacement, which can cause a later unrelated profile to merge into the wrong slot. Neither causes data corruption, but the dedup may be incomplete in adversarial orderings.Confidence Score: 5/5
Safe to merge; all findings are P2 edge cases in the dedup logic that do not corrupt data.
Only P2 findings present — both are incomplete-dedup scenarios requiring specific dict-iteration orderings and profile key configurations that are unlikely in practice. No P0/P1 issues found.
apps/ios/ADE/Services/SyncService.swift — specifically the
deduplicatedProfilesslot-bridging and stale-key logic.Important Files Changed
shouldPreferProfile,deduplicatedProfiles) and removes the deadreconnectToSavedHost/profile(forSavedHost:)pair; two edge-case dedup misses in the slot-bridging and stale-key logic ofdeduplicatedProfiles.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[saveSavedProfiles] --> B[deduplicatedProfiles] B --> C{For each profile} C --> D[profileIdentityKeys] D --> E{Any key in keyToIndex?} E -- yes --> F{shouldPreferProfile?} F -- yes --> G[Replace canonical slot\nUpdate winner keys in keyToIndex] F -- no --> H[Skip] E -- no --> I[Append new canonical slot\nAdd all keys to keyToIndex] G --> C H --> C I --> C C --> J[Rebuild output dict via profileStorageKey] J --> K[Encode and write to UserDefaults]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(ios): broader tailnet detection + cr..." | Re-trigger Greptile