refactor(schema): convert the schema slice to signals (finishes #88) (#91)

CHANGELOG.md

@@ -24,10 +24,13 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 - State reactivity now uses `@preact/signals-core` (the third bundled runtime
   dependency), adopted incrementally per
   [ADR-0001](docs/ADR-0001-reactivity.md): the tab list, side panel, run state
-  (`running`/`resultView`), and the library title repaint via signal `effect`s
-  instead of manual render calls. No user-facing behavior change. A Preact
-  schema-panel spike was evaluated and **rejected** — the app stays
-  framework-free (ADR-0001 addendum). (#88)
+  (`running`/`resultView`), the library title, and now the **schema panel**
+  (`schema`/`schemaError`/`schemaFilter`) repaint via signal `effect`s instead of
+  manual render calls. No user-facing behavior change. A Preact schema-panel spike
+  was evaluated and **rejected** — the app stays framework-free (ADR-0001
+  addendum); the schema slice is the documented imperative exception, converted
+  with a *replaced* Set-valued `expanded` signal and reference-replaced column
+  loads rather than in-place mutation. This **completes the migration**. (#88, #91)
 
 ### Fixed
 - The fullscreen schema / EXPLAIN graph panels were mis-sized on **Safari** (#70).

docs/ADR-0001-reactivity.md

@@ -152,3 +152,20 @@ maintenance drag, convert it with signals-core using *replaced* Set/Map-valued
 signals rather than in-place mutation). The `spike/preact-schema` branch stands
 as the evidence; re-open the decision only when several complex, interdependent,
 rich-local-state panels are actually on the roadmap (per the trigger above).
+
+## Addendum — schema slice landed via signals-core (#91, completes #88)
+
+The schema slice was converted with signals-core exactly as the fallback above
+prescribed — **no Preact**. `schema`/`schemaError`/`schemaFilter` are now
+`signal(...)`; the in-place anti-pattern is gone two ways: per-row expand state
+moved from the mutated `db.expanded` bool + `expandedTables` Set into a single
+**Set-valued `expanded` signal** (keys `db:`/`tb:`), updated copy-on-write; and
+lazy column loads **replace the `schema` reference** (`{...tb, columns}`) instead
+of mutating `tb.columns` in place — `tb.columns` stays the completion cache
+`buildCompletions` reads, so `completions.js` was untouched (lower churn than a
+separate Map-valued signal; the gate held). Two `effect()`s in `createApp`
+(schema tree + error banner) replaced every scattered `renderSchema()` call; the
+expand+first-fetch is wrapped in `batch()` so the row opens with its spinner in
+one repaint. This was the last slice — **#88 is complete**. The
+re-evaluation trigger in #91 still applies: if reference-replacement proves as
+forgettable as the old manual `renderSchema` calls, revisit via a fresh ADR.

src/state.js

@@ -55,10 +55,17 @@ export function createState(read = { loadJSON, loadStr }) {
     // the results pane + Run button react to resultView/running (below).
     tabs: signal([newTabObj('t1')]),
     activeTabId: signal('t1'),
-    schema: null,
-    schemaError: null,
-    schemaFilter: '',
-    expandedTables: new Set(),
+    // Schema panel (signals): the tree repaints via an effect in createApp that
+    // reads these (no manual renderSchema list). `schema` is the db→table array;
+    // each `tb.columns` is a lazily-loaded completion cache replaced by reference
+    // (see loadColumns) — never mutated in place. `expanded` is a Set of expand
+    // keys ('db:'+name / 'tb:'+db.table) replaced copy-on-write. Read/write via
+    // `.value`. (The 'db:'/'tb:' prefixes mirror the dbl-click tracker's keys in
+    // schema.js — a separate store, not shared state.)
+    schema: signal(null),
+    schemaError: signal(null),
+    schemaFilter: signal(''),
+    expanded: signal(new Set()),
     serverVersion: null,
     // Run state (signals): `running` flips the Run button + results pane via
     // effects; `resultView` is the active Table/JSON/Chart tab. Via `.value`.

src/ui/app.js

@@ -309,21 +309,21 @@ export function createApp(env = {}) {
   app.loadSchema = async () => {
     try {
       await ensureConfig();
-      app.state.schema = await ch.loadSchema(chCtx);
-      app.state.schemaError = null;
+      const schema = await ch.loadSchema(chCtx);
+      // One batched write → one repaint (the schema effect + the banner effect
+      // react to these signals; no manual renderSchema/updateBanner needed).
+      batch(() => { app.state.schema.value = schema; app.state.schemaError.value = null; });
     } catch (e) {
-      app.state.schemaError = String((e && e.message) || e);
+      app.state.schemaError.value = String((e && e.message) || e);
     }
     app.rebuildCompletions();
-    renderSchema(app);
-    updateBanner();
   };
   // Editor reference data + autocomplete candidates. Loaded once per connection
   // (the keystroke rule, #25): keywords/functions drive both version-correct
   // highlighting and the autocomplete list; completion then runs client-side.
   app.refData = assembleReferenceData(null); // built-in fallback until loaded
   app.rebuildCompletions = () => {
-    app.completions = buildCompletions(app.refData, app.state.schema);
+    app.completions = buildCompletions(app.refData, app.state.schema.value);
   };
   app.rebuildCompletions();
   // Hover docs (#27) are fetched on demand per entity and cached for reuse —
@@ -356,7 +356,7 @@ export function createApp(env = {}) {
   function updateBanner() {
     const b = app.dom.banner;
     if (!b) return;
-    const err = app.state.schemaError;
+    const err = app.state.schemaError.value;
     if (!err || app._bannerDismissedFor === err) {
       b.style.display = 'none';
       return;
@@ -397,17 +397,26 @@ export function createApp(env = {}) {
     doc.documentElement.style.setProperty('--vp-zoom', String(vp));
   }
   app.applyViewportZoom = applyViewportZoom;
-  async function loadColumns(db, table, tableObj) {
-    tableObj.columns = 'loading';
-    renderSchema(app);
+  // Lazily load a table's columns into the schema signal by REFERENCE (no
+  // in-place mutation): replace the target table object with `{...tb, columns}`.
+  // 'loading' is written synchronously (before the await) so the schema effect
+  // paints the spinner immediately; the result/[] write repaints with the data.
+  // `tb.columns` stays the completion cache that buildCompletions reads.
+  async function loadColumns(db, table) {
+    const setCols = (cols) => {
+      app.state.schema.value = app.state.schema.value.map((d) =>
+        (d.db === db
+          ? { ...d, tables: d.tables.map((t) => (t.name === table ? { ...t, columns: cols } : t)) }
+          : d));
+    };
+    setCols('loading');
     try {
       await ensureConfig();
-      tableObj.columns = await ch.loadColumns(chCtx, db, table, sqlString);
+      setCols(await ch.loadColumns(chCtx, db, table, sqlString));
     } catch {
-      tableObj.columns = [];
+      setCols([]);
     }
     app.rebuildCompletions(); // newly-loaded columns become completion candidates (#26)
-    renderSchema(app);
   }
 
   // --- query run ---------------------------------------------------------
@@ -902,7 +911,7 @@ export function renderApp(app, helpers) {
 
   app.dom.schemaSearchInput = h('input', {
     type: 'text', placeholder: 'Search tables, columns…',
-    oninput: (e) => { state.schemaFilter = e.target.value; renderSchema(app); },
+    oninput: (e) => { state.schemaFilter.value = e.target.value; },
   });
   app.dom.schemaList = h('div', { class: 'schema-list' });
   const schemaPane = h('div', { class: 'side-pane schema-pane', style: { height: state.sideSplitPct + '%', flexShrink: '0', minHeight: '0' } },
@@ -989,7 +998,22 @@ export function renderApp(app, helpers) {
   });
   // The Run button reflects the run state (label + disabled).
   effect(() => app.setRunBtn(app.state.running.value));
-  renderSchema(app);
+  // Reactive repaint of the schema tree — replaces the scattered renderSchema()
+  // calls: re-runs on schema load, load error, filter text, or expand/collapse.
+  // Registered here (post-mount) so app.dom.schemaList already exists; the effect
+  // also runs once now for the initial paint.
+  effect(() => {
+    app.state.schema.value;
+    app.state.schemaError.value;
+    app.state.schemaFilter.value;
+    app.state.expanded.value;
+    renderSchema(app);
+  });
+  // The schema/auth-failure banner reflects schemaError (a separate surface).
+  effect(() => {
+    app.state.schemaError.value;
+    app.updateBanner();
+  });
   // Reactive repaint of the side panel: re-runs when the active panel changes
   // (Library ↔ History). Data-driven repaints (savedQueries/history mutations)
   // still call renderSavedHistory directly until those slices are signals too.

src/ui/schema.js

@@ -1,11 +1,21 @@
 // The schema tree: databases → tables → columns, with a text filter and
 // lazy per-table column loading. Renders into app.dom.schemaList.
 
+import { batch } from '@preact/signals-core';
 import { h } from './dom.js';
 import { Icon } from './icons.js';
 import { formatRows, quoteIdent, qualifyIdent } from '../core/format.js';
 import { IDENT_MIME, SCHEMA_GRAPH_MIME } from './editor.js';
 
+// Copy-on-write expand toggle: returns a new Set with `key` added or removed, so
+// assigning it to the `expanded` signal triggers the repaint effect (signals
+// react to reference changes, never in-place Set mutation).
+const toggleKey = (set, key) => {
+  const next = new Set(set);
+  if (!next.delete(key)) next.add(key);
+  return next;
+};
+
 // Make a tree row a drag source carrying `text` as the schema identifier, so it
 // can be dropped onto the editor (see editor.js drop handler). Click behavior is
 // unaffected — drag is a separate gesture.
@@ -55,49 +65,53 @@ export function renderSchema(app) {
   if (!list) return;
   list.replaceChildren();
   const state = app.state;
+  const schemaError = state.schemaError.value;
+  const schema = state.schema.value;
 
-  if (state.schemaError) {
+  if (schemaError) {
     list.appendChild(h('div', { class: 'schema-empty', style: { color: 'var(--error-fg)' } },
-      'Schema load failed: ' + state.schemaError));
+      'Schema load failed: ' + schemaError));
     return;
   }
-  if (!state.schema) {
+  if (!schema) {
     list.appendChild(h('div', { class: 'schema-empty' }, 'Loading schema…'));
     return;
   }
-  if (state.schema.length === 0) {
+  if (schema.length === 0) {
     list.appendChild(h('div', { class: 'schema-empty' }, 'No databases.'));
     return;
   }
 
-  const filter = state.schemaFilter.trim().toLowerCase();
+  const filter = state.schemaFilter.value.trim().toLowerCase();
   const matches = (s) => !filter || s.toLowerCase().includes(filter);
 
-  for (const db of state.schema) {
+  for (const db of schema) {
     const qdb = quoteIdent(db.db); // SQL-safe db name (reused by the 3 emit sites)
+    const dbKey = 'db:' + db.db;
+    const dbOpen = state.expanded.value.has(dbKey);
     list.appendChild(h('div', {
       class: 'tree-row bold',
       title: 'Click to expand · double-click to insert · shift-click for SHOW CREATE',
       onclick: (e) => {
         if (e.shiftKey) { app.actions.insertCreate('DATABASE ' + qdb); return; }
-        if (isDoubleClick(app, 'db:' + db.db)) { app.actions.insertAtCursor(qdb); return; }
-        db.expanded = !db.expanded;
-        renderSchema(app);
+        if (isDoubleClick(app, dbKey)) { app.actions.insertAtCursor(qdb); return; }
+        state.expanded.value = toggleKey(state.expanded.value, dbKey);
       },
       ...lineageDrag(qdb, { kind: 'db', db: db.db }),
     },
-      ...treeRow(Icon.database(), db.db, String(db.tables.length), { expanded: db.expanded }),
+      ...treeRow(Icon.database(), db.db, String(db.tables.length), { expanded: dbOpen }),
     ));
-    if (!db.expanded) continue;
+    if (!dbOpen) continue;
 
     for (const tb of db.tables) {
       const tableMatch = matches(tb.name);
       const colsHave = Array.isArray(tb.columns) ? tb.columns : [];
       const visibleCols = filter ? colsHave.filter((c) => matches(c.name)) : colsHave;
       if (filter && !tableMatch && visibleCols.length === 0 && tb.columns !== 'loading') continue;
       const key = db.db + '.' + tb.name; // internal identity (Sets, dbl-click tracking)
+      const tbKey = 'tb:' + key;
       const qname = qualifyIdent(db.db, tb.name); // SQL-safe qualified name
-      const isOpen = state.expandedTables.has(key);
+      const isOpen = state.expanded.value.has(tbKey);
       const tbComment = (tb.comment || '').trim();
       const title = tbComment
         ? tbComment + ' · ' + formatRows(tb.total_rows) + ' rows'
@@ -110,11 +124,15 @@ export function renderSchema(app) {
         ...lineageDrag(qname, { kind: 'table', db: db.db, table: tb.name }),
         onclick: (e) => {
           if (e.shiftKey) { app.actions.insertCreate(qname); return; }
-          if (isDoubleClick(app, 'tb:' + key)) { app.actions.replaceEditor('SELECT * FROM ' + qname + ' LIMIT 100'); return; }
-          if (state.expandedTables.has(key)) state.expandedTables.delete(key);
-          else state.expandedTables.add(key);
-          if (state.expandedTables.has(key) && tb.columns == null) app.actions.loadColumns(db.db, tb.name, tb);
-          else renderSchema(app);
+          if (isDoubleClick(app, tbKey)) { app.actions.replaceEditor('SELECT * FROM ' + qname + ' LIMIT 100'); return; }
+          const willOpen = !state.expanded.value.has(tbKey);
+          // Batch the expand + first column fetch so the row opens *with* its
+          // spinner in one repaint (loadColumns' 'loading' write runs synchronously
+          // before its await). Collapse / already-loaded is a single Set write.
+          batch(() => {
+            state.expanded.value = toggleKey(state.expanded.value, tbKey);
+            if (willOpen && tb.columns == null) app.actions.loadColumns(db.db, tb.name);
+          });
         },
       },
         ...treeRow(Icon.table(), tb.name, formatRows(tb.total_rows), { expanded: isOpen, iconColor: 'var(--accent)' }),

tests/unit/app.test.js

@@ -171,7 +171,7 @@ describe('renderApp shell', () => {
     const { app } = rendered();
     app.dom.schemaSearchInput.value = 'foo';
     app.dom.schemaSearchInput.dispatchEvent(new Event('input'));
-    expect(app.state.schemaFilter).toBe('foo');
+    expect(app.state.schemaFilter.value).toBe('foo');
   });
 });
 
@@ -224,7 +224,7 @@ describe('loadVersion / loadSchema', () => {
     const app = createApp(e);
     app.renderApp();
     await new Promise((r) => setTimeout(r));
-    expect(app.state.schemaError).toContain('boom');
+    expect(app.state.schemaError.value).toContain('boom');
   });
 });
 
@@ -248,7 +248,7 @@ describe('loadReference / rebuildCompletions (#25)', () => {
   });
   it('rebuildCompletions folds in already-loaded schema columns', () => {
     const app = createApp(env());
-    app.state.schema = [{ db: 'd', tables: [{ name: 't', columns: [{ name: 'c', type: 'UInt8' }] }] }];
+    app.state.schema.value = [{ db: 'd', tables: [{ name: 't', columns: [{ name: 'c', type: 'UInt8' }] }] }];
     app.rebuildCompletions();
     expect(app.completions.some((c) => c.kind === 'column' && c.label === 'c' && c.parent === 't')).toBe(true);
   });
@@ -262,8 +262,8 @@ describe('loadReference / rebuildCompletions (#25)', () => {
       [(u, sql) => /system\.columns/.test(sql), resp({ json: { data: [{ name: 'id', type: 'UInt64', comment: '' }] } })],
     ]) });
     const app = createApp(e); // no renderApp → loadSchema can't clobber our schema mid-test
-    app.state.schema = [{ db: 'd', expanded: true, tables: [{ name: 't', columns: null }] }];
-    await app.actions.loadColumns('d', 't', app.state.schema[0].tables[0]);
+    app.state.schema.value = [{ db: 'd', tables: [{ name: 't', columns: null }] }];
+    await app.actions.loadColumns('d', 't');
     expect(app.completions.some((c) => c.kind === 'column' && c.label === 'id' && c.parent === 't')).toBe(true);
   });
   it('entityDoc fetches a hover description on demand and caches it (#27)', async () => {
@@ -865,21 +865,26 @@ describe('share + star + columns', () => {
     expect(document.querySelector('.save-popover')).toBeNull(); // committed + closed
     expect(app.state.savedQueries[0].description).toBe('updated reason');
   });
-  it('loadColumns fills the table object', async () => {
+  it('loadColumns fills the target table by reference, leaving siblings untouched', async () => {
     const e = env({ fetch: makeFetch([[(u, sql) => /system\.columns/.test(sql), resp({ json: { data: [{ name: 'id', type: 'UInt64', comment: '' }] } })]]) });
-    const app = createApp(e);
-    app.renderApp();
-    const tbl = { name: 't', columns: null };
-    await app.actions.loadColumns('d', 't', tbl);
-    expect(tbl.columns).toEqual([{ name: 'id', type: 'UInt64', comment: '' }]);
+    const app = createApp(e); // no renderApp → loadSchema can't clobber our seeded schema
+    // Two dbs / two tables so the immutable replace exercises both ternary arms
+    // (non-target db kept, non-target table kept).
+    app.state.schema.value = [
+      { db: 'other', tables: [{ name: 'x', columns: null }] },
+      { db: 'd', tables: [{ name: 's', columns: null }, { name: 't', columns: null }] },
+    ];
+    await app.actions.loadColumns('d', 't');
+    expect(app.state.schema.value[1].tables[1].columns).toEqual([{ name: 'id', type: 'UInt64', comment: '' }]);
+    expect(app.state.schema.value[0].tables[0].columns).toBe(null); // other db untouched
+    expect(app.state.schema.value[1].tables[0].columns).toBe(null); // sibling table untouched
   });
   it('loadColumns falls back to [] on error', async () => {
     const e = env({ fetch: makeFetch([[(u, sql) => /system\.columns/.test(sql), resp({ ok: false, status: 500, text: 'x' })]]) });
     const app = createApp(e);
-    app.renderApp();
-    const tbl = { name: 't', columns: null };
-    await app.actions.loadColumns('d', 't', tbl);
-    expect(tbl.columns).toEqual([]);
+    app.state.schema.value = [{ db: 'd', tables: [{ name: 't', columns: null }] }];
+    await app.actions.loadColumns('d', 't');
+    expect(app.state.schema.value[0].tables[0].columns).toEqual([]);
   });
 });
 
@@ -1053,7 +1058,7 @@ describe('exhaustive controller coverage', () => {
     const app = createApp(e);
     app.renderApp();
     await new Promise((r) => setTimeout(r));
-    expect(app.state.schemaError).toBe('rawfail');
+    expect(app.state.schemaError.value).toBe('rawfail');
   });
 
   it('run uses the performance.now fallback when env.now is absent', async () => {
@@ -1214,7 +1219,7 @@ describe('exhaustive controller coverage', () => {
     app.renderApp();
     app.updateBanner();
     expect(app.dom.banner.style.display).toBe('none'); // no error → hidden
-    app.state.schemaError = 'Token authentication is not configured';
+    app.state.schemaError.value = 'Token authentication is not configured';
     app.updateBanner();
     expect(app.dom.banner.style.display).toBe('');
     expect(app.dom.banner.textContent).toContain('Token authentication is not configured');