Migrated Scribe to a CARE App + Context #8469
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
shivankacker
added
waiting for related PR
|
👋 Hi, @shivankacker, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
shivankacker
added
waiting for more data
and removed
waiting for related PR
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
scribe-context
|
| Run status |
|
| Run duration | 04m 49s |
| Commit |
|
| Committer | Shivank Kacker |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
126
|
| View all changes introduced in this branch ↗︎ | |
github-actions
bot
added
merge conflict
|
👋 Hi, @shivankacker, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@shivankacker please close pending PR's before opening others. |
|
@bodhish working on this currently |
github-actions
bot
added
Deploy-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 (30)
cypress/e2e/patient_spec/PatientBedManagement.cy.ts (1)
Line range hint
6-54: Consider enhancing test robustness and coverage.While the test implementation is solid, consider these improvements:
- Add data cleanup in afterEach to ensure test isolation
- Add error handling for failed bed assignments
- Add test cases for edge scenarios:
- Attempting to assign an already occupied bed
- Handling network errors during bed assignment
- Verifying bed status updates in real-time
Here's a suggested enhancement:
afterEach(() => { cy.saveLocalStorage(); + // Clean up assigned beds + cy.intercept('POST', '**/bed/clear/').as('clearBed'); + patientPage.visitPatient(switchPatientOne); + patientLogupdate.clickSwitchBed(); + cy.get('#clear-button').click(); + cy.wait('@clearBed'); + + patientPage.visitPatient(switchPatientTwo); + patientLogupdate.clickSwitchBed(); + cy.get('#clear-button').click(); + cy.wait('@clearBed'); }); +it("Should handle bed assignment failures gracefully", () => { + // Simulate network error + cy.intercept('POST', '**/bed/assign/', { + statusCode: 500, + body: { detail: 'Internal Server Error' } + }).as('failedAssignment'); + + patientPage.visitPatient(switchPatientOne); + patientLogupdate.clickSwitchBed(); + patientLogupdate.selectBed(switchBedOne); + + // Verify error handling + cy.wait('@failedAssignment'); + cy.verifyNotification("Failed to assign bed"); +});cypress/e2e/patient_spec/PatientDoctorConnect.cy.ts (2)
Line range hint
1-14: Consider moving test data to fixtures.The test data (patient name, user roles) should be moved to a fixture file for better maintainability and reusability. This would make it easier to update test data and share it across different test files.
Example implementation:
// cypress/fixtures/doctor-connect.json { "patientName": "Dummy Patient 11", "users": { "doctor": "Dev Doctor", "nurse": "Dev Staff", "teleIcuDoctor": "Dev Doctor Two" } } // In test file let testData: any; before(() => { cy.fixture('doctor-connect').then((data) => { testData = data; }); loginPage.loginAsDistrictAdmin(); cy.saveLocalStorage(); });
Line range hint
29-54: Split the test case into multiple focused scenarios.The current test case violates the single responsibility principle by testing multiple features (user visibility, copy functionality, icon presence, and sorting) in one test. This makes it harder to debug failures and maintain the test.
Consider splitting into these focused test cases:
it("should display users under correct sections", () => { patientPage.visitPatient(testData.patientName); doctorconnect.clickDoctorConnectButton(); cy.verifyContentPresence("#doctor-connect-home-doctor", [testData.users.doctor]); cy.verifyContentPresence("#doctor-connect-home-nurse", [testData.users.nurse]); cy.verifyContentPresence("#doctor-connect-remote-doctor", [testData.users.teleIcuDoctor]); }); it("should copy phone number correctly", () => { patientPage.visitPatient(testData.patientName); doctorconnect.clickDoctorConnectButton(); doctorconnect.CopyFunctionTrigger(); doctorconnect.clickCopyPhoneNumber("#doctor-connect-home-doctor", testData.users.doctor); doctorconnect.verifyCopiedContent(); }); it("should display communication icons", () => { patientPage.visitPatient(testData.patientName); doctorconnect.clickDoctorConnectButton(); doctorconnect.verifyIconVisible("#whatsapp-icon"); doctorconnect.verifyIconVisible("#phone-icon"); }); it("should filter users by role correctly", () => { patientPage.visitPatient(testData.patientName); doctorconnect.clickDoctorConnectButton(); doctorconnect.clickUsersSortBy("Doctor"); cy.verifyContentPresence("#doctor-connect-home-doctor", [testData.users.doctor]); doctorconnect.clickUsersSortBy("Nurse"); cy.verifyContentPresence("#doctor-connect-home-nurse", [testData.users.nurse]); doctorconnect.clickUsersSortBy("TeleICU Doctor"); cy.verifyContentPresence("#doctor-connect-remote-doctor", [testData.users.teleIcuDoctor]); });cypress/e2e/resource_spec/ResourcesHomepage.cy.ts (4)
Line range hint
5-11: Consider moving test data to a separate constants file.The phone number constant could be moved to a shared test data file to maintain consistency across tests and make updates easier.
- const phone_number = "9999999999";Create a new file
cypress/fixtures/testData.ts:export const TEST_PHONE_NUMBER = "9999999999";
Line range hint
39-63: Enhance resource request creation test reliability.The resource request creation test could be flaky due to the search dependency. Consider these improvements:
- Add retry ability for the search
- Add more specific assertions for the facility search results
- Include error scenarios
it("Create a resource request", () => { cy.visit("/facility"); - cy.get("#search").click().type("dummy facility 40"); + cy.get("#search") + .click() + .type("dummy facility 40") + .should("have.value", "dummy facility 40"); + cy.intercept("GET", "**/api/v1/facility/**").as("loadFacilities"); - cy.get("#facility-details").click(); + cy.get("#facility-details") + .should("be.visible") + .and("contain.text", "dummy facility 40") + .click(); + cy.wait("@loadFacilities").its("response.statusCode").should("eq", 200); + + // Verify facility details are loaded + cy.get("body").should("not.contain.text", "Loading...");
Line range hint
1-89: Consider adding more comprehensive test coverage.The test suite could benefit from additional scenarios:
- Error handling cases (e.g., network errors, validation errors)
- Accessibility checks for the resource page
- Verification of UI elements and their states
Example additions:
it("Should handle network errors gracefully", () => { cy.intercept("GET", "**/api/v1/facility/**", { statusCode: 500, body: { message: "Internal Server Error" } }).as("failedRequest"); // Test error handling }); it("Should meet accessibility standards", () => { cy.visit("/resource"); cy.injectAxe(); cy.checkA11y(); });
Line range hint
65-77: Verify resource status updates more thoroughly.The status update test should verify:
- Available status options
- Status change reflection in UI
- API payload correctness
it("Update the status of resource", () => { cy.visit(createdResource); + // Verify available status options + const expectedStatuses = ["APPROVED", "REJECTED", "PENDING"]; resourcePage.clickUpdateStatus(); + cy.get("[data-testid='status-options']") + .should("be.visible") + .find("option") + .should("have.length", expectedStatuses.length) + .each(($el, index) => { + expect($el.text()).to.equal(expectedStatuses[index]); + }); + + // Intercept status update request + cy.intercept("PATCH", "**/api/v1/resource/**").as("updateStatus"); resourcePage.updateStatus("APPROVED"); resourcePage.clickSubmitButton(); + + // Verify API request + cy.wait("@updateStatus").then((interception) => { + expect(interception.request.body.status).to.equal("APPROVED"); + }); + + // Verify UI update + cy.get("[data-testid='resource-status']") + .should("be.visible") + .and("contain.text", "APPROVED"); + resourcePage.verifySuccessNotification( "Resource request updated successfully", ); });cypress/e2e/facility_spec/FacilityInventory.cy.ts (2)
1-4: Consider implementing a consistent import ordering pattern.While moving the
LoginPageimport is harmless, consider organizing imports consistently (e.g., alphabetically) to improve maintainability.-import FacilityPage from "../../pageobject/Facility/FacilityCreation"; -import FacilityHome from "../../pageobject/Facility/FacilityHome"; -import LoginPage from "../../pageobject/Login/LoginPage"; +import FacilityHome from "../../pageobject/Facility/FacilityHome"; +import FacilityPage from "../../pageobject/Facility/FacilityCreation"; +import LoginPage from "../../pageobject/Login/LoginPage";
Line range hint
28-94: Improve test reliability by following Cypress best practices.Several improvements could make these tests more robust:
- Replace hard-coded
cy.wait(3000)with proper Cypress commands:-cy.wait(3000); +cy.get('#element').should('be.visible');
- Standardize success message verification:
-facilityPage.verifySuccessNotification("Inventory created successfully"); -facilityPage.verifySuccessNotification("Minimum quantiy updated successfully"); +const successMessages = { + inventoryCreated: "Inventory created successfully", + minimumQuantityUpdated: "Minimum quantiy updated successfully" +}; +facilityPage.verifySuccessNotification(successMessages.inventoryCreated);
- Make element visibility checks more robust:
-if ($body.find("#update-minimum-quantity").is(":visible")) { +cy.get('body').then(($body) => { + const updateBtn = $body.find("#update-minimum-quantity"); + if (updateBtn.length && updateBtn.is(":visible")) {src/Utils/useSegmentedRecorder.ts (2)
11-11: Remove redundant comment.The variable name
microphoneAccessis self-explanatory, making the inline comment "New state" unnecessary.- const [microphoneAccess, setMicrophoneAccess] = useState(false); // New state + const [microphoneAccess, setMicrophoneAccess] = useState(false);
130-130: Remove redundant comment.The inline comment "Return microphoneAccess" is unnecessary as the code is self-documenting.
- microphoneAccess, // Return microphoneAccess + microphoneAccess,cypress/e2e/patient_spec/PatientFileUpload.ts (5)
Line range hint
4-28: Consider extracting test constants to a separate file.The test setup follows good practices with page objects and proper test isolation. However, the test constants (patient names, file names) could be moved to a separate configuration file for better maintainability and reuse across other test files.
+ // testData/patients.ts + export const TEST_PATIENTS = { + PATIENT_ONE: "Dummy Patient 3", + PATIENT_TWO: "Dummy Patient 4", + PATIENT_THREE: "Dummy Patient 5" + }; + + export const TEST_FILES = { + AUDIO_NAME: "cypress audio", + FILE_NAME: "cypress name", + MODIFIED_NAME: "cypress modified name" + };
Line range hint
30-46: Add verification for audio file format and content.The audio recording test should verify the format and basic properties of the recorded audio file. Also consider adding error scenarios for when audio recording fails.
it("Record an Audio and download the file", () => { patientPage.visitPatient(patientNameOne); visitPatientFileUploadSection.call(patientFileUpload); patientFileUpload.recordAudio(); patientFileUpload.typeAudioName(cypressAudioName); patientFileUpload.clickUploadAudioFile(); cy.verifyNotification("File Uploaded Successfully"); patientFileUpload.verifyUploadFilePresence(cypressAudioName); cy.get("button").contains("Download").click(); cy.verifyNotification("Downloading file..."); + // Verify audio file properties + cy.get('[data-testid="audio-player"]').should('have.attr', 'src') + .and('match', /^blob:http/); + // Verify file download + cy.readFile('cypress/downloads/' + cypressAudioName) + .should('exist'); });
Line range hint
48-64: Add error scenarios for file upload test.The file upload test should include scenarios for invalid file types, large files, and network failures. Also verify the archived file's accessibility state.
it("Upload a File and archive it", () => { patientPage.visitPatient(patientNameTwo); visitPatientFileUploadSection.call(patientFileUpload); + // Test invalid file type + patientFileUpload.uploadFile('invalid.exe'); + cy.verifyNotification("Invalid file type"); + + // Test file size limit + patientFileUpload.uploadFile('large.pdf'); + cy.verifyNotification("File size exceeds limit"); + patientFileUpload.uploadFile(); patientFileUpload.typeFileName(cypressFileName); patientFileUpload.clickUploadFile(); cy.verifyNotification("File Uploaded Successfully"); cy.closeNotification(); patientFileUpload.verifyUploadFilePresence(cypressFileName); patientFileUpload.archiveFile(); patientFileUpload.clickSaveArchiveFile(); cy.verifyNotification("File archived successfully"); patientFileUpload.verifyArchiveFile(cypressFileName); + // Verify archived file is not editable + patientFileUpload.verifyFileRenameOption(false); });
Line range hint
66-115: Enhance user permission test coverage.The permission test should verify additional operations like file deletion and archival permissions. Also consider testing with more user roles and edge cases.
it("User-level Based Permission for File Modification", () => { loginPage.login("dummynurse1", "Coronasafe@123"); cy.reload(); patientPage.visitPatient(patientNameThree); visitPatientFileUploadSection.call(patientFileUpload); patientFileUpload.uploadFile(); patientFileUpload.typeFileName(cypressFileName); patientFileUpload.clickUploadFile(); cy.verifyNotification("File Uploaded Successfully"); cy.closeNotification(); patientFileUpload.verifyUploadFilePresence(cypressFileName); + // Verify additional permissions + patientFileUpload.verifyDeleteOption(false); + patientFileUpload.verifyArchiveOption(false); + // ... existing permission checks ... + + // Test with additional user roles + loginPage.login("doctor1", "Coronasafe@123"); + cy.reload(); + patientFileUpload.verifyUploadFilePresence(cypressFileName); + patientFileUpload.verifyFileRenameOption(true); + patientFileUpload.verifyDeleteOption(true); + patientFileUpload.verifyArchiveOption(true); });
Line range hint
123-131: Add cross-browser testing configuration.The tests should be configured to run across different browsers to ensure consistent behavior. Also consider adding parallel test execution for better performance.
+ // cypress.config.ts + export default { + e2e: { + browsers: ['chrome', 'firefox', 'edge'], + // Enable parallel execution + parallel: true, + // Group similar tests + testIsolation: false + } + };cypress/e2e/assets_spec/AssetsManage.cy.ts (3)
Line range hint
1-120: Consider enhancing test maintainability and reliability.While reviewing the entire test suite, I noticed some opportunities for improvement:
Test data management:
- Hard-coded values like "Dummy Facility 40" should be moved to test fixtures
- Date manipulation could be centralized in a helper
Test isolation:
- Consider using
cy.intercept()to mock network requests- Add cleanup in
afterEachto ensure consistent stateError handling:
- Add timeout configurations for slow operations
- Include error assertions for negative scenarios
Example implementation for test data management:
// fixtures/asset-data.json { "facility": { "name": "Dummy Facility 40" }, "asset": { "name": "Dummy Camera", "initialLocation": "Camera Location", "newLocation": "Dummy Location 1" } } // support/commands.ts Cypress.Commands.add('loadAssetTestData', () => { cy.fixture('asset-data.json').as('testData'); }); // Example usage in test beforeEach(() => { cy.loadAssetTestData(); // ... other setup }); it('Verify Asset Warranty Expiry Label', function() { const { asset } = this.testData; assetSearchPage.typeSearchKeyword(asset.name); // ... rest of the test });
Line range hint
8-13: Move date utility to a dedicated helper file.The
addDaysToDatefunction should be moved to a shared helper file for reusability across test files.// support/helpers/date.ts export const addDaysToDate = (numberOfDays: number): string => { const inputDate = new Date(); inputDate.setDate(inputDate.getDate() + numberOfDays); return inputDate.toISOString().split('T')[0]; }; // Import in test file import { addDaysToDate } from '../../support/helpers/date';
Line range hint
36-65: Enhance warranty expiry test with data-driven approach.The warranty expiry test could be more maintainable using a data-driven approach.
const warrantyTestCases = [ { days: 100, expectedLabel: '' }, // > 3 months { days: 80, expectedLabel: '3 months' }, // < 3 months { days: 20, expectedLabel: '1 month' }, // < 1 month { days: 100, expectedLabel: '' } // Reset case ]; it('Verify Asset Warranty Expiry Label', () => { assetSearchPage.typeSearchKeyword(assetname); assetSearchPage.pressEnter(); assetSearchPage.verifyBadgeContent(assetname); assetSearchPage.clickAssetByName(assetname); warrantyTestCases.forEach(({ days, expectedLabel }) => { assetPage.clickupdatedetailbutton(); assetPage.scrollintoWarrantyDetails(); assetPage.enterWarrantyExpiryDate(addDaysToDate(days)); assetPage.clickassetupdatebutton(); assetPage.verifyWarrantyExpiryLabel(expectedLabel); }); });cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
Line range hint
89-97: Consider enhancing pagination test coverage.While the basic pagination tests are good, consider adding test cases for:
- Edge cases (first/last page)
- Items per page changes
- Total items count verification
- Page number validation
Example enhancement:
it("handles pagination edge cases", () => { assetPagination.navigateToLastPage(); assetPagination.verifyNextButtonDisabled(); assetPagination.navigateToFirstPage(); assetPagination.verifyPreviousButtonDisabled(); assetPagination.changeItemsPerPage(50); assetPagination.verifyItemsPerPageApplied(50); });cypress/e2e/users_spec/UsersCreation.cy.ts (4)
Line range hint
1-200: Consider enhancing test coverage for iPhone-specific scenarios.Given that Issue #8227 mentions iPhone-specific malfunctions, it would be valuable to add test cases that specifically verify the application's behavior on iPhone viewports.
Consider adding:
it("should work correctly on iPhone viewport", () => { cy.viewport('iphone-x'); // Test user creation flow // Test profile updates // Verify any audio-related functionality });
Line range hint
19-31: Consider extracting themakeidutility function.The
makeidfunction is a utility that could be reused across other test files. Consider moving it to a shared utilities file.Move this to
cypress/pageobject/utils/constants.ts:export const makeid = (length: number) => { let result = ""; const characters = "abcdefghijklmnopqrstuvwxyz0123456789"; const charactersLength = characters.length; for (let i = 0; i < length; i++) { result += characters.charAt(Math.floor(Math.random() * charactersLength)); } return result; };
Line range hint
32-35: Consider using test data management for user lists.The hardcoded array of users could be better managed through test data fixtures.
Consider creating a fixture file
cypress/fixtures/users.json:{ "alreadyLinkedUsers": ["devdoctor", "devstaff2", "devdistrictadmin"] }Then import it in the test:
let users; before(() => { cy.fixture('users').then((data) => { users = data; }); });
Line range hint
36-50: Consider using an enum for error messages.The error message arrays could be better managed using TypeScript enums or constants in a separate file.
Consider creating
cypress/constants/errorMessages.ts:export enum UserCreationErrors { USER_TYPE = "Please select the User Type", PHONE_NUMBER = "Please enter valid phone number", // ... other messages } export enum ProfileErrors { REQUIRED = "This field is required", PHONE_NUMBER = "Please enter valid phone number" }cypress/e2e/patient_spec/PatientRegistration.cy.ts (4)
Line range hint
9-31: Consider improving date handling and age calculation.The current implementation could be enhanced for better maintainability and robustness:
- The hardcoded year value could lead to maintenance issues
- Date calculations could benefit from a more robust approach
Consider this improvement:
-const yearOfBirth = "2001"; +const TEST_YEAR_OF_BIRTH = "2001"; // Make the constant name more descriptive const calculateAge = () => { - const currentYear = new Date().getFullYear(); - return currentYear - parseInt(yearOfBirth); + const today = new Date(); + const birthDate = new Date(`${TEST_YEAR_OF_BIRTH}-01-01`); + let age = today.getFullYear() - birthDate.getFullYear(); + const monthDiff = today.getMonth() - birthDate.getMonth(); + if (monthDiff < 0 || (monthDiff === 0 && today.getDate() < birthDate.getDate())) { + age--; + } + return age; };
Line range hint
32-329: Enhance test organization and error coverage.While the test cases are comprehensive, consider these improvements:
- Group test data setup into a separate fixture file
- Add negative test cases for form validation
- Add assertions for error states in the form fields
Create a new fixture file
cypress/fixtures/patient-registration.json:{ "validPatient": { "name": "Patient With No Consultation", "gender": "Male", "updatedGender": "Female", "address": "Test Patient Address", // ... other test data }, "invalidPatient": { "name": "", // for validation testing "phone": "invalid", // ... invalid test data } }Then refactor the tests to use this fixture:
describe("Patient Creation with consultation", () => { let testData; before(() => { cy.fixture('patient-registration.json').then((data) => { testData = data; }); // ... existing setup }); // Add negative test case it("should show validation errors for invalid input", () => { patientPage.createPatient(); patientPage.selectFacility(testData.validPatient.facility); patientPage.typePatientName(testData.invalidPatient.name); patientPage.typePatientPhoneNumber(testData.invalidPatient.phone); patientPage.clickCreatePatient(); // Assert validation errors cy.get('[data-testid="name-error"]').should('be.visible'); cy.get('[data-testid="phone-error"]').should('be.visible'); }); // ... existing test cases using testData });
Line range hint
276-329: Enhance transfer test error scenarios.The transfer test could be expanded to cover more error cases:
- Transfer with missing required fields
- Transfer with invalid patient data
- Network error handling
Add these test cases:
it("should handle transfer errors appropriately", () => { // Test missing required fields patientPage.createPatient(); patientPage.selectFacility(patientTransferFacility); patientTransfer.clickTransferSubmitButton(); cy.get('[data-testid="transfer-error"]').should('be.visible'); // Test network error cy.intercept('POST', '/api/transfer', { statusCode: 500, body: { message: 'Internal Server Error' } }); // ... complete the transfer flow cy.get('[data-testid="error-notification"]') .should('contain', 'Failed to transfer patient'); });
Line range hint
32-329: Add tests for Scribe integration.Based on the PR objectives of migrating Scribe to CARE App, consider adding test cases for:
- Scribe interaction with existing form data
- Audio upload functionality (especially for iPhone)
- Integration between Scribe and patient registration
Would you like me to help create test cases for these scenarios? I can provide a detailed implementation that covers:
- Scribe awareness of pre-filled form data
- Audio upload validation
- Cross-browser compatibility tests
public/locale/en.json (2)
254-255: Consider adding more voice-related localization keys for better UX.The voice-related translations are clear and user-friendly. However, consider adding these additional keys to enhance the user experience:
- Error states (e.g., "voice_recognition_failed", "network_error_during_recording")
- Accessibility labels (e.g., "voice_record_button_aria_label")
- Progress indicators (e.g., "processing_voice_input")
Also applies to: 490-491, 707-707, 1021-1021, 1078-1078, 1095-1096, 1160-1160, 1164-1164, 1191-1192, 1274-1274
334-334: Make the autofilled fields message more descriptive.The "autofilled_fields" message could be more specific to provide better context to users.
Consider using a more descriptive message like:
- "autofilled_fields": "Autofilled Fields", + "autofilled_fields": "Fields automatically filled from your health records",
📜 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 (25)
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)public/locale/en.json(12 hunks)src/Routers/AppRouter.tsx(1 hunks)src/Utils/request/api.tsx(0 hunks)src/Utils/useSegmentedRecorder.ts(5 hunks)src/components/Common/BloodPressureFormField.tsx(1 hunks)src/components/Common/DateInputV2.tsx(4 hunks)src/components/Common/TemperatureFormField.tsx(2 hunks)src/components/Common/prescription-builder/PrescriptionDropdown.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/Utils/request/api.tsx
✅ Files skipped from review due to trivial changes (7)
- cypress/e2e/assets_spec/AssetHomepage.cy.ts
- cypress/e2e/assets_spec/AssetsCreation.cy.ts
- cypress/e2e/facility_spec/FacilityCreation.cy.ts
- cypress/e2e/facility_spec/FacilityManage.cy.ts
- cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts
- cypress/e2e/patient_spec/PatientLogUpdate.cy.ts
- cypress/e2e/patient_spec/PatientPrescription.cy.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Routers/AppRouter.tsx
- src/components/Common/BloodPressureFormField.tsx
- src/components/Common/DateInputV2.tsx
- src/components/Common/TemperatureFormField.tsx
- src/components/Common/prescription-builder/PrescriptionDropdown.tsx
🔇 Additional comments (14)
cypress/e2e/patient_spec/PatientBedManagement.cy.ts (1)
1-4: LGTM! Import changes align with the migration.
The addition of PatientConsultationPage import is consistent with the PR's objective of migrating Scribe functionality.
cypress/e2e/patient_spec/PatientDoctorConnect.cy.ts (2)
Line range hint 16-28: LGTM! Well-structured test setup.
The test lifecycle hooks are properly implemented with good practices:
- Login state persistence using localStorage
- Proper cleanup of filters before each test
- Clear URL verification
Line range hint 56-58: LGTM! Proper test cleanup.
The afterEach hook correctly persists the localStorage state for subsequent tests.
cypress/e2e/resource_spec/ResourcesHomepage.cy.ts (1)
1-3: LGTM! Import organization follows best practices.
The imports are now properly organized at the top of the file, improving code readability.
cypress/e2e/facility_spec/FacilityInventory.cy.ts (1)
Line range hint 5-27: Verify test stability after Scribe migration.
Since this PR involves migrating Scribe to a CARE app, please ensure these inventory management tests remain stable after the migration. Consider:
- Running these tests against both old and new environments
- Verifying that the page objects and selectors remain valid
- Documenting any environment-specific setup required for these tests
src/Utils/useSegmentedRecorder.ts (2)
Line range hint 29-41: Verify microphone access handling on iOS Safari.
Given the reported issues with iPhone (Issue #8227), please ensure this implementation works correctly on iOS Safari. Consider:
- iOS Safari requires user interaction before allowing media access
- Audio recording might need specific handling for iOS compatibility
#!/bin/bash
# Search for iOS-specific handling in the codebase
rg -i "ios|safari|useragent" --type ts --type tsx104-113:
Enhance error handling in startRecording.
The current implementation has several issues that need attention:
- The error message isn't internationalized
- The recorder state should be cleaned up on error
- The error handling at call sites might need to be documented
Apply these improvements:
const startRecording = async () => {
try {
const newRecorder = await requestRecorder();
setRecorder(newRecorder);
setMicrophoneAccess(true);
setIsRecording(true);
} catch (error) {
setMicrophoneAccess(false);
+ setRecorder(null);
+ setIsRecording(false);
- throw new Error("Microphone access denied");
+ throw new Error(t("audio__permission_denied"));
}
};cypress/e2e/assets_spec/AssetsManage.cy.ts (1)
2-2: LGTM! Import reordering looks good.
The reordering of imports maintains code organization without affecting functionality.
Also applies to: 5-5
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
2-5: LGTM! Import changes are clean and purposeful.
The addition of AssetPagination and reorganization of imports align well with the test file's functionality.
Line range hint 9-168: Test structure follows best practices, but consider adding Scribe-related tests.
The test suite is well-structured using the page object pattern and covers core facility homepage functionality comprehensively. However, given that this PR involves migrating Scribe functionality, consider adding test coverage for:
- Scribe integration with facility data
- Audio processing functionality (especially for iPhone devices, ref: Issue Scribe is malfunctioning in iphone #8227)
- Form data awareness (ref: Issue Make Scribe Aware of already present form data #8134)
Let's verify if there are any Scribe-related tests in the codebase:
cypress/e2e/users_spec/UsersManage.cy.ts (2)
4-4: LGTM! Import change aligns with module organization.
The addition of UserPage import from UserSearch module follows good separation of concerns, keeping page object models organized by functionality.
Line range hint 1-168: Verify test coverage for Scribe migration.
The test suite thoroughly covers user management operations but should be verified against the new Scribe CARE app integration.
Let's check for any Scribe-related test coverage:
Consider adding specific test cases to verify:
- User permissions interaction with the new Scribe CARE app
- Integration points between user management and Scribe functionality
- Data consistency when users interact with Scribe features
cypress/e2e/users_spec/UsersCreation.cy.ts (1)
3-3: LGTM! Import statements are properly organized.
The imports are well-structured and the addition of generatePhoneNumber aligns with its usage in the test cases.
Also applies to: 5-5, 8-8
cypress/e2e/patient_spec/PatientRegistration.cy.ts (1)
1-7: LGTM! Well-organized imports.
The new imports for PatientTransfer and generatePhoneNumber are appropriately added and align with their usage in the test cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/CAREUI/interactive/KeyboardShortcut.tsx (1)
81-98: Consider improving accessibility and error handling.While the implementation is solid, consider these enhancements:
- Add error handling for invalid shortcut keys
- Improve screen reader support
- Consider mobile visibility strategy
export function KeyboardShortcutKey(props: { shortcut: string[]; useShortKeys?: boolean; }) { const { shortcut, useShortKeys } = props; + // Validate shortcut keys + const validShortcuts = shortcut.every(key => + key in SHORTCUT_KEY_MAP || key.length > 0 + ); + + if (!validShortcuts) { + console.warn('Invalid shortcut keys provided:', shortcut); + return null; + } return ( - <div className="hidden shrink-0 items-center md:flex"> + <div + className="hidden shrink-0 items-center md:flex" + role="group" + aria-label="Keyboard shortcuts" + > {shortcut.map((key, idx, keys) => ( <Fragment key={idx}> <kbd + aria-label={`Press ${SHORTCUT_KEY_MAP[key] || key}`} className="relative z-10 flex h-6 min-w-6 shrink-0 items-center justify-center rounded-md border border-b-4 border-secondary-400 bg-secondary-100 px-2 text-xs text-black" >src/Utils/useVoiceRecorder.ts (6)
6-10: Consider using a single state object for related states.The hook uses multiple independent state variables that are conceptually related. Consider combining them into a single state object to make state updates more atomic and prevent potential race conditions.
- const [audioURL, setAudioURL] = useState(""); - const [isRecording, setIsRecording] = useState(false); - const [recorder, setRecorder] = useState<MediaRecorder | null>(null); - const [blob, setBlob] = useState<Blob | null>(null); - const [waveform, setWaveform] = useState<number[]>([]); // Decibel waveform + const [state, setState] = useState({ + audioURL: "", + isRecording: false, + recorder: null as MediaRecorder | null, + blob: null as Blob | null, + waveform: [] as number[], + });
16-20: Add explanatory comment for the recorder reset logic.The purpose of this effect is not immediately clear. Consider adding a comment explaining why the recorder needs to be reset when recording stops and an audio URL exists.
useEffect(() => { + // Reset recorder after successful recording to free up resources and prepare for next recording if (!isRecording && recorder && audioURL) { setRecorder(null); } }, [isRecording, recorder, audioURL]);
28-38: Enhance error handling with specific error types.The error handling could be more specific to help users understand and resolve issues better. Consider categorizing errors based on their type (permission denied, device not found, etc.).
} catch (error) { + let errorMessage = "Please grant microphone permission to record audio."; + if (error instanceof DOMException) { + switch (error.name) { + case 'NotAllowedError': + errorMessage = "Microphone access was denied. Please allow access in your browser settings."; + break; + case 'NotFoundError': + errorMessage = "No microphone found. Please connect a microphone and try again."; + break; + case 'NotReadableError': + errorMessage = "Could not access your microphone. Please check if it's properly connected."; + break; + default: + errorMessage = `Microphone error: ${error.message}`; + } + } Notify.Error({ - msg: error instanceof Error - ? error.message - : "Please grant microphone permission to record audio.", + msg: errorMessage, });
107-120: Add TypeScript return types to control functions.The control functions lack explicit return type annotations. Adding them would improve type safety and documentation.
- const startRecording = () => { + const startRecording = (): void => { setIsRecording(true); }; - const stopRecording = () => { + const stopRecording = (): void => { setIsRecording(false); setWaveform([]); }; - const resetRecording = () => { + const resetRecording = (): void => { setAudioURL(""); setBlob(null); setWaveform([]); };
122-131: Add explicit return type interface.Define an interface for the hook's return value to improve type safety and documentation.
+interface VoiceRecorderHook { + audioURL: string; + isRecording: boolean; + startRecording: () => void; + stopRecording: () => void; + blob: Blob | null; + waveform: number[]; + resetRecording: () => void; +} -const useVoiceRecorder = (handleMicPermission: (allowed: boolean) => void) => { +const useVoiceRecorder = (handleMicPermission: (allowed: boolean) => void): VoiceRecorderHook => {
133-157: Add comprehensive JSDoc documentation.The
requestRecorderfunction would benefit from detailed JSDoc documentation explaining its purpose, parameters, return value, and possible errors.+/** + * Requests access to the user's microphone and creates a MediaRecorder instance. + * Includes special handling for iOS Safari compatibility. + * + * @throws {Error} When microphone access is denied or initialization fails + * @returns {Promise<MediaRecorder>} A configured MediaRecorder instance + */ async function requestRecorder() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/CAREUI/interactive/KeyboardShortcut.tsx(2 hunks)src/Utils/useVoiceRecorder.ts(1 hunks)
🔇 Additional comments (2)
src/CAREUI/interactive/KeyboardShortcut.tsx (2)
1-1: LGTM! Import changes are well-structured.
The import changes follow best practices:
- Using
Fragmentfrom 'react/jsx-runtime' is optimal for performance - Relative import path for utils improves maintainability
Also applies to: 4-4
75-78: LGTM! Well-structured component with clear props interface.
The component has a clear single responsibility and good TypeScript typing.
|
@nihal467 resolved merge conflicts, deploy preview failing due to cache issue, nothing else. |
|
👋 Hi, @shivankacker, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@shivankacker the file upload is still not working |
|
Enhancement
|
|
@nihal467 enhancements can be taken up later, they will also be dependent on the plugin (no changes in care). I have added an ignore rule for oral issue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/components/Form/FormFields/Autocomplete.tsx (2)
187-191: Well-implemented ARIA attributes for enhanced accessibility.The addition of
data-cui-listboxattributes improves accessibility and provides better integration with assistive technologies. The structured data format allows for better state management and value injection.Consider documenting these attributes in your component's API documentation to help other developers understand their purpose and usage.
117-117: Well-structured implementation of value injection pattern.The implementation effectively combines React refs, custom hooks, and ARIA attributes to create a robust value injection system. This aligns well with the PR's objective of making Scribe aware of existing form data.
A few architectural considerations:
- The pattern could be abstracted into a higher-order component for reuse across other form components
- Consider adding error boundaries to handle potential JSON parsing errors
- Document the value injection pattern in your architecture documentation
Would you like help creating a higher-order component to abstract this pattern?
Also applies to: 168-176, 187-191
src/components/Patient/DailyRounds.tsx (3)
Line range hint
826-861: Consider extracting nursing care form handlingThe nursing care form handling logic could be extracted into a custom hook to improve maintainability and reusability.
+// useNursingCareForm.ts +const useNursingCareForm = (initialNursing: any) => { + const handleNursingChange = useCallback((log: any) => ({ + name: "nursing", + value: log.nursing, + }), []); + + return { handleNursingChange }; +}; // In component -<NursingCare - log={{ nursing: state.form.nursing }} - onChange={(log) => - handleFormFieldChange({ - name: "nursing", - value: log.nursing, - }) - } +const { handleNursingChange } = useNursingCareForm(state.form.nursing); +<NursingCare + log={{ nursing: state.form.nursing }} + onChange={(log) => handleFormFieldChange(handleNursingChange(log))} />
869-872: Consider extracting doctor's log sectionThe doctor's log section contains complex UI and logic. Consider extracting it into a separate component to improve maintainability.
+// DoctorsLogSection.tsx +interface DoctorsLogSectionProps { + diagnoses: ConsultationDiagnosis[]; + diagnosisSuggestions: ICD11DiagnosisModel[]; + investigations: any[]; + showDiscontinuedPrescriptions: boolean; + onDiagnosisUpdate: () => void; + onInvestigationsChange: (investigations: any[]) => void; + onDiscontinuedPrescriptionsChange: (show: boolean) => void; +} + +const DoctorsLogSection: React.FC<DoctorsLogSectionProps> = ({ + diagnoses, + diagnosisSuggestions, + investigations, + showDiscontinuedPrescriptions, + onDiagnosisUpdate, + onInvestigationsChange, + onDiscontinuedPrescriptionsChange, +}) => { + return ( + <div className="flex flex-col gap-10 divide-y-2 divide-dashed divide-secondary-600 border-t-2 border-dashed border-secondary-600 pt-6"> + {/* Existing doctor's log content */} + </div> + ); +};🧰 Tools
🪛 Biome
[error] 869-951: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
Line range hint
712-737: Remove unnecessary fragmentThe fragment wrapping the blood sugar level field is unnecessary as it contains only one child element.
-<> <TextFormField {...field("blood_sugar_level")} labelSuffix="mg/dL" min={0} max={700} type="number" thresholds={[ { value: 0, className: "text-danger-500", label: "Low", }, { value: 69, className: "text-primary-500", label: "Normal", }, { value: 110, className: "text-danger-500", label: "High", }, ]} /> -</>public/locale/en.json (1)
506-507: Consider improving clarity and consistency of user feedback messages.Some suggestions to enhance user experience:
- "Copilot is thinking..." could be more specific, e.g., "Processing your voice input..."
- "We are hearing you..." could be more formal, e.g., "Processing audio input..."
- Consider adding an ellipsis (...) to "Reviewing field" for consistency with other loading states
- Consider making "This is what we heard" more formal, e.g., "Transcribed audio content"
Also applies to: 730-730, 1129-1130, 1229-1230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
public/locale/en.json(12 hunks)src/Routers/AppRouter.tsx(1 hunks)src/Utils/request/api.tsx(0 hunks)src/components/Form/FormFields/Autocomplete.tsx(4 hunks)src/components/Patient/DailyRounds.tsx(12 hunks)src/pluginTypes.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/Utils/request/api.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Routers/AppRouter.tsx
- src/pluginTypes.ts
🧰 Additional context used
🪛 Biome
src/components/Patient/DailyRounds.tsx
[error] 712-737: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (4)
src/components/Form/FormFields/Autocomplete.tsx (1)
173-176: 🛠️ Refactor suggestion
Prevent potential infinite update loops in useEffect.
The current implementation might cause infinite loops if the parent component updates its state in response to the onChange callback. Consider adding additional checks or debouncing the updates.
Consider this implementation to prevent potential loops:
useEffect(() => {
- if (props.value !== domValue && typeof domValue !== "undefined")
+ if (props.value !== domValue &&
+ typeof domValue !== "undefined" &&
+ typeof props.value !== "undefined") {
props.onChange(domValue);
+ }
}, [domValue]);src/components/Patient/DailyRounds.tsx (2)
683-688: Improved input validation with TextFormField
The replacement of range fields with TextFormField components includes proper validation constraints:
- Pulse: 0-200 bpm
- Respiratory rate: 0-150 bpm
- SpO2: 0-100%
The implementation includes appropriate min/max values and units.
Also applies to: 744-749, 769-774
658-663: Well-implemented conditional form field visibility
Good implementation of the oral issue field's visibility and state management:
- Field is disabled when nutrition route is not "ORAL"
- Form state is properly cleared when nutrition route changes
- Scribe integration is conditionally disabled
public/locale/en.json (1)
265-266: LGTM! The new translations align with the Scribe migration objectives.
The added translations appropriately support the voice recording, transcription, and auto-filling features being migrated.
Also applies to: 347-347, 506-507, 730-730, 1078-1078, 1108-1108, 1129-1130, 1195-1195, 1199-1199, 1216-1216, 1229-1230, 1320-1320
|
conditional approved it, @shivankacker take up enhancement as separate issue |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Patient/DailyRounds.tsx (3)
561-565: Consider enhancing Scribe integration with additional context attributes.The Scribe integration looks good, but consider adding data attributes to provide more context about the form's purpose and structure:
<form className="w-full max-w-4xl rounded-lg bg-white px-3 py-5 shadow sm:px-6 md:py-11" data-scribe-form + data-scribe-form-type="daily-rounds" + data-scribe-patient-id={patientId} + data-scribe-consultation-id={consultationId} >
856-859: Simplify onChange handler using object shorthand.The onChange handler can be simplified:
-onChange={(log) => - handleFormFieldChange({ - name: "nursing", - value: log.nursing, - }) -} +onChange={(log) => handleFormFieldChange({ name: "nursing", value: log.nursing })}
Line range hint
267-341: Consider extracting form validation logic into a custom hook.The validation logic is complex and could be reused across similar forms. Consider extracting it into a custom hook:
// useDailyRoundsValidation.ts export const useDailyRoundsValidation = (form: DailyRoundsModel, t: TFunction) => { return useCallback(() => { const errors = { ...initError }; let invalidForm = false; // ... existing validation logic ... return { errors, isValid: !invalidForm }; }, [form, t]); };This would improve maintainability and allow reuse in similar medical forms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/DailyRounds.tsx(11 hunks)
🧰 Additional context used
🪛 Biome
src/components/Patient/DailyRounds.tsx
[error] 710-735: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (1)
src/components/Patient/DailyRounds.tsx (1)
681-686: Verify vital signs ranges against medical standards.
The implementation of vital signs validation looks good, but please verify these ranges with medical professionals:
- Pulse: 0-200 bpm
- Respiratory rate: 0-150 bpm
- SpO2: 0-100%
Also applies to: 742-747, 767-772
✅ Verification successful
Vital signs ranges appear to be consistent with medical standards
Based on the codebase analysis, the vital signs ranges are already implemented consistently across different components and align with typical medical standards:
- Pulse (0-200 bpm): Found in both DailyRounds.tsx and Vitals.tsx
- Respiratory rate (0-150 bpm): Consistent in DailyRounds.tsx and Vitals.tsx
- SpO2 (0-100%): Correctly bounded between 0-100% in multiple components
The ranges are also used in conjunction with thresholds for medical interpretation, suggesting they've been previously validated. The implementation shows careful consideration of medical parameters across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other vital signs validation in the codebase
rg -A 2 "min={[0-9].*}.*max={[0-9].*}" --type tsx
Length of output: 167
Script:
#!/bin/bash
# Search for vital signs validation patterns with proper regex escaping
rg -A 2 'min=\{[0-9]+\}.*max=\{[0-9]+\}' --type tsx
# Alternative search for min/max attributes
rg -A 2 '(min|max)="\d+"|min={\d+}|max={\d+}' --type tsx
# Search for specific vital signs related fields
rg -B 2 -A 2 '(pulse|respiratory|spo2|SpO2|oxygen).*type="number"' --type tsx
Length of output: 387
Script:
#!/bin/bash
# Search for vital signs validation patterns without file type restriction
rg -A 2 'min=\{[0-9]' 'src/'
# Search for specific vital signs related fields
rg -B 2 -A 2 '(pulse|respiratory|spo2|SpO2|oxygen).*type=' 'src/'
# Search for any TextFormField with min/max props
rg -B 2 -A 2 'TextFormField.*min.*max' 'src/'
Length of output: 12692
|
@shivankacker @shivankacker Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Moves Scribe to https://github.com/ohcnetwork/care_scribe_fe
Screen.Recording.2024-11-06.at.12.34.23.PM.mov
Requires care_scribe backend plugin pointing to ohcnetwork/care_scribe#12
Requires the care_scribe_fe frontend appp. Add that by updating your env to include
REACT_ENABLED_APPS="ohcnetwork/care_livekit_fe,ohcnetwork/care_scribe_fe"NOTE: Cannot update non arbitrary fields like symptoms, diagnoses for now.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores