Repository navigation
feat(drivers): edit column comments through driver capabilities - #1453
Conversation
| if driver.manifest().is_builtin | ||
| && driver.manifest().capabilities.sql_dialect == Some(SqlDialect::Mysql) | ||
| { | ||
| connected_driver_and_params(runtime, None, connection_id, database).await |
There was a problem hiding this comment.
WARNING: MySQL SQL preview now requires a live connection
sql_builder_driver_and_params deliberately keeps built-in drivers credential-free and tunnel-free (its doc comment: "No tunnel is opened, so a preview works while the server is unreachable"). For MySQL this branch now calls connected_driver_and_params (restores secrets, resolves/opens SSH/k8s/SSM tunnels), and the *_for_connection overrides then call get_mysql_pool + SELECT @@sql_mode. This runs for every Add/Modify Column preview (300ms debounced in ModifyColumnModal) and for every MySQL column edit, not only comment edits. If the server is unreachable, a tunnel cannot start, or a secret is missing, the preview now shows -- <error> and the save fails, whereas it used to render offline.
Consider falling back to the default builder (no_backslash_escapes = false) for the preview when the connection or sql_mode lookup fails, or only taking the connected path when a comment is actually present in the new/old column.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (15 files)
Fix these issues in Kilo Cloud Reviewed by claude-sonnet-5.5 · Input: 278 · Output: 5.6K · Cached: 985.4K Review guidance: REVIEW.md from base branch |
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for keeping the UI capability-driven and making the MySQL literal quoting respect the server's SQL mode. I reviewed 0a2f91d26764bb3a465fe9202e57f63a8a3aa433, including the shared application dispatch and the default methods used by external drivers.
No evident issue found in this pass. The 76 targeted frontend tests, 13 targeted Rust unit tests, new PostgreSQL comment round-trip and MySQL 8.4 integration test passed. Typecheck, targeted source ESLint and the frontend production build passed too. The database fixtures were disposable local containers; this was not yet a desktop UI check.
I will now verify add/edit/clear/rename and preserved column metadata in the real app, including capability gating, and run the full required checks before merging.
|
Tested Exercised add/edit/clear/rename with multiline Unicode, apostrophes and backslashes, checked generated previews and persisted comments/defaults/constraints/ON UPDATE, verified permission errors do not report success or change metadata, and confirmed SQLite hides the unsupported field. The new PostgreSQL round-trip and MySQL integration fixture passed, including both MySQL escaping modes. |
Enables the shared
column_commentscapability for built-in PostgreSQL and MySQL/MariaDB. Add/Modify Column supports multiline comments, with matching preview and execution on desktop and web.PostgreSQL generates only COMMENT ON COLUMN for comment-only changes, removes comments with IS NULL, and targets the new name after a rename. MySQL preserves comments, defaults and ON UPDATE when restating a column; clearing a comment emits COMMENT ''. Its connection-aware builders read the actual sql_mode using resolved secrets, tunnels and the selected database, so quoting works with and without NO_BACKSLASH_ESCAPES.
Closes #1448.
External PostgreSQL support: TabularisDB/tabularis-postgresql-plugin#187. The UI remains capability-driven for built-in and external drivers, with no driver-ID check. Internal connection-aware Rust methods default to the existing builders, preserving the external RPC contract. Drivers without the capability keep the field hidden.
Validation:
Manual checks before marking ready: add/edit/clear and rename with comments; check refreshed metadata, defaults/constraints, keyboard access and permission errors on desktop/web.
Review follow-up: MySQL's connected path is restricted to nonempty new comment text. Ordinary column previews and clearing a comment remain credential-free, tunnel-free and offline. Added driver-level and application-level regressions using missing connection details, an unreachable SSH-enabled host and a secret store that fails on any access. Nonempty comment text still requires the real sql_mode; there is deliberately no unsafe guessed-mode fallback. These new tests await CI because the local main-repo OpenSSL/Perl toolchain remains blocked.