Fixed:Show more monitors in CNS on very large displays #6503 #9013
Conversation
WalkthroughThe changes made to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/CentralNursingStation.tsx (3)
215-215: LGTM! Consider additional improvements for ultra-wide displays.The change from '3xl' to 'min-[2080px]' effectively addresses the screen space utilization issue on 4K displays. The explicit pixel breakpoint provides more precise control over the layout transition.
Consider these additional improvements:
- <div className="mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2 min-[2080px]:grid-cols-3"> + <div className="mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2 min-[2080px]:grid-cols-3 min-[3440px]:grid-cols-4">This would better utilize ultra-wide displays (e.g., 3440x1440) while maintaining the current improvements for 4K displays.
Line range hint
19-19: Consider dynamic pagination limits for larger displays.The fixed
PER_PAGE_LIMITof 6 items might not be optimal for larger displays that can now show more monitors. When displaying 3 or 4 columns, this results in partially filled rows.Consider implementing dynamic pagination limits based on screen size:
// Example implementation const getPerPageLimit = (screenWidth: number) => { if (screenWidth >= 3440) return 12; // 4 columns x 3 rows if (screenWidth >= 2080) return 9; // 3 columns x 3 rows if (screenWidth >= 1024) return 6; // 2 columns x 3 rows return 4; // 1 column x 4 rows };
Line range hint
63-71: Update aspect ratio configuration to match new breakpoint.The aspect ratio configuration uses different breakpoints than the grid layout. This mismatch could lead to suboptimal monitor display proportions on larger screens.
Update the aspect ratio configuration to match the new breakpoint:
const { config, hash } = useVitalsAspectRatioConfig({ default: 6 / 11, sm: 17 / 11, md: 19 / 11, lg: 11 / 11, xl: 13 / 11, "2xl": 16 / 11, - "3xl": 12 / 11, + "min-[2080px]": 12 / 11, + "min-[3440px]": 14 / 11, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/components/Facility/CentralNursingStation.tsx(1 hunks)
|
@i0am0arunava please close the previous PR before picking something else. |
@bodhish ,But I am working on the previous issue |
|
👋 Hi, @i0am0arunava, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (20)
cypress/e2e/patient_spec/PatientDoctorConnect.cy.ts (3)
Line range hint
1-13: Consider extracting test data to fixtures.The hardcoded test data (patient name, user names) should be moved to Cypress fixtures for better maintainability and reusability.
+ import testData from '../../fixtures/doctorConnect.json'; const loginPage = new LoginPage(); const patientPage = new PatientPage(); const doctorconnect = new DoctorConnect(); - const patientName = "Dummy Patient 11"; - const doctorUser = "Dev Doctor"; - const nurseUser = "Dev Staff"; - const teleIcuUser = "Dev Doctor Two"; + const { patientName, doctorUser, nurseUser, teleIcuUser } = testData;
Line range hint
15-24: Enhance URL verification in beforeEach hook.The
cy.awaitUrl("/patients")could be more specific to ensure we're on the exact intended page.beforeEach(() => { cy.restoreLocalStorage(); cy.clearLocalStorage(/filters--.+/); - cy.awaitUrl("/patients"); + cy.awaitUrl("/patients").should("include", "/patients"); + cy.url().should("match", /\/patients$/); });
Line range hint
52-54: Enhance test cleanup.Consider adding more comprehensive cleanup steps to ensure a clean state for subsequent tests.
afterEach(() => { cy.saveLocalStorage(); + // Reset any modified application state + cy.window().then((win) => { + win.sessionStorage.clear(); + }); + // Clear any intercepted requests + cy.clearAllCookies(); });cypress/e2e/patient_spec/PatientFileUpload.ts (4)
Line range hint
9-24: LGTM! Consider extracting test data to fixtures.The test setup and structure follow Cypress best practices. However, the test data could be moved to fixture files for better maintainability.
Consider creating a fixture file:
// cypress/fixtures/patient-file-upload.json { "audioName": "cypress audio", "fileName": "cypress name", "newFileName": "cypress modified name", "patients": { "one": "Dummy Patient 3", "two": "Dummy Patient 4", "three": "Dummy Patient 5" } }
Line range hint
40-57: Verify archived state persistence.The test verifies the archive action but doesn't confirm if the archived state persists after page reload.
Add persistence verification:
patientFileUpload.clickSaveArchiveFile(); cy.verifyNotification("File archived successfully"); patientFileUpload.verifyArchiveFile(cypressFileName); +// Verify persistence +cy.reload(); +patientFileUpload.verifyArchiveFile(cypressFileName);
Line range hint
58-106: Improve test reliability and security.Several improvements could enhance this test:
Hardcoded password should be moved to environment variables or fixtures
Multiple user switches could cause flakiness
Move credentials to cypress.env.json:
{ "nurse_password": "Coronasafe@123" }
- Refactor the test to use separate test cases:
it("allows file upload and rename by owner (Nurse 1)", () => { loginPage.login("dummynurse1", Cypress.env("nurse_password")); // Test upload and rename }); it("prevents file rename by non-owner (Nurse 2)", () => { loginPage.login("dummynurse2", Cypress.env("nurse_password")); // Test rename prevention }); it("allows file rename by admin", () => { loginPage.loginAsDistrictAdmin(); // Test admin rename });
Line range hint
107-116: Document the differences between test scenarios.While reusing tests is efficient, it would be helpful to document the expected differences between patient details and consultation page file uploads.
Add comments explaining the context:
runTests( "Patient File upload in patient details page", + // Tests file upload functionality in the main patient details view patientFileUpload.clickFileUploadIcon, ); runTests( "Patient File upload in patient consultation page", + // Tests file upload functionality within the consultation workflow patientFileUpload.clickFileTab, );cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
2-5: Consider organizing imports by type and source.The imports could be better organized by grouping them:
- Page objects (LoginPage, FacilityPage, etc.)
- Utility classes (AssetPagination)
Consider reorganizing the imports like this:
-// FacilityCreation -import { AssetPagination } from "../../pageobject/Asset/AssetPagination"; -import FacilityPage from "../../pageobject/Facility/FacilityCreation"; -import FacilityHome from "../../pageobject/Facility/FacilityHome"; -import LoginPage from "../../pageobject/Login/LoginPage"; +// Page Objects +import FacilityPage from "../../pageobject/Facility/FacilityCreation"; +import FacilityHome from "../../pageobject/Facility/FacilityHome"; +import LoginPage from "../../pageobject/Login/LoginPage"; + +// Utilities +import { AssetPagination } from "../../pageobject/Asset/AssetPagination";
Line range hint
41-63: Add test coverage for CNS display on large screens.Given that this PR addresses the display of monitors in CNS on large 4K screens (issue #6503), consider adding specific tests to verify this functionality:
- Test the number of monitors displayed on different viewport sizes
- Verify proper utilization of screen space
Consider adding these test cases:
it("Verify CNS monitor display on large screens", () => { // Set viewport to 4K resolution cy.viewport(3840, 2160); facilityHome.clickViewCnsButton(); // Verify more monitors are displayed // Add assertions for monitor count and layout // Test different viewport sizes cy.viewport(1920, 1080); // Add assertions for responsive behavior });cypress/e2e/users_spec/UsersManage.cy.ts (3)
Line range hint
52-61: Replace hard-coded wait with proper Cypress commands.Using
cy.wait(5000)is not recommended as it makes tests brittle and slower than necessary. Consider using Cypress's built-in retry-ability and assertions.Replace the wait with proper assertions:
manageUserPage.clickAddSkillButton(); manageUserPage.clickCloseSlideOver(); - cy.wait(5000); + // Wait for the UI to reflect the changes + cy.get('[data-testid="linked-skills"]').should('exist'); manageUserPage.clicklinkedskillbutton(); manageUserPage.assertSkillInAddedUserSkills(linkedskill); manageUserPage.clickCloseSlideOver(); - cy.wait(5000); + // Wait for navigation to complete + cy.url().should('include', '/users'); manageUserPage.navigateToProfile();
Line range hint
11-17: Consider moving test data to fixtures.Test data like usernames and facility names should be moved to a fixture file for better maintainability and reusability.
Create a new fixture file
cypress/fixtures/users.json:{ "users": { "doctorWithSkills": { "username": "devdoctor", "realName": "Dummy Doctor" }, "doctorsForFacility": { "doctor1": "dummydoctor4", "doctor2": "dummydoctor5", "doctor3": "dummydoctor6" } }, "facilities": { "shiftingCenter": "Dummy Shifting Center", "dummyFacility": "Dummy Facility 40" } }Then update the test to use the fixture:
describe("Manage User", () => { let testData: any; before(() => { cy.fixture('users').then((data) => { testData = data; }); // ... rest of the setup }); // Update variables to use testData const usernametolinkfacilitydoc1 = testData.users.doctorsForFacility.doctor1; // ... rest of the code });
Line range hint
52-156: Extract common patterns into helper functions.There are repeated patterns in the test cases that could be extracted into helper functions for better maintainability and readability.
Create helper functions in a support file:
// cypress/support/helpers/user-management.ts export const linkSkillToUser = (username: string, skill: string) => { cy.log(`Linking skill ${skill} to user ${username}`); userPage.typeInSearchInput(username); userPage.checkUsernameText(username); manageUserPage.clicklinkedskillbutton(); manageUserPage.selectSkillFromDropdown(skill); manageUserPage.clickAddSkillButton(); manageUserPage.clickCloseSlideOver(); }; export const verifySkillInProfile = (username: string, skill: string) => { cy.log(`Verifying skill ${skill} in profile for user ${username}`); manageUserPage.navigateToProfile(); userCreationPage.verifyElementContainsText( "username-profile-details", username ); manageUserPage.assertSkillInAlreadyLinkedSkills(skill); };Then update the test cases to use these helpers:
it("linking skills for users and verify its reflection in profile", () => { linkSkillToUser(usernameforworkinghour, linkedskill); verifySkillInProfile(usernameforworkinghour, linkedskill); });cypress/e2e/patient_spec/PatientPrescription.cy.ts (4)
Line range hint
15-31: Consider enhancing URL verification robustness.While the current setup is good, consider adding timeout and error handling for the URL verification:
- cy.awaitUrl("/patients"); + cy.awaitUrl("/patients", { timeout: 10000 }).should('include', '/patients').then(url => { + if (!url.includes('/patients')) { + throw new Error('Failed to navigate to patients page'); + } + });
Line range hint
33-85: Consider improving test stability for medicine administration workflow.The test case is well-structured but could be more robust:
- cy.wait(2000); + cy.get('[data-testid="administer-form"]').should('be.visible');Also, consider adding retry ability for critical operations:
- cy.submitButton("Submit"); + cy.submitButton("Submit").should(() => { + // Verify the submission was successful + expect(localStorage.getItem('lastPrescription')).to.exist; + });
Line range hint
87-127: Enhance PRN prescription test data isolation.The test relies on specific patient data ("Dummy Patient 6"). Consider making the test data setup more explicit:
// Add at the top of the file const TEST_PATIENTS = { PRN_PATIENT: { name: "Dummy Patient 6", id: "...", // Add relevant ID // Add other required data } };Then use this constant in the test:
- patientPage.visitPatient("Dummy Patient 6"); + patientPage.visitPatient(TEST_PATIENTS.PRN_PATIENT.name);
Line range hint
168-197: Enhance duplicate medicine validation test.The duplicate medicine validation test is well-structured but could benefit from additional edge cases:
Consider adding tests for:
- Case-insensitive medicine name validation
- Different dosage but same medicine
- Discontinued medicine re-prescription
Example:
it("should handle case-insensitive duplicate medicine validation", () => { // ... setup code ... patientPrescription.selectMedicine(medicineNameOne.toLowerCase()); // ... verify duplicate detection ... });cypress/e2e/patient_spec/PatientRegistration.cy.ts (1)
Line range hint
33-320: Consider these test suite improvements.While the test implementation is solid, here are some suggestions to enhance maintainability:
- Break down long test cases into smaller, focused ones using
beforeEachfor common setup- Extract magic strings (like notification messages) into constants
- Replace fixed
cy.wait()calls with more robust waiting patterns usingcy.intercept()Example of improved waiting pattern:
-cy.wait(3000); +cy.intercept('GET', '/api/v1/patient/*').as('getPatient'); +cy.wait('@getPatient');Example of extracting constants:
const NOTIFICATIONS = { TRANSFER_SUCCESS: "Patient Dummy Patient 10 (Male) transferred successfully", TRANSFER_ERROR: "Patient - Patient transfer cannot be completed because the patient has an active consultation in the same facility" };cypress/e2e/patient_spec/PatientLogUpdate.cy.ts (3)
Line range hint
8-324: Consider improving test data management and assertions.While the test cases are comprehensive, here are some suggestions for improvement:
- Extract test data to fixtures or a separate configuration file
- Add more assertions between critical steps
- Consider using custom commands for repetitive patterns
Example refactor for test data:
// fixtures/patient-log-data.json { "patients": { "domiciliary": { "name": "Dummy Patient 11", "category": "Domiciliary Care" }, "normal": { "name": "Dummy Patient 9", "bed": "Dummy Bed 5" } }, "vitals": { "systolic": "149", "diastolic": "119", "pulse": "152" // ... other vitals } }Example custom command for repetitive patterns:
// support/commands.ts Cypress.Commands.add('fillVitals', (vitals) => { const patientLogupdate = new PatientLogupdate(); patientLogupdate.typeSystolic(vitals.systolic); patientLogupdate.typeDiastolic(vitals.diastolic); patientLogupdate.typePulse(vitals.pulse); // ... other vitals });
Line range hint
66-93: Add assertions for form state between steps.In the TeleIcu log update test, consider adding assertions to verify form state after each significant step:
patientLogupdate.typePhysicalExamination(physicalExamination); +cy.get('#physical_examination').should('have.value', physicalExamination); patientLogupdate.selectRoundType("Tele-medicine Log"); +cy.get('[data-testid="round-type"]').should('contain', 'Tele-medicine Log'); patientLogupdate.selectPatientCategory(patientCategory); +cy.get('[data-testid="patient-category"]').should('contain', patientCategory);
Line range hint
285-324: Consider splitting MEWS Score test cases.The MEWS Score functionality test case is testing multiple scenarios. Consider splitting it into separate test cases for better clarity:
it("should display correct MEWS Score when all required fields are filled", () => { // Test complete data scenario }); it("should display dash when MEWS Score cannot be calculated", () => { // Test incomplete data scenario });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
cypress/e2e/assets_spec/AssetHomepage.cy.ts(1 hunks)cypress/e2e/assets_spec/AssetsCreation.cy.ts(1 hunks)cypress/e2e/assets_spec/AssetsManage.cy.ts(1 hunks)cypress/e2e/facility_spec/FacilityCreation.cy.ts(1 hunks)cypress/e2e/facility_spec/FacilityHomepage.cy.ts(1 hunks)cypress/e2e/facility_spec/FacilityInventory.cy.ts(1 hunks)cypress/e2e/facility_spec/FacilityManage.cy.ts(1 hunks)cypress/e2e/patient_spec/PatientBedManagement.cy.ts(1 hunks)cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts(1 hunks)cypress/e2e/patient_spec/PatientDoctorConnect.cy.ts(1 hunks)cypress/e2e/patient_spec/PatientFileUpload.ts(1 hunks)cypress/e2e/patient_spec/PatientLogUpdate.cy.ts(1 hunks)cypress/e2e/patient_spec/PatientPrescription.cy.ts(1 hunks)cypress/e2e/patient_spec/PatientRegistration.cy.ts(1 hunks)cypress/e2e/resource_spec/ResourcesHomepage.cy.ts(1 hunks)cypress/e2e/users_spec/UsersCreation.cy.ts(1 hunks)cypress/e2e/users_spec/UsersManage.cy.ts(1 hunks)src/components/Facility/CentralNursingStation.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (9)
- cypress/e2e/assets_spec/AssetHomepage.cy.ts
- cypress/e2e/assets_spec/AssetsCreation.cy.ts
- cypress/e2e/assets_spec/AssetsManage.cy.ts
- cypress/e2e/facility_spec/FacilityCreation.cy.ts
- cypress/e2e/facility_spec/FacilityInventory.cy.ts
- cypress/e2e/facility_spec/FacilityManage.cy.ts
- cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts
- cypress/e2e/resource_spec/ResourcesHomepage.cy.ts
- cypress/e2e/users_spec/UsersCreation.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/CentralNursingStation.tsx
🔇 Additional comments (11)
cypress/e2e/patient_spec/PatientBedManagement.cy.ts (1)
2-2: Consider adding CNS display tests.
While the import change is valid, this test file focuses on bed management functionality but doesn't cover the CNS display improvements mentioned in the PR objectives. Consider adding test cases to verify that:
- More monitors are displayed on large screens
- The layout adapts correctly to different screen sizes
Here's a script to check if there are any existing CNS display tests:
cypress/e2e/patient_spec/PatientDoctorConnect.cy.ts (1)
Line range hint 26-50: Split test into smaller, focused test cases.
The current test case covers multiple functionalities (user visibility, copy functionality, icon presence, sorting). This makes it harder to maintain and debug failures. Consider splitting it into separate test cases.
it("should display all user types in their respective sections", () => {
patientPage.visitPatient(patientName);
doctorconnect.clickDoctorConnectButton();
cy.verifyContentPresence("#doctor-connect-home-doctor", [doctorUser]);
cy.verifyContentPresence("#doctor-connect-home-nurse", [nurseUser]);
cy.verifyContentPresence("#doctor-connect-remote-doctor", [teleIcuUser]);
});
it("should copy phone number correctly", () => {
doctorconnect.CopyFunctionTrigger();
doctorconnect.clickCopyPhoneNumber("#doctor-connect-home-doctor", doctorUser);
doctorconnect.verifyCopiedContent();
});
// ... more focused test casesAdd error handling test scenarios.
The test suite should include scenarios for:
- Empty or invalid phone numbers
- Network failures during copy operations
- Missing user data
✅ Verification successful
Let me search for the file first since the path seems incorrect. This will help us locate the relevant files and then analyze them for error handling.
Now let's check for error handling patterns and the implementation details to make a more informed decision about the review comment.
Split test cases and add error scenarios for better test coverage
The review comment's suggestions about splitting the test cases and adding error scenarios are valid based on the following findings:
- No error handling patterns were found in either the page object or test file
- The test combines multiple distinct functionalities in a single test case
- Edge cases and error scenarios are not covered
Recommendations:
- Split the test into smaller, focused test cases as suggested in the review
- Add test cases for error scenarios:
- Empty/invalid phone numbers
- Copy operation failures
- Missing user data
- Network errors during data fetch
Note: The verification confirms the original review comment's concerns about test organization and missing error scenarios. The suggested refactoring and additional test cases would improve the test suite's maintainability and robustness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if error handling exists in the page object
rg -A 5 "catch|error|fail" "pageobject/Patient/PatientDoctorConnect.ts"
Length of output: 154
Script:
#!/bin/bash
# Find the correct path for the page object file
fd -t f "PatientDoctorConnect"
# Search for test files related to doctor connect functionality
fd -t f "DoctorConnect.*\.(ts|js|tsx|jsx)$"
Length of output: 282
Script:
#!/bin/bash
# Check error handling in the correct page object path
rg -A 5 "catch|error|fail" "cypress/pageobject/Patient/PatientDoctorConnect.ts"
# Look for error scenarios in the test file
rg -A 5 "error|fail|invalid|empty" "cypress/e2e/patient_spec/PatientDoctorConnect.cy.ts"
# Check the implementation of copy functionality
ast-grep --pattern 'CopyFunctionTrigger() {
$$$
}'
Length of output: 223
cypress/e2e/patient_spec/PatientFileUpload.ts (1)
Line range hint 25-39: Enhance download verification.
The test verifies the download notification but doesn't confirm if the file was actually downloaded successfully.
Consider adding file existence checks:
cy.get("button").contains("Download").click();
cy.verifyNotification("Downloading file...");
+cy.readFile(`cypress/downloads/${cypressAudioName}.mp3`).should('exist');cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
2-2: Verify AssetPagination usage across the codebase.
The newly added AssetPagination import is used in the pagination tests. Let's verify if this is consistent with other test files.
✅ Verification successful
AssetPagination is consistently used across related test files
The verification shows that AssetPagination is imported and instantiated consistently in three test files:
cypress/e2e/facility_spec/FacilityHomepage.cy.tscypress/e2e/facility_spec/FacilityLocation.cy.tscypress/e2e/assets_spec/AssetHomepage.cy.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if AssetPagination is used consistently across test files
# Expected: Find other test files using AssetPagination for pagination
# Search for files importing AssetPagination
rg -l "import.*AssetPagination" "cypress/e2e"
# Search for usage patterns of AssetPagination
ast-grep --pattern 'new AssetPagination()'
Length of output: 530
cypress/e2e/users_spec/UsersManage.cy.ts (1)
4-4: LGTM! Import statement is correctly placed and necessary.
The import of UserPage is properly structured and aligns with the module's usage throughout the test suite.
cypress/e2e/patient_spec/PatientPrescription.cy.ts (2)
Line range hint 1-13: LGTM! Well-structured test setup.
The import statements and test data setup follow good practices with:
- Clear page object pattern implementation
- Well-defined constants for test data
- Proper instantiation of required objects
Line range hint 129-166: Add verification for titrated dosage calculations.
The titrated medicine administration test should verify the dosage calculations:
Add assertions to verify the dosage calculations:
patientPrescription.enterTargetDosage(medicineTargetDosage);
+ // Verify the calculated dosage is within expected range
+ cy.get('[data-testid="calculated-dosage"]')
+ .should('exist')
+ .and(($el) => {
+ const calculatedDosage = parseFloat($el.text());
+ expect(calculatedDosage).to.be.within(
+ parseFloat(medicineBaseDosage),
+ parseFloat(medicineTargetDosage)
+ );
+ });cypress/e2e/patient_spec/PatientRegistration.cy.ts (2)
7-7: Good practice: Using generated phone numbers.
Using generatePhoneNumber utility helps prevent test flakiness by ensuring unique phone numbers across test runs.
Line range hint 1-320:
While the changes to the patient registration tests are well-implemented, they seem unrelated to the PR's stated objective of fixing CNS display issues on large screens. Consider:
- Moving these changes to a separate PR focused on test improvements
- Keeping this PR focused on the CNS display fixes
- Adding tests specific to the CNS display functionality
Let's verify if there are any CNS-related test files that should be modified instead:
cypress/e2e/patient_spec/PatientLogUpdate.cy.ts (2)
5-5: LGTM! Import statement properly uncommented.
The import of PatientLogupdate is correctly placed alongside other related page object imports.
Line range hint 326-328: LGTM! Proper test cleanup.
The afterEach hook correctly preserves localStorage state between tests.
| import FacilityPage from "../../pageobject/Facility/FacilityCreation"; | ||
| import LoginPage from "../../pageobject/Login/LoginPage"; | ||
| import { PatientPage } from "../../pageobject/Patient/PatientCreation"; | ||
| import FacilityPage from "../../pageobject/Facility/FacilityCreation"; | ||
| import { generatePhoneNumber } from "../../pageobject/utils/constants"; | ||
| import PatientTransfer from "../../pageobject/Patient/PatientTransfer"; | ||
| import PatientInsurance from "../../pageobject/Patient/PatientInsurance"; | ||
| import PatientMedicalHistory from "../../pageobject/Patient/PatientMedicalHistory"; | ||
| import PatientTransfer from "../../pageobject/Patient/PatientTransfer"; | ||
| import { generatePhoneNumber } from "../../pageobject/utils/constants"; |
There was a problem hiding this comment.
💡 Codebase verification
Changes to patient registration tests are unrelated to CNS display improvements
The verification shows that this PR primarily focuses on improving the CNS display for large screens by:
- Replacing manual dimension handling with a proper
useBreakpointshook - Updating the grid layout for 4K displays
- Optimizing the number of monitors shown based on screen size
The changes to PatientRegistration.cy.ts imports appear to be unrelated to these CNS display improvements and should be moved to a separate PR to maintain clear scope and change tracking.
🔗 Analysis chain
LGTM, but verify if these changes belong in this PR.
The import changes are well-structured and the new imports are properly utilized in the test suite. However, these changes appear unrelated to the PR's primary objective of fixing CNS display issues on large screens.
Let's check if there are related changes in other files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related changes to verify PR scope
# Look for CNS-related changes that this PR should be focusing on
rg -l "CentralNursingStation|CNS" --type ts --type tsx
Length of output: 84
Script:
#!/bin/bash
# Let's try a different approach to find CNS-related files and changes
# First, let's search without type restrictions
rg -l "CentralNursingStation|CNS"
# Also search for monitor-related changes since PR is about large displays
rg -l "monitor|display" --type ts
Length of output: 3566
Script:
#!/bin/bash
# Let's check the actual changes in CNS-related files to understand the PR scope
git diff HEAD~1 src/components/Facility/CentralNursingStation.tsx
# Also check if there are any changes to the patient registration test file
git diff HEAD~1 cypress/e2e/patient_spec/PatientRegistration.cy.ts
Length of output: 2600
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/hooks/useBreakpoints.ts (2)
3-3: Consider using consistent naming convention for breakpoints.The new "fourK" breakpoint name deviates from the existing numeric prefix pattern (sm, md, lg, xl, 2xl, 3xl). Additionally, 2080px isn't technically 4K resolution (3840×2160).
Consider using "4xl" instead to maintain consistency:
-type Breakpoints = "sm" | "md" | "lg" | "xl" | "2xl" | "3xl" | "fourK"; +type Breakpoints = "sm" | "md" | "lg" | "xl" | "2xl" | "3xl" | "4xl";
Line range hint
21-35: Consider memoization for performance optimization.The hook recalculates breakpoints on every window dimension change. While this is generally fine, for performance-critical components like the CNS that display multiple monitors, consider memoizing the result.
Consider using
useMemo:export default function useBreakpoints<T>( map: Partial<Record<Breakpoints, T>> & { default: T }, ) { const { width } = useWindowDimensions(); - const breakpoint = Object.entries(map).reduce((acc, [key]) => { + const breakpoint = useMemo(() => Object.entries(map).reduce((acc, [key]) => { if (width >= BREAKPOINT_WIDTH[key as Breakpoints]) { return key as Breakpoints; } return acc; - }, "default" as Breakpoints); + }, "default" as Breakpoints), [width, map]); return map[breakpoint] ?? map.default; }tailwind.config.js (1)
25-25: Fix typo in comment.There's a typo in the comment: "insted" should be "instead".
- fourK: "130rem", // rem is used insted of pixel + fourK: "130rem", // rem is used instead of pixels
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/CentralNursingStation.tsx(3 hunks)src/hooks/useBreakpoints.ts(2 hunks)tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/CentralNursingStation.tsx
🔇 Additional comments (3)
src/hooks/useBreakpoints.ts (1)
13-13: Verify if 2080px breakpoint effectively solves the 4K display issue.
The chosen breakpoint of 2080px seems arbitrary and might not be optimal for 4K displays (3840×2160). This could result in the CNS still not utilizing the full screen space effectively on actual 4K monitors.
Consider:
- Using a breakpoint closer to actual 4K width (3840px)
- Adding comments explaining why 2080px was chosen
- Testing on actual 4K displays to ensure optimal layout
tailwind.config.js (2)
24-26: LGTM! Verify breakpoint consistency across the codebase.
The new fourK breakpoint at 130rem is well-named and appropriately sized for 4K displays.
Let's verify the breakpoint is consistently defined across the codebase:
✅ Verification successful
The fourK breakpoint is consistently defined and used correctly across the codebase.
The verification shows that:
- The breakpoint is defined in
tailwind.config.jsasfourK: "130rem" - It's properly typed and defined in
useBreakpoints.tsasfourK: 2080(equivalent to 130rem at 16px base font size) - The breakpoint is correctly used in components like
CentralNursingStation.tsxfor responsive layouts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in breakpoint definitions
# Expected: The fourK breakpoint should be defined consistently in useBreakpoints.ts
# Search for breakpoint definitions
rg -A 5 "fourK.*=.*2080" "src/**/*{hooks,utils,constants}*"
Length of output: 134
Script:
#!/bin/bash
# Let's try a broader search for breakpoint-related files and any mentions of fourK
# First, find all potential files
fd -e ts -e tsx -e js
# Then search for fourK or breakpoint definitions
rg -i "fourK|breakpoint"
Length of output: 29352
24-26: Ensure breakpoint addresses the screen space utilization issue.
While the breakpoint addition looks good, consider the following to ensure it fully addresses the screen space utilization issue:
- Test the breakpoint with different monitor resolutions between 2K and 4K to ensure smooth scaling
- Consider adding a larger breakpoint (e.g.,
fiveK) for future-proofing - Document the expected grid layouts at each breakpoint in the component documentation
Let's check the grid layout implementation:
✅ Verification successful
The fourK breakpoint is correctly implemented for grid columns in the Central Nursing Station component
Found evidence in src/components/Facility/CentralNursingStation.tsx:
<div className="mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2 fourK:grid-cols-3">The implementation:
- Uses responsive grid layout starting with 1 column on mobile
- Increases to 2 columns on large screens (lg)
- Expands to 3 columns at the fourK breakpoint (130rem)
This progressive enhancement of columns will help maximize screen space utilization on larger displays while maintaining readability across different screen sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify grid layout implementation
# Expected: The CentralNursingStation component should use the fourK breakpoint for grid columns
# Search for grid-cols classes using the fourK breakpoint
rg "fourK:grid-cols-[0-9]+" "src/**/*.{tsx,jsx}"
Length of output: 109
Script:
#!/bin/bash
# Let's first find the correct source files
fd "\.(tsx|jsx)$"
# Then search for any grid-cols classes
rg "grid-cols-[0-9]+" -A 2
Length of output: 61934
rithviknishad
removed
blocked
Deploy-Failed
|
@rithviknishad @bodhish ,why the deployment is failing .But in my local system build is working properly |
|
now this look like this is it okay now? |
|
issue talks about "4K" displays right. could you show how it looks in 4K? |
should I increase the column |
is it okay if I increase grid col for 4000px 4k display and all the monitors are rendered in the 1st page |
|
Yup we can. I think 6 column 4 rows should be right for 4K. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/CentralNursingStation.tsx (1)
24-24: Overall implementation feedback.The changes effectively address the screen space utilization issue, but there are a few architectural considerations:
- Consider extracting the grid configuration to a dedicated configuration file for better maintainability
- The current implementation might benefit from a more systematic approach to breakpoints
- Add comments explaining the grid layout decisions for future maintenance
The implementation is a good start but needs refinement to match the exact requirements discussed in the PR (6x4 layout for 4K displays).
Also applies to: 43-48, 222-222
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/CentralNursingStation.tsx(3 hunks)src/hooks/useBreakpoints.ts(2 hunks)tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useBreakpoints.ts
- tailwind.config.js
🔇 Additional comments (1)
src/components/Facility/CentralNursingStation.tsx (1)
222-222: Verify tailwind configuration for new breakpoint classes.
The grid layout implementation needs verification to ensure proper configuration of the new breakpoint classes (largedisplay and fourk).
Let's verify the tailwind configuration:
Additionally, consider these improvements:
- Add responsive gap sizes:
gap-1 lg:gap-2 fourk:gap-4 - Consider using CSS Grid's
auto-fitfor more flexible layouts
- <div className="mt-1 grid grid-cols-1 gap-1 lg:grid-cols-2 largedisplay:grid-cols-3 fourk:grid-cols-4">
+ <div className="mt-1 grid grid-cols-1 gap-1 lg:gap-2 fourk:gap-4 lg:grid-cols-2 largedisplay:grid-cols-3 fourk:grid-cols-6">✅ Verification successful
Tailwind breakpoints are properly configured
The largedisplay (130rem) and fourk (175rem) breakpoints are correctly defined in the tailwind configuration. The grid layout implementation in CentralNursingStation.tsx follows a progressive enhancement pattern that's appropriate for the defined breakpoints:
- Mobile: 1 column
- Large screens (lg): 2 columns
- Large displays (130rem): 3 columns
- 4K displays (175rem): 4 columns
The current gap size (gap-1) is consistent across breakpoints and should work well given the large screen sizes these breakpoints target.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new breakpoint classes are properly configured in tailwind
# Test 1: Check tailwind config file for breakpoint definitions
echo "Checking tailwind configuration..."
rg -A 5 "largedisplay|fourk" "tailwind.config.*"
# Test 2: Look for any other usage of these breakpoints
echo "Checking for other usage of breakpoints..."
rg "largedisplay:|fourk:" --type ts --type tsx
Length of output: 359
Script:
#!/bin/bash
# Let's try different approaches to find the tailwind configuration
# Look for tailwind config with different extensions
echo "Looking for tailwind config files..."
fd -e js -e cjs -e ts "tailwind.config"
# Search for screen/breakpoint configurations in any potential config files
echo -e "\nSearching for screen configurations..."
rg -l "screen|breakpoint|largedisplay|fourk" $(fd -e js -e cjs -e ts "config")
# Check for actual usage of these classes in the codebase
echo -e "\nChecking usage of these grid classes..."
rg "largedisplay:grid-cols-|fourk:grid-cols-" --type-add "tsx:*.tsx" --type tsx
Length of output: 859
Script:
#!/bin/bash
# Let's check the content of tailwind config to verify the breakpoint definitions
echo "Checking tailwind configuration content..."
cat tailwind.config.js
# Also check the aspect ratio config since it might be related
echo -e "\nChecking vitals aspect ratio config..."
cat src/components/VitalsMonitor/useVitalsAspectRatioConfig.ts
# Look for any other similar responsive grid patterns in the codebase
echo -e "\nChecking other responsive grid patterns..."
rg "grid-cols-.*largedisplay:|grid-cols-.*fourk:" --type-add "tsx:*.tsx" --type tsx -B 1 -A 1
Length of output: 3646
| const breakpointValues = { | ||
| default: 6, | ||
| largedisplay: 9, | ||
| fourK: 12, | ||
| }; | ||
| const PER_PAGE_LIMIT = useBreakpoints(breakpointValues); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust breakpoint values to match the requested 6x4 layout for 4K displays.
The current breakpoint values progression (6->9->12) doesn't align with the requested 6x4 layout for 4K displays mentioned in the PR discussion. Additionally, consider the following improvements:
- Move this configuration to a constants file for better maintainability
- Add validation to ensure the values are positive integers
- Consider smoother transitions between breakpoints
Consider this alternative implementation:
- const breakpointValues = {
- default: 6,
- largedisplay: 9,
- fourK: 12,
- };
+ // Move to src/constants/layout.ts
+ const MONITOR_GRID_CONFIG = {
+ default: 6, // 2x3 grid
+ largedisplay: 8, // 2x4 grid
+ fourK: 24, // 4x6 grid (as requested)
+ } as const;
+
+ const PER_PAGE_LIMIT = useBreakpoints(MONITOR_GRID_CONFIG);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/hooks/useBreakpoints.ts (1)
3-11: Standardize breakpoint naming conventions and ordering.The breakpoint type definition could be improved for better maintainability:
- Standardize naming conventions (currently mixing camelCase "fourK" with lowercase "largedisplay")
- Use industry-standard terminology ("4K" instead of "fourK")
- Order breakpoints to match their width values
type Breakpoints = | "sm" | "md" | "lg" | "xl" | "2xl" | "3xl" - | "fourK" - | "largedisplay"; + | "large_display" + | "4k";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/CentralNursingStation.tsx(3 hunks)src/hooks/useBreakpoints.ts(2 hunks)tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Facility/CentralNursingStation.tsx
- tailwind.config.js
🔇 Additional comments (3)
src/hooks/useBreakpoints.ts (3)
Line range hint 25-45: Implementation looks good!
The hook implementation:
- Maintains backward compatibility
- Follows React hooks best practices
- Has proper error handling with fallback to default
- Is well-documented
Line range hint 1-45: Verify the impact on CNS display.
While the breakpoint additions align with the PR objective, please verify:
- That these breakpoints effectively solve the 75% unused space issue on 4K displays
- The visual layout at each new breakpoint (large_display and 4K)
Could you please provide screenshots of the CNS display at each new breakpoint to confirm the improvements in space utilization?
21-22: Verify breakpoint width values and maintain ascending order.
A few concerns about the breakpoint values:
- Standard 4K resolution is 3840px wide, but it's set to 3000px
- The 2080px breakpoint value seems arbitrary
- Values should be sorted in ascending order as per the comment above
Please verify if these values were intentionally chosen based on specific requirements.
✅ Verification successful
Based on the context gathered, I can now provide a final response. The code shows that these breakpoints are actively used in camera feed components, where the "3xl" breakpoint (1920px) is used for determining the number of camera feeds to display, and the usage patterns suggest these breakpoints are designed for specific UI layout requirements rather than matching exact screen resolutions.
The breakpoint values appear to be intentionally chosen for UI layout purposes
The values seem to be deliberately set below standard resolutions to accommodate various UI elements and ensure smooth transitions:
- The 2080px "largedisplay" breakpoint provides a buffer zone between 1920px (3xl) and 4K displays
- The 3000px "fourK" breakpoint is set below 3840px to ensure the layout adapts before reaching full 4K width, likely accounting for browser chrome and other UI elements
The values are maintained in ascending order as required by the comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing usage of these breakpoints to understand the context
rg -l "largedisplay|fourK" --type ts --type tsx
Length of output: 77
Script:
#!/bin/bash
# Search for any existing usage of these breakpoints with correct file type
rg -l "largedisplay|fourK" --type typescript
# Also check the file content to understand the context better
cat src/hooks/useBreakpoints.ts
Length of output: 1155
Script:
#!/bin/bash
# Let's search for any usage of useBreakpoints hook to understand the requirements
rg "useBreakpoints" -l
# Also search for any documentation or discussions about these breakpoints
fd -e md -e txt -x rg -l "breakpoint|resolution|screen size"
Length of output: 530
Script:
#!/bin/bash
# Let's check the actual usage in these files to understand the requirements
rg "useBreakpoints" -A 5 -B 5 src/components/CameraFeed/CentralLiveMonitoring/index.tsx src/components/CameraFeed/CameraFeed.tsx
# Check README for any relevant information
cat README.md
Length of output: 11494
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
]
Summary by CodeRabbit
Summary by CodeRabbit
New Features
fourKandlargedisplay.Bug Fixes
Chores