🪛(fix): breadcrumb not appearing on certain routes #273
Conversation
For example if routing to `/accounting/division-codes` the breadcrumb would not appear, but would appear if you routed to `/accounting/division-codes/`
Reviewer's Guide by SourceryThis pull request addresses the issue of breadcrumbs not appearing on certain routes by updating the route matching logic and refactoring the breadcrumb component into a more structured and reusable format. The layout component has been updated to use the new File-Level Changes
Tips
|
emoss08
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @emoss08 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @emoss08 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @emoss08 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| setCurrentRoute(matchedRoute || null); | ||
| setLoading(false); | ||
| }, [location, setLoading, setCurrentRoute]); | ||
| }; | ||
| }, [location, setCurrentRoute, setLoading, matchingRoute]); |
There was a problem hiding this comment.
suggestion: Dependency array might be over-specified.
Including matchingRoute in the dependency array might be redundant since it is derived from location.pathname. Consider simplifying the dependency array to [location, setCurrentRoute, setLoading].
| }, [location, setCurrentRoute, setLoading, matchingRoute]); | |
| }, [location, setCurrentRoute, setLoading]); |
| const breadcrumbItems = useMemo(() => { | ||
| if (!currentRoute) return []; | ||
| const items: BreadcrumbItemType[] = [ | ||
| { label: "Home", path: "/" }, | ||
| ...(currentRoute.group | ||
| ? [ | ||
| { | ||
| label: upperFirst(currentRoute.group), | ||
| path: `/${currentRoute.group}`, | ||
| }, |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with breadcrumb path construction.
The path construction for breadcrumbItems assumes that currentRoute.group is always defined when currentRoute.subMenu is defined. If this assumption is not always true, it could lead to incorrect paths.
| const BreadcrumbEllipsis = ({ | ||
| className, | ||
| ...props | ||
| }: React.ComponentProps<"span">) => ( | ||
| <span | ||
| role="presentation" | ||
| aria-hidden="true" | ||
| className={cn("flex h-9 w-9 items-center justify-center", className)} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
issue (typo): Typo in displayName.
The displayName for BreadcrumbEllipsis is misspelled as BreadcrumbElipssis. This should be corrected to avoid confusion.
| return parts.join(" - "); | ||
| }, [currentRoute]); | ||
| const breadcrumbItems = useMemo(() => { | ||
| if (!currentRoute) return []; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!currentRoute) return []; | |
| if (!currentRoute) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @emoss08 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| ); | ||
| }); | ||
|
|
||
| const matchedRoute = matchingRoute; |
There was a problem hiding this comment.
issue: Redundant assignment of matchedRoute.
The assignment of matchedRoute to matchingRoute is redundant since matchingRoute is already defined. You can directly use matchingRoute in setCurrentRoute.
| useEffect(() => { | ||
| if (currentRoute) { | ||
| document.title = currentRoute.title; | ||
| } | ||
| }, [currentRoute]); | ||
| }; | ||
| const breadcrumbItems = useMemo(() => { | ||
| if (!currentRoute) return []; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!currentRoute) return []; | |
| if (!currentRoute) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Summary by Sourcery
This pull request addresses the issue of breadcrumbs not appearing on certain routes by updating the route matching logic and refactoring the breadcrumb component. Additionally, it enhances the breadcrumb UI and ensures consistency in route paths.