feat: Db settings and connections editing - #492

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/db-settings/src/components/DbConnectionForm.tsx`:
- Around line 70-85: The effect that captures originalRef only on the first
truthy editing causes stale originals when the parent reuses the form for a
different connection; update the useEffect watching editing so that when editing
becomes truthy you set originalRef.current = editing if originalRef.current is
unset OR originalRef.current.id !== editing.id (i.e., capture the new record),
and when editing is falsy clear originalRef.current and reset
hasInitializedRef.current to false; apply the same guard/clear logic to the
other initialization block referenced (the code around lines 128-133) so
handleCancel() will restore the correct connection.

In `@packages/db-settings/src/components/DbSettings.tsx`:
- Around line 30-38: The settings trigger Button in DbSettings (the Button
element that forwards ref and renders <SettingsIcon />) is missing an accessible
name; add an aria-label (or aria-labelledby referencing a nearby visually-hidden
label) to the Button props so screen readers announce its purpose (e.g.,
aria-label="Database settings" or similar), and ensure the label text is
localized/unique if needed; keep the visual icon-only appearance unchanged and
apply the attribute on the Button element that renders <SettingsIcon />.

In `@packages/db-settings/src/DbSettingsSlice.ts`:
- Around line 212-230: The loop that evicts stale connections only checks
whether an existing connection is in desiredIds, but does not remove entries
whose diagnostics flipped to unavailable; update the first loop over
existingConnections (where existing.isCore is checked) to also look up
diagnosticsByKey using diagnosticsKey(existing.id, existing.engineId) and call
state.db.connectors.removeConnection(existing.id) when the diagnostic exists and
diag.available === false, so unavailable connections are removed even if their
id is still in desiredIds.
- Around line 117-148: The code captures the pre-request connections in
saveToServer but later rebuilds savedSnapshot from the live store, which can
fold in in-flight edits; change the success branch to compute savedSnapshot from
the captured connections variable (use savedSnapshot =
connectionsSnapshot(connections)) and then set
draft.dbSettings.hasUnsavedChanges based on comparing the current
draft.dbSettings.config.connections to that captured snapshot (or set to false
only when they match) so in-flight edits are not incorrectly marked saved;
update references in the success block where savedSnapshot and
draft.dbSettings.hasUnsavedChanges are set.

In `@python/sqlrooms-cli/sqlrooms/web/db_bridge/connectors/postgres.py`:
- Around line 20-22: The resolve_dsn method currently interpolates self.password
directly into the DSN which breaks when the password contains characters like @,
:, /, or %; update resolve_dsn to URL-encode the password (use
urllib.parse.quote) before inserting it into the DSN string, preserving the
existing conditional (pw = f":{encoded_pw}" if self.password else "") and
keeping the rest of the returned
f"postgresql://{self.user}{pw}@{self.host}:{self.port}/{self.database}" intact.

In `@python/sqlrooms-cli/sqlrooms/web/db_bridge/registry.py`:
- Around line 42-44: The code copies connector runtime configs directly via
conn.config_dict() into entry["config"], exposing secrets like password; update
the assignment to sanitize or filter out sensitive keys (e.g., "password",
"secret", "api_key", "token") before storing in entry["config"]—either call a
redact helper (e.g., redact_config(config)) or create a sanitized dict by
removing or masking those keys from the dict returned by conn.config_dict() so
no credentials are sent to the frontend.

In `@python/sqlrooms-cli/sqlrooms/web/launcher.py`:
- Around line 359-388: After successfully writing TOML in put_db_settings (after
_write_db_connectors_to_toml(self.config_path, connections)), reload the runtime
registry so get_db_settings and the connectionId endpoints see the new settings:
either call a reload method on self.db_bridge_registry (e.g.
self.db_bridge_registry.reload_from_config(self.config_path)) or reinitialize it
from the config file (e.g. self.db_bridge_registry =
DbBridgeRegistry(self.config_path) or equivalent), ensuring the refreshed
registry replaces the old one before returning the response; alternatively,
modify get_db_settings to read directly from self.config_path when present
instead of relying on the one-time-built self.db_bridge_registry.
- Around line 77-99: The code currently overlays only non-empty engine_config
fields onto base, so blanking a field in the form leaves the old value in base;
update the loop that processes engine_config (the variable engine_config from
conn.get("config")) to treat explicit empty/blank values as removals: for each
ck, cv in engine_config.items(), if cv is None or cv == "" then remove ck from
base if present (instead of skipping), otherwise set base[ck] = cv; this ensures
connectors_aot built from base no longer retains stale values after a user
clears a field.

In `@python/sqlrooms-cli/tests/test_launcher.py`:
- Around line 105-110: The runtime_connections() response currently includes raw
connector config_dict() values (from PostgresConnector.config_dict and
SnowflakeConnector.to_connect_kwargs), which can leak secrets like "password";
modify registry.py::runtime_connections to apply an explicit allowlist of
non-sensitive keys (e.g., host, port, database, user, maybe account) and filter
each connector.config_dict() through that allowlist before adding it to the API
payload, ensuring any "password" or other secret keys are omitted; then update
the tests in tests/test_launcher.py to assert that sensitive fields (e.g.,
"password") are not present in the returned config objects for connectors.

---

Nitpick comments:
In `@packages/sql-editor/src/components/SchemaExplorer.tsx`:
- Around line 49-54: The fallback "?? []" in SchemaExplorerTree creates a new
array each render causing TableSchemaTree to rerender; fix by returning a stable
empty array instead of a new literal when schemaTrees is undefined—e.g., define
a shared constant (EMPTY_SCHEMA_TREES) or memoize the fallback and pass
schemaTrees ?? EMPTY_SCHEMA_TREES to TableSchemaTree; adjust SchemaExplorerTree
(and any related props) so TableSchemaTree receives a stable reference to avoid
unnecessary re-renders.

In `@python/sqlrooms-cli/tests/test_cli.py`:
- Around line 54-57: Add an assertion that the parsed Postgres config includes
the port value by checking data[0].port equals the expected port from the test
fixture (e.g., assert data[0].port == <expected_port>); update the assertion
alongside the existing checks for data[0].host, data[0].database, data[0].user,
and data[0].password so the test validates the new field-based parsing contract
for the Postgres config.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/sqlrooms-cli/sqlrooms/cli.py`:
- Line 414: The current call passes the loaded config_path through
(config_path=config_path) which lets fresh installs treat absence of a file the
same as an explicit opt-out; change the logic so you preserve two distinct
values: one "loaded_config_path" (the path found or created) used for reading
and a separate "save_config_path" that is None only when the user passed the
explicit --no-config flag; update the call site that currently uses
config_path=config_path to use save_config_path for writable targets and ensure
functions/classes that accept config_path (e.g., the initializer or function
named with parameter config_path) continue to receive None only when user opted
out and otherwise receive a concrete filesystem path for persistence.
- Around line 116-123: The port value can be an int from tomllib, so calling
_normalize_config_string(item.get("port")) returns None and falls back to
"5432"; update the code that constructs the PostgresConnectorSettings (or
_normalize_config_string) to accept integer ports by converting item.get("port")
to a string before normalization (e.g., coerce via str() when item.get("port")
is not None and not already a str) so the actual configured port (e.g., 6543) is
preserved instead of silently defaulting; reference PostgresConnectorSettings
and _normalize_config_string to locate the change.

In `@python/sqlrooms-cli/sqlrooms/web/launcher.py`:
- Around line 78-97: When building each connection entry in the loop over
connections, detect if the incoming conn's engine identifier (e.g.
conn.get("engineId") or conn.get("engine_id")) differs from the stored entry in
existing_by_id for that conn_id; if it does, don't reuse the old entry
fields—reset base to only include non-engine/shared fields (or start with an
empty dict) before merging frontend_to_toml keys and engine_config. Update the
logic around existing_by_id, base and engine_config so that when engineId
changes you clear engine-specific keys (e.g. Snowflake-only keys like account,
warehouse, schema) instead of carrying them forward into the new TOML entry.

---

Duplicate comments:
In `@python/sqlrooms-cli/sqlrooms/web/launcher.py`:
- Around line 385-392: The handler currently writes the TOML via
_write_db_connectors_to_toml but does not update the in-memory registry built in
__init__, so get_db_settings() and connectionId-based endpoints keep using the
stale self.db_bridge_registry; after a successful write (the block that returns
{"ok": True, "configPath": ...}), reload or rebuild self.db_bridge_registry
(e.g., call the registry-loading method used in __init__ or a dedicated refresh
method) so the in-memory registry reflects the newly saved connections and
subsequent get_db_settings()/connectionId endpoints return the updated data.

---

Nitpick comments:
In `@packages/db-settings/src/components/DbConnectionForm.tsx`:
- Around line 147-162: The handleTestConnection function can call
setTestState/setTestError after the component unmounts; add a mounted guard or
AbortController to prevent state updates when unmounted: create a ref/flag
(e.g., isMounted) or an AbortController in the component, set it true on mount
and false/abort on cleanup, then check the flag or signal before calling
setTestState/setTestError (and pass the signal to testConnection if using
AbortController) so any in-flight testConnection results are ignored when the
form unmounts.
- Around line 91-111: The effect that auto-syncs form edits into the store (uses
isEditing, suppressSyncRef, buildConnection) currently disables exhaustive-deps;
add upsertConnection to the dependency array so the effect explicitly depends on
the store action (upsertConnection) while retaining the existing suppression
behavior for editing via suppressSyncRef to avoid the prop/state loop.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/sqlrooms-cli/tests/test_launcher.py`:
- Around line 299-302: Replace the direct import of tomllib in test_launcher.py
with the same compatibility pattern used in cli.py: try to import tomllib, and
on ImportError import tomli as tomllib so the subsequent call doc =
tomllib.load(fh) works on Python 3.10; update the top of the test (where tomllib
is imported and doc = tomllib.load(fh) is used) to use that try/except import
fallback so tomli is used when tomllib is unavailable.

---

Duplicate comments:
In `@python/sqlrooms-cli/sqlrooms/web/launcher.py`:
- Around line 389-411: After successfully writing connectors with
_write_db_connectors_to_toml inside put_db_settings, refresh the in-memory
registry so runtime reads the new connectors: call the same logic used at
startup to rebuild self.db_bridge_registry (e.g., invoke an existing initializer
like self._init_db_bridge_registry() or add a small helper
refresh_db_bridge_registry(config_path) that re-parses the toml and repopulates
self.db_bridge_registry) before returning {"ok": True, ...}; alternatively
change the GET /api/db/settings handler to read directly from self.config_path
on each request so it always returns the on-disk state.

---

Nitpick comments:
In `@python/sqlrooms-cli/sqlrooms/web/launcher.py`:
- Around line 129-134: Remove the redundant local import of the regex module
inside the block that serializes the toml document: delete the inline "import
re" and rely on the module-level "re" import already present; ensure the code
that uses re.sub on the serialized "raw" string (around tomlkit.dumps and
config_path.write_text) still references the top-level re import so the
collapse-of-blank-lines logic continues to work.