Conversation
|
Reviewed against
One thing should be fixed before this merges. Hiding the Details column pins the table to the width of the short columns
table.style.width = d.total + 'px';Master used test('no flex columns: fixed widths only', () => {
const d = distribute([col(65), col(44)], 0, 2000, 120);
assert.deepStrictEqual(Array.from(d.flex), []);
assert.strictEqual(d.total, 109);
});2000px available, 109px table. To reproduce: The new E2E does not catch it: it asserts Fix shape: when Three things worth a decisionThe shared helper changes behaviour for three other pages. Stale min-widths the layout now depends on being ignored.
Two collisions to be aware of, now that #2080 and #2093 have mergedBoth landed on master after this branch was opened.
Minor, no action needed
|
The packets table stored every column width as a percentage of the table, measured once on load. Percentages scale with the screen, so on a 2000px display "17s ago" sat in a 180px column. The measurement also read the virtual-scroll spacer row (one colspan cell as wide as the table) as column 0, which is why the expand column came out 160-950px wide. Grouped rows drew two arrows: a CSS ::before glyph next to the SVG caret, which itself pointed up when collapsed. Columns: fitColumnsToContent (app.js) sizes Time, Hash, Size, HB, Type, Scope, Observer and Rpt to their content in px; Path and Details split the rest. Every visible column gets an explicit width and the table their sum, because a colspan spacer keeps a slot for each hidden column that would otherwise take a share of the slack as a blank strip. When space is short the widest fixed columns shrink first, then the flex columns down to 60px, then the wrapper scrolls. The layout re-runs on container resize (detail panel), on TableResponsive hiding or revealing columns (new table-columns-changed event), after sorting and column toggles; after each render, grow() widens columns for the rows just inserted and never narrows them, so nothing jitters on scroll. Drag handles store px; double-click resets. makeColumnsResizable (nodes, observers, analytics) now skips colspan rows when measuring. Full Names: a toolbar toggle (localStorage, and fullNames=1 in the URL) shows observer and hop names untruncated. Paths stay on one line; hops past the edge go behind the "+N" pill. The pill was appended after the overflowing chips and so never showed; it is now sticky at the right edge, counts only hop chips, and hovering or focusing it shows the full path one hop per line (click, Enter or Space pins it). Rows drawn before /api/observers resolves show raw pubkeys, so loadObservers now re-renders and re-measures instead of leaving Observer at id width. Child rows indent only the Time cell: indenting every cell misaligned them with the headers and widened every column on expand. Perf: grow() is ~0.9ms per full render at 63 rows; a scroll step measures only its inserted rows (~73 rect reads, was 5,472). Constraint: virtual scroll positions rows at one fixed height, so Full Names cannot wrap paths Rejected: Wrap long paths onto extra lines under Full Names | breaks VSCROLL_ROW_HEIGHT row positioning Rejected: Keep percentage widths, fix only the spacer measurement | still inflates short columns on wide screens Rejected: Leave Path and Details width:auto | the phantom spacer slots take a share and render a blank strip Directive: fitColumnsToContent sets table.style.width; a table using it must not rely on width:100% Directive: rows must keep a constant height; anything that wraps in #pktTable breaks virtual scroll offsets Confidence: high Scope-risk: moderate Not-tested: very large expanded groups (hundreds of child rows on screen at once)
packets.js calls fitColumnsToContent, defined in app.js. Every such cross-file global is listed in .eslintrc.json; this one was missing, so eslint failed the Go Build & Test job with no-undef. Confidence: high Scope-risk: narrow
…oth ways Kpa-clawbot#1281 clicked each packet row at its centre. With the short columns packed, an ADVERT row's centre is a hop link in the Path column, which opens the node page instead of selecting the row, so no ADVERT detail with a Location row was ever found. It now clicks the Time cell, which holds nothing interactive, and re-queries the row before each click: selecting a row re-renders the table, so the handles it collected up front were detached after the first click and it only ever tried row 0. The compact-columns URL check assumed Full Names started off. The previous case leaves it on in the page's own state, and returning to Packets can write fullNames=1 back into the hash before the reload, so the click turned it off. It now asserts that the URL matches the state after each of two clicks. Rejected: Also switch test-e2e-playwright.js's row loops to the helper | they then check real rows and hit a hex-strip/hop-count mismatch that exists on master, outside this change Confidence: high Scope-risk: narrow
Hiding both Path and Details left no flex column, so the table shrank to the sum of the short columns (~400px in a 1900px wrapper). distributeColumnWidths now gives the spare width to the last fixed column. That column is marked fit-last and loses its drag handle: it has nothing to resize against, and the handle's 4px overhang (right: -4px) gave the wrapper a scrollbar. Review decisions: - measurableRows, shared with makeColumnsResizable, gets unit tests. Of 20 tables on the nodes, observers and analytics pages, 18 measure identically to master; the Scopes empty state (one colspan row) no longer sizes the Region column to 915px; Route Patterns varies between runs on master too. - #pktTable clears cell min-width: the shared .col-time/.col-path/ .col-observer/.col-scope minimums are for auto-layout tables, and only Chromium ignores them under table-layout: fixed. - fullNames in the URL applies to that page only and no longer overwrites the visitor's saved preference; without the parameter each visit reads the saved preference again. Minor: container resizes are coalesced to one layout per frame; the sizer uses its own data-fit-columns flag instead of makeColumnsResizable's data-resizable; .eslintrc.json globals back in order. E2E: new case hides Path and Details and asserts the table reaches the wrapper's right edge; the grouped-row case fails instead of skipping; fixed sleeps replaced by condition waits except one that must outlast the popover's 150ms close grace; drag tolerance 3px -> 5px. Rejected: table.style.width = '100%' when no flex column is visible | phantom colspan slots take the slack and reappear as a blank strip Rejected: clamp measure() by the cells' CSS min-width | brings Time back to 108px, the waste this PR removes Rejected: write fullNames=0 into every URL | clutters every link for a default-off mode Confidence: high Scope-risk: narrow Not-tested: non-Chromium engines (CI runs Chromium only)
857ca67 to
98c2f7b
Compare
|
Review feedback addressed (commit Rebase Both conflicts you predicted happened, and both were independent additions:
Blocking
Decisions
Minor
Verification
|
|
CI on At 1300px its grid rule gives three columns; one column is what unstyled, block-flow cards give. The harness loaded with To be clear about confidence: I could not reproduce the failure locally (10/10 passes, also with 20× CPU throttling), and among recent failed deploy runs only this one hit it. If you would rather keep that file out of this PR, I can drop the commit and ask for a re-run instead. |
CI run 36735899064 failed "viewport 1440 / wrapper 1300px → side-by-side (≥2 cols): expected ≥2 columns at wrapper 1300px; got 1". At 1300px the grid rule (repeat(auto-fit, minmax(min(100%, 380px), 1fr))) gives three columns; one column is what block-flow, unstyled cards give, so the cards were measured before style.css applied. The harness loaded with waitUntil 'domcontentloaded', which does not wait for a stylesheet on a page without scripts. It now waits for 'load', then for .analytics-charts to compute to display:grid, and fails with a message naming style.css if it never does, so a real CSS regression still fails. Not reproduced locally: 10/10 passes before the change, including with 20x CPU throttling; only this run failed on it among recent failed deploy runs. Confidence: medium Scope-risk: narrow Not-tested: the CI-runner timing that produced the failure
4f0806d to
541bde6
Compare
2 participants
The packets table wastes most of its width on short columns, draws two arrows on grouped rows, and cuts every observer and hop name short. This fixes all three and adds a way to read the full path when it does not fit.
Why the columns were wide
makeColumnsResizablemeasured each column once on load and stored the widths as percentages of the table. Percentages grow with the screen: on a 2000px display "17s ago" sat in a 180px column. The measurement also read the virtual-scroll spacer row — a singlecolspancell as wide as the table — as column 0, so the expand column came out 160px wide locally and about 950px on a live instance with grouped rows.Measured on the fixture at 2000px:
The change
Column packing. A new
fitColumnsToContent(app.js) sizes every column except Path and Details to its widest cell, in px; those two split what is left. Every visible column gets an explicit width and the table gets their sum: a spacer row keeps a slot for each hidden column, and leaving any columnautohands those slots a share of the slack as a blank strip at the right edge.When space is short (detail panel open, narrow window) the widest fixed columns give way first, then Path and Details shrink to 60px, and only then does the wrapper scroll. The split itself is a pure function,
distributeColumnWidths, with unit tests.It re-lays out when the container resizes (detail panel), when TableResponsive hides or reveals columns (a new
table-columns-changedevent), and after sorting or a column toggle. After each rendergrow()widens columns for the rows just inserted and never narrows them, so nothing jitters while scrolling. Drag handles store px; double-clicking one resets it. Path and Details have no handle.makeColumnsResizable, still used by the nodes, observers and analytics tables, now skipscolspanrows when measuring; other rows are measured as before.One expand arrow. A CSS
::before"▶" sat next to the SVG caret, which pointed up when collapsed. The glyph is gone and the caret points right, then down.Full Names. A toolbar toggle next to Hex Paths shows observer and hop names untruncated. It is saved per browser; a link can carry
fullNames=1, which applies to that page without overwriting the visitor's saved preference. Paths stay on one line: the virtual scroller positions rows at one fixed height, so wrapping would make scrolling jump.The "+N" path pill, and a full-path popover. Hops past the column edge were meant to go behind a "+N" pill (#1124), but the pill was appended after the overflowing chips and so sat past the clipped edge: it never showed. It is now sticky at the right edge, opaque on hover, and counts hop chips only (it used to count warning icons too). Hovering or focusing it shows the full path, one hop per line and numbered; click, Enter or Space pins it until an outside click or Escape, and it closes when the page changes or its row scrolls away. Clicking the pill no longer selects the row underneath.
Two related fixes.
/api/observersresolves show raw 64-char pubkeys.loadObserversnow re-renders and re-measures, instead of leaving Observer at the pubkey's width (~600px under Full Names).Anyone who had dragged packet columns gets the new defaults once: the old percentage key is dropped.
Performance
grow()runs after everyrenderVisibleRows, so it is on the hot path. Measured in Chromium on the fixture, 300 iterations, three runs:Rangerect reads, only the inserted rowsBefore narrowing
grow()to inserted rows, one scroll step with 92 rendered rows made 5,472 rect reads. The DOM row count is bounded by the virtual-scroll window, so the cost does not grow with the packet count.distributeColumnWidthsis a binary search over at most ~10 columns.Tests
tests/e2e/test-packets-compact-columns-e2e.js, 16 cases: short columns stay under fixed px bounds at 1920px; Path and Details take the freed width; hiding both still fills the wrapper; no blank strip and no horizontal scroll, with and without the detail panel, at 1920, 1100, 1000 and 375px; responsive reveal and re-hide refit; stale percentage widths dropped; drag follows the pointer and double-click resets; expanding a group leaves widths alone; Full Names untruncates, keeps paths on one line, persists and round-trips through the URL; the Observer column shrinks back when observer names arrive late; the pill popover lists every hop vertically, closes when the pointer leaves, pins on click without selecting the row, closes on Escape and on navigation, and opens on keyboard focus. Registered inscripts/non-unit-tests.jsonanddeploy.yml.masterthe suite fails where it should, and restoring each fix's old behaviour (child-row indent, the late-observer refit) turns its case red.tests/unit/test-packets.js:distributeColumnWidths(room to spare, rounding, widest-first capping, floors, dragged columns, the 60px minimum, overflow), the single chevron and no::beforeglyph, Full Names on flat and grouped observer cells, and the Full Names CSS.tests/unit/test-xss-escape-sinks.jsnow builds the realobsCellNamefrom source: escaping moved into it. Removing itsescapeHtmlturns both observer-cell tests red.test-issue-1189-composed-cell.jsandtest-issue-2012-clear-filters-selection.jsevaluate packets.js fragments with injected helpers and neededobsCellName/showFullNamesadded.test-issue-1956-release-routing.jscould not create a temp directory in my sandbox and is unrelated.Browser validation
Checked in Chromium against the fixture at 2000, 1920, 1280 (panel open and closed), 1100 and 375px, and with the seeded grouped row expanded and collapsed. Screenshots taken at each step; the table fills its container exactly, grouped rows show one arrow, and the pill popover lists full names.
Not done, deliberately