-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -31,7 +31,9 @@ export default { | |||||||||||||||||||||||||||||||||||||||
enum: ["orthogonal", "oblique", "flight", "volume"], | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
"types::MappingType": { | ||||||||||||||||||||||||||||||||||||||||
enum: ["JSON", "HDF5", null], | ||||||||||||||||||||||||||||||||||||||||
type: "string", | ||||||||||||||||||||||||||||||||||||||||
default: "HDF5", | ||||||||||||||||||||||||||||||||||||||||
description: "If value is 'JSON', it is kept. Any other string is treated as 'HDF5'.", | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
Comment on lines
+34
to
37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainBackwards-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".
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 - required: ["mappingName", "mappingType"],
+ required: ["mappingName"], 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||||
"types::Mesh": { | ||||||||||||||||||||||||||||||||||||||||
type: "object", | ||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import jsonschema from "jsonschema"; | ||
import _ from "lodash"; | ||
import ViewConfigurationSchema from "types/schemas/dataset_view_configuration.schema"; | ||
import DatasourceSchema from "types/schemas/datasource.schema"; | ||
import UrlStateSchema from "types/schemas/url_state.schema"; | ||
|
@@ -68,8 +69,19 @@ export const validateLayerViewConfigurationObjectJSON = validateWithSchema( | |
"types::LayerViewConfigurationObject", | ||
); | ||
|
||
export const validateUrlStateJSON = (value: string) => | ||
validateWithSchemaSync("types::UrlManagerState", value); | ||
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; | ||
}); | ||
}; | ||
Comment on lines
+72
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
💡 Verification agent 🧩 Analysis chainAdd lightweight verification to catch lingering HDF5-specific checks. Search for strict 🏁 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
🤖 Prompt for AI Agents
|
||
|
||
export const isValidJSON = (json: string) => { | ||
try { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -103,7 +103,7 @@ export function* ensureMaybeActiveMappingIsLocked( | |||||||||||||||||||||
activeMappingByLayer[volumeTracing.tracingId].mappingStatus === MappingStatusEnum.ENABLED; | ||||||||||||||||||||||
const isHDF5Mapping = | ||||||||||||||||||||||
volumeTracing.tracingId in activeMappingByLayer && | ||||||||||||||||||||||
activeMappingByLayer[volumeTracing.tracingId].mappingType === "HDF5"; | ||||||||||||||||||||||
activeMappingByLayer[volumeTracing.tracingId].mappingType !== "JSON"; | ||||||||||||||||||||||
if (isSomeMappingActive && isHDF5Mapping) { | ||||||||||||||||||||||
Comment on lines
104
to
107
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - 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., 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||
try { | ||||||||||||||||||||||
return yield* call(askUserForLockingActiveMapping, volumeTracing, activeMappingByLayer); | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -843,7 +843,7 @@ class SegmentsView extends React.Component<Props, State> { | |||||||||||||||||||||||||||||||||
const maybeMappingName = | ||||||||||||||||||||||||||||||||||
!isEditableMapping && | ||||||||||||||||||||||||||||||||||
mappingInfo.mappingStatus !== MappingStatusEnum.DISABLED && | ||||||||||||||||||||||||||||||||||
mappingInfo.mappingType === "HDF5" && | ||||||||||||||||||||||||||||||||||
mappingInfo.mappingType !== "JSON" && | ||||||||||||||||||||||||||||||||||
mappingInfo.mappingName != null | ||||||||||||||||||||||||||||||||||
? mappingInfo.mappingName | ||||||||||||||||||||||||||||||||||
: undefined; | ||||||||||||||||||||||||||||||||||
Comment on lines
843
to
849
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, Apply: - mappingInfo.mappingType !== "JSON" &&
+ mappingInfo.mappingType != null &&
+ mappingInfo.mappingType !== "JSON" && Optionally, centralize this check via a shared helper (e.g., 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,2 @@ | ||||||||||
### Fixed | ||||||||||
- Change that WEBKNOSSOS takes mapping types more friendly and defaults back to HDF5 in case the type is unkown. | ||||||||||
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
🧰 Tools🪛 LanguageTool[grammar] ~1-~1: There might be a mistake here. (QB_NEW_EN) [grammar] ~2-~2: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) 🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this suggestion 👍 |
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.
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 invalidation.ts
.