Change local storage and synchronization mechanisms#36
Conversation
- Added support for BinaryFiles in ExcalidrawData type. - Updated createNewPage and setDrawData functions to handle files. - Modified drawDataStore to include files in state management. - Enhanced Mermaid and Page components to manage file uploads and updates.
- Added localforage to manage persistent storage for drawing data. - Updated drawDataStore to utilize localforage for storing and retrieving page data. - Enhanced Page component to fetch data from the server and update local storage accordingly. - Modified UI to include a button for pulling the latest data from the server.
|
@sedationh is attempting to deploy a commit to the Tushar Selvakumar's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes update the persistence mechanism of the draw data store to use Dexie for IndexedDB-backed asynchronous storage. The Excalidraw page's data loading and saving flows are refactored, replacing automatic fetching and periodic saving with explicit user-triggered actions and debounced local autosave. UI elements and tooltips are updated accordingly, and error handling is improved. Additionally, the database query in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageComponent
participant Server
participant DexieDB
participant Excalidraw
User->>PageComponent: Clicks "Download" button
PageComponent->>Server: Fetch latest draw data (manual fetch)
Server-->>PageComponent: Returns draw data
PageComponent->>Excalidraw: Update scene with new data
PageComponent->>DexieDB: Save new data
PageComponent->>User: Show success or error toast
User->>PageComponent: Edits scene
PageComponent->>PageComponent: Debounced save triggered
PageComponent->>DexieDB: Save updated data locally
User->>PageComponent: Clicks "Save" button
PageComponent->>Server: Upload current draw data
Server-->>PageComponent: Confirm save
PageComponent->>User: Show save status toast
sequenceDiagram
participant User
participant LoginComponent
participant AuthModule
participant GoogleOAuth
User->>LoginComponent: Clicks "Continue with Google"
LoginComponent->>AuthModule: signInWithGoogle()
AuthModule->>GoogleOAuth: Initiate OAuth flow with redirect
GoogleOAuth-->>User: Redirect to Google sign-in page
sequenceDiagram
participant User
participant SignUpComponent
participant AuthModule
participant GoogleOAuth
User->>SignUpComponent: Clicks "Continue with Google"
SignUpComponent->>AuthModule: signInWithGoogle()
AuthModule->>GoogleOAuth: Initiate OAuth flow with redirect
GoogleOAuth-->>User: Redirect to Google sign-up page
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/stores/drawDataStore.ts (2)
28-28: Consider removing the emoji from the comment.While the checkmark emoji adds visual appeal, it's generally better to keep code comments professional and text-only for better compatibility across different editors and systems.
-// ✅ Configure persist to use localForage (IndexedDB) +// Configure persist to use localForage (IndexedDB)
54-64: Consider leveraging localforage's native object handling.
localforagecan handle JavaScript objects natively without explicit JSON serialization. This would simplify the code and potentially improve performance.storage: { getItem: async (name) => { - const value = await localforage.getItem(name); - return value ? JSON.parse(value as string) : null; + return await localforage.getItem(name); }, setItem: async (name, value) => { - await localforage.setItem(name, JSON.stringify(value)); + await localforage.setItem(name, value); }, removeItem: async (name) => { await localforage.removeItem(name); }, },Note: Verify that zustand's persist middleware works correctly with localforage's native object handling before applying this change.
src/views/Page.tsx (1)
80-80: Use English for code comments.The Chinese comment should be translated to English to maintain consistency across the codebase.
- // 同时更新本地存储 + // Update local storage simultaneously
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
package.json(1 hunks)src/stores/drawDataStore.ts(3 hunks)src/views/Page.tsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/views/Page.tsx (5)
src/db/draw.ts (1)
getDrawData(31-38)src/stores/drawDataStore.ts (1)
drawDataStore(69-69)src/components/ui/input.tsx (1)
Input(65-65)src/components/ui/button.tsx (1)
Button(83-83)src/components/ui/tooltip.tsx (4)
TooltipProvider(28-28)Tooltip(28-28)TooltipTrigger(28-28)TooltipContent(28-28)
🔇 Additional comments (1)
package.json (1)
29-29: localforage version confirmed – approve with a maintenance caveatI’ve verified that
localforage@^1.10.0is indeed the latest stable release and has no known security vulnerabilities. It’s a solid choice for replacinglocalStoragewith an asynchronous IndexedDB-backed API.• Version 1.10.0 was released in August 2021 and is free of reported issues (Snyk: no critical/high/medium/low vulnerabilities).
• Note that the project hasn’t had a new release in over three years. It’d be wise to:
– Keep an eye on the repo for future updates or security advisories.
– Evaluate actively maintained alternatives if long-term support becomes a concern.
| <p> | ||
| Pull latest data from server. This will overwrite any | ||
| unsaved changes. | ||
| </p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a confirmation dialog for data overwrite.
The current implementation immediately overwrites local changes without confirmation, which could lead to accidental data loss.
Since pulling from the server will overwrite any unsaved local changes, consider adding a confirmation dialog when there are unsaved changes:
<TooltipContent>
<p>
- Pull latest data from server. This will overwrite any
- unsaved changes.
+ Pull latest data from server. WARNING: This will overwrite any
+ unsaved local changes without confirmation.
</p>
</TooltipContent>Would you like me to implement a confirmation dialog that checks for unsaved changes before fetching from the server?
📝 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.
| <p> | |
| Pull latest data from server. This will overwrite any | |
| unsaved changes. | |
| </p> | |
| <p> | |
| Pull latest data from server. WARNING: This will overwrite any | |
| unsaved local changes without confirmation. | |
| </p> |
🤖 Prompt for AI Agents
In src/views/Page.tsx around lines 192 to 195, the current UI immediately pulls
data from the server and overwrites unsaved local changes without user
confirmation. To fix this, add a confirmation dialog that triggers only if there
are unsaved changes before proceeding with the data fetch. Implement a check for
unsaved changes and display a modal or browser confirm dialog asking the user to
confirm overwriting their changes; proceed with the pull only if the user
confirms.
| if ( | ||
| JSON.stringify(existingData?.elements) !== JSON.stringify(scene) || | ||
| JSON.stringify(existingData?.files) !== JSON.stringify(files) | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using JSON.stringify for deep equality comparison.
Using JSON.stringify for comparison is inefficient, especially with large objects containing images. It also may not handle circular references and can produce different results for objects with the same data but different property order.
Consider using a more efficient deep equality check or comparing specific properties that indicate changes (like timestamps or version numbers). You could also use a library like lodash.isEqual or implement a custom comparison that's optimized for your data structure.
- if (
- JSON.stringify(existingData?.elements) !== JSON.stringify(scene) ||
- JSON.stringify(existingData?.files) !== JSON.stringify(files)
- ) {
+ // Compare by reference first, then check if actual data differs
+ const elementsChanged = existingData?.elements !== scene ||
+ (existingData?.elements?.length !== scene.length);
+ const filesChanged = existingData?.files !== files ||
+ (Object.keys(existingData?.files || {}).length !== Object.keys(files || {}).length);
+
+ if (elementsChanged || filesChanged) {📝 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.
| if ( | |
| JSON.stringify(existingData?.elements) !== JSON.stringify(scene) || | |
| JSON.stringify(existingData?.files) !== JSON.stringify(files) | |
| ) { | |
| // Compare by reference first, then check if actual data differs | |
| const elementsChanged = existingData?.elements !== scene || | |
| (existingData?.elements?.length !== scene.length); | |
| const filesChanged = existingData?.files !== files || | |
| (Object.keys(existingData?.files || {}).length !== Object.keys(files || {}).length); | |
| if (elementsChanged || filesChanged) { |
🤖 Prompt for AI Agents
In src/views/Page.tsx around lines 111 to 114, replace the use of JSON.stringify
for deep equality checks on existingData.elements and existingData.files with a
more efficient method. Use a deep equality library like lodash.isEqual or
implement a custom comparison function tailored to your data structure to avoid
inefficiency and issues with circular references or property order.
Alternatively, compare specific change indicators such as timestamps or version
numbers if available.
| try { | ||
| const result = await refetch(); | ||
| if (result.data?.data && excalidrawAPI) { | ||
| const pageData = result.data.data[0].page_elements; | ||
| const elements = pageData.elements || []; | ||
| const files = pageData.files || {}; | ||
|
|
||
| excalidrawAPI.updateScene({ | ||
| elements: elements, | ||
| appState: { theme: theme }, | ||
| }); | ||
|
|
||
| // Update files if they exist | ||
| if (Object.keys(files).length > 0) { | ||
| excalidrawAPI.addFiles(Object.values(files)); | ||
| } | ||
|
|
||
| setName(result.data.data[0].name); | ||
|
|
||
| // 同时更新本地存储 | ||
| const updatedAt = new Date().toISOString(); | ||
| drawDataStore | ||
| .getState() | ||
| .setPageData( | ||
| id, | ||
| elements, | ||
| updatedAt, | ||
| result.data.data[0].name, | ||
| files, | ||
| ); | ||
|
|
||
| toast("Data loaded successfully from server"); | ||
| } | ||
|
|
||
| setName(data.data[0].name); | ||
| } | ||
| if (data?.error) { | ||
| toast("An error occurred", { description: data.error.message }); | ||
| if (result.data?.error) { | ||
| toast("An error occurred", { description: result.data.error.message }); | ||
| } | ||
| } catch (error) { | ||
| toast("Failed to fetch data from server"); | ||
| console.error("Error fetching data:", error); | ||
| } | ||
| } | ||
| }, [refetch, excalidrawAPI, theme, id]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in fetchDataFromServer.
The current error handling could be more specific and the success toast appears even when there's no data.
const fetchDataFromServer = useCallback(async () => {
try {
const result = await refetch();
- if (result.data?.data && excalidrawAPI) {
+ if (!result.data?.data || result.data.data.length === 0) {
+ toast("No data found on server");
+ return;
+ }
+
+ if (!excalidrawAPI) {
+ toast("Editor not ready. Please try again.");
+ return;
+ }
+
+ if (result.data.data && excalidrawAPI) {
const pageData = result.data.data[0].page_elements;
const elements = pageData.elements || [];
const files = pageData.files || {};
excalidrawAPI.updateScene({
elements: elements,
appState: { theme: theme },
});
// Update files if they exist
if (Object.keys(files).length > 0) {
excalidrawAPI.addFiles(Object.values(files));
}
setName(result.data.data[0].name);
- // 同时更新本地存储
+ // Update local storage simultaneously
const updatedAt = new Date().toISOString();
drawDataStore
.getState()
.setPageData(
id,
elements,
updatedAt,
result.data.data[0].name,
files,
);
toast("Data loaded successfully from server");
}
if (result.data?.error) {
toast("An error occurred", { description: result.data.error.message });
}
} catch (error) {
- toast("Failed to fetch data from server");
+ toast("Failed to fetch data from server", {
+ description: error instanceof Error ? error.message : "Unknown error"
+ });
console.error("Error fetching data:", error);
}
}, [refetch, excalidrawAPI, theme, id]);📝 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.
| try { | |
| const result = await refetch(); | |
| if (result.data?.data && excalidrawAPI) { | |
| const pageData = result.data.data[0].page_elements; | |
| const elements = pageData.elements || []; | |
| const files = pageData.files || {}; | |
| excalidrawAPI.updateScene({ | |
| elements: elements, | |
| appState: { theme: theme }, | |
| }); | |
| // Update files if they exist | |
| if (Object.keys(files).length > 0) { | |
| excalidrawAPI.addFiles(Object.values(files)); | |
| } | |
| setName(result.data.data[0].name); | |
| // 同时更新本地存储 | |
| const updatedAt = new Date().toISOString(); | |
| drawDataStore | |
| .getState() | |
| .setPageData( | |
| id, | |
| elements, | |
| updatedAt, | |
| result.data.data[0].name, | |
| files, | |
| ); | |
| toast("Data loaded successfully from server"); | |
| } | |
| setName(data.data[0].name); | |
| } | |
| if (data?.error) { | |
| toast("An error occurred", { description: data.error.message }); | |
| if (result.data?.error) { | |
| toast("An error occurred", { description: result.data.error.message }); | |
| } | |
| } catch (error) { | |
| toast("Failed to fetch data from server"); | |
| console.error("Error fetching data:", error); | |
| } | |
| } | |
| }, [refetch, excalidrawAPI, theme, id]); | |
| try { | |
| const result = await refetch(); | |
| if (!result.data?.data || result.data.data.length === 0) { | |
| toast("No data found on server"); | |
| return; | |
| } | |
| if (!excalidrawAPI) { | |
| toast("Editor not ready. Please try again."); | |
| return; | |
| } | |
| if (result.data.data && excalidrawAPI) { | |
| const pageData = result.data.data[0].page_elements; | |
| const elements = pageData.elements || []; | |
| const files = pageData.files || {}; | |
| excalidrawAPI.updateScene({ | |
| elements: elements, | |
| appState: { theme: theme }, | |
| }); | |
| // Update files if they exist | |
| if (Object.keys(files).length > 0) { | |
| excalidrawAPI.addFiles(Object.values(files)); | |
| } | |
| setName(result.data.data[0].name); | |
| // Update local storage simultaneously | |
| const updatedAt = new Date().toISOString(); | |
| drawDataStore | |
| .getState() | |
| .setPageData( | |
| id, | |
| elements, | |
| updatedAt, | |
| result.data.data[0].name, | |
| files, | |
| ); | |
| toast("Data loaded successfully from server"); | |
| } | |
| if (result.data?.error) { | |
| toast("An error occurred", { description: result.data.error.message }); | |
| } | |
| } catch (error) { | |
| toast("Failed to fetch data from server", { | |
| description: error instanceof Error ? error.message : "Unknown error" | |
| }); | |
| console.error("Error fetching data:", error); | |
| } |
🤖 Prompt for AI Agents
In src/views/Page.tsx around lines 61 to 101, improve error handling in the
fetchDataFromServer function by checking explicitly if valid data exists before
showing the success toast, and handle errors more specifically by distinguishing
between no data, server errors, and fetch exceptions. Adjust the logic so the
success toast only appears when data is successfully loaded, and provide more
detailed error messages or conditions for different failure cases.
| // need to wait for excalidrawAPI to be ready | ||
| setTimeout(() => { | ||
| excalidrawAPI.updateScene({ | ||
| elements: localData.elements, | ||
| appState: { theme: theme }, | ||
| }); | ||
| // Load files if they exist | ||
| if (localData.files && Object.keys(localData.files).length > 0) { | ||
| excalidrawAPI.addFiles(Object.values(localData.files)); | ||
| } | ||
| }, 1000); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Replace setTimeout with a more reliable initialization approach.
Using a fixed 1-second delay is unreliable and may fail on slower devices or when the API takes longer to initialize. This could lead to race conditions.
Consider using the Excalidraw API's ready state or initialization callback instead:
- // need to wait for excalidrawAPI to be ready
- setTimeout(() => {
- excalidrawAPI.updateScene({
- elements: localData.elements,
- appState: { theme: theme },
- });
- // Load files if they exist
- if (localData.files && Object.keys(localData.files).length > 0) {
- excalidrawAPI.addFiles(Object.values(localData.files));
- }
- }, 1000);
+ // Ensure API is ready before updating
+ const updateScene = () => {
+ excalidrawAPI.updateScene({
+ elements: localData.elements,
+ appState: { theme: theme },
+ });
+ // Load files if they exist
+ if (localData.files && Object.keys(localData.files).length > 0) {
+ excalidrawAPI.addFiles(Object.values(localData.files));
+ }
+ };
+
+ // Check if the API is already ready, otherwise defer
+ if (excalidrawAPI.ready) {
+ updateScene();
+ } else {
+ // If Excalidraw doesn't expose a ready state, you might need to
+ // implement a retry mechanism or use requestAnimationFrame
+ requestAnimationFrame(updateScene);
+ }🏁 Script executed:
#!/bin/bash
# Check if Excalidraw API has a ready state or initialization callback
rg -A 5 "ExcalidrawImperativeAPI|ready|onReady|initialized" --type ts --type tsxLength of output: 110
🏁 Script executed:
#!/bin/bash
# Search entire repo for ExcalidrawImperativeAPI and potential ready/init callbacks
rg -A 5 'ExcalidrawImperativeAPI' -n .
rg -A 5 'onReady' -n .
rg -A 5 'initialized' -n .Length of output: 1711
Use React’s lifecycle instead of a fixed timeout
Relying on setTimeout is brittle—on slower devices or under heavy load the API may not be ready in 1 second. Instead, trigger your scene update as soon as excalidrawAPI becomes non-null:
• Remove the setTimeout block in src/views/Page.tsx (lines ~140–151).
• Add a useEffect that watches excalidrawAPI (and localData if it can change) and runs the update immediately when the API is available.
Example replacement:
- // need to wait for excalidrawAPI to be ready
- setTimeout(() => {
- excalidrawAPI.updateScene({
- elements: localData.elements,
- appState: { theme: theme },
- });
- // Load files if they exist
- if (localData.files && Object.keys(localData.files).length > 0) {
- excalidrawAPI.addFiles(Object.values(localData.files));
- }
- }, 1000);
+ // Once the API ref is set, immediately sync our saved state
+ useEffect(() => {
+ if (!excalidrawAPI) return;
+
+ excalidrawAPI.updateScene({
+ elements: localData.elements,
+ appState: { theme },
+ });
+
+ if (localData.files && Object.keys(localData.files).length > 0) {
+ excalidrawAPI.addFiles(Object.values(localData.files));
+ }
+ }, [excalidrawAPI, localData, theme]);This guarantees your scene and files load as soon as excalidrawAPI is ready, avoiding race conditions.
📝 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.
| // need to wait for excalidrawAPI to be ready | |
| setTimeout(() => { | |
| excalidrawAPI.updateScene({ | |
| elements: localData.elements, | |
| appState: { theme: theme }, | |
| }); | |
| // Load files if they exist | |
| if (localData.files && Object.keys(localData.files).length > 0) { | |
| excalidrawAPI.addFiles(Object.values(localData.files)); | |
| } | |
| }, 1000); | |
| // Once the API ref is set, immediately sync our saved state | |
| useEffect(() => { | |
| if (!excalidrawAPI) return; | |
| excalidrawAPI.updateScene({ | |
| elements: localData.elements, | |
| appState: { theme }, | |
| }); | |
| if (localData.files && Object.keys(localData.files).length > 0) { | |
| excalidrawAPI.addFiles(Object.values(localData.files)); | |
| } | |
| }, [excalidrawAPI, localData, theme]); |
🤖 Prompt for AI Agents
In src/views/Page.tsx around lines 140 to 151, replace the setTimeout block that
delays updating excalidrawAPI by 1 second with a useEffect hook that watches
excalidrawAPI and localData. Remove the setTimeout entirely and instead create a
useEffect that triggers immediately when excalidrawAPI becomes non-null, calling
updateScene and addFiles as needed. This ensures the scene updates as soon as
the API is ready without relying on arbitrary delays.
- Updated getPages function to select specific fields (page_id, name, created_at, updated_at) - Avoid returning large page_elements data to improve performance - Reduces bandwidth usage when fetching page list
- Implemented signInWithGoogle function in auth module for Google authentication. - Updated Login and SignUp components to include Google login button and handle Google login process. - Added loading state management for Google login in both components. - Enhanced user experience with toast notifications for login errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/views/SignUp.tsx (1)
84-111: Consider extracting the Google icon to a reusable component.The Google login button implementation is functional, but the inline SVG icon is duplicated between Login and SignUp components.
Create a shared
GoogleIconcomponent:// components/icons/GoogleIcon.tsx export function GoogleIcon({ className = "h-4 w-4" }: { className?: string }) { return ( <svg className={className} viewBox="0 0 24 24"> <path fill="currentColor" d="M22.56 12.25c0-.78-.07-1.53-.2-2.25H12v4.26h5.92c-.26 1.37-1.04 2.53-2.21 3.31v2.77h3.57c2.08-1.92 3.28-4.74 3.28-8.09z" /> <path fill="currentColor" d="M12 23c2.97 0 5.46-.98 7.28-2.66l-3.57-2.77c-.98.66-2.23 1.06-3.71 1.06-2.86 0-5.29-1.93-6.16-4.53H2.18v2.84C3.99 20.53 7.7 23 12 23z" /> <path fill="currentColor" d="M5.84 14.09c-.22-.66-.35-1.36-.35-2.09s.13-1.43.35-2.09V7.07H2.18C1.43 8.55 1 10.22 1 12s.43 3.45 1.18 4.93l2.85-2.22.81-.62z" /> <path fill="currentColor" d="M12 5.38c1.62 0 3.06.56 4.21 1.64l3.15-3.15C17.45 2.09 14.97 1 12 1 7.7 1 3.99 3.47 2.18 7.07l3.66 2.84c.87-2.6 3.3-4.53 6.16-4.53z" /> </svg> ); }Then update the button:
- <svg className="mr-2 h-4 w-4" viewBox="0 0 24 24"> - <path - fill="currentColor" - d="M22.56 12.25c0-.78-.07-1.53-.2-2.25H12v4.26h5.92c-.26 1.37-1.04 2.53-2.21 3.31v2.77h3.57c2.08-1.92 3.28-4.74 3.28-8.09z" - /> - <path - fill="currentColor" - d="M12 23c2.97 0 5.46-.98 7.28-2.66l-3.57-2.77c-.98.66-2.23 1.06-3.71 1.06-2.86 0-5.29-1.93-6.16-4.53H2.18v2.84C3.99 20.53 7.7 23 12 23z" - /> - <path - fill="currentColor" - d="M5.84 14.09c-.22-.66-.35-1.36-.35-2.09s.13-1.43.35-2.09V7.07H2.18C1.43 8.55 1 10.22 1 12s.43 3.45 1.18 4.93l2.85-2.22.81-.62z" - /> - <path - fill="currentColor" - d="M12 5.38c1.62 0 3.06.56 4.21 1.64l3.15-3.15C17.45 2.09 14.97 1 12 1 7.7 1 3.99 3.47 2.18 7.07l3.66 2.84c.87-2.6 3.3-4.53 6.16-4.53z" - /> - </svg> + <GoogleIcon className="mr-2 h-4 w-4" />src/views/Login.tsx (3)
57-67: Consider extracting shared Google login logic.The
handleGoogleLoginfunction is nearly identical to the one in SignUp.tsx, differing only in the error toast message.Create a shared hook for Google authentication:
// hooks/useGoogleAuth.ts import { useState } from "react"; import { signInWithGoogle } from "@/db/auth"; import { toast } from "sonner"; export function useGoogleAuth() { const [isGoogleLoading, setIsGoogleLoading] = useState(false); const handleGoogleLogin = async () => { setIsGoogleLoading(true); const { error } = await signInWithGoogle(); if (error) { setIsGoogleLoading(false); toast("Google Authentication Error", { description: error.message }); } }; return { isGoogleLoading, handleGoogleLogin }; }Then update both Login and SignUp components:
- const [isGoogleLoading, setIsGoogleLoading] = useState<boolean>(false); - - async function handleGoogleLogin() { - setIsGoogleLoading(true); - const { error } = await signInWithGoogle(); - - if (error) { - setIsGoogleLoading(false); - toast("Google Login Error", { description: error.message }); - } - } + const { isGoogleLoading, handleGoogleLogin } = useGoogleAuth();
81-108: Duplicate Google button implementation.This Google login button is identical to the one in SignUp.tsx, including the inline SVG icon. Consider extracting this to a reusable component.
Create a shared
GoogleLoginButtoncomponent:// components/auth/GoogleLoginButton.tsx import { Button } from "@/components/ui/button"; import { GoogleIcon } from "@/components/icons/GoogleIcon"; interface GoogleLoginButtonProps { onClick: () => void; isLoading: boolean; loadingText: string; children: React.ReactNode; } export function GoogleLoginButton({ onClick, isLoading, loadingText, children }: GoogleLoginButtonProps) { return ( <Button type="button" variant="outline" className="w-full" onClick={onClick} isLoading={isLoading} loadingText={loadingText} > <GoogleIcon className="mr-2 h-4 w-4" /> {children} </Button> ); }Then update both components:
- <Button - type="button" - variant="outline" - className="w-full" - onClick={handleGoogleLogin} - isLoading={isGoogleLoading} - loadingText="Signing in with Google..." - > - <svg className="mr-2 h-4 w-4" viewBox="0 0 24 24"> - <!-- SVG paths --> - </svg> - Continue with Google - </Button> + <GoogleLoginButton + onClick={handleGoogleLogin} + isLoading={isGoogleLoading} + loadingText="Signing in with Google..." + > + Continue with Google + </GoogleLoginButton>
110-119: Duplicate divider implementation.The divider component is identical to the one in SignUp.tsx and could be extracted to a shared component.
Create a shared
AuthDividercomponent:// components/auth/AuthDivider.tsx export function AuthDivider() { return ( <div className="relative"> <div className="absolute inset-0 flex items-center"> <span className="w-full border-t" /> </div> <div className="relative flex justify-center text-xs uppercase"> <span className="bg-white px-2 text-gray-500 dark:bg-gray-950"> Or continue with </span> </div> </div> ); }Then update both components:
- <div className="relative"> - <div className="absolute inset-0 flex items-center"> - <span className="w-full border-t" /> - </div> - <div className="relative flex justify-center text-xs uppercase"> - <span className="bg-white px-2 text-gray-500 dark:bg-gray-950"> - Or continue with - </span> - </div> - </div> + <AuthDivider />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/db/auth.ts(1 hunks)src/views/Login.tsx(3 hunks)src/views/SignUp.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/db/auth.ts (1)
src/db/supabase.ts (1)
supabase(6-6)
src/views/Login.tsx (2)
src/db/auth.ts (1)
signInWithGoogle(11-19)src/components/ui/button.tsx (1)
Button(83-83)
🔇 Additional comments (7)
src/db/auth.ts (1)
11-19: Well-implemented Google OAuth integration.The function correctly implements Google OAuth using Supabase's authentication client. The redirect URL configuration ensures users return to the application after authentication, and the error handling pattern is consistent with other auth functions in the file.
src/views/SignUp.tsx (4)
32-32: Import statement is correct.The import properly includes the new
signInWithGooglefunction alongside the existingsignUpfunction.
36-36: Good state management pattern.The
isGoogleLoadingstate follows the same pattern as the existingisLoadingstate, maintaining consistency in the component.
60-70: Well-structured Google login handler.The function properly manages loading state and error handling. The comment about OAuth redirect behavior is helpful for future maintainers.
113-122: Clean divider implementation.The divider with "Or continue with" text provides good visual separation between OAuth and form-based authentication. The styling handles both light and dark themes appropriately.
src/views/Login.tsx (2)
26-26: Import statement is correct.The import properly includes the new
signInWithGooglefunction alongside the existingloginfunction.
33-33: Consistent state management pattern.The
isGoogleLoadingstate follows the same pattern as the existingisLoadingstate and matches the implementation in SignUp.tsx.
- Changed comment from Chinese to English for better understanding. - Ensured consistency in code documentation.
- Added useRef to manage save timeout for scene changes. - Implemented handleSceneChange to debounce and save drawing data to local storage automatically. - Included cleanup effect to clear timeout on component unmount.
- Removed redundant checks before saving drawing data. - Streamlined the saving process to always save locally first, followed by pushing to the API. - Improved code readability by adjusting formatting in the setPageData call.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/views/Page.tsx (3)
60-102: Improve error handling and data validation in fetchDataFromServer.The function has several issues that need attention:
- Success toast appears even when there's no data
- Error handling could be more specific
- Missing validation for data structure
This is similar to the previous review comment about improving error handling in fetchDataFromServer, but the current implementation still has the same issues.
const fetchDataFromServer = useCallback(async () => { try { const result = await refetch(); - if (result.data?.data && excalidrawAPI) { + if (!result.data?.data || result.data.data.length === 0) { + toast("No data found on server"); + return; + } + + if (!excalidrawAPI) { + toast("Editor not ready. Please try again."); + return; + } + + if (result.data.data && excalidrawAPI) { const pageData = result.data.data[0].page_elements; const elements = pageData.elements || []; const files = pageData.files || {}; excalidrawAPI.updateScene({ elements: elements, appState: { theme: theme }, }); // Update files if they exist if (Object.keys(files).length > 0) { excalidrawAPI.addFiles(Object.values(files)); } setName(result.data.data[0].name); - // Update local storage simultaneously + // Update local storage simultaneously const updatedAt = new Date().toISOString(); drawDataStore .getState() .setPageData( id, elements, updatedAt, result.data.data[0].name, files, ); toast("Data loaded successfully from server"); } if (result.data?.error) { toast("An error occurred", { description: result.data.error.message }); } } catch (error) { - toast("Failed to fetch data from server"); + toast("Failed to fetch data from server", { + description: error instanceof Error ? error.message : "Unknown error" + }); console.error("Error fetching data:", error); } }, [refetch, excalidrawAPI, theme, id]);
158-169: Replace setTimeout with a more reliable initialization approach.This is the same issue identified in previous reviews about using fixed timeout delays.
Using a 1-second delay is unreliable and may fail on slower devices or when the API takes longer to initialize.
- // need to wait for excalidrawAPI to be ready - setTimeout(() => { - excalidrawAPI.updateScene({ - elements: localData.elements, - appState: { theme: theme }, - }); - // Load files if they exist - if (localData.files && Object.keys(localData.files).length > 0) { - excalidrawAPI.addFiles(Object.values(localData.files)); - } - }, 1000);Consider using a
useEffectthat watches forexcalidrawAPIreadiness:+ // Load local data when API becomes available + useEffect(() => { + if (!excalidrawAPI) return; + + const localData = drawDataStore.getState().getPageData(id); + if (localData) { + excalidrawAPI.updateScene({ + elements: localData.elements, + appState: { theme: theme }, + }); + + if (localData.files && Object.keys(localData.files).length > 0) { + excalidrawAPI.addFiles(Object.values(localData.files)); + } + + setName(localData.name); + } + }, [excalidrawAPI, id, theme]);
186-233: UI changes improve user control but need confirmation dialog.The changes from automatic refresh to manual download button improve user control. However, the tooltip warning about overwriting unsaved changes should be backed by actual protection.
This matches the previous review comment about adding confirmation dialog for data overwrite.
<TooltipContent> <p> - Pull latest data from server. This will overwrite any - unsaved changes. + Pull latest data from server. WARNING: This will overwrite any + unsaved local changes without confirmation. </p> </TooltipContent>Consider implementing a confirmation dialog that checks for unsaved changes before proceeding with the fetch operation.
🧹 Nitpick comments (1)
src/views/Page.tsx (1)
110-127: Refactor setSceneData to improve error handling and clarity.The current implementation has some issues:
- The
onSettledcallback always setsisSavingto false, but this is redundant since it's already handled in mutation callbacks- The logic flow could be clearer
const setSceneData = useCallback(async () => { if (excalidrawAPI) { const scene = excalidrawAPI.getSceneElements(); const files = excalidrawAPI.getFiles(); const updatedAt = new Date().toISOString(); setIsSaving(true); // Save locally first drawDataStore.getState().setPageData(id, scene, updatedAt, name, files); // Then push to API - mutate( - { - elements: scene as NonDeletedExcalidrawElement[], - name, - files, - }, - { - onSettled() { - setIsSaving(false); - }, - }, - ); + mutate({ + elements: scene as NonDeletedExcalidrawElement[], + name, + files, + }); } }, [excalidrawAPI, id, name, mutate]);The
isSavingstate is already handled by the mutation'sonSuccessandonErrorcallbacks, making theonSettledcallback redundant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/views/Page.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/views/Page.tsx (5)
src/db/draw.ts (1)
getDrawData(31-38)src/stores/drawDataStore.ts (1)
drawDataStore(69-69)src/components/ui/input.tsx (1)
Input(65-65)src/components/ui/button.tsx (1)
Button(83-83)src/components/ui/tooltip.tsx (4)
TooltipProvider(28-28)Tooltip(28-28)TooltipTrigger(28-28)TooltipContent(28-28)
🔇 Additional comments (5)
src/views/Page.tsx (5)
1-1: Import changes look good.The addition of
useReffor timeout management andBinaryFilestype import are appropriate for the new debounced saving functionality. TheDownloadicon import aligns with the UI changes.Also applies to: 14-17, 19-19
33-33: Good addition of timeout ref and query configuration.The
saveTimeoutReffor debouncing andenabled: falsefor manual query triggering are well-implemented changes that align with the PR objectives of moving from automatic to manual synchronization.Also applies to: 35-39
130-152: Well-implemented debounced autosave functionality.The
handleSceneChangefunction properly implements debouncing with cleanup and automatically saves to local storage. The 500ms debounce interval is reasonable for balancing performance and data persistence.
174-181: Good cleanup implementation for timeout references.The cleanup useEffect properly clears the timeout reference on unmount, preventing memory leaks.
186-189: Good integration of onChange handler for debounced saving.The addition of
onChange={handleSceneChange}properly integrates the debounced autosave functionality with Excalidraw's change events.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| return { data, error }; | ||
| } | ||
|
|
||
| export async function signInWithGoogle() { |
There was a problem hiding this comment.
@sedationh Google OAuth is not in the roadmap currently. There's more implications to this such as with the metadata that need to be tested before doing this.
There was a problem hiding this comment.
got it, but I truly need this feature, if you do think so, i can checkout another branch to pull request
There was a problem hiding this comment.
you could expriece it in my deployment. https://draw.sedationh.top/
I do not find problem until now
There was a problem hiding this comment.
@sedationh can you separate out the code for Google OAuth and raise a separate PR for it?
|
@sedationh data is not loading when you click on a Page. |
yeah, that is a problem, when local has no files, download remote files, I will fix it tomorrow |
- Added "ahooks" library for improved async handling. - Introduced loading state management in the Page component to indicate data fetching. - Updated scene loading logic to provide user feedback during data retrieval.
- Moved the cleanup effect for clearing the save timeout to a dedicated useEffect. - Removed redundant cleanup code that was previously commented out. - Improved code organization and readability in the Page component.
- Consolidated loading overlay condition to improve readability. - Enhanced the structure of the loading state rendering in the Page component.
- Updated success callback to display a toast notification upon successful data save. - Consolidated the logic for setting the saving state to ensure it is reset after the mutation settles. - Improved button loading state management for better user feedback during save operations.
- Replaced localforage with Dexie for persistent storage in drawDataStore. - Updated setPageData and getPageData methods to work with Dexie for better performance and reliability. - Refactored Page component to handle asynchronous data operations with the new store structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/stores/drawDataStore.ts (1)
45-77: Excellent error handling implementation addresses previous concerns.The implementation successfully addresses the previous review comment about adding error handling for storage operations. The try-catch blocks are comprehensive and properly handle both logging and re-throwing errors.
The conflict resolution logic using timestamp comparison is well-implemented. However, consider handling the edge case where timestamps are exactly equal:
- if (!existingData || new Date(updatedAt) > new Date(existingData.updatedAt)) { + if (!existingData || new Date(updatedAt) >= new Date(existingData.updatedAt)) {This ensures that data with identical timestamps still gets saved, which could be important for maintaining data consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)src/routeTree.gen.ts(2 hunks)src/stores/drawDataStore.ts(1 hunks)src/views/Page.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/views/Page.tsx
🔇 Additional comments (7)
src/routeTree.gen.ts (4)
13-30: LGTM! Import naming is consistent and improves clarity.The systematic addition of
Importsuffixes to all route imports creates a clear distinction between the imported route definitions and the processed route objects created later in the file.
32-80: All route processing references updated correctly.The route processing code has been properly updated to use the new import names with
Importsuffixes, and the formatting is now more consistent across all route definitions.
149-208: Module augmentation properly updated with new import names.The
FileRoutesByPathmodule augmentation has been correctly updated to use the new import naming convention and repositioned appropriately in the file structure.
210-236: File structure improvements look good.The reorganization of declarations and use of
rootRouteImportin the final export maintains functionality while improving code organization. Since this is an auto-generated file, these systematic improvements indicate proper code generation.src/stores/drawDataStore.ts (3)
6-13: Well-structured interface design.The
PageDatainterface is well-designed for the new IndexedDB approach. The inclusion ofupdatedAtfor conflict resolution is particularly good for the manual synchronization mechanism mentioned in the PR objectives.Consider using
Datetype instead ofstringforupdatedAtto leverage TypeScript's type safety, though the current string approach works fine with ISO date strings.
15-28: Clean and appropriate database schema.The Dexie database setup is well-implemented with a simple schema that fits the use case. Using
page_idas the primary key is appropriate for the page-based storage model.
30-43: Appropriate async method signatures.The store type definition correctly reflects the new asynchronous nature of the IndexedDB operations. The method signatures are well-designed and match the
PageDatainterface.
| onClick={fetchDataFromServer} | ||
| disabled={isFetching} | ||
| > | ||
| <Download className="h-4 w-4" /> |
There was a problem hiding this comment.
@sedationh let's change this back to the original RefreshCcw. Download implies that the Excalidraw File or Image will be downloaded.
There was a problem hiding this comment.
Seems like the Autosave Logic is removed? Why was that done?
There was a problem hiding this comment.
Please remove this pnpm-lock file
There was a problem hiding this comment.
I don't see localforage and ahooks used anywhere in the code, that can be removed.
2 participants
With the addition of image support,
page_elementscan now be quite large (over 5 MB). Therefore,localStoragehas been replaced withIndexedDB, and synchronization is now active/manual. This addresses issues with the previous method being very slow and conflict-prone.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores