OCPBUGS-77044: Fix topology node labels disappearing when moving cursor to kebab menu#16042
OCPBUGS-77044: Fix topology node labels disappearing when moving cursor to kebab menu#16042rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77044, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
component/topology
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77044, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThe PR stabilizes hover-driven visuals by introducing an isHovering composite (combining hover, contextMenuOpen and, where present, innerHover/labelHover) across topology components. Application now accepts and forwards contextMenuOpen to GroupNode and DefaultGroup; GroupNode exposes an optional hover prop. BaseNode and WorkloadNode switch label/details logic to isHovering. Several group components add labelHover refs and combined dragLabelRefs for label drag/hover. withTopologyContextMenu memoizes actionContext with useMemo. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
rhamilto
force-pushed
the
OCPBUGS-77044
branch
11 times, most recently
from
fbaaf5a to
104c053
Compare
openshift-ci
bot
added
component/helm
rhamilto
force-pushed
the
OCPBUGS-77044
branch
from
104c053 to
4b0ffd4
Compare
|
/hold Found additional issues. |
openshift-ci
bot
added
the
do-not-merge/hold
rhamilto
force-pushed
the
OCPBUGS-77044
branch
2 times, most recently
from
9689f9a to
ea065f6
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/packages/topology/src/components/graph-view/components/withTopologyContextMenu.tsx (1)
37-43: Memoization dependency on!!referenceis correct but warrants a clearer explanation.The pattern works because
referencetransitionsnull → {x,y} → nullon each open/close cycle, so!!referencedoes toggle. However, the eslint-disable comment could be more explicit about why the boolean coercion is intentional—future maintainers might "fix" this by addingreferencedirectly, which would defeat the optimization.Consider expanding the comment:
// Memoize the action context to prevent unnecessary re-renders of ActionServiceProvider - // Only recompute when the context menu is opened (reference changes from null to a value) + // Only recompute when the context menu is opened (reference transitions from null to a value). + // We intentionally use `!!reference` (boolean) rather than `reference` itself, so that + // position changes while the menu is open do not trigger a recomputation. const memoizedActionContext = useMemo( () => (reference ? actionContext(element as E) : null), // eslint-disable-next-line react-hooks/exhaustive-deps [!!reference], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/topology/src/components/graph-view/components/withTopologyContextMenu.tsx` around lines 37 - 43, Clarify the intentional use of boolean coercion for the useMemo dependency: update the comment above the memoizedActionContext/useMemo to explain that we deliberately use `!!reference` (not `reference`) so the memo only toggles when reference goes null↔value on open/close cycles, preventing recomputation when the reference object identity changes every render; mention that this is required to avoid adding `reference`, `actionContext`, or `element` to the deps (and why we keep the eslint-disable-next-line react-hooks/exhaustive-deps) and reference the memoizedActionContext, useMemo, reference, actionContext, and element symbols so future maintainers understand the tradeoff.frontend/packages/knative-plugin/src/topology/components/groups/KnativeServiceGroup.tsx (2)
113-113: Consider memoizing the hover condition passed touseShowLabel.
hover || innerHovercreates a new boolean on every render, which is fine for primitives, but worth noting thatuseShowLabelwill re-evaluate on each change. Given the frequency of hover state changes during mouse movement, ensure the hook internally handles rapid updates gracefully.Minor observation—no action required unless you observe performance issues with many Knative groups rendered simultaneously.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/knative-plugin/src/topology/components/groups/KnativeServiceGroup.tsx` at line 113, The hover condition passed into useShowLabel is recomputed each render; memoize it by creating a stable value (e.g., memoizedHover = useMemo(() => hover || innerHover, [hover, innerHover])) and pass memoizedHover into useShowLabel in KnativeServiceGroup.tsx so useShowLabel receives a stable boolean (refer to useShowLabel, hover, innerHover, showLabel).
207-224: Label visibility should account for keyboard focus when keyboard navigation is implemented.The current condition
(showLabel || labelHover || contextMenuOpen)doesn't include focus state. Keyboard-only users navigating via Tab/Enter won't trigger hover states. However, note that topology components currently lack keyboard navigation support; SVG nodes would require tabIndex, keyboard event handlers, and ARIA roles to be keyboard-accessible. If keyboard navigation is planned, ensure the label visibility condition is extended to include focus state to avoid hiding actionable elements (like the context menu kebab) from keyboard users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/knative-plugin/src/topology/components/groups/KnativeServiceGroup.tsx` around lines 207 - 224, Update the NodeLabel visibility condition to include keyboard focus so labels appear for keyboard users: modify the boolean expression around NodeLabel in KnativeServiceGroup.tsx (currently using showLabel, labelHover, contextMenuOpen) to also check a focus state (e.g., labelFocus or element-focused flag) and ensure that focus is set by adding tabIndex and keyboard event handlers (onFocus/onBlur and key handlers) on the interactive node element so the new focus state is driven by real keyboard interactions; keep using the existing NodeLabel, onContextMenu, and dragLabelRefs props but make visibility rely on (showLabel || labelHover || contextMenuOpen || labelFocus).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/packages/knative-plugin/src/topology/components/groups/KnativeServiceGroup.tsx`:
- Line 113: The hover condition passed into useShowLabel is recomputed each
render; memoize it by creating a stable value (e.g., memoizedHover = useMemo(()
=> hover || innerHover, [hover, innerHover])) and pass memoizedHover into
useShowLabel in KnativeServiceGroup.tsx so useShowLabel receives a stable
boolean (refer to useShowLabel, hover, innerHover, showLabel).
- Around line 207-224: Update the NodeLabel visibility condition to include
keyboard focus so labels appear for keyboard users: modify the boolean
expression around NodeLabel in KnativeServiceGroup.tsx (currently using
showLabel, labelHover, contextMenuOpen) to also check a focus state (e.g.,
labelFocus or element-focused flag) and ensure that focus is set by adding
tabIndex and keyboard event handlers (onFocus/onBlur and key handlers) on the
interactive node element so the new focus state is driven by real keyboard
interactions; keep using the existing NodeLabel, onContextMenu, and
dragLabelRefs props but make visibility rely on (showLabel || labelHover ||
contextMenuOpen || labelFocus).
In
`@frontend/packages/topology/src/components/graph-view/components/withTopologyContextMenu.tsx`:
- Around line 37-43: Clarify the intentional use of boolean coercion for the
useMemo dependency: update the comment above the memoizedActionContext/useMemo
to explain that we deliberately use `!!reference` (not `reference`) so the memo
only toggles when reference goes null↔value on open/close cycles, preventing
recomputation when the reference object identity changes every render; mention
that this is required to avoid adding `reference`, `actionContext`, or `element`
to the deps (and why we keep the eslint-disable-next-line
react-hooks/exhaustive-deps) and reference the memoizedActionContext, useMemo,
reference, actionContext, and element symbols so future maintainers understand
the tradeoff.
|
/retest |
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
openshift-merge-robot
added
the
needs-rebase
rhamilto
force-pushed
the
OCPBUGS-77044
branch
from
ea065f6 to
8a95c69
Compare
openshift-merge-robot
removed
the
needs-rebase
frontend/packages/helm-plugin/src/topology/components/HelmReleaseGroup.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/topology/src/operators/components/OperatorBackedServiceGroup.tsx
Outdated
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…or to kebab menu
Fixed an issue where topology node labels with kebab menus would disappear
when users moved their cursor from the node to access the kebab menu or when
the context menu was open.
Changes:
- BaseNode: Added raiseLabelOnHover={false} to prevent double layer switching
that was causing label remounts and hover state loss
- BaseNode & WorkloadNode: Track combined hover state (hover || contextMenuOpen)
to keep labels visible when context menu is open
- Application: Removed local hover management and let PatternFly's DefaultGroup
handle label visibility via internal hover detection
- OperatorBackedServiceGroup: Added separate label hover tracking to maintain
label visibility when hovering over the label itself
- withTopologyContextMenu: Memoized action context to prevent unnecessary
re-renders when context menu is open
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
OCPBUGS-77044
branch
from
8a95c69 to
5bb93ff
Compare
|
@rhamilto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary
Fixed topology node labels disappearing when moving cursor from the node to the kebab menu.
After (KnativeServiceGroup not included)
Screen.Recording.2026-02-18.at.2.51.13.PM.mov
Changes
Group Components (HelmReleaseGroup, KnativeServiceGroup, OperatorBackedServiceGroup)
labelHoverstate tracking usinguseHover()hookisHovering = hover || innerHover || labelHover || contextMenuOpenshowLabel || labelHover || contextMenuOpenuseCombineRefs(dragLabelRef, labelHoverRef)to track hover on the label element itselfApplication Component
useHover()anduseCombineRefs(dragNodeRef, hoverRef)showLabelbased on hover viauseShowLabel(hover)showLabel={showLabel || contextMenuOpen}showLabelOnHoverInventoryItem Component
ResolvedExtensiondirectly from@console/dynamic-plugin-sdkinstead of internal dist pathTechnical Details
The fix uses different approaches for different component types:
Custom Group Components (Helm, Knative, Operator-backed):
hoverandinnerHoverstart 200ms countdownlabelHoverbecomes true(showLabel || labelHover || contextMenuOpen)is still trueApplication Component (using DefaultGroup):
DefaultGroupcomponent which has built-in label hover handlingshowLabel(based on node hover) withcontextMenuOpento keep label visibleshowLabelOnHoverenables the hover behaviorshowLabel={showLabel || contextMenuOpen}ensures label persists when context menu opensTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes