Implemented Next and Previous button for files preview #9196
Conversation
WalkthroughThe changes involve modifications to several components and hooks within the codebase to enhance file management and preview capabilities. The Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ 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: 2
🧹 Outside diff range and nitpick comments (5)
src/components/Common/FilePreviewDialog.tsx (3)
80-86: Consider simplifying the useEffectThe current implementation can be simplified by removing the redundant uploadedFiles check since the show condition is sufficient.
useEffect(() => { - if (uploadedFiles && show) { + if (show) { setIndex(currentIndex); } }, [uploadedFiles, show, currentIndex]);
149-149: Improve metadata display with optional chaining and accessibilityThe file metadata section could benefit from:
- Using optional chaining as suggested by static analysis
- Adding proper ARIA labels for screen readers
- <span className="text-sm text-gray-600">{t("file_preview")}</span> + <span className="text-sm text-gray-600" role="heading" aria-level="1">{t("file_preview")}</span> <div className="mb-2 flex flex-col items-start justify-between md:flex-row"> - <div> + <div role="contentinfo" aria-label="File information"> <p className="text-xl font-semibold text-gray-800"> {file_state.name}.{file_state.extension} </p> - {uploadedFiles && - uploadedFiles[index] && - uploadedFiles[index].created_date && ( + {uploadedFiles?.[index]?.created_date && ( <p className="text-sm text-gray-500"> Created on{" "} {new Date( - uploadedFiles[index].created_date!, + uploadedFiles[index].created_date, ).toLocaleString()} </p> )} </div>Also applies to: 154-169
187-194: Add keyboard navigation supportWhile the navigation buttons are implemented correctly, consider adding keyboard support for better accessibility.
<ButtonV2 className="cursor-pointer bg-primary-500 rounded-md mr-2" onClick={() => handleNext(index - 1)} + aria-label="Previous file" + onKeyDown={(e) => e.key === 'ArrowLeft' && handleNext(index - 1)} > <CareIcon icon="l-arrow-left" className="h-4 w-4" /> </ButtonV2> {/* ... */} <ButtonV2 className="cursor-pointer bg-primary-500 rounded-md" onClick={() => handleNext(index + 1)} + aria-label="Next file" + onKeyDown={(e) => e.key === 'ArrowRight' && handleNext(index + 1)} > <CareIcon icon="l-arrow-right" className="h-4 w-4" /> </ButtonV2>Also applies to: 233-243
src/hooks/useFileManager.tsx (2)
85-86: Add validation for file.idWhile the index finding logic is correct, consider validating
file.idbefore searching to prevent potential issues.-const index = uploadedFiles?.findIndex((f) => f.id === file.id) ?? -1; +const index = file.id && uploadedFiles?.findIndex((f) => f.id === file.id) ?? -1;
232-237: Consider memoizing FilePreviewDialog propsWhile the props are correctly passed, consider memoizing the component or its callbacks to prevent unnecessary re-renders.
+const handleLoadFile = useCallback( + (file: FileUploadModel, associating_id: string) => { + viewFile(file, associating_id); + }, + [viewFile] +); <FilePreviewDialog show={file_state.open} fileUrl={fileUrl} file_state={file_state} setFileState={setFileState} downloadURL={downloadURL} uploadedFiles={uploadedFiles} onClose={handleFilePreviewClose} fixedWidth={false} className="h-[80vh] w-full md:h-screen" - loadFile={viewFile} + loadFile={handleLoadFile} currentIndex={currentIndex} />
📜 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 (5)
.prettierignore(1 hunks)src/components/Common/FilePreviewDialog.tsx(8 hunks)src/components/Files/FileUpload.tsx(2 hunks)src/components/Patient/FileUploadPage.tsx(0 hunks)src/hooks/useFileManager.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/components/Patient/FileUploadPage.tsx
✅ Files skipped from review due to trivial changes (1)
- .prettierignore
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Common/FilePreviewDialog.tsx
[error] 159-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/components/Common/FilePreviewDialog.tsx (1)
46-48: LGTM: Props structure aligns with navigation requirements
The new props (uploadedFiles, loadFile, currentIndex) are well-typed and properly structured to support the file navigation feature.
src/components/Files/FileUpload.tsx (3)
72-73: LGTM: Interface changes support file navigation feature
The addition of optional id and associating_id fields to StateInterface appropriately supports the file navigation functionality.
Line range hint 1-1: Verify implementation of remaining PR objectives
Several PR objectives don't appear to be implemented in this file:
- Next/Previous navigation buttons
- File creation timestamp display
- Font size adjustments
Let's locate the remaining implementations:
213-220:
Consider handling file ordering at the API level
The current implementation of reversing files has several issues:
- Performance:
slice().reverse()creates a new array on every render - Consistency: Reversing after pagination could lead to incorrect ordering as you're only reversing the current page
- Pagination: The current approach doesn't provide true oldest-first ordering across pages
Consider these alternatives:
- Preferred: Handle ordering at the API level by adding a
sortparameter:
const activeFilesQuery = useQuery(routes.viewUpload, {
query: {
file_type: type,
associating_id: associatedId,
is_archived: false,
limit: RESULTS_PER_PAGE_LIMIT,
offset: offset,
+ sort_by: "created_at",
+ order: "asc",
},
});- If API changes aren't possible, move the reversal logic to the query transform:
- uploadedFiles:
- fileQuery?.data?.results
- .slice()
- .reverse()
- .map((file) => ({
- ...file,
- associating_id: associatedId,
- })) || [],
+ uploadedFiles: (fileQuery?.data?.results || []).map((file) => ({
+ ...file,
+ associating_id: associatedId,
+ })),Let's verify if the API supports sorting:
src/hooks/useFileManager.tsx (2)
52-52: LGTM! Clean destructuring of new prop
The new uploadedFiles prop is correctly destructured alongside existing options.
27-27: LGTM! Verify usage of optional prop
The optional uploadedFiles prop is correctly typed and maintains backward compatibility.
✅ Verification successful
Optional uploadedFiles prop is safely implemented
The verification shows that the optional uploadedFiles prop maintains backward compatibility:
PatientConsentRecords.tsxand one instance inFileUpload.tsxdon't provide the prop, working with the default undefined value- Only one instance in
FileUpload.tsxuses the new prop withfileQuery?.data?.results
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that existing usages of useFileManager don't break
rg "useFileManager\(" --type typescript
Length of output: 76
Script:
#!/bin/bash
# Search for useFileManager hook usage with common TypeScript/JavaScript file extensions
rg "useFileManager\(" -t js -t jsx -t ts -t tsx
Length of output: 77
Script:
#!/bin/bash
# Find all JavaScript/TypeScript files first, then search for useFileManager usage
fd -e js -e jsx -e ts -e tsx | xargs rg "useFileManager\("
Length of output: 383
Script:
#!/bin/bash
# Check how useFileManager is used in the components
echo "=== PatientConsentRecords.tsx ==="
rg -A 5 "useFileManager\(" src/components/Patient/PatientConsentRecords.tsx
echo -e "\n=== FileUpload.tsx ==="
rg -A 5 "useFileManager\(" src/components/Files/FileUpload.tsx
Length of output: 723
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Common/FilePreviewDialog.tsx (2)
80-86: Add validation for index bounds in useEffectWhile the effect handles basic state updates, consider adding bounds checking to prevent invalid indices when uploadedFiles changes.
useEffect(() => { if (uploadedFiles && show) { + const validIndex = Math.min(Math.max(currentIndex, 0), uploadedFiles.length - 1); - setIndex(currentIndex); + setIndex(uploadedFiles.length > 0 ? validIndex : -1); } }, [uploadedFiles, show, currentIndex]);
162-171: Simplify conditional rendering with optional chainingThe nested conditions can be simplified using optional chaining for better readability.
- {uploadedFiles && - uploadedFiles[index] && - uploadedFiles[index].created_date && ( + {uploadedFiles?.[index]?.created_date && ( <p className="text-sm text-gray-500"> Created on{" "} {new Date( - uploadedFiles[index].created_date!, + uploadedFiles[index].created_date, ).toLocaleString()} </p> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 162-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/FilePreviewDialog.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Common/FilePreviewDialog.tsx
[error] 162-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/components/Common/FilePreviewDialog.tsx (2)
46-48: LGTM: Well-structured type definitions for file navigation
The new props provide proper typing for managing multiple files and navigation functionality.
104-119: LGTM: Navigation logic implements proper error handling
The implementation includes proper null checks and error handling as previously suggested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/components/Common/FilePreviewDialog.tsx (5)
46-48: Consider making currentIndex an optional propThe
currentIndexprop should be optional with a default value since there might be cases where we're showing a single file without navigation context.- currentIndex: number; + currentIndex?: number;
82-86: Optimize useEffect implementationThe effect can be simplified and should include cleanup for state reset:
useEffect(() => { - if (uploadedFiles && show) { - setIndex(currentIndex); - } + setIndex(show ? currentIndex : -1); + return () => setIndex(-1); }, [uploadedFiles, show, currentIndex]);
165-172: Use user's locale for timestamp formattingThe timestamp formatting is hardcoded to 'en-US'. Consider using the user's locale or a prop-based locale setting.
- ).toLocaleString("en-US", { + ).toLocaleString(undefined, { // Uses user's locale
193-200: Reduce code duplication in navigation buttonsThe Next and Previous button implementations are very similar. Consider extracting a reusable navigation button component.
type NavButtonProps = { direction: 'prev' | 'next'; onClick: () => void; }; const NavigationButton = ({ direction, onClick }: NavButtonProps) => ( <ButtonV2 className={`cursor-pointer bg-primary-500 rounded-md ${ direction === 'prev' ? 'mr-2' : '' }`} onClick={onClick} > <CareIcon icon={direction === 'prev' ? 'l-arrow-left' : 'l-arrow-right'} className="h-4 w-4" /> </ButtonV2> );Then use it like:
- {uploadedFiles && uploadedFiles.length > 1 && index > 0 && ( - <ButtonV2 - className="cursor-pointer bg-primary-500 rounded-md mr-2" - onClick={() => handleNext(index - 1)} - > - <CareIcon icon="l-arrow-left" className="h-4 w-4" /> - </ButtonV2> - )} + {uploadedFiles?.length > 1 && index > 0 && ( + <NavigationButton + direction="prev" + onClick={() => handleNext(index - 1)} + /> + )}Also applies to: 240-249
162-164: Use optional chaining for better readabilityThe nested condition checks can be simplified using optional chaining.
- {uploadedFiles && - uploadedFiles[index] && - uploadedFiles[index].created_date && ( + {uploadedFiles?.[index]?.created_date && (🧰 Tools
🪛 Biome (1.9.4)
[error] 162-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/FilePreviewDialog.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Common/FilePreviewDialog.tsx
[error] 162-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
Hi @rithviknishad, could you please review the PR as soon as possible? Let me know if it's good to merge, or feel free to suggest any changes. Thanks! |
nihal467
added
needs testing
needs review
and removed
question
…into issues/8321/Add_Next_and_Previous
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Common/FilePreviewDialog.tsx (4)
46-48: Consider improving type safety for navigation propsThe
loadFileprop is marked optional but is required whenuploadedFilesis present. Consider making this relationship explicit in the types.- uploadedFiles?: FileUploadModel[]; - loadFile?: (file: FileUploadModel, associating_id: string) => void; - currentIndex: number; + uploadedFiles?: { + files: FileUploadModel[]; + loadFile: (file: FileUploadModel, associating_id: string) => void; + currentIndex: number; + };
104-115: Simplify navigation handler with optional chainingThe navigation handler can be simplified while maintaining the same safety checks.
const handleNext = (newIndex: number) => { - if (!uploadedFiles?.length) return; - if (!loadFile) return; - if (newIndex < 0 || newIndex >= uploadedFiles.length) return; - - const nextFile = uploadedFiles[newIndex]; - if (!nextFile?.id) return; + const nextFile = uploadedFiles?.[newIndex]; + if (!nextFile?.id || !loadFile || newIndex < 0 || newIndex >= (uploadedFiles?.length ?? 0)) { + return; + } const associating_id = nextFile.associating_id || ""; loadFile(nextFile, associating_id); setIndex(newIndex); };
173-185: Simplify conditional rendering with optional chainingFollowing the static analysis suggestion, the nested conditions can be simplified.
- {uploadedFiles && - uploadedFiles[index] && - uploadedFiles[index].created_date && ( + {uploadedFiles?.[index]?.created_date && ( <p className="mt-1 text-sm text-gray-600"> Created on{" "} {new Date( - uploadedFiles[index].created_date!, + uploadedFiles[index].created_date, ).toLocaleString("en-US", { dateStyle: "long", timeStyle: "short", })} </p> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 173-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-206: Adjust navigation button margins for better spacingThe margins between the navigation buttons and the image container should be consistent.
- className="cursor-pointer bg-primary-500 rounded-md mr-4" + className="cursor-pointer bg-primary-500 rounded-md mx-6"- className="cursor-pointer bg-primary-500 rounded-md ml-4" + className="cursor-pointer bg-primary-500 rounded-md mx-6"Also applies to: 258-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/FilePreviewDialog.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Common/FilePreviewDialog.tsx
[error] 173-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/components/Common/FilePreviewDialog.tsx (2)
80-86: LGTM: Clean state management implementation
The state management for the file index is well-implemented with proper effect dependencies.
141-154: LGTM: Well-implemented keyboard navigation
The keyboard navigation implementation is clean with proper cleanup and matches the button behavior.
|
Hi @Jacobjeevan, I have updated the required changes, could you please review the PR again? |
|
LGTM |
|
Hi @rithviknishad could you please review? |
|
Hi @rithviknishad, do I need to make any changes, or is this PR good to merge? |
|
@sarvesh-official 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! 🙌 |
Add Next/Previous Navigation in File Preview Dialog
Fixes #8321
Changes Made
Implementation Details
Navigation Controls
File Order & Display
State Management
Screenshots/Videos
Next_and_Previous_Navigation.mp4
QA Checklist
Summary by CodeRabbit
Release Notes
New Features
FilePreviewDialog, allowing users to navigate through multiple uploaded files with improved display of file information.FileUploadcomponent.Bug Fixes
FileUploadPagecomponent for improved code cleanliness.Documentation
These changes aim to improve user experience by streamlining file handling and navigation within the application.