feat: make ConnectionFormView fully dynamic via plugin metadata#309
Merged
feat: make ConnectionFormView fully dynamic via plugin metadata#309
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConnection UI and plugin metadata were made field-driven: Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Form as ConnectionFormView
participant PM as PluginManager
participant Plugin as DriverPlugin
participant DB as DatabaseDriver
User->>Form: Open/select database type
Form->>PM: supportsSSH(for: type)
PM->>Plugin: resolve plugin & query supportsSSH
Plugin-->>PM: Bool
PM-->>Form: capability
Form->>PM: supportsSSL(for: type)
PM->>Plugin: resolve plugin & query supportsSSL
Plugin-->>PM: Bool
PM-->>Form: capability
Form->>PM: additionalConnectionFields(for: type)
PM->>Plugin: fetch field definitions (section, hidesPassword)
Plugin-->>PM: fields
PM-->>Form: fields
User->>Form: toggle auth fields (e.g., usePgpass)
Form->>Form: update additionalFieldValues
Form->>DB: request resolved password (reads additionalFieldValues["usePgpass"])
DB->>DB: if usePgpass == "true" then return empty password
DB-->>Form: resolved password
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
TablePro/Views/Connection/ConnectionFormView.swift (3)
661-675:⚠️ Potential issue | 🟠 MajorFilter the Advanced tab to
.advancedfields.
additionalConnectionFieldsnow includes authentication-section entries too, so the PostgreSQLusePgpasstoggle renders twice: once under Authentication and again here. Split the list bysectionbefore building the Advanced tab.🧭 Suggested change
- if !additionalConnectionFields.isEmpty { + let advancedFields = additionalConnectionFields.filter { $0.section == .advanced } + if !advancedFields.isEmpty { Section(type.displayName) { - ForEach(additionalConnectionFields, id: \.id) { field in + ForEach(advancedFields, id: \.id) { field in ConnectionFieldRow(🤖 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 661 - 675, The Advanced section is using additionalConnectionFields which currently contains authentication entries too, causing duplicates; update the Section(type.displayName) block to only iterate over additionalConnectionFields filtered by their section (e.g., field.section == .advanced) before creating ConnectionFieldRow, so only fields belonging to the .advanced section are shown; locate the Section/ForEach that references additionalConnectionFields and apply the filter there (using the field.section enum value) so the PostgreSQL usePgpass toggle is not rendered twice.
172-175:⚠️ Potential issue | 🟡 MinorCapability-based tabs need a capability-based fallback.
This still only falls back for file-based drivers. If
selectedTabis.sshor.ssland the user switches to a driver that disables just that capability,selectedTabstill points at the hidden tab andtabFormwill keep rendering unsupported content.🔧 Minimal fix
- let isFileBased = PluginManager.shared.connectionMode(for: newType) == .fileBased - if isFileBased && (selectedTab == .ssh || selectedTab == .ssl) { + if !visibleTabs.contains(selectedTab) { selectedTab = .general }🤖 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 172 - 175, The selectedTab fallback only checks for file-based drivers; change it to validate whether the newly selected driver actually supports the currently selected tab's capability (e.g., if selectedTab is .ssh verify SSH is supported, if .ssl verify TLS/SSL is supported) by querying PluginManager.shared.connectionMode(for: newType) or the driver's capability flags, and if the capability is not supported set selectedTab = .general; update the logic around selectedTab (the block that currently only checks isFileBased) so any unsupported-capability tab is redirected to .general before tabForm renders.
168-182:⚠️ Potential issue | 🟠 MajorThis
onChange(of: type)is clobbering restored/imported state.
loadConnectionData()andparseConnectionURL()both assigntypeprogrammatically. On edit load, this handler rebuildsadditionalFieldValuesand can immediately drop restored plugin fields likeusePgpass,redisDatabase, ormongoAuthSource; once the form is already loaded, URL import can also reset a parsed custom port back to the driver default. These resets need to stay on the picker-driven path only, or be suppressed while loading/importing.🤖 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 168 - 182, The onChange(of: type) handler is running for programmatic assignments (from loadConnectionData()/parseConnectionURL()) and clobbering restored fields; introduce a boolean flag (e.g., isApplyingConnectionData) that loadConnectionData() and parseConnectionURL() set true before they assign type and set false after, then in the onChange(of: type) early-return (or skip the port/defaults/selectedTab/additionalFieldValues mutations) when isApplyingConnectionData is true so only picker-driven changes perform those resets; keep existing hasLoadedData logic for initial-load behavior but suppress the default-port and additionalFieldValues rebuild while isApplyingConnectionData is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/TableProPluginKit/DriverPlugin.swift`:
- Around line 49-50: The DriverPlugin protocol gained new requirements (static
var supportsSSH and static var supportsSSL) which breaks binary compatibility
for runtime-loaded plugins, so bump the plugin-kit compatibility version used by
the runtime check: locate the currentPluginKitVersion constant referenced by
PluginManager’s plugin validation and increment it from 1 (or 0) to 2; keep the
new default implementations on DriverPlugin but ensure PluginManager’s version
check rejects older/unversioned bundles by using the new value so plugins
compiled against the prior protocol shape are not loaded.
---
Outside diff comments:
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 661-675: The Advanced section is using additionalConnectionFields
which currently contains authentication entries too, causing duplicates; update
the Section(type.displayName) block to only iterate over
additionalConnectionFields filtered by their section (e.g., field.section ==
.advanced) before creating ConnectionFieldRow, so only fields belonging to the
.advanced section are shown; locate the Section/ForEach that references
additionalConnectionFields and apply the filter there (using the field.section
enum value) so the PostgreSQL usePgpass toggle is not rendered twice.
- Around line 172-175: The selectedTab fallback only checks for file-based
drivers; change it to validate whether the newly selected driver actually
supports the currently selected tab's capability (e.g., if selectedTab is .ssh
verify SSH is supported, if .ssl verify TLS/SSL is supported) by querying
PluginManager.shared.connectionMode(for: newType) or the driver's capability
flags, and if the capability is not supported set selectedTab = .general; update
the logic around selectedTab (the block that currently only checks isFileBased)
so any unsupported-capability tab is redirected to .general before tabForm
renders.
- Around line 168-182: The onChange(of: type) handler is running for
programmatic assignments (from loadConnectionData()/parseConnectionURL()) and
clobbering restored fields; introduce a boolean flag (e.g.,
isApplyingConnectionData) that loadConnectionData() and parseConnectionURL() set
true before they assign type and set false after, then in the onChange(of: type)
early-return (or skip the port/defaults/selectedTab/additionalFieldValues
mutations) when isApplyingConnectionData is true so only picker-driven changes
perform those resets; keep existing hasLoadedData logic for initial-load
behavior but suppress the default-port and additionalFieldValues rebuild while
isApplyingConnectionData is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69ce6856-4721-4ff1-94a9-5983122ed5fd
📒 Files selected for processing (8)
CHANGELOG.mdPlugins/PostgreSQLDriverPlugin/PostgreSQLPlugin.swiftPlugins/SQLiteDriverPlugin/SQLitePlugin.swiftPlugins/TableProPluginKit/ConnectionField.swiftPlugins/TableProPluginKit/DriverPlugin.swiftTablePro/Core/Database/DatabaseDriver.swiftTablePro/Core/Plugins/PluginManager.swiftTablePro/Views/Connection/ConnectionFormView.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
TablePro/Views/Connection/ConnectionFormView.swift (1)
853-854: Remove duplicated/restating migration comment.Lines 853-854 repeat the same intent and restate obvious code behavior. Keep one concise rationale-only comment.
As per coding guidelines: "Do not add comments that restate what the code says; only comment to explain non-obvious 'why' reasoning or clarify genuinely complex logic".
🤖 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 853 - 854, Remove the duplicated migration comment around the conditional that checks additionalFieldValues["redisDatabase"] (the block that migrates legacy redisDatabase to additionalFields); keep a single concise rationale-only comment explaining why the migration is needed and delete the repeated/restating comment so comments do not restate the code.
🤖 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`:
- Line 935: The redisDatabase parsing can yield nil for non-numeric strings;
change occurrences where you set redisDatabase (in the ConnectionFormView
initializer/mapping and in testConnection()) from redisDatabase:
Int(additionalFieldValues["redisDatabase"] ?? "0") to redisDatabase:
(Int(additionalFieldValues["redisDatabase"] ?? "0") ?? 0) so invalid values fall
back to 0 instead of nil; update both the mapping at the shown diff and the
testConnection() usage.
- Around line 297-301: authSectionFields are being rendered in the
Authentication section and then again because additionalConnectionFields is
rendered unchanged in the Advanced section; update the Advanced rendering to
exclude fields already shown in Authentication by filtering
additionalConnectionFields before rendering (e.g.,
additionalConnectionFields.filter { field in !authSectionFields.contains(where:
{ $0.id == field.id }) } ), so fields like usePgpass are not duplicated; ensure
bindings (toggleBinding(for:)) still resolve for the remaining fields.
- Around line 189-197: After filtering tabs by
PluginManager.shared.supportsSSH(for:) and supportsSSL(for:) you must normalize
the view's selectedTab so it can't point to a now-hidden tab; after building the
tabs array check whether the current selectedTab (property selectedTab in
ConnectionFormView) exists in the resulting tabs (FormTab values like .ssh,
.ssl, .general, .advanced) and if not set selectedTab to a safe default (e.g.
.general or tabs.first!). Implement this normalization immediately before
returning tabs in the same getter/function that constructs the tabs array.
---
Nitpick comments:
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 853-854: Remove the duplicated migration comment around the
conditional that checks additionalFieldValues["redisDatabase"] (the block that
migrates legacy redisDatabase to additionalFields); keep a single concise
rationale-only comment explaining why the migration is needed and delete the
repeated/restating comment so comments do not restate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f91e681-24a9-4b4e-9796-8a182dacb5d5
📒 Files selected for processing (2)
Plugins/TableProPluginKit/ConnectionField.swiftTablePro/Views/Connection/ConnectionFormView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/TableProPluginKit/ConnectionField.swift
… harden redis parsing
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 `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 934-935: The code currently coerces a missing
additionalFieldValues["redisDatabase"] into 0; change both the save and test
builder code paths in ConnectionFormView (where additionalFieldValues is read to
produce redisDatabase and where startupCommands is trimmed) to treat an absent
key as nil and only fall back to 0 when the key exists but cannot be parsed as
an Int; specifically, when constructing the redisDatabase value from
additionalFieldValues["redisDatabase"] return nil if the dictionary lookup
yields nil, otherwise attempt Int(...) and use 0 only on parse failure so
non-Redis connections do not get backfilled with "0" during the migration.
- Around line 296-300: The Authentication fields loop currently only renders
.toggle types (ForEach over authSectionFields with if field.fieldType ==
.toggle) so other plugin-provided field types are dropped; update the rendering
to handle all field.fieldType cases by switching on field.fieldType (or
delegating to a helper like viewForField(_:) that returns the appropriate
SwiftUI view) and keep using toggleBinding(for:) only for .toggle while wiring
textBinding/secureBinding/etc. for other types; also ensure the
authSectionFields/advanced filtering logic that produces authSectionFields
doesn’t mistakenly filter out non-.toggle non-.advanced fields so plugin fields
appear in the correct Authentication or Advanced section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae0685a3-812d-489b-a9fa-1852d653248f
📒 Files selected for processing (1)
TablePro/Views/Connection/ConnectionFormView.swift
3 tasks
datlechin
added a commit
that referenced
this pull request
Mar 13, 2026
* feat: make ConnectionFormView fully dynamic via plugin metadata (#309) * fix: remove dead code, restore Redis default, add Codable backward compat * fix: address PR review — filter Advanced tab, normalize selected tab, harden redis parsing * fix: use ConnectionFieldRow for auth fields, preserve nil redisDatabase for non-Redis * feat: add runtime validation of plugin driver descriptors * fix: address PR review — fix partial registration, wire up dead code, rewrite tests * fix: wire up validateDialectDescriptor, serialize mock-based test suite
This was referenced Mar 14, 2026
4 tasks
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.
Summary
FieldSection,hidesPasswordtoConnectionFieldandsupportsSSH/supportsSSLtoDriverPluginprotocol so plugins can control connection form behaviorpluginTypeIdchecks fromConnectionFormView(pgpass toggle, password visibility, SSH/SSL tab visibility, Redis database persistence)DatabaseDriverFactory.resolvePassword()by removing PostgreSQL/Redshift type guard —usePgpasscan only be true if a plugin declares itauthentication-section field withhidesPassword: truesupportsSSH = false,supportsSSL = falseTest plan
xcodebuild -scheme TablePro build -skipPackagePluginValidation)grep -rn 'pluginTypeId == "PostgreSQL"' TablePro/Views/Connection/returns no resultsSummary by CodeRabbit
New Features
Bug Fixes