Repository navigation
feat(editor): add Copy SQL Without Comments action - #1172
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| : ed.getValue(); | ||
| if (!source) return; | ||
| const stripped = stripSqlComments(source, activeDialectRef.current); | ||
| void copyTextToClipboard(stripped); |
There was a problem hiding this comment.
WARNING: Clipboard failures are silently dropped as an unhandled rejection
copyTextToClipboard rethrows when no onError callback is passed, and void discards the returned promise, so a failed navigator.clipboard.writeText becomes an unhandled promise rejection and the user gets no feedback. Pass an onError handler (e.g. surface the existing alert/toast) so the failure is handled.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| import { | ||
| dialectOptions, | ||
| type Dialect, | ||
| type DialectOptions, |
There was a problem hiding this comment.
SUGGESTION: Dialect allow-list duplicates the splitter's dialect table
DIALECTS re-declares the supported dialect ids, while scanNonCodeSpans validates against DIALECT_TABLE with Object.hasOwn. If a dialect is added to the splitter, this set must be updated separately or stripSqlComments silently returns the input unchanged for it. Consider exporting a shared guard from sqlSplitter (or reusing scanNonCodeSpans's check) so the rules stay in one place, as the file header intends.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (14 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 3cc7f57)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3cc7f57)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by claude-sonnet-5.5 · Input: 254 · Output: 1.9K · Cached: 299.7K Review guidance: REVIEW.md from base branch |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for working on this! Reusing scanToken from the splitter instead of writing yet another lexer is the right call, and it shows: I threw a bunch of awkward inputs at it (MySQL 1--1, # comments, markers inside strings and quoted identifiers, backslash escapes, E-strings, dollar quotes, Oracle q-quotes, [a -- b] on SQL Server, nested comments) and they all came out right. The a/**/b → a b spacing rule is a nice detail too.
There are a few things I need changed before this can go in, though:
- Optimizer hints are stripped. The description (and #1161) say hints are preserved, but
isExecutableBlockCommentonly keeps/*M!(and/*!survives because the tokenizer treats it as data). Nothing keeps/*+. For exampleSELECT /*+ INDEX(t idx) */ * FROM tbecomesSELECT * FROM ton MySQL, Oracle and Postgres (pg_hint_plan), and Oracle's--+ FULL(t)form is removed too. That silently changes the plan of the copied query. Please keep hint comments and add tests for them. - It breaks once rebased on main. The branch is about 15 commits behind. Since #1200,
activeDialectinEditor.tsxis aSqlSyntaxobject fromresolveSqlSyntax(activeCapabilities), not a string, so after merging maintsc -bfails on thestripSqlComments(source, activeDialectRef.current)call (Argument of type 'SqlSyntax | undefined' is not assignable to parameter of type 'string | undefined'). The helper should accept the same dialect input the splitter does. - Hardcoded dialect list.
DIALECTSinstripSqlComments.tsduplicates the splitter's list and already missesbigquery, so for a BigQuery plugin the action copies the text unchanged without saying so.isDialect()/optionsForfromsqlSplitterwould avoid the drift (and see.rules/plugins.mdabout branching on capabilities). - Translations.
editor.copyWithoutCommentsis only inen.json..rules/general.mdasks for new strings in every locale undersrc/i18n/locales/, even if some files are still incomplete. - Non-SQL surfaces. #1161 asks to hide the action where it doesn't apply, but it's always registered; without a dialect it just copies the text as is. A Monaco
preconditionor skipping the registration would match the issue.
Smaller things, fine as follow-ups:
- The test "preserves an unterminated block comment region as a string-like span" actually asserts that the comment is stripped, so the name says the opposite.
- Comment-only lines leave empty lines behind (
-- header\nSELECT 1;\n-- footergives\nSELECT 1;\n). - With CRLF line endings, a line ending in a
--comment loses its\r, so you end up with mixed line endings.
The 19 tests pass on the branch and lint is clean, so the core is in good shape. It's mainly the hints, the rebase and the dialect handling.
Add an editor action that copies the current selection (or the whole document when nothing is selected) with ordinary line and block comments removed. A pure stripSqlComments helper reuses the splitter's tokenizer and takes the same dialect input as the splitter (a dialect name or the connection's resolved syntax, including a plugin's sql_syntax). Kept: strings, quoted identifiers, dollar-quoted bodies, optimizer hints (/*+ ... */ and Oracle's --+) and executable comments (/*! and MariaDB /*M!). A removed comment between two non-whitespace characters becomes a single space, lines that only held comments are dropped, and CRLF line endings are preserved. The action is hidden (Monaco precondition) when the connection declares no SQL dialect, and its label is translated in every locale. Closes TabularisDB#1161
3cc7f57 to
ac730c9
Compare
Preview buildThe preview build of commit |
|
Thanks for the thorough review. All five are done:
I also did the three smaller ones: renamed the unterminated-comment test, lines that only held comments are dropped, and CRLF endings are kept. |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks, this is a great update! I went through all five points again and they're all fixed: hints are kept (/*+ ... */ everywhere and Oracle's --+, while /* + ... */ with a space is still stripped, which is exactly right), the helper takes DialectInput and typechecks on top of main, the dialect list is gone so BigQuery's GoogleSQL syntax works, the key is in every locale, and the Monaco precondition really hides the item (not just greys it out) and updates when the active connection changes. The three smaller ones are fixed too; I especially liked that CRLF endings now survive and that comment-only lines disappear without gluing the surrounding lines together.
I re-ran the tests on the branch (652 across stripSqlComments and the splitter/syntax suites), plus typecheck and lint, and also tried a bunch of awkward inputs again (strings, dollar quotes, q-quotes, [a -- b], 1--1 on MySQL, /*! and /*M!). Everything came out right, so I'm approving. A few follow-ups, none of them blocking:
void copyTextToClipboard(stripped)has no error handling and no feedback: if the copy fails it becomes an unhandled rejection and the user sees nothing.copyAllRowsin the same file shows the toast/alert pattern we use elsewhere.- A connection declaring only
sql_dialect: "generic"(or an unknown name, which the backend maps togeneric) counts as declared, so the action shows and strips with guessed quoting rules. On a backslash-escape engine declared asgeneric,SELECT 'x\' -- y' AS a, b FROM tloses real SQL.PLUGIN_GUIDE.mdtreats "only generic" as undeclared, so I'd do the same here unless the plugin also providessql_syntax. /*+and--+are kept in every dialect, including SQLite and SQL Server where they're just comments. Safe direction, so fine, it could just be limited to the dialects that have hints.- The description is a bit out of date: it still says only the English string was added and lists 19 cases, and it doesn't say how plugin drivers behave (
.rules/plugins.mdasks for that).
I'll do my own checks in the app before merging.
What
Adds a Copy SQL Without Comments editor context-menu action (issue #1161). It copies the current selection — or the whole document when there is no selection — with ordinary line and block comments removed.
Why
Sharing a statement often means manually deleting explanatory comments while being careful not to touch strings or executable SQL annotations. This does that in one action.
How
src/utils/stripSqlComments.tsreuses the existing SQL tokenizer (scanToken) so the quote/comment dialect rules stay defined in one place. It:/*! … */and MariaDB/*M! … */);scanNonCodeSpans), and uses the tab's declared dialect — not a hardcoded driver id.src/pages/Editor.tsxregisters the action next to the existing editor actions and copies via the existingcopyTextToClipboard. It reads the editor model and never mutates the document.editor.copyWithoutComments); other locales fall back to English, consistent with the current partial locale files.Tests
tests/utils/stripSqlComments.test.ts(19 cases): line/block/#/nested comments,SELECT 1--1subtraction vs--comment per dialect, strings and identifiers that contain comment markers, dollar-quoted bodies,/*!and/*M!executable comments kept, multi-statement input, unknown/undefined dialect pass-through, and the no-merge spacing rule.Validation (run locally)
pnpm exec vitest run tests/utils/stripSqlComments.test.ts→ 19 passedpnpm exec tsc -b→ cleanpnpm exec eslint src/pages/Editor.tsx src/utils/stripSqlComments.ts→ 0 errorsnode scripts/check-theme-tokens.mjs→ OKpnpm exec vitest run tests/utils/sqlAnalysis.test.ts tests/utils/sql→ 523 passedNote: the repo's full
vitestrun needs the jsdom environment, which requires Node ≥ 22.12 (require(ESM)); I ran on Node 22.20. The new tests are environment-agnostic.