Fix expected node type from server by renaming resolution to mag #8187
Conversation
WalkthroughThe pull request introduces several updates to the WEBKNOSSOS project, primarily focusing on enhancing metadata handling for Trees and Segments, improving UI elements, and fixing various bugs. Key changes include the renaming of the "Resolution" property to "Magnification" (or "mag") across multiple components, improved loading speeds for precomputed meshes, and the addition of new functionalities in the search interface. Additionally, several bug fixes address issues related to dataset uploads, annotation management, and layout persistence. Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
48-48: LGTM with minor wording suggestionsThe changelog entry accurately documents the fix. Consider this minor rewording for improved clarity:
- Fixed the expected type of a tree node received from the server. Fixes nml export to include the `inMag` field correctly. [#8187](https://github.com/scalableminds/webknossos/pull/8187) + Fixed the server node type mismatch to ensure correct `inMag` field in NML exports. [#8187](https://github.com/scalableminds/webknossos/pull/8187)🧰 Tools
🪛 LanguageTool
[grammar] ~48-~48: If ‘type’ is a classification term, ‘a’ is not necessary. Use “type of”. (The phrases ‘kind of’ and ‘sort of’ are informal if they mean ‘to some extent’.)
Context: ...knossos/pull/8185) - Fixed the expected type of a tree node received from the server. Fix...(KIND_OF_A)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/model/helpers/generate_dummy_trees.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts(1 hunks)frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts(3 hunks)frontend/javascripts/test/fixtures/tasktracing_server_objects.ts(1 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~48-~48: If ‘type’ is a classification term, ‘a’ is not necessary. Use “type of”. (The phrases ‘kind of’ and ‘sort of’ are informal if they mean ‘to some extent’.)
Context: ...knossos/pull/8185) - Fixed the expected type of a tree node received from the server. Fix...
(KIND_OF_A)
🔇 Additional comments (3)
frontend/javascripts/oxalis/model/helpers/generate_dummy_trees.ts (2)
37-37: LGTM! Change aligns with PR objectives.
The switch from resolution to mag in the node properties matches the server's expected field name, addressing the core issue described in the PR.
37-37: Verify consistent property naming across the codebase.
Let's ensure this naming change has been applied consistently across all relevant files.
✅ Verification successful
Property naming is consistent with codebase conventions
The change from 'resolution' to 'mag' for this single magnification value property follows the established pattern where 'mag' is used for individual magnification values, while 'resolutions' is used for collections/arrays of magnifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of 'resolution' that might need to be changed to 'mag'
# in the context of node properties or similar data structures
# Look for resolution in type definitions and interfaces
ast-grep --pattern 'interface $_ {
$$$
resolution: $_
$$$
}'
# Look for resolution in object literals that might be node-related
ast-grep --pattern '{
$$$
resolution: $_
$$$
}'
# Search for resolution in relevant contexts
rg 'resolution' --type ts --type tsx -C 3 | grep -v 'Resolution' | grep -v 'resolution:'
Length of output: 237
Script:
#!/bin/bash
# Description: Search for any remaining instances of 'resolution' that might need to be changed to 'mag'
# in the context of node properties or similar data structures
# Look for resolution in type definitions and interfaces
ast-grep --pattern 'interface $_ {
$$$
resolution: $_
$$$
}'
# Look for resolution in object literals that might be node-related
ast-grep --pattern '{
$$$
resolution: $_
$$$
}'
# Search for resolution in relevant contexts within .ts and .tsx files
rg 'resolution' -g '*.ts' -g '*.tsx' -C 3 | grep -v 'Resolution' | grep -v 'resolution:'
Length of output: 202992
Script:
#!/bin/bash
# Description: Search for any remaining instances of 'resolution' that might need to be changed to 'mag'
# in the context of node properties or similar data structures
# Look for resolution in type definitions and interfaces
ast-grep --pattern 'interface $_ {
$$$
resolution: $_
$$$
}'
# Look for resolution in object literals that might be node-related
ast-grep --pattern '{
$$$
resolution: $_
$$$
}'
# Search for resolution in relevant contexts within .ts and .tsx files
rg 'resolution' -g '*.ts' -g '*.tsx' -C 3 | grep -v 'Resolution' | grep -v 'resolution:'
Length of output: 202992
frontend/javascripts/types/api_flow_types.ts (1)
759-759: LGTM! Type definition aligned with server behavior.
The change from resolution to mag correctly reflects the server's behavior of sending nodes with the "mag" field, fixing the type mismatch.
Let's verify that this change is consistently applied across the codebase:
| @@ -20,7 +20,7 @@ export const tracing: ServerSkeletonTracing = { | |||
| }, | |||
| radius: 120, | |||
| viewport: 1, | |||
| resolution: 1, | |||
| mag: 1, | |||
There was a problem hiding this comment.
💡 Codebase verification
Incomplete renaming of resolution to mag in test fixtures.
Please update the following test fixtures to replace resolution with mag:
frontend/javascripts/test/fixtures/volumetracing_object.tsfrontend/javascripts/test/fixtures/dataset_server_object.ts
🔗 Analysis chain
LGTM! Property name change aligns with server response format.
The change from resolution to mag correctly reflects the server's node field naming, ensuring test fixtures match the actual API response structure.
Let's verify if similar changes were made in all test fixtures:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'resolution' in test fixtures
# and confirm consistent use of 'mag' property in node definitions
# Search for any remaining 'resolution' properties in test fixtures
echo "Checking for remaining 'resolution' properties in test fixtures:"
rg -l "resolution.*:" "frontend/javascripts/test/fixtures/"
# Search for node definitions to verify 'mag' property usage
echo "Verifying 'mag' property usage in node definitions:"
rg -A 5 "nodes.*:.*\[" "frontend/javascripts/test/fixtures/"
Length of output: 2032
| @@ -22,7 +22,7 @@ export const tracing: ServerSkeletonTracing = { | |||
| }, | |||
| radius: 112.39999389648438, | |||
| viewport: 0, | |||
| resolution: 1, | |||
| mag: 1, | |||
There was a problem hiding this comment.
💡 Codebase verification
Residual resolution fields found in type definitions.
Several type definitions still contain resolution fields. Please update these to maintain consistency.
- frontend/javascripts/types/api_flow_types.ts:
readonly resolutions: Array<Vector3>; - frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts:
resolution: 10, - frontend/javascripts/oxalis/shaders/main_data_shaders.glsl.ts:
resolutionsCount: number; - frontend/javascripts/router.tsx:
const resolutionRestrictions: APIMagRestrictions = {}; - frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx:
const getMaxDim = (resolution: Vector3) => Math.max(...resolution); - frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/connectome_view.tsx:
resolution: 0, - frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx:
handleQualityChangeForPrecomputation = (resolutionIndex: number) => - frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx:
handleQualityChangeForAdHocGeneration = (resolutionIndex: number) => - frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx:
resolutions ? resolutions.map(Utils.point3ToVector3) : ([[1, 1, 1]] as Vector3[]), - frontend/javascripts/oxalis/workers/async_bucket_picker.worker.ts:
resolutions: Array<Vector3>, - frontend/javascripts/test/model/model_resolutions.spec.ts:
resolutions: [ - frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts:
resolution: Vector3; - frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts:
const resolutionUnionDict: { [key: number]: Vector3[] } = {}; - frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts:
resolutionIdentifier: number, - frontend/javascripts/test/fixtures/volumetracing_object.ts:
// @ts-expect-error ts-migrate(2322) FIXME: Type '{ resolutions: [number, number, number][]; c... - frontend/javascripts/oxalis/model/helpers/position_converter.ts:
const resolution = magIndex < mags.length ? mags[magIndex] : upsampleMag(mags, magIndex); - frontend/javascripts/types/schemas/datasource.schema.ts:
resolution: { - frontend/javascripts/types/schemas/datasource.types.ts:
resolution: number | Vector3; - frontend/javascripts/admin/admin_rest_api.ts:
resolution: Vector3 | null | undefined;
🔗 Analysis chain
LGTM! Property rename aligns with server behavior.
The change from resolution to mag correctly reflects the server's behavior as mentioned in the PR objectives. The modification is consistent across all nodes and maintains the existing values.
Let's verify the type definitions to ensure consistency:
Also applies to: 73-73, 93-93
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the type definitions have been updated to use 'mag' instead of 'resolution'
# Expected: Should find type definitions using 'mag' and no occurrences of 'resolution' in node types
# Search for node type definitions
ast-grep --pattern 'type $_ = {
$$$
mag: $_
$$$
}'
# Verify no lingering 'resolution' fields in type definitions
rg 'resolution.*:' -g '*.ts' -g '*.tsx' -g '*.d.ts'
Length of output: 11629
| resolution: number; | ||
| mag: number; |
There was a problem hiding this comment.
Here is the root of the problem. The backend sends nodes with the field called mag and not resolution. See:
webknossos/webknossos-datastore/proto/SkeletonTracing.proto
Lines 8 to 19 in 8dec578
By changing the type, TS should show all locations to change and the resulting code should be fine.
There was a problem hiding this comment.
ah, that makes sense!
MichaelBuessemeyer
changed the title
Fixes changes introduced by #8111. The server sends nodes with the field called mag and not resolution. This pr changes the frontend type to expect the name as now called mag.
URL of deployed dev instance (used for testing):
Steps to test:
inMagfield should not be empty.inMagfield should be emptyIssues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores