Fix/postgres alias column autocomplete

[m-tonon]

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:

• Added useSqlAutocompleteRegistration hook to manage SQL autocomplete for active connections
• Updated NotebookView to utilize the new hook, passing the effective schema and active state
• Refactored Editor to replace direct SQL autocomplete registration with the hook
• Introduced disposeSqlAutocomplete to clean up resources on connection changes, reconnect, and failures
• Added support for schema-qualified table names, double-quoted identifiers, and alias resolution in PostgreSQL
• Added regression tests for schema-qualified tables, quoted identifiers, and autocomplete disposal

---

Closes #266

[debba]

@m-tonon Looks awesome!
I will check ASAP

[NewtTheWolf]

@debba you can assign it to me

[NewtTheWolf]

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!

[NewtTheWolf]

The completion session is already scoped to the active schema (getTableColumns(..., schema)), so hard-qualifying every table as "public"."table" is redundant and noisy. Drop the schema prefix and quote only when needed (pairs with the call-site suggestion below + the quoteIdentifierIfNeeded helper from the review body):

Suggested change

	const tableInsertText = (
	tableName: string,
	driver?: string | null,
	schema?: string | null,
	) =>
	schema
	? quoteTableRef(tableName, driver, schema)
	: formatSqlIdentifier(tableName, driver);
	const tableInsertText = (
	tableName: string,
	driver?: string | null,
	) =>
	// Already scoped to the active schema — don't prefix "public". on every
	// table. Quote only when the name actually requires it.
	quoteIdentifierIfNeeded(tableName, driver);

If you want cross-schema qualification later, qualify only when schema !== defaultSchema rather than always.

[NewtTheWolf]

Match the simplified tableInsertText signature (no schema arg):

Suggested change

	insertText: tableInsertText(t.name, driver, schema),
	insertText: tableInsertText(t.name, driver),

quoteTableRef then becomes unused in this file — drop it from the import on line 1.

[NewtTheWolf]

Columns get the same always-quote treatment — "id", "CreatedAt". Quote only when needed so plain lowercase columns stay clean while "AccountId" stays correctly quoted:

Suggested change

	insertText: formatSqlIdentifier(c.label, driver),
	insertText: quoteIdentifierIfNeeded(c.label, driver),

[NewtTheWolf]

Same for the context-column insert path:

Suggested change

	insertText: formatSqlIdentifier(col.label, driver),
	insertText: quoteIdentifierIfNeeded(col.label, driver),

[NewtTheWolf]

This is the root cause of point #1 above. The global provider mirrors the active editor, so only dispose it when the connection being torn down is actually the active one — otherwise disconnecting a background connection kills autocomplete in the editor you're using:

Suggested change

	disposeSqlAutocomplete();
	if (targetId === activeConnectionId) {
	disposeSqlAutocomplete();
	}

The health-check handler at L786 has the same issue, but there activeConnectionId is a stale closure — you'd need an activeConnectionIdRef (or move disposal into the active-connection-change path). Cleanest long-term: let useSqlAutocompleteRegistration own disposal in its cleanup and drop both calls from here entirely.

[m-tonon]

Thanks for the review! I'll take a look at the feedback, make the necessary improvements, and push an update soon. 🚀

[debba]

Hi @m-tonon !
any news about it?

[m-tonon]

Hey, both blocking points are fixed:

#1 — Autocomplete dying on unrelated disconnect

• Autocomplete no longer shuts down when a background connection drops
• Only the active editor's cleanup controls disposal now

#2 — Noisy inserts (always quoted + schema-prefixed)

• Smart quoting: users stays plain, "AccountEventLog" gets quotes
• No more "public"."table" prefix on inserts
• Uses existing formatSqlIdentifier (no extra helper)

Ready for another look when you have time.

[NewtTheWolf]

hey @m-tonon thanks for addressing the change request!

clould you fix the confilcts? after that i will rereview it!

[NewtTheWolf]

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". ✅

[NewtTheWolf]

This leftover effect (L2196-L2214) duplicates the useSqlAutocompleteRegistration hook added just above and — critically — calls registerSqlAutocomplete without the driver argument, so the provider ends up registered with driver=undefined. Since it's the last write to the single global provider, it overrides the hook and breaks smart-quoting (mixed-case identifiers insert unquoted).

Please remove this whole effect — the hook already handles registration with the driver. That also makes the registerSqlAutocomplete import (L86) unused, so it can go too.