-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert optimistic rendering on negative response #7541
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This pull request addresses issue #7299 by implementing error handling and reverting optimistic updates when mutations fail across multiple hooks in the object-record module.
- Added error handling in
useCreateManyRecords
to delete optimistically created records from cache on failure - Implemented reversion of optimistic updates in
useDeleteManyRecords
anduseDeleteOneRecord
hooks - Introduced caching of records before deletion in
useDestroyManyRecords
to allow for restoration on error - Enhanced
useUpdateOneRecord
to revert cache to previous state when server errors occur - Improved consistency between UI and server state across all affected hooks
7 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings
setLoading(false); | ||
}, | ||
}) | ||
.catch((error: Error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider wrapping the error handling logic in a separate function to improve readability and reusability across similar hooks.
shouldMatchRootQueryFilter, | ||
}); | ||
|
||
setLoading(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: setLoading(false) should also be called in the catch block to ensure the loading state is reset on error.
}, | ||
}); | ||
throw error; | ||
}); | ||
|
||
return createdObject.data?.[mutationResponseField] ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: This line might throw an error if createdObject is undefined due to a caught exception. Consider adding a null check.
cachedRecords.forEach((cachedRecord) => { | ||
if (!cachedRecord) { | ||
return; | ||
} | ||
|
||
updateRecordFromCache({ | ||
objectMetadataItems, | ||
objectMetadataItem, | ||
cache: apolloClient.cache, | ||
record: { | ||
...cachedRecord, | ||
deletedAt: null, | ||
}, | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider wrapping this forEach loop in a try-catch block to handle potential errors during the revert process.
cachedRecords = records | ||
.map((record) => getRecordFromCache(record.id, cache)) | ||
.filter(isDefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: This assignment overwrites the initial cachedRecords. Consider merging instead of overwriting
|
||
const cachedRecord = getRecordFromCache(record.id, cache); | ||
const cachedRecord = getRecordFromCache(record.id, cache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This cachedRecord shadows the outer cachedRecord variable. Consider renaming to avoid confusion.
if (!isUndefinedOrNull(cachedRecord)) { | ||
triggerCreateRecordsOptimisticEffect({ | ||
cache: apolloClient.cache, | ||
objectMetadataItem, | ||
recordsToCreate: [cachedRecord], | ||
objectMetadataItems, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: cachedRecord is used here but may be undefined. Consider fetching it again in the catch block to ensure it's up-to-date.
@@ -36,34 +39,48 @@ export const useDestroyOneRecord = ({ | |||
const mutationResponseField = | |||
getDestroyOneRecordMutationResponseField(objectNameSingular); | |||
|
|||
let cachedRecord: ObjectRecord; | |||
|
|||
const destroyOneRecord = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider wrapping the entire operation in a try-catch block to handle any unexpected errors during the process.
const record = data?.[mutationResponseField]; | ||
|
||
if (!record || !cachedRecord) return; | ||
const updatedRecord = await apolloClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The error handling logic looks good, but consider extracting it into a separate function for better readability and reusability
}); | ||
|
||
throw error; | ||
}); | ||
|
||
return updatedRecord?.data?.[mutationResponseField] ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: This line might throw an error if updatedRecord is undefined. Consider adding a null check
Fixes twentyhq#7299 The changes primarily focus on ensuring that records are correctly handled in the cache and optimistic effects are reverted appropriately when mutations fail.
Fixes #7299
The changes primarily focus on ensuring that records are correctly handled in the cache and optimistic effects are reverted appropriately when mutations fail.