Skip to content

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Aug 29, 2025

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):

  • https://___.webknossos.xyz

Steps to test:

Issues:


(Please delete unneeded items, merge only when none are left open)

  • Added changelog entry (create a $PR_NUMBER.md file in unreleased_changes or use ./tools/create-changelog-entry.py)

Copy link
Contributor

coderabbitai bot commented Aug 29, 2025

📝 Walkthrough

Walkthrough

Mapping 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

Cohort / File(s) Summary
Schema and validation normalization
frontend/javascripts/types/schemas/url_state.schema.ts, frontend/javascripts/types/validation.ts
Schema changes MappingType from enum ["JSON","HDF5",null] to string with default "HDF5"; description clarifies only "JSON" is preserved, others treated as "HDF5". Validation now normalizes mappingType accordingly, using lodash for deep cloning.
Viewer model: sagas and adapters
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_adapter.ts, .../model/sagas/annotation_saga.tsx, .../model/sagas/saga_helpers.ts, .../model/sagas/volume/mapping_saga.ts, .../model/sagas/volume/proofread_saga.ts
Conditions previously tied to "HDF5" now treat any non-"JSON" mappingType as agglomerate/remote. Updated checks affect locking prompts, zoom warnings, request parameters, agglomerate detection, and proofread gating.
State/reducer logic
frontend/javascripts/viewer/model/reducers/volumetracing_reducer_helpers.ts
setMappingNameReducer clears mappingName when mappingType is "JSON" or null, or when disabled; not for other types. Previously cleared for non-"HDF5".
UI usage of mapping name
frontend/javascripts/viewer/view/action_bar_view.tsx, .../view/left-border-tabs/modals/add_volume_layer_modal.tsx, .../view/right-border-tabs/segments_tab/segments_view.tsx
Guards for passing maybeMappingName broadened from mappingType === "HDF5" to mappingType !== "JSON".
Dataset settings viewconfig
frontend/javascripts/dashboard/dataset/dataset_settings_viewconfig_tab.tsx
Existence check flips: "JSON" queries mappingsForLayer; other types query agglomeratesForLayer.
Docs
unreleased_changes/8880.md
Notes friendlier handling of mapping types with default fallback to HDF5 for unknown values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • philippotto

Poem

I thump my paws at types so sly,
If not "JSON", then HDF5 we try.
I nibble schemas, normalize the stew,
Sagas hop along, with checks anew.
Unknowns? No fear—default we contrive.
Hippity-hop, our mappings thrive! 🐇✨

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fallback-to-hdf5-for-agglo-mappings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbit in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbit in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbit gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbit read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbit help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbit ignore or @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbit summary or @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbit or @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review August 29, 2025 13:33
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 checking mappingName (typed string | 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 for null, which can clear mappings unexpectedly. Align with isAgglomerate.

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 to ensureAgglomerateMappingIsEnabled 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 omits applyAgglomerate when mappingName is null. To reduce duplication and future drift, extract a helper like shouldApplyAgglomerateOnServer(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 shared isHdf5LikeMapping predicate and align needsLocalHdf5Mapping

  • 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) with isHdf5LikeMapping(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 inline mappingInfo.mappingType !== "JSON" check with the shared isJsonMapping/isHdf5LikeMapping utility to keep mapping logic consistent. addLayerToAnnotation already accepts mappingName?: 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 small mapping_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.

📥 Commits

Reviewing files that changed from the base of the PR and between e768761 and 01959a5.

📒 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.

Comment on lines +34 to 37
type: "string",
default: "HDF5",
description: "If value is 'JSON', it is kept. Any other string is treated as 'HDF5'.",
},
Copy link
Contributor

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.

Suggested change
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.

Comment on lines +72 to +84
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;
});
};
Copy link
Contributor

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).

Comment on lines 104 to 107
const isHDF5Mapping =
volumeTracing.tracingId in activeMappingByLayer &&
activeMappingByLayer[volumeTracing.tracingId].mappingType === "HDF5";
activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON";
if (isSomeMappingActive && isHDF5Mapping) {
Copy link
Contributor

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.

Suggested change
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.

Comment on lines 843 to 849
const maybeMappingName =
!isEditableMapping &&
mappingInfo.mappingStatus !== MappingStatusEnum.DISABLED &&
mappingInfo.mappingType === "HDF5" &&
mappingInfo.mappingType !== "JSON" &&
mappingInfo.mappingName != null
? mappingInfo.mappingName
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

Comment on lines +1 to +2
### Fixed
- Change that WEBKNOSSOS takes mapping types more friendly and defaults back to HDF5 in case the type is unkown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
### 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.

@daniel-wer
Copy link
Member

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 ARRAY (in contrast to the json dict-based mappings if I recall correctly) with a comment indicating that this includes hdf5 and zarr.

@MichaelBuessemeyer
Copy link
Contributor Author

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 ARRAY (in contrast to the json dict-based mappings if I recall correctly) with a comment indicating that this includes hdf5 and zarr.

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? 🤔
@fm3 What do you think about this suggestion?

@MichaelBuessemeyer MichaelBuessemeyer removed the request for review from normanrz September 1, 2025 13:50
@fm3
Copy link
Member

fm3 commented Sep 1, 2025

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.

Copy link
Member

@daniel-wer daniel-wer left a 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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this suggestion 👍

@daniel-wer
Copy link
Member

Open https://fallbacktohdfforagglomappings.webknossos.xyz/datasets/test-agglomerate-file-68b1a05d010000a500a96176/view. It should have agglo mapping 70 active.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants