Repository navigation
refactor(ui): remove antd from classic-mode Observability - #35190
Conversation
github-actions
Bot
added
safe to test
|
✅ SSO Login Nightly — PASSED View workflow run · Commit
Totals: 94 passed · 0 failed · 0 flaky · 0 skipped · duration 19m 02s |
shah-harshit
left a comment
shah-harshit
left a comment
There was a problem hiding this comment.
Two-axis review (Standards / Spec) of the classic Observability antd → core migration.
Standards: 1 raw --om-* colour (--om-grey-1 on a core Card). Raw div/span and an inline style colour remain in the migrated tooltips. Legacy text utilities (font-medium, text-grey-muted, tw:text-secondary) are passed through className where Typography already has weight/color props. Several caller-side restyles of core components remain (Dropdown.SectionHeader, AccordionHeader, Card.Content padding, the [&_label] descendant selector). There are also whole-form form.watch() subscriptions and duplicated source-change/onValuesChange logic. Worst: the --om-grey-1 override of the core Card surface.
Spec (#35189: remove antd, no behaviour change): AddNotificationPage still imports antd Form/Input. Profiler configuration save now drops unknown metric values, where it used to send them unchanged. A minor remount on adding a rule comes from effect-based row keys. Worst: the metrics-dropping change in getProfilerConfigurationPayload.
No --om-* colours elsewhere: the rest of the --om- hits are --om-space-* layout gutters (see the comment on ObservabilityAlertForm.tsx).
A template draft calls onValuesChange on every keystroke, and a full reset also replaced the alert form's default values each time. setValue per key keeps the defaults and still re-syncs the destinations field array when a source change clears it.
…sic-observability-antd-free # Conflicts: # openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-antd.test.mjs # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs
- Use core component props instead of caller classes: Typography size/weight/color in the test summary and heatmap tooltips, recent events tab and quality filters; default Card surface and padding in the test suite pipeline; Box instead of raw divs; Typography ellipsis in the date picker trigger; no !important link override. - Alert forms: subscribe only to the fields each section reads, give Collate widgets and extra buttons their own Watch, and compute validation errors with useWatch so typing no longer re-renders the whole form. - Rule rows get their key in the same render as the rule, so an added row mounts once; rule argument fields hide their label through an isLabelHidden prop instead of a descendant selector. - Notification page reuses getAlertSourceChanges and keeps its rule validator free of state updates. - Profiler configuration keeps metrics the client enum does not know. - Test case filters mark tier and dimension searchable in their descriptors; FilterChip passes the descriptor's onSearch through. - Document the react-aria top-layer marker on Select listboxes with the upstream issues and guard it with a Select-in-modal test. - Regenerate the design-token block and reference after the removed Less rules dropped their last uses.
🚦 Removed from the merge queue —
|
…sic-observability-antd-free # Conflicts: # openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-antd.test.mjs # openmetadata-ui/src/main/resources/ui/eslint-suppressions.json # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs # openmetadata-ui/src/main/resources/ui/specs/tokens/token-reference.md # openmetadata-ui/src/main/resources/ui/src/components/Alerts/AlertDetails/AlertRecentEventsTab/AlertRecentEventsTab.test.tsx # openmetadata-ui/src/main/resources/ui/src/components/Alerts/AlertDetails/AlertRecentEventsTab/AlertRecentEventsTab.tsx
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
The aria-label replaced the button's accessible name, so screen readers heard only "Filters" after a filter was applied. Include the applied filter text in the name so it matches what is shown.
🚦 Removed from the merge queue —
|
…sic-observability-antd-free # Conflicts: # openmetadata-ui/src/main/resources/ui/eslint-rules/openmetadata-antd.test.mjs
🚦 Removed from the merge queue —
|
…sic-observability-antd-free Conflict in ProfilerSettingsModal.tsx with #35370 (keep include-column and metrics dropdowns outside the scroll wrapper): kept this branch's core react-hook-form version. #35370 fixed antd getPopupContainer, which no longer exists here; the core pickers open in a react-aria popover outside the include-column scroll box. Its three antd popup-container tests are replaced by one test asserting that on the core version.
…hange main's #35348 dropped the xxs Button size (sizes are now xs-xl) and moved its own call sites to xs. The xxs Buttons this branch adds (number input steppers, slider reset, profiler and date picker menu actions) reached the merge unchanged, and core Button throws on an unknown size, failing the profiler, slider and date picker suites. Move them to xs as #35348 did.
Code Review ✅ Approved 2 closed / 2 findings🟡 Medium risk · Replacing Ant Design forms across alert, data-quality, and profiler workflows changes saved-value mapping, which could persist unintended filters or profiler settings. Migrates Classic Observability from Ant Design to core components across alerts, test-case and quality filters, profiler configuration, and related forms. Resolved this session: the diagnostic info icon is now keyboard-focusable with a visible focus ring, and the Recent Events filter button's accessible name now includes the applied filter text ( ✅ 2 closed✅ Quality: Diagnostic info icon is now tabbable but has no visible focus ring
✅ Quality: Filter button aria-label hides the applied filter from screen readers
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Fixes #35189
Part of open-metadata/openmetadata-collate#6826
Summary
Classic Observability uses
@openmetadata/ui-core-componentsin place of Ant Design, preserving saved payloads, validation, permissions and selection behavior. Covers alert lists/details/forms/destinations, test-case and quality filters, test-suite owners, data-quality parameter forms, custom metrics, incident SQL actions and profiler configuration.Shared alert widgets use controlled values, while the Classic notification form registers the saved input/template/draft fields. The shared initial-values mapper lives in Alerts utilities. See the controlled-widget decision record.
Behavior and review fixes
Validation
Latest main merge: 1,640 application Jest tests passed (122 OSS suites / 1,559 tests; 6 Collate suites / 81 tests), plus 46 core-component tests and 191 ESLint-rule tests. Both production application builds and the core build passed. Both branches contain their latest fetched main commits: OSS
9b5f77e0d8a, Collate36d04e269.16 existing Playwright cases passed without retries against the combined Collate instance and real APIs: multi-source alerts, both notification/observability permission lifecycles, pipeline alerts including diagnostic keyboard focus, custom metrics and large Select keyboard navigation, SQL-query validation/save, test-case modal filters/selection/footer, template saves, and Spark create/edit/delete with persisted extra configuration.
The original profiler configuration spec is blocked in its beforeAll API fixture by a 409 when creating a test-case result, before exercising the UI. A separate real-browser check on a fresh owned table passed the full profiler save payload and reload round trip, including sampling, include/exclude columns, query and partition settings; the table was cleaned up afterwards.
Conflicts retain the migrated controlled forms/core components and upstream's unreadable-event handling. Generated impact maps were regenerated; suppression counts are synchronized at 459 Ant Design symbols / 581 positional locators. Upstream's dark brand-400 link token is retained, its contract assertion updated, and unused generated tokens/reference rows pruned. Complete PR changed-file imports/lint/format, full source suppression pruning (zero errors), token drift, rule-table freshness and all applicable commit/push hooks passed. Collate builds use the local smoke aliases to canonical OSS source/core.
Full application type checking remains non-green: 569 diagnostics on this merge. The resolved CustomMetric prop error matches the preceding baseline; unrelated type cleanup is outside this merge. A fresh full AUT/CAIP run and completed CI checks remain necessary before merge.
Earlier broader validation covered 723 OSS Jest tests and 13 browser cases, including custom metrics, SQL-query validation/save and keyboard selection with more than 100 options. Core build/type checking, 191 ESLint-rule tests and rule-table/impact-map freshness also passed in the preceding validation. Desktop/narrow layout comparisons at 1500px/1024px matched 96 layout nodes per viewport.
Remaining validation limits
A fresh full AUT/CAIP run is still needed for these heads. Prior nightly run 38133117590 predates the current changes. Earlier CAIP 38155420513 reported 122 passed, 4 failed, 3 flaky and 7 skipped; the credits-banner conversation-loading failures also occur in an unrelated merge-queue run, while Explore attribution remains unconfirmed.
Spark configuration CRUD is now validated against real Collate ingestion-runner APIs. Argo deployment and the complete Spark profiler workflow were not exercised locally. Full source lint retains an unrelated Ant Design Table import in
AppRunsHistory.merge.test.tsx, unchanged from main. Existing application type errors are outside this migration. CI for the latest commits must finish before merge.Merge order
Door: two-way. Blast radius: Classic Observability and shared alert/profiler/select components.
Merge OSS first, then Collate #7276. After OSS merges, restore Collate's OpenMetadata submodule branch to
main, update it to merged OSS and rerun its build before merging Collate.Current OSS head:
307c919823e. Companion Collate head:c0e99c079.Review round 2 (
7ad258d680d,629e713906e)All 21 review threads are answered and resolved. Behaviour-relevant changes:
setValueper key (applyAlertValueChanges), not a fullreset, so the alert form keeps the loaded alert's defaults. Typing no longer re-renders the whole form: sections watch only their fields, each Collate widget has its own<Watch>, and validation errors come fromuseWatch({ compute }).isLabelHidden(witharia-label), not a descendant selector.MetricTypeenum.onSearchthrough, so 2.0 chips for those filters now show a search box.layout-*gutter classes, a candidate for a cross-form layout follow-up.Validation: 161 Jest suites / 2,167 tests, core select 22 tests, 191 ESLint-rule tests, changed-file ESLint (0 errors) and Prettier, and token drift regenerated. Playwright against Collate running this head: Observability/Notification alerts, AlertSeveralSources and ProfilerConfigurationPage 22/23; the miss passed 4/4 on rerun. Main merged as
86a81d6c6e8(suppression baselines remeasured: 426 antd symbols, 579 positional locators). Fresh AUT 38360065314.Screenshots and recording (
629e713906e, classic mode)Classic notification alert: source picker with section headers, then two filter rows added (each row mounts once; labels hidden through
isLabelHidden).pr35190-alert-form.mp4