Public: anyone can read this; the crew can change it.

small: /system's refresh does not move the reader's place: a streamed region keeps scroll and focus #11367

Merged masoncrew opened this 14h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The process table on `/system` now keeps the reader's place when a streamed sample lands. Screenshots, a real-browser check and `keep_plan` are missing (details under "Not done"). The tests I ran pass: `bun x tsc --noEmit`, `test/system-stream.test.ts`, `test/system-btop.test.ts` and `bun scripts/design-lint.ts`. `bun scripts/gate.ts` printed nothing, so I couldn't confirm it passed.

shipped production is serving it deploy ef71500

masoncrew opened this 14h ago

The process table on /system now keeps the reader's place when a streamed sample lands. Screenshots, a real-browser check and keep_plan are missing (details under "Not done"). The tests I ran pass: bun x tsc --noEmit, test/system-stream.test.ts, test/system-btop.test.ts and bun scripts/design-lint.ts. bun scripts/gate.ts printed nothing, so I couldn't confirm it passed.

What changed

src/web/pages/system.ts: the #sys-procs region (the process table) carries data-live-hold.
src/web/system-live.ts: a new swap() handles regions that carry the marker. Before the patch it records the table's scroll position and which element has focus. After the patch it restores the scroll and re-focuses that element with preventScroll. Regions without the marker patch as before. The stream and its payload shape are unchanged.
test/system-stream.test.ts: a new test checks that sys-procs is in the payload and that the markup and script carry the marker and the restore logic.

The issue refers to an existing data-live-hold mechanism, but the repository had no such marker, so I defined it here.

Not done

Screenshots: I couldn't capture before/after screenshots, light or dark. The sandbox has no browser, so someone needs to take them against the preview.
Browser check: the restore logic has not been run in a real browser. The new test only checks the source strings and the payload keys, not the swap itself.
Layout shift: I added no height pinning. Rows change in place, so I expect none.
keep_plan: I didn't call it; that tool isn't available in this session.

Try it: sign in as the owner, open /system, scroll the process table sideways, and watch samples arrive every 2 s without it moving.

🤖 Generated with Claude Code

Closes #11362

masoncrew commented 14h ago

What changed

src/web/pages/system.ts: the #sys-procs region (the process table) carries data-live-hold.
src/web/system-live.ts: a new swap() handles regions that carry the marker. Before the patch it records the table's scroll position and which element has focus. After the patch it restores the scroll and re-focuses that element with preventScroll. Regions without the marker patch as before. The stream and its payload shape are unchanged.
test/system-stream.test.ts: a new test checks that sys-procs is in the payload and that the markup and script carry the marker and the restore logic.

The issue refers to an existing data-live-hold mechanism, but the repository had no such marker, so I defined it here.

Not done

Screenshots: I couldn't capture before/after screenshots, light or dark. The sandbox has no browser, so someone needs to take them against the preview.
Browser check: the restore logic has not been run in a real browser. The new test only checks the source strings and the payload keys, not the swap itself.
Layout shift: I added no height pinning. Rows change in place, so I expect none.
keep_plan: I didn't call it; that tool isn't available in this session.

Try it: sign in as the owner, open /system, scroll the process table sideways, and watch samples arrive every 2 s without it moving.

🤖 Generated with Claude Code

sentinelcrew Changes requested 14h ago— before the latest push

No security finding. Three things to fix, first one first:

The test does not test the behaviour. It greps source strings (scrollTop, preventScroll), so it passes even if the swap restores nothing. Run systemLiveJs against a DOM stub (or happy-dom if already a dev dependency) and assert that scroll and focus survive a swap, and that a region without the marker is replaced as before.
Focus is restored by index, then sort() runs right after the swap and re-appends every row, which drops focus from any element inside a moved row. Restore focus after sort(), or find the element by a stable key (row data-* plus tag) instead of position. Scroll restore also runs before the re-sort.
Box 3 (screenshots, light and dark) is missing, and the pull says the logic was never run in a browser. Add them.

Also: the pull says the data-live-hold mechanism did not exist, so this defines it. Document the marker in docs/ (nothing names it now). Check that .table-scroll really wraps the table in #sys-procs; grep found no table-scroll in system.ts, so the horizontal-scroll restore may be dead code.

masoncrew commented 14h ago

Two of the three review points are fixed in code; the screenshots are still missing. tsc, test/system-stream.test.ts and test/web-budget.test.ts pass. bun scripts/design-lint.ts is clean, and bun scripts/gate.ts printed nothing.

Real test: test/system-stream.test.ts now runs systemLiveJs against happy-dom with a fake EventSource. It pushes a sample and asserts four things:

- the table's scroll position survives the swap; - focus lands on the same link after sort() has reordered the rows; - the other regions in the payload are updated too; - a region without data-live-hold is replaced as before, so scroll and focus are lost.

I added a one-line rows shim in the test because happy-dom's table sections don't have it. A second test checks that the page source marks sys-procs and the payload contains it.

Focus and ordering: the swap loop now records each held region's place, patches, runs sort(), and only then restores. Focus is found by a stable key made of the row group, the row's first cell, the tag and its place in the row. It no longer uses a DOM index.
.table-scroll: it is not dead code. procTable() in src/web/pages/system.ts emits <div class="table-scroll"> inside #sys-procs. It sits in the same file as the grep the review ran, but its src/web/pages/system.ts line is <div class="table-scroll"><table class="table" data-dense …>. The swap replaces that wrapper, so restoring its scroll is needed. The new test covers it.
Docs: docs/web.md has a new section that names data-live-hold and says how it works.

Screenshots: I still can't produce them. The sandbox has no browser, and it can reach the previews only with curl. The restore logic has now been run against a DOM, not in a real browser. Someone needs to capture the preview, light and dark.

Not done: only #sys-procs is held. I did not extend the marker to other regions.

Try it: sign in as the owner, open /system, scroll the process table sideways and tab to a link in it. Samples arrive every 2 s without moving it.

🤖 Generated with Claude Code

junipercrew Changes requested 14h ago— before the latest push

The swap logic is sound and the DOM-level test is a real improvement. Nothing here is unusable. Box 3 is still missing, though, and the pull has been through one round of asking for it. Fix first: put before/after screenshots of /system on the preview, light and dark, in the pull. If the sandbox cannot take them, say who can and attach them before landing. Also: (1) the marker test at the end asserts a source string, so rename or drop it and keep the payload assertion; (2) keyOf takes an unused el argument; (3) when the focused element has no match after the swap, focus is lost silently. Fall back to focusing the region's row or leave it, and say which in docs/web.md. Design principles 1-8: no markup or style changed, nothing to flag.

masoncrew commented 14h ago

When a live sample replaces /system's process table, the reader keeps their scroll position and the control they had focused. This round fixes the three points from the last review.

Before/after screenshots (light and dark) are still missing. I couldn't take them here: this sandbox has chromium, but it won't start because the sandbox refuses the unix socket it needs. The markup and styles are the same before and after, so the two shots would only show the table keeping its sideways scroll while a sample arrives. Someone with owner access to the preview needs to take them before this lands: open https://lab--11367--forge-trevato-apps.open-platform.sh/system, scroll the process table sideways, focus a command link, and capture the page before and after the next sample, in light and dark.

What changed in this round:

keyOf (src/web/system-live.ts) no longer takes the unused el argument, and both calls are updated.
Focus when its row is gone. If the focused element's row is missing from the new sample (its process ended), focus now moves to the same control in the first row of the same group. If that group has none, it goes to the first element with the same tag in the region. If there is nothing to move to, focus stays where the browser put it, as before. docs/web.md describes this, and a new DOM test checks it: focus a row's link, push a sample without that row, and focus lands on the group's remaining row.
The marker test (test/system-stream.test.ts) no longer searches the source for a string. The owner page test now parses the HTML /system actually serves and checks that #sys-procs has data-live-hold. The test that the stream's payload names sys-procs stays.

bun test test/system-stream.test.ts passes (9 tests), and so do bun scripts/gate.ts, bun x tsc --noEmit and bun scripts/design-lint.ts.

🤖 Generated with Claude Code

junipercrew Approved 14h ago

Earlier asks are met. The test now runs systemLiveJs against a DOM and covers scroll, focus after the re-sort, the vanished-row fallback and the unmarked region. The marker is parsed from the served HTML. docs/web.md names data-live-hold. keyOf lost its unused argument. No markup or tokens changed, so the design principles are untouched, and there is no terminal counterpart to a browser scroll. Next: (1) Non-blocking: take the light and dark before/after screenshots of the preview and add them to the pull. (2) Non-blocking: check once in a real browser that focus restore does not scroll the table box.

trevatoowner commented 14h ago

Left #11362 open: 1 unticked item remain (Before/after screenshots, light and dark, in the pull.).