Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display Descriptive Names in Breadcrumbs Instead of External IDs #9863

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+106 −10
Open

Display Descriptive Names in Breadcrumbs Instead of External IDs #9863

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 106 additions & 10 deletions src/components/Common/Breadcrumbs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Link, usePath } from "raviger";
import { useState } from "react";
import { useEffect } from "react";

import CareIcon from "@/CAREUI/icons/CareIcon";

Expand All @@ -19,6 +20,8 @@ import {

import useAppHistory from "@/hooks/useAppHistory";

import routes from "@/Utils/request/api";
import request from "@/Utils/request/request";
import { classNames } from "@/Utils/utils";

const MENU_TAGS: { [key: string]: string } = {
Expand Down Expand Up @@ -59,19 +62,112 @@ export default function Breadcrumbs({
const path = usePath();
const [showFullPath, setShowFullPath] = useState(false);

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";
}
};
Comment on lines +65 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)}...`;
}
};


const fetchPatientName = async (id: string) => {
try {
const response = await request(routes.getPatient, { pathParams: { id } });
return response.data?.name || id;
} catch (error) {
console.error("Error fetching patient name:", error);
return "Error fetching patient";
}
};

const fetchEncounterName = async (id: string) => {
try {
const response = await request(routes.encounter.get, {
pathParams: { id },
});
return "Encounter on " + response.data?.period.start || id;
} catch (error) {
console.error("Error fetching encounter name:", error);
return "Error fetching 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" };
}
}
return null;
})
.filter(Boolean);

const fetchNames = async () => {
const results: Record<string, string> = {};

for (const query of idQueries || []) {
let name = "";
if (query?.type === "facility") {
name = await fetchFacilityName(query.id);
} else if (query?.type === "patient") {
name = await fetchPatientName(query.id);
} else if (query?.type === "encounter") {
name = await fetchEncounterName(query.id);
}
if (query) {
results[query.id] = name;
}
}

return results;
};

const [names, setNames] = useState<Record<string, string>>({});

useEffect(() => {
const getNames = async () => {
const fetchedNames = await fetchNames();
setNames(fetchedNames);
};

getNames();
}, [path]);
Comment on lines +141 to +148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 crumbs = path
?.slice(1)
.split("/")
.map((field, i) => ({
name: replacements[field]?.name || MENU_TAGS[field] || capitalize(field),
uri:
replacements[field]?.uri ||
path
.split("/")
.slice(0, i + 2)
.join("/"),
style: replacements[field]?.style || "",
}));
.map((field, i) => {
const isId = /^[0-9a-fA-F-]{36}$/.test(field);

return {
name:
replacements[field]?.name ||
(isId
? names[field] || "Loading..."
: MENU_TAGS[field] || capitalize(field)),
uri:
replacements[field]?.uri ||
path
.split("/")
.slice(0, i + 2)
.join("/"),
style: replacements[field]?.style || "",
};
});

const renderCrumb = (crumb: any, index: number) => {
const isLastItem = index === crumbs!.length - 1;
Expand Down
Loading