GO-2930: Store device names #1013
Conversation
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
pkg/lib/pb/model/protos/models.proto
Outdated
| @@ -50,6 +50,7 @@ enum SmartBlockType { | |||
| FileObject = 0x215; | |||
|
|
|||
| NotificationObject = 0x217; | |||
| DeviceObject = 0x218; | |||
There was a problem hiding this comment.
I think Page type is sufficient for this task, as you probably don't need special behavior for smartblock
…o-2930-store-device-names # Conflicts: # clientlibrary/service/service.pb.go # core/anytype/bootstrap.go # pb/commands.pb.go # pb/service/service.pb.go # pkg/lib/pb/model/models.pb.go # pkg/lib/pb/model/protos/models.proto
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
WalkthroughThe recent updates introduce device management capabilities within the core system. In summary:
Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Server
participant DeviceManager
User->>Client: Request to Set Device Name
Client->>Server: Invoke DeviceSetName RPC
Server->>DeviceManager: Set Device Name
DeviceManager->>Server: Name Set Confirmation
Server->>Client: Respond with Success
Client->>User: Show Confirmation
User->>Client: Request Device List
Client->>Server: Invoke DeviceList RPC
Server->>DeviceManager: Fetch Device List
DeviceManager->>Server: Return Device List
Server->>Client: Respond with Device List
Client->>User: Display Device List
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| t.Run("add device - device exist", func(t *testing.T) { | ||
| // given | ||
| st := NewDoc("root", nil).(*State) | ||
| st.AddDevice(&model.DeviceInfo{ | ||
| Id: "id", | ||
| Name: "test", | ||
| }) | ||
| newState := st.NewState() | ||
|
|
||
| // when | ||
| newState.AddDevice(&model.DeviceInfo{ | ||
| Id: "id", | ||
| Name: "test1", | ||
| }) | ||
|
|
||
| // then | ||
| assert.NotNil(t, st.deviceStore["id"]) | ||
| assert.Equal(t, "test", st.deviceStore["id"].Name) | ||
| }) |
There was a problem hiding this comment.
Add an assertion to verify that the device name remains unchanged when adding an existing device.
+ assert.Equal(t, "test", newState.deviceStore["id"].Name)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.
| t.Run("add device - device exist", func(t *testing.T) { | |
| // given | |
| st := NewDoc("root", nil).(*State) | |
| st.AddDevice(&model.DeviceInfo{ | |
| Id: "id", | |
| Name: "test", | |
| }) | |
| newState := st.NewState() | |
| // when | |
| newState.AddDevice(&model.DeviceInfo{ | |
| Id: "id", | |
| Name: "test1", | |
| }) | |
| // then | |
| assert.NotNil(t, st.deviceStore["id"]) | |
| assert.Equal(t, "test", st.deviceStore["id"].Name) | |
| }) | |
| t.Run("add device - device exist", func(t *testing.T) { | |
| // given | |
| st := NewDoc("root", nil).(*State) | |
| st.AddDevice(&model.DeviceInfo{ | |
| Id: "id", | |
| Name: "test", | |
| }) | |
| newState := st.NewState() | |
| // when | |
| newState.AddDevice(&model.DeviceInfo{ | |
| Id: "id", | |
| Name: "test1", | |
| }) | |
| // then | |
| assert.NotNil(t, st.deviceStore["id"]) | |
| assert.Equal(t, "test", st.deviceStore["id"].Name) | |
| assert.Equal(t, "test", newState.deviceStore["id"].Name) | |
| }) |
| - [Rpc.Device](#anytype-Rpc-Device) | ||
| - [Rpc.Device.List](#anytype-Rpc-Device-List) | ||
| - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | ||
| - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | ||
| - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | ||
| - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName) | ||
| - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | ||
| - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) |
There was a problem hiding this comment.
Indentation of list items should be consistent with other entries.
- - [Rpc.Device](#anytype-Rpc-Device)
- - [Rpc.Device.List](#anytype-Rpc-Device-List)
- - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request)
- - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response)
- - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error)
- - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName)
- - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request)
- - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response)
- - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error)
+ - [Rpc.Device](#anytype-Rpc-Device)
+ - [Rpc.Device.List](#anytype-Rpc-Device-List)
+ - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request)
+ - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response)
+ - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error)
+ - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName)
+ - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request)
+ - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response)
+ - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error)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.
| - [Rpc.Device](#anytype-Rpc-Device) | |
| - [Rpc.Device.List](#anytype-Rpc-Device-List) | |
| - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | |
| - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | |
| - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | |
| - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName) | |
| - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | |
| - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | |
| - [Rpc.Device](#anytype-Rpc-Device) | |
| - [Rpc.Device.List](#anytype-Rpc-Device-List) | |
| - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | |
| - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | |
| - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | |
| - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName) | |
| - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | |
| - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | |
| - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) |
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
…o-2930-store-device-names # Conflicts: # clientlibrary/service/service.pb.go # docs/proto.md # pb/commands.pb.go # pb/service/service.pb.go # pkg/lib/pb/model/models.pb.go # pkg/lib/pb/model/protos/models.proto
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 17
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.gopb/commands.pb.gois excluded by!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.gopkg/lib/pb/model/models.pb.gois excluded by!**/*.pb.go
Files selected for processing (10)
- core/block/editor/state/change.go (4 hunks)
- core/block/editor/state/state.go (4 hunks)
- core/device/service.go (1 hunks)
- core/device/service_test.go (1 hunks)
- core/device/store.go (1 hunks)
- core/device/store_test.go (1 hunks)
- docs/proto.md (10 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- pkg/lib/pb/model/protos/models.proto (5 hunks)
Additional comments not posted (23)
core/device/store.go (4)
17-25: Type definitions forStoreinterface anddeviceStorestruct are clear and well-structured.
27-29: Constructor functionNewStoreis correctly implemented.
31-46: FunctionSaveDevicecorrectly handles device existence checks and uses transactions effectively.
48-66: FunctionListDevicesis well-implemented with proper transaction management and error handling.core/device/store_test.go (3)
12-55: Test functionTestDeviceStore_SaveDeviceis comprehensive and correctly tests both existing and non-existing device scenarios.
57-96: Test functionTestDeviceStore_ListDeviceseffectively tests the scenarios of no devices and multiple devices.
98-145: Test functionTestDeviceStore_UpdateDeviceNamecorrectly handles scenarios of updating device names for both existing and non-existing devices.core/device/service.go (6)
29-48: Type definitions forServiceinterface anddevicesstruct are clear and well-structured, integrating multiple components effectively.
36-38: Constructor functionNewDevicesis correctly implemented with proper initialization of synchronization mechanisms.
50-60: FunctionInitis well-implemented, correctly setting up dependencies and initializing the store with robust error handling.
66-139: FunctionsRunandloadDevicesare correctly implemented, effectively managing asynchronous device loading with comprehensive error handling and logging.
141-145: FunctionCloseis correctly implemented, ensuring resource cleanup by cancelling the context.
148-184: FunctionsSaveDeviceInfo,UpdateName, andListDevicesare well-implemented, effectively managing device operations with appropriate error handling and integration with other services.core/device/service_test.go (5)
24-91: Test functionTestService_SaveDeviceInfois comprehensive and correctly tests the saving of device information in both the object and the store.
93-158: Test functionTestService_UpdateNameeffectively tests the scenarios of updating device names for both existing and non-existing devices.
160-215: Test functionTestService_ListDevicescorrectly tests the listing of devices, handling scenarios with no devices and multiple devices effectively.
217-292: Test functionTestService_loadDeviceseffectively tests the loading of devices from the tech space and their saving to the store, handling various scenarios appropriately.
294-317: Test functionTestService_Initcorrectly tests the initialization and closure of the service, ensuring error-free startup and shutdown.pb/protos/service/service.proto (2)
351-351: Ensure theDeviceSetNameRPC method is implemented with appropriate request and response structures to handle device name changes effectively.Verification successful
The
DeviceSetNameRPC method is implemented with appropriate request and response structures, and includes robust error handling mechanisms. The method correctly handles the device name change functionality, as evidenced by the detailed implementation in the service handling code and associated protobuf files. Here are the key points:
- Request and Response Structures: The
RpcDeviceSetNameRequestandRpcDeviceSetNameResponseare well-defined, with methods for handling protobuf operations like marshaling and unmarshaling.- Service Method Implementation: The method is implemented in
service.pb.goanddevices.go, showing proper handling of the operation with context and request parameters.- Error Handling: The method uses
RpcDeviceSetNameResponseErrorto handle different error scenarios, which includes error codes and descriptions for better error reporting.Overall, the implementation aligns with the expected functionality for changing the name of a device through the RPC method.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `DeviceSetName` in the service handling code. # Test: Search for the implementation of `DeviceSetName`. Expect: Proper handling of request and response. rg --type go $'DeviceSetName'Length of output: 12215
352-352: Verify that theDeviceListRPC method is correctly implemented to fetch a list of devices, considering potential filters and pagination.pkg/lib/pb/model/protos/models.proto (1)
1285-1290: LGTM! The newDeviceInfomessage is well-structured and aligns with the PR's objectives to enhance device management.core/block/editor/state/state.go (2)
1977-1983: Implementation ofListDeviceslooks correct.This method properly handles the traversal of state hierarchy to fetch device information, returning
nilif no devices are found, which is a safe approach to handle such cases.
2033-2042: Implementation ofGetDevicelooks correct.This method properly handles the retrieval of a device by its ID, correctly traversing the state hierarchy and returning
nilif the device is not found.
| message Device { | ||
| message SetName { | ||
| message Request { | ||
| string deviceId = 1; | ||
| string name = 2; | ||
| } | ||
| message Response { | ||
| Error error = 1; | ||
| message Error { | ||
| Code code = 1; | ||
| string description = 2; | ||
|
|
||
| enum Code { | ||
| NULL = 0; | ||
| UNKNOWN_ERROR = 1; | ||
| BAD_INPUT = 2; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| message List { | ||
| message Request {} | ||
| message Response { | ||
| Error error = 1; | ||
| repeated anytype.model.DeviceInfo devices = 2; | ||
|
|
||
| message Error { | ||
| Code code = 1; | ||
| string description = 2; | ||
|
|
||
| enum Code { | ||
| NULL = 0; | ||
| UNKNOWN_ERROR = 1; | ||
| BAD_INPUT = 2; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of New Device Management RPC Methods
-
Device SetName RPC:
- The
SetNameRPC method allows setting a new name for a device. The request and response structures are well-defined. However, consider adding more specific error codes that could handle more granular errors likeDEVICE_NOT_FOUNDorPERMISSION_DENIED.
- The
-
Device List RPC:
- The
ListRPC method enables listing all devices. The response includes a list ofDeviceInfowhich should encapsulate all necessary device details. Ensure that theDeviceInfomodel is adequately defined to include all relevant device attributes such as device type, status, etc.
- The
- enum Code {
- NULL = 0;
- UNKNOWN_ERROR = 1;
- BAD_INPUT = 2;
- }
+ enum Code {
+ NULL = 0;
+ UNKNOWN_ERROR = 1;
+ BAD_INPUT = 2;
+ DEVICE_NOT_FOUND = 3;
+ PERMISSION_DENIED = 4;
+ }Consider adding more specific error codes to handle cases like device not found or permission issues.
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.
| message Device { | |
| message SetName { | |
| message Request { | |
| string deviceId = 1; | |
| string name = 2; | |
| } | |
| message Response { | |
| Error error = 1; | |
| message Error { | |
| Code code = 1; | |
| string description = 2; | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| } | |
| } | |
| } | |
| } | |
| message List { | |
| message Request {} | |
| message Response { | |
| Error error = 1; | |
| repeated anytype.model.DeviceInfo devices = 2; | |
| message Error { | |
| Code code = 1; | |
| string description = 2; | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| message Device { | |
| message SetName { | |
| message Request { | |
| string deviceId = 1; | |
| string name = 2; | |
| } | |
| message Response { | |
| Error error = 1; | |
| message Error { | |
| Code code = 1; | |
| string description = 2; | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| DEVICE_NOT_FOUND = 3; | |
| PERMISSION_DENIED = 4; | |
| } | |
| } | |
| } | |
| } | |
| message List { | |
| message Request {} | |
| message Response { | |
| Error error = 1; | |
| repeated anytype.model.DeviceInfo devices = 2; | |
| message Error { | |
| Code code = 1; | |
| string description = 2; | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| DEVICE_NOT_FOUND = 3; | |
| PERMISSION_DENIED = 4; | |
| } | |
| } | |
| } | |
| } | |
| } |
| @@ -118,6 +118,7 @@ type State struct { | |||
| localDetails *types.Struct | |||
| relationLinks pbtypes.RelationLinks | |||
| notifications map[string]*model.Notification | |||
| deviceStore map[string]*model.DeviceInfo | |||
There was a problem hiding this comment.
Add documentation for deviceStore.
It's important to document the purpose and usage of new fields in the code, especially for complex structures like State. This helps maintain code readability and ease future maintenance.
core/block/editor/state/change.go
Outdated
| func (s *State) makeDeviceInfoChanges() []*pb.ChangeContent { | ||
| if s.parent == nil && len(s.parent.ListDevices()) == 0 { | ||
| return nil | ||
| } | ||
| var changes []*pb.ChangeContent | ||
| for id, device := range s.deviceStore { | ||
| if d := s.parent.GetDevice(id); d != nil { | ||
| if device.Name != d.Name { | ||
| changes = append(changes, &pb.ChangeContent{ | ||
| Value: &pb.ChangeContentValueOfDeviceUpdate{ | ||
| DeviceUpdate: &pb.ChangeDeviceUpdate{ | ||
| Id: device.Id, | ||
| Name: device.Name, | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
| } else { | ||
| changes = append(changes, &pb.ChangeContent{ | ||
| Value: &pb.ChangeContentValueOfDeviceAdd{ | ||
| DeviceAdd: &pb.ChangeDeviceAdd{Device: device}, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
| return changes | ||
| } |
There was a problem hiding this comment.
Add logging to track device changes in makeDeviceInfoChanges.
+ log.Infof("Device change processed: %s", device.Id)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.
| func (s *State) makeDeviceInfoChanges() []*pb.ChangeContent { | |
| if s.parent == nil && len(s.parent.ListDevices()) == 0 { | |
| return nil | |
| } | |
| var changes []*pb.ChangeContent | |
| for id, device := range s.deviceStore { | |
| if d := s.parent.GetDevice(id); d != nil { | |
| if device.Name != d.Name { | |
| changes = append(changes, &pb.ChangeContent{ | |
| Value: &pb.ChangeContentValueOfDeviceUpdate{ | |
| DeviceUpdate: &pb.ChangeDeviceUpdate{ | |
| Id: device.Id, | |
| Name: device.Name, | |
| }, | |
| }, | |
| }) | |
| } | |
| } else { | |
| changes = append(changes, &pb.ChangeContent{ | |
| Value: &pb.ChangeContentValueOfDeviceAdd{ | |
| DeviceAdd: &pb.ChangeDeviceAdd{Device: device}, | |
| }, | |
| }) | |
| } | |
| } | |
| return changes | |
| } | |
| func (s *State) makeDeviceInfoChanges() []*pb.ChangeContent { | |
| if s.parent == nil && len(s.parent.ListDevices()) == 0 { | |
| return nil | |
| } | |
| var changes []*pb.ChangeContent | |
| for id, device := range s.deviceStore { | |
| if d := s.parent.GetDevice(id); d != nil { | |
| if device.Name != d.Name { | |
| changes = append(changes, &pb.ChangeContent{ | |
| Value: &pb.ChangeContentValueOfDeviceUpdate{ | |
| DeviceUpdate: &pb.ChangeDeviceUpdate{ | |
| Id: device.Id, | |
| Name: device.Name, | |
| }, | |
| }, | |
| }) | |
| log.Infof("Device change processed: %s", device.Id) | |
| } | |
| } else { | |
| changes = append(changes, &pb.ChangeContent{ | |
| Value: &pb.ChangeContentValueOfDeviceAdd{ | |
| DeviceAdd: &pb.ChangeDeviceAdd{Device: device}, | |
| }, | |
| }) | |
| log.Infof("Device change processed: %s", device.Id) | |
| } | |
| } | |
| return changes | |
| } |
| - [Change.DeviceAdd](#anytype-Change-DeviceAdd) | ||
| - [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) |
There was a problem hiding this comment.
Correct the indentation for list items to maintain consistency.
- - [Change.DeviceAdd](#anytype-Change-DeviceAdd)
- - [Change.DeviceUpdate](#anytype-Change-DeviceUpdate)
+ - [Change.DeviceAdd](#anytype-Change-DeviceAdd)
+ - [Change.DeviceUpdate](#anytype-Change-DeviceUpdate)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.
| - [Change.DeviceAdd](#anytype-Change-DeviceAdd) | |
| - [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) | |
| - [Change.DeviceAdd](#anytype-Change-DeviceAdd) | |
| - [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) |
| - [Rpc.Device](#anytype-Rpc-Device) | ||
| - [Rpc.Device.List](#anytype-Rpc-Device-List) | ||
| - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | ||
| - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | ||
| - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | ||
| - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName) | ||
| - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | ||
| - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | ||
| - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) |
There was a problem hiding this comment.
Correct the indentation for list items to maintain consistency.
- - [Rpc.Device](#anytype-Rpc-Device)
- - [Rpc.Device.List](#anytype-Rpc-Device-List)
- - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request)
- - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response)
- - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error)
- - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName)
- - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request)
- - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response)
- - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error)
+ - [Rpc.Device](#anytype-Rpc-Device)
+ - [Rpc.Device.List](#anytype-Rpc-Device-List)
+ - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request)
+ - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response)
+ - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error)
+ - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName)
+ - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request)
+ - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response)
+ - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error)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.
| - [Rpc.Device](#anytype-Rpc-Device) | |
| - [Rpc.Device.List](#anytype-Rpc-Device-List) | |
| - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | |
| - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | |
| - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | |
| - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName) | |
| - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | |
| - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | |
| - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) | |
| - [Rpc.Device](#anytype-Rpc-Device) | |
| - [Rpc.Device.List](#anytype-Rpc-Device-List) | |
| - [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | |
| - [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | |
| - [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | |
| - [Rpc.Device.SetName](#anytype-Rpc-Device-SetName) | |
| - [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | |
| - [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | |
| - [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) |
| | DeviceSetName | [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | | | ||
| | DeviceList | [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | | |
There was a problem hiding this comment.
Remove trailing spaces and ensure there are no multiple consecutive blank lines.
-| DeviceSetName | [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | |
-| DeviceList | [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | |
+| DeviceSetName | [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) |
+| DeviceList | [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | 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.
| | DeviceSetName | [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | | | |
| | DeviceList | [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | | | |
| | DeviceSetName | [Rpc.Device.SetName.Request](#anytype-Rpc-Device-SetName-Request) | [Rpc.Device.SetName.Response](#anytype-Rpc-Device-SetName-Response) | | |
| | DeviceList | [Rpc.Device.List.Request](#anytype-Rpc-Device-List-Request) | [Rpc.Device.List.Response](#anytype-Rpc-Device-List-Response) | |
| | deviceAdd | [Change.DeviceAdd](#anytype-Change-DeviceAdd) | | | | ||
| | deviceUpdate | [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) | | | |
There was a problem hiding this comment.
Ensure there are no multiple consecutive blank lines.
-| deviceAdd | [Change.DeviceAdd](#anytype-Change-DeviceAdd) | | |
-| deviceUpdate | [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) | | |
+| deviceAdd | [Change.DeviceAdd](#anytype-Change-DeviceAdd) | |
+| deviceUpdate | [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) | | 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.
| | deviceAdd | [Change.DeviceAdd](#anytype-Change-DeviceAdd) | | | | |
| | deviceUpdate | [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) | | | | |
| | deviceAdd | [Change.DeviceAdd](#anytype-Change-DeviceAdd) | | | |
| | deviceUpdate | [Change.DeviceUpdate](#anytype-Change-DeviceUpdate) | | |
| <a name="anytype-Change-DeviceAdd"></a> | ||
|
|
||
| ### Change.DeviceAdd | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | device | [model.DeviceInfo](#anytype-model-DeviceInfo) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Change-DeviceUpdate"></a> | ||
|
|
||
| ### Change.DeviceUpdate | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | id | [string](#string) | | | | ||
| | name | [string](#string) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Ensure there are no multiple consecutive blank lines.
-<a name="anytype-Change-DeviceAdd"></a>
-### Change.DeviceAdd
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| device | [model.DeviceInfo](#anytype-model-DeviceInfo) | | |
-<a name="anytype-Change-DeviceUpdate"></a>
-### Change.DeviceUpdate
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| id | [string](#string) | | |
-| name | [string](#string) | | |
+<a name="anytype-Change-DeviceAdd"></a>
+### Change.DeviceAdd
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| device | [model.DeviceInfo](#anytype-model-DeviceInfo) | | |
+<a name="anytype-Change-DeviceUpdate"></a>
+### Change.DeviceUpdate
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| id | [string](#string) | | |
+| name | [string](#string) | | | 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.
| <a name="anytype-Change-DeviceAdd"></a> | |
| ### Change.DeviceAdd | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | device | [model.DeviceInfo](#anytype-model-DeviceInfo) | | | | |
| <a name="anytype-Change-DeviceUpdate"></a> | |
| ### Change.DeviceUpdate | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | id | [string](#string) | | | | |
| | name | [string](#string) | | | | |
| <a name="anytype-Change-DeviceAdd"></a> | |
| ### Change.DeviceAdd | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | device | [model.DeviceInfo](#anytype-model-DeviceInfo) | | | | |
| <a name="anytype-Change-DeviceUpdate"></a> | |
| ### Change.DeviceUpdate | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | id | [string](#string) | | | | |
| | name | [string](#string) | | | |
|
|
||
| <a name="anytype-Rpc-Device-List-Request"></a> | ||
|
|
||
| ### Rpc.Device.List.Request | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-List-Response"></a> | ||
|
|
||
| ### Rpc.Device.List.Response | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | error | [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | | | | ||
| | devices | [model.DeviceInfo](#anytype-model-DeviceInfo) | repeated | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-List-Response-Error"></a> | ||
|
|
||
| ### Rpc.Device.List.Response.Error | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | code | [Rpc.Device.List.Response.Error.Code](#anytype-Rpc-Device-List-Response-Error-Code) | | | | ||
| | description | [string](#string) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-SetName"></a> | ||
|
|
||
| ### Rpc.Device.SetName | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-SetName-Request"></a> | ||
|
|
||
| ### Rpc.Device.SetName.Request | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | deviceId | [string](#string) | | | | ||
| | name | [string](#string) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-SetName-Response"></a> | ||
|
|
||
| ### Rpc.Device.SetName.Response | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | error | [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-SetName-Response-Error"></a> | ||
|
|
||
| ### Rpc.Device.SetName.Response.Error | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | code | [Rpc.Device.SetName.Response.Error.Code](#anytype-Rpc-Device-SetName-Response-Error-Code) | | | | ||
| | description | [string](#string) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Ensure there are no multiple consecutive blank lines.
-<a name="anytype-Rpc-Device"></a>
-### Rpc.Device
-<a name="anytype-Rpc-Device-List"></a>
-### Rpc.Device.List
-<a name="anytype-Rpc-Device-List-Request"></a>
-### Rpc.Device.List.Request
-<a name="anytype-Rpc-Device-List-Response"></a>
-### Rpc.Device.List.Response
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| error | [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | | |
-| devices | [model.DeviceInfo](#anytype-model-DeviceInfo) | repeated | |
-<a name="anytype-Rpc-Device-List-Response-Error"></a>
-### Rpc.Device.List.Response.Error
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| code | [Rpc.Device.List.Response.Error.Code](#anytype-Rpc-Device-List-Response-Error-Code) | | |
-| description | [string](#string) | | |
-<a name="anytype-Rpc-Device-SetName"></a>
-### Rpc.Device.SetName
-<a name="anytype-Rpc-Device-SetName-Request"></a>
-### Rpc.Device.SetName.Request
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| deviceId | [string](#string) | | |
-| name | [string](#string) | | |
-<a name="anytype-Rpc-Device-SetName-Response"></a>
-### Rpc.Device.SetName.Response
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| error | [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) | | |
-<a name="anytype-Rpc-Device-SetName-Response-Error"></a>
-### Rpc.Device.SetName.Response.Error
-| Field | Type | Label | Description |
-| ----- | ---- | ----- | ----------- |
-| code | [Rpc.Device.SetName.Response.Error.Code](#anytype-Rpc-Device-SetName-Response-Error-Code) | | |
-| description | [string](#string) | | |
+<a name="anytype-Rpc-Device"></a>
+### Rpc.Device
+<a name="anytype-Rpc-Device-List"></a>
+### Rpc.Device.List
+<a name="anytype-Rpc-Device-List-Request"></a>
+### Rpc.Device.List.Request
+<a name="anytype-Rpc-Device-List-Response"></a>
+### Rpc.Device.List.Response
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| error | [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | | |
+| devices | [model.DeviceInfo](#anytype-model-DeviceInfo) | repeated | |
+<a name="anytype-Rpc-Device-List-Response-Error"></a>
+### Rpc.Device.List.Response.Error
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| code | [Rpc.Device.List.Response.Error.Code](#anytype-Rpc-Device-List-Response-Error-Code) | | |
+| description | [string](#string) | | |
+<a name="anytype-Rpc-Device-SetName"></a>
+### Rpc.Device.SetName
+<a name="anytype-Rpc-Device-SetName-Request"></a>
+### Rpc.Device.SetName.Request
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| deviceId | [string](#string) | | |
+| name | [string](#string) | | |
+<a name="anytype-Rpc-Device-SetName-Response"></a>
+### Rpc.Device.SetName.Response
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| error | [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) | | |
+<a name="anytype-Rpc-Device-SetName-Response-Error"></a>
+### Rpc.Device.SetName.Response.Error
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| code | [Rpc.Device.SetName.Response.Error.Code](#anytype-Rpc-Device-SetName-Response-Error-Code) | | |
+| description | [string](#string) | | | 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.
| <a name="anytype-Rpc-Device"></a> | |
| ### Rpc.Device | |
| <a name="anytype-Rpc-Device-List"></a> | |
| ### Rpc.Device.List | |
| <a name="anytype-Rpc-Device-List-Request"></a> | |
| ### Rpc.Device.List.Request | |
| <a name="anytype-Rpc-Device-List-Response"></a> | |
| ### Rpc.Device.List.Response | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | error | [Rpc.Device.List.Response.Error](#anytype-Rpc-Device-List-Response-Error) | | | | |
| | devices | [model.DeviceInfo](#anytype-model-DeviceInfo) | repeated | | | |
| <a name="anytype-Rpc-Device-List-Response-Error"></a> | |
| ### Rpc.Device.List.Response.Error | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | code | [Rpc.Device.List.Response.Error.Code](#anytype-Rpc-Device-List-Response-Error-Code) | | | | |
| | description | [string](#string) | | | | |
| <a name="anytype-Rpc-Device-SetName"></a> | |
| ### Rpc.Device.SetName | |
| <a name="anytype-Rpc-Device-SetName-Request"></a> | |
| ### Rpc.Device.SetName.Request | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | deviceId | [string](#string) | | | | |
| | name | [string](#string) | | | | |
| <a name="anytype-Rpc-Device-SetName-Response"></a> | |
| ### Rpc.Device.SetName.Response | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | error | [Rpc.Device.SetName.Response.Error](#anytype-Rpc-Device-SetName-Response-Error) | | | | |
| <a name="anytype-Rpc-Device-SetName-Response-Error"></a> | |
| ### Rpc.Device.SetName.Response.Error | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | code | [Rpc.Device.SetName.Response.Error.Code](#anytype-Rpc-Device-SetName-Response-Error-Code) | | | | |
| | description | [string](#string) | | | |
| <a name="anytype-Rpc-Device-List-Response-Error-Code"></a> | ||
|
|
||
| ### Rpc.Device.List.Response.Error.Code | ||
|
|
||
|
|
||
| | Name | Number | Description | | ||
| | ---- | ------ | ----------- | | ||
| | NULL | 0 | | | ||
| | UNKNOWN_ERROR | 1 | | | ||
| | BAD_INPUT | 2 | | | ||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-Device-SetName-Response-Error-Code"></a> | ||
|
|
||
| ### Rpc.Device.SetName.Response.Error.Code | ||
|
|
||
|
|
||
| | Name | Number | Description | | ||
| | ---- | ------ | ----------- | | ||
| | NULL | 0 | | | ||
| | UNKNOWN_ERROR | 1 | | | ||
| | BAD_INPUT | 2 | | | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Ensure there are no multiple consecutive blank lines.
-<a name="anytype-Rpc-Device-List-Response-Error-Code"></a>
-### Rpc.Device.List.Response.Error.Code
-| Name | Number | Description |
-| ---- | ------ | ----------- |
-| NULL | 0 | |
-| UNKNOWN_ERROR | 1 | |
-| BAD_INPUT | 2 | |
-<a name="anytype-Rpc-Device-SetName-Response-Error-Code"></a>
-### Rpc.Device.SetName.Response.Error.Code
-| Name | Number | Description |
-| ---- | ------ | ----------- |
-| NULL | 0 | |
-| UNKNOWN_ERROR | 1 | |
-| BAD_INPUT | 2 | |
+<a name="anytype-Rpc-Device-List-Response-Error-Code"></a>
+### Rpc.Device.List.Response.Error.Code
+| Name | Number | Description |
+| ---- | ------ | ----------- |
+| NULL | 0 | |
+| UNKNOWN_ERROR | 1 | |
+| BAD_INPUT | 2 | |
+<a name="anytype-Rpc-Device-SetName-Response-Error-Code"></a>
+### Rpc.Device.SetName.Response.Error.Code
+| Name | Number | Description |
+| ---- | ------ | ----------- |
+| NULL | 0 | |
+| UNKNOWN_ERROR | 1 | |
+| BAD_INPUT | 2 | | 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.
| <a name="anytype-Rpc-Device-List-Response-Error-Code"></a> | |
| ### Rpc.Device.List.Response.Error.Code | |
| | Name | Number | Description | | |
| | ---- | ------ | ----------- | | |
| | NULL | 0 | | | |
| | UNKNOWN_ERROR | 1 | | | |
| | BAD_INPUT | 2 | | | |
| <a name="anytype-Rpc-Device-SetName-Response-Error-Code"></a> | |
| ### Rpc.Device.SetName.Response.Error.Code | |
| | Name | Number | Description | | |
| | ---- | ------ | ----------- | | |
| | NULL | 0 | | | |
| | UNKNOWN_ERROR | 1 | | | |
| | BAD_INPUT | 2 | | | |
| <a name="anytype-Rpc-Device-List-Response-Error-Code"></a> | |
| ### Rpc.Device.List.Response.Error.Code | |
| | Name | Number | Description | | |
| | ---- | ------ | ----------- | | |
| | NULL | 0 | | | |
| | UNKNOWN_ERROR | 1 | | | |
| | BAD_INPUT | 2 | | | |
| <a name="anytype-Rpc-Device-SetName-Response-Error-Code"></a> | |
| ### Rpc.Device.SetName.Response.Error.Code | |
| | Name | Number | Description | | |
| | ---- | ------ | ----------- | | |
| | NULL | 0 | | | |
| | UNKNOWN_ERROR | 1 | | | |
| | BAD_INPUT | 2 | | |
| func hasChanges(changes []*pb.ChangeContent) bool { | ||
| for _, ch := range changes { | ||
| if isStoreOrNotificationChanges(ch) { | ||
| if isSuitableChanges(ch) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
It's not clear from the name of the func what hasChanges actually does. It's in the context of the sb package, not notifications
There was a problem hiding this comment.
We can rename hasChanges to something like hasChangesToPush, because we can check such changes in that function: store, notifications, devices changes
core/block/editor/state/change.go
Outdated
| @@ -823,6 +845,34 @@ func (s *State) makeNotificationChanges() []*pb.ChangeContent { | |||
| return changes | |||
| } | |||
|
|
|||
| func (s *State) makeDeviceInfoChanges() []*pb.ChangeContent { | |||
| if s.parent == nil && len(s.parent.ListDevices()) == 0 { | |||
There was a problem hiding this comment.
- Nil deref here(in case s.parent is nil)
- Also for the first change should it work fine if the parent is nil but we have devices in the current state?
There was a problem hiding this comment.
Fixed and add test
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.godocs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (6)
- core/anytype/bootstrap.go (2 hunks)
- core/block/editor/smartblock/smartblock.go (1 hunks)
- core/block/editor/state/state.go (4 hunks)
- core/block/editor/state/state_test.go (1 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
Additional comments not posted (11)
core/anytype/bootstrap.go (2)
56-56: Ensure the newdevicepackage is correctly integrated and used within the application.
286-286: The registration ofNewDevices()from thedevicepackage is a crucial part of integrating the new device management functionality. Verify that theNewDevices()function initializes all necessary components and adheres to the expected lifecycle management (initialization, running, and closure) as outlined in the PR objectives.pb/protos/service/service.proto (1)
352-353: The addition of theDeviceSetNameandDeviceListRPC methods is aligned with the PR objectives to enhance device management capabilities. Ensure that these methods are implemented with the correct request and response types and that they integrate seamlessly with the existing service architecture.core/block/editor/state/state.go (4)
121-121: Add documentation fordeviceStore.
2002-2017: Clarify behavior when adding an existing device.
2019-2040: Consider handling the case where a device does not exist.
1986-1992: The implementation ofListDevicescorrectly handles cases where no device information is available by returning nil. This is a safe and expected behavior in Go.core/block/editor/state/state_test.go (4)
2614-2632: The existing comment is still valid and correctly suggests adding an assertion to ensure the device name remains unchanged when adding an existing device.
2635-2673: The tests forGetDevicemethod are comprehensive and cover various scenarios including non-existent and existent device retrieval.
2676-2701: The tests forListDevicesmethod are comprehensive and cover scenarios with no devices and with devices present.
2614-2632: Consider adding a test to ensure that adding a device with an existing ID does not overwrite the existing device's details unless explicitly intended.Verification successful
The implementation of the
AddDevicefunction incore/block/editor/state/state.goconfirms that if a device with the same ID already exists in thedeviceStore, the function returns early without overwriting the existing device's details. This behavior is consistent with the concern raised in the review comment.Additionally, the test case
add device - device existincore/block/editor/state/state_test.goverifies this behavior by asserting that the existing device's details are not overwritten when a device with the same ID is added.Therefore, the current implementation and test coverage already address the concern mentioned in the review comment.
core/block/editor/state/state.go:
- The
AddDevicefunction checks if a device with the same ID exists and returns early if it does, preventing overwriting.core/block/editor/state/state_test.go:
- The test case
add device - device existensures that adding a device with an existing ID does not overwrite the existing device's details.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that adding a device with an existing ID does not overwrite the existing device's details. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'AddDevice'Length of output: 1054
Script:
#!/bin/bash # Retrieve the implementation of the AddDevice function in core/block/editor/state/state.go ast-grep --lang go --pattern $'func (s *State) AddDevice(device *model.DeviceInfo) { $$$ }' --json # Retrieve the test cases in core/block/editor/state/state_test.go that involve AddDevice ast-grep --lang go --pattern $'func TestState_AddDevice(t *testing.T) { $$$ }' --jsonLength of output: 3259
| if isSuitableChanges(ch) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func isStoreOrNotificationChanges(ch *pb.ChangeContent) bool { | ||
| func isSuitableChanges(ch *pb.ChangeContent) bool { | ||
| return ch.GetStoreKeySet() != nil || | ||
| ch.GetStoreKeyUnset() != nil || | ||
| ch.GetStoreSliceUpdate() != nil || | ||
| ch.GetNotificationCreate() != nil || | ||
| ch.GetNotificationUpdate() != nil | ||
| ch.GetNotificationUpdate() != nil || | ||
| ch.GetDeviceUpdate() != nil || | ||
| ch.GetDeviceAdd() != nil |
There was a problem hiding this comment.
Clarify the purpose of isSuitableChanges function.
The function isSuitableChanges checks various conditions to determine if a change is "suitable". However, the name of the function and its implementation are not immediately clear, especially in the context of the smart block package. Consider renaming this function to more accurately reflect its purpose, such as hasRelevantChangeTypes, and ensure that the function's documentation clearly describes what makes a change "suitable".
| message List { | ||
| message Request {} | ||
| message Response { | ||
| Error error = 1; | ||
| repeated anytype.model.DeviceInfo devices = 2; | ||
|
|
||
| message Error { | ||
| Code code = 1; | ||
| string description = 2; | ||
|
|
||
| enum Code { | ||
| NULL = 0; | ||
| UNKNOWN_ERROR = 1; | ||
| BAD_INPUT = 2; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding more specific error codes for Device.List.
enum Code {
NULL = 0;
UNKNOWN_ERROR = 1;
BAD_INPUT = 2;
+ NO_DEVICES_FOUND = 3;
}This addition will help handle cases where no devices are found, providing clearer feedback to the 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.
| message List { | |
| message Request {} | |
| message Response { | |
| Error error = 1; | |
| repeated anytype.model.DeviceInfo devices = 2; | |
| message Error { | |
| Code code = 1; | |
| string description = 2; | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| } | |
| } | |
| } | |
| } | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| NO_DEVICES_FOUND = 3; | |
| } |
| message Device { | ||
| message SetName { | ||
| message Request { | ||
| string deviceId = 1; | ||
| string name = 2; | ||
| } | ||
| message Response { | ||
| Error error = 1; | ||
| message Error { | ||
| Code code = 1; | ||
| string description = 2; | ||
|
|
||
| enum Code { | ||
| NULL = 0; | ||
| UNKNOWN_ERROR = 1; | ||
| BAD_INPUT = 2; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding more specific error codes for Device.SetName.
enum Code {
NULL = 0;
UNKNOWN_ERROR = 1;
BAD_INPUT = 2;
+ DEVICE_NOT_FOUND = 3;
+ PERMISSION_DENIED = 4;
}This addition will help handle cases where the device is not found or the user does not have the necessary permissions to change the device name.
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.
| message Device { | |
| message SetName { | |
| message Request { | |
| string deviceId = 1; | |
| string name = 2; | |
| } | |
| message Response { | |
| Error error = 1; | |
| message Error { | |
| Code code = 1; | |
| string description = 2; | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| } | |
| } | |
| } | |
| enum Code { | |
| NULL = 0; | |
| UNKNOWN_ERROR = 1; | |
| BAD_INPUT = 2; | |
| DEVICE_NOT_FOUND = 3; | |
| PERMISSION_DENIED = 4; | |
| } |
| func (s *State) SetDeviceName(id, name string) { | ||
| if s.deviceStore == nil { | ||
| s.deviceStore = map[string]*model.DeviceInfo{} | ||
| } | ||
| if s.parent != nil { | ||
| for _, d := range s.parent.ListDevices() { | ||
| if _, ok := s.deviceStore[d.Id]; !ok { | ||
| s.deviceStore[d.Id] = pbtypes.CopyDevice(d) | ||
| } | ||
| } | ||
| } | ||
| if _, ok := s.deviceStore[id]; !ok { | ||
| device := &model.DeviceInfo{ | ||
| Id: id, | ||
| Name: name, | ||
| AddDate: time.Now().Unix(), | ||
| } | ||
| s.deviceStore[id] = device | ||
| return | ||
| } | ||
| s.deviceStore[id].Name = name | ||
| } |
There was a problem hiding this comment.
The behavior of SetDeviceName might be unexpected as it creates a new device if one does not exist. Consider adding error handling or a return value to inform the caller about the creation of a new device.
- func (s *State) SetDeviceName(id, name string) {
+ func (s *State) SetDeviceName(id, name string) error {
if s.deviceStore == nil {
s.deviceStore = map[string]*model.DeviceInfo{}
}
if _, ok := s.deviceStore[id]; !ok {
device := &model.DeviceInfo{
Id: id,
Name: name,
AddDate: time.Now().Unix(),
}
s.deviceStore[id] = device
- return
+ return fmt.Errorf("device created as it did not exist")
}
s.deviceStore[id].Name = name
+ return 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.
| func (s *State) SetDeviceName(id, name string) { | |
| if s.deviceStore == nil { | |
| s.deviceStore = map[string]*model.DeviceInfo{} | |
| } | |
| if s.parent != nil { | |
| for _, d := range s.parent.ListDevices() { | |
| if _, ok := s.deviceStore[d.Id]; !ok { | |
| s.deviceStore[d.Id] = pbtypes.CopyDevice(d) | |
| } | |
| } | |
| } | |
| if _, ok := s.deviceStore[id]; !ok { | |
| device := &model.DeviceInfo{ | |
| Id: id, | |
| Name: name, | |
| AddDate: time.Now().Unix(), | |
| } | |
| s.deviceStore[id] = device | |
| return | |
| } | |
| s.deviceStore[id].Name = name | |
| } | |
| func (s *State) SetDeviceName(id, name string) error { | |
| if s.deviceStore == nil { | |
| s.deviceStore = map[string]*model.DeviceInfo{} | |
| } | |
| if s.parent != nil { | |
| for _, d := range s.parent.ListDevices() { | |
| if _, ok := s.deviceStore[d.Id]; !ok { | |
| s.deviceStore[d.Id] = pbtypes.CopyDevice(d) | |
| } | |
| } | |
| } | |
| if _, ok := s.deviceStore[id]; !ok { | |
| device := &model.DeviceInfo{ | |
| Id: id, | |
| Name: name, | |
| AddDate: time.Now().Unix(), | |
| } | |
| s.deviceStore[id] = device | |
| return fmt.Errorf("device created as it did not exist") | |
| } | |
| s.deviceStore[id].Name = name | |
| return nil | |
| } |
| func (s *State) AddDevice(device *model.DeviceInfo) { | ||
| if s.deviceStore == nil { | ||
| s.deviceStore = map[string]*model.DeviceInfo{} | ||
| } | ||
| if s.parent != nil { | ||
| for _, d := range s.parent.ListDevices() { | ||
| if _, ok := s.deviceStore[d.Id]; !ok { | ||
| s.deviceStore[d.Id] = pbtypes.CopyDevice(d) | ||
| } | ||
| } | ||
| } | ||
| if _, ok := s.deviceStore[device.Id]; ok { | ||
| return | ||
| } | ||
| s.deviceStore[device.Id] = device | ||
| } |
There was a problem hiding this comment.
Consider modifying AddDevice to return a boolean indicating whether the addition was successful. This can help callers handle cases where the device already exists more gracefully.
- func (s *State) AddDevice(device *model.DeviceInfo) {
+ func (s *State) AddDevice(device *model.DeviceInfo) bool {
if s.deviceStore == nil {
s.deviceStore = map[string]*model.DeviceInfo{}
}
if _, ok := s.deviceStore[device.Id]; ok {
- return
+ return false
}
s.deviceStore[device.Id] = device
+ return true
}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.
| func (s *State) AddDevice(device *model.DeviceInfo) { | |
| if s.deviceStore == nil { | |
| s.deviceStore = map[string]*model.DeviceInfo{} | |
| } | |
| if s.parent != nil { | |
| for _, d := range s.parent.ListDevices() { | |
| if _, ok := s.deviceStore[d.Id]; !ok { | |
| s.deviceStore[d.Id] = pbtypes.CopyDevice(d) | |
| } | |
| } | |
| } | |
| if _, ok := s.deviceStore[device.Id]; ok { | |
| return | |
| } | |
| s.deviceStore[device.Id] = device | |
| } | |
| func (s *State) AddDevice(device *model.DeviceInfo) bool { | |
| if s.deviceStore == nil { | |
| s.deviceStore = map[string]*model.DeviceInfo{} | |
| } | |
| if s.parent != nil { | |
| for _, d := range s.parent.ListDevices() { | |
| if _, ok := s.deviceStore[d.Id]; !ok { | |
| s.deviceStore[d.Id] = pbtypes.CopyDevice(d) | |
| } | |
| } | |
| } | |
| if _, ok := s.deviceStore[device.Id]; ok { | |
| return false | |
| } | |
| s.deviceStore[device.Id] = device | |
| return true | |
| } |
| func TestState_SetDeviceName(t *testing.T) { | ||
| t.Run("set device name, device not exist", func(t *testing.T) { | ||
| // given | ||
| st := NewDoc("root", nil).(*State) | ||
|
|
||
| // when | ||
| st.SetDeviceName("id", "test") | ||
|
|
||
| // then | ||
| assert.NotNil(t, st.deviceStore["id"]) | ||
| assert.Equal(t, st.deviceStore["id"].Name, "test") | ||
| }) | ||
|
|
||
| t.Run("set device name, device exists", func(t *testing.T) { | ||
| // given | ||
| st := NewDoc("root", nil).(*State) | ||
| st.AddDevice(&model.DeviceInfo{ | ||
| Id: "id", | ||
| Name: "test", | ||
| }) | ||
|
|
||
| newState := st.NewState() | ||
| // when | ||
| newState.SetDeviceName("id", "test1") | ||
|
|
||
| // then | ||
| assert.NotNil(t, newState.deviceStore["id"]) | ||
| assert.Equal(t, newState.deviceStore["id"].Name, "test1") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider modifying the behavior of SetDeviceName to avoid creating a new device if it does not exist, as this could lead to unintended side effects.
- assert.NotNil(t, st.deviceStore["id"])
- assert.Equal(t, st.deviceStore["id"].Name, "test")
+ assert.Nil(t, st.deviceStore["id"])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.
| func TestState_SetDeviceName(t *testing.T) { | |
| t.Run("set device name, device not exist", func(t *testing.T) { | |
| // given | |
| st := NewDoc("root", nil).(*State) | |
| // when | |
| st.SetDeviceName("id", "test") | |
| // then | |
| assert.NotNil(t, st.deviceStore["id"]) | |
| assert.Equal(t, st.deviceStore["id"].Name, "test") | |
| }) | |
| t.Run("set device name, device exists", func(t *testing.T) { | |
| // given | |
| st := NewDoc("root", nil).(*State) | |
| st.AddDevice(&model.DeviceInfo{ | |
| Id: "id", | |
| Name: "test", | |
| }) | |
| newState := st.NewState() | |
| // when | |
| newState.SetDeviceName("id", "test1") | |
| // then | |
| assert.NotNil(t, newState.deviceStore["id"]) | |
| assert.Equal(t, newState.deviceStore["id"].Name, "test1") | |
| }) | |
| } | |
| func TestState_SetDeviceName(t *testing.T) { | |
| t.Run("set device name, device not exist", func(t *testing.T) { | |
| // given | |
| st := NewDoc("root", nil).(*State) | |
| // when | |
| st.SetDeviceName("id", "test") | |
| // then | |
| assert.Nil(t, st.deviceStore["id"]) | |
| }) | |
| t.Run("set device name, device exists", func(t *testing.T) { | |
| // given | |
| st := NewDoc("root", nil).(*State) | |
| st.AddDevice(&model.DeviceInfo{ | |
| Id: "id", | |
| Name: "test", | |
| }) | |
| newState := st.NewState() | |
| // when | |
| newState.SetDeviceName("id", "test1") | |
| // then | |
| assert.NotNil(t, newState.deviceStore["id"]) | |
| assert.Equal(t, newState.deviceStore["id"].Name, "test1") | |
| }) | |
| } |
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
pkg/lib/pb/model/protos/models.proto (2)
Line range hint
1196-1213: Add documentation for newMembership.Statusenum values.It's good practice to document the purpose and use-case of each enum value to aid understanding and maintenance. Consider adding comments explaining when each status should be used.
1213-1213: Ensure consistency in enum naming conventions.The
PaymentMethodenum uses a prefixMethodwhich is redundant since it's already within an enum namedPaymentMethod. Consider simplifying the names toNone,Stripe,Crypto,InappApple, andInappGooglefor clarity and consistency.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.godocs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/lib/pb/model/models.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (6)
- core/block/editor/smartblock/smartblock.go (2 hunks)
- core/block/editor/state/change.go (4 hunks)
- core/block/editor/state/change_test.go (1 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- pkg/lib/pb/model/protos/models.proto (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- core/block/editor/smartblock/smartblock.go
- pb/protos/commands.proto
- pb/protos/service/service.proto
Additional comments not posted (8)
core/block/editor/state/change_test.go (5)
811-881: Review ofTestRootDeviceChanges:
- No Changes Test: Validates no changes occur with an empty state. This is a good baseline test.
- Add New Device Test: Checks the addition of a new device. It correctly asserts the changes length and the device added.
- Update Device Test: Verifies updating a device's name. It properly checks the change and updated name.
- Add Device - Parent Nil Test: Tests adding a device when the parent is nil. This scenario is less typical and should be documented why it's important to test this edge case.
Line range hint
32-58: Review ofTestState_ChangesCreate_MoveAdd:
- This test effectively checks the movement and addition of blocks within a state. It uses assertions to verify the correct number of changes and the specific types of changes.
- The use of
requirefor preconditions andassertfor postconditions is correctly applied.
Line range hint
60-71: Review ofTestState_ChangesCreate_Collection_Set:
- The test correctly sets a value in the state's store and checks the resulting changes.
- The assertion comparing the change's string representation is a robust way to ensure the change is exactly as expected.
Line range hint
73-84: Review ofTestState_ChangesCreate_Collection_Unset:
- This test effectively checks the removal of a value from the state's store.
- The use of
require.NoErrorto ensure no errors occur during the state application is appropriate.
Line range hint
86-142: Review ofTestState_ChangesCreate_StoreSlice:
- Comprehensive testing of store slice operations (add, remove, move).
- The structured approach using table-driven tests enhances maintainability and clarity.
- Each scenario is well-documented with comments, and the assertions are thorough.
core/block/editor/state/change.go (1)
Line range hint
634-874: Review the logic for generating device info changes.The method
makeDeviceInfoChangesis crucial for handling device updates and additions. It's important to verify that this method handles all edge cases correctly, especially in scenarios where devices are updated or added.pkg/lib/pb/model/protos/models.proto (2)
Line range hint
1255-1273: Review the new fields inMembershipTierData.The new fields and enum values in
MembershipTierDataseem well-defined and relevant to the functionality described in the PR. Ensure that all new fields are properly integrated and tested in the system.
1301-1306: Validate the newDeviceInfomessage structure.The structure of the new
DeviceInfomessage looks appropriate for storing device-related information. Ensure that all fields are used effectively and that the data integrity is maintained when devices are added, updated, or queried.
| func (s *State) updateDevice(update *pb.ChangeDeviceUpdate) { | ||
| if s.deviceStore == nil { | ||
| return | ||
| } | ||
| if _, ok := s.deviceStore[update.Id]; !ok { | ||
| return | ||
| } | ||
| s.deviceStore[update.Id].Name = update.Name | ||
| } |
There was a problem hiding this comment.
Enhance error handling and logging in updateDevice.
+ if _, ok := s.deviceStore[update.Id]; !ok {
+ log.Errorf("Device not found: %s", update.Id)
+ return fmt.Errorf("device not found: %s", update.Id)
+ }
+ log.Infof("Device updated: %s", update.Id)Improving error handling in the updateDevice method ensures that updates are only applied to existing devices, and provides logging for both error and normal cases, enhancing traceability and error diagnosis.
Committable suggestion was skipped due low confidence.
| func (s *State) addDevice(deviceInfo *model.DeviceInfo) { | ||
| if s.deviceStore == nil { | ||
| s.deviceStore = map[string]*model.DeviceInfo{} | ||
| } | ||
| s.deviceStore[deviceInfo.Id] = deviceInfo | ||
| } |
There was a problem hiding this comment.
Consider adding error handling and logging for addDevice.
+ if err := validateDeviceInfo(deviceInfo); err != nil {
+ log.Errorf("Invalid device info: %v", err)
+ return err
+ }Adding error handling and logging in the addDevice method can help identify issues with device information at the time of addition, improving the robustness of device management.
Committable suggestion was skipped due low confidence.
| case ch.GetDeviceAdd() != nil: | ||
| s.addDevice(ch.GetDeviceAdd().GetDevice()) | ||
| case ch.GetDeviceUpdate() != nil: | ||
| s.updateDevice(ch.GetDeviceUpdate()) |
There was a problem hiding this comment.
Ensure proper error handling for device-related changes.
+ if err := s.validateDevice(ch.GetDeviceAdd().GetDevice()); err != nil {
+ return fmt.Errorf("failed to add device: %v", err)
+ }
+ if err := s.validateDeviceUpdate(ch.GetDeviceUpdate()); err != nil {
+ return fmt.Errorf("failed to update device: %v", err)
+ }Consider adding error handling to ensure that device additions and updates are validated before they are processed. This can help prevent issues related to invalid device data being added or updated in the state.
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.
| case ch.GetDeviceAdd() != nil: | |
| s.addDevice(ch.GetDeviceAdd().GetDevice()) | |
| case ch.GetDeviceUpdate() != nil: | |
| s.updateDevice(ch.GetDeviceUpdate()) | |
| case ch.GetDeviceAdd() != nil: | |
| if err := s.validateDevice(ch.GetDeviceAdd().GetDevice()); err != nil { | |
| return fmt.Errorf("failed to add device: %v", err) | |
| } | |
| s.addDevice(ch.GetDeviceAdd().GetDevice()) | |
| case ch.GetDeviceUpdate() != nil: | |
| if err := s.validateDeviceUpdate(ch.GetDeviceUpdate()); err != nil { | |
| return fmt.Errorf("failed to update device: %v", err) | |
| } | |
| s.updateDevice(ch.GetDeviceUpdate()) |
…o-2930-store-device-names # Conflicts: # pb/commands.pb.go
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
docs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (1)
- pb/protos/commands.proto (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pb/protos/commands.proto
…o-2930-store-device-names # Conflicts: # clientlibrary/service/service.pb.go # pb/commands.pb.go # pb/service/service.pb.go # pkg/lib/pb/model/models.pb.go
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.godocs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/lib/pb/model/models.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (3)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- pkg/lib/pb/model/protos/models.proto (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- pb/protos/commands.proto
- pb/protos/service/service.proto
- pkg/lib/pb/model/protos/models.proto
…o-2930-store-device-names # Conflicts: # pb/commands.pb.go # pkg/lib/pb/model/models.pb.go
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
docs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/lib/pb/model/models.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (2)
- pb/protos/commands.proto (1 hunks)
- pkg/lib/pb/model/protos/models.proto (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- pb/protos/commands.proto
- pkg/lib/pb/model/protos/models.proto
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
core/device/service.go (1)
29-34: Ensure theServiceinterface is well-documented.Adding documentation comments for the
Serviceinterface would improve code maintainability and clarity, especially for new developers or external contributors.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/device/service.go (1 hunks)
Additional comments not posted (1)
core/device/service.go (1)
183-186: Verify the synchronization mechanism inListDevices.Ensure that the
finishLoadchannel is correctly synchronized across all operations that depend on device loading.Verification successful
To verify the synchronization mechanism of the
finishLoadchannel in theListDevicesmethod, we need to examine the context around its usage. Specifically, we should look at the initialization, closing, and waiting on this channel to ensure there are no synchronization issues.
To ensure proper synchronization of the
finishLoadchannel, we need to correct the regex error and gather the necessary context around its initialization, defer close, and waiting.
To ensure proper synchronization of the
finishLoadchannel, we need to simplify the regex pattern and gather the necessary context around its initialization, defer close, and waiting.
To ensure proper synchronization of the
finishLoadchannel, we need to re-run the search for the defer close statement and gather the necessary context.
To ensure proper synchronization of the
finishLoadchannel, we need to search for any close statements for thefinishLoadchannel to gather the necessary context.
To ensure proper synchronization of the
finishLoadchannel, we need to search for any other potential mechanisms that might close thefinishLoadchannel.
The
finishLoadchannel is properly synchronized in theListDevicesmethod. The channel is initialized in theNewDevicesfunction and closed in theloadDevicesmethod using adeferstatement. This ensures that the channel is closed once the device loading process is complete, preventing any potential deadlocks or race conditions.
finishLoadis initialized in theNewDevicesfunction.finishLoadis closed in theloadDevicesmethod usingdefer close(d.finishLoad).ListDeviceswaits onfinishLoadto ensure devices are loaded before proceeding.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper synchronization of `finishLoad` channel in device loading. # Test: Search for all usages of `finishLoad`. Expect: Proper synchronization without deadlocks. rg --type go $'finishLoad'Length of output: 485
Script:
#!/bin/bash # Description: Gather context around the usage of `finishLoad` channel to ensure proper synchronization. # Find the initialization of the `finishLoad` channel. rg --context 5 'finishLoad: make(chan struct{})' core/device/service.go # Find the defer close statement for the `finishLoad` channel. rg --context 5 'defer close(d.finishLoad)' core/device/service.go # Find the waiting on the `finishLoad` channel. rg --context 5 '<-d.finishLoad' core/device/service.goLength of output: 500
Script:
#!/bin/bash # Description: Gather context around the usage of `finishLoad` channel to ensure proper synchronization. # Find the initialization of the `finishLoad` channel. rg --context 5 'finishLoad:\s*make\(chan struct{}\)' core/device/service.go # Find the defer close statement for the `finishLoad` channel. rg --context 5 'defer close(d.finishLoad)' core/device/service.go # Find the waiting on the `finishLoad` channel. rg --context 5 '<-d.finishLoad' core/device/service.goLength of output: 511
Script:
#!/bin/bash # Description: Gather context around the usage of `finishLoad` channel to ensure proper synchronization. # Find the initialization of the `finishLoad` channel. rg --context 5 'finishLoad: make' core/device/service.go # Find the defer close statement for the `finishLoad` channel. rg --context 5 'defer close(d.finishLoad)' core/device/service.go # Find the waiting on the `finishLoad` channel. rg --context 5 '<-d.finishLoad' core/device/service.goLength of output: 633
Script:
#!/bin/bash # Description: Gather context around the usage of `finishLoad` channel to ensure proper synchronization. # Find the defer close statement for the `finishLoad` channel. rg --context 10 'defer close(d.finishLoad)' core/device/service.goLength of output: 68
Script:
#!/bin/bash # Description: Gather context around the usage of `finishLoad` channel to ensure proper synchronization. # Find any close statements for the `finishLoad` channel. rg --context 10 'close(d.finishLoad)' core/device/service.goLength of output: 62
Script:
#!/bin/bash # Description: Gather context around the usage of `finishLoad` channel to ensure proper synchronization. # Find any references to `finishLoad` to identify potential mechanisms that might close it. rg --context 10 'finishLoad' core/device/service.goLength of output: 1878
| func NewDevices() Service { | ||
| return &devices{finishLoad: make(chan struct{})} | ||
| } |
There was a problem hiding this comment.
Initialization of devices struct can be optimized.
Consider initializing all fields of devices struct explicitly in NewDevices function to enhance code readability and prevent future errors related to uninitialized fields.
| func (d *devices) loadDevices(ctx context.Context) { | ||
| defer close(d.finishLoad) | ||
| uk, err := domain.NewUniqueKey(sb.SmartBlockTypePage, "") | ||
| if err != nil { | ||
| log.Errorf("failed to get devices object unique key: %v", err) | ||
| return | ||
| } | ||
| techSpace, err := d.spaceService.GetTechSpace(ctx) | ||
| if err != nil { | ||
| return | ||
| } | ||
| deviceObject, err := techSpace.DeriveTreeObject(ctx, objectcache.TreeDerivationParams{ | ||
| Key: uk, | ||
| InitFunc: func(id string) *smartblock.InitContext { | ||
| return &smartblock.InitContext{ | ||
| Ctx: ctx, | ||
| SpaceID: techSpace.Id(), | ||
| State: state.NewDoc(id, nil).(*state.State), | ||
| } | ||
| }, | ||
| }) | ||
| if err != nil && !errors.Is(err, treestorage.ErrTreeExists) { | ||
| log.Errorf("failed to derive device object: %v", err) | ||
| return | ||
| } | ||
| if err == nil { | ||
| d.deviceObjectId = deviceObject.Id() | ||
| } | ||
| if errors.Is(err, treestorage.ErrTreeExists) { | ||
| id, err := techSpace.DeriveObjectID(ctx, uk) | ||
| if err != nil { | ||
| log.Errorf("failed to derive device object id: %v", err) | ||
| return | ||
| } | ||
| d.deviceObjectId = id | ||
| } | ||
| if deviceObject == nil { | ||
| deviceObject, err = techSpace.GetObject(ctx, d.deviceObjectId) | ||
| if err != nil { | ||
| log.Errorf("failed to get device object id: %v", err) | ||
| return | ||
| } | ||
| } | ||
| hostname, err := os.Hostname() | ||
| if err != nil { | ||
| log.Errorf("failed to get hostname: %v", err) | ||
| return | ||
| } | ||
| deviceObject.Lock() | ||
| st := deviceObject.NewState() | ||
| deviceId := d.wallet.GetDevicePrivkey().GetPublic().PeerId() | ||
| st.AddDevice(&model.DeviceInfo{ | ||
| Id: deviceId, | ||
| Name: hostname, | ||
| AddDate: time.Now().Unix(), | ||
| IsConnected: true, | ||
| }) | ||
| err = deviceObject.Apply(st) | ||
| if err != nil { | ||
| log.Errorf("failed to apply device state: %v", err) | ||
| } | ||
| deviceObject.Unlock() | ||
| for _, info := range st.ListDevices() { | ||
| err := d.store.SaveDevice(info) | ||
| if err != nil { | ||
| log.Errorf("failed to save device: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize loadDevices method for better error handling and efficiency.
- Consolidate error handling to reduce redundancy and improve readability.
- Use a single error handling block for the entire method.
- Consider using early returns for error conditions to reduce nesting and improve readability.
| func (d *devices) SaveDeviceInfo(ctx context.Context, device *model.DeviceInfo) error { | ||
| err := d.store.SaveDevice(device) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to save device: %w", err) | ||
| } | ||
|
|
||
| spc, err := d.spaceService.Get(ctx, d.spaceService.TechSpaceId()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get space: %w", err) | ||
| } | ||
| return spc.Do(d.deviceObjectId, func(sb smartblock.SmartBlock) error { | ||
| st := sb.NewState() | ||
| st.AddDevice(device) | ||
| return sb.Apply(st) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Refactor SaveDeviceInfo to reduce duplication and improve error handling.
The error handling and space retrieval logic is duplicated in SaveDeviceInfo and UpdateName. Consider extracting these into a helper function to reduce code duplication and improve maintainability.
| func (d *devices) Init(a *app.App) (err error) { | ||
| d.spaceService = app.MustComponent[space.Service](a) | ||
| d.wallet = a.MustComponent(wallet.CName).(wallet.Wallet) | ||
| datastoreService := app.MustComponent[datastore.Datastore](a) | ||
| db, err := datastoreService.LocalStorage() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize notification store %w", err) | ||
| } | ||
| d.store = NewStore(db) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Improve error handling in Init method.
The error message in line 56 could be more descriptive. It currently mentions "notification store" which might be confusing as this is related to device management.
- return fmt.Errorf("failed to initialize notification store %w", err)
+ return fmt.Errorf("failed to initialize device store: %w", err)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 (d *devices) Init(a *app.App) (err error) { | |
| d.spaceService = app.MustComponent[space.Service](a) | |
| d.wallet = a.MustComponent(wallet.CName).(wallet.Wallet) | |
| datastoreService := app.MustComponent[datastore.Datastore](a) | |
| db, err := datastoreService.LocalStorage() | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize notification store %w", err) | |
| } | |
| d.store = NewStore(db) | |
| return nil | |
| } | |
| func (d *devices) Init(a *app.App) (err error) { | |
| d.spaceService = app.MustComponent[space.Service](a) | |
| d.wallet = a.MustComponent(wallet.CName).(wallet.Wallet) | |
| datastoreService := app.MustComponent[datastore.Datastore](a) | |
| db, err := datastoreService.LocalStorage() | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize device store: %w", err) | |
| } | |
| d.store = NewStore(db) | |
| return nil | |
| } |
| func (d *devices) UpdateName(ctx context.Context, id, name string) error { | ||
| err := d.store.UpdateDeviceName(id, name) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update device name: %w", err) | ||
| } | ||
| spc, err := d.spaceService.Get(ctx, d.spaceService.TechSpaceId()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get space: %w", err) | ||
| } | ||
| return spc.Do(d.deviceObjectId, func(sb smartblock.SmartBlock) error { | ||
| st := sb.NewState() | ||
| st.SetDeviceName(id, name) | ||
| return sb.Apply(st) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Ensure consistent error handling in UpdateName.
The error handling in UpdateName should be consistent with other methods. Consider using the same error message format and handling strategy.
- return fmt.Errorf("failed to update device name: %w", err)
+ return fmt.Errorf("failed to update device name due to: %w", err)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 (d *devices) UpdateName(ctx context.Context, id, name string) error { | |
| err := d.store.UpdateDeviceName(id, name) | |
| if err != nil { | |
| return fmt.Errorf("failed to update device name: %w", err) | |
| } | |
| spc, err := d.spaceService.Get(ctx, d.spaceService.TechSpaceId()) | |
| if err != nil { | |
| return fmt.Errorf("failed to get space: %w", err) | |
| } | |
| return spc.Do(d.deviceObjectId, func(sb smartblock.SmartBlock) error { | |
| st := sb.NewState() | |
| st.SetDeviceName(id, name) | |
| return sb.Apply(st) | |
| }) | |
| } | |
| func (d *devices) UpdateName(ctx context.Context, id, name string) error { | |
| err := d.store.UpdateDeviceName(id, name) | |
| if err != nil { | |
| return fmt.Errorf("failed to update device name due to: %w", err) | |
| } | |
| spc, err := d.spaceService.Get(ctx, d.spaceService.TechSpaceId()) | |
| if err != nil { | |
| return fmt.Errorf("failed to get space: %w", err) | |
| } | |
| return spc.Do(d.deviceObjectId, func(sb smartblock.SmartBlock) error { | |
| st := sb.NewState() | |
| st.SetDeviceName(id, name) | |
| return sb.Apply(st) | |
| }) | |
| } |
…o-2930-store-device-names # Conflicts: # clientlibrary/service/service.pb.go # pkg/lib/pb/model/models.pb.go
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
docs/proto.mdis excluded by!**/proto.mdpkg/lib/pb/model/models.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (3)
- core/anytype/bootstrap.go (2 hunks)
- core/block/editor/smartblock/smartblock.go (2 hunks)
- pkg/lib/pb/model/protos/models.proto (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- core/anytype/bootstrap.go
- core/block/editor/smartblock/smartblock.go
- pkg/lib/pb/model/protos/models.proto
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.godocs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (4)
- core/anytype/bootstrap.go (2 hunks)
- core/block/editor/smartblock/smartblock.go (2 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- core/anytype/bootstrap.go
- core/block/editor/smartblock/smartblock.go
- pb/protos/commands.proto
- pb/protos/service/service.proto
https://linear.app/anytype/issue/GO-2930/store-device-names
DeviceSetNameto change device nameDeviceListto get devices list