Fix merging trees #8359
Fix merging trees #8359
Conversation
|
Warning Rate limit exceeded@MichaelBuessemeyer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request introduces modifications to the skeleton tracing reducer and its helper functions, focusing on tree merging logic. The changes primarily involve adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
already requesting review but I don't have anymore time to add the changelog entry and check whether the tests actually improved / would have shown this bug. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (1)
1062-1159: Well-structured test case for tree type validation.The test case thoroughly verifies that trees of different types cannot be merged, which is crucial for maintaining data integrity.
However, consider adding a few more test scenarios:
- Attempting to merge two agglomerate trees
- Attempting to merge two default trees
test("SkeletonTracing shouldn't merge two trees of different type", (t) => { // ... existing test code ... }); + +test("SkeletonTracing should merge two agglomerate trees", (t) => { + // Add test for merging two agglomerate trees +}); + +test("SkeletonTracing should merge two default trees", (t) => { + // Add test for merging two default trees +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(18 hunks)
🔇 Additional comments (4)
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (4)
12-12: LGTM! Type imports are correctly updated.The addition of
MutableNodeandTreetypes to the imports aligns with the new test requirements.
83-97: LGTM! Well-structured agglomerate tree initialization.The new agglomerate tree is properly initialized with all required properties and follows the same structure as the default tree. The
TreeTypeEnum.AGGLOMERATEtype is correctly set.
114-120: LGTM! Clean state initialization for active tree ID 2.The new initial state variant is properly created using immutable update pattern.
208-214: Test assertions are correctly updated for new tree IDs.The test assertions have been properly updated to account for the new tree ID structure, maintaining consistency with the addition of the agglomerate tree.
Also applies to: 845-847, 854-856, 876-878, 982-984, 1043-1044, 1153-1155
| const oldTrees = getTreesWithType(skeletonTracing, treeType); | ||
| const mergeResult = mergeTrees(oldTrees, sourceNodeId, targetNodeId); | ||
| const oldTrees = skeletonTracing.trees; | ||
| const mergeResult = mergeTrees(oldTrees, sourceNodeId, targetNodeId, treeType); |
The merging trees code accidentally dropped all trees of the other type (which are not merged). The newest version of the code makes sure to not drop the trees of the other (not active) type. Moreover, I improved the test such that both types (default & agglomerate) are not tested in the reducer tests. I added an additional that tests that trees of different types are not merged together.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)