Minor fixes #107
Minor fixes #107
Conversation
Immunization threw an undefined error Added option to view generated composition
WalkthroughThe pull request introduces modifications to the GitHub Actions workflow for building and pushing Docker images, adding steps based on the branch context. It also updates the Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
![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 (2)
src/app/components/PatientSummaryViewer/resources/Immunization/index.tsx (1)
Based on the gathered information, I can now provide a final response:
The data origin field should be implemented using the FHIR Immunization.primarySource attribute
From the code analysis, we can see that:
- The commented code shows a hardcoded 'non-primary source' value, which is incorrect as it should be dynamically determined from the resource data
- The Immunization component is using FHIR resource types, but the primarySource field is not currently being utilized
- No other components in the codebase are handling primarySource, suggesting this is a missing implementation
To properly implement this:
- Remove the TODO comment
- Uncomment and modify the row configuration to use the primarySource field from the FHIR resource
- The value should be dynamically set based on the primarySource boolean field, not hardcoded
Example implementation:
{ type: 'row', config: { label: 'Data origin', render: ({ primarySource }) => primarySource ? 'primary source' : 'non-primary source', }, }🔗 Analysis chain
Line range hint
47-51: Verify the data origin logic and update the TODO comment.The commented code and TODO note suggest that verification is needed for the data origin field logic. This should be addressed to ensure the component correctly handles immunization source information according to FHIR specifications.
Let's check the FHIR specification and codebase for similar patterns:
Would you like me to help investigate the correct implementation for the data origin field or create an issue to track this verification task?
Let me gather more information about the Immunization resource type and its usage in the codebase.
Let me check the FHIR resource definitions and implementations to understand how primarySource is handled.
Let me check one more thing to understand how the Immunization type is defined and used in the codebase.
Let me check one final thing about the FHIR types and interfaces to understand how primarySource should be handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components handling primarySource or similar fields rg -g '*.tsx' -g '*.ts' 'primarySource|dataOrigin' --type-add 'tsx:*.tsx' # Look for FHIR Immunization handling patterns ast-grep --pattern 'type TImmunization = $_'Length of output: 249
Script:
#!/bin/bash # Search for the TType and EResource definitions rg -g '*.ts*' 'export (type TType|enum EResource)' # Search for Immunization-related types and interfaces rg -g '*.ts*' 'interface.*Immunization|type.*Immunization' # Look for actual usage of primarySource in the codebase rg -g '*.ts*' -B 2 -A 2 'primarySource' # Check if there are any test files related to Immunization fd -g '*immunization*.test.*'Length of output: 453
Script:
#!/bin/bash # Check the FHIR resource interfaces rg -g '*.ts*' -A 10 'interface IResource' # Look for any Immunization-related mappings or transformations rg -g '*.ts*' -B 2 -A 2 'Immunization.*\{|\.primarySource' # Check for any configuration or mapping files fd -g '*immunization*.{json,ts,tsx}' # Look for any resource transformation utilities rg -g '*.ts*' 'transform.*Immunization|map.*Immunization'Length of output: 2359
Script:
#!/bin/bash # Look for Immunization type imports and the actual type definition rg -g '*.ts*' 'import.*Immunization|type Immunization' # Check the content of the file containing the Immunization component cat src/app/components/PatientSummaryViewer/resources/Immunization/index.tsx # Look for any configuration related to immunization data display rg -g '*.ts*' -B 5 -A 5 'rows.*=.*\[.*Immunization'Length of output: 1949
Script:
#!/bin/bash # Look for the actual FHIR type definitions rg -g '*.ts*' -A 15 'type (Immunization|Patient) =|interface (Immunization|Patient) ' # Check for any FHIR-related utility functions or transformations rg -g '*.ts*' 'getFHIR|transformFHIR|mapFHIR' # Look for any documentation files related to FHIR resources fd -g 'README*' -x grep -l 'FHIR'Length of output: 169
.github/workflows/tests-build-push.yml (1)
Line range hint
56-72: Consider adding image scanning for security vulnerabilities.Since we're building and pushing Docker images, it would be beneficial to add a security scanning step before pushing to DockerHub.
Consider adding Trivy or Snyk container scanning:
- name: Run Trivy vulnerability scanner uses: aquasecurity/trivy-action@master with: image-ref: 'jembi/smart-health-links-portal:${{ github.sha }}' format: 'table' exit-code: '1' ignore-unfixed: true vuln-type: 'os,library' severity: 'CRITICAL,HIGH'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/tests-build-push.yml (2 hunks)
- src/app/components/PatientSummaryViewer/resources/Composition/index.tsx (2 hunks)
- src/app/components/PatientSummaryViewer/resources/Immunization/index.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
src/app/components/PatientSummaryViewer/resources/Composition/index.tsx
[error] 566-566: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (4)
src/app/components/PatientSummaryViewer/resources/Immunization/index.tsx (1)
40-40: LGTM! Improved null safety with optional chaining.The addition of optional chaining for the
actorproperty is a good defensive programming practice that prevents potential runtime errors when accessing nested FHIR resource properties..github/workflows/tests-build-push.yml (2)
Line range hint
56-64: LGTM: Main branch build configuration is well structured.The configuration correctly handles the main branch case with appropriate platform targets and tagging strategy.
65-72:⚠️ Potential issueVerify Docker credentials for non-main branch pushes.
The workflow attempts to push images for non-main branches, but the Docker login step only runs for the main branch. This could lead to authentication failures.
Apply this diff to fix the authentication issue:
- name: Login to DockerHub - if: ${{ github.ref_name == 'main' }} + if: ${{ github.ref_name == 'main' || github.event_name == 'pull_request' }} uses: docker/login-action@v3 with: username: ${{ secrets.DOCKERHUB_USERNAME }} password: ${{ secrets.DOCKERHUB_TOKEN }}src/app/components/PatientSummaryViewer/resources/Composition/index.tsx (1)
1-1: LGTM: Import statements are appropriate.The new imports for
BoxandStyledButtonare correctly added and align with the new toggle functionality being implemented.Also applies to: 4-4
| - name: Build and push latest - tag | ||
| if: ${{ github.ref_name != 'main' }} | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| platforms: linux/amd64,linux/arm64 | ||
| push: true | ||
| file: docker/production/Dockerfile | ||
| tags: jembi/smart-health-links-portal:hotfix |
There was a problem hiding this comment.
Reconsider the use of 'hotfix' as a static tag.
Using a static 'hotfix' tag for all non-main branches could lead to confusion and potential deployment issues. Consider using a more specific tag format that includes:
- Branch name or PR number
- Commit SHA
- Timestamp
This would provide better traceability and prevent tag conflicts.
Here's a suggested modification:
- tags: jembi/smart-health-links-portal:hotfix
+ tags: jembi/smart-health-links-portal:pr-${{ github.event.pull_request.number }}-${{ github.sha }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build and push latest - tag | |
| if: ${{ github.ref_name != 'main' }} | |
| uses: docker/build-push-action@v5 | |
| with: | |
| platforms: linux/amd64,linux/arm64 | |
| push: true | |
| file: docker/production/Dockerfile | |
| tags: jembi/smart-health-links-portal:hotfix | |
| - name: Build and push latest - tag | |
| if: ${{ github.ref_name != 'main' }} | |
| uses: docker/build-push-action@v5 | |
| with: | |
| platforms: linux/amd64,linux/arm64 | |
| push: true | |
| file: docker/production/Dockerfile | |
| tags: jembi/smart-health-links-portal:pr-${{ github.event.pull_request.number }}-${{ github.sha }} |
| <Box | ||
| sx={{ | ||
| padding: '20px 0px', | ||
| width: '100%', | ||
| '& table': { | ||
| width: '100%', | ||
| borderCollapse: 'collapse', | ||
| marginBottom: '20px', | ||
| '& th, & td': { | ||
| border: '1px solid gray', | ||
| padding: '8px', | ||
| textAlign: 'left', | ||
| }, | ||
| }, | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract styles to a styled component.
Inline styles should be moved to styled components for better maintainability and reusability.
Consider creating a styled component:
const StyledBox = styled(Box)(({ theme }) => ({
padding: '20px 0px',
width: '100%',
'& table': {
width: '100%',
borderCollapse: 'collapse',
marginBottom: '20px',
'& th, & td': {
border: '1px solid gray',
padding: '8px',
textAlign: 'left',
},
},
}));Then use it in your component:
-<Box
- sx={{
- padding: '20px 0px',
- width: '100%',
- '& table': {
- width: '100%',
- borderCollapse: 'collapse',
- marginBottom: '20px',
- '& th, & td': {
- border: '1px solid gray',
- padding: '8px',
- textAlign: 'left',
- },
- },
- }}
->
+<StyledBox>| <StyledButton size="small" variant="contained" onClick={toggleInfo}> | ||
| Toggle Generated | ||
| </StyledButton> |
There was a problem hiding this comment.
Enhance button accessibility.
The toggle button lacks proper accessibility attributes.
Add ARIA attributes to improve accessibility:
-<StyledButton size="small" variant="contained" onClick={toggleInfo}>
+<StyledButton
+ size="small"
+ variant="contained"
+ onClick={toggleInfo}
+ aria-expanded={isVisible}
+ aria-controls="composition-generated"
+>
Toggle Generated
</StyledButton>Committable suggestion was skipped due to low confidence.
| const toggleInfo = () => { | ||
| const infoDiv = document.getElementById('composition-generated'); | ||
|
|
||
| if (!infoDiv) { | ||
| return null; | ||
| } | ||
|
|
||
| if (infoDiv.style.display === 'none') { | ||
| infoDiv.style.display = 'block'; | ||
| } else { | ||
| infoDiv.style.display = 'none'; | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
Replace DOM manipulation with React state management.
The current implementation uses direct DOM manipulation which goes against React's principles and could lead to synchronization issues.
Consider using React's useState hook:
-const toggleInfo = () => {
- const infoDiv = document.getElementById('composition-generated');
-
- if (!infoDiv) {
- return null;
- }
-
- if (infoDiv.style.display === 'none') {
- infoDiv.style.display = 'block';
- } else {
- infoDiv.style.display = 'none';
- }
- return null;
-};
+const [isVisible, setIsVisible] = useState(false);
+const toggleInfo = () => setIsVisible(!isVisible);Committable suggestion was skipped due to low confidence.
| <div | ||
| dangerouslySetInnerHTML={{ __html: text.div }} | ||
| className="extra-info" | ||
| id="composition-generated" | ||
| style={{ display: 'none' }} | ||
| /> |
There was a problem hiding this comment.
Security Risk: Unsafe HTML rendering.
Using dangerouslySetInnerHTML with unsanitized content exposes the application to XSS attacks.
Consider using a sanitization library like DOMPurify:
+import DOMPurify from 'dompurify';
-<div
- dangerouslySetInnerHTML={{ __html: text.div }}
- className="extra-info"
- id="composition-generated"
- style={{ display: 'none' }}
-/>
+<div
+ dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(text.div) }}
+ className="extra-info"
+ id="composition-generated"
+ style={{ display: isVisible ? 'block' : 'none' }}
+/>Would you like me to help set up DOMPurify in your project?
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 566-566: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/tests-build-push.yml (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
.github/workflows/tests-build-push.yml (2)
Line range hint
56-64: LGTM! Well-configured multi-platform build for main branch.The step correctly handles:
- Branch condition
- Multi-platform build
- Appropriate tagging strategy
65-72: The 'hotfix' tag issue remains unaddressedThe static 'hotfix' tag issue raised in the previous review is still applicable. Using a static tag for all non-main branches could lead to tag conflicts and traceability issues.
Let's verify if this tag is being used elsewhere in the codebase:
#!/bin/bash # Search for other occurrences of the hotfix tag rg -i "hotfix" --type yaml
| # if: ${{ github.ref_name == 'main' }} | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| username: ${{ secrets.DOCKERHUB_USERNAME }} | ||
| password: ${{ secrets.DOCKERHUB_TOKEN }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize DockerHub login condition
The login step should only execute when we need to push images. Consider using this condition instead:
- # if: ${{ github.ref_name == 'main' }}
+ if: ${{ github.ref_name == 'main' || github.event_name == 'pull_request' }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # if: ${{ github.ref_name == 'main' }} | |
| uses: docker/login-action@v3 | |
| with: | |
| username: ${{ secrets.DOCKERHUB_USERNAME }} | |
| password: ${{ secrets.DOCKERHUB_TOKEN }} | |
| if: ${{ github.ref_name == 'main' || github.event_name == 'pull_request' }} | |
| uses: docker/login-action@v3 | |
| with: | |
| username: ${{ secrets.DOCKERHUB_USERNAME }} | |
| password: ${{ secrets.DOCKERHUB_TOKEN }} | |
Summary by CodeRabbit
New Features
Bug Fixes
Chores