-
Notifications
You must be signed in to change notification settings - Fork 29
Frontend Only: Default to treating a mapping as HDF5 in case it is not json #8880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughMapping logic across the app switches from explicitly checking for "HDF5" to treating any non-"JSON" value as agglomerate/remote. Schema and validation now accept any string, normalizing unknown types to "HDF5". Multiple UI, reducer, saga, and adapter conditions updated accordingly. A documentation note about defaulting unknown mapping types is added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts (1)
168-176
: Clarify mappingName JSDoc and drop the unnecessary type check
- Change the JSDoc line
“Only null or the name of a HDF5 mapping is stored”
to
“Only null or the name of a non-JSON mapping is stored.”- No change needed to
MappingType
—you’re checkingmappingName
(typedstring | null | undefined
), so the null guard is safe.frontend/javascripts/viewer/view/action_bar_view.tsx (1)
163-170
: Guard against null mappingType before treating as non-JSON.
null !== "JSON"
is true, so we may pass a mapping name unintentionally when the type is unknown. Match the null-safe semantics used in the sagas.Apply:
- mappingInfo.mappingType !== "JSON" + mappingInfo.mappingType != null && + mappingInfo.mappingType !== "JSON"Consider extracting a shared util for this predicate to keep behavior consistent.
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
124-134
: Also null-guard before clearing locally applied mapping.Here,
mapping.mappingType !== "JSON"
will also be true fornull
, which can clear mappings unexpectedly. Align withisAgglomerate
.Apply:
- mapping.mappingType !== "JSON" + mapping.mappingType != null && + mapping.mappingType !== "JSON"
🧹 Nitpick comments (6)
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts (1)
283-291
: Align error message with broadened mapping logic (non-JSON, not just HDF5).The gating now allows any non-JSON mapping. Update the toast to avoid implying HDF5-only.
- Toast.error("An HDF5 mapping needs to be enabled to use the proofreading tool."); + Toast.error("An agglomerate mapping (non-JSON) must be enabled to use the proofreading tool.");Optional: consider renaming
ensureHdf5MappingIsEnabled
toensureAgglomerateMappingIsEnabled
for consistency.frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts (1)
191-200
: Broadened condition looks good; consider extracting a shared predicate.Using
mappingType !== "JSON"
matches the PR intent and correctly omitsapplyAgglomerate
whenmappingName
is null. To reduce duplication and future drift, extract a helper likeshouldApplyAgglomerateOnServer(activeMapping)
and reuse it across sagas/adapters.frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx (1)
80-83
: Existence check logic looks correct; consider centralizing the JSON predicate and updating help text.
- The JSON vs non-JSON split matches the new fallback semantics. Nice.
- To avoid scattering string literals, consider a shared helper (e.g., isJsonMapping(type): boolean) reused across UI and sagas.
- The tooltip example still says type: 'HDF5'. Consider adjusting to “type: 'JSON' or any non-JSON string (treated as HDF5-like)” to prevent confusion.
Also applies to: 132-133
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx (1)
170-173
: Use sharedisHdf5LikeMapping
predicate and alignneedsLocalHdf5Mapping
- Replace all direct
mappingType === "HDF5"
/!== "HDF5"
checks (e.g. model_initialization.ts line 893, mapping_settings_view.tsx line 99, volumetracing_accessor.ts line 855) withisHdf5LikeMapping(mappingType)
to centralize logic.- Ensure
needsLocalHdf5Mapping
uses the same predicate so it treats any non-JSON mapping as HDF5-like.frontend/javascripts/viewer/view/left-border-tabs/modals/add_volume_layer_modal.tsx (1)
201-207
: Use centralized mapping-type helpers
In add_volume_layer_modal.tsx (around line 201), replace the inlinemappingInfo.mappingType !== "JSON"
check with the sharedisJsonMapping
/isHdf5LikeMapping
utility to keep mapping logic consistent.addLayerToAnnotation
already acceptsmappingName?: string|null|undefined
.frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (1)
356-428
: Optional: consolidate “non-JSON” logic into a shared helper.Multiple sites now reimplement the same predicate. Export a single helper (e.g., from
saga_helpers
or a smallmapping_utils
) and use it in UI and sagas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx
(1 hunks)frontend/javascripts/types/schemas/url_state.schema.ts
(1 hunks)frontend/javascripts/types/validation.ts
(2 hunks)frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts
(1 hunks)frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts
(1 hunks)frontend/javascripts/viewer/model/sagas/annotation_saga.tsx
(1 hunks)frontend/javascripts/viewer/model/sagas/saga_helpers.ts
(1 hunks)frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts
(2 hunks)frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
(1 hunks)frontend/javascripts/viewer/view/action_bar_view.tsx
(1 hunks)frontend/javascripts/viewer/view/left-border-tabs/modals/add_volume_layer_modal.tsx
(1 hunks)frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
(1 hunks)unreleased_changes/8880.md
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T17:18:04.217Z
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
frontend/javascripts/viewer/model/sagas/annotation_saga.tsx
frontend/javascripts/viewer/model/sagas/volume/proofread_saga.ts
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts
🪛 LanguageTool
unreleased_changes/8880.md
[grammar] ~1-~1: There might be a mistake here.
Context: ### Fixed - Change that WEBKNOSSOS takes mapping typ...
(QB_NEW_EN)
[grammar] ~2-~2: Ensure spelling is correct
Context: ...faults back to HDF5 in case the type is unkown.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (2)
frontend/javascripts/viewer/model/sagas/volume/mapping_saga.ts (2)
164-166
: Good: null-safe agglomerate detection.Adding
mapping.mappingType != null
avoids misclassifying unknown types as agglomerates. Matches the PR’s intent.
415-427
: Default-else branch is correct for non-JSON types.Routing all non-JSON types through the HDF5 code path is consistent with the new defaulting behavior.
type: "string", | ||
default: "HDF5", | ||
description: "If value is 'JSON', it is kept. Any other string is treated as 'HDF5'.", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Backwards-compat: default alone won’t prevent validation failures.
Setting MappingType to a string with default "HDF5" is fine, but JSON Schema defaults aren’t applied unless your validator injects them before “required” checks. Older URLs lacking mappingType may now fail because mappingInfo.required still includes "mappingType".
- Either ensure validateUrlStateJSON injects the default before validating required, or drop mappingType from required to let the default kick in.
Please confirm your validator applies defaults pre-required. If not, consider:
- required: ["mappingName", "mappingType"],
+ required: ["mappingName"],
🏁 Script executed:
#!/bin/bash
# Locate the URL state schema and inspect its “required” array
rg -nC3 "required" frontend/javascripts/types/schemas/url_state.schema.ts
# Find where validateUrlStateJSON is defined/called
rg -nC3 "validateUrlStateJSON" -g "*.ts"
# Search for Ajv instantiation to see if defaults are applied
rg -nP "new\s+Ajv" -C3 .
# Check for useDefaults or default injection options in the validator setup
rg -nC3 "useDefaults" -C3 .
Length of output: 3479
🏁 Script executed:
#!/bin/bash
# Show mappingType property definition and default in UrlState schema
rg -nC5 '"mappingType"' frontend/javascripts/types/schemas/url_state.schema.ts
# Display the UrlStateByLayer section including required array
sed -n '100,160p' frontend/javascripts/types/schemas/url_state.schema.ts
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Locate the implementation of validateWithSchemaSync to inspect how it invokes AJV or other validator
rg -nC5 "function validateWithSchemaSync" -C3 .
# Fallback: search for its definition if not a function declaration (e.g., imported from another module)
rg -nC5 "validateWithSchemaSync" -C3 .
Length of output: 1784
🏁 Script executed:
#!/bin/bash
rg -n "import Ajv" -C3 frontend/javascripts/types/validation.ts
rg -n "new Ajv" -C3 frontend/javascripts/types/validation.ts
rg -n "const validator" -C3 frontend/javascripts/types/validation.ts
rg -n "useDefaults" -C3 frontend/javascripts/types/validation.ts
Length of output: 695
Inject defaults or drop mappingType
from required
The jsonschema.Validator
used in validateWithSchemaSync
does not apply schema default
values, so URLs without mappingType
will fail the "mappingType"
required
check. Either set "mappingType":"HDF5"
in validateUrlStateJSON
before validating, or update url_state.schema.ts (lines 118–122) as follows:
- required: ["mappingName", "mappingType"],
+ required: ["mappingName"],
📝 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.
type: "string", | |
default: "HDF5", | |
description: "If value is 'JSON', it is kept. Any other string is treated as 'HDF5'.", | |
}, | |
{ | |
title: "Mapping options", | |
properties: { | |
mappingName: { | |
type: "string", | |
// ... | |
}, | |
mappingType: { | |
type: "string", | |
default: "HDF5", | |
description: "If value is 'JSON', it is kept. Any other string is treated as 'HDF5'.", | |
}, | |
}, | |
required: ["mappingName"], | |
}, |
🤖 Prompt for AI Agents
In frontend/javascripts/types/schemas/url_state.schema.ts around lines 34–37,
the schema's "default" for mappingType is not applied by jsonschema.Validator
causing required-check failures; fix by either (A) setting url.mappingType =
url.mappingType || "HDF5" inside validateUrlStateJSON before calling
validateWithSchemaSync, or (B) removing "mappingType" from the schema's required
array (lines ~118–122) so missing fields don't fail validation; choose one of
these two options and implement it consistently.
export const validateUrlStateJSON = (value: string) => { | ||
const json = validateWithSchemaSync("types::UrlManagerState", value); | ||
return _.cloneDeepWith(json, (value, key) => { | ||
if (key === "mappingType") { | ||
if (value === "JSON" || value === null) { | ||
return value; | ||
} | ||
return "HDF5"; | ||
} | ||
// let lodash handle everything else | ||
return undefined; | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Normalize mappingType case-insensitively and preserve null/undefined.
Tolerate inputs like "json"/"Json" and avoid coercing absent values. Current code is case-sensitive.
export const validateUrlStateJSON = (value: string) => {
const json = validateWithSchemaSync("types::UrlManagerState", value);
- return _.cloneDeepWith(json, (value, key) => {
- if (key === "mappingType") {
- if (value === "JSON" || value === null) {
- return value;
- }
- return "HDF5";
- }
- // let lodash handle everything else
- return undefined;
- });
+ return _.cloneDeepWith(json, (val, key) => {
+ if (key === "mappingType") {
+ if (val == null) return val; // preserve null/undefined
+ const s = typeof val === "string" ? val.toUpperCase() : val;
+ return s === "JSON" ? "JSON" : "HDF5";
+ }
+ return undefined; // let lodash handle everything else
+ });
};
🤖 Prompt for AI Agents
In frontend/javascripts/types/validation.ts around lines 72 to 84, the
mappingType normalization is currently case-sensitive and coerces absent values;
modify the cloneDeepWith callback so that when key === "mappingType" you first
if (value == null) return value to preserve null/undefined, then if (typeof
value === "string") compare value.toLowerCase() to "json" and return the
normalized "JSON" when it matches, otherwise return "HDF5"; leave all other
cases to lodash by returning undefined.
💡 Verification agent
🧩 Analysis chain
Add lightweight verification to catch lingering HDF5-specific checks.
Search for strict "HDF5"
comparisons that may need porting to the non-JSON semantic.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find hard-coded HDF5 checks that may need conversion to `mappingType !== "JSON"`
rg -nP -C2 --type=ts --type=tsx '\bmappingType\s*===\s*"HDF5"|\bmappingType\s*!==\s*"HDF5"'
# Spots where ENABLED status is checked alongside mappingType usage
rg -nP -C2 --type=ts --type=tsx 'mappingStatus\s*===\s*MappingStatusEnum\.ENABLED[^\n]*\n.*\bmappingType\b'
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find hard-coded HDF5 checks that may need conversion to `mappingType !== "JSON"` across TS/TSX files
rg -nP -C2 '\bmappingType\s*(?:===|!==)\s*"HDF5"' -g '*.ts' -g '*.tsx'
# Find spots where ENABLED status is checked alongside mappingType usage
rg -nP -C4 'MappingStatusEnum\.ENABLED.*\bmappingType\b' -g '*.ts' -g '*.tsx'
Length of output: 1699
Replace lingering HDF5-specific checks
Update or remove hard-coded "HDF5"
branches to instead handle the new non-JSON mapping semantics:
- frontend/javascripts/viewer/view/left-border-tabs/mapping_settings_view.tsx L99–100
- frontend/javascripts/viewer/model_initialization.ts L893–895
- frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts L855–857
🤖 Prompt for AI Agents
In frontend/javascripts/types/validation.ts around lines 72–84, the function
still hard-codes "HDF5" for non-JSON mapping types; change it so non-JSON cases
follow the new non-JSON mapping semantics instead of returning "HDF5" — either
return the original value (preserve unknown mappingType) or normalize to the new
agreed-upon token (e.g., "NON_JSON") if your app defines one; then update the
three referenced files
(frontend/javascripts/viewer/view/left-border-tabs/mapping_settings_view.tsx
L99–100, frontend/javascripts/viewer/model_initialization.ts L893–895,
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts L855–857)
to stop checking for "HDF5" and instead handle the new non-JSON token or the
preserved value consistently (replace equality checks and switch branches
accordingly).
const isHDF5Mapping = | ||
volumeTracing.tracingId in activeMappingByLayer && | ||
activeMappingByLayer[volumeTracing.tracingId].mappingType === "HDF5"; | ||
activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON"; | ||
if (isSomeMappingActive && isHDF5Mapping) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Guard against undefined mappingType when deciding to prompt for locking.
If mappingType
is undefined, the current condition treats it as non-JSON and prompts unnecessarily.
- const isHDF5Mapping =
- volumeTracing.tracingId in activeMappingByLayer &&
- activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON";
+ const isHDF5Mapping =
+ volumeTracing.tracingId in activeMappingByLayer &&
+ !!activeMappingByLayer[volumeTracing.tracingId].mappingType &&
+ activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON";
Optional: centralize this predicate (e.g., isAgglomerateMapping(activeMapping)
) to avoid drift across files.
📝 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.
const isHDF5Mapping = | |
volumeTracing.tracingId in activeMappingByLayer && | |
activeMappingByLayer[volumeTracing.tracingId].mappingType === "HDF5"; | |
activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON"; | |
if (isSomeMappingActive && isHDF5Mapping) { | |
const isHDF5Mapping = | |
volumeTracing.tracingId in activeMappingByLayer && | |
!!activeMappingByLayer[volumeTracing.tracingId].mappingType && | |
activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON"; | |
if (isSomeMappingActive && isHDF5Mapping) { |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/model/sagas/saga_helpers.ts around lines 104 to
107, the predicate treats an undefined mappingType as non-JSON and will prompt
for locking incorrectly; change the condition to explicitly check mappingType
=== "JSON" (e.g., ensure
activeMappingByLayer[volumeTracing.tracingId].mappingType !== undefined &&
activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON") or better
invert to mappingType === "JSON" to avoid false positives, and optionally
extract this logic into a shared helper like isAgglomerateMapping(activeMapping)
to centralize the predicate across files.
const maybeMappingName = | ||
!isEditableMapping && | ||
mappingInfo.mappingStatus !== MappingStatusEnum.DISABLED && | ||
mappingInfo.mappingType === "HDF5" && | ||
mappingInfo.mappingType !== "JSON" && | ||
mappingInfo.mappingName != null | ||
? mappingInfo.mappingName | ||
: undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not treat null mappingType as agglomerate (add explicit null guard).
Without a null check, mappingType !== "JSON"
evaluates true for null
, causing maybeMappingName
to be sent when the type is unknown. Align with saga’s isAgglomerate
which guards nulls.
Apply:
- mappingInfo.mappingType !== "JSON" &&
+ mappingInfo.mappingType != null &&
+ mappingInfo.mappingType !== "JSON" &&
Optionally, centralize this check via a shared helper (e.g., isNonJsonMappingType(mappingType)
), and reuse across UI and sagas to avoid drift.
📝 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.
const maybeMappingName = | |
!isEditableMapping && | |
mappingInfo.mappingStatus !== MappingStatusEnum.DISABLED && | |
mappingInfo.mappingType === "HDF5" && | |
mappingInfo.mappingType !== "JSON" && | |
mappingInfo.mappingName != null | |
? mappingInfo.mappingName | |
: undefined; | |
const maybeMappingName = | |
!isEditableMapping && | |
mappingInfo.mappingStatus !== MappingStatusEnum.DISABLED && | |
mappingInfo.mappingType != null && | |
mappingInfo.mappingType !== "JSON" && | |
mappingInfo.mappingName != null | |
? mappingInfo.mappingName | |
: undefined; |
🤖 Prompt for AI Agents
In
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segments_view.tsx
around lines 843 to 849, the expression treats null mappingType as non-JSON
(causing maybeMappingName to be set incorrectly); add an explicit null/undefined
guard so mappingInfo.mappingType is checked before comparing to "JSON" (e.g.,
require mappingInfo.mappingType != null && mappingInfo.mappingType !== "JSON"),
and optionally extract this logic into a shared helper like
isNonJsonMappingType(mappingType) to reuse across UI and sagas.
### Fixed | ||
- Change that WEBKNOSSOS takes mapping types more friendly and defaults back to HDF5 in case the type is unkown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix grammar and clarity in changelog entry.
Improve wording and correct typo "unkown".
Apply:
-### Fixed
-- Change that WEBKNOSSOS takes mapping types more friendly and defaults back to HDF5 in case the type is unkown.
+### Fixed
+- WEBKNOSSOS now handles mapping types more flexibly and defaults to HDF5 if the type is unknown.
📝 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.
### Fixed | |
- Change that WEBKNOSSOS takes mapping types more friendly and defaults back to HDF5 in case the type is unkown. | |
### Fixed | |
- WEBKNOSSOS now handles mapping types more flexibly and defaults to HDF5 if the type is unknown. |
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: There might be a mistake here.
Context: ### Fixed - Change that WEBKNOSSOS takes mapping typ...
(QB_NEW_EN)
[grammar] ~2-~2: Ensure spelling is correct
Context: ...faults back to HDF5 in case the type is unkown.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In unreleased_changes/8880.md around lines 1 to 2, the changelog sentence is
grammatically incorrect and contains a typo ("unkown"); replace the line with a
clear, corrected entry such as: "Change: WEBKNOSSOS accepts mapping types more
flexibly and defaults to HDF5 when the type is unknown." Ensure the header
"Fixed" remains and the wording uses present tense and correct spelling.
Please ignore if this complicates things too much, but I'd favor to find a more general term for the non-json mappings (hdf5 and zarr) to avoid confusion. Maybe |
Thanks a very good reasoning imo. But this would me we have to change things in the backend as well and maybe also the worker / vx? 🤔 |
I’d say this renaming would be a good idea generally. I also suggested the name Agglomerate instead of Array. However, since it is not immediately obvious what all the spots are that would have to be touched, let’s not have this block this bugfix PR. Let’s make it a follow-up issue instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code almost LGTM. Thanks for fixing! 👍
@@ -31,7 +31,9 @@ export default { | |||
enum: ["orthogonal", "oblique", "flight", "volume"], | |||
}, | |||
"types::MappingType": { | |||
enum: ["JSON", "HDF5", null], | |||
type: "string", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: "string", | |
type: ["string", "null"], |
according to https://stackoverflow.com/questions/16241333/specify-a-value-can-be-a-string-or-null-with-json-schema.
At least that's how I read the validateUrlStateJSON
method in validation.ts
.
@@ -0,0 +1,2 @@ | |||
### Fixed | |||
- Change that WEBKNOSSOS takes mapping types more friendly and defaults back to HDF5 in case the type is unkown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion 👍
My experience: I opened the link and agglo mapping 70 was indeed active. I wondered why that was the case, since the link only points to the dataset and is not a sharing link, so I opened the default view settings of the dataset where no mapping was configured 🤔 Afterwards, I disabled the mapping and reloaded. Now the mapping is no longer active and I cannot get back to the state where I open the link and the mapping is active. Do you understand this? |
This PR makes the frontend treat any mapping as hdf5 which does not explicitly claim to be json. This helps as there are now also zarr based mappings which have the same api as hdf5 stored mappings. Thus treating them equally in the frontend should be fine. Thus, the frontend is more friendly when not the "correct" type hdf5 is provided.
URL of deployed dev instance (used for testing):
Steps to test:
Zarr3
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.md
file inunreleased_changes
or use./tools/create-changelog-entry.py
)