Render additional components next to Routes#3109
Render additional components next to Routes#3109openshift-merge-bot[bot] merged 1 commit intoopenshift-assisted:masterfrom
Conversation
openshift-ci
bot
added
the
approved
WalkthroughMoved additionalComponents rendering inside a React fragment that also wraps the Routes block, ensuring it renders within the router context. No public API or route structure changes. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Router
participant Routes
participant AdditionalComponents
App->>Router: Initialize routing context
Router->>Routes: Render route tree
Router->>AdditionalComponents: Render inside router context
Estimated code review effort🎯 2 (Simple) | ⏱️ ~6 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
the
size/S
|
/cherry-pick releases/v2.44 |
|
@rawagner: once the present PR merges, I will cherry-pick it on top of 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/Routes.tsx (3)
41-41: Use an absolute path in Navigate to avoid relative resolution pitfallsUsing a relative target can behave unexpectedly under nested routers or differing current locations. Prefer an absolute path.
Apply this diff:
- <Route path="*" element={<Navigate to="assisted-installer/clusters" />} /> + <Route path="*" element={<Navigate to="/assisted-installer/clusters" />} />
43-43: Clarify intended contents of additionalComponentsIf any consumer mistakenly passes elements via additionalComponents, they won’t be registered (since they’re outside ). Keep routes in children and use additionalComponents for non-route UI that needs router context (e.g., modals, toasters, portals).
Would you like me to add a brief JSDoc on the prop to document this distinction?
35-41: Deduplicate the clusters base path to reduce typo riskThe string "assisted-installer/clusters" appears in both the route and Navigate. Extracting a constant improves maintainability.
Apply this diff within the block:
- <Route path="assisted-installer/clusters" element={<Outlet />}> + <Route path={CLUSTERS_BASE_PATH} element={<Outlet />}> @@ - <Route path="*" element={<Navigate to="/assisted-installer/clusters" />} /> + <Route path="*" element={<Navigate to={`/${CLUSTERS_BASE_PATH}`} />} />Add this near the imports (outside the changed block):
const CLUSTERS_BASE_PATH = 'assisted-installer/clusters' as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-lib/lib/ocm/components/Routes.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/Routes.tsx (4)
libs/ui-lib/lib/ocm/components/clusters/NewClusterPage.tsx (1)
NewClusterPage(46-50)libs/ui-lib-tests/cypress/views/pages/NewClusterPage.ts (1)
NewClusterPage(1-5)libs/ui-lib/lib/ocm/components/clusters/ClusterPage.tsx (1)
ClusterPage(215-222)libs/ui-lib-tests/cypress/views/pages/ClusterPage.ts (1)
ClusterPage(3-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: lint
🔇 Additional comments (1)
libs/ui-lib/lib/ocm/components/Routes.tsx (1)
33-44: Renders additionalComponents within router context — good changeMoving additionalComponents into the same fragment as Routes ensures they have router context regardless of HistoryRouter usage. This addresses context issues without altering public API.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82, rawagner 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-merge-bot
bot
merged commit 2148d80
into
openshift-assisted:master
|
@rawagner: new pull request created: #3112 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. |
3 participants
Summary by CodeRabbit
Bug Fixes
Refactor