Cleanup mjs module #9059

knollengewaechs · 2025-11-10T11:06:58Z

URL of deployed dev instance (used for testing):

https://cleanupmjs.webknossos.xyz/

Steps to test:

test that the following still works:
- all view modes (flight, orthogonal, oblique)
- affine transformations
- drag and drop skeleton nodes
- loading of ad-hoc meshes
- area measurement
- brushing

TODOs:

maybe have a look at other places and remove Float32Arrays there, too

Issues:

fixes Clean up mjs module for vector calculation #8666

(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)
Added migration guide entry if applicable (edit the same file as for the changelog)
Updated documentation if applicable
Adapted wk-libs python client if relevant API parts change
Removed dev-only changes like prints and application.conf edits
Considered common edge cases
Needs datastore update after deployment

coderabbitai · 2025-11-10T11:07:17Z

📝 Walkthrough

Walkthrough

Replaces Float32Array/typed-array defaults with plain JavaScript arrays across the mjs utilities and related viewers, introduces explicit Vector16/Matrix4x4 type exports, and updates multiple public signatures and local typings to use generic Array<number> / concrete Vector2/Vector3 types.

Changes

Cohort / File(s)	Summary
Core mjs module & types `frontend/javascripts/libs/mjs.ts`, `frontend/javascripts/types/mjs.d.ts`	Add `Vector16` and `Matrix4x4` exports; replace many typed-array allocations with plain `Array<number>`; broaden public signatures to accept `Array<number>` / `Int32Array` where applicable; remove `Vector2Like`/`Vector3Like` and use concrete `Vector2`/`Vector3` types; normalize helper overloads and internal allocations.
Viewer constants & API internals `frontend/javascripts/viewer/constants.ts`, `frontend/javascripts/viewer/api/api_latest.ts`	Change `Identity4x4` from `Float32Array` to `Matrix4x4` (plain array); switch internal matrix construction in `_createTransformsFromSpecs` from typed arrays to plain 16-element arrays; adjust type-only imports to `mjs`.
Geometry, plane, rotation helpers, and bucket logic `frontend/javascripts/viewer/geometries/helper_geometries.ts`, `frontend/javascripts/viewer/geometries/plane.ts`, `frontend/javascripts/viewer/model/helpers/rotation_helpers.ts`, `frontend/javascripts/viewer/model/bucket_data_handling/bucket_traversals.ts`, `frontend/javascripts/viewer/model/bucket_data_handling/bucket_picker_strategies/flight_bucket_picker.ts`, `frontend/javascripts/viewer/model/bucket_data_handling/bucket_picker_strategies/oblique_bucket_picker.ts`	Add/clarify local type annotations (`Vector3`, `number`), replace `V3.equals` usage with `V3.isEqual` with explicit subarray conversion, remove `ts-ignore` by adding explicit casts, factor negation into named constant, and add typed reduce/map callbacks.
Changelog `unreleased_changes/9059.md`	Add note describing removal of Float32Array defaults and switch to plain arrays for mjs module.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas to pay extra attention:
- frontend/javascripts/libs/mjs.ts — public signature changes (transformPointsAffine, transformPoints, inverse, extractTranslation, V2/V3 helpers) and propagation to callers.
- frontend/javascripts/types/mjs.d.ts — ensure new Vector16/Matrix4x4 aliases match runtime shapes and exported signatures.
- Places previously relying on typed-array identity (e.g., equality checks, binary interop) for subtle behavioral differences.

Possibly related PRs

Update ThreeJS imports #8787 — touches libs/mjs.ts vector/type overloads (divide3/Vector3), closely related to the type/signature work here.
Readd fixed version of "Allow permanent dataset layer rotation in dataset settings"" #8355 — modifies mjs matrix/vector types and Matrix4x4 usage; overlaps in identity/matrix representation changes.
Allow permanent dataset layer rotation in dataset settings #8159 — adjusts M4x4/Matrix4x4 APIs and mjs exports; related to the same core utilities.

Suggested labels

frontend, enhancement, performance

Suggested reviewers

philippotto
daniel-wer

Poem

🐰 I swapped the floats for roads of plain,
Arrays that hop, no typed-array train.
Vectors now cuddle in neat little rows,
M4x4 and friends — off the old rails they go! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Cleanup mjs module' accurately summarizes the main change: refactoring the mjs module to replace Float32Array allocations with plain arrays.
Linked Issues check	✅ Passed	The PR successfully implements the core objective from #8666: reducing Float32Array usage in the mjs module and replacing default allocations with plain arrays while maintaining the optional output parameter pattern.
Out of Scope Changes check	✅ Passed	All changes are directly related to issue #8666. Type refinements and import adjustments in api_latest, constants, and other files are necessary consequences of the mjs module refactoring.
Description check	✅ Passed	The pull request description clearly relates to the changeset. It provides deployment testing instructions, specifies what to test, references the fixed issue (#8666), and includes checklist items related to the changes (changelog added, other items marked done).

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch cleanup-mjs

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

### Steps to test: - CI -- - [x] Needs datastore update after deployment --------- Co-authored-by: MichaelBuessemeyer <[email protected]>

### URL of deployed dev instance (used for testing): - https://datasetliststorage.webknossos.xyz ### Steps to test: - hit `yarn enable-storage-scan`, wait for next tick (every 10 minutes, 1 minute after startup, can be changed in application.conf) - after scan, see used storage in dashboard ### TODOs: - [x] Hide column if orga total used storage is 0 - [x] Gather feedback on tooltip wording and coulmn name (size vs storage vs ??) -> https://scm.slack.com/archives/C5AKLAV0B/p1761836548789409 - [x] CI (api types mismatch?) ### Issues: - contributes to #9005 ------ - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [x] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) --------- Co-authored-by: Charlie Meister <[email protected]> Co-authored-by: Charlie Meister <[email protected]> Co-authored-by: MichaelBuessemeyer <[email protected]>

### URL of deployed dev instance (used for testing): - https://___.webknossos.xyz ### Steps to test: - Open a publicly available annotation without being logged in -> no error toast :) - I tested this on commit ea18578 (the merge of #8961) ### TODOs: - [ ] test on current master. Wasnt able to run wk locally this morning (https://scm.slack.com/archives/C5AKLAV0B/p1762768096605689) ### Issues: - fixes https://scm.slack.com/archives/C5AKLAV0B/p1762764001604969 - follow-up for #8961 ------ (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`) - [ ] Added migration guide entry if applicable (edit the same file as for the changelog) - [ ] Updated [documentation](../blob/master/docs) if applicable - [ ] Adapted [wk-libs python client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client) if relevant API parts change - [ ] Removed dev-only changes like prints and application.conf edits - [ ] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [ ] Needs datastore update after deployment

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)
292-300: Strip debug logging before shipping.

This console.log will fire for every brush step and flood the browser console in production. Please remove it (or guard it behind a proper logger/debug flag) before merging.
-        console.log("Brushing between:", lastPosition, addToLayerAction.position);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12861dd and d6e68c6.

📒 Files selected for processing (6)

frontend/javascripts/libs/mjs.ts (10 hunks)
frontend/javascripts/types/mjs.d.ts (0 hunks)
frontend/javascripts/viewer/api/api_latest.ts (2 hunks)
frontend/javascripts/viewer/constants.ts (2 hunks)
frontend/javascripts/viewer/model/bucket_data_handling/bucket_picker_strategies/flight_bucket_picker.ts (1 hunks)
frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1 hunks)

💤 Files with no reviewable changes (1)

frontend/javascripts/types/mjs.d.ts

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2024-11-22T17:18:04.217Z

Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 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/volumetracing_saga.tsx

📚 Learning: 2024-11-22T17:18:43.411Z

Learnt from: dieknolle3333
Repo: scalableminds/webknossos PR: 8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2024-11-22T17:18:43.411Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.

Applied to files:

frontend/javascripts/viewer/constants.ts

📚 Learning: 2025-07-22T12:56:47.507Z

Learnt from: daniel-wer
Repo: scalableminds/webknossos PR: 8787
File: frontend/javascripts/viewer/model/accessors/dataset_layer_transformation_accessor.ts:132-136
Timestamp: 2025-07-22T12:56:47.507Z
Learning: Three.js Vector3 has explicitly defined x, y, and z properties that can be safely accessed via dynamic string indexing when using union types like "x" | "y" | "z". The pattern `vector[axis] = value` where axis is "x" | "y" | "z" does not cause TypeScript strict mode errors because TypeScript can infer the property access is valid.

Applied to files:

frontend/javascripts/viewer/constants.ts
frontend/javascripts/libs/mjs.ts

🧬 Code graph analysis (5)

frontend/javascripts/viewer/model/sagas/volumetracing_saga.tsx (1)

frontend/javascripts/viewer/model/actions/volumetracing_actions.ts (1)

addToLayerAction (180-184)

frontend/javascripts/viewer/model/bucket_data_handling/bucket_picker_strategies/flight_bucket_picker.ts (2)

frontend/javascripts/libs/mjs.ts (1)

M4x4 (402-402)

frontend/javascripts/viewer/constants.ts (1)

Vector3 (15-15)

frontend/javascripts/viewer/api/api_latest.ts (2)

frontend/javascripts/libs/mjs.ts (1)

Matrix4x4 (33-33)

frontend/javascripts/types/mjs.d.ts (1)

Matrix4x4 (6-24)

frontend/javascripts/viewer/constants.ts (1)

frontend/javascripts/libs/mjs.ts (1)

Vector16 (15-32)

frontend/javascripts/libs/mjs.ts (2)

frontend/javascripts/types/mjs.d.ts (1)

Matrix4x4 (6-24)

frontend/javascripts/viewer/constants.ts (2)

Vector3 (15-15)

Vector2 (14-14)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: backend-tests
GitHub Check: build-smoketest-push

knollengewaechs · 2025-11-12T14:29:43Z

@ philippotto I exchanged every new initialization of float32arrays within the mjs module. I was hesitant about moving away from float32arrays in other places, e.g. api_latest. Do you think it makes sense to move to arrays/numbers there, too?

philippotto

thank you for doing this!

philippotto · 2025-11-14T15:40:47Z

frontend/javascripts/viewer/geometries/plane.ts

    const scaledPosition = V3.multiply(originalPosition, this.datasetScaleFactor);
    // The offset is in world space already so no scaling is necessary.
-    const offsetPosition = V3.add(scaledPosition, positionOffset);
+    const offsetPosition: Vector3 = V3.add(scaledPosition, positionOffset);


shouldn't the return value of V3.add automatically be Vector3?

philippotto · 2025-11-14T15:41:37Z

frontend/javascripts/viewer/model/bucket_data_handling/bucket_traversals.ts

  // In addition, the variables stepX and stepY are initialized to either 1 or -1 indicating whether X and Y are
  // incremented or decremented as the ray crosses voxel boundaries (this is determined by the sign of the x and y components of → v).
-  const [stepX, stepY, stepZ] = v.map((el) => Math.sign(el));
+  const [stepX, stepY, stepZ] = v.map((el: number) => Math.sign(el));


why is this change necessary?

philippotto · 2025-11-14T15:42:42Z

frontend/javascripts/libs/mjs.ts

+    r?: Int32Array | Array<number> | null | undefined,
+  ): Int32Array | Array<number> {


why is Int32Array necessary here?

knollengewaechs added 2 commits November 6, 2025 16:03

dont access three.vector3 via index

3ec3f4b

replace new Float32Array with new Array()

90c0755

knollengewaechs self-assigned this Nov 10, 2025

fm3 and others added 7 commits November 10, 2025 12:25

update backend dependencies, fix typos with codespell (#8991)

bfc44da

### Steps to test: - CI -- - [x] Needs datastore update after deployment --------- Co-authored-by: MichaelBuessemeyer <[email protected]>

Merge branch 'master' into cleanup-mjs

c8f8bb8

remove float32 arrays

565ac89

remove some ts-ignores

bd80ffb

one more init as array

d6e68c6

knollengewaechs marked this pull request as ready for review November 10, 2025 17:04

fix matrix type

0490fe6

coderabbitai bot reviewed Nov 10, 2025

View reviewed changes

knollengewaechs marked this pull request as draft November 11, 2025 08:33

knollengewaechs added 3 commits November 12, 2025 15:13

revert some float32changes

9e395a0

remove dev comment

615f3b5

remove more dev logs

2160508

knollengewaechs added 2 commits November 13, 2025 15:01

completely remove float32 from mjs module

d1fb855

fix types

8ab833f

knollengewaechs marked this pull request as ready for review November 13, 2025 15:20

knollengewaechs and others added 2 commits November 13, 2025 16:22

Merge branch 'master' into cleanup-mjs

45b03db

changelog

30a9003

knollengewaechs requested a review from philippotto November 13, 2025 16:02

philippotto reviewed Nov 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup mjs module #9059

Cleanup mjs module #9059

Uh oh!

knollengewaechs commented Nov 10, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 10, 2025 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

knollengewaechs commented Nov 12, 2025 •

edited

Loading

Uh oh!

philippotto left a comment

Uh oh!

philippotto Nov 14, 2025

Uh oh!

philippotto Nov 14, 2025

Uh oh!

philippotto Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		r?: Int32Array \| Array<number> \| null \| undefined,
		): Int32Array \| Array<number> {

Cleanup mjs module #9059

Are you sure you want to change the base?

Cleanup mjs module #9059

Uh oh!

Conversation

knollengewaechs commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

URL of deployed dev instance (used for testing):

Steps to test:

TODOs:

Issues:

Uh oh!

coderabbitai bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

knollengewaechs commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philippotto left a comment

Choose a reason for hiding this comment

Uh oh!

philippotto Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

philippotto Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

philippotto Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knollengewaechs commented Nov 10, 2025 •

edited

Loading

coderabbitai bot commented Nov 10, 2025 •

edited

Loading

knollengewaechs commented Nov 12, 2025 •

edited

Loading