Send right parameter to createNodeSkeletonAction #8185
Conversation
WalkthroughThe changes in this pull request encompass multiple updates to the WEBKNOSSOS application, including enhancements to user-facing features such as metadata addition for annotations, improved search functionality, and adjustments to the time tracking overview. Key modifications involve renaming properties from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![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
🧹 Outside diff range and nitpick comments (14)
frontend/javascripts/oxalis/store.ts (1)
92-92: Reminder: Update documentation and migration guide.As mentioned in the PR objectives, the migration guide and documentation still need to be updated to reflect this change.
Consider:
- Updating the migration guide to help users transition from
magtoresolution- Updating relevant documentation to reflect the current property name
- Adding a comment explaining why this property is named
resolutioninstead ofmagfrontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/connectome_view.tsx (3)
Line range hint
166-173: Consider making timestamp injectable for testingThe current implementation uses
Date.now()directly, which can make unit tests non-deterministic. Consider making the timestamp injectable for better testability.-const synapseNodeCreator = (synapseId: number, synapsePosition: Vector3): MutableNode => ({ +const synapseNodeCreator = ( + synapseId: number, + synapsePosition: Vector3, + timestamp: number = Date.now() +): MutableNode => ({ untransformedPosition: synapsePosition, radius: Constants.DEFAULT_NODE_RADIUS, rotation: [0, 0, 0], viewport: 0, resolution: 0, id: synapseId, - timestamp: Date.now(), + timestamp, bitDepth: 8, interpolation: false, additionalCoordinates: null, });
Line range hint
391-398: Enhance error handling for API callsThe error handling for mismatched synapse counts is good, but the Promise.all call lacks error handling. Consider adding a catch block to handle potential API failures gracefully.
const [synapseSources, synapseDestinations, synapsePositions, synapseTypesAndNames] = - await Promise.all([ - getSynapseSources(...fetchProperties, allInSynapseIds), - getSynapseDestinations(...fetchProperties, allOutSynapseIds), - getSynapsePositions(...fetchProperties, allSynapseIds), - getSynapseTypes(...fetchProperties, allSynapseIds), - ]); + await Promise.all([ + getSynapseSources(...fetchProperties, allInSynapseIds), + getSynapseDestinations(...fetchProperties, allOutSynapseIds), + getSynapsePositions(...fetchProperties, allSynapseIds), + getSynapseTypes(...fetchProperties, allSynapseIds), + ]).catch(error => { + Toast.error(`Failed to fetch synapse data: ${error.message}`); + throw error; + });
Line range hint
673-682: Remove @ts-ignore by adding proper type definitionsThe code uses @ts-ignore for event handling. Consider adding proper type definitions instead.
- handleChangeActiveSegment = (evt: React.SyntheticEvent) => { - // @ts-ignore - const agglomerateIds = evt.target.value + handleChangeActiveSegment = (evt: React.ChangeEvent<HTMLInputElement>) => { + const agglomerateIds = evt.target.value .split(",") .map((part: string) => Number.parseInt(part, 10)) .filter((id: number) => !Number.isNaN(id)); this.setActiveConnectomeAgglomerateIds(agglomerateIds); - // @ts-ignore - evt.target.blur(); + evt.target.blur(); };frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (10)
Line range hint
161-165: UpdatecreateNodeActioncalls to includetreeIdparameterThe method
createNodeActionnow requires atreeIdparameter as per the updated method signature, but it is missing in this test case. Please include thetreeIdto ensure the test reflects the current implementation.Apply the following diff:
const createNodeAction = SkeletonTracingActions.createNodeAction( position, null, rotation, viewport, resolution, + newSkeletonTracing.activeTreeId, );
Line range hint
178-184: UpdatecreateNodeActioncalls to includetreeIdparameterIn this test case, when adding multiple nodes, the
createNodeActionis called without the requiredtreeIdparameter. Please modify each call to includetreeId.Apply the following diff:
// create three nodes let newState = SkeletonTracingReducer(initialState, createNodeAction); +newState = SkeletonTracingReducer(newState, SkeletonTracingActions.createNodeAction( + position, null, rotation, viewport, resolution, newSkeletonTracing.activeTreeId)); +newState = SkeletonTracingReducer(newState, SkeletonTracingActions.createNodeAction( + position, null, rotation, viewport, resolution, newSkeletonTracing.activeTreeId));
Line range hint
202-208: UpdatecreateNodeActionandcreateTreeActioncalls withtreeIdwhere necessaryThe
createNodeActionand other actions now require atreeId. Ensure that these actions include thetreeIdparameter to reflect the changes in method signatures.Apply the following diff:
const createNodeAction = SkeletonTracingActions.createNodeAction( position, null, rotation, viewport, resolution, + 2, // specify the treeId ); const createTreeAction = SkeletonTracingActions.createTreeAction();
Line range hint
274-275: Provide required parameters fordeleteNodeActionThe
deleteNodeActionnow requirestreeIdandnodeIdparameters. These are missing in this call. Please include them to align with the updated method signature.Apply the following diff:
const deleteNodeAction = SkeletonTracingActions.deleteNodeAction( + newSkeletonTracing.activeTreeId, + newSkeletonTracing.activeNodeId, );
Line range hint
520-521: UpdatecreateBranchPointActioncalls withtreeIdandnodeIdparametersThe
createBranchPointActionnow requirestreeIdandnodeIdparameters. Please include these parameters in the method call.Apply the following diff:
const createBranchPointAction = SkeletonTracingActions.createBranchPointAction( + newSkeletonTracing.activeTreeId, + newSkeletonTracing.activeNodeId, );
Line range hint
592-593: UpdatesetTreeNameActionto includetreeIdparameterThe
setTreeNameActionmethod signature has been updated to includetreeId. Ensure that this parameter is provided in the method call.Apply the following diff:
const setTreeNameAction = SkeletonTracingActions.setTreeNameAction(newName + , newSkeletonTracing.activeTreeId );
Line range hint
716-718: IncludetreeIdparameter inshuffleTreeColorActionThe
shuffleTreeColorActionmethod should specify thetreeIdof the tree whose color is being shuffled. Verify that the correcttreeIdis provided.Apply the following diff:
const shuffleTreeColorAction = SkeletonTracingActions.shuffleTreeColorAction( - 1 + newSkeletonTracing.activeTreeId );
Line range hint
725-729: ProvidenodeIdparameter increateCommentActionThe
createCommentActionnow requires anodeIdparameter. Update the method call to include the appropriatenodeId.Apply the following diff:
const createCommentAction = SkeletonTracingActions.createCommentAction(commentText + , newSkeletonTracing.activeNodeId );
Line range hint
756-757: UpdatedeleteCommentActionto includenodeIdparameterThe
deleteCommentActionrequiresnodeIdas a parameter. Please include it in the call.Apply the following diff:
const deleteCommentAction = SkeletonTracingActions.deleteCommentAction( + newSkeletonTracing.activeNodeId );
Line range hint
793-795: EnsurecreateNodeActionincludestreeIdwhen adding a node to a specific treeIn this test case, when adding a node to a specific tree, make sure the
treeIdparameter is provided tocreateNodeAction.Apply the following diff:
const createNodeAction = SkeletonTracingActions.createNodeAction( position, null, rotation, viewport, resolution, + 2, // specify the treeId );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/model/helpers/nml_helpers.ts(2 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts(2 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/connectome_view.tsx(1 hunks)frontend/javascripts/test/libs/nml.spec.ts(1 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(2 hunks)
🔇 Additional comments (11)
frontend/javascripts/oxalis/store.ts (1)
92-92: LGTM! Reverting mag to resolution in MutableNode type.
The change aligns with the backend requirements as mentioned in the PR objectives.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Let me generate additional scripts to verify the usage of resolution specifically in node-related code and actions:
Property rename from mag to resolution is consistent across node-related code
The verification shows that:
- The
resolutionproperty is correctly defined in theMutableNodetype - All node creation actions and related code consistently use
resolutionparameter - The remaining instances of
magin the codebase are unrelated to node types (they're used in different contexts like magnification maps and viewport settings)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'mag' that might need updating
# and verify that 'resolution' is used consistently in node-related code.
# Search for any remaining references to 'mag' in node-related code
echo "Checking for remaining 'mag' references in node contexts..."
rg -p "mag.*:.*number" --type ts
# Search for the new 'resolution' property usage
echo "Verifying 'resolution' property usage..."
rg -p "resolution.*:.*number" --type ts
# Search specifically in action creators and reducers
echo "Checking node-related actions and reducers..."
ast-grep --pattern 'createNode$_($$$) { $$$resolution$$$}'
Length of output: 3071
Script:
#!/bin/bash
# Check for node-related code using resolution
echo "Checking node creation and updates..."
ast-grep --pattern 'type Node = { $$$ resolution: number $$$ }'
echo "Checking action creators..."
ast-grep --pattern 'export function createNode($$$) {
$$$
resolution: $_
$$$
}'
echo "Checking for any potential inconsistencies..."
rg -p "createNode.*\(" -A 5 --type ts
Length of output: 13565
frontend/javascripts/test/libs/nml.spec.ts (2)
37-37: LGTM: Property rename aligns with PR objectives.
The change from mag to resolution is consistent with the stated goal of maintaining backend compatibility.
Line range hint 1-1024: Test coverage is comprehensive.
The test suite thoroughly covers all aspects of NML serialization and parsing, including edge cases. The property rename is well-tested through the existing test cases.
frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts (2)
149-149: LGTM: Property rename from mag to resolution
The change correctly implements the parameter rename in the node creation logic.
849-849: LGTM: Consistent property mapping in node conversion
The change maintains consistency with the parameter rename in the server-to-mutable node conversion.
frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/connectome_view.tsx (1)
166-166: Verify the property renaming impact
The change from mag to resolution aligns with the PR objectives. However, we should verify that this change is consistently applied across the codebase.
✅ Verification successful
Let me analyze the search results. The change from mag to resolution appears to be part of a broader renaming effort across the codebase. Let's verify if this specific change in connectome_view.tsx is consistent with the codebase's usage.
Let me search for more specific context about the resolution property in the connectome components.
Based on the search results, I can now make a determination about the resolution property in connectome_view.tsx. The property is part of the node creation for synapses in the connectome viewer, and it's used consistently with the rest of the codebase's node creation pattern. The change from mag to resolution is part of a broader renaming effort, and this specific instance is correctly aligned with that change.
The property renaming is consistent with the codebase
The change from mag to resolution in connectome_view.tsx is correct as it follows the same pattern used throughout the codebase for node creation. The property is used with a default value of 0, which is appropriate for this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the 'mag' property in node-related code
rg -l "mag.*:" --type=typescript --type=javascript
# Search for the new 'resolution' property usage
rg -l "resolution.*:" --type=typescript --type=javascript
Length of output: 182
Script:
#!/bin/bash
# Search for any remaining instances of the 'mag' property using ripgrep
rg "mag\s*:"
# Search for the new 'resolution' property usage
rg "resolution\s*:"
Length of output: 126541
Script:
#!/bin/bash
# Search for connectome-related files and their usage of resolution/mag
rg -A 2 -B 2 "connectome.*\{" --type=ts --type=tsx
# Search for specific usage in connectome components
rg -A 5 -B 5 "resolution:\s*\d+" --type=ts --type=tsx
Length of output: 164
Script:
#!/bin/bash
# Search for connectome-related files without file type restriction
rg -A 2 -B 2 "connectome.*\{"
# Search for resolution usage in the specific file and its imports
rg -l "resolution:\s*\d+" frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/
# Search for related files in the same directory
ls -la frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/
Length of output: 23231
Script:
#!/bin/bash
# Search for resolution usage in connectome_view.tsx
rg -A 5 -B 5 "resolution" frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/connectome_view.tsx
# Search for related type definitions
rg "type.*Synapse.*=.*\{" -A 10
Length of output: 7121
frontend/javascripts/oxalis/model/helpers/nml_helpers.ts (2)
966-966: LGTM! Property parsing is consistent with serialization.
The property rename from mag to resolution in the parsing logic maintains consistency with the serialization changes.
415-415: LGTM! Verify consistent usage of 'resolution' property.
The property rename from mag to resolution aligns with the PR objectives.
✅ Verification successful
Property rename from mag to resolution is consistently applied
The search results confirm that the property rename from mag to resolution is consistently applied across the codebase. The occurrences of inMag are only found in:
nml_helpers.tswhere we're making the changetask_type_create_view.tsx,router.tsx, andcreate_explorative_modal.tsxwhere it's used as a URL parameter/query string which is unrelated to the NML property rename
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of 'resolution' property across the codebase
# Expect: No occurrences of 'inMag' property except in migration scripts or tests
# Search for remaining occurrences of 'inMag'
rg 'inMag' --type ts --type tsx
Length of output: 59
Script:
#!/bin/bash
# Search for remaining occurrences of 'inMag' in TypeScript files
rg 'inMag' -t ts
# Search for usage of 'resolution' property in TypeScript files
rg 'resolution' -t ts -C 2
Length of output: 172777
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (3)
Line range hint 939-941: Adjust setTreeColorIndexAction call to match updated parameters
The method setTreeColorIndexAction may have updated parameters. Ensure that the correct parameters are provided, including treeId and colorIndex.
Please confirm the method signature and update the call accordingly.
298-298:
Replace mag with resolution for consistency
Line 298 uses the property mag, but it should be resolution to maintain consistency with the renaming changes introduced in this PR.
Apply the following diff:
- mag: resolution,
+ resolution: resolution,Likely invalid or redundant comment.
456-456:
Replace mag with resolution for consistency
Similarly, in line 456, mag is used instead of resolution. Update it to ensure consistent property naming across the codebase.
Apply the following diff:
- mag: resolution,
+ resolution: resolution,Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
frontend/javascripts/oxalis/model/sagas/proofread_saga.ts (4)
Line range hint
1-100: Consider enhancing error handling in saga functions.The saga error handling pattern could be more consistent. While
runSagaAndCatchSoftErroris used in some places, other sagas might benefit from similar error handling.Consider wrapping more saga functions with
runSagaAndCatchSoftErrorto ensure consistent error handling across the codebase.
Line range hint
500-700: Consider breaking down complex saga functions.Functions like
handleProofreadMergeOrMinCutandperformMinCutare quite long and handle multiple responsibilities. This makes the code harder to maintain and test.Consider splitting these functions into smaller, more focused functions. For example:
- Extract mesh handling logic into separate functions
- Split validation logic into dedicated functions
- Move mapping-related operations to separate utility functions
Line range hint
800-900: Consider improving type safety in mapping functions.The mapping functions use type assertions (
askeyword) which could be replaced with proper type guards for better type safety.Consider:
- Adding proper type guards instead of using type assertions
- Using TypeScript's discriminated unions for better type safety
- Adding runtime type checks where necessary
Line range hint
1-1000: Consider adding memory management for large operations.When dealing with large mappings and agglomerates, memory usage could be optimized.
Consider:
- Implementing pagination or streaming for large datasets
- Cleaning up resources after operations complete
- Adding memory usage monitoring and optimization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/javascripts/oxalis/model/sagas/proofread_saga.ts(1 hunks)frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts(3 hunks)
🔇 Additional comments (2)
frontend/javascripts/oxalis/model/sagas/proofread_saga.ts (1)
273-273: LGTM: Fixed typo in error message.
The spelling correction from "editble" to "editable" improves the error message clarity.
frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts (1)
133-133: Property rename from mag to resolution looks good!
The changes align with the PR objectives to revert the parameter name back to resolution for consistency with the backend. The test assertions and helper functions have been updated accordingly while maintaining the same test coverage and functionality.
Let's verify that all occurrences of mag have been replaced with resolution:
Also applies to: 298-298
✅ Verification successful
Based on the search results, I can see that the change from mag to resolution in the test file is part of a larger pattern where resolution is consistently used throughout the codebase to refer to magnification values. The test file changes align with this convention.
Property rename from mag to resolution is correct and consistent with codebase usage
The codebase shows consistent usage of resolution when referring to magnification values in test files and type definitions, for example:
- In test files:
resolution: numberin type definitions and test cases - In API types:
resolutionproperty in various interfaces - In test fixtures:
resolutionfield in test objects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'mag' that might need to be updated
# and verify that 'resolution' is used consistently.
echo "Checking for any remaining 'mag' occurrences..."
rg --type ts "mag" -A 2 -B 2
echo "Verifying consistent usage of 'resolution'..."
rg --type ts "resolution" -A 2 -B 2
Length of output: 503701
This PR undoes the changes to the
MutableNodetype, renamingmagtoresolutionagain. The change in the opposite direction caused inconsistencies with the backend, the the slack thread below.magseems to work in every place except for the CreateNodeSkeletonAction, so its a bit sad this change is undone. This PR just tries to prevent that information is lost, maybe someone can find a better solution in the future.URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements