feat(server): add photooverlay property [VIZ-1340] #1462
Conversation
WalkthroughThe pull request introduces a new logging function and updates several E2E test functions to return additional property identifiers. A typographical error in a GraphQL constant has been corrected. The GraphQL schema and related adapter code have been enhanced to support a new "PhotoOverlay" type, including resolver functions, model definitions, and enum updates. Additional tests and extensions have been added to validate these changes, and a dependency version has been updated in the project’s go.mod file. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as GraphQL Endpoint
participant R as Resolver
C->>G: Sends query with PhotoOverlay fields
G->>R: Resolves PhotoOverlay (enabled, cameraDuration)
R-->>G: Returns PhotoOverlay data
G-->>C: Responds with PhotoOverlay information
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 2
🧹 Nitpick comments (6)
server/e2e/common.go (1)
192-192: Consider enabling the debugging utility when needed.This commented-out line could be helpful during development, but should remain commented in production code. If you're having issues with specific GraphQL requests, you can uncomment this line temporarily for debugging.
server/e2e/gql_nlslayer_test.go (1)
741-741: Consider utilizing returned values.The function call ignores all returned values. Since the propertyId is now being returned and might be useful for assertions or further operations, consider storing it in a variable for potential use.
- _, _, _, _ = createInfobox(e, layerId) + _, _, _, propertyId := createInfobox(e, layerId)server/e2e/gql_storytelling_test.go (2)
166-168: Consider utilizing the returned property IDs.The function calls ignore the returned property ID (the fourth return value). Since these property IDs might be useful for assertions or further operations, consider storing them in variables for potential use.
- _, _, blockIDa, _ := createBlock(e, sceneID, storyID1, pageID1, "reearth", "textStoryBlock", lo.ToPtr(1)) - _, _, blockIDb, _ := createBlock(e, sceneID, storyID1, pageID1, "reearth", "textStoryBlock", lo.ToPtr(2)) + _, _, blockIDa, propIDa := createBlock(e, sceneID, storyID1, pageID1, "reearth", "textStoryBlock", lo.ToPtr(1)) + _, _, blockIDb, propIDb := createBlock(e, sceneID, storyID1, pageID1, "reearth", "textStoryBlock", lo.ToPtr(2))
187-187: Consider utilizing the returned property ID.Similar to the above comment, consider storing the returned property ID for potential use in assertions or further operations.
- _, _, blockID3, _ := createBlock(e, sceneID, storyID1, pageID1, "reearth", "textStoryBlock", lo.ToPtr(3)) + _, _, blockID3, propID3 := createBlock(e, sceneID, storyID1, pageID1, "reearth", "textStoryBlock", lo.ToPtr(3))server/e2e/gql_property_test.go (2)
161-167: Add missing test coverage.
We see aTODOcomment to “Write detailed tests,” but there is no actual logic here. This might leave parts of the scene property untested.Shall I help provide a more comprehensive test to improve coverage for
TestUpdateScenePropertyValue?
197-198: Remove unnecessary commented code.
The commented-out "height" and "aspectRatio" fields remain in the camera object. If they are not needed, it’s best to remove them for clarity.- // "height": 313.66307524391044, - // "aspectRatio": 0.42407108239095315,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
server/e2e/common.go(2 hunks)server/e2e/gql_nlslayer_test.go(5 hunks)server/e2e/gql_project_export_import_test.go(2 hunks)server/e2e/gql_property_test.go(2 hunks)server/e2e/gql_storytelling_test.go(4 hunks)server/e2e/seeder.go(2 hunks)server/go.mod(1 hunks)server/gql/_shared.graphql(2 hunks)server/gql/property.graphql(1 hunks)server/internal/adapter/gql/generated.go(7 hunks)server/internal/adapter/gql/gqlmodel/convert_property.go(2 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/pkg/builtin/manifest.yml(1 hunks)server/pkg/property/value.go(1 hunks)server/pkg/property/value_photooverlay.go(1 hunks)server/schemas/plugin_manifest.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- server/e2e/gql_project_export_import_test.go
- server/go.mod
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: deploy-server / deploy_test
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (31)
server/e2e/seeder.go (2)
49-49: Global variable addition aligns with consistent NLS layer ID tracking.This new global variable provides a consistent ID for NLS layers across the e2e tests, which helps with predictable test behavior.
408-408: Method signature update to use consistent ID for layer creation.Changed from using a newly generated ID to using the predefined
nlsLayerIdglobal variable. This provides consistency in tests where layer identity is important.server/pkg/builtin/manifest.yml (1)
2712-2714: PhotoOverlay property added to infobox-beta schema.This addition aligns with the PR objectives to introduce a new PhotoOverlay property to the infobox-beta component. The structure follows the established pattern used by other properties in the manifest.
server/gql/property.graphql (1)
195-195: New PHOTOOVERLAY enum value added to ValueType.This addition properly extends the GraphQL schema to support the new PhotoOverlay type, maintaining alphabetical ordering in the enum list which follows good practices.
server/e2e/common.go (1)
279-283: Good debugging utility function added.This function provides a clean way to inspect GraphQL requests, which can be valuable during development and troubleshooting. The indent formatting makes the output more readable.
server/internal/adapter/gql/gqlmodel/convert_property.go (2)
53-57: Implementation looks correct for the PhotoOverlay conversion.The conversion from property.PhotoOverlay to GraphQL model is correctly implemented, handling both the Enabled and CameraDuration fields.
122-126: Implementation looks correct for the reverse PhotoOverlay conversion.The conversion from GraphQL model back to property.PhotoOverlay is properly implemented, maintaining symmetry with the corresponding ToPropertyValue implementation.
server/schemas/plugin_manifest.json (1)
37-38: Added "photooverlay" to valuetype enum correctly.The photooverlay type has been correctly added to the valuetype enum in the JSON schema, allowing it to be used in plugin manifests.
server/gql/_shared.graphql (1)
103-106: PhotoOverlay type definition looks good.The GraphQL type definition for PhotoOverlay properly defines the two fields needed:
enabled: Boolean - to control if the overlay is activecameraDuration: Int - to specify the duration for camera transitionsThis aligns with the PR objective to add a PhotoOverlay property to the infobox-beta component.
server/pkg/property/value.go (1)
37-37:✅ Verification successful
Properly registered PhotoOverlay type handler.
The PhotoOverlay type has been correctly registered in the types map to enable property value conversion and validation for this new type.
Let's verify the implementation of the PhotoOverlay type handler:
🏁 Script executed:
#!/bin/bash # Check for the PhotoOverlay implementation file find server/pkg/property -name "*photooverlay*" -type f # If found, display the content to verify implementation find server/pkg/property -name "*photooverlay*" -type f -exec cat {} \;Length of output: 1783
Properly Registered PhotoOverlay Type Handler
The PhotoOverlay type is correctly registered and implemented. The registration in
server/pkg/property/value.gomatches the implementation inserver/pkg/property/value_photooverlay.go, where all conversion, validation, and JSON schema methods (I2V, V2I, Validate, String, and JSONSchema) are properly defined. No additional changes are required.server/internal/adapter/gql/gqlmodel/models_gen.go (4)
761-764: New struct implementation looks good.The implementation of the PhotoOverlay struct with appropriate field types (pointers to bool and int) follows the project's pattern for similar types. The use of pointers allows for nil values when these properties aren't set.
2266-2266: Enum value added correctly.The ValueTypePhotooverlay enum value follows the naming convention of existing enum values in the ValueType type.
2285-2285: AllValueType array updated appropriately.The new ValueTypePhotooverlay has been correctly added to the AllValueType array to ensure it's included in validation checks.
2290-2291: IsValid method properly updated.The IsValid method has been correctly updated to include the new ValueTypePhotooverlay in its validation check.
server/e2e/gql_nlslayer_test.go (3)
502-534: Function signature update aligns with API changes.The createInfobox function now properly returns the propertyId from the GraphQL response, which is necessary for testing the PhotoOverlay property. This change is consistent with the PR objectives of adding PhotoOverlay as a property.
781-837: Great test implementation for the new PhotoOverlay property.The TestInfoboxProperty function provides comprehensive coverage for testing the new PhotoOverlay property alongside other existing properties. The test correctly verifies that the PhotoOverlay properties (enabled and cameraDuration) can be set and are correctly returned in the response.
828-836: Well-structured PhotoOverlay test case.The test correctly sets both the enabled flag and cameraDuration properties, matching the PhotoOverlay struct definition. The assertions verify that the values are properly persisted.
server/e2e/gql_storytelling_test.go (2)
824-864: Improved query formatting increases readability.The GraphQL query has been reformatted with proper indentation and line breaks, making it more readable while maintaining functionality. The query now also includes the property.id field, which is used to extract the property ID in the return value.
883-886: Function return value enhanced to include property ID.The function now correctly extracts and returns the block's property ID from the GraphQL response, which is aligned with similar changes in other parts of the codebase. This enables testing property-related functionality for story blocks.
server/internal/adapter/gql/generated.go (9)
566-569: New PhotoOverlay complexity struct looks good.This correctly adds the complexity calculation functions for the new PhotoOverlay type, defining both
CameraDurationandEnabledfields that will be used to determine query complexity.
4022-4035: Complexity handling for PhotoOverlay fields is implemented correctly.The implementation follows the established pattern in the codebase for handling field complexity. The conditional checks ensure that no errors occur if complexity functions aren't defined.
6942-6945: PhotoOverlay type definition is properly structured.The GraphQL type definition includes the necessary fields as mentioned in the PR objectives:
enabled: Boolean field to toggle the overlaycameraDuration: Int field to specify the durationThis matches the requirements from the PR description.
7945-7945: ValueType enum correctly updated with PHOTOOVERLAY.This addition to the ValueType enum properly identifies PhotoOverlay as a valid value type in the GraphQL schema, which is consistent with the PR objectives.
29034-29060: Resolver for PhotoOverlay.enabled looks good.The implementation follows the standard resolver pattern in the codebase, including proper error handling, context management, and null value handling.
29075-29101: Resolver for PhotoOverlay.cameraDuration is properly implemented.The implementation correctly extracts and returns the cameraDuration field from PhotoOverlay objects, with appropriate error handling and type marshaling.
57669-57705: PhotoOverlay GraphQL type integration is complete.The implementation properly:
- Defines PhotoOverlay implementors
- Creates the handler function for PhotoOverlay objects
- Handles field selection and dispatching
- Integrates with the deferred resolution system
This follows the established pattern for type handlers in the codebase.
29062-29073: Field context functions are properly implemented.Both field context functions for the PhotoOverlay type follow the standard pattern in the codebase, correctly setting up the field context with appropriate metadata and child field handlers.
Also applies to: 29102-29114
6881-6884:✅ Verification successful
Note about Pagination input type.
This input type definition appears to be part of the generated code but doesn't seem directly related to the PhotoOverlay feature. Ensure this change is intentional and needed for the PR.
🏁 Script executed:
#!/bin/bash # Check if Pagination input was newly added or was already present in the schema grep -r "input Pagination" --include="*.graphql" .Length of output: 100
Pagination input type inclusion is expected
The
Paginationinput type in the generated file comes from the shared schema (server/gql/_shared.graphql) as confirmed by the grep output. Its appearance isn’t a new addition related to the PhotoOverlay feature but rather an expected part of the generated code. No action is required unless the PhotoOverlay feature specifically needs a modification of this type.server/e2e/gql_property_test.go (3)
121-156: Potential panic risk for array indexing.
InaddPropertyItem(), lines 152–153 rely onitems[0]andgroups[0]without any defensive check. If the GraphQL response array is empty, this could cause an out-of-range error.Would you like to add guards to verify the existence of items and groups before attempting to index them?
210-213: Consider validating cameraDuration values.
Adding a large or negativecameraDurationcould result in undesired behavior. Consider constraints or validation to ensure realistic values.
214-217: Verify date/time format compatibility.
You are passing a date string"2025-03-06T11:05:44+09:00"for thetimePoint. Confirm that the backend correctly parses and handles this format.
| func (c *PhotoOverlay) Clone() *PhotoOverlay { | ||
| if c == nil { | ||
| return nil | ||
| } | ||
| return &PhotoOverlay{ | ||
| Enabled: c.Enabled, | ||
| CameraDuration: c.CameraDuration, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use true deep copy for pointer fields.
In (*PhotoOverlay).Clone(), assigning the same pointers to Enabled and CameraDuration does not create a true deep copy. Mutating the copy could unexpectedly affect the original.
Consider this approach:
func (c *PhotoOverlay) Clone() *PhotoOverlay {
if c == nil {
return nil
}
enabledCopy := lo.ClonePtr(c.Enabled) // or manually copy
cameraDurationCopy := lo.ClonePtr(c.CameraDuration)
return &PhotoOverlay{
- Enabled: c.Enabled,
- CameraDuration: c.CameraDuration,
+ Enabled: enabledCopy,
+ CameraDuration: cameraDurationCopy,
}
}📝 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 (c *PhotoOverlay) Clone() *PhotoOverlay { | |
| if c == nil { | |
| return nil | |
| } | |
| return &PhotoOverlay{ | |
| Enabled: c.Enabled, | |
| CameraDuration: c.CameraDuration, | |
| } | |
| } | |
| func (c *PhotoOverlay) Clone() *PhotoOverlay { | |
| if c == nil { | |
| return nil | |
| } | |
| enabledCopy := lo.ClonePtr(c.Enabled) // or manually copy | |
| cameraDurationCopy := lo.ClonePtr(c.CameraDuration) | |
| return &PhotoOverlay{ | |
| Enabled: enabledCopy, | |
| CameraDuration: cameraDurationCopy, | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
server/e2e/gql_workspace_test.go(1 hunks)server/go.mod(1 hunks)server/pkg/property/value_photooverlay.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/go.mod
- server/pkg/property/value_photooverlay.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / ci-server-test
hexaforce
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
2268-2268: Ensure consistent naming convention with other value types.While the implementation is correct, I notice that the constant name uses lowercase 'o' in "Photooverlay" while the struct type uses uppercase 'O' in "PhotoOverlay". This is a minor inconsistency, but it's worth checking if this aligns with the project's naming conventions for other similar value types.
-ValueTypePhotooverlay ValueType = "PHOTOOVERLAY" +ValueTypePhotoOverlay ValueType = "PHOTOOVERLAY"
761-764: Consider adding documentation for the PhotoOverlay type and its fields.To improve maintainability, it would be beneficial to add documentation comments explaining the purpose of the PhotoOverlay type and the meaning of its fields. In particular, documentation about the expected range of values for CameraDuration would be helpful for future developers.
+// PhotoOverlay represents configuration for photo overlay functionality in the infobox-beta component. type PhotoOverlay struct { + // Enabled determines whether the photo overlay feature is active Enabled *bool `json:"enabled,omitempty"` + // CameraDuration specifies the duration (in milliseconds) for camera transitions CameraDuration *int `json:"cameraDuration,omitempty"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/go.mod(1 hunks)server/gql/property.graphql(1 hunks)server/internal/adapter/gql/generated.go(7 hunks)server/internal/adapter/gql/gqlmodel/convert_property.go(2 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/pkg/builtin/manifest.yml(1 hunks)server/schemas/plugin_manifest.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/gql/property.graphql
- server/schemas/plugin_manifest.json
- server/pkg/builtin/manifest.yml
- server/go.mod
- server/internal/adapter/gql/gqlmodel/convert_property.go
🔇 Additional comments (12)
server/internal/adapter/gql/gqlmodel/models_gen.go (4)
761-764: LGTM! The PhotoOverlay type definition is well-structured.The new PhotoOverlay type is correctly defined with appropriate JSON tags and follows the project's conventions for optional fields by using pointer types for both Enabled and CameraDuration fields.
2287-2287: LGTM! The ValueType enum is properly updated.The new PhotoOverlay type is correctly added to the AllValueType slice, ensuring it's recognized by the system.
2292-2293: LGTM! The IsValid method is properly updated.The ValueTypePhotooverlay is correctly added to the IsValid method, ensuring proper validation throughout the application.
761-764:❓ Verification inconclusive
Consider adding validation for the CameraDuration field.
Currently, there's no validation for the CameraDuration field. If there are valid ranges for this value (e.g., minimum or maximum duration), you might want to add validation logic elsewhere in the codebase to ensure that invalid values are not accepted.
🏁 Script executed:
#!/bin/bash # Search for validation patterns applied to similar duration fields echo "Searching for duration validation patterns in the codebase..." rg "Duration.*[<>]" --type goLength of output: 164
Action Required: Validate the CameraDuration field as needed
The code search for duration validations (via patterns like
Duration.*[<>]) returned no results, confirming that similar validation logic is absent. If theCameraDurationfield should have defined minimum or maximum values, please evaluate and add the appropriate validation logic. Given that the file is generated code, consider implementing this validation in the upstream business logic or input handling layer rather than modifying generated output directly.
- File under review:
server/internal/adapter/gql/gqlmodel/models_gen.go(lines 761-764)- No existing duration validation patterns were found in the codebase.
- Recommendation: Determine the valid range for
CameraDurationand implement validation in the proper layer.server/internal/adapter/gql/generated.go (8)
566-569: Structure looks correct for GraphQL complexity calculation.The
PhotoOverlaystruct with its two complexity calculation functions follows the established pattern in the codebase for handling GraphQL query complexity.
4022-4035: Complexity calculation implementation is properly structured.The implementation for complexity calculation of the PhotoOverlay fields follows the consistent pattern used for other types in the codebase, with appropriate null checks before accessing the complexity functions.
6942-6945: Clear GraphQL type definition for PhotoOverlay.The PhotoOverlay type is correctly defined with the two fields mentioned in the PR objectives:
enabled: Boolean - Controls whether the overlay is activecameraDuration: Int - Defines the duration for the cameraThis matches the requirements described in the PR summary.
7946-7946: ValueType enum updated correctly.The enum
ValueTypeis properly updated to includePHOTOOVERLAYas mentioned in the PR description. This is necessary to support the new non-primitive type.
29035-29061: Resolver implementation for enabled field follows standard pattern.The resolver for the
enabledfield ofPhotoOverlayis implemented correctly with proper error handling, middleware integration, and nil checking before accessing the resolved value.
29076-29102: Resolver implementation for cameraDuration field follows standard pattern.The resolver for the
cameraDurationfield ofPhotoOverlayis implemented correctly with proper error handling, middleware integration, and nil checking before accessing the resolved value.
57672-57706: Overall PhotoOverlay type implementation is correct.The implementation for the
PhotoOverlaytype follows the established pattern in the codebase:
- Correct field collection and handling
- Proper dispatch of field resolution
- Appropriate error handling and invalid detection
- Correct management of deferred fields
This matches the implementation of other GraphQL types in the file.
57670-57671: Implementors list correctly defined.The
photoOverlayImplementorsslice is correctly defined with just the "PhotoOverlay" string, which is appropriate for a concrete type that doesn't implement any interfaces.
Overview
I have added PhotoOverlay to the properties of infobox-beta.
What I've done
Similar to position, padding, and gap, it can be added as a member of a single property.
※ It is not a property of ItemGroup.
1.manifest.yml
- id: infobox-beta name: Infobox Beta type: infobox description: Create an information area that appears when a layer is highlighted. Text, pictures, video, etc can be added to an infobox. schema: groups: - id: default title: Basic fields: - id: enabled type: bool title: Enable Infobox ~~~~~~~~~~~~~~ - id: position type: string title: Position ~~~~~~~~~~~~~~ - id: padding type: spacing title: Padding ~~~~~~~~~~~~~~ - id: gap type: number title: gap ~~~~~~~~~~~~~~ + - id: photooverlay + type: photooverlay + title: PhotoOverlay2. graphql
note PHOTOOVERLAY, like TIMELINE, is defined as a non-primitive type.
What I haven't done
How I tested
TestInfoboxProperty
https://github.com/reearth/reearth-visualizer/pull/1462/files#diff-24ad6b984dd7d824485df99bc5f0afbad022e020e1f649feff78a5cdf2962af5R781-R838
TestUpdateStoryPropertyValue
https://github.com/reearth/reearth-visualizer/pull/1462/files#diff-6a43df5512652d569a0f842bd3661694a55e994ce1173c94f2b41c0b0360952cR178-R268
Which point I want you to review particularly
I updated reearthx, but a nil exception occurred because the Role repository could not be referenced.
Since this prevents adding members to the team, I am now referencing a custom branch of reearthx (chore/panic-to-error).
https://github.com/reearth/reearthx/tree/chore/panic-to-error
Memo
Summary by CodeRabbit
New Features
Bug Fixes