Enhancements to the nursing care procedures and routines tables #9079
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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: 5
🧹 Outside diff range and nitpick comments (5)
.github/workflows/auto-testing-label.yml (2)
Line range hint
41-45: Enhance the reminder message for better clarity.The reminder comment is functional but could be more informative by including the context of why the label was removed.
await github.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: pr.number, - body: 'Reminder: To add the "needs testing" label, comment "ready for testing" on this PR.' + body: 'The "needs testing" label has been removed because changes were requested. Once you address the requested changes, please comment "ready for testing" to add the label back.' });
Line range hint
45-52: Consider extracting label removal logic for reusability.The label removal logic is correct but could be made more maintainable by extracting it into a reusable function, as this pattern might be needed elsewhere in the workflow.
+ async function removeLabel(owner, repo, issueNumber, labelName) { + const labels = await github.issues.listLabelsOnIssue({ + owner, + repo, + issue_number: issueNumber + }); + + if (labels.data.some(label => label.name === labelName)) { + await github.issues.removeLabel({ + owner, + repo, + issue_number: issueNumber, + name: labelName + }); + } + } + if (isChangesRequired) { await github.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: pr.number, - body: 'Reminder: To add the "needs testing" label, comment "ready for testing" on this PR.' + body: 'The "needs testing" label has been removed because changes were requested. Once you address the requested changes, please comment "ready for testing" to add the label back.' }); - if (pr.labels.some(label => label.name === 'needs testing')) { - await github.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: pr.number, - name: 'needs testing' - }); - } + await removeLabel(context.repo.owner, context.repo.repo, pr.number, 'needs testing'); }This refactoring:
- Extracts the label removal logic into a reusable function
- Adds proper error handling through the GitHub API
- Makes the code more maintainable and easier to test
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx (1)
20-39: Enhance error handling in getDisplayValue function.The function could benefit from additional safeguards:
- Validate that translation keys exist to prevent missing translations
- Add type guards for the choices object
- Consider logging invalid values for debugging
const getDisplayValue = ( value: string | boolean | null | undefined, field?: string, ): string => { if (value == null) { return " "; } if (typeof value === "boolean") { return t(value ? "yes" : "no"); } if (field && choices[field]) { + // Validate choice exists before attempting translation + const choiceKey = `${field.toUpperCase()}__${choices[field][value]}`; + if (!choices[field][value]) { + console.warn(`Invalid choice value for field ${field}: ${value}`); + return "-"; + } - const choice = - choices[field][value as keyof (typeof choices)[typeof field]]; - return choice ? t(`${field.toUpperCase()}__${choice}`) : "-"; + return t(choiceKey, { defaultValue: "-" }); } if (value && typeof value == "string") return value; return "-"; };src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
105-109: Simplify Conditional Statement Using Optional ChainingConsider simplifying the conditional check by using optional chaining to make the code more concise and readable.
Apply this diff to simplify the condition:
- if (res && res.ok && data) { + if (res?.ok && data) {🧰 Tools
🪛 Biome
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-157: Optimize Empty Fields Check inareFieldsEmptyFunctionYou can optimize the
areFieldsEmptyfunction by using theeverymethod to make the code more concise and expressive.Apply this diff to optimize the function:
- let emptyFieldCount = 0; - for (const field of NURSING_CARE_PROCEDURES) { - if (!filterEmpty(field)) emptyFieldCount++; - } - return emptyFieldCount === NURSING_CARE_PROCEDURES.length; + return NURSING_CARE_PROCEDURES.every((field) => !filterEmpty(field));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/auto-testing-label.yml(1 hunks).github/workflows/cypress.yaml(3 hunks).github/workflows/generate-sbom.yml(0 hunks)src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx(1 hunks)src/components/Facility/Consultations/NursingPlot.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/generate-sbom.yml
- src/components/Facility/Consultations/NursingPlot.tsx
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx (1)
1-95: Implementation successfully meets PR objectives.
The component effectively implements the requirements for enhancing the Nursing tab UI:
- Implements fixed width columns
- Provides a reusable table structure
- Handles various data types appropriately
- Maintains consistent styling
The code is well-structured and maintainable.
.github/workflows/cypress.yaml (3)
4-5: LGTM: Good addition of scheduled runs.
Adding daily scheduled runs at 22:30 UTC is a good practice for catching potential regressions early.
139-139: LGTM: Path correction maintains consistency.
The path correction for video uploads ensures consistent file organization.
20-20: Consider the resource impact of doubling parallel containers.
While increasing containers from 4 to 8 will speed up test execution through increased parallelization, it will also double the GitHub Actions minutes consumption.
Let's analyze the current test execution times to validate this change:
Consider:
- Monitoring the impact on your GitHub Actions quota
- Implementing test splitting strategies to optimize execution
- Setting up job timeouts to prevent long-running tests
✅ Verification successful
Doubling parallel containers appears justified based on execution times
Based on the workflow run history, successful test runs consistently take 8-11 minutes to complete. With 4 containers, this suggests each container runs tests for ~8-11 minutes. Doubling to 8 containers could potentially reduce execution time to 4-6 minutes, which is a reasonable trade-off considering:
- Current execution times are significant enough to benefit from increased parallelization
- The additional GitHub Actions minutes consumption would be justified by the improved developer feedback loop
- Even with doubled resource usage, the total execution time remains relatively modest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check recent workflow runs to analyze execution times and parallel job distribution
# Get the last 10 workflow runs execution times
gh run list --workflow cypress.yaml --limit 10 --json durationInMinutes,conclusion \
| jq -r '.[] | "\(.durationInMinutes) minutes - \(.conclusion)"'
Length of output: 392
Script:
#!/bin/bash
# Let's try with startedAt and updatedAt to calculate duration
gh run list --workflow cypress.yaml --limit 10 --json startedAt,updatedAt,conclusion \
| jq -r '.[] | "Duration: \((((.updatedAt | fromdateiso8601) - (.startedAt | fromdateiso8601))/60 | floor)) minutes - \(.conclusion)"'
Length of output: 510
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (1)
190-190: Conditional Rendering: Ensure Correct Logical Operators
Ensure that the logical operators used in the conditional rendering are correct. The condition totalCount > PAGINATION_LIMIT && !areFieldsEmpty() might prevent the pagination from rendering when there is more data to display.
Please verify if the condition accurately reflects the intended behavior. If areFieldsEmpty() returns false when there is data (fields are not empty), the ! operator may cause issues.
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Outdated
Show resolved
Hide resolved
| import { ConsultationTabProps } from "@/components/Facility/ConsultationDetails/index"; | ||
| import { NursingPlot } from "@/components/Facility/Consultations/NursingPlot"; | ||
| import LogUpdateAnalayseTable from "@/components/Facility/Consultations/LogUpdateAnalayseTable"; |
There was a problem hiding this comment.
Typographical Error in Component Name "LogUpdateAnalayseTable"
There is a consistent typographical error in the component name LogUpdateAnalayseTable. It should be LogUpdateAnalyseTable. Please correct the spelling in the import statement and all usages to ensure proper functionality.
Apply this diff to fix the typo:
- import LogUpdateAnalayseTable from "@/components/Facility/Consultations/LogUpdateAnalayseTable";
+ import LogUpdateAnalyseTable from "@/components/Facility/Consultations/LogUpdateAnalyseTable";
...
- <LogUpdateAnalayseTable data={mappedData} rows={rows} />
+ <LogUpdateAnalyseTable data={mappedData} rows={rows} />Also applies to: 186-186, 248-252
There was a problem hiding this comment.
@sainak this typo suggestion is valid
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
Outdated
Show resolved
Hide resolved
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
issues/8562/Nursing_tab_ui
|
| Run status |
|
| Run duration | 04m 52s |
| Commit |
|
| Committer | Aakash Singh |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
126
|
| View all changes introduced in this branch ↗︎ | |
sainak
force-pushed
the
issues/8562/Nursing_tab_ui
branch
from
1a76ce1 to
1e87eb8
Compare
Deploying care-fe with
|
| Latest commit: |
1cabe0a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b7314b95.care-fe.pages.dev |
| Branch Preview URL: | https://issues-8562-nursing-tab-ui.care-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
134-143: Simplify data transformation logic.The current data transformation is complex and could be simplified using array methods.
Consider this more readable approach:
- const dataToDisplay = data - .map((x) => - x.nursing - ? x.nursing.map((f: any) => { - f["date"] = x.date; - return f; - }) - : [], - ) - .reduce((accumulator, value) => accumulator.concat(value), []); + const dataToDisplay = data + .flatMap((x) => + x.nursing?.map((f) => ({ ...f, date: x.date })) ?? [] + );
278-279: Enhance heading accessibility.The section headings should have proper heading levels and ARIA labels for better accessibility.
Apply this diff:
- <h4>{t("routine")}</h4> + <h2 className="text-xl font-medium" aria-label={t("routine")}>{t("routine")}</h2> <RoutineSection {...props} /> <div> - <h4>{t("nursing_care")}</h4> + <h2 className="text-xl font-medium" aria-label={t("nursing_care")}>{t("nursing_care")}</h2> <NursingPlot {...props} /> </div>Also applies to: 282-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx(1 hunks)src/components/Facility/Consultations/NursingPlot.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/Facility/Consultations/NursingPlot.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
248-252: LGTM! Table implementation successfully standardized.
The replacement of the custom table with LogUpdateAnalayseTable aligns with the PR objectives for creating reusable components.
9-9: Verify column width requirements in LogUpdateAnalayseTable.
The PR objectives mention adjusting widths for the second and subsequent columns. Let's verify this implementation.
✅ Verification successful
Column width requirements are properly implemented
The LogUpdateAnalayseTable component has implemented appropriate width adjustments:
- First column (header): Auto-width with sticky positioning
- Second column onwards:
- Date/time headers:
w-40(40 units width) - Data cells:
w-80(80 units width)
- Date/time headers:
The table is also responsive with:
- Horizontal scroll support:
overflow-x-auto - Mobile/desktop adaptation:
w-full md:w-fit
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for width-related styles in the LogUpdateAnalayseTable component
rg -l "LogUpdateAnalayseTable" | xargs rg "width|w-"
Length of output: 1147
Script:
#!/bin/bash
# Get more context around the table implementation
rg -A 10 "LogUpdateAnalayseTable" src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Length of output: 426
Script:
#!/bin/bash
# Get the full content of LogUpdateAnalayseTable to see table structure and column definitions
cat src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Length of output: 3120
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
109-109: Simplify conditional checks using optional chainingConsider simplifying the conditional check using optional chaining for more concise code.
Apply this diff:
- if (res && res.ok && data) { + if (res?.ok && data) {🧰 Tools
🪛 Biome
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
134-144: Simplify data transformation usingflatMapand avoid mutating parametersUse
flatMapto simplify data transformation and avoid mutating the function parameterfby returning a new object with the addeddateproperty.Apply this diff:
- const dataToDisplay = data - .map((x) => - x.nursing - ? x.nursing.map((f: any) => { - f["date"] = x.date; - return f; - }) - : [], - ) - .reduce((accumulator, value) => accumulator.concat(value), []); + const dataToDisplay = data.flatMap((x) => + x.nursing + ? x.nursing.map((f: any) => ({ ...f, date: x.date })) + : [], + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx(1 hunks)src/components/Facility/Consultations/NursingPlot.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/Facility/Consultations/NursingPlot.tsx
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx (3)
6-10: Type safety concerns remain unaddressed.
The interface still uses loose typing which could lead to runtime errors.
See previous comment about strengthening type definitions for better type safety.
47-57: Fragment and key prop issues remain unaddressed.
See previous comment about removing unnecessary Fragment and fixing key prop placement.
41-92: Table accessibility improvements needed.
See previous comment about adding table accessibility attributes for screen reader support.
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (3)
134-142: Optimize data transformation logicThe current data transformation chain can be simplified for better readability and performance.
Consider this more concise implementation:
- const dataToDisplay = data - .map((x) => - x.nursing - ? x.nursing.map((f) => { - return { ...f, date: x.date }; - }) - : [], - ) - .reduce((accumulator, value) => accumulator.concat(value), []); + const dataToDisplay = data.flatMap((x) => + x.nursing?.map(f => ({ ...f, date: x.date })) ?? [] + );
144-149: Simplify filterEmpty functionThe function can be simplified using Array.some() for better readability.
Consider this implementation:
- const filterEmpty = (field: (typeof NURSING_CARE_PROCEDURES)[number]) => { - const filtered = dataToDisplay.filter( - (i: ProcedureType) => i.procedure === field, - ); - return filtered.length > 0; - }; + const filterEmpty = (field: (typeof NURSING_CARE_PROCEDURES)[number]) => + dataToDisplay.some((i: ProcedureType) => i.procedure === field);
179-183: Enhance accessibility for data presentationThe tables and empty state messages should have proper ARIA attributes for better screen reader support.
Consider adding these attributes:
- <div className="mt-1 w-full rounded-lg border bg-white p-4 shadow"> + <div className="mt-1 w-full rounded-lg border bg-white p-4 shadow" role="alert" aria-live="polite"> - <LogUpdateAnalayseTable data={mappedData} rows={rows} /> + <LogUpdateAnalayseTable + data={mappedData} + rows={rows} + aria-label={t("nursing_care_procedures_table")} + />Also applies to: 247-251
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx(1 hunks)src/components/Facility/models.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
267-286: LGTM! Well-structured component implementation
The ConsultationNursingTab component is well-organized with clear separation of concerns and proper prop handling.
247-251: 🛠️ Refactor suggestion
Ensure consistent error handling across components
Both NursingPlot and RoutineSection components handle API responses differently. Consider standardizing the approach.
Apply consistent error handling and optional chaining:
- if (res && res.ok && data) {
+ if (res?.ok && data) {This should be applied consistently across both components.
Likely invalid or redundant comment.
src/components/Facility/models.tsx (1)
394-397: Type enhancement improves safety and clarity.
The change from any[] to a strongly-typed array with explicit procedure and description fields improves type safety and makes the data structure self-documenting.
Let's verify the impact on existing code:
✅ Verification successful
Type enhancement is safe to implement
The strongly-typed nursing array is already being used consistently across the codebase:
- In
DailyRounds.tsx, the nursing data is properly passed through the form state - In
NursingCare.tsx, the component correctly handles the nursing array with procedure and description fields - In
ConsultationNursingTab.tsxandCriticalCarePreview.tsx, the data is accessed using the expected structure - No instances of incompatible access patterns were found
The type enhancement aligns with the existing implementation and improves type safety without breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage of NursingPlotRes to ensure compatibility
# Test: Search for NursingPlotRes usage and nursing field access patterns
rg -l "NursingPlotRes" | xargs rg "nursing\["
rg -l "NursingPlotRes" | xargs rg "nursing\."
# Test: Search for daily rounds API response handling
ast-grep --pattern 'nursing = $_'
Length of output: 127
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Find all files containing NursingPlotRes
echo "=== Files containing NursingPlotRes ==="
rg -l "NursingPlotRes"
# 2. Search for any nursing-related property access patterns with context
echo -e "\n=== Nursing property access patterns ==="
rg "\.nursing" -A 2 -B 2
# 3. Look for array operations on nursing data
echo -e "\n=== Array operations on nursing data ==="
rg "nursing\[|nursing\.map|nursing\.forEach|nursing\.filter" -A 2 -B 2
# 4. Search for nursing-related component usage
echo -e "\n=== Nursing component usage ==="
rg "NursingPlot|NursingCare" -A 2 -B 2
Length of output: 12331
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
134-142: Simplify data transformation logicThe nested map-reduce operation can be simplified using flatMap for better readability and performance.
Consider this simpler approach:
- const dataToDisplay = data - .map((x) => - x.nursing - ? x.nursing.map((f) => { - return { ...f, date: x.date }; - }) - : [], - ) - .reduce((accumulator, value) => accumulator.concat(value), []); + const dataToDisplay = data.flatMap((x) => + x.nursing?.map((f) => ({ ...f, date: x.date })) ?? [] + );
262-281: LGTM! Consider adding ARIA labelsThe implementation looks good and follows a clean structure. To improve accessibility, consider adding ARIA labels to the section headings.
Add ARIA labels:
- <h4>{t("routine")}</h4> + <h4 aria-label={t("routine")}>{t("routine")}</h4> - <h4>{t("nursing_care")}</h4> + <h4 aria-label={t("nursing_care")}>{t("nursing_care")}</h4>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (3)
242-246: LGTM! Good use of the reusable table component
The implementation successfully transitions the Routine Section to use the shared table component, improving consistency across the UI.
105-113:
Add error handling for the API request
The API request lacks error handling, which could lead to silent failures. Users won't receive feedback if the request fails.
Add error handling:
const fetchDailyRounds = async (
currentPage: number,
consultationId: string,
) => {
+ try {
const { res, data } = await request(routes.dailyRoundsAnalyse, {
body: { page: currentPage, fields: NursingPlotFields },
pathParams: { consultationId },
});
if (res?.ok && data) {
setResults(data.results);
setTotalCount(data.count);
}
+ } catch (error) {
+ console.error('Failed to fetch daily rounds:', error);
+ // Add user-facing error notification
+ }
};Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
242-246: Verify table width configurations
The implementation uses LogUpdateAnalayseTable for both sections, which should handle the fixed widths for time columns. Please verify that the table widths are correctly configured in the LogUpdateAnalayseTable component.
Also applies to: 180-181
✅ Verification successful
Let me analyze the LogUpdateAnalayseTable component further to verify the width configurations.
Let me try a different pattern to get the component implementation.
Table width configurations are properly implemented
The LogUpdateAnalayseTable component has appropriate width configurations:
- Time columns have fixed width of 40 (
w-40) with proper borders and styling - Data cells have consistent width of 80 (
w-80) with proper alignment - Table is responsive with
overflow-x-autoandmd:w-fitfor larger screens - Sticky header and first column for better usability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for width-related configurations in the LogUpdateAnalayseTable component
rg -l "LogUpdateAnalayseTable" | xargs rg "width|style|className"
Length of output: 2471
Script:
#!/bin/bash
# Get the full implementation of LogUpdateAnalayseTable component
ast-grep --pattern 'export function LogUpdateAnalayseTable($_) {
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Get the full content of the LogUpdateAnalayseTable component file
cat src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
Length of output: 3179
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
134-136: Simplify data transformation using optional chainingThe data transformation can be simplified and made more readable using optional chaining and nullish coalescing.
- const dataToDisplay = data.flatMap( - (x) => x.nursing?.map((f) => ({ ...f, date: x.date })) ?? [], - ); + const dataToDisplay = data.flatMap((x) => + x.nursing?.map(f => ({ ...f, date: x.date })) ?? [] + );
155-162: Consider using Object.groupBy for data transformationThe data transformation using reduce could be simplified using the new Object.groupBy method (if targeting modern environments).
- const mappedData = dataToDisplay.reduce( - (acc: Record<string, Record<string, string>>, item) => { - if (!acc[item.date]) acc[item.date] = {}; - acc[item.date][item.procedure] = item.description; - return acc; - }, - {}, - ); + const mappedData = Object.groupBy( + dataToDisplay, + item => item.date, + ) as Record<string, Record<string, string>>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
236-240: LGTM! Successfully migrated to reusable table component
The implementation correctly uses the LogUpdateAnalayseTable component with appropriate props for data, rows, and choices mapping.
256-275: LGTM! Well-structured with good accessibility practices
The component maintains a clear separation of concerns and includes proper ARIA labels for better accessibility.
…dures Co-authored-by: Sahil-Sinha-11 <[email protected]> Co-authored-by: Aakash Singh <[email protected]>
sainak
force-pushed
the
issues/8562/Nursing_tab_ui
branch
from
6cda5b5 to
efff4f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
155-162: Optimize data transformation for better performanceThe reduce operation creates a new object for each date, which could be optimized using Object.groupBy (if available) or a Map.
Consider this alternative implementation:
- const mappedData = dataToDisplay.reduce( - (acc: Record<string, Record<string, string>>, item) => { - if (!acc[item.date]) acc[item.date] = {}; - acc[item.date][item.procedure] = item.description; - return acc; - }, - {}, - ); + const mappedData = Object.fromEntries( + dataToDisplay.reduce((map, item) => { + const dateData = map.get(item.date) || {}; + map.set(item.date, { ...dateData, [item.procedure]: item.description }); + return map; + }, new Map()) + );
256-275: LGTM! Consider enhancing section landmarksThe implementation successfully integrates the new NursingPlot component while maintaining good accessibility with aria-labels.
Consider using semantic HTML5 sections for better accessibility:
- <div> + <section aria-labelledby="routine-heading"> - <h4 aria-label={t("routine")}>{t("routine")}</h4> + <h4 id="routine-heading">{t("routine")}</h4> <RoutineSection {...props} /> - </div> + </section> - <div> + <section aria-labelledby="nursing-care-heading"> - <h4 aria-label={t("nursing_care")}>{t("nursing_care")}</h4> + <h4 id="nursing-care-heading">{t("nursing_care")}</h4> <NursingPlot {...props} /> - </div> + </section>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx(1 hunks)src/components/Facility/Consultations/NursingPlot.tsx(0 hunks)src/components/Facility/models.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Facility/Consultations/NursingPlot.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Facility/Consultations/LogUpdateAnalayseTable.tsx
- src/components/Facility/models.tsx
🧰 Additional context used
🪛 Biome
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (1)
236-240: LGTM! Successfully migrated to reusable table component
The implementation successfully transitions to using the reusable LogUpdateAnalayseTable component, improving code consistency and maintainability.
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
Outdated
Show resolved
Hide resolved
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (3)
144-146: Consider using Array.filter for better readabilityThe Set intersection operation could be simplified using Array methods for better readability and maintainability.
- fieldsToDisplay = fieldsToDisplay.intersection( - new Set(NURSING_CARE_PROCEDURES), - ); + fieldsToDisplay = new Set( + Array.from(fieldsToDisplay).filter(field => + NURSING_CARE_PROCEDURES.includes(field) + ) + );
127-142: Simplify data transformation using Object.entries and reduceThe nested data transformation logic could be simplified for better readability and performance.
- const dataNew = Object.entries(results).reduce( - (acc: Record<string, Record<string, string>>, [date, result]) => { - if ("nursing" in result) { - result.nursing.forEach((field) => { - if (field.procedure && !acc[date]) { - acc[date] = {}; - } - acc[date][field.procedure] = field.description; - fieldsToDisplay.add(field.procedure); - }); - } - return acc; - }, - {}, - ); + const dataNew = Object.entries(results).reduce( + (acc, [date, result]) => { + if (!("nursing" in result)) return acc; + + acc[date] = result.nursing.reduce((fields, field) => { + if (field.procedure) { + fields[field.procedure] = field.description; + fieldsToDisplay.add(field.procedure); + } + return fields; + }, {}); + + return acc; + }, + {} as Record<string, Record<string, string>> + );
248-262: Enhance accessibility with semantic structureWhile the basic accessibility is in place, we can improve it further with proper section landmarks and heading hierarchy.
- <div> + <main role="main"> <PageTitle title={t("nursing_information")} hideBack breadcrumbs={false} /> - <div> + <section aria-labelledby="routine-heading"> - <h4 aria-label={t("routine")}>{t("routine")}</h4> + <h2 id="routine-heading">{t("routine")}</h2> <RoutineSection {...props} /> - </div> - <div> - <h4 aria-label={t("nursing_care")}>{t("nursing_care")}</h4> + </section> + <section aria-labelledby="nursing-care-heading"> + <h2 id="nursing-care-heading">{t("nursing_care")}</h2> <NursingPlot {...props} /> - </div> - </div> + </section> + </main>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (1)
225-229: LGTM! Consistent use of LogUpdateAnalayseTable
The implementation correctly uses the shared table component, maintaining consistency with the NursingPlot section.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
106-114: Add type safety to API response handlingThe type assertion on line 111 could be avoided by properly typing the API response.
- setResults(data.results as { [date: string]: NursingPlotRes }); + setResults(data.results);
127-140: Optimize data transformation logicThe data transformation could be simplified and made more type-safe:
const tableData = Object.entries(results).reduce( (acc: Record<string, Record<string, string>>, [date, result]) => { - if ("nursing" in result) { - result.nursing.forEach((field) => { - if (field.procedure && !acc[date]) acc[date] = {}; - acc[date][field.procedure] = field.description; - // Add procedure to the set of procedures to display - fieldsToDisplay.add(field.procedure); - }); - } + if (!result.nursing?.length) return acc; + + acc[date] = result.nursing.reduce((fields, field) => { + if (field.procedure) { + fields[field.procedure] = field.description; + fieldsToDisplay.add(field.procedure); + } + return fields; + }, {} as Record<string, string>); + return acc; }, {}, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
Learnt from: sainak
PR: ohcnetwork/care_fe#9079
File: src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx:93-117
Timestamp: 2024-11-13T11:33:22.403Z
Learning: In `src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx`, the parent component manages the loading state, so child components like `NursingPlot` should not implement their own loading indicators.
Learnt from: sainak
PR: ohcnetwork/care_fe#9079
File: src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx:0-0
Timestamp: 2024-11-13T11:32:39.734Z
Learning: In this project, the `request` function used for API calls handles displaying error notifications internally, so additional error handling in the components is unnecessary.
🔇 Additional comments (4)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (4)
8-11: Typo in component name persists
The component name LogUpdateAnalayseTable contains a spelling error that was previously identified.
223-227: LGTM! Successful implementation of reusable component
The change successfully implements the reusable table component as specified in the PR objectives.
243-262: LGTM! Well-structured component with good accessibility
The component successfully:
- Implements the required sections from PR objectives
- Uses semantic HTML structure
- Includes proper ARIA labels for accessibility
161-161: Verify column width adjustments
The PR objectives mention adjusting the width for the second column and subsequent columns, but this isn't visible in the current implementation.
There was a problem hiding this comment.
Other than https://github.com/ohcnetwork/care_fe/pull/9079/files#r1836226214 LGTM;
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/components/Facility/Consultations/LogUpdateAnalyseTable.tsx (3)
6-10: Consider strengthening type safety of the props interface.The interface could benefit from more specific types:
interface SharedSectionTableProps { - data: Record<string, Record<string, string | boolean | null>>; - rows: Array<{ title?: string; field?: string; subField?: boolean }>; + data: Record<string, Record<string, string | boolean | null>>; // Consider adding specific field keys + rows: Array< + | { title: string; field?: never; subField?: boolean } + | { title?: never; field: string; subField?: boolean } + >; choices?: Record<string, Record<string | number, string>>; }This would:
- Ensure each row has either a title or field, but not both
- Make the field required when title is not present
21-39: Consider adding safeguards to the translation key construction.The function handles value transformation well, but the translation key construction could be more robust:
- return choice ? t(`${field.toUpperCase()}__${choice}`) : "-"; + const translationKey = choice ? `${field.toUpperCase()}__${choice}`.replace(/\s+/g, '_') : null; + return translationKey ? t(translationKey) : "-";This would:
- Handle potential spaces in field names
- Prevent attempting to translate an empty key
1-93: Implementation successfully meets PR objectives.The component effectively addresses the requirements from issue #8562:
- Creates a reusable table component
- Implements fixed widths for time columns (w-40)
- Provides consistent styling and improved UI
Consider documenting the component's usage patterns in the codebase to help other developers understand how to properly implement it in different contexts.
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
142-144: Consider moving the intersection operationThe Set intersection operation could be performed earlier during data transformation to avoid processing unnecessary data.
Consider moving the intersection before the data transformation:
- fieldsToDisplay = fieldsToDisplay.intersection( - new Set(NURSING_CARE_PROCEDURES), - ); + // Filter procedures during data transformation + if (field.procedure && + NURSING_CARE_PROCEDURES.includes(field.procedure) && + !acc[date]) { + acc[date] = {}; + }
127-140: Simplify data transformation logicThe data transformation could be simplified using Object.entries and reduce.
Consider this more concise approach:
- const tableData = Object.entries(results).reduce( - (acc: Record<string, Record<string, string>>, [date, result]) => { - if ("nursing" in result) { - result.nursing.forEach((field) => { - if (field.procedure && !acc[date]) acc[date] = {}; - acc[date][field.procedure] = field.description; - fieldsToDisplay.add(field.procedure); - }); - } - return acc; - }, - {}, - ); + const tableData = Object.entries(results).reduce((acc, [date, result]) => { + if (!("nursing" in result)) return acc; + + acc[date] = result.nursing.reduce((fields, { procedure, description }) => { + if (procedure) { + fields[procedure] = description; + fieldsToDisplay.add(procedure); + } + return fields; + }, {}); + + return acc; + }, {} as Record<string, Record<string, string>>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx(4 hunks)src/components/Facility/Consultations/LogUpdateAnalyseTable.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
Learnt from: sainak
PR: ohcnetwork/care_fe#9079
File: src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx:93-117
Timestamp: 2024-11-13T11:33:22.403Z
Learning: In `src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx`, the parent component manages the loading state, so child components like `NursingPlot` should not implement their own loading indicators.
Learnt from: sainak
PR: ohcnetwork/care_fe#9079
File: src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx:0-0
Timestamp: 2024-11-13T11:32:39.734Z
Learning: In this project, the `request` function used for API calls handles displaying error notifications internally, so additional error handling in the components is unnecessary.
🔇 Additional comments (3)
src/components/Facility/Consultations/LogUpdateAnalyseTable.tsx (1)
12-20: LGTM! Good use of React patterns.
The component initialization shows good practices:
- Proper type annotation with React.FC
- Effective use of useTranslation hook
- Smart memoization of data values to prevent unnecessary recalculations
src/components/Facility/ConsultationDetails/ConsultationNursingTab.tsx (2)
223-227: LGTM! Table implementation improved
The replacement of the custom table with LogUpdateAnalyseTable component improves consistency and maintainability.
243-262: LGTM! Improved accessibility and organization
The component structure has been improved with:
- Clear section separation
- Added aria-labels for better accessibility
- Logical grouping of nursing information
| return ( | ||
| <div className="m-2 w-full overflow-hidden overflow-x-auto rounded-lg border border-black shadow md:w-fit"> | ||
| <table className="border-collapse rounded-lg border bg-secondary-100"> | ||
| <thead className="sticky top-0 bg-white shadow"> | ||
| <tr> | ||
| <th className="sticky left-0 border-b-2 border-r-2 border-black bg-white"></th> | ||
| {Object.keys(data).map((date) => ( | ||
| <th | ||
| key={date} | ||
| className="w-40 border border-b-2 border-secondary-500 border-b-black p-1 text-sm font-semibold" | ||
| > | ||
| <p>{formatDate(date)}</p> | ||
| <p>{formatTime(date)}</p> | ||
| </th> | ||
| ))} | ||
| </tr> | ||
| </thead> | ||
| <tbody className="bg-secondary-200"> | ||
| {rows.map((row) => ( | ||
| <tr | ||
| key={row.field ?? row.title} | ||
| className={classNames( | ||
| row.title && "border-t-2 border-t-secondary-600", | ||
| )} | ||
| > | ||
| <th | ||
| className={classNames( | ||
| "sticky left-0 border border-r-2 border-secondary-500 border-r-black bg-white p-2", | ||
| row.subField ? "pl-4 font-medium" : "font-bold", | ||
| )} | ||
| > | ||
| {row.title ?? t(`LOG_UPDATE_FIELD_LABEL__${row.field!}`)} | ||
| </th> | ||
| {dataValues.map((obj, idx) => { | ||
| const value = row.field ? obj[row.field] : undefined; | ||
| return ( | ||
| <td | ||
| key={`${row.field}-${idx}`} | ||
| className="w-80 border border-l-2 border-secondary-500 bg-secondary-100 p-2 text-center font-medium" | ||
| > | ||
| {row.field ? getDisplayValue(value, row.field) : "-"} | ||
| </td> | ||
| ); | ||
| })} | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance table accessibility.
While the table structure and styling are well-implemented, consider adding these accessibility improvements:
- <div className="m-2 w-full overflow-hidden overflow-x-auto rounded-lg border border-black shadow md:w-fit">
+ <div
+ className="m-2 w-full overflow-hidden overflow-x-auto rounded-lg border border-black shadow md:w-fit"
+ role="region"
+ aria-label={t('nursing_care_table')}
+ >
<table className="border-collapse rounded-lg border bg-secondary-100">
+ <caption className="sr-only">{t('nursing_care_table_caption')}</caption>
<thead className="sticky top-0 bg-white shadow">
<tr>
- <th className="sticky left-0 border-b-2 border-r-2 border-black bg-white"></th>
+ <th scope="col" className="sticky left-0 border-b-2 border-r-2 border-black bg-white"></th>
{Object.keys(data).map((date) => (
<th
+ scope="col"
key={date}
className="w-40 border border-b-2 border-secondary-500 border-b-black p-1 text-sm font-semibold"
>This adds:
- ARIA roles and labels for screen readers
- Proper table header scoping
- Hidden caption for better table context
Committable suggestion skipped: line range outside the PR's diff.
|
LGTM |
Picked up changes from #8608
Changes made
fixes: #8562
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
NursingPlotcomponent for enhanced display of nursing round data.LogUpdateAnalyseTablecomponent for improved rendering of routine analysis results.Improvements
ConsultationNursingTab.Bug Fixes