korbit-ai[bot] commented on code in PR #35254:
URL: https://github.com/apache/superset/pull/35254#discussion_r2373556278
##########
superset-frontend/src/features/themes/ThemeModal.tsx:
##########
@@ -121,99 +131,160 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
} = useSingleViewResource<ThemeObject>('theme', t('theme'), addDangerToast);
// Functions
- const hide = () => {
+ const hasUnsavedChanges = useCallback(() => {
+ if (!currentTheme || !initialTheme || isReadOnly) return false;
+ return (
+ currentTheme.theme_name !== initialTheme.theme_name ||
+ currentTheme.json_data !== initialTheme.json_data
+ );
+ }, [currentTheme, initialTheme, isReadOnly]);
+
+ const hide = useCallback(() => {
onHide();
setCurrentTheme(null);
- };
+ setInitialTheme(null);
+ setShowCancelConfirmation(false);
+ }, [onHide]);
+
+ const handleCancel = useCallback(() => {
+ if (hasUnsavedChanges()) {
+ setShowCancelConfirmation(true);
+ } else {
+ hide();
+ }
+ }, [hasUnsavedChanges, hide]);
+
+ const handleConfirmCancel = useCallback(() => {
+ hide();
+ }, [hide]);
- const onSave = () => {
+ const handleKeepEditing = useCallback(() => {
+ setShowCancelConfirmation(false);
+ }, []);
+
+ const onSave = useCallback(() => {
if (isEditMode) {
// Edit
if (currentTheme?.id) {
- const update_id = currentTheme.id;
- delete currentTheme.id;
- delete currentTheme.created_by;
- delete currentTheme.changed_by;
- delete currentTheme.changed_on_delta_humanized;
-
- updateResource(update_id, currentTheme).then(response => {
- if (!response) {
- return;
- }
+ const themeData = omit(currentTheme, [
+ 'id',
+ 'created_by',
+ 'changed_by',
+ 'changed_on_delta_humanized',
+ ]);
- if (onThemeAdd) {
- onThemeAdd();
- }
+ updateResource(currentTheme.id, themeData).then(response => {
+ if (!response) return;
+ if (onThemeAdd) onThemeAdd();
hide();
});
}
} else if (currentTheme) {
// Create
createResource(currentTheme).then(response => {
- if (!response) {
- return;
- }
-
- if (onThemeAdd) {
- onThemeAdd();
- }
+ if (!response) return;
+ if (onThemeAdd) onThemeAdd();
hide();
});
}
- };
+ }, [
+ currentTheme,
+ isEditMode,
+ updateResource,
+ createResource,
+ onThemeAdd,
+ hide,
+ ]);
- const onApply = () => {
+ const handleSaveAndClose = useCallback(() => {
+ onSave();
+ setShowCancelConfirmation(false);
+ }, [onSave]);
Review Comment:
### Premature confirmation modal dismissal on save <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The `handleSaveAndClose` function immediately closes the confirmation modal
regardless of whether the save operation succeeds or fails.
###### Why this matters
If the save operation fails, the confirmation modal will be dismissed but
the user will remain in the theme modal with their changes, creating a
confusing UX where the confirmation appears to have worked but the modal didn't
actually close.
###### Suggested change ∙ *Feature Preview*
Only close the confirmation modal after a successful save by handling the
save promise:
```typescript
const handleSaveAndClose = useCallback(async () => {
try {
await onSave();
setShowCancelConfirmation(false);
} catch (error) {
// Keep confirmation modal open if save fails
}
}, [onSave]);
```
Or modify `onSave` to return a promise that resolves on success.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f28507a1-b582-4b91-af90-fa5e3470b8eb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f28507a1-b582-4b91-af90-fa5e3470b8eb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f28507a1-b582-4b91-af90-fa5e3470b8eb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f28507a1-b582-4b91-af90-fa5e3470b8eb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f28507a1-b582-4b91-af90-fa5e3470b8eb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:45e226df-7b6d-4ae9-8606-270b5f50ea78 -->
[](45e226df-7b6d-4ae9-8606-270b5f50ea78)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
