cherrypick commits for 1.4.0 #5566
Conversation
WalkthroughThe pull request refactors the coercion logic in the SchemaIO component by moving the local Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/Input
participant SC as SchemaIOComponent
participant UT as utils/coerceValue
participant VT as shouldCoerceValue
UI->>SC: Trigger onIOChange(path, value, schema)
SC->>UT: Call coerceValue(path, value, schema)
UT->>VT: Evaluate shouldCoerceValue(path, schema)
VT-->>UT: Return coercion decision
UT-->>SC: Return coerced or original value
sequenceDiagram
participant Event as Panel State Update
participant Hook as handlePanelStateChange
participant Merge as lodash.mergeWith
Event->>Hook: Pass new state update
Hook->>Merge: Merge current & new state (custom array handling)
Merge-->>Hook: Return merged state
Hook-->>Event: Reflect updated state
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/utils/index.ts (1)
159-174: Add type annotations to coerceValue parametersThe implementation of
coerceValuelooks good and appropriately uses theshouldCoerceValuehelper. However, the function lacks type annotations for its parameters, which would improve type safety and code documentation.-export function coerceValue(path, value, schema) { +export function coerceValue(path: string, value: any, schema: SchemaType) { const { type } = schema; if (!shouldCoerceValue(path, schema)) { return value; } // coerce the value to None if it is an empty string or empty array if (type === "array" && Array.isArray(value) && value.length === 0) { return null; } if (type === "string" && value === "") { return null; } return value; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/core/src/plugins/SchemaIO/index.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/utils/index.ts(2 hunks)app/packages/operators/src/useCustomPanelHooks.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/useCustomPanelHooks.tsapp/packages/core/src/plugins/SchemaIO/utils/index.tsapp/packages/core/src/plugins/SchemaIO/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
🔇 Additional comments (6)
app/packages/operators/src/useCustomPanelHooks.ts (2)
1-1: Good addition of mergeWith importAdding
mergeWithfrom lodash is appropriate for implementing custom merge logic.
193-200: Improved state merging logic with better array handlingThe implementation now correctly handles array values by using
mergeWithwith a customizer function. Arrays will be overwritten instead of merged, which prevents unexpected merge behavior. Also, the type change fromanytounknownimproves type safety.app/packages/core/src/plugins/SchemaIO/index.tsx (2)
6-6: Good refactoring of coerceValue to a utility functionMoving the coercion logic to a separate utility module improves modularity and code organization.
24-24: Updated function call to match new signatureThe
coerceValuefunction call has been updated to include thepathparameter, which is necessary for the new implementation that considers user interactions with specific paths.app/packages/core/src/plugins/SchemaIO/utils/index.ts (2)
12-12: Good addition of isPathUserChanged importThis import is necessary for the new coercion logic that considers whether a path has been modified by user interaction.
133-157: Well-designed helper function for coercion decisionsThe
shouldCoerceValuefunction properly encapsulates the logic for determining when values should be coerced, taking into account:
- Whether the path was changed by the user
- Special handling for empty arrays and strings with matching default values
This makes the coercion logic more maintainable and explicit.
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
How is this patch tested? If it is not, please explain why.
(Details)
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