fix(web): change detection logic of schema setting #1334
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (2)
web/src/components/molecules/Schema/FieldModal/hooks.ts (2)
260-263: Consider extending array comparison logic to other array fieldsGood job on fixing the change detection for
supportedTypesarray reordering! However, to prevent similar issues, consider:
- Applying the same sorting logic to other array fields that might have order-independent values
- Using non-mutating sort operations to avoid potential side effects
- if (key === "supportedTypes" && Array.isArray(value) && Array.isArray(defaultValue)) { - changedValue = [...value].sort(); - defaultValue = [...defaultValue].sort(); + // Add more array fields that should ignore order + const orderIndependentFields = ["supportedTypes", "values", "tags"]; + if (orderIndependentFields.includes(key) && Array.isArray(value) && Array.isArray(defaultValue)) { + changedValue = [...value].slice().sort(); + defaultValue = [...defaultValue].slice().sort();
Line range hint
417-456: Enhance validation functions with type safety and performance improvementsThe validation logic is well-structured but could be enhanced:
- Add explicit type annotations for better type safety
- Simplify empty value checks
- Optimize duplicate detection using Set
- const emptyValidator = useCallback(async (values?: string[]) => { + const emptyValidator = useCallback(async (values?: readonly string[]) => { if (values) { - const indexes = values - .map((value: string, index: number) => value.length === 0 && index) - .filter(value => typeof value === "number"); + const indexes = values + .map((value, index) => (!value.trim() ? index : -1)) + .filter(index => index !== -1); setEmptyIndexes(indexes); if (indexes.length) { return Promise.reject(); } } }, []); - const duplicatedValidator = useCallback(async (values?: string[]) => { + const duplicatedValidator = useCallback(async (values?: readonly string[]) => { if (values) { + const seen = new Set<string>(); const indexes = values - .map((value: string, selfIndex: number) => { - if (!value) return; - const index = values.findIndex(v => v === value); - return index < selfIndex && selfIndex; + .map((value, index) => { + if (!value.trim()) return -1; + return seen.has(value) ? index : (seen.add(value), -1); }) - .filter(value => typeof value === "number"); + .filter(index => index !== -1); setDuplicatedIndexes(indexes); if (indexes.length) { return Promise.reject(); } } }, []);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/components/molecules/Schema/FieldModal/hooks.ts (1)
Line range hint
442-476: Consider enhancing validation with specific error messages and memoization.The validation functions are well-structured but could be improved:
- Add specific error messages for better UX
- Memoize validation functions to prevent unnecessary recalculations
Apply this diff to enhance the validation:
- const emptyValidator = useCallback(async (values?: string[]) => { + const emptyValidator = useCallback(async (values?: string[], fieldName?: string) => { if (values) { const indexes = values .map((value: string, index: number) => value.length === 0 && index) .filter(value => typeof value === "number"); setEmptyIndexes(indexes); if (indexes.length) { - return Promise.reject(); + return Promise.reject(`${fieldName || 'Values'} cannot be empty`); } } - }, []); + }, []); // Already memoized correctly - const duplicatedValidator = useCallback(async (values?: string[]) => { + const duplicatedValidator = useCallback(async (values?: string[], fieldName?: string) => { if (values) { const indexes = values .map((value: string, selfIndex: number) => { if (!value) return; const index = values.findIndex(v => v === value); return index < selfIndex && selfIndex; }) .filter(value => typeof value === "number"); setDuplicatedIndexes(indexes); if (indexes.length) { - return Promise.reject(); + return Promise.reject(`${fieldName || 'Values'} contain duplicates`); } } - }, []); + }, []); // Already memoized correctly const errorIndexes = useMemo( () => new Set([...emptyIndexes, ...duplicatedIndexes]), [duplicatedIndexes, emptyIndexes], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/components/molecules/Schema/FieldModal/hooks.ts(1 hunks)
🔇 Additional comments (2)
web/src/components/molecules/Schema/FieldModal/hooks.ts (2)
270-272: 🛠️ Refactor suggestion
Enhance type safety and error handling.
The current implementation could benefit from additional type safety and error handling:
- Add a guard clause for empty changedValues
- Use optional chaining for safer defaultValue access
Apply this diff to improve the code:
- const [key, value] = Object.entries(changedValues)[0] as [keyof FormTypes, unknown];
- let changedValue = value;
- let defaultValue = defaultValueRef.current?.[key];
+ const entries = Object.entries(changedValues);
+ if (entries.length === 0) return;
+
+ const [key, value] = entries[0] as [keyof FormTypes, unknown];
+ let changedValue = value;
+ let defaultValue = defaultValueRef.current?.[key];273-276: LGTM: Array sorting ensures consistent change detection.
The added sorting logic for supportedTypes arrays ensures that reordering of values is not incorrectly detected as a change. This fixes the original issue mentioned in the PR description.
Let's verify the change detection behavior:
✅ Verification successful
Let me analyze the results and generate the final response.
Array sorting for supportedTypes is correctly implemented and no other similar cases need attention
The codebase scan reveals that:
- The
supportedTypesarray comparison is unique to the field modal component - Other array comparisons in the codebase already handle array ordering properly:
- In
Request/Details/SidebarWrapper.tsx, reviewer arrays are sorted before comparison - Other array comparisons use
JSON.stringifywithemptyConvertutility for consistent comparison
- In
The sorting implementation is appropriate for this specific use case and doesn't need to be replicated elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other array-type fields that might need similar sorting
# Look for field type definitions and array comparisons
# Search for array type definitions
rg -A 2 "type.*=.*\[\]"
# Search for array comparisons
rg "JSON\.stringify\(.*\)"
Length of output: 4476
Overview
This PR fixes the change detection logic of schema setting.
This fix was added because the change was not detected when the order of multiple default values or options was reordered.
Summary by CodeRabbit
New Features
Bug Fixes
supportedTypesfield to ensure accurate behavior.Improvements
FieldModalcomponent.