Conversation
…e/MSSQL/Oracle plugins
|
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 (6)
📝 WalkthroughWalkthroughAdds SQL dialect descriptors and ParameterStyle to the plugin API, introduces PluginDatabaseDriver hooks for DDL/DML/table-ops/EXPLAIN, routes query/DDL generation to plugins (with fallbacks), updates adapters/managers/builders to delegate to plugins, and expands tests for plugin delegation and parameter styles. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI / Coordinator"
participant PM as "PluginManager"
participant AD as "PluginDriverAdapter"
participant SD as "SchemaStatementGenerator"
participant DB as "Database"
UI->>PM: request plugin/descriptor for databaseType
PM-->>UI: return plugin driver / SQLDialectDescriptor
UI->>AD: call buildExplainQuery / pluginGenerateStatements / DDL hooks
AD-->>SD: delegate DDL generation request (or return nil)
alt plugin returns SQL
AD-->>UI: SQL statements (EXPLAIN / DML / DDL)
else
SD-->>UI: fallback built-in SQL statements
end
UI->>DB: execute SQL statements
DB-->>UI: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
TablePro/Core/Services/Query/TableQueryBuilder.swift (1)
47-67:⚠️ Potential issue | 🟠 MajorThese fallbacks still emit invalid pagination for Oracle and MSSQL.
If
pluginDriveris missing, all four builders drop toLIMIT ... OFFSET.TableQueryBuilderis still created without a plugin driver first, so startup/reconnect paths can now produce SQL that Oracle and MSSQL reject.Also applies to: 89-120, 140-171, 197-247
TablePro/Views/Main/MainContentCoordinator.swift (1)
282-293:⚠️ Potential issue | 🟠 MajorThis makes table-tab sorting a no-op for plugin-backed SQL drivers.
queryBuilder.setPluginDriver(pluginDriver)now runs for every query-building plugin, butTableQueryBuilder.buildSortedQueryandbuildMultiSortQuerycurrently return the original SQL unchanged wheneverpluginDriver != nil. After the driver attaches, clicking a sortable column no longer rewrites the query for plugin-backed SQL databases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/MainContentCoordinator.swift` around lines 282 - 293, The table sorting became a no-op because setupPluginDriver attaches a non-nil pluginDriver to the queryBuilder, and TableQueryBuilder.buildSortedQuery / buildMultiSortQuery currently short-circuit and return the original SQL whenever pluginDriver != nil; update those methods to support plugin-backed drivers by either delegating sort SQL construction to the plugin driver (e.g., call pluginDriver.buildSortedQuery(...) or similar API) or, if the plugin doesn't implement sorting, fall back to the existing local SQL-rewrite logic instead of returning the original SQL; modify the implementations in TableQueryBuilder (buildSortedQuery and buildMultiSortQuery) to check for a plugin-provided sorting implementation and only return the unchanged SQL if neither the plugin nor the local logic can produce a rewritten query.TablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swift (1)
84-105:⚠️ Potential issue | 🔴 CriticalThe new FK hooks are still unreachable for the excluded engines.
Lines 88-89 and Lines 102-103 can return custom plugin SQL, but
generateTableOperationSQLstill short-circuits FK handling behind the oldfkApplicablecheck on Line 43. For.mssql,.oracle,.clickhouse,.duckdb, and.postgresql, the override path never runs even whenignoreForeignKeysis requested.Suggested fix
- let fkApplicable = dbType != .postgresql && dbType != .clickhouse - && dbType != .mssql && dbType != .oracle && dbType != .duckdb - let needsDisableFK = includeFKHandling && fkApplicable && truncates.union(deletes).contains { tableName in + let needsForeignKeyBypass = includeFKHandling && truncates.union(deletes).contains { tableName in options[tableName]?.ignoreForeignKeys == true } + let disableFKStatements = needsForeignKeyBypass ? fkDisableStatements(for: dbType) : [] + let enableFKStatements = needsForeignKeyBypass ? fkEnableStatements(for: dbType) : [] @@ - if needsDisableFK { - statements.append(contentsOf: fkDisableStatements(for: dbType)) + if !disableFKStatements.isEmpty { + statements.append(contentsOf: disableFKStatements) } @@ - if needsDisableFK { - statements.append(contentsOf: fkEnableStatements(for: dbType)) + if !enableFKStatements.isEmpty { + statements.append(contentsOf: enableFKStatements) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+TableOperations.swift around lines 84 - 105, generateTableOperationSQL currently short-circuits FK handling using fkApplicable so plugin-provided FK SQL in fkDisableStatements/fkEnableStatements is never used for .mssql, .oracle, .clickhouse, .duckdb, and .postgresql even when ignoreForeignKeys is requested; update generateTableOperationSQL to first ask fkDisableStatements/fkEnableStatements (or check if currentPluginDriverAdapter?.foreignKeyDisableStatements() != nil) or respect the ignoreForeignKeys flag before skipping FK handling via fkApplicable, so that when plugin overrides exist or ignoreForeignKeys == true the code uses fkDisableStatements/fkEnableStatements for those DatabaseType cases instead of bypassing them. Ensure references to fkApplicable, generateTableOperationSQL, fkDisableStatements, fkEnableStatements, currentPluginDriverAdapter, ignoreForeignKeys and the DatabaseType enum are used to locate and modify the logic.
🧹 Nitpick comments (4)
Plugins/SQLiteDriverPlugin/SQLitePlugin.swift (1)
40-75: Use double-quote (") as the standard identifier quote character for SQLite.SQLite's standard identifier quote character per the official documentation is the double-quote (
"), not backtick. While SQLite does accept backticks for MySQL compatibility, the canonical choice is double-quote.♻️ Suggested change
static let sqlDialect: SQLDialectDescriptor? = SQLDialectDescriptor( - identifierQuote: "`", + identifierQuote: "\"", keywords: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/SQLiteDriverPlugin/SQLitePlugin.swift` around lines 40 - 75, The SQL dialect currently uses a backtick for the identifierQuote in the SQLDialectDescriptor (sqlDialect) but SQLite's canonical identifier quote is a double-quote; update the SQLDialectDescriptor initialiser for sqlDialect to set identifierQuote to a double-quote character (") instead of "`" so identifier quoting matches SQLite's standard.TablePro/Views/Main/MainContentCoordinator.swift (1)
682-725: Move the new EXPLAIN flow into a coordinator extension.This logic fits the existing
Views/Main/Extensions/split better than the root coordinator file and will be easier to maintain beside the existing database-specific helpers.Based on learnings, "Use
MainContentCoordinatoras the central coordinator for the main UI flow, split across extension files inViews/Main/Extensions/" and "When adding MainContentCoordinator functionality, add a new extension file (e.g.,+Alerts.swift) rather than growing the main coordinator file".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/MainContentCoordinator.swift` around lines 682 - 725, Move the EXPLAIN-query flow out of the main MainContentCoordinator file into a new extension file under Views/Main/Extensions by creating an extension on MainContentCoordinator (e.g., MainContentCoordinator+Explain.swift) that contains the logic that builds explainSQL (using DatabaseManager.shared.driver(for:), PluginDriverAdapter.buildExplainQuery, Self.buildMongoExplain, Self.buildRedisDebugCommand), checks safe mode via SafeModeGuard, and calls executeQueryInternal(explainSQL); keep the same parameter access (connection, connectionId, stmt) and ensure the original method in MainContentCoordinator now delegates to the new extension method to preserve behavior and `@MainActor` Task usage.TableProTests/Views/Main/TableOperationsPluginTests.swift (1)
14-34: Add one plugin-override case to this suite.Right now these tests only prove the fallback switches. A regression where custom FK/truncate/drop statements are never reached would still pass; a small mock-adapter case for an engine currently excluded by the old gate on Line 43 would lock down the new plugin-first path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Views/Main/TableOperationsPluginTests.swift` around lines 14 - 34, Add a test case in the TableOperationsPluginTests suite that verifies the coordinator uses a plugin-first path by creating a MainContentCoordinator via makeCoordinator (or TestFixtures.makeConnection) with a mock plugin/adapter registered that returns custom FK/truncate/drop SQL, then assert the coordinator calls the plugin-provided SQL instead of the built-in DatabaseType switches; implement the mock adapter (or small fake driver) that overrides the statements, register it with the coordinator or connection before invoking the operations, and assert the returned SQL strings come from the mock to lock down the plugin-override behavior.TableProTests/Core/Plugins/ExplainQueryPluginTests.swift (1)
6-8: Import ordering: system frameworks should precede local modules.Per coding guidelines, imports should be ordered: system frameworks alphabetically → third-party → local.
Testingis an Apple system framework and should come beforeTableProPluginKit.Proposed fix
import Foundation -import TableProPluginKit import Testing + +import TableProPluginKit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Core/Plugins/ExplainQueryPluginTests.swift` around lines 6 - 8, Reorder the import statements so system frameworks come first and local/third-party modules after: move "import Testing" to appear before "import TableProPluginKit" and ensure system imports (import Foundation, import Testing) are alphabetized if multiple; leave "import TableProPluginKit" after the system imports.
🤖 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/ClickHouseDriverPlugin/ClickHousePlugin.swift`:
- Around line 606-643: The update/delete generators currently call
buildWhereClause and will produce statements that can affect multiple identical
rows; change generateClickHouseUpdate and generateClickHouseDelete to first
verify that a stable unique predicate for the target row can be derived (e.g. by
checking for a primary key/unique key in the provided columns or ensuring some
column(s) in change.originalRow are unique/non-null and not duplicated) before
calling buildWhereClause, and return nil if no unique predicate can be
guaranteed; reference the functions generateClickHouseUpdate,
generateClickHouseDelete, buildWhereClause and the PluginRowChange.originalRow
when adding this uniqueness guard so the driver never emits UPDATE/DELETE for
non-uniquely-identified row images.
In `@Plugins/MongoDBDriverPlugin/MongoDBPluginDriver.swift`:
- Around line 441-499: The SQL and collection interpolations in
buildExplainQuery(_:) are not escaped and can break the generated JSON; update
every string interpolation to use escapeJsonString(_:) — e.g. wrap sql in the
guard fallback and the default case as escapeJsonString(sql), wrap collection
names in all switch arms (.find, .findOne, .aggregate, .countDocuments,
.deleteOne, .deleteMany, .updateOne, .updateMany, .findOneAndUpdate,
.findOneAndReplace, .findOneAndDelete) and also escape any direct string pieces
used to build cmd/findDoc (like the "find" field and the findAndModify cmd) so
that any embedded " or \ are properly escaped before producing the
db.runCommand(...) text.
In `@Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift`:
- Around line 514-530: The current logic uses a fake-PK heuristic (hasPk
checking only that columns.first appears in change.cellChanges) and then calls
generateMssqlBatchDelete which creates OR-combined full-row predicates; instead,
only allow batch deletes when you can verify a real primary key (e.g., an
explicit PK flag or unique key column), and otherwise fall back to per-row
deletes using generateMssqlDelete that emits DELETE TOP (1) to avoid collapsing
duplicates; update the branch around hasPk to validate true uniqueness rather
than presence of columns.first (or add an isPrimaryKey check), call
generateMssqlBatchDelete only when that validation passes, and in all other
cases iterate deleteChanges and use generateMssqlDelete; apply the same change
to the other similar block (the one around lines 597-630) so both places avoid
batching on the fake-PK heuristic.
In `@Plugins/OracleDriverPlugin/OraclePlugin.swift`:
- Around line 911-913: The "REGEX" branch in OraclePlugin.swift currently maps
to a LIKE '%...%' which is incorrect; update the case "REGEX" handling to use
Oracle's REGEXP_LIKE function instead of LIKE. Specifically, build and return a
string like REGEXP_LIKE(<quoted>, '<escapedPattern>') where you escape single
quotes in value (same as escaped = value.replacingOccurrences(of: "'", with:
"''")) and use the existing quoted identifier variable (quoted) as the first
argument; keep any pattern modifiers (e.g., case sensitivity) consistent with
other filters if needed.
- Around line 633-648: The current build of the INSERT statement skips returning
anything when nonDefaultColumns is empty (all values are "__DEFAULT__"), which
drops default-only inserts; update the logic around nonDefaultColumns/parameters
in the function that constructs the INSERT (references: nonDefaultColumns,
parameters, escapeOracleIdentifier, values, columns) so that when
nonDefaultColumns.isEmpty you return a valid Oracle INSERT that preserves
defaults (e.g. "INSERT INTO <table> DEFAULT VALUES" with an empty parameters
array) instead of returning nil; ensure you still escape the table name via
escapeOracleIdentifier and return the tuple (statement: sql, parameters:
parameters).
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift`:
- Around line 364-373: The current buildExplainQuery(_:) re-tokenizes SQL by
whitespace and returns bogus explains; replace that logic to use the existing
RedisCommandParser (or equivalent) to parse the input into a RedisCommand/args,
then only return an explain surrogate when the parsed command is a single-key
operation (e.g., GET/SET/DEL with exactly one key argument); otherwise return
nil (do not default to "INFO commandstats"). Update buildExplainQuery to call
RedisCommandParser, inspect the parsed command name and arguments, and construct
"DEBUG OBJECT <key>" only when the parser confirms a single key.
In `@Plugins/TableProPluginKit/PluginDatabaseDriver.swift`:
- Around line 3-6: The default parameter binding currently only replaces "?"
placeholders but the new ParameterStyle enum allows .dollar, so update the
default executeParameterized(query:parameters:) implementation (in the
PluginDatabaseDriver extension/impl) to honor ParameterStyle: if parameterStyle
== .questionMark continue substituting "?" in order, if parameterStyle ==
.dollar substitute $1, $2, ... (1-based) for each parameter; alternatively, if
you prefer fail-fast, change the default to throw/fatalError when parameterStyle
== .dollar and document that drivers using .dollar must override
executeParameterized; reference ParameterStyle and
executeParameterized(query:parameters:) so reviewers can find and patch the
behavior.
In `@TablePro/Core/ChangeTracking/SQLStatementGenerator.swift`:
- Around line 29-50: The INSERT placeholder numbering is computed from the full
parameters array and then SQLFunctionLiteral entries are stripped, causing gaps
with .dollar style (e.g., VALUES (NOW(), $2) but only one bound value); update
generateInsertSQLFromStoredData(rowIndex:values:) to build the placeholder list
from the final bind array (i.e., remove or skip SQLFunctionLiteral entries
before generating $n placeholders) so dollar placeholders are densely numbered,
and apply the same fix to the similar logic referenced at lines 123-130; use the
types/methods SQLFunctionLiteral, ParameterStyle (.dollar), and the
generateInsertSQLFromStoredData(rowIndex:values:) routine to locate and change
the ordering (compute binds first, then generate placeholders).
In `@TablePro/Core/Services/Query/SQLDialectProvider.swift`:
- Around line 496-509: The createDialect(for:) function currently bypasses
`@MainActor` isolation using Thread.isMainThread + MainActor.assumeIsolated and
can silently fall back to builtInDialect when called off the main actor; change
the function signature to be `@MainActor` so calls are properly isolated when
accessing PluginManager.shared.sqlDialect, and remove the Thread.isMainThread /
MainActor.assumeIsolated branch so the function always queries PluginManager and
falls back to builtInDialect only when the plugin descriptor is nil; if
synchronous background calls are required, first cache
PluginManager.shared.sqlDialect on the main actor into a value-type descriptor
and pass that value into background work instead of calling PluginManager from
background threads.
In `@TablePro/Views/Main/MainContentCoordinator.swift`:
- Around line 676-680: The ClickHouse EXPLAIN handler currently calls
runClickHouseExplain and returns before the confirmation/safe-mode block,
bypassing the "confirm all queries" gate; update the logic so that when
connection.type == .clickhouse you route through the same confirmation flow used
for other queries (i.e., the existing confirmation block) and only call
runClickHouseExplain after confirmation succeeds (or respect the safe-mode
flag), instead of calling runClickHouseExplain immediately and returning.
---
Outside diff comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+TableOperations.swift:
- Around line 84-105: generateTableOperationSQL currently short-circuits FK
handling using fkApplicable so plugin-provided FK SQL in
fkDisableStatements/fkEnableStatements is never used for .mssql, .oracle,
.clickhouse, .duckdb, and .postgresql even when ignoreForeignKeys is requested;
update generateTableOperationSQL to first ask
fkDisableStatements/fkEnableStatements (or check if
currentPluginDriverAdapter?.foreignKeyDisableStatements() != nil) or respect the
ignoreForeignKeys flag before skipping FK handling via fkApplicable, so that
when plugin overrides exist or ignoreForeignKeys == true the code uses
fkDisableStatements/fkEnableStatements for those DatabaseType cases instead of
bypassing them. Ensure references to fkApplicable, generateTableOperationSQL,
fkDisableStatements, fkEnableStatements, currentPluginDriverAdapter,
ignoreForeignKeys and the DatabaseType enum are used to locate and modify the
logic.
In `@TablePro/Views/Main/MainContentCoordinator.swift`:
- Around line 282-293: The table sorting became a no-op because
setupPluginDriver attaches a non-nil pluginDriver to the queryBuilder, and
TableQueryBuilder.buildSortedQuery / buildMultiSortQuery currently short-circuit
and return the original SQL whenever pluginDriver != nil; update those methods
to support plugin-backed drivers by either delegating sort SQL construction to
the plugin driver (e.g., call pluginDriver.buildSortedQuery(...) or similar API)
or, if the plugin doesn't implement sorting, fall back to the existing local
SQL-rewrite logic instead of returning the original SQL; modify the
implementations in TableQueryBuilder (buildSortedQuery and buildMultiSortQuery)
to check for a plugin-provided sorting implementation and only return the
unchanged SQL if neither the plugin nor the local logic can produce a rewritten
query.
---
Nitpick comments:
In `@Plugins/SQLiteDriverPlugin/SQLitePlugin.swift`:
- Around line 40-75: The SQL dialect currently uses a backtick for the
identifierQuote in the SQLDialectDescriptor (sqlDialect) but SQLite's canonical
identifier quote is a double-quote; update the SQLDialectDescriptor initialiser
for sqlDialect to set identifierQuote to a double-quote character (") instead of
"`" so identifier quoting matches SQLite's standard.
In `@TablePro/Views/Main/MainContentCoordinator.swift`:
- Around line 682-725: Move the EXPLAIN-query flow out of the main
MainContentCoordinator file into a new extension file under
Views/Main/Extensions by creating an extension on MainContentCoordinator (e.g.,
MainContentCoordinator+Explain.swift) that contains the logic that builds
explainSQL (using DatabaseManager.shared.driver(for:),
PluginDriverAdapter.buildExplainQuery, Self.buildMongoExplain,
Self.buildRedisDebugCommand), checks safe mode via SafeModeGuard, and calls
executeQueryInternal(explainSQL); keep the same parameter access (connection,
connectionId, stmt) and ensure the original method in MainContentCoordinator now
delegates to the new extension method to preserve behavior and `@MainActor` Task
usage.
In `@TableProTests/Core/Plugins/ExplainQueryPluginTests.swift`:
- Around line 6-8: Reorder the import statements so system frameworks come first
and local/third-party modules after: move "import Testing" to appear before
"import TableProPluginKit" and ensure system imports (import Foundation, import
Testing) are alphabetized if multiple; leave "import TableProPluginKit" after
the system imports.
In `@TableProTests/Views/Main/TableOperationsPluginTests.swift`:
- Around line 14-34: Add a test case in the TableOperationsPluginTests suite
that verifies the coordinator uses a plugin-first path by creating a
MainContentCoordinator via makeCoordinator (or TestFixtures.makeConnection) with
a mock plugin/adapter registered that returns custom FK/truncate/drop SQL, then
assert the coordinator calls the plugin-provided SQL instead of the built-in
DatabaseType switches; implement the mock adapter (or small fake driver) that
overrides the statements, register it with the coordinator or connection before
invoking the operations, and assert the returned SQL strings come from the mock
to lock down the plugin-override behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0763ad91-1c15-4556-87e6-9ec802aaa1a8
📒 Files selected for processing (35)
CHANGELOG.mdPlugins/ClickHouseDriverPlugin/ClickHousePlugin.swiftPlugins/DuckDBDriverPlugin/DuckDBPlugin.swiftPlugins/MSSQLDriverPlugin/MSSQLPlugin.swiftPlugins/MongoDBDriverPlugin/MongoDBPlugin.swiftPlugins/MongoDBDriverPlugin/MongoDBPluginDriver.swiftPlugins/MySQLDriverPlugin/MySQLPlugin.swiftPlugins/MySQLDriverPlugin/MySQLPluginDriver.swiftPlugins/OracleDriverPlugin/OraclePlugin.swiftPlugins/PostgreSQLDriverPlugin/PostgreSQLPlugin.swiftPlugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swiftPlugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swiftPlugins/RedisDriverPlugin/RedisPlugin.swiftPlugins/RedisDriverPlugin/RedisPluginDriver.swiftPlugins/SQLiteDriverPlugin/SQLitePlugin.swiftPlugins/TableProPluginKit/DriverPlugin.swiftPlugins/TableProPluginKit/PluginDatabaseDriver.swiftPlugins/TableProPluginKit/SQLDialectDescriptor.swiftPlugins/TableProPluginKit/SchemaTypes.swiftTablePro/Core/ChangeTracking/SQLStatementGenerator.swiftTablePro/Core/Database/DatabaseDriver.swiftTablePro/Core/Database/DatabaseManager.swiftTablePro/Core/Plugins/PluginDriverAdapter.swiftTablePro/Core/Plugins/PluginManager.swiftTablePro/Core/SchemaTracking/SchemaStatementGenerator.swiftTablePro/Core/Services/Query/SQLDialectProvider.swiftTablePro/Core/Services/Query/TableQueryBuilder.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swiftTablePro/Views/Main/MainContentCoordinator.swiftTablePro/Views/Structure/TableStructureView.swiftTableProTests/Core/ChangeTracking/SQLStatementGeneratorParameterStyleTests.swiftTableProTests/Core/Plugins/ExplainQueryPluginTests.swiftTableProTests/Core/Plugins/SQLDialectDescriptorTests.swiftTableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swiftTableProTests/Views/Main/TableOperationsPluginTests.swift
datlechin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/Core/Services/Query/TableQueryBuilder.swift (1)
69-85:⚠️ Potential issue | 🟠 MajorPreserve filter/search semantics in the fallback path.
These three fallbacks now return a plain
SELECT * ... LIMIT/OFFSET, so when the plugin driver is unavailable or a plugin returnsnil, active filters and quick-search text are silently ignored. That turns the table controls into no-ops instead of degrading gracefully. Please keep a genericWHERE/ORDER BYimplementation here rather than falling back to an unfiltered browse query.Also applies to: 95-107, 120-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Query/TableQueryBuilder.swift` around lines 69 - 85, The fallback path currently returns an unfiltered "SELECT *" when pluginDriver is nil or buildFilteredQuery returns nil, which drops active filters, quick-search text, and sort semantics; update the fallback in the function that calls pluginDriver.buildFilteredQuery (referencing pluginDriver, buildFilteredQuery, sortColumnsAsTuples, filters, sortState, logicMode, columns, limit, offset and databaseType.quoteIdentifier) to construct a generic SQL string that preserves WHERE (from filters and quick-search), ORDER BY (from sortColumnsAsTuples/sortState), and pagination (limit/offset), and apply the same change to the other fallback sites noted (lines covering the other occurrences) so behavior degrades gracefully instead of ignoring filters and search.
♻️ Duplicate comments (3)
TablePro/Core/ChangeTracking/SQLStatementGenerator.swift (1)
178-192:⚠️ Potential issue | 🟠 MajorKeep
$nplaceholders dense when SQL literals are inlined.The placeholder index is still derived from the full
parametersarray and only afterwardsSQLFunctionLiteralentries are removed from the bind list. With.dollar, a row like[NOW(), "x"]becomesVALUES (NOW(), $2)but only one value is bound. Build the bind array first, or increment the placeholder counter only for values that will actually be bound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/ChangeTracking/SQLStatementGenerator.swift` around lines 178 - 192, The INSERT placeholder numbering is computed from the full parameters array which leaves gaps when SQLFunctionLiteral entries are inlined; change the logic in the block that builds placeholders and bindParameters (referencing nonDefaultColumns, parameters, SQLFunctionLiteral, placeholder(at:), and ParameterizedStatement) so that you first build the bindParameters array (or otherwise only increment the placeholder counter for non-SQLFunctionLiteral entries) and then generate placeholders using the bindParameters' indices — this ensures dollar-style placeholders are dense and match the number/order of bound parameters.TablePro/Views/Main/MainContentCoordinator.swift (1)
676-680:⚠️ Potential issue | 🟡 MinorKeep ClickHouse EXPLAIN behind the existing safe-mode gate.
This early return still bypasses the confirmation flow below, so connections configured to confirm all queries no longer apply that policy to ClickHouse EXPLAIN. Route this path through the same permission check before calling
runClickHouseExplain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/MainContentCoordinator.swift` around lines 676 - 680, The ClickHouse-specific branch currently short-circuits the confirmation flow by calling runClickHouseExplain(variant: .plan) and returning; instead, remove the early return and route this path through the same permission/confirmation check used for other queries so connections with "confirm all queries" still prompt. Concretely, when connection.type == .clickhouse, invoke the existing confirmation/permission routine (the same code used below for normal queries) and only call runClickHouseExplain(variant: .plan) if that check grants permission; do not bypass or return early from the function.TablePro/Core/Services/Query/SQLDialectProvider.swift (1)
39-49:⚠️ Potential issue | 🟠 MajorDon't gate dialect resolution on
Thread.isMainThread.Off-main callers always get
EmptyDialect, so formatter/highlighter paths silently lose plugin keywords, functions, and data types.Thread.isMainThreadalso is not the right isolation contract forMainActor.assumeIsolatedhere; this factory should either be@MainActoror consume a descriptor value that was already resolved on the main actor.In Swift concurrency, is `Thread.isMainThread` sufficient to make `MainActor.assumeIsolated { ... }` safe, or must the caller already be isolated to `@MainActor`? What is the recommended pattern for a synchronous factory that needs to read `@MainActor` state?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Query/SQLDialectProvider.swift` around lines 39 - 49, The factory currently returns EmptyDialect for off-main callers because it gates plugin resolution with Thread.isMainThread and then uses MainActor.assumeIsolated incorrectly; remove the Thread.isMainThread check and ensure plugin access is done on the MainActor—either mark createDialect(for:) as `@MainActor` and call PluginManager.shared.sqlDialect(for:) directly (return PluginDialectAdapter or EmptyDialect), or if you cannot make the API main-isolated, change the factory to accept a pre-resolved descriptor (e.g. pass descriptor: SQLDialectDescriptor?) or make the factory async and use await MainActor.run { PluginManager.shared.sqlDialect(for:) } so callers off the main actor still get the real descriptor rather than EmptyDialect; update references to createDialect, SQLDialectProvider, PluginManager.shared.sqlDialect(for:), MainActor.assumeIsolated, PluginDialectAdapter, and EmptyDialect accordingly.
🧹 Nitpick comments (2)
TablePro/Views/Main/MainContentCoordinator.swift (1)
282-293: Please move this new coordinator behavior into an extension file.This file is already carrying a lot of unrelated responsibilities, and adding
setupPluginDriver()here makes that worse. I'd move this into aMainContentCoordinator+Plugins.swift-style extension to keep the main coordinator focused.Based on learnings: Use
MainContentCoordinatoras the central coordinator for the main UI flow, split across extension files inViews/Main/Extensions/, and when addingMainContentCoordinatorfunctionality, add a new extension file rather than growing the main coordinator file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/MainContentCoordinator.swift` around lines 282 - 293, The new setupPluginDriver() method should be moved out of the large MainContentCoordinator.swift into an extension file: create a new file named MainContentCoordinator+Plugins.swift under Views/Main/Extensions and declare "extension MainContentCoordinator" there; move the entire setupPluginDriver() implementation (including its guard for DatabaseManager.shared.driver(for: connectionId), use of driver.queryBuildingPluginDriver, queryBuilder.setPluginDriver(_:), changeManager.pluginDriver assignment, and NotificationCenter removal logic referencing pluginDriverObserver) into that extension and keep the same access level (private) and imports; finally remove the original method from MainContentCoordinator.swift so behavior is identical but responsibilities are separated.TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift (1)
287-303: Clarify: handler returns 2 strings but test expects 1 statement.The
modifyPrimaryKeyHandlerreturns an array with 2 SQL strings, but the test assertsstmts.count == 1. This impliesSchemaStatementGeneratorjoins multiple SQL strings into a singleSchemaStatement. If this is intentional, consider adding a brief comment to clarify this consolidation behavior for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift` around lines 287 - 303, The test is confusing because modifyPrimaryKeyHandler returns two SQL strings but the assertion expects one SchemaStatement from SchemaStatementGenerator.generate; either update the test to expect two statements or (preferred) add a brief clarifying comment where mock.modifyPrimaryKeyHandler is defined (or near SchemaStatementGenerator.generate usage) explaining that the generator consolidates multiple SQL strings returned by the plugin into a single SchemaStatement (and thus stmts.count == 1), referencing modifyPrimaryKeyHandler, SchemaStatementGenerator, and generate so future maintainers understand the consolidation behavior.
🤖 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/Core/SchemaTracking/SchemaStatementGenerator.swift`:
- Around line 47-49: The loop in SchemaStatementGenerator.generate(changes:)
currently drops changes when generateStatement(for:) returns nil; instead, when
generateStatement(for:) yields nil, call the generic fallback SQL generator
(implement or call something like generateGenericStatement(for:) and use its
result) and only skip/throw if that fallback also fails—alternatively, surface
an error to the caller rather than silently continue; update the guard let stmt
= try generateStatement(for: change) else { continue } to attempt the generic
fallback or throw a descriptive error so DDL operations are not silently
no‑oped.
- Around line 23-37: Stored property primaryKeyConstraintName is never used in
SchemaStatementGenerator.generateModifyPrimaryKey and either should be removed
or threaded into the plugin call; update the code so the plugin receives the
constraint name: either (A) remove primaryKeyConstraintName from the initializer
and property and any mention in comments if no DB needs it, or (B) extend the
PluginDatabaseDriver protocol (and its implementations) to accept an optional
primaryKeyConstraintName in generateModifyPrimaryKeySQL and change
SchemaStatementGenerator.generateModifyPrimaryKey to pass
self.primaryKeyConstraintName along with table, oldColumns, newColumns when
calling generateModifyPrimaryKeySQL; ensure the init signature
(primaryKeyConstraintName: String?) and stored property are kept only for option
B and tests updated accordingly.
In `@TablePro/Core/Services/Query/TableQueryBuilder.swift`:
- Around line 144-146: The code in TableQueryBuilder currently short-circuits
and returns baseQuery whenever pluginDriver is non-nil, which prevents
buildMultiSortQuery(...) and any ORDER BY changes from being applied; remove or
alter that early return so plugin-backed tables still apply the sort: locate the
conditional checking pluginDriver (the branch that returns baseQuery) and
instead merge or replace baseQuery with the result of buildMultiSortQuery(...)
(or otherwise apply the constructed ORDER BY clauses) before returning; ensure
the same change is applied to the other identical branch that returns early (the
second instance around the build logic) so server-side sorting via
MainContentCoordinator.handleSort works even when pluginDriver is attached.
In `@TableProTests/Core/ClickHouse/ClickHouseDialectTests.swift`:
- Around line 15-19: The test testFactoryCreatesDialect currently allows
EmptyDialect because it accepts either "\"" or "`"; change the assertion to
verify ClickHouse-specific metadata returned by
SQLDialectFactory.createDialect(for: .clickhouse) — for example assert the
dialect is not EmptyDialect (check type/name) and/or assert the identifierQuote
equals "`" (the ClickHouse quote) or another ClickHouse-only property so the
test fails if the factory falls back to EmptyDialect; reference
SQLDialectFactory.createDialect, testFactoryCreatesDialect, and
EmptyDialect.identifierQuote when making the replacement.
---
Outside diff comments:
In `@TablePro/Core/Services/Query/TableQueryBuilder.swift`:
- Around line 69-85: The fallback path currently returns an unfiltered "SELECT
*" when pluginDriver is nil or buildFilteredQuery returns nil, which drops
active filters, quick-search text, and sort semantics; update the fallback in
the function that calls pluginDriver.buildFilteredQuery (referencing
pluginDriver, buildFilteredQuery, sortColumnsAsTuples, filters, sortState,
logicMode, columns, limit, offset and databaseType.quoteIdentifier) to construct
a generic SQL string that preserves WHERE (from filters and quick-search), ORDER
BY (from sortColumnsAsTuples/sortState), and pagination (limit/offset), and
apply the same change to the other fallback sites noted (lines covering the
other occurrences) so behavior degrades gracefully instead of ignoring filters
and search.
---
Duplicate comments:
In `@TablePro/Core/ChangeTracking/SQLStatementGenerator.swift`:
- Around line 178-192: The INSERT placeholder numbering is computed from the
full parameters array which leaves gaps when SQLFunctionLiteral entries are
inlined; change the logic in the block that builds placeholders and
bindParameters (referencing nonDefaultColumns, parameters, SQLFunctionLiteral,
placeholder(at:), and ParameterizedStatement) so that you first build the
bindParameters array (or otherwise only increment the placeholder counter for
non-SQLFunctionLiteral entries) and then generate placeholders using the
bindParameters' indices — this ensures dollar-style placeholders are dense and
match the number/order of bound parameters.
In `@TablePro/Core/Services/Query/SQLDialectProvider.swift`:
- Around line 39-49: The factory currently returns EmptyDialect for off-main
callers because it gates plugin resolution with Thread.isMainThread and then
uses MainActor.assumeIsolated incorrectly; remove the Thread.isMainThread check
and ensure plugin access is done on the MainActor—either mark
createDialect(for:) as `@MainActor` and call PluginManager.shared.sqlDialect(for:)
directly (return PluginDialectAdapter or EmptyDialect), or if you cannot make
the API main-isolated, change the factory to accept a pre-resolved descriptor
(e.g. pass descriptor: SQLDialectDescriptor?) or make the factory async and use
await MainActor.run { PluginManager.shared.sqlDialect(for:) } so callers off the
main actor still get the real descriptor rather than EmptyDialect; update
references to createDialect, SQLDialectProvider,
PluginManager.shared.sqlDialect(for:), MainActor.assumeIsolated,
PluginDialectAdapter, and EmptyDialect accordingly.
In `@TablePro/Views/Main/MainContentCoordinator.swift`:
- Around line 676-680: The ClickHouse-specific branch currently short-circuits
the confirmation flow by calling runClickHouseExplain(variant: .plan) and
returning; instead, remove the early return and route this path through the same
permission/confirmation check used for other queries so connections with
"confirm all queries" still prompt. Concretely, when connection.type ==
.clickhouse, invoke the existing confirmation/permission routine (the same code
used below for normal queries) and only call runClickHouseExplain(variant:
.plan) if that check grants permission; do not bypass or return early from the
function.
---
Nitpick comments:
In `@TablePro/Views/Main/MainContentCoordinator.swift`:
- Around line 282-293: The new setupPluginDriver() method should be moved out of
the large MainContentCoordinator.swift into an extension file: create a new file
named MainContentCoordinator+Plugins.swift under Views/Main/Extensions and
declare "extension MainContentCoordinator" there; move the entire
setupPluginDriver() implementation (including its guard for
DatabaseManager.shared.driver(for: connectionId), use of
driver.queryBuildingPluginDriver, queryBuilder.setPluginDriver(_:),
changeManager.pluginDriver assignment, and NotificationCenter removal logic
referencing pluginDriverObserver) into that extension and keep the same access
level (private) and imports; finally remove the original method from
MainContentCoordinator.swift so behavior is identical but responsibilities are
separated.
In `@TableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swift`:
- Around line 287-303: The test is confusing because modifyPrimaryKeyHandler
returns two SQL strings but the assertion expects one SchemaStatement from
SchemaStatementGenerator.generate; either update the test to expect two
statements or (preferred) add a brief clarifying comment where
mock.modifyPrimaryKeyHandler is defined (or near
SchemaStatementGenerator.generate usage) explaining that the generator
consolidates multiple SQL strings returned by the plugin into a single
SchemaStatement (and thus stmts.count == 1), referencing
modifyPrimaryKeyHandler, SchemaStatementGenerator, and generate so future
maintainers understand the consolidation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c384a126-d3ab-485f-8c6b-70992b71e64a
📒 Files selected for processing (16)
TablePro/Core/ChangeTracking/DataChangeManager.swiftTablePro/Core/ChangeTracking/SQLStatementGenerator.swiftTablePro/Core/Database/DatabaseManager.swiftTablePro/Core/SchemaTracking/SchemaStatementGenerator.swiftTablePro/Core/Services/Query/SQLDialectProvider.swiftTablePro/Core/Services/Query/TableQueryBuilder.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+MongoDB.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+Redis.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swiftTablePro/Views/Main/MainContentCoordinator.swiftTablePro/Views/Structure/TableStructureView.swiftTableProTests/Core/ClickHouse/ClickHouseDialectTests.swiftTableProTests/Core/Plugins/SQLDialectDescriptorTests.swiftTableProTests/Core/SchemaTracking/SchemaStatementGeneratorMSSQLTests.swiftTableProTests/Core/SchemaTracking/SchemaStatementGeneratorPluginTests.swiftTableProTests/Core/SchemaTracking/SchemaStatementGeneratorTests.swift
💤 Files with no reviewable changes (4)
- TablePro/Views/Main/Extensions/MainContentCoordinator+MongoDB.swift
- TableProTests/Core/SchemaTracking/SchemaStatementGeneratorMSSQLTests.swift
- TableProTests/Core/SchemaTracking/SchemaStatementGeneratorTests.swift
- TablePro/Views/Main/Extensions/MainContentCoordinator+Redis.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- TablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swift
This was referenced Mar 13, 2026
10 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
ConnectionFieldwith.number,.toggle, and.stepperfield types for dynamic connection form renderingSQLDialectDescriptorto PluginKit so plugins self-describe their SQL dialect (keywords, functions, data types, identifier quoting), withSQLDialectFactorypreferring plugin-provided infoParameterStyle(.questionMarkvs.dollar) to plugin protocol; ClickHouse/MSSQL/Oracle plugins implementgenerateStatements()for database-specific DML syntaxPluginDatabaseDriver(ADD/MODIFY/DROP COLUMN, INDEX, FK, PK), withSchemaStatementGeneratortrying plugin methods firsttruncateTableStatements,dropObjectStatement,foreignKeyDisableStatements/Enable) to plugin protocolOFFSET...FETCH NEXTpagination into pluginbuildBrowseQuery/buildFilteredQueryhooks, removing 270 lines fromTableQueryBuilderbuildExplainQueryto plugin protocol for database-specific EXPLAIN syntaxAll phases follow the plugin-first-then-fallback pattern: the app delegates to plugins and only falls back to generic SQL when the plugin returns
nil.New files:
SQLDialectDescriptor.swift,SchemaTypes.swift(PluginKit transfer types),ConnectionFieldRow.swift42 files changed: +4618 / -383 lines
6 new test files: 45 tests covering parameter style, DDL generation, table operations, dialect descriptors, explain queries, and connection fields
Test plan
xcodebuild buildsucceeds (all 11 targets)swiftlint lint --strict— no new violationsSummary by CodeRabbit
New Features
Improvements
Tests