-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ui): temp fix added for rjsf schema anyof type. #722
fix(ui): temp fix added for rjsf schema anyof type. #722
Conversation
WalkthroughThis pull request introduces improvements to JSON schema handling in the UI components. A new utility function Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ui/src/components/SchemaForm/patchSchemaTypes.ts (4)
4-5
: Typographical fix suggestion in comments
There's a minor spelling mistake in the comment: "We should regualry check this issue."-// We should regualry check this issue... +// We should regularly check this issue...
16-24
: Performance concern: Avoid spread in reduce accumulator
When using the spread operator on an accumulator in a reduce callback, the time complexity becomes O(n²) for large inputs. Consider using an imperative approach to append new entries to the accumulator instead.Example fix:
- newSchema.properties = Object.entries(newSchema.properties).reduce( - (acc, [key, value]) => ({ - ...acc, - [key]: patchAnyOfType(value), - }), - {}, - ); + const props: NonNullable<JSONSchema7["properties"]> = {}; + for (const [key, value] of Object.entries(newSchema.properties)) { + props[key] = patchAnyOfType(value); + } + newSchema.properties = props;🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Avoid the use of spread (
...
) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce
) because it causes a time complexity ofO(n^2)
.
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
26-34
: Performance concern: Avoid spread in reduce accumulator
Similarreduce
usage appears for definitions. Consider using the same approach as above for a more efficient solution.Example fix:
- newSchema.definitions = Object.entries(newSchema.definitions).reduce( - (acc, [key, value]) => ({ - ...acc, - [key]: patchAnyOfType(value), - }), - {}, - ); + const defs: NonNullable<JSONSchema7["definitions"]> = {}; + for (const [key, value] of Object.entries(newSchema.definitions)) { + defs[key] = patchAnyOfType(value); + } + newSchema.definitions = defs;🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the use of spread (
...
) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce
) because it causes a time complexity ofO(n^2)
.
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
36-47
: Avoid using the delete operator to remove properties
Using the delete operator can have performance implications and affect object shape optimizations. Consider assigning to undefined instead or restructuring your logic to skip the property.Example fix:
- delete newSchema.anyOf; + newSchema.anyOf = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/components/SchemaForm/Templates/ArrayFieldItemTemplate.tsx
(1 hunks)ui/src/components/SchemaForm/patchSchemaTypes.ts
(1 hunks)ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx
(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/src/components/SchemaForm/Templates/ArrayFieldItemTemplate.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/components/SchemaForm/patchSchemaTypes.ts
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 19-19: Avoid the use of spread (...
) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce
) because it causes a time complexity of O(n^2)
.
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
[error] 29-29: Avoid the use of spread (...
) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce
) because it causes a time complexity of O(n^2)
.
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (3)
ui/src/components/SchemaForm/patchSchemaTypes.ts (1)
49-56
: Logic confirmed for patching array items
This recursive approach to patch items (whether it's an array or single schema) is valid and clear.
ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (2)
34-41
: Maintain clarity on temporary patch usage
This segment correctly applies the temporary patch to fix the known RJSF issue. Ensure you remove or update this patch once the upstream library provides the official fix.
Line range hint 74-79
: Schema form integration looks solid
The usage of patchedSchema in the SchemaForm ensures that the custom fix seamlessly addresses the RJSF anyOf issue without disturbing other logic.
Overview
react-jsonschema-form
any of
was not working as intended and if type "null" was set then the parameter would not work as intended.This is a known issue and is actively being worked on: rjsf-team/react-jsonschema-form#4380
Seems like currently two solutions:
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
This temporary fix and we should update react-jsonschema-form when the issue is patched.
Summary by CodeRabbit
New Features
anyOf
types.ParamEditor
component to utilize the new schema processing logic.Bug Fixes
ArrayFieldItemTemplate
to address potential styling issues with icons.Documentation