Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHigh-level changes: Adjusted plugin table-operation APIs and adapter fallbacks, removed per-table quoting in table operations, added MySQL driver internal active-database state, updated tests for adapter table ops, and updated CHANGELOG entry. Changes
Sequence Diagram(s)mermaid UI->>Adapter: request truncate/drop for table Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 |
datlechin
force-pushed
the
fix/table-ops-and-mysql-fk-after-db-switch
branch
from
76000bf to
53a9ae0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
TablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swift (2)
114-123: Same cleanup opportunity forquotedNameanddbTypeparameters.Similar to
truncateStatements, thequotedNameparameter is now unused. Additionally,dbTypeis no longer used in this method since the adapter handles everything.♻️ Suggested cleanup
private func dropTableStatement( - tableName: String, quotedName: String, isView: Bool, - options: TableOperationOptions, dbType: DatabaseType + tableName: String, isView: Bool, options: TableOperationOptions ) -> String { let keyword = isView ? "VIEW" : "TABLE" guard let adapter = currentPluginDriverAdapter else { return "" } return adapter.dropObjectStatement( name: tableName, objectType: keyword, schema: nil, cascade: options.cascade ) }🤖 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 114 - 123, The dropTableStatement method currently accepts unused parameters quotedName and dbType; update the signature of dropTableStatement to remove those unused parameters (keep tableName, isView, and options), and adjust any callers to match the new signature, then let the method continue to use currentPluginDriverAdapter.dropObjectStatement(name:objectType:schema:cascade:) as it does now; reference the dropTableStatement function, the quotedName and dbType parameters, and currentPluginDriverAdapter.dropObjectStatement when making the changes.
105-112: Consider removing unusedquotedNameparameter.The
quotedNameparameter is still being passed to this method but is no longer used since the adapter now handles all quoting internally. This creates dead code in the function signature and at the call site (line 53).♻️ Suggested cleanup
private func truncateStatements( - tableName: String, quotedName: String, options: TableOperationOptions, dbType: DatabaseType + tableName: String, options: TableOperationOptions, dbType: DatabaseType ) -> [String] { guard let adapter = currentPluginDriverAdapter else { return [] } return adapter.truncateTableStatements( table: tableName, schema: nil, cascade: options.cascade ) }And update the call site at line 55-57:
statements.append(contentsOf: truncateStatements( - tableName: tableName, quotedName: quotedName, options: tableOptions, dbType: dbType + tableName: tableName, options: tableOptions, dbType: dbType ))🤖 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 105 - 112, The function truncateStatements currently declares an unused parameter quotedName; remove quotedName from the truncateStatements signature and from any calls, leaving parameters (tableName: String, options: TableOperationOptions, dbType: DatabaseType), and update its body to continue using currentPluginDriverAdapter and adapter.truncateTableStatements(table: tableName, schema: nil, cascade: options.cascade) unchanged; also update all call sites that still pass quotedName to call the revised truncateStatements without that argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+TableOperations.swift:
- Around line 114-123: The dropTableStatement method currently accepts unused
parameters quotedName and dbType; update the signature of dropTableStatement to
remove those unused parameters (keep tableName, isView, and options), and adjust
any callers to match the new signature, then let the method continue to use
currentPluginDriverAdapter.dropObjectStatement(name:objectType:schema:cascade:)
as it does now; reference the dropTableStatement function, the quotedName and
dbType parameters, and currentPluginDriverAdapter.dropObjectStatement when
making the changes.
- Around line 105-112: The function truncateStatements currently declares an
unused parameter quotedName; remove quotedName from the truncateStatements
signature and from any calls, leaving parameters (tableName: String, options:
TableOperationOptions, dbType: DatabaseType), and update its body to continue
using currentPluginDriverAdapter and adapter.truncateTableStatements(table:
tableName, schema: nil, cascade: options.cascade) unchanged; also update all
call sites that still pass quotedName to call the revised truncateStatements
without that argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0aaa60c-300f-45d5-bcfe-068f4ed9f491
📒 Files selected for processing (5)
CHANGELOG.mdPlugins/MySQLDriverPlugin/MySQLPluginDriver.swiftTablePro/Core/Plugins/PluginDriverAdapter.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swiftTableProTests/Core/Plugins/PluginDriverAdapterTableOpsTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
TablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swift (1)
105-122: Remove now-unused parameters from private SQL builders.After the delegation change (Line 108 and Line 119),
quotedNameanddbTypeare no longer used in these helper signatures or implementations.♻️ Suggested cleanup
- private func truncateStatements( - tableName: String, quotedName: String, options: TableOperationOptions, dbType: DatabaseType - ) -> [String] { + private func truncateStatements( + tableName: String, options: TableOperationOptions + ) -> [String] { guard let adapter = currentPluginDriverAdapter else { return [] } return adapter.truncateTableStatements( table: tableName, schema: nil, cascade: options.cascade ) } - private func dropTableStatement( - tableName: String, quotedName: String, isView: Bool, - options: TableOperationOptions, dbType: DatabaseType - ) -> String { + private func dropTableStatement( + tableName: String, isView: Bool, options: TableOperationOptions + ) -> String { let keyword = isView ? "VIEW" : "TABLE" guard let adapter = currentPluginDriverAdapter else { return "" } return adapter.dropObjectStatement( name: tableName, objectType: keyword, schema: nil, cascade: options.cascade ) }- statements.append(contentsOf: truncateStatements( - tableName: tableName, quotedName: quotedName, options: tableOptions, dbType: dbType - )) + statements.append(contentsOf: truncateStatements( + tableName: tableName, options: tableOptions + )) - let stmt = dropTableStatement( - tableName: tableName, quotedName: quotedName, - isView: viewNames.contains(tableName), options: tableOptions, dbType: dbType - ) + let stmt = dropTableStatement( + tableName: tableName, + isView: viewNames.contains(tableName), options: tableOptions + )🤖 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 105 - 122, The private helpers truncateStatements(tableName:quotedName:options:dbType:) and dropTableStatement(tableName:quotedName:isView:options:dbType:) still declare unused parameters (quotedName and dbType); remove those parameters from both function signatures and from their implementations so they become truncateStatements(tableName:options:) and dropTableStatement(tableName:isView:options:), update any internal references/call sites to pass the reduced argument list, and ensure imports/types (TableOperationOptions, DatabaseType) remain only where actually used so the code compiles.
🤖 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/MySQLDriverPlugin/MySQLPluginDriver.swift`:
- Around line 580-584: switchDatabase(to:) updates the in-memory _activeDatabase
but connect() (and reconnects) still use config.database, so a transient
reconnect can revert the DB; fix by making the state persistent: either have
connect() prefer _activeDatabase if set, or update config.database when
switching. Concretely, inside switchDatabase(to:) assign the new database into
config.database in addition to setting _activeDatabase (or modify connect() to
use _activeDatabase when non-nil) so reconnect()/connect() will open the same
active database.
---
Nitpick comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+TableOperations.swift:
- Around line 105-122: The private helpers
truncateStatements(tableName:quotedName:options:dbType:) and
dropTableStatement(tableName:quotedName:isView:options:dbType:) still declare
unused parameters (quotedName and dbType); remove those parameters from both
function signatures and from their implementations so they become
truncateStatements(tableName:options:) and
dropTableStatement(tableName:isView:options:), update any internal
references/call sites to pass the reduced argument list, and ensure
imports/types (TableOperationOptions, DatabaseType) remain only where actually
used so the code compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe7f701a-2e43-4766-964e-681d7798ef62
📒 Files selected for processing (5)
CHANGELOG.mdPlugins/MySQLDriverPlugin/MySQLPluginDriver.swiftTablePro/Core/Plugins/PluginDriverAdapter.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+TableOperations.swiftTableProTests/Core/Plugins/PluginDriverAdapterTableOpsTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- TableProTests/Core/Plugins/PluginDriverAdapterTableOpsTests.swift
- CHANGELOG.md
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
PluginDriverAdapternow generates standard SQL fallbacks (DROP TABLE/VIEW,TRUNCATE TABLE) when plugin returns nil. Previously all SQL databases silently produced empty statements. Coordinator simplified to pure delegation._activeDatabaseand updates it onswitchDatabase(). PreviouslyfetchForeignKeys,fetchAllColumns, andfetchApproximateRowCountused staleconfig.databaseafter switching databases.Test plan
DROP TABLE \tablename``TRUNCATE TABLE \tablename``fk_test_db, browseorder_items— FK chevrons should appear onorder_idandproduct_idfk_test_dbstill works as beforeSummary by CodeRabbit
Bug Fixes
Refactor
Tests
Documentation