Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions frontend/javascripts/viewer/api/api_latest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ import {
refreshMeshesAction,
removeMeshAction,
updateCurrentMeshFileAction,
updateMeshOpacityAction,
updateMeshVisibilityAction,
} from "viewer/model/actions/annotation_actions";
import { setLayerTransformsAction } from "viewer/model/actions/dataset_actions";
Expand Down Expand Up @@ -2756,12 +2757,12 @@ class DataApi {

/**
* Set the RGB color of a segment (and its mesh) for a given segmentation layer. If layerName is not passed,
* the currently visible segmentation layer will be used.
* the currently visible segmentation layer will be used. If the mesh is loaded, optionally update its opacity.
*
* @example
* api.data.setSegmentColor(3, [0, 1, 1]);
* api.data.setSegmentColor(3, [0, 1, 1], "segmentation", 0.5);
*/
setSegmentColor(segmentId: number, rgbColor: Vector3, layerName?: string) {
setSegmentColor(segmentId: number, rgbColor: Vector3, layerName?: string, meshOpacity?: number) {
const effectiveLayerName = getRequestedOrVisibleSegmentationLayerEnforced(
Store.getState(),
layerName,
Expand All @@ -2778,6 +2779,10 @@ class DataApi {
true,
),
);

if (meshOpacity != null) {
Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity));
}
Comment on lines +2782 to +2785
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 | 🔴 Critical

🧩 Analysis chain

Consider opacity validation and mesh existence check.

The implementation dispatches updateMeshOpacityAction without:

  1. Validating that meshOpacity is in a valid range (typically 0-1 for opacity values)
  2. Checking whether a mesh is loaded for the given segment

Per your PR comment, you noted uncertainty about whether to assume the mesh is loaded. For consistency with other mesh methods in this API (e.g., setMeshVisibility at lines 2579-2598 checks mesh existence and throws an error), consider similar validation here.

Run this script to compare error handling patterns in other mesh-related methods:

Additionally, consider validating the opacity range:

     if (meshOpacity != null) {
+      if (meshOpacity < 0 || meshOpacity > 1) {
+        throw new Error(`meshOpacity must be between 0 and 1, but got ${meshOpacity}`);
+      }
       Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity));
     }

🏁 Script executed:

#!/bin/bash
# Check how other mesh methods handle mesh existence validation
rg -nP -A10 'setMeshVisibility|removeMesh' --type=ts frontend/javascripts/viewer/api/api_latest.ts

Length of output: 2625


[/run_scripts]


🏁 Script executed:

#!/bin/bash
# Extract complete implementation context for updateMeshOpacity and setMeshVisibility
echo "=== setMeshVisibility implementation (lines 2579-2610) ==="
sed -n '2579,2610p' frontend/javascripts/viewer/api/api_latest.ts

echo ""
echo "=== updateMeshOpacity context (lines 2775-2790) ==="
sed -n '2775,2790p' frontend/javascripts/viewer/api/api_latest.ts

echo ""
echo "=== Check for opacity validation patterns in file ==="
rg -n 'opacity.*[0-1]|opacity.*range|opacity.*clamp|opacity.*min|opacity.*max' --type=ts frontend/javascripts/viewer/api/api_latest.ts | head -20

Length of output: 1659


[/run_scripts]


🏁 Script executed:

#!/bin/bash
# Find the method that contains updateMeshOpacity at line 2782
echo "=== Method containing updateMeshOpacity (searching backwards from line 2782) ==="
sed -n '2700,2790p' frontend/javascripts/viewer/api/api_latest.ts | grep -n 'setMeshOpacity\|^\s*[a-zA-Z].*(\s*[a-zA-Z].*:\s*' | tail -5

echo ""
echo "=== Full context from line 2760 to 2790 to see method signature ==="
sed -n '2760,2790p' frontend/javascripts/viewer/api/api_latest.ts

Length of output: 1354


Add mesh existence check and opacity validation in setSegmentColor method.

The implementation at line 2782-2785 in setSegmentColor dispatches updateMeshOpacityAction without checking mesh existence or validating the opacity value. This is inconsistent with other mesh methods like setMeshVisibility (line 2579) and removeMesh (line 2607), which both validate mesh existence and throw an error if the mesh is not found.

Add both checks:

  • Validate that meshOpacity is in the valid range (0-1)
  • Check that the mesh exists before dispatching, consistent with the established pattern
    if (meshOpacity != null) {
+     if (meshOpacity < 0 || meshOpacity > 1) {
+       throw new Error(`meshOpacity must be between 0 and 1, but got ${meshOpacity}`);
+     }
+     const state = Store.getState();
+     const additionalCoordinates = state.flycam.additionalCoordinates;
+     const additionalCoordKey = getAdditionalCoordinatesAsString(additionalCoordinates);
+     if (state.localSegmentationData[effectiveLayerName]?.meshes?.[additionalCoordKey]?.[segmentId] != null) {
       Store.dispatch(updateMeshOpacityAction(effectiveLayerName, segmentId, meshOpacity));
+     }
    }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In frontend/javascripts/viewer/api/api_latest.ts around lines 2782 to 2785, the
code dispatches updateMeshOpacityAction without validating meshOpacity or
checking that the mesh exists; update it to first verify meshOpacity is a number
between 0 and 1 (inclusive) and throw an error or reject if out of range, then
check for the mesh's existence for the effectiveLayerName and segmentId (same
pattern as setMeshVisibility/removeMesh) and throw a descriptive error if not
found, and only call Store.dispatch(updateMeshOpacityAction(...)) when both
validations pass.

}
}
/**
Expand Down
2 changes: 2 additions & 0 deletions unreleased_changes/9011.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
### Added
- Added an optional `meshOpacity` parameter to `api.data.setSegmentColor`, allowing callers to set a segment's mesh opacity.