OCPBUGS-76558: Fix Add page getting started cards not clickable#16041
OCPBUGS-76558: Fix Add page getting started cards not clickable#16041sg00dwin wants to merge 1 commit intoopenshift:mainfrom
Conversation
…onry layout Assisted by: Claude Code
openshift-ci-robot
added
jira/severity-important
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-76558, 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-76558, 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. |
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-76558, 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 Masonry component refactoring introduces a dedicated MeasuredItem internal component that handles height measurement and reporting via an onMeasure callback. A memoized setHeight function using useCallback updates the heights state, while an effectiveKey derived from item.key and itemIndex maintains consistent height tracking across layout operations. Measured items receive stable keys based on this effectiveKey to ensure proper reconciliation during re-renders, and the grouping logic now references heights via this derived key instead of direct index access. 🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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/layout/Masonry.tsx`:
- Around line 17-28: MeasuredItem currently only measures height once (useEffect
with [itemKey, onMeasure]) so late changes (async images, lazy content,
responsive shifts) won't update column balancing; modify MeasuredItem to observe
size changes by attaching a ResizeObserver to measureRef (use the existing
ResizeObserver utility or the pattern from MasonryLayout), compare against
lastHeightRef, and call onMeasure(itemKey, newHeight) whenever height changes;
ensure observer is cleaned up on unmount and that measureRef/null checks are
handled.
| // Stable component definition - prevents unmount/remount cycles | ||
| const MeasuredItem: FC<MeasuredItemProps> = ({ item, itemKey, onMeasure }) => { | ||
| const measureRef = useRef<HTMLDivElement>(null); | ||
| const lastHeightRef = useRef<number>(0); | ||
|
|
||
| useEffect(() => { | ||
| const newHeight = measureRef.current?.getBoundingClientRect().height; | ||
| if (newHeight && lastHeightRef.current !== newHeight) { | ||
| lastHeightRef.current = newHeight; | ||
| onMeasure(itemKey, newHeight); | ||
| } | ||
| }, [itemKey, onMeasure]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Masonry usage to assess if children can resize after mount
echo "=== Masonry usages ==="
rg -n --type=tsx --type=ts '\bMasonry\b' -C 3
echo -e "\n=== ResizeObserver availability ==="
rg -n 'ResizeObserver' -SRepository: openshift/console
Length of output: 6356
🏁 Script executed:
#!/bin/bash
# Check the actual Masonry.tsx file
echo "=== Masonry.tsx content ==="
cat -n frontend/packages/dev-console/src/components/add/layout/Masonry.tsx | head -60
echo -e "\n=== MasonryLayout.tsx for comparison ==="
cat -n frontend/packages/dev-console/src/components/add/layout/MasonryLayout.tsx | head -60
echo -e "\n=== Check if Masonry is imported/used in MasonryLayout ==="
rg -n 'import.*Masonry' frontend/packages/dev-console/src/components/add/layout/Repository: openshift/console
Length of output: 5464
Individual item height changes after mount won't update column balancing
MeasuredItem measures height only on mount (dependency array never triggers re-measure since onMeasure is memoized and itemKey is stable). If Add cards can change height after mount—e.g., async images, lazy-loaded content, or responsive resizing within items—column heights become stale and balancing drifts. If cards are guaranteed static, this is fine; otherwise, observe size changes.
🔧 Suggested fix using ResizeObserver
useEffect(() => {
- const newHeight = measureRef.current?.getBoundingClientRect().height;
- if (newHeight && lastHeightRef.current !== newHeight) {
- lastHeightRef.current = newHeight;
- onMeasure(itemKey, newHeight);
- }
+ const element = measureRef.current;
+ if (!element) return;
+
+ const updateHeight = (height: number) => {
+ if (lastHeightRef.current !== height) {
+ lastHeightRef.current = height;
+ onMeasure(itemKey, height);
+ }
+ };
+
+ // Initial measure
+ updateHeight(element.getBoundingClientRect().height);
+
+ const observer = new ResizeObserver((entries) => {
+ const height = entries[0]?.contentRect.height ?? 0;
+ updateHeight(height);
+ });
+ observer.observe(element);
+ return () => observer.disconnect();
}, [itemKey, onMeasure]);ResizeObserver is already available in the codebase and used in similar measurement patterns (e.g., MasonryLayout for container resizing). Confirm whether Add items can realistically resize—if so, this is a good safeguard.
🤖 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/layout/Masonry.tsx` around
lines 17 - 28, MeasuredItem currently only measures height once (useEffect with
[itemKey, onMeasure]) so late changes (async images, lazy content, responsive
shifts) won't update column balancing; modify MeasuredItem to observe size
changes by attaching a ResizeObserver to measureRef (use the existing
ResizeObserver utility or the pattern from MasonryLayout), compare against
lastHeightRef, and call onMeasure(itemKey, newHeight) whenever height changes;
ensure observer is cleaned up on unmount and that measureRef/null checks are
handled.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sg00dwin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@sg00dwin: 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. |
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-76558. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
|
Closing in favor of going with this fix |
sg00dwin
deleted the
OCPBUGS-76558-getting-started-cards-not-clickable
branch
3 participants
Summary
Fixes intermittent navigation failures on Add page cards caused by unstable component identity in the Masonry
layout.
Problem
After removing
react-measuredependency (PR #14844), cards on the Add page would not navigate when clicked.The issue was caused by:
MeasuredItemwas defined insideChildren.forEachloop, creating newcomponent identity on every render
This caused React to constantly unmount/remount DOM nodes, losing click events during transitions.
Solution
MeasuredItemoutside the loop with stable identitysetHeightinuseCallbackto prevent unnecessary re-renderseffectiveKeythroughout for both React keys and height trackinglastHeightRefinstead of closure to avoid stale dataTesting
Related
react-measure#14844 (react-measure removal)Assisted by: Claude Code
Summary by CodeRabbit