Update Sample File Upload and upgrade buttons to Shadcn button #9157
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Patient/UpdateStatusDialog.tsx (2)
239-245: Enhance accessibility of the clear file buttonWhile the implementation works, consider improving accessibility by:
- Adding an aria-label
- Including a tooltip to clarify the action
{file && ( <CareIcon icon="l-times" className="text-lg cursor-pointer mt-2 mr-4" onClick={removeFile} + aria-label="Clear selected file" + title="Clear selected file" /> )}
Missing test coverage for file upload functionality
Based on the verification attempts, there appears to be no existing test coverage for the UpdateStatusDialog component. The component handles critical functionality including:
- File upload state management
- API integration with
routes.createUpload- Status updates for sample tests
- Multiple user interactions (file selection, removal, upload)
Action items:
- Create test suite for UpdateStatusDialog component
- Cover core scenarios:
- File selection and removal workflow
- Upload state transitions
- API integration with proper mocking
- Status update validations
- Edge cases (upload cancellation, invalid files)
🔗 Analysis chain
Line range hint
157-249: Verify test coverage for the new functionalityPlease ensure test coverage for:
- File selection and clearing workflow
- Upload button state transitions
- Edge cases (clearing during upload, multiple clear attempts)
Would you like help creating test cases for the new functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test files and their coverage fd -e test.tsx -e spec.tsx UpdateStatusDialogLength of output: 47
Script:
#!/bin/bash # Let's try a broader search for test files related to Patient components fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . src/components/Patient/ # Also search for any test files containing UpdateStatusDialog rg -l "UpdateStatusDialog" -g "*.test.*" -g "*.spec.*" # Check the implementation details to understand testable scenarios ast-grep --pattern 'const handleUpload = async () => { $$$ }'Length of output: 1994
Script:
#!/bin/bash # Let's check for any existing test setup patterns in the Patient directory rg -l "render|describe|test|it\(" --type ts --type tsx src/components/Patient/ # Look for any mocks or test utilities that might be relevant fd -e mock.ts -e mock.tsx -e test-utils.ts -e test-utils.tsx . src/ # Check for any existing dialog component tests as reference rg -l "ConfirmDialog.*test" --type ts --type tsxLength of output: 257
🧰 Tools
🪛 Biome
[error] 249-249: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/UpdateStatusDialog.tsx(2 hunks)
🧰 Additional context used
🪛 Biome
src/components/Patient/UpdateStatusDialog.tsx
[error] 249-249: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (1)
src/components/Patient/UpdateStatusDialog.tsx (1)
157-159: LGTM: Clear implementation of file removal functionality
The removeFile function provides a clean way to clear the selected file, directly addressing the issue #9153.
| <Submit | ||
| type="submit" | ||
| onClick={handleUpload} | ||
| disabled={uploadDone} | ||
| disabled={file ? false : true} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the disabled prop logic
The current boolean expression can be simplified.
- disabled={file ? false : true}
+ disabled={!file}📝 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.
| disabled={file ? false : true} | |
| disabled={!file} |
🧰 Tools
🪛 Biome
[error] 249-249: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
There was a problem hiding this comment.
@nithish1018 Great! Thanks for making the change.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/UpdateStatusDialog.tsx (1)
157-159: Reset contentType state when clearing fileThe
removeFilefunction should also reset thecontentTypestate to maintain consistency with the file selection logic.const removeFile = () => { setfile(undefined); + setcontentType(""); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/UpdateStatusDialog.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Patient/UpdateStatusDialog.tsx (1)
246-246: LGTM! Clean boolean expression
The simplified disabled prop logic is more readable and maintainable.
| {file && ( | ||
| <CareIcon | ||
| icon="l-times" | ||
| className="text-lg cursor-pointer mt-2 mr-4" | ||
| onClick={removeFile} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for the clear file button
The clear file icon button needs accessibility improvements to help screen reader users understand its purpose.
{file && (
<CareIcon
icon="l-times"
className="text-lg cursor-pointer mt-2 mr-4"
onClick={removeFile}
+ role="button"
+ aria-label={t("clear_file")}
+ title={t("clear_file")}
/>
)}Committable suggestion skipped: line range outside the PR's diff.
| const removeFile = () => { | ||
| setfile(undefined); | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add upload state check to prevent file removal during upload
The verification reveals that uploadStarted state is properly managed in the component and is used during file uploads. However, the removeFile function currently lacks this check, which could lead to inconsistent state if a file is removed during upload.
- Add upload state check in
removeFilefunction to prevent file removal during active uploads - Consider adding proper ARIA attributes and visual feedback for better accessibility
The suggested changes in the review comment are valid and necessary:
- Prevent file removal during upload with the upload state check
- Add proper visual and accessibility attributes to the clear button
🔗 Analysis chain
Verify clear file behavior during upload
Please ensure that the clear file functionality is properly disabled when a file upload is in progress to prevent potential issues.
Consider adding an upload state check:
const removeFile = () => {
+ if (uploadStarted) return;
setfile(undefined);
setcontentType("");
}; {file && (
<CareIcon
icon="l-times"
className="text-lg cursor-pointer mt-2 mr-4"
onClick={removeFile}
+ style={{ opacity: uploadStarted ? 0.5 : 1 }}
+ role="button"
+ aria-disabled={uploadStarted}
/>
)}Also applies to: 239-246
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any upload state checks before allowing file removal
rg -A 5 "uploadStarted.*removeFile|removeFile.*uploadStarted" .
Length of output: 65
Script:
#!/bin/bash
# Let's first check if uploadStarted state exists in the file
rg -A 5 "uploadStarted|setUploadStarted" src/components/Patient/UpdateStatusDialog.tsx
# Also check the component's state management
ast-grep --pattern 'const [$_] = useState<$_>($$$)' src/components/Patient/UpdateStatusDialog.tsx
Length of output: 1590
There was a problem hiding this comment.
@nithish1018 we need to migrate the entire flow to use the new useFileUpload and useFileManager hooks. Look at consent and file upload component for reference.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/UpdateStatusDialog.tsx (1)
102-110: Enhance error handling with specific messagesWhile the error handling is good, consider providing more specific error messages to help users understand and resolve issues.
- Notification.Error({ msg: `Upload failed: ${fileUpload.error}` }); + Notification.Error({ + msg: fileUpload.error === 'FILE_SIZE_ERROR' + ? t('file_size_exceeded_error') + : fileUpload.error === 'FILE_TYPE_ERROR' + ? t('invalid_file_type_error') + : t('upload_failed_try_again') + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/UpdateStatusDialog.tsx(5 hunks)
🔇 Additional comments (3)
src/components/Patient/UpdateStatusDialog.tsx (3)
166-172: **** Previous review comment about enhancing accessibility for the clear file button is still valid.
174-179: LGTM! Upload button implementation is solid
The button's disabled state and internationalization implementation look good.
62-66: Verify allowed file extensions match requirements
The file upload configuration looks good, but please verify that the allowed file extensions match the business requirements for sample test reports.
✅ Verification successful
Let me search for more context about the file upload configurations and their requirements.
Let me check the file extension constants defined in the codebase.
File upload configuration aligns with system standards
The allowed file extensions in UpdateStatusDialog.tsx (pdf, jpg, jpeg, png) are consistent with the system's previewable file extensions defined in constants and match the configuration used in other patient-related components. These formats are suitable for sample test reports as they cover both document (PDF) and image formats commonly used in medical documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other file upload configurations in the codebase
rg -A 3 "allowedExtensions.*pdf|allowedExtensions.*jpg|allowedExtensions.*jpeg|allowedExtensions.*png" .
Length of output: 686
Script:
#!/bin/bash
# Search for any documentation or comments about file upload requirements
rg -i "file.*upload.*requirement|document.*upload.*requirement|upload.*extension|file.*extension" .
# Search for any constants or configurations related to file uploads
ast-grep --pattern 'const $_ = {
$$$
allowedExtensions: $$$
$$$
}'
Length of output: 3977
Script:
#!/bin/bash
# Check the file extension constants
cat src/common/constants.tsx | grep -A 10 "FILE_EXTENSIONS"
Length of output: 419
nihal467
added
needs testing
needs review
changes required
and removed
changes required
needs testing
needs review
labels
|
check the code rabbit suggestion |
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Patient/UpdateStatusDialog.tsx (2)
64-68: Consider extracting configuration values to constantsThe file upload configuration contains magic values that could be moved to constants for better maintainability.
+const ALLOWED_FILE_EXTENSIONS = ["pdf", "jpg", "jpeg", "png"] as const; +const UPLOAD_TYPE = "SAMPLE_MANAGEMENT" as const; const fileUpload = useFileUpload({ - type: "SAMPLE_MANAGEMENT", - allowedExtensions: ["pdf", "jpg", "jpeg", "png"], + type: UPLOAD_TYPE, + allowedExtensions: ALLOWED_FILE_EXTENSIONS, allowNameFallback: true, });
104-117: Enhance error messages for better user experienceThe error messages could be more specific to help users understand and resolve issues better.
if (fileUpload.files.length > 0) { if (sample.id) { await fileUpload.handleFileUpload(sample.id); } else { - Notification.Error({ msg: "Sample ID is missing" }); + Notification.Error({ msg: t("error_missing_sample_id") }); } if (!fileUpload.error) { - Notification.Success({ msg: "File Uploaded Successfully" }); + Notification.Success({ msg: t("success_file_upload") }); } else { Notification.Error({ msg: `Upload failed: ${fileUpload.error}` }); } } else { - Notification.Error({ msg: "No file selected for upload" }); + Notification.Error({ msg: t("error_no_file_selected") }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/components/Patient/UpdateStatusDialog.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🔇 Additional comments (2)
src/components/Patient/UpdateStatusDialog.tsx (2)
180-185: LGTM! Upload button implementation is well done
The button implementation follows best practices with proper:
- Disabled state management
- Internationalization
- Visual feedback with icon
172-178: Verify clear file functionality implementation
The implementation aligns with the PR objectives to add clear image functionality. Let's verify its usage across the codebase.
✅ Verification successful
Clear file functionality is consistently implemented across the codebase
The implementation in UpdateStatusDialog.tsx follows the established pattern used in other components:
FileUpload.tsx- UsesclearFileswith both button and icon variantsPatientConsentRecords.tsx- Implements clear functionality with a trash icon- All components properly utilize the
useFileUploadhook and itsclearFilesmethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the clear file functionality implementation
# Check for other instances where clearFiles might be used
rg -A 5 "clearFiles|useFileUpload" .
# Check for similar file upload implementations that might benefit from this pattern
ast-grep --pattern 'useFileUpload({
$$$
})'
Length of output: 8058
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Shifting/ShiftDetails.tsx (1)
495-500: LGTM! Consider adding type safety for status checks.The disabled condition and onClick handler are well-implemented. However, consider defining an enum or union type for the status to make the code more type-safe.
type ShiftingStatus = 'COMPLETED' | 'CANCELLED' | 'PENDING' | 'APPROVED';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Shifting/ShiftDetails.tsx(1 hunks)src/components/ui/button.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/button.tsx
🔇 Additional comments (1)
src/components/Shifting/ShiftDetails.tsx (1)
505-508: LGTM!
The print referral button implementation is clean and follows React best practices.
nihal467
added
tested
reviewed
|
LGTM |
rithviknishad
added
changes required
and removed
reviewed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/UpdateStatusDialog.tsx (1)
64-68: Consider adding i18n support for file type validationThe hook configuration is good, but error messages for invalid file types should be internationalized.
const fileUpload = useFileUpload({ type: "SAMPLE_MANAGEMENT", allowedExtensions: ["pdf", "jpg", "jpeg", "png"], allowNameFallback: true, + invalidTypeMessage: t("invalid_file_type"), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Patient/UpdateStatusDialog.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Patient/UpdateStatusDialog.tsx (1)
Learnt from: nithish1018
PR: ohcnetwork/care_fe#9157
File: src/components/Patient/UpdateStatusDialog.tsx:235-244
Timestamp: 2024-11-28T04:24:30.394Z
Learning: The existing `FileUpload` component does not implement keyboard accessibility enhancements like adding role attributes, tabIndex, or onKeyDown handlers to labels.
🔇 Additional comments (3)
src/components/Patient/UpdateStatusDialog.tsx (3)
1-25: LGTM! Import changes align with requirements
The changes properly import the shadcn Button component and the new useFileUpload hook while maintaining proper type safety with SampleTestModel.
165-207: LGTM! UI changes meet requirements
The implementation successfully:
- Uses shadcn Button components with appropriate variants
- Provides clear file functionality as required
- Maintains consistent UI with proper progress indication
Note: The file input label maintains consistency with the existing FileUpload component regarding keyboard accessibility.
102-121: 🛠️ Refactor suggestion
Improve error handling structure and messages
The current implementation can be simplified and improved:
- Use early returns to reduce nesting
- Add i18n support for error messages
- Improve error message clarity
const handleUpload = async () => {
- if (fileUpload.files.length > 0) {
- if (!fileUpload.fileNames[0]) {
- Notification.Error({
- msg: "Please enter a file name before uploading",
- });
- return;
- }
- if (sample.id) {
- await fileUpload.handleFileUpload(sample.id);
- if (!fileUpload.error) {
- return;
- } else {
- Notification.Error({ msg: `Upload failed: ${fileUpload.error}` });
- }
- } else {
- Notification.Error({ msg: "Sample ID is missing" });
- }
- } else {
- Notification.Error({ msg: "No file selected for upload" });
- }
+ if (!fileUpload.files.length) {
+ Notification.Error({ msg: t("no_file_selected") });
+ return;
+ }
+
+ if (!fileUpload.fileNames[0]) {
+ Notification.Error({ msg: t("file_name_required") });
+ return;
+ }
+
+ if (!sample.id) {
+ Notification.Error({ msg: t("sample_id_missing") });
+ return;
+ }
+
+ await fileUpload.handleFileUpload(sample.id);
+ if (fileUpload.error) {
+ Notification.Error({ msg: t("upload_failed", { error: fileUpload.error }) });
+ }
};Likely invalid or redundant comment.
|
@nithish1018 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
New Features
primarybutton variant for enhanced visual options.Bug Fixes