Conversation
rotemergty5
requested review from
GilHeller and
Tamir198
and removed request for
Tamir198
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new PrintQR page component and styles, renders a QRGeneration component with hardcoded credentials and a localized button, and registers a new ChangesPrintQR Page Feature
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Router
participant PrintQR
participant SGLCard
participant QRGeneration
Browser->>Router: navigate to /printQR
Router->>PrintQR: render PrintQR component
PrintQR->>SGLCard: render card (cardContainer)
SGLCard->>QRGeneration: render QRGeneration token="mock-token"
PrintQR->>SGLCard: render credentials and localized button
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/UI/Select/SGLSelect.test.tsx`:
- Around line 33-45: The test currently only checks that "Banana" isn't
selected, which doesn't verify Escape closed the menu; update the assertion to
confirm the dropdown menu/listbox is dismissed after pressing Escape by querying
for the listbox/options (e.g., using queryByRole('listbox') or
queryAllByRole('option')) and asserting they are not present (e.g.,
expect(queryByRole('listbox')).not.toBeInTheDocument()) after
fireEvent.keyDown(select, { key: 'Escape' }); keep the same render and
interactions around SGLSelect and the select variable so the change is minimal.
In `@src/main.tsx`:
- Line 25: The /printQR route is registered publicly (path: '/printQR', element:
<PrintQR />) and exposes sensitive patient credentials; move this route into the
ProtectedRoute children (or wrap its element with the same authentication guard
used by ProtectedRoute) so PrintQR is only reachable after authentication.
Locate the routes array where ProtectedRoute is defined and add the '/printQR'
entry alongside the other protected child routes (or replace its element with
<ProtectedRoute><PrintQR/></ProtectedRoute>) to enforce auth.
In `@src/pages/QRPrint/PrintQR.tsx`:
- Around line 41-45: The two SGLTypography usages render raw patientNumber and
password without labels and the SGLButton uses t('login.login') with no onClick;
update the UI to show labeled fields (e.g., prepend "Patient No:" and
"Password:" or use distinct SGLTypography for labels) so users can identify
patientNumber and password, and replace or repurpose the SGLButton
(styles.submitButtonContent) to match the page intent—either remove it or change
its text from t('login.login') to a proper action like t('print.print') and wire
an onClick handler (e.g., trigger print or the QR export function) in the
PrintQR component so the button performs the relevant print/export action.
- Around line 11-12: The component currently hardcodes and renders sensitive
values (patientNumber and password) in PrintQR; remove those literal assignments
and instead source them from authenticated state or props (e.g., accept
patientNumber/password as props or read from your auth/context hook like
useAuth/getUser) and ensure the route is protected by ProtectedRoute; also avoid
rendering the raw password—either omit it from UI, mask it, or display a
placeholder, and validate that PrintQR no longer contains hardcoded '123456' or
'add123' literals.
In `@src/pages/QRPrint/styles.ts`:
- Line 22: The style rule contains an invalid declaration "gap: '-1rem'";
replace it with a valid spacing approach by either removing the gap property,
using a small positive gap (e.g., "gap: '0.25rem'") or applying negative margins
on the child elements instead (e.g., adjust child marginTop/marginLeft), and
ensure the declaration for the style rule that currently has "gap: '-1rem'" is
updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 843122b6-1dca-407c-8b5a-161cfe70cefd
📒 Files selected for processing (4)
src/components/UI/Select/SGLSelect.test.tsxsrc/main.tsxsrc/pages/QRPrint/PrintQR.tsxsrc/pages/QRPrint/styles.ts
| it('should not select any item when dropdown is opened and closed', () => { | ||
| render( | ||
| <ThemeProvider theme={theme}> | ||
| <SGLSelect options={options} value="" /> | ||
| </ThemeProvider>, | ||
| ) | ||
|
|
||
| const select = screen.getByRole('combobox') | ||
|
|
||
| fireEvent.mouseDown(select) | ||
| fireEvent.keyDown(select, { key: 'Escape' }) | ||
|
|
||
| expect(select).not.toHaveTextContent('Banana') |
There was a problem hiding this comment.
Assert that Escape actually closes the menu.
This test currently passes even if Escape does nothing, because Banana was never selected in the first place. Please assert the dismissal outcome directly, e.g. that the listbox/options are no longer present after pressing Escape.
Suggested tightening
it('should not select any item when dropdown is opened and closed', () => {
render(
<ThemeProvider theme={theme}>
<SGLSelect options={options} value="" />
</ThemeProvider>,
)
const select = screen.getByRole('combobox')
fireEvent.mouseDown(select)
fireEvent.keyDown(select, { key: 'Escape' })
- expect(select).not.toHaveTextContent('Banana')
+ expect(select).not.toHaveTextContent('Banana')
+ expect(screen.queryByRole('listbox')).not.toBeInTheDocument()
})📝 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.
| it('should not select any item when dropdown is opened and closed', () => { | |
| render( | |
| <ThemeProvider theme={theme}> | |
| <SGLSelect options={options} value="" /> | |
| </ThemeProvider>, | |
| ) | |
| const select = screen.getByRole('combobox') | |
| fireEvent.mouseDown(select) | |
| fireEvent.keyDown(select, { key: 'Escape' }) | |
| expect(select).not.toHaveTextContent('Banana') | |
| it('should not select any item when dropdown is opened and closed', () => { | |
| render( | |
| <ThemeProvider theme={theme}> | |
| <SGLSelect options={options} value="" /> | |
| </ThemeProvider>, | |
| ) | |
| const select = screen.getByRole('combobox') | |
| fireEvent.mouseDown(select) | |
| fireEvent.keyDown(select, { key: 'Escape' }) | |
| expect(select).not.toHaveTextContent('Banana') | |
| expect(screen.queryByRole('listbox')).not.toBeInTheDocument() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/UI/Select/SGLSelect.test.tsx` around lines 33 - 45, The test
currently only checks that "Banana" isn't selected, which doesn't verify Escape
closed the menu; update the assertion to confirm the dropdown menu/listbox is
dismissed after pressing Escape by querying for the listbox/options (e.g., using
queryByRole('listbox') or queryAllByRole('option')) and asserting they are not
present (e.g., expect(queryByRole('listbox')).not.toBeInTheDocument()) after
fireEvent.keyDown(select, { key: 'Escape' }); keep the same render and
interactions around SGLSelect and the select variable so the change is minimal.
| const router = createBrowserRouter([ | ||
| { path: '/', element: <Navigate to="/login" replace /> }, | ||
| { path: '/login', element: <Login /> }, | ||
| { path: '/printQR', element: <PrintQR /> }, |
There was a problem hiding this comment.
/printQR is publicly accessible.
This route is registered alongside /login, outside the ProtectedRoute block (Lines 26–40), so it is reachable without authentication. Since PrintQR displays a patient number and password, this exposes sensitive data to anonymous users. If this page should require auth, move it under the ProtectedRoute children.
Proposed change
- { path: '/printQR', element: <PrintQR /> },
{
element: <ProtectedRoute />,
children: [
{
path: '/',
element: <MainLayout />,
children: [
{ path: 'home', element: <Home /> },
{ path: 'schedule', element: <Schedule /> },
{ path: 'lifeStyle', element: <LifeStyle /> },
{ path: 'dailyReports', element: <DailyReports /> },
+ { path: 'printQR', element: <PrintQR /> },
],
},
],
},📝 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.
| { path: '/printQR', element: <PrintQR /> }, | |
| { | |
| element: <ProtectedRoute />, | |
| children: [ | |
| { | |
| path: '/', | |
| element: <MainLayout />, | |
| children: [ | |
| { path: 'home', element: <Home /> }, | |
| { path: 'schedule', element: <Schedule /> }, | |
| { path: 'lifeStyle', element: <LifeStyle /> }, | |
| { path: 'dailyReports', element: <DailyReports /> }, | |
| { path: 'printQR', element: <PrintQR /> }, | |
| ], | |
| }, | |
| ], | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.tsx` at line 25, The /printQR route is registered publicly (path:
'/printQR', element: <PrintQR />) and exposes sensitive patient credentials;
move this route into the ProtectedRoute children (or wrap its element with the
same authentication guard used by ProtectedRoute) so PrintQR is only reachable
after authentication. Locate the routes array where ProtectedRoute is defined
and add the '/printQR' entry alongside the other protected child routes (or
replace its element with <ProtectedRoute><PrintQR/></ProtectedRoute>) to enforce
auth.
| const patientNumber = '123456' | ||
| const password = 'add123' |
There was a problem hiding this comment.
Hardcoded credentials displayed in the UI.
patientNumber and password are hardcoded and rendered directly on the page (Lines 41–43). Shipping a literal password string in source and showing it as plain text is a security/PII concern, especially given this route is not behind ProtectedRoute (see src/main.tsx Line 25) and is therefore publicly reachable. These values should come from authenticated state/props rather than being baked into the component.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/QRPrint/PrintQR.tsx` around lines 11 - 12, The component currently
hardcodes and renders sensitive values (patientNumber and password) in PrintQR;
remove those literal assignments and instead source them from authenticated
state or props (e.g., accept patientNumber/password as props or read from your
auth/context hook like useAuth/getUser) and ensure the route is protected by
ProtectedRoute; also avoid rendering the raw password—either omit it from UI,
mask it, or display a placeholder, and validate that PrintQR no longer contains
hardcoded '123456' or 'add123' literals.
| <SGLTypography styles={styles.textStyle}>{patientNumber}</SGLTypography> | ||
|
|
||
| <SGLTypography styles={styles.textStyle}>{password}</SGLTypography> | ||
|
|
||
| <SGLButton styles={styles.submitButtonContent}>{t('login.login')}</SGLButton> |
There was a problem hiding this comment.
Unlabeled values and a misleading button.
The two SGLTypography lines render bare values with no labels, so a user cannot tell which is the patient number and which is the password. Additionally, the button uses t('login.login') ("Login") on a print/QR page and has no onClick handler, so it neither matches the page intent nor performs any action. Consider labeling each field and either wiring up a print/relevant action or removing the button.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/QRPrint/PrintQR.tsx` around lines 41 - 45, The two SGLTypography
usages render raw patientNumber and password without labels and the SGLButton
uses t('login.login') with no onClick; update the UI to show labeled fields
(e.g., prepend "Patient No:" and "Password:" or use distinct SGLTypography for
labels) so users can identify patientNumber and password, and replace or
repurpose the SGLButton (styles.submitButtonContent) to match the page
intent—either remove it or change its text from t('login.login') to a proper
action like t('print.print') and wire an onClick handler (e.g., trigger print or
the QR export function) in the PrintQR component so the button performs the
relevant print/export action.
| cleanup() | ||
| }) | ||
|
|
||
| describe('SGLSelect', () => { |
There was a problem hiding this comment.
I dont think that those changes are related to this pr, fix it
There was a problem hiding this comment.
Fixed, removed the unrelated SGLSelect changes from this PR.
There was a problem hiding this comment.
Fixed, removed the unrelated SGLSelect changes.
|
|
||
| return ( | ||
| <div | ||
| style={{ |
| const { t } = useTranslation() | ||
|
|
||
| const patientNumber = '123456' | ||
| const password = 'add123' |
There was a problem hiding this comment.
If this is a mock user you should call your variables MOCK_PATIENT_NUMBER and MOCK_PASSWORD
| minHeight: '100vh', | ||
| }} | ||
| > | ||
| <div |
| <SGLCard> | ||
| <div style={styles.containerStyle}> | ||
| <SGLImage | ||
| src="https://www.dangot.com/Media/Uploads/2D-BAROCDE(1).png" |
There was a problem hiding this comment.
Who told you to create image of qr code? we already have a code to generate qr code. what you are doing right now is not dynamic or scalable
There was a problem hiding this comment.
The implementation was based on the task requirements I received, which explicitly included using an image component (SGLImage) as part of the PrintQR page.
I found the existing QRGeneration component and can update the implementation to use it instead.
However, QRGeneration currently requires a token, and this page doesn't have a token generation flow yet. Would you like me to use a mock token for now, or should I connect it to the actual QR login flow?
|
@rotemergty5 There are a few comments from @Tamir198 that haven't been addressed yet. Please take a look and update accordingly. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/QRPrint/styles.ts`:
- Around line 12-14: The cardContainer style currently forces width: '400px'
which overflows on narrow screens; update the cardContainer CSSProperties (the
exported cardContainer object) to use a responsive sizing pattern such as width:
'100%' with maxWidth: '400px' and include boxSizing: 'border-box' so the element
scales down on small viewports while keeping the 400px target on larger screens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 219d0f42-36de-4f70-8e7d-b82d2e57d157
📒 Files selected for processing (2)
src/pages/QRPrint/PrintQR.tsxsrc/pages/QRPrint/styles.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/QRPrint/PrintQR.tsx
| export const cardContainer: CSSProperties = { | ||
| width: '400px', | ||
| } |
There was a problem hiding this comment.
Make the card responsive on narrow screens.
width: '400px' can cause horizontal overflow on mobile widths below 400px. Keep the target size but cap it responsively.
Proposed fix
export const cardContainer: CSSProperties = {
- width: '400px',
+ width: '400px',
+ maxWidth: '100%',
}📝 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.
| export const cardContainer: CSSProperties = { | |
| width: '400px', | |
| } | |
| export const cardContainer: CSSProperties = { | |
| width: '400px', | |
| maxWidth: '100%', | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/QRPrint/styles.ts` around lines 12 - 14, The cardContainer style
currently forces width: '400px' which overflows on narrow screens; update the
cardContainer CSSProperties (the exported cardContainer object) to use a
responsive sizing pattern such as width: '100%' with maxWidth: '400px' and
include boxSizing: 'border-box' so the element scales down on small viewports
while keeping the 400px target on larger screens.
|
I went through all of Tamir's comments and updated the PR accordingly. |
| const { t } = useTranslation() | ||
|
|
||
| const patientNumber = '123456' | ||
| const password = 'add123' |
3 participants
Created the PrintQR page and its styling, built the UI using SGLCard, SGLButton, SGLTypography and SGLImage, and added a React Router DOM route to allow navigation to the PrintQR page.
Summary by CodeRabbit