Fix/postgres alias column autocomplete#267
Conversation
|
@m-tonon Looks awesome! |
|
@debba you can assign it to me |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Hey @m-tonon — first off, this is genuinely cool work. 🙌 Bringing autocomplete to the notebook view, pulling the registration into a shared useSqlAutocompleteRegistration hook, and especially the #266 fix itself — resolving quoted / mixed-case / schema-qualified identifiers in parseTablesFromQuery — is solid, and the test coverage is great. Really nice.
Two things I'd love to see addressed before merge:
1. (Blocking) Autocomplete dies when an unrelated connection disconnects.
disposeSqlAutocomplete() tears down the single global completion provider, but it's called on every connection's disconnect / health-failure. Repro: open two connections, work in connection A's editor, disconnect B → A's autocomplete goes dead, because A's registration hook doesn't re-run (its deps didn't change). Confirmed locally. The provider should follow the active editor's lifecycle, not get torn down on arbitrary disconnects — see the inline note in DatabaseProvider.
2. (Blocking, UX) Inserts are always fully quoted and always schema-prefixed.
Before this PR autocomplete inserted bare names; now every Postgres insert becomes "public"."users", "id". That's noisier than every reference client: Postgres' own quote_ident(), DataGrip and DBeaver all quote only when needed (reserved word / mixed case / special char) and don't prefix the default schema. Crucially, the #266 fix is about resolving quoted identifiers for lookup — it doesn't require emitting quotes on every insert, so the two can be decoupled. The mixed-case correctness ("AccountEventLog" stays quoted) must stay; only the always-on quoting of plain lowercase names + the forced public. prefix should go.
To support the inline suggestions, I'd add a small helper to src/utils/identifiers.ts (it can't be a one-click suggestion since that file isn't in this diff):
// PostgreSQL folds unquoted identifiers to lowercase and only needs quotes for
// reserved words, mixed case, or special characters — mirroring quote_ident().
const PG_SAFE_IDENTIFIER = /^[a-z_][a-z0-9_$]*$/;
const PG_RESERVED = new Set([
"select","from","where","table","user","order","group","join","and","or",
"as","in","on","by","null","true","false","default","check","column","limit","offset",
]);
/** Like formatSqlIdentifier, but quotes only when the identifier actually needs it. */
export function quoteIdentifierIfNeeded(
identifier: string,
driver: string | null | undefined,
): string {
if (!shouldQuoteIdentifiers(driver)) return identifier; // non-PG: unchanged
if (PG_SAFE_IDENTIFIER.test(identifier) && !PG_RESERVED.has(identifier)) {
return identifier;
}
return quoteIdentifier(identifier, driver);
}The column/table inline suggestions below reference this helper, so they apply as a set. Net result: SELECT id, name FROM users for plain lowercase, SELECT "AccountId" FROM "AccountEventLog" for mixed-case — exactly what you'd want. 🚀
Thanks again — really nice contribution, this is the right direction!
|
Thanks for the review! I'll take a look at the feedback, make the necessary improvements, and push an update soon. 🚀 |
|
Hi @m-tonon ! |
…e identifier formatting for PostgreSQL
|
Hey, both blocking points are fixed: #1 — Autocomplete dying on unrelated disconnect
#2 — Noisy inserts (always quoted + schema-prefixed)
Ready for another look when you have time. |
|
hey @m-tonon thanks for addressing the change request! clould you fix the confilcts? after that i will rereview it! |
m-tonon
force-pushed
the
fix/postgres-alias-column-autocomplete
branch
from
5cb9aec to
7141e00
Compare
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Re-reviewed on the rebased branch (7141e00), pulled locally and tested against a real Postgres schema with mixed-case tables.
The two original blockers are addressed — disposal is now owned by the hook (no more autocomplete dying on a background disconnect 👍), and inserts go through formatSqlIdentifier for smart quoting. But I hit a regression that blocks merge:
On Postgres, mixed-case identifiers now insert unquoted — e.g. autocompleting AccountEventLog yields SELECT * FROM AccountEventLog instead of "AccountEventLog", which is invalid SQL (Postgres folds it to lowercase). Verified live: the completion provider receives driver=undefined.
Root cause: Editor.tsx registers the completion provider twice — once via the new useSqlAutocompleteRegistration hook (which passes activeDriver), and once via a leftover useEffect that calls registerSqlAutocomplete(...) without the driver argument. There's a single global provider (last registration wins), so the leftover effect clobbers the hook's registration → driver=undefined → shouldQuoteIdentifiers returns false → nothing gets quoted. Looks like the rebase onto the new main re-introduced the direct registration this PR had replaced.
Fix: remove that leftover effect — the hook already covers registration (with the driver). Details inline. After deleting it locally I re-confirmed drv becomes "postgres" and AccountEventLog inserts as "AccountEventLog". ✅
…tion When a completion's insertText became a fully quoted identifier, typing an opening quote first (which Monaco auto-closes) produced ""Name" because the replacement range only covered the bare word. Expanding the range to swallow surrounding quotes broke filtering (Monaco matched items against the leading quote and hid them all). Now, when an opening quote precedes the range, swallow the surrounding quote(s), emit a fully quoted identifier, and set filterText to the same quoted form so suggestions still match. Canonical result for 0, 1 or 2 surrounding quotes.
|
Hey @m-tonon, thanks again for this — works great. While testing on Postgres I hit one edge case: typing an opening To avoid a review ping-pong round, I pushed a fix straight to your branch ( Could you give it a quick validate on your side before we merge? 🙏 |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
LGTM 🚀 Postgres alias + quoted-identifier autocomplete works end to end (verified on a live demo DB). Full suite green. Approving — just give the quote-completion fix I pushed a quick validate on your side before merge.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Resolved Issues
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 089a335)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 089a335)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Resolved Issues
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit 2daa3ec)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Reviewed by kimi-k2.6-20260420 · Input: 33.1K · Output: 506 · Cached: 40.4K |
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
3 participants
This PR implements a shared SQL autocomplete registration flow for both the editor and notebook views, improving lifecycle management and preventing stale or duplicate Monaco providers.
Changes:
useSqlAutocompleteRegistrationhook to manage SQL autocomplete for active connectionsNotebookViewto utilize the new hook, passing the effective schema and active stateEditorto replace direct SQL autocomplete registration with the hookdisposeSqlAutocompleteto clean up resources on connection changes, reconnect, and failuresfromPatternwith asplitTopLevelCommas()pre-processing step that only splits at depth-0 commasCloses #266
Built for NewtTheWolf by Kilo