feat(ui): added project duplication and document queries #1019
Conversation
WalkthroughThis pull request integrates project duplication into the application. The changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PC as ProjectCard
participant DUP as useProjectDuplicate
participant API as Backend API/GraphQL
U->>PC: Click "Duplicate Project"
PC->>DUP: Invoke handleProjectDuplication
DUP->>API: Call createProject API with latest snapshot data
API-->>DUP: Return new project details
DUP->>PC: Signal duplication complete (disable loading)
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 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
Documentation and Community
|
✅ Deploy Preview for reearth-flow 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: 6
🧹 Nitpick comments (11)
ui/src/types/document.ts (1)
3-7: Type definition structure looks good, consider adding JSDoc commentsThe
ProjectSnapshottype is well-structured with clear property names. Consider adding JSDoc comments to provide more context about what each property represents, especially forupdateswhere the type (number[]) doesn't immediately convey its purpose.export type ProjectSnapshot = { + /** ISO timestamp when the snapshot was created */ timestamp: string; + /** Version number of the snapshot */ version: number; + /** Array of update identifiers contained in this snapshot */ updates: number[]; };ui/src/lib/gql/convert.ts (1)
79-85: Inconsistent function naming:toProjectSnapShotvs other converter functionsThe function name uses inconsistent capitalization compared to other converter functions in this file. All other functions use camelCase (e.g.,
toProject,toDeployment), but this one uses camelCase with an internal capitalSinSnap*S*hot.-export const toProjectSnapShot = ( +export const toProjectSnapshot = ( projectSnapshot: ProjectSnapshotFragment, ): ProjectSnapshot => ({ timestamp: projectSnapshot.timestamp, version: projectSnapshot.version, updates: projectSnapshot.updates, });ui/src/features/WorkspaceProjects/components/ProjectCard.tsx (1)
56-61: Consider handling potential undefined project IDWhen calling
useGetLatestProjectSnapshot, you're providing a fallback empty string forproject?.id, but it would be clearer to handle the case when project ID is undefined more explicitly.- const { projectDocument } = useGetLatestProjectSnapshot(project?.id ?? ""); + const { projectDocument } = useGetLatestProjectSnapshot(project.id);This is safer since your component already expects
projectto be defined based on the Props type, and using the non-optional property access reinforces this contract.ui/src/lib/i18n/locales/zh.json (1)
209-209: Translation values are missing for the new localization keys.All the newly added keys for project duplication and rollback functionality have empty string values. While this works technically, users will see empty text in the UI when these messages are displayed.
Consider adding appropriate Chinese translations for these keys to ensure a complete localization experience for Chinese users:
- "Duplicating...": "", + "Duplicating...": "复制中...", - "(duplicate)": "", + "(duplicate)": "(副本)", - "Project Rolled Back": "", + "Project Rolled Back": "项目已回滚", - "Project has been successfully rolled back to version {{version}}.": "", + "Project has been successfully rolled back to version {{version}}.": "项目已成功回滚到版本 {{version}}。", - "Project Rollback Failed": "", + "Project Rollback Failed": "项目回滚失败", - "There was an error rolling back the project.": "", + "There was an error rolling back the project.": "回滚项目时出现错误。"Also applies to: 281-281, 299-302
ui/src/lib/i18n/locales/es.json (1)
209-209: Translation values are missing for the new localization keys.All the newly added keys for project duplication and rollback functionality have empty string values. While this works technically, users will see empty text in the UI when these messages are displayed.
Consider adding appropriate Spanish translations for these keys to ensure a complete localization experience for Spanish users:
- "Duplicating...": "", + "Duplicating...": "Duplicando...", - "(duplicate)": "", + "(duplicate)": "(duplicado)", - "Project Rolled Back": "", + "Project Rolled Back": "Proyecto restaurado", - "Project has been successfully rolled back to version {{version}}.": "", + "Project has been successfully rolled back to version {{version}}.": "El proyecto se ha restaurado con éxito a la versión {{version}}.", - "Project Rollback Failed": "", + "Project Rollback Failed": "Falló la restauración del proyecto", - "There was an error rolling back the project.": "", + "There was an error rolling back the project.": "Hubo un error al restaurar el proyecto."Also applies to: 281-281, 299-302
ui/src/lib/gql/document/useQueries.ts (4)
18-30: Improve error handling in useLatestProjectSnapshotQuery.The current implementation only checks if data or specific properties exist but doesn't handle network errors or API failures.
Consider adding better error handling:
const useLatestProjectSnapshotQuery = (projectId: string) => useQuery({ queryKey: [DocumentQueryKeys.GetLatestProjectSnapshot, projectId], queryFn: async () => { + try { const data = await graphQLContext?.GetLatestProjectSnapshot({ projectId, }); if (!data?.latestProjectSnapshot) return; return toProjectDocument(data.latestProjectSnapshot); + } catch (error) { + console.error("Error fetching latest project snapshot:", error); + throw error; // Re-throw to let React Query handle the error state + } }, enabled: !!projectId, });
31-51: Improve error handling in useProjectHistoryQuery.The current implementation lacks proper error handling for network failures or API errors.
Consider adding better error handling:
const useProjectHistoryQuery = (projectId: string) => useQuery({ queryKey: [DocumentQueryKeys.GetProjectHistory, projectId], queryFn: async () => { + try { const data = await graphQLContext?.GetProjectHistory({ projectId, }); if (!data) return; const { projectHistory } = data; - console.log("projectHistory", projectHistory); const history: ProjectSnapshot[] = projectHistory .filter(isDefined) .map((projectSnapshot) => toProjectSnapShot(projectSnapshot)); return history; + } catch (error) { + console.error("Error fetching project history:", error); + throw error; // Re-throw to let React Query handle the error state + } }, enabled: !!projectId, refetchOnMount: false, refetchOnWindowFocus: false, });
53-83: Improve error handling in rollbackProjectMutation.The mutation doesn't provide clear error handling or feedback about why it might have failed.
Add proper error handling to the mutation:
const rollbackProjectMutation = useMutation({ mutationFn: async ({ projectId, version, }: { projectId: string; version: number; }) => { + try { const data = await graphQLContext?.RollbackProject({ projectId, version, }); if (data?.rollbackProject) { return data?.rollbackProject; } + throw new Error("Failed to rollback project: No response data"); + } catch (error) { + console.error("Error rolling back project:", error); + throw error; // Re-throw to let React Query handle the error state + } }, onSuccess: (projectDocument) => { if (projectDocument) { queryClient.invalidateQueries({ queryKey: [ DocumentQueryKeys.GetLatestProjectSnapshot, projectDocument.id, ], }); queryClient.invalidateQueries({ queryKey: [DocumentQueryKeys.GetProjectHistory, projectDocument.id], }); } }, });
14-89: Unify function definition style for better consistency.The hook uses different styles for defining query functions.
useLatestProjectSnapshotQueryanduseProjectHistoryQueryare defined as inner functions that return the result ofuseQuery, whilerollbackProjectMutationis directly assigned the result ofuseMutation.For better consistency, consider using the same style for all functions:
export const useQueries = () => { const graphQLContext = useGraphQLContext(); const queryClient = useQueryClient(); const useLatestProjectSnapshotQuery = (projectId: string) => useQuery({ // ... existing implementation }); const useProjectHistoryQuery = (projectId: string) => useQuery({ // ... existing implementation }); - const rollbackProjectMutation = useMutation({ + const useRollbackProjectMutation = () => useMutation({ // ... existing implementation }); return { useLatestProjectSnapshotQuery, useProjectHistoryQuery, - rollbackProjectMutation, + useRollbackProjectMutation, }; };Or alternatively, make all functions follow the direct assignment pattern:
export const useQueries = () => { const graphQLContext = useGraphQLContext(); const queryClient = useQueryClient(); - const useLatestProjectSnapshotQuery = (projectId: string) => - useQuery({ + const getLatestProjectSnapshot = (projectId: string) => useQuery({ // ... existing implementation }); - const useProjectHistoryQuery = (projectId: string) => - useQuery({ + const getProjectHistory = (projectId: string) => useQuery({ // ... existing implementation }); const rollbackProjectMutation = useMutation({ // ... existing implementation }); return { - useLatestProjectSnapshotQuery, - useProjectHistoryQuery, + getLatestProjectSnapshot, + getProjectHistory, rollbackProjectMutation, }; };ui/src/hooks/useProjectDuplicate.ts (1)
28-44: Add user feedback for partial duplication failuresThe hook successfully handles errors during the duplication process, but it doesn't provide any user feedback if the duplication fails after the project is created but before the document is duplicated.
Consider adding toast notifications for both successful duplication and failure scenarios similar to how the
useRollbackProjecthook works:import { useCallback, useState } from "react"; import { WebsocketProvider } from "y-websocket"; import * as Y from "yjs"; import { config } from "@flow/config"; import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; import { useAuth } from "@flow/lib/auth"; import { useProject } from "@flow/lib/gql"; import { useT } from "@flow/lib/i18n"; +import { useToast } from "@flow/features/NotificationSystem/useToast"; import { useCurrentWorkspace } from "@flow/stores"; import { Project, ProjectDocument } from "@flow/types"; export default (project: Project, projectDocument?: ProjectDocument) => { const { getAccessToken } = useAuth(); const t = useT(); + const { toast } = useToast(); const [isDuplicating, setIsDuplicating] = useState<boolean>(false); const [currentWorkspace] = useCurrentWorkspace(); const { createProject } = useProject(); const { websocket } = config();Then in the catch block:
} catch (error) { console.error("Project duplication failed:", error); + toast({ + title: t("Project Duplication Failed"), + description: t("There was an error duplicating the project."), + variant: "warning", + }); setIsDuplicating(false); }And add a success notification at the end of the successful duplication flow.
ui/src/lib/gql/document/useApi.ts (1)
64-68: Add JSDoc comments for exported hooksThe hooks in this file would benefit from JSDoc comments to improve code documentation and IDE intellisense for consumers of these functions.
+/** + * Provides hooks for working with project documents and history + * @returns Object containing hooks for project document operations + */ export const useDocument = () => { // ...implementation + /** + * Hook to get the latest project snapshot + * @param projectId - The ID of the project + * @returns Object containing the project document and query state + */ const useGetLatestProjectSnapshot = (projectId: string) => { // ...implementation }; + /** + * Hook to get project history + * @param projectId - The ID of the project + * @returns Object containing the project history and query state + */ const useGetProjectHistory = (projectId: string) => { // ...implementation }; + /** + * Hook to rollback a project to a specific version + * @param projectId - The ID of the project + * @param version - The version to rollback to + * @returns Promise resolving to the rollback result + */ const useRollbackProject = async ( // ...implementation }; return { useGetLatestProjectSnapshot, useGetProjectHistory, useRollbackProject, }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
ui/src/lib/gql/__gen__/gql.tsis excluded by!**/__gen__/**ui/src/lib/gql/__gen__/graphql.tsis excluded by!**/__gen__/**ui/src/lib/gql/__gen__/plugins/graphql-request.tsis excluded by!**/__gen__/**
📒 Files selected for processing (16)
ui/src/features/WorkspaceProjects/components/ProjectCard.tsx(4 hunks)ui/src/hooks/index.ts(1 hunks)ui/src/hooks/useProjectDuplicate.ts(1 hunks)ui/src/lib/gql/convert.ts(3 hunks)ui/src/lib/gql/document/index.ts(1 hunks)ui/src/lib/gql/document/queries.graphql(1 hunks)ui/src/lib/gql/document/useApi.ts(1 hunks)ui/src/lib/gql/document/useQueries.ts(1 hunks)ui/src/lib/gql/fragment.graphql(1 hunks)ui/src/lib/i18n/locales/en.json(3 hunks)ui/src/lib/i18n/locales/es.json(3 hunks)ui/src/lib/i18n/locales/fr.json(3 hunks)ui/src/lib/i18n/locales/ja.json(3 hunks)ui/src/lib/i18n/locales/zh.json(3 hunks)ui/src/types/document.ts(1 hunks)ui/src/types/index.ts(1 hunks)
🔇 Additional comments (16)
ui/src/types/index.ts (1)
4-4: Good addition of document type exports.The addition of this export statement is appropriate and follows the existing pattern in the file. It makes the document-related types available throughout the application.
ui/src/lib/gql/document/index.ts (1)
1-1: Clean module organization for document API.This barrel file export follows good practices for module organization, making the document API hooks available for import from a single location.
ui/src/hooks/index.ts (1)
5-5: Clean addition of useProjectDuplicate hook export.The export statement for the project duplication hook follows the established pattern in this file and maintains alphabetical ordering of exports.
ui/src/lib/gql/fragment.graphql (2)
69-74: Good structure for ProjectDocument fragment.The fragment is well-defined with appropriate fields for tracking project document state including id, timestamp, updates, and version.
76-80: Appropriate fields for ProjectSnapshot fragment.This fragment is correctly defined with the necessary fields for capturing a point-in-time representation of a project. The omission of the ID field compared to ProjectDocument makes sense as snapshots represent specific versions rather than independent entities.
ui/src/types/document.ts (2)
9-14: LGTM:ProjectDocumentextendsProjectSnapshotwith an ID fieldThe type structure is logical, inheriting the same properties as
ProjectSnapshotwith an additionalidfield.
16-18: ApiResponse inheritance pattern looks correctUsing the intersection type to extend
ApiResponseis a good approach. The optionalprojectDocumentfield indicates this type is used for API responses that may or may not contain document data.ui/src/lib/gql/document/queries.graphql (3)
1-8: Query structure looks good for fetching latest project snapshotThe GraphQL query is well-structured, requesting all necessary fields (id, timestamp, updates, version) for the latest project snapshot based on a project ID.
10-16: EnsureprojectHistoryreturns data in chronological orderThe query is well-structured, but there's no explicit ordering of the history items. If the backend doesn't implicitly order by timestamp or version, you might receive an unordered history.
Verify that the backend returns history items in the expected order (typically chronological). If not, consider adding a sorting mechanism in the UI when processing the results.
18-25: RollbackProject mutation looks correctThe mutation takes the required parameters (projectId and version) and returns the expected fields from the rolled back project document.
ui/src/lib/gql/convert.ts (2)
8-9: Imports are properly organizedThe new imports are correctly added to the existing import groups, maintaining organization between GraphQL fragments and type definitions.
Also applies to: 18-19
87-94:toProjectDocumentconversion function looks correctThe function properly maps all required fields from the fragment to the document type.
ui/src/features/WorkspaceProjects/components/ProjectCard.tsx (3)
29-30: Imports for new functionality look goodThe required hooks are properly imported for project duplication and document retrieval functionality.
99-103: Loading indicator implementation is consistent with existing patternThe duplication loading indicator follows the same pattern as the existing export loading indicator, providing a consistent user experience.
151-151: Project duplication is now directly wired up to the menu itemThe dropdown menu item for "Duplicate Project" now correctly triggers the duplication functionality.
ui/src/lib/i18n/locales/ja.json (1)
209-209: Japanese translations properly implementedGreat job providing comprehensive Japanese translations for all the new UI strings related to project duplication and rollback features. This ensures a consistent user experience for Japanese users.
Also applies to: 281-281, 299-302
| @@ -206,6 +206,7 @@ | |||
| "Copied to clipboard": "", | |||
| "{{project}} project's share URL copied to clipboard": "", | |||
| "Exporting...": "", | |||
| "Duplicating...": "", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Translation values are missing for the default English localization keys.
All the newly added keys for project duplication and rollback functionality have empty string values. This is particularly important for the English localization as it's likely the default language.
Add English text values for these keys to ensure proper messaging in the UI:
- "Duplicating...": "",
+ "Duplicating...": "Duplicating...",
- "(duplicate)": "",
+ "(duplicate)": "(duplicate)",
- "Project Rolled Back": "",
+ "Project Rolled Back": "Project Rolled Back",
- "Project has been successfully rolled back to version {{version}}.": "",
+ "Project has been successfully rolled back to version {{version}}.": "Project has been successfully rolled back to version {{version}}.",
- "Project Rollback Failed": "",
+ "Project Rollback Failed": "Project Rollback Failed",
- "There was an error rolling back the project.": "",
+ "There was an error rolling back the project.": "There was an error rolling back the project."Also applies to: 281-281, 299-302
| @@ -206,6 +206,7 @@ | |||
| "Copied to clipboard": "Copié dans le presse-papiers", | |||
| "{{project}} project's share URL copied to clipboard": "URL de partage du projet {{project}} copiée dans le presse-papiers", | |||
| "Exporting...": "Exportation...", | |||
| "Duplicating...": "", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing French translations for new UI strings
The newly added localization keys related to project duplication and rollback features are missing their French translations. Currently, they have empty string values which will cause these UI elements to fall back to English for French users, creating an inconsistent user experience.
Consider adding appropriate French translations for these keys:
- "Duplicating...": "",
+ "Duplicating...": "Duplication en cours...",
- "(duplicate)": "",
+ "(duplicate)": "(duplicata)",
- "Project Rolled Back": "",
+ "Project Rolled Back": "Projet restauré",
- "Project has been successfully rolled back to version {{version}}.": "",
+ "Project has been successfully rolled back to version {{version}}.": "Le projet a été restauré avec succès à la version {{version}}.",
- "Project Rollback Failed": "",
+ "Project Rollback Failed": "La restauration du projet a échoué",
- "There was an error rolling back the project.": "",
+ "There was an error rolling back the project.": "Une erreur s'est produite lors de la restauration du projet."Also applies to: 281-281, 299-302
|
|
||
| if (!data) return; | ||
| const { projectHistory } = data; | ||
| console.log("projectHistory", projectHistory); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/src/hooks/useProjectDuplicate.ts (1)
49-69:⚠️ Potential issueReset
isDuplicatingstate after WebSocket operationsCurrently, when using WebSockets, the
isDuplicatingstate is never set back to false after successful duplication. This would leave the UI in a perpetual loading state after the first successful duplication.}); yWebSocketProvider?.destroy(); + setIsDuplicating(false); } else { setIsDuplicating(false); }
🧹 Nitpick comments (3)
ui/src/hooks/useProjectDuplicate.ts (3)
46-66: Add timeout handling for WebSocket connectionThe current implementation waits indefinitely for the WebSocket to sync. If the sync event never occurs (due to network issues or other problems), the promise will never resolve and the UI will remain in the loading state.
const yDoc = new Y.Doc(); const convertedUpdates = new Uint8Array(updates); if (websocket) { const token = await getAccessToken(); const yWebSocketProvider = new WebsocketProvider( websocket, `${newProject.id}:${DEFAULT_ENTRY_GRAPH_ID}`, yDoc, { params: { token } }, ); await new Promise<void>((resolve, reject) => { + const timeout = setTimeout(() => { + yWebSocketProvider?.destroy(); + reject(new Error("WebSocket sync timeout")); + }, 30000); // 30 seconds timeout + yWebSocketProvider.once("sync", () => { yDoc.transact(() => { Y.applyUpdate(yDoc, convertedUpdates); }); + clearTimeout(timeout); resolve(); }); }); yWebSocketProvider?.destroy(); } else {
31-31: Consider a more robust naming strategy for duplicated projectsThe current approach simply appends "(duplicate)" to the original name. This could become confusing if a project is duplicated multiple times, resulting in names like "Project (duplicate) (duplicate)".
- name: project.name + t("(duplicate)"), + name: `${project.name} ${t("(duplicate)")}${new Date().toISOString().slice(0, 10)}`,Or even better, implement a function that checks for existing projects with similar names and adds a counter:
const generateDuplicateName = (originalName, existingProjects) => { const baseName = `${originalName} ${t("(duplicate)")}`; const duplicateCount = existingProjects .filter(p => p.name.startsWith(baseName)) .length; return duplicateCount > 0 ? `${baseName} (${duplicateCount + 1})` : baseName; };
40-44: Provide more context in guard clause commentsThe guard clauses are good, but adding comments explaining the reasoning would improve maintainability.
const updates = projectDocument?.updates; if (!updates || !updates.length) { + // Skip duplication if there are no updates to apply setIsDuplicating(false); return; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/hooks/useProjectDuplicate.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (1)
ui/src/hooks/useProjectDuplicate.ts (1)
13-88: Overall implementation is well-structuredThe hook is well-organized and follows good practices:
- Clear separation of concerns
- Proper use of React hooks (useState, useCallback)
- Comprehensive dependency array
- Appropriate error handling structure
- Good use of async/await for handling asynchronous operations
With the suggested fixes for state reset and error handling, this will be a robust implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/src/hooks/useProjectDuplicate.ts (1)
67-72: 🛠️ Refactor suggestionEnhance error feedback for users.
The current error handling only logs to the console, but doesn't provide any user-facing feedback about what went wrong during duplication.
+ const [error, setError] = useState<Error | null>(null); // In the catch block } catch (error) { console.error("Project duplication failed:", error); + setError(error instanceof Error ? error : new Error(String(error))); } finally { setIsDuplicating(false); } // In the return object return { isDuplicating, handleProjectDuplication, + error, };
🧹 Nitpick comments (7)
ui/src/hooks/useProjectDuplicate.ts (7)
21-37: Consider adding null checks for project properties.While you check if the project exists, there are no checks for required properties like
project.nameandproject.descriptionbefore using them.const handleProjectDuplication = useCallback(async () => { if (!project || !currentWorkspace) { return; } + // Ensure project has required properties + if (!project.name) { + console.error("Cannot duplicate project: missing name"); + return; + } try { setIsDuplicating(true); const { project: newProject } = await createProject({ workspaceId: currentWorkspace.id, name: project.name + t("(duplicate)"), description: project.description, });
39-43: Inconsistent optional chaining for updates check.Line 39 uses optional chaining while line 40 directly checks the property. This should be consistent.
- const updates = projectDocument?.updates; - if (!updates || !updates.length) { + if (!projectDocument?.updates || !projectDocument.updates.length) { setIsDuplicating(false); return; } + const updates = projectDocument.updates;
67-72: Redundant state management in error handling.The
setIsDuplicating(false)is called both in the catch block and in the finally block, making the call in the catch block redundant.} catch (error) { console.error("Project duplication failed:", error); - setIsDuplicating(false); } finally { setIsDuplicating(false); }
73-81: Update dependency array for accurate updates array tracking.Since you're specifically using
projectDocument?.updates, consider tracking changes to the updates directly in the dependency array.}, [ currentWorkspace, t, getAccessToken, createProject, websocket, project, - projectDocument, + projectDocument?.updates, ]);
83-87: Consider adding more information in the return object.For better integration with UI components, consider returning additional information such as the newly created project ID or any errors that occurred.
return { isDuplicating, handleProjectDuplication, + // Return additional useful information + duplicatedProjectId: null, // Could be set after successful duplication };
30-31: Implement a more robust naming convention for duplicated projects.The current approach simply appends "(duplicate)" to the project name, which may lead to issues if a project is duplicated multiple times.
- name: project.name + t("(duplicate)"), + name: `${project.name} ${t("(duplicate)")}${new Date().toISOString().slice(0, 10)}`, description: project.description,This would add a date stamp to the duplicated project name, making it unique and more informative.
45-47: Consider cleanup of Yjs document resources.The
Y.Docinstance should be properly cleaned up to prevent memory leaks, especially in scenarios where the duplication process might be aborted or fail.const yDoc = new Y.Doc(); const convertedUpdates = new Uint8Array(updates); + + // Setup cleanup function for finally block + const cleanup = () => { + // Clean up Yjs document resources + yDoc.destroy(); + }; // Add try/finally around the websocket block + try { if (websocket) { // ...existing code } + } finally { + cleanup(); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/hooks/useProjectDuplicate.ts(1 hunks)
🔇 Additional comments (2)
ui/src/hooks/useProjectDuplicate.ts (2)
1-12: Clean imports structure.The imports are well organized, separating external libraries from internal modules, which aids in maintainability.
13-20: Hook signature is clear and appropriate.The hook accepts a project object and an optional projectDocument parameter, which aligns well with its purpose. The state and hooks setup is also correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ui/src/hooks/useProjectDuplicate.ts (3)
21-81: Add protection against double-clicking the duplicate buttonCurrently there's no check to prevent multiple duplication attempts if a user clicks the button multiple times quickly. This could result in multiple duplicate projects being created.
const handleProjectDuplication = useCallback(async () => { if (!project || !currentWorkspace) { return; } + + // Prevent duplicate calls while already duplicating + if (isDuplicating) { + return; + } try { setIsDuplicating(true); // Rest of the function...
13-87: Consider adding a success callback for better UI feedbackThe hook currently manages the duplication state but doesn't provide a way for the calling component to know when duplication was successful. Adding a success callback would improve the user experience by allowing UI feedback.
- export default (project: Project, projectDocument?: ProjectDocument) => { + export default ( + project: Project, + projectDocument?: ProjectDocument, + onSuccess?: (newProject: Project) => void + ) => { // ...existing code... // After successful duplication (around line 63) setIsDuplicating(false); + onSuccess?.(newProject); resolve(); // In the returned object return { isDuplicating, handleProjectDuplication, };
40-44: Improve handling of empty updatesThe current implementation checks if
updatesis empty or undefined, but doesn't provide any feedback to the user about why the duplication might have been "successful" but didn't copy any content.const updates = projectDocument?.updates; if (!updates || !updates.length) { setIsDuplicating(false); + // Consider adding some feedback about empty project content + console.warn("Project duplication completed with empty updates"); return; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/hooks/useProjectDuplicate.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (3)
ui/src/hooks/useProjectDuplicate.ts (3)
49-68: WebSocketProvider cleanup should be in a finally blockThe current implementation calls the
destroy()method directly after the Promise, but it won't be executed if an error occurs during the Promise execution. This could lead to memory leaks and zombie connections.if (websocket) { const token = await getAccessToken(); const yWebSocketProvider = new WebsocketProvider( websocket, `${newProject.id}:${DEFAULT_ENTRY_GRAPH_ID}`, yDoc, { params: { token } }, ); + try { await new Promise<void>((resolve) => { yWebSocketProvider.once("sync", () => { yDoc.transact(() => { Y.applyUpdate(yDoc, convertedUpdates); }); setIsDuplicating(false); resolve(); }); }); + } finally { + yWebSocketProvider?.destroy(); + } - yWebSocketProvider?.destroy(); }
57-66: Add timeout and rejection handling to WebSocket sync PromiseThe current implementation doesn't handle cases where the "sync" event never fires or the connection fails. This could leave the application in a waiting state indefinitely.
- await new Promise<void>((resolve) => { + await new Promise<void>((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("WebSocket sync timeout")); + }, 30000); // 30 second timeout yWebSocketProvider.once("sync", () => { yDoc.transact(() => { Y.applyUpdate(yDoc, convertedUpdates); }); setIsDuplicating(false); + clearTimeout(timeout); resolve(); }); + + yWebSocketProvider.once("connection-error", (error) => { + clearTimeout(timeout); + reject(new Error(`WebSocket connection error: ${error}`)); + }); });
69-72: Enhance error feedback for usersThe current error handling only logs to the console, but doesn't provide any user-facing feedback about what went wrong during duplication.
+ const [error, setError] = useState<Error | null>(null); // In the catch block } catch (error) { console.error("Project duplication failed:", error); + setError(error instanceof Error ? error : new Error(String(error))); setIsDuplicating(false); } // In the return object return { isDuplicating, handleProjectDuplication, + error, };
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/src/hooks/useProjectDuplicate.ts (1)
48-68: 🛠️ Refactor suggestionImplement proper WebSocket provider cleanup and error handling
The current implementation properly creates and uses the WebSocketProvider but has multiple reliability issues:
- The provider's
destroy()method isn't guaranteed to be called if an error occurs inside the Promise- There's no timeout handling for the WebSocket connection
- Connection errors aren't being handled or reported
if (websocket) { const token = await getAccessToken(); const yWebSocketProvider = new WebsocketProvider( websocket, `${newProject.id}:${DEFAULT_ENTRY_GRAPH_ID}`, yDoc, { params: { token } }, ); - await new Promise<void>((resolve) => { - yWebSocketProvider.once("sync", () => { - yDoc.transact(() => { - Y.applyUpdate(yDoc, convertedUpdates); - }); - - setIsDuplicating(false); - resolve(); - }); - }); - yWebSocketProvider?.destroy(); + try { + await new Promise<void>((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("WebSocket sync timeout")); + }, 30000); // 30 second timeout + + yWebSocketProvider.once("sync", () => { + yDoc.transact(() => { + Y.applyUpdate(yDoc, convertedUpdates); + }); + + clearTimeout(timeout); + setIsDuplicating(false); + resolve(); + }); + + yWebSocketProvider.once("connection-error", (error) => { + clearTimeout(timeout); + reject(new Error(`WebSocket connection error: ${error}`)); + }); + }); + } finally { + yWebSocketProvider?.destroy(); + } }
🧹 Nitpick comments (6)
ui/src/hooks/useProjectDuplicate.ts (6)
20-23: Add safeguards against concurrent duplicationsThe current implementation doesn't prevent multiple concurrent duplication requests if the user clicks rapidly, which could lead to undefined behavior.
const handleProjectDuplication = useCallback(async () => { + if (isDuplicating) { + return; + } if (!project || !currentWorkspace) { return; }
31-32: Use template string for clearer concatenationUsing a template string would make the concatenation more readable and maintainable.
- name: project.name + t("(duplicate)"), + name: `${project.name} ${t("(duplicate)")}`,
40-44: Simplify validation and improve defensive codingThe current validation patterns can be improved for better readability and safety.
- const updates = projectDocument?.updates; - if (!updates || !updates.length) { + const updates = projectDocument.updates; + if (!Array.isArray(updates) || updates.length === 0) { setIsDuplicating(false); return; }
82-85: Add documentation for the return valuesConsider adding JSDoc comments to document the return values of the hook, which would improve API clarity for other developers.
+ /** + * @returns {Object} Hook result object + * @returns {boolean} result.isDuplicating - Whether a duplication operation is in progress + * @returns {Function} result.handleProjectDuplication - Function to trigger project duplication + */ return { isDuplicating, handleProjectDuplication, };
1-12: Organize imports by groupingThe imports could be better organized by grouping them into categories like external dependencies, internal modules, and types.
-import { useCallback, useState } from "react"; -import { WebsocketProvider } from "y-websocket"; -import * as Y from "yjs"; - -import { config } from "@flow/config"; -import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; -import { useAuth } from "@flow/lib/auth"; -import { useProject } from "@flow/lib/gql"; -import { useT } from "@flow/lib/i18n"; -import { useCurrentWorkspace } from "@flow/stores"; -import { Project, ProjectDocument } from "@flow/types"; +// External dependencies +import { useCallback, useState } from "react"; +import { WebsocketProvider } from "y-websocket"; +import * as Y from "yjs"; + +// Internal modules +import { config } from "@flow/config"; +import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; +import { useAuth } from "@flow/lib/auth"; +import { useProject } from "@flow/lib/gql"; +import { useT } from "@flow/lib/i18n"; +import { useCurrentWorkspace } from "@flow/stores"; + +// Types +import { Project, ProjectDocument } from "@flow/types";
13-19: Add JSDoc comment to document the hook purpose and parametersThe hook lacks documentation about its purpose and parameters, which would help other developers understand its usage.
+/** + * Hook for duplicating a project and its document data + * @param {Project} project - The project to duplicate + * @param {ProjectDocument} [projectDocument] - Optional project document containing updates to copy + * @returns {Object} Hook API with duplication state and handler + */ export default (project: Project, projectDocument?: ProjectDocument) => { const { getAccessToken } = useAuth(); const t = useT(); const [isDuplicating, setIsDuplicating] = useState<boolean>(false); const [currentWorkspace] = useCurrentWorkspace(); const { createProject } = useProject();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/hooks/useProjectDuplicate.ts(1 hunks)
🔇 Additional comments (1)
ui/src/hooks/useProjectDuplicate.ts (1)
69-72: Enhance error feedback for usersThe current error handling only logs to the console, without providing any user-facing feedback about what went wrong during duplication.
Consider returning the error or adding a state variable to track errors:
+ const [error, setError] = useState<Error | null>(null); // In the catch block } catch (error) { console.error("Project duplication failed:", error); + setError(error instanceof Error ? error : new Error(String(error))); setIsDuplicating(false); } // In the return object return { isDuplicating, handleProjectDuplication, + error, };This would allow the component using this hook to display appropriate error messages to the user.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ui/src/features/WorkspaceProjects/components/ProjectCard.tsx (1)
68-75: 🛠️ Refactor suggestionEnhance error feedback for duplication failures.
While the duplication process correctly triggers start/end callbacks, it lacks user-facing error notifications when duplication fails.
Add toast notifications to inform users about failures:
const handleProjectDuplicate = async () => { onDuplicationStart(); try { await handleProjectDuplication(); + toast({ + title: t("Project duplicated"), + description: t("{{project}} has been duplicated successfully", { + project: name, + }), + }); } catch (error) { + toast({ + title: t("Duplication failed"), + description: t("Failed to duplicate project. Please try again."), + variant: "destructive", + }); + console.error("Project duplication failed:", error); } finally { onDuplicationEnd(); } };ui/src/hooks/useProjectDuplicate.ts (2)
70-73: 🛠️ Refactor suggestionEnhance error handling and reporting.
The current error handling only logs to the console but doesn't provide a way for components to access error information.
+ const [error, setError] = useState<Error | null>(null); // In the catch block } catch (error) { console.error("Project duplication failed:", error); + setError(error instanceof Error ? error : new Error(String(error))); setIsDuplicating(false); } // In the return object return { isDuplicating, handleProjectDuplication, + error, };
49-69:⚠️ Potential issueFix WebSocket provider resource management.
The WebSocket provider isn't properly cleaned up in all scenarios, which could lead to resource leaks.
if (websocket) { const token = await getAccessToken(); const yWebSocketProvider = new WebsocketProvider( websocket, `${newProject.id}:${DEFAULT_ENTRY_GRAPH_ID}`, yDoc, { params: { token } }, ); - await new Promise<void>((resolve) => { - yWebSocketProvider.once("sync", () => { - yDoc.transact(() => { - Y.applyUpdate(yDoc, convertedUpdates); - }); - - setIsDuplicating(false); - resolve(); - }); - }); - yWebSocketProvider?.destroy(); + try { + await new Promise<void>((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("WebSocket sync timeout")); + }, 30000); // 30 second timeout + + yWebSocketProvider.once("sync", () => { + yDoc.transact(() => { + Y.applyUpdate(yDoc, convertedUpdates); + }); + + clearTimeout(timeout); + setIsDuplicating(false); + resolve(); + }); + + yWebSocketProvider.once("connection-error", (error) => { + clearTimeout(timeout); + reject(new Error(`WebSocket connection error: ${error}`)); + }); + }); + } finally { + yWebSocketProvider?.destroy(); + } }
🧹 Nitpick comments (3)
ui/src/features/WorkspaceProjects/components/ProjectCard.tsx (1)
60-65: Document retrieval for duplication needs error handling.The implementation fetches the latest project snapshot for duplication but doesn't handle scenarios where the document might be unavailable or loading.
const { useGetLatestProjectSnapshot } = useDocument(); - const { projectDocument } = useGetLatestProjectSnapshot(project?.id ?? ""); + const { projectDocument, loading, error } = useGetLatestProjectSnapshot(project?.id ?? ""); const { isDuplicating, handleProjectDuplication } = useProjectDuplicate( project, projectDocument, );ui/src/hooks/useProjectDuplicate.ts (2)
20-44: Add missing validation for duplicated project creation.The function handles basic validation but doesn't validate whether the new project was successfully created before proceeding.
const { project: newProject } = await createProject({ workspaceId: currentWorkspace.id, name: project.name + t("(duplicate)"), description: project.description, }); + // Ensure new project was created successfully + if (!newProject) { + throw new Error("Failed to create duplicated project"); + } if (!projectDocument || !newProject) { setIsDuplicating(false); return; }
83-86: Consider adding a reset function for error state.The hook returns duplication state and the handler, but lacks a way to reset error state after it's been handled.
+ const resetError = useCallback(() => { + setError(null); + }, []); return { isDuplicating, handleProjectDuplication, + error, + resetError, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ui/src/features/WorkspaceProjects/components/ProjectCard.tsx(5 hunks)ui/src/features/WorkspaceProjects/hooks.ts(2 hunks)ui/src/features/WorkspaceProjects/index.tsx(4 hunks)ui/src/hooks/useProjectDuplicate.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (11)
ui/src/features/WorkspaceProjects/hooks.ts (2)
15-15: Clean state management added for project duplication.Good addition of state to track duplication status. This state variable will help provide visual feedback to users during duplication operations.
92-92: Appropriate exposure of state and setter in hook return object.The state variable and its setter function are properly exposed in the hook's return object, making them accessible to components that use this hook.
Also applies to: 99-99
ui/src/features/WorkspaceProjects/components/ProjectCard.tsx (4)
29-30: Good imports of required hooks for duplication functionality.The imports for
useProjectDuplicateanduseDocumenthooks are correctly added to enable project duplication functionality.
42-43: Well-structured callback props for duplication status management.The addition of
onDuplicationStartandonDuplicationEndcallback props provides a clean way for parent components to respond to duplication lifecycle events.Also applies to: 52-53
112-116: Good user feedback during duplication process.The conditional rendering for the "Duplicating..." message provides appropriate visual feedback to users during the duplication process.
164-164: Duplication functionality successfully implemented.The "Duplicate Project" menu item is now properly connected to the duplication function, enabling the feature for users.
ui/src/hooks/useProjectDuplicate.ts (1)
13-19: Well-structured hook initialization with proper dependencies.The hook is correctly set up with appropriate dependencies and state management for tracking the duplication process.
ui/src/features/WorkspaceProjects/index.tsx (4)
50-50: Appropriate integration of duplication state.The duplication state and setter function are correctly destructured from the hook, maintaining consistency with the existing pattern.
Also applies to: 57-57
80-86: Well-defined handlers for duplication lifecycle.The handlers for duplication start and end are clear and focused, providing a clean interface for managing the duplication state.
145-145: Good conditional rendering for loading states.The loading skeleton is now correctly displayed during duplication in addition to fetching and importing, providing consistent feedback to users.
159-161: Proper callback props passed to ProjectCard.The duplication lifecycle callbacks are correctly passed to the ProjectCard component, enabling coordinated state management.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ui/src/hooks/useProjectDuplicate.ts (2)
57-67: 🛠️ Refactor suggestionAdd timeout and connection error handling to WebSocket Promise
The current implementation doesn't handle cases where the "sync" event never fires or the connection fails, which could leave users waiting indefinitely.
- await new Promise<void>((resolve) => { + await new Promise<void>((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error("WebSocket sync timeout")); + }, 30000); // 30 second timeout + yWebSocketProvider.once("sync", () => { yDoc.transact(() => { Y.applyUpdate(yDoc, convertedUpdates); }); setIsDuplicating(false); + clearTimeout(timeout); resolve(); }); + + yWebSocketProvider.once("connection-error", (error) => { + clearTimeout(timeout); + reject(new Error(`WebSocket connection error: ${error}`)); + }); });
72-75: 🛠️ Refactor suggestionEnhance error feedback for users
The current error handling only logs to the console, but doesn't provide any user-facing feedback about what went wrong during duplication.
+ const [error, setError] = useState<Error | null>(null); // In the catch block } catch (error) { console.error("Project duplication failed:", error); + setError(error instanceof Error ? error : new Error(String(error))); setIsDuplicating(false); } // In the return object return { isDuplicating, handleProjectDuplication, + error, };
🧹 Nitpick comments (3)
ui/src/hooks/useProjectDuplicate.ts (3)
40-44: Add null coalescing operator for clean codeThe current check for updates can be simplified using nullish coalescing operators for better readability.
- const updates = projectDocument?.updates; - if (!updates || !updates.length) { + const updates = projectDocument?.updates ?? []; + if (!updates.length) { setIsDuplicating(false); return; }
13-89: Consider adding callback props for duplication lifecycle eventsThe hook currently manages the duplication state internally but doesn't provide callback hooks for consumers to respond to key events (start, success, failure).
Consider updating the hook to accept callback props:
-export default (project: Project, projectDocument?: ProjectDocument) => { +export default ( + project: Project, + projectDocument?: ProjectDocument, + options?: { + onStart?: () => void; + onSuccess?: (newProject: Project) => void; + onError?: (error: Error) => void; + } +) => { // ... const handleProjectDuplication = useCallback(async () => { // ... try { setIsDuplicating(true); + options?.onStart?.(); // ... + options?.onSuccess?.(newProject); } catch (error) { console.error("Project duplication failed:", error); + const processedError = error instanceof Error ? error : new Error(String(error)); + options?.onError?.(processedError); setIsDuplicating(false); } }, [ // ... + options?.onStart, + options?.onSuccess, + options?.onError, ]);This would allow components using the hook to respond to duplication events more flexibly.
20-75: Consider extracting project duplication logic into a serviceThe hook contains both UI state management and complex business logic. This creates tightly coupled code that's harder to test in isolation.
Consider extracting the core duplication logic into a separate service:
// services/projectDuplication.ts export async function duplicateProject( params: { project: Project, projectDocument?: ProjectDocument, workspaceId: string, getAccessToken: () => Promise<string>, t: (key: string) => string, createProject: (params: CreateProjectParams) => Promise<{ project: Project }>, } ) { const { project, projectDocument, workspaceId, getAccessToken, t, createProject } = params; const { websocket } = config(); const { project: newProject } = await createProject({ workspaceId, name: project.name + t("(duplicate)"), description: project.description, }); if (!projectDocument || !newProject) { return newProject; } const updates = projectDocument?.updates ?? []; if (!updates.length) { return newProject; } // Yjs document duplication logic here... return newProject; } // In your hook const handleProjectDuplication = useCallback(async () => { if (!project || !currentWorkspace) { return; } try { setIsDuplicating(true); await duplicateProject({ project, projectDocument, workspaceId: currentWorkspace.id, getAccessToken, t, createProject, }); setIsDuplicating(false); } catch (error) { // Error handling } }, [dependencies]);This separation would make the code more testable and maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/hooks/useProjectDuplicate.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/src/features/WorkspaceLeftPanel/components/TopSection/index.tsx (1)
27-30: Project state is properly cleared before navigation.The implementation correctly resets the current project state before navigating to the projects page, ensuring a clean state transition. This prevents potential issues where stale project data might affect the projects list view.
Consider adding a brief comment explaining why clearing the project state before navigation is necessary, as this reasoning might not be immediately obvious to future developers.
onClick={() => { + // Clear current project state before navigating to projects list setCurrentProject(undefined); navigate({ to: `/workspaces/${currentWorkspace?.id}/projects` }); }}>ui/src/features/Editor/components/LeftPanel/index.tsx (1)
10-10: Navigation implementation looks good, but consider error handling.The change from a declarative Link component to programmatic navigation using
useNavigatehook is well implemented. ThehandleNavigationToDashboardfunction is properly memoized withuseCallbackand has the correct dependencies.Consider adding error handling to the navigation function to gracefully handle any potential navigation failures:
const handleNavigationToDashboard = useCallback(() => { - navigate({ to: `/workspaces/${workspaceId}/projects` }); + try { + navigate({ to: `/workspaces/${workspaceId}/projects` }); + } catch (error) { + console.error("Navigation failed", error); + // Consider showing a user-friendly error message + } }, [workspaceId, navigate]);Also applies to: 52-52, 57-59
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui/src/features/Editor/components/LeftPanel/index.tsx(3 hunks)ui/src/features/WorkspaceLeftPanel/components/TopSection/index.tsx(2 hunks)ui/src/features/WorkspaceProjects/components/ProjectCard.tsx(4 hunks)ui/src/features/WorkspaceProjects/hooks.ts(3 hunks)ui/src/features/WorkspaceProjects/index.tsx(3 hunks)ui/src/hooks/useProjectDuplicate.ts(1 hunks)ui/src/lib/gql/document/useQueries.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/src/features/WorkspaceProjects/hooks.ts
- ui/src/features/WorkspaceProjects/index.tsx
- ui/src/lib/gql/document/useQueries.ts
- ui/src/hooks/useProjectDuplicate.ts
- ui/src/features/WorkspaceProjects/components/ProjectCard.tsx
🔇 Additional comments (2)
ui/src/features/WorkspaceLeftPanel/components/TopSection/index.tsx (2)
7-7: Good addition of the useCurrentProject hook.Adding this hook alongside useCurrentWorkspace makes sense for managing project state throughout the application. This allows for proper state management of both workspace and project contexts.
19-19: Good implementation of the state setter.You're correctly destructuring only the setter function from useCurrentProject since you only need to reset the state in this component. This follows React best practices for state management.
| <div | ||
| className="flex shrink-0 items-center justify-center gap-2 text-lg font-semibold md:size-8 md:text-base" | ||
| onClick={handleNavigationToDashboard}> | ||
| <FlowLogo className="size-7 transition-all hover:size-[30px] hover:text-[#46ce7c]" /> | ||
| <span className="sr-only">{t("Dashboard")}</span> | ||
| </Link> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for the dashboard navigation element.
The div element used for navigation doesn't have proper accessibility attributes. When converting from a Link to a div with click handler, it's important to maintain keyboard accessibility and proper semantic indicators.
Add the following accessibility improvements:
<div
className="flex shrink-0 items-center justify-center gap-2 text-lg font-semibold md:size-8 md:text-base"
+ role="button"
+ tabIndex={0}
+ aria-label={t("Navigate to Dashboard")}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ handleNavigationToDashboard();
+ }
+ }}
+ style={{ cursor: 'pointer' }}
onClick={handleNavigationToDashboard}>
<FlowLogo className="size-7 transition-all hover:size-[30px] hover:text-[#46ce7c]" />
<span className="sr-only">{t("Dashboard")}</span>
</div>This ensures:
- Keyboard users can access the navigation element
- Screen readers announce it properly with the correct role
- The cursor styling indicates it's clickable
- Both Enter and Space keys trigger the navigation (standard for buttons)
📝 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.
| <div | |
| className="flex shrink-0 items-center justify-center gap-2 text-lg font-semibold md:size-8 md:text-base" | |
| onClick={handleNavigationToDashboard}> | |
| <FlowLogo className="size-7 transition-all hover:size-[30px] hover:text-[#46ce7c]" /> | |
| <span className="sr-only">{t("Dashboard")}</span> | |
| </Link> | |
| </div> | |
| <div | |
| className="flex shrink-0 items-center justify-center gap-2 text-lg font-semibold md:size-8 md:text-base" | |
| role="button" | |
| tabIndex={0} | |
| aria-label={t("Navigate to Dashboard")} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| handleNavigationToDashboard(); | |
| } | |
| }} | |
| style={{ cursor: 'pointer' }} | |
| onClick={handleNavigationToDashboard}> | |
| <FlowLogo className="size-7 transition-all hover:size-[30px] hover:text-[#46ce7c]" /> | |
| <span className="sr-only">{t("Dashboard")}</span> | |
| </div> |
…t-duplication-setup
…t-duplication-setup
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit