fix(ui): column widths ignore colspan rows and empty first renders (#258) by adminopenclaw8-sketch · Pull Request #268 · dborup/CoreScope

adminopenclaw8-sketch · 2026-10-05T14:58:36Z

Relates to #258

Follow-up to #252 (#244). It addresses finding 1 and the "UX remainder" of the #252 review. Plan: comment on #258.

Problem

makeColumnsResizable() (public/app.js) sizes a table's columns from the header and the first body rows. Two things went wrong:

Colspan rows counted. It measured cells by index, so a full-width colspan row (the packets vscroll spacers, "No packets found") was credited to column 0.
One-shot measurement. It measured only once, from whatever the first render had.

On packets with an aged fixture, the default 15-min window is empty. At 1200 px the expand column got 43 % of the table and Details 65 px. Long advert names then sat entirely on the line the #67 clamp hides, and only the icon showed. Even with a fresh fixture, expand took 176 px for a chevron.

Fix

Skip colspan rows. columnMeasureRows() keeps only rows with one cell per header cell and no colspan.
Re-measure once, later. With fewer than 5 usable rows the widths are provisional.
- One MutationObserver (childList on the tbody) measures again once real rows arrive, then disconnects.
- It does not re-measure if the user has saved widths by then.
- A table measured from a full body gets no observer.
The re-measure sees the table as the first measure did:
- Hidden columns: TableResponsive.unhidden(table, fn) (packets.js) runs the measurement with its own column hiding lifted. Its tbody observer runs after ours, so the new rows' cells are not yet marked; without this, at 900 px HB came out at 12.2 % instead of 3.8 %.
- Handles: the resize handles are hidden while measuring. They stick out 4 px past their th.
- Page-set widths: widths that the page's markup sets on a th are restored, for example observers' style="width:32px".
Saved widths (meshcore-*-col-widths) are applied exactly as before, and the handles are unchanged.
Fit logic unchanged. The measure and fit steps are extracted as they were (measureColumnWidths, fitColumnWidths, applyMeasuredColumnWidths, readSavedColumnWidths). The first measure is the same as master's except that colspan rows are skipped.

Perf

First call: as before, at most 30 rows × columns, once per table.
Provisional tables only: each tbody childList mutation runs a guard:
- isConnected;
- a localStorage read for the storage key;
- tbody.rows.length;
- when that length is ≥ 5, a scan for 5 usable rows.
One re-measure, then nothing. After it, the observer is disconnected, so no per-render or per-row work is left.
unhidden: two querySelectorAll calls and a class toggle on the hidden cells, only during a measurement.
Callers: tables measured from a full body (nodes, observers, analytics, new-nodes on the fixture) never create an observer. No new API calls.

Tests

Test-first: each fix commit follows a red test commit.

test-issue-258-column-widths.js (new, in test-all.sh) runs the real app.js in a vm sandbox with a fake table DOM. Its cells report content width, assigned width and handle overhang. It covers:
- colspan, short and same-length colspan rows;
- an empty first body and a near-empty one (2 rows; 4 rows are not enough, 6 rows are);
- one re-measure, then a disconnected observer;
- no feedback from the provisional widths;
- a th width set by the page;
- hidden columns measured inside unhidden;
- saved widths before and during the provisional phase;
- idempotent re-calls;
- a detached table.
15 cases, 3/15 on master.
test-packets.js: the real TableResponsive.unhidden lifts and restores, also when its callback throws. Both new cases are red on master.
test-issue-258-column-widths-e2e.js (new, in deploy.yml after the bug(packets): filter UX disaster — help panel overlaps table, toolbar chaotic, path chips spill rows Kpa-clawbot/CoreScope#1122 Details clamp E2E). It ages the fixture by moving the browser clock to an effective 20 and 120 min, with the offset taken from the newest packet. At 1200 and 900 px it checks:
- the default window renders empty;
- after widening, Details is ≥ 15 % and expand ≤ 10 % of the table;
- every column's share matches a page opened with that window right away;
- every on-screen advert link shows ≥ 12 px, and the long pinned name shows at least half;
- an adverts-only filter, the empty window and the wide window again do not move the widths.
It also checks that saved widths apply unchanged and that a handle drag resizes and saves. 10/22 on master (12 red).
test-issue-1122-details-row-clamp-e2e.js:
- 375 px: the pinned long name must still be cut by its clip there.
- 900 / 1200 px: it must show at least half of itself, since Details now fits it.

Gates

scripts/check-xss-sinks.sh --diff origin/master is clean.
No hardcoded colours.
Fork guards are unchanged: 9 in deploy.yml and 1 in release-fast-path.yml.
One new E2E line in deploy.yml.

🤖 Generated with Claude Code

…rst renders (#258) makeColumnsResizable() credits full-width colspan rows (vscroll spacers, "No packets found") to column 0 and locks the widths from an empty or near-empty first body. The new vm-sandbox unit test runs the real app.js against a minimal fake table: colspan and short rows must not change any width, an (almost) empty body must be measured again once when rows arrive (and only then), saved widths must be applied as before. Red on master: 3 passed, 9 failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

New test-issue-258-column-widths-e2e.js, registered after the Kpa-clawbot#1122 Details clamp E2E. It ages the fixture by moving the browser clock (20 and 120 min effective, offset from the newest packet), checks that the default window renders empty, then widens the window at 1200 and 900 px: Details >= 15% and expand <= 10% of the table, advert names show text (the long pinned name at least half), widths stay through later renders, saved widths apply unchanged and a resize handle still resizes and saves. Red on master: 9 passed, 9 failed. test-issue-1122-details-row-clamp-e2e.js: the pinned long advert name must be cut by its clip only on mobile (375 px). At 900/1200 px Details is sized from the real rows after the fix, so the name must show at least half of itself instead (finding 1 of the #252 review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

) makeColumnsResizable() measured each column by row index, so a full-width colspan row (vscroll spacer, "No packets found") was credited to column 0, and it measured only once, from whatever the first render had. On packets with an aged fixture the default 15-min window is empty: expand got ~43% of the table and Details 65 px at 1200 px, hiding long advert names behind the #67 clamp. - columnMeasureRows() keeps only rows with one cell per header cell and no colspan. - With fewer than 5 usable rows the widths are provisional; one MutationObserver (childList on the tbody) measures again once real rows arrive, unless widths have been saved by then, and disconnects. A table measured from a full body gets no observer. - Saved widths are read by readSavedColumnWidths() and applied as before. - The measure and fit steps are extracted unchanged (measureColumnWidths, fitColumnWidths, applyMeasuredColumnWidths); the re-measure clears the old th widths first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…258) - A row with as many cells as the header, one of them a colspan, is not measured either. - A width the page's markup puts on a th (observers' compare column has style="width:32px") counts in the first measure, as on master, and again in the re-measure. Red on the previous commit, which cleared it before measuring and changed the observers table's widths slightly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The first measure no longer clears the header cells' inline widths, so tables measured from a full body (nodes, observers, analytics, new-nodes) get exactly master's widths. The re-measure restores the widths the page's markup set before measuring, instead of the provisional percentages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- E2E: after the deferred re-measure, every column's share must equal (within 1 point) that of a page opened with the wide window right away. Red on the previous commit: at 900 px TableResponsive had hidden five header cells but not yet the new rows' cells, so the re-measure saw a skewed table (HB 12.2% instead of 3.8%); at 1200 px the resize handles, which stick out 4px past their th, widened the narrow columns. - Unit: columns TableResponsive hid are measured inside TableResponsive.unhidden (stubbed here), and a shown handle's 4px overhang does not count. - test-packets.js: the real TableResponsive.unhidden lifts col-hidden and the pills only while its callback runs, also when it throws. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- TableResponsive.unhidden(table, fn) (packets.js) runs fn with its own column hiding lifted (no col-hidden, no pill) and restores it. measureColumnWidths() measures inside it, so the re-measure, which runs before TableResponsive has marked the new rows' cells, sees the same columns as the first measure (before register()). Other CSS that hides a column still applies. - The resize handles are hidden while measuring; they did not exist at the first measure and their overhang inflated th.scrollWidth. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dborup-agent · 2026-10-05T21:17:55Z

Review — CS-pve-agent2 PR#268 — head `da436cd`

Dom: APPROVE med nits

Independent, read-only review. Evidence tags: [T] I ran it myself, [A] code analysis, [K] from CI logs or the PR description.

Trees tested:

git merge-tree --write-tree origin/master <head> against 2e8deaed (tree 33f7cd0b): everything below.
Re-checked against the newer master f91339f2 (fix(app): let an explicit refresh bypass an in-flight api() request (#243) #263, touches api() in app.js, tree c3e9f37c): merges cleanly, makeColumnsResizable and its helpers are byte-identical, and the unit suites pass.
Head was da436cd6 per git ls-remote both before and after the review.

Findings

#	Severity	Where	Finding	Evidence
1	nit	`test-issue-258-column-widths-e2e.js` header	The header comment says "the Path handle is dragged left, which gives the room to the columns after it", but the test drags the Time handle right, as its step name and comment say. The header is stale.	[A]
2	nit	PR description, Perf → Callers	It says analytics tables "never create an observer". Analytics tables with fewer than 5 usable rows do get one: on the fixture `analytics-tbl-rf-0` (1 row) and `analytics-tbl-nodes-1` (0 rows), at 1200 and 900 px. They are static, so it never fires and is collected with the table. Harmless, but the description is inaccurate.	[T]
3	info	`columnMeasureRows`	It now walks `tbody.rows` instead of `tbody.querySelectorAll('tr')`, so rows of nested tables inside a cell are no longer measured. No current caller nests tables in a tbody, so this is arguably a fix with no visible effect.	[A]
4	info	`test-issue-1122-details-row-clamp-e2e.js`	The new guard does not depend on the #258 fix. It also passes against master's frontend (18/18), because that test opens the wide window directly. That is fine: `test-issue-258-column-widths-e2e.js` is the test that tells the two apart.	[T]

No blocking findings.

Answers to the review points

1. Colspan rows not measured. Yes. [A] [T]

columnMeasureRows(tbody, colCount, limit) drops rows with children.length !== colCount and rows with any colSpan > 1, including a colspan row whose cell count matches the header.
The first measure and the re-measure both use it.
Unit cases cover a spacer/empty-state row, a short row and a same-length colspan row.

2. No locking from an empty or near-empty first render. Yes. [A] [T]

With fewer than COL_MEASURE_MIN_ROWS = 5 usable rows, one MutationObserver is set up with childList only.
Its callback disconnects if the table is detached or saved widths now exist. It returns if tbody.rows.length < 5 or if there are fewer than 5 usable rows.
Otherwise it disconnects first and then re-measures once.
A table measured from a full body gets no observer. Per mutation, only a cheap guard runs while a table is provisional. After the re-measure nothing runs per render or per row.
The E2E "later renders keep the widths" step and the unit "widths stay after later renders" case pin this down.
Before re-measuring, the th widths the page itself set are restored, so the provisional widths do not feed back. The measurement runs inside TableResponsive.unhidden, and resize handles are hidden while measuring.
unhidden only toggles classes and style.display, and TableResponsive's own observer is childList-only, so no observer loop is possible. [A]

3. Saved widths and handles; the other callers. Unchanged. [T] [A]

Saved-widths path: readSavedColumnWidths keeps the same validation (array, same length, sum between 90 and 110). The path still sets fixed layout, width:100%, the th percentages and the handles, and does no measurement.
Browser comparison, master vs merged: at 1200 and 900 px, header widths in px are identical for:
- #nodesTable;
- #obsTable, including its style="width:32px" compare column;
- analytics rf, topology, channels and nodes (6 tables).
- No page errors on either side.
Observers per table: none on nodes, observers, topology and channels; only the small analytics tables from finding 2 get one.
nodes and observers:
- preset meshcore-nodes-col-widths / meshcore-obs-col-widths are applied as saved, identically on master and merged;
- dragging a handle widens the column by 40 px and saves, also identically.
packets: the E2E covers saved widths (also through the empty→full transition) and handle drag+save.

4. pinned.truncated guard in the Kpa-clawbot#1122 E2E. Updated and not vacuous. [T] [A]

At 375 px it still requires truncation.
At 900 and 1200 px it requires visibleW >= nameW/2, and the pinned row must be on screen.
If the link had no fragment in the clip, visibleW would be undefined and the assertion would fail. It cannot pass vacuously.
Mutant M7: the old guard (assert(pinned.truncated)) on the merged tree is red at desktop-1200 and tablet-900 ("pinned advert name fits its Details clip"), 16/18. So the update was required.

5. Column widths before and after. [T]

Packets, local Go server with the CI-prepared fixture. Each cell gives the first render, then → after widening the window (24 h at 1200 px, 3 h at 900 px). "Pinned" is the visible/total px of KN6PLV-BrkOxfLA-Yebes.

Viewport	Fixture age	master expand	master Details	merged expand	merged Details	pinned (master → merged)	links < 12 px (master → merged)
1200	fresh (51 rows)	15.0 %	178 px	3.1 %	301 px	125/175 → 148/175	0 → 0
1200	20 min	43.6 % → 43.6 %	64 → 64 px	3.1 % → 3.1 %	64 → 305 px	0/175 → 148/175	3/5 → 0/5
1200	120 min	43.6 % → 43.6 %	64 → 64 px	3.1 % → 3.1 %	64 → 305 px	0/175 → 148/175	3/5 → 0/5
900	fresh (51 rows)	18.9 %	170 px	6.8 %	205 px	125/169 → 148/169	0 → 0
900	20 min	38.8 % → 38.8 %	104 → 104 px	6.7 % → 6.8 %	116 → 205 px	66/169 → 148/169	0 → 0
900	120 min	38.8 % → 38.8 %	104 → 104 px	6.7 % → 6.8 %	116 → 205 px	66/169 → 148/169	0 → 0

Aging: done with the browser clock, as the PR's E2E does.
Cross-check: I also aged the DB itself, shifting transmissions.first_seen and observations.timestamp back by 20 and 120 min, on separate merged servers without a clock offset. The results are identical (1200: Details 64 → 305 px, expand 3.1 %; 900: 116 → 205 px).
Screenshots: at 1200 px, aged 20 min, master shows only the advert icon followed by "…" in Details. Merged shows full names, e.g. "KN6PLV-BrkOxfLA-Yebes" and "CLTR Repeater".

6. Mutant: colspan rows back in the measurement. Red. [T]

M1 (both filters removed in columnMeasureRows): unit 10/15 (5 red); E2E 18/22. All four "expand ≤ 10 %" steps are red, with expand at 176 px (15.0 %) at 1200 and 166 px (18.9 %) at 900.
M1b (only the colSpan > 1 check removed): unit 14/15, with the same-length colspan case red.

Always-checks

Each acceptance criterion has a red-before / green-after test. [T]
- Colspan skip: unit test, 3/15 on master and 15/15 on merged. makeColumnsResizable measures once and credits colspan rows to column 0 (packets Details column too narrow) #258 E2E "expand ≤ 10 %": red on master, green on merged.
- Deferred single re-measure, not with saved widths: unit cases red on master and green on merged. E2E: 10/22 on master (12 red), 22/22 on merged.
- TableResponsive.unhidden: the 2 new test-packets.js cases are red on master (139/141) and green on merged.
- bug(packets): filter UX disaster — help panel overlaps table, toolbar chaotic, path chips spill rows Kpa-clawbot/CoreScope#1122 guard: the old guard is red on merged and the new one is green (M7).
- "Keep the other callers' behaviour" is a preservation criterion with no red-before. I checked it in the browser (point 3).

My own mutants: 9 in total, all killed. [T]

Mutant	Unit result	E2E result
M1 colspan back	red, 5/15	red, 4/22
M1b same-length colspan	red, 1/15	—
M2 no deferral (master's one-shot behaviour)	red, 9/15	red, 12/22
M3 no disconnect (re-measure every mutation)	red, 2/15	—
M4 re-measure despite saved widths	red, 1/15	—
M5 provisional `th` widths not reset	red, 5/15	—
M6 measure outside `unhidden`	red, 1/15	red, 2/22: 900 px deferred vs direct differ by up to 8.6 points
M7 old bug(packets): filter UX disaster — help panel overlaps table, toolbar chaotic, path chips spill rows Kpa-clawbot/CoreScope#1122 guard	—	red, 2/18

No behaviour change beyond the purpose. [T] [A] Other callers are pixel-identical. The only side effect is finding 2. The fit logic was extracted unchanged, and the saved path is unchanged.
cmd/server stays read-only. No Go files are changed, and 0 new map[string]interface{}. [A]
No hardcoded colours. The only #xxx match among added public/ lines is the issue reference #258. [T]
scripts/check-xss-sinks.sh --diff origin/master on head: exit 0, no findings. [T]
Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, on master, head and the merged tree. [T]
Workflow change: exactly one new E2E line in deploy.yml, after the bug(packets): filter UX disaster — help panel overlaps table, toolbar chaotic, path chips spill rows Kpa-clawbot/CoreScope#1122 Details clamp line. [A]
Closing keywords: none in the PR body (it says "Relates to makeColumnsResizable measures once and credits colspan rows to column 0 (packets Details column too narrow) #258"; closingIssuesReferences is empty) and none in the commit messages. [T]
Commit author and committer: dborup <kontakt@meshview.dk> on all 7 commits. [T]

Tests run

All on the tree merged with 2e8deaed, unless noted.

Go: cmd/server go test -timeout 25m ./... passes (719 s). cmd/ingestor passes too (868 s). [T]
- A first run with Go's default 10 m timeout timed out on the review machine while browsers ran in parallel. CI uses -timeout 20m. The PR changes no Go code.
sh test-all.sh: 220/220 pass. On the newer master f91339f2: 221/221. [T]
- The first run, under heavy parallel load, had one failure in test-channels-client-state-152.js (N1 remap mid-decrypt, timing). It passed 4× standalone and in the unloaded rerun. It is unrelated to this PR, which does not touch channels.
node test-frontend-helpers.js: 707/707 (both master bases). test-issue-258-column-widths.js: 15/15. test-packets.js: 141/141. [T]

E2E setup: a local Go server on e2e-fixture.db, prepared as in CI: freshen-fixture.sh, the seed SQL from deploy.yml, corescope-migrate, seeds 2073 and 199 (and 245, which deploy.yml on master also applies). The server was stopped via its port. [T] Results:

E2E	Result
`test-issue-258-column-widths-e2e.js`	22/22
`test-issue-1122-details-row-clamp-e2e.js`	18/18
`test-issue-1122-packets-filter-ux-e2e.js`	6/6
`test-issue-1128-packets-layout-e2e.js`	5/5
`test-issue-1128-multi-viewport-e2e.js`	15/15
`test-issue-189-group-caret-e2e.js`	3/3
`test-issue-254-affinity-toggle-mobile-aria-e2e.js`	12/12
`test-table-fluid-e2e.js`	41/41
`test-observer-iata-1188-e2e.js`	pass
`test-e2e-playwright.js`	1 failure ("Version info lives on Perf dashboard"); identical on master, so local-build environment, not this PR

CI on head da436cd6: Go Build & Test, Playwright E2E Tests and Build & Publish Docker Image all pass. Deploy, Release and Badges are skipped as usual on a PR. [K]
- The Playwright log shows #258 … 22 passed, 0 failed and #1122 Details row clamp … passed 18 failed 0, with 0 ✗/❌ lines in the job.
- Neither known flake (flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256, flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267) showed up.

Not verified

Browser behaviour outside headless Chromium (Firefox, Safari, real touch devices).
The live path where rows trickle in over WebSocket into an empty 15-min window. The re-measure then happens at the 5th usable row. [A] only; the E2E switches windows instead.
new-nodes, node-changes, gps-sanity and position-gaps in the browser: my script found no resizable table on those routes with the fixture. I covered them only by code analysis: they share the same function, and their behaviour changes only when the first render has fewer than 5 usable rows.
The instrumented frontend (public-instrumented) and coverage numbers. I served the plain public/.
The E2E suites against the tree merged with the newer master f91339f2. Only the unit suites ran there; its app.js change is in api() and does not touch the column code.

#268 (issue #258, column widths) and this branch both appended a line to test-all.sh's runner list. Kept both, in issue order next to #254: run test-issue-258-column-widths.js run test-issue-259-nodes-esc-listener.js Everything else auto-merged and was verified to be the exact union of both sides: .github/workflows/deploy.yml keeps the #258 and #259 E2E lines, public/packets.js keeps #258's TableResponsive.unhidden() and #259's groupIsExpandedInView(), and test-packets.js keeps both sets of cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…wups fix(packets): one pktEsc listener per page and #268/#260/#212 follow-ups (#282)

dborup and others added 7 commits October 5, 2026 16:12

dborup marked this pull request as ready for review October 6, 2026 01:13

dborup merged commit 406f848 into master Oct 6, 2026
6 checks passed

This was referenced Oct 6, 2026

makeColumnsResizable measures once and credits colspan rows to column 0 (packets Details column too narrow) #258

Closed

Follow-ups to #268, #260 and #212: pktEsc listener leak, test gaps and comment fixes #282

Closed

dborup mentioned this pull request Oct 6, 2026

fix(ui): follow-ups to #255 — mobile group dead end, nodesEsc leak, test gaps (#259) #260

Merged

dborup-agent mentioned this pull request Oct 6, 2026

fix(packets): one pktEsc listener per page and #268/#260/#212 follow-ups (#282) #313

Merged

dborup added a commit that referenced this pull request Oct 6, 2026

Merge pull request #313 from dborup/codex/issue-282-pktesc-leak-follo…

30c7de4

…wups fix(packets): one pktEsc listener per page and #268/#260/#212 follow-ups (#282)

Navigation Menu

Repository navigation

fix(ui): column widths ignore colspan rows and empty first renders (#258) - #268

fix(ui): column widths ignore colspan rows and empty first renders (#258)#268
dborup merged 7 commits into
masterfrom
codex/issue-258-column-widths

adminopenclaw8-sketch commented Oct 5, 2026

Uh oh!

dborup-agent commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

adminopenclaw8-sketch commented Oct 5, 2026

Problem

Fix

Perf

Tests

Gates

Uh oh!

dborup-agent commented Oct 5, 2026

Review — CS-pve-agent2 PR#268 — head da436cd

Findings

Answers to the review points

Always-checks

Tests run

Not verified

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Review — CS-pve-agent2 PR#268 — head `da436cd`