fix(web): updating metadata from the table is rejected #1260
Conversation
…x-web/metadata-from-table
WalkthroughThe changes in this pull request enhance the error handling and data management strategies within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentDetails
participant API
User->>ContentDetails: Create Item
ContentDetails->>API: Call createItem
API-->>ContentDetails: Return success
ContentDetails->>ContentDetails: Notify user of success
ContentDetails->>ContentDetails: Refetch GetRequests
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Outside diff range and nitpick comments (4)
web/src/components/molecules/Content/types.ts (1)
20-24: LGTM! Consider simplifying thefieldsproperty type.The new
Metadatatype is well-structured and provides good flexibility for metadata representation. However, thefieldsproperty type could be simplified.Consider changing the
fieldsproperty type fromItemField[] | undefined | nullto justItemField[] | null. In TypeScript,undefinedis often interchangeable withnull, and using both can lead to unnecessary type checking. Here's the suggested change:export type Metadata = { id?: string; version: string; - fields: ItemField[] | undefined | null; + fields: ItemField[] | null; };This change would simplify the type without losing any meaningful functionality.
web/e2e/project/item/metadata/update.spec.ts (2)
29-30: Approved: Changes align with PR objective and improve test robustness.The additions to click the switch and close the notification before checking the attribute state improve the test's accuracy by simulating real user interactions. The change in the expected state from "true" to "false" reflects the updated behavior after these interactions.
Consider adding a brief comment explaining why the expected state is now "false" after the interactions. This would improve the test's readability and make it easier for other developers to understand the expected behavior.
Also applies to: 34-34
Line range hint
1-43: Summary: Test improvements align with PR objective, but consistency should be verified.The changes to the "Updating metadata added later from table has succeeded" test case improve its accuracy by simulating more realistic user interactions. This aligns well with the PR objective of fixing issues with metadata updates.
However, the discrepancy between the expected outcomes in the table update and edit page update test cases raises a question about consistency.
To ensure the robustness of these tests and the underlying functionality:
- Verify if the different expected outcomes for table updates vs. edit page updates are intentional.
- If not intentional, update the edit page test case to match the new expected behavior.
- Consider adding comments to explain any intentional differences in behavior between these two update methods.
- If possible, add more test cases to cover edge cases and ensure comprehensive coverage of the metadata update functionality.
These steps will help maintain the integrity of the tests and provide clear documentation for future developers working on this feature.
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (1)
Line range hint
240-275: Improved error handling in handleItemCreate functionThe error handling in the
handleItemCreatefunction has been significantly improved. It now provides separate error handling for metadata item creation and main item creation, with appropriate user notifications for both success and failure scenarios. This enhancement improves the overall robustness and user experience of the function.Consider extracting the error notification logic into a separate function to reduce code duplication. For example:
const notifyError = (message: string) => { Notification.error({ message: t(message) }); }; // Usage if (metaItem.errors || !metaItem.data?.createItem) { notifyError("Failed to create item."); return; }This would make the code more DRY and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/e2e/project/item/metadata/update.spec.ts (1 hunks)
- web/src/components/molecules/Content/types.ts (2 hunks)
- web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (1 hunks)
- web/src/components/organisms/Project/Content/ContentList/hooks.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (7)
web/src/components/molecules/Content/types.ts (2)
37-37: LGTM! Good refactoring of theItemtype.The update to the
Itemtype, using the newly definedMetadatatype for itsmetadataproperty, is a good refactoring decision. This change improves code readability and maintainability by centralizing the metadata structure definition.This change aligns well with the PR objectives of addressing metadata-related issues. It provides a more structured approach to handling metadata, which could help prevent bugs like the one described in the PR summary.
Line range hint
20-37: Good refactoring, but verify impact on metadata update functionality.The introduction of the
Metadatatype and its integration into theItemtype is a positive step towards better metadata management. These changes provide a more structured approach to handling metadata, which aligns with the PR's objective of fixing metadata update issues.However, it's important to note that type changes alone may not fully resolve the issue of rejected metadata updates described in the PR summary.
To ensure these changes effectively address the reported bug, please run the following verification steps:
- Create a model without any metadata fields.
- Create an item for this model.
- Add a metadata field to the model.
- Update the metadata of the previously saved item from the table.
- Attempt to update the same metadata again in the same manner.
If the second update (step 5) is successful, it indicates that these changes have resolved the issue. If not, further investigation may be needed in the components or hooks that handle the actual metadata update process.
This script will help identify components or hooks that might need additional changes to fully resolve the metadata update issue.
web/e2e/project/item/metadata/update.spec.ts (1)
Line range hint
37-43: Verify consistency between table and edit page update tests.While changes were made to the "Updating metadata added later from table has succeeded" test, the "Updating metadata added later from edit page has succeeded" test remains unchanged. The expected state of the switch is different between these two tests ("false" for table update, "true" for edit page update).
Please confirm if this difference in behavior between updating from the table and the edit page is intentional. If not, consider updating this test case to match the new expected behavior.
web/src/components/organisms/Project/Content/ContentDetails/hooks.ts (2)
Line range hint
445-449: Improved error handling and refetchQueries updateThe
handleRequestCreateandhandleRequestUpdatefunctions have been enhanced with proper error handling and user notifications. This improvement provides better feedback to users and increases the robustness of the request creation and update processes.The
refetchQueriesforcreateRequestMutationhas been updated to["GetModalRequests", "GetItem"]. This change might affect data consistency across the application.To ensure this change doesn't introduce any data inconsistencies, please verify the following:
- Check if
GetModalRequestsis sufficient to update all necessary UI components after request creation.- Confirm that
GetItemis needed and correctly updates the item details after request creation.Run the following script to help with this verification:
#!/bin/bash # Description: Verify the usage of GetModalRequests and GetItem queries # Check for components using GetModalRequests echo "Components using GetModalRequests:" rg --type typescript 'useQuery.*GetModalRequests' # Check for components using GetItem echo "Components using GetItem:" rg --type typescript 'useQuery.*GetItem' # Check for other places where requests are created or updated echo "Other places handling request creation or update:" rg --type typescript 'createRequest|updateRequest'Review the results to ensure that all necessary components are updated after request creation and that no unintended side effects are introduced by this change.
Also applies to: 465-484
234-234: Consider the impact of removing "SearchItem" from refetchQueriesThe
refetchQueriesparameter for thecreateItemmutation has been updated to only include"GetRequests". This change may improve performance by reducing unnecessary refetches. However, it's important to ensure that this doesn't negatively impact the immediate visibility of newly created items in search results.To verify the impact of this change, we can check if there are any other mechanisms in place to update the search results after item creation. Run the following script:
web/src/components/organisms/Project/Content/ContentList/hooks.ts (2)
13-13: ImportingMetadatais appropriateThe
Metadatatype is correctly imported for use in the component.
44-44: ImportingItemFieldInputis appropriateThe
ItemFieldInputtype is correctly imported and will be used in GraphQL mutations.
web/src/components/organisms/Project/Content/ContentList/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/components/organisms/Project/Content/ContentList/hooks.ts
Outdated
Show resolved
Hide resolved
web/src/components/organisms/Project/Content/ContentList/hooks.ts
Outdated
Show resolved
Hide resolved
Overview
This PR fixes the bug that updating metadata from the table is rejected.
How to reproduce this bug
Summary by CodeRabbit
New Features
Bug Fixes
Refactor