Fixes deploy: missing plugin map missing import; calendar component; and other minor fixes #9632
Conversation
WalkthroughThe pull request introduces several modifications across different files. In Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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. |
rithviknishad
changed the title
![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
🧹 Nitpick comments (3)
src/Utils/request/errorHandler.ts (2)
2-2: Consider unifying toast usage with the existing Notifications system.
Usingtoastfrom "sonner" introduces a second notification mechanism. For consistency, consider unifying the approach to maintain a standardized user experience.
23-27: Internationalize the'Not found'toast message.
If the rest of the app employs i18n for user-facing strings, localizing this error message will ensure consistency.src/components/ui/calendar.tsx (1)
63-64: Ensure component renaming aligns with desired semantics.
WhileIconLeftandIconRightmight be clear internally, confirm they convey navigational context for the next developers. Alternatively, renaming them toNavLeftandNavRightor something similar might be more intuitive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(5 hunks)src/Utils/request/errorHandler.ts(3 hunks)src/components/Resource/ResourceFilter.tsx(1 hunks)src/components/Users/CreateUserForm.tsx(2 hunks)src/components/ui/calendar.tsx(1 hunks)
🔇 Additional comments (6)
src/Utils/request/errorHandler.ts (1)
62-64: Minimalistic 404 check looks great.
The isNotFound helper function is concise, self-descriptive, and effective at capturing 404 responses.
src/components/Resource/ResourceFilter.tsx (1)
28-34: Initializing filters with null is a sensible approach.
Switching from empty strings to null can reduce confusion around unset values, especially when checking for the presence of a filter. Verify any code that expects string defaults still functions correctly.
src/components/Users/CreateUserForm.tsx (2)
277-277: Good use of type="tel" for phone number.
This ensures the correct input type on mobile devices. For further clarity and validation, you could incorporate additional attributes (e.g., pattern or inputMode) to guide user input.
293-293: Revisit disabling the alt phone field when WhatsApp is checked.
If a user wants to enter a different alt phone number, they must uncheck “WhatsApp number is same as phone number.” Make sure the user experience is clear: consider adding a tooltip or helper text so they understand why the field is disabled.
package.json (2)
62-67: LGTM! New Radix UI components added.
The addition of Radix UI components (@radix-ui/react-dialog, @radix-ui/react-label, @radix-ui/react-popover) aligns with the PR objectives for UI enhancements. The versions are stable and compatible with existing Radix dependencies.
Run the following script to verify the usage of new components:
✅ Verification successful
All newly added Radix UI components are actively used in the codebase
The verification confirms that all the newly added Radix UI components (@radix-ui/react-dialog, @radix-ui/react-label, @radix-ui/react-popover) are being actively used throughout the codebase:
- Dialog components are used in multiple features including file management, encounters, patient registration, and medicine administration
- Label components are extensively used in forms, questionnaires, and user interfaces
- Popover components are implemented in date pickers, user selectors, and various interactive elements
The components are properly integrated into the application's UI system and follow consistent usage patterns, confirming that these dependencies are necessary additions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of newly added Radix UI components
echo "Checking Dialog component usage..."
rg "Dialog\." --type ts --type tsx
echo "Checking Label component usage..."
rg "Label\." --type ts --type tsx
echo "Checking Popover component usage..."
rg "Popover\." --type ts --type tsx
Length of output: 431
Script:
#!/bin/bash
# Description: Verify the usage of newly added Radix UI components with correct file type flags
echo "Checking Dialog component usage..."
rg "from '@radix-ui/react-dialog'"
echo "Checking Label component usage..."
rg "from '@radix-ui/react-label'"
echo "Checking Popover component usage..."
rg "from '@radix-ui/react-popover'"
# Also check for any direct component usage
echo "Checking component usage patterns..."
rg "<(Dialog|Label|Popover)"
Length of output: 19670
113-113: Verify the react-day-picker downgrade impact.
The downgrade from ^9.5.0 to ^8.10.1 is a major version change that could affect calendar functionality. Please ensure:
- All calendar features work as expected
- No breaking changes affect the application
- Compatibility with date-fns ^3.6.0 is maintained
Run the following script to check for potential breaking changes:
✅ Verification successful
The react-day-picker downgrade is safe and compatible
The implementation review shows that the downgrade from v9 to v8 is safe because:
- The components only use basic features that are stable across v8 and v9 (
DayPicker,DateRange) - The styling and functionality implementation uses standard props (
showOutsideDays,mode,selected, etc.) - The date-fns dependency is compatible, as react-day-picker v8.10.1 supports both date-fns v2 and v3
The calendar components are using a straightforward implementation that focuses on:
- Basic date picking functionality
- Range selection support
- Custom styling through className props
- Standard navigation with chevron icons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for calendar-related component usage that might be affected by the downgrade
# Search for react-day-picker component usage
echo "Checking react-day-picker usage patterns..."
rg -l "react-day-picker|DayPicker|DatePicker"
# Search for specific calendar components that might be affected
echo "Checking calendar component implementations..."
ast-grep --pattern 'import { $$$ } from "react-day-picker"'
Length of output: 807
Script:
#!/bin/bash
# Let's check the actual usage in calendar components to understand the impact
# Check calendar.tsx implementation
echo "=== calendar.tsx implementation ==="
cat src/components/ui/calendar.tsx
# Check date-range-picker.tsx implementation
echo -e "\n=== date-range-picker.tsx implementation ==="
cat src/components/ui/date-range-picker.tsx
# Check if there are any specific v9 features being used
echo -e "\n=== Checking for v9 specific features ==="
rg "DayPicker|DateRange" -A 5 -B 5 src/components/ui/calendar.tsx src/components/ui/date-range-picker.tsx
# Check package-lock.json for date-fns version compatibility
echo -e "\n=== Checking date-fns dependency tree ==="
rg '"date-fns"' package-lock.json -A 2
Length of output: 9122
Deploying care-fe with
|
| Latest commit: |
5f11ced
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4ef644f4.care-fe.pages.dev |
| Branch Preview URL: | https://rithviknishad-enhancements-2.care-fe.pages.dev |
rithviknishad
changed the title
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
rithviknishad/enhancements-2
|
| Run status |
|
| Run duration | 01m 09s |
| Commit |
|
| Committer | Rithvik Nishad |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
3
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Utils/request/errorHandler.ts(3 hunks)src/Utils/request/uploadFile.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/request/errorHandler.ts
| Notification.BadRequest({ errs: error.errors }); | ||
| reject(new Error("Client error")); |
There was a problem hiding this comment.
Eliminate redundant reject call and handle fallback for string errors.
Currently, the code calls reject(new Error("Client error")) twice in succession, which is unnecessary. After the first reject() call, the Promise is already rejected, so this second call never takes effect. Moreover, if JSON parsing fails, error will just be a string, and accessing error.errors would be undefined. A more resilient approach might handle both object and string cases.
Consider this patch to remove the duplicated reject call and safeguard against string-based errors:
- Notification.BadRequest({ errs: error.errors });
- reject(new Error("Client error"));
- reject(new Error("Client error"));
+ if (typeof error === "object" && "errors" in error) {
+ Notification.BadRequest({ errs: error.errors });
+ } else {
+ Notification.BadRequest({ errs: [String(error)] });
+ }
+ reject(new Error("Client error"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Notification.BadRequest({ errs: error.errors }); | |
| reject(new Error("Client error")); | |
| if (typeof error === "object" && "errors" in error) { | |
| Notification.BadRequest({ errs: error.errors }); | |
| } else { | |
| Notification.BadRequest({ errs: [String(error)] }); | |
| } | |
| reject(new Error("Client error")); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
16-19: Document the build process changesThe addition of
postinstallandsetupsteps suggests a significant change in the build process. Consider adding comments to explain:
- Why
--ignore-scriptsis necessary- What each step (
postinstallandsetup) accomplishes- The dependency order of these steps
COPY package.json package-lock.json ./ +# Prevent execution of third-party scripts during installation for security RUN npm install --ignore-scripts COPY . . +# Run controlled post-installation steps RUN npm run postinstall +# Execute application-specific setup RUN npm run setup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/deploy.yaml(0 hunks)Dockerfile(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/deploy.yaml
🔇 Additional comments (1)
Dockerfile (1)
12-12: Verify the impact of --ignore-scripts flag
While using --ignore-scripts is a good security practice, ensure that all necessary setup steps are properly handled by the subsequent postinstall and setup commands. This flag might be related to the plugin map issue mentioned in the PR.
Let's verify what scripts would normally run during installation:
✅ Verification successful
Let me gather more information about the setup scripts that are being run after the npm install.
Using --ignore-scripts is safe in this setup
The --ignore-scripts flag in the Dockerfile is safe because:
- The critical setup steps are explicitly handled by dedicated scripts that run after the installation:
postinstallscript installs platform-specific dependencies for rollup and esbuildsetupscript generates the plugin map for enabled Care apps
- These scripts are executed through explicit RUN commands in the Dockerfile rather than npm lifecycle hooks, ensuring better control and transparency in the build process.
The flag actually helps by:
- Preventing execution of third-party package scripts during installation
- Maintaining explicit control over which setup steps run and when
- Improving build security by avoiding potentially harmful scripts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json for scripts that might be affected
echo "Checking package.json for lifecycle scripts:"
jq '.scripts' package.json
# List dependencies that might have install/postinstall scripts
echo -e "\nChecking dependencies for install scripts:"
jq '.dependencies | keys[]' package.json | xargs -I {} sh -c 'if [ -f "node_modules/{}/package.json" ]; then echo "{}:" && jq ".scripts | select(.install, .postinstall)" "node_modules/{}/package.json"; fi'
Length of output: 1490
Script:
#!/bin/bash
# Check the content of setup and postinstall scripts
echo "Content of setup script:"
cat scripts/setup-care-apps.ts || echo "File not found"
echo -e "\nContent of postinstall script:"
cat scripts/install-platform-deps.ts || echo "File not found"
# Check if there are any other setup-related scripts
echo -e "\nOther setup-related files:"
fd -t f . scripts/ -x echo "=== {}" \; -x cat "{}" \; | grep -A 5 "==="
Length of output: 4387
|
@rithviknishad 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
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
Dependencies
react-day-pickerversion.date-fnsdependency.Error Handling
User Interface
Workflow
Docker