Integrated Excalidraw for drawing Files #10680
Conversation
WalkthroughThis pull request integrates Excalidraw drawing capabilities into the application. It adds new dependencies and localization strings, introduces new routes for patient and consultation drawing operations, and enhances navigation within file-related components. New components—ExcalidrawEditor and ExcalidrawView—handle drawing creation and viewing with lazy loading and suspense fallbacks. Additional utility functions and environment configuration updates further support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as FilesTab
participant R as Router
participant S as Suspense Loader
participant E as Excalidraw Component
U->>F: Clicks drawing button
F->>R: Calls navigate()
R->>S: Matches route & lazy loads component
S->>E: Renders ExcalidrawEditor/ExcalidrawView
E-->>U: Displays drawing interface
sequenceDiagram
participant U as User
participant E as ExcalidrawEditor
participant M as Mutation Handler
U->>E: Clicks Save button
E->>M: Initiates handleSave (upload process)
M-->>E: Returns success/error response
E-->>U: Displays notification via toast
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (8)
src/components/Files/ExcalidrawDialog.tsx (4)
40-40: Remove console.log statement.Remove debugging console.log statement before production deployment.
- console.log(data);
82-86: Consider using application/json content type for Excalidraw files.The current implementation uses text/plain which works but application/json would be more semantically correct for JSON data.
- const file = new File([JSON.stringify(obj)], `${name}.excalidraw`, { - type: "text/plain", - }); + const file = new File([JSON.stringify(obj)], `${name}.excalidraw`, { + type: "application/json", + });
164-166: Consider memoizing the debounced function.The current implementation creates a new debounced function on every render. Consider memoizing it with useCallback for better performance.
+ const debouncedOnChange = useCallback( + debounce(async (elements) => { + setElements(elements); + }, 100), + [] + ); return ( // ... <Excalidraw // ... - onChange={debounce(async (elements) => { - setElements(elements); - }, 100)} + onChange={debouncedOnChange} /> );
108-112: Improve error handling for upload failures.The current implementation only shows a generic error message. Consider providing more detailed error information to help users understand and resolve issues.
if (!upload.ok) { - toast.error("Error uploading file"); + const errorMessage = await upload.text(); + toast.error(`Error uploading file: ${errorMessage || upload.statusText}`); return; }src/components/Files/FilesTab.tsx (2)
112-113: Remove console.log statements.Remove debugging console.log statements before production deployment.
- console.log("facilityId", props.facilityId); - console.log("PatientId", props.patientId); - console.log("fileName", file.extension);Also applies to: 550-550
265-279: Consider using a type-safe routing solution.The current implementation uses template literals for route construction. Consider using a type-safe routing solution to prevent potential runtime errors.
For example, you could create a route builder utility:
const routes = { drawings: { list: (facilityId: string, patientId: string) => `/facility/${facilityId}/patient/${patientId}/drawings`, detail: (facilityId: string, patientId: string, drawingId: string) => `/facility/${facilityId}/patient/${patientId}/drawings/${drawingId}` } };Then use it in the component:
- onClick={() => - navigate( - `/facility/${props.facilityId}/patient/${props.patientId}/drawings/${file.id}`, - ) - } + onClick={() => + navigate(routes.drawings.detail(props.facilityId, props.patientId, file.id)) + }Also applies to: 443-457
src/lib/utils.ts (1)
8-19: Add error handling and cleanup mechanism.The debounce implementation could be improved with:
- Error handling for the callback execution
- A cleanup mechanism to prevent memory leaks
export function debounce<T extends unknown[], U>( callback: (...args: T) => PromiseLike<U> | U, wait: number ) { let timer: ReturnType<typeof setTimeout>; return (...args: T): Promise<U> => { clearTimeout(timer); - return new Promise((resolve) => { + return new Promise((resolve, reject) => { timer = setTimeout(() => { - resolve(callback(...args)) + try { + const result = callback(...args); + if (result instanceof Promise) { + result.then(resolve).catch(reject); + } else { + resolve(result); + } + } catch (error) { + reject(error); + } }, wait); }); }; }Additionally, consider adding a
cancelmethod to clear the timer when needed:export function debounce<T extends unknown[], U>( callback: (...args: T) => PromiseLike<U> | U, wait: number ) { let timer: ReturnType<typeof setTimeout>; const debouncedFn = (...args: T): Promise<U> => { clearTimeout(timer); return new Promise((resolve, reject) => { timer = setTimeout(() => { try { const result = callback(...args); if (result instanceof Promise) { result.then(resolve).catch(reject); } else { resolve(result); } } catch (error) { reject(error); } }, wait); }); }; debouncedFn.cancel = () => { clearTimeout(timer); }; return debouncedFn; }src/hooks/useFilters.tsx (1)
5-5: LGTM! Successful migration with room for improvement.The replacement of
classNameswithcnis correctly implemented. Consider extracting the base classes for better readability:- className={cn( - "flex w-full justify-center", - totalCount > limit ? "visible" : "invisible", - !noMargin && "mt-4", - )} + const baseClasses = "flex w-full justify-center" + className={cn( + baseClasses, + totalCount > limit ? "visible" : "invisible", + !noMargin && "mt-4", + )}Also applies to: 227-231
📜 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 (14)
package.json(3 hunks)public/locale/en.json(2 hunks)src/Routers/routes/PatientRoutes.tsx(3 hunks)src/Utils/utils.ts(0 hunks)src/components/Common/LanguageSelector.tsx(2 hunks)src/components/Common/LanguageSelectorLogin.tsx(2 hunks)src/components/Common/UpdatableApp.tsx(2 hunks)src/components/Files/ExcalidrawDialog.tsx(1 hunks)src/components/Files/FilesTab.tsx(7 hunks)src/components/Users/UserFormValidations.tsx(2 hunks)src/components/ui/sidebar/patient-switcher.tsx(2 hunks)src/hooks/useFilters.tsx(2 hunks)src/lib/utils.ts(1 hunks)vite.config.mts(1 hunks)
💤 Files with no reviewable changes (1)
- src/Utils/utils.ts
🔇 Additional comments (11)
src/Routers/routes/PatientRoutes.tsx (1)
16-18: LGTM! Well-implemented drawing routes with lazy loading.The implementation uses lazy loading for the ExcalidrawPage component and shows a loading indicator during component load. The routes are properly structured with required parameters.
Also applies to: 54-71
public/locale/en.json (1)
294-294: LGTM! Well-structured translations for new drawing features.The new translations follow the existing naming conventions and provide clear, user-friendly messages.
Also applies to: 1056-1056
src/components/Common/LanguageSelector.tsx (1)
32-35: LGTM!The migration from
classNamestocnutility is correctly implemented, maintaining proper class merging behavior.src/components/Common/LanguageSelectorLogin.tsx (1)
37-42: LGTM!The migration from
classNamestocnutility is correctly implemented, maintaining proper conditional class handling.src/components/Users/UserFormValidations.tsx (1)
75-81: LGTM!The migration from
classNamestocnutility is correctly implemented, maintaining proper conditional class handling for validation states.src/components/ui/sidebar/patient-switcher.tsx (1)
3-3: LGTM! Successful migration to the newcnutility.The replacement of the deprecated
classNamesutility withcnis correctly implemented, maintaining the same className concatenation functionality.Also applies to: 33-33
src/components/Common/UpdatableApp.tsx (1)
4-4: LGTM! Successful migration to the newcnutility.The replacement of
classNameswithcnis correctly implemented, maintaining the same conditional animation class functionality.Also applies to: 123-123
vite.config.mts (1)
164-164:✅ Verification successful
Verify Excalidraw's Preact configuration.
Let's confirm if setting
IS_PREACTto "true" is the recommended configuration for Excalidraw.
🌐 Web query:
Does Excalidraw require IS_PREACT environment variable to be set to true?💡 Result:
Excalidraw requires the
IS_PREACTenvironment variable to be set only when using Preact instead of React. This requirement exists because Excalidraw's UMD build includes React dependencies by default, which conflict with Preact[1][2][9].Key implementation details:
- Set
IS_PREACTas a string"true"in your build configuration:// vite.config.js define: { "process.env.IS_PREACT": JSON.stringify("true"), // Must be string "true" },
- Vite-specific configuration requires explicit declaration since Vite strips process.env variables by default[2][9].
This environment variable switch tells Excalidraw to use its Preact-compatible build instead of the default React build[1][2]. The requirement doesn't apply if you're using React or other frameworks without Preact integration[1].
Citations:
- 1: https://docs.excalidraw.com/docs/@excalidraw/excalidraw/integration
- 2: https://github.com/excalidraw/excalidraw/releases
- 3: https://www.npmjs.com/package/@excalidraw/excalidraw-preview
- 4: https://unpkg.com/browse/@excalidraw/[email protected]/README.md
- 5: Allow setting EXCALIDRAW_ASSET_PATH as env variable excalidraw/excalidraw#7672
- 6: https://zsviczian.github.io/obsidian-excalidraw-plugin/API/utility.html
- 7: https://unpkg.com/browse/@excalidraw/[email protected]/README.md
- 8: https://excalidraw-obsidian.online/wiki/settings
- 9: https://docs.excalidraw.com/docs/@excalidraw/excalidraw/faq
- 10: https://libraries.excalidraw.com
Excalidraw Preact Configuration Verified
Setting
"process.env.IS_PREACT"to"true"invite.config.mts(line 164) is correct. This configuration aligns with Excalidraw’s requirements when using Preact, as it instructs the library to use its Preact-compatible build. No changes are needed here.package.json (3)
98-100: LGTM! Dependencies align with drawing feature requirements.The added dependencies make sense for the drawing feature:
idb-keyvalfor local storage of drawingsjotaifor state management
120-120: LGTM! Minor version update.The update to
sonnerfrom 1.7.2 to 1.7.4 is a minor version bump, likely containing bug fixes.
52-52:✅ Verification successful
Verify Excalidraw version.
Let's check if there's a newer version of Excalidraw available with potential bug fixes or improvements.
🌐 Web query:
What is the latest version of @excalidraw/excalidraw?💡 Result:
The latest version of
@excalidraw/excalidrawis 0.17.6, released approximately 10 months ago as of February 2025[4][6][9]. This version includes features like:
- Programmatic API support for frames and element alignment[1]
- Enhanced security measures for URL sanitization and embed validation[1]
- Improved dark mode themes and UI components[1]
The package follows semantic versioning and maintains backward compatibility where possible, though some breaking changes were introduced in earlier versions (e.g., v0.16.x)[1]. For access to unreleased features, developers can use
@excalidraw/excalidraw@nextwhich includes experimental builds with commit hash suffixes[2][5].Citations:
- 1: https://github.com/excalidraw/excalidraw/releases
- 2: How to build @excalidraw/excalidraw package excalidraw/excalidraw#8207
- 3: https://forum.obsidian.md/t/excalidraw-full-featured-sketching-plugin-in-obsidian/17367
- 4: https://www.npmjs.com/package/@excalidraw/excalidraw/v/0.14.2
- 5: https://docs.excalidraw.com/docs/@excalidraw/excalidraw/installation
- 6: https://www.npmjs.com/package/@excalidraw/excalidraw
- 7: https://docs.excalidraw.com/docs/@excalidraw/excalidraw/integration
- 8: https://github.com/excalidraw/excalidraw
- 9: https://debricked.com/select/package/npm-@excalidraw/excalidraw
- 10: https://excalidraw.com
Excalidraw dependency is up-to-date.
The specified version, 0.17.6, is confirmed to be the latest available release as of February 2025. No action is required regarding an update for this dependency. The additional dependency updates and additions (idb-keyval, jotai, and the patch update for sonner) appear appropriate.
NikhilA8606
force-pushed
the
issues/#10616/Integrating-Excalidraw-for-drawing-files
branch
2 times, most recently
from
d2732f1 to
ed2bd40
Compare
NikhilA8606
force-pushed
the
issues/#10616/Integrating-Excalidraw-for-drawing-files
branch
from
ed2bd40 to
1837fbb
Compare
…k#10616/Integrating-Excalidraw-for-drawing-files
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/components/Files/ExcalidrawView.tsx (1)
40-42: Add loading state for fetch operation.The fetch operation should show a loading state while retrieving the drawing data.
+ const [isLoadingElements, setIsLoadingElements] = useState(false); const fetchData = async () => { + setIsLoadingElements(true); try { const response = await fetch(data.read_signed_url!); const json = await response.json(); setElements(json.elements); + } finally { + setIsLoadingElements(false); + } };Then update the loading check:
- if (isLoading || elements === null) { + if (isLoading || isLoadingElements || elements === null) {src/Routers/routes/PatientRoutes.tsx (2)
53-59: Maintain consistent parameter naming across routes.The parameter naming is inconsistent between routes (
:idvs:patientId). Consider using consistent parameter names for better maintainability.- "/facility/:facilityId/patient/:id/drawings": ({ id }) => { + "/facility/:facilityId/patient/:patientId/drawings": ({ patientId }) => { return ( <Suspense fallback={<Loading />}> - <ExcalidrawPage associatingId={id} fileType="patient" /> + <ExcalidrawPage associatingId={patientId} fileType="patient" /> </Suspense> ); },Also applies to: 61-69
55-57: Add error boundary around lazy-loaded components.Consider wrapping the Suspense components with an error boundary to handle loading failures gracefully.
// Create an error boundary component const ErrorBoundary = ({ children }: { children: React.ReactNode }) => { return ( <ErrorBoundaryComponent fallback={<div>Failed to load drawing component</div>} > {children} </ErrorBoundaryComponent> ); }; // Use it in the routes <ErrorBoundary> <Suspense fallback={<Loading />}> <ExcalidrawPage ... /> </Suspense> </ErrorBoundary>Also applies to: 65-67
src/components/Files/ExcalidrawPage.tsx (1)
166-182: Optimize Excalidraw component rendering.The debounce delay of 100ms for onChange might be too aggressive and cause unnecessary re-renders.
Consider:
- Increasing the debounce delay
- Memoizing the onChange handler
- onChange={debounce(async (elements) => { - setElements(elements); - }, 100)} + onChange={useMemo( + () => + debounce(async (elements) => { + setElements(elements); + }, 300), + [] + )}src/components/Files/FilesTab.tsx (1)
478-479: Add aria-label to improve accessibility.The drawing button lacks proper accessibility attributes.
- <CareIcon icon="l-pen" /> - <span>{t("add_drawings")}</span> + <CareIcon icon="l-pen" aria-hidden="true" /> + <span aria-label={t("add_drawings")}>{t("add_drawings")}</span>
📜 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 (10)
package.json(2 hunks)public/locale/en.json(2 hunks)src/Routers/routes/ConsultationRoutes.tsx(3 hunks)src/Routers/routes/PatientRoutes.tsx(3 hunks)src/components/Files/ExcalidrawPage.tsx(1 hunks)src/components/Files/ExcalidrawView.tsx(1 hunks)src/components/Files/FilesTab.tsx(5 hunks)src/lib/utils.ts(1 hunks)src/pages/Encounters/tabs/EncounterFilesTab.tsx(1 hunks)vite.config.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- vite.config.mts
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/pages/Encounters/tabs/EncounterFilesTab.tsx (1)
10-10: Verify patient object existence.Consider adding a null check for
props.patientto prevent potential runtime errors if the patient object is undefined.- patientId={props.patient.id} + patientId={props.patient?.id}src/Routers/routes/ConsultationRoutes.tsx (2)
1-1: LGTM! Good use of code splitting.The implementation of lazy loading for Excalidraw components is well done, which will help reduce the initial bundle size.
Also applies to: 12-13
36-48: LGTM! Well-structured route definitions.The new routes for drawings are properly structured and follow the existing patterns. The Suspense wrapper with Loading fallback is correctly implemented.
src/components/Files/ExcalidrawPage.tsx (1)
85-92:✅ Verification successful
Add version validation for Excalidraw data format.
The hardcoded version "2" might cause compatibility issues if Excalidraw's data format changes.
Let's check the latest Excalidraw version and data format:
🌐 Web query:
What is the latest version of Excalidraw's data format?💡 Result:
The latest version of Excalidraw's data format as of February 2025 remains version 2, based on documentation and recent updates[8][10]. This JSON-based structure includes elements like
type,version,elements,appState, andfiles, maintaining backward compatibility while introducing incremental improvements[8].Key details from recent updates:
- Compression defaults: Starting with Excalidraw-Obsidian plugin v2.2.0 (May 2024), compressed JSON became the default storage format using LZ-String compression[1][8]
- Font handling: Recent updates introduced embedded font subsets in SVG exports and CJK font support while keeping file sizes manageable[10]
- Security: Added URL sanitization for element links and improved encryption for shared diagrams[4][5]
The core schema remains stable since v2, though implementation details like compression methods and export options continue evolving[1][8]. Developers should reference the official JSON schema documentation for implementation specifics[8].
Citations:
- 1: https://www.youtube.com/watch?v=dV0NEOwn5NM
- 2: https://github.com/excalidraw/excalidraw/blob/master/packages/excalidraw/data/types.ts
- 3: https://forum.obsidian.md/t/excalidraw-image-annotate-new-feature-templater/79538
- 4: https://github.com/excalidraw/excalidraw/releases
- 5: https://tech.loveholidays.com/we-love-excalidraw-8dbd60a02511
- 6: https://mwc360.github.io/data-engineering/2024/11/20/Fabric-Library-for-Excalidraw.html
- 7: https://libraries.excalidraw.com
- 8: https://docs.excalidraw.com/docs/codebase/json-schema
- 9: https://github.com/excalidraw/excalidraw/blob/master/README.md
- 10: https://plus.excalidraw.com/blog/excalidraw-in-2024
Excalidraw Data Format Version is Up-to-Date
The Excalidraw JSON schema still uses version "2" as of February 2025 based on the latest documentation. While hardcoding the version isn’t a current issue, you might consider abstracting it (e.g., using a constant or config value) to ease future updates should the schema change.
- File:
src/components/Files/ExcalidrawPage.tsx(lines 85–92)- Current State: The hardcoded
"2"aligns with the latest Excalidraw format.- Suggestion: For increased maintainability, abstract the version into a configurable constant if you're anticipating future changes.
| export function debounce<T extends unknown[], U>( | ||
| callback: (...args: T) => PromiseLike<U> | U, | ||
| wait: number | ||
| ) { | ||
| let timer: ReturnType<typeof setTimeout>; | ||
| return (...args: T): Promise<U> => { | ||
| clearTimeout(timer); | ||
| return new Promise((resolve) => { | ||
| timer = setTimeout(() => resolve(callback(...args)), wait); | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup mechanism to prevent memory leaks.
The debounce implementation should clean up the timer when the component unmounts. Consider returning a cleanup function that can be used in useEffect.
export function debounce<T extends unknown[], U>(
callback: (...args: T) => PromiseLike<U> | U,
wait: number
) {
let timer: ReturnType<typeof setTimeout>;
- return (...args: T): Promise<U> => {
+ const debouncedFn = (...args: T): Promise<U> => {
clearTimeout(timer);
return new Promise((resolve) => {
timer = setTimeout(() => resolve(callback(...args)), wait);
});
};
+ debouncedFn.cancel = () => {
+ clearTimeout(timer);
+ };
+ return debouncedFn;
}Usage example:
useEffect(() => {
const debouncedFn = debounce(callback, 1000);
return () => debouncedFn.cancel();
}, [callback]);| <CareIcon icon="l-pen" className="text-lg text-primary-500" /> | ||
| </div> | ||
| <div className="m-4"> | ||
| <Input type="text" value={name} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disable input field in view mode.
The name input field should be read-only since this is a view-only component.
- <Input type="text" value={name} />
+ <Input type="text" value={name} readOnly />📝 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.
| <Input type="text" value={name} /> | |
| <Input type="text" value={name} readOnly /> |
| useEffect(() => { | ||
| if (!data) { | ||
| return; | ||
| } | ||
| setName(data.name!); | ||
| const fetchData = async () => { | ||
| const response = await fetch(data.read_signed_url!); | ||
| const json = await response.json(); | ||
| setElements(json.elements); | ||
| }; | ||
| fetchData(); | ||
| }, [data]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential race conditions in useEffect.
The fetch operation should be cancelled if the component unmounts or if data changes before the fetch completes.
useEffect(() => {
if (!data) {
return;
}
setName(data.name!);
+ const controller = new AbortController();
const fetchData = async () => {
- const response = await fetch(data.read_signed_url!);
+ const response = await fetch(data.read_signed_url!, {
+ signal: controller.signal
+ });
const json = await response.json();
setElements(json.elements);
};
fetchData();
+ return () => controller.abort();
}, [data]);📝 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.
| useEffect(() => { | |
| if (!data) { | |
| return; | |
| } | |
| setName(data.name!); | |
| const fetchData = async () => { | |
| const response = await fetch(data.read_signed_url!); | |
| const json = await response.json(); | |
| setElements(json.elements); | |
| }; | |
| fetchData(); | |
| }, [data]); | |
| useEffect(() => { | |
| if (!data) { | |
| return; | |
| } | |
| setName(data.name!); | |
| const controller = new AbortController(); | |
| const fetchData = async () => { | |
| const response = await fetch(data.read_signed_url!, { | |
| signal: controller.signal | |
| }); | |
| const json = await response.json(); | |
| setElements(json.elements); | |
| }; | |
| fetchData(); | |
| return () => controller.abort(); | |
| }, [data]); |
| const fetchData = async () => { | ||
| const response = await fetch(data.read_signed_url!); | ||
| const json = await response.json(); | ||
| setElements(json.elements); | ||
| }; | ||
| fetchData(); | ||
| }, [data]); |
There was a problem hiding this comment.
Add error handling for fetch operations.
The fetch operation lacks error handling, which could lead to unhandled promise rejections.
const fetchData = async () => {
+ try {
const response = await fetch(data.read_signed_url!);
+ if (!response.ok) {
+ throw new Error(`HTTP error! status: ${response.status}`);
+ }
const json = await response.json();
setElements(json.elements);
+ } catch (error) {
+ console.error('Failed to fetch drawing:', error);
+ // Consider showing an error state to the user
+ }
};
fetchData();📝 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.
| const fetchData = async () => { | |
| const response = await fetch(data.read_signed_url!); | |
| const json = await response.json(); | |
| setElements(json.elements); | |
| }; | |
| fetchData(); | |
| }, [data]); | |
| const fetchData = async () => { | |
| try { | |
| const response = await fetch(data.read_signed_url!); | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| const json = await response.json(); | |
| setElements(json.elements); | |
| } catch (error) { | |
| console.error('Failed to fetch drawing:', error); | |
| // Consider showing an error state to the user | |
| } | |
| }; | |
| fetchData(); | |
| }, [data]); |
| const handleSave = async () => { | ||
| if (!name.trim()) { | ||
| toast.error("Please enter a name for the drawing."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in the save operation.
The error handling in the save operation could be improved:
- The error message in the catch block only logs to console
- No user feedback for upload failures
- No cleanup or state reset on error
Consider this implementation:
try {
const file = new File([JSON.stringify(obj)], `${name}.excalidraw`, {
type: "text/plain",
});
let signedUrl = data?.read_signed_url || "";
let response: CreateFileResponse | null = null;
if (!id) {
response = await createUpload({
original_name: `${name}.excalidraw`,
name: name,
file_type: fileType,
file_category: "unspecified",
associating_id: associatingId,
mime_type: "text/plain",
});
signedUrl = response.signed_url;
}
} catch (error) {
- console.error("Error in Step 1 (createUpload):", error);
+ toast.error(t("file_error__create_upload_failed"));
+ setIsDirty(true); // Preserve changes
+ return;
}Also applies to: 94-124
| {file.extension === ".excalidraw" && ( | ||
| <Button | ||
| variant="secondary" | ||
| onClick={() => { | ||
| if (type === "encounter") { | ||
| navigate( | ||
| `/facility/${props.facilityId}/patient/${props.patientId}/encounter/${props.encounter?.id}/drawings/${file.id}`, | ||
| ); | ||
| } else { | ||
| navigate( | ||
| `/facility/${props.facilityId}/patient/${props.patientId}/drawings/${file.id}`, | ||
| ); | ||
| } | ||
| }} | ||
| > | ||
| <span className="flex flex-row items-center gap-1"> | ||
| <CareIcon icon="l-eye" /> | ||
| {t("view")} | ||
| </span> | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate navigation logic.
The navigation logic for drawings is duplicated between DetailButtons and FileUploadButtons. This violates the DRY principle and makes maintenance harder.
Extract the navigation logic into a utility function:
+const navigateToDrawings = (props: FilesTabProps, fileId?: string) => {
+ const basePath = `/facility/${props.facilityId}/patient/${props.patientId}`;
+ const drawingsPath = props.type === "encounter"
+ ? `${basePath}/encounter/${props.encounter?.id}/drawings`
+ : `${basePath}/drawings`;
+ return fileId ? `${drawingsPath}/${fileId}` : drawingsPath;
+};
// In DetailButtons
-if (type === "encounter") {
- navigate(`/facility/${props.facilityId}/patient/${props.patientId}/encounter/${props.encounter?.id}/drawings/${file.id}`);
-} else {
- navigate(`/facility/${props.facilityId}/patient/${props.patientId}/drawings/${file.id}`);
-}
+navigate(navigateToDrawings(props, file.id));
// In FileUploadButtons
-if (type === "encounter") {
- navigate(`/facility/${props.facilityId}/patient/${props.patientId}/encounter/${props.encounter?.id}/drawings`);
-} else {
- navigate(`/facility/${props.facilityId}/patient/${props.patientId}/drawings`);
-}
+navigate(navigateToDrawings(props));Also applies to: 461-481
| const ExcalidrawPage = lazy(() => import("@/components/Files/ExcalidrawPage")); | ||
| const ExcalidrawView = lazy(() => import("@/components/Files/ExcalidrawView")); | ||
|
|
There was a problem hiding this comment.
Nice, but we could simplify things by lazy loading the Excalidraw component itself directly inside the ExcalidrawPage and ExcalidrawView component instead;
There was a problem hiding this comment.
But what about the import("")); we have no Excalidraw.tsx (or .jsx) file inside the File
There was a problem hiding this comment.
we could do something like this inside ExcalidrawEditor and Viewer since excalidraw doesn't have a default export
const Excalidraw = lazy(() =>
import("@excalidraw/excalidraw").then((module) => ({
default: module.Excalidraw,
})),
);|
|
||
| const handleSave = async () => { | ||
| if (!name.trim()) { | ||
| toast.error("Please enter a name for the drawing."); |
| "/facility/:facilityId/patient/:patientId/encounter/:encounterId/drawings/:drawingId": | ||
| ({ drawingId }) => ( | ||
| <Suspense fallback={<Loading />}> | ||
| <ExcalidrawView drawingId={drawingId} /> | ||
| </Suspense> | ||
| ), |
There was a problem hiding this comment.
@bodhish marking this for hold since backend would not allow editing a file due to audit reasons.
@vigneshhari suggests making a plug and writing the elements to the DB itself instead of a file upload.
There was a problem hiding this comment.
Buckets have versions what stops us from editing 🤔
… i18n conversions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/Files/ExcalidrawEditor.tsx (2)
27-38: Consider using useReducer for better state management.The component manages multiple related states that could be consolidated using
useReducerfor better maintainability and state transitions.Here's a suggested implementation:
- const [elements, setElements] = useState<readonly ExcalidrawElement[] | null>([]); - const [name, setName] = useState(""); - const [id, setId] = useState(""); - const [isDirty, setIsDirty] = useState(false); - - useEffect(() => { - setIsDirty(!!elements?.length); - }, [elements?.length]); + type State = { + elements: readonly ExcalidrawElement[] | null; + name: string; + id: string; + isDirty: boolean; + }; + + type Action = + | { type: 'SET_ELEMENTS'; payload: readonly ExcalidrawElement[] | null } + | { type: 'SET_NAME'; payload: string } + | { type: 'SET_ID'; payload: string } + | { type: 'RESET' }; + + const initialState: State = { + elements: [], + name: '', + id: '', + isDirty: false, + }; + + const reducer = (state: State, action: Action): State => { + switch (action.type) { + case 'SET_ELEMENTS': + return { + ...state, + elements: action.payload, + isDirty: !!action.payload?.length, + }; + case 'SET_NAME': + return { ...state, name: action.payload }; + case 'SET_ID': + return { ...state, id: action.payload }; + case 'RESET': + return initialState; + default: + return state; + } + }; + + const [state, dispatch] = useReducer(reducer, initialState);
113-159: Improve component rendering and configuration.Consider the following improvements:
- Memoize the debounced onChange handler
- Support system theme preference
- Simplify height calculations
+ const debouncedOnChange = useMemo( + () => + debounce((elements) => { + setElements(elements); + }, 100), + [] + ); + + const theme = window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; + return ( - <div className="flex flex-col h-[calc(100vh-4rem)]"> + <div className="flex flex-col h-screen"> {/* ... */} - <div className="flex-grow h-[calc(100vh-10rem)] -m-2"> + <div className="flex-grow -m-2"> <Excalidraw UIOptions={{ canvasActions: { saveAsImage: true, export: false, loadScene: false, }, }} initialData={{ - appState: { theme: "light" }, + appState: { theme }, }} - onChange={debounce((elements) => { - setElements(elements); - }, 100)} + onChange={debouncedOnChange} /> </div> </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(4 hunks)src/Routers/routes/ConsultationRoutes.tsx(3 hunks)src/Routers/routes/PatientRoutes.tsx(3 hunks)src/components/Files/ExcalidrawEditor.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Routers/routes/ConsultationRoutes.tsx
- src/Routers/routes/PatientRoutes.tsx
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Files/ExcalidrawEditor.tsx (1)
1-25: LGTM! Well-organized imports and clear type definitions.The imports are logically grouped and the Props interface is well-defined with appropriate types.
| const handleSave = async () => { | ||
| if (!name.trim()) { | ||
| toast.error(t("please_enter_a_name_for_the_drawing")); | ||
| return; | ||
| } | ||
|
|
||
| const obj = { | ||
| type: "excalidraw", | ||
| version: "2", | ||
| source: window.location.origin, | ||
| elements: elements, | ||
| appState: {}, | ||
| files: {}, | ||
| }; | ||
|
|
||
| try { | ||
| const file = new File([JSON.stringify(obj)], `${name}.excalidraw`, { | ||
| type: "application/vnd.excalidraw", | ||
| }); | ||
| let signedUrl = ""; | ||
| let response: CreateFileResponse | null = null; | ||
| if (!id) { | ||
| response = await createUpload({ | ||
| original_name: `${name}.excalidraw`, | ||
| name: name, | ||
| file_type: fileType, | ||
| file_category: "unspecified", | ||
| associating_id: associatingId, | ||
| mime_type: "text/plain", | ||
| }); | ||
| signedUrl = response.signed_url; | ||
| } | ||
|
|
||
| const formData = new FormData(); | ||
| formData.append("file", file); | ||
|
|
||
| const upload = await fetch(signedUrl, { | ||
| method: "PUT", | ||
| body: file, | ||
| }); | ||
|
|
||
| if (!upload.ok) { | ||
| toast.error(t("error_uploading_file")); | ||
|
|
||
| return; | ||
| } | ||
| await markUploadComplete({ id: response?.id }); | ||
| if (markUploadCompleteError) { | ||
| toast.error(t("file_error__mark_complete_failed")); | ||
|
|
||
| return; | ||
| } else { | ||
| toast.success(t("file_success__upload_complete")); | ||
| navigate(`drawings/${response!.id}`); | ||
| } | ||
| } catch { | ||
| toast.error(t("error_in_createUpload")); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix file upload implementation and improve error handling.
Several issues need attention:
- FormData is created but not used
- Generic error handling could be more specific
- Upload progress tracking is missing
- const formData = new FormData();
- formData.append("file", file);
-
const upload = await fetch(signedUrl, {
method: "PUT",
body: file,
+ headers: {
+ 'Content-Type': 'application/vnd.excalidraw',
+ },
});
if (!upload.ok) {
- toast.error(t("error_uploading_file"));
+ toast.error(t("error_uploading_file_with_status", { status: upload.status }));
return;
}Also consider adding upload progress tracking:
const upload = await fetch(signedUrl, {
method: "PUT",
body: file,
headers: {
'Content-Type': 'application/vnd.excalidraw',
},
onUploadProgress: (progressEvent) => {
const percentCompleted = Math.round((progressEvent.loaded * 100) / progressEvent.total);
// Update progress state
},
});| const { mutateAsync: markUploadComplete, error: markUploadCompleteError } = | ||
| useMutation({ | ||
| mutationFn: mutate(routes.markUploadCompleted, { | ||
| pathParams: { id: id || "" }, | ||
| }), | ||
| }); | ||
|
|
||
| const { mutateAsync: createUpload } = useMutation({ | ||
| mutationFn: mutate(routes.createUpload), | ||
| onSuccess: (response: CreateFileResponse) => { | ||
| setId(response.id); | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance mutation hooks with better error handling and retry logic.
The mutation hooks could benefit from:
- Error handling for createUpload
- Retry configuration for transient failures
- Loading states for better UX
const { mutateAsync: markUploadComplete, error: markUploadCompleteError } =
useMutation({
mutationFn: mutate(routes.markUploadCompleted, {
pathParams: { id: id || "" },
}),
+ retry: 3,
+ retryDelay: 1000,
});
- const { mutateAsync: createUpload } = useMutation({
+ const { mutateAsync: createUpload, error: createUploadError, isLoading: isUploading } = useMutation({
mutationFn: mutate(routes.createUpload),
onSuccess: (response: CreateFileResponse) => {
setId(response.id);
},
+ onError: (error) => {
+ toast.error(t("error_in_createUpload"));
+ },
+ retry: 3,
+ retryDelay: 1000,
});📝 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.
| const { mutateAsync: markUploadComplete, error: markUploadCompleteError } = | |
| useMutation({ | |
| mutationFn: mutate(routes.markUploadCompleted, { | |
| pathParams: { id: id || "" }, | |
| }), | |
| }); | |
| const { mutateAsync: createUpload } = useMutation({ | |
| mutationFn: mutate(routes.createUpload), | |
| onSuccess: (response: CreateFileResponse) => { | |
| setId(response.id); | |
| }, | |
| }); | |
| const { mutateAsync: markUploadComplete, error: markUploadCompleteError } = | |
| useMutation({ | |
| mutationFn: mutate(routes.markUploadCompleted, { | |
| pathParams: { id: id || "" }, | |
| }), | |
| retry: 3, | |
| retryDelay: 1000, | |
| }); | |
| const { mutateAsync: createUpload, error: createUploadError, isLoading: isUploading } = useMutation({ | |
| mutationFn: mutate(routes.createUpload), | |
| onSuccess: (response: CreateFileResponse) => { | |
| setId(response.id); | |
| }, | |
| onError: (error) => { | |
| toast.error(t("error_in_createUpload")); | |
| }, | |
| retry: 3, | |
| retryDelay: 1000, | |
| }); |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Integrated Excalidraw for drawing Files
Fixes Add support for drawings as files by integrating excalidraw #10616
Added another field "Add Drawings" for open Excalidraw page
This is the Excalidraw page interface
After saving the drawing using filename users can view and update the file as they needed
@rithviknishad
Merge Checklist
Summary by CodeRabbit
New Features
Documentation