Skip to content

Fixes 36423: Align incident table surfaces, chart borders, and card corners - #36426

Merged
shah-harshit merged 4 commits into
mainfrom
shah-harshit/table-chart-ui-fixes
Oct 28, 2026
Merged

shah-harshit merged 4 commits into
mainfrom
shah-harshit/table-chart-ui-fixes

Conversation

@shah-harshit

@shah-harshit shah-harshit commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #36423

Use the darker muted surface for incident tables and their filter band, blend dark chart segment borders into the card background, and round entity table containers to match adjacent cards. Recalibrate the locale-layout test for TableCard's outline frame.

Type of change:

  • Bug fix

High-level design:

Reuse the existing core TableCard muted variant in the incident drawer, group listing, group drill-down, and entity incident tab. The entity incident filters and table share the same dark surface. ADR:2026-10-28-incident-tables-and-filters-use-the-muted-step records this scoped exception to the settings-modal-only restriction; the earlier record names the amendment. Update the shared dark chart theme so every pie/donut segment uses the card surface for its border. Add the standard rounded-xl token to the shared table container. No backend, schema, dependency, or API changes.

Tests:

Use cases covered

  • Incident tables and the entity incident filter band use the darker muted surface in dark mode. Filter controls remain readable against that surface.
  • Chart outlines blend into their card while empty segments remain visible.
  • Entity table corners match neighboring cards in both themes.
  • Existing incident status, severity, selection, navigation, and pagination tests still pass.

Unit tests

155 Jest tests passed across 7 suites (paths relative to the application UI src directory):

  • components/IncidentManager/IncidentManager.test.tsx
  • components/IncidentManager/IncidentManagerTable.component.test.tsx
  • components/common/Table/TableV2.test.tsx
  • components/observability/IncidentManager/IncidentGroups/IncidentGroupDetail.test.tsx
  • components/observability/IncidentManager/IncidentGroups/IncidentGroupDrawer.test.tsx
  • components/observability/IncidentManager/IncidentGroups/IncidentGroupsView.test.tsx
  • components/observability/IncidentManager/IncidentGroups/IncidentList.test.tsx

110 core tests passed across 5 suites (paths relative to the core UI src directory):

  • components/application/table/table.test.tsx
  • charts/theme.test.ts
  • charts/options/pie.test.ts
  • charts/options/cartesian.test.ts
  • charts/options/geo.test.ts

The updated chart theme and pie-border assertions failed before the fix and pass after it.

Measured application line coverage at 53b1cae (the same incident surface variants as the final implementation): IncidentManager 98.36%; IncidentGroupDetail, IncidentGroupDrawer, and IncidentGroupsView 100%; TableV2 67.94% (78.16% aggregate). The TableV2 change is a CSS class; its broader existing branches remain partly uncovered.

Application and core type checks, changed-file ESLint/Prettier/import organization, production builds, Tailwind audit, deprecation guard, and git diff --check pass. The repository-wide token audit reports 77 errors in unchanged CSS/LESS files; this diff changes none of those files.

Validation at ade57be: all 155 application tests above, application type check, changed-file formatting/lint, production build, Tailwind audit, deprecation guard, decision-record validation, and git diff --check passed. The 110 core tests above remain the results from 53b1cae; no core files changed in this follow-up.

Backend integration tests

Not applicable — no backend changes.

NOT NEEDED — UI styling and chart theme changes only.

Ingestion integration tests

Not applicable — no ingestion changes.

Playwright (UI) tests

The merge-queue failure in run 40743224472 was the container-width precondition: expected 1178px, received 1180px. TableCard draws an outline instead of Card's 1px side borders. Updated the calibration and its comments; the Assignee visibility, severity cap, and tooltip assertions remain unchanged.

Reported test after the fix: 10 passed, 0 failed, 0 flaky, no retries, using 4 Chromium workers at a214eb7:

  • Spec: Features/DataQuality/IncidentManagerLocaleLayout.spec.ts
  • Case: keeps the Assignee column on screen when the Russian severity placeholder is rendered
  • Local command: playwright test --config .context/locale-layout.playwright.config.ts --project chromium --grep 'keeps the Assignee column on screen' --repeat-each 10 --workers 4
  • Frontend: production build of this branch; backend: existing local server at localhost:8585, 2.0.0-SNAPSHOT, revision ffa573ad08504a611cbb362125d408f3ce134d5a. Local config used the existing spec and base fixture, API sign-in helper, Classic interface, and zero retries; it did not run the unrelated entity-seeding projects.

Full spec: 6 passed, 1 failed locally. The separate truncated-label tooltip case cannot find its tooltip. It also fails against unchanged main at 471bce3 locally, and passed in the reported CI run. It is not changed or suppressed by this fix.

Full Playwright ESLint passed with 0 errors and 197 existing warnings. Changed-file import organization, ESLint, Prettier, production builds, and git diff --check passed.

Additional local reproduction and control results
  • Before the calibration update, the reported case reproduced the exact 1178px/1180px CI failure.
  • Both the cached-build and clean-build full-file runs returned 6 passed / 1 tooltip failure. The isolated tooltip case failed on the branch and on the unchanged main frontend.
  • A longer six-case stress run returned 46 passed / 14 failures at the Classic-sidebar setup precondition after the local manual-mode hint expired. The final reported-case run above used a fresh Classic session and completed all 10 repetitions without retries.
  • The first local setup attempt used the local server's AI default and failed before geometry checks because the Classic sidebar was absent. The local authentication setup was then switched to Classic to match CI.

Reported merge-queue case: PASSED — 10/10 at a214eb7, 4 workers, 0 retries.
Full local file: 6/7; separate tooltip case also fails on unchanged main locally. Details above.

Manual testing performed

  1. Built the core library and production frontend for main at 471bce3. Rebuilt the branch frontend at ade57be; its core library code is unchanged since the previously tested 53b1cae build.
  2. Opened both builds against the same local backend at localhost:8585 (2.0.0-SNAPSHOT, revision ffa573ad08504a611cbb362125d408f3ce134d5a) and its existing sample data as admin.
  3. Compared the Column Values To Be Between incident drawer, dim_address incident filters/table, data-quality summary charts, and dim_address Columns table in light and dark themes.
  4. Confirmed the entity table radius changes from 0px to 12px. Opened the incident preview and full drill-down, and verified populated tables and charts render in both themes. Confirmed the entity incident table, its filter band, group listing, and group drill-down use rgb(19, 22, 27) in dark mode, darker than the surrounding rgb(34, 38, 47) cards.

UI screen recording:

Not applicable — standalone styling fixes. Before/after screenshots are included below.

Screenshots

Production frontend comparison at 1512 × 772, using the same backend and sample entities. Drawer and entity incident screenshots refreshed at ade57be; chart and column-table screenshots remain from 53b1cae because those views are unchanged.

Dark theme

View Before (main) After
Incident group drawer Before Incident group drawer (dark) After Incident group drawer (dark)
Entity incident table and filters Before Entity incident table and filters (dark) After Entity incident table and filters (dark)
Data-quality donut charts Before Data-quality donut charts (dark) After Data-quality donut charts (dark)
Entity table corners Before Entity table corners (dark) After Entity table corners (dark)
Light theme comparisons
View Before (main) After
Incident group drawer Before Incident group drawer (light) After Incident group drawer (light)
Entity incident table and filters Before Entity incident table and filters (light) After Entity incident table and filters (light)
Data-quality donut charts Before Data-quality donut charts (light) After Data-quality donut charts (light)
Entity table corners Before Entity table corners (light) After Entity table corners (light)

Checklist:

  • I have read CONTRIBUTING.md.
  • PR title and closing issue reference follow the repository convention.
  • Comments explain the non-obvious chart rendering and incident surface constraints.
  • JSON Schema migrations: not applicable.
  • UI feature recording requirement: not applicable to standalone styling fixes.
  • Applicable tests and visual verification are listed above.
  • Self-review completed against REVIEW.md at a214eb7; no unresolved important findings. Each changed component maps to its co-located suite above; chart theme changes map to theme and pie option tests. CSS-only behavior is covered by the screenshots.
  • Decision check completed: ADR:2026-10-28-incident-tables-and-filters-use-the-muted-step records the incident-table exception; the earlier settings-modal record names it in its Status and amendment.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no actionable issues were found.

Summary

Aligns incident tables, chart borders, and table corners with existing card styles.

  • Incident views use a muted surface for their filters and tables.
  • Dark chart outlines blend into the card background.
  • TableV2 containers have rounded corners across the app.

No actionable issues found. Review checks were read-only; tests and browser comparisons were not rerun.

Reviews (1) · Last reviewed commit: "fix(ui): align incident table surfaces a..." · Reviewed by Greptile

@shah-harshit
shah-harshit requested a review from a team as a code owner October 28, 2026 09:39
@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@shah-harshit shah-harshit added UI UI specific issues skip-pr-checks Bypass PR metadata validation check safe to test Add this label to run secure Github workflows on PRs labels Oct 28, 2026
@shah-harshit shah-harshit self-assigned this Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 80%
79.95% (131616/164606) 66.44% (46428/69870) 69.5% (26376/37949)

@shah-harshit shah-harshit added the To release Will cherry-pick this PR into the release branch label Oct 28, 2026
@shah-harshit
shah-harshit added this pull request to the merge queue Oct 28, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — failed_checks (2026-10-28T19:05:49Z)

Blocked the queue: playwright-summary

Failed Playwright tests:

  • Features/DataQuality/IncidentManagerLocaleLayout.spec.ts › keeps the Assignee column on screen when the Russian severity placeholder is rendered

@gitar-bot

gitar-bot Bot commented Oct 28, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 closed / 1 findings

🟡 Medium risk · Changing shared table corners, incident surfaces, and dark chart borders affects multiple UI views; incorrect styling could obscure controls or table boundaries.

Aligns incident drawer rows to the muted surface while keeping page-level incident tables and filters on the default card surface, blends dark chart segment borders into the card background, and rounds entity table containers to match adjacent cards. Resolved the earlier finding where the muted step was applied to page-level incident cards, contrary to the muted-variant ADR. No open issues found.

✅ 1 closed
✅ Quality: Muted step used on page-level incident cards, against muted-variant ADR

📄 openmetadata-ui/src/main/resources/ui/src/components/IncidentManager/IncidentManager.component.tsx:166 📄 openmetadata-ui/src/main/resources/ui/src/components/IncidentManager/IncidentManager.component.tsx:362 📄 openmetadata-ui/src/main/resources/ui/src/components/observability/IncidentManager/IncidentGroups/IncidentGroupsView.tsx:331 📄 openmetadata-ui/src/main/resources/ui/src/components/observability/IncidentManager/IncidentGroups/IncidentGroupDetail.tsx:174
ADR:2026-10-27-settings-modal-surfaces-use-the-muted-step (Accepted) limits the muted step (bg-primary_alt, gray-900 in dark) to section containers inside the settings modal. It also says "a shared component reused on a classic page keeps its default; shared components take a variant prop that only the modal sets." Page cards stay on bg-surface (gray-800).

This PR sets variant="muted" on three page-level containers:

  • IncidentManager, which renders on IncidentManagerPage and on the table profiler Incidents tab.
  • The group table in IncidentGroupsView.
  • The drill-down in IncidentGroupDetail.

On IncidentManagerPage, IncidentManager sits right below the <Card> that holds the page header, which stays gray-800. In dark mode the incident card now renders gray-900 next to a gray-800 header card. That is the opposite of the PR's goal of aligning with the surrounding cards. It also stops matching the chart segmentBorder, which this PR sets to the card surface (#22262f). The drawer (IncidentGroupDrawer) is different: its panel is bg-overlay-surface, the case the ADR was written for, so muted fits there.

Options:

  • Keep the default TableCard/Card variant for the three page-level containers, or
  • If incident pages are meant to sit on the muted step, add a record that supersedes the ADR and set the old record's Status to name it. It would say something like: "Incident Manager page and incident-group tables use the muted surface step (bg-primary_alt) in AI mode, so they sit one step below the page's other cards." That rule comes from the PR's own constraint: "Incident tables and their filter container reuse the existing TableCard muted variant instead of a new style."
Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

@shah-harshit
shah-harshit added this pull request to the merge queue Oct 28, 2026
Merged via the queue into main with commit 82e7c38 Oct 28, 2026
71 checks passed
@shah-harshit
shah-harshit deleted the shah-harshit/table-chart-ui-fixes branch October 28, 2026 23:11
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Backport to 2.0 is under way. It is built from openmetadata-collate, which holds the credentials needed to open a PR here, and it will comment again with the result — nothing is needed from you unless it reports otherwise.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Backport to 2.1 is under way. It is built from openmetadata-collate, which holds the credentials needed to open a PR here, and it will comment again with the result — nothing is needed from you unless it reports otherwise.

@gh-bot-collate

Copy link
Copy Markdown

🤖 Backport to 2.1 opened as #36493 (clean cherry-pick). It merges itself once the checks pass.

@gh-bot-collate

Copy link
Copy Markdown

🤖 Not backported to 2.0. Claude looked at the conflict and declined to resolve it:

Backport aborted: PR #36426 does not apply to 2.0

Source PR: #36426 — "Fixes 36423: Align incident table surfaces, chart borders, and card corners"
Merge commit: 82e7c38

Why this can't be sensibly adapted

Every hunk in this PR is a variant="muted" styling tweak (or a matching dark-mode
color adjustment) applied to UI infrastructure that was introduced on main after
the 2.0 branch point and has never been backported:

  • TableCard component and its variant="muted" prop — do not exist on 2.0 at
    all (no TableCard* file anywhere in the 2.0 tree). The PR changes
    IncidentManager.component.tsx, IncidentGroupDetail.tsx, IncidentGroupDrawer.tsx,
    and IncidentGroupsView.tsx from TableCard.Root to TableCard.Root variant="muted".
    On 2.0, IncidentManager.component.tsx still uses a plain <div> with hand-rolled
    Tailwind classes (tw:border tw:border-border-secondary tw:rounded-[10px] tw:bg-primary)
    — it was never migrated to TableCard in the first place.
  • The grouped-incidents feature (IncidentGroups/IncidentGroupDetail.tsx,
    IncidentGroupDrawer.tsx, IncidentGroupsView.tsx) was added by Fixes #34616: Grouped incidents in the AI-mode incident manager #34599 and friends,
    entirely post-2.0. These files don't exist on 2.0 (confirmed via git log --all
    on each path — first appearance is d51818cce5, not reachable from 2.0).
  • The ECharts charts module (openmetadata-ui-core-components/.../charts/theme.ts,
    theme.test.ts, charts/options/pie.test.ts) was added by feat(ui-core): ECharts-based /charts module to replace recharts #34273/refactor(ui): move every OpenMetadata chart to core charts and drop recharts (recharts → ECharts, 1–7/7) #34317 and the
    dark-palette work in Apply product-primary dark palette and keep classic mode light-only #35290, also entirely post-2.0. The charts/ directory with
    this theme file does not exist in 2.0's history.
  • TableV2.tsx's muted variant (MUTED_CONTAINER_CLASSES, variant === 'muted'
    branch) that the PR adds tw:rounded-xl to does not exist on 2.0's TableV2.tsx
    either — the component there has no muted-variant branch at all.
  • The Playwright spec (IncidentManagerLocaleLayout.spec.ts) only recalibrates pixel
    widths that result from the TableCard outline-based frame introduced by the same
    post-2.0 refactor; it asserts against a component shape 2.0 doesn't have.

In short: PR #36426 is a narrow follow-up fix to a large UI-surface refactor
(Card/TableCard muted-variant system, ECharts migration, grouped incidents) that
was never ported to 2.0. There is no equivalent code on 2.0 to apply this fix to,
and reconstructing the prerequisite refactors ourselves would go far beyond "port this
change and nothing else."

Action taken

No files were modified. The cherry-pick is left in progress (conflicted, unstaged)
for the workflow to clean up — nothing from this PR should be merged into 2.0.

Recommendation

Skip this backport. If the underlying TableCard/muted-variant/ECharts refactors are
ever backported to 2.0 as their own effort, this fix should be re-evaluated and
ported alongside them at that time.

resolver run

shah-harshit added a commit that referenced this pull request Oct 29, 2026
…orners (#36426) (#36493)

* fix(ui): align incident table surfaces and chart borders

* fix(ui): keep incident page tables on the default surface

* fix(ui): restore dark incident tables and filter surfaces

* test(ui): recalibrate incident table locale layout width

(cherry picked from commit 82e7c38)

Co-authored-by: Harshit Shah <harshit.shah@getcollate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent dark incident tables, chart outlines, and entity table corners

2 participants