You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found during #164's code review (D9). The complex surface (sortable/resizable grid) is already shared — renderGrid — but its state wiring ({sort, widths} holder + repaint-on-sort closure) is now hand-rolled at four sites, each slightly different:
renderTable (src/ui/results.js:~670) — global app.state.resultSort + full renderResults repaint
openRowsViewer (src/ui/results.js:~356) — entry-persisted sort/widths + local paint
expandDataPane (src/ui/results.js:~709) — local sort/widths inside a 3-view paint
dashboard table tile (src/ui/dashboard.js:~172) — slot-persisted, with a schemaKey reset (the only copy that has one)
Goal
Extract the repeated state-and-repaint wiring around renderGrid() into a small shared adapter, and move the grid renderer out of results.js into its own module.
This is a behavior-preserving preparation step for #166. The new abstraction must support the current three grid consumers and become the table-panel foundation for #166 without knowing anything about panels, dashboards, or saved-query configuration.
Why now
renderGrid() already centralizes the complex table DOM and interactions:
row sorting;
sortable headers;
column resize handles;
fixed-width application;
row-number column;
numeric alignment;
cell click handling;
display caps.
However, every consumer still hand-wires the same controller responsibilities:
create or locate sort state;
create or locate column-width state;
mutate sort state on header click;
repaint the correct UI scope;
preserve width state across repaint;
decide how long state should live;
decide when stale state should reset.
There are currently three consumers on main:
main results table;
script-result rows viewer;
detached Data pane.
PR #168 will not be merged. Its dashboard table implementation is not a fourth source to preserve, but #166 will add a real fourth consumer through the table panel type.
The shape is now sufficiently clear to extract before #166 rather than duplicating the wiring again inside the panel registry.
The renderer should not calculate schemaKey() or reset state automatically.
A small pure helper is allowed if useful:
ensureGridState(holder,schemaKey)
or:
gridStateForSchema(previous,key)
Possible contract:
{key,sort: {col: null,dir: 'asc'},widths: {},}
But schema-reset policy belongs to the state owner.
For the current three consumers, preserve existing behavior unless a stale-state bug is explicitly demonstrated. #166 can use schema-keyed state for table panels.
Module extraction
Move grid-specific code out of src/ui/results.js into a dedicated module, for example:
src/ui/grid-render.js
Move together:
renderGrid;
column resize logic;
fixed-width application;
width-key mapping used by the data grid;
colResizeWidth;
the new state-wiring adapter.
Do not move unrelated script-grid behavior unless sharing the same low-level resize primitive is clearly cleaner.
The extraction must avoid a dependency from the grid module back into results.js.
This prepares #166's required renderer split and prevents:
Found during #164's code review (D9). The complex surface (sortable/resizable grid) is already shared —
renderGrid— but its state wiring ({sort, widths} holder + repaint-on-sort closure) is now hand-rolled at four sites, each slightly different:renderTable(src/ui/results.js:~670) — globalapp.state.resultSort+ full renderResults repaintopenRowsViewer(src/ui/results.js:~356) — entry-persisted sort/widths + local paintexpandDataPane(src/ui/results.js:~709) — local sort/widths inside a 3-view paintschemaKeyreset (the only copy that has one)Goal
Extract the repeated state-and-repaint wiring around
renderGrid()into a small shared adapter, and move the grid renderer out ofresults.jsinto its own module.This is a behavior-preserving preparation step for #166. The new abstraction must support the current three grid consumers and become the table-panel foundation for #166 without knowing anything about panels, dashboards, or saved-query configuration.
Why now
renderGrid()already centralizes the complex table DOM and interactions:However, every consumer still hand-wires the same controller responsibilities:
There are currently three consumers on
main:PR #168 will not be merged. Its dashboard table implementation is not a fourth source to preserve, but #166 will add a real fourth consumer through the
tablepanel type.The shape is now sufficiently clear to extract before #166 rather than duplicating the wiring again inside the panel registry.
Current duplication
Main results table
State ownership is split:
Characteristics:
Script-result rows viewer
Characteristics:
Detached Data pane
Characteristics:
Problem
The duplicated code is small, but it encodes important behavior. A sort/resize change currently requires separate verification across every consumer.
Likely future changes affected by this duplication include:
A local fix can easily create inconsistent behavior:
Design principles
Keep
renderGrid()statelessrenderGrid()should remain a pure DOM renderer driven by explicit inputs.It must not own:
Extract a thin wiring adapter
The new helper should centralize only the repeated grid controller behavior:
renderGrid();Suggested shape:
Equivalent naming is acceptable, but the state seams must remain explicit.
Example behavior:
The adapter may support mutable state holders where that produces cleaner call sites, but it must not silently choose state ownership.
Caller owns state lifetime
Each surface must continue deciding where sort and widths live:
Caller owns repaint scope
The helper receives a
rerendercallback. It must not callrenderResults()or manipulate a specific container directly.This preserves:
Schema reset is separate
The renderer should not calculate
schemaKey()or reset state automatically.A small pure helper is allowed if useful:
or:
Possible contract:
But schema-reset policy belongs to the state owner.
For the current three consumers, preserve existing behavior unless a stale-state bug is explicitly demonstrated. #166 can use schema-keyed state for table panels.
Module extraction
Move grid-specific code out of
src/ui/results.jsinto a dedicated module, for example:Move together:
renderGrid;colResizeWidth;Do not move unrelated script-grid behavior unless sharing the same low-level resize primitive is clearly cleaner.
The extraction must avoid a dependency from the grid module back into
results.js.This prepares #166's required renderer split and prevents:
circular dependencies.
Proposed API
Exact names may differ, but the module should expose responsibilities approximately like this:
Optional schema-state helper:
renderGridView()inputs:Required behavior:
setSort()is called exactly once per sort click;rerender()is called after sort state is updated;Consumer migration
Main results table
Preserve:
app.state.resultSort;r.colWidths;renderResults(app)repaint;visCap(r);Expected shape:
Script-result rows viewer
Preserve:
entry;Expected shape:
The implementation may keep a stable mutable sort object if existing tests depend on object identity, but this should be deliberate and documented.
Detached Data pane
Preserve:
The helper must not know that the repaint also handles charts.
#166 integration contract
This issue should land before #166.
The
tablepanel arm in #166 should be able to call the shared adapter directly:This issue must not add panel-specific code, registry knowledge, or saved-query migration.
Acceptance criteria
results.jsinto a dedicated UI module.renderGrid()remains stateless and behavior-equivalent.Tests
Shared adapter
setSort({col, dir}).columns,rows,onCell, andcapare forwarded unchanged.renderGrid()reachsetSort()correctly.Main results table
app.state.resultSort.Script rows viewer
Detached Data pane
Regression
renderGridsorting tests remain valid.Non-goals
Sequencing
tablepanel arm as the next real consumer.Suggested title