Added the CSV Export button In Discharged Patients #9142
Conversation
WalkthroughThis pull request introduces several enhancements to the codebase, focusing on CSV export functionality and date range calculations. A new utility function Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
203-210: UseforEachinstead ofmapfor side effectsThe
Array.prototype.mapmethod is used for transforming arrays and returns a new array. Since the returned array isn't utilized,forEachis more appropriate for iterating over the array for side effects.Apply this diff to use
forEach:- lines.map((line: any, i: number) => { + lines.forEach((line: any, i: number) => {
199-245: Correct grammatical errors in commentsThere are typographical errors and grammatical issues in the comments, such as "imforamation" and "there" instead of "their". Correcting these will improve code readability.
Example correction:
- // Generate a array which contains imforamation of duplicate patient IDs and there respective linenumbers + // Generate an array that contains information about duplicate patient IDs and their respective line numbers
455-465: Adjust scroll target to appropriate filter sectionWhen the export is not allowed, clicking the export button opens the advanced filter and scrolls to an element with the ID
bed-type-select. Since the issue is related to selecting a date range, the scroll target should be the date range selector.Apply this diff to update the scroll target:
- const element = - document.getElementById("bed-type-select"); + const element = + document.getElementById("created-date-select");Ensure that the date range selector has the corresponding ID
created-date-selector adjust the ID accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
195-196:
Reevaluate the logic for allowing export
The isExportAllowed condition may not align with the requirement that the export is permitted only when a valid seven-day period is selected. Currently, if no date ranges are selected (durations contains zeros), isExportAllowed might erroneously allow exports.
Please verify and adjust the logic to ensure that export is allowed only when at least one date range of up to seven days is selected. Consider modifying the condition as follows:
- durations.every((x) => x >= 0 && x <= 7) &&
- !durations.every((x) => x === 0);
+ durations.some((x) => x > 0 && x <= 7);
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
452-505: Improve configurability of export UI behaviorThe implementation has several hard-coded values that should be configurable:
- Timeout duration for scroll animation
- Scroll behavior configuration
- Magic number in tooltip text
Consider these improvements:
+ const EXPORT_CONFIG = { + SCROLL_DELAY_MS: 500, + MAX_EXPORT_DAYS: 7, + SCROLL_BEHAVIOR: 'smooth' as ScrollBehavior + }; + <div className="tooltip w-full md:w-auto" id="patient-export"> {!isExportAllowed ? ( <ButtonV2 onClick={() => { advancedFilter.setShow(true); setTimeout(() => { const element = document.getElementById("bed-type-select"); if (element) element.scrollIntoView({ - behavior: "smooth" + behavior: EXPORT_CONFIG.SCROLL_BEHAVIOR }); Notification.Warn({ - msg: "Please select a seven day period.", + msg: `Please select a ${EXPORT_CONFIG.MAX_EXPORT_DAYS} day period.`, }); - }, 500); + }, EXPORT_CONFIG.SCROLL_DELAY_MS); }}
473-497: Add loading state to export functionalityThe export action should indicate its progress to prevent multiple clicks and provide feedback to users.
Consider adding loading state:
<ExportMenu disabled={!isExportAllowed} exportItems={[ { label: "Export Discharged patients", + showLoading: true, action: async () => { + try { const query = { ...qParams, csv: true, }; const pathParams = { facility_external_id }; const { data } = await request( routes.listFacilityDischargedPatients, { query, pathParams, }, ); return data ?? null; + } catch (error) { + Notification.Error({ + msg: "Failed to export patients data", + }); + return null; + } }, parse: preventDuplicatePatientsDuetoPolicyId, }, ]} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
183-192:
Fix duration calculation logic
The current implementation has a few issues:
- The duration calculation can result in negative values
- The comment about XOR is misleading as it's using logical operators
Apply this diff to fix the issues:
const durations = date_range_fields.map((field: string[]) => {
- // XOR (checks if only one of the dates is set)
+ // Check if exactly one date is set
if ((field[0] && !field[1]) || (!field[0] && field[1])) {
return -1;
}
if (field[0] && field[1]) {
- return dayjs(field[0]).diff(dayjs(field[1]), "days");
+ return Math.abs(dayjs(field[1]).diff(dayjs(field[0]), "days"));
}
return 0;
});Likely invalid or redundant comment.
|
@AnveshNalimela Any updates here? |
|
I will push the requested changes by tmrw |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/Utils/removeDuplicates.ts (3)
15-20: Improve code clarity with proper type definitionsThe duplicate identification logic could be more explicit with proper types.
Consider this improvement:
+interface DuplicateInfo { + patientId: string; + indexSame: number[]; +} + -const linesWithSameId = Array.from(idsMap.entries()) +const linesWithSameId: DuplicateInfo[] = Array.from(idsMap.entries()) - .filter(([_, indices]) => indices.length > 1) + .filter(([, indices]) => indices.length > 1) .map(([patientId, indices]) => ({ patientId, indexSame: indices, }));
37-50: Optimize deduplication performanceThe final deduplication logic could be more efficient for large datasets.
Consider this more efficient approach:
-const uniqueLines = []; -const ids = new Set(); // To keep track of unique patient IDs - -for (const line of lines) { - const patientId = line.split(",")[0]; // Extract the patient ID from each line - if (!ids.has(patientId)) { - uniqueLines.push(line); - ids.add(patientId); - } -} - -const cleanedData = uniqueLines.join("\n"); +const cleanedData = Array.from( + lines.reduce((map, line) => { + const patientId = line.split(",")[CSV_COLUMNS.PATIENT_ID]; + if (!map.has(patientId)) { + map.set(patientId, line); + } + return map; + }, new Map()) + .values() +).join("\n");
1-51: Consider architectural improvements and add testsThe current implementation could benefit from:
- Separation of concerns (CSV parsing, deduplication, policy merging)
- A proper CSV parsing library to handle edge cases
- Comprehensive unit tests
Would you like me to:
- Propose a refactored architecture with separate modules?
- Generate unit test cases covering various scenarios?
- Suggest a robust CSV parsing library?
src/components/Facility/DischargedPatientsList.tsx (2)
173-182: Simplify date range validation logicThe current implementation can be made more readable by extracting the validation logic into a separate function.
Consider this refactor:
-const durations = date_range_fields.map((field: string[]) => { - // XOR (checks if only one of the dates is set) - if ((field[0] && !field[1]) || (!field[0] && field[1])) { - return -1; - } - if (field[0] && field[1]) { - return dayjs(field[0]).diff(dayjs(field[1]), "days"); - } - return 0; -}); +const calculateDateRangeDuration = (startDate: string, endDate: string): number => { + if (!startDate && !endDate) return 0; + if (startDate && endDate) { + return Math.abs(dayjs(endDate).diff(dayjs(startDate), "days")); + } + return -1; // Invalid range when only one date is set +}; + +const durations = date_range_fields.map(([startDate, endDate]) => + calculateDateRangeDuration(startDate, endDate) +);
390-443: Improve export button UXThe implementation has a few areas that could be enhanced:
- The setTimeout delay of 500ms for the notification is unnecessary
- The scroll behavior could be smoother with a proper offset
Consider these improvements:
onClick={() => { advancedFilter.setShow(true); - setTimeout(() => { const element = document.getElementById("bed-type-select"); if (element) - element.scrollIntoView({ behavior: "smooth" }); + element.scrollIntoView({ + behavior: "smooth", + block: "center" + }); Notification.Warn({ msg: "Please select a seven day period.", }); - }, 500); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Utils/removeDuplicates.ts(1 hunks)src/components/Facility/DischargedPatientsList.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
411-434: LGTM: Export functionality implementation
The CSV export implementation correctly follows the requirements:
- Validates date range before allowing export
- Processes data to prevent duplicates
- Provides clear user feedback
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Utils/removeDuplicates.ts (3)
1-4: Enhance input validation for CSV dataWhile the basic string validation is good, consider adding CSV-specific validation:
export const preventDuplicatePatientsDuetoPolicyId = (data: string): string => { if (!data || typeof data !== "string") { throw new Error("Input must be a non-empty string"); } + const lines = data.trim().split("\n"); + if (lines.length === 0) { + throw new Error("CSV data must contain at least one line"); + } + if (!lines.every(line => line.includes(","))) { + throw new Error("Invalid CSV format: each line must contain comma-separated values"); + }
5-5: Fix typo in commentThe word "imforamation" should be "information".
- // Generate a array which contains imforamation of duplicate patient IDs and there respective linenumbers + // Generate an array which contains information of duplicate patient IDs and their respective line numbers
57-67: Enhance final data cleanup with validationThe cleanup logic is efficient but could use additional validation:
const cleanedData = Array.from( lines .reduce((map, line) => { - const patientId = line.split(",")[CSV_COLUMNS.PATIENT_ID]; + const columns = line.split(","); + if (columns.length <= CSV_COLUMNS.PATIENT_ID) { + console.warn("Skipping invalid line in cleanup:", line); + return map; + } + const patientId = columns[CSV_COLUMNS.PATIENT_ID].trim(); + if (!patientId) { + console.warn("Skipping line with empty patient ID:", line); + return map; + } if (!map.has(patientId)) { map.set(patientId, line); } return map; }, new Map()) .values(), ).join("\n"); + if (!cleanedData) { + throw new Error("No valid data remains after cleanup"); + }src/components/Facility/DischargedPatientsList.tsx (1)
173-182: Consider enhancing date validation robustnessThe current implementation could be improved to handle edge cases and provide better type safety.
Consider this enhanced implementation:
- const calculateDateRangeDuration = ( - startDate: string, - endDate: string, - ): number => { + const calculateDateRangeDuration = ( + startDate: string | undefined, + endDate: string | undefined, + ): number => { if (!startDate && !endDate) return 0; if (startDate && endDate) { + const start = dayjs(startDate); + const end = dayjs(endDate); + if (!start.isValid() || !end.isValid()) return -1; return Math.abs(dayjs(endDate).diff(dayjs(startDate), "days")); } return -1; // Invalid range when only one date is set };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Utils/removeDuplicates.ts(1 hunks)src/components/Facility/DischargedPatientsList.tsx(5 hunks)
🔇 Additional comments (4)
src/Utils/removeDuplicates.ts (3)
27-32: LGTM! Well-structured duplicate identification logic
The code effectively identifies duplicates using Map entries with proper type safety and clear transformations.
18-25:
Replace map with forEach and improve type safety
The current implementation has several issues:
- Uses
mapbut discards the return value - Uses
anytype which reduces type safety - Lacks validation for CSV structure
- lines.map((line: any, i: number) => {
+ lines.forEach((line: string, i: number) => {
+ const columns = line.split(",");
+ if (columns.length <= CSV_COLUMNS.PATIENT_ID) {
+ throw new Error(`Invalid CSV structure at line ${i + 1}: missing patient ID`);
+ }
- const patientId = line.split(",")[0];
+ const patientId = columns[CSV_COLUMNS.PATIENT_ID];
+ if (!patientId.trim()) {
+ throw new Error(`Invalid CSV structure at line ${i + 1}: empty patient ID`);
+ }
if (idsMap.has(patientId)) {
idsMap.get(patientId).push(i);
} else {
idsMap.set(patientId, [i]);
}
});Likely invalid or redundant comment.
36-53: 🛠️ Refactor suggestion
Improve policy ID merging robustness
The policy ID merging logic needs better error handling and consistency:
linesWithSameId.forEach((lineInfo) => {
const indexes = lineInfo.indexSame;
if (!indexes.length) return;
- //get policyid of all the duplicate patients and merge them by seperating them with a semicolon
+ // Get policy ID of all duplicate patients and merge them with semicolons
const mergedPolicyId = indexes
.map((currentIndex: number) => {
const columns = lines[currentIndex].split(",");
+ if (columns.length <= CSV_COLUMNS.POLICY_ID) {
+ throw new Error(`Invalid CSV structure at line ${currentIndex + 1}: missing policy ID column`);
+ }
const policyId = columns[CSV_COLUMNS.POLICY_ID];
return policyId?.trim() || "";
})
.filter(Boolean)
.join(";");
+
+ if (!mergedPolicyId) {
+ console.warn(`No valid policy IDs found for patient ${lineInfo.patientId}`);
+ }
+
// replace the policy ID of the first patient with the merged policy ID
const arrayOfCurrentLine = lines[indexes[0]].split(",");
- arrayOfCurrentLine[5] = mergedPolicyId;
+ arrayOfCurrentLine[CSV_COLUMNS.POLICY_ID] = mergedPolicyId;
const lineAfterMerge = arrayOfCurrentLine.join(",");
lines[indexes[0]] = `${lineAfterMerge}`;
});Likely invalid or redundant comment.
src/components/Facility/DischargedPatientsList.tsx (1)
394-447: LGTM! Export implementation looks solid
The implementation provides good user experience with:
- Clear visual feedback
- Proper error handling
- User guidance through tooltips
Let's verify the export functionality implementation:
✅ Verification successful
Let me gather more information about the ExportMenu component and its usage.
Let me check the ExportMenu component implementation and its usage pattern.
Let me check the implementation of the duplicate prevention utility to complete the verification.
Export implementation is correctly implemented with proper duplicate handling
The verification confirms that:
- ExportMenu component is properly implemented with consistent usage across the codebase
- The
preventDuplicatePatientsDuetoPolicyIdutility correctly handles duplicate patients by:- Merging policy IDs of duplicate patients with semicolon separation
- Keeping only unique patient entries in the exported CSV
- Error messages for the 7-day period requirement are consistent across components
- Export functionality follows the same pattern in both ManagePatients and DischargedPatientsList
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the export implementation
# Test 1: Verify the ExportMenu component usage
ast-grep --pattern 'ExportMenu disabled={$_} exportItems={$_}'
# Test 2: Check for consistent error messages
rg -A 1 "Please select a seven day period"
Length of output: 670
Script:
#!/bin/bash
# Check ExportMenu component implementation and its usage
ast-grep --pattern 'export const ExportMenu = ({$_}) => {$$$}'
# Check for any other export-related components
rg -l "ExportMenu"
# Check for any other instances of export functionality
rg -A 2 "export.*csv.*true"
Length of output: 317
Script:
#!/bin/bash
# Check the ExportMenu component implementation
cat src/components/Common/Export.tsx
# Check for the preventDuplicatePatientsDuetoPolicyId utility
rg -A 3 "preventDuplicatePatientsDuetoPolicyId"
# Check for consistent export functionality pattern
ast-grep --pattern 'ExportMenu disabled={$_} exportItems={[{label: $_, action: $_}]}'
Length of output: 5996
Script:
#!/bin/bash
# Check the implementation of preventDuplicatePatientsDuetoPolicyId
cat src/Utils/removeDuplicates.ts
# Check for any related test files
fd removeDuplicates.*test
Length of output: 2617
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/Facility/DischargedPatientsList.tsx (3)
83-146: Consider adding TypeScript interfaces for params objectThe params object contains many properties but lacks type definitions. Consider creating an interface to improve type safety and maintainability.
interface DischargedPatientsParams { page: number; limit: number; name?: string; patient_no?: string; phone_number?: string; // ... add other properties }
179-181: Add comments explaining the export validation logicThe export validation logic could benefit from comments explaining why 7 days is the maximum allowed range and what the validation conditions mean.
+ // Export is allowed only when: + // 1. All selected date ranges are between 0 and 7 days + // 2. At least one date range is selected (not all durations are 0) const isExportAllowed = durations.every((x) => x >= 0 && x <= 7) && !durations.every((x) => x === 0);
390-395: Consider using ref instead of setTimeout for scrollingUsing setTimeout for scrolling to an element is not ideal. Consider using a ref for better reliability and performance.
+const bedTypeRef = useRef<HTMLElement>(null); + <ButtonV2 onClick={() => { advancedFilter.setShow(true); - setTimeout(() => { - const element = document.getElementById("bed-type-select"); - if (element) element.scrollIntoView({ behavior: "smooth" }); + // Wait for the filter to be shown + requestAnimationFrame(() => { + bedTypeRef.current?.scrollIntoView({ behavior: "smooth" }); + }); Notification.Warn({ msg: "Please select a seven day period.", }); - }, 500); }} >src/Utils/utils.ts (1)
548-557: Add documentation and improve input validationThe function implementation looks good but could benefit from these improvements:
- Add JSDoc documentation to describe the purpose, parameters, and return values
- Add input validation for date string format
- Consider handling invalid date strings
Here's a suggested implementation:
+/** + * Calculates the duration in days between two dates. + * Used to validate date ranges for CSV export functionality. + * + * @param startDate - Start date in ISO format + * @param endDate - End date in ISO format + * @returns + * - Positive number: Number of days between dates + * - 0: No dates provided + * - -1: Invalid range (only one date provided or invalid date format) + */ export const calculateDateRangeDuration = ( startDate: string, endDate: string, ): number => { if (!startDate && !endDate) return 0; if (startDate && endDate) { + // Validate date formats + const start = dayjs(startDate); + const end = dayjs(endDate); + if (!start.isValid() || !end.isValid()) return -1; + - return Math.abs(dayjs(endDate).diff(dayjs(startDate), "days")); + return Math.abs(end.diff(start, "days")); } return -1; // Invalid range when only one date is set };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Utils/utils.ts(1 hunks)src/components/Facility/DischargedPatientsList.tsx(4 hunks)src/components/Patient/ManagePatients.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/ManagePatients.tsx
🔇 Additional comments (1)
src/Utils/utils.ts (1)
548-557: Verify integration with CSV export requirements
The function supports the 7-day period validation requirement for CSV export. However, let's verify its integration with the date-based advanced filter.
✅ Verification successful
Function is correctly integrated with CSV export requirements
The calculateDateRangeDuration function is properly integrated with the CSV export functionality in the DischargedPatientsList component:
- It validates multiple date ranges from advanced filters
- Enforces the 7-day period requirement through
durations.every((x) => x >= 0 && x <= 7) - Controls CSV export availability through
isExportAllowed - Provides appropriate user feedback with a tooltip when date range is invalid
- Integrates with the ExportMenu component for CSV generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the calculateDateRangeDuration function is used in the codebase
# Expected: Usage in DischargedPatientsList component for CSV export validation
# Search for function usage
echo "Searching for function usage..."
rg "calculateDateRangeDuration" --type ts --type tsx -A 5
# Search for CSV export related code
echo "Searching for CSV export related code..."
rg "ExportMenu|CSV" --type ts --type tsx -A 5
Length of output: 325
Script:
#!/bin/bash
# Let's try with correct file type flags and more specific search patterns
# Search for function usage
echo "Searching for function usage..."
rg "calculateDateRangeDuration" -t typescript -t javascriptreact -t typescriptreact -A 5
# Search for CSV export and date filter related code
echo "Searching for CSV export and date filter related code..."
rg "export.*csv|ExportMenu|dateRange" -t typescript -t javascriptreact -t typescriptreact -A 5
# Search for discharged patients list component
echo "Searching for discharged patients list component..."
rg "DischargedPatients" -t typescript -t javascriptreact -t typescriptreact -A 5
Length of output: 680
Script:
#!/bin/bash
# Let's try simpler approach without file type flags
# Search for function usage
echo "Searching for function usage..."
rg "calculateDateRangeDuration"
# Search for CSV export and date filter related code
echo "Searching for CSV export and date filter related code..."
rg -l "export.*csv|ExportMenu|dateRange"
# Search for discharged patients list component
echo "Searching for discharged patients list component..."
fd -e ts -e tsx . | xargs rg "DischargedPatients"
Length of output: 1711
Script:
#!/bin/bash
# Let's examine the DischargedPatientsList component to understand the integration
# Check the implementation details of DischargedPatientsList
echo "Checking DischargedPatientsList implementation..."
rg -A 20 "calculateDateRangeDuration" src/components/Facility/DischargedPatientsList.tsx
# Check for date range validation logic
echo "Checking date range validation..."
rg "startDate|endDate" src/components/Facility/DischargedPatientsList.tsx
Length of output: 1448
Script:
#!/bin/bash
# Let's check the date range fields and export implementation
# Check where date_range_fields is defined
echo "Checking date_range_fields definition..."
rg "date_range_fields" src/components/Facility/DischargedPatientsList.tsx -B 5 -A 5
# Check export implementation
echo "Checking export implementation..."
rg "isExportAllowed|ExportMenu" src/components/Facility/DischargedPatientsList.tsx -B 5 -A 10
Length of output: 3759
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@AnveshNalimela Can you please share the current status of the PR? Otherwise, the PR will be closed in the next 48 hours, and you will be unassigned from the issue due to inactivity. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
66-82: Consider memoizing the filter hook results.The
useFiltershook returns multiple values that could trigger unnecessary re-renders.- const { - qParams, - updateQuery, - advancedFilter, - FilterBadges, - resultsPerPage, - updatePage, - } = useFilters({ + const filterResults = useMemo(() => useFilters({ limit: 12, cacheBlacklist: [ "name", "patient_no", "phone_number", "emergency_phone_number", ], - }); + }), []); + const { + qParams, + updateQuery, + advancedFilter, + FilterBadges, + resultsPerPage, + updatePage, + } = filterResults;
175-181: Improve date range validation logic.The current implementation might allow edge cases where the duration is exactly 7 days. Consider using inclusive bounds.
- const isExportAllowed = - durations.every((x) => x >= 0 && x <= 7) && - !durations.every((x) => x === 0); + const isExportAllowed = + durations.every((x) => x >= 0 && x < 8) && + durations.some((x) => x > 0);
388-399: Improve UX for date range selection.The current implementation uses a timeout to scroll to the date range selector, which might be unreliable.
- setTimeout(() => { - const element = - document.getElementById("bed-type-select"); - if (element) - element.scrollIntoView({ behavior: "smooth" }); - Notification.Warn({ - msg: "Please select a seven day period.", - }); - }, 500); + // Use a ref instead of getElementById + const dateRangeRef = useRef<HTMLDivElement>(null); + + dateRangeRef.current?.scrollIntoView({ behavior: "smooth" }); + Notification.Warn({ + msg: "Please select a seven day period.", + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Facility/DischargedPatientsList.tsx(4 hunks)src/components/Patient/ManagePatients.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/ManagePatients.tsx
🔇 Additional comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
41-54: LGTM: New imports enhance the component's capabilities.
The added imports provide necessary functionality for notifications, CSV export handling, and utility functions.
385-438:
Add error handling for the export action.
The export functionality should handle potential failures gracefully.
action: async () => {
+ try {
const query = {
...params,
csv: true,
};
const pathParams = { facility_external_id };
const { data } = await request(
routes.listFacilityDischargedPatients,
{
query,
pathParams,
},
);
return data ?? null;
+ } catch (error) {
+ Notification.Error({
+ msg: "Failed to export discharged patients data. Please try again.",
+ });
+ return null;
+ }
},427-427: Verify the CSV parsing implementation.
Ensure that the preventDuplicatePatientsDuetoPolicyId function handles all edge cases for the CSV data.
✅ Verification successful
CSV parsing implementation has robust edge case handling
The implementation of preventDuplicatePatientsDuetoPolicyId function includes comprehensive handling of edge cases:
- Input validation for non-empty strings
- Handles missing or empty policy IDs by filtering them out
- Properly manages CSV parsing with defined column indices
- Merges duplicate patient records by combining policy IDs with semicolon separator
- Maintains data integrity by keeping the first occurrence of duplicate patients
- Handles empty lines through the filtering process
However, it's recommended to add unit tests to verify these edge cases systematically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of preventDuplicatePatientsDuetoPolicyId
# Search for test cases and implementation
ast-grep --pattern 'function preventDuplicatePatientsDuetoPolicyId($_) {
$$$
}'
# Search for related test files
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . | grep -i "duplicate"
Length of output: 156
Script:
#!/bin/bash
# Let's try a different approach to find the implementation and test files
# Search for the function name in all files
rg "preventDuplicatePatientsDuetoPolicyId" -A 10
# Search for any files containing "duplicate" and "patient" to find related tests
rg -l "duplicate.*patient|patient.*duplicate" -g "*.{test,spec}.{ts,tsx}"
Length of output: 4830
Script:
#!/bin/bash
# Let's examine the full implementation of the function and any related test files
# Get the complete implementation from removeDuplicates.ts
cat src/Utils/removeDuplicates.ts
# Try a different approach to find test files
fd "removeDuplicates" -e test.ts -e test.tsx -e spec.ts -e spec.tsx
Length of output: 2655
|
@nihal467 Due to ongoing exams there is inactivity in PR . I made all the requested changes and few are left to be made. I will make them and push by 12-12-2024 |
github-actions
bot
added
merge conflict
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/Utils/removeDuplicates.ts (1)
54-62: Document CSV structure and use constants for column indicesThe function uses magic numbers for column indices without documenting the expected CSV structure.
Apply this diff to improve maintainability:
+// CSV column indices for patient data +export const CSV_COLUMNS = { + PATIENT_ID: 0, + POLICY_ID: 5, +} as const; + +/** + * Prevents duplicate patient entries by merging policy IDs for the same patient. + * Expected CSV structure: + * Column 0: Patient ID + * Column 5: Policy ID + * + * @param data CSV string containing patient data + * @returns CSV string with merged policy IDs for duplicate patients + */ export const preventDuplicatePatientsDuetoPolicyId = (data: string): string => { const result = groupAndMergeByColumn( data, - 0, // Group by Patient ID - [5], // Merge policy IDs + CSV_COLUMNS.PATIENT_ID, + [CSV_COLUMNS.POLICY_ID], ";", // Use ";" as the delimiter ); return result; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Utils/removeDuplicates.ts(1 hunks)src/Utils/utils.ts(1 hunks)src/components/Patient/ManagePatients.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Utils/utils.ts
- src/components/Patient/ManagePatients.tsx
🔇 Additional comments (1)
src/Utils/removeDuplicates.ts (1)
1-62: Add comprehensive test coverage
The utility functions lack test coverage, which is critical for data transformation logic.
Would you like me to help create a test suite in src/Utils/__tests__/removeDuplicates.test.ts that covers:
- Input validation
- CSV parsing edge cases
- Grouping and merging scenarios
- Error handling
- Performance with large datasets
github-actions
bot
added
merge conflict
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
41-54: LGTM! Consider grouping related importsThe new imports are appropriate for the added functionality. Consider grouping related imports together (e.g., all utility functions).
-import * as Notification from "@/Utils/Notifications"; -import { preventDuplicatePatientsDuetoPolicyId } from "@/Utils/removeDuplicates"; -import routes from "@/Utils/request/api"; -import request from "@/Utils/request/request"; -import useTanStackQueryInstead from "@/Utils/request/useQuery"; -import { - calculateDateRangeDuration, - formatPatientAge, - humanizeStrings, - parsePhoneNumber, -} from "@/Utils/utils"; +// Request related imports +import routes from "@/Utils/request/api"; +import request from "@/Utils/request/request"; +import useTanStackQueryInstead from "@/Utils/request/useQuery"; + +// Utility functions +import * as Notification from "@/Utils/Notifications"; +import { preventDuplicatePatientsDuetoPolicyId } from "@/Utils/removeDuplicates"; +import { + calculateDateRangeDuration, + formatPatientAge, + humanizeStrings, + parsePhoneNumber, +} from "@/Utils/utils";
160-182: Extract date range validation logic into a separate functionThe date range validation logic is crucial for export functionality. Consider extracting it into a separate utility function for better maintainability and reusability.
+const isValidDateRange = (startDate: string | undefined, endDate: string | undefined): boolean => { + const duration = calculateDateRangeDuration(startDate, endDate); + return duration >= 0 && duration <= 7; +}; + +const hasAtLeastOneDateRange = (durations: number[]): boolean => { + return !durations.every((x) => x === 0); +}; + const date_range_fields = [ [params.created_date_before, params.created_date_after], [params.modified_date_before, params.modified_date_after], [params.date_declared_positive_before, params.date_declared_positive_after], [params.last_vaccinated_date_before, params.last_vaccinated_date_after], [params.last_consultation_encounter_date_before, params.last_consultation_encounter_date_after], [params.last_consultation_discharge_date_before, params.last_consultation_discharge_date_after], ]; const durations = date_range_fields.map(([startDate, endDate]) => calculateDateRangeDuration(startDate, endDate), ); -const isExportAllowed = - durations.every((x) => x >= 0 && x <= 7) && - !durations.every((x) => x === 0); +const isExportAllowed = durations.every(isValidDateRange) && hasAtLeastOneDateRange(durations);
385-404: Improve export button implementationThe current implementation has several areas for improvement:
- Using setTimeout with a hard-coded delay is fragile
- Error messages should be managed in I18n files
- Element ID reference could be managed as a constant
+const SCROLL_DELAY_MS = 500; +const BED_TYPE_SELECT_ID = 'bed-type-select'; + <div className="tooltip w-full md:w-auto" id="patient-export"> {!isExportAllowed ? ( <ButtonV2 onClick={() => { advancedFilter.setShow(true); setTimeout(() => { - const element = document.getElementById("bed-type-select"); + const element = document.getElementById(BED_TYPE_SELECT_ID); if (element) element.scrollIntoView({ behavior: "smooth" }); Notification.Warn({ - msg: "Please select a seven day period.", + msg: t("select_seven_day_period"), }); - }, 500); + }, SCROLL_DELAY_MS); }} className="mr-5 w-full lg:w-fit" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(2 hunks)src/components/Facility/DischargedPatientsList.tsx(4 hunks)src/components/Patient/ManagePatients.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/ManagePatients.tsx
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
411-425:
Add error handling for export action
The export action should include error handling to provide feedback to users when the export fails.
action: async () => {
+ try {
const query = {
...params,
csv: true,
};
const pathParams = { facility_external_id };
const { data } = await request(
routes.listFacilityDischargedPatients,
{
query,
pathParams,
},
);
return data ?? null;
+ } catch (error) {
+ Notification.Error({
+ msg: "Failed to export discharged patients data. Please try again.",
+ });
+ return null;
+ }
},Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (2)
216-234: Simplify date range validation conditionThe condition for checking valid date ranges has redundant checks. The
days !== 0check is unnecessary sincedays > 0already implies this.Apply this diff to simplify the condition:
const isExportAllowed = dateRangeFields.some(([startDate, endDate]) => { const days = calculateDaysBetween(startDate, endDate); - return days > 0 && days <= 7 && days !== 0; + return days > 0 && days <= 7; });
454-456: Enhance tooltip message for better user guidanceThe current tooltip message could be more informative by explicitly stating the date range requirement.
Consider updating the translation key to provide more specific guidance:
- {t("select_seven_day_period")} + {t("select_seven_day_period_for_export")}Add a new translation entry in en.json:
"select_seven_day_period_for_export": "Please select a date range of 7 days or less to enable export."public/locale/en.json (1)
733-734: Add missing translations for export functionalityWhile the basic translations are added, consider adding translations for all export-related messages to support a complete internationalization.
Add these additional translations to support the export feature fully:
{ "export": "Export", "select_seven_day_period": "Please select a seven day period.", + "exporting_data": "Exporting data...", + "export_success": "Data exported successfully", + "export_failed": "Failed to export data. Please try again.", + "select_seven_day_period_for_export": "Please select a date range of 7 days or less to enable export." }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(2 hunks)src/components/Facility/DischargedPatientsList.tsx(6 hunks)src/components/Patient/ManagePatients.tsx(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/ManagePatients.tsx
🔇 Additional comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
12-14: LGTM! Well-organized imports
The new imports are properly organized and consistently use absolute paths.
Also applies to: 42-52
421-435: 🛠️ Refactor suggestion
Add error handling and loading state to export action
The export action should handle potential API errors and provide feedback to users during the export process.
Apply this diff to enhance error handling:
action: async () => {
+ try {
const query = {
...params,
csv: true,
};
const pathParams = { facility_external_id };
+ Notification.Info({ msg: t("exporting_data") });
const { data } = await request(
routes.listFacilityDischargedPatients,
{
query,
pathParams,
},
);
+ Notification.Success({ msg: t("export_success") });
return data ?? null;
+ } catch (error) {
+ Notification.Error({ msg: t("export_failed") });
+ return null;
+ }
},Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/components/Patient/ManagePatients.tsx (3)
229-231: Simplify date range validation condition.The condition
days !== 0is redundant sincedays > 0already ensures the same check.- return days > 0 && days <= 7 && days !== 0; + return days > 0 && days <= 7;
240-249: Remove console.log statement.The implementation correctly uses useQuery as suggested. However, there's a debug console.log that should be removed.
- console.log(result);
Line range hint
877-896: Define timeout value as a constant.The implementation has good UX, but the hardcoded timeout value should be defined as a constant at the top of the file for better maintainability.
+const SCROLL_TIMEOUT_MS = 500; + // ... later in the code ... - }, 500); + }, SCROLL_TIMEOUT_MS);src/components/Facility/DischargedPatientsList.tsx (3)
Line range hint
1-53: LGTM! Consider grouping imports by typeThe imports are well-organized but could be further improved by grouping them into these categories:
- External dependencies
- UI components
- Common components
- Models
- Hooks
- Utils
140-203: Add TypeScript interface for query parametersThe params object contains many fields and would benefit from type safety. Consider creating a dedicated interface:
interface DischargedPatientsQueryParams { page: number; limit: number; name?: string; patient_no?: string; phone_number?: string; // ... add other fields }
232-235: Simplify the date range validation conditionThe condition
days > 0 && days <= 7 && days !== 0has redundant checks. Ifdays > 0is true, thendays !== 0is always true.- return days > 0 && days <= 7 && days !== 0; + return days > 0 && days <= 7;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/DischargedPatientsList.tsx(7 hunks)src/components/Patient/ManagePatients.tsx(10 hunks)
🔇 Additional comments (1)
src/components/Patient/ManagePatients.tsx (1)
234-237: LGTM! Query parameters are well-structured.
The export query setup correctly includes the CSV flag and facility ID.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Patient/ManagePatients.tsx (1)
Line range hint
868-908: Extract common export menu logic into a reusable component.The ExportMenu implementation is duplicated between components and could be standardized.
// src/components/Common/PatientExportMenu.tsx interface PatientExportMenuProps { isExportAllowed: boolean; exportAction: () => Promise<any>; label: string; } const PatientExportMenu = ({ isExportAllowed, exportAction, label }: PatientExportMenuProps) => { return ( <ExportMenu disabled={!isExportAllowed} exportItems={[ { label, action: exportAction, parse: (data) => { try { return csvGroupByColumn(data, "Patient ID"); } catch (e) { if (e instanceof Error) { Notification.Error({ msg: e.message }); } return ""; } }, }, ]} /> ); };
🧹 Nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (2)
140-203: Consider adding TypeScript interface and grouping related parameters.While the params object is well-structured, it could benefit from:
- A TypeScript interface to define the shape of the params
- Grouping related parameters (e.g., all date-related fields, all patient-related fields)
+interface DischargedPatientsParams { + // Pagination + page: number; + limit: number; + offset: number; + + // Patient Info + name?: string; + patient_no?: string; + phone_number?: string; + emergency_phone_number?: string; + + // Dates + created_date_before?: string; + created_date_after?: string; + // ... other date fields +} + const params = { // Pagination page: qParams.page || 1, limit: resultsPerPage, offset: (qParams.page ? qParams.page - 1 : 0) * resultsPerPage, // Patient Info name: qParams.name || undefined, // ... rest of the params grouped by category } as DischargedPatientsParams;
232-235: Simplify date range validation logic.The current condition has redundant checks.
const isExportAllowed = dateRangeFields.some(([startDate, endDate]) => { const days = calculateDaysBetween(startDate, endDate); - return days > 0 && days <= 7 && days !== 0; + return days > 0 && days <= 7; });src/components/Patient/ManagePatients.tsx (1)
Line range hint
763-805: Create a standardized button component with icon support.Multiple buttons share similar structure and styling. Consider creating a reusable component.
// src/components/Common/IconButton.tsx interface IconButtonProps { icon: string; label: string; onClick: () => void; variant?: "primary" | "secondary"; className?: string; } const IconButton = ({ icon, label, onClick, variant = "primary", className = "" }: IconButtonProps) => ( <Button variant={variant} onClick={onClick} className={`w-full lg:w-fit ${className}`} > <CareIcon icon={icon} className="mr-2 text-lg" /> <span className="lg:my-[3px]">{label}</span> </Button> );Also applies to: 840-855
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/DischargedPatientsList.tsx(7 hunks)src/components/Patient/ManagePatients.tsx(10 hunks)
🔇 Additional comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
Line range hint 1-53: LGTM! Well-organized imports.
The imports are properly organized and all appear to be necessary for the component's functionality.
232-244: 🛠️ Refactor suggestion
Add error handling and loading state to export functionality.
The current implementation lacks proper error handling and loading state management.
- const { refetch: exportAsCSV } = useQuery({
+ const { refetch: exportAsCSV, isLoading: isExporting } = useQuery({
queryKey: ["discharged-patients-csv"],
queryFn: query(routes.listFacilityDischargedPatients, {
queryParams: { ...params, csv: true },
pathParams: { facility_external_id },
}),
enabled: false,
+ onError: (error) => {
+ Notification.Error({
+ msg: error instanceof Error
+ ? error.message
+ : "Failed to export patients data",
+ });
+ },
});Likely invalid or redundant comment.
403-457: 🛠️ Refactor suggestion
Improve scroll behavior, error handling, and accessibility.
Several areas need improvement:
- Replace setTimeout with requestAnimationFrame
- Enhance error handling in parse function
- Add proper ARIA attributes to tooltip
<div
- className="tooltip w-full md:w-auto"
+ className="tooltip w-full md:w-auto"
+ role="tooltip"
+ aria-label={!isExportAllowed ? t("select_seven_day_period") : undefined}
id="patient-export">
{!isExportAllowed ? (
<Button
variant="primary"
size="lg"
onClick={() => {
advancedFilter.setShow(true);
- setTimeout(() => {
+ requestAnimationFrame(() => {
const element = document.getElementById("bed-type-select");
- if (element)
- element.scrollIntoView({ behavior: "smooth" });
+ element?.scrollIntoView({ behavior: "smooth" });
Notification.Warn({
msg: t("select_seven_day_period"),
});
- }, 500);
});
}}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Patient/ManagePatients.tsx (1)
Line range hint
865-914: Provide user more explicit guidance when the export is disabled.
Disabling the export with a tooltip is good. Consider automatically scrolling to or highlighting the date range fields when the user clicks the button to further guide them.-onClick={() => { advancedFilter.setShow(true); ... }} +onClick={() => { advancedFilter.setShow(true); // Smooth scroll or highlight date range fields // ... }}
🧹 Nitpick comments (7)
src/hooks/usePatientExport.ts (1)
6-19: Use a descriptive query key for clarity.
The query key["csv"]is conceptually correct but might be more descriptive by including the route name, e.g.,[route, "csv"], to avoid collisions and clarify the purpose of the query key. Additionally, consider adding anonErrorcallback to handle potential network errors or unexpected exceptions gracefully.src/components/Patient/ManagePatients.tsx (3)
Line range hint
213-230: Ensure consistent checks for valid date ranges.
Currently, you check ifdays > 0 && days <= 7 && days !== 0. The conditiondays !== 0is redundant since you already checkdays > 0. Consider simplifying to(days > 0 && days <= 7)for cleaner logic.- return days > 0 && days <= 7 && days !== 0; + return days > 0 && days <= 7;
233-237: Improve naming or referencing of route in usePatientExport.
Usingroutes.patientListis correct, but if you plan to extend the usage ofusePatientExportwith different routes, you may want to renameexportAsCSVor pass in a more descriptive route reference so that the exported function name is consistent with its target resource.
Line range hint
760-802: Streamline conditional logic for adding patients.
You have a multi-layered if-else block to navigate users. It might be more readable to extract major transitions into separate helper functions or to refactor repeated patterns, simplifying maintenance and preventing conditional sprawl.src/components/Facility/DischargedPatientsList.tsx (3)
139-202: Consider grouping related parameters and adding TypeScript interfacesThe params object contains many fields that could be organized better for maintainability.
Consider grouping related parameters and adding type definitions:
interface DateRangeParams { created_date_before?: string; created_date_after?: string; // ... other date-related fields } interface PatientFilterParams { gender?: string; category?: string; age_min?: string; age_max?: string; // ... other patient-related fields } interface PaginationParams { page: number; limit: number; offset: number; ordering?: string; } interface SearchParams { name?: string; patient_no?: string; phone_number?: string; emergency_phone_number?: string; } interface DischargedPatientsParams extends DateRangeParams, PatientFilterParams, PaginationParams, SearchParams { // ... any additional fields }
231-234: Simplify the date range validation conditionThe condition
days > 0 && days <= 7 && days !== 0contains redundant checks sincedays > 0already impliesdays !== 0.const isExportAllowed = dateRangeFields.some(([startDate, endDate]) => { const days = calculateDaysBetween(startDate, endDate); - return days > 0 && days <= 7 && days !== 0; + return days > 0 && days <= 7; });
405-415: Replace setTimeout with requestAnimationFrame for smoother scrollingUsing setTimeout for DOM operations is less reliable than requestAnimationFrame.
- setTimeout(() => { - const element = document.getElementById("bed-type-select"); - if (element) element.scrollIntoView({ behavior: "smooth" }); - Notification.Warn({ - msg: t("select_seven_day_period"), - }); - }, 500); + requestAnimationFrame(() => { + const element = document.getElementById("bed-type-select"); + if (element) { + element.scrollIntoView({ behavior: "smooth" }); + Notification.Warn({ + msg: t("select_seven_day_period"), + }); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Facility/DischargedPatientsList.tsx(7 hunks)src/components/Patient/ManagePatients.tsx(10 hunks)src/hooks/usePatientExport.ts(1 hunks)
🔇 Additional comments (3)
src/components/Patient/ManagePatients.tsx (1)
837-838: Confirm real-world usage of Doctor Connect.
Ensure that the location where this button appears is consistent with the user’s workflow and that the Doctor Connect feature is integrated (or intentionally absent) in other relevant contexts.
src/components/Facility/DischargedPatientsList.tsx (2)
Line range hint 12-52: LGTM! Well-organized imports using absolute paths.
The imports are properly structured and use absolute paths consistently.
399-453: 🛠️ Refactor suggestion
Improve tooltip accessibility
The tooltip implementation lacks proper ARIA attributes for accessibility.
- <div className="tooltip w-full md:w-auto" id="patient-export">
+ <div
+ className="tooltip w-full md:w-auto"
+ id="patient-export"
+ role="tooltip"
+ aria-label={!isExportAllowed ? t("select_seven_day_period") : undefined}
+ >
{/* ... */}
- <span className="tooltip-text tooltip-bottom -translate-x-1/2">
+ <span
+ className="tooltip-text tooltip-bottom -translate-x-1/2"
+ role="tooltip"
+ aria-hidden={isExportAllowed}
+ >
{t("select_seven_day_period")}
</span>Likely invalid or redundant comment.
…e-patients-export
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/components/Patient/ManagePatients.tsx (2)
Line range hint
213-237: Maintain consistency with DischargedPatientsList exportThe export implementation should be consistent with DischargedPatientsList:
- Error handling patterns
- User feedback
- Date validation
Consider extracting the common export logic into a shared utility:
// src/utils/exportUtils.ts export const validateDateRange = (dateRangeFields: [string | undefined, string | undefined][]) => { return dateRangeFields.some(([startDate, endDate]) => { if (!startDate || !endDate) return false; if (dayjs(startDate).isAfter(dayjs())) return false; if (dayjs(endDate).isAfter(dayjs())) return false; const days = calculateDaysBetween(startDate, endDate); return days > 0 && days <= 7; }); };
Line range hint
865-884: Extract export button into a reusable componentThe export button implementation is duplicated between ManagePatients and DischargedPatientsList.
Consider creating a shared component:
// src/components/Common/ExportButton.tsx interface ExportButtonProps { isExportAllowed: boolean; onExportClick: () => void; className?: string; } export const ExportButton: React.FC<ExportButtonProps> = ({ isExportAllowed, onExportClick, className }) => { return ( <Button variant="primary" size="lg" onClick={onExportClick} className={className} > <CareIcon icon="l-export" className="mr-2" /> <span className="lg:my-[3px]">{t("export")}</span> </Button> ); };
🧹 Nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
139-202: Consider improving params object organizationThe params object is quite large and could benefit from:
- TypeScript interface definition
- Grouping related fields (e.g., dates, patient info, consultation info)
- Extracting phone number parsing logic into a separate function
+ interface PatientListParams { + pagination: { + page: number; + limit: number; + offset: number; + }; + patient: { + name?: string; + patient_no?: string; + phone_number?: string; + emergency_phone_number?: string; + }; + // ... other field groups + } + const parsePhoneNumbers = (params: Pick<typeof qParams, "phone_number" | "emergency_phone_number">) => ({ + phone_number: params.phone_number ? parsePhoneNumber(params.phone_number) : undefined, + emergency_phone_number: params.emergency_phone_number + ? parsePhoneNumber(params.emergency_phone_number) + : undefined, + }); const params = { + ...parsePhoneNumbers(qParams), // rest of the params };
216-234: Enhance date range validation robustnessThe current implementation correctly validates 7-day periods but could be more robust:
- Consider edge cases where dates might be invalid
- Add validation for future dates
- Consider timezone implications
const isExportAllowed = dateRangeFields.some(([startDate, endDate]) => { + if (!startDate || !endDate) return false; + if (dayjs(startDate).isAfter(dayjs())) return false; + if (dayjs(endDate).isAfter(dayjs())) return false; const days = calculateDaysBetween(startDate, endDate); return days > 0 && days <= 7; });
399-421: Improve scroll behavior and accessibilityThe current implementation has several areas for improvement:
- Using setTimeout for scrolling is unreliable
- Missing ARIA attributes for accessibility
- Tooltip implementation could be more accessible
- setTimeout(() => { - const element = document.getElementById("bed-type-select"); - if (element) element.scrollIntoView({ behavior: "smooth" }); - }, 500); + requestAnimationFrame(() => { + const element = document.getElementById("bed-type-select"); + element?.scrollIntoView({ behavior: "smooth" }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/DischargedPatientsList.tsx(7 hunks)src/components/Patient/ManagePatients.tsx(10 hunks)
🔇 Additional comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
12-14: LGTM: Button component migration
The switch to shadcn's Button component aligns with the ongoing modernization efforts.
422-443: 🛠️ Refactor suggestion
Enhance export error handling and user feedback
The export functionality should provide better feedback to users and handle errors more gracefully.
action: async () => {
+ try {
+ Notification.Info({ msg: t("export_in_progress") });
const result = await exportDischargedAsCSV();
+ if (!result.data) {
+ throw new Error(t("export_failed_no_data"));
+ }
+ Notification.Success({ msg: t("export_completed") });
return (result.data as string) || null;
+ } catch (error) {
+ Notification.Error({
+ msg: error instanceof Error
+ ? error.message
+ : t("export_failed")
+ });
+ return null;
+ }
},Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
221-245: Consider renaming dateRangeFields for clarity.The variable name could be more descriptive to indicate its purpose in export validation.
- const dateRangeFields = [ + const exportDateRangeValidations = [
410-431: Improve scroll behavior implementation.Using setTimeout for scroll behavior is unreliable and could fail if the element isn't loaded. Consider using IntersectionObserver or requestAnimationFrame for more reliable behavior.
- setTimeout(() => { - const element = document.getElementById("bed-type-select"); - if (element) - element.scrollIntoView({ behavior: "smooth" }); + requestAnimationFrame(() => { + const element = document.getElementById("bed-type-select"); + element?.scrollIntoView({ behavior: "smooth" }); Notification.Warn({ msg: t("select_seven_day_period"), }); - }, 500); + });
459-464: Improve tooltip accessibility.The tooltip lacks proper ARIA attributes for accessibility.
- <span className="tooltip-text tooltip-bottom -translate-x-1/2"> + <span + className="tooltip-text tooltip-bottom -translate-x-1/2" + role="tooltip" + aria-label={t("select_seven_day_period")} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/DischargedPatientsList.tsx(7 hunks)src/components/Patient/ManagePatients.tsx(11 hunks)
🔇 Additional comments (5)
src/components/Facility/DischargedPatientsList.tsx (2)
Line range hint 1-57: LGTM! Well-organized imports and modern React practices.
The migration to @tanstack/react-query and the organization of imports by category (UI components, utilities, etc.) improves code maintainability.
438-454: 🛠️ Refactor suggestion
Enhance error handling in export functionality.
The export action should include proper error handling and user feedback.
action: async () => {
+ try {
+ Notification.Info({ msg: t("export_in_progress") });
const result = await exportDischargedAsCSV();
+ if (!result.data) {
+ throw new Error(t("export_no_data"));
+ }
+ Notification.Success({ msg: t("export_completed") });
return (result.data as string) || null;
+ } catch (error) {
+ Notification.Error({
+ msg: error instanceof Error
+ ? error.message
+ : t("export_failed")
+ });
+ return null;
+ }
},Likely invalid or redundant comment.
src/components/Patient/ManagePatients.tsx (3)
Line range hint 214-237: LGTM! Consistent implementation with DischargedPatientsList.
The export functionality follows the same pattern as DischargedPatientsList, promoting code consistency and maintainability.
Line range hint 873-892: Apply the same scroll behavior improvement as DischargedPatientsList.
The setTimeout usage for scroll behavior should be replaced with requestAnimationFrame for consistency and reliability.
900-913: 🛠️ Refactor suggestion
Align error handling with DischargedPatientsList.
For consistency, the error handling should match the enhanced version implemented in DischargedPatientsList.
parse: (data) => {
try {
+ if (!data) {
+ throw new Error(t("export_empty_data"));
+ }
return csvGroupByColumn(data, "Patient ID");
} catch (e) {
if (e instanceof Error) {
Notification.Error({
- msg: e.message,
+ msg: t("export_parse_error", { error: e.message }),
});
}
+ Notification.Error({
+ msg: t("export_unknown_error"),
+ });
return "";
}
},Likely invalid or redundant comment.
…e-patients-export
…e-patients-export
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
no longer relevant in new architecture |
Proposed Changes
added export button with restriction based on seven-day period selection.
Introduced an ExportMenu component to handle export of live patients when export is allowed.
Implemented tooltip guidance for when export is not allowed, prompting the user to select a valid seven-day period.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes