Fixed Breadcrumb to Display Detailed Name Instead of ID #9200
Conversation
WalkthroughThe changes in this pull request introduce a new property, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Patient/SampleDetails.tsx (1)
302-304: Consider adding a type check for patient_name.While the loading check prevents null/undefined errors, it would be good to add an additional type check for patient_name to handle edge cases where the API response might be incomplete.
<Page title={t("sample_test_details")} - crumbsReplacements={{ [id]: { name: sampleDetails.patient_name } }} + crumbsReplacements={{ [id]: { name: sampleDetails.patient_name || t("unknown_patient") } }} backUrl="/sample"src/components/Shifting/ShiftDetails.tsx (3)
481-483: LGTM! Consider enhancing null safety.The implementation correctly replaces the ID with the patient's name in the breadcrumb navigation. However, we could enhance the null safety by providing a fallback value.
crumbsReplacements={{ - [props.id]: { name: data?.patient_object?.name }, + [props.id]: { name: data?.patient_object?.name || t("unknown_patient") }, }}
Line range hint
32-38: Fix clipboard copy feedback and timer management.The clipboard copy functionality has potential issues:
- The
isCopiedstate isn't reset when copying different data- The
setTimeoutfor resettingisCopiedcould cause memory leaksConsider refactoring the clipboard functionality:
+ import { useEffect } from "react"; const [isCopied, setIsCopied] = useState(false); + + useEffect(() => { + if (isCopied) { + const timer = setTimeout(() => { + setIsCopied(false); + }, 5000); + return () => clearTimeout(timer); + } + }, [isCopied]); const showCopyToclipBoard = (data: any) => { return ( <a href="#"> <CopyToClipboard text={copyContent(data)} - onCopy={() => setIsCopied(true)} + onCopy={() => { + setIsCopied(false); // Reset first to ensure state change even if same data + setIsCopied(true); + }} >Also applies to: 481-483
Line range hint
1-783: Consider splitting the component for better maintainability.The component has grown quite large and handles multiple responsibilities. Consider splitting it into smaller, focused components:
- Extract print-related logic into a separate component
- Move facility and patient card rendering into their own components
- Consider using a custom hook for clipboard functionality
Example structure:
// ShiftDetailsPrint.tsx export const ShiftDetailsPrint = ({ data, onClose }) => { // printData logic here }; // ShiftDetailsCards.tsx export const FacilityCard = ({ facility }) => { // showFacilityCard logic here }; export const PatientCard = ({ patient }) => { // showPatientCard logic here }; // hooks/useClipboard.ts export const useClipboard = (timeout = 5000) => { // clipboard logic here };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/components/Patient/SampleDetails.tsx(1 hunks)src/components/Patient/models.tsx(1 hunks)src/components/Shifting/ShiftDetails.tsx(1 hunks)
🔇 Additional comments (5)
src/components/Patient/models.tsx (2)
168-168: LGTM! Type change aligns with requirements
The change from number to string for patient_name is correct and maintains consistency with other interfaces in the file that use string-based name fields.
168-168: Verify the impact of this breaking change
This type change from number to string could affect existing code that expects patient_name to be a number.
Let's verify the usage across the codebase:
src/components/Patient/SampleDetails.tsx (3)
303-303: LGTM! Breadcrumb replacement implementation looks correct.
The implementation correctly uses the patient_name from sampleDetails to replace the ID in the breadcrumb. The null check on line 302 (if (isLoading || !sampleDetails)) ensures this prop is only set when data is available.
303-303: Verify consistency of patient name display.
The patient name is displayed in both the breadcrumb and the details section. Let's verify that the same value is used consistently.
Also applies to: 371-373
✅ Verification successful
Based on the search results, I can now provide a final response regarding the consistency of patient name display:
Patient name is consistently displayed in both locations
The SampleDetails component consistently uses sampleDetails.patient_name in both locations:
- In the breadcrumb:
crumbsReplacements={{ [id]: { name: sampleDetails.patient_name } }} - In the details section:
<span id="patient_name">{sampleDetails?.patient_name}</span>
The same data source is used for both displays, ensuring consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent patient name usage across components
# Look for all occurrences of patient_name to ensure consistent usage
rg -A 2 'patient_name' --type tsx --type ts
Length of output: 71
Script:
#!/bin/bash
# Search for patient_name occurrences without file type restrictions
rg -A 2 'patient_name'
Length of output: 6878
Script:
#!/bin/bash
# Search for components that might display patient names using common patterns
rg -A 2 'patient\.name|patientName|patient_name'
Length of output: 21615
Script:
#!/bin/bash
# Look for the SampleDetails component implementation
ast-grep --pattern 'const SampleDetails = $_'
Length of output: 38042
303-303: Verify Page component compatibility and type definitions.
Let's verify that:
- The Page component properly handles the crumbsReplacements prop
- The patient_name type is correctly defined as string in models.tsx
✅ Verification successful
The crumbsReplacements prop and patient_name type are properly defined
Based on the verification results:
- The Page component correctly inherits and handles the crumbsReplacements prop through PageTitleProps interface, which is properly typed as
{ [key: string]: { name?: string; uri?: string; style?: string } }. - The patient_name is correctly defined as an optional string (
patient_name?: string) in the SampleTestModel interface in models.tsx.
The usage in SampleDetails.tsx is type-safe and consistent with these definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Page component props and patient_name type definition
# Check Page component props
ast-grep --pattern 'interface $props {
$$$
crumbsReplacements?: $_
$$$
}'
# Check patient_name type in models
ast-grep --pattern 'interface $model {
$$$
patient_name: string
$$$
}'
Length of output: 160
Script:
#!/bin/bash
# Let's try a different approach to find the Page component and its props
# Search for Page component definition and crumbsReplacements usage
rg -A 5 "interface PageProps"
# Search for any interface or type containing patient_name
rg -A 3 "patient_name.*:"
# Search for actual usage of crumbsReplacements in Page component
rg "crumbsReplacements"
# Look for Page component implementation
ast-grep --pattern 'const Page = $_'
Length of output: 7303
Script:
#!/bin/bash
# Let's check the PageTitleProps interface and crumbsReplacements type definition
rg -A 10 "interface PageTitleProps"
# Check the actual implementation of crumbsReplacements in PageTitle
cat src/components/Common/PageTitle.tsx
# Check the patient_name type in the specific interface from models.tsx
cat src/components/Patient/models.tsx
Length of output: 15508
package-lock.json
Outdated
| } |
There was a problem hiding this comment.
Minor thing, but can you get changes in package-lock removed from the commit?
You can reset the HEAD, remove from staging and do a force push.
rayyjeb
force-pushed
the
issues/9197/BreadCrumb_ID_Fix
branch
from
9ca4a4a
to
2fdffa2
Compare
|
@rayyjeb Ensure that other changes are in staging though. |
|
@Jacobjeevan Should I reopen the pull request once again since, the it is showing as closed |
Yea |
|
@rithviknishad Could you please review the PR again? If there's anything, Do let me know |
He already approved it 🤔 and there hasn't been any changes since approval. |
|
LGTM |
|
@rayyjeb Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
New Features
crumbsReplacementsfor enhanced breadcrumb navigation on the Sample Details and Shift Details pages, displaying the patient's name.Bug Fixes
patient_nameproperty type in the SampleTestModel for better data handling.