Conversation
…slider - Add isovalue_min/isovalue_max fields to Isosurface model with model validator, replacing hardcoded ge/le bounds. Users can now set custom ranges for SDF data (values 1-30+) instead of being capped at [-0.25, 0.25]. - Add sigma field for server-side Gaussian smoothing before marching cubes. - New EditableRangeSlider frontend component: slider with click-to-edit min, max, and value labels driven by x-custom-type: "editable-range". - New HiddenRenderer to suppress x-hidden fields from the form. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add le=5.0 upper bound to sigma query parameter (prevent DoS via huge sigma) - Add scipy as explicit dependency (was only transitive via scikit-image) - Validate commitMin/commitMax against current bounds in EditableRangeSlider - Remove hardcoded fallback defaults (-0.25/0.25) from frontend component - Fix aria-label to use actual label instead of non-existent element ID - Reorder editableRangeSliderTester with other Priority 10 renderers - Fix misleading test docstring for hidden fields test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughIntroduces Gaussian smoothing support for isosurface geometry with a new sigma parameter. Adds EditableRangeSlider and HiddenRenderer JSON Forms components for range value editing and field visibility control. Implements isovalue range validation through new isovalue_min/isovalue_max fields. Backend applies Gaussian filtering on request and includes scipy dependency. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Frontend as Frontend (Isosurface)
participant API as Backend API
participant Filter as Gaussian Filter
participant Mesh as Mesh Extraction
User->>Frontend: Set sigma > 0
Frontend->>API: fetchIsosurface(..., sigma)
API->>Filter: gaussian_filter(grid, sigma)
Filter->>API: smoothed_grid
API->>Mesh: Extract mesh from smoothed_grid
Mesh->>API: IsosurfaceMesh (reduced fragmentation)
API->>Frontend: Mesh result
Frontend->>User: Render smoothed isosurface
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
+ Coverage 91.42% 91.48% +0.05%
==========================================
Files 180 180
Lines 17396 17466 +70
==========================================
+ Hits 15905 15978 +73
+ Misses 1491 1488 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_isosurface.py (1)
560-609: Please add one provider-materialization regression test for storage misses.The new tests cover stored-frame and smoothing paths well, but they still miss the
storage.get(...) is None+ provider-dispatch path for this endpoint. Adding that case would protect the known regression-prone flow.Based on learnings, "when isosurface data is unavailable in storage (storage.get returns None), follow the same provider-dispatch/materialization flow used by get_frame() instead of treating the miss as a hard 404."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_isosurface.py` around lines 560 - 609, Add a regression test that exercises the storage miss + provider materialization path for the isosurface endpoint: simulate storage.get(...) returning None for the frame cube key and assert the server dispatches to the provider/materializer (same flow as get_frame()) and returns a 200 with valid mesh after materialization; locate the test scaffolding in tests/test_isosurface.py near test_isosurface_sigma_smoothing and reuse the iso_client, iso_session, iso_storage fixtures, creating a frame where iso_storage.get will return None (or clear the stored frame) and then call GET /v1/rooms/{room.id}/frames/0/isosurface with appropriate params (cube_key, isovalue) and auth_header(token), finally unpack the msgpack response and assert non-empty vertices to confirm materialization succeeded.frontend/src/components/jsonforms-renderers/EditableRangeSlider.tsx (2)
127-127: SlideronChangereceivesnumber | number[]— explicitly narrow to avoid type issues.The MUI
Slidercomponent'sonChangehandler provides avalueparameter typed asnumber | number[]. Since this is a single-value slider,newValuewill always be anumber, but passing it directly tohandleChangemay cause TypeScript strictness issues or unexpected behavior if the slider were ever converted to a range slider.Proposed fix
<Slider value={value} - onChange={(_event, newValue) => handleChange(path, newValue)} + onChange={(_event, newValue) => + handleChange(path, Array.isArray(newValue) ? newValue[0] : newValue) + } min={min} max={max}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/jsonforms-renderers/EditableRangeSlider.tsx` at line 127, The Slider onChange handler passes a value typed number | number[]; update the handler in EditableRangeSlider.tsx so the second arg is narrowed to a single number before calling handleChange (e.g., check Array.isArray(newValue) or typeof newValue === 'number' and extract the first element when it's an array), and then call handleChange(path, narrowedNumber) to avoid TypeScript strictness and future range/union issues.
36-42: Consider defensive handling for missing schema annotations.If
x-min-fieldorx-max-fieldare missing from the schema,minFieldandmaxFieldwill beundefined. Subsequent calls likehandleChange(minField, parsed)on line 54 would then operate on an undefined path, androotData[undefined]would returnundefined(converted to0byNumber()).While the tester ensures
x-custom-type === "editable-range", it doesn't validate sibling fields exist. Consider adding validation or early return if required fields are missing.Proposed defensive check
const ext = schema as Record<string, unknown>; const minField = ext["x-min-field"] as string; const maxField = ext["x-max-field"] as string; + +if (!minField || !maxField) { + console.warn("EditableRangeSlider requires x-min-field and x-max-field"); + return null; +} + const step = (ext.step as number) || 0.001;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/jsonforms-renderers/EditableRangeSlider.tsx` around lines 36 - 42, The component currently assumes ext["x-min-field"] and ext["x-max-field"] exist (stored in minField/maxField) which can produce undefined paths and incorrect behavior; update the EditableRangeSlider to defensively validate those annotations early (check ext["x-min-field"] and ext["x-max-field"] and ensure they are non-empty strings) and if missing either return early (e.g., render null or a fallback) or disable interactions; ensure subsequent logic in value calculation and calls to handleChange(minField, ...) and handleChange(maxField, ...) only runs when minField/maxField are valid, and use rootData lookup fallbacks only after confirming the field names exist to avoid indexing with undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zndraw/routes/isosurface.py`:
- Around line 100-103: The route parameter sigma currently hardcodes 0.0; change
it to read the default from the Pydantic Isosurface model so the model remains
the single source of truth. Import Isosurface and set the Query default to the
model's sigma default (e.g. use Isosurface.__fields__['sigma'].default or
Isosurface.model_fields['sigma'].default depending on your Pydantic version)
when constructing the Annotated Query for the sigma parameter so the endpoint
default always matches the Isosurface model.
---
Nitpick comments:
In `@frontend/src/components/jsonforms-renderers/EditableRangeSlider.tsx`:
- Line 127: The Slider onChange handler passes a value typed number | number[];
update the handler in EditableRangeSlider.tsx so the second arg is narrowed to a
single number before calling handleChange (e.g., check Array.isArray(newValue)
or typeof newValue === 'number' and extract the first element when it's an
array), and then call handleChange(path, narrowedNumber) to avoid TypeScript
strictness and future range/union issues.
- Around line 36-42: The component currently assumes ext["x-min-field"] and
ext["x-max-field"] exist (stored in minField/maxField) which can produce
undefined paths and incorrect behavior; update the EditableRangeSlider to
defensively validate those annotations early (check ext["x-min-field"] and
ext["x-max-field"] and ensure they are non-empty strings) and if missing either
return early (e.g., render null or a fallback) or disable interactions; ensure
subsequent logic in value calculation and calls to handleChange(minField, ...)
and handleChange(maxField, ...) only runs when minField/maxField are valid, and
use rootData lookup fallbacks only after confirming the field names exist to
avoid indexing with undefined.
In `@tests/test_isosurface.py`:
- Around line 560-609: Add a regression test that exercises the storage miss +
provider materialization path for the isosurface endpoint: simulate
storage.get(...) returning None for the frame cube key and assert the server
dispatches to the provider/materializer (same flow as get_frame()) and returns a
200 with valid mesh after materialization; locate the test scaffolding in
tests/test_isosurface.py near test_isosurface_sigma_smoothing and reuse the
iso_client, iso_session, iso_storage fixtures, creating a frame where
iso_storage.get will return None (or clear the stored frame) and then call GET
/v1/rooms/{room.id}/frames/0/isosurface with appropriate params (cube_key,
isovalue) and auth_header(token), finally unpack the msgpack response and assert
non-empty vertices to confirm materialization succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4dfe3c2-064e-4110-b476-88fbb35b8491
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
frontend/src/components/jsonforms-renderers/EditableRangeSlider.tsxfrontend/src/components/jsonforms-renderers/HiddenRenderer.tsxfrontend/src/components/three/Isosurface.tsxfrontend/src/myapi/client.tsfrontend/src/utils/jsonforms.tspyproject.tomlsrc/zndraw/geometries/isosurface.pysrc/zndraw/routes/isosurface.pytests/test_isosurface.py
| sigma: Annotated[ | ||
| float, | ||
| Query(ge=0.0, le=5.0, description="Gaussian smoothing sigma (0=disabled)"), | ||
| ] = 0.0, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Derive the sigma endpoint default from the geometry model.
Line 103 hardcodes 0.0, which duplicates the Isosurface model default and can drift over time. Please source this default from the model definition instead of duplicating it here.
Proposed change
+from zndraw.geometries.isosurface import Isosurface
+
+DEFAULT_ISOSURFACE_SIGMA = Isosurface().sigma
+
async def get_isosurface(
@@
sigma: Annotated[
float,
Query(ge=0.0, le=5.0, description="Gaussian smoothing sigma (0=disabled)"),
- ] = 0.0,
+ ] = DEFAULT_ISOSURFACE_SIGMA,
) -> Response:As per coding guidelines, "All default values must be defined in the Pydantic model as the single source of truth".
📝 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.
| sigma: Annotated[ | |
| float, | |
| Query(ge=0.0, le=5.0, description="Gaussian smoothing sigma (0=disabled)"), | |
| ] = 0.0, | |
| from zndraw.geometries.isosurface import Isosurface | |
| DEFAULT_ISOSURFACE_SIGMA = Isosurface().sigma | |
| async def get_isosurface( | |
| sigma: Annotated[ | |
| float, | |
| Query(ge=0.0, le=5.0, description="Gaussian smoothing sigma (0=disabled)"), | |
| ] = DEFAULT_ISOSURFACE_SIGMA, | |
| ) -> Response: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zndraw/routes/isosurface.py` around lines 100 - 103, The route parameter
sigma currently hardcodes 0.0; change it to read the default from the Pydantic
Isosurface model so the model remains the single source of truth. Import
Isosurface and set the Query default to the model's sigma default (e.g. use
Isosurface.__fields__['sigma'].default or
Isosurface.model_fields['sigma'].default depending on your Pydantic version)
when constructing the Annotated Query for the sigma parameter so the endpoint
default always matches the Isosurface model.
2 participants
Summary by CodeRabbit
New Features
Tests