Display Descriptive Names in Breadcrumbs Instead of External IDs #9863
Conversation
WalkthroughThe pull request enhances the Breadcrumbs component by introducing dynamic name fetching for facilities, patients, and encounters. New asynchronous functions are implemented to retrieve names based on IDs from the current path. The component now intelligently identifies ID segments, fetches corresponding names via API calls, and updates the breadcrumbs display accordingly. This allows for more informative and context-aware navigation by replacing raw IDs with human-readable names during rendering. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Nitpick comments (4)
src/components/Common/Breadcrumbs.tsx (4)
3-3: Consider wrapping async operations in an error boundaryWhile the new imports support the async functionality, consider implementing a React Error Boundary to gracefully handle any potential API failures at the component level, preventing the entire navigation from breaking.
Also applies to: 23-24
87-97: Improve encounter date formattingThe encounter date concatenation could benefit from proper date formatting and localization.
- return "Encounter on " + response.data?.period.start || id; + const date = new Date(response.data?.period.start); + return `Encounter on ${date.toLocaleDateString()}` || id;
99-117: Refactor ID type detection for better maintainabilityThe current implementation uses sequential if-else statements which could become harder to maintain as more types are added. Consider using a mapping object for better maintainability.
+ const ID_TYPE_MAPPING = { + facility: "facility", + patient: "patient", + encounter: "encounter" + }; const idQueries = path ?.slice(1) .split("/") .map((field, i, arr) => { const isId = /^[0-9a-fA-F-]{36}$/.test(field); const prevBreadcrumb = arr[i - 1]; - if (isId) { - if (prevBreadcrumb === "facility") { - return { id: field, type: "facility" }; - } else if (prevBreadcrumb === "patient") { - return { id: field, type: "patient" }; - } else if (prevBreadcrumb === "encounter") { - return { id: field, type: "encounter" }; - } - } + if (isId && prevBreadcrumb in ID_TYPE_MAPPING) { + return { id: field, type: ID_TYPE_MAPPING[prevBreadcrumb] }; + } return null; })
153-170: Enhance loading and error state UIThe current implementation uses a simple "Loading..." text. Consider implementing a more user-friendly loading state and error handling UI.
- (isId - ? names[field] || "Loading..." - : MENU_TAGS[field] || capitalize(field)), + (isId + ? names[field] === undefined + ? <span className="animate-pulse">Loading...</span> + : names[field].startsWith("Error") + ? <span className="text-red-500">{names[field]}</span> + : names[field] + : MENU_TAGS[field] || capitalize(field)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/Breadcrumbs.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/components/Common/Breadcrumbs.tsx (1)
Line range hint
1-170: Implementation successfully addresses the PR objectivesThe changes effectively replace external IDs with descriptive names in the breadcrumbs, improving the user experience as intended. While there are suggestions for improvements, the core functionality is well-implemented.
| useEffect(() => { | ||
| const getNames = async () => { | ||
| const fetchedNames = await fetchNames(); | ||
| setNames(fetchedNames); | ||
| }; | ||
|
|
||
| getNames(); | ||
| }, [path]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper cleanup and caching
The current implementation lacks cleanup for pending requests and doesn't cache previously fetched names. This could lead to unnecessary API calls and potential memory leaks.
useEffect(() => {
+ const controller = new AbortController();
const getNames = async () => {
const fetchedNames = await fetchNames();
setNames(fetchedNames);
};
getNames();
+ return () => controller.abort();
}, [path]);Consider implementing a caching mechanism to store previously fetched names:
const [nameCache, setNameCache] = useState<Record<string, string>>({});
const fetchNames = async () => {
const results: Record<string, string> = {};
for (const query of idQueries || []) {
if (query?.id in nameCache) {
results[query.id] = nameCache[query.id];
continue;
}
// ... existing fetch logic
}
setNameCache(prev => ({...prev, ...results}));
return results;
};| const fetchFacilityName = async (id: string) => { | ||
| try { | ||
| const response = await request(routes.getAnyFacility, { | ||
| pathParams: { id }, | ||
| }); | ||
| return response.data?.name || id; | ||
| } catch (error) { | ||
| console.error("Error fetching facility name:", error); | ||
| return "Error fetching facility"; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with user-friendly messages
The current error handling returns a generic message. Consider providing more context-specific messages and implementing proper error tracking.
} catch (error) {
- console.error("Error fetching facility name:", error);
- return "Error fetching facility";
+ console.error("Failed to fetch facility name:", { id, error });
+ return `Facility ${id.slice(0, 8)}...`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchFacilityName = async (id: string) => { | |
| try { | |
| const response = await request(routes.getAnyFacility, { | |
| pathParams: { id }, | |
| }); | |
| return response.data?.name || id; | |
| } catch (error) { | |
| console.error("Error fetching facility name:", error); | |
| return "Error fetching facility"; | |
| } | |
| }; | |
| const fetchFacilityName = async (id: string) => { | |
| try { | |
| const response = await request(routes.getAnyFacility, { | |
| pathParams: { id }, | |
| }); | |
| return response.data?.name || id; | |
| } catch (error) { | |
| console.error("Failed to fetch facility name:", { id, error }); | |
| return `Facility ${id.slice(0, 8)}...`; | |
| } | |
| }; |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit