feat(ios): let the user pick or create the target vault when accepting a share (#52) - #55
Conversation
…g a share (#52) Auto-accept stays the default, but it had two gaps the #45 follow-up exposed: the local folder name was always derived from the share label (share "Obsidian-Vault-Life" → folder "Obsidian-Vault-Life" even when the user's vault is "Life"), and a removed vault was silently re-accepted moments later while a peer still shared it — so "Remove Vault" never stuck and no manual flow was ever reachable. A new "Choose Vault…" option on every pending-share row opens a picker: link the share to an existing empty vault (nothing inside but .obsidian) or create a folder with a custom name; the share label on peers is untouched. The choice is stored per folder ID and deliberately survives removal, so remove + re-accept returns the share to the chosen location; an override that became unsafe is refused with guidance (path, reason, next step), never silently replaced by the label default, which would split one vault across two directories. removeFolder now marks the ID as user-removed and auto-accept skips marked shares (they stay visible under Pending Shares; an explicit accept lifts the mark) — doctrine 002 applied to accepts, recorded in docs/decisions/006. The #45 recovery prose now says "then accept it again under Pending Shares" accordingly. Safety: the #45 overlap guards are untouched and cover every path the picker or the override can produce — one shared overlap predicate for the label mapping and manual targets in VaultManager, plus the unchanged Go hard floor in AcceptPendingFolder (no go/ change, no xcframework rebuild needed). Only empty vaults are eligible as existing targets; linking into a non-empty folder stays refused. The user-removed set is never pruned on purpose: pruning against currently pending offers would drop the record before the offer reappears after a reconnect. New ShareTargetPickerView sheet; ManualShareTargetTests cover overlap/emptiness rules, override precedence including the refusal-message contract, sidecar round-trip, and auto-accept suppression. Strings added/updated in EN/DE/ES/zh-Hans (key counts 4×607, no duplicates); design-token lint green. Not verified on this machine: Swift cannot be compiled or tested on Linux — run the Xcode test plan (including the new suite) on macOS before merging.
📝 WalkthroughWalkthroughAdds a manual "Choose Vault…" flow for accepting pending Syncthing shares, letting users pick an existing empty vault or name a new folder instead of the auto-derived share label. Tracks user-removed folder IDs to suppress auto-re-accept, refactors overlap/collision checks, updates UI, localization, docs, and tests. ChangesManual share target and no-reaccept behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/VaultSyncTests/ManualShareTargetTests.swift (1)
211-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShared
UserDefaults.standardrisks cross-suite flakiness.This test mutates the real
.standarddefaults domain under a hardcoded key. Swift Testing parallelizes tests by default, so any other test (in this file or elsewhere) touching"vaultsync.manualShareTargets"concurrently can race with this one and produce flaky failures. Same applies touserRemovedSetPersistsbelow with"syncthing.userRemovedFolderIDs".Consider injecting a dedicated
UserDefaults(suiteName:)instance (or serializing these specific tests) so state doesn't leak between concurrent test runs or into the app's real defaults domain.🤖 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 `@ios/VaultSyncTests/ManualShareTargetTests.swift` around lines 211 - 224, The test is mutating shared UserDefaults.standard state, which can race with other parallel tests and leak across suites. Update sidecarRoundTrip in ManualShareTargetTests to use an isolated UserDefaults instance (for example via a suite name) and pass it through ManualShareTargetStore instead of relying on the global defaults domain. Also apply the same isolation approach to userRemovedSetPersists and any other tests that touch the same hardcoded keys so the state stays test-local.
🤖 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.
Nitpick comments:
In `@ios/VaultSyncTests/ManualShareTargetTests.swift`:
- Around line 211-224: The test is mutating shared UserDefaults.standard state,
which can race with other parallel tests and leak across suites. Update
sidecarRoundTrip in ManualShareTargetTests to use an isolated UserDefaults
instance (for example via a suite name) and pass it through
ManualShareTargetStore instead of relying on the global defaults domain. Also
apply the same isolation approach to userRemovedSetPersists and any other tests
that touch the same hardcoded keys so the state stays test-local.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c5db6b9-83dd-4855-bb60-ecaf5f0cf136
📒 Files selected for processing (12)
CHANGELOG.mddocs/decisions/006-no-auto-reaccept-manual-share-targets.mdios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/de.lproj/Localizable.stringsios/VaultSync/en.lproj/Localizable.stringsios/VaultSync/es.lproj/Localizable.stringsios/VaultSync/zh-Hans.lproj/Localizable.stringsios/VaultSyncTests/ManualShareTargetTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Go Tests
⚠️ CI failures not shown inline (2)
GitHub Actions: Security / Go Lint: feat(ios): let the user pick or create the target vault when accepting a share (#52)
Conclusion: failure
##[group]Run unformatted="$(gofmt -l go/bridge notify)"
�[36;1munformatted="$(gofmt -l go/bridge notify)"�[0m
�[36;1mif [ -n "$unformatted" ]; then�[0m
�[36;1m echo "::error::These files need gofmt:"�[0m
GitHub Actions: Security / 1_Go Lint.txt: feat(ios): let the user pick or create the target vault when accepting a share (#52)
Conclusion: failure
##[group]Run unformatted="$(gofmt -l go/bridge notify)"
�[36;1munformatted="$(gofmt -l go/bridge notify)"�[0m
�[36;1mif [ -n "$unformatted" ]; then�[0m
�[36;1m echo "::error::These files need gofmt:"�[0m
🧰 Additional context used
📓 Path-based instructions (9)
**/*.swift
📄 CodeRabbit inference engine (Custom checks)
For Swift background execution changes, pass if work is bounded, cancellation-aware, handles expiration callbacks, and records errors without leaking private vault data. Fail only when background work can continue unbounded, miss cleanup, or violate iOS background execution constraints.
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
ios/**/*.swift
📄 CodeRabbit inference engine (README.md)
ios/**/*.swift: Use Swift 6 and SwiftUI for iOS app development
Implement VoiceOver and Dynamic Type accessibility support throughout the app
UseBGAppRefreshTaskandBGContinuedProcessingTask(iOS 26+ when available) for background sync operations
Use APNs silent push notifications via Cloud Relay for server-to-iPhone wake-ups
Implement side-by-side diff resolution for Markdown file conflicts
Provide an activity timeline and diagnostics interface showing exactly what synced and when
Implement QR code pairing for Syncthing Device ID connection setup
Detect and list available Obsidian vaults automatically upon connection
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
⚙️ CodeRabbit configuration file
ios/**/*.swift: Focus on Swift 6 strict concurrency, Sendable/MainActor correctness, Task cancellation,
retain cycles, memory pressure, SwiftUI observation state, StoreKit/APNs flows, and iOS background execution limits.
Pay special attention to BGAppRefreshTask and BGContinuedProcessingTask behavior, expiration handling,
bounded work, and cleanup when the app is suspended or terminated.
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
ios/**/*.{swift,pbxproj}
📄 CodeRabbit inference engine (README.md)
Target iOS / iPadOS 18 or later as the minimum deployment target
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
ios/**/*.{swift,strings,stringsdict}
📄 CodeRabbit inference engine (README.md)
Support localization in English, German, Spanish, and Simplified Chinese
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/de.lproj/Localizable.stringsios/VaultSync/en.lproj/Localizable.stringsios/VaultSync/Views/ContentView.swiftios/VaultSync/es.lproj/Localizable.stringsios/VaultSync/zh-Hans.lproj/Localizable.stringsios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
**
⚙️ CodeRabbit configuration file
**:
VaultSync
Self-hosted Obsidian vault sync for iPhone and iPad.
Your notes sync peer-to-peer over Syncthing, straight into Obsidian's iOS sandbox — no note cloud, no account, no tracking.
🔭 Why VaultSync
- Peer-to-peer & private — syncs directly between your own devices over Syncthing. No note cloud, no account, no tracking.
- Lands in Obsidian — files sync into Obsidian's iOS sandbox, where the app already looks for them.
- Pair by QR, resolve conflicts — connect your server in seconds; settle Markdown conflicts with side-by-side diffs.
- Server changes wake your iPhone — optional Cloud Relay nudges the app the moment your server updates, so incoming notes land eve...
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftdocs/decisions/006-no-auto-reaccept-manual-share-targets.mdCHANGELOG.mdios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/de.lproj/Localizable.stringsios/VaultSync/en.lproj/Localizable.stringsios/VaultSync/Views/ContentView.swiftios/VaultSync/es.lproj/Localizable.stringsios/VaultSync/zh-Hans.lproj/Localizable.stringsios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
**/*
⚙️ CodeRabbit configuration file
**/*: VaultSync syncs private Obsidian notes through Syncthing. Treat data loss,
privacy leaks, security regressions, and broken sync behavior as high priority.
Do not nitpick formatting unless it affects maintainability, correctness, or public API clarity.
Flag any accidental logging, telemetry, crash reporting, or network transfer of note contents,
vault paths, filenames with private context, API keys, APNs tokens, relay keys, or security-scoped bookmark data.
Files:
ios/VaultSync/Views/ShareTargetPickerView.swiftdocs/decisions/006-no-auto-reaccept-manual-share-targets.mdCHANGELOG.mdios/VaultSync/Views/PendingSharesView.swiftios/VaultSyncTests/ManualShareTargetTests.swiftios/VaultSync/de.lproj/Localizable.stringsios/VaultSync/en.lproj/Localizable.stringsios/VaultSync/Views/ContentView.swiftios/VaultSync/es.lproj/Localizable.stringsios/VaultSync/zh-Hans.lproj/Localizable.stringsios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
docs/**
⚙️ CodeRabbit configuration file
docs/**: # ArchitectureVaultSync embeds Syncthing's Go reference implementation as an iOS library via gomobile — no reimplementation of the protocol in Swift, and guaranteed wire compatibility.
┌─────────────────────────────────┐ │ SwiftUI Frontend │ iOS-native UI, Swift 6 ├─────────────────────────────────┤ │ Swift ↔ Go Bridge │ thin API via gomobile │ │ → exported as .xcframework ├─────────────────────────────────┤ │ syncthing/lib (Go) │ protocol, discovery, sync └─────────────────────────────────┘ ↕ filesystem ┌─────────────────────────────────┐ │ Obsidian Vault (direct) │ Obsidian's iOS sandbox └─────────────────────────────────┘🔄 Sync strategy
- Foreground — Syncthing runs unrestricted: immediate, continuous sync.
- Background —
BGAppRefreshTask(requested ~15 min out; iOS decides the actual timing) +BGProcessingTask(overnight catch-up: multi-minute budget while charging with network) +BGContinuedProcessingTask(iOS 26+, longer runtime for user-initiated tasks). A ~30s grace window after backgrounding lets in-flight work finish.- Push (Cloud Relay) — optional. Near-realtime
server → iPhonewake-ups via APNs silent push. See relay-spec.md.VaultSync is intentionally asymmetric:
Direction Path Server → iPhone vaultsync-notifyspots outgoing changes → Cloud Relay silent push → VaultSync wakes and pulls.iPhone → Server iOS doesn't guarantee timely background execution for local edits. The reliable path is to open VaultSync and let embedded Syncthing run in the foreground — a Shortcuts automation can do that automatically whenever you leave Obsidian. Cloud Relay is a
server → iPhoneacceleration path, not a guarantee of symmetric real-time background sync.Connection paths & iOS network privacy
How peers are reached, fastest f...
Files:
docs/decisions/006-no-auto-reaccept-manual-share-targets.md
docs/decisions/**
⚙️ CodeRabbit configuration file
docs/decisions/**: # 001 — Two Syncthing folders never overlap on disk (three enforcement layers)Context: A server sharing more than one vault could hand the second share the same local folder as the first (1.6.0–1.7.0) or a subfolder inside an existing vault (1.7.1). Overlapping folders sync each other's content as their own — deleting the stray copy on any peer would have deleted the inner vault everywhere (
#45).Decision: The no-overlap invariant (equal, nested, or containing paths) is enforced in three independent layers, each with its own tests: the Go hard floor (
AcceptPendingFolder/AddFolderreject withfolderPathOverlapError), the Swift mapping (VaultManager.resolveSharePathreturnsnilrather than an overlapping path), and the launch shield (PathCollisionGuardpauses already-overlapping folders exactly once).Why: The single-layer version failed twice — both
#45bugs lived in the Swift mapping. The Go floor backstops future mapping bugs, the Swift layer turns a hard engine error into user guidance, and only the shield catches damage that predates the fix.Rejected alternative: Enforcing only in the Swift mapping — the proven failure mode; a bug there would silently re-open the hole with no backstop.
Links:
#45, PR#47(same-folder merge, 1.7.1), PR#51(nesting, 1.7.2).
docs/decisions/**: # 002 — Recovery from data damage is never automaticContext: When VaultSync detects existing damage (two vaults merged into one folder, or one nested inside another,
#45), an automatic repair would have to move, rename, delete, or re-accept user data whose true state only the user knows — after a merge, the app cannot tell which files belong to which vault.Decision: Recovery is always: pause the affected folders (exactly once — a folder the user deliberately resumes is never re-paused), explain the problem in a critical issue with concrete recovery steps, and let the user act.
Why: Sync propagates every local act...
Files:
docs/decisions/006-no-auto-reaccept-manual-share-targets.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Review public documentation for technical accuracy, privacy/security claims, App Store-facing wording,
setup correctness, and consistency with the free app plus optional Cloud Relay subscription model.
Files:
docs/decisions/006-no-auto-reaccept-manual-share-targets.mdCHANGELOG.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: psimaker/vaultsync
Timestamp: 2026-07-07T11:01:01.629Z
Learning: Removed vaults are never auto-re-accepted: when `removeFolder` records a folder ID as user-removed, auto-accept must skip that share until the user explicitly accepts it again.
Learnt from: CR
Repo: psimaker/vaultsync
Timestamp: 2026-07-07T11:01:01.629Z
Learning: Manually picked share targets must persist per folder ID across removals and later accepts; if the saved override becomes unsafe, the accept flow must refuse it with guidance and must not silently fall back to the share-label default.
Learnt from: CR
Repo: psimaker/vaultsync
Timestamp: 2026-07-07T11:01:01.629Z
Learning: Only empty folders containing nothing but `.obsidian` are eligible as existing targets for re-adding a share.
📚 Learning: 2026-06-10T18:47:10.724Z
Learnt from: psimaker
Repo: psimaker/vaultsync PR: 38
File: ios/VaultSync/Views/ContentView.swift:605-611
Timestamp: 2026-06-10T18:47:10.724Z
Learning: In the SwiftUI codebase under ios/VaultSync, do not flag missing localization for SwiftUI string literals used as Text("…") or DisclosureGroup("…") titles/labels. In SwiftUI, these string literals are treated as LocalizedStringKey and resolve via the app’s Localizable.strings automatically—so they only need attention if the corresponding key is actually missing. Only require an explicit localization helper (e.g., L10n.tr(…)) when the string is not being passed through SwiftUI’s LocalizedStringKey path (e.g., plain String values provided to non-SwiftUI APIs).
Applied to files:
ios/VaultSync/Views/ShareTargetPickerView.swiftios/VaultSync/Views/PendingSharesView.swiftios/VaultSync/Views/ContentView.swiftios/VaultSync/Services/SyncthingManager.swiftios/VaultSync/Services/VaultManager.swift
🔇 Additional comments (19)
ios/VaultSyncTests/ManualShareTargetTests.swift (3)
1-207: LGTM!
228-242: LGTM!
244-252: 🎯 Functional CorrectnessNo extra teardown needed here.
SyncthingManager.init()only restores persisted state and registers a notification observer; it doesn't start Syncthing or schedule background work.> Likely an incorrect or invalid review comment.ios/VaultSync/Services/SyncthingManager.swift (2)
87-92: LGTM!Also applies to: 116-116, 163-163, 320-336, 765-775, 862-867, 1832-1842
473-473: LGTM!Also applies to: 498-498
ios/VaultSync/Services/VaultManager.swift (2)
216-251: LGTM!Also applies to: 310-311, 322-322, 345-373, 460-467, 474-528, 530-572, 574-580, 643-682
375-417: 🩺 Stability & AvailabilityNo actor-isolation issue here:
L10nis a plainenumwith static helpers, so thenonisolated staticmethods can callL10n.trandL10n.fmtwithout a Swift 6 concurrency conflict.> Likely an incorrect or invalid review comment.CHANGELOG.md (1)
7-16: LGTM!docs/decisions/006-no-auto-reaccept-manual-share-targets.md (2)
1-6: LGTM!Also applies to: 8-11
7-7: 📐 Maintainability & Code QualityKeep the decision link as-is.
> Likely an incorrect or invalid review comment.ios/VaultSync/de.lproj/Localizable.strings (1)
32-34: LGTM!Also applies to: 68-68, 79-88, 128-128, 139-140, 165-165, 247-249, 638-643
ios/VaultSync/en.lproj/Localizable.strings (1)
32-34: LGTM!Also applies to: 68-68, 79-88, 128-128, 139-140, 165-165, 247-249, 638-643
ios/VaultSync/es.lproj/Localizable.strings (1)
32-34: LGTM!Also applies to: 68-68, 79-88, 128-128, 139-140, 165-165, 247-249, 638-643
ios/VaultSync/zh-Hans.lproj/Localizable.strings (1)
32-34: LGTM!Also applies to: 68-68, 79-88, 128-128, 139-140, 165-165, 247-249, 638-643
ios/VaultSync/Views/PendingSharesView.swift (2)
146-155: This button is the entry point for the auto-accept race flagged inContentView.swift(autoAcceptPendingShares) — implementation here is otherwise correct; no change needed in this file.
13-13: LGTM!Also applies to: 51-66
ios/VaultSync/Views/ContentView.swift (2)
19-19: LGTM!
139-148: 🩺 Stability & AvailabilityExclude the open share target from auto-accept. While the picker is showing, keep that folder out of
autoAcceptEligiblePendingFoldersor mark it in-flight before presenting the sheet; otherwise a refresh can auto-accept the same pending share into the default path before the user confirms.ios/VaultSync/Views/ShareTargetPickerView.swift (1)
1-125: LGTM!
1 participant
Closes #52. Adds a per-share "Choose Vault…" picker (existing empty vault or custom-named new folder; share label on peers untouched), persists the choice per folder ID so remove + re-accept returns to the chosen location (unsafe override → refusal with guidance, never a silent fallback), and stops auto-re-accepting shares whose vault the user removed (doctrine 002, decision 006) — which also makes Remove Vault actually stick while a peer keeps sharing. #45 overlap guards untouched (shared Swift predicate + Go hard floor); no go/ change, no xcframework rebuild. 18 new tests in ManualShareTargetTests; strings in EN/DE/ES/zh-Hans. Note for review: Swift suite not run locally (Linux) — please rely on the macOS Build & Test job / run the test plan before merge.
This PR adds a manual “Choose Vault…” flow for accepting incoming shares on iOS. Users can now link a share to an existing empty vault or create a new local folder with a custom name, while the Syncthing share label stays unchanged.
It also makes manual target selection persistent per folder ID, so removing and re-accepting a share returns it to the same location. If a saved target becomes unsafe, the app now refuses to auto-fallback and shows guidance instead. Removed vaults also stay removed instead of being silently re-added by background auto-accept.
Updated coverage includes new tests for manual target validation, path resolution, persistence, and user-removed share suppression, plus localized strings and documentation updates.