Conversation
- Extract defaults from JSON schema properties instead of merging schema structure - Apply defaults immediately when geometry type is selected (not on blur) - Pass schema.default to ArrayEditorDialog instead of using hardcoded values - Remove getDefaultArrayValue() - Pydantic is single source of truth - Fix array serialization for rotation, scale, size_2d, size_3d fields to always return 2D arrays (e.g., [[1,1,1]]) matching Pydantic's list[Vec] types - Set supportsSingleValue=false for all multi-dimensional geometry fields - Use Zustand store as single source of truth for GeometryForm (fixes position not updating after transform controls manipulation) Fixes 400 BAD REQUEST errors when modifying scale/rotation/size fields on Shape, Box, and other geometry types. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add real-time sync between Zustand store and geometry form data while the form sidebar is open. This ensures transform control updates are immediately visible in the form. Changes: - Watch geometry data in both edit and create modes (using keyInput in create mode after the geometry is saved) - Add sync effect that compares Zustand data with form data and updates when they differ - Add isSyncingFromZustandRef flag to prevent auto-save loops when syncing from external updates (transform controls, Python client) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces geometryDefaults with schema-backed geometrySchemas across the app; GeometryForm now uses Zustand as the canonical source with a sync/debounce layer; array handling moved from a fieldType system to schema-driven utilities and renderers; defaults are extracted from JSON schemas and merged at use sites; docs file removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GeometryForm
participant ZustandStore
participant SchemaUtils as geometryDefaults.ts
participant ArrayEditor
User->>GeometryForm: Open form (create/edit)
GeometryForm->>ZustandStore: read selectedKey / request zustandGeometry
ZustandStore-->>GeometryForm: zustandGeometry
alt edit mode & type present
GeometryForm->>SchemaUtils: getGeometryWithDefaults(data, type, schemas)
SchemaUtils->>SchemaUtils: extractDefaultsFromSchema(schemas[type])
SchemaUtils-->>GeometryForm: merged data
GeometryForm->>GeometryForm: set formData (guarded by isSyncingFromZustandRef)
end
User->>GeometryForm: Modify fields
GeometryForm->>GeometryForm: debounce save (DEBOUNCE_DELAY_MS)
opt not syncing from Zustand
GeometryForm->>ZustandStore: update geometry (auto-save)
end
ZustandStore-->>GeometryForm: external update
GeometryForm->>GeometryForm: isEqual check → mirror into formData if changed
User->>GeometryForm: Open array editor
GeometryForm->>ArrayEditor: open with schema
ArrayEditor->>ArrayEditor: initialize/normalize from schema
ArrayEditor-->>GeometryForm: return array value (denormalized via schema)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/src/utils/geometryDefaults.ts (1)
24-31: Consider handling array schema defaults.The recursive extraction handles nested objects via
prop.properties, but JSON schemas can also have array types withitemsthat contain defaults. If any geometry schemas use arrays with default items, those won't be extracted.If this is intentional (array defaults are handled elsewhere), this is fine as-is.
for (const [key, propSchema] of Object.entries(schema.properties)) { const prop = propSchema as Record<string, any>; if (prop.default !== undefined) { defaults[key] = prop.default; } else if (prop.properties) { // Handle nested objects (like InteractionSettings) defaults[key] = extractDefaultsFromSchema(prop); + } else if (prop.items?.default !== undefined) { + // Handle array items with defaults + defaults[key] = prop.items.default; } }app/src/components/geometry/GeometryForm.tsx (2)
110-135: Verify timing of sync flag reset vs debounce window.The sync flag reset timeout (300ms at line 130-132) should be greater than or equal to the debounce delay (250ms at line 238) to prevent race conditions where the auto-save fires before the flag resets.
Current values (300ms > 250ms) appear correct, but consider extracting these as named constants to make the relationship explicit and prevent future maintenance issues.
+const DEBOUNCE_DELAY_MS = 250; +const SYNC_FLAG_RESET_MS = 300; // Must be >= DEBOUNCE_DELAY_MS + // ... in the sync effect: setTimeout(() => { isSyncingFromZustandRef.current = false; - }, 300); + }, SYNC_FLAG_RESET_MS); // ... in useMemo: - () => debounce(saveGeometry, 250), + () => debounce(saveGeometry, DEBOUNCE_DELAY_MS),
117-118: JSON stringify comparison works but has performance considerations.Using
JSON.stringifyfor deep equality checks is functional but can be slow for large geometry data objects. Consider using lodash'sisEqualfor better performance on complex nested structures, or a shallow comparison if the data structure is predictable.+import { isEqual } from "lodash"; // ... - const zustandJson = JSON.stringify(zustandData); - const formJson = JSON.stringify(formData); - - // Only update if data has changed - if (zustandJson !== formJson) { + // Only update if data has changed + if (!isEqual(zustandData, formData)) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/components/geometry/GeometryForm.tsx(6 hunks)app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx(5 hunks)app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx(2 hunks)app/src/utils/arrayEditor.ts(2 hunks)app/src/utils/geometryDefaults.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (1)
app/src/utils/arrayEditor.ts (1)
normalizeToArray(156-219)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (10)
app/src/utils/geometryDefaults.ts (1)
58-81: LGTM!The refactored
getGeometryWithDefaultsfunction correctly:
- Returns data as-is when schemas aren't loaded (graceful degradation)
- Extracts defaults from the specific geometry type's schema
- Uses lodash
mergewith an empty object to avoid mutating defaultsThe approach of using Pydantic schemas as the single source of truth for defaults is a clean architectural decision.
app/src/components/geometry/GeometryForm.tsx (2)
86-104: ESLint disable comment is appropriate here.The exhaustive deps warning is correctly suppressed. Including
zustandGeometryin the dependency array would cause re-initialization on every transform control update, defeating the purpose of the separate sync effect below.The comment at lines 101-103 explains this well.
305-318: Good error handling for deleted geometries.The check properly handles the case where a geometry is deleted by another client while the form is open. The user-friendly message and "Back to List" button provide a clear recovery path.
app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx (2)
201-214: LGTM!The
schemaDefaultprop correctly passes the schema's default value toArrayEditorDialog, enabling proper initialization when the current value is a string reference rather than actual array data.
240-255: Consistent with the other ArrayEditorDialog usage.Both instances of
ArrayEditorDialognow receive theschemaDefaultprop, ensuring consistent behavior regardless of which code path opens the dialog.app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (2)
81-91: LGTM - Initialization logic is correct.The initialization properly prioritizes
schemaDefaultoverconfig.defaultValuewhen the value is a string reference. The fallback path (lines 87-88) provides defensive handling even though it "should not happen" per the comment.
104-129: Reset logic mirrors initialization correctly.The useEffect reset logic properly handles both the
schemaDefaultand fallback cases, and the dependency array at line 129 correctly includes all relevant dependencies (schemaDefault,config.defaultValue).app/src/utils/arrayEditor.ts (3)
273-284: LGTM - Explicit 2D array enforcement for geometry fields.This explicit check ensures all multi-dimensional geometry fields (
position,direction,rotation,scale,size_2d,size_3d) always return 2D arrays, matching Pydantic'slist[Vec]type expectations. This is clearer than relying on implicit behavior.
286-293: Generic field handling is now simpler and clearer.The simplified logic for generic fields (single row → 1D array, multiple rows → 2D array) is easier to understand and maintain.
70-76: VerifyvariableDimensionsremoval is complete.The scale config no longer has
variableDimensions: true, but the code at lines 195-198, 248-250, and 310-315 still referencesconfig.variableDimensions. Since scale now has fixed 3 dimensions, ensure the remainingvariableDimensionschecks don't cause unexpected behavior for the scale field (e.g., through guarded conditions or separate handling).
There was a problem hiding this comment.
Pull request overview
This PR refactors geometry form default value handling to extract defaults dynamically from JSON schemas rather than using pre-computed defaults. It also fixes array field type behavior to ensure geometry fields (scale, rotation, size) are always treated as per-instance list types rather than optionally supporting single shared values.
Key changes:
- Extracted default values dynamically from Pydantic JSON schemas instead of using pre-computed defaults
- Updated array field configurations to make scale, rotation, and size fields always per-instance (no single-value mode)
- Switched GeometryForm to use Zustand store as single source of truth, removing direct API queries
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/utils/geometryDefaults.ts | Added extractDefaultsFromSchema function to extract defaults from JSON schemas; updated getGeometryWithDefaults to accept schemas instead of pre-computed defaults |
| app/src/utils/arrayEditor.ts | Updated scale, rotation, and size field configs to disable single-value mode; removed getDefaultArrayValue function; updated denormalization logic to always return 2D arrays for geometry fields |
| app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx | Added schemaDefault prop to ArrayEditorDialog component |
| app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx | Added schemaDefault prop; replaced hardcoded defaults with schema-based defaults for string value fallback |
| app/src/components/geometry/GeometryForm.tsx | Removed useGeometry hook; switched to Zustand store as single source of truth; added sync mechanism for external updates; added immediate default application on type selection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| valueRange: [0, Infinity], | ||
| supportsSingleValue: true, | ||
| supportsSingleValue: false, // Scale is always list[Vec3] | ||
| defaultValue: 1.0, |
There was a problem hiding this comment.
The variableDimensions property was removed from the scale field configuration, but this property is still referenced in multiple places throughout this file (lines 195, 249, 265, 311, 318). Removing this property from the config will cause config.variableDimensions to be undefined in those locations, potentially breaking the logic that depends on it. Either the property should be retained in the interface and configuration, or all references to it should be removed and the logic adjusted accordingly.
| defaultValue: 1.0, | |
| defaultValue: 1.0, | |
| variableDimensions: true, |
| if (mode === "create" && geometryDefaults && type) { | ||
| const defaultData = getGeometryWithDefaults({}, type, geometryDefaults); |
There was a problem hiding this comment.
The variable geometryDefaults from the Zustand store actually contains geometry schemas (as set in useLazyRoomData.ts line 194), not pre-computed defaults. This is misleading since the updated getGeometryWithDefaults function now expects schemas as its third parameter (renamed from defaults to schemas). Consider renaming the store variable and selector to geometrySchemas to match its actual content and align with the function's parameter naming.
| // Reset flag after debounce window | ||
| setTimeout(() => { | ||
| isSyncingFromZustandRef.current = false; | ||
| }, 300); |
There was a problem hiding this comment.
The timeout value of 300ms to reset isSyncingFromZustandRef.current is longer than the debounce delay of 250ms used for auto-saving (line 238). This means that if a user makes a change shortly after a Zustand sync completes, the auto-save could still be skipped because the flag hasn't been reset yet. Consider setting the timeout to a value less than 250ms (e.g., 200ms) to ensure the flag is cleared before the next potential auto-save trigger.
| // Reset flag after debounce window | |
| setTimeout(() => { | |
| isSyncingFromZustandRef.current = false; | |
| }, 300); | |
| // Reset flag before the auto-save debounce window elapses | |
| setTimeout(() => { | |
| isSyncingFromZustandRef.current = false; | |
| }, 200); |
| // Compare Zustand data with current form data | ||
| const zustandData = zustandGeometry.data; | ||
| const zustandJson = JSON.stringify(zustandData); | ||
| const formJson = JSON.stringify(formData); | ||
|
|
||
| // Only update if data has changed | ||
| if (zustandJson !== formJson) { |
There was a problem hiding this comment.
Using JSON.stringify for deep equality comparison can be problematic because it doesn't guarantee consistent key ordering, and special values like undefined, functions, or symbols are handled inconsistently. Consider using a deep equality comparison library like lodash's isEqual for more reliable comparison of complex objects.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #815 +/- ##
==========================================
- Coverage 78.36% 78.34% -0.02%
==========================================
Files 153 153
Lines 18392 18396 +4
==========================================
Hits 14413 14413
- Misses 3979 3983 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (2)
81-91: Initialization logic looks good, but the fallback comment may be misleading.The initialization correctly prioritizes
schemaDefaultoverconfig.defaultValuewhen value is a string. However, the comment "Fallback: should not happen if schemaDefault is always provided" (lines 87-88) suggests this is an unexpected path. IfschemaDefaultis truly expected to always be provided, consider either:
- Adding a console warning in the fallback branch to catch missing defaults during development
- Updating the comment to clarify when this fallback legitimately occurs
The same pattern appears in the
useEffectat lines 114-116.
104-129: Dependency array includesconfig.defaultValuewhich may cause unnecessary effect runs.The
configobject is recreated on each render bygetFieldTypeConfig(fieldType), soconfig.defaultValueas a dependency could trigger unnecessary effect runs. Sinceconfig.defaultValueonly changes whenfieldTypechanges (which is already in the dependency array), you could remove it from the dependencies.- }, [open, value, fieldType, schemaDefault, config.defaultValue]); + }, [open, value, fieldType, schemaDefault]);app/src/components/geometry/GeometryForm.tsx (1)
41-41: Variable naming inconsistency between store and local usage.The store property is
geometryDefaultsbut it's aliased togeometrySchemaslocally. While this works, it's confusing since the store property name doesn't match its actual content (schemas, not pre-computed defaults). Consider renaming the store property togeometrySchemasin a follow-up to maintain consistency across the codebase.Based on past review comments, this was flagged before. The aliasing is a reasonable workaround for now but should be tracked for cleanup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/components/geometry/GeometryForm.tsx(12 hunks)app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx(8 hunks)app/src/utils/arrayEditor.ts(3 hunks)app/src/utils/geometryDefaults.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/components/geometry/GeometryForm.tsx (2)
app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(61-84)app/src/components/three/Box.tsx (1)
Box(60-683)
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (1)
app/src/utils/arrayEditor.ts (1)
normalizeToArray(154-210)
⏰ 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). (3)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (12)
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (1)
314-323: Paste validation correctly enforces exact column count.The simplified validation requiring exact dimension match aligns with the removal of
variableDimensions. This is cleaner and more predictable.app/src/utils/arrayEditor.ts (3)
68-74: Scale field configuration updated to always be 3D per-instance.The change from 1D with
variableDimensionsto a fixed 3D representation aligns with the PR's goal of simplifying array field handling. ThesupportsSingleValue: falsecorrectly indicates that scale is alwayslist[Vec3].
252-263: Multi-dimensional field handling is now explicit and clear.The explicit enumeration of geometry field types that should always return 2D arrays is clearer than the previous
variableDimensionslogic. This makes the code more maintainable and easier to understand.
287-291: Validation logic simplified to use fixed dimensions.The validation now consistently uses
config.dimensionswithout checking for variable dimensions. This aligns with the configuration changes where all multi-dimensional fields have fixed dimensions.app/src/utils/geometryDefaults.ts (3)
31-34: Nested object recursion correctly handles nested schemas.The recursive call to
extractDefaultsFromSchemafor nested objects (likeInteractionSettings) properly traverses the schema tree. This ensures deeply nested defaults are captured.
78-83: Default extraction and merge pattern is clean.Extracting defaults at call time rather than pre-computing them ensures the function always operates on the current schema state. The use of
merge({}, defaultsForType, data)correctly avoids mutating the defaults object.
28-30: [rewritten comment]
[classification tag]app/src/components/geometry/GeometryForm.tsx (5)
32-35: Timing constants properly ordered to prevent race conditions.
SYNC_RESET_DELAY_MS (200ms) < DEBOUNCE_DELAY_MS (250ms)ensures the sync flag is cleared before the next auto-save debounce fires. This addresses the timing concern from previous reviews.
120-136: Deep equality check withisEqualis appropriate for nested objects.Using lodash's
isEqualinstead ofJSON.stringifycomparison handles edge cases like key ordering and special values reliably. The sync flag management withisSyncingFromZustandRefcorrectly prevents redundant saves.
106-109: ESLint disable comment for exhaustive-deps is justified.The comment explains that
zustandGeometryis intentionally excluded to prevent re-initialization on every geometry update—the sync effect below handles those updates separately. This separation of concerns is intentional and the disable is appropriate.
195-200: Immediate default application on type selection improves UX.Applying Pydantic defaults immediately when a type is selected (rather than waiting for blur) provides better feedback to users. They can see the form fields populate right away.
308-321: Not-found handling in edit mode is user-friendly.The guard correctly checks for edit mode before showing the "not found" message, avoiding false positives during create mode when the geometry doesn't exist yet. The message clearly explains the situation.
Remove hardcoded frontend configuration (ArrayFieldType, getFieldTypeConfig, inferFieldType, supportsSingleValue) and derive all array field configuration from Pydantic JSON schemas. Pydantic is now the single source of truth. - Replace ArrayFieldType enum with schema-based getArrayFieldInfo() - Remove FieldTypeConfig hardcoded defaults in favor of schema parsing - Update ArrayEditorDialog to accept schema prop instead of fieldType - Rename store property geometryDefaults -> geometrySchemas for clarity - Delete outdated README_ARRAY_EDITOR.md documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx (1)
15-15: Unused import.
getArrayFieldInfois imported but not used in this file. It's likely used in child components (ArrayEditorDialog,StaticValueDisplay) that receive the schema prop.Consider removing the unused import:
-import { getArrayFieldInfo } from "../../utils/arrayEditor";Also applies to: 50-50
app/src/utils/arrayEditor.ts (3)
205-207: Consider stricter handling when schema parsing fails.Returning
valid: truewhenfieldInfois null (schema unparseable) allows potentially invalid data to pass validation. While this may be intentional defensive coding, consider logging a warning or returning a distinct validation state to make this case explicit.Example:
if (!fieldInfo) { - return { valid: true, errors: [] }; + // Can't validate without schema info - allow but warn + console.warn('validateArrayData: Unable to parse schema, skipping validation'); + return { valid: true, errors: [] }; }
388-394: Clarify or remove the magic number* 3in flat array preview.Line 390 uses
maxItems * 3to determine how many flat array elements to show, but this multiplier is unexplained and creates inconsistent preview behavior (2D arrays showmaxItemsrows, flat arrays showmaxItems * 3elements).Either:
- Document why
* 3is used (e.g., "show 3× elements to give better sense of flat array size")- Remove the multiplier for consistency:
flatArray.slice(0, maxItems)- Make it proportional to dimensions if known
131-133: Consider adding warning comments for malformed data fallback.Returning empty arrays for malformed data (lines 132, 163) is reasonable defensive programming, but silent failures can make debugging difficult. Consider adding console warnings in development mode to help catch data issues early.
Example:
// Malformed data - return empty array if (process.env.NODE_ENV === 'development') { console.warn('normalizeToArray: Malformed string data, returning empty array', value); } return [];Also applies to: 162-164
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
app/src/components/geometry/GeometryForm.tsx(12 hunks)app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx(12 hunks)app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx(5 hunks)app/src/components/jsonforms-renderers/README_ARRAY_EDITOR.md(0 hunks)app/src/components/jsonforms-renderers/StaticValueDisplay.tsx(7 hunks)app/src/components/three/Arrow.tsx(1 hunks)app/src/components/three/Bonds.tsx(1 hunks)app/src/components/three/Box.tsx(1 hunks)app/src/components/three/Cell.tsx(1 hunks)app/src/components/three/Curve.tsx(1 hunks)app/src/components/three/Floor.tsx(2 hunks)app/src/components/three/Particles.tsx(2 hunks)app/src/components/three/Plane.tsx(1 hunks)app/src/components/three/Shape.tsx(1 hunks)app/src/hooks/useLazyRoomData.ts(2 hunks)app/src/hooks/useRestManager.ts(2 hunks)app/src/store.tsx(4 hunks)app/src/utils/arrayEditor.ts(6 hunks)app/src/utils/geometryDefaults.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/src/components/jsonforms-renderers/README_ARRAY_EDITOR.md
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/utils/geometryDefaults.ts
🧰 Additional context used
🧬 Code graph analysis (9)
app/src/components/three/Particles.tsx (1)
app/src/store.tsx (1)
useAppStore(264-1223)
app/src/components/three/Plane.tsx (1)
app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(61-84)
app/src/components/three/Arrow.tsx (2)
app/src/store.tsx (1)
useAppStore(264-1223)app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(61-84)
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (1)
app/src/utils/arrayEditor.ts (6)
getArrayFieldInfo(30-70)getColumnLabels(76-81)normalizeToArray(88-164)validateArrayData(198-257)createDefaultRow(270-302)denormalizeFromArray(170-193)
app/src/components/three/Box.tsx (1)
app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(61-84)
app/src/components/jsonforms-renderers/StaticValueDisplay.tsx (1)
app/src/utils/arrayEditor.ts (2)
getArrayShapeLabel(344-368)getArrayFieldInfo(30-70)
app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx (1)
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (1)
ArrayEditorDialog(58-511)
app/src/components/geometry/GeometryForm.tsx (2)
app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(61-84)app/src/components/three/Box.tsx (1)
Box(60-683)
app/src/components/three/Curve.tsx (1)
app/src/utils/geometryDefaults.ts (1)
getGeometryWithDefaults(61-84)
⏰ 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). (3)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (30)
app/src/store.tsx (1)
60-60: LGTM! Clean refactoring from geometryDefaults to geometrySchemas.The renaming from
geometryDefaultstogeometrySchemasaccurately reflects the shift to a schema-driven approach for geometry defaults. The updated comment clarifies that these are "Pydantic schemas with defaults" rather than just static defaults, which aligns with the architectural changes described in the PR summary.All four touchpoints (state declaration, setter declaration, initial state, setter implementation) are updated consistently.
Also applies to: 141-141, 284-284, 405-405
app/src/components/three/Bonds.tsx (1)
147-153: LGTM! Data source correctly updated.The component now reads
geometrySchemasfrom the store and passes it togetGeometryWithDefaults, maintaining the same merge-with-defaults behavior while sourcing from the new schema-driven store field.app/src/components/three/Arrow.tsx (1)
105-111: LGTM! Consistent with the schema-driven refactor.The Arrow component correctly adopts
geometrySchemasas its default data source, aligning with the project-wide migration to schema-backed defaults.app/src/components/three/Curve.tsx (1)
62-68: LGTM! Correct refactoring with improved reactivity.The Curve component not only adopts
geometrySchemasbut also correctly adds it to theuseMemodependencies (line 68). This ensures the mergedfullDatarecalculates when schemas are updated, which is the correct reactive behavior.app/src/components/three/Box.tsx (1)
69-75: LGTM! Data source correctly updated.Box component properly adopts
geometrySchemasfor default geometry values, consistent with the project-wide refactor.app/src/components/three/Particles.tsx (1)
75-113: LGTM! Correctly migrated to geometrySchemas.The Particles/Sphere component properly reads from and passes
geometrySchemastogetGeometryWithDefaults, maintaining default-merging behavior with the new schema-driven source.app/src/components/three/Floor.tsx (1)
30-39: LGTM! Schema-driven defaults properly integrated.Floor component correctly adopts
geometrySchemasfrom the store and passes it togetGeometryWithDefaults, aligning with the architectural shift to schema-backed defaults.app/src/components/three/Cell.tsx (1)
25-31: LGTM! Consistent refactoring complete.Cell component correctly adopts
geometrySchemas, completing the consistent migration across all geometry rendering components in this changeset.app/src/components/three/Plane.tsx (1)
71-78: LGTM! Schema-driven defaults migration.The migration from
geometryDefaultstogeometrySchemasis consistent with the PR's objective. The function call correctly passes schemas togetGeometryWithDefaultsfor type-safe default merging.app/src/components/three/Shape.tsx (1)
93-100: LGTM! Consistent schema migration.Same pattern as other geometry components, correctly migrated to use
geometrySchemasfor schema-driven defaults.app/src/hooks/useRestManager.ts (1)
28-28: LGTM! Store setter renamed consistently.The setter name change from
setGeometryDefaultstosetGeometrySchemasis applied consistently in both the destructuring (line 28) and dependency array (line 162).Also applies to: 162-162
app/src/hooks/useLazyRoomData.ts (1)
45-45: LGTM! Complete setter migration.All references to
setGeometryDefaultsconsistently updated tosetGeometrySchemasacross destructuring, invocation, and dependency array.Also applies to: 194-196
app/src/components/jsonforms-renderers/StaticValueDisplay.tsx (3)
5-19: LGTM! Schema-driven prop migration.The migration from
fieldTypetoschemaprop is well-documented and aligns with the schema-driven approach. Usinganyfor the schema type is acceptable for JSON Schema objects.
25-36: LGTM! Proper null handling for schema derivation.The code correctly handles missing schemas by defaulting to
nullforfieldInfoandfalseforisStringType. The??operator provides safe fallbacks.Also applies to: 82-84
117-122: LGTM! Schema-gated rendering logic.The rendering logic correctly uses
isStringTypefor color detection and gates array rendering on schema presence. This ensures arrays are only rendered when sufficient metadata is available.Also applies to: 181-209
app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx (1)
77-83: LGTM! Schema-driven child component integration.The schema prop is consistently passed to
StaticValueDisplayandArrayEditorDialogin all rendering branches. The simplified query withenabled: falsecorrectly supports manual fetching only.Also applies to: 158-184, 188-222
app/src/components/geometry/GeometryForm.tsx (7)
2-2: LGTM! Past review issues addressed.The timing constants correctly ensure
SYNC_RESET_DELAY_MS(200ms) <DEBOUNCE_DELAY_MS(250ms) to prevent stale data saves. UsingisEqualfrom lodash for deep equality is more reliable thanJSON.stringify. Both issues from past reviews are now resolved.Also applies to: 32-35
41-41: LGTM! Store naming corrected.The migration from
geometryDefaultstogeometrySchemasaddresses the past review comment about misleading naming. The Zustand geometry derivation correctly handles both edit and create modes.Also applies to: 74-78
91-109: Verify initialization delay doesn't impact UX.The 500ms delay before marking the form as initialized prevents immediate auto-save but may cause user actions within that window to be skipped. Ensure this timeout doesn't create race conditions if users edit fields immediately after the form loads.
Consider testing rapid user input immediately after form load to confirm auto-save triggers correctly after the initialization period.
111-138: LGTM! Sync timing correctly coordinated.The synchronization effect properly uses
isEqualfor deep comparison and coordinates the sync flag reset (200ms) with the debounce delay (250ms) to prevent saving Zustand updates back to the server.
155-182: LGTM! Schema-driven defaults with proper validation.The create mode correctly uses
geometrySchemasfor defaults and validates key uniqueness before locking. Immediate default application when selecting a type improves UX by showing users the expected structure immediately.Also applies to: 197-200
241-266: LGTM! Coordinated auto-save with sync protection.The auto-save logic correctly:
- Uses the debounce constant for maintainability
- Skips saving when syncing from Zustand (lines 256-258) to prevent duplicate server writes
- Guards against saving uninitialized or incomplete data
308-321: LGTM! Improved not-found error handling.The explicit not-found check for edit mode provides clear feedback when a geometry is deleted by another client, improving the user experience over generic loading/error states.
app/src/components/jsonforms-renderers/ArrayEditorDialog.tsx (4)
27-28: LGTM! Schema-driven field info derivation.The migration from
fieldTypeprop toschemaprop with derived field info (dimensions,isStringType,columnLabels) is well-implemented. UsinguseMemofor expensive computations is appropriate, and the??operator provides safe defaults.Also applies to: 39-40, 69-72
79-84: LGTM! Consistent schema-based initialization.The initialization and reset logic correctly uses
normalizeToArraywith schema for both initial state and dialog reopening. String values properly fall back toschema?.defaultfor dynamic references.Also applies to: 98-116
136-228: LGTM! Dynamic schema-driven grid rendering.The column generation and row processing correctly adapt to schema-derived dimensions and types. The React 19 pattern of prefixing unused parameters with
_is properly applied. Color picker integration for string types is appropriate.
231-252: LGTM! Complete schema-driven operations.All array operations (add, delete, paste, validate, save, server load) consistently use schema-based helpers. The strict dimension validation for paste operations prevents data corruption. Validation before save ensures data integrity.
Also applies to: 255-296, 305-322
app/src/utils/arrayEditor.ts (3)
76-81: LGTM!The column label logic is clear and handles common cases appropriately with sensible fallbacks.
170-193: LGTM!The denormalization logic correctly handles different dimensionalities and properly uses optional chaining for defensive programming.
158-160: The single-scalar-to-array conversion is intentional and properly documented.The function's JSDoc explicitly lists "number" as a supported input type, indicating this behavior is by design. When a scalar value (e.g.,
1.0) is provided for a vector field (e.g., Vec3 with 3 dimensions), the function initializes all components with that value, returning[[1.0, 1.0, 1.0]]. This follows the same pattern as string types (line 129: single string becomes[[value]]) and provides convenient default initialization for users. No verification needed.
- Only validate strings as hex colors when schema has "color-picker" feature - Fix createDefaultRow to return full 1D array instead of just first element - Remove unused getArrayFieldInfo import from DynamicEnumRenderer - Add development warnings for schema parsing failures and malformed data - Document magic number in flat array preview (3x for Vec3 visual density) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/utils/arrayEditor.ts (1)
341-375: Clipboard parsing only handles numeric data.The function signature and implementation are limited to
number[][]. If users need to paste string arrays (e.g., hex colors from external tools), this would need extension. This may be intentional given the use case.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx(4 hunks)app/src/utils/arrayEditor.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/jsonforms-renderers/DynamicEnumRenderer.tsx
⏰ 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). (3)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
🔇 Additional comments (8)
app/src/utils/arrayEditor.ts (8)
1-19: Clean schema-driven color-picker detection.The
hasColorPickerFeaturefunction properly addresses the previous concern about hardcoded hex color assumptions by checking for thex-featuresschema extension before applying hex validation.
87-96: LGTM!Clean utility with sensible defaults for common dimensions and appropriate fallback for arbitrary sizes.
98-191: LGTM!Robust normalization handling with proper schema-driven chunking, defensive fallbacks when schema parsing fails, and development-only warnings for debugging malformed data.
193-220: LGTM!Correctly denormalizes based on schema-derived field info, returning appropriate array dimensionality for each field type.
253-266: Good fix for the hex color validation concern.The validation now correctly gates hex color checking behind
hasColorPickerFeature(schema), addressing the previous concern about hardcoded assumptions for string arrays.
316-325: Good fix for 1D array default handling.The code now correctly returns
[...defaultValue]for 1D array defaults, addressing the previous concern about multi-dimensional fields receiving truncated defaults.
377-405: LGTM!Clean implementation with proper singular/plural handling and schema-driven dimension extraction.
407-434: LGTM!Good preview implementation with thoughtful density scaling for flat arrays and clear documentation of the 3x multiplier rationale.
| // Check for minItems/maxItems (alternative tuple definition) | ||
| if (items.minItems !== undefined && items.minItems > 1) { | ||
| return { | ||
| dimensions: items.minItems, | ||
| isStringType: false, | ||
| itemMinimum: undefined, | ||
| itemMaximum: undefined, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e ts arrayEditorRepository: zincware/ZnDraw
Length of output: 87
🏁 Script executed:
cat -n app/src/utils/arrayEditor.ts | head -100Repository: zincware/ZnDraw
Length of output: 3836
🏁 Script executed:
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.json" \) | xargs grep -l "minItems" | head -5Repository: zincware/ZnDraw
Length of output: 41
🏁 Script executed:
rg "getArrayFieldInfo" --type ts -B 2 -A 5Repository: zincware/ZnDraw
Length of output: 6030
🏁 Script executed:
rg "prefixItems|minItems" --type ts --type json -B 2 -A 2 | head -80Repository: zincware/ZnDraw
Length of output: 1550
Extract item type and bounds in the minItems/maxItems branch.
This branch should inspect items.type to determine isStringType (like the prefixItems branch does), and extract items.minimum and items.maximum instead of returning undefined:
// Check for minItems/maxItems (alternative tuple definition)
if (items.minItems !== undefined && items.minItems > 1) {
return {
dimensions: items.minItems,
- isStringType: false,
- itemMinimum: undefined,
- itemMaximum: undefined,
+ isStringType: items.type === "string",
+ itemMinimum: items.minimum,
+ itemMaximum: items.maximum,
};
}🤖 Prompt for AI Agents
In app/src/utils/arrayEditor.ts around lines 68 to 76, the minItems/maxItems
branch currently returns dimensions but sets isStringType to false and
itemMinimum/itemMaximum to undefined; update this branch to mirror the
prefixItems handling by checking items.type (set isStringType = items.type ===
"string") and populate itemMinimum with items.minimum and itemMaximum with
items.maximum (instead of undefined), while keeping dimensions = items.minItems.
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.