OCPBUGS-77204: Override appendTo only if needed#16108
OCPBUGS-77204: Override appendTo only if needed#16108openshift-merge-bot[bot] merged 2 commits 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. |
openshift-ci
bot
added
component/core
📝 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. |
|
error was greatly reduced but sometimes I can still get and can be more easily reproduced each time I logged out and log back in console then check pod logs
|
logonoff
force-pushed
the
OCPBUGS-77204-term
branch
from
b461766 to
a83df0f
Compare
|
tested the latest code, I'm not seeing the error again /verified by @yapei |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by 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. |
|
/remove verified |
|
/remove-label verified |
|
@yapei: The label(s) 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 kubernetes-sigs/prow repository. |
|
/verified cancel need to wait for PF to release the patch |
|
@logonoff: The 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
the
do-not-merge/hold
|
/verified remove |
openshift-ci-robot
removed
the
verified
|
@yapei: The 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
force-pushed
the
OCPBUGS-77204-term
branch
from
a83df0f to
7a6e189
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by 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
force-pushed
the
OCPBUGS-77204-term
branch
from
7a6e189 to
f36f906
Compare
openshift-ci-robot
removed
the
verified
|
/assign @yapei |
|
/lgtm |
openshift-ci
bot
added
the
tide/merge-method-squash
|
[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 |
|
@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. |
|
verified the latest changes and it's working |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by 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: Jira Issue Verification Checks: Jira Issue OCPBUGS-77204 Jira Issue OCPBUGS-77204 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
4 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