fix(web): disable the save button when creating a new item with invalid metadata #1327
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve enhancements to the end-to-end test cases for text metadata creation and editing, specifically in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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 (4)
web/e2e/project/item/metadata/text.spec.ts (2)
134-137: Test case successfully validates the save button disable functionality.The test case effectively verifies the PR objective by checking that the save button is disabled when the input exceeds the maximum length. However, consider these improvements for better test reliability and clarity:
await page.getByLabel("new text1(unique)").click(); await page.getByLabel("new text1(unique)").fill("text22"); + // Verify button is disabled due to exceeding max length of 5 await expect(page.getByRole("button", { name: "Save" })).toBeDisabled(); await page.getByLabel("new text1(unique)").fill("text2"); + // Verify button becomes enabled after fixing the value + await expect(page.getByRole("button", { name: "Save" })).toBeEnabled();Additionally, consider adding more test cases to verify other invalid metadata scenarios, such as:
- Empty required field
- Duplicate value for unique field
Line range hint
177-178: Replace hardcoded waits with explicit wait conditions.The test uses multiple
waitForTimeout(100)calls which can make the test flaky. Instead, wait for specific conditions using Playwright's built-in wait methods.Replace the hardcoded waits with appropriate wait conditions. For example:
- // eslint-disable-next-line playwright/no-wait-for-timeout - await page.waitForTimeout(100); + // Wait for the textbox value to be updated + await expect(page.getByRole('textbox')).toHaveValue('text2'); - // eslint-disable-next-line playwright/no-wait-for-timeout - await page.waitForTimeout(100); + // Wait for the arrow buttons to be clickable + await expect(page.getByRole('button', { name: 'arrow-down' })).toBeEnabled();This will make the tests more reliable and faster by only waiting as long as necessary.
Also applies to: 182-183, 187-188, 192-193
web/src/components/molecules/Content/Form/index.tsx (2)
443-454: Implementation looks good, but consider adding error loggingThe new
handleMetaValuesChangemethod correctly implements the validation logic for metadata fields when creating new items. However, consider adding error logging to help with debugging validation failures.const handleMetaValuesChange = useCallback(async () => { if (itemId) return; try { await metaForm.validateFields(); } catch (e) { if ((e as ValidateErrorEntity).errorFields.length > 0) { setIsDisabled(true); + console.error('Metadata validation failed:', e); return; } } setIsDisabled(false); }, [itemId, metaForm]);
443-454: Consider adding a debounce to validationThe validation runs on every change which might be expensive for complex metadata forms. Consider adding a debounce to improve performance.
+ import { debounce } from 'lodash'; const handleMetaValuesChange = useCallback( + debounce( async () => { if (itemId) return; try { await metaForm.validateFields(); } catch (e) { if ((e as ValidateErrorEntity).errorFields.length > 0) { setIsDisabled(true); return; } } setIsDisabled(false); }, + 300 + ), [itemId, metaForm] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/e2e/project/item/metadata/text.spec.ts(1 hunks)web/src/components/molecules/Content/Form/index.tsx(2 hunks)
🔇 Additional comments (3)
web/e2e/project/item/metadata/text.spec.ts (1)
Line range hint 6-14: Well-structured test setup and teardown.
The test file follows good practices with proper test isolation using beforeEach and afterEach hooks for setup and cleanup.
web/src/components/molecules/Content/Form/index.tsx (2)
672-676: LGTM! Form implementation correctly uses the new handler
The metadata form is properly configured to use the new handleMetaValuesChange method for validation.
443-454: Verify the save button behavior
Let's verify that the save button is properly disabled when metadata is invalid during item creation.
Also applies to: 672-676
✅ Verification successful
Based on the code analysis, I can now provide a final response regarding the save button behavior verification:
Save button correctly implements disabled state for invalid metadata
The implementation properly handles the save button's disabled state during item creation:
- The button's disabled state is controlled by
isDisabledstate:<Button disabled={isDisabled}> handleMetaValuesChangecallback correctly setsisDisabledto:truewhen metadata validation failsfalsewhen validation passes
- The validation is triggered appropriately through
metaForm.validateFields() - Initial state is properly set with
useState(!!itemId)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify the save button's disabled state
rg -A 5 "save.*button.*disabled" "**/*.{test,spec}.{ts,tsx}"
# Search for related test files
fd -g "*content*.test.{ts,tsx}" -g "*form*.test.{ts,tsx}"
Length of output: 296
Script:
#!/bin/bash
# Search for tests related to the Form component
fd "test|spec" web/src/components/molecules/Content/Form/
# Search for save button disabled state tests with proper context
rg -A 5 "save.*button.*disabled"
# Search for test files containing "form" or "content" in their name
fd "form.*\.(test|spec)\."
fd "content.*\.(test|spec)\."
# Look for form validation tests
rg -A 5 "validateFields|setIsDisabled" "web/src/components/molecules/Content/Form/"
Length of output: 6439
Script:
#!/bin/bash
# Search for the save button implementation and its disabled state
rg -A 5 "Button.*disabled={isDisabled}" web/src/components/molecules/Content/Form/
# Look for the form submission handler to understand the save flow
rg -A 10 "const handleSubmit" web/src/components/molecules/Content/Form/
# Search for the meta form initialization
rg -B 5 "metaForm = Form.useForm" web/src/components/molecules/Content/Form/
Length of output: 1479
Script:
#!/bin/bash
# Search for the save button with proper regex
rg -A 5 "Button.*disabled=\{isDisabled\}" web/src/components/molecules/Content/Form/
# Look for the form render section with the save button
rg -A 10 "return \(" web/src/components/molecules/Content/Form/
# Search for any button component with disabled prop
rg -A 5 "<Button.*disabled" web/src/components/molecules/Content/Form/
Length of output: 30253
Overview
This PR fixes to disable the save button when creating a new item with invalid metadata.
Summary by CodeRabbit
ContentFormcomponent.