feat(web): project recycle bin #1181
Conversation
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…t/project-recylebin
…t/project-recylebin
WalkthroughThe pull request introduces extensive modifications to the translation files for English and Japanese, specifically enhancing project management messaging. New translation keys related to project deletion and confirmation have been added, while several outdated or unclear keys have been removed or modified for clarity. The overall structure of the translation files remains consistent, focusing on improving user-facing messages regarding project management actions. Additionally, the changes extend to various hooks and components, enhancing functionality related to project removal and management within the dashboard. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (37)
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/types.ts (1)
11-11: LGTM! Consider enhancing type safety.The addition of the
onProjectDeletecallback is well-implemented and aligns with the PR objectives. It follows the existing naming conventions and provides flexibility with its optional nature.To further improve type safety, consider making the
projectIdparameter required:onProjectDelete?: (projectId: string) => void;This change would ensure that a project ID is always provided when the delete function is called, potentially preventing accidental deletions of unintended projects.
web/src/beta/pages/EditorPage/index.tsx (2)
Line range hint
8-20: Consider improving type safety and error handlingWhile the current implementation works, there are a couple of suggestions to enhance the code:
- Improve type safety by using more specific types for the URL parameters.
- Add error handling for cases where
sceneIdortabmight be undefined.Here's a suggested implementation with these improvements:
import Editor from "@reearth/beta/features/Editor"; import { isTab } from "@reearth/beta/features/Navbar"; import NotFound from "@reearth/beta/features/NotFound"; import Page from "@reearth/beta/pages/Page"; import { FC } from "react"; import { useParams } from "react-router-dom"; type EditorPageParams = { sceneId?: string; tab?: string; }; const EditorPage: FC = () => { const { sceneId, tab } = useParams<EditorPageParams>(); if (!sceneId || !tab || !isTab(tab)) { return <NotFound />; } return ( <Page sceneId={sceneId} renderItem={(props) => <Editor tab={tab} sceneId={sceneId} {...props} />} /> ); }; export default EditorPage;This implementation:
- Defines a specific type for the URL parameters.
- Handles the case where
sceneIdortabmight be undefined.- Simplifies the component's logic by using an early return for the
NotFoundcase.
Remove Unnecessary
console.logStatementThe
console.log("sceneId", sceneId);statement inweb/src/beta/pages/EditorPage/index.tsxdoes not contribute to the PR's objectives of adding delete functionality or a Recycle Bin page. Please remove it to maintain code cleanliness.🔗 Analysis chain
Line range hint
1-20: Verify relevance of changes to PR objectivesThe changes in this file, particularly the added console.log statement, don't seem to directly contribute to the PR objectives of adding delete functionality or a Recycle Bin page for projects.
To ensure we're not missing any relevant changes, please run the following command to check for other modifications related to project deletion or recycle bin functionality:
If this file isn't meant to be part of the PR's main objectives, consider removing it from the PR or explaining its relevance in the PR description.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for relevant changes in the PR git diff origin/main | grep -n -i '\(delete\|recycle\|bin\|trash\)'Length of output: 45678
web/src/beta/features/Dashboard/ContentsContainer/index.tsx (1)
27-28: LGTM: Conditional rendering for RecycleBin component.The conditional rendering for the
RecycleBincomponent is correctly implemented and consistent with the existing pattern in theContentsContainercomponent. It appropriately uses thetabprop to determine when to render theRecycleBincomponent and passes theworkspaceIdprop as expected.For consistency with the other conditions, consider removing the blank line after this condition. Here's a suggested change:
{tab === "bin" && <RecycleBin workspaceId={workspaceId} />} - {tab === "members" && <Members currentWorkspace={currentWorkspace} />}web/src/beta/features/Dashboard/type.ts (2)
10-10: LGTM! Consider adding JSDoc comments for clarity.The changes to the
Projecttype look good:
- Making
descriptionoptional allows for projects without descriptions.- Adding the optional
deletedfield suggests a soft delete mechanism, which is a good practice.Consider adding JSDoc comments to explain the purpose of these fields, especially the new
deletedfield. This would improve code readability and maintainability. For example:/** Project description (optional) */ description?: string; /** Indicates if the project has been marked for deletion */ deleted?: boolean;Also applies to: 16-17
19-23: Good addition. Consider improvements for consistency and maintainability.The new
DeletedProjecttype is a good addition for managing deleted projects. However, there are a few points to consider:
- The
isDeletedfield is optional, which seems inconsistent with the purpose of this type. Consider making it required and always true.- There's some duplication between
DeletedProjectandProjecttypes, which could lead to maintenance issues.Consider the following improvements:
- Make
isDeletedrequired and always true:export type DeletedProject = { id: string; name: string; imageUrl?: string | null; isDeleted: true; };
- To reduce duplication and improve maintainability, consider using a discriminated union:
export type ProjectBase = { id: string; name: string; imageUrl?: string | null; }; export type ActiveProject = ProjectBase & { isDeleted: false; description?: string; // ... other fields specific to active projects }; export type DeletedProject = ProjectBase & { isDeleted: true; }; export type Project = ActiveProject | DeletedProject;This approach ensures type safety when dealing with deleted projects and reduces code duplication.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1)
16-29: LGTM: useEffect hook processes projects correctly, with a minor suggestion.The
useEffecthook efficiently processes the deleted projects, mapping them to the required structure and filtering out undefined values. This ensures that only valid projects are stored in the state.Consider replacing the type assertion
as DeletedProject[]with a type guard for improved type safety. Here's a suggested modification:const mappedProjects = (projects ?? []) .map((project): DeletedProject | undefined => { if (!project) return undefined; return { id: project.id, name: project.name, imageUrl: project.imageUrl, isDeleted: project.isDeleted }; }) .filter((project): project is DeletedProject => project !== undefined);This approach leverages TypeScript's type inference to ensure type safety without the need for a type assertion.
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx (1)
24-52: Modal structure is well-implemented, but there's a minor typo.The modal structure is well-organized and follows the UI library's conventions. The use of Typography components for text content ensures consistency, and the internationalization of warning messages is excellent for multilingual support.
However, there's a minor typo in one of the warning messages:
- {t("This means the project will no longer be published. But you can still see and restore you project from recycle bin.")} + {t("This means the project will no longer be published. But you can still see and restore your project from recycle bin.")}web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (2)
15-37: LGTM: Component structure and rendering logic are well-implemented.The component's structure is clean and follows React best practices. The use of a custom hook for managing state and actions promotes good separation of concerns. The rendering logic is clear and efficient.
Consider adding a check for an empty
deletedProjectsarray to display a message when there are no deleted projects. This would improve the user experience. For example:{deletedProjects?.length === 0 ? ( <EmptyState message="No deleted projects found" /> ) : ( <ProjectsGroup> {deletedProjects.map((project) => ( // ... existing code ))} </ProjectsGroup> )}
41-72: LGTM: Styled components are well-structured and promote responsive design.The styled components are well-organized and make good use of the theme for consistent styling. The responsive design using media queries is a great approach to ensure the UI adapts to different screen sizes.
Consider extracting the media query breakpoints into theme constants for better maintainability. This would allow for easier updates and ensure consistency across the application. For example:
const theme = { // ... other theme properties breakpoints: { mobile: '600px', tablet: '900px', desktop: '1200px', }, }; const ProjectsGroup = styled("div")(({ theme }) => ({ // ... other styles [`@media (max-width: ${theme.breakpoints.desktop})`]: { gridTemplateColumns: "repeat(3, 1fr)" }, [`@media (max-width: ${theme.breakpoints.tablet})`]: { gridTemplateColumns: "repeat(2, 1fr)" }, [`@media (max-width: ${theme.breakpoints.mobile})`]: { gridTemplateColumns: "1fr" } }));web/src/beta/features/Navbar/LeftSection/Profile/index.tsx (1)
40-40: LGTM! Consider updating other workspace-related icons for consistency.The icon change from "switch" to "arrowLeftRight" for the "Switch workspace" menu item is a good improvement. It likely provides a more intuitive visual representation of the action.
For consistency, you might want to review and update other workspace-related icons in the application to ensure they all follow the same visual language. This could enhance the overall user experience and make the interface more intuitive.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/RecycleBinProject.tsx (2)
13-23: LGTM: Component definition and hooks are well-implemented.The component is well-structured, using appropriate hooks for state management and optimization. The use of
useTfor translations is a good practice for internationalization.Consider adding a type annotation to the
setIsHoveredstate setter for improved type safety:- const [isHovered, setIsHovered] = useState(false); + const [isHovered, setIsHovered] = useState<boolean>(false);
40-61: LGTM: Component render function is well-implemented.The render function is well-structured, using appropriate styled components for layout and styling. The hover functionality and popup menu integration are correctly implemented.
Consider adding an
aria-labelto the PopupMenu button for improved accessibility:<PopupMenu menu={popupMenu} label={ - <Button icon="dotsThreeVertical" iconButton appearance="simple" /> + <Button icon="dotsThreeVertical" iconButton appearance="simple" aria-label={t("Project options")} /> } />This change will provide better context for screen reader users.
server/pkg/project/builder.go (1)
165-168: LGTM! Consider adding a comment for consistency.The new
Deletedmethod is well-implemented and follows the existing pattern of theBuilderstruct. It correctly sets theisDeletedfield and allows for method chaining.For consistency with other methods in this file, consider adding a brief comment describing the method's purpose. For example:
// Deleted sets the deletion status of the project. func (b *Builder) Deleted(deleted bool) *Builder { b.p.isDeleted = deleted return b }server/internal/infrastructure/mongo/mongodoc/project.go (1)
Line range hint
28-28: LGTM! Consider adding a comment for theDeletedfield.The addition of the
Deletedfield is appropriate for implementing the delete functionality for projects. Good job on placing it at the end of the struct to maintain backward compatibility.Consider adding a brief comment to explain the purpose of the
Deletedfield, similar to other fields in the struct. For example:Deleted bool // Indicates whether the project has been marked as deletedweb/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (2)
2-2: Consider relocating the ProjectDeleteModal component.The import statement is correct, but the
ProjectDeleteModalcomponent is imported from the Dashboard feature. This might indicate a potential architectural concern.Consider moving the
ProjectDeleteModalcomponent to a shared location if it's used in multiple features, or to the ProjectSettings feature if it's only used here. This would improve the overall architecture and make the component easier to maintain.
132-132: Correct implementation, but function name has a typo.The onClick handler for the delete button has been correctly updated to use the new modal opening function. However, there's a typo in the function name.
Change
handleDeleteModelOpentohandleDeleteModalOpenfor consistency and clarity. This typo should also be corrected in the function declaration (lines 68-70) and anywhere else it's used.web/src/beta/features/ProjectSettings/hooks.ts (2)
48-64: LGTM: Improved project deletion with recycle bin functionality.The refactored
handleProjectDeletefunction implements a "soft delete" approach, which aligns well with the PR objectives. It toggles the project's deleted state, allowing for both deletion and restoration, which is a significant improvement in project management.A minor suggestion for improvement:
Consider adding a user confirmation before deleting the project to prevent accidental deletions. You could use a confirmation dialog or a similar UX pattern.
Example:
const handleProjectDelete = useCallback(async () => { if (window.confirm("Are you sure you want to delete this project?")) { const updatedProject = { projectId, deleted: !project?.isDeleted }; const { status } = await useUpdateProjectRecycleBin(updatedProject); if (status === "success") { navigate(`/dashboard/${workspaceId}/`); } } }, [ navigate, project?.isDeleted, projectId, useUpdateProjectRecycleBin, workspaceId ]);
Line range hint
1-167: Consider adding error handling and user feedback.The implementation of the recycle bin functionality is robust and aligns well with the PR objectives. To further improve the user experience and code reliability, consider the following suggestions:
- Add error handling in the
handleProjectDeletefunction to manage potential failures in theuseUpdateProjectRecycleBincall.- Implement user feedback (e.g., toast notifications) to inform the user about the success or failure of the delete operation.
Example:
const handleProjectDelete = useCallback(async () => { try { const updatedProject = { projectId, deleted: !project?.isDeleted }; const { status } = await useUpdateProjectRecycleBin(updatedProject); if (status === "success") { navigate(`/dashboard/${workspaceId}/`); // Show success notification showNotification("Project successfully moved to recycle bin"); } else { // Show error notification showNotification("Failed to move project to recycle bin", "error"); } } catch (error) { console.error("Error updating project recycle bin status:", error); // Show error notification showNotification("An error occurred while updating the project", "error"); } }, [ navigate, project?.isDeleted, projectId, useUpdateProjectRecycleBin, workspaceId ]);This will provide a more robust user experience and make debugging easier in case of issues.
web/src/services/gql/queries/project.ts (2)
100-100: LGTM! Consider adding a comment for clarity.The addition of the
deletedparameter to theUPDATE_PROJECTmutation is well-implemented and aligns with the PR objective of adding delete functionality to projects.Consider adding a brief comment above the
deletedparameter to explain its purpose, for example:+ # Whether the project should be marked as deleted $deleted: BooleanThis would enhance code readability and make the purpose of the new parameter immediately clear to other developers.
Also applies to: 116-116
229-241: LGTM! Consider adding pagination for consistency.The new
GET_DELETED_PROJECTSquery is well-structured and aligns with the PR objective of managing deleted projects. It provides the necessary information for displaying deleted projects in a Recycle Bin page.For consistency with other queries in this file (e.g.,
GET_PROJECTS), consider adding pagination support:export const GET_DELETED_PROJECTS = gql(` - query GetDeletedProjects($teamId: ID!) { + query GetDeletedProjects($teamId: ID!, $pagination: Pagination) { - deletedProjects(teamId: $teamId) { + deletedProjects(teamId: $teamId, pagination: $pagination) { + edges { + node { + id + name + isDeleted + imageUrl + } + } nodes { id name isDeleted imageUrl } + pageInfo { + endCursor + hasNextPage + hasPreviousPage + startCursor + } totalCount } } `);This change would make the query more flexible and consistent with other queries in the file, allowing for easier implementation of pagination in the UI if needed.
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (4)
86-89: Function implemented correctly, but contains a typo.The
handleDeleteModelOpenfunction is well-implemented usinguseCallbackfor performance optimization. However, there's a typo in the function name.Please update the function name to fix the typo:
-const handleDeleteModelOpen = useCallback(() => { +const handleDeleteModalOpen = useCallback(() => { setProjectDeleteModalVisible(true); }, []);
90-93: Function implemented correctly, but contains a typo.The
handleDeleteModelClosefunction is well-implemented usinguseCallbackfor performance optimization. However, there's a typo in the function name, similar to the previous function.Please update the function name to fix the typo:
-const handleDeleteModelClose = useCallback(() => { +const handleDeleteModalClose = useCallback(() => { setProjectDeleteModalVisible(false); }, []);
113-117: LGTM: "Remove" option added correctly to popupMenu.The "Remove" option is well-implemented and consistent with other menu items. This addition aligns with the PR objective of adding delete functionality.
Once you've fixed the typo in the
handleDeleteModelOpenfunction name, please update theonClickhandler here as well:{ id: "remove", title: t("Remove"), icon: "trash", - onClick: () => handleDeleteModelOpen() + onClick: () => handleDeleteModalOpen() }
164-169: LGTM:handleProjectDeletefunction implemented correctly.The
handleProjectDeletefunction is well-implemented, usinguseCallbackfor optimization, checking for a validprojectId, and properly callingonProjectDeletebefore closing the modal.Once you've fixed the typo in the
handleDeleteModelClosefunction name, please update its usage here as well:const handleProjectDelete = useCallback((projectId?: string) => { if (!projectId) return; onProjectDelete?.(projectId); - handleDeleteModelClose() + handleDeleteModalClose() }, [handleDeleteModelClose, onProjectDelete])Also, don't forget to update the dependency array:
-}, [handleDeleteModelClose, onProjectDelete]) +}, [handleDeleteModalClose, onProjectDelete])server/internal/infrastructure/mongo/project.go (2)
117-120: LGTM! Consider adding a comment for clarity.The addition of the
$orclause to filter out deleted projects is a good improvement. It ensures that only active (non-deleted) starred projects are returned, which aligns with the PR objective of adding delete functionality to projects.Consider adding a brief comment explaining the purpose of the
$orclause for better code readability:// Filter out deleted projects "$or": []bson.M{ {"deleted": false}, {"deleted": bson.M{"$exists": false}}, },
Inconsistent Handling of Deleted Projects in Multiple Methods
Several
Findmethods inserver/internal/infrastructure/mongo/project.godo not apply filtering for deleted projects. To ensure consistent behavior across the application, it's recommended to add similar "deleted" filters to the following methods:
FindByIDFindBySceneFindByIDsFindByWorkspaceFindByPublicNameThis will help prevent unintended visibility of deleted projects and maintain uniform data integrity.
🔗 Analysis chain
Line range hint
1-120: Consider applying similar filtering to other relevant methods.The changes to
FindStarredByWorkspaceimprove the handling of deleted projects. For consistency, it might be beneficial to review other methods in this file (e.g.,FindByWorkspace,FindByIDs, etc.) to ensure they handle deleted projects similarly where appropriate.To help identify other methods that might benefit from similar filtering, you can run the following script:
This will help identify methods that might need review for consistent handling of deleted projects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find other methods in the file that query projects and might need similar filtering # Search for methods that query projects rg --type go -n 'func \(r \*Project\) Find' server/internal/infrastructure/mongo/project.go # Search for existing filters on the "deleted" field rg --type go -n '"deleted"' server/internal/infrastructure/mongo/project.goLength of output: 1154
web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (2)
194-194: LGTM: Project deletion enabled for grid viewThe
onProjectDeleteprop has been correctly added to theProjectGridViewItemcomponent, enabling project deletion functionality in the grid view. The implementation looks good.Consider using a callback to memoize the
onProjectDeletefunction to optimize performance, especially if there are many projects:useCallback(() => handleProjectDelete(project), [handleProjectDelete, project])
204-204: LGTM: Project deletion enabled for list viewThe
onProjectDeleteprop has been correctly added to theProjectListViewItemcomponent, enabling project deletion functionality in the list view. The implementation is consistent with the changes made to the grid view.As suggested for the grid view, consider using a callback to memoize the
onProjectDeletefunction:useCallback(() => handleProjectDelete(project), [handleProjectDelete, project])web/src/services/i18n/translations/en.yml (3)
57-58: Approve new project deletion translations with minor correction.The new translations for project deletion provide clear information about the consequences of moving a project to trash. The 'Recover' and 'Delete' actions are also important additions for managing deleted projects.
There's a minor grammatical error in the first message. Please apply the following correction:
-'Your project will be move to trash.': '' +'Your project will be moved to trash.': ''Also applies to: 60-61
Line range hint
397-406: Approve new workspace removal translations with minor correction.The new translations for workspace removal provide clear and consistent information about the process. They align well with the project deletion messages reviewed earlier.
There's a minor typo in one of the messages. Please apply the following correction:
-'Please to make sure you don't have any projects in your workspace, then you can continue.': '' +'Please make sure you don't have any projects in your workspace, then you can continue.': ''
Inconsistent terminology detected between "scene", "project", and "story".
The translations use multiple terms that may lead to confusion. Please standardize the terminology to ensure consistency throughout the application.
🔗 Analysis chain
Line range hint
397-423: Approve new project and story action translations with terminology clarification.The new translations for project and story actions provide clear feedback messages for various operations such as publishing, unpublishing, and updating. The messages are consistent in style and tone.
There seems to be inconsistent use of terms like "scene", "project", and "story". It's unclear if these terms are interchangeable or represent different concepts. Please clarify the usage of these terms to ensure consistency throughout the application.
To verify the usage of these terms, you can run the following script:
Based on the results, consider standardizing the terminology or providing clear distinctions between these terms in the user interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of terms "scene", "project", and "story" in the codebase echo "Occurrences of 'scene':" rg --type yaml 'scene' -i echo "\nOccurrences of 'project':" rg --type yaml 'project' -i echo "\nOccurrences of 'story':" rg --type yaml 'story' -iLength of output: 24591
web/src/beta/lib/reearth-ui/components/Icon/icons.ts (1)
164-164: LGTM: New icon added to the exported object correctly.The
arrowCounterClockWiseentry has been added to the exported object, following the established pattern and naming conventions.There's a minor formatting issue: a space is missing after the colon. Consider adding a space for consistency with other entries:
- arrowCounterClockWise:ArrowCounterClockWise, + arrowCounterClockWise: ArrowCounterClockWise,web/src/services/i18n/translations/ja.yml (1)
Line range hint
1-465: Overall review of Japanese translationsThe changes in this file align well with the PR objectives of enhancing project management functionality, particularly for project deletion and recovery features. Most of the new translations are accurate and consistent with existing ones. However, there are a few missing translations that need to be addressed:
- Project trash-related messages (lines 57-58)
- "Recover" action (line 60)
- Confirmation message for moving a project to trash (line 397)
These missing translations have been flagged in separate comments with suggested translations.
After adding the missing translations, please perform a final consistency check to ensure all project management related terms (e.g., "delete", "remove", "recover", "trash", "recycle bin") are translated consistently throughout the file.
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (1)
87-88: Ensure consistent property naming between 'deleted' and 'isDeleted'In the
filtedProjectsmapping, thedeletedproperty is assigned fromproject.isDeleted. For consistency and to avoid confusion, consider using the same property name throughout your codebase. Either renameproject.isDeletedtoproject.deletedor vice versa to maintain uniformity.server/e2e/gql_project_test.go (2)
218-221: Nitpick: Correct the comment for clarity and grammarThe comment on line 218 should be adjusted for grammatical correctness and clarity.
Apply this diff to improve the comment:
-// star and deleted 'Project 5' +// Star and delete 'Project 5'
Line range hint
479-508: Suggestion: Enhance test coverage for deleted projects retrievalThe current test checks for a single deleted project. To ensure robustness, consider adding additional deleted projects to test pagination and multiple deletions.
- Add more deleted projects to the test setup.
- Verify that
totalCountandnodescorrectly reflect all deleted projects.This enhancement will improve confidence in the system's handling of multiple deletions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
web/src/beta/lib/reearth-ui/components/Icon/Icons/ArrowCounterClockWise.svgis excluded by!**/*.svgweb/src/beta/lib/reearth-ui/components/Icon/Icons/switch.svgis excluded by!**/*.svgweb/src/services/gql/__gen__/gql.tsis excluded by!**/__gen__/**web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (27)
- server/e2e/gql_project_test.go (4 hunks)
- server/internal/adapter/gql/gqlmodel/convert_project.go (1 hunks)
- server/internal/infrastructure/mongo/mongodoc/project.go (1 hunks)
- server/internal/infrastructure/mongo/project.go (1 hunks)
- server/pkg/project/builder.go (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/types.ts (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/RecycleBinProject.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/index.tsx (2 hunks)
- web/src/beta/features/Dashboard/type.ts (1 hunks)
- web/src/beta/features/Navbar/LeftSection/Profile/index.tsx (1 hunks)
- web/src/beta/features/ProjectSettings/hooks.ts (3 hunks)
- web/src/beta/features/ProjectSettings/index.tsx (2 hunks)
- web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (3 hunks)
- web/src/beta/lib/reearth-ui/components/Icon/icons.ts (2 hunks)
- web/src/beta/pages/EditorPage/index.tsx (1 hunks)
- web/src/services/api/projectApi.ts (5 hunks)
- web/src/services/gql/fragments/project.ts (1 hunks)
- web/src/services/gql/queries/project.ts (3 hunks)
- web/src/services/i18n/translations/en.yml (2 hunks)
- web/src/services/i18n/translations/ja.yml (2 hunks)
🧰 Additional context used
🔇 Additional comments (50)
web/src/services/gql/fragments/project.ts (1)
24-24: LGTM! Verify usage of the new field across the codebase.The addition of the
isDeletedfield is consistent with the PR objectives and follows the existing naming conventions. This change enables the implementation of delete functionality for projects.To ensure proper integration, please verify the usage of this new field across the codebase. Run the following script to check for potential places where this field might need to be handled:
✅ Verification successful
Verification Successful! The
isDeletedfield has been correctly integrated and is consistently used across the codebase, including in all relevant GraphQL fragments and operations. No issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential usage of ProjectFragment that might need updating # Search for files using ProjectFragment echo "Files potentially using ProjectFragment:" rg --type typescript --type javascript "ProjectFragment" # Search for GraphQL queries or mutations that might need to include the new field echo "\nPotential GraphQL operations that might need updating:" rg --type typescript --type javascript "query.*Project|mutation.*Project"Length of output: 416
Script:
#!/bin/bash # Description: Find potential usage of ProjectFragment that might need updating # Search for files using ProjectFragment in .ts, .tsx, .js, and .jsx files echo "Files potentially using ProjectFragment:" rg "ProjectFragment" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" # Search for GraphQL queries or mutations that might need to include the new field in .ts, .tsx, .js, and .jsx files echo "\nPotential GraphQL operations that might need updating:" rg "query.*Project|mutation.*Project" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx"Length of output: 105546
web/src/beta/features/Dashboard/ContentsContainer/index.tsx (2)
9-9: LGTM: Import statement for RecycleBin component.The import statement for the
RecycleBincomponent is correctly formatted and aligns with the PR objectives of adding a Recycle Bin page to manage deleted projects.
9-9: Summary: RecycleBin component integration aligns with PR objectives.The changes in this file successfully integrate the
RecycleBincomponent into theContentsContainer. The implementation is consistent with the existing code structure and style, and it aligns well with the PR objectives of adding a Recycle Bin page to manage deleted projects.The new import statement and conditional rendering clause are correctly implemented, allowing the
RecycleBincomponent to be displayed when thetabprop is set to "bin". This enhancement expands the functionality of theContentsContaineras intended.Also applies to: 27-28
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (5)
1-4: LGTM: Imports are appropriate and well-organized.The imports cover all necessary dependencies for the hook's functionality, including React hooks and the custom
useProjectFetcherhook. The separation of theDeletedProjecttype into a separate file promotes good code organization.
6-14: LGTM: Hook declaration and initial setup are well-structured.The hook's signature with an optional
workspaceIdparameter provides flexibility. The destructuring of methods fromuseProjectFetcherand the initialization of thedeletedProjectsstate are appropriate for the hook's functionality.
31-50: LGTM: Callback functions are well-implemented and optimized.Both
handleProjectRecoveryandhandleProjectDeletefunctions are correctly implemented usinguseCallbackfor memoization. They include appropriate null checks to prevent errors when dealing with undefined projects. The functions correctly utilize the API methods for updating and deleting projects, respectively.The dependency arrays for both
useCallbackhooks are correctly specified, ensuring that the callbacks are only recreated when necessary.
52-57: LGTM: Hook return value is comprehensive and well-structured.The hook returns an object containing the
deletedProjectsstate and both callback functions (handleProjectDeleteandhandleProjectRecovery). This structure provides all necessary data and functions for managing deleted projects, following React hooks best practices.
1-57: Overall: Excellent implementation of the RecycleBin hook.This custom hook provides a robust and well-structured solution for managing deleted projects within a workspace's recycle bin. The code follows React best practices, makes effective use of React hooks (useState, useEffect, useCallback), and integrates well with the existing project management system.
Key strengths:
- Clear and logical structure
- Effective use of memoization with useCallback
- Proper handling of asynchronous operations
- Good separation of concerns
The minor suggestion for improving type safety in the useEffect hook is the only point of improvement in an otherwise excellent implementation.
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx (3)
1-16: LGTM: Imports and type definition are well-structured.The imports are appropriate for the component's functionality, and the Props type is well-defined with all necessary properties. The use of the FC (FunctionComponent) type for the component is a good practice for ensuring type safety.
17-23: LGTM: Component structure and props handling are well-implemented.The component is correctly defined as a functional component with proper typing. Props destructuring is done effectively, enhancing code readability. The use of the useT hook for internationalization is a good practice for maintaining multilingual support.
57-68: LGTM: Styled components are well-implemented.The use of styled components for Wrapper and WarningIcon is a good practice for encapsulating styles. The theme object is correctly utilized, ensuring consistency with the overall design system. The styles applied are appropriate for their intended use, contributing to a cohesive UI.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (2)
1-13: LGTM: Imports and component declaration are well-structured.The imports are appropriately organized, and the component declaration follows React best practices. The use of TypeScript for prop typing is a good practice for maintaining code quality.
39-39: LGTM: Component export is correct.The default export of the RecycleBin component is appropriate and follows common React practices.
server/internal/adapter/gql/gqlmodel/convert_project.go (1)
68-68: LGTM! Verify theIsDeleted()method implementation.The addition of the
IsDeletedfield to theProjectstruct is consistent with the PR objectives and will support the new delete functionality and Recycle Bin feature. The change is simple, straightforward, and backwards compatible.To ensure the correctness of this change, please verify the implementation of the
IsDeleted()method in theproject.Projectstruct. Run the following script to check its implementation:✅ Verification successful
Verified the
IsDeleted()method implementation.The
IsDeleted()method exists and correctly returns theisDeletedfield. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the IsDeleted() method in the project.Project struct. # Test: Search for the IsDeleted method in the project package rg --type go -A 5 'func \(.*Project\) IsDeleted\(\) bool' server/pkg/projectLength of output: 394
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/RecycleBinProject.tsx (1)
1-12: LGTM: Imports and type definitions are well-structured.The imports cover all necessary components and hooks. The
Proptype is well-defined, including optional callbacks for delete and recovery actions, which aligns with the component's purpose.server/pkg/project/builder.go (2)
165-168: Summary of changes and next stepsThe addition of the
Deletedmethod to theBuilderstruct is a well-implemented change that follows existing patterns in the codebase. It introduces project deletion functionality, which is aligned with the PR objectives of enhancing project management capabilities.Next steps:
- Add a brief comment to the
Deletedmethod for consistency with other methods.- Verify the proper handling of the
isDeletedfield across the codebase using the provided script.- Ensure that all components affected by this new feature (database, API, UI) are updated accordingly.
- Consider adding or updating unit tests to cover the new deletion functionality.
165-168: Verify handling ofisDeletedfield across the codebaseThe addition of the
Deletedmethod implies the introduction of anisDeletedfield in theProjectstruct. To ensure proper implementation of this new feature, please verify:
- The
isDeletedfield is properly defined in theProjectstruct.- Database operations (create, read, update) handle the
isDeletedfield correctly.- API responses include or exclude deleted projects as appropriate.
- UI components respect the deletion status of projects.
To assist in this verification, you can run the following script:
This script will help identify areas where the
isDeletedfield is being used or where it might need to be implemented.server/internal/infrastructure/mongo/mongodoc/project.go (2)
Line range hint
28-28: Summary: Delete functionality for projects implemented correctlyThe changes in this file successfully implement the delete functionality for projects:
- The
Deletedfield has been added to theProjectDocumentstruct.- The
Model()method has been updated to include the deletion status when converting to aproject.Project.These changes are minimal, focused, and consistent with the PR objectives. They provide the necessary infrastructure for managing deleted projects in the database.
Also applies to: 131-131
131-131: LGTM! Verify theDeletedmethod in theproject.Projecttype.The addition of
Deleted(d.Deleted)in theModel()method correctly sets the deletion status when converting aProjectDocumentto aproject.Project. This change is consistent with the newDeletedfield in theProjectDocumentstruct.To ensure full compatibility, let's verify the existence and signature of the
Deletedmethod in theproject.Projecttype:web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (3)
67-74: Improved component structure with modal handling.The changes simplify the component by moving the deletion logic to a separate modal. The new state and functions for managing the modal visibility are correctly implemented using React hooks. This improves the component's readability and maintainability.
139-145: Well-implemented modal component integration.The ProjectDeleteModal is correctly integrated into the component. It's conditionally rendered based on the deleteModelVisible state, and the necessary props (isVisible, onClose, and onProjectDelete) are properly passed to it. This implementation improves the overall structure and modularity of the code.
Line range hint
1-145: Overall improvement in project deletion functionality.The changes in this file effectively modularize and simplify the project deletion process. The introduction of the ProjectDeleteModal component and the associated state management improve both the code structure and user experience. These modifications are well-integrated with the existing code and don't introduce any apparent issues.
Some points to consider:
- The architectural placement of the ProjectDeleteModal component could be reviewed.
- There's a minor typo in the function names related to modal handling.
Overall, these changes represent a positive improvement to the codebase.
web/src/beta/features/ProjectSettings/hooks.ts (2)
29-30: LGTM: New import for project recycle bin functionality.The addition of
useUpdateProjectRecycleBinto the destructured imports fromuseProjectFetcheraligns well with the PR objectives of implementing delete functionality and introducing a Recycle Bin for projects.
159-159: LGTM: Consistent renaming in the return statement.The renaming of
handleDeleteProjecttohandleProjectDeletein the return object is consistent with the earlier function renaming. This ensures that the new function name is correctly exposed for use by components consuming this hook.web/src/services/gql/queries/project.ts (1)
Line range hint
1-241: Overall, the changes look good and align with the PR objectives.The modifications to the
UPDATE_PROJECTmutation and the addition of theGET_DELETED_PROJECTSquery effectively support the new delete functionality for projects. The changes are well-structured and follow existing conventions in the file.Consider implementing the suggested improvements for better consistency and clarity:
- Adding a comment to explain the purpose of the
deletedparameter inUPDATE_PROJECT.- Adding pagination support to the
GET_DELETED_PROJECTSquery for consistency with other queries.These changes will enhance the maintainability and scalability of the code.
web/src/beta/features/ProjectSettings/index.tsx (3)
103-103: LGTM: Consistent prop update.The update of the
onDeleteProjectprop to usehandleProjectDeleteis consistent with the earlier renaming. This change maintains the integrity of the component's functionality and the overall consistency of the codebase.
50-50: Approve renaming and verify impact.The renaming of
handleDeleteProjecttohandleProjectDeleteimproves clarity and aligns better with React naming conventions for event handlers. This change is approved.To ensure this change doesn't introduce any issues, please run the following script to check for any remaining occurrences of
handleDeleteProjectin the codebase:#!/bin/bash # Description: Check for any remaining occurrences of handleDeleteProject # Test: Search for handleDeleteProject in all TypeScript and TypeScript React files rg --type-add 'typescript:*.{ts,tsx}' --type typescript 'handleDeleteProject'
Line range hint
1-203: Summary: Approve changes and suggest broader verification.The changes in this file are minimal and focused on improving naming consistency. They are part of a larger refactoring effort to enhance code quality and maintainability. While the changes in this file are approved, it's important to ensure that this refactoring has been consistently applied across the entire codebase.
To verify the broader impact of this refactoring, please run the following script:
This script will help ensure that the refactoring has been applied consistently and identify any potential areas that might need further attention.
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (4)
18-18: LGTM: NewonProjectDeleteprop added correctly.The addition of the
onProjectDeleteprop is well-implemented and aligns with the PR objective of adding delete functionality to projects. The prop is correctly typed as optional, maintaining backwards compatibility, and follows the established naming convention in the component.
25-26: LGTM:onProjectDeleteparameter added correctly.The
onProjectDeleteparameter is correctly added to the component parameters, corresponding to the new prop in the Props type. Its placement among other callback props is appropriate.
37-38: LGTM:projectDeleteModalVisiblestate added correctly.The
projectDeleteModalVisiblestate variable is well-implemented to manage the visibility of the project delete modal. The naming is clear and consistent with the component's conventions, and it's correctly initialized tofalse.
Line range hint
1-187: Overall, great implementation of project deletion functionality.The changes in this file successfully implement the project deletion feature, aligning well with the PR objectives. The new functions and state variables are well-integrated into the existing component structure.
There's one main point to address:
- Consistently rename functions from "Model" to "Modal":
handleDeleteModelOpen→handleDeleteModalOpenhandleDeleteModelClose→handleDeleteModalCloseOnce these changes are made, remember to update all references to these functions throughout the file.
Great job on implementing this feature!
web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (2)
43-44: LGTM: New function added correctlyThe
handleProjectDeletefunction has been correctly added to the destructured return value of theuseHookscustom hook. This change is consistent with the new project deletion functionality being implemented.
Line range hint
1-335: Summary: Project deletion functionality successfully implementedThe changes in this file successfully implement project deletion functionality for both grid and list views in the Projects component. The implementation is consistent and well-integrated with the existing code structure.
Key points:
- The
handleProjectDeletefunction is correctly added to the custom hook's return value.- Both
ProjectGridViewItemandProjectListViewItemcomponents now have theonProjectDeleteprop, enabling deletion in both view modes.Minor optimization suggestions were made to use
useCallbackfor theonProjectDeletefunctions to potentially improve performance with large numbers of projects.Overall, the changes look good and achieve the intended functionality.
web/src/services/i18n/translations/en.yml (1)
Line range hint
1-486: Address overall consistency issues in the translation file.There are several consistency issues throughout the file that should be addressed:
Inconsistent quoting style: Some keys are single-quoted while others are unquoted. This may lead to parsing issues or confusion for developers.
Empty translation values: Many keys have empty string values. These should be filled with appropriate translations to avoid missing text in the UI.
Unusual key at the end: The file ends with a key 'Style_' which seems out of place and might be a leftover or mistake.
Please address these issues to improve the overall quality and consistency of the translation file:
- Standardize the quoting style for all keys. Preferably, use single quotes for all keys.
- Fill in translations for all empty string values.
- Review the 'Style_' key at the end of the file and remove it if it's not needed.
To help identify these issues, you can run the following script:
#!/bin/bash # Description: Check for consistency issues in the translation file echo "Keys with inconsistent quoting:" rg --type yaml '^[^''#].*:' web/src/services/i18n/translations/en.yml echo "\nEmpty translations:" rg --type yaml ": ''" web/src/services/i18n/translations/en.yml echo "\nUnusual keys:" rg --type yaml "Style_" web/src/services/i18n/translations/en.ymlReview the output and make necessary corrections to improve consistency.
web/src/beta/lib/reearth-ui/components/Icon/icons.ts (2)
5-5: LGTM: New icon import added correctly.The import statement for
ArrowCounterClockWisefollows the established pattern in the file, using the correct naming convention and file path structure.
Line range hint
1-300: Verify icon usage in the codebase.The changes to this file (adding
ArrowCounterClockWiseand removingSwitch) may impact other parts of the application where these icons are used.Please run the following script to check for any existing usage of the removed
Switchicon and potential places where the newArrowCounterClockWiseicon could be utilized:Ensure that any occurrences of the
Switchicon are updated or removed, and consider if the newArrowCounterClockWiseicon could be beneficial in places where undo, reset, or revert functionality is implemented.web/src/services/i18n/translations/ja.yml (2)
50-50: Translation for "Remove" looks good.The Japanese translation "削除" for "Remove" is accurate and commonly used in user interfaces.
61-61: Translation for "Delete" is correct.The Japanese translation "削除" for "Delete" is accurate and consistent with the translation for "Remove".
server/e2e/gql_project_test.go (3)
213-213: Approved: Addition of 'Project 5' for deletion testsThe creation of
Project 5is appropriate for testing the deletion and starring functionalities. This setup enhances the coverage of edge cases in the test suite.
259-260: Approved: Validation of starred projects count after deletionThe assertion correctly verifies that only two starred projects (
Project 1andProject 3) remain after deletingProject 5. This ensures that deleted starred projects are excluded from the starred projects list.
476-476: Approved: Deletion of 'project2-test' for testingThe deletion of
project2-testsets up the test case for verifying the retrieval of deleted projects. This action aligns with the test objectives.web/src/services/api/projectApi.ts (3)
32-33: New GraphQL query 'GET_DELETED_PROJECTS' importedThe
GET_DELETED_PROJECTSquery is correctly added to the imports.
354-354: Ensure deleted projects list is updated after deletionAdding
"GetDeletedProjects"torefetchQueriesin thedeleteProjectMutationensures the deleted projects list is refreshed after a project is deleted.
574-576: Export new hooks for managing deleted projectsThe
useUpdateProjectRecycleBinanduseDeletedProjectsQueryhooks are now exported, enabling other components to interact with deleted projects functionality.web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx (5)
5-5: ImportingProjectDeleteModalThe
ProjectDeleteModalcomponent is correctly imported, enabling project deletion functionality within the grid view item.
15-16: AddingonProjectDeleteprop toProjectGridViewItemThe
onProjectDeletecallback prop is properly added to the component's props, allowing parent components to handle project deletion events.
27-34: Destructuring new handlers and state fromuseHooksThe additional handlers and state variables (
projectDeleteModalVisible,handleDeleteModalClose,handleProjectDelete) are correctly destructured fromuseHooks, integrating the deletion logic into the component.
42-43: PassingonProjectDeletetouseHooksThe
onProjectDeleteprop is correctly passed touseHooks, allowing the hook to utilize the deletion callback provided by the parent component.
97-103: Conditional rendering ofProjectDeleteModalThe
ProjectDeleteModalis properly integrated with conditional rendering based onprojectDeleteModalVisible. The modal receives the necessary props and handlers, ensuring it functions as intended within the component.
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/RecycleBinProject.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/RecycleBinProject.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts
Outdated
Show resolved
Hide resolved
|
|
||
| func deleteProject(e *httpexpect.Expect, projectID string) { | ||
|
|
||
| updateProjectMutation := GraphQLRequest{ | ||
| OperationName: "UpdateProject", | ||
| Query: `mutation UpdateProject($input: UpdateProjectInput!) { | ||
| updateProject(input: $input) { | ||
| project { | ||
| id | ||
| name | ||
| isDeleted | ||
| updatedAt | ||
| __typename | ||
| } | ||
| __typename | ||
| } | ||
| }`, | ||
| Variables: map[string]any{ | ||
| "input": map[string]any{ | ||
| "projectId": projectID, | ||
| "deleted": true, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| response := e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("X-Reearth-Debug-User", uID.String()). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithJSON(updateProjectMutation). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON(). | ||
| Object(). | ||
| Value("data").Object(). | ||
| Value("updateProject").Object(). | ||
| Value("project").Object() | ||
|
|
||
| response.ValueEqual("id", projectID). | ||
| ValueEqual("isDeleted", true) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
'deleteProject' Mutation Exists and Can Be Utilized
- The GraphQL schema includes a
deleteProjectmutation, which can directly mark a project as deleted. - It's advisable to rename the
deleteProjectfunction and update its implementation to use thedeleteProjectmutation instead ofupdateProject.
🔗 Analysis chain
Recommendation: Rename 'deleteProject' function for clarity
The deleteProject function uses the updateProject mutation to mark a project as deleted. Renaming the function to reflect its operation more accurately can improve code readability.
Apply this diff to rename the function:
-func deleteProject(e *httpexpect.Expect, projectID string) {
+func markProjectAsDeleted(e *httpexpect.Expect, projectID string) {Additionally, if the API supports a dedicated deleteProject mutation, consider using it for a more precise operation.
To check if a deleteProject mutation exists, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if a 'deleteProject' mutation exists in the GraphQL schema.
# Expected Result: If 'deleteProject' exists, it will be listed in the output.
# Command:
gh api graphql -F query='{ __schema { mutationType { fields { name } } } }' | grep deleteProject
Length of output: 8684
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (4)
8-19: LGTM: Well-structured hook setup with a minor suggestion.The hook is well-defined with appropriate use of custom hooks and state initialization. The optional
workspaceIdparameter provides flexibility.Consider adding type annotation for the
workspaceIdparameter to enhance type safety:-export default (workspaceId?: string) => { +export default (workspaceId?: string): { + filteredDeletedProjects: DeletedProject[]; + handleProjectDelete: (project?: DeletedProject) => Promise<void>; + handleProjectRecovery: (project?: DeletedProject) => Promise<void>; +} => {
21-34: LGTM: Effective filtering of deleted projects with a suggestion for improvement.The
useEffecthook correctly maps and filters thedeletedProjectsarray, handling potential undefined values. The dependency array is properly set to re-run the effect whendeletedProjectschanges.To improve type safety, consider using a type guard instead of a type assertion:
- .filter(Boolean) as DeletedProject[]; + .filter((project): project is DeletedProject => Boolean(project));This change ensures that TypeScript understands that
undefinedvalues have been removed, eliminating the need for a type assertion.
36-58: LGTM: Well-implemented project recovery function with room for improvement.The
handleProjectRecoveryfunction is correctly implemented usinguseCallbackfor performance optimization. It properly handles the project update process and provides user feedback through notifications.Consider adding more robust error handling:
- const { status } = await useUpdateProjectRecycleBin(updatedProject); - if (status === "success") { + try { + const { status } = await useUpdateProjectRecycleBin(updatedProject); + if (status === "success") { + setNotification({ + type: "success", + text: t("Successfully recover the project!") + }); + } else { + throw new Error("Update failed"); + } + } catch (error) { setNotification({ - type: "success", - text: t("Successfully recover the project!") - }); - } else { - setNotification({ type: "error", - text: t("Failed to recover the project.") + text: t("Failed to recover the project: {error}", { error: error.message }) }); }This change provides more detailed error information to the user and handles potential exceptions from the API call.
60-73: LGTM: Well-structured project deletion function and hook return, with suggestions for improvement.The
handleProjectDeletefunction and the hook's return value are correctly implemented. The use ofuseCallbackforhandleProjectDeleteis appropriate for performance optimization.Consider adding error handling and user feedback for the delete operation:
const handleProjectDelete = useCallback( async (project?: DeletedProject) => { if (!project) return; - await useDeleteProject({ projectId: project.id }); + try { + await useDeleteProject({ projectId: project.id }); + setNotification({ + type: "success", + text: t("Successfully deleted the project!") + }); + } catch (error) { + setNotification({ + type: "error", + text: t("Failed to delete the project: {error}", { error: error.message }) + }); + } }, - [useDeleteProject] + [useDeleteProject, setNotification, t] );This change provides error handling and user feedback for the delete operation, improving the user experience and making the behavior consistent with the recovery function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (8 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (1 hunks)
- web/src/beta/features/ProjectSettings/hooks.ts (4 hunks)
- web/src/services/api/projectApi.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx
🧰 Additional context used
🔇 Additional comments (5)
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (2)
1-7: LGTM: Imports are appropriate and well-organized.The imports cover all necessary dependencies for the hook's functionality, including API services, internationalization, state management, and React hooks. The relative import of the
DeletedProjecttype is also correctly implemented.
1-73: Overall: Well-implemented custom hook with minor suggestions for improvement.This custom hook for managing deleted projects in a workspace's recycle bin is well-structured and follows React best practices. It effectively uses React hooks, including
useState,useEffect, anduseCallback, for state management and performance optimization.Key strengths:
- Proper use of custom hooks for API interactions and notifications.
- Effective filtering and mapping of deleted projects.
- Clear separation of concerns between project recovery and deletion.
Suggestions for improvement:
- Enhance type safety by using type annotations and type guards.
- Implement more robust error handling, especially for the delete operation.
- Provide consistent user feedback for both recovery and deletion operations.
These improvements will further enhance the reliability and user experience of the recycle bin feature.
web/src/services/api/projectApi.ts (3)
32-33: LGTMThe addition of
GET_DELETED_PROJECTSto the imports is appropriate.
341-341: LGTMAdding
"GetDeletedProjects"torefetchQueriesindeleteProjectMutationensures the list of deleted projects is refreshed after a project is deleted.
561-563: LGTMExporting
useUpdateProjectRecycleBinanduseDeletedProjectsQuerymakes the new hooks available for managing deleted projects.
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (19)
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1)
17-56: LGTM: Well-structured component with minor improvement needed.The component is well-structured and follows React best practices. The use of the
useThook for internationalization is good for maintainability. However, there's a minor typo in the warning message.Please fix the typo in the warning message:
- "This means the project will no longer be published. But you can still see and restore you project from recycle bin." + "This means the project will no longer be published. But you can still see and restore your project from recycle bin."web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (3)
6-17: LGTM: Hook initialization and state setup are well-structured.The hook follows React best practices and provides flexibility with the optional
workspaceIdparameter. The use ofuseStatefor local state management is appropriate.Consider adding type annotations for the
workspaceIdparameter and the return type of the hook for improved type safety:export default (workspaceId?: string): { filteredDeletedProjects: DeletedProject[]; isLoading: boolean; deleteModalVisible: boolean; handleDeleteModalClose: (value: boolean) => void; handleProjectDelete: (project?: DeletedProject) => Promise<void>; handleProjectRecovery: (project?: DeletedProject) => Promise<void>; } => { // ... existing code ... }
18-31: LGTM: Effective processing of deleted projects.The
useEffecthook appropriately processes the fetched deleted projects, ensuring that only valid projects are stored in the state. The dependency array is correctly set to[deletedProjects].Consider memoizing the
mappedProjectscalculation usinguseMemoto optimize performance, especially ifdeletedProjectsis a large array:const filteredDeletedProjects = useMemo(() => { return (deletedProjects ?? []) .map<DeletedProject | undefined>((project) => { if (!project) return undefined; return { id: project.id, name: project.name, imageUrl: project.imageUrl, isDeleted: project.isDeleted }; }) .filter(Boolean) as DeletedProject[]; }, [deletedProjects]);This way, the mapping and filtering will only be recalculated when
deletedProjectschanges.
35-55: LGTM: Project recovery and deletion functions are well-implemented.The
handleProjectRecoveryandhandleProjectDeletefunctions are correctly memoized usinguseCallback. The null checks before performing actions prevent potential errors.Consider adding error handling to these functions to manage potential API failures:
const handleProjectRecovery = useCallback( async (project?: DeletedProject) => { if (!project) return; try { await useUpdateProjectRecycleBin({ projectId: project.id, deleted: false }); // Optionally, update local state or trigger a refetch here } catch (error) { console.error("Failed to recover project:", error); // Optionally, show an error message to the user } }, [useUpdateProjectRecycleBin] ); const handleProjectDelete = useCallback( async (project?: DeletedProject) => { if (!project) return; try { await useDeleteProject({ projectId: project.id }); // Optionally, update local state or trigger a refetch here } catch (error) { console.error("Failed to delete project:", error); // Optionally, show an error message to the user } }, [useDeleteProject] );This will provide better feedback to the user and help with debugging if API calls fail.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (2)
29-66: LGTM: Modal structure and content are well-organized, with a minor typo.The modal structure is well-organized, providing clear instructions and warnings to the user. The conditional disabling of the delete button is a good safety measure.
However, there's a minor typo in the warning message:
- "This action cannot be undone. This will permanently delete the project totally. You will lost all your project data" + "This action cannot be undone. This will permanently delete the project entirely. You will lose all your project data"
20-67: Consider adding a comment about error handling.The component structure and functionality are well-implemented. The confirmation mechanism adds an extra layer of safety for project deletion.
However, it would be beneficial to add a comment indicating that error handling for the delete action is handled in another file. This would improve code clarity and maintainability.
Consider adding a comment like this at the top of the component:
// Note: Error handling for the delete action is managed in [specify the file name].web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/items/RecycleBinItem.tsx (2)
21-23: Consider simplifying thehandleProjectHoverfunction.The
handleProjectHoverfunction can be simplified since it's just setting the state directly.You could replace the
handleProjectHoverfunction with inline state updates:- const handleProjectHover = useCallback((value: boolean) => { - setIsHovered(value); - }, []); // In the JSX - onMouseEnter={() => handleProjectHover(true)} - onMouseLeave={() => handleProjectHover(false)} + onMouseEnter={() => setIsHovered(true)} + onMouseLeave={() => setIsHovered(false)}This change reduces unnecessary abstraction and improves readability.
100-112: Consider using a cross-browser solution for text truncation.The
CardTitlecomponent uses webkit-specific properties for text truncation, which may not work in all browsers.Consider using a more cross-browser compatible solution. Here's an example:
const CardTitle = styled("div")(({ theme }) => ({ flex: "1", padding: `0 ${theme.spacing.smallest + 1}px`, color: theme.content.main, fontSize: theme.fonts.sizes.body, fontWeight: theme.fonts.weight.regular, - display: "-webkit-box", - WebkitBoxOrient: "vertical", - WebkitLineClamp: 1, + display: "block", overflow: "hidden", textOverflow: "ellipsis", + whiteSpace: "nowrap", cursor: "pointer" }));This change ensures the text truncation works across different browsers.
web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (3)
36-36: Prop rename looks good, consider adding JSDoc.The rename from
onDeleteProjecttoonProjectRemoveis consistent with the new terminology.Consider adding a JSDoc comment to describe the purpose of this prop:
/** Callback function to handle project removal */ onProjectRemove: () => void;
116-125: LGTM: Updated "Danger Zone" section for project removal.The changes in text and functionality are consistent with the new project removal process. The updated description provides clearer information about what happens when a project is removed.
Consider extracting the "Remove project" button text to a constant or using the
tfunction directly in the title prop for consistency with other translated strings:title={t("Remove project")}
Line range hint
1-153: Overall implementation aligns well with PR objectives.The changes in this file successfully implement the new project removal functionality, enhancing the project management features as intended. The code is clean, follows React best practices, and maintains the existing component structure.
Key points:
- Consistent terminology change from "delete" to "remove".
- Addition of the ProjectRemoveModal for a more modular approach.
- Updated UI text to provide clearer information about the removal process.
These changes align well with the PR objectives of enhancing the project dashboard and project settings by adding delete (now remove) functionality.
Consider adding unit tests for the new functionality, especially for the
handleProjectRemoveModalfunction and the conditional rendering of theProjectRemoveModal.web/src/beta/features/ProjectSettings/hooks.ts (1)
158-158: Update return statement to match function renamingThe renaming of
handleDeleteProjecttohandleProjectRemovein the return statement is consistent with the earlier function renaming. However, if you implement the suggestion to rename the function tohandleToggleProjectDeletedState, make sure to update this line accordingly:handleToggleProjectDeletedState,This ensures consistency throughout the component.
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx (2)
15-16: LGTM: New onProjectRemove prop addedThe new
onProjectRemoveprop is correctly typed and follows the existing naming convention.Consider grouping related props together for better readability. You might want to place
onProjectRemoveright afteronProjectUpdatefor a more logical grouping of project modification actions.
97-103: LGTM: ProjectRemoveModal correctly implementedThe
ProjectRemoveModalis correctly implemented with conditional rendering and proper prop passing.Consider simplifying the
onCloseprop by directly passing the function reference:- onClose={() => handleProjectRemoveModal(false)} + onClose={handleProjectRemoveModal}This assumes that
handleProjectRemoveModalis defined to accept a boolean parameter. If it's not, you'll need to update its definition in theuseHooksfunction.web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (1)
88-89: LGTM: Enhanced Project type with starred and deleted propertiesThe addition of
starredanddeletedproperties to the Project type enhances the project management capabilities and aligns with the PR objectives.Minor suggestion: Consider renaming
deletedtoisDeletedfor consistency withisArchived.web/src/services/i18n/translations/en.yml (1)
50-50: Approve changes for project removal and recycle bin functionalityThe new translation keys for project removal and recycle bin functionality provide better clarity on the project management process. The language has been updated to differentiate between "remove" (moving to recycle bin) and "delete" (permanent removal), which should improve user understanding.
Consider updating the following key for better grammar:
-'Your project will be move to trash.: '' +'Your project will be moved to trash.: ''Also applies to: 57-58, 60-67, 235-237
web/src/services/i18n/translations/ja.yml (2)
Line range hint
445-446: Translate workspace deletion messagesThe following workspace deletion messages are missing translations:
- "Failed to delete workspace."
- "Successfully deleted workspace!"
Suggested translations:
- "ワークスペースの削除に失敗しました。"
- "ワークスペースが正常に削除されました！"
Please add these translations to provide a complete Japanese interface for workspace deletion functionality.
Line range hint
1-450: General feedback on Japanese translationsThank you for the ongoing efforts to improve the Japanese localization. Here's a summary of the review findings:
- Several new translations have been added, which is great progress.
- Some translations are still missing, particularly for new features like project trash and workspace management.
- There are a few inconsistencies in terminology that could be addressed.
To further improve the localization:
- Complete all missing translations, especially those highlighted in the review comments.
- Ensure consistency in terminology across similar features (e.g., "削除" for delete/remove actions).
- Consider having a native Japanese speaker review the translations for natural phrasing and consistency with common UI conventions in Japanese software.
- Fix the typos in the English source text for some messages (e.g., "Failde restored the project!").
Keep up the good work on improving the user experience for Japanese users!
web/src/services/api/projectApi.ts (1)
110-123: LGTM: New hook for querying deleted projectsThe
useDeletedProjectsQueryhook is well-implemented and follows the established patterns in the file. Good use ofuseMemofor performance optimization.Consider adding error handling to provide feedback to the user in case the query fails.
You could add error handling like this:
const useDeletedProjectsQuery = useCallback((teamId?: string) => { - const { data, ...rest } = useQuery(GET_DELETED_PROJECTS, { + const { data, error, ...rest } = useQuery(GET_DELETED_PROJECTS, { variables: { teamId: teamId ?? "" }, skip: !teamId }); + useEffect(() => { + if (error) { + setNotification({ + type: "error", + text: t("Failed to fetch deleted projects.") + }); + } + }, [error, setNotification, t]); const deletedProjects = useMemo( () => data?.deletedProjects.nodes, [data?.deletedProjects] ); - return { deletedProjects, ...rest }; + return { deletedProjects, error, ...rest }; }, []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectListViewItem.tsx (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/types.ts (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (4 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/items/RecycleBinItem.tsx (1 hunks)
- web/src/beta/features/ProjectSettings/hooks.ts (3 hunks)
- web/src/beta/features/ProjectSettings/index.tsx (2 hunks)
- web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (4 hunks)
- web/src/services/api/projectApi.ts (5 hunks)
- web/src/services/i18n/translations/en.yml (3 hunks)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/types.ts
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx
- web/src/beta/features/ProjectSettings/index.tsx
🧰 Additional context used
📓 Learnings (3)
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx:17-53 Timestamp: 2024-10-18T08:21:56.740Z Learning: Error handling for the delete action in the `ProjectDeleteModal` component is handled in another file.web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx:17-53 Timestamp: 2024-10-18T08:21:56.740Z Learning: Error handling for the delete action in the `ProjectDeleteModal` component is handled in another file.web/src/services/api/projectApi.ts (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/services/api/projectApi.ts:110-123 Timestamp: 2024-10-18T08:13:08.942Z Learning: In `web/src/services/api/projectApi.ts`, when `teamId` is undefined, passing an empty string `""` as `teamId` to the GraphQL query is correct because the API requires a string and handles empty strings appropriately.
🔇 Additional comments (42)
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (3)
1-16: LGTM: Imports and type definition are well-structured.The imports are appropriate for the component's functionality, and the
Propstype is well-defined with the necessary properties for the modal.
58-69: LGTM: Well-defined styled components.The styled components
WrapperandWarningIconare well-defined and make good use of the theme object for consistent styling. The use oftheme.spacingfor layout and the theme's warning color for the icon contributes to a cohesive design.
1-69: Summary: Well-implemented ProjectRemoveModal component with minor improvements suggested.This new
ProjectRemoveModalcomponent aligns well with the PR objectives of enhancing project deletion functionality. It provides a clear confirmation step before removing a project, which is crucial for preventing accidental deletions.The component is well-structured, uses appropriate UI components, and implements internationalization. The suggested improvements (fixing a typo and adding loading/error handling) will further enhance the user experience and code robustness.
Overall, this component contributes positively to the feature of adding delete functionality to the project dashboard and settings.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (3)
1-5: LGTM: Imports and type definitions are well-organized.The imports are appropriate for the hook's functionality, and the separation of the
DeletedProjecttype into a different file promotes good type organization.
61-69: LGTM: Hook return value is comprehensive and well-structured.The hook returns all necessary state values and handler functions for managing deleted projects. The naming is clear and descriptive, making it easy for consumers of this hook to understand and use the provided functionality.
1-69: Overall, excellent implementation of the RecycleBin hook.This custom hook effectively encapsulates the logic for managing deleted projects in a workspace's recycle bin, aligning well with the PR objectives of enhancing project management functionality. The code is well-structured, follows React best practices, and provides a clean interface for components to interact with the recycle bin features.
Key strengths:
- Proper use of React hooks (useState, useEffect, useCallback)
- Clear separation of concerns
- Efficient data processing and state management
Minor suggestions for improvement have been provided in previous comments, mainly focusing on type safety and error handling. These enhancements would further improve the robustness of the implementation.
This file significantly contributes to the PR's goal of introducing a Recycle Bin page to manage deleted projects, providing the necessary hooks for fetching, recovering, and permanently deleting projects.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (3)
1-19: LGTM: Imports and type definition are well-structured.The imports are appropriate for the component's functionality, and the
Propstype is well-defined with all necessary properties.
20-27: LGTM: Component declaration and hooks usage are correct.The
ProjectDeleteModalis properly declared as a functional component. The use ofuseTfor internationalization anduseStatefor managing thedeleteInputNamestate follows React best practices.
71-77: LGTM: Styled component is well-defined.The
ModalContentWrapperstyled component is well-defined and makes good use of theme properties for consistent styling.web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/items/RecycleBinItem.tsx (4)
1-12: LGTM: Imports and type definitions are well-structured.The imports cover all necessary dependencies, and the
Proptype is correctly defined with optional properties. This provides a good foundation for the component.
13-61: LGTM: Component logic is well-structured and follows React best practices.The component makes good use of React hooks, handles localization properly, and has a clear rendering logic. The popup menu implementation is also well done.
65-98: LGTM: Styled components are well-implemented with good use of theming.The styled components make effective use of the theme object and implement responsive design. The
CardImagecomponent's handling of background image and hover state is particularly well done.
1-112: Overall, excellent implementation of the RecycleBinItem component.The
RecycleBinItemcomponent is well-structured, making good use of React hooks, styled components, and following best practices. It effectively handles the display and interaction for project items in a recycle bin interface. The code is clean, readable, and maintainable.A few minor improvements were suggested:
- Simplifying the
handleProjectHoverfunction.- Using a more cross-browser compatible solution for text truncation in the
CardTitlecomponent.These suggestions are minor optimizations and don't detract from the overall high quality of the implementation. Great job!
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (3)
1-15: LGTM: Imports and component declaration are well-structured.The imports cover all necessary dependencies, and the component is correctly declared as a functional component with TypeScript props.
77-113: LGTM: Well-structured styled components with responsive design.The styled components are well-implemented, using theme variables and media queries for responsive design. The layout structure is clear and adaptable to different screen sizes.
1-113: Overall assessment: Good implementation with room for improvement.The RecycleBin component is well-structured and implements the required functionality. However, there are a few areas that could be improved for better performance, robustness, and user experience. Please address the issues mentioned in the previous comments to enhance the overall quality of the component.
web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (2)
2-2: LGTM: New component import for project removal.The import of
ProjectRemoveModalis correctly added and aligns with the new project removal functionality.
42-42: LGTM: Prop rename in destructuring.The rename from
onDeleteProjecttoonProjectRemovein the component's props destructuring is consistent with the Props type definition update.web/src/beta/features/ProjectSettings/hooks.ts (2)
29-30: LGTM: New import for project recycle bin functionalityThe addition of
useUpdateProjectRecycleBinaligns with the PR objectives of implementing delete functionality and a Recycle Bin for projects.
Line range hint
1-166: Overall implementation aligns well with PR objectivesThe changes in this file successfully implement the recycle bin functionality for projects, which aligns well with the PR objectives. The modifications include:
- Adding the necessary import for
useUpdateProjectRecycleBin.- Implementing a toggle function for project deletion state.
- Updating the return statement to include the new function.
These changes enhance the project management capabilities by allowing users to move projects to a recycle bin instead of permanent deletion, which improves user experience and data safety.
Consider implementing the suggested improvements in the previous comments to further refine the implementation.
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx (3)
5-5: LGTM: New import for ProjectRemoveModalThe new import for
ProjectRemoveModalis correctly placed and follows proper naming conventions. It's appropriately used later in the component.
27-27: LGTM: New variables and functions for project removalThe new variables and functions for project removal are correctly added and follow existing naming conventions. The
onProjectRemoveprop is properly passed to theuseHookscall.Reminder: There's still a typo in the function name
handleDeleteModelClose. It should behandleDeleteModalCloseto match the naming convention and the relatedProjectDeleteModalcomponent. This was mentioned in a previous review and hasn't been addressed yet.Also applies to: 33-34, 42-43
Line range hint
1-203: Overall assessment: Project removal functionality well implementedThe changes to implement the project removal functionality are well-executed. The new modal, props, and hooks are correctly integrated into the existing component. There are a few minor suggestions for improvement:
- Consider grouping related props together (e.g.,
onProjectUpdateandonProjectRemove).- Address the typo in
handleDeleteModelClosefrom a previous review.- Simplify the
onCloseprop of theProjectRemoveModal.These changes enhance the component's functionality while maintaining good code quality and consistency.
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (4)
42-43: LGTM: Addition of useUpdateProjectRecycleBinThe addition of
useUpdateProjectRecycleBinto the destructured return ofuseProjectFetcheraligns well with the PR objectives of adding delete functionality. This is a logical extension of the existing project management capabilities.
125-128: LGTM: Refetch on search or sort changesThe addition of this
useEffecthook ensures that the project list is updated when search terms or sorting criteria change. This is a good practice and improves the user experience by keeping the displayed data in sync with the user's actions.
296-297: LGTM: Exposed handleProjectRemove in hook returnThe addition of
handleProjectRemoveto the return object of the custom hook is appropriate. This makes the new delete functionality accessible to components that use this hook, which is necessary for implementing the project deletion feature.
Line range hint
1-365: Overall assessment: Well-implemented delete functionality with minor improvements suggestedThe changes in this file successfully implement the delete functionality for projects, aligning well with the PR objectives. The code is well-structured and maintains consistency with the existing codebase.
Key points:
- The addition of
useUpdateProjectRecycleBinand thedeletedproperty to the Project type provide the necessary infrastructure for project deletion.- The
handleProjectRemovefunction implements the core deletion logic.- The new functionality is properly exposed through the hook's return object.
Suggestions for improvement:
- Consider adding error handling to
handleProjectRemovefor better user feedback.- For consistency, consider renaming the
deletedproperty toisDeletedin the Project type.These changes significantly enhance the project management capabilities of the dashboard.
web/src/services/i18n/translations/en.yml (3)
402-405: Approve changes for workspace managementThe new translation keys for workspace management provide clear and consistent messaging for removal and deletion actions. The distinction between removing a workspace and deleting it permanently is well-defined, which should help prevent accidental data loss.
404-405: Approve success message additionsThe new success messages for project and workspace actions provide clear and positive feedback to users. They accurately reflect the actions taken and are consistent with the new workflow for project and workspace management.
Line range hint
1-405: Approve overall changes and consistencyThe changes to the translation file maintain consistency in style and format while introducing new keys for enhanced project and workspace management functionality. The removal of obsolete keys and the addition of new ones reflect the updated workflow in the application.
To ensure all new keys are being used in the application and that removed keys are no longer referenced, please run the following verification script:
✅ Verification successful
Approve overall changes and consistency
The updates to the translation file are correctly reflected in the codebase. All new translation keys are actively used, and the removed keys are no longer referenced, ensuring consistency and adherence to the updated project workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of new translation keys and absence of removed keys # Check for usage of new keys echo "Checking usage of new translation keys:" rg --type typescript --type javascript "t\(['\"]Remove['\"]" ./src rg --type typescript --type javascript "t\(['\"]Your project will be move to trash['\"]" ./src rg --type typescript --type javascript "t\(['\"]No Project in recycle bin['\"]" ./src rg --type typescript --type javascript "t\(['\"]Recover['\"]" ./src rg --type typescript --type javascript "t\(['\"]Delete project['\"]" ./src rg --type typescript --type javascript "t\(['\"]Remove this project['\"]" ./src rg --type typescript --type javascript "t\(['\"]Remove workspace['\"]" ./src rg --type typescript --type javascript "t\(['\"]Delete workspace['\"]" ./src echo "\nChecking for usage of removed keys:" rg --type typescript --type javascript "t\(['\"]Delete this project['\"]" ./src rg --type typescript --type javascript "t\(['\"]This process will remove this project to recycle bin['\"]" ./src echo "\nPlease review the output to ensure new keys are used and removed keys are not referenced."Length of output: 1594
Script:
#!/bin/bash # Description: Verify usage of new translation keys and absence of removed keys by searching relevant file extensions # Define file extensions to search extensions=("ts" "tsx" "js" "jsx") echo "Checking usage of new translation keys:" for ext in "${extensions[@]}"; do echo "\nSearching in .$ext files:" rg "t\(['\"]Remove['\"]" --glob "*.$ext" ./src rg "t\(['\"]Your project will be move to trash['\"]" --glob "*.$ext" ./src rg "t\(['\"]No Project in recycle bin['\"]" --glob "*.$ext" ./src rg "t\(['\"]Recover['\"]" --glob "*.$ext" ./src rg "t\(['\"]Delete project['\"]" --glob "*.$ext" ./src rg "t\(['\"]Remove this project['\"]" --glob "*.$ext" ./src rg "t\(['\"]Remove workspace['\"]" --glob "*.$ext" ./src rg "t\(['\"]Delete workspace['\"]" --glob "*.$ext" ./src done echo "\nChecking for usage of removed keys:" for ext in "${extensions[@]}"; do echo "\nSearching in .$ext files:" rg "t\(['\"]Delete this project['\"]" --glob "*.$ext" ./src rg "t\(['\"]This process will remove this project to recycle bin['\"]" --glob "*.$ext" ./src done echo "\nPlease review the output to ensure new keys are used and removed keys are not referenced."Length of output: 5647
Script:
#!/bin/bash # Description: Verify usage of new translation keys and absence of removed keys by searching all relevant file extensions in the repository # Define file extensions to search extensions=("ts" "tsx" "js" "jsx") echo "Checking usage of new translation keys:" for ext in "${extensions[@]}"; do echo "\nSearching in .$ext files:" rg "t\(['\"]Remove['\"]" --glob "*.$ext" . rg "t\(['\"]Your project will be move to trash['\"]" --glob "*.$ext" . rg "t\(['\"]No Project in recycle bin['\"]" --glob "*.$ext" . rg "t\(['\"]Recover['\"]" --glob "*.$ext" . rg "t\(['\"]Delete project['\"]" --glob "*.$ext" . rg "t\(['\"]Remove this project['\"]" --glob "*.$ext" . rg "t\(['\"]Remove workspace['\"]" --glob "*.$ext" . rg "t\(['\"]Delete workspace['\"]" --glob "*.$ext" . done echo "\nChecking for usage of removed keys:" for ext in "${extensions[@]}"; do echo "\nSearching in .$ext files:" rg "t\(['\"]Delete this project['\"]" --glob "*.$ext" . rg "t\(['\"]This process will remove this project to recycle bin['\"]" --glob "*.$ext" . done echo "\nPlease review the output to ensure new keys are used and removed keys are not referenced."Length of output: 4583
Script:
#!/bin/bash # Description: Verify usage of new translation keys and absence of removed keys by searching within ./web/src directory # Define file extensions to search extensions=("ts" "tsx" "js" "jsx") # Define the directory to search search_dir="./web/src" # Check if the search directory exists if [ ! -d "$search_dir" ]; then echo "Error: Directory $search_dir does not exist." exit 1 fi echo "Checking usage of new translation keys in $search_dir:" for ext in "${extensions[@]}"; do echo "\nSearching in .$ext files:" rg "t\(['\"]Remove['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]Your project will be move to trash['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]No Project in recycle bin['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]Recover['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]Delete project['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]Remove this project['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]Remove workspace['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]Delete workspace['\"]" --glob "*.$ext" "$search_dir" done echo "\nChecking for usage of removed keys in $search_dir:" for ext in "${extensions[@]}"; do echo "\nSearching in .$ext files:" rg "t\(['\"]Delete this project['\"]" --glob "*.$ext" "$search_dir" rg "t\(['\"]This process will remove this project to recycle bin['\"]" --glob "*.$ext" "$search_dir" done echo "\nPlease review the output to ensure new keys are used and removed keys are not referenced."Length of output: 4893
web/src/services/i18n/translations/ja.yml (4)
50-50: LGTM: Appropriate translation for 'Remove'The translation "削除" (delete) for "Remove" is correct and commonly used in Japanese interfaces.
57-58: Skip: Missing translations already addressed in past reviewThe missing translations for project trash-related messages have already been addressed in a previous review comment. Please refer to the existing comment for suggested translations.
61-61: Skip: Missing translation for 'Recover' already addressedThe missing translation for 'Recover' has already been addressed in a previous review comment. Please refer to the existing comment for the suggested translation.
63-63: LGTM: Appropriate translation for 'Delete project'The translation "プロジェクト削除" (project deletion) for "Delete project" is correct and clearly conveys the intended meaning.
web/src/services/api/projectApi.ts (4)
32-33: LGTM: New import for deleted projects queryThe addition of
GET_DELETED_PROJECTSimport is appropriate for the new functionality being implemented to manage deleted projects.
358-358: LGTM: Updated refetch queries for delete project mutationThe addition of "GetDeletedProjects" to the
refetchQueriesarray is appropriate. This ensures that the list of deleted projects is updated immediately after a project is deleted, keeping the UI in sync with the backend state.
578-580: LGTM: New hooks exportedThe addition of
useUpdateProjectRecycleBinanduseDeletedProjectsQueryto the returned object is appropriate. This makes the new functionality for managing deleted projects available to other parts of the application.
Line range hint
1-581: Overall assessment: Well-implemented feature for managing deleted projectsThe changes in this file successfully implement new functionality for managing deleted projects. The new hooks and mutations are consistent with the existing patterns in the file and are properly integrated into the API service.
Key points:
- New query and mutation for deleted projects are added.
- Error handling and success notifications are implemented.
- The changes are well-integrated with existing code.
Minor improvements suggested:
- Fix a typo in an error message.
- Consider adding error handling to the
useDeletedProjectsQueryhook.Great job on implementing this feature!
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectListViewItem.tsx (4)
11-12: Import Statement Added SuccessfullyThe import of
ProjectRemoveModalis correctly added and necessary for the new project removal functionality.
44-44: Destructure 'projectRemoveModalVisible' CorrectlyThe
projectRemoveModalVisiblestate is destructured fromuseHooks, which is appropriate and ensures the modal visibility is managed correctly.
49-51: Destructure Handlers CorrectlyThe handlers
handleProjectStarClick,handleProjectRemoveModal, andhandleProjectRemoveare correctly destructured fromuseHooks, facilitating proper event handling.
59-60: Pass 'onProjectRemove' to 'useHooks'Passing
onProjectRemovetouseHooksis appropriate, assuming thatuseHooksaccepts this parameter and utilizes it correctly within its logic.
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectListViewItem.tsx
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectListViewItem.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/src/services/i18n/translations/ja.yml (1)
Line range hint
1-397: Overall translation review and suggestions for improvementThe file has seen significant updates with many new translations added. However, there are still some areas that need attention:
- Several crucial messages, especially those related to project removal and trash operations, are missing translations.
- Some English messages contain grammatical errors that should be corrected.
- There might be inconsistencies in terminology usage across different messages.
To improve the overall quality of the Japanese localization:
- Review all empty translations and provide appropriate Japanese text.
- Double-check the consistency of terminology, especially for terms like "project", "trash", "recycle bin", etc.
- Consider having a native Japanese speaker review the translations for natural language flow and cultural appropriateness.
- Implement a process to ensure that new English strings are immediately translated to Japanese to prevent gaps in localization.
These improvements will greatly enhance the user experience for Japanese users of the application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
web/src/services/i18n/translations/ja.yml (3)
50-50: LGTM: New translation for "Remove" addedThe Japanese translation for "Remove" has been correctly added as "削除".
57-58:⚠️ Potential issueEmpty translations for project trash-related messages
The translations for the following messages are still empty:
- "Your project will be move to trash."
- "This means the project will no longer be published. But you can still see and restore you project from recycle bin."
These messages are crucial for users to understand the implications of moving a project to trash.
Please add the following translations:
- "プロジェクトはゴミ箱に移動されます。"
- "プロジェクトは公開されなくなりますが、ゴミ箱から確認および復元することができます。"
Also, there's a typo in the English version: "move" should be "moved" and "you" should be "your".
227-229:⚠️ Potential issueEmpty translations for project removal messages
The following project removal messages are still missing translations:
- "Remove this project"
- "This process will move this project to recycle bin."
- "Remove project"
Please add the following translations:
- "このプロジェクトを削除"
- "このプロセスでプロジェクトをゴミ箱に移動します。"
- "プロジェクトを削除"
These translations are necessary to provide a complete Japanese interface for project removal functionality.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (1)
web/src/services/i18n/translations/en.yml (1)
Line range hint
1-467: Consider standardizing key formatting in a future taskWhile reviewing the changes, I noticed some inconsistencies in the formatting of keys throughout the file. These inconsistencies existed before the new changes and are not specific to the newly added keys. To improve maintainability, consider creating a future task to standardize the formatting of all keys in this file.
Specifically, you might want to address:
- Consistent use of sentence case or title case for keys
- Consistent use of punctuation at the end of keys (e.g., whether to include a colon or not)
This standardization would improve the overall consistency and readability of the translation file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/services/i18n/translations/en.yml (3 hunks)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
web/src/services/i18n/translations/en.yml (1)
Line range hint
1-467: Approve changes with minor corrections neededThe changes to this translation file align well with the PR objectives of enhancing delete functionality and introducing a Recycle Bin for projects. The new keys provide necessary translations for these features, improving the user experience for project management.
Please ensure that you address the issues raised in the previous comments, including:
- Adding missing translations for empty keys
- Fixing grammatical errors in some keys
- Maintaining consistency in key formatting
Once these minor corrections are made, the changes will significantly enhance the localization support for the new project management features.
web/src/services/i18n/translations/ja.yml (1)
50-50: LGTM: Accurate translation for "Remove"The translation "削除" (sakujo) is correct and commonly used for "Remove" in Japanese user interfaces.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
web/src/beta/features/Dashboard/type.ts (2)
16-16: LGTM: NewisDeletedfield for soft delete functionalityThe addition of the
isDeletedfield to theProjecttype is excellent. It aligns well with the PR objectives and enables soft delete functionality, which is a best practice for data management.Consider adding a JSDoc comment to explain the purpose of this field, especially its role in soft delete functionality. For example:
/** Indicates whether the project has been soft-deleted. */ isDeleted?: boolean;
19-23: LGTM with suggestions: NewDeletedProjecttypeThe introduction of the
DeletedProjecttype is a great addition that supports the new Recycle Bin feature. It provides a clear structure for managing deleted projects.Consider the following improvements:
The
isDeletedfield might be redundant in a type specifically for deleted projects. If it's needed for consistency with theProjecttype, consider making it required and always true.Adding a
deletedAttimestamp could be beneficial for sorting and potential automatic cleanup of old deleted projects.Here's a suggested refactor:
export type DeletedProject = { id: string; name: string; imageUrl?: string | null; isDeleted: true; deletedAt: Date; };This change ensures all
DeletedProjectinstances are marked as deleted and includes information about when the deletion occurred.web/src/beta/features/Published/NotFound.tsx (2)
6-25: LGTM: Well-structured NotFound component with a minor suggestion.The NotFound component is well-implemented, utilizing hooks for theming and translations effectively. The layout is clean and follows React best practices.
Consider adding an appropriate
aria-labelto the wrapper div for better accessibility:- <Wrapper> + <Wrapper aria-label={t("Page not found")}>
29-43: LGTM: Well-implemented styled components with a performance optimization suggestion.The styled components are well-structured, utilizing theme values consistently and employing flexbox for responsive layouts.
Consider memoizing the styled components to potentially optimize performance, especially if they're used in multiple instances:
-const Wrapper = styled("div")(({ theme }) => ({ +const Wrapper = React.memo(styled("div")(({ theme }) => ({ // ... existing styles -})); +}))); -const EmptyContent = styled("div")(({ theme }) => ({ +const EmptyContent = React.memo(styled("div")(({ theme }) => ({ // ... existing styles -})); +})));This optimization might be beneficial if these components are rendered frequently or in lists.
web/src/beta/pages/Page.tsx (1)
57-76: LGTM: Improved rendering logic with proper error handling.The new
renderContentlogic usinguseMemois a good improvement:
- It properly handles loading states and deleted projects, enhancing the user experience.
- The use of
useMemocan help optimize performance by memoizing the render content.- The dependencies array for
useMemois complete and correct.Consider extracting the
project?.isDeletedcheck into a separate variable for improved readability:const isProjectDeleted = project?.isDeleted ?? false; const renderContent = useMemo(() => { if (loading) return <Loading animationSize={80} />; if (isProjectDeleted) return <NotFound />; // ... rest of the code }, [loading, isProjectDeleted, /* ... other dependencies */]);This minor refactoring can make the code slightly more readable and easier to maintain.
web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (2)
43-44: LGTM! Consider grouping related functions for better readability.The addition of
handleProjectRemovealigns well with the PR objective of adding delete functionality. To improve code readability, consider grouping related functions together in the destructured assignment. For example:const { // ... other properties handleProjectCreate, handleProjectUpdate, handleProjectOpen, handleProjectSelect, handleProjectRemove, handleImportProject, // ... other properties } = useHooks(workspaceId);This grouping makes it easier to identify all project-related functions at a glance.
Line range hint
1-324: Overall implementation looks good, with a few minor suggestions for improvement.The changes effectively integrate the new project removal functionality into the existing
Projectscomponent. The implementation is consistent across both grid and list view modes, aligning well with the PR objectives. Here are a few suggestions for further improvement:
- Consider adding error handling for the
handleProjectRemovefunction, especially if it involves asynchronous operations.- Update the component's PropTypes or TypeScript interface to include the new
onProjectRemoveprop for bothProjectGridViewItemandProjectListViewItem.- If not already implemented, consider adding a confirmation dialog before project removal to prevent accidental deletions.
- Update any relevant documentation or comments to reflect the new project removal functionality.
These suggestions aim to enhance the robustness and maintainability of the code. Great job on implementing the new feature!
web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (1)
67-69: Consider simplifyinghandleProjectRemoveModalfor clarity.Since
handleProjectRemoveModalis toggling the modal visibility, you might simplify the function to toggle the state without needing a parameter.Simplified function:
-const handleProjectRemoveModal = useCallback((value: boolean) => { - setProjectRemoveModalVisible(value); -}, []); +const handleProjectRemoveModal = useCallback(() => { + setProjectRemoveModalVisible(prev => !prev); +}, []);Update the onClick handlers accordingly:
- onClick={() => handleProjectRemoveModal(true)} + onClick={handleProjectRemoveModal}- onClose={() => handleProjectRemoveModal(false)} + onClose={handleProjectRemoveModal}web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (1)
44-44: Consider renaminguseUpdateProjectRemovefor clarityThe hook name
useUpdateProjectRemovecombines "Update" and "Remove," which might be confusing. Consider renaming it touseRemoveProjectoruseDeleteProjectto clearly convey its purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (3 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (1 hunks)
- web/src/beta/features/Dashboard/type.ts (1 hunks)
- web/src/beta/features/ProjectSettings/hooks.ts (4 hunks)
- web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (4 hunks)
- web/src/beta/features/Published/NotFound.tsx (1 hunks)
- web/src/beta/features/Published/index.tsx (1 hunks)
- web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Timeline/UI/index.tsx (3 hunks)
- web/src/beta/lib/reearth-ui/components/Icon/icons.ts (2 hunks)
- web/src/beta/pages/Page.tsx (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Timeline/UI/index.tsx
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx:56-60 Timestamp: 2024-10-18T09:53:50.873Z Learning: In the `RecycleBin` component (`web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx`), there is no need to display the loading state when there are no projects.
🔇 Additional comments (25)
web/src/beta/features/Published/index.tsx (1)
4-4: LGTM! Verify the new location of the NotFound component.The import statement has been updated to use a local path, which suggests improved code organization. This change should not affect the functionality of the
Publishedcomponent.To ensure the
NotFoundcomponent is correctly located, run the following script:✅ Verification successful
Verified: NotFound.tsx exists in the expected location.
The import statement correctly references the local NotFound component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the NotFound component in the current directory # Test: Check if NotFound.tsx exists in the same directory if [ -f "$(dirname "$0")/NotFound.tsx" ]; then echo "NotFound.tsx exists in the correct location." else echo "Error: NotFound.tsx not found in the expected location." exit 1 fiLength of output: 207
Script:
#!/bin/bash # Description: Search for all instances of NotFound.tsx in the repository fd NotFound.tsxLength of output: 62
web/src/beta/features/Dashboard/type.ts (2)
10-10: LGTM: Optional description fieldMaking the
descriptionfield optional in theProjecttype is a good change. It allows for more flexibility in project management, especially when creating new projects or handling existing projects that might not have a description.
Line range hint
1-23: Overall assessment: Well-structured changes supporting new featuresThe modifications to this file effectively support the new delete functionality and Recycle Bin feature described in the PR objectives. The changes are well-structured and maintain consistency with the existing codebase. The suggestions provided aim to further improve clarity and functionality.
To ensure these changes are properly utilized, let's verify their usage in related files:
This script will help verify that the new fields and types are being used appropriately in related files within the Dashboard feature.
✅ Verification successful
Verification Successful: The new fields and types are appropriately utilized within the Dashboard feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of new types and fields in related files # Test 1: Check usage of optional description field echo "Checking usage of optional description field:" rg "description\?:" web/src/beta/features/Dashboard # Test 2: Check usage of isDeleted field echo "Checking usage of isDeleted field:" rg "isDeleted\?:" web/src/beta/features/Dashboard # Test 3: Check usage of DeletedProject type echo "Checking usage of DeletedProject type:" rg "DeletedProject" web/src/beta/features/Dashboard # Test 4: Check for any TODO comments related to these changes echo "Checking for related TODO comments:" rg "TODO.*(?:description|isDeleted|DeletedProject)" web/src/beta/features/DashboardLength of output: 2296
web/src/beta/features/Published/NotFound.tsx (2)
1-4: LGTM: Imports are appropriate and concise.The imports are well-organized and relevant to the component's functionality, utilizing internal libraries for UI components, internationalization, and theming.
1-43: Overall assessment: Well-implemented NotFound component.This new NotFound component is a solid addition to the project, aligning well with the PR objectives. It follows React best practices, utilizes project-specific libraries effectively, and maintains consistency with the existing architecture. The minor suggestions for accessibility and performance optimization can further enhance its quality, but the current implementation is already robust and ready for integration.
web/src/beta/pages/Page.tsx (5)
7-7: LGTM: Import changes enhance code clarity and functionality.The modifications to the import statements are appropriate:
- Importing
FCdirectly instead ofReact.FCis a common practice that slightly improves code readability.- The addition of the
NotFoundcomponent import aligns with the new functionality for handling deleted projects.Also applies to: 11-12
26-26: LGTM: Component signature update is consistent.The change from
React.FC<Props>toFC<Props>aligns with the import modifications and improves overall code consistency.
49-50: LGTM: Project query and loading state improvements.The changes enhance the component's functionality and correctness:
- Including the
projectobject in the destructuring allows for checking if a project is deleted.- The loading state calculation now correctly uses
||instead of??. This is more appropriate as||is used for boolean operations, while??is for nullish coalescing.Also applies to: 53-53
79-79: LGTM: Consistent component signature update.The change from
React.FC<Props>toFC<Props>for thePagecomponent is consistent with the earlier modifications to imports and thePageWrappercomponent. This maintains code consistency throughout the file.
Line range hint
1-91: Overall, excellent improvements to error handling and rendering logic.The changes in this file significantly enhance the
PageandPageWrappercomponents:
- Improved error handling for deleted projects.
- Optimized rendering logic using
useMemo.- Better loading state management.
- Consistent use of
FCtype across components.These modifications align well with the PR objectives of enhancing project management functionality, particularly in handling deleted projects. The code is now more robust and provides a better user experience.
web/src/beta/features/Dashboard/ContentsContainer/Projects/index.tsx (2)
194-194: LGTM! Project removal functionality added correctly.The
onProjectRemoveprop has been correctly added to theProjectGridViewItemcomponent, passing a callback function that invokeshandleProjectRemovewith the project as an argument. This implementation aligns with the PR objective of adding delete functionality to the project dashboard.
204-204: LGTM! Consistent implementation of project removal across view modes.The
onProjectRemoveprop has been correctly added to theProjectListViewItemcomponent, mirroring the implementation inProjectGridViewItem. This ensures consistency in the project removal functionality across different view modes (grid and list).To ensure complete consistency, let's verify that both
ProjectGridViewItemandProjectListViewItemcomponents handle theonProjectRemoveprop identically:✅ Verification successful
Verification Successful! Project removal is consistently implemented across view modes.
Both
ProjectGridViewItemandProjectListViewItemcomponents correctly include theonProjectRemoveprop, ensuring consistent project removal functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent implementation of onProjectRemove in ProjectGridViewItem and ProjectListViewItem # Test: Check for onProjectRemove prop in both components echo "Checking ProjectGridViewItem:" rg -A 3 "ProjectGridViewItem" web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx echo "\nChecking ProjectListViewItem:" rg -A 3 "ProjectListViewItem" web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectListViewItem.tsxLength of output: 825
Script:
#!/bin/bash # Description: Search for onProjectRemove prop in ProjectGridViewItem and ProjectListViewItem echo "Searching for onProjectRemove in ProjectGridViewItem.tsx:" rg "onProjectRemove" web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx echo "\nSearching for onProjectRemove in ProjectListViewItem.tsx:" rg "onProjectRemove" web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectListViewItem.tsxLength of output: 685
web/src/beta/lib/reearth-ui/components/Icon/icons.ts (3)
5-5: LGTM: New icon import added correctly.The new import for
ArrowCounterClockWisefollows the established pattern and naming convention used for other icons in this file.
164-164: LGTM: New icon correctly added to the export object.The
ArrowCounterClockWiseicon is properly added to the export object, maintaining consistency with the existing naming conventions and alphabetical ordering.
5-5: Summary: New icon added for potential undo or history functionality.The addition of the
ArrowCounterClockWiseicon aligns with the PR objectives of enhancing project management functionality. This icon could be used for undo operations or representing history, which is relevant to the new recycle bin feature for deleted projects.The changes are minimal, focused, and maintain consistency with the existing codebase. No issues or inconsistencies were introduced.
Also applies to: 164-164
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1)
45-47: Evaluate the necessity of refetching in useEffectThe
useEffecthook callsrefetch()on every render due to how dependencies are handled. This might cause unnecessary network requests. If the initial query inuseDeletedProjectsQuerysuffices, you may not need this effect at all.Please verify if the
useEffectis necessary. If not, consider removing it to optimize performance.web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (2)
14-21: LGTM on theRecycleBincomponentThe
RecycleBincomponent is well-structured and makes appropriate use of hooks and props.
28-60: Consider adjusting the loading state logicBased on your previous feedback, there is no need to display the loading state when there are no projects in the
RecycleBincomponent. Currently, the loading state is displayed whenfilteredDeletedProjectsis empty andisLoadingistrue. Consider adjusting the logic to align with your preference.web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (7)
2-2: Import ofProjectRemoveModalis correctly added.The import statement ensures that the
ProjectRemoveModalcomponent is available for use within this file.
35-35: Addition of the optionaldisabledprop enhances flexibility.Introducing the
disabledprop to thePropstype allows the component to handle disabled states, improving its reusability and control over user interactions.
37-37:onProjectRemovecallback is appropriately defined.Updating the callback function to
onProjectRemovealigns with the new project removal functionality and improves clarity.
64-69: State management forprojectRemoveModalVisibleis correctly implemented.The use of
useStateanduseCallbackto control the visibility of theProjectRemoveModalensures proper state handling and performance optimization through memoization.
118-127: Text updates in the Danger Zone accurately reflect the new functionality.Changing the wording to "Remove this project" and updating the description clarifies that the project will be moved to the recycle bin instead of being permanently deleted.
134-141: Conditional rendering ofProjectRemoveModalis correctly implemented.The modal is rendered based on
projectRemoveModalVisible, and all necessary props are passed appropriately, ensuring proper functionality.
118-121: Verify consistent terminology for project actions across the application.Ensure that the terms "Remove" and "Recycle Bin" are consistently used instead of "Delete" to prevent user confusion.
Run the following script to identify inconsistent usage of "Delete" in project-related contexts:
✅ Verification successful
Consistent terminology verified. No instances of "Delete" found in project-related user-facing contexts. The application consistently uses "Remove" and "Recycle Bin" for project actions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for instances where "Delete" is used with "project" to ensure consistent terminology. # Test: Find lines containing both "Delete" and "project". rg -i -C 3 'Delete.*project|project.*Delete'Length of output: 232988
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (7)
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1)
27-55: Enhance accessibility with ARIA attributes.While the component uses semantic HTML elements, which is good for accessibility, it could benefit from additional ARIA attributes to improve screen reader support.
Consider adding the following ARIA attributes:
- Add a descriptive label to the modal:
<Modal size="small" visible={isVisible} aria-labelledby="project-remove-modal-title">
- Add a title to the modal content:
<Typography size="body" id="project-remove-modal-title"> {t("Confirm Project Removal")} </Typography>
- Add aria-label to the close button if it doesn't have visible text:
<Button size="normal" title="Cancel" onClick={onClose} aria-label={t("Cancel project removal")} />These changes will improve the component's accessibility and provide better context for screen reader users.
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (1)
29-49: LGTM with a minor suggestion: Modal setup is well-implemented.The Modal and ModalPanel are correctly set up with appropriate props and internationalization. The conditional disabling of the delete button is a good safety measure.
Consider adding an
aria-labelto the Modal for improved accessibility:- <Modal visible={isVisible} size="small"> + <Modal visible={isVisible} size="small" aria-label={t("Delete project confirmation")}>web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (1)
185-193: Consider removing unnecessary null check inhandleProjectRemove.The
if (!projectId) return;check inhandleProjectRemoveseems unnecessary sinceprojectIdis a required parameter. This check might hide potential bugs ifprojectIdis unexpectedly undefined or an empty string.Consider removing the check:
const handleProjectRemove = useCallback( async (projectId: string) => { - if (!projectId) return; handleProjectPublish(projectId); onProjectRemove?.(projectId); handleProjectRemoveModal(false); }, [handleProjectRemoveModal, handleProjectPublish, onProjectRemove] );web/src/beta/features/ProjectSettings/hooks.ts (2)
58-71: Improved publication status handlingThe introduction of
projectPublishedandstoriesPublishedstates usinguseMemois a good improvement. It centralizes the logic for determining publication status and ensures efficient recalculation.One minor suggestion:
Consider extracting the publication status check into a separate utility function for reusability:const isPublished = (status: string) => { const publishmentStatus = toPublishmentStatus(status); return publishmentStatus === "published" || publishmentStatus === "limited"; };Then use it in both
projectPublishedandstoriesPublished:const projectPublished = useMemo(() => isPublished(project?.publishmentStatus), [project?.publishmentStatus]); const storiesPublished = useMemo(() => scene?.stories?.some(story => isPublished(story.publishmentStatus)), [scene?.stories]);This would further improve code reusability and maintainability.
73-101: Efficient project unpublishing with room for error handlingThe
handleProjectPublishfunction is well-implemented, efficiently handling the unpublishing of both the project and its associated stories. The use ofPromise.allfor concurrent story unpublishing is a good approach for performance.Suggestion for improvement:
Consider adding error handling to manage potential failures in the unpublishing process. Here's an example:const handleProjectPublish = useCallback(async (projectId: string) => { try { if (projectPublished) { await usePublishProject("unpublished", projectId); } if (storiesPublished && scene?.stories) { await Promise.all( scene.stories.map(async (story) => { const publishmentStatus = toPublishmentStatus(story.publishmentStatus); if (publishmentStatus === "published" || publishmentStatus === "limited") { await usePublishStory("unpublished", story.id); } }) ); } } catch (error) { console.error("Error during project unpublishing:", error); // Consider adding a notification to the user about the failure } }, [projectPublished, scene?.stories, storiesPublished, usePublishProject, usePublishStory]);This will help in catching and logging any errors that occur during the unpublishing process, improving the robustness of the function.
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (1)
259-281: Implement error handling inhandleProjectRemoveThe
handleProjectRemovefunction is well-implemented and aligns with the PR objectives. It correctly handles the deletion process, including edge cases like "limited" status projects, and manages the Apollo cache for immediate UI updates.However, to improve robustness, consider adding error handling:
const handleProjectRemove = useCallback( async (project: Project) => { + try { const updatedProject = { projectId: project.id, deleted: true }; if (project?.status === "limited") { await usePublishProject("unpublished", project.id); } await useUpdateProjectRemove(updatedProject); client.cache.evict({ id: client.cache.identify({ __typename: "Project", id: project.id }) }); client.cache.gc(); + } catch (error) { + console.error('Failed to remove project:', error); + // TODO: Implement user-friendly error notification + } }, [client, usePublishProject, useUpdateProjectRemove] );This change will prevent silent failures and allow for better error reporting to the user.
web/src/services/i18n/translations/en.yml (1)
Line range hint
1-405: Conduct a comprehensive review of all empty translationsWhile the new keys added align well with the PR objectives of adding delete functionality and introducing a Recycle Bin page, there's a consistent issue of empty translations throughout the file. This could lead to a poor user experience with missing text in various parts of the UI.
Please conduct a thorough review of all empty translations in this file, ensuring that each key has an appropriate English translation. This will help maintain a consistent and informative user interface.
Would you like assistance in generating a script to identify all empty translations in this file?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (5 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts (1 hunks)
- web/src/beta/features/ProjectSettings/hooks.ts (4 hunks)
- web/src/services/api/projectApi.ts (5 hunks)
- web/src/services/i18n/translations/en.yml (3 hunks)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/hooks.ts
🧰 Additional context used
📓 Learnings (3)
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx:17-53 Timestamp: 2024-10-18T08:21:56.740Z Learning: Error handling for the delete action in the `ProjectDeleteModal` component is handled in another file.web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectDeleteModal.tsx:17-53 Timestamp: 2024-10-18T08:21:56.740Z Learning: Error handling for the delete action in the `ProjectDeleteModal` component is handled in another file.web/src/services/api/projectApi.ts (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/services/api/projectApi.ts:110-123 Timestamp: 2024-10-18T08:13:08.942Z Learning: In `web/src/services/api/projectApi.ts`, when `teamId` is undefined, passing an empty string `""` as `teamId` to the GraphQL query is correct because the API requires a string and handles empty strings appropriately.
🔇 Additional comments (26)
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (2)
18-57: LGTM: Component structure and implementation.The component structure is well-organized and follows React best practices. The use of the
useThook for internationalization is appropriate, and the modal structure effectively serves as a confirmation dialog.
61-72: LGTM: Styling approach.The use of styled components for custom styling is consistent with the project's approach. The separation of the
WrapperandWarningIconcomponents keeps the styling clean and modular.web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx (4)
1-18: LGTM: Imports and type definition are well-structured.The imports are appropriate for the component's functionality, and the Props type is well-defined with all necessary properties.
19-27: LGTM: Component declaration and state initialization are well-implemented.The component is properly declared as a functional component, props are correctly destructured, and state is appropriately initialized using the useState hook.
71-77: LGTM: Styled component is well-defined.The ModalContentWrapper styled component is well-implemented, using theme properties for consistent styling and flexbox for layout.
50-63:⚠️ Potential issueEnhance accessibility by adding a label to TextInput.
The modal content is well-structured and informative. However, the TextInput component lacks a label, which may impact accessibility.
Consider adding a label or using aria-label for screen readers:
- <TextInput onChange={(name) => setDeleteInputName(name)} /> + <TextInput + label={t("Project name")} + aria-label={t("Enter project name to confirm deletion")} + onChange={(name) => setDeleteInputName(name)} + />web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (10)
18-18: LGTM: New prop for project removal.The addition of the
onProjectRemoveprop aligns with the PR objective of adding delete functionality to the project dashboard.
25-26: LGTM: Hook parameters updated.The
onProjectRemoveprop has been correctly added to the hook's parameters, maintaining consistency with thePropstype.
29-30: LGTM: Hook imports updated for project removal functionality.The addition of
usePublishStoryandusePublishProjectto the respective fetcher hooks is consistent with the new project removal functionality.
37-38: LGTM: New state for project removal modal.The addition of
projectRemoveModalVisiblestate is appropriate for managing the visibility of the project removal modal.
86-89: LGTM: New function for handling project removal modal.The
handleProjectRemoveModalfunction is well-implemented, memoized, and follows the component's naming conventions.
108-113: LGTM: New menu item for project removal.The addition of the project removal menu item is consistent with the existing menu structure and correctly utilizes the
handleProjectRemoveModalfunction.
146-161: LGTM: Memoized values for publication status.The addition of memoized values for
projectPublishedandstoriesPublishedimproves performance by avoiding unnecessary recalculations.
163-183: LGTM: New function for handling project publishing.The
handleProjectPublishfunction correctly manages the unpublishing of both the project and its associated stories.
202-210: LGTM: Updated return object.The return object of the hook has been correctly updated to include the new functions and state related to project removal.
Line range hint
1-211: Overall, the changes implement project removal functionality effectively.The additions and modifications in this file successfully introduce project removal functionality, aligning with the PR objectives. The code is well-structured, follows React best practices, and includes appropriate error handling. The only minor suggestion is to remove an unnecessary null check in the
handleProjectRemovefunction.web/src/beta/features/ProjectSettings/hooks.ts (3)
1-6: Improved data handling with Apollo Client and storytelling integrationThe changes in imports and function usage, particularly the introduction of
useApolloClientanduseStorytellingFetcher, along with the replacement ofuseDeleteProjectwithuseUpdateProjectRemove, suggest an improvement in data handling and better integration with the GraphQL backend. This change aligns well with modern React and GraphQL best practices.Also applies to: 36-38, 41-43
215-215: Appropriate updates to the hook's return objectThe changes to the return object of the hook, including the addition of the
disabledstate and thehandleProjectRemovefunction, are appropriate and align well with the new implementation of project removal. These additions provide the necessary tools for the component using this hook to manage both the removal process and the associated UI state.Also applies to: 217-217
Line range hint
1-224: Comprehensive enhancement of project management functionalityThe changes in this file represent a significant improvement in project management functionality, particularly in handling project removal and publishing processes. Key improvements include:
- Integration with Apollo Client for better data handling.
- Introduction of separate states for project and story publication status.
- Implementation of a comprehensive project removal process, including unpublishing and cache updates.
- Addition of a
disabledstate to manage UI during operations.These changes provide more granular control over project management processes and align well with modern React and GraphQL best practices. However, there are still opportunities for further improvement:
- Enhance error handling across all asynchronous operations.
- Improve the usage of the
disabledstate to prevent multiple submissions and provide better user feedback.- Consider adding user notifications for operation success/failure.
Overall, these changes significantly enhance the project management capabilities of the application, providing a solid foundation for future improvements.
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (3)
43-45: LGTM: New hooks for project management added.The addition of
useUpdateProjectRemoveandusePublishProjecthooks aligns well with the PR objectives of enhancing project management capabilities, particularly for deletion functionality.
91-92: LGTM: 'isDeleted' property added to project object.The addition of the
isDeletedproperty to the project object is a key component for implementing the recycle bin functionality mentioned in the PR objectives. This boolean flag will allow for efficient filtering and management of deleted projects.
308-308: LGTM:handleProjectRemoveexposed in hook return.The inclusion of
handleProjectRemovein the hook's return object is correct and necessary. This allows components using this hook to access the new delete functionality, aligning with the PR objectives.web/src/services/i18n/translations/ja.yml (3)
50-50: LGTM: Correct translation for "Remove"The translation "削除" for "Remove" is accurate and commonly used in Japanese user interfaces.
Line range hint
1-405: Conduct a final review for consistencyThe majority of the new entries have been addressed in the previous comments. However, it's recommended to conduct a final review of the entire file to ensure:
- All suggested translations have been implemented correctly.
- Consistency in terminology across similar messages (e.g., "project", "delete", "remove", "trash", "recycle bin").
- No remaining empty translations for important messages.
This final review will help maintain a high-quality, consistent Japanese localization for the user interface.
235-237:⚠️ Potential issueAdd missing translations for project removal messages
Please add translations for the following messages, ensuring consistency with previously suggested translations:
-Remove this project: '' +Remove this project: 'このプロジェクトを削除' -This process will move this project to recycle bin.: '' +This process will move this project to recycle bin.: 'このプロセスでプロジェクトをゴミ箱に移動します。' -Remove project: '' +Remove project: 'プロジェクトを削除'These translations are important for Japanese users to understand the project removal process.
Likely invalid or redundant comment.
web/src/services/api/projectApi.ts (1)
110-122: New hook for fetching deleted projects is correctly implementedThe
useDeletedProjectsQueryhook correctly retrieves deleted projects based onteamId, following the established pattern in the file.
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/ProjectDeleteModal.tsx
Show resolved
Hide resolved
|
|
||
| const { project } = useProjectQuery(projectId); | ||
| const [disabled, setDisabled] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance usage of disabled state
The introduction of the disabled state is a good step towards managing component behavior during project removal operations. However, its current implementation could be improved:
- The state is toggled in
handleProjectRemove, but it's not clear where it's being used to actually disable any UI elements. - There's no mechanism to reset the
disabledstate after the operation completes.
Consider the following improvements:
- Use the
disabledstate to prevent multiple submissions of the remove action. - Reset the
disabledstate after the operation completes (success or failure). - Ensure that the
disabledstate is properly used in the component that renders the remove button.
Also applies to: 108-108, 215-215
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (2)
146-182: LGTM: Implemented comprehensive publish status checks and unpublish handlingThe additions of memoized publish status checks and the
handleProjectPublishfunction are well-implemented. They efficiently handle the unpublishing of both projects and all associated stories, addressing a previous review comment.Consider adding a check to avoid unnecessary API calls if there are no published stories:
if (storiesPublished && stories?.length) { + const publishedStories = stories.filter(story => { + const status = toPublishmentStatus(story.publishmentStatus); + return status === "published" || status === "limited"; + }); + if (publishedStories.length > 0) { const storyPromises = stories.map((story) => usePublishStory("unpublished", story.id) ); await Promise.all(storyPromises); + } }This change would prevent unnecessary API calls for unpublished stories.
Line range hint
185-210: LGTM: Implemented project removal handler and updated return objectThe
handleProjectRemovefunction is well-implemented, correctly handling the unpublishing process before removal. The return object is properly updated to include the new functions and state.Consider removing the redundant check for
projectId:const handleProjectRemove = useCallback( async (projectId: string) => { - if (!projectId) return; handleProjectPublish(projectId); onProjectRemove?.(projectId); handleProjectRemoveModal(false); }, [handleProjectRemoveModal, handleProjectPublish, onProjectRemove] );Since
projectIdis a required parameter, the check is unnecessary and can be removed to simplify the function.web/src/services/i18n/translations/en.yml (1)
Line range hint
1-1000: Overall LGTM with suggestions for improvement.The new keys added to this translation file align well with the PR objectives of adding delete functionality and a Recycle Bin page. However, there are a few areas that need attention:
- All new translations are currently empty. Please ensure all new keys have appropriate English translations.
- There are some grammatical and punctuation inconsistencies in the new keys. Please review and correct these for clarity and consistency.
- Ensure capitalization is consistent across all keys, especially for terms like "project" and "recycle bin".
Addressing these points will greatly improve the user experience and maintain consistency throughout the application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (5 hunks)
- web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (4 hunks)
- web/src/services/api/projectApi.ts (5 hunks)
- web/src/services/i18n/translations/en.yml (3 hunks)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/services/i18n/translations/ja.yml
🧰 Additional context used
📓 Learnings (1)
web/src/services/api/projectApi.ts (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/services/api/projectApi.ts:110-123 Timestamp: 2024-10-18T08:13:08.942Z Learning: In `web/src/services/api/projectApi.ts`, when `teamId` is undefined, passing an empty string `""` as `teamId` to the GraphQL query is correct because the API requires a string and handles empty strings appropriately.
🔇 Additional comments (25)
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.ts (6)
18-18: LGTM: onProjectRemove prop added correctlyThe
onProjectRemoveprop has been added with the correct type signature, addressing a previous review comment. This addition aligns well with the PR objective of implementing project deletion functionality.
25-26: LGTM: onProjectRemove prop correctly destructuredThe
onProjectRemoveprop is correctly destructured from the component props, allowing it to be used within the hook.
29-30: LGTM: Added necessary publish-related hooksThe addition of
usePublishStoryandusePublishProjecthooks is a good preparation for handling the unpublishing of stories and projects during the removal process. This change addresses a previous review comment about unpublishing all stories.
37-38: LGTM: Added state for project removal modalThe addition of
projectRemoveModalVisiblestate is a good practice. It allows for a confirmation step before project removal, enhancing user experience and preventing accidental deletions.
86-89: LGTM: Added modal visibility handlerThe
handleProjectRemoveModalfunction is correctly implemented usinguseCallback. It provides a way to toggle the visibility of the project removal confirmation modal, which is a good practice for user interaction.
108-113: LGTM: Added project removal option to menuThe addition of the 'Move to Recycle Bin' option in the popup menu is well-implemented. It correctly uses the
handleProjectRemoveModalfunction to show the confirmation modal, which aligns with the PR objective of adding delete functionality.web/src/services/i18n/translations/en.yml (7)
50-50: LGTM! New key added for Recycle Bin functionality.The new key "Move to Recycle Bin" aligns well with the PR objective of adding delete functionality. However, please note the existing comment about adding the missing translation for this key.
57-58: LGTM! New keys added for project deletion messaging.These new keys provide important information about moving projects to the trash, which aligns with the PR objectives. However, please note the existing comment about adding the missing translations and correcting the grammatical error in the second key.
60-62: LGTM! New keys added for Recycle Bin functionality.These new keys provide essential functionality for the Recycle Bin page, which aligns with the PR objectives. However, please note the existing comment about adding the missing translations and addressing the capitalization inconsistency in the first key.
63-67: LGTM! New keys added for project deletion confirmation.These new keys provide crucial information for the project deletion process, which aligns with the PR objectives. However, please note the existing comment about adding the missing translations and addressing the grammatical and punctuation issues in these keys.
235-236: LGTM! New keys added for project removal process.These new keys provide important information about the project removal process, which aligns with the PR objectives. However, please note the existing comment about adding the missing translations for these keys.
402-403: LGTM! New keys added for error messages in project management.These new keys provide important error messages for the project management process, which aligns with the PR objectives. However, please note the existing comment about adding the missing translations and correcting the grammatical error in the second key.
404-405: LGTM! New keys added for success messages in project management.These new keys provide important success messages for the project management process, which aligns with the PR objectives. However, please note the existing comment about adding the missing translations for these keys.
web/src/services/api/projectApi.ts (5)
32-33: LGTM: New imports added for project managementThe new imports for
IMPORT_PROJECTandGET_DELETED_PROJECTSare correctly added and align with the new functionalities introduced in this file.
110-123: LGTM: Well-implemented hook for fetching deleted projectsThe
useDeletedProjectsQueryhook is well-structured and consistent with other query hooks in this file. It correctly handles the optionalteamIdparameter and returns the deleted projects data in a structured format.
290-320: LGTM: Comprehensive implementation of project removal/restoration functionalityThe
updateProjectRemoveMutationanduseUpdateProjectRemovehook are well-implemented, following the established patterns in the file. The hook correctly handles both project removal and restoration scenarios, with appropriate error handling and success notifications. The use of localized messages is a good practice for internationalization.
358-358: LGTM: Improved data consistency in delete operationThe addition of "GetDeletedProjects" to the
refetchQueriesarray indeleteProjectMutationensures that the list of deleted projects is updated after a deletion operation. This change is crucial for maintaining data consistency across the application.
578-580: LGTM: New hooks exported for extended project managementThe addition of
useImportProject,useUpdateProjectRemove, anduseDeletedProjectsQueryto the return statement makes these new functionalities available to other parts of the application. The ordering is logical, grouping related functions together.web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (7)
2-2: ImportingProjectRemoveModalis correctly implementedThe
ProjectRemoveModalcomponent is properly imported from the correct path, ensuring that it can be used within this component.
35-35: Addition of optionaldisabledprop enhances flexibilityThe inclusion of the optional
disabled?: boolean;prop in thePropstype allows the component to handle disabled states, improving its reusability and control over user interactions.
42-44: Destructuring of new props is correctly addedThe destructuring of the
disabledandonProjectRemoveprops in the component parameters ensures that these props are accessible within the component.
64-69: State and handler forProjectRemoveModalare properly implementedThe state variable
projectRemoveModalVisibleand the handlerhandleProjectRemoveModalare correctly set up. The previous typo in the state variable name has been fixed, aligning with the naming conventions.
118-121: User messages updated for clarity on project removalThe updated text provides clear information to the user about moving the project to the recycle bin, enhancing user understanding of the action's consequences.
134-141: Conditional rendering ofProjectRemoveModalwith correct propsThe
ProjectRemoveModalis conditionally rendered based onprojectRemoveModalVisible, and all necessary props, includingisVisible,onClose,onProjectRemove, anddisabled, are correctly passed to the modal.
37-37: Ensure all references are updated toonProjectRemoveThe
onProjectRemovecallback is added to theProps. To maintain consistency and prevent any potential issues with undefined functions, verify that all previous references toonDeleteProjecthave been updated toonProjectRemove.Run the following script to check for any remaining references to
onDeleteProject:
| title={t("Move to Recycle Bin")} | ||
| appearance="dangerous" | ||
| onClick={() => setDeleteModelVisible(true)} | ||
| onClick={() => handleProjectRemoveModal(true)} |
There was a problem hiding this comment.
Consider disabling the removal button based on the disabled state
The "Move to Recycle Bin" button does not currently reflect the disabled state of the component. To prevent user interaction when the action is not permitted, consider adding the disabled prop to the button.
Apply this diff to include the disabled prop:
<Button
title={t("Move to Recycle Bin")}
appearance="dangerous"
+ disabled={disabled}
onClick={() => handleProjectRemoveModal(true)}
/>📝 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.
| title={t("Move to Recycle Bin")} | |
| appearance="dangerous" | |
| onClick={() => setDeleteModelVisible(true)} | |
| onClick={() => handleProjectRemoveModal(true)} | |
| title={t("Move to Recycle Bin")} | |
| appearance="dangerous" | |
| disabled={disabled} | |
| onClick={() => handleProjectRemoveModal(true)} |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (2)
38-39: Consider utilizing the disabled prop in UI feedback.The
disabledprop is passed toRecycleBinItembut its effect on user interaction isn't immediately clear. Consider:
- Adding visual feedback when disabled
- Preventing actions when disabled
Example implementation in RecycleBinItem:
const RecycleBinItem = ({ disabled, onProjectRecovery, ...props }) => { const handleRecovery = () => { if (disabled) return; onProjectRecovery(); }; return ( <ItemWrapper disabled={disabled}> {/* existing content */} </ItemWrapper> ); };
100-106: Consider using relative height for LoadingWrapper.Using
height: 100vhmight cause issues in different contexts, such as when the component is nested inside a scrollable container. Consider using a relative height or min-height instead.const LoadingWrapper = styled("div")(() => ({ display: "flex", justifyContent: "center", alignItems: "center", width: "100%", - height: "100vh" + height: "100%", + minHeight: "400px" // Provide reasonable minimum height }));web/src/services/i18n/translations/en.yml (3)
50-50: Maintain consistent capitalization of "Recycle Bin"There are inconsistencies in the capitalization of "Recycle Bin" across translations:
- Line 50: "Recycle Bin"
- Line 58: "Recycle bin"
- Line 60: "Recycle bin"
Please use "Recycle Bin" consistently throughout the translations.
-No Project in Recycle bin.: '' +No Project in Recycle Bin.: '' -This means the project will no longer be published. But you can still see and recover your project from the Recycle bin.: '' +This means the project will no longer be published. But you can still see and recover your project from the Recycle Bin.: ''Also applies to: 58-58, 60-60
66-67: Improve clarity of deletion confirmation messages
- The word "totally" is redundant in the permanent deletion message
- Remove the trailing period from the project name prompt for consistency
-This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: '' +This action cannot be undone. This will permanently delete the project. You will lose all your project data: '' -Please type your project name to continue.: '' +Please type your project name to continue: ''
402-405: Standardize status message formattingThe success/failure messages should follow consistent patterns:
- Use consistent verb tense between failure and success messages
- Standardize punctuation (exclamation marks)
-Failed to move to the Recycle bin.: '' +Failed to move to Recycle Bin: '' -Failed to recover the project!: '' +Failed to recover project: '' -Successfully moved to Recycle bin!: '' +Successfully moved to Recycle Bin: '' -Successfully recovered the project!: '' +Successfully recovered project: ''
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx (1 hunks)
- web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (1 hunks)
- web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (4 hunks)
- web/src/services/api/projectApi.ts (5 hunks)
- web/src/services/i18n/translations/en.yml (3 hunks)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx
🧰 Additional context used
📓 Learnings (2)
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (2)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx:83-98 Timestamp: 2024-10-21T21:37:50.814Z Learning: In this project, `theme.breakpoints.down` is not available; therefore, suggestions to use it are not applicable.Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx:56-60 Timestamp: 2024-10-18T09:53:50.873Z Learning: In the `RecycleBin` component (`web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx`), there is no need to display the loading state when there are no projects.web/src/services/api/projectApi.ts (1)
Learnt from: mkumbobeaty PR: reearth/reearth-visualizer#1181 File: web/src/services/api/projectApi.ts:110-123 Timestamp: 2024-10-18T08:13:08.942Z Learning: In `web/src/services/api/projectApi.ts`, when `teamId` is undefined, passing an empty string `""` as `teamId` to the GraphQL query is correct because the API requires a string and handles empty strings appropriately.
🔇 Additional comments (16)
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/index.tsx (3)
1-13: LGTM! Well-organized imports.The imports are logically grouped and include all necessary dependencies for the component's functionality.
14-65: LGTM! Well-structured component with proper state handling.The component effectively manages different states (loading, empty, and data) and implements proper error prevention with optional chaining and null checks.
67-73: Verify the height: 0 implementation.The combination of
flex: 1andheight: 0is unusual. Could you clarify if this is intentional for scroll containment? If not, consider removingheight: 0or documenting the reason for this specific implementation.web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx (6)
2-3: LGTM: Import and type changes are well-structured!The changes properly introduce the ProjectRemoveModal component and update the Props type to support the new removal functionality.
Also applies to: 35-35, 37-37
42-42: LGTM: Props destructuring is consistent!The destructuring properly includes the new disabled prop and renamed onProjectRemove callback.
Also applies to: 44-44
64-69: LGTM: Modal state management is well-implemented!The modal visibility state and handler are correctly implemented with proper naming (fixing the previous "Model" typo) and optimization using useCallback.
118-127: LGTM: UI text changes align with the Recycle Bin feature!The text changes properly reflect that projects are moved to the Recycle Bin rather than being permanently deleted.
134-141: LGTM: Modal rendering is properly implemented!The ProjectRemoveModal is correctly rendered with all necessary props, including the disabled state and callbacks.
2-2: Verify translation keys and component dependencies.Let's verify the translation keys and component dependencies:
Also applies to: 118-121, 125-125
✅ Verification successful
Let me gather more information about the translation keys and component usage.
The previous results showed that:
- The specified translation file wasn't found, but there might be translations in other locations
- The ProjectRemoveModal component exists at the correct path
- We need to verify its implementation and usage to ensure compatibility
Let me check these aspects before making a final determination.
Let me check the translation keys in the correct translation file and verify the component's props to ensure compatibility.
This will help us verify:
- If the translation keys exist in the correct location
- If the props passed in GeneralSettings match the component's requirements
- The exact props interface of ProjectRemoveModal
Let me fix the regex for props definition and check one last time.
From the previous results, we learned:
- All required translation keys exist in the correct location
- The component is being used in GeneralSettings with isVisible, onClose, onProjectRemove, and disabled props
- Need to verify these props match the component's interface and other usage patterns
Component and translations are properly integrated
The verification shows that:
- All required translation keys exist in
web/src/services/i18n/translations/en.yml- The
ProjectRemoveModalcomponent exists and is properly imported- The props used in GeneralSettings (
isVisible,onClose,onProjectRemove,disabled) match exactly with the component's Props interface- The usage pattern is consistent with other implementations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify translation keys and ProjectRemoveModal component # Test 1: Check if translation keys exist in en.yml echo "Checking translation keys..." rg "Remove this project|This process will move this project to Recycle bin|Move to Recycle Bin" "web/src/locales/en.yml" # Test 2: Verify ProjectRemoveModal component exists echo "Checking ProjectRemoveModal component..." fd "ProjectRemoveModal.(tsx|js)" "web/src/beta/features/Dashboard/ContentsContainer/Projects"Length of output: 517
Script:
#!/bin/bash # Check for translation files in different locations and component usage echo "Checking for translation files..." fd "en.json|en.yaml|en.yml" "web" echo -e "\nChecking ProjectRemoveModal usage..." rg -l "ProjectRemoveModal" "web/src" echo -e "\nChecking ProjectRemoveModal implementation..." rg -A 5 "const ProjectRemoveModal|export.*ProjectRemoveModal" "web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx"Length of output: 1212
Script:
#!/bin/bash # Check translation keys in the correct file and component props echo "Checking translation keys in the correct file..." rg "Remove this project|This process will move this project to Recycle bin|Move to Recycle Bin" "web/src/services/i18n/translations/en.yml" echo -e "\nChecking ProjectRemoveModal props usage in GeneralSettings..." rg -A 5 "ProjectRemoveModal" "web/src/beta/features/ProjectSettings/innerPages/GeneralSettings/index.tsx" echo -e "\nChecking ProjectRemoveModal props definition..." rg "type Props = {" -A 5 "web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx"Length of output: 2079
Script:
#!/bin/bash # Check ProjectRemoveModal props definition with fixed regex echo "Checking ProjectRemoveModal props definition..." rg -A 5 "^type Props" "web/src/beta/features/Dashboard/ContentsContainer/Projects/ProjectRemoveModal.tsx" echo -e "\nChecking other usages of ProjectRemoveModal for prop patterns..." rg -A 2 "<ProjectRemoveModal" "web/src"Length of output: 1690
web/src/services/i18n/translations/ja.yml (1)
63-63:⚠️ Potential issueRemove duplicate translation key
The "Delete project" entry appears to be a duplicate of the one that appears later in the file. Consider removing this duplicate entry to maintain consistency and prevent confusion.
-Delete project: ''Likely invalid or redundant comment.
web/src/services/api/projectApi.ts (6)
32-33: LGTM: Import statements are properly organizedThe new imports follow the established pattern and are correctly placed with other query imports.
110-122: LGTM: Well-implemented query hook for deleted projectsThe hook follows the established patterns in the file:
- Proper handling of
teamId(confirmed by previous feedback)- Efficient memoization of results
- Consistent error handling
290-292: LGTM: Comprehensive refetch queriesThe mutation correctly refetches all necessary queries to maintain data consistency across the UI.
358-358: LGTM: Proper cache managementThe mutation correctly refetches the deleted projects query while maintaining the existing cache update logic.
578-580: LGTM: Proper export of new functionalityThe new hooks are correctly exported and well-organized with related functionality.
293-320: Consider improving error loggingThe implementation is solid, but the console error message could be more descriptive based on the action being performed.
Apply this diff to improve error logging:
- console.log("GraphQL: Failed to move project to Recycle bin", errors); + console.log( + `GraphQL: Failed to ${input.deleted ? 'move project to Recycle bin' : 'recover project'}`, + errors + );Also, verify that the GraphQL mutation accepts the
deletedfield:✅ Verification successful
Let me try a different approach to verify the GraphQL schema.
This should help us locate and verify the GraphQL schema definition for the UpdateProject mutation and its input type.
Based on the search results, I can now provide a final response as we have found the necessary information about the GraphQL schema. The
UpdateProjectInputtype is defined in the schema and includes thedeletedfield as a valid input parameter.The error logging improvement suggestion is valid and the mutation accepts the
deletedfieldThe suggested improvement to the error logging is appropriate, and the verification confirms that the
deletedfield is a valid input parameter for theUpdateProjectmutation. This is evidenced by:
- The
UpdateProjectInputtype definition inserver/gql/project.graphqland its generated types- The mutation being used in the code correctly handles the
deletedfieldYou can proceed with implementing the suggested error message improvement:
- console.log("GraphQL: Failed to move project to Recycle bin", errors); + console.log( + `GraphQL: Failed to ${input.deleted ? 'move project to Recycle bin' : 'recover project'}`, + errors + );🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UpdateProjectInput type definition ast-grep --pattern 'input UpdateProjectInput { $$$ }'Length of output: 57
Script:
#!/bin/bash # Search for GraphQL schema files fd -e graphql -e gql # Search for UpdateProject mutation and input type in the schema rg -A 10 "UpdateProjectInput|updateProject.*mutation"Length of output: 26287
| @@ -47,13 +47,24 @@ No Project has been created yet: '' | |||
| Rename: 名称変更 | |||
| Project Setting: プロジェクト設定 | |||
| Export: エクスポート | |||
| Move to Recycle Bin: '' | |||
There was a problem hiding this comment.
Add missing translations for recycle bin messages
These important messages related to the recycle bin functionality are missing translations:
-Move to Recycle Bin: ''
+Move to Recycle Bin: 'ゴミ箱に移動'
-Your project will be moved to Recycle Bin.: ''
+Your project will be moved to Recycle Bin.: 'プロジェクトはゴミ箱に移動されます。'
-This means the project will no longer be published. But you can still see and recover your project from the Recycle bin.: ''
+This means the project will no longer be published. But you can still see and recover your project from the Recycle bin.: 'プロジェクトは公開されなくなりますが、ゴミ箱から確認および復元することができます。'Also applies to: 57-58
| Failed to move to the Recycle bin.: '' | ||
| Failed to recover the project!: '' | ||
| Successfully moved to Recycle bin!: '' | ||
| Successfully recovered the project!: '' |
There was a problem hiding this comment.
Add missing translations for status messages
Add translations for the operation status messages:
-Failed to move to the Recycle bin.: ''
+Failed to move to the Recycle bin.: 'ゴミ箱への移動に失敗しました。'
-Failed to recover the project!: ''
+Failed to recover the project!: 'プロジェクトの復元に失敗しました。'
-Successfully moved to Recycle bin!: ''
+Successfully moved to Recycle bin!: 'ゴミ箱への移動が完了しました。'
-Successfully recovered the project!: ''
+Successfully recovered the project!: 'プロジェクトの復元が完了しました。'📝 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.
| Failed to move to the Recycle bin.: '' | |
| Failed to recover the project!: '' | |
| Successfully moved to Recycle bin!: '' | |
| Successfully recovered the project!: '' | |
| Failed to move to the Recycle bin.: 'ゴミ箱への移動に失敗しました。' | |
| Failed to recover the project!: 'プロジェクトの復元に失敗しました。' | |
| Successfully moved to Recycle bin!: 'ゴミ箱への移動が完了しました。' | |
| Successfully recovered the project!: 'プロジェクトの復元が完了しました。' |
| Remove this project: '' | ||
| This process will move this project to Recycle bin.: '' |
There was a problem hiding this comment.
Add missing translations for project removal process
Add translations for the project removal confirmation messages:
-Remove this project: ''
+Remove this project: 'このプロジェクトを削除'
-This process will move this project to Recycle bin.: ''
+This process will move this project to Recycle bin.: 'このプロセスでプロジェクトをゴミ箱に移動します。'📝 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.
| Remove this project: '' | |
| This process will move this project to Recycle bin.: '' | |
| Remove this project: 'このプロジェクトを削除' | |
| This process will move this project to Recycle bin.: 'このプロセスでプロジェクトをゴミ箱に移動します。' |
| No Project in Recycle bin.: '' | ||
| Recover: '' | ||
| Delete: '' | ||
| Delete project: '' | ||
| Cancel: '' | ||
| I am sure I want to delete this project: '' | ||
| This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: '' | ||
| Please type your project name to continue.: '' |
There was a problem hiding this comment.
Add missing translations for project deletion dialog
These messages are crucial for users to understand the project deletion process:
-No Project in Recycle bin.: ''
+No Project in Recycle bin.: 'ゴミ箱にプロジェクトはありません。'
-Recover: ''
+Recover: '復元'
-Delete: ''
+Delete: '削除'
-Delete project: ''
+Delete project: 'プロジェクトを削除'
-Cancel: ''
+Cancel: 'キャンセル'
-I am sure I want to delete this project: ''
+I am sure I want to delete this project: 'このプロジェクトを削除することを確認しました'
-This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: ''
+This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: 'この操作は取り消せません。プロジェクトは完全に削除され、すべてのプロジェクトデータが失われます。'
-Please type your project name to continue.: ''
+Please type your project name to continue.: '続行するにはプロジェクト名を入力してください。'📝 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.
| No Project in Recycle bin.: '' | |
| Recover: '' | |
| Delete: '' | |
| Delete project: '' | |
| Cancel: '' | |
| I am sure I want to delete this project: '' | |
| This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: '' | |
| Please type your project name to continue.: '' | |
| No Project in Recycle bin.: 'ゴミ箱にプロジェクトはありません。' | |
| Recover: '復元' | |
| Delete: '削除' | |
| Delete project: 'プロジェクトを削除' | |
| Cancel: 'キャンセル' | |
| I am sure I want to delete this project: 'このプロジェクトを削除することを確認しました' | |
| This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: 'この操作は取り消せません。プロジェクトは完全に削除され、すべてのプロジェクトデータが失われます。' | |
| Please type your project name to continue.: '続行するにはプロジェクト名を入力してください。' |
…t/project-recylebin
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
web/src/services/i18n/translations/en.yml (1)
Based on the shell script results, I can now generate a final response since I have clear evidence of terminology inconsistencies in the translation files.
Inconsistent capitalization and terminology found in translation strings
The following inconsistencies need to be addressed in both
en.ymlandja.yml:
"Recycle Bin" capitalization varies across usage:
- Some use "Recycle Bin" (correct)
- Others use "Recycle bin" (incorrect)
- Found in messages like "No Project in Recycle bin" and "Successfully moved to Recycle bin!"
The term "recover" is used consistently throughout the translations, so my earlier suggestion to change it to "restore" was incorrect. We should maintain "recover" as it's already standardized in:
- Button label: "Recover"
- Status messages: "Successfully recovered the project!" and "Failed to recover the project!"
Please update all instances to use consistent capitalization of "Recycle Bin" while keeping the existing "recover" terminology.
🔗 Analysis chain
Line range hint
50-404: Verify terminology consistency across the application.Please ensure consistent terminology usage throughout the application:
- "Recycle Bin" should always be capitalized
- Use "restore" instead of "recover" for consistency
- Status messages should follow the pattern: "Successfully !" or "Failed to ."
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for inconsistent terminology in translation files echo "Checking for terminology inconsistencies..." # Check for inconsistent capitalization of "Recycle Bin" echo "Checking 'Recycle Bin' capitalization:" rg -i "recycle.?bin" --type yaml # Check for mixed usage of recover/restore echo "Checking recover/restore usage:" rg -i "recover|restore" --type yamlLength of output: 2405
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL📒 Files selected for processing (3)
- web/src/beta/features/ProjectSettings/index.tsx (2 hunks)
- web/src/services/i18n/translations/en.yml (3 hunks)
- web/src/services/i18n/translations/ja.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/services/i18n/translations/ja.yml
🔇 Additional comments (2)
web/src/beta/features/ProjectSettings/index.tsx (2)
106-107: LGTM! Verify GeneralSettings implementation.The prop changes are consistent with the hook changes and align with the PR objectives. The
disabledstate is correctly passed to control project interactions.#!/bin/bash # Description: Verify the GeneralSettings component implementation # Expected: Find how the disabled prop and onProjectRemove are used # Search for the GeneralSettings component implementation ast-grep --pattern 'const GeneralSettings = ({ disabled, onProjectRemove }) => { $$$ }' # Check for any disabled state UI feedback rg -A 5 "disabled.*prop" "web/src/beta/features/ProjectSettings/innerPages/GeneralSettings"
51-53: Verify the disabled state implementation.The addition of the
disabledstate suggests a mechanism to prevent interactions with deleted projects. However, let's verify its implementation in the hooks file.Consider adding JSDoc comments to document the purpose of the
disabledstate and when it's activated.✅ Verification successful
Let me gather more information about the disabled state and related changes by searching through the codebase with different strategies.
Let's analyze the implementation details of these changes to verify their correctness and provide appropriate feedback.
Based on the search results, I can now provide a comprehensive verification of the disabled state implementation and the handleProjectRemove changes.
The disabled state and project removal implementation are correctly implemented
The code shows a well-structured implementation where:
- The
disabledstate is properly managed inhooks.tsusinguseState(false)- It's correctly passed down to the
GeneralSettingscomponent which has proper type definitions- The
handleProjectRemovefunction follows the same pattern as other implementations in the codebase, properly handling project deletion with cache updatesKey findings:
- The disabled state is toggled during project removal to prevent multiple deletion attempts
- The project removal flow includes:
- Setting the disabled state
- Handling project publishing state
- Updating the project as deleted
- Cleaning up the cache
- Navigating away on success
The implementation aligns with the PR objectives for project deletion and recycle bin functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of the disabled state in hooks.ts # Expected: Find the disabled state declaration and its usage # Search for disabled state implementation rg -A 5 "disabled.*=" "web/src/beta/features/ProjectSettings/hooks.ts" # Search for disabled state usage in related files rg "disabled.*=" "web/src/beta/features/"Length of output: 183
Script:
#!/bin/bash # Search for the useHooks implementation in the ProjectSettings directory rg -l "useHooks" "web/src/beta/features/ProjectSettings/" # Look for disabled prop usage in GeneralSettings component rg -A 5 "disabled" "web/src/beta/features/ProjectSettings/" # Search for handleProjectRemove implementation ast-grep --pattern 'const handleProjectRemove = $_' # Check the component's props interface/type definition ast-grep --pattern 'interface $_Props { $$$ disabled?: $_ $$$ }'Length of output: 12143
| @@ -47,13 +47,24 @@ No Project has been created yet: '' | |||
| Rename: '' | |||
| Project Setting: '' | |||
| Export: '' | |||
| Move to Recycle Bin: '' | |||
There was a problem hiding this comment.
Fix capitalization consistency and add missing translations for recycle bin UI.
Several translation keys are missing their values and have inconsistent capitalization:
Apply these corrections:
-Move to Recycle Bin: ''
+Move to Recycle Bin: 'Move to Recycle Bin'
-Your project will be moved to Recycle Bin.: ''
+Your project will be moved to Recycle Bin.: 'Your project will be moved to Recycle Bin.'
-This means the project will no longer be published. But you can still see and recover your project from the Recycle bin.: ''
+This means the project will no longer be published. But you can still see and recover your project from the Recycle Bin.: 'This means the project will no longer be published. But you can still see and recover your project from the Recycle Bin.'
-No Project in Recycle bin.: ''
+No projects in Recycle Bin.: 'No projects in Recycle Bin.'
-Recover: ''
+Recover: 'Recover'
-Delete: ''
+Delete: 'Delete'Also applies to: 57-58, 60-62
| Remove this project: '' | ||
| This process will move this project to Recycle bin.: '' |
There was a problem hiding this comment.
Add missing translations for project removal actions.
The project removal messages need translations:
Apply these corrections:
-Remove this project: ''
+Remove this project: 'Remove this project'
-This process will move this project to Recycle bin.: ''
+This process will move this project to Recycle Bin.: 'This process will move this project to Recycle Bin.'📝 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.
| Remove this project: '' | |
| This process will move this project to Recycle bin.: '' | |
| Remove this project: 'Remove this project' | |
| This process will move this project to Recycle Bin.: 'This process will move this project to Recycle Bin.' |
| Failed to move to the Recycle bin.: '' | ||
| Failed to recover the project!: '' | ||
| Successfully moved to Recycle bin!: '' | ||
| Successfully recovered the project!: '' |
There was a problem hiding this comment.
Fix consistency in status messages and add translations.
The status messages need translations and have inconsistent wording:
Apply these corrections:
-Failed to move to the Recycle bin.: ''
+Failed to move project to Recycle Bin.: 'Failed to move project to Recycle Bin.'
-Failed to recover the project!: ''
+Failed to restore the project!: 'Failed to restore the project!'
-Successfully moved to Recycle bin!: ''
+Successfully moved project to Recycle Bin!: 'Successfully moved project to Recycle Bin!'
-Successfully recovered the project!: ''
+Successfully restored the project!: 'Successfully restored the project!'📝 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.
| Failed to move to the Recycle bin.: '' | |
| Failed to recover the project!: '' | |
| Successfully moved to Recycle bin!: '' | |
| Successfully recovered the project!: '' | |
| Failed to move project to Recycle Bin.: 'Failed to move project to Recycle Bin.' | |
| Failed to restore the project!: 'Failed to restore the project!' | |
| Successfully moved project to Recycle Bin!: 'Successfully moved project to Recycle Bin!' | |
| Successfully restored the project!: 'Successfully restored the project!' |
| Delete project: '' | ||
| Cancel: '' | ||
| I am sure I want to delete this project: '' | ||
| This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: '' | ||
| Please type your project name to continue.: '' |
There was a problem hiding this comment.
Add translations and fix punctuation for deletion confirmation dialog.
The deletion confirmation messages need translations and have inconsistent punctuation:
Apply these corrections:
-Delete project: ''
+Delete project: 'Delete project'
-Cancel: ''
+Cancel: 'Cancel'
-I am sure I want to delete this project: ''
+I am sure I want to delete this project: 'I am sure I want to delete this project'
-This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: ''
+This action cannot be undone. This will permanently delete the project. You will lose all your project data: 'This action cannot be undone. This will permanently delete the project. You will lose all your project data'
-Please type your project name to continue.: ''
+Please type your project name to continue: 'Please type your project name to continue'📝 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.
| Delete project: '' | |
| Cancel: '' | |
| I am sure I want to delete this project: '' | |
| This action cannot be undone. This will permanently delete the project totally. You will lose all your project data: '' | |
| Please type your project name to continue.: '' | |
| Delete project: 'Delete project' | |
| Cancel: 'Cancel' | |
| I am sure I want to delete this project: 'I am sure I want to delete this project' | |
| This action cannot be undone. This will permanently delete the project. You will lose all your project data: 'This action cannot be undone. This will permanently delete the project. You will lose all your project data' | |
| Please type your project name to continue: 'Please type your project name to continue' |
Overview
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
ProjectRemoveModalandProjectDeleteModal.NotFoundcomponent for improved user experience when a project is not found.Improvements
ProjectGridViewItemandProjectListViewItemcomponents to handle project deletions and integrate with the recycle bin.Translations