Skip to content

fix(table-chart): wrap mode now breaks long URLs/IDs instead of overflowing into adjacent columns#2234

Open
teeohhem wants to merge 4 commits into
mainfrom
teeohhem/table-chart-text-overlap
Open

fix(table-chart): wrap mode now breaks long URLs/IDs instead of overflowing into adjacent columns#2234
teeohhem wants to merge 4 commits into
mainfrom
teeohhem/table-chart-text-overlap

Conversation

@teeohhem
Copy link
Copy Markdown
Contributor

@teeohhem teeohhem commented May 7, 2026

Summary

  • Re-add .text-break, .align-top, .align-middle, .align-bottom utility classes to _bootstrap-utilities.scss. These were referenced by HDXMultiSeriesTableChart and DBRowTable but dropped during the Bootstrap removal refactor (Refactor: Remove bootstrap, adopt semantic tokens, and improve Mantine UI usage #1347), leaving wrap mode silently broken.
  • Add overflow: hidden to <td> and max-width: 100% to the inner cell <div> in HDXMultiSeriesTableChart as belt-and-suspenders containment for long unbreakable strings.

Why

With .text-break undefined, toggling wrap mode on a Table chart cell containing a long URL/identifier without whitespace leaves the cell with only overflow: hidden and default word-break: normal. The browser can't break the string, partial fragments leak across cell boundaries during fallback rendering, and the visible result is text appearing to overlap into adjacent columns. Reproducible with many narrow columns + URLs like login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=….

Test plan

  • Build a Table chart over data with long unbreakable strings (URLs, trace IDs, hashes)
  • Toggle the wrap-lines icon — long values should break onto multiple lines within their column rather than fragmenting across columns
  • Toggle wrap-lines off — truncate-with-ellipsis behavior unchanged
  • Existing DBTableChart.test.tsx still passes
  • [ ]

Before:
image

After:
image

Ref: HDX-4180

The Bootstrap removal refactor (#1347) deleted .text-break, .align-top,
and the .align-{middle,bottom} utility classes from
_bootstrap-utilities.scss but left HDXMultiSeriesTableChart.tsx and
DBRowTable.tsx still referencing .text-break and .align-top by name.

With .text-break undefined, toggling wrap mode on a Table chart cell
containing a long URL/identifier without whitespace leaves the cell
with only overflow: hidden and default word-break: normal. The browser
can't break the string, partial fragments leak across cell boundaries
during fallback rendering, and the visible result is text appearing to
overlap into adjacent columns.

Re-add the missing utility classes, and as a belt-and-suspenders
containment add overflow: hidden to <td> and max-width: 100% to the
inner cell <div> in HDXMultiSeriesTableChart so layout can never push
content past the cell boundary regardless of word-break behavior.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 7, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 11, 2026 5:53pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 7, 2026

🦋 Changeset detected

Latest commit: b288341

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

<!-- claude-code-review -->

PR Review

✅ No critical issues found.

The fix is focused and correct: the utility classes (.text-break, .align-top/middle/bottom) were silently broken across multiple call sites (HDXMultiSeriesTableChart, DBRowTable, DBTraceWaterfallChart, NetworkPropertyPanel, LogSidePanelElements, ApiKeysSection), so restoring them in _bootstrap-utilities.scss is the right scope. The overflow: hidden on <td> plus maxWidth: 100% on the inner cell <div> is appropriate belt-and-suspenders containment. Changeset is correct (@hyperdx/app patch).

Minor nits (non-blocking):

  • 💡 cellDivStyle object is allocated on every cell render — could be hoisted to a module-level constant, but impact is negligible given React's reference-equality fast paths and virtualization.
  • 💡 word-break: break-word is deprecated in the CSS spec in favor of overflow-wrap: break-word/anywhere, but it matches the original Bootstrap 5 .text-break definition, which is presumably the intent for parity.

teeohhem added 2 commits May 7, 2026 11:21
The table chart's wrap mode has been updated to handle long URLs and IDs by breaking them instead of allowing overflow into adjacent columns.
@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label May 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 2
  • Production lines changed: 41
  • Branch: teeohhem/table-chart-text-overlap
  • Author: teeohhem

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

E2E Test Results

All tests passed • 168 passed • 3 skipped • 1253s

Status Count
✅ Passed 168
❌ Failed 0
⚠️ Flaky 4
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Deep Review

✅ No critical issues found.

🟡 P2 -- recommended

  • packages/app/src/HDXMultiSeriesTableChart.tsx:144 -- Cell renderer changes (cellDivStyle, text-break/text-truncate branching, new <td> overflow: hidden) ship with zero test coverage; the only sibling test, DBTableChart.test.tsx, mocks Table as jest.fn(() => null), which is exactly how the original Bootstrap-removal regression slipped through and how a future re-removal will too.
    • Fix: Add focused RTL tests rendering <Table> with wrapLinesEnabled toggled both ways and with/without onRowClick, asserting the text-break vs text-truncate className choice, role="link" + tabIndex={0} on the click branch, maxWidth: '100%' on both branches, and overflow: 'hidden' on the parent <td>.
    • testing, previous-comments
🔵 P3 nitpicks (3)
  • packages/app/src/HDXMultiSeriesTableChart.tsx:152 -- cellDivStyle is a pure literal ({ maxWidth: '100%' }) with no closure deps yet is reallocated for every cell on every virtualized scroll tick, and .mw-100 already exists in _bootstrap-utilities.scss:799 for exactly this purpose; both prior bot reviews flagged the hoisting concern and the diff still ships the inline closure literal.
    • Fix: Drop cellDivStyle entirely and append mw-100 to the existing cx('align-top overflow-hidden py-1 pe-3 mw-100', {...}) call, or hoist const CELL_DIV_STYLE: React.CSSProperties = { maxWidth: '100%' } to module scope and reference from both branches.
    • maintainability, kieran-typescript, previous-comments
  • packages/app/styles/_bootstrap-utilities.scss:987 -- .align-bottom is added with zero consumers anywhere under packages/app/src/, and only .align-top is exercised by this diff; the prior deep-review already called this out and it remains in the restored set.
    • Fix: Drop the .align-bottom block (lines 987-989) and add it back when a real consumer needs it.
    • maintainability, previous-comments
  • packages/app/src/HDXMultiSeriesTableChart.tsx:353 -- <td> uses inline style={{ overflow: 'hidden' }} while the inner cell div uses the overflow-hidden utility class via cx(...), mixing two style mechanisms in the same render tree and allocating a fresh { overflow: 'hidden' } object per cell per scroll frame.
    • Fix: Replace the inline style with className="overflow-hidden" on the <td> to match the cell-div convention.
    • maintainability, kieran-typescript

Reviewers (8): correctness, testing, maintainability, project-standards, agent-native, learnings, kieran-typescript, previous-comments.

Testing gaps:

  • No Playwright/visual test asserts that long unbreakable strings (URLs, hashes) stay contained within column width — the only environment that can meaningfully verify a CSS-level fix has no scenario for table-cell text overflow.
  • Nothing asserts that .text-break / .align-* exist in the compiled stylesheet; a future utility cleanup could silently re-introduce the same regression.
  • No test covers the wrapLinesEnabled toggle that swaps text-break for text-truncate on cell className.

@github-actions
Copy link
Copy Markdown
Contributor

PR Review

✅ No critical issues found.

Small, well-scoped CSS regression fix. A few minor notes (non-blocking):

  • ℹ️ .text-break uses word-wrap: break-word + word-break: break-word. Bootstrap 5 actually uses word-wrap: break-word !important only (no word-break), since word-break: break-word is non-standard/deprecated alias for overflow-wrap. Current form works in all major browsers but consider overflow-wrap: break-word for forward-compat — not a blocker.
  • ℹ️ cellDivStyle is recreated on every cell render. Negligible perf impact, but could be hoisted as a module-level constant since it's static.
  • ✅ Root-cause fix (re-adding the dropped utility classes) is correct and benefits the other callers (DBRowTable, LogSidePanelElements, etc.) that also reference these classes.
  • ✅ Changeset included; manual test plan documented with before/after screenshots.

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

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant