feat: support user search #1285
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces significant modifications to the user-related GraphQL functionalities. Key changes include renaming and restructuring existing test functions and GraphQL query methods to enhance clarity and specificity. A new search functionality has been added, allowing users to be retrieved by name or email or via a keyword search. The GraphQL schema and resolver implementations are updated accordingly, reflecting these changes, while the 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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
server/schemas/user.graphql (1)
41-42: Add query descriptions and consider paginationThe new search queries lack descriptions explaining their behavior. Additionally,
userSearchreturns an unbounded array which could cause performance issues with large datasets.Consider adding descriptions and implementing pagination:
extend type Query { me: Me - userSearch(keyword: String!): [User!]! - userByNameOrEmail(nameOrEmail: String!): User + """ + Search users by keyword matching against name and email + """ + userSearch( + keyword: String! + first: Int + after: String + ): UserConnection! + """ + Find a single user by exact name or email match + """ + userByNameOrEmail(nameOrEmail: String!): User } +""" +Connection type for paginated user results +""" +type UserConnection { + edges: [UserEdge!]! + pageInfo: PageInfo! + totalCount: Int! +} + +type UserEdge { + node: User! + cursor: String! +}This change:
- Adds clear descriptions of query behavior
- Implements cursor-based pagination for scalability
- Provides total count for UI features like "showing X of Y results"
server/internal/adapter/gql/loader_user.go (2)
44-52: Consider adding input validation for nameOrEmail parameter.The method implementation looks good and the rename makes the purpose clearer. However, consider adding validation to ensure
nameOrEmailis not empty or contains valid characters.func (c *UserLoader) ByNameOrEmail(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) { + if strings.TrimSpace(nameOrEmail) == "" { + return nil, errors.New("nameOrEmail cannot be empty") + } res, err := c.usecase.FetchByNameOrEmail(ctx, nameOrEmail)
53-62: Consider adding pagination and input validation.The implementation is clean and effectively uses
lo.Mapfor type conversion. However, consider these improvements:
- Add pagination to handle potentially large result sets efficiently
- Add input validation for the search parameter
- Consider adding a minimum search term length requirement to prevent overly broad searches
Consider implementing cursor-based pagination:
-func (c *UserLoader) Search(ctx context.Context, nameOrEmail string) ([]*gqlmodel.User, error) { +func (c *UserLoader) Search(ctx context.Context, nameOrEmail string, first int, after *string) (*gqlmodel.UserConnection, error) { + if strings.TrimSpace(nameOrEmail) == "" { + return nil, errors.New("search term cannot be empty") + } + if len(nameOrEmail) < 3 { + return nil, errors.New("search term must be at least 3 characters") + } res, err := c.usecase.SearchUser(ctx, nameOrEmail) if err != nil { return nil, err } - return lo.Map(res, func(u *user.Simple, _ int) *gqlmodel.User { + users := lo.Map(res, func(u *user.Simple, _ int) *gqlmodel.User { return gqlmodel.SimpleToUser(u) - }), nil + }) + return gqlmodel.NewUserConnection(users, first, after), nil }server/e2e/gql_user_test.go (2)
Line range hint
210-243: Consider adding more test cases for comprehensive coverage.While the current test cases cover basic name lookup and not found scenarios, consider adding:
- Email-based lookup test case
- Invalid input validation (empty string, special characters)
- Error cases (unauthorized access)
Would you like me to help generate these additional test cases?
Line range hint
210-300: Add test documentation for better maintainability.Consider adding documentation comments to describe:
- The purpose and scope of each test function
- The test data setup requirements
- The expected behavior for each test case
Example documentation format:
// TestUserByNameOrEmail verifies the userByNameOrEmail GraphQL query // by testing user lookup by name or email. // // Test cases: // - Successful lookup by name // - User not foundserver/internal/adapter/gql/generated.go (4)
561-562: Add documentation comments for new methodsThe new methods
UserByNameOrEmailandUserSearchin theComplexityRootstruct lack documentation comments. Adding comments will improve code maintainability and readability.
1026-1027: Add documentation comments for new interface methodsThe methods
UserSearchandUserByNameOrEmailin theQueryResolverinterface should include documentation comments to explain their purpose and usage.
6627-6628: Add descriptions to new GraphQL query fieldsThe new query fields
userSearchanduserByNameOrEmailin the GraphQL schema lack descriptions. Adding GraphQL schema descriptions enhances API documentation.
Line range hint
45851-45857: Consistent error recovery in 'userByNameOrEmail' resolverEnsure that the error recovery mechanism in the
userByNameOrEmailresolver matches the patterns used elsewhere to maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
- server/e2e/gql_user_test.go (3 hunks)
- server/go.mod (1 hunks)
- server/internal/adapter/gql/generated.go (12 hunks)
- server/internal/adapter/gql/loader_user.go (1 hunks)
- server/internal/adapter/gql/resolver_user.go (1 hunks)
- server/schemas/user.graphql (1 hunks)
🔇 Additional comments (9)
server/schemas/user.graphql (1)
Line range hint
8-19: Add field descriptions and verify non-null constraintsThe new fields
myWorkspaceIdandauthslack descriptions explaining their purpose. Additionally, both fields are marked as non-nullable which may cause issues with existing data.Consider adding descriptions and verifying the constraints:
type Me { id: ID! name: String! email: String! lang: Lang! theme: Theme! host: String - myWorkspaceId: ID! - auths: [String!]! + """ + The ID of the user's primary workspace + """ + myWorkspaceId: ID! + """ + List of authentication methods enabled for this user + """ + auths: [String!]! workspaces: [Workspace!]! myWorkspace: Workspace integrations: [Integration!]! }Let's verify if all users have workspaces:
✅ Verification successful
Let's gather more specific information about workspace handling in the codebase.
Let's check one more aspect focusing on user creation and initialization:
Based on the evidence gathered, I can now generate the final response:
Non-null constraints for
myWorkspaceIdandauthsare safe to implementThe codebase shows that:
- Workspace ID is required during user signup (server/e2e/http_user_test.go)
- User-workspace relationship is a core part of the system's design (CreateUserInput in http/user.go includes WorkspaceID)
- Authentication is handled through the auth_client, ensuring users always have at least one auth method
Consider adding descriptions to improve schema documentation:
""" The ID of the user's primary workspace. Always present as workspace is required during signup. """ myWorkspaceId: ID! """ List of authentication methods enabled for this user. Always contains at least one auth method. """ auths: [String!]!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any existing users without workspaces # Note: This helps validate if the non-null constraint is safe to add # Search for user-related database queries or models rg -l "workspace.*user|user.*workspace" --type go # Search for workspace assignment logic ast-grep --pattern 'func $$$($$$) { $$$ workspace$$$ $$$ }'Length of output: 314
Script:
#!/bin/bash # Check workspace-related code in auth_client and resolver rg -A 10 "workspace.*user|user.*workspace" server/internal/app/auth_client.go server/internal/adapter/gql/resolver_workspace.go # Look for workspace creation/assignment logic ast-grep --pattern 'func $$$($$$) { $$$ createWorkspace$$$ $$$ }' # Check if there's any default/fallback workspace logic rg -A 5 "default.*workspace|fallback.*workspace" --type goLength of output: 1971
Script:
#!/bin/bash # Look for user creation/initialization logic rg -A 10 "func.*CreateUser|func.*InitializeUser|func.*SignUp" --type go # Check for workspace initialization during user creation ast-grep --pattern 'type User struct { $$$ }' # Look for user-workspace relationship in models/entities rg -A 5 "type.*User" --type goLength of output: 15096
server/internal/adapter/gql/resolver_user.go (2)
91-93:⚠️ Potential issueEnhance input validation and error handling
The current implementation could expose user enumeration vulnerabilities. Consider the following improvements:
- Validate email format when the input appears to be an email
- Return consistent error messages regardless of whether the user exists
- Add rate limiting to prevent brute force attempts
Let's check the loader implementation for existing validation:
#!/bin/bash # Search for the ByNameOrEmail implementation in the loader ast-grep --pattern 'type UserLoader struct { $$$ ByNameOrEmail(ctx context.Context, nameOrEmail string) $$ { $$$ } $$$ }'Consider implementing:
func (r *queryResolver) UserByNameOrEmail(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) { + if strings.Contains(nameOrEmail, "@") { + if err := validateEmail(nameOrEmail); err != nil { + return nil, gqlmodel.NewError(err) + } + } return loaders(ctx).User.ByNameOrEmail(ctx, nameOrEmail) }
86-89: Consider adding input validation and paginationThe implementation could benefit from several improvements:
- Add input validation for the keyword parameter to prevent DoS attacks via extremely long search terms
- Implement pagination to handle large result sets efficiently
- Consider adding rate limiting to prevent abuse
Let's check if pagination is implemented in the loader:
Consider implementing:
- Pagination using cursor-based or offset/limit approach
- Input validation:
if len(keyword) > MaxKeywordLength { return nil, ErrKeywordTooLong }- Rate limiting middleware for search endpoints
server/go.mod (1)
30-31: LGTM! Verify the reearthx dependency update.The update of
github.com/reearth/reearthxto the newer version looks good and aligns with the PR's user search feature implementation.Let's verify the changes in the reearthx dependency:
✅ Verification successful
Based on the search results, I can see that the reearthx dependency is heavily used throughout the codebase, particularly for user-related functionality through the
account/accountdomain/userpackage. The imports show it's used consistently in tests and implementation code for user authentication, workspace management, and GraphQL resolvers.The dependency update appears safe to proceed.
- The update is a minor version bump (two days difference in pseudo-versions)
- The imports show the dependency is used in a stable way across the codebase
- The user-related imports are primarily from the
accountpackage, which aligns with the PR's user search feature- No breaking changes are evident as the imports follow a consistent pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in the reearthx dependency between the old and new versions # Get the commit messages between the two versions to understand the changes git clone https://github.com/reearth/reearthx.git /tmp/reearthx cd /tmp/reearthx git log --oneline v0.0.0-20241023075926-e29bdd6c4ae3..v0.0.0-20241025125329-f01a05daf443 # Check if there are any changes to user-related functionality git diff v0.0.0-20241023075926-e29bdd6c4ae3..v0.0.0-20241025125329-f01a05daf443 -- "*user*"Length of output: 707
Script:
#!/bin/bash # Let's try a different approach to check the dependency changes # First, let's check if there are any references to reearthx in the codebase rg -l "reearthx" --type go # Then, let's look for any user-related imports or usage from reearthx rg "reearthx.*user" --type go -A 2 # Also check for any breaking changes in the go.sum cat server/go.sum | grep "github.com/reearth/reearthx" | sortLength of output: 24705
server/internal/adapter/gql/generated.go (5)
9688-9698: Ensure argument parsing handles all casesIn the function
field_Query_userByNameOrEmail_args, verify that all possible argument scenarios are handled correctly, including error cases.
9720-9750: Proper handling of required 'keyword' argumentThe
field_Query_userSearch_argsfunction should ensure that thekeywordargument is properly validated, as it is a required field. The current implementation seems to handle it correctly.
Line range hint
25286-25313: Handle null results in 'UserByNameOrEmail' resolverIn the
_Query_userByNameOrEmailresolver, ensure that the function correctly handles cases where a user is not found and returnsnullas per GraphQL specifications.
45820-45842: Ensure proper error handling in concurrent executionIn the concurrent execution logic for
userSearch, make sure that errors are properly captured and returned to prevent panics during field resolution.
3485-3507:⚠️ Potential issueSyntax error: Incorrect colon in if statement
At line 3498, there is a colon
:instead of an opening brace{in theifstatement, which will result in a syntax error.Apply this diff to fix the syntax error:
- if e.complexity.Query.UserSearch == nil: + if e.complexity.Query.UserSearch == nil {Likely invalid or redundant comment.
| func TestUserSearch(t *testing.T) { | ||
| e := StartServer(t, &app.Config{}, true, baseSeederUser) | ||
| query := fmt.Sprintf(` { userSearch(keyword: "%s"){ id name email } }`, "e2e") | ||
| request := GraphQLRequest{ | ||
| Query: query, | ||
| } | ||
| jsonData, err := json.Marshal(request) | ||
| if err != nil { | ||
| assert.NoError(t, err) | ||
| } | ||
| res := e.POST("/api/graphql"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithHeader("X-Reearth-Debug-User", uId1.String()). | ||
| WithBytes(jsonData).Expect().Status(http.StatusOK).JSON().Object() | ||
| ul := res.Value("data").Object().Value("userSearch").Array() | ||
| ul.Length().IsEqual(4) | ||
| o := ul.Value(0).Object() | ||
| o.Value("id").String().IsEqual(uId1.String()) | ||
| o.Value("name").String().IsEqual("e2e") | ||
| o.Value("email").String().IsEqual("[email protected]") | ||
|
|
||
| query = fmt.Sprintf(` { userSearch(keyword: "%s"){ id name email } }`, "e2e2") | ||
| request = GraphQLRequest{ | ||
| Query: query, | ||
| } | ||
| jsonData, err = json.Marshal(request) | ||
| if err != nil { | ||
| assert.NoError(t, err) | ||
| } | ||
| res = e.POST("/api/graphql"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithHeader("X-Reearth-Debug-User", uId1.String()). | ||
| WithBytes(jsonData).Expect().Status(http.StatusOK).JSON().Object() | ||
| ul = res.Value("data").Object().Value("userSearch").Array() | ||
| ul.Length().IsEqual(1) | ||
| o = ul.Value(0).Object() | ||
| o.Value("id").String().IsEqual(uId2.String()) | ||
| o.Value("name").String().IsEqual("e2e2") | ||
| o.Value("email").String().IsEqual("[email protected]") | ||
|
|
||
| query = fmt.Sprintf(` { userSearch(keyword: "%s"){ id name email } }`, "notfound") | ||
| request = GraphQLRequest{ | ||
| Query: query, | ||
| } | ||
| jsonData, err = json.Marshal(request) | ||
| if err != nil { | ||
| assert.NoError(t, err) | ||
| } | ||
| e.POST("/api/graphql"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithHeader("X-Reearth-Debug-User", uId1.String()). | ||
| WithBytes(jsonData).Expect().Status(http.StatusOK).JSON().Object(). | ||
| Value("data").Object().Value("userSearch").Array().IsEmpty() |
There was a problem hiding this comment.
Improve test reliability and coverage.
The current implementation has several areas that could be improved:
-
The test is tightly coupled to specific test data:
- Assumes exactly 4 users match "e2e"
- Assumes specific ordering of results
-
Missing test cases:
- Pagination parameters
- Case sensitivity
- Special characters in search
- Minimum keyword length validation
Consider refactoring the test to be more resilient to data changes:
- ul.Length().IsEqual(4)
+ ul.Length().Gt(0) // Assert some results existAlso, consider adding validation for result ordering or explicitly documenting if the order is undefined.
Would you like me to help implement the additional test cases and make the tests more resilient?
Committable suggestion was skipped due to low confidence.
| func (ec *executionContext) _Query_userSearch(ctx context.Context, field graphql.CollectedField) (ret graphql.Marshaler) { | ||
| fc, err := ec.fieldContext_Query_userSearch(ctx, field) | ||
| if err != nil { | ||
| return graphql.Null | ||
| } | ||
| ctx = graphql.WithFieldContext(ctx, fc) | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| ec.Error(ctx, ec.Recover(ctx, r)) | ||
| ret = graphql.Null | ||
| } | ||
| }() | ||
| resTmp, err := ec.ResolverMiddleware(ctx, func(rctx context.Context) (interface{}, error) { | ||
| ctx = rctx // use context from middleware stack in children | ||
| return ec.resolvers.Query().UserSearch(rctx, fc.Args["keyword"].(string)) | ||
| }) | ||
| if err != nil { | ||
| ec.Error(ctx, err) | ||
| return graphql.Null | ||
| } | ||
| if resTmp == nil { | ||
| if !graphql.HasFieldError(ctx, fc) { | ||
| ec.Errorf(ctx, "must not be null") | ||
| } | ||
| return graphql.Null | ||
| } | ||
| res := resTmp.([]*gqlmodel.User) | ||
| fc.Result = res | ||
| return ec.marshalNUser2ᚕᚖgithubᚗcomᚋreearthᚋreearthᚑcmsᚋserverᚋinternalᚋadapterᚋgqlᚋgqlmodelᚐUserᚄ(ctx, field.Selections, res) | ||
| } | ||
|
|
||
| func (ec *executionContext) fieldContext_Query_userSearch(ctx context.Context, field graphql.CollectedField) (fc *graphql.FieldContext, err error) { | ||
| fc = &graphql.FieldContext{ | ||
| Object: "Query", | ||
| Field: field, | ||
| IsMethod: true, | ||
| IsResolver: true, | ||
| Child: func(ctx context.Context, field graphql.CollectedField) (*graphql.FieldContext, error) { | ||
| switch field.Name { | ||
| case "id": | ||
| return ec.fieldContext_User_id(ctx, field) | ||
| case "name": | ||
| return ec.fieldContext_User_name(ctx, field) | ||
| case "email": | ||
| return ec.fieldContext_User_email(ctx, field) | ||
| case "host": | ||
| return ec.fieldContext_User_host(ctx, field) | ||
| } | ||
| return nil, fmt.Errorf("no field named %q was found under type User", field.Name) | ||
| }, | ||
| } | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = ec.Recover(ctx, r) | ||
| ec.Error(ctx, err) | ||
| } | ||
| }() | ||
| ctx = graphql.WithFieldContext(ctx, fc) | ||
| if fc.Args, err = ec.field_Query_userSearch_args(ctx, field.ArgumentMap(ec.Variables)); err != nil { | ||
| ec.Error(ctx, err) | ||
| return fc, err | ||
| } | ||
| return fc, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement pagination for 'UserSearch' results
Returning all users in UserSearch without pagination could lead to performance issues with large user datasets.
Consider adding pagination parameters to the userSearch query to handle large datasets efficiently.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
- server/go.mod (1 hunks)
- server/internal/adapter/gql/resolver_user.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/go.mod
🔇 Additional comments (1)
server/internal/adapter/gql/resolver_user.go (1)
86-93: Implementation aligns well with PR objectives.The code successfully implements user search functionality with two complementary methods:
UserSearchfor general keyword-based searchUserByNameOrEmailfor specific user lookupThe separation of concerns between these methods is clean and logical.
Let's verify the GraphQL schema matches these resolvers:
✅ Verification successful
Implementation aligns well with GraphQL schema and PR objectives
The GraphQL schema in
server/schemas/user.graphqlperfectly matches the resolver implementation:extend type Query { userSearch(keyword: String!): [User!]! userByNameOrEmail(nameOrEmail: String!): User }Both the resolver methods and their corresponding schema definitions are consistent in terms of:
- Method names
- Parameter types
- Return types
- Nullability constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the GraphQL schema defines these query fields rg -A 2 'type Query' server/internal/adapter/gql/schema/*.graphqlLength of output: 154
Script:
#!/bin/bash # Let's first find the GraphQL schema files fd -e graphql -e gql # Then check their content for Query type definitions rg -A 5 'type Query'Length of output: 19309
| // UserSearch is the resolver for the userSearch field. | ||
| func (r *queryResolver) UserSearch(ctx context.Context, keyword string) ([]*gqlmodel.User, error) { | ||
| return loaders(ctx).User.Search(ctx, keyword) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation and pagination.
While the implementation is clean, consider these improvements for production readiness:
- Add input validation for the keyword parameter (e.g., minimum length)
- Implement pagination to handle large result sets efficiently
Example implementation with validation:
func (r *queryResolver) UserSearch(ctx context.Context, keyword string) ([]*gqlmodel.User, error) {
+ if len(strings.TrimSpace(keyword)) < 2 {
+ return nil, errors.New("search keyword must be at least 2 characters")
+ }
return loaders(ctx).User.Search(ctx, keyword)
}Committable suggestion was skipped due to low confidence.
| // UserByNameOrEmail is the resolver for the userByNameOrEmail field. | ||
| func (r *queryResolver) UserByNameOrEmail(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) { | ||
| return loaders(ctx).User.ByNameOrEmail(ctx, nameOrEmail) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and clarify search behavior.
The implementation could benefit from:
- Input validation for the nameOrEmail parameter
- Documentation clarifying the search priority (name vs. email) and matching behavior
Example implementation with validation and documentation:
+// UserByNameOrEmail searches for a user by their name or email address.
+// The search is case-insensitive and returns the first matching user.
func (r *queryResolver) UserByNameOrEmail(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) {
+ if nameOrEmail = strings.TrimSpace(nameOrEmail); nameOrEmail == "" {
+ return nil, errors.New("name or email parameter cannot be empty")
+ }
return loaders(ctx).User.ByNameOrEmail(ctx, nameOrEmail)
}📝 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.
| // UserByNameOrEmail is the resolver for the userByNameOrEmail field. | |
| func (r *queryResolver) UserByNameOrEmail(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) { | |
| return loaders(ctx).User.ByNameOrEmail(ctx, nameOrEmail) | |
| // UserByNameOrEmail searches for a user by their name or email address. | |
| // The search is case-insensitive and returns the first matching user. | |
| func (r *queryResolver) UserByNameOrEmail(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) { | |
| if nameOrEmail = strings.TrimSpace(nameOrEmail); nameOrEmail == "" { | |
| return nil, errors.New("name or email parameter cannot be empty") | |
| } | |
| return loaders(ctx).User.ByNameOrEmail(ctx, nameOrEmail) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (8)
web/src/gql/queries/user.ts (1)
Line range hint
15-21: Consider adding pagination and improving naming clarity.While the query implementation is functional, consider these improvements:
- Add pagination to handle large result sets efficiently
- Consider renaming to
GET_USERS_BY_KEYWORDfor better clarity- Consider adding a limit parameter to control result size
Here's a suggested implementation:
export const GET_USERS = gql` - query GetUsers($keyword: String!) { - userSearch(keyword: $keyword) { + query GetUsersByKeyword($keyword: String!, $limit: Int, $offset: Int) { + userSearch(keyword: $keyword, limit: $limit, offset: $offset) { id name email + cursor + } + pageInfo { + hasNextPage + totalCount + } } `;web/src/components/organisms/Settings/Members/hooks.ts (1)
258-264: Consider adding a TypeScript interface for the hook return type.The hook returns a large object with many properties. Adding a TypeScript interface would improve maintainability and provide better type safety.
interface UseSettingsMembersReturn { searchedUsers: User[]; selectedUsers: User[]; handleUserSearch: (keyword: string) => Promise<void>; searchLoading: boolean; handleUserAdd: (user: User) => void; // ... other properties }web/src/i18n/translations/en.yml (2)
287-288: Standardize template string formatThe template strings use a different syntax compared to other messages. Consider using a consistent format across all translations.
-Are you sure you want to delete this model: Are you sure you want to delete this model <strong>{{name}}</strong>? +Are you sure you want to delete model "{{name}}"?: '' -Are you sure you want to delete this group: Are you sure you want to delete this group <strong>{{name}}</strong>? +Are you sure you want to delete group "{{name}}"?: ''
1-559: Consider organizing translations into logical sectionsThe translation file would benefit from a more structured organization. Consider:
- Grouping related translations using YAML sections
- Adding comments to separate different functional areas
- Using nested keys for better maintainability
Example structure:
# Common UI elements common: loading: 'Loading...' error: 'Oops!' # User management users: search: title: 'Search Users' no_results: 'No Results Found'This organization would make the file more maintainable and easier to update as the application grows.
web/src/i18n/translations/ja.yml (1)
395-395: Untranslated technical termsSeveral technical terms are left untranslated, which might affect consistency in the Japanese UI:
- "example" (line 395)
- "WRITER", "READER", "MAINTAINER", "OWNER" (lines 422-425) are in all caps English
Consider translating these terms consistently:
-example: '' +example: '例' -WRITER: 編集者 +writer: 編集者 -READER: 閲覧者 +reader: 閲覧者 -MAINTAINER: メンテナー +maintainer: メンテナー -OWNER: オーナー +owner: オーナーAlso applies to: 422-425
web/src/gql/graphql.schema.json (2)
Line range hint
10699-10727: LGTM! Consider adding field documentation.The
userByNameOrEmailquery implementation looks good. The parameter and return types are appropriate for looking up a single user by name or email.Consider adding a description field to document:
- The query's purpose
- The parameter format/requirements
- When null might be returned
"name": "userByNameOrEmail", -"description": null, +"description": "Lookup a single user by their name or email address. Returns null if no matching user is found.",
10728-10767: LGTM! Consider adding field documentation.The
userSearchquery implementation looks good. The parameter and return types are appropriate for searching multiple users by keyword.Consider adding a description field to document:
- The query's purpose
- The keyword parameter format/requirements
- Search behavior (e.g., partial matches, case sensitivity)
"name": "userSearch", -"description": null, +"description": "Search for users using a keyword that matches against names and email addresses. Returns an empty list if no matches are found.",web/src/components/molecules/Member/MemberAddModal/index.tsx (1)
193-198: Ensure all users are displayed correctlyWhen rendering user information, ensure that all user details are accurately displayed and properly formatted, especially for users with long names or emails.
Consider adding styles to handle text overflow:
const UserName = styled.div` max-width: 50%; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; `;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- web/src/components/atoms/AutoComplete/index.ts (1 hunks)
- web/src/components/molecules/Member/MemberAddModal/index.tsx (2 hunks)
- web/src/components/organisms/Settings/Members/hooks.ts (4 hunks)
- web/src/components/organisms/Settings/Members/index.tsx (2 hunks)
- web/src/gql/graphql-client-api.tsx (5 hunks)
- web/src/gql/graphql.schema.json (2 hunks)
- web/src/gql/queries/user.ts (1 hunks)
- web/src/i18n/translations/en.yml (1 hunks)
- web/src/i18n/translations/ja.yml (1 hunks)
🔇 Additional comments (11)
web/src/components/atoms/AutoComplete/index.ts (2)
1-1: LGTM! Clean import statement.The addition of
AutoCompletePropstype import alongside the component follows TypeScript best practices for component typing.
3-4: LGTM! Clean export structure.The file maintains a clean re-export pattern:
- Default export for the component
- Named export for the type definition
This follows the common pattern for component libraries and provides good TypeScript support for consumers.
web/src/gql/queries/user.ts (1)
5-13: LGTM! Verify backend compatibility.The query is well-structured and the rename from
GET_USER_BY_SEARCHtoGET_USER_BY_NAME_OR_EMAILbetter reflects its specific purpose.Let's verify the backend implementation:
✅ Verification successful
Query rename is properly implemented and compatible with backend
The verification shows that the
userByNameOrEmailquery is fully implemented and integrated:
- Backend schema defines the query in
server/schemas/user.graphql- Backend resolver is implemented in
server/internal/adapter/gql/resolver_user.go- E2E tests in
server/e2e/gql_user_test.goverify the query functionality- Frontend types and query definition are properly updated in generated files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the backend schema supports the new query name and structure # Test: Check if the backend resolver and schema are properly updated ast-grep --pattern 'func (*_) UserByNameOrEmail(ctx context.Context, nameOrEmail string) (*model.User, error)' # Test: Check for any remaining references to the old query name rg -l 'searchUser.*nameOrEmail'Length of output: 146
Script:
#!/bin/bash # Let's check the GraphQL schema and resolver files for the query definition # First, find schema files fd "schema.graphql|schema.gql" --type f --exec cat {} # Then check resolver implementations fd "resolver.*" --type f --exec cat {} # Also check for any references to the old query name in the frontend rg "GET_USER_BY_SEARCH"Length of output: 76852
Script:
#!/bin/bash # Let's check the GraphQL schema and type definitions for the query fd "schema.graphql|schema.gql|user.ts" --type f --exec grep -l "userByNameOrEmail" # Also check for any references to the old query name in test files fd "test|spec|__tests__" --type f --exec grep -l "GET_USER_BY_SEARCH" # Check for any imports or usages of the query rg "userByNameOrEmail" -A 2 -B 2Length of output: 8086
web/src/components/organisms/Settings/Members/index.tsx (2)
12-18: LGTM: Well-structured state managementThe state variables are well-organized with clear separation between searched and selected users, following React conventions. The addition of
searchLoadingstate properly handles loading states during user searches.
Line range hint
1-89: Verify consistent naming across the codebaseLet's ensure the new naming conventions are consistently used across related files.
✅ Verification successful
Naming conventions are consistent across the codebase
The variable
searchedUsersand its related props are consistently used across all relevant files:
- Component implementation in
web/src/components/organisms/Settings/Members/index.tsx- Hook implementation in
web/src/components/organisms/Settings/Members/hooks.ts- Modal component in
web/src/components/molecules/Member/MemberAddModal/index.tsxAll instances follow the same plural form
searchedUsersand there are no inconsistent naming patterns found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent naming of user search related variables and components # Test 1: Check for old variable names that should have been updated echo "Checking for old variable names that should have been updated..." rg -l "searchedUser[^s]" --type=typescript --type=tsx # Test 2: Verify MemberAddModal prop usage is consistent echo "Checking MemberAddModal prop usage..." ast-grep --pattern 'MemberAddModal $props' # Test 3: Check related test files for updated variable names echo "Checking test files for updated naming..." fd -e test.tsx -e test.ts | xargs rg "searchedUser[^s]"Length of output: 510
Script:
#!/bin/bash # Let's try a different approach with correct file type flags # Check for any variations of searchedUser in all files echo "Checking for searchedUser variations..." rg "searchedUser" --type-add 'ts:*.{ts,tsx}' --type ts # Check MemberAddModal component usage echo "Checking MemberAddModal usage..." rg "<MemberAddModal" --type-add 'ts:*.{ts,tsx}' --type ts -A 10 # Check for related hook implementations echo "Checking hook implementations..." rg "searchedUsers.*=" --type-add 'ts:*.{ts,tsx}' --type ts # Check test files specifically echo "Checking test files..." rg "searchedUser" --glob "**/*.test.{ts,tsx}"Length of output: 2691
web/src/components/organisms/Settings/Members/hooks.ts (2)
43-44: LGTM! Clear state separation.The state management changes improve clarity by separating search results (
searchedUsers) from selected users (selectedUsers).
99-101: LGTM! Simple and focused handler.The user add handler is straightforward and follows React's state update best practices.
web/src/i18n/translations/ja.yml (1)
1-559: Verify context-dependent translationsThe translations appear technically correct, but some might benefit from additional context verification:
- Error messages (lines 439-559) follow a consistent pattern
- Technical terms are generally well translated
- UI elements have appropriate translations
Let's verify the usage context of these translations:
✅ Verification successful
Let's verify the usage context of some key translations by examining their implementation:
Let's check the actual usage of translations in the components to verify the context:
Based on the extensive code analysis, I can now provide a final assessment of the translations:
Japanese translations are properly implemented and contextually accurate
The translations in
ja.ymlare correctly used throughout the codebase with proper context:
- UI elements use appropriate Japanese terms (e.g., モデル for "Model", フィールド for "Field")
- Technical terms maintain consistency (e.g., アセット for "Asset", プレビュー for "Preview")
- Form validations and error messages follow natural Japanese grammar
- Action buttons use clear Japanese verbs (e.g., 保存 for "Save", 削除 for "Delete")
- Status messages maintain proper formality level in Japanese
The translations are properly integrated via i18next with:
- Fallback to English when needed
- Support for phrase-based keys
- Browser language detection
- Proper initialization of both English and Japanese resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of these translations in the codebase rg -l "useTranslation|i18n" "web/src"Length of output: 10237
Script:
#!/bin/bash # Check usage of error messages and technical terms rg -A 2 "Failed to|Successfully" "web/src/components/molecules" # Check specific UI elements translations rg -A 2 "useTranslation.*" "web/src/components/molecules/Content/Form"Length of output: 506
Script:
#!/bin/bash # Check translation usage in key components rg -A 3 "t\(['\"]" "web/src/components/molecules/Content/Form" "web/src/components/molecules/Schema" "web/src/components/molecules/Asset" # Check i18n setup and configuration cat web/src/i18n/i18n.tsLength of output: 82528
web/src/gql/graphql-client-api.tsx (2)
1318-1319: LGTM: Clean implementation of user lookup by name/emailThe query is well-structured with appropriate return fields for user lookup functionality.
1417-1419: LGTM: Well-implemented keyword-based user searchThe search query implementation is clean and consistent with the lookup query, providing flexible user search capability.
web/src/components/molecules/Member/MemberAddModal/index.tsx (1)
202-208:⚠️ Potential issueCorrect the
Form.Itemnameprop usageThe
nameprop should be a string or an array for nested fields. Wrappinguser.idin an array when it's already a string may cause issues with form data handling.Update the
nameprop:- <FormItemRole name={[user.id]}> + <FormItemRole name={user.id}>Likely invalid or redundant comment.
yk-eukarya
changed the title
…at/support-user-search
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/src/components/molecules/Member/MemberAddModal/index.tsx(2 hunks)web/src/components/organisms/Settings/Members/hooks.ts(4 hunks)web/src/i18n/translations/en.yml(1 hunks)web/src/i18n/translations/ja.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/organisms/Settings/Members/hooks.ts
🔇 Additional comments (4)
web/src/i18n/translations/en.yml (1)
206-208: Verify all user search related translations are covered
Let's ensure we haven't missed any translations related to the user search feature.
✅ Verification successful
All user search related translations are present and consistent
The codebase search reveals that all necessary translations for the user search feature are covered in the translation file:
- "Search user" - for the search input label
- "No result" - for empty search results
- "Email address or user name" - for input placeholder
These translations align with the feature's implementation in the codebase, particularly in MemberAddModal component where these strings are used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any user search related keys in the codebase
rg -i "user.*search|search.*user" --type ts --type tsx
Length of output: 84
Script:
#!/bin/bash
# Search for user search related strings in the codebase with correct file extensions
rg -i "user.*search|search.*user"
# Also check the translation file directly to see all related keys
cat web/src/i18n/translations/en.yml
Length of output: 26149
web/src/i18n/translations/ja.yml (3)
206-207: LGTM! New translations for user search functionality.
The Japanese translations for "Search user" (ユーザー検索) and "No result" (見つかりませんでした) are accurate and appropriate.
Line range hint 3-3: Empty translation for "Oops!"
Line range hint 449-450: Empty translation and potential string interpolation issue
| Search user: '' | ||
| No result: '' |
There was a problem hiding this comment.
Populate missing translations for user search feature
The translations for the user search feature are currently empty. These need to be populated to ensure a proper user experience:
Search user: 'Search Users'
No result: 'No Results Found'
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
web/src/gql/graphql-client-api.tsx (2)
1325-1326: LGTM! Clean implementation of userByNameOrEmail queryThe query is well-structured with proper typing and null handling.
Consider adding JSDoc comments to document the query's purpose and usage:
+/** + * Query to find a user by their name or email address + * @param nameOrEmail - The name or email to search for + * @returns User object if found, null otherwise + */ userByNameOrEmail?: Maybe<User>;
1424-1426: LGTM! Clean implementation of userSearch queryThe query implementation is correct with proper array return type.
Consider adding JSDoc comments to document the query's purpose and usage:
+/** + * Query to search users by keyword + * @param keyword - The search term to filter users + * @returns Array of matching User objects + */ userSearch: Array<User>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
server/go.mod(1 hunks)server/internal/adapter/gql/generated.go(12 hunks)web/src/components/organisms/Settings/Members/hooks.ts(4 hunks)web/src/components/organisms/Settings/Members/index.tsx(2 hunks)web/src/gql/graphql-client-api.tsx(5 hunks)web/src/gql/graphql.schema.json(2 hunks)web/src/i18n/translations/en.yml(1 hunks)web/src/i18n/translations/ja.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/go.mod
- web/src/components/organisms/Settings/Members/index.tsx
- web/src/i18n/translations/ja.yml
🔇 Additional comments (7)
server/internal/adapter/gql/generated.go (4)
563-564: Schema changes look good!
The new GraphQL queries are well-defined:
userSearch(keyword: String!): [User!]!- Returns a non-null array of non-null User objectsuserByNameOrEmail(nameOrEmail: String!): User- Returns a nullable User object
The return types are appropriate for their use cases - search returns multiple results while lookup by name/email returns a single result.
Also applies to: 1029-1030, 6656-6657
Line range hint 9749-9811: Argument handling implementation is correct!
The argument handling code:
- Properly validates required arguments
- Implements correct type conversions
- Includes appropriate error handling
- Follows the standard GraphQL resolver pattern
25380-25409: Query execution implementations are robust!
Both query implementations include:
- Proper error handling
- Panic recovery
- Middleware integration
- Correct null handling
- Type-safe result marshaling
Also applies to: 25445-25459
25411-25443: Verify completeness of User field handling
The field context implementations look correct, but let's verify that all User fields are properly handled in the child resolver.
Also applies to: 25473-25500
✅ Verification successful
Let me try a different search pattern to gather the necessary information.
All User fields are properly handled in the resolvers
Based on the search results, I can confirm that:
- The GraphQL schema defines User type with 4 fields:
id,name,email, andhost - The generated code in
server/internal/adapter/gql/generated.goincludes field context implementations and resolvers for all 4 fields:_User_idandfieldContext_User_id_User_nameandfieldContext_User_name_User_emailandfieldContext_User_email_User_hostandfieldContext_User_host
The field context implementations are complete and match the User type definition in the schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all User type fields are properly handled in the resolver
# Search for User type definition in schema
echo "Searching for User type definition..."
rg -A 10 "type User \{"
# Search for User field resolvers
echo "Searching for User field resolvers..."
ast-grep --pattern 'func \(.*\) _User_.*\(ctx context.Context, obj \*.*User.*\)'
Length of output: 278
Script:
#!/bin/bash
# Search for User type definition in schema files
echo "Searching for User type definition in schema..."
fd -e graphql -e gql . | xargs rg -l "type User"
# If found, show the content
echo "Showing User type definition..."
fd -e graphql -e gql . | xargs rg -A 10 "type User"
# Search for User resolvers in generated.go
echo "Searching for User resolvers in generated code..."
rg "_User_" server/internal/adapter/gql/generated.go
# Search for User type struct
echo "Searching for User type struct..."
rg "type User struct" -A 5
Length of output: 4631
web/src/i18n/translations/en.yml (1)
217-218:
Critical: Empty translations will affect user search functionality
These translations are essential for the new user search feature. Please refer to the existing review comments for suggested translations to maintain consistency in the UI.
web/src/gql/graphql.schema.json (2)
Line range hint 10740-10768: LGTM! The userByNameOrEmail query is well-defined.
The query has a clear purpose with appropriate nullability and parameter types for exact user lookup by name or email.
10769-10808: LGTM! The userSearch query is well-structured.
The query provides a flexible keyword-based search capability with appropriate non-null list return type for multiple results.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/src/components/molecules/Member/MemberAddModal/index.tsx (1)
224-241: Consider constraining modal widthThe modal width is not constrained, which could lead to inconsistent UI on different screen sizes.
Add width constraint:
const StyledModal = styled(Modal)` + width: 100% !important; + max-width: 600px; .ant-modal-content { padding: 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/components/molecules/Member/MemberAddModal/index.tsx(2 hunks)
🔇 Additional comments (1)
web/src/components/molecules/Member/MemberAddModal/index.tsx (1)
17-27: LGTM: Type definitions are well-structured
The Props interface and FormValues type are well-defined for the new user search functionality. The use of React.Dispatch for setSelectedUsers ensures type safety.
Also applies to: 30-30
Summary by CodeRabbit
Release Notes
New Features
userSearchfor keyword-based searches anduserByNameOrEmailfor specific user retrieval.Metype to includemyWorkspaceIdandauthsfields for more user-specific information.MemberAddModalcomponent for better user search and selection management.Bug Fixes
Documentation
Metype.