-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed the cypress env config file and removed hardcoded fallback #9184
Conversation
Deploying care-fe with Cloudflare Pages
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cypress.config.ts (1)
3-3
: Consider improving dotenv import and configuration.
- Use default import for dotenv instead of namespace import
- Add error handling for dotenv configuration
-import * as dotenv from "dotenv"; +import dotenv from "dotenv"; -dotenv.config(); +try { + const result = dotenv.config(); + if (result.error) { + console.error('Error loading .env file:', result.error); + } +} catch (error) { + console.error('Error configuring dotenv:', error); +}Also applies to: 6-6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (1)
cypress.config.ts
(2 hunks)
🔇 Additional comments (1)
cypress.config.ts (1)
38-40
:
Verify environment variable handling.
- Removing the fallback value for
API_URL
could cause runtime issues ifREACT_CARE_API_URL
is undefined - Inconsistent handling between
API_URL
(no fallback) andENABLE_HCX
(has fallback)
Consider adding validation:
env: {
- API_URL: process.env.REACT_CARE_API_URL,
+ API_URL: process.env.REACT_CARE_API_URL || (() => {
+ throw new Error('REACT_CARE_API_URL environment variable is required');
+ })(),
ENABLE_HCX: process.env.REACT_ENABLE_HCX ?? false,
},
CARE Run #3944
Run Properties:
|
Project |
CARE
|
Branch Review |
cypress-env
|
Run status |
Passed #3944
|
Run duration | 05m 10s |
Commit |
4d5929f8d4: Fixed the cypress env config file and removed hardcoded fallback
|
Committer | Mohammed Nihal |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
135
|
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/pageobject/Patient/PatientFileupload.ts
(1 hunks)src/components/Files/AudioCaptureDialog.tsx
(1 hunks)
🔇 Additional comments (1)
src/components/Files/AudioCaptureDialog.tsx (1)
136-136
: LGTM! Adding ID improves test stability.
The addition of id="start-recording"
provides a more reliable selector for Cypress tests.
Let's verify if the reduced wait time in Cypress tests (from 5000ms to 2000ms) is reliable:
✅ Verification successful
Based on the shell script results, I can see that:
- The
getUserMedia
operation in AudioCaptureDialog.tsx is asynchronous and affects the recording button's availability - The Cypress test file (PatientFileupload.ts) shows that after clicking the start recording button, there's a 2000ms wait before clicking stop
- The component handles microphone permissions asynchronously before enabling recording
Let's verify if there are any other timing-related operations in the component:
Reduced wait time in tests is reliable for AudioCaptureDialog component
The 2000ms wait time is appropriate because:
- The component handles microphone permissions asynchronously before enabling recording
- The timer starts only after permissions are granted and recording begins
- The component has proper state management (
WAITING_TO_RECORD
,RECORDING
,PERMISSION_DENIED
,RECORDED
) - The test waits for the stable
#start-recording
selector before interacting - No other async operations block the recording functionality after permissions are granted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions or timing issues in the component
# Look for async operations that might affect the recording button's availability
rg -A 5 "getUserMedia|setTimeout|setInterval|Promise" src/components/Files/AudioCaptureDialog.tsx
# Check if there are any other Cypress commands with longer wait times
rg -A 5 "cy\.wait|cy\.intercept" cypress/
Length of output: 64117
Script:
#!/bin/bash
# Check for timer-related code and state management in AudioCaptureDialog.tsx
rg -A 10 "timer\.|useState|useEffect" src/components/Files/AudioCaptureDialog.tsx
Length of output: 2305
cy.wait(2000); | ||
cy.get("#start-recording").click(); | ||
cy.wait(2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider replacing hard-coded waits with proper element state checks
The current implementation uses multiple cy.wait()
calls with hard-coded timeouts, which could make tests flaky. Consider making the following improvements:
recordAudio() {
cy.get("#record-audio").click();
- cy.wait(2000);
- cy.get("#start-recording").click();
- cy.wait(2000);
+ cy.get("#start-recording")
+ .should("be.visible")
+ .and("be.enabled")
+ .click();
+ // Wait for recording to start
+ cy.get("#recording-indicator")
+ .should("be.visible")
+ .and("have.class", "recording");
cy.get("#stop-recording").click();
- cy.wait(1000);
+ // Wait for recording to complete
+ cy.get("#recording-indicator")
+ .should("not.have.class", "recording");
cy.get("#save-recording").click();
}
This refactor:
- Removes arbitrary wait times
- Adds proper element state checks
- Makes the test more reliable by waiting for actual UI states
- Follows Cypress best practices for handling async operations
Note: Adjust the selectors (#recording-indicator
) and classes (recording
) according to your actual implementation.
Committable suggestion skipped: line range outside the PR's diff.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
.env
file for improved flexibility.API_URL
, streamlining how the application retrieves its API endpoint.AudioCaptureDialog
component.recordAudio
method for a more responsive recording experience.