refactor(server): refactor test code #1256
Conversation
WalkthroughThe changes in this pull request involve significant refactoring of the server's end-to-end (e2e) testing framework, particularly focusing on GraphQL request handling and server initialization. The modifications include the introduction of new server initialization functions, the consolidation of HTTP request logic into a centralized 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: 5
🧹 Outside diff range and nitpick comments (31)
server/e2e/mock_test.go (3)
47-49: Consider enhancing response validation.While the basic checks are good, consider adding more robust validation:
- Verify the ID format/structure
- Add length checks for name and email
- Consider adding negative test cases
Example enhancement:
- response2.ContainsKey("id") - response2.Value("name").String().Equal("Mock User") - response2.Value("email").String().Equal("[email protected]") + id := response2.Value("id").String().NotEmpty() + // Verify ID format (assuming UUID) + id.Match("^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$") + + name := response2.Value("name").String().Equal("Mock User") + name.Length().GreaterThan(0) + + email := response2.Value("email").String().Equal("[email protected]") + email.Match("^[^@]+@[^@]+\\.[^@]+$")
Line range hint
13-49: Consider restructuring the test for better organization.The test currently combines two distinct operations (signup and GetMe query) in a single test function. Consider splitting these into separate test functions for better maintainability and clarity.
Example restructuring:
-func TestMockAuth(t *testing.T) { +func TestMockAuth_Signup(t *testing.T) { e := StartServer(t, &config.Config{...}) requestBody := map[string]interface{}{ "email": "[email protected]", "username": "Mock User", } response := e.POST("/api/signup")... // Signup assertions } + +func TestMockAuth_GetMe(t *testing.T) { + e := StartServer(t, &config.Config{...}) + + // First create a user + userId := createMockUser(t, e) + + requestBody := GraphQLRequest{ + OperationName: "GetMe", + Query: "query GetMe { \n me { \n id \n name \n email\n } \n}", + Variables: map[string]any{}, + } + + response := Request(e, userId, requestBody)... + // GetMe assertions +} + +func createMockUser(t *testing.T, e *httpexpect.Expect) string { + // Helper function to create a mock user + ... +}
Line range hint
13-15: Add documentation for the mock authentication flow.The test would benefit from clear documentation explaining the mock authentication flow, including what the
MockAuthconfig flag does and how it affects the authentication process.Example enhancement:
// go test -v -run TestMockAuth ./e2e/... + +// TestMockAuth verifies the mock authentication flow when MockAuth is enabled. +// The flow consists of two parts: +// 1. User signup: Creates a new user with mock credentials +// 2. GetMe query: Verifies the authenticated user's information +// +// When MockAuth is enabled: +// - The auth service is bypassed (AuthSrv.Disabled = true) +// - User creation is simplified without password verification +// - Authentication tokens are automatically accepted for the created userserver/e2e/gql_property_test.go (1)
50-50: LGTM! Consider adding error handling.The refactoring to use the centralized
Requestfunction aligns well with the PR objectives of standardizing request calls. However, consider adding error handling to gracefully handle potential request failures.- res := Request(e, uID.String(), requestBody) + res := Request(e, uID.String(), requestBody) + if res.Raw().StatusCode != 200 { + t.Fatalf("Failed to update property value: %v", res.Raw().Status) + }server/e2e/gql_scene_test.go (2)
Line range hint
16-33: Consider using table-driven tests for field validations.The current implementation has repetitive if-blocks for each field validation. Consider refactoring to use a table-driven test approach for better maintainability and readability.
func TestGetScenePlaceholderEnglish(t *testing.T) { e := ServerEnglish(t) pID := createProjectWithExternalImage(e, "test") _, _, sID := createScene(e, pID) r := getScene(e, sID, language.English.String()) + + tests := []struct { + fieldID string + placeholder string + }{ + {"tile_type", "please enter tile type"}, + {"tile_url", "please enter tile url"}, + {"tile_zoomLevel", "please enter tile zoomLevel"}, + {"tile_opacity", "please enter tile opacity"}, + } for _, group := range r.Value("property").Object().Value("schema").Object().Value("groups").Array().Iter() { for _, field := range group.Object().Value("fields").Array().Iter() { fieldId := field.Object().Value("fieldId").Raw().(string) - - if fieldId == "tile_type" { - field.Object().Value("translatedPlaceholder").Equal("please enter tile type") - } - if fieldId == "tile_url" { - field.Object().Value("translatedPlaceholder").Equal("please enter tile url") - } - if fieldId == "tile_zoomLevel" { - field.Object().Value("translatedPlaceholder").Equal("please enter tile zoomLevel") - } - if fieldId == "tile_opacity" { - field.Object().Value("translatedPlaceholder").Equal("please enter tile opacity") + for _, tt := range tests { + if fieldId == tt.fieldID { + field.Object().Value("translatedPlaceholder").Equal(tt.placeholder) + } } } } }
Line range hint
44-61: Consider using table-driven tests and verify translations.Similar to the English test, consider using a table-driven approach. Additionally, please verify the Japanese translations with a native speaker or translation team.
func TestGetScenePlaceholderJapanese(t *testing.T) { e := ServerJapanese(t) pID := createProjectWithExternalImage(e, "test") _, _, sID := createScene(e, pID) r := getScene(e, sID, language.Japanese.String()) + + tests := []struct { + fieldID string + placeholder string + }{ + {"tile_type", "タイルのタイプを選択"}, + {"tile_url", "タイルのURLを入力"}, + {"tile_zoomLevel", "ズームレベルを入力"}, + {"tile_opacity", "不透明度を入力"}, + } for _, group := range r.Value("property").Object().Value("schema").Object().Value("groups").Array().Iter() { for _, field := range group.Object().Value("fields").Array().Iter() { fieldId := field.Object().Value("fieldId").Raw().(string) - - if fieldId == "tile_type" { - field.Object().Value("translatedPlaceholder").Equal("タイルのタイプを選択") - } - if fieldId == "tile_url" { - field.Object().Value("translatedPlaceholder").Equal("タイルのURLを入力") - } - if fieldId == "tile_zoomLevel" { - field.Object().Value("translatedPlaceholder").Equal("ズームレベルを入力") - } - if fieldId == "tile_opacity" { - field.Object().Value("translatedPlaceholder").Equal("不透明度を入力") + for _, tt := range tests { + if fieldId == tt.fieldID { + field.Object().Value("translatedPlaceholder").Equal(tt.placeholder) + } } } } }server/e2e/gql_project_export_test.go (2)
Line range hint
87-108: Multiple improvements needed in the export implementation.
- Function name has a typo:
exporProjectshould beexportProject- Using a fixed filename "project_data.zip" could cause issues in parallel tests
- File permissions are too permissive with
os.ModePermConsider applying these improvements:
-func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { +func exportProject(t *testing.T, e *httpexpect.Expect, p string) string { // ... - fileName := "project_data.zip" + fileName := fmt.Sprintf("project_data_%s.zip", p) // ... - err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) + err := os.WriteFile(fileName, []byte(downloadResponse), 0600) // ... }
Line range hint
20-39: Add basic error checking for entity creation.While detailed GraphQL assertions might be problematic (as per previous learnings), consider adding basic error checks for entity creation to ensure the test setup is successful before proceeding with the export test.
Example for scene creation:
- _, _, sID := createScene(e, pID) + res, obj, sID := createScene(e, pID) + assert.NotEmpty(t, sID, "Scene creation failed") + assert.Equal(t, http.StatusOK, res.Raw().StatusCode)server/e2e/gql_user_test.go (2)
48-54: Add timeout to context in database operations.The tests use
context.Background()directly for database operations. Consider using a timeout context to prevent potential test hangs:- u, err := r.User.FindByID(context.Background(), uId1) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + u, err := r.User.FindByID(ctx, uId1)Also applies to: 62-68
75-76: Remove unnecessary empty line.The empty line between
ServerUser(t)and the query definition can be removed for consistency with other test functions.server/e2e/gql_workspace_test.go (4)
15-18: Consider adding more test cases for team creation.While the basic success case is covered, consider adding tests for:
- Invalid team names (empty, too long)
- Duplicate team names
- Permission checks for non-authorized users
40-57: Consider adding context timeout for repository operations.The repository operations could benefit from a timeout context to prevent potential test hangs:
- w, err := r.Workspace.FindByID(context.Background(), wId1) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + w, err := r.Workspace.FindByID(ctx, wId1)
60-77: Add test cases for invalid member roles.Consider adding test cases for:
- Invalid role values
- Adding member to non-existent team
- Adding non-existent user
Line range hint
1-114: Successful refactoring with room for enhancement.The refactoring has successfully standardized the test structure using the new helper functions. The code is more readable and maintainable. Consider:
- Adding more edge cases and error scenarios
- Implementing consistent timeout contexts
- Adding test cases for concurrent operations
server/e2e/gql_style_test.go (2)
149-149: Document server configuration requirements.While the
Server(t)function simplifies initialization, it's not immediately clear what configuration is being used. This could make it harder to understand test requirements and debug issues.Consider adding a comment explaining the server configuration being used:
+// Server() initializes a test server with default configuration +// including <list relevant settings> e := Server(t)
Line range hint
148-214: Consider adding test cases for error scenarios.While the happy path is well tested, there's no coverage for error scenarios such as:
- Invalid style IDs
- Duplicate style names
- Invalid style values
- Permission-related errors
Would you like me to help generate additional test cases for these scenarios?
server/e2e/seeder.go (2)
40-44: Maintain consistent naming conventions for IDsThe new ID declarations use a different naming convention (camelCase) compared to the existing IDs above (uppercase). Consider standardizing the naming:
- uId1 = accountdomain.NewUserID() - uId2 = accountdomain.NewUserID() - uId3 = accountdomain.NewUserID() - wId1 = accountdomain.NewWorkspaceID() - wId2 = accountdomain.NewWorkspaceID() + uID1 = accountdomain.NewUserID() + uID2 = accountdomain.NewUserID() + uID3 = accountdomain.NewUserID() + wID1 = accountdomain.NewWorkspaceID() + wID2 = accountdomain.NewWorkspaceID()Also consider grouping these with similar ID declarations above for better organization.
134-200: Consider breaking down the function for better maintainabilityThe
baseSeederUserfunction is quite long and handles multiple responsibilities. Consider splitting it into smaller, focused functions:+func createTestUser(ctx context.Context, r *repo.Container, id idx.ID[accountdomain.User], name, email string, opts ...func(*user.User)) error { + u := user.New().ID(id). + Name(name). + Email(email) + for _, opt := range opts { + opt(u) + } + return r.User.Save(ctx, u.MustBuild()) +} + +func createTestWorkspace(ctx context.Context, r *repo.Container, id idx.ID[accountdomain.Workspace], name string, members map[idx.ID[accountdomain.User]]workspace.Member) error { + w := workspace.New().ID(id). + Name(name). + Members(members). + Integrations(map[idx.ID[accountdomain.Integration]]workspace.Member{ + iId1: {Role: workspace.RoleOwner}, + }). + MustBuild() + return r.Workspace.Save(ctx, w) +} + func baseSeederUser(ctx context.Context, r *repo.Container) error { - auth := user.ReearthSub(uId1.String()) - u := user.New().ID(uId1)... + if err := createTestUser(ctx, r, uId1, "e2e", "[email protected]", + func(u *user.User) { + auth := user.ReearthSub(uId1.String()) + u.Auths([]user.Auth{*auth}) + u.Theme(user.ThemeDark) + u.Lang(language.Japanese) + u.Workspace(wId1) + }); err != nil { + return err + } // Similar refactoring for other users and workspacesThis refactoring would:
- Make the code more maintainable and reusable
- Reduce duplication in workspace creation
- Make it easier to add new test users and workspaces
server/e2e/common.go (1)
223-249: Consolidate Request and RequestQuery functionsRequestQuery duplicates much of the functionality in Request. Consider merging them into a single function with options pattern.
+type RequestOptions struct { + Query string + OperationName string + Variables map[string]any + User string + Raw bool +} +func NewRequest(e *httpexpect.Expect, opts RequestOptions) *httpexpect.Value { + body := GraphQLRequest{ + Query: opts.Query, + OperationName: opts.OperationName, + Variables: opts.Variables, + } + + req := e.POST("/api/graphql"). + WithHeader("Origin", "https://example.com"). + WithHeader("authorization", "Bearer test"). + WithHeader("X-Reearth-Debug-User", opts.User). + WithHeader("Content-Type", "application/json") + + if opts.Raw { + jsonData, err := json.Marshal(body) + if err != nil { + panic(err) + } + req.WithBytes(jsonData) + } else { + req.WithJSON(body) + } + + return req.Expect().Status(http.StatusOK).JSON() +}server/e2e/gql_project_import_test.go (3)
Line range hint
14-30: Enhance test coverage and error handlingThe test function could be improved in several areas:
- Add test cleanup to ensure proper teardown
- Include error scenarios (e.g., invalid zip file)
- Validate actual imported data against expected values
- Test multiple language configurations
Consider refactoring like this:
func TestCallImportProject(t *testing.T) { + t.Cleanup(func() { + // Clean up imported project data + }) + + tests := []struct { + name string + file string + lang string + wantErr bool + expected map[string]interface{} + }{ + { + name: "valid English project", + file: "test.zip", + lang: language.English.String(), + expected: map[string]interface{}{ + // Expected values + }, + }, + { + name: "invalid zip file", + file: "invalid.zip", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { e := Server(t) - r := importProject(e, "test.zip") + r := importProject(e, tt.file) + if tt.wantErr { + r.Value("errors").Array().NotEmpty() + return + } // Existing assertions... + // Compare with expected values + }) + } }
Line range hint
32-50: Improve error handling and maintainabilityThe function needs several improvements:
- Validate file existence before upload
- Make teamId configurable
- Move GraphQL query to constants
- Add proper error handling
Consider this refactoring:
-func importProject(e *httpexpect.Expect, filePath string) *httpexpect.Object { +func importProject(e *httpexpect.Expect, filePath string, opts ...Option) *httpexpect.Object { + if _, err := os.Stat(filePath); os.IsNotExist(err) { + panic(fmt.Sprintf("file not found: %s", filePath)) + } + + config := defaultOptions() + for _, opt := range opts { + opt(config) + } + requestBody := map[string]interface{}{ "operationName": "ImportProject", "variables": map[string]interface{}{ - "teamId": wID.String(), + "teamId": config.teamID, "file": nil, }, - "query": `mutation ImportProject($teamId: ID!, $file: Upload!) { - importProject(input: {teamId: $teamId, file: $file}) { - projectData - __typename - } - }`, + "query": ImportProjectMutation, } r := RequestWithMultipart(e, uID.String(), requestBody, filePath).Object() - projectData := r.Value("data").Object().Value("importProject").Object().Value("projectData") + + // Check for errors first + if r.Value("errors").Array().Length().Raw() > 0 { + return r + } + + projectData := r.Value("data").Object(). + Value("importProject").Object(). + Value("projectData") projectData.NotNull() return projectData.Object() }
Line range hint
52-65: Refactor for better error handling and maintainabilityThe function and its associated query need improvements:
- Add proper error handling
- Make userID configurable
- Consider breaking down the large GraphQL query into smaller, reusable fragments
Consider this refactoring:
-func getScene(e *httpexpect.Expect, s string, l string) *httpexpect.Object { +func getScene(e *httpexpect.Expect, sceneID, lang string, opts ...Option) *httpexpect.Object { + config := defaultOptions() + for _, opt := range opts { + opt(config) + } + requestBody := GraphQLRequest{ OperationName: "GetScene", Query: GetSceneGuery, Variables: map[string]any{ - "sceneId": s, - "lang": l, + "sceneId": sceneID, + "lang": lang, }, } - r := Request(e, uID.String(), requestBody).Object() + r := Request(e, config.userID, requestBody).Object() + + // Check for errors first + if r.Value("errors").Array().Length().Raw() > 0 { + return r + } + v := r.Value("data").Object().Value("node") v.NotNull() return v.Object() }Additionally, consider breaking down the large GraphQL query into smaller, reusable fragments in a separate file:
// queries/scene.go const ( SceneBasicInfoFragment = ` fragment SceneBasicInfo on Scene { id rootLayerId teamId projectId } ` ScenePropertiesFragment = ` fragment SceneProperties on Scene { property { ...PropertyFragment } } ` // ... more fragments GetSceneQuery = SceneBasicInfoFragment + ScenePropertiesFragment + ` query GetScene($sceneId: ID!, $lang: Lang) { node(id: $sceneId, type: SCENE) { ...SceneBasicInfo ...SceneProperties } } ` )server/e2e/gql_featureCollection_test.go (1)
Line range hint
179-449: Consider adding error case testsThe test thoroughly covers the happy path for all CRUD operations and geometry types. Consider adding tests for error cases such as:
- Invalid geometry structures
- Missing required properties
- Invalid feature IDs during update/delete
- Concurrent modifications
server/e2e/gql_nlslayer_test.go (2)
Line range hint
476-496: Address commented-out removeNLSInfobox function.The
removeNLSInfoboxfunction is commented out, suggesting incomplete functionality. Either implement the function if it's needed or remove the commented code to maintain code cleanliness.
Line range hint
317-824: Add test cases for error scenarios.The tests thoroughly cover successful operations but lack coverage for error scenarios. Consider adding test cases for:
- Invalid layer/block IDs
- Unauthorized access
- Malformed requests
- Concurrent modifications
Example test structure:
func TestNLSLayerCRUDErrors(t *testing.T) { e := Server(t) // Test invalid layer ID _, res := removeNLSLayer(e, "invalid-id") res.Status(http.StatusOK). JSON().Object(). Path("$.errors").Array(). Length().Gt(0) // Test unauthorized access // Test malformed requests // Test concurrent modifications }server/e2e/gql_storytelling_test.go (6)
40-40: Consider extracting common GraphQL request handling logic.The pattern of making requests and basic response validation is repeated across multiple functions. This could be extracted into a helper function to reduce duplication and improve maintainability.
Consider creating a helper function like:
func executeGraphQLRequest(e *httpexpect.Expect, operation string, query string, variables map[string]any) *httpexpect.Value { requestBody := GraphQLRequest{ OperationName: operation, Query: query, Variables: variables, } return Request(e, uID.String(), requestBody) }Also applies to: 62-62, 117-117, 142-142, 176-176, 201-201, 229-229, 266-266, 302-302, 336-336, 367-367, 403-403, 445-445, 494-494, 546-546, 586-586, 624-624
Line range hint
720-724: Improve error handling test coverage.Good test for invalid page ID, but consider adding more error scenarios:
- Invalid story ID
- Invalid scene ID
- Missing required fields
Line range hint
4-11: Consider using test constants for common test data.Test values like "test", "reearth", "textStoryBlock" are repeated throughout the tests. Consider extracting these into constants for better maintainability.
const ( testProjectName = "test" testPluginID = "reearth" testExtensionID = "textStoryBlock" )
Line range hint
636-685: Consider using test table patterns for CRUD operations.The CRUD test could be more maintainable using table-driven tests, especially for testing different combinations of inputs and expected outcomes.
Example refactor:
func TestStoryCRUD(t *testing.T) { tests := []struct { name string storyTitle string storyIndex int wantErr bool expectedBG string }{ { name: "basic story creation", storyTitle: "test", storyIndex: 0, wantErr: false, expectedBG: "", }, // Add more test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { e := StartServer(t, &config.Config{ Origins: []string{"https://example.com"}, AuthSrv: config.AuthSrvConfig{ Disabled: true, }, }, true, baseSeeder) pID := createProject(e, tt.storyTitle) // ... rest of the test }) } }
Line range hint
893-924: Add validation for published story structure.The story publishing test uses a regex to validate the published JSON structure. Consider using proper JSON parsing and structured validation for better test reliability and maintainability.
Example improvement:
type PublishedStory struct { SchemaVersion int `json:"schemaVersion"` ID string `json:"id"` PublishedAt string `json:"publishedAt"` // ... other fields } func TestStoryPublishing(t *testing.T) { // ... existing setup code ... var story PublishedStory err = json.Unmarshal(buf.Bytes(), &story) assert.NoError(t, err) assert.Equal(t, 1, story.SchemaVersion) assert.Equal(t, sID, story.ID) // ... validate other fields }
Line range hint
1-924: Consider adding integration test scenarios.While the current tests cover basic CRUD operations, consider adding tests for:
- Concurrent operations on the same story/page
- Large story structures with many pages and blocks
- Edge cases in story publishing
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
server/e2e/common.go(4 hunks)server/e2e/dataset_export_test.go(1 hunks)server/e2e/gql_featureCollection_test.go(3 hunks)server/e2e/gql_layer_test.go(1 hunks)server/e2e/gql_me_test.go(1 hunks)server/e2e/gql_nlslayer_test.go(13 hunks)server/e2e/gql_project_export_test.go(3 hunks)server/e2e/gql_project_import_test.go(4 hunks)server/e2e/gql_property_test.go(1 hunks)server/e2e/gql_scene_test.go(2 hunks)server/e2e/gql_storytelling_test.go(18 hunks)server/e2e/gql_style_test.go(5 hunks)server/e2e/gql_user_test.go(1 hunks)server/e2e/gql_workspace_test.go(1 hunks)server/e2e/mock_test.go(1 hunks)server/e2e/seeder.go(3 hunks)
🧰 Additional context used
📓 Learnings (2)
server/e2e/gql_project_export_test.go (4)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.151Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
server/e2e/gql_project_import_test.go (4)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.151Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (30)
server/e2e/gql_me_test.go (3)
8-8: LGTM! Clean server initialization.
The change to use Server(t) aligns well with the PR's objective to standardize server startup across tests.
10-14: LGTM! Clear GraphQL query structure.
The GraphQL request is well-structured with clear operation name and query string.
Line range hint 16-21: Verify the source of test user variables.
The test uses undefined variables (uID, uEmail, uName). Let's verify their source to ensure proper test setup.
✅ Verification successful
Let me generate additional scripts to find the test setup and variable definitions.
Let me check the imports and global variables in seeder.go since it seems to be the source of test variables.
Test variables are properly defined in the package-level scope.
The test variables are defined in server/e2e/seeder.go as package-level variables:
uID: Generated usinguser.NewID()uEmail: Set to "[email protected]"uName: Set to "e2e"
These variables are used across all e2e tests and are initialized during test setup, making them available for the gql_me_test.go test.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the source of test user variables and common package imports
# Check for variable definitions in common.go or other test files
echo "Searching for test user variable definitions..."
rg -A 5 "var (u|test).*ID|Email|Name" server/e2e/
# Check imports and package-level variables
echo "Checking common package imports and variables..."
ast-grep --pattern 'import ($_) "server/e2e/common"'
Length of output: 34401
Script:
#!/bin/bash
# Check for test setup and variable initialization in the test file and its imports
# Look for test setup in the test file
echo "Checking test setup in gql_me_test.go..."
rg -A 10 "func Test|func setup|var u" server/e2e/gql_me_test.go
# Look for variable declarations in seeder.go which seems to be imported
echo "Checking seeder.go for variable declarations..."
rg "^var (uID|uEmail|uName)" server/e2e/seeder.go
# Check imports in the test file
echo "Checking imports in gql_me_test.go..."
rg "^import|package" server/e2e/gql_me_test.go
Length of output: 735
Script:
#!/bin/bash
# Check seeder.go content for variable declarations and initialization
# Look for all variable declarations at package level
echo "Checking all package level variables in seeder.go..."
rg "^var \(" -A 10 server/e2e/seeder.go
# Look for initialization of these variables
echo "Checking initialization of test variables..."
rg "^func Setup|^func baseSetup" -A 15 server/e2e/seeder.go
Length of output: 946
server/e2e/gql_layer_test.go (3)
32-32: Verify the new Request function implementation.
The code uses the newly introduced Request function from the common package. Let's verify its implementation to ensure proper error handling and request configuration.
#!/bin/bash
# Description: Verify the Request function implementation
# Expected: Find the Request function definition and its error handling
# Search for the Request function definition
ast-grep --pattern 'func Request($_, $_, $_) $_'
# Look for error handling in the implementation
rg -A 10 'func Request' server/e2e/common.go32-33: 🛠️ Refactor suggestion
Add error handling for the GraphQL response.
The code assumes the GraphQL response will always be successful and contain the expected data structure. Consider adding error handling:
- res := Request(e, uID.String(), requestBody)
- return requestBody, res, res.Path("$.data.addLayerItem.layer.id").Raw().(string)
+ res := Request(e, uID.String(), requestBody)
+ layerID := res.Path("$.data.addLayerItem.layer.id")
+
+ // Verify the response structure
+ layerID.Required()
+
+ // Safe type assertion with error handling
+ if id, ok := layerID.Raw().(string); ok {
+ return requestBody, res, id
+ }
+ panic("Invalid response structure: missing or invalid layer ID")Likely invalid or redundant comment.
32-32: Verify the undefined uID variable.
The code uses uID.String() but uID is not defined within the visible scope of this function. This could lead to runtime errors.
Let's verify the variable usage:
server/e2e/mock_test.go (1)
44-45: LGTM! Good use of the new Request abstraction.
The refactoring aligns well with the PR's objective of standardizing request calls. The code is more concise and maintainable now.
server/e2e/gql_property_test.go (1)
50-50: Verify the source of uID variable
The uID variable used in the Request call appears to be undefined in the shown context. Please ensure it's properly defined and accessible within this scope.
server/e2e/gql_scene_test.go (2)
39-39: Verify server initialization parameters.
The change to use ServerJapanese aligns with the refactoring goals. However, let's verify that the server initialization includes all necessary configurations from the previous implementation.
✅ Verification successful
Let me try a different search strategy to find the server initialization details.
Let me search for the old implementation to complete the comparison.
Server initialization parameters are correctly preserved
The new ServerJapanese function maintains identical configuration parameters from the old implementation:
- Same Origins configuration (
https://example.com) - Same AuthSrv disabled setting
- Same Japanese language seeder using
baseSeederWithLang - Same
trueparameter for the server start
The refactoring successfully encapsulated the common configuration into a reusable function without losing any functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare old and new server initialization parameters
# Look for the old StartServer usage and new ServerJapanese implementation
# Find the old server initialization
echo "Old server initialization:"
git log -p | rg -A 10 "StartServer.*TestGetScenePlaceholderJapanese"
# Find the new ServerJapanese implementation
echo "New ServerJapanese implementation:"
ast-grep --pattern 'func ServerJapanese($_) { $$$ }'
Length of output: 267
Script:
#!/bin/bash
# Search for ServerJapanese function definition and usage
echo "Looking for ServerJapanese function:"
rg -l "func ServerJapanese"
echo -e "\nServerJapanese implementation:"
rg -A 10 "func ServerJapanese"
echo -e "\nOld implementation (looking for Japanese test):"
rg -B 5 -A 10 "TestGetScenePlaceholderJapanese"
Length of output: 1916
Script:
#!/bin/bash
# Search for the old implementation in git history
echo "Looking for old implementation:"
git log -p server/e2e/gql_scene_test.go | rg -B 5 -A 15 "func TestGetScenePlaceholderJapanese"
# Also check if there are any other server initialization patterns
echo -e "\nChecking other server initializations in the test file:"
rg "StartServer|Server.*\(" server/e2e/gql_scene_test.go
Length of output: 1297
11-11: Verify server initialization parameters.
The change to use ServerEnglish aligns with the refactoring goals. However, let's verify that the server initialization includes all necessary configurations from the previous implementation.
✅ Verification successful
Let me try a different approach to gather the necessary information.
Let's check the implementation of ServerEnglish to compare it with StartServer.
Based on the gathered information, I can now provide the final response:
Server initialization parameters are properly maintained
The ServerEnglish function is a well-designed wrapper that:
- Initializes the server with appropriate config (Origins, AuthSrv settings)
- Uses
StartServerinternally with the same core functionality - Adds English-specific seeding through
baseSeederWithLang
The refactoring maintains all necessary configurations while making the test setup more idiomatic and language-specific.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare old and new server initialization parameters
# Look for the old StartServer usage and new ServerEnglish implementation
# Find the old server initialization
echo "Old server initialization:"
git log -p | rg -A 10 "StartServer.*TestGetScenePlaceholderEnglish"
# Find the new ServerEnglish implementation
echo "New ServerEnglish implementation:"
ast-grep --pattern 'func ServerEnglish($_) { $$$ }'
Length of output: 263
Script:
#!/bin/bash
# Let's first find the test file and its history
echo "=== Finding the test file ==="
fd "gql_scene_test.go$"
echo -e "\n=== Looking for ServerEnglish definition ==="
rg -l "func ServerEnglish"
echo -e "\n=== Checking recent changes in the test file ==="
git log -p -- "**/gql_scene_test.go" | rg -B 2 -A 5 "@@"
echo -e "\n=== Looking for any helper functions in test files ==="
rg "func Server.*English" --type go
echo -e "\n=== Looking for StartServer usage in test files ==="
rg "StartServer" --type go
Length of output: 3099
Script:
#!/bin/bash
echo "=== ServerEnglish implementation ==="
rg -A 10 "func ServerEnglish" server/e2e/common.go
echo -e "\n=== StartServer implementation ==="
rg -A 10 "func StartServer" server/e2e/common.go
echo -e "\n=== Config usage in ServerEnglish ==="
rg -B 3 -A 3 "ServerEnglish" server/e2e/common.go
Length of output: 1807
server/e2e/dataset_export_test.go (2)
11-11: LGTM! Server initialization has been simplified.
The change aligns with the PR's goal of standardizing server startup across e2e tests.
11-11: Verify authentication configuration in Server(t)
Since this test previously used a specific configuration with disabled authentication, let's verify that Server(t) maintains the same behavior.
server/e2e/gql_project_export_test.go (2)
83-84: LGTM! Request handling standardization improves consistency.
The change to use the common Request() function aligns with the PR's goal of standardizing request calls. The response handling remains appropriately specific to the export functionality.
Also applies to: 95-103
18-18: Verify server configuration requirements.
The simplified server initialization looks good, but please ensure that Server(t) provides all necessary configuration that might have been present in the original implementation.
✅ Verification successful
Let me gather more context about the server configuration requirements by checking the original implementation and any specific configuration needs in the test file.
From the git history, I can see that the original implementation used StartServer with specific configuration (Origins and disabled AuthSrv). Let me verify if the new Server(t) function in common.go provides the same configuration by checking its complete implementation.
Server initialization change is safe to use
The new Server(t) function in common.go provides identical configuration as the original implementation:
- Same Origins configuration:
[]string{"https://example.com"} - Same AuthSrv configuration:
Disabled: true - Uses the same underlying
StartServerfunction withbaseSeeder
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the test requires any special server configuration
rg -B 5 -A 5 "func Server\(" server/e2e/common.go
Length of output: 287
Script:
#!/bin/bash
# Check the test file for any specific server configuration needs
rg -B 5 -A 5 "Server\(" server/e2e/gql_project_export_test.go
# Check for any auth or config related code in the test file
rg "config\." server/e2e/gql_project_export_test.go
# Look for the original implementation in git history
git log -p server/e2e/gql_project_export_test.go | rg -B 5 -A 5 "Server\("
Length of output: 1185
Script:
#!/bin/bash
# Check the complete Server function implementation
rg -B 10 -A 10 "func Server\(" --type go server/e2e/common.go
# Check if there are any other tests using specific server configurations
rg "StartServer" --type go server/e2e/
Length of output: 2359
server/e2e/gql_user_test.go (2)
38-98: Well-structured refactoring with consistent patterns.
The refactoring has successfully:
- Standardized server initialization using
ServerUser(t) - Simplified GraphQL request handling with
RequestQuery - Maintained consistent error checking patterns
- Improved code readability
The changes align well with the PR objectives of standardizing server startup and request calls.
Line range hint 13-33: Verify test coverage for signup functionality.
The TestSignUp function appears to be commented out. We should ensure that signup functionality is still adequately tested elsewhere in the codebase.
server/e2e/gql_workspace_test.go (2)
22-35: LGTM! Well-structured test with good coverage.
The test effectively covers:
- Successful team deletion
- Verification of actual deletion
- Error case for non-existent team
- Permission checks
98-114: LGTM! Comprehensive role update testing.
The test effectively covers:
- Role update verification
- Repository state validation
- Error handling for invalid operations
server/e2e/gql_style_test.go (1)
Line range hint 8-146: Verify authorization checks for GraphQL operations.
The test includes several GraphQL operations that should have proper authorization checks in production code.
✅ Verification successful
Authorization checks are properly implemented
The GraphQL operations for styles are properly secured:
- All style operations require an operator object obtained from the authenticated context
- GraphQL endpoint is protected with authentication middleware
- Style interactor methods validate the operator for each operation
- GraphQL resolvers use
getOperator()to ensure authenticated access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for authorization checks in the GraphQL resolvers
# Look for resolver implementations
echo "Searching for style-related resolver implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func (r $_) AddStyle($_, $_) $_ { $$$ }
func (r $_) UpdateStyle($_, $_) $_ { $$$ }
func (r $_) RemoveStyle($_, $_) $_ { $$$ }
func (r $_) DuplicateStyle($_, $_) $_ { $$$ }
'
# Look for authorization middleware or checks
echo "Searching for authorization checks..."
rg -l "(?i)(authorize|permission|access)" --type go
Length of output: 1221
Script:
#!/bin/bash
# Let's check the resolver implementation and middleware setup more specifically
# Check for style-related resolvers with authorization
echo "Checking style mutation resolvers..."
rg "func.*(?:AddStyle|UpdateStyle|RemoveStyle|DuplicateStyle).*\{" -A 10 --type go
# Check middleware setup in main app
echo "Checking middleware setup..."
rg "middleware" -A 5 --type go server/internal/app/
# Check auth checks in GraphQL context
echo "Checking GraphQL context auth..."
rg "type ResolverRoot struct" -A 20 --type go server/internal/adapter/gql/
Length of output: 34558
Script:
#!/bin/bash
# Let's check for authorization in the style interactor and GraphQL context
# Check for operator usage in style operations
echo "Checking operator usage in style operations..."
rg "operator \*usecase\.Operator" -A 5 --type go server/internal/usecase/interactor/style.go
# Check GraphQL context and auth setup
echo "Checking GraphQL context and auth setup..."
rg "func getOperator|func usecases" -A 5 --type go server/internal/adapter/gql/
# Check for auth middleware in GraphQL setup
echo "Checking GraphQL auth middleware..."
rg "graphql|playground" -A 10 --type go server/internal/app/app.go
Length of output: 3988
server/e2e/seeder.go (1)
147-162: Consider adding auth configuration for all test users
User u1 has auth configuration but u2 and u3 don't. If authentication is required for testing different scenarios, consider adding auth configuration for all users to maintain consistency and prevent potential test failures.
server/e2e/common.go (4)
5-5: LGTM: Import additions are appropriate
The added imports for JSON handling and test assertions are well-suited for the new test utility functions.
Also applies to: 25-26
187-215: LGTM: Language-specific server implementations
The implementation of ServerEnglish and ServerJapanese with inline seeder functions is clean and maintainable. The use of language.English and language.Japanese constants ensures type safety.
217-221: LGTM: GraphQLRequest struct definition
The struct is well-defined with appropriate field types and JSON tags.
170-172: Verify the necessity of disabled authentication
All server initialization functions disable authentication by default. This might not be suitable for testing authentication-related scenarios.
Also applies to: 180-182, 190-192, 205-207
server/e2e/gql_project_import_test.go (1)
Line range hint 19-27: Verify assertion execution
Based on previous learnings from similar test files, we should verify that all assertions are properly executing and not silently passing.
Let's verify the test execution:
✅ Verification successful
Let me gather more information about the test file in question to verify the assertions.
Let me check the test helper function implementation to understand how assertions are handled.
Based on the gathered information, I can now provide the final response:
Assertions are properly implemented and executing
The assertions in the test file are correctly implemented and will fail if the conditions are not met because:
- The test uses
httpexpectlibrary's assertion chain which throws errors on failed assertions - The
NotNull()assertions are properly chained afterValue()calls - The test helper
RequestQueryis correctly implemented to return the JSON response value that can be asserted on - The assertions are actively used and not commented out
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if assertions are properly executing
# Expected: The test should fail when assertions are intentionally broken
# Search for similar test files to compare assertion patterns
echo "Searching for similar test patterns..."
rg -l "Value\(.*\)\.NotNull\(\)" server/e2e/
# Check test execution patterns
echo "Checking test execution patterns..."
rg "testing\.(T|M)" server/e2e/gql_*_test.go -A 5 -B 5
# Look for any commented out or disabled assertions
echo "Checking for disabled assertions..."
rg "//.*Value\(.*\)\.(NotNull|Equal)" server/e2e/
Length of output: 24877
Script:
#!/bin/bash
# Check the test file content and execution pattern
echo "Checking test file content..."
cat server/e2e/gql_project_import_test.go
# Check for test helper functions that might be used for assertions
echo "Checking test helper functions..."
rg "func.*Request.*Query" server/e2e/
Length of output: 8917
Script:
#!/bin/bash
# Check the test helper implementation
echo "Checking test helper implementation..."
cat server/e2e/common.go
# Check for any similar test patterns in other files
echo "Checking similar test patterns..."
rg "RequestQuery.*NotNull\(\)" server/e2e/
rg "Value\(.*\)\.NotNull\(\)" server/e2e/
Length of output: 7871
server/e2e/gql_featureCollection_test.go (3)
145-145: LGTM! Consistent with the refactoring pattern
The change aligns well with the standardization goals of the PR.
171-171: LGTM! Consistent with the refactoring pattern
The change aligns well with the standardization goals of the PR.
74-74: Verify the source of uID variable
The uID variable is used here but its declaration is not visible in the current scope. Please ensure it's properly defined and accessible.
server/e2e/gql_nlslayer_test.go (2)
72-72: Verify error handling in the new Request function.
The refactoring to use the Request function improves code maintainability. However, ensure that the new function properly handles:
- Network errors
- GraphQL errors
- Invalid responses
- Rate limiting
#!/bin/bash
# Description: Look for error handling patterns in the Request function implementation
# Search for the Request function definition
ast-grep --pattern 'func Request($_, $_, $_) {
$$$
}'
# Look for error handling patterns
rg "Error|err|panic|recover" --type goAlso applies to: 91-91, 141-141, 163-163, 213-213, 311-311, 462-462, 492-492, 538-538, 587-587, 633-633, 739-739
317-317: Verify test coverage with standardized server configuration.
The server initialization has been simplified to use Server(t). While this improves code consistency, ensure that the standard configuration adequately covers all test scenarios, including edge cases that might have been tested with custom configurations previously.
Also applies to: 643-643, 745-745
| w, err = r.Workspace.FindByID(context.Background(), wId1) | ||
| assert.Nil(t, err) | ||
| assert.False(t, w.Members().HasUser(uId3)) |
There was a problem hiding this comment.
Fix incorrect workspace ID in verification.
There's a bug in the verification step. The test removes a member from wId2 but verifies the removal from wId1.
- w, err = r.Workspace.FindByID(context.Background(), wId1)
+ w, err = r.Workspace.FindByID(context.Background(), wId2)📝 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.
| w, err = r.Workspace.FindByID(context.Background(), wId1) | |
| assert.Nil(t, err) | |
| assert.False(t, w.Members().HasUser(uId3)) | |
| w, err = r.Workspace.FindByID(context.Background(), wId2) | |
| assert.Nil(t, err) | |
| assert.False(t, w.Members().HasUser(uId3)) |
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
| res := Request(e, uID.String(), requestBody) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider passing user ID as a parameter instead of using a global variable.
The use of a global uID variable makes the code harder to understand and maintain. It also potentially violates test isolation principles and could cause issues in parallel test execution.
Consider modifying the function signature to include the user ID:
-func addStyle(e *httpexpect.Expect, sId, name string) (GraphQLRequest, *httpexpect.Value, string) {
+func addStyle(e *httpexpect.Expect, userId, sId, name string) (GraphQLRequest, *httpexpect.Value, string) {
// ...
- res := Request(e, uID.String(), requestBody)
+ res := Request(e, userId, requestBody)
// ...
}Committable suggestion skipped: line range outside the PR's diff.
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("X-Reearth-Debug-User", user). |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make authentication and origin headers configurable
The hardcoded values for authentication token and origin should be configurable to support different test scenarios.
+type TestHeaders struct {
+ Origin string
+ Token string
+ TestUser string
+}
+var DefaultTestHeaders = TestHeaders{
+ Origin: "https://example.com",
+ Token: "Bearer test",
+ TestUser: "",
+}
+func WithHeaders(headers TestHeaders) func(*httpexpect.Request) {
+ return func(req *httpexpect.Request) {
+ if headers.Origin != "" {
+ req.WithHeader("Origin", headers.Origin)
+ }
+ if headers.Token != "" {
+ req.WithHeader("authorization", headers.Token)
+ }
+ if headers.TestUser != "" {
+ req.WithHeader("X-Reearth-Debug-User", headers.TestUser)
+ }
+ }
+}Also applies to: 242-244, 258-260
| func toJSONString(v interface{}) string { | ||
| jsonData, _ := json.Marshal(v) | ||
| return string(jsonData) | ||
| } |
There was a problem hiding this comment.
Handle JSON marshaling errors in toJSONString
The function silently ignores potential JSON marshaling errors, which could mask issues in tests.
-func toJSONString(v interface{}) string {
- jsonData, _ := json.Marshal(v)
- return string(jsonData)
+func toJSONString(v interface{}) (string, error) {
+ jsonData, err := json.Marshal(v)
+ if err != nil {
+ return "", fmt.Errorf("failed to marshal JSON: %w", err)
+ }
+ return string(jsonData), nil
+}📝 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.
| func toJSONString(v interface{}) string { | |
| jsonData, _ := json.Marshal(v) | |
| return string(jsonData) | |
| } | |
| func toJSONString(v interface{}) (string, error) { | |
| jsonData, err := json.Marshal(v) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to marshal JSON: %w", err) | |
| } | |
| return string(jsonData), nil | |
| } |
| func Server(t *testing.T) *httpexpect.Expect { | ||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
| return StartServer(t, c, true, baseSeeder) | ||
| } | ||
|
|
||
| func ServerUser(t *testing.T) (*httpexpect.Expect, *accountrepo.Container) { | ||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
| return StartGQLServer(t, c, true, baseSeederUser) | ||
| } | ||
|
|
||
| func ServerEnglish(t *testing.T) *httpexpect.Expect { | ||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
| return StartServer(t, c, true, | ||
| // English user Seeder | ||
| func(ctx context.Context, r *repo.Container) error { | ||
| return baseSeederWithLang(ctx, r, language.English) | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| func ServerJapanese(t *testing.T) *httpexpect.Expect { | ||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
| return StartServer(t, c, true, | ||
| // Japanese user Seeder | ||
| func(ctx context.Context, r *repo.Container) error { | ||
| return baseSeederWithLang(ctx, r, language.Japanese) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting common server configuration
There's significant duplication in the config creation across Server, ServerUser, ServerEnglish, and ServerJapanese functions. Consider extracting the common configuration into a helper function.
+func newTestConfig() *config.Config {
+ return &config.Config{
+ Origins: []string{"https://example.com"},
+ AuthSrv: config.AuthSrvConfig{
+ Disabled: true,
+ },
+ }
+}
func Server(t *testing.T) *httpexpect.Expect {
- c := &config.Config{
- Origins: []string{"https://example.com"},
- AuthSrv: config.AuthSrvConfig{
- Disabled: true,
- },
- }
+ c := newTestConfig()
return StartServer(t, c, true, baseSeeder)
}📝 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.
| func Server(t *testing.T) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, baseSeeder) | |
| } | |
| func ServerUser(t *testing.T) (*httpexpect.Expect, *accountrepo.Container) { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartGQLServer(t, c, true, baseSeederUser) | |
| } | |
| func ServerEnglish(t *testing.T) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, | |
| // English user Seeder | |
| func(ctx context.Context, r *repo.Container) error { | |
| return baseSeederWithLang(ctx, r, language.English) | |
| }, | |
| ) | |
| } | |
| func ServerJapanese(t *testing.T) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, | |
| // Japanese user Seeder | |
| func(ctx context.Context, r *repo.Container) error { | |
| return baseSeederWithLang(ctx, r, language.Japanese) | |
| }, | |
| ) | |
| } | |
| func newTestConfig() *config.Config { | |
| return &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| } | |
| func Server(t *testing.T) *httpexpect.Expect { | |
| c := newTestConfig() | |
| return StartServer(t, c, true, baseSeeder) | |
| } | |
| func ServerUser(t *testing.T) (*httpexpect.Expect, *accountrepo.Container) { | |
| c := newTestConfig() | |
| return StartGQLServer(t, c, true, baseSeederUser) | |
| } | |
| func ServerEnglish(t *testing.T) *httpexpect.Expect { | |
| c := newTestConfig() | |
| return StartServer(t, c, true, | |
| // English user Seeder | |
| func(ctx context.Context, r *repo.Container) error { | |
| return baseSeederWithLang(ctx, r, language.English) | |
| }, | |
| ) | |
| } | |
| func ServerJapanese(t *testing.T) *httpexpect.Expect { | |
| c := newTestConfig() | |
| return StartServer(t, c, true, | |
| // Japanese user Seeder | |
| func(ctx context.Context, r *repo.Container) error { | |
| return baseSeederWithLang(ctx, r, language.Japanese) | |
| }, | |
| ) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (27)
web/.husky/pre-commit (1)
4-5: Add error handling for robustness.The script would benefit from basic error handling to provide clear feedback if either command fails.
Consider applying this improvement:
#!/bin/sh . "$(dirname "$0")/_/husky.sh" -yarn --cwd web run type -yarn --cwd web run lint-staged +if ! yarn --cwd web run type; then + echo "Type checking failed" + exit 1 +fi + +if ! yarn --cwd web run lint-staged; then + echo "Lint checks failed" + exit 1 +fiweb/src/beta/features/PluginPlayground/Plugins/constants.ts (2)
18-31: Fix YAML indentation for better readability.The YAML configuration string has inconsistent indentation which could lead to parsing issues. Consider using consistent 2-space indentation.
Apply this diff to fix the indentation:
sourceCode: `id: demo-widget name: Test plugin version: 1.0.0 extensions: - id: demo-widget type: widget name: Demo Widget description: Demo widget widgetLayout: defaultLocation: zone: outer section: left area: top `,
18-31: Consider adding JSDoc comments to document the widget configuration structure.The widget configuration would benefit from documentation explaining the purpose of each field and the available options for the layout configuration (zones, sections, areas).
Add JSDoc comments above the constant:
/** * Default configuration for the demo widget plugin. * @property id - Unique identifier for the plugin * @property name - Display name of the plugin * @property extensions - List of plugin extensions * @property extensions[].widgetLayout.defaultLocation - Widget positioning * - zone: 'outer' - Outer frame of the interface * - section: 'left'|'right' - Section within the zone * - area: 'top'|'middle'|'bottom' - Vertical position */ export const REEARTH_YML_FILE = {web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/index.tsx (2)
16-20: Consider reordering props for consistency.While the implementation is correct, consider ordering the destructured props alphabetically (
extensionId,properties,selectedFeature) to maintain consistency across the codebase.const CustomFields: FC<Props> = ({ extensionId, - properties, - selectedFeature + selectedFeature, + properties }) => {
16-24: Consider adding tests for the selectedFeature integration.While the implementation looks good, it would be beneficial to add test cases that verify:
- Component behavior with and without selectedFeature
- Proper property evaluation with different selectedFeature values
Would you like me to help generate test cases for these scenarios?
web/src/beta/features/PluginPlayground/Code/index.tsx (1)
34-34: Add accessibility improvements to execute buttonWhile the button implementation works, it could benefit from accessibility enhancements:
- Add an
aria-labelto describe the button's purpose- Consider adding a tooltip for better UX
- <Button icon="playRight" iconButton onClick={executeCode} /> + <Button + icon="playRight" + iconButton + onClick={executeCode} + aria-label="Execute code" + title="Execute code" + />Also applies to: 40-40
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts (3)
7-10: Add JSDoc documentation for the Props type.Consider adding JSDoc comments to document the purpose and expected values of each property in the Props type.
+/** + * Props for the useEvaluateProperties hook + * @property selectedFeature - The currently selected computed feature to evaluate properties against + * @property properties - List of property items to be evaluated + */ type Props = { selectedFeature?: ComputedFeature; properties?: PropertyListItem[]; };
Line range hint
22-63: Consider optimizing the useEffect implementation.The current implementation has several areas for improvement:
- The effect runs on every render due to the comment "We want the useEffect to be called on each render"
- The feature transformation could be memoized
- The isReady state might be unnecessary
Consider this refactoring:
+ const simpleFeature = useMemo( + () => + selectedFeature + ? { + id: selectedFeature.id, + type: "feature" as const, + geometry: selectedFeature.geometry, + interval: selectedFeature.interval, + properties: selectedFeature.properties, + metaData: selectedFeature.metaData, + range: selectedFeature.range, + } + : undefined, + [selectedFeature] + ); + useEffect(() => { - if (!isReady) { - setIsReady(true); - return; - } if (currentValue !== properties) { setCurrentValue(properties); setEvaluatedResult(undefined); return; } - if (selectedFeature) { - const simpleFeature: Feature = { - id: selectedFeature.id, - type: "feature", - geometry: selectedFeature.geometry, - interval: selectedFeature.interval, - properties: selectedFeature.properties, - metaData: selectedFeature.metaData, - range: selectedFeature.range - }; + if (simpleFeature) { const es = currentValue?.map((v) => { const ev = evalExpression( { expression: v.value }, undefined, simpleFeature ); return ev ? { ...v, value: ev } : undefined; }); if (!isEqual(es, evaluatedProperties)) { setEvaluatedResult(es as PropertyListItem[]); } } - }, [isReady, currentValue, properties, evaluatedProperties, selectedFeature]); + }, [currentValue, properties, evaluatedProperties, simpleFeature]);
Line range hint
38-52: Add error handling for expression evaluation.The current implementation silently handles evaluation failures. Consider adding proper error handling and logging.
const es = currentValue?.map((v) => { + try { const ev = evalExpression( { expression: v.value }, undefined, simpleFeature ); return ev ? { ...v, value: ev } : undefined; + } catch (error) { + console.error(`Failed to evaluate expression: ${v.value}`, error); + return undefined; + } });web/src/beta/features/PluginPlayground/hooks.tsx (2)
97-103: Consider adding error handling for updateFileSourceCodeWhile the fallback to
selectedFile.sourceCodeis good, there's no error handling for theupdateFileSourceCodeoperation.Consider wrapping the operation in a try-catch:
onChangeSourceCode={(value) => { + try { updateFileSourceCode( value ?? selectedFile.sourceCode, selectedFile.id ); + } catch (error) { + console.error('Failed to update file source code:', error); + // Consider showing user feedback + } }}
115-115: Consider organizing dependencies alphabeticallyFor better maintainability, consider organizing the dependency array alphabetically.
- [selectedFile, handlePluginDownload, executeCode, updateFileSourceCode] + [executeCode, handlePluginDownload, selectedFile, updateFileSourceCode]web/src/services/api/widgetsApi/utils.ts (2)
85-85: Address the pending design requirementsThe TODO comment indicates that the icon mappings need to be updated based on design specifications.
Would you like me to create a GitHub issue to track this design requirement?
84-94: Use exported constants in switch statementThe switch statement uses string literals that duplicate the exported constants. Consider refactoring to use the constants directly.
- switch (`${pluginId}/${extensionId}`) { - // TODO: get the correct icon from design - case BUTTON_BUILTIN_WIDGET_ID: + switch (true) { + case `${pluginId}/${extensionId}` === BUTTON_BUILTIN_WIDGET_ID: return "return"; - case NAVIGATOR_BUILTIN_WIDGET_ID: + case `${pluginId}/${extensionId}` === NAVIGATOR_BUILTIN_WIDGET_ID: return "crosshair"; - case DATA_ATTRIBUTION_WIDGET_ID: + case `${pluginId}/${extensionId}` === DATA_ATTRIBUTION_WIDGET_ID: return "listDashes"; default: return undefined; }web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/items/RecycleBinItem.tsx (2)
5-5: Consider adding type definitions and browser compatibility for the image asset.While .webp format offers better compression, ensure browser compatibility is handled appropriately.
Consider adding type definitions for the image import:
declare module "*.webp" { const content: string; export default content; }
98-99: Consider optimizing background image rendering.While the current implementation works, it could be more performant by using CSS properties directly.
Consider this alternative implementation:
- background: backgroundImage ? `url(${backgroundImage}) center/cover` : "", + backgroundImage: backgroundImage ? `url(${backgroundImage})` : "none", + backgroundPosition: "center", + backgroundSize: "cover", backgroundColor: theme.bg[1],This approach:
- Separates concerns between background-image and its properties
- Potentially better browser optimization
- More maintainable CSS properties
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/Content.tsx (1)
Line range hint
42-71: Consider simplifying the conditional rendering logic.The nested ternary operations and multiple conditions make the code harder to maintain. Consider extracting the rendering logic into separate components or using a more declarative approach.
Here's a suggested refactor:
- return ( - <Wrapper> - {!context?.editMode ? ( - displayTypeField?.value === "custom" ? ( - properties ? ( - <CustomFields - selectedFeature={selectedComputedFeature} - properties={properties} - extensionId={block?.extensionId} - /> - ) : ( - <Template icon={block?.extensionId} height={120} /> - ) - ) : ( - <DefaultFields properties={properties} isEditable={isEditable} /> - ) - ) : ( - <Template icon={block?.extensionId} height={120} /> - )} - {context?.editMode && ( - <ListEditor - propertyId={block?.propertyId} - displayTypeField={displayTypeField} - propertyListField={propertyListField} - {...props} - /> - )} - </Wrapper> - ); + const renderContent = () => { + if (context?.editMode) { + return <Template icon={block?.extensionId} height={120} />; + } + + if (displayTypeField?.value === "custom") { + return properties ? ( + <CustomFields + selectedFeature={selectedComputedFeature} + properties={properties} + extensionId={block?.extensionId} + /> + ) : ( + <Template icon={block?.extensionId} height={120} /> + ); + } + + return <DefaultFields properties={properties} isEditable={isEditable} />; + }; + + return ( + <Wrapper> + {renderContent()} + {context?.editMode && ( + <ListEditor + propertyId={block?.propertyId} + displayTypeField={displayTypeField} + propertyListField={propertyListField} + {...props} + /> + )} + </Wrapper> + );web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts (1)
21-22: Consider adding basic 3dtiles propertiesFor consistency with other 3dtiles styles in the file, consider adding basic properties like
colorBlendModeandpbrthat are common across other presets.}, - "3dtiles": {} + "3dtiles": { + colorBlendMode: "highlight", + pbr: false + }web/src/beta/lib/reearth-ui/components/TextInput/index.tsx (1)
70-79: Consider making the Enter key behavior configurableWhile the implementation is technically sound, forcing a blur on Enter key press might not be desirable for all use cases (e.g., multi-line input scenarios or when Enter should trigger a different action).
Consider adding a prop to make this behavior optional:
export type TextInputProps = { // ... existing props + blurOnEnter?: boolean; }; const handleKeyDown = useCallback( (e: KeyboardEvent<HTMLInputElement>) => { - if (e.key === "Enter" || e.key === "Return") { + if (blurOnEnter && (e.key === "Enter" || e.key === "Return")) { e.preventDefault(); (e.target as HTMLInputElement).blur(); } onKeyDown?.(e); }, - [onKeyDown] + [onKeyDown, blurOnEnter] );web/src/beta/lib/reearth-ui/components/NumberInput/index.tsx (1)
Line range hint
11-23: Consider enhancing component configurability and accessibility.While the current implementation is solid, consider these improvements:
- Make the Enter key behavior configurable through props
- Add aria-labels for better accessibility
Here's a suggested enhancement:
export type NumberInputProps = { value?: number | string; size?: "normal" | "small"; disabled?: boolean; placeholder?: string; appearance?: "readonly"; extendWidth?: boolean; unit?: string; min?: number; max?: number; + submitOnEnter?: boolean; + ariaLabel?: string; onChange?: (value?: number) => void; onBlur?: (value?: number) => void; };Then update the component accordingly:
const handleKeyDown = useCallback((e: KeyboardEvent<HTMLInputElement>) => { - if (e.key === "Enter" || e.key === "Return") { + if (submitOnEnter && (e.key === "Enter" || e.key === "Return")) { e.preventDefault(); (e.target as HTMLInputElement).blur(); } - }, []); + }, [submitOnEnter]); return ( <Wrapper size={size} status={isFocused ? "active" : "default"} extendWidth={extendWidth} > <StyledInput + aria-label={ariaLabel || `Number input ${unit ? `in ${unit}` : ''}`} value={currentValue} disabled={disabled}web/src/beta/features/PluginPlayground/Plugins/hook.ts (1)
15-41: Consider enhancing the demo widget template for production use.The demo widget template is well-structured, but could be more production-ready:
Consider these improvements:
- sourceCode: `reearth.ui.show(\` - <style> - body { - margin: 0; - } - #wrapper { - background: #232226; - height: 100%; - color: white; - border: 3px dotted red; - border-radius: 5px; - padding: 20px 0; - } - </style> - <div id="wrapper"> - <h2 style="text-align: center; margin: 0;">Hello World</h2> - </div> -\`); + sourceCode: `reearth.ui.show(\` + <style> + .demo-widget { + background: var(--theme-background, #232226); + height: 100%; + color: var(--theme-text, white); + border-radius: 4px; + padding: 1rem; + box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1); + } + .demo-widget__title { + text-align: center; + margin: 0; + font-size: 1.5rem; + } + </style> + <div class="demo-widget"> + <h2 class="demo-widget__title">Hello World</h2> + </div> +\`);Changes include:
- Using BEM naming convention for CSS classes
- Adding CSS variables for theming
- Removing development-specific styles (dotted border)
- Using more maintainable units (rem)
- Adding subtle visual enhancements (box-shadow)
web/src/beta/lib/reearth-ui/components/PopupMenu/index.tsx (2)
305-306: Consider adding ARIA attributes for better accessibilityWhile the flex layout changes are good, consider enhancing accessibility by adding appropriate ARIA attributes to the styled link component.
const StyledLink = styled(Link)(() => ({ textDecoration: "none", flex: 1, - display: "flex" + display: "flex", + "&[aria-current]": { + fontWeight: "bold" + } }));
367-376: Add prop documentation for better maintainabilityConsider adding JSDoc comments to document the purpose and behavior of the component props, especially the new
flexprop.+/** + * TitleWrapper component for popup menu items + * @prop {boolean} disabled - Whether the item is disabled + * @prop {boolean} flex - Whether to use flex layout (used when tileComponent is present) + */ const TitleWrapper = styled("div")<{ disabled?: boolean; flex?: boolean }>( ({ theme, disabled, flex }) => ({ fontSize: theme.fonts.sizes.body, color: disabled ? theme.content.weak : theme.content.main, whiteSpace: "nowrap", overflow: "hidden", textOverflow: "ellipsis", gap: theme.spacing.small, flex: 1, ...(flex ? { display: "flex", alignItems: "center" } : {}) }) );server/e2e/gql_project_test.go (3)
Line range hint
195-228: Consider extracting assertion helpers for better readabilityThe test assertions, while thorough, mix different levels of validation (count, structure, content). Consider extracting common assertion patterns into helper functions for better readability and reuse.
Example helper function:
func assertStarredProject(t *testing.T, node *httpexpect.Object, expectedID string, expectedName string) { id := node.Value("id").String().Raw() name := node.Value("name").String().Raw() starred := node.Value("starred").Boolean().Raw() assert.Equal(t, expectedName, name) assert.True(t, starred) assert.Equal(t, expectedID, id) }
Line range hint
234-260: Consider enhancing helper functionsThe helper functions could be improved in two ways:
- Add error handling for failed mutations
- Extract GraphQL queries as constants to avoid duplication
Example implementation:
const ( updateProjectMutation = ` mutation UpdateProject($input: UpdateProjectInput!) { updateProject(input: $input) { project { id name isDeleted starred updatedAt __typename } __typename } }` ) func starProject(e *httpexpect.Expect, projectID string) error { response := Request(e, uID.String(), GraphQLRequest{ OperationName: "UpdateProject", Query: updateProjectMutation, Variables: map[string]any{ "input": map[string]any{ "projectId": projectID, "starred": true, }, }, }) // Add error handling if response.Raw() == nil { return fmt.Errorf("failed to star project %s", projectID) } return nil }Also applies to: 421-450
Add error case tests for project operations
The test suite needs additional error case coverage. Current tests only handle successful operations, with minimal error checking. Add tests for:
- Attempting to create/update/delete a project with an invalid project ID
- Creating a project with invalid team ID
- Accessing projects without proper authorization
- Handling malformed GraphQL mutation inputs
Example locations needing error tests:
TestCreateProject: Add invalid team ID caseTestDeleteProjects: Add invalid project ID case- Add new test
TestProjectErrorsfor comprehensive error scenarios🔗 Analysis chain
Line range hint
1-450: Consider adding error case testsThe test suite thoroughly covers the happy path but could benefit from additional tests for error cases such as:
- Invalid project IDs
- Unauthorized access attempts
- Malformed GraphQL queries
Let's check if there are any existing error case tests in other files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error case tests in other files rg -l "TestError|invalid|unauthorized" "server/e2e/"Length of output: 88
Script:
#!/bin/bash # Let's check the content of the error test file to understand the error testing patterns rg -A 5 "TestError|invalid|unauthorized" "server/e2e/gql_storytelling_test.go" # Also search for any other test files that might contain error handling patterns fd "test.go" "server/e2e/" --exec rg -l "t.Error|assert.*Error|expect.*Error"Length of output: 433
Script:
#!/bin/bash # Let's check the error handling patterns in the project test file rg "t.Error|assert.*Error|expect.*Error" "server/e2e/gql_project_test.go" -B 2 -A 2 # Also check if there are any invalid ID tests in the project tests rg "invalid.*ID|NewProjectID\(\)" "server/e2e/gql_project_test.go" -B 2 -A 2Length of output: 267
web/src/beta/features/PluginPlayground/Code/hook.ts (2)
56-56: Enhance file lookup to support additional file extensionsCurrently, the code searches for YAML files with the
.ymlextension and JavaScript files with the.jsextension. To improve flexibility and user experience, consider supporting additional common file extensions such as.yaml,.ts, and.tsx.Proposed changes:
At line 56:
- const ymlFile = files.find((file) => file.title.endsWith(".yml")); + const ymlFile = files.find((file) => + file.title.endsWith(".yml") || file.title.endsWith(".yaml") + );At line 79:
- const file = files.find((file) => file.title === `${cur.id}.js`); + const file = files.find((file) => + [`${cur.id}.js`, `${cur.id}.ts`, `${cur.id}.tsx`].includes(file.title) + );This enhancement allows users to work with different file types, improving compatibility and user experience.
Also applies to: 79-79
58-59: Notify the user when no YAML file is foundIf no YAML file is found in the
filesarray, theexecuteCodefunction returns silently. This may confuse users who expect some feedback. Consider adding a notification to inform the user that no YAML file was found.Proposed change:
if (!ymlFile) { + setNotification({ type: "error", text: "No YAML file (.yml or .yaml) found." }); return; }This change enhances user feedback and improves the overall user experience.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (21)
server/e2e/gql_project_test.go(13 hunks)web/.husky/pre-commit(1 hunks)web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx(1 hunks)web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/items/RecycleBinItem.tsx(3 hunks)web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/StyleNode/ConditionsTab.tsx(1 hunks)web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts(1 hunks)web/src/beta/features/Editor/Visualizer/hooks.ts(2 hunks)web/src/beta/features/Editor/Widgets/WidgetManagerPanel/Action.tsx(1 hunks)web/src/beta/features/PluginPlayground/Code/hook.ts(1 hunks)web/src/beta/features/PluginPlayground/Code/index.tsx(2 hunks)web/src/beta/features/PluginPlayground/Plugins/constants.ts(1 hunks)web/src/beta/features/PluginPlayground/Plugins/hook.ts(3 hunks)web/src/beta/features/PluginPlayground/Viewer/index.tsx(2 hunks)web/src/beta/features/PluginPlayground/hooks.tsx(4 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/Content.tsx(1 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/index.tsx(2 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts(2 hunks)web/src/beta/lib/reearth-ui/components/NumberInput/index.tsx(3 hunks)web/src/beta/lib/reearth-ui/components/PopupMenu/index.tsx(4 hunks)web/src/beta/lib/reearth-ui/components/TextInput/index.tsx(2 hunks)web/src/services/api/widgetsApi/utils.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/ProjectGridViewItem.tsx
- web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/StyleNode/ConditionsTab.tsx
- web/src/beta/features/Editor/Widgets/WidgetManagerPanel/Action.tsx
🧰 Additional context used
🪛 Biome
web/src/beta/features/PluginPlayground/Code/hook.ts
[error] 96-96: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (27)
web/.husky/pre-commit (1)
4-4: LGTM! Consider monitoring commit times.
Adding type checking to the pre-commit hook is a good practice for catching type errors early. However, type checking can be time-consuming for larger codebases.
Let's check the type script's configuration and potential impact:
✅ Verification successful
Type checking in pre-commit hook should have acceptable performance impact
The type checking configuration looks well-optimized:
- Codebase size is moderate (719 TypeScript files, 4.5K total)
- TypeScript config has performance-friendly settings:
noEmit: true(skips emit phase)skipLibCheck: true(skips checking declaration files)
- Excludes unnecessary directories from type checking
The performance impact of running type checks during commits should be reasonable given these factors. The added safety of catching type errors before commit outweighs the small time increase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the type checking configuration and estimate its impact
# Check the type script definition in package.json
echo "Checking type script configuration:"
cat web/package.json | jq '.scripts.type'
# Check TypeScript configuration
echo -e "\nChecking TypeScript configuration:"
cat web/tsconfig.json 2>/dev/null || echo "tsconfig.json not found"
Length of output: 1110
Script:
#!/bin/bash
# Let's check the size of the TypeScript codebase to better assess the impact
echo "TypeScript files count and size:"
fd -e ts -e tsx . web/src web/e2e 2>/dev/null | wc -l
fd -e ts -e tsx . web/src web/e2e -x du -ch {} + 2>/dev/null | tail -n1
# Check if there are any type-heavy dependencies that might slow down type checking
echo -e "\nDependencies that might affect type checking performance:"
cat web/package.json | jq '.dependencies + .devDependencies | with_entries(select(.key | test("@types|typescript")))'
Length of output: 1127
web/src/beta/features/PluginPlayground/Viewer/index.tsx (3)
6-8: LGTM! Well-structured type definition
Good use of ComponentProps utility to maintain type consistency with the Visualizer component.
10-10: LGTM! Proper TypeScript implementation
The component signature is correctly typed and follows React best practices with prop destructuring.
29-29: Verify Visualizer component prop requirements
The widgets prop is correctly passed down, but let's verify that the Visualizer component doesn't have any other required widget-related props that should be handled.
✅ Verification successful
Based on the code inspection, I can now generate the final response:
The widgets prop is correctly passed and matches the Visualizer component's requirements
The Visualizer component accepts a widgets prop with the following shape:
widgets?: {
floating: (Omit<Widget, "layout" | "extended"> & { extended?: boolean })[];
alignSystem?: WidgetAlignSystem;
ownBuiltinWidgets: string[];
layoutConstraint?: Record<string, WidgetLayoutConstraint>;
}The current usage in the Viewer component correctly extracts and forwards just the widgets prop from the parent Visualizer component's props using ComponentProps type helper, ensuring type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Visualizer component for any related widget props or types
# Search for Visualizer component definition and its prop types
ast-grep --pattern 'type $_ = {
widgets?: $_;
$$$
}'
# Search for any widget-related props usage in Visualizer
rg -A 5 'widgets[^:]*:' --type typescript
Length of output: 137
Script:
#!/bin/bash
# Let's try a different approach to find Visualizer component and its props
# Search for Visualizer component definition and type declarations
ast-grep --pattern 'interface $_ {
widgets$_: $_
$$$
}'
# Also try with type declaration
ast-grep --pattern 'type $_ = {
widgets$_: $_
$$$
}'
# Look for Visualizer component definition
ast-grep --pattern 'const Visualizer = $_'
# Search for widget-related props in tsx files
rg -t tsx 'widgets[^:]*:' -A 5
# Find and check the Visualizer component file
fd Visualizer.tsx
Length of output: 246
Script:
#!/bin/bash
# Let's try one more approach focusing on the file content
# Find the Visualizer component file first
fd -e tsx -e ts Visualizer
# Look for any files containing Visualizer and widgets
rg "Visualizer.*widgets"
# Try to find type definitions or interfaces with widgets
rg "type.*widgets|interface.*widgets" -A 5
# Look for widget-related imports or exports
rg "import.*Visualizer|export.*Visualizer" -A 2
Length of output: 118729
Script:
#!/bin/bash
# Let's try to find the Visualizer component and its type definitions
# Look for files containing Visualizer in the component name
fd -e tsx -e ts "Visualizer"
# Search for Visualizer type definitions or imports
rg "type.*Visualizer|interface.*Visualizer|import.*Visualizer" --type=ts --type=tsx
# Look for the specific file we're reviewing
cat "web/src/beta/features/PluginPlayground/Viewer/index.tsx"
Length of output: 968
Script:
#!/bin/bash
# Let's check the Visualizer component's implementation
# Look for the Visualizer component file
cat "web/src/beta/features/Visualizer/index.tsx"
# Also check for any type definitions in the same directory
fd -e ts -e tsx . "web/src/beta/features/Visualizer/"
# Search for any widget-related types or interfaces
rg -g "*.{ts,tsx}" "type.*widget|interface.*widget" "web/src/beta/features/Visualizer/"
Length of output: 24766
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/index.tsx (2)
2-2: LGTM! Type definitions are well-structured.
The addition of ComputedFeature import and the optional selectedFeature prop maintains type safety while keeping backward compatibility.
Also applies to: 12-12
21-24: Verify hook behavior with undefined selectedFeature.
Please ensure that useEvaluateProperties properly handles the case when selectedFeature is undefined.
✅ Verification successful
Hook properly handles undefined selectedFeature
The implementation of useEvaluateProperties safely handles the case when selectedFeature is undefined:
- The evaluation logic is wrapped in a
if (selectedFeature)check - When
selectedFeatureis undefined, the hook returnsevaluatedPropertieswhich would be undefined initially - The component properly handles this case by showing a template when
evaluatedPropertiesis falsy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the hook implementation to verify undefined handling
ast-grep --pattern 'const useEvaluateProperties = $$$' -f 'useEvaluateProperties.ts'
# Search for tests to verify undefined handling
rg -t ts -t tsx "describe.*useEvaluateProperties" -A 10
Length of output: 348
Script:
#!/bin/bash
# Search for the hook implementation
ast-grep --pattern 'const useEvaluateProperties = $_'
# Search for tests with proper file extensions
rg -t typescript -t javascriptreact -t typescriptreact "describe.*useEvaluateProperties" -A 10
# Search for the hook usage to understand the context
rg -t typescript -t javascriptreact -t typescriptreact "useEvaluateProperties"
Length of output: 301
Script:
#!/bin/bash
# First, let's find the file containing the hook implementation
fd -e ts -e tsx useEvaluateProperties
# Then search for the hook implementation and its usages
rg "useEvaluateProperties" --type-add 'ts:*.{ts,tsx}' -t ts
# Look for test files
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx
Length of output: 1566
Script:
#!/bin/bash
# Let's examine the hook implementation
cat web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts
# And check the component using it for more context
cat web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/index.tsx
Length of output: 3039
web/src/beta/features/PluginPlayground/Code/index.tsx (2)
25-30: LGTM! Clean prop destructuring
The component signature is well-structured and properly typed.
8-9: Verify undefined handling in onChangeSourceCode
The onChangeSourceCode callback accepts undefined as a valid value. Please ensure that parent components properly handle this case to prevent potential runtime issues.
✅ Verification successful
Undefined handling in onChangeSourceCode is properly implemented
The verification shows that onChangeSourceCode is safely handled:
- In
hooks.tsx, when undefined is received, it falls back toselectedFile.sourceCodeusing the nullish coalescing operator (??) - The component is used in a controlled manner with proper value/onChange pattern
- The type definition accurately reflects the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of onChangeSourceCode to verify undefined handling
ast-grep --pattern 'onChangeSourceCode={$_}'
Length of output: 46
Script:
#!/bin/bash
# Search for usages of onChangeSourceCode with surrounding context
rg -l "onChangeSourceCode" --type tsx --type ts
# For each file found, show the content to analyze the handling
rg "onChangeSourceCode" -A 5 -B 5 --type tsx --type ts
Length of output: 158
Script:
#!/bin/bash
# Search for files containing onChangeSourceCode
rg -l "onChangeSourceCode"
# Show context around onChangeSourceCode usage
rg "onChangeSourceCode" -A 5 -B 5
Length of output: 3485
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts (1)
11-11: LGTM! Clean and explicit function signature.
The destructured props pattern makes the hook's dependencies clear and maintainable.
web/src/beta/features/PluginPlayground/hooks.tsx (2)
38-38: LGTM!
The widgets prop is correctly passed to the Viewer component, and the dependency array is properly updated to include widgets.
Also applies to: 41-41
5-5: Verify Code hook integration
Let's verify the integration with the Code hook to ensure it's properly implemented.
Also applies to: 27-29
✅ Verification successful
Let me gather more information about the Code hook implementation.
Let me try a different approach to locate the Code hook implementation.
Let me try one final search to locate the Code hook implementation with a different path pattern.
Based on the comprehensive output showing the implementation of the Code hook and its usage, I can now provide the final response:
Code hook integration is properly implemented
The Code hook is correctly implemented and integrated:
- It provides
widgetsandexecuteCodefunctionality for plugin playground - The hook processes YAML configuration and JavaScript files to create widget instances
- The implementation follows React patterns with proper TypeScript typing
- It's properly used in the parent component with correct prop passing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Code hook implementation and usage
echo "Checking Code hook implementation..."
ast-grep --pattern 'export const useCode = ({ files }) => {
$$$
}'
echo "Checking for other usages of useCode..."
rg "useCode" -A 2
Length of output: 757
Script:
#!/bin/bash
# Check the actual implementation of the Code hook
echo "Checking Code hook implementation in Code/hook.ts(x)..."
fd "hook.tsx?" --full-path "*/Code/" --exec cat {}
echo -e "\nChecking for any test files related to Code hook..."
fd "hook.test" --full-path "*/Code/"
echo -e "\nChecking for any additional files in Code directory..."
fd . --full-path "*/Code/" --type f
Length of output: 779
Script:
#!/bin/bash
# Find the Code directory and its contents
echo "Finding Code directory..."
fd -t d "Code" --max-depth 4
echo -e "\nLooking for hook files..."
fd "hook" -e ts -e tsx --max-depth 5
echo -e "\nChecking imports and exports related to useCode..."
rg -g "*.{ts,tsx}" "export.*useCode|import.*useCode" -B 2 -A 2
Length of output: 837
Script:
#!/bin/bash
# Search for the Code hook implementation in the PluginPlayground directory
echo "Searching for hook implementation in PluginPlayground..."
fd . "web/src/beta/features/PluginPlayground" -t f --exec cat {}
echo -e "\nChecking for any references to widgets or executeCode..."
rg -g "*.{ts,tsx}" "widgets.*executeCode" -B 2 -A 2
Length of output: 25781
web/src/services/api/widgetsApi/utils.ts (2)
62-64: LGTM! Good separation of concerns
The icon assignment logic has been improved by extracting the built-in icon mapping to a separate function while maintaining proper fallback handling.
1-1: Verify the stability of beta module dependency
The import uses @reearth/beta/lib/reearth-ui which might be subject to breaking changes. Consider using a stable version if available.
✅ Verification successful
Beta module dependency is the standard approach
The import from @reearth/beta/lib/reearth-ui is consistent with the codebase's pattern. All components and types from reearth-ui are consistently imported from the beta module across the entire application, with no stable version alternatives found. This is the established pattern in the project, and the import is safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for stable version imports of reearth-ui
rg -g '*.ts' -g '*.tsx' '@reearth/(?!beta).*reearth-ui'
Length of output: 299
Script:
#!/bin/bash
# Search for all reearth-ui imports to understand the usage pattern
rg --pcre2 '@reearth.*reearth-ui' -g '*.ts' -g '*.tsx'
Length of output: 21616
web/src/beta/features/Dashboard/ContentsContainer/RecycleBin/items/RecycleBinItem.tsx (1)
52-52: LGTM! Clean implementation of fallback image handling.
The use of the nullish coalescing operator (??) is appropriate here as it only falls back to the default image when project?.imageUrl is null or undefined.
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/Content.tsx (1)
76-76: LGTM! Consider verifying prop handling.
The addition of selectedFeature prop to CustomFields improves direct access to feature data, which aligns with the PR's objective of streamlining request handling.
Let's verify how CustomFields handles this prop:
✅ Verification successful
Let me analyze the results and generate one more verification to check the actual implementation of the CustomFields component:
Prop handling is properly implemented
The selectedFeature prop is well-handled in the CustomFields component and its hook:
- It's properly typed as an optional prop (
selectedFeature?: ComputedFeature) - The
useEvaluatePropertieshook safely handles the undefined case with proper null checks - When present, the feature data is correctly used to evaluate expressions with all required properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if CustomFields component properly handles the selectedFeature prop
# Look for prop type definitions and null checks
# Search for CustomFields component definition and its prop types
ast-grep --pattern 'type $_ = {
selectedFeature?: $_
$$$
}'
# Search for null checks or default values
rg -A 3 'selectedFeature(\?|\|\|)'
Length of output: 8050
Script:
#!/bin/bash
# Check the implementation of CustomFields component to see how it handles the selectedFeature prop
ast-grep --pattern 'const CustomFields = ({ selectedFeature, $$$ }: Props) => {
$$$
}'
# Also check for any direct usage of selectedFeature within the component
rg -A 5 'selectedFeature[^?]' web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/
Length of output: 4386
web/src/beta/features/Editor/Map/LayerStylePanel/PresetLayerStyle/presetLayerStyles.ts (1)
21-22: Verify the empty 3dtiles style object usage
While adding the "3dtiles" property maintains consistency with other style presets, an empty object might cause issues if the code expects certain default properties to be present (as seen in threeDTilesStyle and colorBuildingsByHeight).
Let's verify how the defaultStyle is used:
✅ Verification successful
Empty 3dtiles style object appears to be safe to add
Based on the search results:
- The
defaultStyleis used in layer style management and is imported inlayer-style.ts - No specific type constraints or interface requirements were found for the 3dtiles style properties
- The type "3dtiles" is already recognized in the layer configuration as seen in
LayerInspector/index.tsx - The empty object serves as a valid initial state, consistent with the pattern of adding new layer type support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for defaultStyle usage to understand the impact
rg -t typescript -t javascript "defaultStyle.*3dtiles" ./web/src
# Check if there are any type definitions that expect specific 3dtiles properties
ast-grep --pattern 'interface $_ {
$$$
"3dtiles": {
$$$
}
$$$
}'
Length of output: 180
Script:
#!/bin/bash
# Search for defaultStyle usage with correct file extensions
rg -t ts -t js "defaultStyle.*3dtiles" ./web/src
# Look for interfaces or types related to 3dtiles styling
rg -t ts -t js "interface.*3dtiles|type.*3dtiles" ./web/src
# Find all usages of defaultStyle
rg -t ts -t js "defaultStyle" ./web/src
# Check for any existing 3dtiles style properties
rg -t ts -t js "3dtiles.*style" ./web/src
Length of output: 939
Script:
#!/bin/bash
# Check LayerAppearanceTypes interface definition
ast-grep --pattern 'interface LayerAppearanceTypes {
$$$
}'
# Look for any 3dtiles-related type definitions
ast-grep --pattern 'type $_ = {
$$$
"3dtiles"
$$$
}'
# Check the implementation of handleLayerStyleAddition
ast-grep --pattern 'function handleLayerStyleAddition($_) {
$$$
}'
Length of output: 195
web/src/beta/lib/reearth-ui/components/TextInput/index.tsx (1)
105-105: LGTM!
The handleKeyDown handler is properly connected to the input element.
web/src/beta/lib/reearth-ui/components/NumberInput/index.tsx (2)
89-94: LGTM! Well-implemented keyboard event handler.
The handleKeyDown implementation is clean and follows React best practices:
- Properly memoized with useCallback
- Correctly typed with KeyboardEvent
- Handles both "Enter" and "Return" keys
- Prevents default behavior before blur
110-110: LGTM! Proper event handler connection.
The onKeyDown event handler is correctly connected to the StyledInput component.
web/src/beta/features/PluginPlayground/Plugins/hook.ts (2)
131-147: LGTM! Well-structured state update logic.
The implementation follows React best practices with proper memoization and maintains consistency with other state update methods in the file.
239-239: LGTM! Consistent export of the new method.
The new method is properly exposed in the hook's return object, maintaining consistency with the existing API.
web/src/beta/lib/reearth-ui/components/PopupMenu/index.tsx (2)
Line range hint 130-140: LGTM: Consistent handling of tile components
The addition of the flex prop based on the presence of tileComponent ensures consistent layout behavior across both link and non-link variants.
327-329: LGTM: Improved layout containment
The addition of width, overflow, and alignment properties ensures proper content containment and consistent vertical alignment.
server/e2e/gql_project_test.go (2)
11-11: LGTM: Clean server initialization
The change to use Server(t) aligns with the PR objective of standardizing server startup across tests.
26-29: LGTM: Consistent request handling pattern
The standardization of GraphQL request handling using the Request helper function improves code maintainability and reduces duplication across test cases.
Also applies to: 51-54, 148-151, 195-195, 253-256, 292-292, 370-373, 410-412, 443-446
web/src/beta/features/Editor/Visualizer/hooks.ts (1)
126-141: 🛠️ Refactor suggestion
Avoid direct mutation of viewerProperty and improve type safety.
The current implementation directly mutates the viewerProperty object and could benefit from improved type safety.
Consider this refactoring:
const viewerProperty = sceneProperty
? (convertData(
sceneProperty,
sceneProperty2ViewerPropertyMapping
) as ViewerProperty)
: undefined;
- if (
- viewerProperty &&
- tempDisableTerrain &&
- viewerProperty.terrain &&
- viewerProperty.terrain.enabled
- ) {
- viewerProperty.terrain.enabled = false;
- }
+ const finalViewerProperty = viewerProperty
+ ? {
+ ...viewerProperty,
+ terrain: viewerProperty.terrain
+ ? {
+ ...viewerProperty.terrain,
+ enabled: tempDisableTerrain ? false : viewerProperty.terrain.enabled,
+ }
+ : undefined,
+ }
+ : undefined;
return {
- viewerProperty,
+ viewerProperty: finalViewerProperty,
cesiumIonAccessToken
};This refactoring:
- Avoids direct mutation by creating a new object
- Preserves type safety
- Simplifies the condition checking
- Maintains immutability principles
Let's check for similar patterns in the codebase:
Also applies to: 143-146
web/src/beta/features/PluginPlayground/Code/hook.ts (1)
34-49: getYmlJson function is well-implemented
The function correctly handles empty file content and parsing errors, providing clear success or failure responses. This ensures robust error handling when processing YAML files.
| sourceCode: `id: demo-widget | ||
| name: Test plugin | ||
| version: 1.0.0 | ||
| extensions: | ||
| - id: demo-widget | ||
| type: widget | ||
| name: Demo Widget | ||
| description: Demo widget | ||
| widgetLayout: | ||
| defaultLocation: | ||
| zone: outer | ||
| section: left | ||
| area: top | ||
| `, |
There was a problem hiding this comment.
💡 Codebase verification
TypeScript interfaces already provide type validation, but runtime validation could be enhanced
The codebase already has TypeScript interfaces (ReearthYML) that define the expected structure of the YAML configuration. While this provides compile-time type checking, the runtime validation is basic (only checking for YAML parsing errors). Consider:
- The current implementation uses
js-yaml.load()with type assertion - Basic error handling exists for YAML parsing errors
- TypeScript interfaces define the expected structure
However, for more robust runtime validation:
- Consider using libraries like
zod,yup, orio-tsto validate the parsed YAML against the schema - This would catch runtime issues like missing required fields or invalid field types
🔗 Analysis chain
Consider adding type validation for the YAML configuration.
The YAML string lacks runtime type validation. Consider using a schema validation library or generating types from a schema to ensure configuration correctness.
Let's check if any schema validation is already in place:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for YAML schema or validation related code
rg -t ts "yaml.*schema|yaml.*validate"
Length of output: 40
Script:
#!/bin/bash
# Let's search for YAML parsing and validation related imports and usage
rg -t ts "import.*yaml|from.*yaml" -A 3
# Also search for any validation related code near YAML usage
rg -t ts "yaml" -A 5 -B 5
Length of output: 7971
Script:
#!/bin/bash
# Let's check for type definitions related to the YAML configuration
ast-grep --pattern 'type ReearthYML = {
$$$
}'
# Also check for any validation functions
rg -t ts "validatePlugin|validateConfig|validate.*yml|validate.*yaml" -A 3
Length of output: 1911
| const { widgets, executeCode } = useCode({ | ||
| files: selectedPlugin.files | ||
| }); |
There was a problem hiding this comment.
Add null check for selectedPlugin
The code assumes selectedPlugin is always defined. This could lead to runtime errors if selectedPlugin is null or undefined.
Consider adding a null check:
- const { widgets, executeCode } = useCode({
- files: selectedPlugin.files
- });
+ const { widgets, executeCode } = useCode({
+ files: selectedPlugin?.files ?? []
+ });📝 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 { widgets, executeCode } = useCode({ | |
| files: selectedPlugin.files | |
| }); | |
| const { widgets, executeCode } = useCode({ | |
| files: selectedPlugin?.files ?? [] | |
| }); |
| // Workaround: Temporarily disable terrain when terrain is enabled | ||
| // We don't know the root cause yet, but it seems that terrain is not loaded properly when terrain is enabled on Editor | ||
| const [tempDisableTerrain, setTempDisableTerrain] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| setTimeout(() => { | ||
| setTempDisableTerrain(false); | ||
| }, 0); | ||
| }, []); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
The setTimeout workaround should be replaced with proper terrain initialization
The codebase shows that terrain handling is a core feature with proper APIs and types defined. The current workaround using setTimeout(0) is addressing a symptom rather than the root cause. Here's why:
-
The codebase has robust terrain-related functionality:
- Proper terrain property types in
web/src/beta/types/sceneProperty.ts - Terrain-related APIs like
getTerrainHeightAsync,moveOverTerrain - Terrain configuration options including exaggeration, depth testing, etc.
- Proper terrain property types in
-
The only other
setTimeout(0)usage in the codebase is in the plugin system's event loop (PluginFrame/hooks.ts), which is a different use case.
Instead of using a timeout workaround, consider:
const [isTerrainReady, setTerrainReady] = useState(false);
useEffect(() => {
if (viewerProperty?.terrain?.enabled) {
// Initialize terrain and set ready state only when initialization is complete
engineRef.initializeTerrain().then(() => setTerrainReady(true));
}
}, [viewerProperty?.terrain?.enabled]);
// Use isTerrainReady state instead of tempDisableTerrain🔗 Analysis chain
Consider a more robust solution for terrain loading issues.
The current workaround has several potential issues:
- Using
setTimeout(0)is an anti-pattern that may not reliably solve the terrain loading problem. - The effect lacks cleanup, which could lead to memory leaks if the component unmounts before the timeout completes.
- There's no tracking to help identify and fix the root cause.
Consider these improvements:
- const [tempDisableTerrain, setTempDisableTerrain] = useState(true);
+ const [tempDisableTerrain, setTempDisableTerrain] = useState(true);
+ const [terrainLoadError, setTerrainLoadError] = useState<Error | null>(null);
useEffect(() => {
- setTimeout(() => {
+ const timeoutId = setTimeout(() => {
setTempDisableTerrain(false);
}, 0);
+
+ return () => clearTimeout(timeoutId);
}, []);
+ // Log terrain load errors for debugging
+ useEffect(() => {
+ if (terrainLoadError) {
+ console.error('Terrain loading failed:', terrainLoadError);
+ // Consider sending to error tracking service
+ }
+ }, [terrainLoadError]);Let's check for similar terrain loading issues in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for terrain-related error handling patterns
rg -g '*.{ts,tsx}' 'terrain.*error|error.*terrain'
# Search for similar setTimeout workarounds
rg -g '*.{ts,tsx}' 'setTimeout.*0.*terrain|terrain.*setTimeout.*0'
Length of output: 121
Script:
#!/bin/bash
# Let's search for terrain-related code and error handling patterns more broadly
rg -g '*.{ts,tsx}' -B 2 -A 2 'terrain' --ignore-case
# Search for any setTimeout with 0ms delay to understand if this is a pattern
rg -g '*.{ts,tsx}' -B 2 -A 2 'setTimeout\([^,]+,\s*0\)'
# Look for terrain-related component implementations
ast-grep --pattern 'const $_ = $$$terrain$$$'
Length of output: 25149
| return { | ||
| ...prv, | ||
| alignSystem: { | ||
| ...prv?.alignSystem, | ||
| [zone]: { | ||
| ...zoneAlignSystem, | ||
| [section]: { | ||
| ...sectionAlignSystem, | ||
| [area]: { | ||
| ...areaAlignSystem, | ||
| widgets: [ | ||
| ...(areaAlignSystem.widgets ?? []), | ||
| { | ||
| id: cur.id, | ||
| __REEARTH_SOURCECODE: file.sourceCode | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize accumulator handling in .reduce to improve performance
Using the spread operator (...) on the accumulator prv inside a .reduce function can lead to O(n^2) time complexity due to repeated copying of the accumulator object on each iteration. This can significantly impact performance with larger datasets.
Consider mutating the accumulator directly to avoid unnecessary copies:
- return {
- ...prv,
- alignSystem: {
- ...prv?.alignSystem,
- [zone]: {
- ...zoneAlignSystem,
- [section]: {
- ...sectionAlignSystem,
- [area]: {
- ...areaAlignSystem,
- widgets: [
- ...(areaAlignSystem.widgets ?? []),
- {
- id: cur.id,
- __REEARTH_SOURCECODE: file.sourceCode
- }
- ]
- }
- }
- }
- }
- };
+ if (!prv.alignSystem[zone]) {
+ prv.alignSystem[zone] = {};
+ }
+ if (!prv.alignSystem[zone][section]) {
+ prv.alignSystem[zone][section] = {};
+ }
+ if (!prv.alignSystem[zone][section][area]) {
+ prv.alignSystem[zone][section][area] = { widgets: [] };
+ }
+ prv.alignSystem[zone][section][area].widgets.push({
+ id: cur.id,
+ __REEARTH_SOURCECODE: file.sourceCode
+ });
+ return prv;By directly mutating the accumulator, you avoid the overhead of copying the entire object at each iteration, thus improving performance.
Alternatively, if immutability is required for state management considerations, consider using libraries like Immer that optimize performance while maintaining immutability.
📝 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.
| return { | |
| ...prv, | |
| alignSystem: { | |
| ...prv?.alignSystem, | |
| [zone]: { | |
| ...zoneAlignSystem, | |
| [section]: { | |
| ...sectionAlignSystem, | |
| [area]: { | |
| ...areaAlignSystem, | |
| widgets: [ | |
| ...(areaAlignSystem.widgets ?? []), | |
| { | |
| id: cur.id, | |
| __REEARTH_SOURCECODE: file.sourceCode | |
| } | |
| ] | |
| } | |
| } | |
| } | |
| } | |
| }; | |
| }, | |
| if (!prv.alignSystem[zone]) { | |
| prv.alignSystem[zone] = {}; | |
| } | |
| if (!prv.alignSystem[zone][section]) { | |
| prv.alignSystem[zone][section] = {}; | |
| } | |
| if (!prv.alignSystem[zone][section][area]) { | |
| prv.alignSystem[zone][section][area] = { widgets: [] }; | |
| } | |
| prv.alignSystem[zone][section][area].widgets.push({ | |
| id: cur.id, | |
| __REEARTH_SOURCECODE: file.sourceCode | |
| }); | |
| return prv; |
🧰 Tools
🪛 Biome
[error] 96-96: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
hexaforce
force-pushed
the
chore/refactoring-test-code
branch
from
46d411a to
4798c8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
server/e2e/gql_user_test.go (4)
75-84: Comprehensive test coverage with both positive and negative scenariosGood implementation testing both successful and unsuccessful search cases.
Remove the unnecessary empty line at line 76 for better code consistency.
89-91: Consider adding negative test casesWhile the current test verifies successful node retrieval, consider adding test cases for:
- Invalid node ID
- Invalid node type
- Non-existent node
96-98: Consider expanding test coverageWhile the current test verifies successful nodes retrieval, consider adding test cases for:
- Multiple nodes
- Empty result set
- Invalid node types
The commented-out test case should be restored using the new testing patterns
The verification shows that:
- The signup test is the only test covering this mutation
- The codebase has moved to a new standardized testing pattern using
ServerUser()andRequestQuery()- Other user-related tests (TestUpdateMe, TestSearchUser, etc.) have been successfully migrated to use these new patterns
The test should be uncommented and updated to use the new pattern:
func TestSignUp(t *testing.T) { e, _ := ServerUser(t) query := `mutation { signup(input: {lang: "ja",theme: DEFAULT,secret: "Ajsownndww1"}){ user{ id name email } }}` o := RequestQuery(t, e, query, uId1.String()). Object().Value("data").Object().Value("signup").Object().Value("user").Object() o.Value("name").String().Equal("updated") }🔗 Analysis chain
Line range hint
13-33: Verify the removal of TestSignUp test caseThe TestSignUp test case has been commented out. This could reduce test coverage for the signup functionality.
Consider either:
- Restoring this test case using the new
ServerUserandRequestQuerypattern, or- Documenting why this test case is no longer needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if signup test coverage exists elsewhere rg -l "TestSignUp|signup.*mutation" --type goLength of output: 75
Script:
#!/bin/bash # Let's check if there are any other signup-related tests or if the functionality has been moved elsewhere rg "signup.*mutation|mutation.*signup" -A 5 --type goLength of output: 459
Script:
#!/bin/bash # Let's also check for any recent changes or additions to user-related tests fd -e go -e graphql -e gql . | xargs rg "test.*user|user.*test" -iLength of output: 785
Script:
#!/bin/bash # Check if there are any new test patterns or frameworks being used rg "ServerUser|RequestQuery" -A 5 --type goLength of output: 12543
server/e2e/common.go (1)
62-64: Make gateway URL configurableThe gateway URL is hardcoded to "https://example.com". Consider making this configurable through the config parameter to support different test environments.
gateways := &gateway.Container{ - File: lo.Must(fs.NewFile(afero.NewMemMapFs(), "https://example.com")), + File: lo.Must(fs.NewFile(afero.NewMemMapFs(), cfg.GatewayURL)), }server/e2e/gql_project_import_test.go (1)
Line range hint
52-63: Add test coverage for GraphQL error scenariosThe
getScenefunction now uses a centralizedRequestfunction, which is good. However, based on the retrieved learnings about previous GraphQL assertion issues, we should add test cases for:
- Invalid scene IDs
- Network errors
- GraphQL errors
Consider adding test cases like:
func TestGetSceneErrors(t *testing.T) { e := Server(t) // Test invalid scene ID invalidID := "invalid-id" r := getScene(e, invalidID, language.English.String()) r.Value("errors").Array().NotEmpty() // Add more error scenarios... }server/e2e/gql_project_test.go (1)
Line range hint
234-260: Consider moving helper functions to common.goThe helper functions (
starProject,deleteProject) are well-implemented but could be moved tocommon.gofor reuse across test files. Also consider:
- Adding error return values for better error handling
- Adding documentation for the expected behavior
Example refactor for
deleteProject:-func deleteProject(e *httpexpect.Expect, projectID string) { +// deleteProject marks a project as deleted and verifies the operation +func deleteProject(e *httpexpect.Expect, projectID string) error { updateProjectMutation := GraphQLRequest{ // ... existing code ... } response := Request(e, uID.String(), updateProjectMutation) + if response.Raw().Status != 200 { + return fmt.Errorf("failed to delete project: %s", projectID) + } // ... rest of the code ... + return nil }Also applies to: 421-450
server/e2e/seeder.go (5)
40-45: Follow Go naming conventions for variable namesAccording to Go conventions, acronyms and initialisms should be capitalized. Consider renaming
uId1,uId2, anduId3touID1,uID2, anduID3, and similarly forwId1,wId2, andiId1towID1,wID2, andiID1for clarity and consistency.
135-162: Refactor user creation to eliminate code duplicationThe creation of users
u,u2, andu3involves repetitive code. Consider extracting a helper function that accepts parameters like ID, name, workspace ID, email, and other attributes. This will reduce code duplication and enhance maintainability.
163-170: Combine role definitions into a helper functionThe role definitions for
roleOwnerandroleReaderare similar in structure. Consider creating a helper function to generateworkspace.Memberinstances based on the role and inviter. This will make the code more concise and easier to extend.
172-198: Refactor workspace creation to eliminate code duplicationThe code for creating workspaces
wandw2shares a similar pattern. Consider extracting a helper function for workspace creation that accepts parameters such as ID, name, members, and integrations. This will improve code readability and reduce duplication.
135-162: Ensure workspaces exist before assigning them to usersUsers are being created with workspace IDs (
wId1,wId2) that may not have been persisted yet. While this might not cause issues in the test environment, it's generally good practice to create and save workspaces before assigning them to users to ensure data integrity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
server/e2e/common.go(4 hunks)server/e2e/dataset_export_test.go(1 hunks)server/e2e/gql_featureCollection_test.go(3 hunks)server/e2e/gql_layer_test.go(1 hunks)server/e2e/gql_me_test.go(1 hunks)server/e2e/gql_nlslayer_test.go(13 hunks)server/e2e/gql_project_export_test.go(3 hunks)server/e2e/gql_project_import_test.go(4 hunks)server/e2e/gql_project_test.go(13 hunks)server/e2e/gql_property_test.go(1 hunks)server/e2e/gql_scene_test.go(2 hunks)server/e2e/gql_storytelling_test.go(18 hunks)server/e2e/gql_style_test.go(5 hunks)server/e2e/gql_user_test.go(1 hunks)server/e2e/gql_workspace_test.go(1 hunks)server/e2e/mock_test.go(1 hunks)server/e2e/seeder.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- server/e2e/dataset_export_test.go
- server/e2e/gql_featureCollection_test.go
- server/e2e/gql_layer_test.go
- server/e2e/gql_me_test.go
- server/e2e/gql_nlslayer_test.go
- server/e2e/gql_project_export_test.go
- server/e2e/gql_property_test.go
- server/e2e/gql_scene_test.go
- server/e2e/gql_storytelling_test.go
- server/e2e/gql_style_test.go
- server/e2e/gql_workspace_test.go
- server/e2e/mock_test.go
🧰 Additional context used
📓 Learnings (1)
server/e2e/gql_project_import_test.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (10)
server/e2e/gql_user_test.go (3)
38-40: Clean refactoring using new helper functions
Good use of the new ServerUser and RequestQuery helpers, making the test more concise while maintaining the same functionality.
48-54: Well-structured test with proper state verification
Good implementation that verifies both the API response and the actual database state changes.
62-68: Consistent implementation with proper error verification
Good implementation that follows the established pattern and properly verifies the error case.
server/e2e/gql_project_import_test.go (3)
Line range hint 65-428: Well-structured GraphQL query with comprehensive coverage
The GraphQL query is well-organized with clear fragment separation and proper handling of:
- Translations through language parameters
- Complex nested data structures
- Type-specific fields
Line range hint 32-50: Verify multipart request handling for file uploads
The function now uses a centralized RequestWithMultipart function, which is good for maintainability. However, we should ensure proper handling of:
- Large file uploads
- Invalid file formats
- File read errors
Let's check the implementation of RequestWithMultipart:
✅ Verification successful
Multipart request handling is properly implemented with size limits
The implementation is secure and handles file uploads appropriately:
RequestWithMultipartinserver/e2e/common.gocorrectly implements multipart file upload with proper headers and form fields- File size validation is implemented in both S3 and GCS storage backends with a 100MB limit
- Files are sanitized before storage (using
sanitize.Path) - Error handling exists for oversized files (
gateway.ErrFileTooLarge)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RequestWithMultipart implementation
ast-grep --pattern 'func RequestWithMultipart($params) $ret {
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Let's try searching for RequestWithMultipart using ripgrep for a broader search
rg -A 10 "RequestWithMultipart"
Length of output: 1604
Script:
#!/bin/bash
# Let's check for any error handling tests related to file uploads
rg -B 2 -A 5 "TestImportProject.*Error"
# Also check for any size limits or file format validations in the project
rg -A 5 "fileSize|maxSize|allowedFormat|validation" server/
Length of output: 1667
15-17: Verify error handling in simplified server initialization
The server initialization has been simplified, which improves readability. However, we should ensure that error scenarios are still properly handled.
Let's check how errors are handled in the new Server function:
✅ Verification successful
Error handling is properly maintained in the simplified initialization
The simplified server initialization is safe and maintains proper error handling through multiple layers:
Server(t)usesStartServerwhich internally usesStartServerAndReposandStartServerWithRepos- The test helper functions properly handle errors and use
t.Helper()for accurate test failure reporting - The
importProjectfunction returns anhttpexpect.Objectthat allows for assertion chaining and error checking in tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the Server function implementation and its error handling
ast-grep --pattern 'func Server($params) $ret {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Let's try a different approach to find the Server function
# First, let's locate the file containing the Server function
rg -l "func Server" --type go
# Then, let's examine the Server function implementation with context
rg "func Server" --type go -B 2 -A 10
Length of output: 2241
Script:
#!/bin/bash
# Let's examine the StartServer function to understand the complete error handling flow
rg "func StartServer" --type go -B 2 -A 10
# Also check the importProject function to verify its error handling
rg "func importProject" --type go -B 2 -A 10
Length of output: 2423
server/e2e/gql_project_test.go (4)
Line range hint 195-224: Well-structured test assertions
The test assertions are comprehensive and well-organized, effectively validating:
- Response structure
- Data integrity
- Business logic (e.g., starred projects handling)
Line range hint 1-450: Overall improvements look good
The refactoring successfully:
- Standardizes server initialization
- Consolidates request handling
- Maintains comprehensive test coverage
- Improves code maintainability
The changes align well with the PR objectives to standardize the server startup and request calls.
11-11: Verify server configuration in common.go
The server initialization has been standardized using Server(t), which aligns with the PR objectives. However, we should verify that the common configuration meets all test requirements.
Also applies to: 63-63, 162-162, 263-263, 382-382
✅ Verification successful
Server initialization is properly configured and standardized
The Server(t) function is a standardized test helper that initializes a test server with consistent configuration:
- Uses MongoDB for data persistence
- Configures CORS with origin "https://example.com"
- Disables authentication service
- Seeds test data using
baseSeederwhich sets up:- Test user with workspace
- Required workspace permissions
- Common test data setup
The standardization aligns with the PR objectives and provides consistent test environment setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Server() implementation and its configuration
ast-grep --pattern 'func Server($t testing.T) $_ {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Let's try a different approach to find the Server function
# First, let's check the common.go file in the same directory
fd "common.go" --type f --exec cat {} \;
# Also search for Server function in all go files in the e2e directory
fd ".*\.go$" --type f -e go -p "server/e2e" --exec grep -l "func Server" {} \;
# Get the content of these files to analyze the Server implementation
fd ".*\.go$" --type f -e go -p "server/e2e" --exec grep -A 10 "func Server" {} \;
Length of output: 32892
Script:
#!/bin/bash
# Let's check the baseSeeder implementation
fd ".*\.go$" --type f -e go -p "server/e2e" --exec grep -A 20 "baseSeeder" {} \;
# Also check StartServer implementation to understand the server configuration
fd ".*\.go$" --type f -e go -p "server/e2e" --exec grep -A 20 "func StartServer" {} \;
Length of output: 9243
26-29: Verify error handling in Request function
The request handling has been standardized using the new Request() function. We should verify:
- Error handling for failed requests
- Header management
- Response validation
Also applies to: 148-151, 195-195, 253-256, 370-373, 410-412, 443-446
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Request function implementation is properly handling basic HTTP requirements
The Request function in server/e2e/common.go is a test helper that:
- Sets all required headers (Origin, Authorization, Content-Type)
- Validates HTTP status (expects 200 OK)
- Handles JSON request/response properly
- Is consistently used across test cases
However, since this is a test helper function, error cases are expected to be handled in individual test cases rather than in the Request function itself. The function is working as intended by enforcing a successful response status (200 OK) for the happy path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Request() implementation
ast-grep --pattern 'func Request($e httpexpect.Expect, $token string, $body GraphQLRequest) $_ {
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try to find the Request function with a more flexible pattern
ast-grep --pattern 'func Request($$$) $_ {
$$$
}'
# Also search for the file containing the Request function
rg -l "func Request"
Length of output: 760
Script:
#!/bin/bash
# Search for error handling patterns in test files
rg "Request.*Expect.*Error|ExpectStatus" server/e2e/
Length of output: 54
Script:
#!/bin/bash
# Look for the actual implementation of Request function
rg -A 10 "^func Request" server/e2e/
Length of output: 2025
Script:
#!/bin/bash
# Let's check how these Request functions are used in test files, particularly for error cases
rg -B 2 -A 2 "Request\(" server/e2e/gql_project_test.go
# Also check for any error expectations in the test file
rg "ExpectStatus|ExpectError|Object\(\)\.Value\(\"errors\"\)" server/e2e/gql_project_test.go
Length of output: 1152
| func RequestQuery(t *testing.T, e *httpexpect.Expect, query string, user string) *httpexpect.Value { | ||
| request := GraphQLRequest{ | ||
| Query: query, | ||
| } | ||
| jsonData, err := json.Marshal(request) | ||
| assert.Nil(t, err) | ||
| return e.POST("/api/graphql"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("Content-Type", "application/json"). | ||
| WithHeader("X-Reearth-Debug-User", user). | ||
| WithBytes(jsonData). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize header usage across request functions
The headers in RequestQuery are inconsistent with those in Request. The Origin header is missing, while other functions include it.
return e.POST("/api/graphql").
+ WithHeader("Origin", "https://example.com").
WithHeader("authorization", "Bearer test").
WithHeader("Content-Type", "application/json").
WithHeader("X-Reearth-Debug-User", user).📝 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.
| func RequestQuery(t *testing.T, e *httpexpect.Expect, query string, user string) *httpexpect.Value { | |
| request := GraphQLRequest{ | |
| Query: query, | |
| } | |
| jsonData, err := json.Marshal(request) | |
| assert.Nil(t, err) | |
| return e.POST("/api/graphql"). | |
| WithHeader("authorization", "Bearer test"). | |
| WithHeader("Content-Type", "application/json"). | |
| WithHeader("X-Reearth-Debug-User", user). | |
| WithBytes(jsonData). | |
| Expect(). | |
| Status(http.StatusOK). | |
| JSON() | |
| } | |
| func RequestQuery(t *testing.T, e *httpexpect.Expect, query string, user string) *httpexpect.Value { | |
| request := GraphQLRequest{ | |
| Query: query, | |
| } | |
| jsonData, err := json.Marshal(request) | |
| assert.Nil(t, err) | |
| return e.POST("/api/graphql"). | |
| WithHeader("Origin", "https://example.com"). | |
| WithHeader("authorization", "Bearer test"). | |
| WithHeader("Content-Type", "application/json"). | |
| WithHeader("X-Reearth-Debug-User", user). | |
| WithBytes(jsonData). | |
| Expect(). | |
| Status(http.StatusOK). | |
| JSON() | |
| } |
| func RequestWithMultipart(e *httpexpect.Expect, user string, requestBody map[string]interface{}, filePath string) *httpexpect.Value { | ||
| return e.POST("/api/graphql"). | ||
| WithHeader("Origin", "https://example.com"). | ||
| WithHeader("authorization", "Bearer test"). | ||
| WithHeader("X-Reearth-Debug-User", user). | ||
| WithMultipart(). | ||
| WithFormField("operations", toJSONString(requestBody)). | ||
| WithFormField("map", `{"0": ["variables.file"]}`). | ||
| WithFile("0", filePath). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for multipart request
The RequestWithMultipart function should handle potential errors from toJSONString and file operations.
-func RequestWithMultipart(e *httpexpect.Expect, user string, requestBody map[string]interface{}, filePath string) *httpexpect.Value {
+func RequestWithMultipart(e *httpexpect.Expect, user string, requestBody map[string]interface{}, filePath string) (*httpexpect.Value, error) {
+ jsonStr, err := toJSONString(requestBody)
+ if err != nil {
+ return nil, fmt.Errorf("failed to marshal request body: %w", err)
+ }
+
+ if _, err := os.Stat(filePath); err != nil {
+ return nil, fmt.Errorf("failed to access file: %w", err)
+ }
+
return e.POST("/api/graphql").
WithHeader("Origin", "https://example.com").
WithHeader("authorization", "Bearer test").
WithHeader("X-Reearth-Debug-User", user).
WithMultipart().
- WithFormField("operations", toJSONString(requestBody)).
+ WithFormField("operations", jsonStr).
WithFormField("map", `{"0": ["variables.file"]}`).
WithFile("0", filePath).
Expect().
Status(http.StatusOK).
- JSON()
+ JSON(), nil
}📝 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.
| func RequestWithMultipart(e *httpexpect.Expect, user string, requestBody map[string]interface{}, filePath string) *httpexpect.Value { | |
| return e.POST("/api/graphql"). | |
| WithHeader("Origin", "https://example.com"). | |
| WithHeader("authorization", "Bearer test"). | |
| WithHeader("X-Reearth-Debug-User", user). | |
| WithMultipart(). | |
| WithFormField("operations", toJSONString(requestBody)). | |
| WithFormField("map", `{"0": ["variables.file"]}`). | |
| WithFile("0", filePath). | |
| Expect(). | |
| Status(http.StatusOK). | |
| JSON() | |
| } | |
| func RequestWithMultipart(e *httpexpect.Expect, user string, requestBody map[string]interface{}, filePath string) (*httpexpect.Value, error) { | |
| jsonStr, err := toJSONString(requestBody) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to marshal request body: %w", err) | |
| } | |
| if _, err := os.Stat(filePath); err != nil { | |
| return nil, fmt.Errorf("failed to access file: %w", err) | |
| } | |
| return e.POST("/api/graphql"). | |
| WithHeader("Origin", "https://example.com"). | |
| WithHeader("authorization", "Bearer test"). | |
| WithHeader("X-Reearth-Debug-User", user). | |
| WithMultipart(). | |
| WithFormField("operations", jsonStr). | |
| WithFormField("map", `{"0": ["variables.file"]}`). | |
| WithFile("0", filePath). | |
| Expect(). | |
| Status(http.StatusOK). | |
| JSON(), nil | |
| } |
Overview
In the e2e test code, I standardized the server startup and request calls.
What I've done
Main changes:
1.Consolidated common processes into server/e2e/common.go.
Server
Request
2.Grouped individual seeders into server/e2e/seeder.go.
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Refactor
Requestfunction across multiple test files.Bug Fixes
Documentation