fix(web): handling initial values of multiple field #1345
Conversation
WalkthroughThis pull request introduces new type definitions and modifications across several TypeScript files in the web components. The changes primarily focus on enhancing type safety and flexibility for form handling, particularly in content management. New types Changes
Possibly related PRs
Suggested labels
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
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
🧹 Nitpick comments (6)
web/src/components/molecules/Content/types.ts (1)
21-21: Consider adding JSDoc comments for better documentation.While the type definition is clear, adding JSDoc comments would help other developers understand the purpose and usage of these types, especially for complex form structures.
+/** Represents a group of form values, where each key maps to a single form value or array of values */ export type FormGroupValue = Record<string, FormValue>;web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (5)
7-8: LGTM! Well-structured type handling with a minor suggestion.The valueGet function implementation is now more type-safe and handles multiple fields correctly. Consider extracting the field type mapping to a separate constant for better maintainability.
+const FIELD_TYPE_DEFAULT_VALUE_MAP = { + Select: (field: Field) => field.typeProperty?.selectDefaultValue, + Integer: (field: Field) => field.typeProperty?.integerDefaultValue, + Asset: (field: Field) => field.typeProperty?.assetDefaultValue, + Date: (field: Field) => dateConvert(field.typeProperty?.defaultValue), + default: (field: Field) => field.typeProperty?.defaultValue, +} as const; const valueGet = useCallback((field: Field) => { let result: FormValue; - switch (field.type) { - case "Select": - result = field.typeProperty?.selectDefaultValue; - break; - case "Integer": - result = field.typeProperty?.integerDefaultValue; - break; - case "Asset": - result = field.typeProperty?.assetDefaultValue; - break; - case "Date": - result = dateConvert(field.typeProperty?.defaultValue); - break; - default: - result = field.typeProperty?.defaultValue; - } + const getDefaultValue = FIELD_TYPE_DEFAULT_VALUE_MAP[field.type as keyof typeof FIELD_TYPE_DEFAULT_VALUE_MAP] + ?? FIELD_TYPE_DEFAULT_VALUE_MAP.default; + result = getDefaultValue(field); if (field.multiple && !result) { result = []; } return result; }, []);Also applies to: 371-391
408-410: Consider creating a type alias for form values record.The explicit type definition improves type safety, but readability could be enhanced.
+type FormValuesRecord = Record<string, FormValue | FormGroupValue>; -const [initialFormValues, setInitialFormValues] = useState< - Record<string, FormValue | FormGroupValue> ->({}); +const [initialFormValues, setInitialFormValues] = useState<FormValuesRecord>({});
415-421: Consider extracting the nested group values update function.The groupInitialValuesUpdate function could be moved outside for better readability and potential reuse.
+const updateGroupInitialValues = ( + group: Group, + itemGroupId: string, + initialValues: Record<string, FormValue | FormGroupValue>, + valueGetter: (field: Field) => FormValue +) => { + group?.schema?.fields?.forEach(field => { + initialValues[field.id] = { + ...(initialValues[field.id] as FormGroupValue), + ...{ [itemGroupId]: valueGetter(field) }, + }; + }); +}; const handleInitialValuesSet = async () => { const initialValues: Record<string, FormValue | FormGroupValue> = {}; - const groupInitialValuesUpdate = (group: Group, itemGroupId: string) => { - group?.schema?.fields?.forEach(field => { - initialValues[field.id] = { - ...(initialValues[field.id] as FormGroupValue), - ...{ [itemGroupId]: valueGet(field) }, - }; - }); - }; + const updateGroup = (group: Group, itemGroupId: string) => + updateGroupInitialValues(group, itemGroupId, initialValues, valueGet);
Line range hint
562-568: Improve type handling and readability in reference check.The function could be more robust with early returns and explicit type handling.
const handleCheckItemReference = useCallback( async (itemId: string, correspondingFieldId: string, groupId?: string) => { + if (!itemId || !correspondingFieldId) return false; + const initialValue = groupId - ? (initialFormValues[groupId] as FormGroupValue)[correspondingFieldId] - : initialFormValues[correspondingFieldId]; + ? (initialFormValues[groupId] as FormGroupValue | undefined)?.[correspondingFieldId] + : initialFormValues[correspondingFieldId] ?? null; - if (initialValue === itemId) { - return false; - } + if (initialValue === itemId) return false;
Line range hint
1-653: Consider splitting this large hook into smaller, focused custom hooks.This file has grown quite large and handles multiple concerns. Consider breaking it down into smaller, more focused custom hooks:
- useFormValues (form state and initialization)
- useItemOperations (create, update)
- useRequestOperations (request-related functionality)
- useNavigationHandlers (navigation-related callbacks)
This would improve maintainability, testability, and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/components/molecules/Content/types.ts(1 hunks)web/src/components/molecules/Schema/types.ts(1 hunks)web/src/components/organisms/Project/Content/ContentDetails/hooks.ts(5 hunks)web/src/components/organisms/Project/Content/ContentDetails/utils.ts(1 hunks)
🔇 Additional comments (3)
web/src/components/molecules/Content/types.ts (1)
9-19: LGTM! Comprehensive type definition for form values.
The FormValue type properly handles all possible form field values, including arrays for multiple fields and explicit null/undefined cases. The special handling for date fields with ("" | Dayjs)[] is particularly well thought out.
web/src/components/molecules/Schema/types.ts (1)
84-84: LGTM! Consistent null handling across field types.
The updates to TypeProperty properly align with the new FormValue type and support multiple field scenarios. The addition of array support for assetDefaultValue and integerDefaultValue is particularly valuable for handling multiple fields.
However, let's verify the impact of these changes on existing code:
Also applies to: 86-88
✅ Verification successful
Based on the search results, I can see that the changes to defaultValue and related properties in Schema/types.ts are consistently used throughout the codebase in:
- GraphQL type definitions that define the schema field types and their properties
- Form handling components that manage field default values
- Field rendering components that display the values
The changes are properly aligned with the type system and there are no inconsistencies in how these properties are used. The null support is consistently handled across all field types and the array support matches the multiple field scenarios.
LGTM! The changes are properly integrated across the codebase.
The type changes are well-aligned with both the GraphQL schema and the UI components that consume these types. The null and array support is consistently implemented throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing usage of these properties that might be affected
rg -A 2 "defaultValue|assetDefaultValue|selectDefaultValue|integerDefaultValue" --type ts
Length of output: 70039
web/src/components/organisms/Project/Content/ContentDetails/utils.ts (1)
Line range hint 5-11: LGTM! Proper null handling in date conversion.
The updated signature properly handles null values while maintaining existing functionality for arrays and single values. The function correctly returns empty string for null/undefined cases, which is consistent with the form value handling requirements.
Let's verify the usage of this function across the codebase:
✅ Verification successful
Let's analyze the usage of dateConvert further by checking the type definitions and imports:
The dateConvert function update is safe and properly used across the codebase
The verification shows that:
- The function is imported and used in two locations:
RequestDetails/utils.tsandContentDetails/hooks.ts - All usages are for date type fields and properly handle the nullable values
- The ItemValue type supports both single values and arrays, which aligns with the function's implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for dateConvert usage to ensure compatibility
rg -A 3 "dateConvert\(" --type ts
Length of output: 2966
Script:
#!/bin/bash
# Check imports of dateConvert
rg "import.*dateConvert" --type ts
# Check ItemValue type definition
rg "type ItemValue" --type ts -A 3
Length of output: 673
Overview
What I've done
Capture
The saving button behaved incorrectly when creating a new item with multiple field.
2024-12-17.12.15.46.mov
Summary by CodeRabbit
New Features
FormValueandFormGroupValuefor enhanced data structure handling.Bug Fixes
TypePropertyto allownullvalues for several properties, improving data handling flexibility.Improvements
dateConvertfunction to acceptnullvalues, improving robustness.