feat(web): support set photo overlay on layer inspector [VIZ-1349] #1522
Conversation
airslice
requested review from
mkumbobeaty,
pyshx and
soneda-yuya
as code owners
WalkthroughThe changes update the photo overlay configuration and integration across the backend and frontend. The manifest files now set default values and add descriptions for the photo overlay fields. New React hooks and components are introduced to manage photo overlay settings in the layer inspector and visualizer, along with a new tab in the editor. Enhancements to processing and converting NLS layers include the integration of photo overlay properties. Additionally, new API services and GraphQL mutations are added to support creating a photo overlay, and localization files receive new translation keys. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PS as PhotoOverlaySettings Component
participant H as useCreateNLSPhotoOverlay Hook
participant GS as GraphQL Server
participant NS as Notification Service
U->>PS: Toggle photo overlay switch
PS->>H: Invoke handlePhotoOverlayCreate
H->>GS: Send CREATE_NLSPHOTOOVERLAY mutation
GS-->>H: Return mutation response
H-->>PS: Callback with success/error result
PS->>NS: Trigger success/error notification
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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. |
airslice
changed the title
![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
🧹 Nitpick comments (5)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/hooks.ts (1)
7-8: Consider restructuring the photo overlay fetcher usage.The
usePhotoOverlayFetcherhook returnsuseCreateNLSPhotoOverlay, which is then directly used within this hook. This creates a somewhat confusing pattern where a function that appears to be a hook (prefixed with "use") is being returned from another hook and called directly.Consider one of these approaches:
- Rename the returned function in the API to not use the "use" prefix if it's not a hook itself
- Extract the function call outside this hook and pass the result as a parameter
web/src/services/api/photoOverlayApi/index.ts (1)
23-43: Consider renaming the inner function to avoid "use" prefix confusion.The function named
useCreateNLSPhotoOverlayis not directly a React hook but is returned from another function. This naming pattern is confusing because functions starting with "use" typically follow React's hook rules.- const useCreateNLSPhotoOverlay = useCallback( + const createNLSPhotoOverlay = useCallback( async ( input: CreateNlsPhotoOverlayInput ): Promise<MutationReturn<CreateNlsPhotoOverlayMutation>> => { // ...implementation... }, [createNLSPhotoOverlayMutation, setNotification, t] ); return { - useCreateNLSPhotoOverlay + createNLSPhotoOverlay };web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/index.tsx (1)
15-43: Consider handling potential errors when creating photo overlayThe
handlePhotoOverlayCreatefunction is used when the switch is toggled, but there's no visible error handling for failed creations in this component. Consider adding appropriate user feedback in case the creation fails.web/src/beta/features/Visualizer/Crust/PhotoOverlay/index.tsx (1)
52-53: Add null check for nlsLayers before attempting to find a layerThe current implementation could attempt to access properties on an undefined object if
nlsLayersis undefined.- const nlsLayer = nlsLayers?.find((l) => l.id === selectedLayer?.layer.id); + const nlsLayer = nlsLayers && nlsLayers.find((l) => l.id === selectedLayer?.layer.id);web/src/services/api/propertyApi/processNewProperty.ts (1)
12-12: Consider a more descriptive type nameThe type alias
Pis very short and not descriptive. Consider using a more descriptive name likePropertyValueorProcessedPropertyto improve code readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
web/src/services/gql/__gen__/gql.tsis excluded by!**/__gen__/**web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (20)
server/pkg/builtin/manifest.yml(1 hunks)server/pkg/builtin/manifest_ja.yml(1 hunks)web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/hooks.ts(1 hunks)web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/index.tsx(1 hunks)web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/index.tsx(4 hunks)web/src/beta/features/Editor/Visualizer/hooks.ts(2 hunks)web/src/beta/features/Editor/hooks/useLayers.ts(1 hunks)web/src/beta/features/Published/convert-nls-layers.ts(1 hunks)web/src/beta/features/Published/hooks.ts(3 hunks)web/src/beta/features/Published/index.tsx(2 hunks)web/src/beta/features/Published/types.ts(3 hunks)web/src/beta/features/Visualizer/Crust/PhotoOverlay/index.tsx(3 hunks)web/src/services/api/index.ts(1 hunks)web/src/services/api/layersApi/utils.ts(5 hunks)web/src/services/api/photoOverlayApi/index.ts(1 hunks)web/src/services/api/propertyApi/processNewProperty.ts(1 hunks)web/src/services/gql/fragments/layer.ts(1 hunks)web/src/services/gql/queries/photoOverlay.ts(1 hunks)web/src/services/i18n/translations/en.yml(2 hunks)web/src/services/i18n/translations/ja.yml(2 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
web/src/services/gql/queries/photoOverlay.ts (1)
web/src/services/gql/__gen__/gql.ts (1)
gql(443-445)
web/src/services/api/photoOverlayApi/index.ts (2)
web/src/services/gql/__gen__/graphql.ts (3)
CreateNlsPhotoOverlayMutation(2497-2497)MutationCreateNlsPhotoOverlayArgs(680-682)CreateNlsPhotoOverlayInput(184-186)web/src/services/gql/queries/photoOverlay.ts (1)
CREATE_NLSPHOTOOVERLAY(3-11)
web/src/beta/features/Published/convert-nls-layers.ts (2)
web/src/beta/features/Published/types.ts (1)
PublishedNLSLayer(57-83)web/src/services/api/layersApi/utils.ts (1)
NLSLayer(47-59)
web/src/services/api/propertyApi/processNewProperty.ts (1)
web/src/services/gql/__gen__/graphql.ts (6)
PropertyFragmentFragment(2316-2316)PropertySchemaGroupFragmentFragment(2302-2302)PropertyItemFragmentFragment(2312-2312)PropertyGroupFragmentFragment(2306-2306)PropertySchemaFieldFragmentFragment(2300-2300)PropertyFieldFragmentFragment(2304-2304)
web/src/beta/features/Published/hooks.ts (1)
web/src/beta/features/Published/convert-nls-layers.ts (1)
convertNLSLayers(5-29)
web/src/services/api/layersApi/utils.ts (2)
web/src/beta/features/Published/convert-new-property.ts (1)
processNewProperty(12-22)web/src/services/api/propertyApi/processNewProperty.ts (1)
processNewProperty(14-76)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/index.tsx (2)
web/src/services/api/layersApi/utils.ts (1)
NLSPhotoOverlay(12-20)web/src/services/api/photoOverlayApi/index.ts (1)
t(14-48)
web/src/beta/features/Visualizer/Crust/PhotoOverlay/index.tsx (2)
web/src/beta/features/Visualizer/Crust/PhotoOverlay/useCamera.ts (1)
DEFAULT_CAMERA_DURATION(6-6)web/src/beta/utils/sketch.ts (1)
getPhotoOverlayValue(52-58)
🪛 GitHub Check: ci-web / ci
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/hooks.ts
[warning] 6-6:
Unexpected any. Specify a different type
🪛 Biome (1.9.4)
web/src/services/api/propertyApi/processNewProperty.ts
[error] 30-30: 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)
[error] 92-92: 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)
⏰ 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 (35)
web/src/services/api/index.ts (1)
14-14: New export for photo overlay functionalityThis export makes the photo overlay API functionality available to other parts of the application, aligning with the PR objective of supporting photo overlay property in the layer inspector.
web/src/services/gql/queries/photoOverlay.ts (1)
1-11: New GraphQL mutation for creating photo overlaysThis mutation allows creation of a new NLS photo overlay, which is essential for the photo overlay feature. The implementation correctly defines the mutation with proper input parameter and returns the created layer ID.
One suggestion: Consider adding error handling in the components that will use this mutation, especially since GraphQL operations can fail for various reasons (network issues, validation errors, etc.).
server/pkg/builtin/manifest_ja.yml (1)
1466-1466: Added Japanese description for cameraDuration fieldThe description provides clear information about what the cameraDuration field represents and its unit of measurement (seconds). This improves documentation for Japanese users.
web/src/services/gql/fragments/layer.ts (1)
31-38: Added photoOverlay field to layer fragmentThis change extends the GraphQL layer fragment to include photo overlay information, which is necessary for implementing the photo overlay functionality in the layer inspector. The structure follows the same pattern as the existing infobox field, maintaining consistency.
The implementation correctly includes all necessary fields (layerId, propertyId, and the nested property with its id and PropertyFragment).
web/src/beta/features/Published/index.tsx (1)
15-15: Nice implementation of nlsLayers integration.The nlsLayers variable has been properly added to both the destructuring from useHooks and passed down to the Visualizer component, enabling the photo overlay functionality to work correctly throughout the component hierarchy.
Also applies to: 38-38
web/src/beta/features/Editor/hooks/useLayers.ts (1)
209-227: Code formatting improvement is good.The reformatting of the selectedFeature useMemo hook improves readability while maintaining the same functionality. The indentation and structure now follow a more consistent pattern.
web/src/beta/features/Editor/Visualizer/hooks.ts (1)
10-10: Import statements reorganized for better readability.The consolidation of type imports on line 10 and multi-line formatting of atom imports on lines 33-36 improves code organization and readability.
Also applies to: 33-36
web/src/beta/features/Published/hooks.ts (4)
23-23: Import for convertNLSLayers added correctly.The import statement for the new convertNLSLayers function has been properly added, enabling the transformation of NLS layers data.
260-263: Good use of useMemo for nlsLayers conversion.The implementation correctly uses useMemo to optimize performance by only recomputing when data?.nlsLayers changes. This ensures efficient handling of the photo overlay data.
317-317: Properly exposing nlsLayers in the hook's return object.The nlsLayers variable is correctly added to the return object, making the processed layers with photo overlay information available to consuming components.
260-263:❓ Verification inconclusive
Verify nlsLayers integration with the rest of the application.
Based on the provided context, the convertNLSLayers function processes photoOverlay properties. Let's verify that these properties are correctly used by the components consuming this data.
🏁 Script executed:
#!/bin/bash # Search for components that use photoOverlay properties from nlsLayers echo "Searching for components that use photoOverlay properties..." rg -A 5 "photoOverlay" --type ts --type tsx # Search for the usage of nlsLayers in components echo -e "\nSearching for nlsLayers usage in components..." rg -A 2 "nlsLayers" --type ts --type tsxLength of output: 380
Please manually verify the integration of the
nlsLayersdata with its consumers.
Based on an initial review, the conversion viaconvertNLSLayers(which processesphotoOverlayproperties) should properly feed downstream components. However, our earlier automated searches using predefined file types failed due to unrecognized file type errors. The above revised script targets all.tsand.tsxfiles. Once these commands produce output, you should ensure that:
- Components consuming
nlsLayersare correctly accessing and using thephotoOverlayproperties.- The overall overlay rendering flow in the application remains intact after this conversion.
Given the current ambiguity in the automated verification, please perform a manual inspection if the output is inconclusive.
🏁 Script executed:
#!/bin/bash echo "Searching for photoOverlay usage in TS/TSX files..." rg -g "*.ts" -g "*.tsx" -A 5 "photoOverlay" . echo -e "\nSearching for nlsLayers usage in TS/TSX files..." rg -g "*.ts" -g "*.tsx" -A 2 "nlsLayers" .Length of output: 95919
Action: Verify nlsLayers integration across consumers
Our automated search shows that the conversion function (i.e.
convertNLSLayersin the Published feature) indeed processes thephotoOverlayproperties and that these properties are referenced in various components (e.g. in API utilities, Visualizer, Editor, and related PhotoOverlay components). This suggests the integration is in line with expectations. However, since the search confirms only static usage, please do a final manual check to ensure that all downstream consumers are receiving and processing the convertedphotoOverlaydata as intended.web/src/services/i18n/translations/en.yml (2)
188-189: LGTM: Clear and consistent translation keys for the photo overlay feature.The added translation keys for photo overlay follow the same pattern as other similar features (like infobox at lines 185-186), making the UI language consistent.
570-570: LGTM: Success message for photo overlay enablement.This success message aligns with the notification shown when a photo overlay is created successfully.
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/hooks.ts (2)
14-18: LGTM: Clear conditional logic for photo overlay creation.The function correctly checks if a property already exists before attempting to create a new photo overlay, preventing duplicate creation.
6-6:✅ Verification successful
Type
anyshould be replaced with a more specific type.The
propertyparameter is typed asany, which is flagged by static analysis. Usinganycircumvents TypeScript's type checking.-export default ({ layerId, property }: { layerId: string; property?: any }) => { +export default ({ layerId, property }: { layerId: string; property?: Record<string, unknown> }) => {
🌐 Web query:
What's the recommended TypeScript alternative to using `any` type?💡 Result:
The recommended alternatives to TypeScript's
anytype prioritize type safety while maintaining flexibility. Here are the top solutions:1.
unknownType
The safest replacement foranyrequires explicit type checking before use:function process(input: unknown) { if (typeof input === "string") { console.log(input.trim()); // Now type-safe } }Unlike
any,unknownforces validation, preventing accidental type errors[1][2][7][11].2. Interfaces/Types
Define object shapes explicitly using interfaces or type aliases:interface User { id: number; name: string; }This ensures type consistency and enables autocomplete[2][6][12].
3. Utility Types
Leverage built-in utilities for common patterns:
Partial<User>: Makes all properties optionalPick<User, "id">: Selects specific propertiesRecord<string, number>: Dictionary with string keys[3][5][11]4. Union Types
Handle multiple possible types explicitly:function format(input: string | number) { // Type narrowing required }Safer than
anywhile accommodating different input types[8][9][10].5. Type Guards
Combine withneverfor exhaustive checks:type Shape = Circle | Square; function area(shape: Shape): number { switch(shape.type) { case "circle": return Math.PI * shape.radius²; default: const _exhaustive: never = shape; return _exhaustive; } }Ensures all union variants are handled[2][9].
When You Can't Avoid Flexibility
Use constrained patterns instead of rawany:// Safer than any[] const data: Array<Record<string, unknown>> = await fetchAPI(); // Temporary type placeholder type APIResponse = any; // Replace later with actual type[4][8]Key Reasons to Avoid
any
- Disables type checking, enabling runtime errors[1][4][6]
- Reduces code maintainability and refactoring safety[1][3]
- Requires extra unit tests to compensate for lost type safety[3]
Best practice: Enable TypeScript's
strictmode and use lint rules likeno-explicit-anyto enforce type safety[4][5][12].Citations:
- 1: https://www.allthingstypescript.dev/p/why-avoid-the-any-type-in-typescript
- 2: https://dev.to/yugjadvani/how-to-write-better-typescript-code-best-practices-for-clean-effective-and-scalable-code-38d2
- 3: https://jessewarden.com/2024/09/six-alternatives-to-using-any-in-typescript.html
- 4: https://www.codiga.io/blog/typescript-avoid-type-any/
- 5: https://www.telerik.com/blogs/mastering-typescript-benefits-best-practices
- 6: https://blog.bitsrc.io/stop-using-any-type-in-typescript-48ebefc8b299
- 7: https://43081j.com/2019/02/typescript-avoid-using-any
- 8: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html
- 9: https://www.typescriptlang.org/docs/handbook/advanced-types.html
- 10: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
- 11: https://dev.to/saulodias/a-safer-alternative-to-any-in-typescript-2g67
- 12: https://google.github.io/styleguide/tsguide.html
Action Required: Replace
anywith a safer, more specific type.
Thepropertyparameter currently usesany, which bypasses TypeScript’s type checking. To ensure type safety while preserving the intent of the parameter (i.e., treating it as an object with unspecified keys), replaceanywithRecord<string, unknown>. This change aligns with best practices by enforcing an expectation thatpropertyis an object, without assuming the structure of its values.
Before:
export default ({ layerId, property }: { layerId: string; property?: any }) => {After:
export default ({ layerId, property }: { layerId: string; property?: Record<string, unknown> }) => {If you later have a better understanding of the actual structure of
property, consider defining a dedicated interface or type alias to further enhance type precision.🧰 Tools
🪛 GitHub Check: ci-web / ci
[warning] 6-6:
Unexpected any. Specify a different typeweb/src/services/api/photoOverlayApi/index.ts (3)
18-21: LGTM: Well-typed GraphQL mutation setup.The mutation is properly typed with TypeScript generics for both the return type and arguments, ensuring type safety throughout the mutation process.
30-34: LGTM: Robust error handling implementation.The function properly checks for GraphQL errors and verifies the expected data structure is returned before proceeding, providing clear error feedback.
35-38: LGTM: Success notification with localized message.The success message properly uses the translation function, matching the key added in the localization file for consistency across the application.
web/src/beta/features/Published/convert-nls-layers.ts (2)
5-29: LGTM: Clean and well-structured conversion function.The function properly maps the published layer format to the format expected by the UI components, with explicit handling of the photo overlay properties. The null/undefined checks are appropriately implemented.
20-27: LGTM: Well-implemented photo overlay property processing.The code correctly transforms the photo overlay data structure, extracting the necessary properties (
enabledandcameraDuration) into a processed format that can be directly used by the UI components.web/src/services/i18n/translations/ja.yml (2)
185-186: Appropriate translations for the new PhotoOverlay feature.The translations for "Enable PhotoOverlay" and "Show photo overlay when the user clicks on a layer" are clear and consistent with the existing translation style in the file.
567-567: Success message translation looks good.The translation for "Successfully enabled photo overlay" is well-formatted and matches the pattern of other success messages in the file.
server/pkg/builtin/manifest.yml (2)
2842-2842: Default value for photo overlay enabled property set to true.The default value for the photo overlay
enabledproperty has been set totrue, which will ensure that the feature is enabled by default when users add it.
2845-2847: Clear title and helpful description for cameraDuration property.The title has been simplified to "Camera Duration" and a default value of 1 second has been added. The description clearly explains that this represents the duration for the camera to fly to the captured position.
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/index.tsx (3)
7-7: Added required import for GeoJsonFeatureUpdateProps.This import is used in the component props interface but wasn't needed for the PhotoOverlay changes specifically.
19-19: New import for PhotoOverlaySettings component.The import is correctly added for the new PhotoOverlaySettings component that will be used in the conditional tab.
55-140: Well-implemented conditional tab for PhotoOverlay settings.The code has been refactored to dynamically generate tab items, adding the Photo Overlay tab only when the selected layer is a sketch layer. This is a good approach to avoid cluttering the UI with irrelevant tabs.
The dependencies array for the useMemo hook has been correctly updated to include all the variables used in the tab creation logic.
web/src/services/api/layersApi/utils.ts (4)
2-2: Added import for processNewProperty function.This import is required for processing the photoOverlay property values.
12-20: Well-structured NLSPhotoOverlay type definition.The type definition for NLSPhotoOverlay is clear and includes all necessary fields. The processedProperty object contains the expected fields for the photo overlay feature.
58-58: Added photoOverlay property to NLSLayer type.The NLSLayer type now includes an optional photoOverlay property of type NLSPhotoOverlay, which is needed for the new feature.
149-165: Photo overlay processing correctly extracts property values.The code properly processes the photo overlay property and extracts the enabled and cameraDuration values using the processNewProperty function. This ensures that the UI components have access to the processed values.
web/src/beta/features/Visualizer/Crust/PhotoOverlay/index.tsx (1)
63-65: LGTM: Camera duration handling is well implementedThe camera duration is properly extracted from the photo overlay settings with a fallback to the default value.
web/src/beta/features/Published/types.ts (1)
74-82: LGTM: Well-structured type definition for photo overlayThe
nlsPhotoOverlayproperty is well-structured with appropriate optional fields, ensuring backward compatibility.web/src/services/api/propertyApi/processNewProperty.ts (2)
14-76: Good implementation of property merging logicThe property merging logic is well-implemented, handling different cases for property groups and lists with appropriate fallbacks. The function successfully prioritizes original over parent values and falls back to schema defaults when needed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
102-138: LGTM: Field processing logic is well-implementedThe field processing logic is clean and handles all necessary metadata, with appropriate fallbacks to default values when needed.
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/PhotoOverlaySettings/index.tsx
Show resolved
Hide resolved
Overview
This PR connects the layer's photo overlay property and added the settings UI on layer inspector.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Documentation