Skip to content
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.

Sign up for GitHub

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

Minor fixes #107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Minor fixes #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72c0930
temp: add build push for hotfix
BMartinos Oct 23, 2024
3caba78
Minor fixes
BMartinos Oct 23, 2024
1e49db6
Login into dockerhub to push image
BMartinos Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions .github/workflows/tests-build-push.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ jobs:
file: docker/production/Dockerfile

- name: Login to DockerHub
if: ${{ github.ref_name == 'main' }}
# if: ${{ github.ref_name == 'main' }}
uses: docker/login-action@v3
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

Comment on lines +50 to +55
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
# 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 }}

- name: Build and push latest
if: ${{ github.ref_name == 'main' }}
uses: docker/build-push-action@v5
Expand All @@ -61,3 +61,12 @@ jobs:
push: true
file: docker/production/Dockerfile
tags: jembi/smart-health-links-portal:latest

- name: Build and push 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
53 changes: 53 additions & 0 deletions src/app/components/PatientSummaryViewer/resources/Composition/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import Box from '@mui/material/Box';
import { JSX } from 'react';

import { StyledButton } from '@/app/components/StyledButton';
import {
camelCaseToFlat,
getSection,
Expand Down Expand Up @@ -520,6 +522,57 @@ const rows: TRow<TComposition>[] = [
) as JSX.Element[][],
},
},
{
type: 'row',
config: {
label: '',
render: ({ text }) => {
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;
};
Comment on lines +530 to +543
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


return (
<Box
sx={{
padding: '20px 0px',
width: '100%',
'& table': {
width: '100%',
borderCollapse: 'collapse',
marginBottom: '20px',
'& th, & td': {
border: '1px solid gray',
padding: '8px',
textAlign: 'left',
},
},
}}
>
Comment on lines +546 to +561
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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>
Comment on lines +562 to +564
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

<div
dangerouslySetInnerHTML={{ __html: text.div }}
className="extra-info"
id="composition-generated"
style={{ display: 'none' }}
/>
Comment on lines +565 to +570
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)

</Box>
);
},
},
},
];

export const Composition = (props: Omit<TTabProps<TComposition>, 'rows'>) => (
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/PatientSummaryViewer/resources/Immunization/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const rows: TRow<TImmunization>[] = [
render: ({ performer }, references) =>
getResource<EResource.Organization>(
references,
performer?.[0]?.actor.reference,
performer?.[0]?.actor?.reference,
)?.name,
},
},
Expand Down
Loading