feat(server): remove unnecessary code [VIZ-BE-582] #1458
Conversation
hexaforce
requested review from
airslice,
mkumbobeaty,
pyshx and
soneda-yuya
as code owners
WalkthroughThis pull request introduces a new Makefile target called Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Makefile
participant C as Go Toolchain
U->>M: Run `make run-clear-cache`
M->>C: Execute command "go clean -cache -modcache -testcache -fuzzcache"
C-->>M: Cache cleared confirmation
M-->>U: Output confirmation message
Possibly related PRs
Suggested reviewers
Poem
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 canceled.
|
hexaforce
changed the title
hexaforce
changed the title
hexaforce
changed the title
hexaforce
changed the title
hexaforce
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: 0
🧹 Nitpick comments (1)
server/internal/adapter/gql/loader_property.go (1)
57-57: Consider removing the stub method or clarifying its usage.The
FetchMergedmethod now returnsnil, niland provides no functionality. If it’s not needed elsewhere, removing it entirely can reduce confusion and maintenance overhead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
server/go.sumis excluded by!**/*.sumserver/pkg/layer/decoding/shapetest/shapes.zipis excluded by!**/*.zipweb/src/services/gql/__gen__/fragmentMatcher.jsonis excluded by!**/__gen__/**web/src/services/gql/__gen__/gql.tsis excluded by!**/__gen__/**web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (107)
server/Makefile(3 hunks)server/e2e/gql_storytelling_test.go(0 hunks)server/e2e/seeder.go(0 hunks)server/go.mod(0 hunks)server/gql/layer.graphql(0 hunks)server/gql/property.graphql(0 hunks)server/gql/storytelling.graphql(0 hunks)server/internal/adapter/gql/gqldataloader/dataloader.go(0 hunks)server/internal/adapter/gql/gqldataloader/layergrouploader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/layeritemloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/layerloader_gen.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_layer.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_property.go(2 hunks)server/internal/adapter/gql/gqlmodel/convert_storytelling.go(0 hunks)server/internal/adapter/gql/gqlmodel/models.go(0 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(0 hunks)server/internal/adapter/gql/gqlmodel/scalar_id.go(0 hunks)server/internal/adapter/gql/loader.go(0 hunks)server/internal/adapter/gql/loader_layer.go(0 hunks)server/internal/adapter/gql/loader_property.go(1 hunks)server/internal/adapter/gql/resolver_Storytelling.go(0 hunks)server/internal/adapter/gql/resolver_layer.go(0 hunks)server/internal/adapter/gql/resolver_mutation_property.go(0 hunks)server/internal/adapter/gql/resolver_property.go(0 hunks)server/internal/adapter/gql/resolver_query.go(0 hunks)server/internal/adapter/http/dataset.go(0 hunks)server/internal/app/app.go(0 hunks)server/internal/app/private.go(0 hunks)server/internal/infrastructure/memory/container.go(0 hunks)server/internal/infrastructure/memory/dataset.go(0 hunks)server/internal/infrastructure/memory/dataset_schema.go(0 hunks)server/internal/infrastructure/memory/layer.go(0 hunks)server/internal/infrastructure/memory/layer_test.go(0 hunks)server/internal/infrastructure/memory/property.go(1 hunks)server/internal/infrastructure/memory/tag.go(0 hunks)server/internal/infrastructure/memory/tag_test.go(0 hunks)server/internal/infrastructure/mongo/container.go(0 hunks)server/internal/infrastructure/mongo/dataset.go(0 hunks)server/internal/infrastructure/mongo/dataset_schema.go(0 hunks)server/internal/infrastructure/mongo/layer.go(0 hunks)server/internal/infrastructure/mongo/layer_test.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/dataset.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/dataset_schema.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/layer.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/property.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/scene.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/tag.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/tag_test.go(0 hunks)server/internal/infrastructure/mongo/property.go(0 hunks)server/internal/infrastructure/mongo/tag.go(0 hunks)server/internal/usecase/gateway/container.go(0 hunks)server/internal/usecase/gateway/datasouce.go(0 hunks)server/internal/usecase/interactor/common.go(1 hunks)server/internal/usecase/interactor/dataset.go(0 hunks)server/internal/usecase/interactor/layer.go(0 hunks)server/internal/usecase/interactor/layer_test.go(0 hunks)server/internal/usecase/interactor/nlslayer.go(10 hunks)server/internal/usecase/interactor/plugin.go(0 hunks)server/internal/usecase/interactor/plugin_upload.go(1 hunks)server/internal/usecase/interactor/plugin_upload_test.go(1 hunks)server/internal/usecase/interactor/project.go(2 hunks)server/internal/usecase/interactor/property.go(0 hunks)server/internal/usecase/interactor/scene.go(1 hunks)server/internal/usecase/interactor/scene_plugin.go(3 hunks)server/internal/usecase/interactor/scene_plugin_test.go(2 hunks)server/internal/usecase/interactor/storytelling.go(2 hunks)server/internal/usecase/interactor/storytelling_test.go(0 hunks)server/internal/usecase/interactor/tag.go(0 hunks)server/internal/usecase/interfaces/common.go(0 hunks)server/internal/usecase/interfaces/dataset.go(0 hunks)server/internal/usecase/interfaces/layer.go(0 hunks)server/internal/usecase/interfaces/property.go(0 hunks)server/internal/usecase/interfaces/scene.go(0 hunks)server/internal/usecase/interfaces/tag.go(0 hunks)server/internal/usecase/repo/container.go(0 hunks)server/internal/usecase/repo/dataset.go(0 hunks)server/internal/usecase/repo/dataset_schema.go(0 hunks)server/internal/usecase/repo/layer.go(0 hunks)server/internal/usecase/repo/property.go(0 hunks)server/internal/usecase/repo/tag.go(0 hunks)server/pkg/dataset/builder.go(0 hunks)server/pkg/dataset/csvparser.go(0 hunks)server/pkg/dataset/csvparser_test.go(0 hunks)server/pkg/dataset/dataset.go(0 hunks)server/pkg/dataset/dataset_test.go(0 hunks)server/pkg/dataset/diff.go(0 hunks)server/pkg/dataset/export.go(0 hunks)server/pkg/dataset/export_test.go(0 hunks)server/pkg/dataset/field.go(0 hunks)server/pkg/dataset/graph_iterator.go(0 hunks)server/pkg/dataset/graph_iterator_test.go(0 hunks)server/pkg/dataset/graph_loader.go(0 hunks)server/pkg/dataset/id.go(0 hunks)server/pkg/dataset/list.go(0 hunks)server/pkg/dataset/list_test.go(0 hunks)server/pkg/dataset/loader.go(0 hunks)server/pkg/dataset/schema.go(0 hunks)server/pkg/dataset/schema_builder.go(0 hunks)server/pkg/dataset/schema_field.go(0 hunks)server/pkg/dataset/schema_field_builder.go(0 hunks)server/pkg/dataset/schema_field_diff.go(0 hunks)server/pkg/dataset/schema_graph_iterator.go(0 hunks)server/pkg/dataset/schema_graph_iterator_test.go(0 hunks)server/pkg/dataset/schema_list.go(0 hunks)server/pkg/dataset/schema_list_test.go(0 hunks)server/pkg/dataset/schema_test.go(0 hunks)server/pkg/dataset/value.go(0 hunks)
⛔ Files not processed due to max files limit (38)
- server/pkg/dataset/value_optional.go
- server/pkg/dataset/value_optional_test.go
- server/pkg/dataset/value_test.go
- server/pkg/id/id.go
- server/pkg/layer/builder.go
- server/pkg/layer/decoding/common.go
- server/pkg/layer/decoding/common_test.go
- server/pkg/layer/decoding/czml.go
- server/pkg/layer/decoding/czml_test.go
- server/pkg/layer/decoding/decoder.go
- server/pkg/layer/decoding/format.go
- server/pkg/layer/decoding/geojson.go
- server/pkg/layer/decoding/geojson_test.go
- server/pkg/layer/decoding/kml.go
- server/pkg/layer/decoding/kml_test.go
- server/pkg/layer/decoding/reearth.go
- server/pkg/layer/decoding/reearth_test.go
- server/pkg/layer/decoding/shp.go
- server/pkg/layer/decoding/shp_test.go
- server/pkg/layer/encoding/common.go
- server/pkg/layer/encoding/common_test.go
- server/pkg/layer/encoding/czml.go
- server/pkg/layer/encoding/czml_test.go
- server/pkg/layer/encoding/encoder.go
- server/pkg/layer/encoding/exporter.go
- server/pkg/layer/encoding/geojson.go
- server/pkg/layer/encoding/geojson_test.go
- server/pkg/layer/encoding/kml.go
- server/pkg/layer/encoding/kml_test.go
- server/pkg/layer/encoding/shp.go
- server/pkg/layer/encoding/shp_test.go
- server/pkg/layer/group.go
- server/pkg/layer/group_builder.go
- server/pkg/layer/group_builder_test.go
- server/pkg/layer/group_test.go
- server/pkg/layer/id.go
- server/pkg/layer/id_list.go
- server/pkg/layer/id_list_test.go
💤 Files with no reviewable changes (94)
- server/internal/usecase/gateway/container.go
- server/internal/adapter/gql/gqlmodel/convert_storytelling.go
- server/gql/property.graphql
- server/internal/usecase/interactor/storytelling_test.go
- server/pkg/dataset/diff.go
- server/e2e/gql_storytelling_test.go
- server/internal/adapter/gql/gqlmodel/scalar_id.go
- server/internal/usecase/repo/property.go
- server/internal/infrastructure/mongo/property.go
- server/go.mod
- server/internal/adapter/gql/resolver_mutation_property.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/internal/infrastructure/memory/layer_test.go
- server/e2e/seeder.go
- server/internal/infrastructure/mongo/layer_test.go
- server/pkg/dataset/csvparser_test.go
- server/internal/usecase/interfaces/common.go
- server/pkg/dataset/export.go
- server/internal/usecase/gateway/datasouce.go
- server/gql/storytelling.graphql
- server/internal/usecase/interactor/plugin.go
- server/internal/app/private.go
- server/internal/usecase/repo/container.go
- server/internal/adapter/gql/resolver_query.go
- server/internal/infrastructure/memory/tag_test.go
- server/pkg/dataset/graph_iterator_test.go
- server/pkg/dataset/list_test.go
- server/internal/usecase/interfaces/property.go
- server/internal/usecase/repo/tag.go
- server/pkg/dataset/loader.go
- server/internal/adapter/gql/resolver_property.go
- server/internal/infrastructure/memory/container.go
- server/pkg/dataset/schema_graph_iterator_test.go
- server/internal/adapter/gql/loader.go
- server/internal/adapter/gql/resolver_Storytelling.go
- server/internal/usecase/repo/dataset.go
- server/internal/usecase/interactor/layer_test.go
- server/internal/adapter/gql/resolver_layer.go
- server/internal/adapter/gql/gqldataloader/dataloader.go
- server/pkg/dataset/dataset_test.go
- server/internal/adapter/gql/gqlmodel/models.go
- server/pkg/dataset/graph_iterator.go
- server/internal/usecase/repo/layer.go
- server/pkg/dataset/schema_field_diff.go
- server/internal/adapter/http/dataset.go
- server/internal/adapter/gql/gqlmodel/convert_layer.go
- server/pkg/dataset/schema_list.go
- server/internal/usecase/interfaces/tag.go
- server/internal/infrastructure/mongo/mongodoc/property.go
- server/internal/infrastructure/mongo/mongodoc/tag_test.go
- server/pkg/dataset/schema_field_builder.go
- server/internal/usecase/interactor/property.go
- server/internal/app/app.go
- server/pkg/dataset/schema_test.go
- server/pkg/dataset/schema_field.go
- server/pkg/dataset/builder.go
- server/internal/usecase/interfaces/scene.go
- server/pkg/dataset/schema_graph_iterator.go
- server/pkg/dataset/export_test.go
- server/gql/layer.graphql
- server/pkg/dataset/schema_list_test.go
- server/internal/usecase/interfaces/dataset.go
- server/pkg/dataset/schema_builder.go
- server/internal/usecase/interactor/tag.go
- server/internal/infrastructure/memory/dataset_schema.go
- server/internal/infrastructure/memory/dataset.go
- server/pkg/dataset/dataset.go
- server/internal/usecase/repo/dataset_schema.go
- server/internal/adapter/gql/gqldataloader/layergrouploader_gen.go
- server/pkg/dataset/field.go
- server/pkg/dataset/csvparser.go
- server/internal/infrastructure/mongo/mongodoc/dataset.go
- server/internal/adapter/gql/gqldataloader/layerloader_gen.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/usecase/interfaces/layer.go
- server/internal/infrastructure/mongo/dataset.go
- server/internal/infrastructure/mongo/dataset_schema.go
- server/internal/infrastructure/mongo/mongodoc/dataset_schema.go
- server/pkg/dataset/schema.go
- server/pkg/dataset/list.go
- server/internal/infrastructure/mongo/mongodoc/tag.go
- server/internal/infrastructure/mongo/mongodoc/layer.go
- server/internal/usecase/interactor/dataset.go
- server/pkg/dataset/value.go
- server/internal/infrastructure/mongo/container.go
- server/internal/infrastructure/mongo/layer.go
- server/internal/adapter/gql/loader_layer.go
- server/internal/infrastructure/mongo/tag.go
- server/internal/infrastructure/memory/layer.go
- server/pkg/dataset/graph_loader.go
- server/pkg/dataset/id.go
- server/internal/usecase/interactor/layer.go
- server/internal/adapter/gql/gqldataloader/layeritemloader_gen.go
- server/internal/infrastructure/memory/tag.go
✅ Files skipped from review due to trivial changes (1)
- server/internal/usecase/interactor/plugin_upload_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci-server / ci-server-test
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (33)
server/internal/usecase/interactor/scene_plugin_test.go (2)
72-72: Consistent error reference update.The error reference has been correctly updated from
interfaces.ErrPluginNotFoundtoErrPluginNotFoundas part of the code cleanup. This is in line with the PR objective of removing unnecessary code and dependencies.
79-79: Consistent error reference update.The error reference has been updated from
interfaces.ErrPluginNotFoundtoErrPluginNotFound, matching the same change in line 72. This maintains consistency in error handling throughout the file.server/internal/usecase/interactor/scene_plugin.go (2)
45-45: Error handling refactored to use local error constant.The code has been updated to use
ErrPluginNotFounddirectly instead ofinterfaces.ErrPluginNotFound. This change aligns with the PR objective of removing unnecessary code and simplifying dependencies.Ensure that
ErrPluginNotFoundis properly defined in this package and that all other references throughout the codebase have been updated consistently.Also applies to: 50-50, 53-53, 110-110, 192-192
82-158: Layer operations removed from plugin uninstallation.The code for plugin uninstallation has been simplified by removing layer operations, which is consistent with the PR objective of eliminating unused layer-related code. The current implementation now focuses only on handling properties and widgets.
The refactoring streamlines the codebase while maintaining the core functionality. Make sure that any UI components or client code that might have depended on layer operations during plugin uninstallation have been updated accordingly.
server/internal/usecase/interactor/nlslayer.go (12)
38-54: Good addition of local error variablesThese error variables were likely moved from the removed interfaces package. The addition is well-structured with consistent formatting and follows Go's error handling best practices.
246-302: Simplified error handling in Remove methodThe removal of the
interfaces.ErrCannotRemoveLayerToLinkedLayerGroupcheck simplifies the control flow in this method while maintaining the core functionality.
388-388: Error variable migration from interfaces packageReplaced
interfaces.ErrInfoboxAlreadyExistswith the locally defined error variable.
446-446: Error variable migration from interfaces packageReplaced
interfaces.ErrInfoboxNotFoundwith the locally defined error variable.
478-478: Error variable migration from interfaces packageReplaced
interfaces.ErrPluginNotFoundwith the locally defined error variable.
489-489: Error variable migration from interfaces packageReplaced
interfaces.ErrExtensionNotFoundwith the locally defined error variable.
523-523: Error variable migration from interfaces packageReplaced
interfaces.ErrInfoboxNotFoundwith the locally defined error variable.
531-531: Error variable migration from interfaces packageReplaced
interfaces.ErrExtensionTypeMustBeBlockwith the locally defined error variable.
601-601: Error variable migration from interfaces packageReplaced
interfaces.ErrInfoboxNotFoundwith the locally defined error variable.
648-648: Error variable migration from interfaces packageReplaced
interfaces.ErrInfoboxNotFoundwith the locally defined error variable.
774-774: Error variable migration from interfaces packageReplaced
interfaces.ErrSketchNotFoundwith the locally defined error variable.
840-840: Error variable migration from interfaces packageReplaced
interfaces.ErrSketchNotFoundwith the locally defined error variable.server/Makefile (3)
23-23: Good addition to help text.Thank you for adding a helpful description for the
run-clear-cachetarget. This improves the discoverability of the new Makefile command.
93-95: Command properly clears Go caches.
go clean -cache -modcache -testcache -fuzzcacheis a comprehensive approach to clearing caches, ensuring a fresh build environment.
116-116: Properly included in .PHONY list.Adding
run-clear-cacheto.PHONYclarifies that this target is not associated with any file, preventing potential naming conflicts.server/internal/adapter/gql/gqlmodel/convert_property.go (2)
337-339: Ensure the logic aligns with merged property requirements.These lines drop additional fields and set
Groupstonil. Confirm thatGroupswill be populated by the resolver or other logic, and verify removing any references to datasets or older fields does not break the property resolution flow.
349-351: Schema reference updated.This change substitutes older references with
SchemaID,OriginalID, andParentIDgleaned fromm.Schema. It looks consistent with the removal of dataset coupling. Keep an eye on any place that depends on the old dataset or layer references.server/internal/infrastructure/memory/property.go (1)
78-78:✅ Verification successful
Simplified property filtering logic
The condition for including properties in the
FindLinkedAllmethod has been simplified to only check if the property's scene matches the provided scene ID. According to the PR summary, this is part of removing unused dataset-related functionality.To ensure this change doesn't impact existing code, verify that no consumers of this method depend on the previous behavior that apparently included additional checks for linked fields:
🏁 Script executed:
#!/bin/bash # Search for usages of FindLinkedAll to verify impact rg "FindLinkedAll" --type goLength of output: 422
Property Filtering Simplification Verified
The simplified condition in the memory implementation now solely checks for a matching scene ID (
if p.Scene() == s), aligning with the cleanup of unused dataset-related checks as mentioned in the PR summary. A search forFindLinkedAllusages (found inserver/internal/usecase/repo/property.goandserver/internal/infrastructure/mongo/property.go) did not reveal any dependency on the previously extra linked field validations.No further action is required.
server/internal/usecase/interactor/project.go (3)
35-48: Good removal of unused dependencies.
These fields cleanly replace old references to dataset/layer/tag repos with the appropriate new repos. No immediate concerns regarding type safety or null references.
52-65: No issues found with the constructor.
All fields are properly initialized, and references to old dataset/layer/tag repositories are fully removed.
482-484: SceneDeleter initialization is concise.
The references toScene,SceneLock, andPropertyalign with the rest of the refactoring.server/internal/usecase/interactor/common.go (1)
143-146: Proper cleanup of SceneDeleter fields.
Removing unused references to layers/datasets keeps the codebase aligned with the stated objectives of this PR.server/internal/usecase/interactor/scene.go (2)
743-744: Updated error reference.
Switching toErrPluginNotFoundimproves consistency with distinct error definitions.
749-750: Updated error reference.
UsingErrExtensionNotFoundis consistent with the new error conventions.server/internal/usecase/interactor/storytelling.go (3)
1128-1128: Error constant refactoring is consistent.The change from
interfaces.ErrPluginNotFoundtoErrPluginNotFoundis part of the larger codebase cleanup, moving error definitions to a more appropriate location. This aligns with the PR's objective of removing unnecessary code and dependencies.
1139-1139: Error constant refactoring is consistent.Similar to the previous change, replacing
interfaces.ErrExtensionNotFoundwithErrExtensionNotFoundis consistent with the PR's refactoring goals, moving error definitions from the interfaces package.
366-370: Simplification of builder initialization confirms removal of unused dependencies.This code confirms the successful removal of unnecessary loaders from the
builder.New()call (specifically the layer, dataset, and tag loaders mentioned in the PR description). The implementation now uses only the property and NLS layer loaders that are necessary for the current functionality.server/internal/usecase/interactor/plugin_upload.go (2)
295-297:❓ Verification inconclusive
Confirm simplified block deletion aligns with overall refactoring.
Similar to the layer deletion function above, this function has been simplified to a stub that returns an empty property ID slice. This appears consistent with the PR's goal of removing unnecessary code.
Since both of these methods previously returned property IDs that would be deleted, make sure that plugin migration still functions correctly without these deletions occurring.
🏁 Script executed:
#!/bin/bash # Check if there are any calls to these methods elsewhere in the codebase rg "deleteLayersByPluginExtension|deleteBlocksByPluginExtension" --type go --files-with-matches server/Length of output: 157
Action Required: Verify Plugin Migration Integrity with Simplified Deletion Stubs
The simplified implementation of
deleteBlocksByPluginExtension—returning an empty property ID slice—is consistent with the refactoring direction demonstrated by the similar handling ofdeleteLayersByPluginExtension. Our search confirms these methods are only referenced within theserver/internal/usecase/interactor/plugin_upload.gofile, indicating no hidden external dependencies.Please ensure that the plugin migration process remains robust. In particular, verify that the removal of these deletion operations does not unintentionally affect plugin data cleanup during migration.
291-293:❓ Verification inconclusive
Confirm simplified layer deletion aligns with overall refactoring.
The layer deletion functionality has been completely removed, leaving just an empty stub. This change aligns with the PR objective to remove unused layer-related code. Since the property IDs returned by this method are still used in the
migrateScenePluginfunction, verify that returning an empty slice won't cause issues with plugin migration workflows.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to layerRepo in this package rg "layerRepo\." --type go server/internal/usecase/interactor/Length of output: 3825
Layer Deletion Refactoring – Verify Empty Return Is Handled in Migration
The simplified deletion function now just returns an empty slice, which appears aligned with the goal of removing unused layer deletion code. Our search shows that the remaining layer‐related operations (via the nlslayerRepo) are confined to other files (e.g. in nlslayer.go) and no other part of the interactor package explicitly expects non‐empty property ID results. However, since the property IDs from this method are passed into the
migrateScenePluginworkflow, please ensure that the migration logic correctly handles an empty slice (for instance, by safely iterating over it or skipping deletion steps when no IDs are returned).• Confirm that
migrateScenePlugin(or any caller) does not assume a non‑empty slice and that its behavior remains correct without further layer deletions.
• Validate via tests or manual review that no unintended side effects occur in the migration workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
1542-1571: 🛠️ Refactor suggestionRemove unused NodeType enum values
The enum values
NodeTypeDatasetSchema,NodeTypeDataset,NodeTypeLayerGroup, andNodeTypeLayerItemare still defined, even though the corresponding types have been removed from the codebase. This could lead to confusion or errors if these node types are referenced elsewhere.type NodeType string const ( NodeTypeAsset NodeType = "ASSET" NodeTypeUser NodeType = "USER" NodeTypeTeam NodeType = "TEAM" NodeTypeProject NodeType = "PROJECT" NodeTypePlugin NodeType = "PLUGIN" NodeTypeScene NodeType = "SCENE" NodeTypePropertySchema NodeType = "PROPERTY_SCHEMA" NodeTypeProperty NodeType = "PROPERTY" - NodeTypeDatasetSchema NodeType = "DATASET_SCHEMA" - NodeTypeDataset NodeType = "DATASET" - NodeTypeLayerGroup NodeType = "LAYER_GROUP" - NodeTypeLayerItem NodeType = "LAYER_ITEM" ) var AllNodeType = []NodeType{ NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty, - NodeTypeDatasetSchema, - NodeTypeDataset, - NodeTypeLayerGroup, - NodeTypeLayerItem, } func (e NodeType) IsValid() bool { switch e { - case NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty, NodeTypeDatasetSchema, NodeTypeDataset, NodeTypeLayerGroup, NodeTypeLayerItem: + case NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty: return true } return false }
1073-1085: 💡 Verification agent🧩 Analysis chain
Consider refactoring StoryPage structure field names
The
StoryPagestructure still hasLayersIdsandSwipeableLayersIdsfields that reference layers, even though layer-related code is being removed. Consider renaming these fields to reflect their new purpose or remove them if they're no longer needed.
🏁 Script executed:
#!/bin/bash # Search for usage of StoryPage's layer-related fields echo "Checking usage of LayersIds and SwipeableLayersIds..." rg -A 5 "\\.LayersIds|\\.SwipeableLayersIds" --type goLength of output: 2131
Action Required: Refactor StoryPage Layer Field Usage
The
StoryPagestruct still definesLayersIdsandSwipeableLayersIds, and our search indicates that these fields are still actively referenced in the generated GraphQL code (e.g., inserver/internal/adapter/gql/generated.gofor nil checks and return values). If layer-related functionality is being removed, either update the field names to reflect their new purpose or remove them entirely. Please ensure that any changes are coordinated with the generated code to prevent inconsistencies.
📜 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 (13)
server/e2e/gql_custom_property_test.go(0 hunks)server/e2e/gql_project_export_import_test.go(0 hunks)server/gql/property.graphql(0 hunks)server/gql/storytelling.graphql(0 hunks)server/gql/workspace.graphql(0 hunks)server/gqlgen.yml(0 hunks)server/internal/adapter/gql/gqlmodel/convert_storytelling.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_workspace.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_workspace_test.go(0 hunks)server/internal/adapter/gql/gqlmodel/models.go(0 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/internal/adapter/gql/loader_property.go(0 hunks)server/internal/adapter/gql/resolver_property.go(1 hunks)
💤 Files with no reviewable changes (10)
- server/internal/adapter/gql/gqlmodel/convert_workspace.go
- server/internal/adapter/gql/loader_property.go
- server/e2e/gql_custom_property_test.go
- server/internal/adapter/gql/gqlmodel/convert_workspace_test.go
- server/gql/workspace.graphql
- server/gql/storytelling.graphql
- server/gqlgen.yml
- server/internal/adapter/gql/gqlmodel/models.go
- server/e2e/gql_project_export_import_test.go
- server/gql/property.graphql
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/adapter/gql/gqlmodel/convert_storytelling.go
🔇 Additional comments (4)
server/internal/adapter/gql/resolver_property.go (2)
98-103:❓ Verification inconclusive
Simplified Groups method returns nil for cases that previously fetched data
The
Groupsmethod now simply returnsnil, nilwhenobj.Groupsis nil, whereas previously it attempted to fetch and process merged property groups. This change is consistent with removing layer functionality, but ensure that any front-end code relying on this resolver has been updated to handle the absence of this data.
🏁 Script executed:
#!/bin/bash # Search for GraphQL queries that expect merged property groups echo "Searching for GraphQL queries that might be affected by this change..." rg -t js -t ts "MergedPropertyGroup" --glob '!node_modules'Length of output: 16736
Attention: Check Front-End Handling for Nil MergedPropertyGroup Data
The updated
Groupsresolver inserver/internal/adapter/gql/resolver_property.go(lines 98–103) now returnsnil, nilwhenobj.Groupsis nil, instead of processing group data as it did previously. The GraphQL fragments and types in files likeweb/src/services/gql/fragments/property.tsand the generated files underweb/src/services/gql/__gen__still referenceMergedPropertyGroup. Please verify that the front-end code consuming this resolver is updated to gracefully handle anilresponse or an empty array for merged property groups. Consider checking that:
- Queries expecting merged property group details (e.g., through fragments such as
MergedPropertyGroupCommonFragmentandMergedPropertyGroupFragment) correctly handle cases where group data is absent.- Downstream UI logic is robust against receiving
nilinstead of a populated group list.
43-45:❓ Verification inconclusive
Verify downstream impact of simplifying the Merged method
The
Mergedmethod has been significantly simplified to just returnnil, nil, removing all previous layer-related functionality. This aligns with the PR's goal of removing the layer package, but verify that any GraphQL clients expecting merged property data have been updated accordingly to handle this change.
🏁 Script executed:
#!/bin/bash # Search for GraphQL queries that expect merged property data echo "Searching for GraphQL queries that might be affected by this change..." rg -t js -t ts "MergedProperty" --glob '!node_modules'Length of output: 17815
Action Required: Confirm Client Handling of Null Merged Property
TheMergedresolver has been simplified to returnnil, nil, effectively removing its prior merging logic. The downstream GraphQL clients still reference variousMergedPropertyfragments and types across the codebase. Please ensure that:
- Client queries and fragments (e.g. in
web/src/services/api/propertyApi/utils.ts,web/src/services/gql/fragments/property.ts, and the generated files inweb/src/services/gql/__gen__) are updated to gracefully handle anullmerged property.- Any potential runtime dependencies on non-null merged property data are refactored to avoid errors, now that the field will consistently be empty.
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
414-422: LGTM: MergedProperty structure appropriately modifiedThe
MergedPropertystructure has been properly refactored to remove fields related to layers, which aligns with the PR objective of removing unnecessary layer-related code.
1061-1068: LGTM: StoryBlock structure properly maintainedThe
StoryBlockstructure has been correctly preserved while other unnecessary components have been removed.
| type LinkDatasetToPropertyValueInput struct { | ||
| PropertyID ID `json:"propertyId"` | ||
| SchemaGroupID *ID `json:"schemaGroupId,omitempty"` | ||
| ItemID *ID `json:"itemId,omitempty"` | ||
| FieldID ID `json:"fieldId"` | ||
| DatasetSchemaIds []ID `json:"datasetSchemaIds"` | ||
| DatasetSchemaFieldIds []ID `json:"datasetSchemaFieldIds"` | ||
| DatasetIds []ID `json:"datasetIds,omitempty"` | ||
| PropertyID ID `json:"propertyId"` | ||
| SchemaGroupID *ID `json:"schemaGroupId,omitempty"` | ||
| ItemID *ID `json:"itemId,omitempty"` | ||
| FieldID ID `json:"fieldId"` | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider removing LinkDatasetToPropertyValueInput structure
This PR aims to remove unnecessary code related to datasets, yet this input structure remains. If datasets are being completely removed, this structure might be unnecessary.
🏁 Script executed:
#!/bin/bash
# Check if this struct is still being referenced elsewhere in the codebase
echo "Checking references to LinkDatasetToPropertyValueInput..."
rg -l "LinkDatasetToPropertyValueInput" | grep -v "models_gen.go"Length of output: 355
Action: Review the usage of LinkDatasetToPropertyValueInput across the codebase
The structure is still in use—references were found in:
web/src/services/gql/__gen__/graphql.tsserver/gql/property.graphqlserver/internal/adapter/gql/resolver_mutation_property.goserver/internal/adapter/gql/generated.go
If the goal is to completely remove dataset-related code, please ensure that all consumers of this input structure are updated or removed accordingly to avoid breaking changes. If these references are still needed, please retain the structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
1564-1564:❓ Verification inconclusive
Consider removing unused layer-related node types.
IfNodeTypeLayerGroupandNodeTypeLayerItemare no longer relevant due to the code removals, please remove them from this enum to keep it clean and consistent. Otherwise, confirm they are still in use:If no references exist besides this line, you can safely remove them:
- case NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty, NodeTypeLayerGroup, NodeTypeLayerItem: + case NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty:
🏁 Script executed:
#!/bin/bash # Checking references for NodeTypeLayerGroup and NodeTypeLayerItem across the codebase rg -A 5 "NodeTypeLayerGroup" rg -A 5 "NodeTypeLayerItem"Length of output: 2690
Action Required: Verify and Remove Unused Layer Node Types
The repository search indicates that bothNodeTypeLayerGroupandNodeTypeLayerItemare referenced only within the generated file (server/internal/adapter/gql/gqlmodel/models_gen.go). If these types are no longer needed—especially given recent code removals—they can be safely removed to clean up the enum.Next Steps:
- Confirm external dependencies: Double-check that removing these types does not affect external clients or API contracts.
- If removal is safe, update the file by:
- Removing
NodeTypeLayerGroupandNodeTypeLayerItemfrom the enum declaration (e.g., in theAllNodeTypeslice).- Removing them from the
switchstatement in theIsValid()method.Diff Example:
- case NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty, NodeTypeLayerGroup, NodeTypeLayerItem: + case NodeTypeAsset, NodeTypeUser, NodeTypeTeam, NodeTypeProject, NodeTypePlugin, NodeTypeScene, NodeTypePropertySchema, NodeTypeProperty:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (5)
server/gql/_shared.graphql(1 hunks)server/gql/property.graphql(0 hunks)server/gqlgen.yml(0 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/internal/adapter/gql/resolver_mutation_property.go(0 hunks)
💤 Files with no reviewable changes (3)
- server/internal/adapter/gql/resolver_mutation_property.go
- server/gqlgen.yml
- server/gql/property.graphql
⏰ 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 (4)
server/gql/_shared.graphql (2)
18-29: Removal of Deprecated Enum ValuesThe
NodeTypeenum no longer includes theDATASET_SCHEMAandDATASETvalues. This update aligns with the PR objectives to eliminate unused code and simplify the server-side schema. Please verify that downstream systems, such as GraphQL resolvers and front-end queries, do not depend on these removed values.
40-45: Improved Formatting on Pagination InputThe change from
input Pagination{toinput Pagination {enhances readability and consistency with the rest of the schema formatting. This is a simple, non-functional cosmetic improvement.server/internal/adapter/gql/gqlmodel/models_gen.go (2)
408-414: Looks good.
These fields withinMergedPropertyappear to be valid references to remaining property functionality. They do not seem tied to the removed dataset/layer code, so there is no immediate concern here.
1054-1060: No issues found.
The updated fields inStoryBlocklook consistent with the new plugin-based architecture and do not appear to be referencing any removed dataset or layer components.
Overview
Remove any unnecessary code that is not currently being used
What I've done
1.Deleted all of the following packages:
※ This includes the removal of loaders, interfaces, and other related components.
2.Deleted the following MongoDocuments:
3.Deleted the following IDs:
Note: This also includes refactoring the affected areas due to the above modifications.
4.fixed the FE GraphQL.
5.removed all old layers and Infoboxes from the FE.
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Refactor
Chores