spaces: the table's sorting and cell editing were unreachable code - #433
Open
nyblnet wants to merge 1 commit into
Open
spaces: the table's sorting and cell editing were unreachable code#433nyblnet wants to merge 1 commit into
nyblnet wants to merge 1 commit into
Conversation
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 === '<word>'` 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.
Build size
Updated: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
spaces/src/render.tsonmaincarries twoif (layout === 'table')branches. The first returns, so the second — the one #390 wrote, with click-to-sort headers and edit-in-place cells — is unreachable.The table still renders, which is why nobody saw it. It just quietly became read-only and unsortable.
Measured, in the built shell
Not read out of the source, because the source is what fooled everyone the first time. A view with three pages and an
estimatefield, in a real browser on a served build:beb5a19(main)data-sort-colpresentSo the whole of #390 — one of the four features the Bases round shipped — has been dead on
mainsince covers and the gallery landed.Where it came from
beb5a19(#392): one branch at its parent, two at the commit. A rebase of a stacked branch onto a squashed base duplicated the block.The same rebase also duplicated a whole section of
scripts/test-spaces-model.ts. That copy 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.tsfor the click wiring. That wiring exists and is correct — the handler was fine, the markup it needed was never rendered. A source grep answering a question about the wrong file. I made the same mistake by hand at the time: grepped fordataset.sortCol, found it, and reported #390 intact.The guard
It counts dispatch branches rather than checking any one branch's contents — every
layout === '<word>'inrender.tsmust appear exactly once. Sabotage-verified against main's actualrender.ts, which fails it and names the duplicate:A second assertion pins that the surviving branch still renders sortable headers, because deleting the wrong half of a duplicate pair passes a count check and loses the feature. Worth recording: that assertion was first written as
/sortCol/and stayed green when the property was renamed tosortColX— the sabotage found my assertion, not the code. It is now anchored on\bdataset\.sortCol\band fails under that rename.Checks
node scripts/test-spaces.mjs— all 8 rigs pass, model 936/936tsc --noEmitcleannpm run build:singleclean; shell 270KB (−1KB, dead code removed)Flagged
scripts/test-spaces-model.tsis an ops-owned surface; this is a spaces branch touching it, which is the known cross-zone shape for spaces rig coverage. Claimed on the board.This should land before the five feature branches in flight — they all touch
render.tsand will rebase onto it anyway, and it is a live regression onmain.