Sitelet https://github.com/feldera/feldera/pull/6939
Skip to content

New log UI - #6939

Draft
Karakatiza666 wants to merge 6 commits into
mainfrom
new-log-ui
Draft

New log UI#6939
Karakatiza666 wants to merge 6 commits into
mainfrom
new-log-ui

Conversation

@Karakatiza666

Copy link
Copy Markdown
Contributor

No description provided.

common-ui had no test project of its own, so anything covering its components
had to live in web-console and import through the package barrel. That barrel
pulls in Monaco, and the log-search spec was carrying a comment explaining why
its pure functions had to ride along in a browser project belonging to another
package.

Give common-ui the project instead: a Playwright-backed browser project, and
`tests/logSearch.svelte.spec.ts` moved in from web-console to import `$lib`
directly. `requireAssertions` is on because the suites that follow assert on
geometry, where a spec that measures nothing passes silently.

CI runs it ahead of the web-console suite, capped at four workers: vitest's
autodetection sees every host core and oversubscribes the pod, which starves
browser tests into per-test timeouts.
The log viewer that follows remounts its virtualiser whenever a fresh size
cache arrives. 0.48.6 starts observing from a `tick()` microtask with no guard,
so a virtualiser destroyed before that microtask resolves still attaches its
observers to a detached container; 0.50.4 carries the fix (inokawa/virtua#914).

Bumped in every workspace that depends on virtua so the monorepo resolves one
copy. The API surface used here is unchanged between the two versions.
`useReverseScrollContainer` needed a fixed number of scroll passes to reach the
bottom, which is why the scroll-to-bottom button had to be pressed twice: the
target is `scrollHeight`, scrolling there mounts more content, and that moves
the target. It also read user intent off scroll geometry, so a virtualiser
correcting its own `scrollTop` after measuring a row cancelled the scroll that
was doing the correcting.

`useStickToBottom` settles by iterating to a fixed point instead, and takes
release exclusively from input events, which carry their author by construction.
Two collaborators keep the rest out of the anchor itself:

  releaseGestures    what counts as the user taking over (wheel, keys, touch)
  viewportDormancy   when the container's geometry may be believed at all

Dormancy is the tab-switch case: a hidden container keeps receiving content
against a frozen viewport, and the catch-up scroll event on return used to read
as "the user scrolled up" and detach a streaming log for good.

Both anchors are exported while the migration runs. `TabAdHocQuery` moves over
here because it drives no FAB; the rest share `ScrollDownFab`, whose contract
changes with the log viewer.
…iewport

The rendered window was `visibleCount - 1` rows, added as a workaround for what
was recorded as a flickering issue when scrolling to the bottom of a long list.

The flicker was the missing row. `indexOffset` can start half a row above the
viewport and the count can round half a row down; `visibleCount` carries two
spare rows to pay for both, so dropping one leaves a strip up to a row tall
blank along the bottom edge at some offsets — which reads as the list failing
to keep up rather than as arithmetic.

The spec sweeps two full row heights of scroll offsets, since a window one row
short only leaves a gap at some offsets within the rounding period.
The decoder emitted each line with its `\n` or `\r\n` still attached, and every
consumer then had to undo it — the log host was passing an empty join separator
to the copy builder precisely to avoid doubling them.

A line that carries its own newline is not a line. Worse for the log viewer that
follows: its height model predicts wrapped row heights over printable ASCII and
tabs, and declines anything else. A surviving terminator puts every row outside
that grid, so the model declines all of them and the size cache is empty for the
whole stream. A stray `\r` is the nastier half — invisible in the rendered row,
identical in effect.

The `\r` test is bounded by `lineStart`, so an empty line, whose whole record is
the terminator, cannot read the previous line's last byte.
`LogList` virtualised by line and could not keep a row mounted, which cost it
three separate behaviours: a selection collapsed the moment either endpoint
scrolled out, a copy silently truncated to whatever happened to be mounted, and
a search highlight had to be re-attempted every frame until the scroll happened
to mount its row. Row heights depend on the container width, and a virtualiser
only measures what it mounts, so halving the width left the reported scroll
range 42.8% short on a 600-line corpus of mixed-length lines — two fifths of
the log unreachable for the rest of the session.

LogView renders in 50-line chunks and seeds the virtualiser with a full size
cache instead of letting it discover heights. Monospace rows make wrapped
heights arithmetic, so the offsets are right in the rebuild's first frame; that
is what lets a reading position be restored with one assignment and a search
jump land without a convergence loop. The parts are separated by concern:

  logChunks       cutting on absolute-line boundaries, so keys survive eviction
  logRowMetrics   the wrap model and the predicted layout
  logLayout       when to re-predict, and restoring the view after the rebuild
  logLineAlign    converging a search jump on the centre of the viewport
  selectionPin    holding the chunks a live selection touches
  logCopy         reconstructing clipboard text from the source lines
  selectScope     replaces 682 lines of virtualSelect with native selection

Adopting the anchor everywhere finishes the migration started two commits back:
`ScrollDownFab` now reads the raw `stuck` value, so it disappears on the press
that re-arms rather than a frame later, and `Query` and `ChangeStream` move with
it. `useReverseScrollContainer`, `userSelect` and the already-orphaned
`ReverseScrollList` go with their last consumers.

Verified against the running console on a 6000-line pipeline log as well as the
suites: streaming stays anchored, one press reaches the true end, and a width
change never paints a frame away from it.

// A pointer press begins a fresh selection, which is the one unambiguous signal that the
// select-all is over. Normal pinning resumes from the next selectionchange.
const onPointerDown = () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pointer press is the only thing that ends a select-all, and isHeld() returns true unconditionally while selectAll is set — which logLayout.reseed() treats as "do not rebuild". So after Ctrl+A a keyboard-only user can resize the window (or toggle the gutter) as much as they like and the size cache is never re-predicted; the 42.8%-stale-heights behaviour this PR fixes comes back until some pointerdown lands anywhere in the document. Esc / a caret-moving key / focus leaving the log would all be reasonable additional exits.

No spec covers "a live selection defers the reseed, and the reseed happens once it clears" in either direction — worth one, since the retry path in logLayout hangs off pinned.length and selectAll changing and a select-all changes neither on its way out except through this handler.

}
}

const onKeyDown = (event: KeyboardEvent) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stickToBottom.svelte.spec.ts exercises wheel (up/down) and all four touch cases, but nothing covers the keyboard branch — RELEASE_KEYS and this handler are entirely untested. Cheap to add alongside the wheel specs, and it is the one release path that a refactor could silently delete without turning anything red.

Minor: the header says "Every listener is passive bar the keyboard one", but keydown never calls preventDefault() either, and passive has no meaning for keydown — the asymmetry reads as deliberate when it is not.

Comment on lines +307 to +313
it('paints no frame away from the end when a width change rebuilds the view', async () => {
const { component, scroll } = mountFixture({
initialLines: mixedCorpus(4000),
initialWidth: 800
})
component.stick()
await frames(30)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one and stays anchored when the container is made shorter (l.343) are the only tests in the file without an explicit { timeout: 120_000 }, so they run on vitest's 5s default — while mounting 4000/3000 lines and then awaiting 150 / 70 requestAnimationFrame ticks plus a 120ms sleep. At 60fps that is already ~2.5s of pure frame budget; CI runs these four workers at a time in a container, and headless Chromium's rAF cadence drops well below 60fps under that load. Given the zero-tolerance flaky-test rule, please give both the same explicit timeout as their neighbours rather than leaving them one slow frame away from red.

Comment on lines +317 to +320
style:counter-set={showLineNumbers
? `line ${chunk.startLine + i + 1}`
: undefined}
data-line={chunk.startLine + i}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gutter counts chunk.startLine + i + 1, which is the index into lines, not the absolute line number — it ignores firstLineIndex. So a streaming log whose circular buffer has evicted from the front renumbers its rows from 1 on every eviction, which is the one case where a line number is worth anything.

Latent today (LogsStreamList passes firstLineIndex but not showLineNumbers; BundleLogsView is the reverse), but the two props are public and combine, so this is a trap for the next host. firstLineIndex + chunk.startLine + i + 1 is the fix; either way the prop docs should say which numbering they mean.

@mythical-fred-oss

Copy link
Copy Markdown

Reviewed against feldera-pr-checks. This is a well-built PR — the commit messages are genuinely exemplary (each explains the why, with measured numbers), the six commits are cleanly separated, and the test suite is unusually thorough for frontend work.

What I ran

Command Result
bun run test-unit in js-packages/common-ui (×4) 122/122 passed, 9 files — no flakes across repeats
bun run test-unit in js-packages/web-console 566/568; the 2 failures are 15s timeouts in AdminPage.svelte.spec.ts and MetricsTables.svelte.spec.ts, untouched by this PR and almost certainly this loaded runner. The touched MonitoringPanel spec passed
pre-commit run --files <50 changed files> Passed

Gates: no pull_request_target; no Rust/unsafe/connector/format/SQL-type surface, so those gates don't apply; virtua 0.48.6→0.50.4 is a minor bump justified by a linked upstream fix, and the new devDeps (playwright pinned to the CI container's v1.58.2 tag, vitest, vitest-browser-svelte) are all MIT/Apache-2.0. The PR body is empty — the manual-testing rule is satisfied only because the last commit message carries the verification note (6000-line pipeline log) and the coverage is strong; please copy that into the description.

Uncovered cases beyond the inline notes: a live selection deferring reseed (and the retry once it clears) is untested in both directions; the releaseGestures keyboard branch has no spec at all; and sliceLinesForCopy throws TypeError on result[0] for an empty or out-of-range slice rather than returning '' — not obviously reachable through resolveCopySlice, but a one-line guard is cheaper than proving it isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant