From 37de9452bf9049586a6ebc4f2a6d9799e74f6b83 Mon Sep 17 00:00:00 2001 From: Isolator acm Date: Sun, 21 Jun 2026 23:38:46 +0200 Subject: [PATCH] refactor: de-duplicate SVG building, tree rows, and anchored popovers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavior-preserving cleanup (no UX change), net -28 src lines, smaller bundle. - dom.js: add s() — an SVG-namespace sibling of h() sharing one prop/children applier; class is set via setAttribute so it works for SVG too. - icons.js: collapse svg/svgFilled/iconEl onto s(); drop the repeated createElementNS + setAttribute blocks. - results.js: build the bar chart (svg/line/text/rect/title) with s() instead of local createElementNS builders. - schema.js: factor the shared chevron/icon/label/meta row into treeRow(); use it for the db, table, and column rows. - app.js: extract anchoredPopover() for the shared open/close/anchor/Esc/ outside-click lifecycle; openSavePopover and openUserMenu delegate to it. - state.js: makeId(prefix, now) + tabsForSaved(state, id) replace the repeated id-minting and tab-relink loops. Tests: all green at the per-file gate (dom/icons/results/schema at 100%); added s() coverage to dom.test.js. Verified e2e on otel — schema tree expand, bar chart, Save popover (prefill+focus, outside-click close), and user menu (Esc close) all render identically. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef --- src/state.js | 12 +++++---- src/ui/app.js | 60 ++++++++++++++++++++---------------------- src/ui/dom.js | 27 ++++++++++++++----- src/ui/icons.js | 48 +++++++++------------------------ src/ui/results.js | 41 ++++++++--------------------- src/ui/schema.js | 24 ++++++++--------- tests/unit/dom.test.js | 21 ++++++++++++++- 7 files changed, 112 insertions(+), 121 deletions(-) diff --git a/src/state.js b/src/state.js index ff3869c..68ce12e 100644 --- a/src/state.js +++ b/src/state.js @@ -65,6 +65,8 @@ export function allocTabId(state) { } const rnd = () => Math.random().toString(36).slice(2, 6); +const makeId = (prefix, now) => prefix + now + rnd(); +const tabsForSaved = (state, id) => state.tabs.filter((t) => t.savedId === id); /** The saved query a tab is linked to (via tab.savedId), or null. */ export function savedForTab(state, tab) { @@ -86,7 +88,7 @@ export function saveQuery(state, tab, name, save = saveJSON, now = Date.now()) { entry.name = nm; entry.sql = sql; } else { - entry = { id: 's' + now + rnd(), name: nm, sql, favorite: false }; + entry = { id: makeId('s', now), name: nm, sql, favorite: false }; state.savedQueries.unshift(entry); tab.savedId = entry.id; } @@ -101,7 +103,7 @@ export function renameSaved(state, id, name, save = saveJSON) { const entry = state.savedQueries.find((q) => q.id === id); if (!entry || !nm) return; entry.name = nm; - for (const t of state.tabs) if (t.savedId === id) t.name = nm; + for (const t of tabsForSaved(state, id)) t.name = nm; save(KEYS.saved, state.savedQueries); } @@ -125,7 +127,7 @@ export function sortedSaved(state) { * Merge imported queries into savedQueries (dedupe by content, update by id, * else add). Returns { added, updated, skipped }. */ -export function importSaved(state, queries, save = saveJSON, genId = () => 's' + Date.now() + rnd()) { +export function importSaved(state, queries, save = saveJSON, genId = () => makeId('s', Date.now())) { const { merged, added, updated, skipped } = mergeSaved(state.savedQueries, queries, genId); state.savedQueries = merged; save(KEYS.saved, state.savedQueries); @@ -135,7 +137,7 @@ export function importSaved(state, queries, save = saveJSON, genId = () => 's' + /** Delete a saved query by id and clear any tab pointer to it. */ export function deleteSaved(state, id, save = saveJSON) { state.savedQueries = state.savedQueries.filter((q) => q.id !== id); - for (const t of state.tabs) if (t.savedId === id) t.savedId = null; + for (const t of tabsForSaved(state, id)) t.savedId = null; save(KEYS.saved, state.savedQueries); } @@ -144,7 +146,7 @@ export function recordHistory(state, tab, save = saveJSON, now = Date.now()) { const sql = String(tab.sql || '').trim(); if (!sql) return; state.history.unshift({ - id: 'h' + now + rnd(), + id: makeId('h', now), sql, ts: now, rows: tab.result.rawText != null ? null : tab.result.rows.length, diff --git a/src/ui/app.js b/src/ui/app.js index 045858c..8af225f 100644 --- a/src/ui/app.js +++ b/src/ui/app.js @@ -419,6 +419,30 @@ export function createApp(env = {}) { app.dom.saveBtn.replaceChildren(Icon.bookmark(), h('span', null, clean ? 'Saved' : 'Save')); app.dom.saveBtn.title = clean ? 'Saved — edit to re-save (⌘S)' : 'Save query (⌘S)'; }; + // Open `node` as a popover anchored under `anchorEl`: fixed-position below the + // button, Esc + click-outside close (capture listeners), stored at + // app.dom[refKey] and cleared on close. Returns { close }. + function anchoredPopover(node, anchorEl, refKey) { + const close = () => { + doc.removeEventListener('keydown', onKey, true); + doc.removeEventListener('mousedown', onOutside, true); + if (app.dom[refKey]) { app.dom[refKey].remove(); app.dom[refKey] = null; } + }; + const onKey = (e) => { if (e.key === 'Escape') { e.preventDefault(); close(); } }; + const onOutside = (e) => { + if (app.dom[refKey] && !node.contains(e.target) && !anchorEl.contains(e.target)) close(); + }; + app.dom[refKey] = node; + const r = anchorEl.getBoundingClientRect(); + node.style.position = 'fixed'; + node.style.top = (r.bottom + 6) + 'px'; + node.style.right = Math.max(8, (win.innerWidth || 0) - r.right) + 'px'; + doc.body.appendChild(node); + doc.addEventListener('keydown', onKey, true); + doc.addEventListener('mousedown', onOutside, true); + return { close }; + } + // Name popover anchored under the Save button. Prefill with the tab's name (or // a name inferred from the SQL); Enter/Save → saveQuery (create or update in // place) + relink the tab; Esc / click-outside cancels. @@ -429,11 +453,7 @@ export function createApp(env = {}) { const entry = savedForTab(app.state, tab); const prefill = entry ? entry.name : (tab.name && tab.name !== 'Untitled' ? tab.name : inferQueryName(tab.sql)); const input = h('input', { class: 'sp-input', value: prefill }); - const close = () => { - doc.removeEventListener('keydown', onKey, true); - doc.removeEventListener('mousedown', onOutside, true); - if (app.dom.savePopover) { app.dom.savePopover.remove(); app.dom.savePopover = null; } - }; + let close; const commit = () => { if (!input.value.trim()) return; saveQuery(app.state, tab, input.value, saveJSON); @@ -443,23 +463,14 @@ export function createApp(env = {}) { renderSavedHistory(app); flashToast('Saved', { document: doc }); }; - const onKey = (e) => { if (e.key === 'Escape') { e.preventDefault(); close(); } }; - const onOutside = (e) => { if (app.dom.savePopover && !app.dom.savePopover.contains(e.target) && e.target !== app.dom.saveBtn) close(); }; input.addEventListener('keydown', (e) => { if (e.key === 'Enter') { e.preventDefault(); commit(); } }); const pop = h('div', { class: 'save-popover' }, h('div', { class: 'sp-label' }, 'Save query as'), input, h('div', { class: 'sp-actions' }, - h('button', { class: 'sp-cancel', onclick: close }, 'Cancel'), + h('button', { class: 'sp-cancel', onclick: () => close() }, 'Cancel'), h('button', { class: 'sp-save', onclick: commit }, 'Save'))); - app.dom.savePopover = pop; - const r = app.dom.saveBtn.getBoundingClientRect(); - pop.style.position = 'fixed'; - pop.style.top = (r.bottom + 6) + 'px'; - pop.style.right = Math.max(8, (win.innerWidth || 0) - r.right) + 'px'; - doc.body.appendChild(pop); - doc.addEventListener('keydown', onKey, true); - doc.addEventListener('mousedown', onOutside, true); + ({ close } = anchoredPopover(pop, app.dom.saveBtn, 'savePopover')); setTimeout(() => { input.focus(); input.select(); }); } app.openSavePopover = openSavePopover; @@ -468,24 +479,11 @@ export function createApp(env = {}) { // a Log out item. Same close model as the save popover (Esc + outside click). function openUserMenu() { if (app.dom.userMenu) return; - const close = () => { - doc.removeEventListener('keydown', onKey, true); - doc.removeEventListener('mousedown', onOutside, true); - if (app.dom.userMenu) { app.dom.userMenu.remove(); app.dom.userMenu = null; } - }; - const onKey = (e) => { if (e.key === 'Escape') { e.preventDefault(); close(); } }; - const onOutside = (e) => { if (app.dom.userMenu && !app.dom.userMenu.contains(e.target) && e.target !== app.dom.userBtn && !app.dom.userBtn.contains(e.target)) close(); }; + let close; const menu = h('div', { class: 'user-menu' }, h('div', { class: 'um-id' }, app.email()), h('button', { class: 'um-item danger', onclick: () => { close(); app.signOut(); } }, Icon.logout(), h('span', null, 'Log out'))); - app.dom.userMenu = menu; - const r = app.dom.userBtn.getBoundingClientRect(); - menu.style.position = 'fixed'; - menu.style.top = (r.bottom + 6) + 'px'; - menu.style.right = Math.max(8, (win.innerWidth || 0) - r.right) + 'px'; - doc.body.appendChild(menu); - doc.addEventListener('keydown', onKey, true); - doc.addEventListener('mousedown', onOutside, true); + ({ close } = anchoredPopover(menu, app.dom.userBtn, 'userMenu')); } app.openUserMenu = openUserMenu; diff --git a/src/ui/dom.js b/src/ui/dom.js index 0490f62..5d53d66 100644 --- a/src/ui/dom.js +++ b/src/ui/dom.js @@ -1,16 +1,19 @@ -// Minimal hyperscript helper. `h(tag, props, ...children)` builds a DOM node. -// Supports function components, style objects, class/className, raw html, -// on* event listeners, boolean/null skipping, and nested/array children. +// Minimal hyperscript helper. `h(tag, props, ...children)` builds a DOM node; +// `s(tag, ...)` is the same in the SVG namespace. Both support function +// components (h only), style objects, class/className, raw html, on* event +// listeners, boolean/null skipping, and nested/array children. -export function h(tag, props, ...children) { - if (typeof tag === 'function') return tag(props || {}, children); - const el = document.createElement(tag); +const SVG_NS = 'http://www.w3.org/2000/svg'; + +// Shared prop/children application — the only difference between h and s is +// which document factory creates the element. +function apply(el, props, children) { if (props) { for (const k in props) { const v = props[k]; if (v == null || v === false) continue; if (k === 'style' && typeof v === 'object') Object.assign(el.style, v); - else if (k === 'class' || k === 'className') el.className = v; + else if (k === 'class' || k === 'className') el.setAttribute('class', v); else if (k === 'html') el.innerHTML = v; else if (k.startsWith('on') && typeof v === 'function') { el.addEventListener(k.slice(2).toLowerCase(), v); @@ -23,3 +26,13 @@ export function h(tag, props, ...children) { } return el; } + +export function h(tag, props, ...children) { + if (typeof tag === 'function') return tag(props || {}, children); + return apply(document.createElement(tag), props, children); +} + +// Build an element in the SVG namespace (same prop rules as h()). +export function s(tag, props, ...children) { + return apply(document.createElementNS(SVG_NS, tag), props, children); +} diff --git a/src/ui/icons.js b/src/ui/icons.js index 39eaf35..7ce7917 100644 --- a/src/ui/icons.js +++ b/src/ui/icons.js @@ -1,54 +1,32 @@ // Inline SVG icons. `svg`/`svgFilled` build single-path icons; `iconEl` builds // multi-element icons from an innerHTML body. `Icon` is the named set the UI -// uses. All return detached SVG elements. +// uses. All return detached SVG elements (built via the `s()` SVG hyperscript). -const NS = 'http://www.w3.org/2000/svg'; +import { s } from './dom.js'; + +// Shared stroke attributes for the outlined icons. +const stroked = (stroke) => ({ + stroke: 'currentColor', 'stroke-width': stroke, + 'stroke-linecap': 'round', 'stroke-linejoin': 'round', +}); /** Single-path stroked icon. */ export function svg(d, w = 12, hgt = 12, opts = {}) { const { stroke = 1.4, fill = 'none' } = opts; - const el = document.createElementNS(NS, 'svg'); - el.setAttribute('width', w); - el.setAttribute('height', hgt); - el.setAttribute('viewBox', `0 0 ${w} ${hgt}`); - if (fill) el.setAttribute('fill', fill); - el.setAttribute('stroke', 'currentColor'); - el.setAttribute('stroke-width', stroke); - el.setAttribute('stroke-linecap', 'round'); - el.setAttribute('stroke-linejoin', 'round'); - const path = document.createElementNS(NS, 'path'); - path.setAttribute('d', d); - el.appendChild(path); - return el; + return s('svg', { width: w, height: hgt, viewBox: `0 0 ${w} ${hgt}`, fill: fill || null, ...stroked(stroke) }, + s('path', { d })); } /** Single-path filled icon. `vbW`/`vbH` default to the display size, but can * differ when the path is authored in a different coordinate space. */ export function svgFilled(d, w = 12, hgt = 12, vbW = w, vbH = hgt) { - const el = document.createElementNS(NS, 'svg'); - el.setAttribute('width', w); - el.setAttribute('height', hgt); - el.setAttribute('viewBox', `0 0 ${vbW} ${vbH}`); - el.setAttribute('fill', 'currentColor'); - const path = document.createElementNS(NS, 'path'); - path.setAttribute('d', d); - el.appendChild(path); - return el; + return s('svg', { width: w, height: hgt, viewBox: `0 0 ${vbW} ${vbH}`, fill: 'currentColor' }, + s('path', { d })); } /** Multi-element stroked icon from an innerHTML body. */ export function iconEl(body, w = 14, hgt = 14, stroke = 1.4) { - const el = document.createElementNS(NS, 'svg'); - el.setAttribute('width', w); - el.setAttribute('height', hgt); - el.setAttribute('viewBox', `0 0 ${w} ${hgt}`); - el.setAttribute('fill', 'none'); - el.setAttribute('stroke', 'currentColor'); - el.setAttribute('stroke-width', stroke); - el.setAttribute('stroke-linecap', 'round'); - el.setAttribute('stroke-linejoin', 'round'); - el.innerHTML = body; - return el; + return s('svg', { width: w, height: hgt, viewBox: `0 0 ${w} ${hgt}`, fill: 'none', ...stroked(stroke), html: body }); } export const Icon = { diff --git a/src/ui/results.js b/src/ui/results.js index 34fc531..68ca836 100644 --- a/src/ui/results.js +++ b/src/ui/results.js @@ -2,7 +2,7 @@ // view for TSV/JSON output) plus the renderers. Heavy logic (sorting, axis // selection) lives in core/ and is reused here. -import { h } from './dom.js'; +import { h, s } from './dom.js'; import { Icon } from './icons.js'; import { formatRows, formatBytes, isNumericType } from '../core/format.js'; import { looksLikeHtml, prettyValue } from '../core/cell.js'; @@ -10,7 +10,6 @@ import { sortRows } from '../core/sort.js'; import { pickChartAxes, chartSeries } from '../core/chart-data.js'; const VIS_CAP = 5000; -const NS = 'http://www.w3.org/2000/svg'; const MIN_COL = 48; // px floor for a resized column /** @@ -292,27 +291,14 @@ export function renderChart(r) { const step = innerW / Math.max(values.length, 1); const barW = step * 0.7; - const svg = document.createElementNS(NS, 'svg'); - svg.setAttribute('viewBox', `0 0 ${W} ${H}`); - svg.setAttribute('preserveAspectRatio', 'xMidYMid meet'); - svg.style.width = '100%'; - svg.style.height = '100%'; + const svg = s('svg', { + viewBox: `0 0 ${W} ${H}`, preserveAspectRatio: 'xMidYMid meet', + style: { width: '100%', height: '100%' }, + }); - const line = (x1, y1, x2, y2) => { - const el = document.createElementNS(NS, 'line'); - el.setAttribute('x1', x1); el.setAttribute('y1', y1); - el.setAttribute('x2', x2); el.setAttribute('y2', y2); - el.setAttribute('stroke', 'currentColor'); el.setAttribute('opacity', '0.25'); - return el; - }; - const text = (x, y, anchor, str) => { - const el = document.createElementNS(NS, 'text'); - el.setAttribute('x', x); el.setAttribute('y', y); - el.setAttribute('text-anchor', anchor); el.setAttribute('font-size', '10'); - el.setAttribute('fill', 'currentColor'); el.setAttribute('opacity', '0.6'); - el.textContent = str; - return el; - }; + const line = (x1, y1, x2, y2) => s('line', { x1, y1, x2, y2, stroke: 'currentColor', opacity: '0.25' }); + const text = (x, y, anchor, str) => + s('text', { x, y, 'text-anchor': anchor, 'font-size': '10', fill: 'currentColor', opacity: '0.6' }, str); svg.appendChild(line(P.l, P.t + innerH, P.l + innerW, P.t + innerH)); svg.appendChild(text(P.l - 6, P.t + 10, 'end', formatRows(max))); @@ -321,14 +307,9 @@ export function renderChart(r) { values.forEach((v, i) => { const x = P.l + i * step + (step - barW) / 2; const hgt = max > 0 ? (v / max) * innerH : 0; - const rect = document.createElementNS(NS, 'rect'); - rect.setAttribute('x', x); rect.setAttribute('y', P.t + innerH - hgt); - rect.setAttribute('width', barW); rect.setAttribute('height', hgt); - rect.setAttribute('fill', 'var(--accent)'); rect.setAttribute('rx', '1.5'); - const t = document.createElementNS(NS, 'title'); - t.textContent = labels[i] + ': ' + v; - rect.appendChild(t); - svg.appendChild(rect); + svg.appendChild(s('rect', { + x, y: P.t + innerH - hgt, width: barW, height: hgt, fill: 'var(--accent)', rx: '1.5', + }, s('title', null, labels[i] + ': ' + v))); }); const every = Math.max(1, Math.ceil(labels.length / 12)); diff --git a/src/ui/schema.js b/src/ui/schema.js index 9abb9fc..954f783 100644 --- a/src/ui/schema.js +++ b/src/ui/schema.js @@ -14,6 +14,15 @@ const dragProps = (text) => ({ ondragstart: (e) => e.dataTransfer.setData(IDENT_MIME, text), }); +// The four spans every tree row shares: chevron, icon, label, meta. `expanded` +// null → an empty chevron (column rows); true/false → the open/closed chevron. +const treeRow = (icon, label, meta, { expanded, iconColor } = {}) => [ + h('span', { class: 'chev' }, expanded == null ? null : (expanded ? Icon.chevDown() : Icon.chev())), + h('span', { class: 'icon', style: iconColor ? { color: iconColor } : null }, icon), + h('span', { class: 'label' }, label), + h('span', { class: 'meta' }, meta), +]; + export function renderSchema(app) { const list = app.dom.schemaList; if (!list) return; @@ -49,10 +58,7 @@ export function renderSchema(app) { ondblclick: (e) => { e.stopPropagation(); app.actions.insertAtCursor(db.db); }, ...dragProps(db.db), }, - h('span', { class: 'chev' }, db.expanded ? Icon.chevDown() : Icon.chev()), - h('span', { class: 'icon' }, Icon.database()), - h('span', { class: 'label' }, db.db), - h('span', { class: 'meta' }, String(db.tables.length)), + ...treeRow(Icon.database(), db.db, String(db.tables.length), { expanded: db.expanded }), )); if (!db.expanded) continue; @@ -82,10 +88,7 @@ export function renderSchema(app) { }, ondblclick: (e) => { e.stopPropagation(); app.actions.insertTopLine('SELECT * FROM ' + key + ' LIMIT 100'); }, }, - h('span', { class: 'chev' }, isOpen ? Icon.chevDown() : Icon.chev()), - h('span', { class: 'icon', style: { color: 'var(--accent)' } }, Icon.table()), - h('span', { class: 'label' }, tb.name), - h('span', { class: 'meta' }, formatRows(tb.total_rows)), + ...treeRow(Icon.table(), tb.name, formatRows(tb.total_rows), { expanded: isOpen, iconColor: 'var(--accent)' }), )); if (!isOpen && !(filter && visibleCols.length > 0)) continue; @@ -106,10 +109,7 @@ export function renderSchema(app) { ondblclick: (e) => { e.stopPropagation(); app.actions.insertAtCursor(c.name); }, ...dragProps(c.name), }, - h('span', { class: 'chev' }), - h('span', { class: 'icon', style: { color: 'var(--fg-faint)' } }, Icon.col()), - h('span', { class: 'label' }, c.name), - h('span', { class: 'meta' }, c.type), + ...treeRow(Icon.col(), c.name, c.type, { expanded: null, iconColor: 'var(--fg-faint)' }), )); } } diff --git a/tests/unit/dom.test.js b/tests/unit/dom.test.js index 76ce043..e4c21e8 100644 --- a/tests/unit/dom.test.js +++ b/tests/unit/dom.test.js @@ -1,5 +1,24 @@ import { describe, it, expect, vi } from 'vitest'; -import { h } from '../../src/ui/dom.js'; +import { h, s } from '../../src/ui/dom.js'; + +const SVG_NS = 'http://www.w3.org/2000/svg'; + +describe('s (SVG namespace)', () => { + it('creates elements in the SVG namespace with attrs, style, events, and children', () => { + const onclick = vi.fn(); + const el = s('svg', { viewBox: '0 0 10 10', class: 'c', style: { width: '100%' }, onclick, title: null }, + s('path', { d: 'M0 0' }), 'x'); + expect(el.namespaceURI).toBe(SVG_NS); + expect(el.getAttribute('viewBox')).toBe('0 0 10 10'); + expect(el.getAttribute('class')).toBe('c'); + expect(el.style.width).toBe('100%'); + expect(el.hasAttribute('title')).toBe(false); // null skipped + expect(el.firstChild.namespaceURI).toBe(SVG_NS); // child path is namespaced + expect(el.textContent).toContain('x'); + el.dispatchEvent(new Event('click')); + expect(onclick).toHaveBeenCalled(); + }); +}); describe('h', () => { it('builds an element with no props', () => {