Skip to content

feat(drivers): edit column comments through driver capabilities - #1453

Merged
NewtTheWolf merged 5 commits into
TabularisDB:mainfrom
lejyfps:feat/1448-postgres-column-comments
Nov 9, 2026
Merged

NewtTheWolf merged 5 commits into
TabularisDB:mainfrom
lejyfps:feat/1448-postgres-column-comments

Conversation

@lejyfps

@lejyfps lejyfps commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Enables the shared column_comments capability 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:

  • 139 focused frontend tests, typecheck, targeted ESLint/Prettier, theme-token and diff checks passed for the UI implementation.
  • PostgreSQL integration CI passed after fixing the optional-comment comparison and cleanup on assertion failures.
  • Added MySQL unit coverage for add/change/clear, rename, comment/default/ON UPDATE preservation and both escaping modes.
  • Extended the existing MySQL 5.7/8.4 CI fixture with real comment round-trips, Add Column and unchanged metadata under both sql_mode variants.
  • Added a test that connection-aware default methods preserve existing PostgreSQL builder output.
  • Main-repo Rust tests remain blocked locally by the vendored OpenSSL/Perl setup; the new MySQL cases await CI.
  • Companion PostgreSQL plugin: 449 library and 5 binary tests passed; the real-database stdio test compiles and awaits CI.
  • New Rust test code passes rustfmt; unrelated existing formatting differences are left unchanged. GitNexus upstream impact and staged change detection completed; index gaps were supplemented by direct caller inspection.

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.

@lejyfps lejyfps changed the title feat(postgres): edit column comments from the column editor feat(drivers): edit column comments through driver capabilities Nov 9, 2026
@lejyfps
lejyfps marked this pull request as ready for review November 9, 2026 11:35
if driver.manifest().is_builtin
&& driver.manifest().capabilities.sql_dialect == Some(SqlDialect::Mysql)
{
connected_driver_and_params(runtime, None, connection_id, database).await

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kilo-code-bot

kilo-code-bot Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
src-tauri/src/application/database_objects.rs 1084 MySQL Add/Modify Column SQL preview now requires secrets restoration, tunnel resolution and a live SELECT @@sql_mode round trip. This breaks the documented offline-preview contract of sql_builder_driver_and_params and affects every MySQL column edit, not only comment edits. Consider a graceful fallback to the default builder when the connection or sql_mode lookup fails, or only taking the connected path when a comment is present.
Files Reviewed (15 files)
  • packages/web-ui/src/components/modals/ModifyColumnModal.tsx
  • packages/web-ui/src/utils/columnTypes.ts
  • packages/web-ui/tests/components/modals/ModifyColumnModal.test.tsx
  • packages/web-ui/tests/utils/columnTypes.test.ts
  • src-tauri/src/application/database_objects.rs - 1 issue
  • src-tauri/src/drivers/driver_trait.rs
  • src-tauri/src/drivers/mysql/column_comment_tests.rs
  • src-tauri/src/drivers/mysql/mod.rs
  • src-tauri/src/drivers/postgres/column_comments.rs
  • src-tauri/src/drivers/postgres/column_comments_tests.rs
  • src-tauri/src/drivers/postgres/column_ddl_tests.rs
  • src-tauri/src/drivers/postgres/mod.rs
  • src-tauri/tests/mysql_columns.rs
  • src-tauri/tests/postgres_integration/column_comments.rs
  • src-tauri/tests/postgres_integration/main.rs

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 main

@NewtTheWolf NewtTheWolf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@NewtTheWolf

Copy link
Copy Markdown
Collaborator

Tested 0a2f91d26764bb3a465fe9202e57f63a8a3aa433 in the real Linux GTK/WebKitGTK app with disposable PostgreSQL 17 and MySQL 8.4 databases. Ready to merge.

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. pnpm lint && pnpm typecheck && pnpm test:all passed, including 8,637 frontend tests; GitHub checks are green. Clipboard permission noise matches the main-branch reproduction noted in #1452; the deliberately denied database operation produced the expected owner-permission error.

@NewtTheWolf
NewtTheWolf merged commit a260ba6 into TabularisDB:main Nov 9, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat]: Edit PostgreSQL column comments from the column editor

2 participants