Skip to content

chore: refactor TimelineChart for performance#2185

Merged
kodiakhq[bot] merged 2 commits into
mainfrom
karl/timeline-viewer-perf
May 8, 2026
Merged

chore: refactor TimelineChart for performance#2185
kodiakhq[bot] merged 2 commits into
mainfrom
karl/timeline-viewer-perf

Conversation

@karl-power

@karl-power karl-power commented May 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Refactors TimelineChart for performance on traces with thousands of spans. The previous implementation re-rendered the entire chart on every wheel tick because pan/zoom were React state. This branch moves to native horizontal scrolling for pan, an imperative scale ref for zoom, and imperative DOM updates for the X-axis ticks and mouse cursor — so a wheel event no longer triggers a React render of all virtualized rows.

  • Pan is now native horizontal scroll, not drag-to-pan. The previous useDrag handler is gone; users scroll with trackpad/scroll wheel as with any scrollable container. Cmd/Ctrl + scroll still zooms.
  • Single resize handle between label column and events column, instead of one per row.

A Storybook story (TimelineChart.stories.tsx) is added with 1000 synthetic rows for working on this in isolation.

How to test locally or on Vercel

  1. Open a trace with many spans (ideally several hundred). The "Default" Storybook story under Components → TimelineChart also has 1000 synthetic rows for isolated testing.
  2. Scroll vertically — virtualized rows should behave as before; X-axis stays pinned at top, label column stays pinned at left.
  3. Cmd/Ctrl + scroll over the events area to zoom in/out. Confirm:
    • cursor under the mouse stays anchored to the same point in time across the zoom step (no drift)
    • X-axis ticks recompute with the new interval
    • mouse cursor overlay shows the time at the cursor and flips its label to the opposite side past the midpoint
  4. Drag horizontally inside the events area (native scroll bar at bottom, or two-finger swipe). Label column stays in place.
  5. Hover an event bar — Mantine tooltip with the bar's text appears.
  6. On a span with span-events configured, confirm the diamond markers show up inside the bar and have their own tooltip with timestamp + attributes.
  7. On non-Log spans, confirm the duration label (X.YYms) renders beside the bar (right side for early spans, left side for spans past the timeline midpoint).
  8. Drag the resize handle between label column and events column — the label width should resize live across all rows.
  9. Resize the browser window — the mouse-cursor vertical line height should track the new container height (previously stuck at the first-render value).

References

  • Linear Issue: Closes HDX-4145

@changeset-bot

changeset-bot Bot commented May 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: d31cd11

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label May 4, 2026
@github-actions

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Large diff: 1417 production lines changed (threshold: 1000)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 11
  • Production lines changed: 1417
  • Branch: karl/timeline-viewer-perf
  • Author: karl-power

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@vercel

vercel Bot commented May 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 8, 2026 1:07pm

Request Review

@karl-power karl-power requested review from a team and elizabetdev May 4, 2026 16:06
@github-actions

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

PR Review

Solid performance refactor — imperative DOM writes on hot paths, rAF-coalesced wheel zoom, native horizontal scroll, and well-commented rationale for the non-obvious bits. No critical bugs found, but a couple of cross-cutting behavior changes deserve confirmation before merge.

  • ⚠️ ResizablePanel.module.scss change is global (width: 6px → 3px, right: -3px → 0; active 8px → 5px) and also affects DBSearchPageFilters.tsx:1687 → confirm shrinking the hit area / moving the handle inside the boundary is intentional everywhere it's used; if this is timeline-only, scope it to a local class.
  • ⚠️ Drag-to-pan was removed with useDrag (only ⌘/Ctrl + scroll zoom + native horizontal scroll remain) → confirm this is intentional and update the on-screen <Kbd> hint or docs if pan-via-drag was a documented affordance.
  • ⚠️ onEventClick now passes a hardcoded aliasWith: [] (DBTraceWaterfallChart.tsx:1149) instead of event.aliasWith ?? []; this matches the deliberate aliasWith: [] at DBTraceWaterfallChart.tsx:380, but the local type was widened to drop the field — consider plumbing aliasWith through the Row/TTimelineEvent type (or dropping it from the row data entirely) so the contract is explicit rather than implicit.
  • ℹ️ TimelineXAxis.recompute writes to the DOM synchronously while TimelineMouseCursor.recompute schedules via rAF — minor inconsistency; fine because the parent's flushWheel is already in a rAF, but worth a one-line comment so future readers don't try to "fix" it.
  • ℹ️ Imperative tick management in TimelineXAxis.tsx reaches into container.children/firstElementChild with type assertions; the SCSS comment warns against extra DOM children but the xAxisTicks element has no runtime guard — acceptable for perf, just be aware any future child added in JSX will silently corrupt tick rendering.

@karl-power karl-power force-pushed the karl/timeline-viewer-perf branch from c525fbb to 4ffb856 Compare May 4, 2026 16:08
@github-actions

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 164 passed • 3 skipped • 1186s

Status Count
✅ Passed 164
❌ Failed 0
⚠️ Flaky 5
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@teeohhem

teeohhem commented May 6, 2026

Copy link
Copy Markdown
Contributor

@claude review

1 similar comment
@teeohhem

teeohhem commented May 6, 2026

Copy link
Copy Markdown
Contributor

@claude review

@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

Deep Review

🔴 P0/P1 -- must fix

  • packages/app/src/components/DBTraceWaterfallChart.tsx:1149 -- onEventClick now hardcodes aliasWith: [], dropping the per-event aliasWith previously threaded as event.aliasWith ?? [], so clicks bubbling through the row's outer handler reach onClick with the wrong context while the inner-label handler at line 851 still passes the correct value.
    • Fix: Thread aliasWith through TimelineChart's Row/onEventClick types (or look up the original visibleNode by event.id inside the handler) so the outer click path delivers the same aliasWith as the label click.

🟡 P2 -- recommended

  • packages/app/src/components/TimelineChart/TimelineMouseCursor.tsx:171 -- The wrapper JSX literal carries style={{ display: 'none' }}, which React re-applies on every parent reconcile, clobbering the imperative wrapper.style.display = 'block' written in flushRecompute and hiding the cursor mid-hover during any sibling re-render (e.g., useResizable drag firing parent state at ~60 Hz).

    • Fix: Drop the inline display style from the JSX and toggle visibility via a CSS class (classList.toggle(styles.visible, visible)) or set the initial hidden state in a useLayoutEffect.
  • packages/app/src/components/TimelineChart/TimelineXAxis.tsx:41 -- The component renders an empty <div ref={ticksContainerRef}> and then mutates its children imperatively via appendChild, removeChild(lastChild!), and firstElementChild as HTMLDivElement, with the "no other element children" invariant guarded only by an SCSS comment and eslint-disable directives.

    • Fix: Hold the ticks in a useRef<HTMLDivElement[]>([]) array (or wrap the imperative tick manager in a small class) so the casts and non-null assertions disappear and a future contributor adding a sibling node cannot silently break the diff loop.
    • maintainability, kieran-typescript, julik-frontend-races
  • packages/app/src/components/TimelineChart/TimelineMouseCursor.tsx:87 -- When the user resizes the label column past the timeline's container width (useResizable clamps against window.innerWidth, not the container), xPerc = (x - labelW) / (clientWidth - labelW) flips both numerator and denominator negative, the visibility test xPerc > 0 still passes, and the cursor renders at negative translateX with a negative time label.

    • Fix: Early-return from flushRecompute when clientWidth - labelW <= 0 and clamp labelWidth in useResizable against the actual container width rather than window.innerWidth.
  • packages/app/src/components/TimelineChart/TimelineChart.tsx:218 -- The new cursor-anchored wheel-zoom math (scale clamping, fraction preservation, label-column cursor clamp, rAF coalescing) ships without a single assertion-bearing test; the added Storybook story is a manual harness only.

    • Fix: Extract the scale/fraction/scrollLeft computation into a pure helper and unit-test the boundaries — scale clamped to [1, 100], newScale === oldScale early-return, cursor-on-label-column clamp, and fraction preservation across a zoom step.
  • packages/app/src/components/TimelineChart/TimelineChartRowEvents.tsx:32 -- The render contract changed from sibling flex bars with cumulative marginLeft to absolutely positioned bars at left: percentX%, and minWidthPerc semantics changed (was scaled by the overall scale factor, now applied directly), but no test pins the new positioning math.

    • Fix: Add a unit test that mounts known events and asserts each bar's computed left/width percentages, including a regression case with overlapping events that the old algorithm visually concatenated and the new one overlaps.
  • packages/app/src/components/TimelineChart/TimelineXAxis.tsx:51 -- recompute performs manual DOM diffing (add/remove tick children to match numTicks) with no test coverage for transitions like 10 → 0 → 50 ticks across maxVal/scale changes, where the children-count loop or the firstElementChild cast can drift.

    • Fix: Mount the component, drive recompute through the imperative ref handle at multiple maxVal/scale combinations, and assert tick count, label text, and marginLeft percentages.
🔵 P3 nitpicks (11)
  • packages/app/src/components/TimelineChart/TimelineMouseCursor.tsx:108 -- Cursor's translateX reads stale labelW after a label-column resize (and during native scroll without mousemove) until the user nudges the mouse, since neither labelWidth changes nor scroll events trigger an imperative recompute from the parent.

    • Fix: Add a useEffect keyed on [labelWidth] that calls xAxisHandleRef.current?.recompute() and mouseCursorHandleRef.current?.recompute().
    • correctness, julik-frontend-races
  • packages/app/src/components/TimelineChart/TimelineChart.tsx:196 -- Zoom anchor drifts at the right edge: when fraction * eventsWNew - cursorPx exceeds scrollWidth - clientWidth, the browser silently clamps scrollLeft, so successive zoom-out steps walk the visual anchor away from the cursor.

    • Fix: Read back container.scrollLeft after the assignment (or compute the clamp ourselves) and feed the clamped value into the next recompute pair.
    • correctness, adversarial
  • packages/app/src/components/TimelineChart/TimelineXAxis.tsx:53 -- When maxVal === 0 (empty rows or all-zero durations), interval = calculateInterval(0) returns 0, numTicks = Math.floor(0 / 0) = NaN, both diff loops fall through, and TimelineChartRowEvents renders left: NaN%; pre-existing but newly exposed by the imperative axis path.

    • Fix: Early-return from recompute when max <= 0 (clearing existing children) and guard percentX against zero maxVal in TimelineChartRowEvents.
    • correctness, adversarial
  • packages/app/src/components/TimelineChart/TimelineChart.tsx:249 -- handleRowClick resolves the clicked row via rows.find(r => r.id === id), which is O(rows) per click and silently fires the wrong handler when two rows share an id (synthetic test data, log+span pair on the same OTel id).

    • Fix: Read the existing data-index={virtualRow.index} attribute and index into rows directly: rows[Number(e.currentTarget.dataset.index)].
    • maintainability, kieran-typescript, adversarial
  • packages/app/src/components/TimelineChart/TimelineChart.tsx:139 -- return max * 1.1; lost its // add 10% padding comment during the refactor, leaving an unexplained magic number on the maxVal computation.

    • Fix: Restore the comment or extract a named constant such as MAX_VAL_PADDING_FACTOR = 1.1.
  • packages/app/src/components/TimelineChart/TimelineMouseCursor.tsx:109 -- overlay.style.transform = translateX(${xPerc < 0.5 ? 12 : -150}px) hardcodes the overlay's assumed width (-150); if the overlay's content grows (longer renderMs/timezone string), the flip silently misaligns.

    • Fix: Name the constants (OVERLAY_RIGHT_OFFSET = 12, OVERLAY_LEFT_OFFSET = -150 with a comment) or measure overlay.offsetWidth once and use it.
  • packages/app/src/components/TimelineChart/TimelineMouseCursor.tsx:71 -- The cursor's visibility and overlay-flip branches (xPerc > 0, xPerc < 0.5, Math.max(1, eventsContentWidth) clamp, scrollLeft-aware time math) have no test coverage.

    • Fix: Mount the component, fire mouseenter/mousemove with known clientX over both the label column and events area, and assert the wrapper's display, the cursor transform, and the rendered label text.
  • packages/app/src/components/TimelineChart/TimelineChart.tsx:183 -- initialScrolledRef is now reset whenever initialScrollRowIndex changes, so a parent prop change re-scrolls every time; the previous useState gate fired only once and a future revert would be silent.

    • Fix: Add a unit test that locks in the chosen behavior (re-scroll on every prop change) so the next refactor cannot flip it back unnoticed.
  • packages/app/src/components/TimelineChart/TimelineXAxis.tsx:48 -- scaleRef.current ?? 1 and heightRef.current ?? 0 (also in TimelineMouseCursor.tsx:93,111) are dead branches because the parent uses useRef(1)/useRef(0) and the prop type is RefObject<number> whose .current is non-null.

    • Fix: Drop the fallbacks (or widen the prop types to RefObject<number | null> if nullability is actually intended).
  • packages/app/src/components/TimelineChart/TimelineChart.tsx:353 (pre-existing) -- File now sits at 353 lines, beyond the project's 300-line target documented in agent_docs/code_style.md; the wheel-zoom block (flushWheel/onWheel/wheel-effect cleanup, ~lines 142-205) is a clean extraction candidate.

    • Fix: Extract flushWheel/onWheel into a useTimelineWheelZoom hook to bring the main component back under the line target.
  • packages/app/src/components/__tests__/DBTraceWaterfallChart.test.tsx:234 (pre-existing) -- Existing tests assert only MockTimelineChart.latestProps.rows.length and truthy rows; with Row.style/className removed and onEventClick newly required, these assertions cannot catch row-shape regressions from this PR.

    • Fix: Strengthen the mock assertions to inspect row.id, row.events[0], and onEventClick presence so future row-shape changes break the test.

Reviewers (10): correctness, testing, maintainability, project-standards, ce-agent-native, ce-learnings-researcher, performance, adversarial, kieran-typescript, julik-frontend-races.

Testing gaps:

  • No test exercises the cmd/ctrl+wheel zoom anchor across coalesced events in one rAF.
  • No test covers the ResizeObserver-driven cursor-line height update when the timeline panel is vertically resized (the bug this PR claims to fix).
  • No test confirms unmount mid-zoom cancels the pending rAF and never calls into a null timelineRef.
  • No test mounts with duplicate row.id values to verify which row's onEventClick actually fires.

elizabetdev
elizabetdev previously approved these changes May 8, 2026
@kodiakhq kodiakhq Bot merged commit f775901 into main May 8, 2026
21 checks passed
@kodiakhq kodiakhq Bot deleted the karl/timeline-viewer-perf branch May 8, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants