UX-908: fix record expansion slowness on release-2.8

[malinskibeniamin]

What

Optimize topic message expansion for object/protobuf payloads without replacing Monaco.

Why

On release-2.8, expanding the 2nd+ protobuf/object message can become progressively slower because each expanded row mounts a full Monaco viewer and the table still does extra work on rerenders. We want the read-only message viewer path to stay on Monaco while removing avoidable layout and React overhead.

How

• keep Monaco in KowlJsonView, but disable automaticLayout and other edit-only features for this read-only JSON viewer
• manually relayout the editor only when the container size actually changes
• memoize ExpandedMessage and move row-specific callbacks inside it so stable parent props can skip rerenders
• reuse cached keyJson / valueJson for object previews instead of re-running JSON.stringify(...) in render
• add regression tests for the Monaco viewer options/layout path and the topic message preview + expanded-row memoization path

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

Testing Steps

• run cd frontend && ./node_modules/.bin/vitest run src/components/misc/KowlJsonView.test.tsx src/components/pages/topics/Tab.Messages/index.test.tsx
• manually open a topic with object/protobuf messages on a build from release-2.8
• 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]

@claude review

[claude]

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

---

Code Review

• Gather context and read changed files
• Review KowlJsonView.tsx changes
• Review KowlJsonView.test.tsx (new test file)
• Review Tab.Messages/index.tsx changes
• Review Tab.Messages/index.tsx (new test file)
• Post final review

---

Overall this is a well-scoped performance fix. The approach is sound and the test coverage is good. A few items worth addressing:

---

KowlJsonView.tsx

Issue 1 — options object is recreated on every render (minor perf)

The options literal at line 100–124 is a new object reference every time KowlJsonView renders. KowlEditor merges it via Object.assign({}, defaultOptions, givenOptions) and @monaco-editor/react calls editor.updateOptions() via useEffect when its options reference changes. Since KowlJsonView is wrapped in observer(), any MobX state change that triggers a re-render will also trigger an unnecessary updateOptions call with the same values. It's idempotent but wasteful. Wrapping with useMemo (or hoisting to a module-level const) would prevent this.

// current — new object every render
options={{
  readOnly: true,
  automaticLayout: false,
  ...
}}

// suggested
const READ_ONLY_OPTIONS = {
  readOnly: true,
  automaticLayout: false,
  // ...
} as const satisfies editor.IEditorOptions;
// or useMemo(() => ({ ... }), []) inside the component

Fix this →

Issue 2 — onMount callback not memoized (minor)

The onMount prop at line 95 is an inline arrow function, producing a new reference each render. In practice @monaco-editor/react stores the latest onMount in a ref and only calls it once on mount, so this doesn't cause a remount — but it's inconsistent with how scheduleLayout is handled (correctly wrapped in useCallback). A useCallback with [scheduleLayout] dep keeps things consistent.

const handleMount = useCallback(
  (editor: IStandaloneCodeEditor) => {
    editorRef.current = editor;
    lastSizeRef.current = { width: 0, height: 0 };
    scheduleLayout();
  },
  [scheduleLayout],
);

Fix this →

---

Tab.Messages/index.tsx

Looks good — onLoadLargeMessage with empty [] deps

const onLoadLargeMessage = useCallback(
  (...) => this.loadLargeMessage(...),
  [],   // ← empty
);

This is correct: this is the class instance, which is stable for the component lifetime, so the empty array is intentional and safe.

subComponent callback is correctly memoized

Wrapping the subComponent render function in useCallback at line 1000–1012 with accurate deps (onCopyKey, onCopyValue, onLoadLargeMessage, onSetDownloadMessages, topicName) is the key fix for preventing DataTable from remounting expanded rows on each table re-render. Good.

ExpandedMessage — React.memo interaction with MobX observables

ExpandedMessage is React.memo-wrapped but is not an observer. If msg is a MobX observable object that is mutated in-place (e.g., after loadLargeMessage updates fields on it), React.memo's shallow comparison will see the same object reference and skip the re-render. PayloadComponent inside is an observer and will react correctly to those mutations — so this is fine for the current behavior. Worth noting as a future gotcha if direct ExpandedMessage rendering of observable fields were ever needed.

---

Test files

KowlJsonView.test.tsx

The test setup correctly stubs ResizeObserver, requestAnimationFrame, and getBoundingClientRect. The "relayouts only when size changes" test is particularly valuable for verifying the core deduplification logic.

Minor: act() wrapping at line 123/131 is used for the resize callbacks, but await waitFor is only used after the size-change case (line 135). The first act() call (line 123–125) checks synchronously at line 127 — this will work because requestAnimationFrame is stubbed to call synchronously, but making both checks use await waitFor would be more robust across stub changes.

index.test.tsx

The JSON.stringify spy approach at line 138 is a clean way to assert the caching path is taken. The memoization test passes the same msg reference and same callback references — correctly validating React.memo shallow comparison prevents kowlJsonViewSpy from being called on re-render.

One gap: there is no test that verifies the ExpandedMessage does re-render when msg changes (i.e., that memoization doesn't suppress needed updates). Adding a test that passes a different msg on rerender and confirms kowlJsonViewSpy is called again would make the suite more complete.

---

Summary

The optimization strategy is correct: disabling automaticLayout, adding a change-detecting ResizeObserver-driven layout, memoizing ExpandedMessage, and reusing cached JSON strings are all solid choices. The two minor issues (options not memoized, onMount not memoized) won't cause bugs today but could leak performance under MobX re-renders. Everything else looks good.

[malinskibeniamin]

Addressed the review follow-ups in 1dd9885: hoisted the read-only Monaco options, stabilized the mount callback, and expanded the regression coverage around relayouts and changed-message rerenders.

[eblairmckee]

nit: no need for useCallback if no dependencies?

[malinskibeniamin]

@claude review all recent comments on this PR

[claude]

Claude finished @malinskibeniamin's task in 45s —— View job

---

Review of recent comments

• Gather context from PR comments
• Read relevant code at the referenced lines
• Analyze eblairmckee's nit about useCallback at line 781
• Post findings

---

@eblairmckee's nit — useCallback at line 781

The comment suggests removing useCallback since there are no external dependencies. However, the empty-deps useCallback is intentional and necessary here.

onSetDownloadMessages is listed as a dependency of the subComponent useCallback at line 1011:

subComponent={useCallback(
  ({ row: { original } }) => (
    <ExpandedMessage
      ...
      onSetDownloadMessages={onSetDownloadMessages}
      ...
    />
  ),
  [onCopyKey, onCopyValue, onLoadLargeMessage, onSetDownloadMessages], // ← listed here
)}

Removing useCallback from onSetDownloadMessages would give it a new function reference on every render. That would cascade: the subComponent callback would also be recreated on every render (since its dep changed), which is exactly the root cause this PR is fixing — DataTable remounting expanded rows on each table re-render.

The same pattern is used for onLoadLargeMessage at line 773 for the same reason. Both are useCallback(fn, []) because this (the class instance) is stable for the component lifetime, so no deps are needed — but a stable reference is still required.

Conclusion: the useCallback should be kept as-is. The nit is based on a common heuristic ("no deps = no need for useCallback") that doesn't apply when the goal is reference stability for downstream memoization.

---