-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(my-pages): small tweaks for vaccinations #17138
Conversation
WalkthroughThe pull request includes modifications to the Vaccination API's OpenAPI specification, particularly for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17138 +/- ##
==========================================
+ Coverage 35.73% 35.74% +0.01%
==========================================
Files 6928 6927 -1
Lines 147824 147578 -246
Branches 42129 42005 -124
==========================================
- Hits 52818 52754 -64
+ Misses 95006 94824 -182
Flags with carried forward coverage won't be shown. Click here to find out more. see 47 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
libs/portals/my-pages/health/src/screens/HealthOverview/HealthOverview.tsx (1)
Line range hint
39-210
: Well-structured component with good separation of concerns.The component demonstrates good practices:
- Proper error handling and loading states
- Reusable UI components from shared libraries
- Consistent spacing using shared constants
- Type-safe implementation
Consider extracting the insurance confirmation logic into a custom hook for better reusability across other health-related components.
Example hook extraction:
const useInsuranceConfirmation = () => { const [displayConfirmationErrorAlert, setDisplayConfirmationErrorAlert] = useState(false); const [getInsuranceConfirmationLazyQuery, { loading, error }] = useGetInsuranceConfirmationLazyQuery(); const getConfirmation = async () => { const { data } = await getInsuranceConfirmationLazyQuery(); const downloadData = data?.rightsPortalInsuranceConfirmation; if (downloadData?.data && downloadData.fileName) { downloadLink(downloadData.data, 'application/pdf', downloadData.fileName); } }; useEffect(() => { if (error) { setDisplayConfirmationErrorAlert(true); } }, [error]); return { getConfirmation, loading, displayConfirmationErrorAlert, setDisplayConfirmationErrorAlert }; };libs/portals/my-pages/health/src/screens/Vaccinations/VaccinationsWrapper.tsx (1)
Line range hint
42-93
: Consider extracting button section to a separate componentThe buttons section could be extracted to improve reusability and maintain the component's single responsibility.
Consider creating a separate component:
interface VaccinationActionsProps { makeAppointmentLink: string; readMoreLink: string; makeAppointmentText: string; readMoreText: string; } const VaccinationActions: React.FC<VaccinationActionsProps> = ({ makeAppointmentLink, readMoreLink, makeAppointmentText, readMoreText, }) => ( <Box printHidden display="flex" flexDirection="row" marginBottom={6}> <LinkButton to={readMoreLink} icon="open" variant="utility" text={readMoreText} /> <Box marginLeft={1}> <LinkButton to={makeAppointmentLink} icon="open" variant="utility" text={makeAppointmentText} /> </Box> </Box> );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json
(8 hunks)libs/portals/my-pages/core/src/components/SortableTable/SortableTable.tsx
(3 hunks)libs/portals/my-pages/health/src/screens/HealthOverview/HealthOverview.tsx
(4 hunks)libs/portals/my-pages/health/src/screens/Vaccinations/VaccinationsWrapper.tsx
(3 hunks)libs/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx
(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/my-pages/health/src/screens/Vaccinations/VaccinationsWrapper.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/my-pages/health/src/screens/HealthOverview/HealthOverview.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/my-pages/core/src/components/SortableTable/SortableTable.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (10)
libs/portals/my-pages/health/src/screens/HealthOverview/HealthOverview.tsx (1)
7-7
: LGTM! Import changes are well-organized.
The imports are properly structured and follow good tree-shaking practices by importing specific components.
Also applies to: 10-10, 15-15, 25-25, 26-26
libs/portals/my-pages/core/src/components/SortableTable/SortableTable.tsx (2)
169-172
: Verify the disabled prop behavior for Tag component
The disabled
prop has been added to Tag components without conditional logic. This might affect all tags in the table regardless of their state.
Let's check if the disabled prop is defined in the Tag component and its intended usage:
Also applies to: 190-194
✅ Verification successful
Let me search for the Tag component interface to understand if the disabled prop is properly defined.
Let me try a different pattern to find the Tag component definition.
Based on the code analysis, I can now provide a definitive response about the disabled prop usage in the Tag component:
The disabled prop is properly implemented and safe to use
The Tag component in libs/island-ui/core/src/lib/Tag/Tag.tsx
properly handles the disabled prop by:
- Applying appropriate styling through the
disabled
CSS class - Setting the disabled attribute on button elements when Tag is rendered as a button
- Using consistent styling for disabled state across all variants
The disabled prop is widely used across the codebase in a consistent manner, and the implementation in SortableTable is following the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Tag component definition and its usage
ast-grep --pattern 'interface $interface {
$$$
disabled?: $_
$$$
}'
# Search for other usages of disabled Tag
rg -B 2 -A 2 '<Tag.*disabled.*>'
Length of output: 32669
Script:
#!/bin/bash
# Search for Tag component interface/props definition
ast-grep --pattern 'interface $interface {
$$$
disabled?: boolean
$$$
}'
# Also search for Tag component implementation
ast-grep --pattern 'const Tag = ({$$$}) => {'
Length of output: 128
Script:
#!/bin/bash
# Search for Tag component in island-ui/core
fd "Tag" libs/island-ui/core -t f -x cat {}
# Also search for any TypeScript interfaces containing "TagProps"
ast-grep --pattern 'interface $interface Props {
$$$
}'
# And search for any exports containing Tag
rg "export.*Tag" -A 5
Length of output: 99435
Line range hint 51-51
: LGTM: Default sort by status aligns with requirements
The change from sorting by "vaccine" to "status" implements the auto-sorting of vaccination status requirement from the PR objectives.
libs/portals/my-pages/health/src/screens/Vaccinations/VaccinationsWrapper.tsx (1)
42-47
: LGTM: IntroWrapper implementation is correct
The replacement of IntroHeader with IntroWrapper maintains all necessary props and improves consistency across health-related screens as mentioned in the PR summary.
libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json (3)
Line range hint 404-404
: LGTM: 404 response addition for disease not found.
The addition of a 404 response to /v1/me/vaccinations/diseases/{diseaseId}
improves error handling by explicitly documenting the "disease not found" scenario.
869-869
: Verify handling of hideIfNoVaccinations
in UI components.
The addition of hideIfNoVaccinations
to Disease DTOs suggests a new UI feature for conditional display of diseases.
Consider implementing this logic at the API level to reduce unnecessary data transfer when hideIfNoVaccinations
is true.
Also applies to: 888-888, 902-902
Line range hint 600-621
: Parameter renaming improves clarity but verify client impact.
The renaming of age
to agePatient
and addition of vaccineCodes
parameter in the /v1/visualizer/diseases/{diseaseId}
endpoint improves clarity but requires client updates.
Ensure all API clients are updated to:
- Use
agePatient
instead ofage
- Include the new required
vaccineCodes
parameter
libs/portals/my-pages/health/src/screens/Vaccinations/tables/SortedVaccinationsTable.tsx (3)
Line range hint 1-14
: LGTM! Well-structured imports and type definitions.
The code follows best practices for reusability and type safety:
- Specific imports enable effective tree-shaking
- Clear TypeScript interface for props
- Modular structure suitable for cross-app usage
Line range hint 16-99
: LGTM! Well-structured and reusable component.
The component demonstrates good practices:
- Clear separation of concerns
- Proper error handling
- Reusable structure suitable for the libs directory
- Strong TypeScript typing throughout
51-51
: Verify sorting behavior with empty or undefined status.
The change to sort by status aligns with the PR objectives. However, please ensure that:
- Empty or undefined status values are handled gracefully in the sorting logic
- The sorting order (ascending/descending) meets user expectations
✅ Verification successful
Sorting by status is safely implemented
The code review reveals that:
- The
SortableTable
component handles undefined values gracefully using string comparison (localeCompare
) - Empty/undefined status values are handled by the
tagSelector
utility which returns a default 'blue' tag - The sorting implementation in
useSortableData
hook ensures stable sorting behavior
The change to sort by status is safe and well-implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how undefined/empty status values are handled in the codebase
ast-grep --pattern 'status: $status ?? $default'
Length of output: 50
Script:
#!/bin/bash
# Let's check the sorting implementation and status handling
ast-grep --pattern 'const $_ = $$$sort$$$status$$$'
# Also check for any sorting utility functions
rg -A 5 "sortBy.*status"
Length of output: 80
Script:
#!/bin/bash
# Let's look for the table component implementation
fd "SortedVaccinationsTable" --exec cat {}
# Also check for any sorting related code in the same directory
rg -g "*.{ts,tsx}" "sort" libs/portals/my-pages/health/src/screens/Vaccinations/
Length of output: 3591
Script:
#!/bin/bash
# Let's check the SortableTable component to understand how sorting is handled
fd "SortableTable" --type f --exec cat {}
# Also check the tagSelector utility to see how status is handled
rg -A 5 "tagSelector" libs/portals/my-pages/health/src/
Length of output: 10235
libs/portals/my-pages/health/src/screens/HealthOverview/HealthOverview.tsx
Show resolved
Hide resolved
libs/clients/health-directorate/src/lib/clients/vaccinations/clientConfig.json
Show resolved
Hide resolved
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(ads): Update code for explicit and manual use (#17193) * Update code for explicit and manual use * Fix tests to handle logging warn and info --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(my-pages): small tweaks for vaccinations (#17138) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Ásdís Erna Guðmundsdóttir <disa@hugsmidjan.is>
Health - Vaccinations
What
Checklist:
Summary by CodeRabbit
New Features
hideIfNoVaccinations
in several disease-related schemas.Improvements
SortableTable
component with adisabled
prop for tags.IntroHeader
withIntroWrapper
in multiple components for improved layout.SortedVaccinationsTable
to prioritizestatus
.Bug Fixes
404
response for specific disease retrieval in the Vaccination API.