Skip to content

UX-908: fix record expansion slowness on master#2266

Merged
malinskibeniamin merged 3 commits intomasterfrom
UX-908-release-3-fix-record-expansion-slowness
Mar 9, 2026
Merged

UX-908: fix record expansion slowness on master#2266
malinskibeniamin merged 3 commits intomasterfrom
UX-908-release-3-fix-record-expansion-slowness

Conversation

@malinskibeniamin
Copy link
Contributor

@malinskibeniamin malinskibeniamin commented Mar 6, 2026

What

Optimize topic message expansion for object/protobuf payloads on master while keeping Monaco in the read-only JSON viewer path.

Screenshot 2026-03-06 at 15 49 49 Screenshot 2026-03-06 at 15 50 03

Why

The topic message expansion slowdown is still rooted in cumulative frontend work: mounted Monaco viewers in expanded rows, extra rerenders in expanded-message rendering, and repeated JSON.stringify(...) work in object previews. master already has a modular message-display structure, so this backport applies the same behavioral fix to those current files.

How

  • keep Monaco in kowl-json-view, disable automaticLayout, and manually relayout only when the container size actually changes
  • preserve the existing large-payload Monaco tuning on master and layer the read-only viewer optimizations on top
  • memoize ExpandedMessage in the modular message-display path and move row-specific callbacks inside it so stable parent props can skip rerenders
  • reuse cached keyJson / valueJson in the key/value preview components instead of re-stringifying payload objects during render
  • add focused regression tests for the Monaco viewer and the modular message-display path
  • add a lightweight canvas mock in frontend/vitest.setup.ts so the new tests run cleanly in this branch’s jsdom environment

Acceptance Criteria

  • expanding multiple protobuf/object rows no longer incurs large incremental frontend stalls from the read-only Monaco path
  • object preview cells reuse cached JSON strings instead of re-stringifying payload objects during render
  • expanded message rows do not rerender when their props are unchanged
  • the Monaco message viewer still renders correctly and relayouts after real size changes
  • the new regression tests pass on master

Testing Steps

  • run cd frontend && ./node_modules/.bin/vitest run src/components/misc/kowl-json-view.test.tsx src/components/pages/topics/Tab.Messages/message-display/message-display.test.tsx
  • manually open a topic with object/protobuf messages on a master build
  • expand rows 1, 2, 3, and 4 while leaving earlier rows open
  • verify the Value tab still renders JSON, copy actions still work, and later expansions do not degrade sharply

@malinskibeniamin malinskibeniamin self-assigned this Mar 6, 2026
@malinskibeniamin
Copy link
Contributor Author

@claude review

@claude
Copy link

claude bot commented Mar 6, 2026

Claude finished @malinskibeniamin's task in 2m 19s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review feedback

Good performance-focused PR. The three core optimizations are sound: disabling Monaco automaticLayout and replacing it with a size-gated ResizeObserver, memoizing ExpandedMessage to skip rerenders when props are stable, and reusing cached keyJson/valueJson strings instead of re-stringifying.

Issues to Address

🔴 Biome lint failures (will block CI)

Four issues flagged by the automated reviewer need to be fixed:

1. kowl-json-view.tsx:38 — prefer !(editor && container) over !editor || !container:

// current
if (!editor || !container) {
// suggested
if (!(editor && container)) {

Fix this →

2. message-display.test.tsx:23 — Button forwardRef: move disabled before the rest spread to avoid overriding it:

// current
({ children, isDisabled, ...props }, ref) => (
  <button ref={ref} disabled={Boolean(isDisabled)} {...props}>
// suggested (spread before disabled to avoid overwriting)
({ children, isDisabled, ...props }, ref) => (
  <button disabled={Boolean(isDisabled)} ref={ref} {...props}>

Fix this →

3. message-display.test.tsx:32 — remove the blank line between import groups (Biome wants no empty lines in import blocks).

4. message-display.test.tsx:36 — same blank-line/import ordering issue following from #3.

Fix all test file lint issues →%20on%20line%2023%2C%20change%20Button%20forwardRef%20to%20put%20%60disabled%3D%7BBoolean(isDisabled)%7D%60%20before%20%60ref%3D%7Bref%7D%60%20and%20before%20%60%7B...props%7D%60%3B%202)%20remove%20the%20blank%20line%20before%20the%20%60import%20%7B%20CompressionType%2C%20type%20Payload%2C%20type%20TopicMessage%20%7D%60%20import%20on%20line%2033.&repo=redpanda-data/console)


Observations / Minor Notes

kowl-json-view.tsx — Monaco manual layout

The scheduleLayoutResizeObserver approach is correct and well-structured. A few small notes:

  • onMount inline arrow function (line 105–109): this creates a new function reference on every render. In production Monaco this is fine because onMount fires only once on mount, but it's slightly inconsistent with the rest of the file's memoization intent. Not a bug.

  • ResizeObserver wrapper (lines 80–83): new ResizeObserver(() => { scheduleLayout(); }) can be simplified to new ResizeObserver(scheduleLayout) since scheduleLayout already has the right signature (it ignores its arguments). Minor nit, not a bug.

expanded-message.tsxReact.memo effectiveness

React.memo does a shallow prop comparison. The msg prop comes from row.original in TanStack Table, which maintains stable object references unless the underlying data array changes. This is the correct assumption and should work well in practice. The useCallback deps [msg, onCopyKey] etc. are correct — if msg identity changes the callbacks should update too.

One behavioral change worth noting: in the old code, the headers tab footer was gated on Boolean(onDownloadRecord). In the new code it's gated on onSetDownloadMessages. These are equivalent in practice (both represent "download is supported") but the semantics are cleaner now.

message-key-preview.tsx / message-preview.tsx — cached JSON fallback

// key preview
text = cullText((msg.keyJson as string | undefined) ?? JSON.stringify(key.payload), 44);
// value preview
text = cullText((msg.valueJson as string | undefined) ?? JSON.stringify(value.payload), 300);

The as string | undefined cast suggests the type of keyJson/valueJson is broader than string. If these fields can be null (not just undefined), then null ?? JSON.stringify(...) will incorrectly fall through to re-stringify. Worth verifying the actual type of TopicMessage.keyJson / valueJson to ensure ?? is the right operator here (use || if it can be null).

Fix this →

vitest.setup.ts — canvas mock placement

The Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', ...) block is added at module scope (runs once), which is correct for a setup file. This is Monaco's dependency; no issues here.

kowl-json-view.test.tsx — ResizeObserver teardown

The afterEach calls both vi.unstubAllGlobals() and then manually reassigns globalThis.ResizeObserver. Since vi.unstubAllGlobals() already restores the global to whatever vitest.setup.ts set (the no-op mock), the manual reassignment after it is a no-op. It won't cause failures but it's redundant cleanup.


Summary

The optimizations are correct and well-targeted. The main blocker is the 4 Biome lint errors that need to be fixed before merge. The keyJson/valueJson null-vs-undefined question is worth a quick check but likely fine given the existing API contract.

@malinskibeniamin
Copy link
Contributor Author

Addressed the review follow-ups in a0a9df5: stabilized the Monaco viewer config/callbacks, removed redundant cached-JSON fallback casts, and added the changed-message rerender regression test. Focused tests are passing locally.

@malinskibeniamin malinskibeniamin requested review from a team, c-julin, datamali, r-vasquez and yougotashovel and removed request for a team March 6, 2026 14:53
@malinskibeniamin malinskibeniamin merged commit f8eaf2b into master Mar 9, 2026
14 checks passed
@malinskibeniamin malinskibeniamin deleted the UX-908-release-3-fix-record-expansion-slowness branch March 9, 2026 17:50
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.

2 participants