fix(web): validate values and avoid redundant queries when updating metadata from table #1338
Conversation
…om/reearth/reearth-cms into fix-web/update-metadata-from-table
WalkthroughThe pull request introduces significant modifications to various components, primarily focusing on enhancing notification handling and validation logic. 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 (6)
web/e2e/common/notification.ts (1)
6-13: LGTM! More robust notification validation.The switch from text-based to attribute-based validation is a good improvement that:
- Eliminates i18n-related flakiness
- Provides more precise validation
- Adds verification that the alert is properly hidden
Consider adding a timeout parameter to handle slow UI updates:
-export async function closeNotification(page: Page, isSuccess = true) { +export async function closeNotification(page: Page, isSuccess = true, timeout = 5000) { const text = isSuccess ? "check-circle" : "close-circle"; - await expect(page.getByRole("alert").last().getByRole("img")).toHaveAttribute("aria-label", text); + await expect(page.getByRole("alert").last().getByRole("img")).toHaveAttribute("aria-label", text, { timeout }); await page .locator(".ant-notification-notice") .last() .locator(".ant-notification-notice-close") .click(); - await expect(page.getByRole("alert").last()).toBeHidden(); + await expect(page.getByRole("alert").last()).toBeHidden({ timeout }); }web/e2e/project/item/metadata/update.spec.ts (1)
33-36: Enhance test coverage for metadata updates.While the added validation is good, consider strengthening the test by:
- Verifying the persisted state after page reload
- Adding negative test cases (invalid inputs)
- Testing concurrent updates
Example enhancement:
test("Updating metadata added later from table has succeeded", async ({ page }) => { // ... existing code ... // Verify persisted state await page.reload(); await expect(page.getByLabel("boolean")).toHaveAttribute("aria-checked", "true"); // Test concurrent updates const promises = [ page.getByRole("switch").click(), page.getByRole("switch").click(), ]; await Promise.all(promises); await expect(page.getByLabel("boolean")).toHaveAttribute("aria-checked", "false"); });web/src/components/molecules/Content/RenderField/index.tsx (1)
Line range hint
13-17: Add validation to prevent redundant updates.The update callback is passed directly without any validation, which could lead to unnecessary backend queries.
Consider memoizing the update callback and adding validation:
+import { useCallback, useMemo } from "react"; + export const renderField = ( el: { props: { children: string | string[] } }, field: Field, update?: (value?: string | string[] | boolean, index?: number) => void, ) => { const value = el.props.children; const items = Array.isArray(value) ? value : [value]; + + const handleUpdate = useCallback((newValue?: string | string[] | boolean, index?: number) => { + if (JSON.stringify(newValue) === JSON.stringify(value)) return; + update?.(newValue, index); + }, [value, update]);web/src/components/molecules/Content/RenderField/ItemFormat.tsx (2)
34-44: Consider adding validation in handleTextBlurWhile the implementation correctly prevents unnecessary updates, consider adding validation for the text input, similar to the URL validation.
const handleTextBlur = useCallback( (e: FocusEvent<HTMLInputElement>) => { const value = e.target.value; if (itemState === value) { return; } + if (field.typeProperty?.maxLength && value.length > field.typeProperty.maxLength) { + Notification.error({ + message: t("Maximum length error: Character count in {{field}} field exceeds the maximum number!", + { field: field.name }) + }); + return; + } update?.(value, index); setItemState(value); }, - [index, itemState, update], + [index, itemState, update, field, t], );
47-61: Add URL sanitization before validationThe URL validation is good, but consider sanitizing the URL input to handle common issues like leading/trailing spaces.
const handleUrlBlur = useCallback( (e: FocusEvent<HTMLInputElement>) => { - const value = e.target.value; + const value = e.target.value.trim(); if (itemState === value) { setIsEditable(false); return; } if (value && !validateURL(value)) { Notification.error({ message: t("Please input a valid URL") }); return; } update?.(value, index); setItemState(value); setIsEditable(false); }, [index, itemState, t, update], );web/src/components/organisms/Project/Content/ContentList/hooks.ts (1)
243-274: Consider extracting validation logic into a separate function.The validation implementation is correct and thorough, handling both required fields and maximum length constraints. However, the validation logic could be extracted into a separate function for better maintainability and reusability.
Consider refactoring the validation logic like this:
+const validateMetadataFields = ( + fields: ItemField[], + metaFieldsMap: Map<string, any> +): { requiredErrorFields: string[], maxLengthErrorFields: string[] } => { + const requiredErrorFields: string[] = []; + const maxLengthErrorFields: string[] = []; + + fields.forEach(field => { + const metaField = metaFieldsMap.get(field.schemaFieldId); + const fieldValue = field.value; + + if (metaField?.required) { + if (Array.isArray(fieldValue)) { + if (fieldValue.every(v => checkIfEmpty(v))) { + requiredErrorFields.push(metaField.key); + } + } else if (checkIfEmpty(fieldValue)) { + requiredErrorFields.push(metaField.key); + } + } + + const maxLength = metaField?.typeProperty?.maxLength; + if (maxLength) { + if (Array.isArray(fieldValue)) { + if (fieldValue.some(v => typeof v === "string" && v.length > maxLength)) { + maxLengthErrorFields.push(metaField.key); + } + } else if (typeof fieldValue === "string" && fieldValue.length > maxLength) { + maxLengthErrorFields.push(metaField.key); + } + } + }); + + return { requiredErrorFields, maxLengthErrorFields }; +};Then use it in the handleMetaItemUpdate function:
if (metadata?.fields && metadata.id) { - const requiredErrorFields: string[] = []; - const maxLengthErrorFields: string[] = []; + const { requiredErrorFields, maxLengthErrorFields } = validateMetadataFields( + metadata.fields, + metaFieldsMap + ); const fields = metadata.fields.map(field => { // ... existing field mapping logic });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
web/e2e/common/notification.ts(1 hunks)web/e2e/project/item/metadata/tag.spec.ts(1 hunks)web/e2e/project/item/metadata/update.spec.ts(1 hunks)web/src/components/molecules/Content/RenderField/ItemFormat.tsx(3 hunks)web/src/components/molecules/Content/RenderField/index.tsx(1 hunks)web/src/components/organisms/Project/Content/ContentList/hooks.ts(2 hunks)web/src/i18n/translations/en.yml(2 hunks)web/src/i18n/translations/ja.yml(2 hunks)
🔇 Additional comments (9)
web/src/components/molecules/Content/RenderField/index.tsx (1)
49-52:
Verify impact of removing field.multiple check.
The simplified condition could allow updates on multiple fields that weren't previously allowed. This might lead to unexpected behavior.
Let's check for any existing multiple field usage:
Consider adding validation before the update:
if ((field.type === "Text" || field.type === "Date" || field.type === "URL") && update) {
+ if (field.multiple) {
+ console.warn(`Updating multiple ${field.type} fields directly is not supported`);
+ return <span>-</span>;
+ }
return <ItemFormat item="" field={field} update={update} />;
}web/src/components/molecules/Content/RenderField/ItemFormat.tsx (2)
11-11: LGTM: New imports enhance error handling and internationalization
The additions of Notification and useT support proper error handling with localized messages.
Also applies to: 17-17
138-138: LGTM: Tooltip prop update
The change from showArrow to arrow={false} aligns with the component's API.
web/e2e/project/item/metadata/tag.spec.ts (1)
76-76: LGTM: Enhanced test reliability with notification handling
The addition of closeNotification calls after actions ensures a clean state for subsequent test steps.
Also applies to: 77-77
✅ Verification successful
Based on the extensive search results, I can see that closeNotification is consistently used across many E2E test files after actions that trigger notifications, such as saving, creating, updating, or deleting items. The pattern shows that closeNotification is called after operations that modify state to ensure a clean test environment.
LGTM: Proper notification handling pattern verified
The codebase shows consistent usage of closeNotification after state-modifying operations across E2E tests, confirming this is an established pattern for maintaining test reliability. The addition of this call in the reviewed file follows the same best practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent notification handling across all E2E tests
rg -l 'closeNotification' 'web/e2e/**/*.ts' | while read -r file; do
echo "=== $file ==="
rg -A 2 'click\(' "$file" | rg -v 'closeNotification' || true
done
Length of output: 114
Script:
#!/bin/bash
# Let's first verify if the file exists and check its content
fd -e ts -p 'web/e2e'
# Then search for closeNotification usage
rg 'closeNotification' -l
# Finally, let's look for click patterns followed by notification handling
rg -A 2 'click\(' 'web/e2e'
Length of output: 110251
web/src/i18n/translations/en.yml (2)
488-489: LGTM: Clear and well-structured error messages
The new error messages use proper interpolation syntax with {{field}} and provide clear feedback to users.
521-521: LGTM: Consistent success message
The updated success message maintains consistency with other success messages in the application.
web/src/components/organisms/Project/Content/ContentList/hooks.ts (2)
5-5: LGTM! Good reuse of existing validation utility.
The addition of checkIfEmpty utility function promotes code reuse and consistency in validation logic across the application.
232-235: LGTM! Improved field value handling.
The changes properly handle different field types:
- Tag type fields are handled using
selectedTagIdsGet - Empty values are handled consistently
- Array and single value fields are handled appropriately
web/src/i18n/translations/ja.yml (1)
488-489: LGTM! Clear and consistent error messages.
The new translations:
- Provide clear feedback for validation errors
- Use proper placeholders for field names
- Maintain consistency with existing message patterns
Also applies to: 521-521
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/src/components/organisms/Project/Content/ContentList/hooks.ts (2)
228-243: Field value normalization logic needs refactoringThe field value normalization logic is complex and handles multiple cases. Consider extracting this into a separate function for better maintainability.
+ const normalizeFieldValue = (field: ItemField, value?: string | string[] | boolean | boolean[], index?: number) => { + if (Array.isArray(field.value)) { + if (field.type === "Tag") { + const tags = metaField?.typeProperty?.tags; + return tags ? selectedTagIdsGet(value as string[], tags) : []; + } + field.value[index ?? 0] = value === "" ? undefined : value; + return field.value; + } + return value ?? ""; + };
266-274: Consider grouping error notificationsMultiple individual notifications might overwhelm the user. Consider grouping them into a single notification with a list of errors.
- requiredErrorFields.forEach(field => { - Notification.error({ message: t("Required field error", { field }) }); - }); - maxLengthErrorFields.forEach(field => { - Notification.error({ message: t("Maximum length error", { field }) }); - }); + if (requiredErrorFields.length || maxLengthErrorFields.length) { + const errors = [ + ...requiredErrorFields.map(field => t("Required field error", { field })), + ...maxLengthErrorFields.map(field => t("Maximum length error", { field })) + ]; + Notification.error({ + message: t("Validation Errors"), + description: errors.join('\n') + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/components/organisms/Project/Content/ContentList/hooks.ts(2 hunks)
🔇 Additional comments (2)
web/src/components/organisms/Project/Content/ContentList/hooks.ts (2)
5-5: LGTM: Import statement is correctly placed
The import of checkIfEmpty utility aligns with the PR's objective of implementing value validation.
232-235: Verify tag handling edge cases
The tag handling logic needs verification for edge cases, such as empty arrays or invalid tag IDs.
✅ Verification successful
Let me check the implementation of selectedTagIdsGet to verify the edge case handling.
Tag handling is properly implemented for edge cases
The implementation of selectedTagIdsGet in web/src/components/molecules/Content/utils.ts already handles edge cases appropriately:
- Empty arrays: When
value.lengthis 0, it returns an empty array[] - Invalid tag IDs: The
filteroperation ensures only valid tags (those present in thetagsarray) are included - Null/undefined tags: The code checks for
tagsexistence before callingselectedTagIdsGet
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Tag field usage and potential edge cases
ast-grep --pattern 'type === "Tag"'
rg -A 5 'selectedTagIdsGet'
Length of output: 2670
Script:
#!/bin/bash
# Get the full implementation of selectedTagIdsGet and surrounding code
ast-grep --pattern 'export const selectedTagIdsGet = $_'
# Check for Tag type definition
rg -A 5 'type Tag'
Length of output: 3862
| if (metaField?.required) { | ||
| if (Array.isArray(fieldValue)) { | ||
| if (fieldValue.every(v => checkIfEmpty(v))) { | ||
| requiredErrorFields.push(metaField.key); | ||
| } | ||
| } else if (checkIfEmpty(fieldValue)) { | ||
| requiredErrorFields.push(metaField.key); | ||
| } | ||
| } | ||
| const maxLength = metaField?.typeProperty?.maxLength; | ||
| if (maxLength) { | ||
| if (Array.isArray(fieldValue)) { | ||
| if (fieldValue.some(v => typeof v === "string" && v.length > maxLength)) { | ||
| maxLengthErrorFields.push(metaField.key); | ||
| } | ||
| } else if (typeof fieldValue === "string" && fieldValue.length > maxLength) { | ||
| maxLengthErrorFields.push(metaField.key); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validation logic could be simplified
The validation checks are nested deeply and handle multiple cases. Consider extracting these into separate validation functions for better readability and maintainability.
+ const validateRequiredField = (fieldValue: any, metaField?: MetadataField) => {
+ if (!metaField?.required) return true;
+ if (Array.isArray(fieldValue)) {
+ return !fieldValue.every(v => checkIfEmpty(v));
+ }
+ return !checkIfEmpty(fieldValue);
+ };
+ const validateMaxLength = (fieldValue: any, maxLength?: number) => {
+ if (!maxLength) return true;
+ if (Array.isArray(fieldValue)) {
+ return !fieldValue.some(v => typeof v === "string" && v.length > maxLength);
+ }
+ return !(typeof fieldValue === "string" && fieldValue.length > maxLength);
+ };Committable suggestion skipped: line range outside the PR's diff.
Overview
This PR fixes to validate values and avoid redundant queries when updating metadata from table.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates aim to enhance user experience by providing clearer feedback and ensuring data integrity during interactions.