fix: render popover template content#1255
Conversation
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
Before, we were having empty white cards when hovering on links, look after 39 seconds evidence.mp4After fix Screencast.from.2026-07-16.18-34-50.webm |
There was a problem hiding this comment.
Pull request overview
This PR fixes an Angular CDK overlay timing issue where PopoverDirective could show an empty popover when content (string message or TemplatePortal) is assigned after the overlay component is attached, and adds regression tests to prevent recurrence (notably for the date-time popover use case in #1252).
Changes:
- Force an immediate change-detection pass for the dynamically attached popover component during
show()so newly assigned message/template content renders before the popover becomes visible. - Expose a
detectChanges()helper onPopoverComponentto support the above. - Add regression tests for: (a) updating a string-message popover while it is shown, and (b) repeated date-time template hovers without overlay accumulation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| components/crud-web-apps/common/frontend/kubeflow-common-lib/projects/kubeflow/src/lib/popover/popover.directive.ts | Triggers synchronous change detection after assigning message/template/class to ensure content is rendered before visibility changes. |
| components/crud-web-apps/common/frontend/kubeflow-common-lib/projects/kubeflow/src/lib/popover/popover.component.ts | Adds a detectChanges() wrapper on the component’s ChangeDetectorRef. |
| components/crud-web-apps/common/frontend/kubeflow-common-lib/projects/kubeflow/src/lib/popover/popover.directive.spec.ts | New test covering rendering + updating string popover content while shown and verifying overlay pane removal. |
| components/crud-web-apps/common/frontend/kubeflow-common-lib/projects/kubeflow/src/lib/date-time/date-time.component.spec.ts | Adds repeated-hover regression coverage for date-time template popovers and verifies overlays are not accumulated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| this.popoverInstance.detectChanges(); | ||
| this.popoverInstance.show(delay); | ||
| this.popoverInstance.afterHidden().subscribe(() => this.detach()); |
There was a problem hiding this comment.
Moved the one-shot afterHidden() subscription into createPopover() and added a rapid re-entry regression test that verifies each popover instance detaches exactly once.
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
/retest |
christian-heusel
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution! 🤗
Did you use AI in any capacity while creating these changes? I see that your commits do not carry any Assisted-By: <...> / Co-authored-by: <...> tags, so I wanted to make sure that you're aware of the Kubeflow AI Policy. 🤗 🤖
Problem
PopoverDirectivecreates and attaches the overlay component before assigning its message or template portal. A deferredmarkForCheck()does not render newly assigned content before the dynamically attached card becomes visible, so the date-time popover can appear empty.Change
show()after assigning the message, template portal, and classes.updatePosition()on deferredmarkForCheck()so dynamic string-input updates do not trigger synchronous change detection during the parent check.afterHidden()subscription withtake(1)per attached popover instance so rapid re-entry cannot accumulate duplicate detach callbacks.Red-green evidence
With the original behavior, the date-time regression test fails because the overlay text is empty. With the immediate view check in
show(), Local and UTC values render across repeated hover cycles, while dynamic string messages also render and update correctly. The rapid re-entry test fails with twodetach()calls before the lifecycle correction and passes with one subscription per popover instance.Verification
npm run format:checknpm run lint-checknpm run test:prod(63 tests passed)npm run buildFixes #1252