OCPBUGS-65728: fix dynamic plugin horizontal nav tabs failing with loader error#16055
OCPBUGS-65728: fix dynamic plugin horizontal nav tabs failing with loader error#16055upalatucci wants to merge 1 commit intoopenshift:mainfrom
Conversation
…ader error Replace AsyncComponent with React.lazy for dynamic plugin horizontal nav tabs. AsyncComponent expects loader to return a Promise, but CodeRef properties on shared extension objects can get mutated from async functions to resolved React components, causing "this.state.loader(...).then is not a function". React.lazy properly awaits the CodeRef and is resilient to this mutation, matching the pattern already used for console.page/route extensions in usePluginRoutes. Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
added
jira/valid-reference
|
@upalatucci: This pull request references Jira Issue OCPBUGS-72522, 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 NOT APPROVED This pull-request has been approved by: upalatucci 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 |
📝 WalkthroughWalkthroughThis change refactors dynamic component loading in the horizontal navigation module by introducing a 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 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/horizontal-nav.tsx`:
- Around line 315-326: The tab wrapper currently defines component as
parameterless arrow functions (in the navTabExtensions map and resolvedNavTab
map) so LazyDynamicTab never receives PageComponentProps; update those mappings
to accept and forward props (e.g., component: (props) => <LazyDynamicTab
component={tab.properties.component} {...props} />) so LazyDynamicTab receives
PageComponentProps and can forward them to the lazy-loaded component; ensure
both places that create component use the same prop-forwarding pattern and that
LazyDynamicTab passes all received props through to the inner component.
- Around line 40-46: The LazyDynamicTab currently calls component() and returns
a lazy component but does not accept or forward any props, and it will throw if
component is already resolved (not executable); update the LazyDynamicTab FC to
accept props (e.g., props: any or PageComponentProps) and pass them through to
the lazy-loaded component when rendering, and change the lazy loader to safely
handle both executable and already-resolved CodeRefs (use isExecutableCodeRef
guard or try/catch to call component() only when callable, otherwise use the
value directly). Ensure you reference and update the LazyDynamicTab definition
and the inner lazy(...) factory that uses the component variable so props are
forwarded and CodeRef resolution is robust.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
frontend/public/components/utils/horizontal-nav.tsx
| const LazyDynamicTab: FC<{ component: CodeRef<React.ComponentType> }> = ({ component }) => { | ||
| const LazyComponent = useMemo( | ||
| () => | ||
| lazy(async () => { | ||
| const Component = await component(); | ||
| return { default: Component }; | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find CodeRef type definition in dynamic-plugin-sdk or types
fd -type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "type CodeRef|interface CodeRef" | head -5Repository: openshift/console
Length of output: 231
🏁 Script executed:
# Locate usePluginRoutes.tsx and similar resolver patterns
fd -type f "usePluginRoutes.tsx" -o -name "*route*.ts" -o -name "*route*.tsx" | head -10Repository: openshift/console
Length of output: 231
🏁 Script executed:
# Search for CodeRef usage patterns and any resolveCodeRef/isCodeRef helpers
rg -n "CodeRef|resolveCodeRef|isCodeRef" -t ts -t tsx | head -40Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Examine the full context of horizontal-nav.tsx around the LazyDynamicTab and pluginPages
cat -n frontend/public/components/utils/horizontal-nav.tsx | head -100Repository: openshift/console
Length of output: 4143
🏁 Script executed:
# Correct search for CodeRef type definition
rg "type CodeRef|interface CodeRef" --type-list | head -20Repository: openshift/console
Length of output: 450
🏁 Script executed:
# Find CodeRef definition in dynamic-plugin-sdk
fd "types" | xargs rg -A 5 "type CodeRef|interface CodeRef" | head -30Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find usePluginRoutes.tsx
fd "usePluginRoutes" | head -5Repository: openshift/console
Length of output: 120
🏁 Script executed:
# Find pluginPages mapping in horizontal-nav.tsx
rg -n "pluginPages|component: \(\)" frontend/public/components/utils/horizontal-nav.tsx | head -20Repository: openshift/console
Length of output: 373
🏁 Script executed:
# Search for resolveExtension pattern
rg -n "resolveExtension|resolveCodeRef" --type-list | head -20Repository: openshift/console
Length of output: 450
🏁 Script executed:
# Find CodeRef definition in dynamic-plugin-sdk/src/types
find . -path "*dynamic-plugin-sdk*" -name "*.ts" | xargs rg -A 3 "type CodeRef|interface CodeRef" 2>/dev/nullRepository: openshift/console
Length of output: 45
🏁 Script executed:
# Inspect usePluginRoutes.tsx
cat -n frontend/packages/console-app/src/hooks/usePluginRoutes.tsx | head -80Repository: openshift/console
Length of output: 3505
🏁 Script executed:
# Find resolveExtension or similar resolvers
rg -n "resolveExtension|resolveCodeRef|isCodeRef" frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Get full context of pluginPages mapping including how tabs are used
cat -n frontend/public/components/utils/horizontal-nav.tsx | sed -n '305,340p'Repository: openshift/console
Length of output: 1605
🏁 Script executed:
# Check HorizontalNav props passed to tabs
rg -n "Route.*component|obj|params" frontend/public/components/utils/horizontal-nav.tsx | head -30Repository: openshift/console
Length of output: 1118
🏁 Script executed:
# Find CodeRef type definition
find . -name "*.ts" | xargs grep -l "type CodeRef\|interface CodeRef" 2>/dev/null | head -5Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Get full CodeRef type definition
rg "type CodeRef" -A 2 2>/dev/null | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for how component props are actually used - check page/component signatures
rg -A 3 "type Page = |interface Page" frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Check if HorizontalNavTab components expect props like obj or params
rg "HorizontalNavTab|NavTab" -B 2 -A 5 | head -40Repository: openshift/console
Length of output: 5317
🏁 Script executed:
# Verify if resolveExtension exists and what it does
rg -n "resolveExtension" --type-list 2>/dev/null || rg "resolveExtension" | head -10Repository: openshift/console
Length of output: 1386
🏁 Script executed:
# Inspect coderef-resolver.ts
cat -n frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts | head -50Repository: openshift/console
Length of output: 2204
🏁 Script executed:
# Find CodeRef type definition
find frontend/packages/console-dynamic-plugin-sdk -name "*.ts" | xargs grep "type CodeRef\|interface CodeRef" 2>/dev/nullRepository: openshift/console
Length of output: 45
🏁 Script executed:
# Check if CodeRef can be already-resolved or is always a function
rg "CodeRef" frontend/packages/console-dynamic-plugin-sdk/src/types.ts -A 2 -B 2 | head -30Repository: openshift/console
Length of output: 332
🏁 Script executed:
# Get the full CodeRef type definition from types
cat frontend/packages/console-dynamic-plugin-sdk/src/types.ts | head -100Repository: openshift/console
Length of output: 413
🏁 Script executed:
# Search for CodeRef export specifically in types.ts
rg "export.*CodeRef" frontend/packages/console-dynamic-plugin-sdk/src/types.ts -A 5Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check imports from `@openshift/dynamic-plugin-sdk` to understand CodeRef origin
rg "from '@openshift/dynamic-plugin-sdk'" frontend/packages/console-dynamic-plugin-sdk/src/types.tsRepository: openshift/console
Length of output: 100
🏁 Script executed:
# Verify how the LazyDynamicTab component is actually used in practice - check if any HorizontalNavTab implementations pass props
fd -type f "\.tsx\|\.ts" | xargs rg "HorizontalNavTab|NavTab" -l | head -10Repository: openshift/console
Length of output: 696
🏁 Script executed:
# Check the HorizontalNavTab type definition
cat -n frontend/packages/console-dynamic-plugin-sdk/src/extensions/horizontal-nav-tabs.ts | head -80Repository: openshift/console
Length of output: 2231
🏁 Script executed:
# Look for any real plugin implementations using HorizontalNavTab or NavTab
find . -path "*/plugins/*" -name "*.ts" -o -name "*.tsx" | xargs rg "HorizontalNavTab\|NavTab" 2>/dev/null | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check the full resolveExtension function signature and implementation
cat -n frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts | sed -n '1,150p'Repository: openshift/console
Length of output: 3872
🏁 Script executed:
# Verify if component prop in page signatures can receive parameters
rg "component.*props|component.*obj|component.*params" frontend/public/components/utils/horizontal-nav.tsx -B 2 -A 2Repository: openshift/console
Length of output: 263
🏁 Script executed:
# Verify how routes pass props to tab components
cat -n frontend/public/components/utils/horizontal-nav.tsx | sed -n '335,360p'Repository: openshift/console
Length of output: 1017
Forward PageComponentProps to lazy-loaded tab components.
The LazyDynamicTab wrapper silently drops all props passed by the Route handler (obj, params, filters, etc.). Tab components declared as CodeRef<React.ComponentType<PageComponentProps>> expect to receive these properties but will render without context. Update LazyDynamicTab to accept and forward props:
Suggested fix
-const LazyDynamicTab: FC<{ component: CodeRef<React.ComponentType> }> = ({ component }) => {
+const LazyDynamicTab: FC<{ component: CodeRef<React.ComponentType<PageComponentProps>> } & PageComponentProps> = ({ component, ...props }) => {
const LazyComponent = useMemo(
() =>
lazy(async () => {
const Component = await component();
return { default: Component };
}),
[component],
);
- return <LazyComponent />;
+ return <LazyComponent {...props} />;
};Additionally, for robustness: if the extension system ever uses useResolvedExtensions upstream, the CodeRef will already be resolved (not a function), and calling component() will fail. Consider guarding with isExecutableCodeRef or using a try-catch pattern to handle both resolved and unresolved CodeRefs safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/horizontal-nav.tsx` around lines 40 - 46,
The LazyDynamicTab currently calls component() and returns a lazy component but
does not accept or forward any props, and it will throw if component is already
resolved (not executable); update the LazyDynamicTab FC to accept props (e.g.,
props: any or PageComponentProps) and pass them through to the lazy-loaded
component when rendering, and change the lazy loader to safely handle both
executable and already-resolved CodeRefs (use isExecutableCodeRef guard or
try/catch to call component() only when callable, otherwise use the value
directly). Ensure you reference and update the LazyDynamicTab definition and the
inner lazy(...) factory that uses the component variable so props are forwarded
and CodeRef resolution is robust.
| .map((tab) => ({ | ||
| ...tab.properties.page, | ||
| component: (pageProps: PageComponentProps) => ( | ||
| <AsyncComponent {...pageProps} loader={tab.properties.component} /> | ||
| ), | ||
| component: () => <LazyDynamicTab component={tab.properties.component} />, | ||
| })); | ||
|
|
||
| const resolvedNavTab = navTabExtensions | ||
| .filter((tab) => tab.properties.contextId === contextId) | ||
| .map((tab) => ({ | ||
| name: tab.properties.name, | ||
| href: tab.properties.href, | ||
| component: (pageProps: PageComponentProps) => ( | ||
| <AsyncComponent {...pageProps} loader={tab.properties.component} /> | ||
| ), | ||
| component: () => <LazyDynamicTab component={tab.properties.component} />, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the horizontal-nav.tsx file structure and find LazyDynamicTab
wc -l frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Let's examine the LazyDynamicTab implementation and the context around lines 315-326
sed -n '1,50p' frontend/public/components/utils/horizontal-nav.tsx | head -50Repository: openshift/console
Length of output: 2034
🏁 Script executed:
#!/bin/bash
# Look for LazyDynamicTab definition and the code context
rg -n "LazyDynamicTab" frontend/public/components/utils/horizontal-nav.tsx -B 2 -A 5Repository: openshift/console
Length of output: 1021
🏁 Script executed:
#!/bin/bash
# Check for PageComponentProps type definition
rg -n "PageComponentProps" frontend/public/components/utils/ -A 2Repository: openshift/console
Length of output: 606
🏁 Script executed:
#!/bin/bash
# Look at the NavPage type definition and how component is used
rg -n "type NavPage|interface NavPage" -A 8 frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check how the component field is rendered - where navPages are rendered
sed -n '250,315p' frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 2571
🏁 Script executed:
#!/bin/bash
# Look for where the component function is called to understand prop passing
rg -n "component\(" frontend/public/components/utils/horizontal-nav.tsx -B 1 -A 1 | head -40Repository: openshift/console
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Look at more of the render logic to find where component() is called
sed -n '200,350p' frontend/public/components/utils/horizontal-nav.tsx | tail -100Repository: openshift/console
Length of output: 3744
🏁 Script executed:
#!/bin/bash
# Search for where routes are created from navPages
rg -n "Route\|component\(" frontend/public/components/utils/horizontal-nav.tsx -A 1 | head -60Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at the entire file structure to understand the flow better
sed -n '100,200p' frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 2503
🏁 Script executed:
#!/bin/bash
# Get the full PageComponentProps definition
sed -n '400,420p' frontend/public/components/utils/horizontal-nav.tsxRepository: openshift/console
Length of output: 522
🏁 Script executed:
#!/bin/bash
# Search for plugins or external tabs that might expect these props
rg -n "PageComponentProps|tab.properties" frontend/public/components/utils/ -A 1 -B 1 | head -40Repository: openshift/console
Length of output: 2005
🏁 Script executed:
#!/bin/bash
# Check the NavTab and HorizontalNavTab extension types to understand expected prop shape
rg -n "export.*NavTab|interface.*NavTab|type.*NavTab" --type ts --type tsx | grep -E "(NavTab|HorizontalNav)" | head -20Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the plugin SDK types for NavTab and HorizontalNavTab
find . -path "*/node_modules" -prune -o -name "*.ts" -o -name "*.tsx" | xargs grep -l "HorizontalNavTab\|export.*NavTab" | head -5Repository: openshift/console
Length of output: 347
🏁 Script executed:
#!/bin/bash
# Look for any test files or examples that show how tab components should work
rg -n "HorizontalNavTab\|NavTab" -A 3 --type ts --type tsx -g "*.test.*" -g "*.spec.*" 2>/dev/null | head -30Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at the NavTab and HorizontalNavTab type definitions
cat ./frontend/packages/console-dynamic-plugin-sdk/src/extensions/horizontal-nav-tabs.tsRepository: openshift/console
Length of output: 1846
🏁 Script executed:
#!/bin/bash
# Also check what the component property is supposed to accept
rg -n "component.*CodeRef|CodeRef.*component" ./frontend/packages/console-dynamic-plugin-sdk/src/extensions/horizontal-nav-tabs.ts -B 2 -A 2Repository: openshift/console
Length of output: 360
Forward PageComponentProps through LazyDynamicTab wrapper to tab components.
Plugin tab components are typed to accept PageComponentProps (obj, params, filters, selected, customData, etc.), but the wrapper silently discards these. The arrow functions on lines 317 and 325 have no parameters, and LazyDynamicTab doesn't forward any props to the lazy-loaded component. This breaks tabs that rely on resource context.
✅ Proposed fix (prop forwarding)
-const LazyDynamicTab: FC<{ component: CodeRef<React.ComponentType> }> = ({ component }) => {
+const LazyDynamicTab: FC<PageComponentProps & { component: CodeRef<React.ComponentType> }> = ({
+ component,
+ ...props
+}) => {
const LazyComponent = useMemo(
() =>
lazy(async () => {
const Component = await component();
return { default: Component };
}),
[component],
);
- return <LazyComponent />;
+ return <LazyComponent {...props} />;
};
@@
- component: () => <LazyDynamicTab component={tab.properties.component} />,
+ component: (props) => <LazyDynamicTab component={tab.properties.component} {...props} />,
@@
- component: () => <LazyDynamicTab component={tab.properties.component} />,
+ component: (props) => <LazyDynamicTab component={tab.properties.component} {...props} />,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/horizontal-nav.tsx` around lines 315 - 326,
The tab wrapper currently defines component as parameterless arrow functions (in
the navTabExtensions map and resolvedNavTab map) so LazyDynamicTab never
receives PageComponentProps; update those mappings to accept and forward props
(e.g., component: (props) => <LazyDynamicTab
component={tab.properties.component} {...props} />) so LazyDynamicTab receives
PageComponentProps and can forward them to the lazy-loaded component; ensure
both places that create component use the same prop-forwarding pattern and that
LazyDynamicTab passes all received props through to the inner component.
upalatucci
changed the title
|
@upalatucci: This pull request references Jira Issue OCPBUGS-65728, 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
|
@upalatucci: This pull request references Jira Issue OCPBUGS-65728, 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. |
|
@upalatucci: The following test failed, say
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. |
|
/retest |
|
@logonoff ops... yes its already solved by your pr. I didn't have the branch updated i think. |
|
@logonoff can we backport it to 4.20 at least? |
3 participants
Solution description
Dynamic plugin
console.tab/horizontalNavandconsole.tabextensions useAsyncComponentto load their tab content via CodeRef functions.AsyncComponentcallsthis.state.loader().then(...), expecting the loader to return a Promise. However, CodeRef properties on shared extension objects can get mutated from async loader functions to resolved React components (sinceObject.freezeon extensions is shallow andresolveExtensionmutatesextension.propertiesin place). When this happens, calling the component as a loader returns JSX instead of a Promise, causing:This PR replaces
AsyncComponentwithReact.lazyfor dynamic plugin horizontal nav tabs.React.lazyproperly awaits the CodeRef and renders the resolved component, making it resilient to this mutation. This matches the pattern already used forconsole.page/routeextensions inusePluginRoutes.tsx.Reproduction repo: https://github.com/portworx/ocp-dynamic-plugin-bug-reproduction
Test cases
console.tab/horizontalNavextensions (e.g. the Portworx reproduction plugin)Additional info
The existing
LazyDynamicTabcomponent pattern mirrorsLazyDynamicRoutePageinusePluginRoutes.tsx, which already handlesconsole.page/routeCodeRefs correctly usingReact.lazy. The<Suspense>wrapper already present around the<Routes>inHorizontalNavprovides the loading fallback.Made with Cursor
Summary by CodeRabbit