SmarForm'd Annotation Sidebar #6608
Conversation
WalkthroughThis PR adds a SmartForm system and a SchemaIO→RJSF translation pipeline, plus integration, docs, and tests. New/changed items include: SmartForm wrapper, RJSF renderer with custom widgets and templates, translation modules (utils/schema/ui) producing JSON Schema and UiSchema, a SchemaIOComponent wrapper that delegates to SmartForm when Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SchemaIOComp as SchemaIOComponent (wrapper)
participant SmartForm as SmartForm
participant Trans as Translators
participant RJSF as RJSF Form
participant Widgets as Custom Widgets
App->>SchemaIOComp: render(schema, data, smartForm?, jsonSchema?, uiSchema?)
alt smartForm or jsonSchema provided
SchemaIOComp->>SmartForm: delegate props
alt SchemaIO schema provided
SmartForm->>Trans: translateSchema(schemaIO)
Trans->>Trans: translateToJSONSchema() & translateToUISchema()
Trans-->>SmartForm: { schema, uiSchema, warnings }
else jsonSchema/uiSchema provided
SmartForm->>RJSF: use provided jsonSchema + uiSchema
end
SmartForm->>RJSF: render(formSchema, uiSchema, formData)
RJSF->>Widgets: render widgets (AutoComplete, Dropdown, TextWidget)
User->>Widgets: interact
Widgets->>RJSF: widget onChange
RJSF->>SmartForm: onChange(formData)
SmartForm->>SchemaIOComp: onChange(formData)
SchemaIOComp->>App: propagate onChange
else
SchemaIOComp->>SchemaIOComp: render legacy SchemaIO renderer
end
sequenceDiagram
participant Py as Python Backend
participant Annotation as AnnotationSchema
participant Position as Position component
participant SchemaIOComp as SchemaIOComponent (wrapper)
participant SmartForm as SmartForm/RJSF
Py->>Py: emit field schema with ftype (+ multipleOf for Float)
Py-->>Annotation: annotation schema
Annotation->>SchemaIOComp: render(schema, smartForm=true)
Position->>SchemaIOComp: render(positionSchema)
SchemaIOComp->>SmartForm: delegate to SmartForm (translate if SchemaIO)
SmartForm->>RJSF: render translated form
User->>RJSF: edit values
RJSF->>SchemaIOComp: onChange -> upstream handlers
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing careful review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 25
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
app/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (27)
app/packages/components/src/components/SmartForm/README.md(1 hunks)app/packages/components/src/components/SmartForm/USAGE_EXAMPLES.md(1 hunks)app/packages/components/src/components/SmartForm/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/templates/FieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/templates/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/translators/data.test.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/data.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/index.test.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/index.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/schema.test.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/schema.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/ui.test.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/ui.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/utils.test.ts(1 hunks)app/packages/components/src/components/SmartForm/translators/utils.ts(1 hunks)app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx(1 hunks)app/packages/components/src/components/SmartForm/widgets/Dropdown.tsx(1 hunks)app/packages/components/src/components/SmartForm/widgets/TextWidget.tsx(1 hunks)app/packages/components/src/components/SmartForm/widgets/index.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx(5 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/USAGE_EXAMPLES.md(1 hunks)app/packages/core/src/plugins/SchemaIO/__tests__/SchemaIOComponent.integration.test.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/index.tsx(1 hunks)fiftyone/core/annotation.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/SmartForm/widgets/index.tsxapp/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsxapp/packages/components/src/components/SmartForm/translators/utils.test.tsapp/packages/components/src/components/SmartForm/templates/FieldTemplate.tsxapp/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsxapp/packages/core/src/plugins/SchemaIO/__tests__/SchemaIOComponent.integration.test.tsxapp/packages/components/src/components/SmartForm/translators/schema.test.tsapp/packages/components/src/components/SmartForm/translators/schema.tsapp/packages/components/src/components/SmartForm/widgets/Dropdown.tsxapp/packages/components/src/components/SmartForm/templates/index.tsxapp/packages/components/src/components/SmartForm/translators/ui.test.tsapp/packages/components/src/components/SmartForm/translators/data.test.tsapp/packages/components/src/components/SmartForm/translators/index.tsapp/packages/components/src/components/SmartForm/translators/data.tsapp/packages/components/src/components/SmartForm/index.tsxapp/packages/components/src/components/SmartForm/widgets/AutoComplete.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsxapp/packages/components/src/components/SmartForm/widgets/TextWidget.tsxapp/packages/components/src/components/SmartForm/translators/index.test.tsapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsxapp/packages/components/src/components/SmartForm/translators/utils.tsapp/packages/core/src/plugins/SchemaIO/index.tsxapp/packages/components/src/components/SmartForm/translators/ui.ts
🧠 Learnings (4)
📚 Learning: 2024-11-06T20:52:30.520Z
Learnt from: benjaminpkane
Repo: voxel51/fiftyone PR: 5051
File: app/packages/utilities/src/index.ts:12-12
Timestamp: 2024-11-06T20:52:30.520Z
Learning: In `app/packages/utilities/src/index.ts`, when an export statement like `export * from "./Resource";` is moved within the file, it is not a duplication even if it appears in both the removed and added lines of a diff.
Applied to files:
app/packages/components/src/components/SmartForm/widgets/index.tsxapp/packages/components/src/components/SmartForm/translators/utils.test.ts
📚 Learning: 2025-07-22T15:19:26.438Z
Learnt from: imanjra
Repo: voxel51/fiftyone PR: 6164
File: app/packages/core/src/plugins/SchemaIO/index.tsx:23-34
Timestamp: 2025-07-22T15:19:26.438Z
Learning: In the FiftyOne codebase, specifically in app/packages/core/src/plugins/SchemaIO/index.tsx, the store object accessed via props.otherProps.store is always defined and initialized to an empty object, so defensive checks for store existence are not needed.
Applied to files:
app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsxapp/packages/core/src/plugins/SchemaIO/__tests__/SchemaIOComponent.integration.test.tsxapp/packages/core/src/plugins/SchemaIO/USAGE_EXAMPLES.mdapp/packages/core/src/plugins/SchemaIO/index.tsx
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/components/src/components/SmartForm/index.tsx
📚 Learning: 2025-10-02T21:53:53.778Z
Learnt from: tom-vx51
Repo: voxel51/fiftyone PR: 6372
File: app/packages/core/src/client/annotationClient.ts:32-46
Timestamp: 2025-10-02T21:53:53.778Z
Learning: In `app/packages/core/src/client/annotationClient.ts`, the `delta` field in `PatchSampleRequest` intentionally does not accept simple scalar values (strings, numbers, booleans). The `AttributeField` type is correctly defined as `Record<string, unknown>` to enforce that all delta entries must be structured objects, not primitives.
Applied to files:
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx
🧬 Code graph analysis (14)
app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx (4)
app/packages/core/src/plugins/SchemaIO/index.tsx (1)
SchemaIOComponent(32-38)app/packages/core/src/plugins/SchemaIO/utils/index.ts (2)
coerceValue(159-176)getLiteValue(182-197)app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (1)
DynamicIO(17-58)app/packages/plugins/src/index.ts (1)
registerComponent(29-36)
app/packages/core/src/plugins/SchemaIO/__tests__/SchemaIOComponent.integration.test.tsx (1)
app/packages/core/src/plugins/SchemaIO/index.tsx (1)
SchemaIOComponentProps(9-30)
app/packages/components/src/components/SmartForm/translators/schema.ts (2)
app/packages/components/src/components/SmartForm/translators/utils.ts (2)
TranslationContext(15-19)addWarning(21-27)app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)
app/packages/components/src/components/SmartForm/translators/data.test.ts (1)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)
app/packages/components/src/components/SmartForm/translators/index.ts (5)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/utils.ts (3)
TranslationOptions(11-13)TranslationResult(4-9)TranslationContext(15-19)app/packages/components/src/components/SmartForm/translators/schema.ts (2)
translateToJSONSchema(11-131)addChoicesToSchema(136-203)app/packages/components/src/components/SmartForm/translators/ui.ts (1)
translateToUISchema(10-220)app/packages/components/src/components/SmartForm/translators/data.ts (1)
convertSchemaIODataToRJSF(10-79)
app/packages/components/src/components/SmartForm/translators/data.ts (2)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/utils.ts (1)
getEmptyValueForType(29-47)
app/packages/components/src/components/SmartForm/index.tsx (3)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/index.ts (2)
translateSchemaComplete(46-59)convertRJSFDataToSchemaIO(21-21)app/packages/components/src/components/SmartForm/translators/data.ts (1)
convertRJSFDataToSchemaIO(87-163)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (4)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (2)
SchemaType(23-27)NumberSchemaType(17-21)app/packages/core/src/components/Modal/Sidebar/Annotate/state.ts (2)
schema(78-94)AnnotationSchema(21-27)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (2)
currentField(81-94)currentData(64-77)app/packages/state/src/recoil/schema.ts (1)
expandPath(469-490)
app/packages/components/src/components/SmartForm/translators/index.test.ts (3)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/index.ts (4)
translateSchema(26-41)translateSchemaComplete(46-59)isSchemaIOSchema(15-15)isJSONSchema(16-16)app/packages/components/src/components/SmartForm/translators/utils.ts (2)
isSchemaIOSchema(54-61)isJSONSchema(66-73)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (4)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (2)
currentOverlay(96-98)currentData(64-77)app/packages/lighter/src/overlay/BoundingBoxOverlay.ts (1)
BoundingBoxOverlay(71-812)app/packages/core/src/plugins/SchemaIO/index.tsx (1)
SchemaIOComponent(32-38)app/packages/lighter/src/commands/TransformOverlayCommand.ts (1)
TransformOverlayCommand(22-43)
app/packages/components/src/components/SmartForm/translators/utils.ts (1)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)
app/packages/core/src/plugins/SchemaIO/index.tsx (4)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/index.tsx (4)
SmartFormProps(14-14)SmartFormProps(20-27)isJSONSchema(13-13)SmartForm(29-77)app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx (1)
SchemaIOComponent(8-82)app/packages/components/src/components/SmartForm/translators/utils.ts (1)
isJSONSchema(66-73)
app/packages/components/src/components/SmartForm/translators/ui.ts (1)
app/packages/components/src/components/SmartForm/translators/utils.ts (2)
TranslationContext(15-19)addWarning(21-27)
fiftyone/core/annotation.py (1)
fiftyone/core/fields.py (3)
schema(691-693)schema(696-697)FloatField(958-1000)
🪛 LanguageTool
app/packages/components/src/components/SmartForm/README.md
[grammar] ~169-~169: Use a hyphen to join words.
Context: ...ropdownView|Dropdown| Single/multi select dropdown | |RadioView|radio...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
app/packages/components/src/components/SmartForm/USAGE_EXAMPLES.md
116-116: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
117-117: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
143-143: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
144-144: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
152-152: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
153-153: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
303-303: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
352-352: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
353-353: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
359-359: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
360-360: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
377-377: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
389-389: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
app/packages/components/src/components/SmartForm/README.md
5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
68-68: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
176-176: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
198-198: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
224-224: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
225-225: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
236-236: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
237-237: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
247-247: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
248-248: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
258-258: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
259-259: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
274-274: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
275-275: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
290-290: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
291-291: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
307-307: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
308-308: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
325-325: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
326-326: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
337-337: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
338-338: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
350-350: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
351-351: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
362-362: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
363-363: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
377-377: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
378-378: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
396-396: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
397-397: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
434-434: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
435-435: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 Pylint (4.0.3)
fiftyone/core/annotation.py
[convention] 156-156: Line too long (115/79)
(C0301)
⏰ 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). (13)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (20)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (4)
12-35: LGTM!The helper functions cleanly generate schema fragments for inputs and horizontal stacks.
53-64: LGTM!Initial bounds are correctly loaded from the overlay.
66-96: LGTM!Event listener correctly syncs overlay changes to both local state and the atom. The handler ignores event parameters and directly queries the overlay, ensuring consistency.
127-143: Circular update flow exists but debouncing impact is inconclusive—verify actual keystroke behavior.The concern is valid. The circular update flow is confirmed:
- User edits input → SchemaIOComponent.onIOChange fires immediately
- Position.onChange executes TransformOverlayCommand (synchronous)
- Overlay event fires → Position event handler calls setState()
- data prop updates → SchemaIOComponent syncs stateRef via useEffect
- DynamicIO re-renders with new data
No debouncing is implemented in SchemaIOComponent, DynamicIO, Position event handlers, or the onChange callback chain. TransformOverlayCommand.execute() is synchronous (directly calls overlay.setBounds), so there's no async buffer.
However, SchemaIOComponent buffers state internally via stateRef, which may mitigate actual mid-keystroke interruption depending on how the underlying view components (input fields) handle controlled updates.
To confirm if this causes UX issues, verify:
- Actual keystroke behavior in the form inputs during overlay manipulation
- Whether users experience mid-keystroke interruption or character loss
- If the stateRef buffering is sufficient or if debouncing/batching updates would improve responsiveness
app/packages/components/src/components/SmartForm/templates/index.tsx (1)
1-7: LGTM!Clean re-export pattern for aggregating template components.
app/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsx (1)
1-69: LGTM!Well-structured template with proper defensive checks, sensible defaults, and clear layout logic. The vertical-by-default approach with optional horizontal layout aligns with the SchemaIO style mentioned in the comments.
app/packages/components/src/components/SmartForm/widgets/index.tsx (1)
1-9: LGTM!Clean re-export pattern for aggregating widget components.
Based on learnings.
app/packages/core/src/plugins/SchemaIO/USAGE_EXAMPLES.md (1)
1-523: LGTM!Comprehensive documentation with clear examples covering various SchemaIO usage patterns, field types, and complex schemas. The RJSF mode integration examples are particularly helpful.
app/packages/components/src/components/SmartForm/translators/schema.test.ts (1)
1-306: LGTM!Comprehensive test coverage for schema translation logic. Well-organized with clear test cases covering primitive types, complex structures, edge cases, and error conditions. The recursive processing tests are particularly important for nested schemas.
app/packages/components/src/components/SmartForm/translators/utils.test.ts (1)
1-169: Utility behavior is well specified by testsThis suite clearly pins down the contracts for
addWarning,getEmptyValueForType,isSchemaIOSchema, andisJSONSchema(including strict mode and a variety of negative cases). The expectations look consistent and give good safety against regressions.app/packages/components/src/components/SmartForm/translators/index.test.ts (1)
10-366: Translator integration tests are thorough and well targetedThese tests hit the important surfaces of the translator layer: object/array/nested structures, required fields, min/max constraints, choices→enum mapping, widget/ui options, strict mode warnings, and end‑to‑end data propagation via
translateSchemaComplete. The type‑guard integration at the end closes the loop on conditional paths.No issues from a correctness standpoint; this is a solid regression net for the translators.
Also applies to: 368-402
app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx (1)
8-14: Type the SchemaIOComponent props using SchemaIOComponentPropsThe suggestion is correct.
SchemaIOComponentPropsis already defined inindex.tsxand fully covers the props destructured and passed through inSchemaIOComponent.tsx. Use a type-only import to avoid circular dependencies:import type { SchemaIOComponentProps } from "./index"; export function SchemaIOComponent(props: SchemaIOComponentProps) { const { onChange, onPathChange, id, shouldClearUseKeyStores, data } = props; // ... }This restores type safety on the public plugin surface without creating runtime cycles.
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (3)
122-129: Usage ofcreateInputwith full attribute object is acceptablePassing the full
attributes[attr]object intocreateInputis fine given the helper only readsftype(andmultipleOf), and it future‑proofs the call ifcreateInputever needs other fields. OncemultipleOfis made optional, this call site will also line up with the types cleanly.
154-176: Numeric coercion inuseHandleChangeslooks soundThe Recoil callback correctly:
- Expands the current field path and looks up the field schema.
- Coerces string values to
number/intonly whenschema.ftypematchesFLOAT_FIELDorINT_FIELD.- Treats empty strings and non‑finite parses as
null, which is a reasonable “cleared/invalid” sentinel for annotation data.This should safely normalize numeric inputs before they hit the overlay/update pipeline.
182-208: JSON Schema integration intoSchemaIOComponentlooks correctUsing
useJSONSchema={true}here and mappingchangesthroughhandleChangesfor every key before merging intodatais consistent with the new SmartForm/JSON‑schema flow:
- All keys in
changesgo through type normalization.- You avoid unnecessary eventBus dispatches via the
isEqual(value, overlay.label)guard.- The component will now always drive the JSON‑schema path for annotation editing, which matches the PR’s intent.
No issues from a React/Recoil integration standpoint.
app/packages/components/src/components/SmartForm/translators/ui.ts (1)
53-60: Scopeui:submitButtonOptionsto root only—Autocomplete defaults are correctOne actionable issue to fix:
ui:submitButtonOptionsapplied on every nested schemaLines 213-217 unconditionally set
ui:submitButtonOptionson every call, including nested schemas. RJSF only reads this at the form root, making it redundant on nested objects and arrays. Use the existingcontext.pathtracking to scope it to the root:// Hide submit button by default on the root schema only if (context.path.length === 0) { uiSchema["ui:submitButtonOptions"] = { norender: true, }; }Autocomplete defaults are intentionally true: The concern about loosened behavior is incorrect. SchemaIO's AutocompleteView.tsx (lines 16–17) uses
!== falsefor its defaults (meaning true when undefined), matching your?? truepattern. The test suite confirms this is the intended behavior.⛔ Skipped due to learnings
Learnt from: minhtuev Repo: voxel51/fiftyone PR: 4957 File: app/packages/components/src/components/Toast/Toast.tsx:29-36 Timestamp: 2024-10-22T16:25:08.017Z Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.app/packages/components/src/components/SmartForm/translators/index.ts (1)
26-59: Translator composition and public surface look solid
translateSchema+translateSchemaCompletecorrectly share a singlewarningsarray, build aTranslationContextwithstrictMode, and layeraddChoicesToSchemaand optionalconvertSchemaIODataToRJSFon top, while re-exporting lower-level utilities from a single entry point. No functional or typing issues stand out.app/packages/components/src/components/SmartForm/translators/data.ts (1)
10-78: Data conversion between SchemaIO and RJSF is consistent and well-scoped
convertSchemaIODataToRJSFandconvertRJSFDataToSchemaIOcorrectly handle null/undefined, objects with/withoutproperties, tuple vs homogeneous arrays, andoneOfpassthrough, with an opt‑incoerceEmptyflag for empty strings/arrays. The implementations match the documented intent and integrate cleanly with the higher‑level translators.Also applies to: 87-163
app/packages/components/src/components/SmartForm/index.tsx (1)
54-62: Review comment is incorrect; no changes neededThe current handler signature is correct. RJSF's onSubmit handler receives IChangeEvent (the same shape as onChange), with an optional React.FormEvent as the second parameter. The code at lines 54–62 aligns with RJSF's documented pattern. ISubmitEvent is not the standard export, and using IChangeEvent is the correct approach here.
Likely an incorrect or invalid review comment.
app/packages/components/src/components/SmartForm/translators/utils.ts (1)
29-47:getEmptyValueForTypebehavior looks consistentThe mapping from JSON Schema types to empty values is coherent, and returning
undefinedfor numeric types avoids silently pre‑filling0where “no value yet” is desired. No changes needed here from a correctness or safety standpoint.
| export default function SmartForm(props: SmartFormProps) { | ||
| const { | ||
| schema: jsonSchema, | ||
| uiSchema: generatedUiSchema, | ||
| warnings, | ||
| formData, | ||
| } = translateSchemaComplete(props.schema, props.data); | ||
|
|
||
| const mergedUiSchema = { ...generatedUiSchema, ...props.uiSchema }; | ||
|
|
||
| if (warnings.length > 0) { | ||
| console.warn("[SmartForm] Schema translation warnings:", warnings); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid re-translating schema and re-logging warnings on every render
translateSchemaComplete plus warning logging runs on every render, which is unnecessary if schema/data are stable and can spam logs when warnings are non‑empty.
Wrap the translation and warning/reporting in useMemo keyed on schema, data, and (optionally) translation options, and log warnings once per translation rather than per render.
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/index.tsx around lines 29 to
41, the call to translateSchemaComplete and the console.warn run on every
render; memoize the translation and derived values using React.useMemo keyed on
props.schema and props.data (and any translation options if present) so
translation only runs when inputs change, and move the warning/logging into a
React.useEffect that depends on the memoized warnings so warnings are logged
once per translation rather than on every render.
| # SmartForm | ||
|
|
||
| A React JSON Schema Form (RJSF) wrapper that automatically translates SchemaIO schemas to JSON Schema format. | ||
|
|
||
| ## Features | ||
|
|
||
| ✅ **Automatic Translation** - Converts SchemaIO schemas to JSON Schema + UI Schema | ||
| ✅ **Bidirectional Data Conversion** - Handles SchemaIO ↔ RJSF data formats | ||
| ✅ **Custom Widgets** - Reuses existing SchemaIO components (Dropdown, AutoComplete) | ||
| ✅ **Type Safe** - Full TypeScript support with exported types | ||
| ✅ **Material-UI Styled** - Built on @rjsf/mui for consistent design | ||
| ✅ **Extensible** - Override generated UI schema for fine control | ||
|
|
||
| --- | ||
|
|
||
| ## Quick Start | ||
|
|
||
| ```tsx | ||
| import SmartForm from "@fiftyone/components/SmartForm"; | ||
|
|
||
| const schema = { | ||
| type: "object", | ||
| view: { component: "ObjectView" }, | ||
| properties: { | ||
| name: { | ||
| type: "string", | ||
| view: { label: "Name", placeholder: "Enter name" }, | ||
| }, | ||
| email: { | ||
| type: "string", | ||
| view: { label: "Email" }, | ||
| required: true, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| <SmartForm | ||
| schema={schema} | ||
| data={{ name: "John Doe" }} | ||
| onChange={(data) => console.log(data)} | ||
| /> | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Installation | ||
|
|
||
| SmartForm is part of the `@fiftyone/components` package: | ||
|
|
||
| ```bash | ||
| yarn add @fiftyone/components | ||
| ``` | ||
|
|
||
| **Dependencies:** | ||
| - `@rjsf/core` - React JSON Schema Form core | ||
| - `@rjsf/mui` - Material-UI theme | ||
| - `@rjsf/validator-ajv8` - JSON Schema validation | ||
| - `@rjsf/utils` - RJSF utilities | ||
|
|
||
| --- | ||
|
|
||
| ## Basic Example | ||
|
|
||
| ```tsx | ||
| import SmartForm, { type SmartFormProps } from "@fiftyone/components/SmartForm"; | ||
|
|
||
| function MyForm() { | ||
| const schema = { | ||
| type: "object", | ||
| properties: { | ||
| category: { | ||
| type: "string", | ||
| view: { | ||
| component: "DropdownView", | ||
| label: "Category", | ||
| choices: [ | ||
| { value: "cat", label: "Cat" }, | ||
| { value: "dog", label: "Dog" }, | ||
| ], | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| return ( | ||
| <SmartForm | ||
| schema={schema} | ||
| onChange={(data) => console.log("Changed:", data)} | ||
| onSubmit={(data) => console.log("Submitted:", data)} | ||
| /> | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Props | ||
|
|
||
| ```tsx | ||
| interface SmartFormProps { | ||
| schema: SchemaType; // SchemaIO schema (required) | ||
| data?: unknown; // Initial form data | ||
| uiSchema?: UiSchema; // Override generated UI schema | ||
| validator?: ValidatorType; // Custom JSON Schema validator | ||
| onChange?: (data: unknown) => void; // Change handler | ||
| onSubmit?: (data: unknown) => void; // Submit handler | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## How It Works | ||
|
|
||
| SmartForm performs three main operations: | ||
|
|
||
| ### 1. Schema Translation | ||
| ```tsx | ||
| // Input: SchemaIO format | ||
| { | ||
| type: "string", | ||
| view: { | ||
| component: "DropdownView", | ||
| choices: [ | ||
| { value: "a", label: "Option A" }, | ||
| { value: "b", label: "Option B" } | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| // Output: JSON Schema + UI Schema | ||
| { | ||
| schema: { | ||
| type: "string", | ||
| enum: ["a", "b"], | ||
| enumNames: ["Option A", "Option B"] | ||
| }, | ||
| uiSchema: { | ||
| "ui:widget": "Dropdown" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### 2. Data Conversion (Input) | ||
| ```tsx | ||
| // SchemaIO format: null for empty | ||
| { name: "John", age: null } | ||
|
|
||
| // Converted to RJSF format: appropriate empty values | ||
| { name: "John", age: undefined } | ||
| ``` | ||
|
|
||
| ### 3. Data Conversion (Output) | ||
| ```tsx | ||
| // RJSF format from onChange | ||
| { name: "John", age: undefined } | ||
|
|
||
| // Converted back to SchemaIO format | ||
| { name: "John", age: null } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Supported Field Types | ||
|
|
||
| | SchemaIO Component | RJSF Widget | Description | | ||
| |-------------------|-------------|-------------| | ||
| | `FieldView` | `TextWidget` | Text input | | ||
| | `CheckboxView` | `checkbox` | Checkbox | | ||
| | `DropdownView` | `Dropdown` | Single/multi select dropdown | | ||
| | `RadioView` | `radio` | Radio buttons | | ||
| | `AutocompleteView` | `AutoComplete` | Searchable autocomplete | | ||
| | `ColorView` | `color` | Color picker | | ||
| | `CodeView` | `textarea` | Code/text area | | ||
| | `FileView` | `file` | File upload | | ||
| | `ObjectView` | (fieldset) | Nested object | | ||
| | `ListView` | (array) | Dynamic list | | ||
| | `TupleView` | (array) | Fixed tuple | | ||
|
|
||
| --- | ||
|
|
||
| ## Advanced Usage | ||
|
|
||
| ### Custom UI Schema | ||
|
|
||
| ```tsx | ||
| const uiSchema = { | ||
| name: { | ||
| "ui:placeholder": "John Doe", | ||
| "ui:help": "Your display name", | ||
| }, | ||
| email: { | ||
| "ui:widget": "email", | ||
| }, | ||
| }; | ||
|
|
||
| <SmartForm | ||
| schema={schema} | ||
| uiSchema={uiSchema} // Overrides generated UI schema | ||
| /> | ||
| ``` | ||
|
|
||
| ### Validation | ||
|
|
||
| ```tsx | ||
| import validator from "@rjsf/validator-ajv8"; | ||
|
|
||
| <SmartForm | ||
| schema={schema} | ||
| validator={validator} // Custom validator | ||
| /> | ||
| ``` | ||
|
|
||
| ### Type Safety | ||
|
|
||
| ```tsx | ||
| import type { SmartFormProps } from "@fiftyone/components/SmartForm"; | ||
|
|
||
| const props: SmartFormProps = { | ||
| schema: mySchema, | ||
| data: initialData, | ||
| onChange: (data) => { | ||
| // data is typed as unknown | ||
| // Validate or assert type as needed | ||
| }, | ||
| }; | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Translation API | ||
|
|
||
| SmartForm exports translation functions for advanced use cases: | ||
|
|
||
| ```tsx | ||
| import { | ||
| translateSchemaComplete, | ||
| translateSchema, | ||
| convertSchemaIODataToRJSF, | ||
| convertRJSFDataToSchemaIO, | ||
| isSchemaIOSchema, | ||
| isJSONSchema, | ||
| } from "@fiftyone/components/SmartForm/translators"; | ||
|
|
||
| // Full translation with data | ||
| const result = translateSchemaComplete(schema, data); | ||
| console.log(result.schema); // JSON Schema | ||
| console.log(result.uiSchema); // UI Schema | ||
| console.log(result.formData); // Converted data | ||
| console.log(result.warnings); // Translation warnings | ||
|
|
||
| // Schema only | ||
| const { schema, uiSchema, warnings } = translateSchema(schemaIO); | ||
|
|
||
| // Manual data conversion | ||
| const rjsfData = convertSchemaIODataToRJSF(schemaIOData, schema); | ||
| const schemaIOData = convertRJSFDataToSchemaIO(rjsfData, schema); | ||
|
|
||
| // Type guards | ||
| if (isSchemaIOSchema(someSchema)) { | ||
| // someSchema is SchemaType | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Custom Widgets | ||
|
|
||
| SmartForm includes custom widgets that wrap SchemaIO components: | ||
|
|
||
| ### AutoComplete Widget | ||
|
|
||
| ```tsx | ||
| // Automatically used for AutocompleteView | ||
| { | ||
| type: "array", | ||
| view: { | ||
| component: "AutocompleteView", | ||
| choices: [...], | ||
| allow_user_input: true, | ||
| allow_duplicates: false, | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Dropdown Widget | ||
|
|
||
| ```tsx | ||
| // Automatically used for DropdownView | ||
| { | ||
| type: "string", | ||
| view: { | ||
| component: "DropdownView", | ||
| choices: [...], | ||
| multiple: true, | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Architecture | ||
|
|
||
| ``` | ||
| SmartForm/ | ||
| ├── index.tsx # Main component | ||
| ├── translators/ # Schema translation | ||
| │ ├── index.ts # Main API | ||
| │ ├── schema.ts # JSON Schema translation | ||
| │ ├── ui.ts # UI Schema translation | ||
| │ ├── data.ts # Data conversion | ||
| │ └── utils.ts # Shared utilities | ||
| ├── widgets/ # Custom RJSF widgets | ||
| │ ├── AutoComplete.tsx | ||
| │ ├── Dropdown.tsx | ||
| │ └── TextWidget.tsx | ||
| ├── templates/ # Custom RJSF templates | ||
| │ ├── FieldTemplate.tsx | ||
| │ └── ObjectFieldTemplate.tsx | ||
| └── __tests__/ # Comprehensive tests | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Testing | ||
|
|
||
| SmartForm has comprehensive test coverage: | ||
|
|
||
| ```bash | ||
| # Run all tests | ||
| yarn test SmartForm | ||
|
|
||
| # Run specific test suites | ||
| yarn test translators | ||
| yarn test schema.test | ||
| yarn test ui.test | ||
| yarn test data.test | ||
| ``` | ||
|
|
||
| **Test Coverage:** | ||
| - ✅ 170+ test cases | ||
| - ✅ All field types | ||
| - ✅ Data conversion (bidirectional) | ||
| - ✅ Edge cases | ||
| - ✅ Integration tests | ||
|
|
||
| --- | ||
|
|
||
| ## Integration with SchemaIOComponent | ||
|
|
||
| SmartForm can be used directly or through `SchemaIOComponent`: | ||
|
|
||
| ### Direct Usage | ||
| ```tsx | ||
| import SmartForm from "@fiftyone/components/SmartForm"; | ||
|
|
||
| <SmartForm schema={schema} data={data} /> | ||
| ``` | ||
|
|
||
| ### Through SchemaIOComponent (Auto-detection) | ||
| ```tsx | ||
| import { SchemaIOComponent } from "@fiftyone/core/plugins/SchemaIO"; | ||
|
|
||
| // Automatically uses SmartForm for JSON schemas | ||
| <SchemaIOComponent schema={jsonSchema} /> | ||
|
|
||
| // Force SmartForm for SchemaIO schemas | ||
| <SchemaIOComponent schema={schemaIOSchema} useJSONSchema={true} /> | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Migration Guide | ||
|
|
||
| ### From SchemaIO DynamicIO | ||
|
|
||
| **Before:** | ||
| ```tsx | ||
| import { SchemaIOComponent } from "@fiftyone/core/plugins/SchemaIO"; | ||
|
|
||
| <SchemaIOComponent | ||
| schema={schema} | ||
| data={data} | ||
| onChange={(data, liteValues) => handleChange(data)} | ||
| onPathChange={(path, value) => handlePathChange(path, value)} | ||
| /> | ||
| ``` | ||
|
|
||
| **After:** | ||
| ```tsx | ||
| import SmartForm from "@fiftyone/components/SmartForm"; | ||
|
|
||
| <SmartForm | ||
| schema={schema} | ||
| data={data} | ||
| onChange={(data) => handleChange(data)} | ||
| // Note: onPathChange not supported | ||
| // Use onChange with full data instead | ||
| /> | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Limitations | ||
|
|
||
| 1. **Custom Components** - Some SchemaIO components don't have RJSF equivalents: | ||
| - `PlotlyView` | ||
| - `DashboardView` | ||
| - `FileExplorerView` | ||
| - `MarkdownView` | ||
| - `ButtonView` | ||
| - `LinkView` | ||
| - `NoticeView` | ||
| - `MenuView` | ||
| - `LazyFieldView` | ||
| - `ProgressView` | ||
|
|
||
| **Workaround:** Use `SchemaIOComponent` without `useJSONSchema` flag. | ||
|
|
||
| 2. **Path-based Updates** - RJSF doesn't support path-based change handlers like SchemaIO's `onPathChange`. Use `onChange` with full data instead. | ||
|
|
||
| 3. **Lite Values** - RJSF doesn't have an equivalent to SchemaIO's "lite values" concept. | ||
|
|
||
| --- | ||
|
|
||
| ## Performance | ||
|
|
||
| SmartForm is optimized for: | ||
| - ✅ Fast schema translation (cached) | ||
| - ✅ Minimal re-renders | ||
| - ✅ Efficient data conversion | ||
| - ✅ Large forms (100+ fields) | ||
|
|
||
| --- | ||
|
|
||
| ## Browser Support | ||
|
|
||
| Same as RJSF: | ||
| - Chrome (latest) | ||
| - Firefox (latest) | ||
| - Safari (latest) | ||
| - Edge (latest) | ||
|
|
||
| --- | ||
|
|
||
| ## Contributing | ||
|
|
||
| When adding new features: | ||
|
|
||
| 1. Update translators if adding new field types | ||
| 2. Add tests for new functionality | ||
| 3. Update this documentation | ||
| 4. Ensure TypeScript types are correct | ||
|
|
||
| --- | ||
|
|
||
| ## Resources | ||
|
|
||
| - [Full Usage Examples](./USAGE_EXAMPLES.md) | ||
| - [Translator Tests](./translators/__tests__/) | ||
| - [RJSF Documentation](https://rjsf-team.github.io/react-jsonschema-form/) | ||
| - [JSON Schema Specification](https://json-schema.org/) | ||
|
|
||
| --- | ||
|
|
||
| ## License | ||
|
|
||
| Apache-2.0 | ||
|
|
||
| --- | ||
|
|
||
| ## Support | ||
|
|
||
| For issues or questions: | ||
| - GitHub: [voxel51/fiftyone](https://github.com/voxel51/fiftyone) | ||
| - Documentation: [docs.voxel51.com](https://docs.voxel51.com) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider addressing markdownlint and minor grammar nits
The README content is solid. markdownlint and LanguageTool only report minor issues:
- Several headings and fenced code blocks lack the recommended blank lines (MD022/MD031).
- The phrase “Single/multi select dropdown” should be hyphenated (“single‑/multi‑select dropdown”) for correctness.
These are non‑blocking, but fixing them will keep docs lint‑clean and polished.
🧰 Tools
🪛 LanguageTool
[grammar] ~169-~169: Use a hyphen to join words.
Context: ...ropdownView|Dropdown| Single/multi select dropdown | |RadioView|radio...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
68-68: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
176-176: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
198-198: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
224-224: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
225-225: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
236-236: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
237-237: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
247-247: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
248-248: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
258-258: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
259-259: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
274-274: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
275-275: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
290-290: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
291-291: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
307-307: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
308-308: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
325-325: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
326-326: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
337-337: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
338-338: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
350-350: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
351-351: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
362-362: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
363-363: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
377-377: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
378-378: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
396-396: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
397-397: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
434-434: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
435-435: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/README.md (lines 1-475), fix
markdownlint and minor grammar nits by ensuring there is a blank line before and
after each heading and before/after fenced code blocks (address MD022/MD031)
throughout the file, and update the phrase "Single/multi select dropdown" to a
hyphenated form "single-/multi-select dropdown" (or similar consistent
hyphenation) where it appears; scan the document and apply these small
formatting and wording changes to make the README lint-clean.
app/packages/components/src/components/SmartForm/RJSF/templates/FieldTemplate.tsx
Show resolved
Hide resolved
| describe("convertSchemaIODataToRJSF", () => { | ||
| describe("null and undefined handling", () => { | ||
| it("should convert null to empty string for string type", () => { | ||
| const schema: SchemaType = { type: "string", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(null, schema)).toBe(""); | ||
| }); | ||
|
|
||
| it("should convert undefined to empty string for string type", () => { | ||
| const schema: SchemaType = { type: "string", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(undefined, schema)).toBe(""); | ||
| }); | ||
|
|
||
| it("should convert null to undefined for number type", () => { | ||
| const schema: SchemaType = { type: "number", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(null, schema)).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("should convert null to false for boolean type", () => { | ||
| const schema: SchemaType = { type: "boolean", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(null, schema)).toBe(false); | ||
| }); | ||
|
|
||
| it("should convert null to empty object for object type", () => { | ||
| const schema: SchemaType = { type: "object", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(null, schema)).toEqual({}); | ||
| }); | ||
|
|
||
| it("should convert null to empty array for array type", () => { | ||
| const schema: SchemaType = { type: "array", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(null, schema)).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("primitive types", () => { | ||
| it("should pass through string values", () => { | ||
| const schema: SchemaType = { type: "string", view: {} }; | ||
| expect(convertSchemaIODataToRJSF("test", schema)).toBe("test"); | ||
| }); | ||
|
|
||
| it("should pass through number values", () => { | ||
| const schema: SchemaType = { type: "number", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(42, schema)).toBe(42); | ||
| }); | ||
|
|
||
| it("should pass through boolean values", () => { | ||
| const schema: SchemaType = { type: "boolean", view: {} }; | ||
| expect(convertSchemaIODataToRJSF(true, schema)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("object types", () => { | ||
| it("should convert object with properties", () => { | ||
| const schema: SchemaType = { | ||
| type: "object", | ||
| view: {}, | ||
| properties: { | ||
| name: { type: "string", view: {} }, | ||
| age: { type: "number", view: {} }, | ||
| }, | ||
| }; | ||
| const data = { name: "John", age: 30 }; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual({ name: "John", age: 30 }); | ||
| }); | ||
|
|
||
| it("should convert nested objects", () => { | ||
| const schema: SchemaType = { | ||
| type: "object", | ||
| view: {}, | ||
| properties: { | ||
| person: { | ||
| type: "object", | ||
| view: {}, | ||
| properties: { | ||
| name: { type: "string", view: {} }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
| const data = { person: { name: "John" } }; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual({ person: { name: "John" } }); | ||
| }); | ||
|
|
||
| it("should handle null property values", () => { | ||
| const schema: SchemaType = { | ||
| type: "object", | ||
| view: {}, | ||
| properties: { | ||
| name: { type: "string", view: {} }, | ||
| age: { type: "number", view: {} }, | ||
| }, | ||
| }; | ||
| const data = { name: "John", age: null }; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual({ name: "John", age: undefined }); | ||
| }); | ||
|
|
||
| it("should return empty object for array data", () => { | ||
| const schema: SchemaType = { type: "object", view: {} }; | ||
| const result = convertSchemaIODataToRJSF([], schema); | ||
|
|
||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| it("should handle objects without properties schema", () => { | ||
| const schema: SchemaType = { type: "object", view: {} }; | ||
| const data = { name: "John", age: 30 }; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual({ name: "John", age: 30 }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("array types", () => { | ||
| it("should convert array with single item type", () => { | ||
| const schema: SchemaType = { | ||
| type: "array", | ||
| view: {}, | ||
| items: { type: "string", view: {} }, | ||
| }; | ||
| const data = ["a", "b", "c"]; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual(["a", "b", "c"]); | ||
| }); | ||
|
|
||
| it("should convert tuple array", () => { | ||
| const schema: SchemaType = { | ||
| type: "array", | ||
| view: {}, | ||
| items: [ | ||
| { type: "string", view: {} }, | ||
| { type: "number", view: {} }, | ||
| ], | ||
| }; | ||
| const data = ["test", 42]; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual(["test", 42]); | ||
| }); | ||
|
|
||
| it("should convert array of objects", () => { | ||
| const schema: SchemaType = { | ||
| type: "array", | ||
| view: {}, | ||
| items: { | ||
| type: "object", | ||
| view: {}, | ||
| properties: { | ||
| name: { type: "string", view: {} }, | ||
| }, | ||
| }, | ||
| }; | ||
| const data = [{ name: "John" }, { name: "Jane" }]; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual([{ name: "John" }, { name: "Jane" }]); | ||
| }); | ||
|
|
||
| it("should return empty array for non-array data", () => { | ||
| const schema: SchemaType = { type: "array", view: {} }; | ||
| const result = convertSchemaIODataToRJSF("not an array", schema); | ||
|
|
||
| expect(result).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("oneOf types", () => { | ||
| it("should pass through data for oneOf", () => { | ||
| const schema: SchemaType = { type: "oneOf", view: {} }; | ||
| const data = { value: "test" }; | ||
|
|
||
| const result = convertSchemaIODataToRJSF(data, schema); | ||
|
|
||
| expect(result).toEqual({ value: "test" }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good bidirectional coverage; consider adding reverse oneOf cases
The expectations for primitives, objects, arrays, and null/undefined handling on both convertSchemaIODataToRJSF and convertRJSFDataToSchemaIO look solid and aligned with the translator responsibilities. The only noticeable gap is that oneOf is exercised only in the SchemaIO→RJSF direction.
Consider adding a small convertRJSFDataToSchemaIO test for type: "oneOf" to lock in the expected pass‑through/null behavior in the reverse direction as well.
Also applies to: 198-395
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/translators/data.test.ts
around lines 8-195 (also apply same change to 198-395), add a new unit test for
convertRJSFDataToSchemaIO that covers schema with type: "oneOf": create a schema
const { type: "oneOf", view: {} } and assert that convertRJSFDataToSchemaIO
passes through a non-null object unchanged and returns undefined (or the
expected null-to-undefined mapping used elsewhere) when given null; keep
assertions consistent with existing translator expectations for
pass-through/null handling.
| case "number": | ||
| case "integer": | ||
| schema.type = "number"; | ||
| if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; | ||
| if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; | ||
| if (schemaIO.multipleOf !== undefined) | ||
| schema.multipleOf = schemaIO.multipleOf; | ||
| break; |
There was a problem hiding this comment.
Preserve integer semantics in JSON Schema mapping
integer is currently mapped to schema.type = "number", which relaxes validation and allows non‑integers even when SchemaIO declares an integer.
Map integer to JSON Schema "integer" while still applying numeric constraints:
- case "number":
- case "integer":
- schema.type = "number";
- if (schemaIO.min !== undefined) schema.minimum = schemaIO.min;
- if (schemaIO.max !== undefined) schema.maximum = schemaIO.max;
- if (schemaIO.multipleOf !== undefined)
- schema.multipleOf = schemaIO.multipleOf;
- break;
+ case "number":
+ schema.type = "number";
+ if (schemaIO.min !== undefined) schema.minimum = schemaIO.min;
+ if (schemaIO.max !== undefined) schema.maximum = schemaIO.max;
+ if (schemaIO.multipleOf !== undefined)
+ schema.multipleOf = schemaIO.multipleOf;
+ break;
+ case "integer":
+ schema.type = "integer";
+ if (schemaIO.min !== undefined) schema.minimum = schemaIO.min;
+ if (schemaIO.max !== undefined) schema.maximum = schemaIO.max;
+ if (schemaIO.multipleOf !== undefined)
+ schema.multipleOf = schemaIO.multipleOf;
+ break;📝 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.
| case "number": | |
| case "integer": | |
| schema.type = "number"; | |
| if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; | |
| if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; | |
| if (schemaIO.multipleOf !== undefined) | |
| schema.multipleOf = schemaIO.multipleOf; | |
| break; | |
| case "number": | |
| schema.type = "number"; | |
| if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; | |
| if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; | |
| if (schemaIO.multipleOf !== undefined) | |
| schema.multipleOf = schemaIO.multipleOf; | |
| break; | |
| case "integer": | |
| schema.type = "integer"; | |
| if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; | |
| if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; | |
| if (schemaIO.multipleOf !== undefined) | |
| schema.multipleOf = schemaIO.multipleOf; | |
| break; |
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/translators/schema.ts around
lines 23 to 30, the switch currently maps both "number" and "integer" to
schema.type = "number", which loses integer validation; update the logic to set
schema.type = "number" for the "number" case and schema.type = "integer" for the
"integer" case, while keeping the same numeric constraints
(minimum/maximum/multipleOf) applied when schemaIO provides min, max, or
multipleOf so integer semantics are preserved in the generated JSON Schema.
| export interface SchemaIOComponentProps { | ||
| schema: SchemaType | RJSFSchema; | ||
| data?: unknown; | ||
| id?: string; | ||
| useJSONSchema?: boolean; | ||
| onChange?: (data: unknown, liteValues?: Record<string, unknown>) => void; | ||
|
|
||
| // SchemaIO only | ||
| shouldClearUseKeyStores?: boolean; | ||
| onPathChange?: ( | ||
| path: string, | ||
| value: unknown, | ||
| schema?: SchemaType, | ||
| updatedState?: unknown, | ||
| liteValue?: unknown | ||
| ) => void; | ||
|
|
||
| // RJSF only | ||
| onSubmit?: (data: unknown) => void; | ||
| uiSchema?: SmartFormProps["uiSchema"]; | ||
| validator?: SmartFormProps["validator"]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify mode-specific props and onChange contract
You’re merging two modes behind a single SchemaIOComponentProps, but the contract is asymmetric:
shouldClearUseKeyStoresandonPathChangeare only honored by the legacySchemaIObranch.onSubmit,uiSchema, andvalidatorare only honored by the SmartForm/RJSF branch.onChange’sliteValuesargument is only produced by the SchemaIO branch; SmartForm will never provide it.
This is easy to misuse from the call site (e.g., assuming liteValues is always available once you flip useJSONSchema on).
Consider tightening the typing to reflect the two modes explicitly, e.g. a discriminated union keyed on useJSONSchema or on the schema shape, and document that liteValues is undefined in SmartForm mode:
type SchemaIOProps = {
schema: SchemaType;
useJSONSchema?: false;
onChange?: (data: unknown, liteValues?: Record<string, unknown>) => void;
onPathChange?: (/* ... */) => void;
shouldClearUseKeyStores?: boolean;
};
type JSONSchemaProps = {
schema: RJSFSchema;
useJSONSchema?: true;
onChange?: (data: unknown) => void; // no liteValues
onSubmit?: (data: unknown) => void;
uiSchema?: SmartFormProps["uiSchema"];
validator?: SmartFormProps["validator"];
};
export type SchemaIOComponentProps = (SchemaIOProps | JSONSchemaProps) & {
data?: unknown;
id?: string;
};This makes it clear at compile time which callbacks/props are valid in each mode.
🤖 Prompt for AI Agents
In app/packages/core/src/plugins/SchemaIO/index.tsx around lines 9 to 29, the
current SchemaIOComponentProps mixes props for two distinct modes (legacy
SchemaIO and RJSF/SmartForm) causing unsafe assumptions about callbacks like
onChange(liteValues). Replace the single interface with a discriminated union
keyed on useJSONSchema (or on schema shape): one branch for legacy SchemaIO that
includes schema: SchemaType, onChange(data, liteValues?), onPathChange,
shouldClearUseKeyStores; and one branch for JSONSchema/RJSF that includes
schema: RJSFSchema, useJSONSchema: true, onChange(data) without liteValues,
onSubmit, uiSchema, validator; keep data and id common. Update exported type
accordingly and adjust call sites and tests to satisfy the stricter signatures
and document that liteValues is undefined in JSONSchema mode.
| const onIOChange = useCallback( | ||
| (path, value, schema, ancestors) => { | ||
| const computedValue = coerceValue(path, value, schema); | ||
| const currentState = stateRef.current; | ||
| const updatedState = cloneDeep(currentState); | ||
| set(updatedState, path, cloneDeep(computedValue)); | ||
| stateRef.current = updatedState; | ||
|
|
||
| const liteValue = getLiteValue(value, schema); | ||
| const store = storeRef.current; | ||
| if (liteValue) { | ||
| store.liteValues[path] = liteValue; | ||
| } | ||
|
|
||
| if (onPathChange) { | ||
| onPathChange(path, computedValue, schema, updatedState, liteValue); | ||
| } | ||
|
|
||
| if (onChange) { | ||
| onChange(updatedState, store.liteValues); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix stale and skipped liteValues entries in onIOChange
liteValues can get out of sync with the main state:
- When a value transitions from non‑null to null/undefined,
getLiteValuereturnsundefined, theif (liteValue)guard is false, and the previousliteValues[path]entry is left behind. - If
getLiteValueever validly returns a falsy value (e.g.,0or""for future lite schemas), it will be silently dropped.
Both lead to onChange(updatedState, store.liteValues) exposing stale or incomplete lite data.
You can make this robust and future‑proof with:
- const liteValue = getLiteValue(value, schema);
+ const liteValue = getLiteValue(computedValue, schema);
const store = storeRef.current;
- if (liteValue) {
- store.liteValues[path] = liteValue;
- }
+ if (liteValue !== undefined) {
+ store.liteValues[path] = liteValue;
+ } else {
+ delete store.liteValues[path];
+ }This keeps liteValues aligned with the coerced value and avoids leaking or dropping entries based on truthiness.
I'll verify the review comment by examining the codebase structure and the specific concerns raised.
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate and verify the file exists
fd "SchemaIOComponent.tsx" --type f
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Extract and verify the onIOChange callback at lines 29-49
sed -n '29,49p' app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsxLength of output: 689
🏁 Script executed:
#!/bin/bash
# Search for getLiteValue definition
rg -n "getLiteValue\s*=" --type ts --type tsx -A 5Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for coerceValue definition
rg -n "coerceValue\s*=" --type ts --type tsx -A 5Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check all usages of liteValues in the file
rg -n "liteValues" app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx -C 2Length of output: 385
🏁 Script executed:
#!/bin/bash
# Search for references to getLiteValue to understand how it's used elsewhere
rg -n "getLiteValue" --type ts --type tsx -B 2 -A 2Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for getLiteValue definition (without type filter)
rg -n "getLiteValue\s*=" -A 8
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for coerceValue definition (without type filter)
rg -n "coerceValue\s*=" -A 8Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for import statements in the file to understand dependencies
head -30 app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsxLength of output: 927
🏁 Script executed:
#!/bin/bash
# Search for all getLiteValue calls to understand usage patterns
rg -n "getLiteValue" -B 3 -A 3Length of output: 2055
🏁 Script executed:
#!/bin/bash
# Check if other parts of codebase delete from liteValues
rg -n "delete.*liteValues\|liteValues.*=\s*null\|liteValues.*=\s*undefined" -C 2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full getLiteValue implementation
sed -n '182,220p' app/packages/core/src/plugins/SchemaIO/utils/index.ts
Length of output: 410
🏁 Script executed:
#!/bin/bash
# Get the full coerceValue implementation
rg -n "export function coerceValue|const coerceValue" -A 25Length of output: 2075
🏁 Script executed:
#!/bin/bash
# Check the isLiteSchema function
sed -n '177,180p' app/packages/core/src/plugins/SchemaIO/utils/index.tsLength of output: 91
🏁 Script executed:
#!/bin/bash
# Look at the entire getLiteValue function and context around it
sed -n '177,210p' app/packages/core/src/plugins/SchemaIO/utils/index.tsLength of output: 503
Update onIOChange to keep liteValues synchronized with coerced state
The review correctly identifies a data consistency issue. getLiteValue returns undefined in two scenarios: (1) when schema is not lite, and (2) when value is nullish. With the current if (liteValue) guard, stale entries persist when values transition to null, and any falsy return values from getLiteValue are silently dropped.
Changes required:
- Pass
computedValue(the actual stored state) togetLiteValueinstead of the inputvalue - Check
!== undefinedto distinguish between intentional falsy values and absence - Delete stale entries explicitly when the result is
undefined
Apply the suggested diff at lines 37–41 in app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx:
- const liteValue = getLiteValue(value, schema);
+ const liteValue = getLiteValue(computedValue, schema);
const store = storeRef.current;
- if (liteValue) {
- store.liteValues[path] = liteValue;
- }
+ if (liteValue !== undefined) {
+ store.liteValues[path] = liteValue;
+ } else {
+ delete store.liteValues[path];
+ }🤖 Prompt for AI Agents
In app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx around lines 29
to 49, onIOChange currently calls getLiteValue with the original input value and
only assigns when the result is truthy, causing stale entries and dropped falsy
lite values; change it to call getLiteValue(computedValue, schema), then test
the result with !== undefined and if !== undefined set store.liteValues[path] =
result, otherwise delete store.liteValues[path] to remove stale entries; ensure
this happens before invoking onChange/onPathChange so liteValues remain
synchronized with the coerced updatedState.
| ), | ||
| ): | ||
| return {"type": "input", "default": None} | ||
| schema = {"type": "input", "default": None, "ftype": field.__class__.__name__.replace("Field", "").lower()} |
There was a problem hiding this comment.
LGTM with minor line length issue.
The ftype extraction logic is correct. However, the line exceeds the recommended length (115 characters vs 79).
Apply this diff to fix the line length:
- schema = {"type": "input", "default": None, "ftype": field.__class__.__name__.replace("Field", "").lower()}
+ ftype = field.__class__.__name__.replace("Field", "").lower()
+ schema = {"type": "input", "default": None, "ftype": ftype}📝 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.
| schema = {"type": "input", "default": None, "ftype": field.__class__.__name__.replace("Field", "").lower()} | |
| ftype = field.__class__.__name__.replace("Field", "").lower() | |
| schema = {"type": "input", "default": None, "ftype": ftype} |
🧰 Tools
🪛 Pylint (4.0.3)
[convention] 156-156: Line too long (115/79)
(C0301)
🤖 Prompt for AI Agents
In fiftyone/core/annotation.py around line 156, the schema definition line is
too long; split the ftype extraction into its own variable before building the
schema so each line stays under the 79-character limit (e.g., compute ftype =
field.__class__.__name__.replace("Field", "").lower() on its own line, then set
schema = {"type": "input", "default": None, "ftype": ftype}). Ensure formatting
and indentation match surrounding code.
| if isinstance(field, fof.FloatField): | ||
| schema["multipleOf"] = 0.01 | ||
| return schema |
There was a problem hiding this comment.
🧩 Analysis chain
Verify hardcoded multipleOf suits all float use cases.
The hardcoded 0.01 step increment assumes 2-decimal precision is appropriate for all float fields. This may not work well for:
- Scientific measurements requiring higher precision (e.g., 0.001 or smaller)
- Large-scale values where 0.01 is too granular (e.g., coordinates, large monetary values)
- Fields that should accept arbitrary precision
Confirm that SmartForm and downstream consumers can handle this constraint across all float field use cases.
Run the following script to find float field usage patterns in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find FloatField declarations to assess precision requirements
# Search for FloatField instantiations with context
rg -nP --type=py -C3 'FloatField\s*\('Length of output: 7360
🏁 Script executed:
#!/bin/bash
# Get context around the specific lines in annotation.py
sed -n '140,175p' fiftyone/core/annotation.pyLength of output: 1075
🏁 Script executed:
#!/bin/bash
# Search for where this schema is used and SmartForm references
rg -n 'multipleOf' --type=py fiftyone/Length of output: 167
🏁 Script executed:
#!/bin/bash
# Find SmartForm usage and how it handles multipleOf
rg -n 'SmartForm' --type=py -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find where the annotation schema is used/consumed
rg -n 'def.*annotation.*schema|get_annotation_schema|_get_field_schema' --type=py -A2Length of output: 695
🏁 Script executed:
#!/bin/bash
# Search for form rendering or UI libraries that might consume this schema
rg -n 'annotation.*schema|schema.*input|step.*increment' --type=py fiftyone/Length of output: 1512
🏁 Script executed:
#!/bin/bash
# Look at the full function containing this code
sed -n '100,180p' fiftyone/core/annotation.pyLength of output: 2488
Hardcoded 0.01 step increment is too generic for diverse float field semantics.
The unconditional multipleOf: 0.01 in the annotation schema ignores semantic differences:
- Frame rates/durations: Standard values (23.976, 29.97, etc.) don't align well with 0.01 steps
- 3D coordinates/spatial data: Typically require finer precision (0.001 or smaller)
- Confidence/opacity (0-1): 0.01 is reasonable
Consider making the step size context-aware based on field semantics or allowing per-field configuration, rather than applying a uniform constraint to all FloatField instances.
🤖 Prompt for AI Agents
In fiftyone/core/annotation.py around lines 158-160, the schema currently
unconditionally sets "multipleOf": 0.01 for all fof.FloatField instances; change
this to be context-aware by removing the hardcoded 0.01 and instead determine
step from field metadata or name patterns (e.g., use field.step or
field.metadata if present; fallback heuristics: 0.01 for normalized values like
confidence/opacity, 0.001 (or smaller) for coordinates/spatial data, and
specialized values for known semantic fields like fps/duration), or allow an
explicit per-field config option to override; ensure the schema generation uses
the provided step when present and otherwise omits "multipleOf" (or uses a
reasonable heuristic), preserving backward compatibility by honoring an explicit
step attribute first.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
app/packages/components/src/components/SmartForm/widgets/TextWidget.tsx (1)
26-28: The numeric coercion and theme color issues flagged in the previous review remain unaddressed.Lines 26-28:
handleChangestill passes raw strings for numeric schemas (type: "number"or"integer"). RJSF consumers expect actualnumbertypes; otherwise JSON Schema validation fails.Line 65: The inline
style={{ color: "error.main" }}won't resolve the MUI palette key. Usesxprop instead (e.g.,<Box component="span" sx={{ color: "error.main", ml: 0.5 }}>*</Box>).The previous review provides complete diff suggestions for both fixes.
Also applies to: 65-65
app/packages/components/src/components/SmartForm/templates/FieldTemplate.tsx (1)
12-14: PreserveclassNamesandstylein the hidden branch.The previous review correctly identified that dropping
classNameswhenhidden=truecan break CSS selectors and tests. Apply the suggested fix.app/packages/components/src/components/SmartForm/widgets/Dropdown.tsx (2)
23-26: Replaceanywithunknownfor type safety.Using
anydefeats TypeScript's type checking. Enum values can be strings, numbers, or other types.Apply this fix:
- const choices = enumValues.map((val: any, index: number) => ({ + const choices = enumValues.map((val: unknown, index: number) => ({ value: val, - label: enumNames[index] || val, + label: (enumNames[index] ?? String(val)), }));
46-48: Replaceanywithunknownand prefix unused parameter.Using
anyfor the onChange parameters defeats type safety. Thepathparameter is unused and should be prefixed with_.Apply this fix:
- const handleChange = (path: string, newValue: any) => { + const handleChange = (_path: string, newValue: unknown) => { onChange(newValue); };app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx (2)
22-25: Fix overly restrictive type annotation.Enum values can be any type (string, number, boolean, null), not just strings. The current type annotation will cause TypeScript errors for non-string enum values.
Apply this fix:
- const choices = enumValues.map((val: string, index: number) => ({ + const choices = enumValues.map((val: unknown, index: number) => ({ value: val, - label: enumNames[index] || val, + label: (enumNames[index] ?? String(val)), }));
30-42: Add missingcomponentproperty for SchemaIO routing.The view object only sets
name: "AutocompleteView"but omits thecomponentproperty. SchemaIO's routing logic usesschema.view.componentto select the correct view component. Without this, the form will renderUnsupportedViewinstead ofAutocompleteView.Apply this fix:
const schemaIOSchema = { type: multiple ? "array" : "string", view: { name: "AutocompleteView", + component: "AutocompleteView", label: label || schema.title, placeholder: uiSchema?.["ui:placeholder"], readOnly: readonly || disabled, choices: choices, allow_user_input: uiSchema?.["ui:options"]?.freeSolo ?? true, allow_clearing: uiSchema?.["ui:options"]?.allowClear ?? true, allow_duplicates: uiSchema?.["ui:options"]?.allowDuplicates ?? true, }, };app/packages/components/src/components/SmartForm/index.tsx (1)
28-40: Memoize schema translation to prevent unnecessary re-computation.
translateSchemaCompleteruns on every render, which is wasteful ifschemaanddataare stable. Warning logs will also spam the console on each render.Apply this fix:
+ const translated = React.useMemo( + () => translateSchemaComplete(props.schema, props.data), + [props.schema, props.data] + ); + const { schema: jsonSchema, uiSchema: generatedUiSchema, warnings, formData, - } = translateSchemaComplete(props.schema, props.data); + } = translated; const mergedUiSchema = { ...generatedUiSchema, ...props.uiSchema }; + React.useEffect(() => { - if (warnings.length > 0) { - console.warn("[SmartForm] Schema translation warnings:", warnings); - } + if (warnings.length > 0) { + console.warn("[SmartForm] Schema translation warnings:", warnings); + } + }, [warnings]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
app/packages/components/src/components/SmartForm/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/templates/FieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx(1 hunks)app/packages/components/src/components/SmartForm/widgets/Dropdown.tsx(1 hunks)app/packages/components/src/components/SmartForm/widgets/TextWidget.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsxapp/packages/components/src/components/SmartForm/widgets/AutoComplete.tsxapp/packages/components/src/components/SmartForm/widgets/Dropdown.tsxapp/packages/components/src/components/SmartForm/index.tsxapp/packages/components/src/components/SmartForm/templates/FieldTemplate.tsxapp/packages/components/src/components/SmartForm/widgets/TextWidget.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-02T21:53:53.778Z
Learnt from: tom-vx51
Repo: voxel51/fiftyone PR: 6372
File: app/packages/core/src/client/annotationClient.ts:32-46
Timestamp: 2025-10-02T21:53:53.778Z
Learning: In `app/packages/core/src/client/annotationClient.ts`, the `delta` field in `PatchSampleRequest` intentionally does not accept simple scalar values (strings, numbers, booleans). The `AttributeField` type is correctly defined as `Record<string, unknown>` to enforce that all delta entries must be structured objects, not primitives.
Applied to files:
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsxapp/packages/components/src/components/SmartForm/index.tsxapp/packages/components/src/components/SmartForm/widgets/TextWidget.tsx
🧬 Code graph analysis (1)
app/packages/components/src/components/SmartForm/index.tsx (3)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/index.ts (2)
translateSchemaComplete(46-59)convertRJSFDataToSchemaIO(21-21)app/packages/components/src/components/SmartForm/translators/data.ts (1)
convertRJSFDataToSchemaIO(87-163)
⏰ 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). (13)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-app
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (1)
app/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsx (1)
19-67: Layout implementation looks solid.The horizontal/vertical layout switching, conditional title/description rendering, and flex-based property arrangement are all correctly implemented. The use of
element.nameas the key is appropriate.
| const handleChange = (event: IChangeEvent, _id?: string) => { | ||
| if (props.onChange) { | ||
| const schemaIOData = convertRJSFDataToSchemaIO( | ||
| event.formData, | ||
| props.schema | ||
| ); | ||
|
|
||
| props.onChange(schemaIOData); | ||
| } | ||
| }; | ||
|
|
||
| const handleSubmit = (event: IChangeEvent, _nativeEvent: React.FormEvent) => { | ||
| if (props.onSubmit) { | ||
| const schemaIOData = convertRJSFDataToSchemaIO( | ||
| event.formData, | ||
| props.schema | ||
| ); | ||
|
|
||
| props.onSubmit(schemaIOData); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider memoizing event handlers.
Event handlers are recreated on every render. While not critical, memoizing them with useCallback would prevent unnecessary re-renders of child components.
Apply this refactor:
- const handleChange = (event: IChangeEvent, _id?: string) => {
+ const handleChange = React.useCallback((event: IChangeEvent, _id?: string) => {
if (props.onChange) {
const schemaIOData = convertRJSFDataToSchemaIO(
event.formData,
props.schema
);
props.onChange(schemaIOData);
}
- };
+ }, [props.onChange, props.schema]);
- const handleSubmit = (event: IChangeEvent, _nativeEvent: React.FormEvent) => {
+ const handleSubmit = React.useCallback((event: IChangeEvent, _nativeEvent: React.FormEvent) => {
if (props.onSubmit) {
const schemaIOData = convertRJSFDataToSchemaIO(
event.formData,
props.schema
);
props.onSubmit(schemaIOData);
}
- };
+ }, [props.onSubmit, props.schema]);🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/index.tsx around lines 42 to
62, the handleChange and handleSubmit functions are recreated on every render;
wrap each in React.useCallback to memoize them: create handleChange =
useCallback((event, _id?) => { if (props.onChange) { const schemaIOData =
convertRJSFDataToSchemaIO(event.formData, props.schema);
props.onChange(schemaIOData); } }, [props.onChange, props.schema]) and similarly
create handleSubmit = useCallback((event, _nativeEvent) => { if (props.onSubmit)
{ const schemaIOData = convertRJSFDataToSchemaIO(event.formData, props.schema);
props.onSubmit(schemaIOData); } }, [props.onSubmit, props.schema]); import
useCallback from React if not already imported and keep the same parameter types
and behavior.
| // Check if this object should use horizontal layout (GridView) | ||
| const layout = uiSchema?.["ui:options"]?.layout; | ||
| const isHorizontal = layout === "horizontal"; | ||
| const gap = uiSchema?.["ui:options"]?.gap ?? 2; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate the gap value.
The gap extracted from ui:options is used directly without validation. If a user provides a negative number or non-numeric value, it could cause layout issues or React warnings.
Apply this diff to add basic validation:
- const gap = uiSchema?.["ui:options"]?.gap ?? 2;
+ const gap = typeof uiSchema?.["ui:options"]?.gap === "number" && uiSchema["ui:options"].gap >= 0
+ ? uiSchema["ui:options"].gap
+ : 2;📝 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.
| const gap = uiSchema?.["ui:options"]?.gap ?? 2; | |
| const gap = typeof uiSchema?.["ui:options"]?.gap === "number" && uiSchema["ui:options"].gap >= 0 | |
| ? uiSchema["ui:options"].gap | |
| : 2; |
🤖 Prompt for AI Agents
In
app/packages/components/src/components/SmartForm/templates/ObjectFieldTemplate.tsx
around line 16, the gap pulled from uiSchema["ui:options"] is used without
validation; update the code to coerce and validate gap: parse numeric/string
values (e.g., Number or parseInt), ensure it is a finite number and non-negative
(or clamp with Math.max(0, value)), and if validation fails fall back to the
default 2; implement this check inline where gap is assigned so downstream
layout always receives a safe numeric gap.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/packages/components/src/components/SmartForm/index.tsx (2)
25-36: Memoize translation to prevent unnecessary re-computation.The translation runs on every render, which is wasteful when
props.schemahasn't changed. Additionally, warnings are logged on every render, which can spam the console.Wrap the translation in
useMemo:- const { - schema: jsonSchema, - uiSchema: generatedUiSchema, - warnings, - } = translateSchemaComplete(props.schema); + const { schema: jsonSchema, uiSchema: generatedUiSchema, warnings } = React.useMemo( + () => translateSchemaComplete(props.schema), + [props.schema] + );And log warnings only when they change:
+ React.useEffect(() => { + if (warnings.length > 0) { + console.warn("[SmartForm] Schema translation warnings:", warnings); + } + }, [warnings]); - if (warnings.length > 0) { - console.warn("[SmartForm] Schema translation warnings:", warnings); - }
38-48: Memoize event handlers to prevent child re-renders.Handlers are recreated on every render. While not breaking, memoizing them prevents unnecessary re-renders of the RJSF Form.
Apply this diff:
- const handleChange = (event: IChangeEvent, _id?: string) => { + const handleChange = React.useCallback((event: IChangeEvent, _id?: string) => { if (props.onChange) { props.onChange(event.formData); } - }; + }, [props.onChange]); - const handleSubmit = (event: IChangeEvent, _nativeEvent: React.FormEvent) => { + const handleSubmit = React.useCallback((event: IChangeEvent, _nativeEvent: React.FormEvent) => { if (props.onSubmit) { props.onSubmit(event.formData); } - }; + }, [props.onSubmit]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
app/packages/components/src/components/SmartForm/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/translators/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/SmartForm/index.tsxapp/packages/components/src/components/SmartForm/translators/index.ts
🧠 Learnings (1)
📚 Learning: 2024-07-03T15:23:55.253Z
Learnt from: ritch
Repo: voxel51/fiftyone PR: 4546
File: app/packages/looker-3d/src/hooks/use-track-status.ts:14-38
Timestamp: 2024-07-03T15:23:55.253Z
Learning: When optimizing state updates in React components to avoid unnecessary re-renders, consider using a debounce mechanism or batching updates at regular intervals.
Applied to files:
app/packages/components/src/components/SmartForm/index.tsx
🧬 Code graph analysis (2)
app/packages/components/src/components/SmartForm/index.tsx (2)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/index.ts (1)
translateSchemaComplete(44-52)
app/packages/components/src/components/SmartForm/translators/index.ts (4)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/translators/utils.ts (3)
TranslationOptions(11-13)TranslationResult(4-9)TranslationContext(15-19)app/packages/components/src/components/SmartForm/translators/schema.ts (2)
translateToJSONSchema(11-131)addChoicesToSchema(136-203)app/packages/components/src/components/SmartForm/translators/ui.ts (1)
translateToUISchema(10-220)
⏰ 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: modified-files
- GitHub Check: build
🔇 Additional comments (5)
app/packages/components/src/components/SmartForm/index.tsx (3)
1-23: LGTM on imports and interface definition.The imports are well-organized, and the
SmartFormPropsinterface provides appropriate flexibility withunknowntypes for data and callbacks, allowing the component to handle various schema shapes.
32-32: LGTM on UI schema merge precedence.Correctly allows
props.uiSchemato override generated hints, providing the right extension point for consumers.
50-61: LGTM on Form configuration.The RJSF Form is properly configured with translated schemas, custom widgets/templates, and validation.
app/packages/components/src/components/SmartForm/translators/index.ts (2)
1-20: LGTM on module structure.Clean organization with appropriate type exports and utility re-exports.
21-39: LGTM ontranslateSchemaimplementation.Correctly initializes context and orchestrates translation, with proper handling of warnings and options.
app/packages/components/src/components/SmartForm/RJSF/translators/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
app/packages/components/src/components/SmartForm/README.md(1 hunks)app/packages/components/src/components/SmartForm/USAGE_EXAMPLES.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
app/packages/components/src/components/SmartForm/README.md
304-304: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (12)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (2)
app/packages/components/src/components/SmartForm/README.md (1)
1-481: Documentation is comprehensive and well-structured.The README provides clear overviews of SmartForm's capabilities, features, and integration patterns. Examples are practical and cover core use cases (basic forms, custom UI schema, validation, custom widgets). The migration guide and limitations section are particularly helpful for users transitioning from SchemaIO. The architecture diagram and test coverage details round out a solid reference document.
app/packages/components/src/components/SmartForm/USAGE_EXAMPLES.md (1)
1-1037: Excellent comprehensive usage documentation.This guide provides extensive, well-organized examples covering SmartForm from basic usage through advanced patterns. The Field Types section is particularly thorough with each type paired with generated UI details. The real-world annotation form example and migration guide demonstrate practical value. The tips and troubleshooting sections preemptively address common developer questions. Content quality and breadth are excellent for users learning SmartForm.
| ``` | ||
| SmartForm/ | ||
| ├── index.tsx # Main component | ||
| ├── translators/ # Schema translation | ||
| │ ├── index.ts # Main API | ||
| │ ├── schema.ts # JSON Schema translation | ||
| │ ├── ui.ts # UI Schema translation | ||
| │ ├── data.ts # Data conversion | ||
| │ └── utils.ts # Shared utilities | ||
| ├── widgets/ # Custom RJSF widgets | ||
| │ ├── AutoComplete.tsx | ||
| │ ├── Dropdown.tsx | ||
| │ └── TextWidget.tsx | ||
| ├── templates/ # Custom RJSF templates | ||
| │ ├── FieldTemplate.tsx | ||
| │ └── ObjectFieldTemplate.tsx | ||
| └── __tests__/ # Comprehensive tests | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to architecture diagram code block.
Line 304 opens a fenced code block without a language identifier. The static analysis tool flags this as MD040. Since the block contains a text-based file tree, add a language tag.
-```
+```
SmartForm/
├── index.tsx # Main component
├── translators/ # Schema translation
│ ├── index.ts # Main API
│ ├── schema.ts # JSON Schema translation
│ ├── ui.ts # UI Schema translation
│ ├── data.ts # Data conversion
│ └── utils.ts # Shared utilities
├── widgets/ # Custom RJSF widgets
│ ├── AutoComplete.tsx
│ ├── Dropdown.tsx
│ └── TextWidget.tsx
├── templates/ # Custom RJSF templates
│ ├── FieldTemplate.tsx
│ └── ObjectFieldTemplate.tsx
└── __tests__/ # Comprehensive tests
-```
+```Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
304-304: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/README.md around lines 304
to 321 the fenced code block for the file-tree has no language specifier
(MD040); update the opening fence to include a plain-text language tag such as
"text" (e.g., change ``` to ```text) and leave the closing fence as-is so the
static analysis warning is resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx (1)
16-25: Fix enum typing and ensure labels are always strings inchoices
schema.enumcan contain numbers, booleans, and nulls, not just strings. Typing the map callback parameter asval: stringis too narrow and can cause TypeScript errors when the enum contains non-string values. Also,label: enumNames[index] || valcan yield non-string labels.Recommend loosening the value type and normalizing labels to strings:
- const examples = schema.examples || []; - const enumValues = schema.enum || examples; - const enumNames = uiSchema?.["ui:enumNames"] || enumValues; - - // Build choices array in SchemaIO format - const choices = enumValues.map((val: string, index: number) => ({ - value: val, - label: enumNames[index] || val, - })); + const examples = schema.examples || []; + const enumValues = (schema.enum ?? examples) ?? []; + const enumNames = + (uiSchema?.["ui:enumNames"] as (string | undefined)[] | undefined) ?? + []; + + // Build choices array in SchemaIO format + const choices = enumValues.map((val: unknown, index: number) => ({ + value: val, + label: String(enumNames[index] ?? val), + }));This preserves the original enum value type on
choices.valuewhile guaranteeingchoices.labelis a string suitable for display.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
app/packages/components/src/components/SmartForm/translators/ui.ts(1 hunks)app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsxapp/packages/components/src/components/SmartForm/translators/ui.tsapp/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-02T21:53:53.778Z
Learnt from: tom-vx51
Repo: voxel51/fiftyone PR: 6372
File: app/packages/core/src/client/annotationClient.ts:32-46
Timestamp: 2025-10-02T21:53:53.778Z
Learning: In `app/packages/core/src/client/annotationClient.ts`, the `delta` field in `PatchSampleRequest` intentionally does not accept simple scalar values (strings, numbers, booleans). The `AttributeField` type is correctly defined as `Record<string, unknown>` to enforce that all delta entries must be structured objects, not primitives.
Applied to files:
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx
🧬 Code graph analysis (2)
app/packages/components/src/components/SmartForm/widgets/AutoComplete.tsx (1)
app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx (1)
AutocompleteView(7-91)
app/packages/components/src/components/SmartForm/translators/ui.ts (2)
app/packages/components/src/components/SmartForm/translators/index.ts (3)
translateToUISchema(19-19)TranslationContext(10-10)addWarning(12-12)app/packages/components/src/components/SmartForm/translators/utils.ts (2)
TranslationContext(15-19)addWarning(21-27)
⏰ 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). (13)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-app
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (1)
app/packages/core/src/plugins/SchemaIO/components/AutocompleteView.tsx (1)
26-26: Controlled value change looks correct; watch for controlled/uncontrolled warningsSwitching from
defaultValuetovaluemakes the MUIAutocompleteproperly controlled bydata, which is desirable. Just ensure your tests or manual runs don’t surface React/MUI warnings about changing from uncontrolled to controlled; if they do, you may want to normalizegetDefaultValue(data, choices)tonullinstead ofundefined.
| // Hide submit button by default | ||
| uiSchema["ui:submitButtonOptions"] = { | ||
| norender: true, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Scope ui:submitButtonOptions to the root schema only
Because translateToUISchema is recursive, these lines attach ui:submitButtonOptions to every nested field, even though RJSF only uses it at the form root. This is harmless but noisy and can confuse future maintainers.
Consider restricting this to the root context, for example:
if (context.path.length === 0) {
uiSchema["ui:submitButtonOptions"] = {
norender: true,
};
}🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/translators/ui.ts around
lines 214 to 217, the code unconditionally sets ui:submitButtonOptions which,
because translateToUISchema is recursive, attaches this root-only option to
every nested field; change it to only add ui:submitButtonOptions when the
current context is the root (e.g., check context.path.length === 0 or
equivalent) so the norender flag is only applied at the form root.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
app/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsx (1)
18-26: Tighten choice typing and avoidanyin enum mapping
choicescurrently usesany:const choices = enumValues.map((val: any, index: number) => ({ value: val, label: enumNames[index] || val, }));This loses type safety and can leave
labelas a non‑string.Prefer a safer mapping that keeps values opaque but guarantees string labels, e.g.:
const enumValues = schema.enum ?? []; const enumNames = schema.enumNames ?? enumValues; const choices = enumValues.map((val: unknown, index: number): { value: unknown; label: string; } => ({ value: val, label: String(enumNames[index] ?? val), }));This handles string/number/boolean/null enums while keeping RJSF/SchemaIO integration type‑safe.
app/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsx (1)
16-25: Relax enum value type inchoicesto support non‑string enumsHere:
const choices = enumValues.map((val: string, index: number) => ({ value: val, label: enumNames[index] || val, }));
val: stringis too restrictive—JSON Schema enums can be numbers, booleans, or null. This will mis‑type valid schemas and can force unsafe casts elsewhere.Update to keep values opaque and normalize labels:
const choices = enumValues.map((val: unknown, index: number): { value: unknown; label: string; } => ({ value: val, label: String(enumNames[index] ?? val), }));This matches JSON Schema’s enum flexibility while ensuring
labelis always a string for the UI.app/packages/core/src/plugins/SchemaIO/index.tsx (1)
7-36: Separate SchemaIO vs SmartForm modes in the props type and callback contract
SchemaIOComponentPropscurrently merges two different modes behind one interface:
- SchemaIO mode cares about
shouldClearUseKeyStores,onPathChange, andonChange(data, liteValues?).- SmartForm mode cares about
jsonSchema,uiSchema,onSubmit, and a simpleronChange(data).Forwarding
propsdirectly into<SmartForm {...props} />means:
- Callers see
onChange(data, liteValues?)but in SmartForm modeliteValuesis alwaysundefined.- SmartForm receives extra props it doesn’t use, and the type relationship between its
onChangeand this wrapper’sonChangeis unclear.To make this safer and harder to misuse:
- Split the props into a discriminated union:
type SchemaIOModeProps = { schema: SchemaType; smartForm?: false; data?: unknown; id?: string; onChange?: (data: unknown, liteValues?: Record<string, unknown>) => void; shouldClearUseKeyStores?: boolean; onPathChange?: (/* ... */) => void; }; type SmartFormModeProps = { schema: SchemaType; smartForm: true; data?: unknown; id?: string; onChange?: (data: unknown) => void; // no liteValues here onSubmit?: (data: unknown) => void; jsonSchema?: SmartFormProps["jsonSchema"]; uiSchema?: SmartFormProps["uiSchema"]; }; export type SchemaIOComponentProps = SchemaIOModeProps | SmartFormModeProps;
- When rendering SmartForm, map only the relevant props and adapt
onChangeif needed:if (props.smartForm) { const { schema, data, id, onChange, onSubmit, jsonSchema, uiSchema } = props; return ( <SmartForm schema={schema} jsonSchema={jsonSchema} uiSchema={uiSchema} data={data} onChange={onChange} onSubmit={onSubmit} /> ); }This makes it explicit that
liteValuesis not available in SmartForm mode and prevents accidental reliance on it.app/packages/components/src/components/SmartForm/RJSF/translators/schema.ts (1)
18-30: Preserve integer semantics when translating numeric typesLines 23–30 map both
"number"and"integer"toschema.type = "number":case "number": case "integer": schema.type = "number"; if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; if (schemaIO.multipleOf !== undefined) schema.multipleOf = schemaIO.multipleOf; break;This relaxes validation: a SchemaIO field declared as integer will accept non‑integral values in the resulting JSON Schema/RJSF form.
Split these cases so
integerremains an integer in JSON Schema while reusing the same constraints:case "number": schema.type = "number"; if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; if (schemaIO.multipleOf !== undefined) schema.multipleOf = schemaIO.multipleOf; break; case "integer": schema.type = "integer"; if (schemaIO.min !== undefined) schema.minimum = schemaIO.min; if (schemaIO.max !== undefined) schema.maximum = schemaIO.max; if (schemaIO.multipleOf !== undefined) schema.multipleOf = schemaIO.multipleOf; break;This keeps RJSF validation aligned with the original SchemaIO intent.
app/packages/components/src/components/SmartForm/RJSF/translators/ui.test.ts (1)
244-305: Add test coverage forGridViewbranch intranslateToUISchema
translateToUISchemahas a dedicatedGridViewbranch that:
- Sets
ui:options.hideTitle,gap,align_x,align_y, and optionallayout: "horizontal".- Recursively processes
schemaIO.propertiessimilarly toObjectView.This test file does not exercise that code path, leaving behavior around layout options and nested properties unverified.
Add at least one
GridViewtest that:
- Asserts correct mapping of
gap,align_x,align_y, andhideTitle.- Covers the horizontal orientation variant (
orientation: "horizontal"⇒ui:options.layout = "horizontal").- Verifies that nested properties under
propertiesare still translated correctly.Also applies to: 307-367
app/packages/components/src/components/SmartForm/RJSF/translators/index.ts (1)
21-39: WiretranslateSchemaCompleteto data conversion and avoid mutating the resultTwo issues around
translateSchemaComplete(Lines 44–52):
Missing data-conversion logic vs JSDoc and tests
The JSDoc says “Complete translation with choice processing and optional data conversion”, and tests in
index.test.tspass a second argument as data and assert onresult.formData. The current implementation:export function translateSchemaComplete( schemaIO: SchemaType, options: TranslationOptions = {} ): TranslationResult { const result = translateSchema(schemaIO, options); result.schema = addChoicesToSchema(result.schema, schemaIO); return result; }
- Ignores everything in
optionsexceptstrictMode(viatranslateSchema).- Never sets
formData.To make the contract coherent:
- Extend
TranslationOptions(inutils.ts) with adata?: unknown(orinitialData) field.- Add a data-conversion step here that computes
formDatafromschemaIOandoptions.data(e.g., via a dedicated helper), and include it in the returnedTranslationResult.- Update call sites/tests to pass
{ data }in options rather than raw data.Unnecessary mutation of the
TranslationResult
result.schema = addChoicesToSchema(...)mutates the object returned bytranslateSchema. This can surprise callers if they rely on the original result or share it.Prefer returning a new object:
export function translateSchemaComplete( schemaIO: SchemaType, options: TranslationOptions = {} ): TranslationResult {
- const result = translateSchema(schemaIO, options);
- result.schema = addChoicesToSchema(result.schema, schemaIO);
- return result;
- const result = translateSchema(schemaIO, options);
- const schemaWithChoices = addChoicesToSchema(result.schema, schemaIO);
- return {
- ...result,
- schema: schemaWithChoices,
- };
}Also applies to: 41-52
app/packages/components/src/components/SmartForm/RJSF/translators/ui.ts (1)
200-217: Scopeui:submitButtonOptionsto the root schema onlyBecause
translateToUISchemais recursive, Lines 214–217 attach:uiSchema["ui:submitButtonOptions"] = { norender: true, };to every nested field, even though RJSF only uses this option at the form root. This is harmless but adds noise and can confuse maintainers.
Restrict this to the root context:
- // Hide submit button by default - uiSchema["ui:submitButtonOptions"] = { - norender: true, - }; + // Hide submit button by default (root only) + if (context.path.length === 0) { + uiSchema["ui:submitButtonOptions"] = { + norender: true, + }; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
app/packages/components/src/components/SmartForm/RJSF/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/templates/FieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/templates/ObjectFieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/templates/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/index.test.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/index.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/schema.test.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/schema.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/ui.test.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/ui.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/utils.test.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/index.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/SmartForm/RJSF/index.tsxapp/packages/components/src/components/SmartForm/RJSF/templates/index.tsxapp/packages/components/src/components/SmartForm/RJSF/templates/FieldTemplate.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/index.tsxapp/packages/components/src/components/SmartForm/RJSF/translators/schema.test.tsapp/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsxapp/packages/components/src/components/SmartForm/index.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsxapp/packages/components/src/components/SmartForm/RJSF/translators/utils.test.tsapp/packages/components/src/components/SmartForm/RJSF/translators/ui.tsapp/packages/components/src/components/SmartForm/RJSF/translators/utils.tsapp/packages/components/src/components/SmartForm/RJSF/translators/index.test.tsapp/packages/components/src/components/SmartForm/RJSF/translators/schema.tsapp/packages/components/src/components/SmartForm/RJSF/translators/ui.test.tsapp/packages/components/src/components/SmartForm/RJSF/translators/index.tsapp/packages/components/src/components/SmartForm/RJSF/templates/ObjectFieldTemplate.tsxapp/packages/core/src/plugins/SchemaIO/index.tsx
🧠 Learnings (6)
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/components/src/components/SmartForm/RJSF/index.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsxapp/packages/core/src/plugins/SchemaIO/index.tsx
📚 Learning: 2024-11-06T20:52:30.520Z
Learnt from: benjaminpkane
Repo: voxel51/fiftyone PR: 5051
File: app/packages/utilities/src/index.ts:12-12
Timestamp: 2024-11-06T20:52:30.520Z
Learning: In `app/packages/utilities/src/index.ts`, when an export statement like `export * from "./Resource";` is moved within the file, it is not a duplication even if it appears in both the removed and added lines of a diff.
Applied to files:
app/packages/components/src/components/SmartForm/RJSF/templates/index.tsxapp/packages/components/src/components/SmartForm/RJSF/translators/index.ts
📚 Learning: 2025-10-02T21:53:53.778Z
Learnt from: tom-vx51
Repo: voxel51/fiftyone PR: 6372
File: app/packages/core/src/client/annotationClient.ts:32-46
Timestamp: 2025-10-02T21:53:53.778Z
Learning: In `app/packages/core/src/client/annotationClient.ts`, the `delta` field in `PatchSampleRequest` intentionally does not accept simple scalar values (strings, numbers, booleans). The `AttributeField` type is correctly defined as `Record<string, unknown>` to enforce that all delta entries must be structured objects, not primitives.
Applied to files:
app/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsx
📚 Learning: 2024-07-03T15:23:55.253Z
Learnt from: ritch
Repo: voxel51/fiftyone PR: 4546
File: app/packages/looker-3d/src/hooks/use-track-status.ts:14-38
Timestamp: 2024-07-03T15:23:55.253Z
Learning: When optimizing state updates in React components to avoid unnecessary re-renders, consider using a debounce mechanism or batching updates at regular intervals.
Applied to files:
app/packages/components/src/components/SmartForm/index.tsx
📚 Learning: 2025-07-22T15:19:26.438Z
Learnt from: imanjra
Repo: voxel51/fiftyone PR: 6164
File: app/packages/core/src/plugins/SchemaIO/index.tsx:23-34
Timestamp: 2025-07-22T15:19:26.438Z
Learning: In the FiftyOne codebase, specifically in app/packages/core/src/plugins/SchemaIO/index.tsx, the store object accessed via props.otherProps.store is always defined and initialized to an empty object, so defensive checks for store existence are not needed.
Applied to files:
app/packages/core/src/plugins/SchemaIO/index.tsx
📚 Learning: 2025-07-11T22:23:39.205Z
Learnt from: imanjra
Repo: voxel51/fiftyone PR: 6125
File: app/packages/spaces/src/components/Workspaces/WorkspaceEditor.tsx:140-145
Timestamp: 2025-07-11T22:23:39.205Z
Learning: In app/packages/spaces/src/components/Workspaces/WorkspaceEditor.tsx, the SaveWorkspace operator expects an `old_name` parameter but the frontend currently sends `current_name`. This parameter mismatch prevents proper workspace renaming functionality, though it doesn't affect edit-only scenarios where the name doesn't change.
Applied to files:
app/packages/core/src/plugins/SchemaIO/index.tsx
🧬 Code graph analysis (11)
app/packages/components/src/components/SmartForm/RJSF/index.tsx (2)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/RJSF/translators/index.ts (1)
translateSchemaComplete(44-52)
app/packages/components/src/components/SmartForm/RJSF/translators/schema.test.ts (2)
app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (1)
TranslationContext(15-19)app/packages/components/src/components/SmartForm/RJSF/translators/schema.ts (2)
translateToJSONSchema(11-131)addChoicesToSchema(136-203)
app/packages/components/src/components/SmartForm/index.tsx (2)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/RJSF/index.tsx (1)
RJSF(25-62)
app/packages/components/src/components/SmartForm/RJSF/translators/utils.test.ts (1)
app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (5)
TranslationContext(15-19)addWarning(21-27)getEmptyValueForType(29-47)isSchemaIOSchema(54-61)isJSONSchema(66-73)
app/packages/components/src/components/SmartForm/RJSF/translators/ui.ts (2)
app/packages/components/src/components/SmartForm/RJSF/translators/index.ts (3)
translateToUISchema(19-19)TranslationContext(10-10)addWarning(12-12)app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (2)
TranslationContext(15-19)addWarning(21-27)
app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (1)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)
app/packages/components/src/components/SmartForm/RJSF/translators/index.test.ts (3)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/RJSF/translators/index.ts (4)
translateSchema(24-39)translateSchemaComplete(44-52)isSchemaIOSchema(14-14)isJSONSchema(15-15)app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (2)
isSchemaIOSchema(54-61)isJSONSchema(66-73)
app/packages/components/src/components/SmartForm/RJSF/translators/schema.ts (2)
app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (2)
TranslationContext(15-19)addWarning(21-27)app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)
app/packages/components/src/components/SmartForm/RJSF/translators/ui.test.ts (2)
app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (1)
TranslationContext(15-19)app/packages/components/src/components/SmartForm/RJSF/translators/ui.ts (1)
translateToUISchema(10-220)
app/packages/components/src/components/SmartForm/RJSF/translators/index.ts (4)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/RJSF/translators/utils.ts (3)
TranslationOptions(11-13)TranslationResult(4-9)TranslationContext(15-19)app/packages/components/src/components/SmartForm/RJSF/translators/schema.ts (2)
translateToJSONSchema(11-131)addChoicesToSchema(136-203)app/packages/components/src/components/SmartForm/RJSF/translators/ui.ts (1)
translateToUISchema(10-220)
app/packages/core/src/plugins/SchemaIO/index.tsx (3)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/index.tsx (2)
SmartFormProps(7-14)SmartForm(16-19)app/packages/core/src/plugins/SchemaIO/SchemaIOComponent.tsx (1)
SchemaIOComponent(8-82)
⏰ 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). (13)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test / test-app
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
| export interface SmartFormProps { | ||
| schema?: SchemaType; | ||
| jsonSchema?: RJSFSchema; | ||
| uiSchema?: UiSchema; | ||
| data?: unknown; | ||
| onChange?: (data: unknown) => void; | ||
| onSubmit?: (data: unknown) => void; | ||
| } | ||
|
|
||
| export default function SmartForm(props: SmartFormProps) { | ||
| // potentially support RJSF alternatives here | ||
| return <RJSF {...props} />; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Strengthen SmartFormProps to enforce a valid schema source
Right now both schema and jsonSchema are optional, and SmartForm blindly forwards props to RJSF. A caller can accidentally render <SmartForm> without either, which will surface as a runtime error inside RJSF instead of a compile‑time failure.
Consider tightening the contract, e.g. a union that enforces “exactly one of schema or jsonSchema”:
type SmartFormProps =
| {
schema: SchemaType;
jsonSchema?: never;
uiSchema?: UiSchema;
data?: unknown;
onChange?: (data: unknown) => void;
onSubmit?: (data: unknown) => void;
}
| {
schema?: never;
jsonSchema: RJSFSchema;
uiSchema?: UiSchema;
data?: unknown;
onChange?: (data: unknown) => void;
onSubmit?: (data: unknown) => void;
};or at least add a runtime guard that throws if neither is provided so misconfiguration fails loudly and early.
🏁 Script executed:
#!/bin/bash
# Check current state of SmartForm file
cat app/packages/components/src/components/SmartForm/index.tsxLength of output: 538
🏁 Script executed:
#!/bin/bash
# Search for SmartForm usages to understand real-world impact
rg -n "SmartForm" --type=tsx --type=ts -A2 -B2 -g '!**/test/**' | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check if there are any runtime guards or validation
rg -n "schema.*jsonSchema|jsonSchema.*schema" --type=tsx --type=ts app/packages/components/src/components/SmartForm/Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for package.json to identify RJSF version
find . -name "package.json" -type f | head -5 | xargs grep -l "react-jsonschema-form\|@rjsf"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check RJSF imports to understand what's being used
rg -n "from.*rjsf|import.*RJSF" --type=tsx --type=ts -B1 -A1 app/packages/components/src/components/SmartForm/Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check if SmartForm component has been modified with type guards or validation
rg -n "SmartForm.*function|export.*SmartForm" --type=tsx app/packages/components/src/components/SmartForm/ -A10Length of output: 27
🏁 Script executed:
#!/bin/bash
# Verify SchemaType and RJSFSchema import sources
rg -n "SchemaType|RJSFSchema" --type=tsx app/packages/components/src/components/SmartForm/ -B1Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check for any union type patterns or discriminated unions in props definitions
rg -n "type.*Props\s*=\s*\|" --type=tsx app/packages/components/src/components/ -A15Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for error boundaries or try-catch around RJSF
rg -n "try|catch|Error.*Boundary" app/packages/components/src/components/SmartForm/ --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Check for onChange/onSubmit handler patterns which might validate
rg -n "onChange|onSubmit" app/packages/components/src/components/SmartForm/index.tsx -B1 -A1Length of output: 107
🏁 Script executed:
#!/bin/bash
# Look at test files to understand expected usage patterns
fd "SmartForm.*test\|SmartForm.*spec" --type f -x cat {} \;Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's middleware or wrapper logic around SmartForm
rg -n "SmartForm" app/packages/components/src/components/ --type=tsx --type=ts -g '**/index.*' -B3 -A3 | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Confirm neither field is being validated at form level
ast-grep --pattern 'interface SmartFormProps {
$$$
}'Length of output: 695
🏁 Script executed:
#!/bin/bash
# Check the RJSF package.json for required dependencies
fd "package.json" -x grep -l "@rjsf\|react-jsonschema-form" {} \; | xargs cat | jq '.dependencies, .peerDependencies' 2>/dev/null | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a build or type checking that would catch this
find . -name "tsconfig.json" -type f -exec grep -l "strict" {} \;Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the exact props passed to RJSF to understand spread behavior
rg -n "RJSF.*props\|<RJSF" app/packages/components/src/components/SmartForm/ --type=tsx -B2 -A2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check if schema/jsonSchema appear in validation files
fd -e "ts" -e "tsx" -e "js" -e "jsx" | xargs rg -l "validateSchema|schema.*validation" | head -10Length of output: 95
🏁 Script executed:
#!/bin/bash
# Summary: List all exports from SmartForm folder
ls -la app/packages/components/src/components/SmartForm/ 2>/dev/null || echo "Directory not found"Length of output: 362
🏁 Script executed:
#!/bin/bash
# Get the actual file structure
fd . app/packages/components/src/components/SmartForm --max-depth 3 2>/dev/null || echo "Path check"Length of output: 1661
🏁 Script executed:
#!/bin/bash
# Final verification: complete SmartForm file
find app -name "index.tsx" -path "*/SmartForm/*" -exec cat {} \; 2>/dev/null || find . -path "*/SmartForm/index.tsx" -exec cat {} \; 2>/dev/nullLength of output: 2482
🏁 Script executed:
#!/bin/bash
# Alternative path search
fd "SmartForm" -t d -x find {} -name "index.tsx" -exec cat {} \;Length of output: 33
🏁 Script executed:
#!/bin/bash
# Look for the exact file mentioned
find . -type f -name "index.tsx" | xargs grep -l "SmartFormProps\|SmartForm" 2>/dev/null | grep -i smartformLength of output: 126
🏁 Script executed:
#!/bin/bash
# Search at root
rg -l "SmartFormProps" --max-count 5Length of output: 233
🏁 Script executed:
#!/bin/bash
# Simpler: just find TypeScript files with SmartFormProps
fd -e tsx -e ts -x grep -l "SmartFormProps" {} \;Length of output: 111
🏁 Script executed:
#!/bin/bash
# Get absolute paths of ts/tsx files
fd -e tsx -e ts --path "*/SmartForm/*"Length of output: 233
🏁 Script executed:
#!/bin/bash
# Check packages directory structure
ls -la app/packages/components/src/components/ 2>/dev/null | head -20 || ls -la packages/components/src/components/ 2>/dev/null | head -20 || find . -type d -name "components" | head -5Length of output: 1152
🏁 Script executed:
#!/bin/bash
# Direct check for the file structure
ls -la app/packages/components/src/ 2>/dev/null || ls -la packages/components/src/ 2>/dev/null || ls -la components/src/ 2>/dev/nullLength of output: 428
🏁 Script executed:
#!/bin/bash
# Recursive find with a common component pattern
find . -type f \( -name "index.tsx" -o -name "index.ts" \) -path "*components*" 2>/dev/null | grep -i formLength of output: 353
🏁 Script executed:
#!/bin/bash
# List full directory tree structure for debugging
find . -maxdepth 4 -type d -name "components" 2>/dev/nullLength of output: 25
🏁 Script executed:
#!/bin/bash
# Check if path exists and what's in it
test -d "app/packages/components/src/components/SmartForm" && ls -la "app/packages/components/src/components/SmartForm" || echo "Path does not exist; searching..."
fd "SmartForm" -t d 2>/dev/null | head -10Length of output: 412
🏁 Script executed:
#!/bin/bash
# Show repository structure
find . -maxdepth 3 -type d 2>/dev/null | head -30Length of output: 541
🏁 Script executed:
#!/bin/bash
# Check if it's in a different location entirely
rg "export interface SmartFormProps" --type tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Global search for the exact pattern
rg "export.*SmartForm\|interface SmartFormProps" 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# List root dirs
ls -la | grep -E "^d"Length of output: 746
🏁 Script executed:
#!/bin/bash
# Check what's in the repo
ls -laLength of output: 2237
🏁 Script executed:
#!/bin/bash
# Find all tsx/ts files with SmartForm
find . -type f \( -name "*.tsx" -o -name "*.ts" \) -exec grep -l "SmartForm" {} \; 2>/dev/nullLength of output: 177
🌐 Web query:
react-jsonschema-form schema required prop behavior
💡 Result:
Summary of react-jsonschema-form's "required" behavior
-
JSON-Schema semantics: for objects, "required" is an array of property names on the object schema — it marks those properties as required (must be present/validated). It applies only to the immediate properties of that object (not a global flag). [1]
-
Arrays: marking an array property as "required" only requires the array to exist; an empty array is considered valid. To require at least one element use minItems (e.g., "minItems": 1). [2]
-
rjsf UI/validation behavior:
- rjsf uses the JSON Schema required info to compute field.required and passes that into FieldTemplates and widgets. [3]
- By default rjsf also uses HTML5 required attributes (so browsers may show native messages); you can disable HTML5 validation with the Form prop noHtml5Validate. Live validation is off by default (validate on submit) unless you set liveValidate. [4]
-
Nested/edge cases: include "required" on the schema for the specific nested object whose properties you want required. For arrays of objects, put required on the object items schema to require fields inside each item. [1][2]
References
- rjsf API / schema docs. [1]
- rjsf Arrays doc (empty array is valid; use minItems). [2]
- FieldTemplate / template props (required passed to templates). [3]
- Validation & HTML5 / liveValidate / noHtml5Validate. [4]
🌐 Web query:
RJSF library required props validation
💡 Result:
Short answer — how RJSF handles "required" and the props your custom widgets/fields receive:
-
Mark fields required in your JSON Schema using the standard required array on the parent object: e.g. { type: "object", required: ["name"], properties: { name: { type: "string" } } }. The schema validator (AJV via an RJSF validator package) will produce validation errors for missing required fields. [1]
-
RJSF passes a required boolean prop to custom widgets and to field components. Use that prop to render a required marker/attribute on the input (e.g. required on an ) or to change UI. Field/widget prop docs list required among the provided props. [1]
-
To customize validation messaging/behavior:
- Use the Form validate(formData, errors) callback to add custom validation logic. [1]
- Use transformErrors to change validator-generated errors before display. [1]
-
If you need to suppress default error display for a subtree, set uiSchema 'ui:hideError': true and render/handle rawErrors or errorSchema yourself. [2]
Minimal example:
- JSON Schema:
{ type: "object", required: ["email"], properties: { email: { type: "string", format: "email" } } } - Custom widget (props): it receives props.required (boolean); set <input required={props.required} ... />
- Add extra validation via Form validate or transformErrors if you need non-schema checks.
Sources:
[1] RJSF docs — Custom Widgets and Fields (props, required) and validation overview.
[2] RJSF docs — uiSchema reference (ui:hideError and related UI directives).
🌐 Web query:
react-jsonschema-form error handling missing schema
💡 Result:
Short answer — react-jsonschema-form (RJSF) requires a valid schema prop; handle a missing/invalid schema by guarding before render, providing a safe default schema, or using React error boundaries and RJSF hooks (onError / transformErrors). See RJSF docs: schema is required and onError/validation options. [1][2]
Practical options
- Guard and conditional-render:
- if (!schema) return Loading / select a schema;
- if (!schema) return
- Provide a minimal fallback schema:
- const fallback = { type: 'object', properties: {} }; <Form schema={schema || fallback} ... />
- Use onError to catch validation issues on submit:
- /* show message */} />
- Use a React ErrorBoundary for runtime exceptions coming from missing/invalid schema.
- For unsupported schema types, consider useFallbackUiForUnsupportedType to show a fallback UI instead of throwing.
References
- RJSF Form props (schema is required; onError, useFallbackUiForUnsupportedType). [1]
- RJSF Validation docs (error handling / transformErrors / meta-schemas). [2]
Would you like a tiny code example for your situation?
🏁 Script executed:
#!/bin/bash
# Check how SmartForm is used in SchemaIO
cat app/packages/core/src/plugins/SchemaIO/index.tsx | head -150Length of output: 969
🏁 Script executed:
#!/bin/bash
# Search for SmartForm prop usage patterns
grep -A5 -B5 "SmartForm\|schema\|jsonSchema" app/packages/core/src/plugins/SchemaIO/index.tsx | head -80Length of output: 969
🏁 Script executed:
#!/bin/bash
# Check if there's validation around SmartForm calls
grep -n "schema\|jsonSchema" app/packages/core/src/plugins/SchemaIO/index.tsxLength of output: 147
🏁 Script executed:
#!/bin/bash
# Check USAGE_EXAMPLES.md for guidance on required props
cat app/packages/components/src/components/SmartForm/USAGE_EXAMPLES.mdLength of output: 17807
🏁 Script executed:
#!/bin/bash
# Check README for prop requirements
cat app/packages/components/src/components/SmartForm/README.mdLength of output: 10132
🏁 Script executed:
#!/bin/bash
# Search for any tests showing expected prop combinations
find . -type f -name "*.test.ts*" -path "*SmartForm*" -exec cat {} \;Length of output: 35922
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling or guards elsewhere
grep -r "schema.*jsonSchema\|jsonSchema.*schema" app/packages/components/src/components/SmartForm/ --include="*.tsx" --include="*.ts"Length of output: 97
🏁 Script executed:
#!/bin/bash
# Look for error boundaries or validations wrapping SmartForm
grep -B10 -A10 "return.*SmartForm\|<SmartForm" app/packages/core/src/plugins/SchemaIO/index.tsxLength of output: 362
Tighten SmartFormProps to enforce a valid schema source
The review correctly identifies a real issue. Both schema and jsonSchema are optional in SmartFormProps, allowing callers to render <SmartForm /> with neither—which causes a runtime error inside RJSF rather than failing at the type level.
While SchemaIOComponent includes a guard (if (props.smartForm || props.jsonSchema)), this is insufficient because:
SmartFormcan be imported and used directly, bypassing that guard- The guard doesn't validate the presence of a schema source when
smartForm=true
The RJSF component (lines 26–30 of RJSF/index.tsx) shows the vulnerability: if props.schema is absent, it uses props.jsonSchema (potentially undefined), then passes that to the Form component, triggering a runtime error.
Implement the suggested discriminated union to enforce "exactly one of schema or jsonSchema" at compile time, preventing misconfiguration entirely.
| export interface RJSFProps { | ||
| schema?: SchemaType; | ||
| jsonSchema?: RJSFSchema; | ||
| uiSchema?: UiSchema; | ||
| data?: unknown; | ||
| onChange?: (data: unknown) => void; | ||
| onSubmit?: (data: unknown) => void; | ||
| } |
There was a problem hiding this comment.
Guard against missing schemas before rendering <Form>
Lines 25–32 derive schema/uiSchema but allow the case where both props.schema and props.jsonSchema are undefined, which would pass an undefined schema into @rjsf/mui (Lines 50–56). That is likely to throw at runtime.
Add a runtime guard (and ideally enforce via typing) so callers must provide either schema or jsonSchema, and fail fast otherwise. For example:
export default function RJSF(props: RJSFProps) {
- const { schema, uiSchema, warnings } = props.schema
+ if (!props.schema && !props.jsonSchema) {
+ console.error(
+ "[SmartForm][RJSF] Either `schema` or `jsonSchema` must be provided"
+ );
+ return null;
+ }
+
+ const { schema, uiSchema, warnings } = props.schema
? translateSchemaComplete(props.schema)
: {
schema: props.jsonSchema,
uiSchema: props.uiSchema,
warnings: [],
};Also applies to: 25-32, 50-60
app/packages/components/src/components/SmartForm/RJSF/translators/index.test.ts
Outdated
Show resolved
Hide resolved
app/packages/components/src/components/SmartForm/RJSF/translators/ui.test.ts
Show resolved
Hide resolved
| {required && ( | ||
| <span style={{ color: "error.main", marginLeft: "4px" }}>*</span> | ||
| )} |
There was a problem hiding this comment.
Fix inline style color reference.
Line 65 uses "error.main" as a CSS color value, but this is a MUI theme path string, not a valid color. Inline styles don't resolve theme tokens.
Apply this diff:
{label}
{required && (
- <span style={{ color: "error.main", marginLeft: "4px" }}>*</span>
+ <Box component="span" sx={{ color: "error.main", marginLeft: "4px" }}>*</Box>
)}
</Typography>Or use a theme hook:
+ const theme = useTheme();
...
- <span style={{ color: "error.main", marginLeft: "4px" }}>*</span>
+ <span style={{ color: theme.palette.error.main, marginLeft: "4px" }}>*</span>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsx
around lines 64 to 66, the inline style uses the string "error.main" which is a
MUI theme key, not a valid CSS color; replace this by resolving the theme color
(for example use the theme hook or MUI sx/typography props) — import and call
useTheme() and set color to theme.palette.error.main, or replace the inline
style with an sx prop like sx={{ color: 'error.main', ml: 0.5 }} on an MUI
component so the theme token is resolved instead of passing the raw string to
CSS.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (1)
140-140: Throw anErrorobject instead of a string.Throwing a string literal prevents proper error handling and loses stack trace information.
- throw "text"; + throw new Error("Unsupported attribute type: text");
♻️ Duplicate comments (5)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (1)
37-61: MakemultipleOfoptional in the type signature.The type signature declares
multipleOf: number(required), but at line 128 you're passingattributes[attr]which may not containmultipleOf. The runtime guard at line 57 handles this safely, but the type is misleading.Change the signature to:
{ ftype: string; multipleOf?: number }This issue was already flagged in a previous review.
app/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsx (1)
30-42: Add missingcomponentproperty to schema view.Based on the past review, the
componentproperty was supposed to be added to enable SchemaIO's routing (commit 859f98a), but it's not present in the current code. Without it, SchemaIO'sgetComponent()may not correctly resolve toAutocompleteView.Apply this fix:
const schemaIOSchema = { type: multiple ? "array" : "string", view: { name: "AutocompleteView", + component: "AutocompleteView", label: label || schema.title, placeholder: uiSchema?.["ui:placeholder"], readOnly: readonly || disabled, choices: choices, allow_user_input: uiSchema?.["ui:options"]?.freeSolo ?? true, allow_clearing: uiSchema?.["ui:options"]?.allowClear ?? true, allow_duplicates: uiSchema?.["ui:options"]?.allowDuplicates ?? false, }, };app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (3)
12-24: Addminimum: 0for width/height in the schema (and plumb it throughcreateInput).Width and height should not be negative; the current schema omits any lower bound, so invalid boxes can be submitted. You already centralize field definitions via
createInput, so it’s straightforward to add a minimum constraint and use it only for dimensions.Example fix:
-const createInput = (name: string) => { +const createInput = (name: string, minimum?: number) => { return { [name]: { type: "number", view: { name: "View", label: name, component: "FieldView", }, multipleOf: 0.01, + ...(minimum !== undefined ? { minimum } : {}), }, }; };Then in the schema:
dimensions: { type: "object", view: createStack(), properties: { - ...createInput("width"), - ...createInput("height"), + ...createInput("width", 0), + ...createInput("height", 0), }, },This keeps x/y unconstrained while enforcing non‑negative dimensions at the form level.
Also applies to: 26-35, 100-125
37-46: TightenCoordinatestyping or handle partials explicitly.
Coordinatesmarksx/y/width/heightas optional, but in practice the component expects fully populated numbers (after effects run) and passesstatedirectly into the form. This makes it easy to accidentally treatundefinedas a valid numeric value later.Either:
- Make the four fields required and adjust initial state to account for “no overlay yet”, or
- Keep them optional but ensure all consumers (including
onChange) defensively handle missing values instead of assuming numbers.Right now, the type does not reflect actual expectations and can mask bugs when partial objects flow through.
Also applies to: 126-127
126-143: Avoid overwriting bounds withundefinedwhen merging form data.The spread merge:
{ ...oldBounds, ...data.dimensions, ...data.position, }will overwrite valid
oldBoundsvalues withundefinedif the form sends explicitundefined(e.g., cleared but not re‑filled fields), producing invalid rectangles.Build
newBoundsfield‑by‑field with nullish coalescing so only defined values replace the old ones:- const oldBounds = overlay.getAbsoluteBounds(); - scene?.executeCommand( - new TransformOverlayCommand(overlay, overlay.id, oldBounds, { - ...oldBounds, - ...data.dimensions, - ...data.position, - }) - ); + const oldBounds = overlay.getAbsoluteBounds(); + const newBounds = { + x: data.position.x ?? oldBounds.x, + y: data.position.y ?? oldBounds.y, + width: data.dimensions.width ?? oldBounds.width, + height: data.dimensions.height ?? oldBounds.height, + }; + + scene?.executeCommand( + new TransformOverlayCommand(overlay, overlay.id, oldBounds, newBounds) + );This keeps undo working while preventing corrupted boxes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
app/packages/components/src/components/SmartForm/RJSF/index.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/templates/FieldTemplate.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsx(1 hunks)app/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsx(1 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx(5 hunks)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/SmartForm/RJSF/templates/FieldTemplate.tsxapp/packages/components/src/components/SmartForm/RJSF/index.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx
🧠 Learnings (3)
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/components/src/components/SmartForm/RJSF/index.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsxapp/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsxapp/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx
📚 Learning: 2025-10-02T21:53:53.778Z
Learnt from: tom-vx51
Repo: voxel51/fiftyone PR: 6372
File: app/packages/core/src/client/annotationClient.ts:32-46
Timestamp: 2025-10-02T21:53:53.778Z
Learning: In `app/packages/core/src/client/annotationClient.ts`, the `delta` field in `PatchSampleRequest` intentionally does not accept simple scalar values (strings, numbers, booleans). The `AttributeField` type is correctly defined as `Record<string, unknown>` to enforce that all delta entries must be structured objects, not primitives.
Applied to files:
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx
📚 Learning: 2024-07-27T04:14:08.421Z
Learnt from: sashankaryal
Repo: voxel51/fiftyone PR: 4310
File: app/packages/looker-3d/src/fo3d/Gizmos.tsx:110-0
Timestamp: 2024-07-27T04:14:08.421Z
Learning: The grid rendering logic in `Gizmos.tsx` is part of an external component and not directly modifiable within the project's codebase.
Applied to files:
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx
🧬 Code graph analysis (3)
app/packages/components/src/components/SmartForm/RJSF/index.tsx (2)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (1)
SchemaType(23-27)app/packages/components/src/components/SmartForm/RJSF/translators/index.ts (1)
translateSchema(24-42)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (4)
app/packages/core/src/plugins/SchemaIO/utils/types.ts (2)
SchemaType(23-27)NumberSchemaType(17-21)app/packages/core/src/components/Modal/Sidebar/Annotate/state.ts (2)
schema(78-94)AnnotationSchema(21-27)app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (2)
currentField(81-94)currentData(64-77)app/packages/state/src/recoil/schema.ts (1)
expandPath(469-490)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (4)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/state.ts (2)
currentOverlay(96-98)currentData(64-77)app/packages/lighter/src/overlay/BoundingBoxOverlay.ts (1)
BoundingBoxOverlay(71-812)app/packages/core/src/plugins/SchemaIO/index.tsx (1)
SchemaIOComponent(30-36)app/packages/lighter/src/commands/TransformOverlayCommand.ts (1)
TransformOverlayCommand(22-43)
⏰ 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). (12)
- GitHub Check: test-windows / test-python (windows-latest, 3.10)
- GitHub Check: test-windows / test-python (windows-latest, 3.9)
- GitHub Check: test-windows / test-python (windows-latest, 3.12)
- GitHub Check: test-windows / test-python (windows-latest, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (8)
app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/AnnotationSchema.tsx (3)
20-23: LGTM: Type imports correctly added.The
NumberSchemaTypeandSchemaTypeimports support the enhanced typing increateInput.
154-178: LGTM: Numeric parsing logic is correct.The refactored
useHandleChangescorrectly handles string-to-number conversion forFLOAT_FIELDandINT_FIELD, returningnullfor empty inputs and parsed numbers otherwise.
182-224: LGTM: SmartForm integration and simplified onChange handler.The addition of
smartForm={true}enables the new SmartForm rendering path per PR objectives. The simplifiedonChangehandler that processes all changes without pre-filtering by schema keys is correct and may actually prevent edge cases where valid changes were filtered out.app/packages/components/src/components/SmartForm/RJSF/templates/FieldTemplate.tsx (1)
9-53: LGTM! Clean implementation of FieldTemplate.The template correctly handles hidden fields while preserving classNames, delegates rendering to children, and displays errors and help text appropriately. The structure follows RJSF best practices.
app/packages/components/src/components/SmartForm/RJSF/widgets/AutoComplete.tsx (1)
22-25: LGTM on type handling for enum values.The use of
unknowntype and nullish coalescing correctly handles enum values of any type while ensuring labels are strings.app/packages/components/src/components/SmartForm/RJSF/widgets/TextWidget.tsx (1)
10-91: LGTM! Proper theme usage for required indicator.The widget correctly uses
useTheme()to accesstheme.error.mainfor the required indicator color, resolving the previous issue. The implementation properly handles text/number inputs, validation errors, and event propagation.app/packages/components/src/components/SmartForm/RJSF/index.tsx (1)
33-39: LGTM on schema translation flow.The ternary correctly delegates to
translateSchemawhen a SchemaIO schema is provided, otherwise uses the provided JSON Schema directly.app/packages/core/src/components/Modal/Sidebar/Annotate/Edit/Position.tsx (1)
66-96: Event wiring and Jotai integration look correct.Subscribing to the three
LIGHTER_EVENTSwith a single handler, re‑derivingstatefromgetAbsoluteBounds(), and pushingbounding_boxfromgetRelativeBounds()intocurrentDatais consistent and avoids stale data. Dependency array and cleanup correctly trackoverlayandscene, so there’s no obvious leak or race here.
| export default function RJSF(props: RJSFProps) { | ||
| if (!props.schema || !props.jsonSchema) { | ||
| console.log( | ||
| "[SmartForm][RJSF] Either `schema` or `jsonSchema` must be provided" | ||
| ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix inverted guard condition logic.
Line 26 uses OR (||) which incorrectly returns null when either schema is missing. The guard should ensure at least one is provided, not require both. This breaks all normal usage where only one schema type is provided.
Apply this fix:
- if (!props.schema || !props.jsonSchema) {
+ if (!props.schema && !props.jsonSchema) {
console.log(
"[SmartForm][RJSF] Either `schema` or `jsonSchema` must be provided"
);
return null;
}📝 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.
| export default function RJSF(props: RJSFProps) { | |
| if (!props.schema || !props.jsonSchema) { | |
| console.log( | |
| "[SmartForm][RJSF] Either `schema` or `jsonSchema` must be provided" | |
| ); | |
| return null; | |
| } | |
| export default function RJSF(props: RJSFProps) { | |
| if (!props.schema && !props.jsonSchema) { | |
| console.log( | |
| "[SmartForm][RJSF] Either `schema` or `jsonSchema` must be provided" | |
| ); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/RJSF/index.tsx around lines
25 to 31, the guard incorrectly uses OR so it returns null when either schema is
missing; change the check to assert that both are missing (use AND / both falsy)
before logging and returning null so the component proceeds when at least one of
props.schema or props.jsonSchema is provided.
| // Build SchemaIO-compatible schema | ||
| const schemaIOSchema = { | ||
| type: multiple ? "array" : "string", | ||
| view: { | ||
| name: "DropdownView", | ||
| label: label || schema.title, | ||
| placeholder: uiSchema?.["ui:placeholder"], | ||
| readOnly: readonly || disabled, | ||
| choices: choices, | ||
| multiple: multiple || uiSchema?.["ui:options"]?.multiple, | ||
| compact: uiSchema?.["ui:options"]?.compact, | ||
| color: uiSchema?.["ui:options"]?.color, | ||
| variant: uiSchema?.["ui:options"]?.variant, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Add missing component property to schema view.
Similar to AutoComplete, the view object needs a component property for SchemaIO's routing to work correctly.
Apply this fix:
const schemaIOSchema = {
type: multiple ? "array" : "string",
view: {
name: "DropdownView",
+ component: "DropdownView",
label: label || schema.title,
placeholder: uiSchema?.["ui:placeholder"],
readOnly: readonly || disabled,
choices: choices,
multiple: multiple || uiSchema?.["ui:options"]?.multiple,
compact: uiSchema?.["ui:options"]?.compact,
color: uiSchema?.["ui:options"]?.color,
variant: uiSchema?.["ui:options"]?.variant,
},
};🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsx
around lines 30 to 44, the view object in schemaIOSchema is missing the required
component property used by SchemaIO routing; update the view to include a
component key (e.g., component: "Dropdown") alongside name, label, placeholder,
etc., so SchemaIO can correctly map to the Dropdown component.
| const handleChange = (path: string, newValue: any) => { | ||
| onChange(newValue); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any with proper type.
The newValue parameter should be typed as unknown for type safety.
Apply this fix:
- const handleChange = (path: string, newValue: any) => {
+ const handleChange = (_path: string, newValue: unknown) => {
onChange(newValue);
};📝 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.
| const handleChange = (path: string, newValue: any) => { | |
| onChange(newValue); | |
| }; | |
| const handleChange = (_path: string, newValue: unknown) => { | |
| onChange(newValue); | |
| }; |
🤖 Prompt for AI Agents
In app/packages/components/src/components/SmartForm/RJSF/widgets/Dropdown.tsx
around lines 46 to 48, the handleChange function types its newValue as any;
change the parameter type to unknown for type safety (const handleChange =
(path: string, newValue: unknown) => { ... }). If onChange expects a narrower
type, add an explicit cast or perform a runtime type guard before calling
onChange(newValue) (or pass the casted value), and update any affected call
sites or signatures accordingly so TypeScript no longer relies on any.
What changes are proposed in this pull request?
How is this patch tested? If it is not, please explain why.
Unit tests
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.