OCPBUGS-77054, OCPBUGS-55742: Fix Add page issues#16045
OCPBUGS-77054, OCPBUGS-55742: Fix Add page issues#16045rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77054, 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. |
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77054, 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
component/shared
rhamilto
changed the title
rhamilto
force-pushed
the
OCPBUGS-77054
branch
from
0d6b35d to
7e043b4
Compare
📝 WalkthroughWalkthroughThe PR refactors add-flow and related UI: wraps AddCard and AddCardItem with 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/dev-console/src/components/add/AddCardItem.tsx`:
- Around line 51-68: In the AddCardItem component's onClick handler, avoid
calling e.preventDefault() for modified clicks (metaKey/ctrlKey/shiftKey/altKey
or middle-button e.button === 1) when an href is present so the browser can open
links in a new tab/window; update the handler in AddCardItem (the onClick that
uses fireTelemetryEvent, resolvedHref, navigate, callback, namespace, toast, id,
label) to first detect modifiedClick = e.metaKey || e.ctrlKey || e.shiftKey ||
e.altKey || e.button === 1, still call fireTelemetryEvent, and if href and
modifiedClick do not preventDefault or call navigate/callback (just return)
while preserving the existing behavior for non-modified clicks (preventDefault,
navigate(resolvedHref(...)) or callback(...)).
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77054, which is valid. 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. |
rhamilto
changed the title
rhamilto
force-pushed
the
OCPBUGS-77054
branch
from
7e043b4 to
c2a9295
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/dev-console/src/components/add/layout/Masonry.tsx`:
- Around line 35-40: The setHeight function inside the Masonry component is
recreated on every render which breaks MeasuredItem's memoization because
onHeightMeasured changes; wrap setHeight in useCallback (e.g., const setHeight =
useCallback((key, height) => { setHeights(old => { if (old[key] === height)
return old; return { ...old, [key]: height }; }); }, [setHeights])) so the
function identity is stable when passed to MeasuredItem; keep the same logic and
dependencies (use the state setter functional updater) to preserve behavior.
frontend/packages/dev-console/src/components/add/layout/Masonry.tsx
Outdated
Show resolved
Hide resolved
rhamilto
force-pushed
the
OCPBUGS-77054
branch
3 times, most recently
from
9780747 to
877f0ca
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/dev-console/src/components/add/__tests__/MasonryLayout.spec.tsx (1)
124-131: Extract the debounce delay to a shared constant to keep tests in sync with implementation.The test waits 150ms, but
MasonryLayout.tsx(line 42) uses a hard-coded 100ms debounce. The test delay exceeds the actual debounce, which masks drift if the implementation changes. Extract the 100ms to an exported constant and import it in the test to ensure both stay synchronized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/src/components/add/__tests__/MasonryLayout.spec.tsx` around lines 124 - 131, Extract the hard-coded debounce delay in MasonryLayout.tsx into a new exported constant (e.g., MASONRY_DEBOUNCE_MS) and replace the inline 100ms value used by the debounce in the MasonryLayout component with that constant; then update the test in MasonryLayout.spec.tsx to import MASONRY_DEBOUNCE_MS and use it for the setTimeout wait (optionally add a small buffer like +10ms if needed) so the test and implementation remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/dev-console/src/components/add/layout/Masonry.tsx`:
- Around line 20-27: The current useEffect in Masonry's measured item only
checks height once and relies on prop changes (itemKey/currentHeight) so DOM
height changes from internal toggles (like AddCardItem's showDetails) are
missed; replace or augment this logic with a ResizeObserver attached to
measureRef.current that watches for size changes and calls
onHeightMeasured(itemKey, newHeight) whenever the measured element's height
changes (debounce or ignore sub-pixel changes similar to the existing Math.abs >
1 check), and ensure you clean up the observer on unmount; reference measureRef,
onHeightMeasured, itemKey and currentHeight in the change so the observer
updates correctly and respects the existing minimal-change guard.
---
Nitpick comments:
In
`@frontend/packages/dev-console/src/components/add/__tests__/MasonryLayout.spec.tsx`:
- Around line 124-131: Extract the hard-coded debounce delay in
MasonryLayout.tsx into a new exported constant (e.g., MASONRY_DEBOUNCE_MS) and
replace the inline 100ms value used by the debounce in the MasonryLayout
component with that constant; then update the test in MasonryLayout.spec.tsx to
import MASONRY_DEBOUNCE_MS and use it for the setTimeout wait (optionally add a
small buffer like +10ms if needed) so the test and implementation remain
synchronized.
877f0ca to
febe4ff
Compare
|
@coderabbitai review |
|
/retest |
Confirmed. This PR also has a fix for an accessibility issue in the getting started card. |
sg00dwin
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77054, which is valid. 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. This pull request references Jira Issue OCPBUGS-55742, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-77054, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-55742, 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: 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. |
|
/retest |
openshift-merge-robot
added
the
needs-rebase
rhamilto
force-pushed
the
OCPBUGS-77054
branch
from
febe4ff to
748f512
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
frontend/packages/dev-console/src/components/add/layout/MasonryLayout.tsx
Outdated
Show resolved
Hide resolved
This fixes multiple issues on the Add page:
1. Console accessibility errors for Card components (PF 6.4.0)
2. Add page cards not appearing on first load
3. Add page cards not clickable (PF 6.4.0)
4. Infinite re-rendering when browser DevTools is open
PatternFly 6.4.0 compatibility changes:
- Remove unnecessary isClickable/isSelectable props from GettingStartedExpandableGrid
- Add isControlled={false} to SimpleList (PF 6.4.0 changed default to true)
- Replace navigateTo with useNavigate hook and add e.preventDefault()
Masonry layout fixes:
- Move MeasuredItem component outside render function to prevent re-creation
- Add requestAnimationFrame for initial measurement timing
- Add debouncing and proper cleanup for resize handling
- Wrap components in memo for performance optimization
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
OCPBUGS-77054
branch
from
748f512 to
19ab3a2
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@rhamilto: all tests passed! 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. |
| let rafId: number; | ||
|
|
||
| // Use ResizeObserver to detect DOM height changes from internal toggles | ||
| const resizeObserver = new ResizeObserver((entries) => { |
There was a problem hiding this comment.
nit: can we use useResizeObserver from console/shared or getResizeObserver from PF here?
| }; | ||
|
|
||
| const measuredItem = <MeasuredItem key={item.key ?? itemIndex} />; | ||
| const itemKey = (item.key as string) ?? itemIndex.toString(); |
There was a problem hiding this comment.
just curious - was item.key ever undefined? do we need this fallback to fix the bug?
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
should add a comment here to explain the eslint ignore
| // Only observe window resizes (not container resizes) to avoid | ||
| // infinite loops caused by scrollbar appearing/disappearing |
There was a problem hiding this comment.
getResizeObserver is already "supposed" to be using RAF, but because the first param containerRefElement is undefined it's not working (since it falls back to the window resize event as there's nothing to observe).
we can probably write this cleaner if this was written to actually use a resizeobserver vs using the window resize event. thoughts?
6 participants
Summary
This PR fixes multiple issues on the Add page:
Note: the bugs only occur when the browser's inspector is open!
After
Screen.Recording.2026-02-18.at.4.03.06.PM.mov
Changes
Accessibility and PatternFly 6 Compatibility Fixes
isClickable/isSelectableprops that require aria labelsisControlled={false}to SimpleList (PatternFly default behavior changed)navigateTowithuseNavigatehooke.preventDefault()to onClick handlerMasonry Layout Fixes
MeasuredItemcomponent outside render function (fixes infinite re-rendering when DevTools is open)setHeightto prevent unnecessary state updatesrequestAnimationFramefor initial measurement to ensure DOM is readyPerformance Optimizations
AddCard,AddCardItem, andMeasuredIteminmemoto prevent unnecessary re-rendersaddCardsarray inAddCardSectionto prevent recreating on every renderTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Changes
Performance
Tests