feat: new topic messages page behind enableNewTopicMessagesPage flag - #2588
Open
jvorcak wants to merge 26 commits into
Open
feat: new topic messages page behind enableNewTopicMessagesPage flag#2588jvorcak wants to merge 26 commits into
jvorcak wants to merge 26 commits into
Conversation
Redesigned messages UX (from the Console Messages UX prototype): - Read scope popover (newest/oldest/offset/timestamp) with live tail in the menu, continuous pagination for newest/oldest only, and an illustrated "how reading starts" doc sheet - Filter bar with typed tokens (partition:1, offset>5, key:abc), autocomplete with value suggestions and ghost completion, keyboard chip editing (ArrowLeft unwraps a badge into editable text, Enter recommits in place), and JS predicate filters with live preview - Field filters, partition, quick search and selected message persist in the URL; JS filters stay in sessionStorage - Message detail as docked resizable panel or expanded sheet, with persisted view state (mode, widths, section expansion) and a metadata table, headers grid and troubleshoot reports - View settings sidebar: row density, drag-reorderable columns with per-column config (timestamp format, deserializers, preview fields) - Quick info stats rebuilt on the registry Stat component Fixes surfaced while building it: - assignDeep now replaces arrays wholesale; the index-wise merge mutated shared elements through the uiSettings sync and corrupted reordered arrays (duplicate/lost message columns after drag'n'drop); getMessageColumns heals already-corrupted persisted entries - SidebarInset gets min-w-0 so wide content (messages table) scrolls inside its container instead of widening the page past the viewport
Standalone dev no longer seeds initialFlags from constants.ts defaults overlaid with debug-dialog overrides; back to only E2E globals.
Replace arbitrary pixel font sizes (text-[10px], text-[11px], text-[12.5px], text-[13px]) with the semantic text-caption/text-body-sm/text-label utilities from the redpanda-ui typography scale.
Contributor
🚨 Registry drift detectedApp:
Components needing attention
Refresh command: bunx shadcn@latest add @redpanda/combobox @redpanda/sidebar @redpanda/theme --overwrite🎨 Off-token colours (palette literals)Use semantic tokens (
🔢 Ad-hoc utility classes (arbitrary values)These bypass the design tokens. Prefer a named scale entry or add a new token.
Generated by lookout audit-changes. |
StatGroup now uses CSS subgrid so stat values align on a shared baseline across a row even when labels wrap.
Drop the client-side predicate preview sandbox (hit list, error highlighting, apply-gating) from the JS filter dialog. The backend already validates and applies the predicate on save.
jvorcak
marked this pull request as ready for review
August 7, 2026 16:26
Collaborator
Author
|
The filter bar is still under developments, other things can be reviewed |
…th pill highlights Replaces the chip-based filter bar with a single real text input whose caret behaves natively (click/arrow/select/backspace anywhere) while recognized field:value words are painted with a subtle pill highlight overlay — no discrete chip elements, no synthetic keyboard "browse" state. - New utils/filter-line.ts continuously parses the whole line into partitionId/fieldTokens/quickSearch, replacing the old commit-on-space model - Quote-aware value parsing (`field:"multi word"`) in filter-token.ts - Suggestions/ghost-completion now scope to the word under the caret - JS predicates keep a small separate chip (can't live inline in free text); js:<label> now seeds the new filter's name instead of its code when the typed text doesn't look like an actual JS expression - Fixes a duplicate-filter bug where a stray Enter after committing a token could reinsert it
…ogic Follow-up review pass on the filter bar rewrite: - Match the JS filter chip's border-only look on the highlighted pills (outline instead of a flat bg-muted fill) and extract a shared FILTER_BADGE_TEXT_CLASS so the chip, the pills, and the suggestion dropdown's labels can't drift out of sync again (they'd already diverged: 13px chip text next to 14px pill text) - Extract sameFieldTokens into filter-token.ts, shared by the filter bar's own resync check and use-messages-url-state's fieldTokensParser.eq (previously two independent copies of the same array-equality check) - parseFilterLine now returns tokenRanges directly instead of the filter bar re-tokenizing/re-parsing the same text a second way for the highlight overlay - Swap arbitrary text-[10.5px]/text-[11px] for the registry's text-caption utility in the suggestion dropdown's headers and sub-labels - Use registry Button (secondary-ghost, icon-xs) for the clear-all control instead of a hand-rolled icon button that happened to reinvent the same preset
…he registry Chip, HighlightedInput, and Listbox are now generic, domain-agnostic registry components (src/components/redpanda-ui/components/) — filter-bar.tsx keeps only the filter-parsing/suggestion logic, with rendering served from the registry.
…pe popover Renders the calendar/time panel as normal flowed content instead of a click-to-open popup nested inside the already-open read-scope popover, with a Unix timestamp label, a registry ToggleGroup for Local/UTC, and matching indigo-600 styling between the selected day and the Now button. Also excludes .playwright-mcp/ from rsbuild's dev-server watcher — it was triggering rebuilds (and occasional rspack panics) on every screenshot/log written into the project root while driving the dev server with Playwright.
malinskibeniamin
left a comment
malinskibeniamin
left a comment
Contributor
There was a problem hiding this comment.
Inline findings: P1/P2. No P0/P3 identified.
…d-scope popover Blocker: partition:<non-numeric> produced NaN that desynced the filter bar's resync guard and wiped whatever the user had typed. Majors: - Removed stat.tsx, which shadowed the newer subgrid-aligned stat/index.tsx via file-before-directory module resolution — it was never actually loaded. - Chip's remove button didn't stop propagation, so removing a JS-filter chip bubbled into the filter bar's container and reopened the suggestion dropdown. - Listbox had no ARIA roles; wired role="combobox"/aria-expanded/aria-controls/ aria-activedescendant on the filter input to match. - The filter bar's resync effect couldn't tell "still catching up to our own emission" from "genuinely external," so a parent whose three pieces of state echo back across separate renders (rather than atomically) could destroy in-progress typing. - tokenQueryText wasn't actually lossless — values with whitespace or embedded quotes changed meaning after a URL round-trip. Added backslash-escaping. - An unbalanced/stray quote could swallow the rest of the typed line into one token, silently discarding every filter after it. - offset:N matched via substring containment (offset:12 matched 1123); it now means equality, and the existing-but-unreachable offset comparison-suggestion UI is wired up to the current word-under-caret architecture. - neq returned false (excluding the message) when the field was absent, when "does not equal" should keep it. - The 'c' row-copy shortcut fired on Cmd/Ctrl+C too, clobbering the user's own clipboard selection with the row's JSON. - Keyboard nav stayed armed while the view-settings panel, docs sheet, or read-scope popover was open; ReadScopePopover now reports its own open state via a new onOpenChange prop so all of these can gate on it. - Toggling continuous pagination didn't reset pageIndex like every sibling read-scope handler, so staying on a later page could show a permanently blank table with no page controls to recover with. Every fix is paired with a regression test that fails on the prior behavior and passes with the fix (verified via temporary revert for each).
- Disable partition/max-results/deserializer controls while live tail is streaming, since edits there update state the running stream never picks up. - Stop showing JS filter chips as active when continuous mode has actually dropped them from the request; reset pageIndex when field tokens or quick search change. - Fix a shared-closure bug where hovering any filter suggestion row highlighted the last row instead of the hovered one. - Restrict the partition filter to exact match — reject partition>/</!= instead of silently collapsing them to equality.
…lter - Keyboard nav (visiblePageKeys) now skips re-sorting in continuous mode, matching the table's own sortingDisabled — previously it kept applying urlState.sorting even when the table fell back to server order, so j/arrow-key selection landed on a different row than what was displayed. Extracted messageKey into its own leaf module so the new pure-logic test doesn't pull in sonner/React transitively. - parseFilterInput/parseFilterLine take an optional partitionCount and reject an out-of-range partition:N (e.g. partition:9999 on a 3-partition topic) instead of sending it through as a real request. Threaded through FilterBar from topic.partitionCount.
…e gate Swap the enableNewTopicMessagesPage check from the custom feature-flag provider hook to the shared isFeatureFlagEnabled helper.
malinskibeniamin
left a comment
malinskibeniamin
left a comment
Contributor
There was a problem hiding this comment.
Automated /review: 3 finding(s).
offset<=N and offset>=N previously fell through to plain text since parseOperator matched the bare </> prefix first, stranding a leading = in the value. Adds gte/lte to FilterOp, wires them through parsing and client-side matching, and centralizes the operator<->symbol mapping in one table shared by parsing and display formatting.
- Fix "Download Record" silently doing nothing: append the anchor to the DOM before click() (required in Firefox) and revoke the object URL on a later tick instead of racing the download. - Fix keyboard nav (j/k, arrow keys) selecting a different row than what's rendered: derive which column is sortable from a single isSortableColumnId predicate shared by the table's enableSorting and visiblePageKeys, instead of independently guessing and defaulting unrecognized sort ids (stale key/value sorts from the legacy page) to an offset tiebreak the table itself never applies. - Fix column-visibility checkboxes having no accessible name by wiring each one to its label via aria-labelledby. Also drops .playwright-mcp/ from .gitignore per review feedback to use the Playwright CLI instead of MCP.
…-check on quick search - messages-footer.tsx: read onLoadMore/isLoadingMore through refs instead of effect deps, so a fresh inline onLoadMore from the parent no longer tears down and rebuilds the IntersectionObserver every render (which re-fired against the sentinel's current intersection state and auto-loaded the entire topic with no scrolling). - use-client-filters.ts: thread partitionCount through to parseFilterInput so quick search rejects out-of-range partition tokens the same way the filter bar already does, instead of silently blanking the table.
malinskibeniamin
left a comment
malinskibeniamin
left a comment
Contributor
There was a problem hiding this comment.
Automated /review: 2 finding(s).
| } | ||
| flush(); | ||
| setIsLoadingMore(false); | ||
| setPhase('done'); |
Contributor
There was a problem hiding this comment.
Priority: P1
A superseded stream finalizes the state of the stream that replaced it.
runStream starts by calling stop(), which aborts the previous controller, then installs its own. But the previous run's finally block still executes unconditionally — after the new run has already set phase='connecting', cleared bufferRef, and reset stats. The identity check just above guards only abortControllerRef; every state write below it is ungated:
flush()publishes whatever is in the now-sharedbufferRefsetIsLoadingMore(false)setPhase('done')← this linesetBackendPhase(null)
The abort rejection surfaces on a later microtask than the synchronous setup of the new run, so the stale finally reliably lands after it. Reachable on ordinary flows in topic-messages-view.tsx: toggling live tail (the live effect's start aborts the paged stream, whose finally then reports done while live tail is still connecting), changing partition/deserializer/read scope within the 100ms debounce window, and clicking "Load more" while an auto-search restart is in flight.
Consequence: the toolbar and table stop showing progress for a request that is still running — isSearching goes false, the backend phase label disappears, and a continuous-mode load can lose its loading state. With flush() publishing a buffer the new run just emptied, the table can also briefly render as "done, no messages" for a live search.
Correction: capture a generation counter (or compare abortControllerRef.current === abortController) and gate all of the finally writes plus every setPhase/setStats/setBackendPhase inside the for await loop on it, so only the current run may write state. Add a regression test that starts search A, starts search B before A settles, and asserts phase never becomes 'done' from A.
Verify: cd frontend && bun vitest run src/components/pages/topics/messages/hooks/use-message-search.test.tsx
Automated /review.
Collaborator
Author
There was a problem hiding this comment.
It should be fixed now
| // header's top-right — keep the last column's title clear of them. | ||
| className={cn(canSort && 'cursor-pointer select-none', 'last:pr-20')} | ||
| key={header.id} | ||
| onClick={canSort ? header.column.getToggleSortingHandler() : undefined} |
Contributor
There was a problem hiding this comment.
Priority: P1
Column sorting is mouse-only — no keyboard path exists.
The sort affordance is an onClick on TableHead (a plain <th>) with cursor-pointer select-none styling. There is no role="button"/<button> wrapper, no tabIndex, and no onKeyDown, so the header is not focusable and Enter/Space do nothing. aria-sort correctly announces the current state, which makes the omission worse: a screen-reader or keyboard-only user is told the table is sortable but has no way to sort it. The page's own useKeyboardNav covers row movement and copy, not sorting, so no alternative route exists.
This also conflicts with the repo standard for click handlers on non-interactive elements (role + tabIndex + keyboard handler).
Consequence: WCAG 2.1.1 (keyboard) failure on a primary table interaction of a new customer-facing page.
Correction: render the sortable header label as a real <button type="button"> inside the <th> (keeping aria-sort on the th), move onClick onto it, and give it focus-visible:ring-*; keep the plain <th> for non-sortable columns. Add a test that tabs to the Timestamp header and presses Enter, asserting onSortingChange fired.
Verify: cd frontend && bun vitest run src/components/pages/topics/messages/table/messages-table.test.tsx
Automated /review.
Collaborator
Author
There was a problem hiding this comment.
I think this is more like a component library issue?
Contributor
|
Left some feedback with some fairly issues via Slack. Let me know when those are resolved and I can check this branch again. |
Adding/editing/toggling a JS filter while live-tailing updated the filter chip UI but never restarted the stream, since the live-tail effect intentionally excludes searchParams from its deps. Add filterInterpreterCode as an explicit dependency so committing a JS filter restarts the tail with the new predicate applied.
Partition changes updated urlState.partitionId while live-tailing but never restarted the stream, for the same reason as the JS filter chip issue: the live-tail effect intentionally excludes searchParams from its deps. Add partitionId as an explicit dependency so changing the partition restarts the tail on the new partition, and re-enable the read-scope popover's partition select while live (it now actually works, matching the filter bar's partition:N token).
…nel and abort superseded searches promptly Timestamp mode holds startTimestamp at -1 for one render until the date picker's mount effect corrects it to "now" — the auto-search effect could fire on that sentinel and send an invalid timestamp before the correction lands. Skip searching until it's initialized. Also stop() the stream in this effect's cleanup, not just clear the pending debounce timer: if the timer had already fired and a search was in flight, a later param change only aborted it implicitly whenever the next start() happened to fire. Mirrors the live-tail effect's own cleanup.
… live, and clamp out-of-range pages Three P1s from review: - Continuous + Newest lost newest-first ordering. Continuous mode disables the table's own sort entirely, and the backend's "Newest" origin emits messages grouped per partition (collected and reversed per partition, never interleaved across partitions by timestamp) rather than globally chronological. Add orderForContinuousNewest (timestamp desc, offset desc tiebreak) and apply it right where rows are finalized for both the table and keyboard nav, matching the legacy page's force-sort for this exact scope. - Live tail claimed to be "streaming" long after the backend silently ended it. The backend bounds a "from newest" consume to maxResults and then completes — it isn't an endless stream. The live-tail effect now restarts itself (with append:true, so the table doesn't reset) on every natural completion for as long as the toggle stays on, instead of going stale while the toolbar still shows a live indicator. Threads an `append` option through useMessageSearch's start() to support this. - An out-of-range `page` (stale deep link, or filters shrinking the row set) rendered a permanently blank table: tanstack's autoResetPageIndex is deliberately off (URL-driven pagination), and continuous mode's footer has no pager to click back with. Clamp pageIndex against the real page count once a fetch settles (`done`), so a page number that's still loading isn't wiped out early but a genuinely invalid one gets corrected.
…over a newer one
runStream's finally block only guarded the abortControllerRef nulling against
being superseded — flush(), setIsLoadingMore, setPhase('done'), and
setBackendPhase ran unconditionally. Since the previous run's abort rejection
surfaces on a later microtask than the new run's synchronous setup (stop() +
fresh AbortController), the stale run's finally reliably lands after the new
one has already reset state, flipping phase back to 'done', clearing the
backend phase label, and publishing whatever's now in the shared bufferRef —
reachable via toggling live tail, changing partition/deserializer/read scope
within the debounce window, or Load more racing an auto-search restart.
Gate the whole finalization on whether a newer run has taken ownership of
abortControllerRef, not just the ref-nulling line. Added a regression test
that starts A, supersedes it with B before A settles, and asserts A's delayed
completion never flips phase back from B's in-progress state.
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
3 participants
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.
Redesigned messages UX (from the Console Messages UX prototype):
field:valuewords pill-highlighted (parsed continuously as you type, no commit step), word-scoped autocomplete/ghost-completion, and JS predicate filters as a separate chipFixes surfaced while building it:
Before:
After:
