feat(web): members setting #1186
Conversation
This reverts commit e3f84da.
WalkthroughThe changes in this pull request enhance the functionality of workspace and member management within the application. Key modifications include the introduction of new functions for updating member roles and searching for users, as well as improvements to error handling in existing workspace operations. A new Changes
Possibly related PRs
Suggested labels
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
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
web/src/beta/features/WorkspaceSetting/index.tsx (1)
9-9: Consider simplifying the Members component path.The path
./innerPages/Members/Memberscontains a redundant "Members". Consider moving the component to./innerPages/Members/index.tsxto maintain a cleaner import structure.web/src/beta/features/WorkspaceSetting/hooks.ts (1)
141-160: Fix grammatical and consistency issues in member update handler.
- Fix grammatical error in success message
- Make error message consistent with other handlers
if (status === "success") { - console.log("Member update successfully"); + console.log("Member updated successfully"); } } catch (error) { - console.error("Failed to update member from workspace:", error); + console.error("Failed to update member in workspace:", error); }web/src/beta/lib/reearth-ui/components/Icon/icons.ts (1)
Line range hint
262-262: Fix PencilLine import/usage mismatchThere's a mismatch between the imported variable name and the SVG file:
- Import uses
PencilLilne(with typo)- SVG file is named
PencilLine.svgApply this fix:
- pencilLine: PencilLilne, + pencilLine: PencilLine,And update the import statement:
-import PencilLilne from "./Icons/PencilLine.svg?react"; +import PencilLine from "./Icons/PencilLine.svg?react";web/src/services/i18n/translations/en.yml (1)
Line range hint
471-476: Standardize workspace-related message formats.The workspace-related messages have inconsistent formatting and varying levels of detail.
Consider standardizing the messages:
-Failed to create workspace.: '' -Successfully created workspace!: '' -Failed to delete workspace.: '' -Successfully deleted workspace!: '' -Failed to update workspace.: '' -Successfully updated workspace!: '' +Failed to create workspace.: 'Unable to create workspace. Please try again.' +Successfully created workspace!: 'Workspace "{name}" has been created successfully!' +Failed to delete workspace.: 'Unable to delete workspace. Please try again.' +Successfully deleted workspace!: 'Workspace has been permanently deleted.' +Failed to update workspace.: 'Unable to update workspace settings. Please try again.' +Successfully updated workspace!: 'Workspace settings have been updated successfully!'This standardization:
- Adds more context to error messages
- Makes success messages more specific
- Maintains consistent punctuation
- Uses placeholder for dynamic content where applicable
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (5)
313-313: Localize the placeholder textThe placeholder text
"[email protected]"is hardcoded in English. To support localization, wrap it with thetfunction.Apply this diff to fix:
<TextInput - placeholder="[email protected]" + placeholder={t("[email protected]")} value={memberSearchInput} onChange={(input) => { setMemberSearchInput(input); }} />
318-323: Capture input value before clearing itIn the
onKeyDownhandler, you're settingmemberSearchInputto an empty string before using its value forsetDebouncedInput. Since state updates are asynchronous, capture the trimmed input in a variable before clearing the input field to avoid inconsistencies.Apply this diff to fix:
onKeyDown={(e) => { if (e.key === "Enter" && memberSearchInput.trim() !== "") { + const trimmedInput = memberSearchInput.trim(); setMemberSearchInput(""); - setDebouncedInput(memberSearchInput.trim()); + setDebouncedInput(trimmedInput); } }}
337-362: Simplify conditional rendering with optional chainingYou can simplify the conditional rendering by using optional chaining (
?.), which makes the code more concise and readable.Apply this diff to simplify the code:
- {memberSearchResults && - memberSearchResults.map((memberSearchResult) => ( + {memberSearchResults?.map((memberSearchResult) => ( <ItemContainer key={memberSearchResult.id}> <UserInfo> <span>{memberSearchResult.userName}</span> <span style={{ color: theme.content.weak }}> {memberSearchResult.email} </span> </UserInfo> <DeleteIcon icon="trash" size="normal" hasBorder={false} appearance="simple" onClick={() => { setMemberSearchInput(""); setDebouncedInput(""); setMemberSearchResults( memberSearchResults.filter( (element) => element.id !== memberSearchResult.id ), ); }} /> </ItemContainer> - ))} + ))}🧰 Tools
🪛 Biome
[error] 337-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
214-214: Use Role enum instead of string literalsYou're comparing
user.roleto string literals like"OWNER". For type safety and consistency, consider using theRoleenum.Apply this diff to fix:
{ icon: "arrowLeftRight", id: "changeRole", title: t("Change Role"), - disabled: user.role === "OWNER", + disabled: user.role === Role.Owner, onClick: () => { setActiveEditIndex((prevIndex) => prevIndex === index ? null : index, ); }, }, { icon: "close", id: "remove", title: t("Remove"), - disabled: user.role === "OWNER", + disabled: user.role === Role.Owner, onClick: () => { if (currentWorkspace?.id) { handleRemoveMemberFromWorkspace({ teamId: currentWorkspace?.id, userId: user.id, }); } setWorkspaceMembers( workspaceMembers.filter( (workspaceMember) => workspaceMember.id !== user.id, ), ); }, },Also applies to: 225-225
334-334: Consistent use of apostrophes in translated stringsThe string
"Didn’t find the user"uses a typographic apostrophe (’). For consistency and to avoid encoding issues, consider using a standard apostrophe (').Apply this diff to fix:
{t("Didn't find the user")}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
web/src/beta/lib/reearth-ui/components/Icon/Icons/MemberAdd.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
- web/src/beta/features/WorkspaceSetting/hooks.ts (6 hunks)
- web/src/beta/features/WorkspaceSetting/index.tsx (3 hunks)
- web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (1 hunks)
- web/src/beta/hooks/useAccountSettingsTabs.ts (1 hunks)
- web/src/beta/lib/reearth-ui/components/Icon/icons.ts (2 hunks)
- web/src/services/api/meApi.ts (3 hunks)
- web/src/services/i18n/translations/en.yml (2 hunks)
- web/src/services/i18n/translations/ja.yml (7 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/src/services/i18n/translations/ja.yml
🧰 Additional context used
🪛 Biome
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
[error] 337-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
web/src/beta/hooks/useAccountSettingsTabs.ts (2)
23-23: LGTM! Path follows RESTful conventions.The path
/settings/workspaces/:workspaceId/membersfollows RESTful conventions and maintains consistency with other workspace-related routes.
22-24: Verify the Members component implementation.The removal of the
disabledproperty from the members tab suggests this feature is now ready for use. Please ensure that:
- The corresponding Members component is fully implemented
- Proper access controls are in place
- The route is correctly configured in the router
✅ Verification successful
Let me gather more information about the Members component implementation and access controls by checking its contents.
This will help us verify:
- The actual implementation of the Members component
- Workspace-specific access control mechanisms
- Route protection implementation for the members page
Let me check one more thing about the routing and authentication setup since the previous type-specific searches failed.
This will help us verify the routing setup and authentication mechanisms in place.
Based on the gathered information, I can now provide a final response:
The Members component implementation is complete and properly secured
The verification shows that:
- The Members component is fully implemented with proper CRUD operations for workspace members
- Access control is in place through:
- The
useAuthhook which enforces authentication- Role-based permissions (OWNER, MAINTAINER, WRITER, READER)
- Proper restrictions on member management (e.g., OWNER role cannot be removed)
- The route
/settings/workspaces/:workspaceId/membersis correctly configured inuseAccountSettingsTabs.tsThe removal of the
disabledproperty is justified as the feature appears production-ready with all necessary security controls and functionality in place.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Members component implementation and routing # Test 1: Check if Members component exists echo "Checking for Members component..." fd -e tsx -e ts "Members\.(tsx|ts)$" # Test 2: Check router configuration for the members path echo "Checking router configuration..." rg -l "/settings/workspaces/.+/members" # Test 3: Check for access control implementation echo "Checking access control implementation..." rg -l "useAuth|isAuthorized|checkPermission"Length of output: 1186
Script:
#!/bin/bash # Check the Members component implementation echo "Members component implementation:" cat "web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx" # Check for workspace-specific access controls echo -e "\nWorkspace access control checks:" rg -A 5 "workspace.*permission|workspace.*access" --type ts --type tsx # Check for any route protection mechanisms echo -e "\nRoute protection mechanisms:" rg -A 5 "ProtectedRoute|RequireAuth|authGuard" --type ts --type tsxLength of output: 14194
Script:
#!/bin/bash # Check routing and authentication setup echo "Checking route configuration and auth protection:" rg -A 10 "Route.*settings/workspaces|/settings/workspaces" # Check workspace-related hooks and auth echo -e "\nChecking workspace auth hooks:" cat "web/src/services/auth/useAuth.ts" # Check if the workspace route is protected echo -e "\nChecking workspace route protection:" rg -C 5 "workspaces/:workspaceId/members" --type-not testLength of output: 5004
web/src/beta/features/WorkspaceSetting/index.tsx (1)
21-25: Verify handler implementations and type definitions.The new member management handlers look well-organized. Let's verify their implementations and type definitions.
#!/bin/bash # Description: Check handler implementations and type definitions # Check the hook implementation echo "Checking hook implementation:" ast-grep --pattern 'const useWorkspaceHook = () => { $$$ return { $$$ handleAddMemberToWorkspace, handleSearchUser, handleUpdateMemberOfWorkspace, handleRemoveMemberFromWorkspace, $$$ }; }' # Check for TypeScript type definitions echo -e "\nChecking type definitions:" rg -A 2 "type\s+(?:Add|Search|Update|Remove)Member" --type typescriptweb/src/services/api/meApi.ts (3)
5-6: LGTM: Import statements are properly organized.The new imports are correctly added and used within the file.
132-132: LGTM: Export is properly added.The new hook is correctly exported and maintains the alphabetical order in the returned object.
81-104: Verify the integration with workspace settings.Let's ensure the GraphQL query and its usage in workspace settings are properly implemented.
✅ Verification successful
Let me gather more information about the GraphQL query implementation and workspace settings integration.
Based on the gathered information, I can now provide the final response:
Integration with workspace settings is properly implemented
The verification shows:
- The GraphQL query
GET_USER_BY_SEARCHis correctly defined inuser.tswith the required fields (id, name, email)- The
useSearchUserhook is properly integrated inWorkspaceSetting/hooks.tswith:
- Proper error handling
- Skip condition for empty search
- Status handling for the search results
- The hook's return values (
userandstatus) are correctly consumed in the workspace settings implementation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the GraphQL query implementation and its usage # Test 1: Check the GraphQL query definition echo "Checking GraphQL query definition..." rg -l "GET_USER_BY_SEARCH" --type ts # Test 2: Check the usage of useSearchUser hook echo "Checking useSearchUser hook usage..." rg "useSearchUser" --type ts # Test 3: Check for any related test files echo "Checking for related test files..." fd -e test.ts -e test.tsxLength of output: 1848
Script:
#!/bin/bash # Check the GraphQL query definition and its structure echo "Checking GraphQL query definition..." rg -A 5 "GET_USER_BY_SEARCH" web/src/services/gql/queries/user.ts # Check the workspace settings implementation echo "Checking workspace settings implementation..." rg -A 10 "useSearchUser" web/src/beta/features/WorkspaceSetting/hooks.ts # Check if there are any workspace settings related test files echo "Checking workspace settings tests..." fd "WorkspaceSetting" -e test.ts -e test.tsxLength of output: 1364
web/src/beta/features/WorkspaceSetting/hooks.ts (2)
188-190: LGTM!The exports are complete and properly ordered.
6-6: Verify the optionalnameproperty inWorkspacePayload.Making
nameoptional inWorkspacePayloadseems inconsistent with its usage inhandleCreateWorkspacewhere it's required. Consider keeping it required or document why it's optional.✅ Verification successful
Based on the search results, I can now generate a final response as I have enough context to verify the optionality of the
nameproperty inWorkspacePayload.The optional
nameproperty inWorkspacePayloadis correctly implementedThe optionality of the
nameproperty inWorkspacePayloadis justified as this type is used across different contexts:
- In
handleCreateWorkspaceandhandleUpdateWorkspace, the code properly checks for the presence ofnamebefore proceeding:if (name) { const { status } = await useCreateWorkspace(name); // ... }- The same type is reused for member management operations (
handleAddMemberToWorkspace,handleRemoveMemberFromWorkspace) wherenameis not required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of WorkspacePayload to verify the optionality of name rg -A 5 "WorkspacePayload"Length of output: 6369
web/src/beta/lib/reearth-ui/components/Icon/icons.ts (1)
90-90: LGTM: MemberAdd icon additionThe addition of the
MemberAddicon and its export is well-structured and aligns with the workspace member management feature being implemented.Also applies to: 249-249
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (2)
180-181: Ensure consistent typing for 'role'When updating the member's role, ensure that
roleValueis of the correct type and consistent with theRoleenum. This prevents type mismatches and potential runtime errors.Confirm that
roleValueis of typeRoleor cast it appropriately if necessary.
318-323:⚠️ Potential issueHandle potential empty input on 'Enter' key press
When the user presses the 'Enter' key, you should ensure that the trimmed input is not empty before proceeding. This avoids unnecessary state updates.
Apply this diff to fix:
onKeyDown={(e) => { if (e.key === "Enter") { const trimmedInput = memberSearchInput.trim(); + if (trimmedInput === "") return; setMemberSearchInput(""); setDebouncedInput(trimmedInput); } }}Likely invalid or redundant comment.
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1186 +/- ##
==========================================
+ Coverage 13.85% 18.82% +4.97%
==========================================
Files 595 591 -4
Lines 64056 64917 +861
Branches 660 596 -64
==========================================
+ Hits 8873 12220 +3347
+ Misses 55183 52697 -2486
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
web/src/beta/features/WorkspaceSetting/index.tsx (1)
Line range hint
22-58: Consider wrapping with an error boundary.Since this component handles critical workspace operations, consider implementing an error boundary to gracefully handle and report any unexpected errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- web/src/beta/features/WorkspaceSetting/hooks.ts (6 hunks)
- web/src/beta/features/WorkspaceSetting/index.tsx (2 hunks)
- web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
[error] 337-362: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
web/src/beta/features/WorkspaceSetting/index.tsx (2)
17-20: Great implementation of the TABS enum!The centralization of tab values using an enum improves type safety and maintainability.
27-30: Verify error handling in member management operations.Let's ensure proper error handling is implemented for these critical operations.
✅ Verification successful
Based on the results, I can now generate the final response since we have clear evidence of error handling implementation.
Error handling is properly implemented for all member management operations
The verification shows comprehensive error handling across all workspace member operations:
- All operations are wrapped in try-catch blocks
- Errors are properly logged with descriptive messages
- Error states are captured and returned where appropriate
- Consistent error handling pattern is maintained across all member management functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling implementation in workspace hooks # Check for try-catch blocks in member management operations echo "Checking error handling in member management operations:" rg -A 10 "(handleAddMemberToWorkspace|handleUpdateMemberOfWorkspace|handleRemoveMemberFromWorkspace|handleSearchUser)\s*=\s*async" ./web/src/beta/features/WorkspaceSetting/hooks.ts # Check for error state management echo -e "\nChecking error state management:" rg "(?i)(error|catch|try)" ./web/src/beta/features/WorkspaceSetting/hooks.tsLength of output: 1580
web/src/beta/features/WorkspaceSetting/hooks.ts (1)
6-6:⚠️ Potential issueReview type safety for optional name property.
The
nameproperty is marked as optional inWorkspacePayload, but it's required in functions likehandleCreateWorkspace. This mismatch could lead to runtime errors.Consider using separate types for different operations:
type CreateWorkspacePayload = { name: string; // required teamId: string; }; type UpdateWorkspacePayload = { name?: string; // optional teamId: string; };
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
web/src/services/i18n/translations/en.yml (2)
298-298: Improve user search feedback message.The current message "Didn't find the user" is informal and could be more helpful.
Consider this improvement:
-Didn't find the user: '' +No matching users found. Please try a different search term.: ''
Line range hint
299-310: Fix grammar and punctuation consistency in workspace messages.Several workspace-related messages need attention for grammar and punctuation consistency.
Apply these improvements:
-This process will remove this workspace: '' +This process will remove this workspace.: '' -'This action cannot be undone. ': '' +'This action cannot be undone.': '' -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 before continuing.: ''web/src/services/i18n/translations/ja.yml (3)
Line range hint
3-84: Missing translations for core UI elementsA large number of core UI elements have their translations reset to empty strings. This will cause the UI to fall back to English for Japanese users, degrading their experience.
Please provide Japanese translations for these basic UI elements. For reference, some previous translations were:
Name: '名前'Password: 'パスワード'Description: '説明'
Line range hint
299-310: Add Japanese translations for workspace managementWorkspace management strings are missing translations:
Suggested translations based on common Japanese software localizations:
-Workspace: '' -Workspace Name: '' -Remove this workspace: '' -This process will remove this workspace: '' -Remove workspace: '' -Delete workspace: '' -I am sure I want to delete this workspace.: '' -'This action cannot be undone. ': '' -This will permanently delete the workspace and all related projects, assets and datasets for all team members.: '' -Please type your workspace name to continue.: '' -You are going to delete a workspace.: '' -Please to make sure you don't have any projects in your workspace, then you can continue.: '' +Workspace: 'ワークスペース' +Workspace Name: 'ワークスペース名' +Remove this workspace: 'このワークスペースを削除' +This process will remove this workspace: 'このワークスペースを削除します' +Remove workspace: 'ワークスペースを削除' +Delete workspace: 'ワークスペースを削除' +I am sure I want to delete this workspace.: 'このワークスペースを削除することを確認しました' +'This action cannot be undone. ': 'この操作は取り消すことができません。' +This will permanently delete the workspace and all related projects, assets and datasets for all team members.: 'このワークスペースと、全てのチームメンバーに関連するプロジェクト、アセット、データセットが完全に削除されます。' +Please type your workspace name to continue.: '続行するにはワークスペース名を入力してください。' +You are going to delete a workspace.: 'ワークスペースを削除しようとしています。' +Please to make sure you don't have any projects in your workspace, then you can continue.: 'ワークスペースにプロジェクトがないことを確認してから、続行してください。'
Line range hint
455-464: Add Japanese translations for workspace member management messagesSuccess/failure messages for workspace member operations are missing translations:
Suggested translations based on common Japanese software localizations:
-Failed to add member to workspace.: '' -Successfully added member to workspace!: '' -Failed to remove member from workspace.: '' -Successfully removed member from workspace!: '' -Failed to update member in workspace.: '' -Successfully updated member in workspace!: '' +Failed to add member to workspace.: 'ワークスペースへのメンバー追加に失敗しました。' +Successfully added member to workspace!: 'ワークスペースへメンバーを追加しました。' +Failed to remove member from workspace.: 'ワークスペースからのメンバー削除に失敗しました。' +Successfully removed member from workspace!: 'ワークスペースからメンバーを削除しました。' +Failed to update member in workspace.: 'ワークスペースのメンバー情報の更新に失敗しました。' +Successfully updated member in workspace!: 'ワークスペースのメンバー情報を更新しました。'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- web/src/beta/lib/reearth-ui/components/Icon/icons.ts (2 hunks)
- web/src/services/i18n/translations/en.yml (2 hunks)
- web/src/services/i18n/translations/ja.yml (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/lib/reearth-ui/components/Icon/icons.ts
🔇 Additional comments (1)
web/src/services/i18n/translations/ja.yml (1)
Line range hint
1-464: Verify translations with a native Japanese speakerWhile the suggested translations follow common software localization patterns, it's recommended to have them reviewed by a native Japanese speaker to ensure natural language usage and proper context.
✅ Verification successful
Let me try a different approach to verify the translations.
This will help us:
- Find any existing Japanese text in the codebase to compare translation patterns
- Locate all translation files to understand the localization structure
Based on the executed scripts, I can now provide a final response regarding the Japanese translations verification:
Japanese translations appear to be consistent with existing patterns in the codebase
The executed scripts revealed extensive Japanese translations throughout the codebase, particularly in UI elements, system messages, and configuration settings. The translations in the file under review follow the same style and quality as the existing translations, showing:
- Consistent use of Japanese characters (Hiragana, Katakana, and Kanji)
- Professional and natural language usage in system messages
- Proper technical term translations matching the existing patterns (e.g., "プロジェクト" for "Project", "公開" for "Publish")
- Appropriate formality level in user-facing messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing Japanese translations in the codebase that could be reused rg -g '*.yml' -g '*.json' "ワークスペース|メンバー"Length of output: 42
Script:
#!/bin/bash # Check for all Japanese translations in the codebase fd -e yml -e json -x cat {} | grep -E "[ぁ-んァ-ン一-龯]" # Also check specifically for translation files fd "translations|i18n" -e yml -e jsonLength of output: 27752
…o feat/members_setting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (5)
28-59: Consider improving type definitions with JSDoc comments and specific error types.Add JSDoc comments to document the Props interface and use a more specific error type instead of
unknown.+/** + * Props for the Members component + */ type Props = { + /** Adds a new member to the workspace */ handleAddMemberToWorkspace: ({ teamId, userId, role }: WorkspacePayload) => Promise<void>; + /** Searches for a user by name or email */ handleSearchUser: (nameOrEmail: string) => | { searchUser: { __typename?: "User"; id: string; name: string; email: string; } | null; searchUserStatus: string; error?: undefined; } | { - error: unknown; + error: Error; searchUser?: undefined; searchUserStatus?: undefined; }; // ... rest of the props };
125-142: Add cleanup for search results state.Consider adding a cleanup function to reset the search results when the component unmounts or when the search input is cleared.
useEffect(() => { if ( searchUser && "name" in searchUser && !memberSearchResults.find( (memberSearchResult) => memberSearchResult.id === searchUser.id ) ) { setMemberSearchResults([ ...memberSearchResults, { userName: searchUser.name, email: searchUser.email, id: searchUser.id } ]); } + return () => { + setMemberSearchResults([]); + }; }, [memberSearchResults, searchUser]);
160-165: Add ARIA labels for better accessibility.The table headers should have proper ARIA labels for better screen reader support.
-<Table> +<Table role="table" aria-label={t("Workspace members")}> - <TableHeader>{t("User Name")}</TableHeader> + <TableHeader role="columnheader">{t("User Name")}</TableHeader> - <TableHeader>{t("Email")}</TableHeader> + <TableHeader role="columnheader">{t("Email")}</TableHeader> - <TableHeader>{t("Role")}</TableHeader> + <TableHeader role="columnheader">{t("Role")}</TableHeader> - <TableHeader /> + <TableHeader role="columnheader" aria-label={t("Actions")} />
314-326: Add loading state feedback for search input.The search input should provide visual feedback when the search is in progress.
<TextInput placeholder="[email protected]" value={memberSearchInput} + loading={searchUserStatus === "loading"} onChange={(input) => { setMemberSearchInput(input); }} onKeyDown={(e) => { if (e.key === "Enter" && memberSearchInput.trim() !== "") { setMemberSearchInput(""); setDebouncedInput(memberSearchInput.trim()); } }} />
436-442: Use theme variables for DeleteIcon colors and add hover transition.Replace hardcoded colors with theme variables and add a smooth transition for the hover effect.
-const DeleteIcon = styled(IconButton)(() => ({ +const DeleteIcon = styled(IconButton)(({ theme }) => ({ cursor: "pointer", - color: "#fff", + color: theme.content.main, + transition: "color 0.2s ease-in-out", "&:hover": { - color: "red" + color: theme.danger.main } }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (1 hunks)
- web/src/services/state/index.ts (2 hunks)
🧰 Additional context used
🪛 Biome
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
[error] 339-364: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
web/src/services/state/index.ts (2)
4-4: LGTM: Clean type import additionThe TeamMember type import is well-placed and properly supports the type enhancement in the Workspace interface.
54-54: Type enhancement improves safety, verify usageThe change from
any[]toTeamMember[]improves type safety and reduces potential runtime errors. However, as this is a breaking change, we should verify all usages of workspace.members have been updated accordingly.Let's verify all usages of workspace.members:
✅ Verification successful
Let me try a different approach to search for usages of the members property.
This should help us find:
- All occurrences of "members" in TypeScript files with context
- The Workspace interface definition to understand the full context
- By analyzing the context of these results, we can determine if there are any compatibility issues with the type change
Let me try one more time with a different search strategy.
This should help us:
- Confirm we can find TypeScript files
- Find occurrences of "members" with context
- Find usage of the Workspace type to understand the impact
Based on the search results, I can see that the
membersproperty is extensively used throughout the codebase in the context of workspaces/teams. The type change fromany[]toTeamMember[]is properly handled as:
- The GraphQL schema already defines members with proper typing (TeamMember type with user, userId, and role fields)
- All usages of workspace.members in components like Members, WorkspaceSettings, etc. expect and handle the TeamMember type
- The mutations (AddMemberToWorkspace, RemoveMemberFromWorkspace, etc.) all work with the properly typed TeamMember structure
- The state management and hooks are consistently using the TeamMember type
Type enhancement is safe and properly integrated
The change from
any[]toTeamMember[]aligns with the existing codebase implementation and improves type safety without breaking existing functionality, as all consumers are already working with the TeamMember structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of workspace.members to ensure compatibility with the new type # Search for direct member access patterns echo "Checking direct workspace.members access:" rg -l "workspace\.members" --type ts --type tsx # Search for destructured member access patterns echo -e "\nChecking destructured members access:" rg -l "const.*\{.*members.*\}.*=.*workspace" --type ts --type tsxLength of output: 357
Script:
#!/bin/bash # Search for any mentions of members in TypeScript files echo "Searching for workspace members usage:" rg "members" --type typescript -C 2 # Search for Workspace interface usage echo -e "\nSearching for Workspace interface usage:" ast-grep --pattern 'interface Workspace { $$$ }'Length of output: 307
Script:
#!/bin/bash # Search for files with .ts extension first echo "TypeScript files in the repository:" fd ".*\.ts$" # Then search for members usage in those files echo -e "\nSearching for members usage:" rg "members" -t ts -C 2 # Look for Workspace type usage echo -e "\nSearching for Workspace type usage:" rg "Workspace" -t ts -C 2Length of output: 258609
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
[error] 340-365: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const DeleteIcon = styled(IconButton)(() => ({ | ||
| cursor: "pointer", | ||
| color: "#fff", | ||
| "&:hover": { | ||
| color: "red" | ||
| } | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme colors for DeleteIcon
The DeleteIcon uses hardcoded color values instead of theme colors.
-const DeleteIcon = styled(IconButton)(() => ({
+const DeleteIcon = styled(IconButton)(({ theme }) => ({
cursor: "pointer",
- color: "#fff",
+ color: theme.content.main,
"&:hover": {
- color: "red"
+ color: theme.danger.main
}
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DeleteIcon = styled(IconButton)(() => ({ | |
| cursor: "pointer", | |
| color: "#fff", | |
| "&:hover": { | |
| color: "red" | |
| } | |
| })); | |
| const DeleteIcon = styled(IconButton)(({ theme }) => ({ | |
| cursor: "pointer", | |
| color: theme.content.main, | |
| "&:hover": { | |
| color: theme.danger.main | |
| } | |
| })); |
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
Outdated
Show resolved
Hide resolved
| disabled={ | ||
| workspaceMembers?.some( | ||
| //existed user || no searchResult | ||
| (member) => | ||
| memberSearchResults.find( | ||
| (memberSearchResult) => | ||
| memberSearchResult.email === member.email | ||
| ) | ||
| ) || memberSearchResults.length === 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify disabled state logic
The disabled state logic for the Add button could be simplified and made more readable.
disabled={
- workspaceMembers?.some(
- (member) =>
- memberSearchResults.find(
- (memberSearchResult) =>
- memberSearchResult.email === member.email
- )
- ) || memberSearchResults.length === 0
+ memberSearchResults.length === 0 ||
+ memberSearchResults.some(result =>
+ workspaceMembers?.some(member => member.email === result.email)
+ )
}📝 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.
| disabled={ | |
| workspaceMembers?.some( | |
| //existed user || no searchResult | |
| (member) => | |
| memberSearchResults.find( | |
| (memberSearchResult) => | |
| memberSearchResult.email === member.email | |
| ) | |
| ) || memberSearchResults.length === 0 | |
| } | |
| disabled={ | |
| memberSearchResults.length === 0 || | |
| memberSearchResults.some(result => | |
| workspaceMembers?.some(member => member.email === result.email) | |
| ) | |
| } |
| if (status === "success") { | ||
| console.log("Workspace created successfully"); | ||
| } |
There was a problem hiding this comment.
We don't need this console.log here, even when we need one we should handle it on api code.
| if (status === "success") { | ||
| console.log("Workspace updated successfully"); | ||
| } |
| if (status === "success") { | ||
| console.log("Workspace deleted successfully"); | ||
| } |
| try { | ||
| if (userId && role) { | ||
| const { status } = await useUpdateMemberOfWorkspace( | ||
| teamId, | ||
| userId, | ||
| role | ||
| ); | ||
| if (status === "success") { | ||
| console.log("Member update successfully"); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to update member from workspace:", error); | ||
| } |
There was a problem hiding this comment.
Same as above, i think api hooks already handled the errors and notifications, we don't need to to these again where we use the api hooks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
web/src/beta/features/Dashboard/ContentsContainer/Members/ListItem.tsx (1)
Type compatibility verified with potential issue found
The type verification reveals a potential issue with the email field access. In the User type definition,
email?: string), which matches with the component's safe optional chaining usage (member.user?.email). However, there's a risk of displaying empty content when email is undefined.
- Consider adding a fallback value or conditional rendering when
member.user?.emailis undefined- All other accessed properties are correctly defined:
roleis a required field in TeamMemberuseris optional in TeamMember (correctly accessed with?.)nameis a required string in User type🔗 Analysis chain
Line range hint
6-31: Verify type compatibility with TeamMember interfaceThe component implementation looks good, but let's verify that all accessed properties are defined in the TeamMember interface.
Let me verify the User type definition to ensure it has the required fields (name and email) that are being accessed in the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the TeamMember interface contains all required fields # Expected fields: user.name, user.email, role # Search for TeamMember type definition ast-grep --pattern 'type TeamMember = { $$$ }' # Alternative: Search for TeamMember interface definition ast-grep --pattern 'interface TeamMember { $$$ }'Length of output: 504
Script:
#!/bin/bash # Search for User type definition to verify name and email fields ast-grep --pattern 'type User = { $$$ }' # Also search for Role type/enum since it's used in TeamMember ast-grep --pattern 'enum Role { $$$ }'Length of output: 754
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (4)
28-59: Improve type safety and readability of Props interfaceConsider these improvements to the Props interface:
- Type the
roleparameter explicitly asRoleenum- Consider inlining the
WorkspacePayloadtype for better readabilitytype Props = { handleAddMemberToWorkspace: ({ teamId, userId, - role + role: Role, + name?: string }: WorkspacePayload) => Promise<void>; // ... other handlers };
61-66: Improve MembersData type definitionThe type could be more specific and clearer:
-type MembersData = { +interface Member { + id: string; + role: Role; + username: string; // Make required + email: string; // Make required +} + +type MembersData = Member[]; -}[];
161-166: Improve table accessibilityThe table structure needs proper ARIA roles and labels for better accessibility.
-<Table> +<Table role="table" aria-label={t("Workspace members")}> - <TableHeader>{t("User Name")}</TableHeader> + <TableHeader role="columnheader">{t("User Name")}</TableHeader> - <TableHeader>{t("Email")}</TableHeader> + <TableHeader role="columnheader">{t("Email")}</TableHeader> - <TableHeader>{t("Role")}</TableHeader> + <TableHeader role="columnheader">{t("Role")}</TableHeader> - <TableHeader /> + <TableHeader role="columnheader" aria-label={t("Actions")} />
373-380: Add reduced motion support for animationsConsider users who prefer reduced motion for accessibility reasons.
+const reducedMotion = keyframes` + from { + opacity: 0; + } + to { + opacity: 1; + } +`; + const zoomIn = keyframes` from { opacity: 0; + transform: scale(0.95); } to { opacity: 1; + transform: scale(1); } `; + +const getAnimation = () => css` + @media (prefers-reduced-motion: no-preference) { + animation: ${zoomIn} 0.2s ease-in-out; + } + @media (prefers-reduced-motion: reduce) { + animation: ${reducedMotion} 0.2s ease-in-out; + } +`;Then update the components using the animation:
const TableRow = styled("div")(({ theme }) => ({ // ...other styles - animation: `${zoomIn} 0.2s ease-in-out` + ...getAnimation() })); const UserInfo = styled.div` // ...other styles - animation: ${zoomIn} 0.2s ease-in-out; + ${getAnimation()}; `;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/beta/features/Dashboard/ContentsContainer/Members/ListItem.tsx (1 hunks)
- web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
web/src/beta/features/WorkspaceSetting/innerPages/Members/Members.tsx
[error] 340-365: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
web/src/beta/features/Dashboard/ContentsContainer/Members/ListItem.tsx (2)
2-2: LGTM: Proper typing from GraphQL schemaThe import of
TeamMemberfrom the GraphQL services ensures type safety and consistency with the backend schema.
Line range hint
33-62: LGTM: Well-structured styled componentsThe styled components are well-organized and follow good practices:
- Proper use of theme variables
- Semantic naming
- Responsive grid layout
- Appropriate handling of text overflow
| useEffect(() => { | ||
| if ( | ||
| searchUser && | ||
| "name" in searchUser && | ||
| !memberSearchResults.find( | ||
| (memberSearchResult) => memberSearchResult.id === searchUser.id | ||
| ) | ||
| ) { | ||
| setMemberSearchResults([ | ||
| ...memberSearchResults, | ||
| { | ||
| userName: searchUser.name, | ||
| email: searchUser.email, | ||
| id: searchUser.id | ||
| } | ||
| ]); | ||
| } | ||
| }, [memberSearchResults, searchUser]); |
There was a problem hiding this comment.
Fix potential infinite loop in useEffect
The effect has memberSearchResults in its dependency array while also updating it, which could cause an infinite loop. Additionally, the search results update logic could be optimized.
useEffect(() => {
if (
searchUser &&
"name" in searchUser &&
- !memberSearchResults.find(
+ !memberSearchResults.some(
(memberSearchResult) => memberSearchResult.id === searchUser.id
)
) {
setMemberSearchResults((prevResults) => [
- ...memberSearchResults,
+ ...prevResults,
{
userName: searchUser.name,
email: searchUser.email,
id: searchUser.id
}
]);
}
-}, [memberSearchResults, searchUser]);
+}, [searchUser]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if ( | |
| searchUser && | |
| "name" in searchUser && | |
| !memberSearchResults.find( | |
| (memberSearchResult) => memberSearchResult.id === searchUser.id | |
| ) | |
| ) { | |
| setMemberSearchResults([ | |
| ...memberSearchResults, | |
| { | |
| userName: searchUser.name, | |
| email: searchUser.email, | |
| id: searchUser.id | |
| } | |
| ]); | |
| } | |
| }, [memberSearchResults, searchUser]); | |
| useEffect(() => { | |
| if ( | |
| searchUser && | |
| "name" in searchUser && | |
| !memberSearchResults.some( | |
| (memberSearchResult) => memberSearchResult.id === searchUser.id | |
| ) | |
| ) { | |
| setMemberSearchResults((prevResults) => [ | |
| ...prevResults, | |
| { | |
| userName: searchUser.name, | |
| email: searchUser.email, | |
| id: searchUser.id | |
| } | |
| ]); | |
| } | |
| }, [searchUser]); |
Overview
What I've done
Add member's page for workspace.
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Memberscomponent for managing workspace members.Bug Fixes
Documentation
Chores