GO-2361 Save activeView to DB #1267
Conversation
WalkthroughThe changes introduce new functionality for managing active views within the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Service
participant SmartBlock
participant ObjectStore
User->>Service: SetActiveView(request)
Service->>SmartBlock: SetActiveView(blockId, viewId)
SmartBlock->>ObjectStore: SetActiveViews(objectId, views)
ObjectStore-->>SmartBlock: ActiveViews saved
SmartBlock-->>Service: ActiveView set
Service-->>User: Success response
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
core/block/editor/smartblock/smartblock.go (2)
721-722: Consider adding logging or metrics forsaveActiveViewsto monitor its performance and error rates.
1372-1386: Consider adding more detailed logging for failures ininjectActiveViewsto aid in debugging.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
docs/proto.mdis excluded by!**/proto.mdpb/events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/lib/localstore/objectstore/mock_objectstore/mock_ObjectStore.gois excluded by!**/mock_*.gopkg/lib/pb/model/models.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (8)
- core/block/editor/smartblock/smartblock.go (5 hunks)
- core/block/editor/smartblock/smartblock_test.go (2 hunks)
- core/block/simple/dataview/diff.go (2 hunks)
- pb/protos/events.proto (2 hunks)
- pkg/lib/localstore/objectstore/activeview.go (1 hunks)
- pkg/lib/localstore/objectstore/objects.go (2 hunks)
- pkg/lib/pb/model/protos/models.proto (1 hunks)
- util/testMock/objectstore_mock.go (2 hunks)
Additional comments not posted (12)
pkg/lib/localstore/objectstore/activeview.go (2)
5-7: The implementation ofSetActiveViewcorrectly uses thebadgerhelper.SetValueto store the active view. Ensure that thepagesActiveViewBaseis correctly initialized and accessible before this method is called.
9-11: The implementation ofGetActiveViewcorrectly retrieves the active view usingbadgerhelper.GetValue. Ensure that error handling is robust, especially for cases where the view ID might not be set.core/block/simple/dataview/diff.go (1)
253-266: The implementation ofdiffActiveViewcorrectly identifies changes in theActiveViewand appends the appropriate event message. This is crucial for tracking changes in the active view state.pkg/lib/localstore/objectstore/objects.go (2)
41-41: The introduction ofpagesActiveViewBaseis correctly implemented usingds.NewKeyto construct a datastore key for active views. This is essential for the new active view functionality.
149-151: The addition ofSetActiveViewandGetActiveViewmethods to theObjectStoreinterface is appropriate and necessary for supporting active view functionality across different implementations of the interface.core/block/editor/smartblock/smartblock_test.go (1)
468-510: TheTestInjectActiveViewsfunction correctly tests the injection of active views into dataview blocks. It ensures that theGetActiveViewmethod is called as expected and that the active views are set correctly in the state.util/testMock/objectstore_mock.go (2)
235-242: Implementation ofGetActiveViewmethod and its recorder looks correct and aligns with the mocking framework's standards.Also applies to: 244-248
784-790: Implementation ofSetActiveViewmethod and its recorder looks correct and aligns with the mocking framework's standards.Also applies to: 792-796
pb/protos/events.proto (2)
74-74: Field addition looks good. Ensure that tag number128does not conflict with existing tags.
907-910: The new messageActiveViewIsSetis well-structured and appropriately documented.pkg/lib/pb/model/protos/models.proto (1)
336-336: The addition of theactiveViewfield with session-specific storage is noted.Ensure that the transient nature of this field is well-documented and understood by developers interacting with this model, to avoid misuse.
core/block/editor/smartblock/smartblock.go (1)
358-358: Ensure proper error handling forinjectActiveViews.Verification successful
The
injectActiveViewsmethod incore/block/editor/smartblock/smartblock.godoes include error handling. It logs warnings if there are issues during the injection process.
- The method checks for errors when calling
sb.objectStore.GetActiveViewand logs a warning if an error occurs.- It also logs a warning if the
s.Iteratefunction returns an error.Therefore, the error handling for
injectActiveViewsis already implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling is implemented for `injectActiveViews` method. # Test: Search for error handling patterns around the usage of `injectActiveViews`. ast-grep --lang go --pattern $'func $_($$$) { $$$ $_.injectActiveViews($$$) $$$ }'Length of output: 89
Script:
#!/bin/bash # Description: Locate the definition of the `injectActiveViews` method and inspect its implementation for error handling. # Find the definition of `injectActiveViews` method in Go files. ast-grep --lang go --pattern 'func ($_ $_).injectActiveViews($$$) $$$'Length of output: 72
Script:
#!/bin/bash # Description: Locate the definition of the `injectActiveViews` method and inspect its implementation for error handling. # Search for the definition of `injectActiveViews` method in Go files. rg 'func \(\w+ \*?\w+\) injectActiveViews\(' --glob '*.go'Length of output: 161
Script:
#!/bin/bash # Description: Extract the implementation of the `injectActiveViews` method to inspect for error handling. # Extract the implementation of the `injectActiveViews` method from the specified file. sed -n '/func (sb \*smartBlock) injectActiveViews(s \*state.State) {/,/^}/p' core/block/editor/smartblock/smartblock.goLength of output: 656
| func (sb *smartBlock) saveActiveViews(msgs []simple.EventMessage) []simple.EventMessage { | ||
| i := 0 | ||
| for _, msg := range msgs { | ||
| ev := msg.Msg.GetBlockDataviewActiveViewIsSet() | ||
| if ev != nil { | ||
| err := sb.objectStore.SetActiveView(sb.Id(), ev.BlockId, ev.ViewId) | ||
| if err != nil { | ||
| log.With("objectID", sb.Id()).Warnf("failed to update active view: %v", err) | ||
| } | ||
| } else { | ||
| msgs[i] = msg | ||
| i++ | ||
| } | ||
| } | ||
| // we exclude BlockDataViewActiveViewIsSet messages, because we do not need to send events on it | ||
| return msgs[:i] | ||
| } |
There was a problem hiding this comment.
Refactor saveActiveViews to handle errors more gracefully and ensure that all messages are processed correctly.
func (sb *smartBlock) saveActiveViews(msgs []simple.EventMessage) []simple.EventMessage {
i := 0
for _, msg := range msgs {
ev := msg.Msg.GetBlockDataviewActiveViewIsSet()
if ev != nil {
err := sb.objectStore.SetActiveView(sb.Id(), ev.BlockId, ev.ViewId)
if err != nil {
- log.With("objectID", sb.Id()).Warnf("failed to update active view: %v", err)
+ log.With("objectID", sb.Id()).Errorf("failed to update active view: %v", err)
+ continue // Skip this message to ensure others are processed
}
} else {
msgs[i] = msg
i++
}
}
return msgs[:i]
}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 (sb *smartBlock) saveActiveViews(msgs []simple.EventMessage) []simple.EventMessage { | |
| i := 0 | |
| for _, msg := range msgs { | |
| ev := msg.Msg.GetBlockDataviewActiveViewIsSet() | |
| if ev != nil { | |
| err := sb.objectStore.SetActiveView(sb.Id(), ev.BlockId, ev.ViewId) | |
| if err != nil { | |
| log.With("objectID", sb.Id()).Warnf("failed to update active view: %v", err) | |
| } | |
| } else { | |
| msgs[i] = msg | |
| i++ | |
| } | |
| } | |
| // we exclude BlockDataViewActiveViewIsSet messages, because we do not need to send events on it | |
| return msgs[:i] | |
| } | |
| func (sb *smartBlock) saveActiveViews(msgs []simple.EventMessage) []simple.EventMessage { | |
| i := 0 | |
| for _, msg := range msgs { | |
| ev := msg.Msg.GetBlockDataviewActiveViewIsSet() | |
| if ev != nil { | |
| err := sb.objectStore.SetActiveView(sb.Id(), ev.BlockId, ev.ViewId) | |
| if err != nil { | |
| log.With("objectID", sb.Id()).Errorf("failed to update active view: %v", err) | |
| continue // Skip this message to ensure others are processed | |
| } | |
| } else { | |
| msgs[i] = msg | |
| i++ | |
| } | |
| } | |
| return msgs[:i] | |
| } |
| @@ -1005,6 +1009,7 @@ | |||
| } | |||
| sb.updateRestrictions() | |||
| sb.injectDerivedDetails(d.(*state.State), sb.SpaceID(), sb.Type()) | |||
| sb.injectActiveViews(d.(*state.State)) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling for injectActiveViews during state rebuild.
The search results indicate that there is no direct error handling implemented around the injectActiveViews method calls in the smartblock.go file. This could potentially lead to unhandled errors during state rebuild.
- File:
core/block/editor/smartblock/smartblock.go- Lines:
- 1012:
sb.injectActiveViews(d.(*state.State)) - 1008:
sb.injectActiveViews(s) - 1004:
sb.injectActiveViews(ctx.State)
- 1012:
- Lines:
Consider adding appropriate error handling around these method calls to ensure robustness.
Analysis chain
Ensure proper error handling for injectActiveViews during state rebuild.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that error handling is implemented for `injectActiveViews` method during state rebuild.
# Test: Search for error handling patterns around the usage of `injectActiveViews` in state rebuild scenarios.
ast-grep --lang go --pattern $'func $_($$$) {
$$$
$_.injectActiveViews($$$)
$$$
}'
Length of output: 89
Script:
#!/bin/bash
# Description: Find all occurrences of `injectActiveViews` in the codebase and check for error handling.
# Step 1: Find all occurrences of `injectActiveViews`.
rg 'injectActiveViews' --glob '*.go'
# Step 2: Check for error handling around these occurrences.
# This will be done by searching for common error handling patterns around the found occurrences.
rg -A 5 -B 5 'injectActiveViews' --glob '*.go' | grep -E 'if err|return err|log\.Error|panic|fmt\.Errorf'
Length of output: 1105
| @@ -759,6 +762,7 @@ | |||
| sb.storeFileKeys(s) | |||
| sb.injectLocalDetails(s) | |||
| sb.injectDerivedDetails(s, sb.SpaceID(), sb.Type()) | |||
| sb.injectActiveViews(s) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The injectActiveViews method in core/block/editor/smartblock/smartblock.go does include error handling within its implementation. Specifically, it logs warnings when it encounters errors while injecting active views. However, the usages of injectActiveViews do not consistently handle potential errors returned by this method.
- In the
injectActiveViewsmethod itself, errors are logged but not propagated. - In the usages of
injectActiveViews, there is no explicit error handling after calling this method.
To ensure proper error handling during the reset process, you should consider propagating errors from injectActiveViews and handling them appropriately in the calling functions.
Locations to address:
core/block/editor/smartblock/smartblock.go:- Lines where
injectActiveViewsis called without error handling.
- Lines where
Suggested changes:
- Modify
injectActiveViewsto return an error. - Update the calling functions to handle the returned error.
Analysis chain
Ensure proper error handling for injectActiveViews during the reset process.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that error handling is implemented for `injectActiveViews` method during the reset process.
# Test: Search for error handling patterns around the usage of `injectActiveViews` in reset scenarios.
ast-grep --lang go --pattern $'func $_($$$) {
$$$
$_.injectActiveViews($$$)
$$$
}'
Length of output: 89
Script:
#!/bin/bash
# Description: Locate the definition of the `injectActiveViews` method and its usages, focusing on error handling.
# Search for the definition of the `injectActiveViews` method.
ast-grep --lang go --pattern $'func ($_).injectActiveViews($$$) {
$$$
}'
# Search for usages of the `injectActiveViews` method.
rg 'injectActiveViews'
Length of output: 501
Script:
#!/bin/bash
# Description: Extract the definition of the `injectActiveViews` method and its usages for detailed inspection.
# Extract the definition of the `injectActiveViews` method.
ast-grep --lang go --pattern $'func (sb *smartBlock) injectActiveViews(s *state.State) {
$$$
}' core/block/editor/smartblock/smartblock.go
# Extract the context around the usages of the `injectActiveViews` method.
rg -C 5 'injectActiveViews' core/block/editor/smartblock/smartblock.go
Length of output: 2975
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
|
|
||
| func (sb *smartBlock) injectActiveViews(s *state.State) { | ||
| if err := s.Iterate(func(b simple.Block) (isContinue bool) { | ||
| if d := b.Model().GetDataview(); d != nil { |
There was a problem hiding this comment.
it feels like, instead of iterating all blocks for all smartblocks when opening them the reverse approach may be more efficient and also utilise the objectstore indexes.
- get all saved activeViews from objectstore for specific objectId
- then pick specific blocks and modify them
There was a problem hiding this comment.
Yes, I have rewritten this logic, so we store separate map-like string for each object
pb/protos/events.proto
Outdated
| @@ -902,6 +903,11 @@ message Event { | |||
| string id = 1; // dataview block's id | |||
| bool value = 2; | |||
| } | |||
|
|
|||
| message ActiveViewIsSet { | |||
There was a problem hiding this comment.
better be ActiveViewSet? because the last word in events naming is just a verb
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/events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/lib/localstore/objectstore/mock_objectstore/mock_ObjectStore.gois excluded by!**/mock_*.go
Files selected for processing (8)
- core/block/editor/smartblock/smartblock.go (6 hunks)
- core/block/editor/smartblock/smartblock_test.go (6 hunks)
- core/block/simple/dataview/diff.go (2 hunks)
- pb/protos/events.proto (2 hunks)
- pkg/lib/localstore/objectstore/activeview.go (1 hunks)
- pkg/lib/localstore/objectstore/activeview_test.go (1 hunks)
- pkg/lib/localstore/objectstore/objects.go (2 hunks)
- util/testMock/objectstore_mock.go (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- core/block/editor/smartblock/smartblock.go
- core/block/editor/smartblock/smartblock_test.go
- core/block/simple/dataview/diff.go
- pb/protos/events.proto
- pkg/lib/localstore/objectstore/activeview.go
- pkg/lib/localstore/objectstore/objects.go
Additional comments not posted (4)
pkg/lib/localstore/objectstore/activeview_test.go (2)
11-16: The test cases inTestViewsMapToStringare comprehensive and well-structured.
18-39: The table-driven tests inTestParseViewsMapare effectively structured and cover a wide range of scenarios.util/testMock/objectstore_mock.go (2)
236-249: The mock implementation forGetActiveViewsis correctly set up and follows the expected patterns for mocking withgomock.
785-797: The mock implementation forSetActiveViewsis correctly set up and follows the expected patterns for mocking withgomock.
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/localstore/objectstore/mock_objectstore/mock_ObjectStore.gois excluded by!**/mock_*.go
Files selected for processing (9)
- core/block/editor.go (1 hunks)
- core/block/editor/dataview/dataview.go (6 hunks)
- core/block/editor/dataview/dataview_test.go (3 hunks)
- core/block/editor/smartblock/smartblock_test.go (4 hunks)
- pb/protos/commands.proto (2 hunks)
- pkg/lib/localstore/objectstore/activeview.go (1 hunks)
- pkg/lib/localstore/objectstore/activeview_test.go (1 hunks)
- pkg/lib/localstore/objectstore/objects.go (2 hunks)
- util/testMock/objectstore_mock.go (3 hunks)
Files skipped from review due to trivial changes (1)
- pb/protos/commands.proto
Files skipped from review as they are similar to previous changes (5)
- core/block/editor/smartblock/smartblock_test.go
- pkg/lib/localstore/objectstore/activeview.go
- pkg/lib/localstore/objectstore/activeview_test.go
- pkg/lib/localstore/objectstore/objects.go
- util/testMock/objectstore_mock.go
Additional comments not posted (9)
core/block/editor/dataview/dataview_test.go (3)
4-10: Imports are well-organized and relevant to the test cases.
25-42: Thefixturestruct andnewFixturefunction are well-implemented for setting up test environments.
81-157: The tests forinjectActiveViewsmethod are comprehensive, covering scenarios of successful injection, absence of views, and handling of database errors.core/block/editor/dataview/dataview.go (5)
24-24: The import ofbadgerhelperis correctly added to support the new functionality related to Badger DB operations.
39-39: The simplification of theSetActiveViewmethod signature aligns with the PR's objective to streamline dataview operations.
59-59: Adding theinjectActiveViewsmethod as a hook is a good use of the existing architecture to ensure that active views are set before any operations are applied.
Line range hint
206-224: The implementation ofSetActiveViewis robust, handling potential errors effectively and ensuring that the active view is updated in the object store.
428-454: TheinjectActiveViewsmethod is well-implemented. It handles the retrieval of active views and injects them into the appropriate dataview blocks, with proper error handling and logging.core/block/editor.go (1)
242-242: Simplified method signature forSetDataviewActiveViewaligns with the PR's objectives.
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 (2 hunks)
- pkg/lib/pb/model/protos/models.proto (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- pb/protos/commands.proto
- pkg/lib/pb/model/protos/models.proto
https://linear.app/anytype/issue/GO-2361/back-to-the-last-view-of-the-set-collection
Save activeView to badger and inject it to dataview blocks on smartblock Init/Apply