OCPBUGS-77204: Override appendTo only if needed#16108
OCPBUGS-77204: Override appendTo only if needed#16108logonoff wants to merge 1 commit intoopenshift:mainfrom
appendTo only if needed#16108Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-77204, 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff 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
the
approved
📝 WalkthroughWalkthroughThe resource-log component was updated to improve popper positioning behavior during fullscreen display. A new toolbar identifier was introduced to enable dynamic anchoring of popper elements. The implementation changes the popper append strategy from a static inline placement to conditionally anchoring against the toolbar container when fullscreen mode is active. The Toolbar component now renders with the corresponding id attribute to support this anchor reference mechanism. 🚥 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 |
![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
🧹 Nitpick comments (1)
frontend/public/components/utils/resource-log.tsx (1)
219-220: Strengthen toolbar ID uniqueness for safer anchoring.Line 219 uses only
resource.metadata.name, which can collide (e.g., same pod name in different namespaces / multiple log viewers). Since Line 417 binds this into DOMid, collisions can make anchor resolution non-deterministic.Proposed refactor
- const toolbarId = `resource-log-toolbar-${resource?.metadata?.name || 'unknown'}`; + const toolbarId = `resource-log-toolbar-${ + resource?.metadata?.namespace || 'unknown-ns' + }-${resource?.metadata?.name || 'unknown-name'}-${resource?.metadata?.uid || 'unknown-uid'}`;Also applies to: 417-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/utils/resource-log.tsx` around lines 219 - 220, The toolbarId constructed from resource?.metadata?.name is not unique enough; change the construction of toolbarId to include additional stable identifiers such as resource?.metadata?.namespace and resource?.metadata?.uid (falling back to namespace or name if uid is missing), sanitize/slugify the concatenated string to remove invalid DOM id characters, and use that new toolbarId wherever the DOM id is bound (the existing toolbarId variable and its consumer) so anchors are deterministic across namespaces and duplicated names.
🤖 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/public/components/utils/resource-log.tsx`:
- Around line 277-278: The appendTo callback can return null when
document.getElementById(toolbarId) is not found during rapid transitions; update
the appendTo for the fullscreen path to return a non-null fallback (e.g.,
document.body or a created fallback element) so Popper never receives null.
Specifically modify the appendTo that currently uses () =>
document.getElementById(toolbarId) (and the similar occurrence around the
toolbarId use at the other location) to check for a found element and fallback
to document.body (or a persistent container) before returning.
---
Nitpick comments:
In `@frontend/public/components/utils/resource-log.tsx`:
- Around line 219-220: The toolbarId constructed from resource?.metadata?.name
is not unique enough; change the construction of toolbarId to include additional
stable identifiers such as resource?.metadata?.namespace and
resource?.metadata?.uid (falling back to namespace or name if uid is missing),
sanitize/slugify the concatenated string to remove invalid DOM id characters,
and use that new toolbarId wherever the DOM id is bound (the existing toolbarId
variable and its consumer) so anchors are deterministic across namespaces and
duplicated names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a04b13a2-1690-45ff-8d55-e02a626685fd
📒 Files selected for processing (1)
frontend/public/components/utils/resource-log.tsx
| appendTo: isFullscreen ? () => document.getElementById(toolbarId) : undefined, | ||
| }} |
There was a problem hiding this comment.
Prevent null popper container in fullscreen append path.
At Line 277 and Line 361, document.getElementById(toolbarId) can return null during rapid unmount/remount transitions. Add a safe fallback container to avoid intermittent crashes.
Proposed fix
+ const getPopperContainer = useCallback(
+ () => document.getElementById(toolbarId) ?? document.body,
+ [toolbarId],
+ );
...
popperProps={{
- appendTo: isFullscreen ? () => document.getElementById(toolbarId) : undefined,
+ appendTo: isFullscreen ? getPopperContainer : undefined,
}}
...
popperProps={{
- appendTo: isFullscreen ? () => document.getElementById(toolbarId) : undefined,
+ appendTo: isFullscreen ? getPopperContainer : undefined,
}}Also applies to: 361-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/resource-log.tsx` around lines 277 - 278,
The appendTo callback can return null when document.getElementById(toolbarId) is
not found during rapid transitions; update the appendTo for the fullscreen path
to return a non-null fallback (e.g., document.body or a created fallback
element) so Popper never receives null. Specifically modify the appendTo that
currently uses () => document.getElementById(toolbarId) (and the similar
occurrence around the toolbarId use at the other location) to check for a found
element and fallback to document.body (or a persistent container) before
returning.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: This pull request references Jira Issue OCPBUGS-77204, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@logonoff: 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. |
2 participants
Root cause: I dunno
The issue was introduced in 4.20 where we added
appendTotoresource-log. This triggers an upstream issue (patternfly/patternfly-react#12255).Not sure if this 100% fixes it but by removing the
appendTooverride when not needed, I can no longer get the documentElement crash as before (by going into pod logs, and both rapidly switching tabs and opening dropdowns at the same time)Summary by CodeRabbit