Fixes #31726: replace native title tooltip with styled Tooltip in Incident Manager and DataQualityTab - #31802
Conversation
|
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! |
bedbbe1 to
f194ea1
Compare
|
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! |
f194ea1 to
bc2fd76
Compare
|
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! |
… Manager and DataQualityTab tables Fixes open-metadata#31726 - Wrap truncated table-name/test-case-name links in Tooltip/TooltipTrigger instead of relying on the native title attribute, fixing missing keyboard-focus and touch support (WCAG 1.4.13) - Keep Link as the real navigational element, wrapped in react-aria's Focusable to correctly bridge the tooltip's hover/focus context onto it. An earlier version of this fix used TooltipTrigger as the trigger itself (via onPress + navigate), but TooltipTrigger renders a <button>, which loses native anchor semantics (no href, no Ctrl/Cmd+click or middle-click to open in a new tab, no right-click 'copy link', and screen readers announce it as a button instead of a link). Focusable preserves all of this while still correctly wiring the tooltip. - Fix broken ?? fallback (always-truthy getNameFromFQN result) to || so an empty name correctly falls back to the full FQN - Fix tooltip content to show the table-level FQN (tableFqn) instead of ref?.fullyQualifiedName, which incorrectly included the trailing test case name - Apply the same fix pattern to the Name column tooltip in DataQualityTab.tsx, which had the identical native-title/nested-Link bug - Fix TooltipTrigger in the shared ui-core-components library to use React.forwardRef, so AriaTooltipTrigger can correctly attach and open the tooltip overlay (without this, no tooltip in this fix can open on hover or focus, even with correct trigger structure) - Add tw:break-all to the tooltip's title span so long unbroken FQN strings wrap within the tooltip's max-width instead of overflowing - Update tests to assert Tooltip rendering and correct content instead of the removed title attribute, cover the empty-name fallback, and assert real anchor semantics (tagName, href) on the navigational trigger
bc2fd76 to
a1ee108
Compare
|
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! |
Code Review ✅ Approved 3 resolved / 3 findingsReplaces native title tooltips with styled TooltipTrigger components in Incident Manager and DataQualityTab tables, resolving keyboard/touch accessibility issues and fixing the underlying TooltipTrigger forwardRef defect. ✅ 3 resolved✅ Bug: Link nested inside TooltipTrigger button = invalid interactive nesting
✅ Quality: Test mock hides the real nested-button DOM structure
✅ Bug: Navigation link replaced with button loses anchor semantics
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
| Count | Rule |
|---|---|
| 3 | sonarjs/cyclomatic-complexity |
| 2 | react/jsx-no-constructed-context-values |
| 2 | openmetadata-imports/no-lower-layer-page-imports |
| 2 | react/no-array-index-key |
| 1 | sonarjs/no-collapsible-if |
| 1 | react-hooks/exhaustive-deps |
| 1 | sonarjs/no-duplicate-string |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.test.tsx:56:38 |
react/jsx-no-constructed-context-values |
The object passed as the value prop to the Context provider (at line 56) changes every render. To fix this consider wrapping it in a useMemo hook. |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.test.tsx:156:34 |
react/jsx-no-constructed-context-values |
The object passed as the value prop to the Context provider (at line 156) changes every render. To fix this consider wrapping it in a useMemo hook. |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:47:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:130:25 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 13 which is greater than 10 authorized.","cost":3,"secondaryLocations":[{"line":130,"column":24,"endLine":130,"endColum |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:290:57 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 11 which is greater than 10 authorized.","cost":1,"secondaryLocations":[{"line":290,"column":56,"endLine":290,"endColum |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:319:12 |
sonarjs/no-collapsible-if |
Merge this if statement with the nested one. |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:413:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'collectInlineIncidentStatuses' and 'fetchTestCasePermissions'. Either include them or remove the dependency arra |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:516:48 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 13 which is greater than 10 authorized.","cost":3,"secondaryLocations":[{"line":516,"column":47,"endLine":516,"endColum |
| 🟡 | src/components/Database/Profiler/DataQualityTab/DataQualityTab.tsx:846:28 |
react/no-array-index-key |
Do not use Array index in keys |
| 🟡 | src/components/IncidentManager/IncidentManagerTable.component.tsx:33:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/components/IncidentManager/IncidentManagerTable.component.tsx:116:23 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 3 times. |
| 🟡 | src/components/IncidentManager/IncidentManagerTable.component.tsx:147:58 |
react/no-array-index-key |
Do not use Array index in keys |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed| {getNameFromFQN(tableFqn) ?? ref?.fullyQualifiedName} | ||
| </Link> | ||
| <Tooltip placement="top" title={tableFqn}> | ||
| <Focusable> |
There was a problem hiding this comment.
Why we need to introduce focusable, we should be using Tooltip trigger, Tooltip just like we have done in other places.
| </Link> | ||
| <Tooltip placement="top" title={tableFqn}> | ||
| <Focusable> | ||
| <Link |
There was a problem hiding this comment.
Can we not untitled button component with link variant here ?
<button>and nesting an<a>inside a<button>is invalid HTML??fallback (always-truthy getNameFromFQN result) to||so an empty name correctly falls back to the full FQNDataQualityTab.tsx(Name column, Table column, dimensions-count tooltip) that had the identical bugTooltipTriggerin the sharedui-core-componentslibrary to useReact.forwardRef, soAriaTooltipTriggercan correctly attach and open the tooltip overlay — without this, the tooltip never opens on hover/focus even with the correct trigger structure. This is the root cause that made the visible fix possible.tw:break-allto the tooltip's title span so long FQN strings wrap within the tooltip's max-width instead of overflowingDescribe your changes:
Fixes #31726
I fixed the Incident Manager and DataQualityTab table-name/test-case-name tooltips, which had multiple related defects: they relied on the native
titleattribute (slow, unstyled, and inaccessible via keyboard focus or touch — violating WCAG 2.1 SC 1.4.13), had a dead??fallback that could never trigger, and showed incorrect or duplicated content in the tooltip. While fixing this, I found the underlyingTooltipTriggercomponent in@openmetadata/ui-core-componentswas missingforwardRef, meaning tooltips silently never opened for any consumer using the library's documented pattern — I fixed that too, since none of the UI-level fixes could work without it. I also found and fixed two more instances of the same native-title/nested-Link pattern inDataQualityTab.tsxwhile auditing the codebase for this bug.I made these changes to bring these components in line with the design system's Tooltip pattern, fix the accessibility violation, and make the tooltips actually functional and useful — showing the full qualified name so users can disambiguate same-named tables/test cases.
Type of change:
High-level design:
Small, targeted bug fix — no new architecture. The one non-trivial change is in the shared
tooltip.tsxcomponent (addingforwardRef), which is a well-scoped fix matching howAriaButtonand other forwardRef components in the same library already work.Tests:
Use cases covered
Unit tests
IncidentManagerTable.component.test.tsx,DataQualityTab.test.tsxManual testing performed
UI screen recording / screenshots:
Verified manually in browser — tooltip renders correctly with proper content and wrapping.
Checklist:
Fixes #31726: replace native title tooltip with styled Tooltip in Incident Manager and DataQualityTabFixes #31726above.Greptile Summary
The PR replaces inaccessible native title tooltips with styled React Aria tooltips while preserving native anchor navigation semantics.
Focusable > Linkfor table and test-case tooltip triggers.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(ui): replace native title tooltip wi..." | Re-trigger Greptile