From e4262d08e7ccaaceaf933e24c0f2123e6968e3e1 Mon Sep 17 00:00:00 2001 From: nyblnet <306268465+nyblnet@users.noreply.github.com> Date: Wed, 9 Sep 2026 02:22:47 +0100 Subject: [PATCH] spaces: the table's sorting and cell editing were unreachable code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit render.ts carried TWO `if (layout === 'table')` branches. The first returned, so the second — the one #390 wrote, with sortable headers and editable cells — could never run. The table still rendered, which is why nobody saw it: it just quietly became read-only and unsortable. Measured in the built shell rather than read out of the source. Before, on beb5a19: the table draws three headers and 0 header buttons, 0 cell buttons, and no sort attribute anywhere in the document. After: 2 header buttons, 6 cell buttons, and clicking the Estimate header reorders the rows 5,2,9 -> 2,5,9. So the whole of #390 has been dead on main since covers and the gallery landed. Introduced by beb5a19 (#392): one branch at its parent, two at the commit. The same rebase duplicated a whole section of scripts/test-spaces-model.ts, which was found and removed in #409 — I removed the rig copy and missed this one. WHY IT SURVIVED REVIEW TWICE. The rig covering #390 checks editor.ts for the click wiring, and that wiring exists and is correct: the handler was fine, the markup it needed was never rendered. A source grep answered a question about the wrong file. I made the same mistake by hand at the time — I grepped for dataset.sortCol, found it, and reported #390 intact. So the guard added here counts DISPATCH BRANCHES rather than checking any one branch's contents: every `layout === ''` in render.ts must appear once. Sabotage-verified against main's actual render.ts, which fails it and names the duplicate. The second assertion — that the surviving branch still renders sortable headers — was written as /sortCol/ and stayed GREEN when the property was renamed to sortColX, which is precisely what deleting the wrong half of a duplicate pair looks like. The sabotage found my assertion rather than the code. It is now anchored on \bdataset\.sortCol\b and fails under that rename. Rig coverage lands in scripts/test-spaces-model.ts, an ops-owned surface; flagged rather than hidden. --- scripts/test-spaces-model.ts | 41 ++++++++++++++++++++++ spaces/CHANGELOG.md | 9 +++++ spaces/src/render.ts | 68 ------------------------------------ 3 files changed, 50 insertions(+), 68 deletions(-) diff --git a/scripts/test-spaces-model.ts b/scripts/test-spaces-model.ts index a61883d3..dbfb1775 100644 --- a/scripts/test-spaces-model.ts +++ b/scripts/test-spaces-model.ts @@ -3594,5 +3594,46 @@ function fsTable(f: string): string { } +// ---- ONE BRANCH PER LAYOUT ------------------------------------------------- +// A rebase duplicated a whole `if (layout === 'table')` block into render.ts. +// Both copies compiled, both were valid, and the FIRST one returned — so the +// second, which is the one carrying #390's sortable headers and editable +// cells, was unreachable. Click-to-sort and edit-in-place shipped to main and +// silently did nothing; the table rendered, so nothing looked broken. +// +// It survived review twice. The rig that was supposed to cover #390 checked +// editor.ts for the click WIRING, which exists and is correct — the handler +// was fine, the markup it needed was never rendered. That is a source grep +// answering a question about the wrong file. +// +// The same commit also duplicated a whole section of THIS rig, which was +// caught and removed. So the class is: a rebase of a stacked branch onto a +// squashed base silently duplicates a block. Count the dispatch branches; +// duplicates are the thing to fail on, not any one of their contents. +{ + const fs = await import('node:fs') + const ren = fs.readFileSync(new URL('../spaces/src/render.ts', import.meta.url), 'utf8') + + const seen = new Map() + for (const [, word] of ren.matchAll(/layout === '([a-z]+)'/g)) { + seen.set(word, (seen.get(word) ?? 0) + 1) + } + const dupes = [...seen].filter(([, n]) => n > 1).map(([w, n]) => `${w}×${n}`) + ok(dupes.length === 0, + `each layout is dispatched from exactly ONE branch in render.ts${dupes.length ? ' — duplicated: ' + dupes.join(', ') : ''}`) + + // …and the branch that survives is the one that can actually sort and edit. + // Deleting the wrong copy of a duplicate pair passes the count check above + // and loses the feature, so name what the surviving branch must contain. + // \b, not a bare substring: the first draft of this check was /sortCol/, and + // renaming the property to `sortColX` — which is exactly what deleting the + // wrong half of the pair looks like — left it GREEN. The sabotage found my + // assertion, not the code. + ok(/\bdataset\.sortCol\b/.test(ren), + 'the surviving table branch renders sortable headers') + ok(seen.get('table') === 1, 'exactly one table branch, so that header markup is reachable') +} + + console.log(`\n${checks - failures}/${checks} checks passed`) if (failures) process.exit(1) diff --git a/spaces/CHANGELOG.md b/spaces/CHANGELOG.md index 820b8829..90e380bb 100644 --- a/spaces/CHANGELOG.md +++ b/spaces/CHANGELOG.md @@ -14,6 +14,15 @@ Versions follow `0.MINOR.PATCH` while pre-1.0. ## [Unreleased] +- **The table sorts and edits again.** A rebase duplicated the whole + `layout === 'table'` branch in the renderer. Both copies compiled, and the + first one returned — so the second, the one carrying click-to-sort headers + and edit-in-place cells, was unreachable from the moment covers and the + gallery landed. The table still drew, so nothing looked broken; it was + simply read-only and unsortable. Measured in the built shell before the fix: + 0 header buttons, 0 cell buttons, no sort attribute anywhere. After: 2 and 6, + and clicking a column header actually reorders the rows. + - **The whole gallery card is the target, and a long title stops inflating its row.** In a shelf of covers the picture is what you point at, so the title's link now stretches over the card rather than the card holding a second one — diff --git a/spaces/src/render.ts b/spaces/src/render.ts index 8d5c05d3..644ad853 100644 --- a/spaces/src/render.ts +++ b/spaces/src/render.ts @@ -1281,74 +1281,6 @@ function renderView(host: HTMLElement, b: Block, doc: SpacesDoc, opts: RenderOpt return a } - // TABLE — the shape a base is usually looked at in, and the one this app did - // not have. Columns are the fields the ROWS ACTUALLY CARRY, in the schema's - // declared order: a table of books should not carry an Estimate column - // because the vocabulary happens to contain one, and a page that has a field - // the others lack should not be the reason everyone gets an empty column. - if (layout === 'table') { - const keys = fieldsOf(doc).map((f) => f.key).filter((k) => rows.some((r) => r.values.has(k))) - const wrap = document.createElement('div') - // its own scroller: a wide table must not make the PAGE scroll sideways - wrap.className = 'sp-view-tablewrap' - const table = document.createElement('table') - table.className = 'sp-view-table' - const thead = document.createElement('thead') - const hr = document.createElement('tr') - const th0 = document.createElement('th') - th0.textContent = t('Page') - hr.appendChild(th0) - for (const k of keys) { - const th = document.createElement('th') - th.textContent = fieldByKey(doc, k)?.label ?? k - hr.appendChild(th) - } - thead.appendChild(hr) - table.appendChild(thead) - const tb = document.createElement('tbody') - for (const r of rows) { - const tr = document.createElement('tr') - const td0 = document.createElement('td') - // the page itself, reached the same way a card reaches it - const a = document.createElement('a') - a.className = 'sp-view-cellink' - a.href = `#p/${r.page.id}` - a.dataset.page = r.page.id - a.textContent = r.page.title || t('Untitled') - td0.appendChild(a) - tr.appendChild(td0) - for (const k of keys) { - const td = document.createElement('td') - const f = fieldByKey(doc, k) - const v = r.values.get(k) - // THROUGH THE OPTION, so a select shows its label and its colour rather - // than the id the model stores — the same thing propHtml does for the - // header strip, and for the same reason: the id is not for reading. - const opt = optionOf(f, v) - if (opt) { - const chip = document.createElement('span') - chip.className = 'sp-prop-chip' - const dot = document.createElement('span') - dot.className = 'sp-prop-dot' - if (opt.color) dot.style.background = opt.color - chip.append(dot, document.createTextNode(opt.label)) - td.appendChild(chip) - } else if (v !== undefined && v !== null && String(v) !== '') { - td.textContent = String(v) - } else { - td.className = 'sp-view-empty' - td.textContent = '—' - } - tr.appendChild(td) - } - tb.appendChild(tr) - } - table.appendChild(tb) - wrap.appendChild(table) - host.appendChild(wrap) - return - } - // GALLERY — the shape that makes a reading list look like a reading list. A // board answers "what state is this in" and a table answers "what does it // say"; a gallery answers "which one is it", which for books, films, recipes