feat: add reusable SSH tunnel profiles#385
Merged
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:
📝 WalkthroughWalkthroughAdds reusable SSH tunnel profiles: new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (ConnectionFormView)
participant CStor as ConnectionStorage
participant SStor as SSHProfileStorage
participant DBMgr as DatabaseManager
participant Keychain as KeychainHelper
participant Tunnel as SSH Tunnel
User->>CStor: Save connection (includes sshProfileId?)
CStor-->>User: Persisted
User->>DBMgr: Test/Build connection
DBMgr->>CStor: Load connection
CStor-->>DBMgr: connection (+ sshProfileId)
alt sshProfileId present
DBMgr->>SStor: profile(for: sshProfileId)
SStor-->>DBMgr: SSHProfile?
DBMgr->>Keychain: load secrets (owner = profileId)
else inline or no profile
DBMgr->>Keychain: load secrets (owner = connectionId)
end
Keychain-->>DBMgr: secrets
DBMgr->>DBMgr: resolve effective SSH config
DBMgr->>Tunnel: create tunnel with resolved config
Tunnel-->>DBMgr: tunnel established
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/Views/Connection/ConnectionFormView.swift (1)
1278-1289:⚠️ Potential issue | 🟡 MinorTest connection saves SSH secrets even when using a profile.
When
sshProfileIdis set, the test connection flow still saves SSH credentials to the connection-level keychain keys (lines 1278-1289). This is inconsistent with the save logic (lines 1111-1126) which skips saving secrets when a profile is used.For test connections with a profile, the credentials should come from the profile storage, not be temporarily saved to connection storage.
🐛 Suggested fix
if !password.isEmpty { ConnectionStorage.shared.savePassword(password, for: testConn.id) } - if sshEnabled - && (sshAuthMethod == .password || sshAuthMethod == .keyboardInteractive) - && !sshPassword.isEmpty - { - ConnectionStorage.shared.saveSSHPassword(sshPassword, for: testConn.id) - } - if sshEnabled && sshAuthMethod == .privateKey && !keyPassphrase.isEmpty { - ConnectionStorage.shared.saveKeyPassphrase(keyPassphrase, for: testConn.id) - } - if sshEnabled && totpMode == .autoGenerate && !totpSecret.isEmpty { - ConnectionStorage.shared.saveTOTPSecret(totpSecret, for: testConn.id) + // Only save SSH secrets for test when using inline config (not a profile) + if sshEnabled && sshProfileId == nil { + if (sshAuthMethod == .password || sshAuthMethod == .keyboardInteractive) + && !sshPassword.isEmpty + { + ConnectionStorage.shared.saveSSHPassword(sshPassword, for: testConn.id) + } + if sshAuthMethod == .privateKey && !keyPassphrase.isEmpty { + ConnectionStorage.shared.saveKeyPassphrase(keyPassphrase, for: testConn.id) + } + if totpMode == .autoGenerate && !totpSecret.isEmpty { + ConnectionStorage.shared.saveTOTPSecret(totpSecret, for: testConn.id) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Connection/ConnectionFormView.swift` around lines 1278 - 1289, The test connection flow is incorrectly saving SSH secrets to connection storage even when an SSH profile is used (sshProfileId is set); update the block in the test connection path that calls ConnectionStorage.shared.saveSSHPassword, saveKeyPassphrase, and saveTOTPSecret to first check that sshProfileId is nil/empty (i.e., only save when no profile is selected), mirroring the save logic used in the main save flow (so use sshProfileId guard around the save calls for testConn.id).
🧹 Nitpick comments (3)
TablePro/Views/Connection/ConnectionFormView.swift (1)
476-485: Consider caching the profiles list.
SSHProfileStorage.shared.loadProfiles()is called directly in theForEachwithin the view body. This could cause repeated decoding if the view re-renders frequently. Consider loading profiles into a@Stateproperty on appear or using a computed property with caching.♻️ Suggested approach
+ `@State` private var availableSSHProfiles: [SSHProfile] = [] + // In onAppear or .task: + availableSSHProfiles = SSHProfileStorage.shared.loadProfiles() // In sshProfileSection: Picker(String(localized: "Profile"), selection: $sshProfileId) { Text("Inline Configuration").tag(UUID?.none) - ForEach(SSHProfileStorage.shared.loadProfiles()) { profile in + ForEach(availableSSHProfiles) { profile in Text("\(profile.name) (\(profile.username)@\(profile.host))").tag(UUID?.some(profile.id)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Connection/ConnectionFormView.swift` around lines 476 - 485, The view currently calls SSHProfileStorage.shared.loadProfiles() directly inside the ForEach in sshProfileSection which can re-decode profiles on every render; change this by adding a `@State` (e.g., var sshProfiles: [SSHProfile]) to hold the loaded profiles, populate it once in onAppear (or update it when profiles change) and replace the ForEach(SSHProfileStorage.shared.loadProfiles()) with ForEach(sshProfiles) so sshProfileSection, Picker and ForEach use the cached sshProfiles list instead of calling loadProfiles() repeatedly.TablePro/Core/Storage/SSHProfileStorage.swift (1)
9-18: DeclareSSHProfileStorage’s visibility explicitly.Please make the type’s access level explicit here instead of relying on the default, e.g.
internal final class SSHProfileStorage.As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types. Specify on the extension itself, not on individual members."♻️ Minimal fix
-final class SSHProfileStorage { +internal final class SSHProfileStorage {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Storage/SSHProfileStorage.swift` around lines 9 - 18, The SSHProfileStorage type currently relies on the default access level; explicitly declare its visibility (e.g., change the declaration of SSHProfileStorage to include an access modifier like internal or public) so the class has a clear access control, updating the top-level declaration for final class SSHProfileStorage accordingly.TablePro/Models/Connection/SSHProfile.swift (1)
8-8: DeclareSSHProfile’s visibility explicitly.Please spell out the intended access level here instead of relying on the default, e.g.
internal struct SSHProfile.As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types. Specify on the extension itself, not on individual members."♻️ Minimal fix
-struct SSHProfile: Identifiable, Hashable, Codable, Sendable { +internal struct SSHProfile: Identifiable, Hashable, Codable, Sendable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Models/Connection/SSHProfile.swift` at line 8, The type SSHProfile currently relies on default access control; explicitly declare its intended visibility (for example change "struct SSHProfile" to "internal struct SSHProfile" or "public struct SSHProfile" as appropriate) so the SSHProfile declaration and its conformance (Identifiable, Hashable, Codable, Sendable) have an explicit access level per project guidelines; update the SSHProfile definition to include the correct access modifier and run a quick build to ensure no downstream visibility errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro.xcodeproj/project.pbxproj`:
- Around line 1204-1221: The TablePro app target is missing a dependency on the
new DynamoDBDriverPlugin target and a Copy Plug-Ins build phase to embed
DynamoDBDriverPlugin.tableplugin into the app; update the TablePro
PBXNativeTarget to add DynamoDBDriverPlugin (target name DynamoDBDriverPlugin /
productReference 5AE460EC2F6CEDB70097AC5B) to its dependencies and add or update
a "Copy Plug-Ins" build phase that lists DynamoDBDriverPlugin.tableplugin
(productName DynamoDBDriverPlugin) so the .tableplugin bundle is copied into the
app bundle at build time.
- Around line 658-663: The project links TableProPluginKit.framework in the
PBXFrameworksBuildPhase but is missing the runtime search path used by other
plugin targets; update the build settings for the native target that links
TableProPluginKit.framework (the PBXNativeTarget for
DynamoDBDriverPlugin.tableplugin or the corresponding build configuration block)
to include LD_RUNPATH_SEARCH_PATHS = "$(inherited)
`@executable_path/`../Frameworks" (or add `@executable_path/`../Frameworks to the
existing LD_RUNPATH_SEARCH_PATHS array) so dyld can resolve
TableProPluginKit.framework at runtime.
In `@TablePro/Core/Storage/SSHProfileStorage.swift`:
- Around line 22-32: The decode failure should not be treated as an empty store:
make loadProfiles surface the error instead of returning [] (e.g., change func
loadProfiles() to throw or return Result<[SSHProfile], Error>) and remove the
silent catch that returns an empty array; then update all mutating methods
(addProfile / updateProfile / removeProfile or whichever methods at 44-66 call
loadProfiles) to check for a load error and block any writes (propagate the
error or return a failure) until the stored payload is recovered/migrated; keep
the existing error logging (Self.logger.error) but also ensure callers receive
the error so the UI/consumer can show recovery options.
---
Outside diff comments:
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 1278-1289: The test connection flow is incorrectly saving SSH
secrets to connection storage even when an SSH profile is used (sshProfileId is
set); update the block in the test connection path that calls
ConnectionStorage.shared.saveSSHPassword, saveKeyPassphrase, and saveTOTPSecret
to first check that sshProfileId is nil/empty (i.e., only save when no profile
is selected), mirroring the save logic used in the main save flow (so use
sshProfileId guard around the save calls for testConn.id).
---
Nitpick comments:
In `@TablePro/Core/Storage/SSHProfileStorage.swift`:
- Around line 9-18: The SSHProfileStorage type currently relies on the default
access level; explicitly declare its visibility (e.g., change the declaration of
SSHProfileStorage to include an access modifier like internal or public) so the
class has a clear access control, updating the top-level declaration for final
class SSHProfileStorage accordingly.
In `@TablePro/Models/Connection/SSHProfile.swift`:
- Line 8: The type SSHProfile currently relies on default access control;
explicitly declare its intended visibility (for example change "struct
SSHProfile" to "internal struct SSHProfile" or "public struct SSHProfile" as
appropriate) so the SSHProfile declaration and its conformance (Identifiable,
Hashable, Codable, Sendable) have an explicit access level per project
guidelines; update the SSHProfile definition to include the correct access
modifier and run a quick build to ensure no downstream visibility errors.
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 476-485: The view currently calls
SSHProfileStorage.shared.loadProfiles() directly inside the ForEach in
sshProfileSection which can re-decode profiles on every render; change this by
adding a `@State` (e.g., var sshProfiles: [SSHProfile]) to hold the loaded
profiles, populate it once in onAppear (or update it when profiles change) and
replace the ForEach(SSHProfileStorage.shared.loadProfiles()) with
ForEach(sshProfiles) so sshProfileSection, Picker and ForEach use the cached
sshProfiles list instead of calling loadProfiles() repeatedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c76171d8-3915-49ea-bae3-1455a62d6a1f
📒 Files selected for processing (8)
CHANGELOG.mdTablePro.xcodeproj/project.pbxprojTablePro/Core/Database/DatabaseManager.swiftTablePro/Core/Storage/ConnectionStorage.swiftTablePro/Core/Storage/SSHProfileStorage.swiftTablePro/Models/Connection/DatabaseConnection.swiftTablePro/Models/Connection/SSHProfile.swiftTablePro/Views/Connection/ConnectionFormView.swift
…ist, fix indentation
- Fix testConnection leaking Keychain secrets and overriding profile passwords - Clean up all temporary Keychain entries (password, passphrase, TOTP) after test - Skip inline SSH validation when a profile is selected (isValid) - Move profile list loading from section onAppear to loadConnectionData - Separate onSave/onDelete callbacks in SSHProfileEditorView - Pass inline secrets to Save as Profile flow so TOTP secret is preserved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
TablePro/Views/Connection/SSHProfileEditorView.swift (1)
8-8: DeclareSSHProfileEditorViewwith an explicit access level.The new top-level type currently relies on Swift’s default
internalvisibility.As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types. Specify on the extension itself, not on individual members."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Connection/SSHProfileEditorView.swift` at line 8, Declare the top-level struct with an explicit access level by updating the SSHProfileEditorView declaration to include an access modifier (e.g., `internal struct SSHProfileEditorView` or `public struct SSHProfileEditorView` as appropriate for its usage); set the modifier on the type itself (not its members) so the view’s visibility conforms to the coding guideline requiring explicit access control for top-level types like SSHProfileEditorView.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 480-570: Extract the entire SSH profile UI and helpers into a
dedicated subview/extension: move sshProfileSection, reloadProfiles(),
buildProfileFromInlineConfig(), and sshProfileSummarySection(_:) into a new
SwiftUI View (e.g., SSHProfilesSection) in its own file and replace the inline
code in ConnectionFormView with that subview. Expose and pass required `@Binding`
and values (sshProfileId, sshProfiles, showingCreateProfile, editingProfile,
showingSaveAsProfile, sshEnabled, sshHost, sshPort, sshUsername, sshAuthMethod,
sshPrivateKeyPath, selectedSSHConfigHost, resolvedSSHAgentSocketPath, jumpHosts,
totpMode, totpAlgorithm, totpDigits, totpPeriod) and a reloadProfiles callback
so the new view can call SSHProfileStorage.shared.loadProfiles() and update
selection logic exactly as in reloadProfiles(); keep Sheet presentations and
buildProfileFromInlineConfig behavior inside the new view so ConnectionFormView
only instantiates SSHProfilesSection.
- Around line 1047-1051: Don't re-enable SSH when a profile exists: stop
unconditionally setting sshEnabled = true based on existing.sshProfileId and
SSHProfileStorage.shared.profile(for:). Instead respect the persisted flag by
initializing/keeping sshEnabled from existing.sshConfig.enabled (e.g. only set
sshEnabled = true if existing.sshConfig.enabled is true and the profile exists),
or alternatively ensure that when the SSH toggle is turned off elsewhere you
clear existing.sshProfileId; update the code around existing.sshProfileId,
SSHProfileStorage.shared.profile(for:), and sshEnabled to implement one of these
behaviors so reopening the form does not flip the user's persisted off-state
back on.
- Around line 455-472: The form becomes invalid when an SSH profile is selected
because isValid still checks the inline draft fields (sshHost, sshUsername,
auth, jumpHosts) even when sshProfileId is non-nil; update the validation logic
(isValid) to short-circuit or validate against the selected profile when
SSHProfileStorage.shared.profile(for: sshProfileId) returns a profile (used by
sshProfileSection and sshProfileSummarySection), i.e., if sshProfileId != nil
treat SSH as valid based on the profile (or validate against that profile's
values) instead of the hidden sshInlineFields, so Save/Test enablement reflects
the selected profile state.
- Around line 1181-1195: The current code only calls
storage.deleteTOTPSecret(for:) inside the inline-SSH branch, so when a user
switches an existing connection from inline SSH to a profile (sshProfileId !=
nil) or disables SSH the old per-connection TOTP secret is left in Keychain;
update the logic in ConnectionFormView (the block that handles SSH inline saving
for connectionToSave.id, using sshEnabled, sshProfileId, totpMode, totpSecret,
and storage.saveTOTPSecret/saveKeyPassphrase/saveSSHPassword) so that
storage.deleteTOTPSecret(for: connectionToSave.id) is also invoked whenever the
connection is no longer using inline SSH (i.e. when !(sshEnabled && sshProfileId
== nil)) or when totpMode != .autoGenerate or totpSecret.isEmpty, rather than
only in the inline-config branch.
In `@TablePro/Views/Connection/SSHProfileEditorView.swift`:
- Around line 48-49: existingProfile is being used both to prefill the editor
and to indicate "editing an existing stored profile", causing saveProfile() to
call updateProfile(_:) for synthesized drafts and showing edit/delete UI for new
drafts; change the logic to distinguish the two by introducing a computed flag
like editingStoredProfile (e.g., let editingStoredProfile: Bool =
existingProfile != nil && SSHProfileStorage.shared.contains(id:
existingProfile!.id)) and use that: (1) replace the current isEditing computed
property with editingStoredProfile, (2) in saveProfile() call updateProfile(_:)
only when editingStoredProfile is true otherwise call createProfile, and (3)
gate edit/delete affordances in the view on editingStoredProfile; update
SSHProfileStorage usage (contains or fetchById) to reliably detect stored
profiles before treating existingProfile as editable.
- Around line 50-53: The current isValid computed property only checks
profileName and host; update isValid to mirror ConnectionFormView validation by
also verifying port is numeric and within 1–65535, ensuring when authMethod ==
.privateKey the privateKeyPath is non-empty (and optionally exists), and
validating any jumpHosts entries (host non-empty and their ports
numeric/in-range) using the same rules as ConnectionFormView (reuse its
validation logic or helper functions) so the Save action cannot persist profiles
with invalid port, missing key path, or malformed jump hosts; reference the
isValid property, profileName, host, port, authMethod, privateKeyPath and
jumpHosts/JumpHost validation helpers when making the change.
---
Nitpick comments:
In `@TablePro/Views/Connection/SSHProfileEditorView.swift`:
- Line 8: Declare the top-level struct with an explicit access level by updating
the SSHProfileEditorView declaration to include an access modifier (e.g.,
`internal struct SSHProfileEditorView` or `public struct SSHProfileEditorView`
as appropriate for its usage); set the modifier on the type itself (not its
members) so the view’s visibility conforms to the coding guideline requiring
explicit access control for top-level types like SSHProfileEditorView.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48a2a1f2-bc05-4bc4-991f-f046e2fbb462
📒 Files selected for processing (2)
TablePro/Views/Connection/ConnectionFormView.swiftTablePro/Views/Connection/SSHProfileEditorView.swift
- Add SyncRecordType.sshProfile with CKRecord mapping - Add syncSSHProfiles toggle to SyncSettings - Add sync tracking (markDirty/markDeleted) in SSHProfileStorage - Add saveProfilesWithoutSync for applying remote changes - Update SyncCoordinator: push, pull, delete, conflict handling - Handle .sshProfile in ConflictResolutionView - Add SSH Profiles feature docs (EN/VI/ZH) - Update SSH Tunneling docs with link to profiles - Update docs.json navigation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #381
Summary
SSHProfilemodel with all SSH configuration fields (host, port, auth method, jump hosts, TOTP)SSHProfileStoragefor profile persistence (UserDefaults) and secrets (Keychain)sshProfileId: UUID?toDatabaseConnectionfor profile referenceDatabaseManager.buildEffectiveConnectionto resolve SSH config from profile or inlineArchitecture
Follows the existing
ConnectionGroup/ConnectionTagpattern:SSHProfile(Identifiable, Codable, Sendable) withtoSSHConfiguration()conversionSSHProfileStoragesingleton with CRUD + 3 Keychain secret categoriesbuildEffectiveConnection— plugins remain unawareTest plan
Summary by CodeRabbit