refactor(ui): converge app/entity, app/entityV2 and alchemy-components with SaaS - #19862
Conversation
PR SummaryOverview Bug fixes: Search: Components & styling: API hygiene: Exports added for Reviewed by Cursor Bugbot for commit a4f0924. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87cfc8e. Configure here.
| <EntityHealthPopover health={health} baseUrl={baseUrl} placement={tooltipPlacement}> | ||
| {icon} | ||
| <EntityHealthPopover | ||
| health={health.filter((h) => h.message)} |
There was a problem hiding this comment.
Filtered health flips popover summary
Medium Severity
EntityHealth now passes only entries with a message into EntityHealthPopover, but the popover still derives its header icon and healthy/unhealthy label from that same array. A failing assertion or incident without a message is dropped, so the heading can read healthy while the profile icon stays unhealthy.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 87cfc8e. Configure here.
Bundle ReportChanges will increase total bundle size by 525 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
|
🔴 Meticulous spotted visual differences in 21 of 1066 screens tested: view and approve differences detected. Meticulous evaluated ~19 hours of user flows which tested 1679/1735 of the executable lines edited in your PR1. 1. These 56 untested lines will likely automatically gain test coverage over the coming days, however if you wish to increase coverage immediately you can do so by interacting with your feature on localhost. Last updated for commit |
Initial JS bundle size ➡️
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
maggiehays
added
the
needs-review
… counts Widens three shared HierarchicalBrowseSidebar files so they carry the options the Cloud fork already needs, and stop diverging from it: - HierarchicalBrowseTreeRow: optional `multilineLabel` lets a row label wrap onto several lines instead of truncating with an ellipsis. - treeRow.styles: `$multilineLabel` / `$multiline` handling on TreeRowContainer, TreeRowExpandZone, TreeRowLeftContent and TreeRowTitle, with the row padding factored into a `treeRowPadding` helper. - TreeSectionHeader: optional `count` renders a Pill with the direct child count while the section is collapsed. Every new prop is optional and defaults to the current behaviour, so existing call sites (Glossary, Domains, Documents, Metrics, Marketplace) render unchanged.
…nClick }
`Alert`'s `action` accepted the structured `{ label, onClick }` shape only, so a call
site needing a custom element had nowhere to put it.
- `action` now takes `AlertAction | React.ReactNode`. A new `isAlertAction` type guard
keeps the structured shape rendering as a variant-coloured text button; anything else
is rendered as given.
- `AlertContainer` gains a 4px gap so a header row and body no longer sit flush.
The structured shape is still the one to prefer and its behaviour is unchanged, so
existing call sites are unaffected.
Six files still imported `Tooltip`/`Popover` straight from antd, which the `no-antd-imports` rule rejects for anything but pre-existing call sites. Point them at the alchemy components instead; both are re-exported from `@components`.
Export `PreviewImage` and `getParentContainerNames` from `PlatformContentView`, and `shouldShowVerificationPrompt` from its own module. Exposing a symbol brings no behaviour with it; it only removes the reason to keep a second copy of the file.
Eight files still picked colors from `ANTD_GRAY`, `REDESIGN_COLORS` or a literal hex, so they did not follow the theme — dark mode in particular. Point them at the semantic tokens instead: - `ANTD_GRAY[7]` → `textTertiary`, `ANTD_GRAY[4]` → `border` - `#373d44` → `text`, `#5e666e` / `ANTD_GRAY_V2[8]` → `textSecondary` - `#7532a4` → `iconBrand`, `#f9f0ff` → `bgSurfaceBrand`, `#8338b8` → `borderBrand` `ParentNodesView` also takes `Tooltip` from `@components` rather than antd, and the two form-info wrappers get `text-wrap: wrap` so long prompt titles no longer overflow.
…healthy state - `ViewDefinitionBuilder` now passes the URNs it has already requested to `isResolutionRequired`. A URN that comes back as a null slot never lands in the cache, so without this the effect re-ran the query on every render. Uses the three-argument form already present in `app/entity/view/builder/utils`, and drops a hand-cast of the query result. - `EntityHealth` renders its icon when an entity is healthy as well as unhealthy, passes only health entries that carry a message to the popover, and tags the icon with a `data-testid` derived from the entity urn.
`DomainFilter` carried its own copy of the search-and-merge machinery every other filtersV2 filter gets from `BaseEntityFilter`: four hooks plus a utils module to fetch domains from aggregations and suggestions, merge them and map them to options. Delegate to `BaseEntityFilter` and render each option with the shared `DomainLink`, then delete what nobody needs any more: - hooks/useDomainsFromAggregations.ts (+ test) - hooks/useDomainsFromSuggestions.ts - hooks/useMergedDomains.ts - hooks/useOptionsFromDomains.ts (+ test) - utils.ts Nothing outside this directory imported any of them.
…e ANTD_GRAY Same treatment as the earlier app/entity pass, for six files the first sweep missed: - `Tooltip`/`Popover` now come from `@components` rather than antd in `OwnerContent`, `ActionDropdown`, `SelectPlatformModal`, `TestResultsList` and `EntityHealthPopover`. - `DatasetLink` and `EntityHealthPopover` take their colours from the theme — `ANTD_GRAY[7]` → `textTertiary`, `ANTD_GRAY[1]` → `bg`, `ANTD_GRAY[5]` → `border`, and the popover's hardcoded `#262626` → `bgTooltip`. - `EntityHealthPopover` also tags its content so tests can address it.
… user preview - `app/shared/tags/term/TermContent` marks a term applied by the propagation actor with a thunderbolt, the way `app/sharedV2/tags/term/TermContent` already does. The two copies rendered the same association differently. - The user preview wrapped its whole title block in a `Link` that already contained a second `Link` around the name — nested anchors. The outer one is gone. - The preview now takes `previewType` and renders attribution details only in a hover card, where there is room for them; `UserEntity.renderPreview` passes the type through instead of discarding it. - `UserEntity.displayName` labels the `__datahub_system` actor, next to the existing `__ingestion` case.
`HTML_EXTENSIONS` in the document-import helpers and `setPageAttributes` in `otel.ts` are used only by the modules that declare them.
`skip` before `fetchPolicy`, so the hook reads the same way as its siblings. No behaviour change.
87cfc8e to
a4f0924
Compare
i18n locale parityStatus:
Per-key detail is in the job summary of the ./gradlew :datahub-web-react:yarnI18nParityCheck # all locales
cd datahub-web-react && yarn check-i18n-parity --lang de # one locale |
maggiehays
added
pending-submitter-merge
and removed
needs-review
This branch was successfully deployed
3 participants
Reduces divergence between this repository and the DataHub Cloud fork under
datahub-web-react, so the scheduled upstream merge conflicts less. Frontend only; no schema, API or backend changes.45 files, +220/-442 — most of it deletion, because several changes remove duplicated machinery rather than adding anything.
Fixes
entities(urns:)returns a null slot for a missing or deleted entity, so the entity never landed in the cache and the effect re-ran the query on every render.ViewDefinitionBuildernow passes the URNs it has already requested, using the three-argumentisResolutionRequiredthatapp/entity/view/builder/utilsalready exposed, and drops aas Entity[]cast that hid the nulls from the compiler.Linkthat already contained a secondLinkaround the name.DomainFiltercarried its own copy of the search-and-merge machinery every other filtersV2 filter gets fromBaseEntityFilter— four hooks and a utils module. Delegating toBaseEntityFilterremoves all five files, plus two tests for them; nothing outside that directory imported any of them.TermContentcopies disagreed: the v2 one marks a term applied by the propagation actor with a thunderbolt, the v1 one did not.EntityHealthnow renders its icon for healthy entities as well as unhealthy ones, and passes only health entries that carry a message to the popover.Shared components gain optional capabilities
All optional, all defaulting to current behaviour, so existing call sites are unaffected:
HierarchicalBrowseTreeRowtakesmultilineLabel;TreeSectionHeadertakescountand shows it as a pill while collapsed.Alert'sactionaccepts aReactNodeas well as{ label, onClick }, via a type guard, so a caller can supply a custom element.PreviewImage,getParentContainerNamesandshouldShowVerificationPromptare exported;HTML_EXTENSIONSandsetPageAttributesare unexported, having no callers outside their modules.Styling
app/entityfiles take colours from the theme instead ofANTD_GRAY,REDESIGN_COLORSor literal hex — so they follow dark mode.ANTD_GRAY[7]->textTertiary,[4]/[5]->border,[1]->bg,#373d44->text,#5e666e->textSecondary,#7532a4->iconBrand,#f9f0ff->bgSurfaceBrand,#8338b8->borderBrand,#262626->bgTooltip.Tooltip/Popoverfrom@componentsrather than antd, which theno-antd-importsrule wants anyway.text-wrap: wrapso long prompt titles stop overflowing.Summary by cubic
Reduces divergence between this repository and the DataHub Cloud fork, so the scheduled upstream merge conflicts less. Frontend only; no schema, API, or backend changes — most of the ~45-file diff is deletion.
Bug Fixes
ViewDefinitionBuildernow remembers the URNs it has requested, so a missing entity's null slot no longer makes the query re-run on every render.Linkinside anotherLink, and shows attribution details only in hover cards.TermContentnow marks terms applied by the propagation actor with a thunderbolt, matching the v2 copy.EntityHealthnow shows its icon for healthy entities, and passes only health entries that carry a message to the popover.__datahub_systemactor renders as "Datahub System" instead of its raw username.Refactors
DomainFilternow delegates toBaseEntityFilter, deleting four hooks, a utils module, and their tests.ANTD_GRAY,REDESIGN_COLORS, or literal hex, so they follow dark mode;Tooltip/Popovercome from@componentsinstead of antd.HierarchicalBrowseTreeRowtakes an optionalmultilineLabel,TreeSectionHeadertakes an optionalcountpill, andAlert'sactionaccepts aReactNode— all defaulting to current behavior.PreviewImage,getParentContainerNames, andshouldShowVerificationPromptare now exported;HTML_EXTENSIONSandsetPageAttributesare unexported, matching the fork.text-wrap: wrap.Written for commit a4f0924. Summary will update on new commits.