fix(iop): separate route for pathway details#1237
Open
Fewwy wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
IopPathwayDetails, you can drop theuseRouteMatchhook and readslugfromprops.match.params.slug, since the router already passes route params in via therenderprop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `IopPathwayDetails`, you can drop the `useRouteMatch` hook and read `slug` from `props.match.params.slug`, since the router already passes route params in via the `render` prop.
## Individual Comments
### Comment 1
<location path="webpack/IopPathwayDetails/IopPathwayDetails.js" line_range="12-16" />
<code_context>
+const scope = 'advisor';
+const pathwayModule = './PathwayDetailsWrapped';
+
+const IopPathwayDetails = props => {
+ const pathwayMatch = useRouteMatch(
+ '/foreman_rh_cloud/recommendations/pathways/:slug'
+ );
+ const slug = pathwayMatch?.params?.slug;
+
+ return (
</code_context>
<issue_to_address>
**suggestion:** Consider using the route props instead of `useRouteMatch` to avoid redundant matching.
Because this component is rendered via `<Route render={props => <IopPathwayDetails {...props} />}`, you can read the slug directly from `props.match.params.slug` (or the router props you pass down) instead of calling `useRouteMatch` with the same pattern. This avoids duplicate matching and keeps the route definition as the single source of truth if the path changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+12
to
+16
| const IopPathwayDetails = props => { | ||
| const pathwayMatch = useRouteMatch( | ||
| '/foreman_rh_cloud/recommendations/pathways/:slug' | ||
| ); | ||
| const slug = pathwayMatch?.params?.slug; |
There was a problem hiding this comment.
suggestion: Consider using the route props instead of useRouteMatch to avoid redundant matching.
Because this component is rendered via <Route render={props => <IopPathwayDetails {...props} />}, you can read the slug directly from props.match.params.slug (or the router props you pass down) instead of calling useRouteMatch with the same pattern. This avoids duplicate matching and keeps the route definition as the single source of truth if the path changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pathway details and recommendation details were rendered by a single component (IopRecommendationDetails) on one catch-all route, using useRouteMatch internally to switch between views.
When inside the federated PathwayDetailsWrapped module navigated to a recommendation details URL, the route still matched the same catch-all, so the host router never re-mounted the component and useRouteMatch didn't re-evaluate — leaving the user stuck on the Pathway details page despite the URL changing.
Splitting into separate routes with a dedicated IopPathwayDetails component ensures the router treats pathway-to-recommendation navigation as a proper route change, unmounting one view and mounting the other.
How to repeat the bug:
Run the dev environment, open pathway details page, click on Link to the Recommendation details in Recommendations table - you will see URL is changed but you are still on the same page and not navigated to the Recommendation details page