Repository navigation
Fixes #35519: Stabilise flaky glossary Playwright specs - #35520
Merged
anuj-kumary merged 9 commits intoOct 16, 2026
Merged
Conversation
- Related terms: pick the relation type through selectOptionWithRetry and re-pick until the trigger shows it; the bottom-row non-modal Select closed on scroll or took a stale click on the neighbouring option. - addAssetToGlossaryTerm: wait for PUT /assets/add before returning so teardown cannot delete the term mid-add. - Tag label row mapper: tolerate a tag_usage row whose tag/term is gone instead of throwing an NPE that made the asset unreadable and undeletable. - Request tags: open the glossary by FQN so the /glossary auto-select redirect cannot replace the request-tags navigation. - Glossary form tags: centre the trigger before opening so the drawer's late scroll does not close the dropdown. - Glossary import: use saveBulkImport so the toast wait starts before Update. - Bulk edit glossary: clean up on a fresh API context (idle keep-alive socket hang up).
anuj-kumary
requested review from
a team,
ShaileshParmar11,
chirag-madlani and
karanh37
as code owners
October 15, 2026 09:36
Contributor
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Contributor
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
anuj-kumary
added
the
safe to test
Rows are found by the id the add-row click introduces (ids are Date.now()), the relation-type trigger by button[aria-haspopup=listbox], and the row to remove by its trigger's selected value. Prune the now-stale suppressions.
Contributor
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
ShaileshParmar11
approved these changes
Oct 15, 2026
anuj-kumary
enabled auto-merge
October 15, 2026 10:49
sonika-shah
approved these changes
Oct 15, 2026
Rohit0301
added a commit
that referenced
this pull request
Oct 15, 2026
The `settings matches baseline` visual-regression test was failing on this branch and only on this branch — PR #35520 carries main's sidebar rewrite and passes, so the stale-baseline theory I had been working from was wrong. Measuring the card top-borders in CI's own PNGs shows the cards drift: the first is identical at y=157, then 218->219, 240->241, 276->278, 297->299, growing down the page. That is this branch's SettingItemCard change. antd's <Icon> wraps the svg in a `.anticon` span, and antd injects `.anticon { display: inline-flex; line-height: 0; vertical-align: -0.125em }` at runtime from @ant-design/icons/lib/utils.js. Replacing the wrapper with a bare <svg> dropped that offset: a baseline-aligned inline svg reserves room for the descender, making the icon box 2px taller. Measured in Chrome against the real antd CSS: legacy 90x93, bare svg 90x95 (the 2px), and 90x93 again with `vertical-align: -0.125em` restored. `display: block` overshoots at 90x90, which is why the earlier attempts at this missed. `tw:align-[-0.125em]` is already used in EntitySummaryDetails on this branch, so the utility compiles. Verified: eslint 0 errors; jest 4/4; the ui-checkstyle pipeline leaves the file clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
🚦 Removed from the merge queue —
|
…ermrelatedterms-flaky-fix # Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs
anuj-kumary
enabled auto-merge
October 15, 2026 19:36
Contributor
🚦 Removed from the merge queue —
|
# Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs
anuj-kumary
enabled auto-merge
October 16, 2026 05:24
Code Review ✅ Approved🟡 Medium risk · Null-safe tag-label mapping prevents orphan-row read crashes but could expose incomplete labels when joined glossary terms or tags are missing. Stabilises flaky glossary Playwright specs by switching related-terms relation-type selection to a retrying helper, waiting for asset-add responses before teardown, and making the tag-label mapper tolerate orphaned tag_usage rows. No issues found. 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 |
timothybrush
pushed a commit
to timothybrush/OpenMetadata
that referenced
this pull request
Oct 16, 2026
…er (open-metadata#35205) * feat(ui): Implemented New Persona setting ui * fixedall the UI issues and added unit and playwright test * feat(ui): finish persona settings migration and drop antd from home customize - Render persona fullscreen customize views in normal flow (no z-index overrides) - Guard persona customize exits with the unsaved-changes prompt - Wait for the customize store page type before mounting entity customize pages - Migrate Curated Assets modal, header theme, search bar and carousel off antd - Save persona home header colour; restore Members/Bots profile nav ids - Move CustomizeStore and page-type utils out of pages/ - Add unit tests for persona settings and Playwright save coverage * lint fix * chore(playwright): auto-refresh impact-map.generated.json * removed antd refs * fix(playwright): replace antd modal waits with role-based dialog waits in CustomizeDetailPage spec The Add Tab modal was migrated from antd Modal to SimpleModal, so .ant-modal never renders. Drop the dead .ant-modal.waitFor and waitForAntdModalToSettle calls; the existing getByRole('dialog') check already gates the dialog, leaving only expect(addButton).toBeEnabled() before the click. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * lint fix * Persona settings review fixes: filled eye and save icons, header action alignment, tailwind tokens for home widgets, revert import-only churn, stabilize curated assets e2e * Remove accidentally committed local playwright report * Update NavigationBlocker and CustomizeLandingPage specs for the core UnsavedChangesModal * Sort imports * Restore Members nav item and ProfileNavId re-export lost in merge * Fix Sonar warnings in persona settings: void floating promises, Select value/onChange, useState naming * lint fix * Address review: persist home reset, sync doc-store cache, guard dirty in-modal editors, scroll-only dialog dividers, move usePersonaUsersField to hooks * Keep the unsaved-changes prompt open when save-before-exit fails * Label test-only buttons for jsx-a11y * lint fix * Fix hook dependency and async lint warnings without disabling rules * Fix remaining lint warnings: hook deps in AddWidgetModal/CustomiseSearchBar, isolated admin fixture in persona specs * chore(playwright): auto-refresh impact-map.generated.json * Fix hook deps in CustomiseHomeModal/EmptyWidgetPlaceholder and move CustomizeLandingPage to the isolated admin fixture * chore(playwright): auto-refresh impact-map.generated.json * feat(ui): migrate antd icons to core UI icon library Add 11 new icons to @openmetadata/ui-core-components and replace every named @ant-design/icons import across OMD with a core-UI equivalent. Icons added (SVG + generated TSX): CheckCircleFilled, CloseCircleFilled, ExclamationCircleFilled, EyeInvisibleFilled, Drag, Mail, Paperclip, Send, SortAscending, SortDescending. EyeFilled already existed. Call-site migration: all 29 distinct named antd icons are gone (CloseOutlined->XClose, PlusOutlined->Plus, CheckOutlined->Check, InfoCircleOutlined->InfoCircle, and 25 more). The package itself stays for now: 104 files still use the default <Icon component={Svg} />, which core-UI's Icon (iconValue: string) cannot accept. Naming: drop the "Outlined" suffix (MailOutlined->Mail, SaveOutlined->Save); "Filled" is kept as a real variant marker. Deduplicate X/XClose: same glyph, differing only in inset. XClose has 68 imports on main vs 11 for X, so X is removed and its users converted. LoadingOutlined (an animated antd Spin indicator) becomes the in-house spinner idiom used by tree/tree-select/button: RefreshCw01 + tw:animate-spin. CheckCircleTwoTone keeps its green via color=. Fix generate-icons.mjs: SVGO's convertColors rewrites `white` to `#fff` before replaceHardcodedColors runs, so the guard matching only the literal "white" let the knockout on filled icons be themed into currentColor - making the tick/cross/bang invisible against the disc. Match the hex forms too. This also corrects SecurityService's clipPath. Tests: 5 suites asserted antd implementation details (incomplete icon mocks, getByRole('img'), and an accessible name of "left label.browse-app-plural" from LeftOutlined's aria-label). Core-UI icons are aria-hidden, so assertions move to data-testid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): render SVG assets directly instead of antd's Icon wrapper Replace every `<Icon component={Svg} />` with the SVG component itself, and drop the direct @ant-design/icons dependency. No named antd icon imports and no uses of its default Icon component remain; antd keeps the package as its own transitive dependency, so the yarn.lock entry stays until antd goes. antd's Icon rendered `<span role="img" class="anticon">` around an `<svg>` it sized at `1em`. Rendering the asset directly removes that wrapper, which four kinds of styling quietly depended on: - `.anticon { font-size }` sized the icon. Each call site now carries the size the CSS asked for (the left sidebar was the worst case at 28px, and had been rendering at 14px), and the dead font-size declarations are removed. - `.anticon svg { fill }` and `.class svg { width/height }` targeted a descendant. The icon *is* the svg now, so those rules match the element itself; the sidebar's `fill: transparent` and the onboarding icon's 24px were both being dropped. - `.ant-btn > .anticon + span { margin-left: 8px }` gave antd buttons their icon/label gap. Icons inside an antd `<Button icon=…>` with a label get `tw:mr-2`. - `.anticon` was `inline-flex` and would not shrink. A bare `<svg>` in a flex row can be squashed by a long sibling, so converted icons get `tw:shrink-0`. Sizes are preserved rather than standardised: `1em` resolved against antd's `body { font-size: 14px }`, so sites with no explicit size become `tw:size-3.5` and sites with `style={{ fontSize }}` keep their own value. `alt` is carried to `aria-label` + `role="img"`. antd put it on the wrapper span, where it meant nothing. `getColumnDataTypeIcon` returned null for an unknown dataType, which antd's `component={null}` tolerated; it now guards instead of crashing the lineage node. Tests that asserted antd internals (`.anticon`, `role="img"`, an accessible name of "left …" from LeftOutlined's aria-label, and svg mocks that dropped className) assert the rendered element instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): size the settings card icon at 90px, not the inherited 14px `.setting-card-item .setting-card-icon svg { font-size: var(--om-font-size-90) }` sized this icon while it was wrapped in antd's `.anticon`. font-size cannot size an `<svg>`, so once the asset rendered directly the rule stopped applying and the icon fell back to the 14px default every other converted icon takes — a 6x shrink on every card in Settings. The size moves onto the icon's class and the dead declaration is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * minor fix * fix(ui): size the converted icons and restore the antd button icon gap Two things the icon conversion left behind, both found in review. Icons from `@openmetadata/ui-core-components/icons` default to `size = 24` and write that to the svg's width/height, where the antd icons they replaced were `1em` — about 14px against antd's `body { font-size: 14px }`. 67 call sites rendered the icon bare, so they all grew to 24px: overflowing 18px and 24px icon-only buttons, and dwarfing the `text-xs` labels they sit beside. Each one now carries the size it used to render at, keeping any explicit `fontSize` the old call site set (so KPILatestResultsV1 stays 20px and 14px). `.ant-btn > .anticon + span` and `span + .anticon` gave antd buttons their 8px icon/label gap, and neither matches a bare svg. 11 buttons with the icon before the label get `tw:mr-2`; 2 with it after get `tw:ml-2`. Also: WhatsNewAlert's close icon was invisible. `padding: 16px` used to apply to antd's wrapper span; on the svg itself, under the global border-box reset, 32px of padding against a 14px box leaves a 0x0 content area — measured in Chrome as `box 32x32, content 0x0`. It is the only element carrying the dismiss handler. `box-sizing: content-box` restores it to `box 46x46, content 14x14`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): size the icons written as bare tags, and drop the dead fontSize The previous pass keyed off an existing `className` on the icon, so the call sites written as a bare `<XClose />` or `<Check />` were skipped and kept rendering at the core-ui default of 24px. 13 of them, in buttons sized for a 14px glyph: BotDetails and RolesCard's `size="small"` save/cancel pair, ObservabilityFormFiltersItem and TriggerItem, Execution, TermBoost's Select suffix, CustomizeAppModeSidebarPage and ProfilerConfigurationPage. Two take their size from the button's own CSS rather than the 14px default: `.include-columns-add-button` is 18x18px with an 8px glyph, so both of ProfilerSettingsModal's Plus icons are `tw:size-2`. Also removed 7 `style={{ fontSize }}` declarations left on converted icons. font-size cannot size an `<svg>`, and each site's size class already matched the value it was asking for. IncidentList's ShieldTick is left unsized: it belongs to main, not this branch, and is deliberately large in an empty-state placeholder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): merge the duplicate className on SearchInputIcon The icon-sizing pass appended a second `className` instead of merging into the one already there. JSX keeps the last, so the `className` the caller passes in was silently dropped and only the size class survived. Merge both through `classNames`, which the file already imports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): restore the button icon gap on 11 more sites, prune stale suppressions The gap sweep matched `icon={<X ... />}` literally, so it missed icons wrapped in an expression — EmailConfigSettingsPage renders its Edit icon behind an `!isUndefined(...) && ...` guard, and its label sat flush against the icon. Widening the scan to the whole `icon=` prop value found 11 labelled antd buttons still missing the 8px gap `.ant-btn > .anticon + span` used to supply. `yarn lint:src:suppressions` drops 36 `no-antd-import` suppressions for files this branch moved off antd. Verified none of the 36 still imports antd, so no real violation is being hidden. Without this, `make ui-checkstyle-changed` fails with "There are suppressions left that do not occur anymore". The five `LeftOutlined` prev-buttons in the DataContract tabs keep their current spacing: they are pre-existing SVGR assets with explicit width/height that this branch never converted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(ui): lower the antd suppression ratchet to 559 `openmetadata-antd.test.mjs` pins the exact number of suppressed antd symbols so re-running `--suppress-rule` cannot quietly absorb new usage, and its comment says to lower the figure when a PR removes antd imports and prunes the baseline. This branch removes 164, so the count is 723 -> 559. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): size core-ui icons with the size prop instead of a class The icons from `@openmetadata/ui-core-components/icons` take a `size` prop, so 73 call sites pass it directly rather than carrying a `tw:size-*` class. Each keeps the size it already rendered at, including the odd ones: `size={8}` for the 18px include-columns button, `size={18}`, `size={20}`. The 61 SVGR assets imported from `assets/` keep their class. Those components spread props straight onto the `<svg>` and have no `size` handling, so `size` would land as a meaningless `size="14"` attribute and leave the icon unsized. One consequence worth knowing: `size` writes width/height attributes, which CSS outranks, where a class does not. Checked every class still attached to a converted icon for a CSS width/height rule — the only matches are generic utilities (`d-flex`, `cursor-pointer`) and a `drawer-close` prefix match on `drawer-close-icon`, which belongs to a ButtonUtility this branch never touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): move the last 12 core-ui icons onto the size prop A re-audit of all 160 changed .tsx files, with a brace- and quote-aware scanner that also reads multi-line tags and dynamic classNames, found 12 core-ui icons still sized by a `tw:size-*` class. They were added after the earlier conversion, in the pass that gave bare tags an explicit size. The 76 SVGR assets keep their class: they have no `size` prop, so `size` would land as a meaningless `size="14"` attribute. Six were traced back to their `assets/*.svg` import to confirm the classification, including `Calendar`, whose name also exists in the core-ui icon set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): drop the redundant white style and the vestigial anticon class The seven `style={{ color: 'white' }}` on Plus icons sit inside antd `<Button type="primary">`, which already sets `color: #fff`, so the icon inherits white through `currentColor` without them. The four `anticon` classNames were antd's wrapper hook. Nothing styles them now that the icon is the svg itself — the one rule that could have matched, `.alert-icon .anticon`, is scoped to a container none of these sit in. The other 17 style props stay. They are not redundant: core-ui icons hardcode `stroke="currentColor"` on the path, which overrides the root's `stroke={color}`, so the glyph follows the inherited CSS color. Verified in Chrome — dropping `style={{ color }}` renders the path black rather than the intended colour. Those set greys (GRAYED_OUT_COLOR, #37352F4D) or spacing and all came across from the antd icons they replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): move icon classes onto Tailwind tokens, dark-mode aware 63 icon sites carried legacy utility classes. Each is mapped to its Tailwind equivalent after resolving the actual CSS value, so nothing shifts: m-r-xss is 4px so `tw:mr-1`, m-l-xs is 8px so `tw:ml-2`, m-l-sm is 12px so `tw:ml-3`. The colours matter most. `text-grey-muted` is a hardcoded #757575 with no dark variant; `tw:text-quaternary` resolves through `--color-gray-500`, which `.dark-mode` redefines. Measured in Chrome: legacy light rgb(117,117,117) dark rgb(117,117,117) unchanged token light rgb(117,117,117) dark rgb(123,127,134) adapts Also `text-grey-600` -> `tw:text-tertiary`, `text-base-color` -> `tw:text-secondary`, `text-primary` -> `tw:text-brand-secondary` (what DataAssetsHeader.utils and ServiceFlowStepper already use for brand text). 26 classes stay: `sla-icon`, `status-icon`, `inherit-icon`, `configure-arrow`, `primary-key-section-line` and the like are component classes carrying their own colours and transforms, not utilities — replacing them means deleting their stylesheets. `TagsUtils` had `className="d-block'"`, a stray apostrophe that made the class match nothing. Now `tw:block`. NavbarUtils asserts the literal className, so its expectation moves with the component. No other test asserts on a converted class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): drop the sizing classes that do nothing on an icon An `<svg>` with an explicit size ignores font-size, so `tw:text-md`, `tw:text-xs` and `tw:font-medium` on 15 already-sized icons were dead. Measured in Chrome: the same icon renders 14x14 with and without them. Only sites that already carry a size prop, a size class or width/height attributes are touched — an unsized icon still inherits font-size through `1em`, and none of those exist here. ArrayFieldTemplate's delete icon carried `tw:w-7 tw:h-0` alongside `tw:size-3.5`, which is a real conflict: whichever Tailwind emits last wins and `h-0` would collapse it. The original had `w-7 h-0` on antd's wrapper span, where it was harmless and, as it turns out, already dead — neither class is defined in the LESS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): drop the icon styles and classes that carry nothing Style props on icons go from 17 to 5. The six `marginRight` become `tw:mr-2` / `tw:mr-2.5`, the six `color: GRAYED_OUT_COLOR` become `tw:text-quaternary` (#959595 hardcoded, so it never adapted to dark mode), and one `transform: rotate(90deg)` becomes `tw:rotate-90`. Four GRAYED_OUT_COLOR imports are no longer used. ProfileCard carried `className="tw:mr-1"` and `style={{ marginRight: '10px' }}` on the same icon. The inline style won, so the class was dead and the markup said 4px while rendering 10px. One `tw:mr-2.5` now. `close-btn` and `ant-icon` are defined in no stylesheet, antd's included. Both came across from the antd icons they replaced, where they did nothing either. Five style props stay because nothing else expresses them: a dynamic `theme.primaryColor`, an intentional `currentColor`, and three specific values (TEXT_BODY_COLOR, #546E7A, #37352F4D) with no exact token — #546E7A is close to `tw:text-tertiary` (#535862) but not equal, and swapping it would change the colour rather than tidy it. The 23 remaining classes were each checked against the stylesheets and do carry CSS, `ant-form-item-tooltip` included: antd defines it with a colour and `cursor: help`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(ui): convert remaining legacy icon classes and fix dead sidebar CSS Follow-up review pass on the antd icon migration. - Convert the legacy utility classes still sitting on migrated icons to their Tailwind equivalents: m-r-xss/m-t-xss (4px) -> tw:mr-1/tw:mt-1, m-r-xs (8px) -> tw:mr-2, m-x-sm (12px) -> tw:mx-3, align-middle and self-center -> tw:align-middle / tw:self-center. - GlobalSearchBar: text-primary (!important ant var) -> tw:text-brand-secondary, and drop 'text-color', which no stylesheet defines. - Replace two hardcoded colours with dark-mode-aware tokens: LimitBanner's #546E7A -> tw:text-quaternary, and AddAttributeModal's #37352F4D -> tw:text-placeholder. The latter was a real dark-mode bug: at 30% alpha the glyph flattens to #c3c2c0 on light but #191c22 on dark, i.e. it got darker against a dark background. - left-sidebar.less: antd's Menu clones the `icon` node and appends `ant-menu-item-icon` to its className -- it never wraps it in `.anticon`. Verified against the rendered DOM (0 `.anticon`, 3 `.ant-menu-item-icon`). Every `.anticon` rule in the file was therefore dead once the icons became bare <svg>, including the hover colour at `.ant-menu-item-active`. Point them at `.ant-menu-item-icon`, collapse the four-selector `fill: transparent` block into one rule, and drop the `font-size` rules (a sized <svg> ignores font-size) along with the now-unused @left-sidebar-icon-size, whose 28px the `tw:size-7` class already carries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * lint fix * style(ui): drop the inline styles this branch introduced on icons Addresses the review comments on open-metadata#35205. - TaskFeedCardFromTask: the footer Box already supports `gap`, so use gap={2} (tw:gap-2, 8px — what tw:mr-2 was) instead of margins on the icon and the replies-count button. - The seven `<Plus size={14} style={{ color: 'white', fontSize: '12px' }} />` icons inside `<Button color="primary">` become `iconLeading={Plus}`, the component form used at 235 other call sites. All three properties were dead: Button's primary root carries tw:text-white (inherited through currentColor) and sizes its icon with `tw:*:data-icon:size-*`, a class that outranks the size attribute; fontSize never sized a sized <svg>. - TreeSelectWidget: TEXT_BODY_COLOR (#37352F, no dark variant) -> the tw:text-secondary token, and drop the now-unused import. - executionUtils: color: 'transparent' -> tw:text-transparent, twice. - AddAttributeModal: a second #37352F4D Search prefix the main merge brought in -> tw:text-placeholder, matching the one already converted. - ExploreV1: drop `fontSize: '14px'` from the two sort icons — font-size cannot size an <svg>, and the enclosing core Button sizes them anyway. Left inline deliberately: `theme.primaryColor` (EntityLineageSidebar) and `currentColor` (AppearanceConfigSettingsPage) are user-configurable brand colours from Appearance settings, so no static Tailwind class can track them. Verified: tsc 573 vs origin/main's 626 baseline; jest 872/872 across the 64 affected suites; eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(ui): replace the last inline styles and hardcoded status colours - EntityLineageSidebar: both inline styles go. `cursor: 'grab'` becomes a conditional `tw:cursor-grab`, and `theme.primaryColor` becomes `tw:text-fg-brand-primary`. I previously argued this one had to stay inline because the colour is user-configurable; that was wrong. The theme provider publishes primaryColor into --tw-color-fg-brand-primary at runtime (theme-provider.utils.ts), so the class tracks the configured colour and the component no longer needs useApplicationStore at all. - SSOConfigurationForm: the status disc was a hardcoded @green-7/@Red-7 with no dark variant and a raw tw:text-white glyph. Both sides move to semantic tokens — tw:bg-success-solid / tw:bg-error-solid, which globals.css redefines under the dark block, with tw:text-fg-white on the glyph. The two now-unused LESS rules are deleted. - app.less: drop `.pagination-prev-icon`, whose `margin-top: calc(-1 * var(--om-space-3))` and `transform: rotate(180deg)` become `tw:-mt-[3px] tw:rotate-180` at the call site (--om-space-3 is 3px, off Tailwind's 4px scale, hence the arbitrary value). `.pagination-next-icon` was referenced but never defined, so that class is dropped too. Its three tests asserted the old implementation — inline `cursor: grab` and `useApplicationStore` being called — so they now assert the class and the rendered handle. The global svg.mock drops props, so the brand tint is not assertable in jsdom; the test says so rather than pretending to check it. Verified: tsc 573 vs origin/main's 626 baseline; jest 450/450 across the 29 affected suites; eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * lint fix * chore(playwright): auto-refresh impact-map.generated.json * fix(ui): restore the icon sizes and line box the visual baselines expect The visual-regression suite failed on applications, settings and glossary. Both causes are this branch's, and both come from replacing an antd <Icon> with a bare <svg>. 1. Badge icons were sized 14 when the antd icon they replaced rendered at 1em of the badge's own font-size: ApplicationCard .text-xss -> 10px (was 14) EntityHeaderTitle .text-xs -> 12px (was 14) TeamsHeadingLabel .text-xs -> 12px (was 14) AppDetails .text-xs -> 12px (was 14) On the marketplace the 4px of extra width pushed "Cache Warmup" onto a second line, which cascaded ~12px down the grid and reordered the cards. My earlier size audits only looked for icons with no size at all, so a size that was present but wrong went unnoticed. 2. setting-item-card: dropping `font-size: 90px` from the icon's <svg> also dropped the inline line box the `.anticon` wrapper used to contribute, and that box is part of the card's height. Measured in Chrome: the antd markup gives a 109px row, the bare svg 93px inline / 90px block, and putting the font-size back on `.setting-card-icon` returns it to 109px exactly. The svg keeps its tw:size-[90px]; the font-size is only there for the line box. Verified: tsc 573 vs origin/main's 626 baseline; jest 63/63 across the 10 affected suites. The screenshots themselves can only be re-checked by CI, which runs them in the pinned Playwright container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ui): express the settings-card line box as a Tailwind class Replaces the `font-size: var(--om-font-size-90)` I had put back into setting-item-card.style.less with `tw:text-[90px]` on the container, so the LESS carries no om token and its only change remains the removal of the old rule. Measured in Chrome, both give the card the same 109px row the antd `.anticon` wrapper produced; the bare svg alone gives 93px inline / 90px block. Verified: tsc 574 vs origin/main's 626 baseline; jest 4/4; eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): size the settings-card icon the way the legacy markup did My previous two attempts here were based on a wrong reconstruction of the old DOM. The real markup is `<svg width="1em" height="1em">` inside a plain `.anticon` span that carries no font-size of its own — the 90px came from the LESS rule `.setting-card-icon svg { font-size: 90px }` applying to the svg, where font-size resolves `1em`. So it sized the glyph; it was never a line box. Measured against that real markup in Chrome: legacy gives a 90x93 icon box, a 93px row and the beta badge at (9, 82) from the box corner. A bare inline <svg> sized by `tw:size-[90px]`, with no height pin and no font-size, gives exactly the same three numbers. The `tw:h-[109px]` I added before was 16px too tall and made every settings card taller than the baseline instead of matching. The diff is now just: drop the antd wrapper, move the 90px from the LESS onto the icon as a Tailwind class. Verified: jest 4/4; eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * lint fix * fix(core-ui): restore the 16 icons the last merge regenerated Valid review comment on Edit01. Resolving the icon conflicts in the previous merge, I re-ran `yarn icons:generate` to rebuild Paperclip/X — but that rewrites every icon, and main's checked-in output does not match what main's own generator produces. 450 of main's icons carry `stroke={color}` on the path, while the generator emits `stroke="currentColor"`, which overrides the root's `stroke={color}` and makes the `color` prop inert. Edit01 was the clearest casualty: main keeps a hand-maintained three-path pencil-with-baseline whose edit01.svg source is the stock single-path UntitledUI pencil, so regenerating collapsed 3 paths into 1 and changed the glyph at every call site (IntakeFormsTable, RelationshipTypeTable, DomainDetails, BotDetailPanel, the access-control pages, EntityBulkEditUtils). Restore all 16 pre-existing icons to main's version. The 11 this PR genuinely adds (CheckCircleFilled, CloseCircleFilled, Drag, ExclamationCircleFilled, EyeInvisibleFilled, Mail, Paperclip, Save, Send, SortAscending, SortDescending) are untouched, and index.ts still exports them plus main's X. Not fixing the source/output drift here: it predates this branch, spans ~450 icons, and belongs in its own PR rather than one whose goal is no visual change. Verified: Edit01.tsx byte-identical to main (3 paths, stroke={color}); tsc 564 vs origin/main's 568 baseline; core-components rebuilds clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(ui): organise the icon imports the merge left out of order ui-checkstyle failed its "ESLint + Prettier + Organise Imports (src)" gate on seven files. That step runs organize-imports-cli, eslint --fix and prettier over the changed files and fails if any of them rewrites something. All seven are files where the last merge needed a core-icon import added back (main's side of the conflict had dropped it). I appended each import at the end of the block instead of in sorted position, so organize-imports-cli moved it and the gate tripped. No code change — only the import lines move. Verified by running the three CI commands in order over the same file list: the tree is clean afterwards, so the gate's `git status --porcelain` check now passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * lint fix * chore(playwright): auto-refresh impact-map.generated.json * lint fix * fix(ui): keep the settings card icon's baseline offset The `settings matches baseline` visual-regression test was failing on this branch and only on this branch — PR open-metadata#35520 carries main's sidebar rewrite and passes, so the stale-baseline theory I had been working from was wrong. Measuring the card top-borders in CI's own PNGs shows the cards drift: the first is identical at y=157, then 218->219, 240->241, 276->278, 297->299, growing down the page. That is this branch's SettingItemCard change. antd's <Icon> wraps the svg in a `.anticon` span, and antd injects `.anticon { display: inline-flex; line-height: 0; vertical-align: -0.125em }` at runtime from @ant-design/icons/lib/utils.js. Replacing the wrapper with a bare <svg> dropped that offset: a baseline-aligned inline svg reserves room for the descender, making the icon box 2px taller. Measured in Chrome against the real antd CSS: legacy 90x93, bare svg 90x95 (the 2px), and 90x93 again with `vertical-align: -0.125em` restored. `display: block` overshoots at 90x90, which is why the earlier attempts at this missed. `tw:align-[-0.125em]` is already used in EntitySummaryDetails on this branch, so the utility compiles. Verified: eslint 0 errors; jest 4/4; the ui-checkstyle pipeline leaves the file clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(playwright): locate the selected-policy check by testid, not .anticon-check Roles.spec asserted the selected policy with `.locator('.anticon-check')`. That class came from antd's <CheckOutlined>; this branch renders core <Check>, which emits no `.anticon-*` class, so the locator could never match. Give the icon `data-testid="selected-check-icon"` and locate by that, which is how the rest of that assertion already finds its elements and does not depend on a library's internal class names. Verified in jsdom that the core icon spreads `data-testid` through to the <svg>. Also swaps the icon's legacy `text-primary` (an antd-variable class with `!important` and no dark-mode variant) for `tw:text-brand-secondary`, matching the conversion done elsewhere on this branch. Left alone: the two `.anticon` locators in playwright/utils/user.ts. They sit in `editTeams` and `editDescription`, which have no callers, and the first is already stale on main — TeamsSelectableNew renders core TreeSelect, not antd Select. Neither is a regression from this PR. Verified: eslint 0 errors; jest 16/16 across src/pages/RolesPage; the ui-checkstyle pipeline leaves both files clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * lint fix --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
anuj-kumary
deleted the
anuj-kumary/glossarytermrelatedterms-flaky-fix
branch
October 16, 2026 09:39
shah-harshit
pushed a commit
that referenced
this pull request
Oct 16, 2026
* Fixes #35519: Stabilise flaky glossary Playwright specs - Related terms: pick the relation type through selectOptionWithRetry and re-pick until the trigger shows it; the bottom-row non-modal Select closed on scroll or took a stale click on the neighbouring option. - addAssetToGlossaryTerm: wait for PUT /assets/add before returning so teardown cannot delete the term mid-add. - Tag label row mapper: tolerate a tag_usage row whose tag/term is gone instead of throwing an NPE that made the asset unreadable and undeletable. - Request tags: open the glossary by FQN so the /glossary auto-select redirect cannot replace the request-tags navigation. - Glossary form tags: centre the trigger before opening so the drawer's late scroll does not close the dropdown. - Glossary import: use saveBulkImport so the toast wait starts before Update. - Bulk edit glossary: clean up on a fresh API context (idle keep-alive socket hang up). * chore(playwright): auto-refresh impact-map.generated.json * Drop positional locators from glossary related-terms helpers and spec Rows are found by the id the add-row click introduces (ids are Date.now()), the relation-type trigger by button[aria-haspopup=listbox], and the row to remove by its trigger's selected value. Prune the now-stale suppressions. * chore(playwright): auto-refresh impact-map.generated.json * Trim review comments to one-line summaries * Lower recorded no-positional-locator suppression count to 552 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Fixes #35519
I pulled the last 24h of merge-queue Playwright results (
playwright_flaky_report.py, 2026-10-14 10:00 → 2026-10-15 09:00 UTC, 208 runs). For every glossary test that flaked or failed, I pulled the failed attempt's log, screenshot and aria snapshot from the shard blobs.Already fixed by #35416 (merged 07:28 today, no change here). Every occurrence predates the merge and shows that race's signature: the button reads Collapse All, only first-level rows render, and the child row never appears.
GlossaryTermRelationsGraph:387(26×),LargeGlossaryPerformance:176(13×),GlossaryApprovalAfterMove:311/:199,GlossaryHierarchy:39/:108/:210/:341,Glossary.specExpand All:1539/ row-action:2394,GlossaryWorkflow:560.Fixed in this PR
GlossaryTermRelatedTerms:292, :220Selectsits at the viewport's bottom edge (y≈702/720). Its popover is react-aria non-modal, souseCloseOnScrollcloses it on any ancestor scroll. While it repositioned, the click went to a detached node (60s timeout), or a stale click picked the neighbour. The CI screenshot shows Antonym saved instead of Has Part / See Also.addRelatedTermsByRelationType(and the inline pick in the edit test) usesselectOptionWithRetry(centre trigger, settle, reopen) and re-picks until the trigger shows the requested label.GlossaryAssets:46(hard fail)addAssetToGlossaryTermreturned without waiting forPUT …/assets/add. Teardown hard-deleted the term while the add was in flight, leaving an orphantag_usagerow.TagLabelRowMapperWithTargetFqnHashthen threw an NPE onGlossaryTerm.getName(), so the topic returned 500 on read and on delete.applyTagCommonFieldsGracefully. New unit testTagLabelRowMapperWithTargetFqnHashTest.Glossary.spec"Request tags for Glossary"/glossaryauto-selects it and redirects in an effect.selectActiveGlossaryskipped its wait, and the redirect replaced the request-tags navigation./glossary/<fqn>directly.Glossary.spec"Create term with related terms, tags and owners"selectTagInGlossaryFormcallsscrollIntoViewAndSettlebefore opening.GlossaryImportExport"Circular Reference"saveBulkImport(wait hoisted before the click), in all 3 spots in the file.BulkEditEntityGlossaryDELETEwent out on a keep-alive socket idle for ~66s, past the server's 60s timeout ("socket hang up"). DELETE is not retried.Not changed:
GlossaryBulkAssetPermissions:139(3 failed runs): one stacked queue batch headed by Fixes #34689: record every Assets tab edit on the asset as its own PATCH #35026, which changes the bulk endpoint from 403 to 200. That PR has since updated the spec.GlossaryTermRightPanel:203: a one-off app crash (React Search highlight #306, lazy component undefined) before the test started.No positional locators in the related-terms flow. The new helpers in
utils/glossary.tsaregetRelationRows,getOnlyRelationRow,addRelationRowandselectRelationType.Date.now()ids, so the row an add-row click creates is found by the id it introduces.button[aria-haspopup="listbox"].<select>lists every type, so matching row text hits every row.GlossaryTermRelatedTerms.spec.tshas no positional locators left, so itseslint-suppressions.jsonentry is pruned, andutils/glossary.tsdrops from 11 to 8.GlossaryTermRelatedTerms.spec.ts(6 tests) andGlossaryTermDetails.spec.ts:205each ran ×2 locally with--retries=0, and all 17 runs passed (including setup).Type of change:
Tests:
Use cases covered
Unit tests
TagLabelRowMapperWithTargetFqnHashTest: an orphan glossary/classificationtag_usagerow maps to an unenriched label instead of throwing. I couldn't run it locally (the localopenmetadata-specjar is stale), so it relies on CI.Playwright
Local runs against a running server,
--retries=0, every test passed:GlossaryTermRelatedTerms.spec.ts(all 6 tests)GlossaryAssets:46GlossaryImportExportCircular ReferenceGlossary.specRequest tags / Create term with related terms, tags and ownersI didn't run the
BulkEditEntityGlossary test locally (import-export lane).yarn lint:playwright: 0 errors.tsc: clean for the touched files.🤖 Generated with Claude Code