Skip to content
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

fix(useToastHook): don't re-render text field on focus #1341

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Jul 13, 2023

Problem

We have a bug in production, see video here:

Screen.Recording.2023-07-13.at.12.03.25.PM.mov

This is not because of design system, I was able to replicate this even with chakra ui's toast. This seems to suggest the bug lies in how we are using this hook. video here:

Screen.Recording.2023-07-13.at.12.17.52.PM.mov

Solution

I think I know the cause for this, please feel free to correct me on this ya.

  1. This does not happen at all the places of the code base.
    eg.
Screen.Recording.2023-08-03.at.12.24.35.PM.mov
  1. There exists a useEffect in Settings.tsx that states
  // Reset cache on success
  useEffect(() => {
    if (isSuccess) {
      // Update to the new form state
      initialSettings.current = getValues()
      reset(settings)
    }
  }, [isSuccess, getValues, reset, settings])

  useEffect(() => {
    if (isSuccess) {
      successToast({
        id: "update-settings-success",
        title: "Success!",
        description: "Successfully updated settings!",
      })
    }
  }, [isSuccess, successToast])

I think there are unintended side effects here as the useUpdateSettings hook gives us, which then leads to the model being opened again. This bug is also so far observable when using in the settings page, which this PR solely attempts to solve.

I am ok to revert this if this should not be the best practice to do this, but I feel this should still go in to at least fix the bug in prod first.

Tests

https://github.com/isomerpages/isomercms-frontend/assets/42832651/5a973b89-473f-4757-83eb-ebf2e5b9fdfa
Should be similar to what is shown in the video above.

  • open up settings page
  • change anything in the site desc and click save
  • wait for modal to pop out and close
  • change anything in the text field
  • assert that modal does not pop out again

@kishore03109 kishore03109 requested a review from a team July 13, 2023 04:11
@kishore03109 kishore03109 temporarily deployed to staging July 13, 2023 04:11 — with GitHub Actions Inactive
@kishore03109 kishore03109 temporarily deployed to staging July 13, 2023 04:33 — with GitHub Actions Inactive
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

successToast isnt' the hook - useSuccessToast is, and that's the one htat calls useXxxToast under the hood. this is called at top level so it should be ok? i can pair with you on this

@seaerchin
Copy link
Contributor

hello, can we close this if it's not being worked on ah? i can take this up on fri

@kishore03109 kishore03109 marked this pull request as draft August 3, 2023 04:17
@kishore03109 kishore03109 requested a review from a team August 3, 2023 04:35
@kishore03109 kishore03109 changed the title fix(useToastHook): make sure hook is not called conditionally fix(useToastHook): don't re-render text field on focus Aug 3, 2023
@kishore03109 kishore03109 marked this pull request as ready for review August 3, 2023 04:36
@kishore03109 kishore03109 requested a review from seaerchin August 3, 2023 05:09
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm ok with this for now but i think the root cause is how our errorToast/successToast is defined (which we found out).

could you file a ticket for that?

@kishore03109 kishore03109 merged commit 0d0bcea into develop Aug 3, 2023
Comment on lines +108 to +129
} = useUpdateSettings(
siteName,
() => {
successToast({
id: "update-settings-success",
title: "Success",
description: "Site settings have been updated",
})
},
() => {
const errorMessage =
updateSettingsError?.response?.status === 409
? "Your site settings have recently been changed by another user. You can choose to either override their changes, or go back to editing. We recommend you to make a copy of your changes elsewhere, and come back later to reconcile your changes."
: `Site settings could not be updated. ${DEFAULT_RETRY_MSG}`
return errorToast({
id: "update-settings-error",
title: "Error",
description: errorMessage,
})
}
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good if we shifted all these to the hook and mark as optional but is a good to have bah...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof merge liao :/ + I abit dont get this what do you mean by shift to hook ah?

@mergify mergify bot deleted the fix/toastsHookForSettingsPage branch August 3, 2023 05:51
@kishore03109
Copy link
Contributor Author

@seaerchin Hmm how do we feel about prioritising this such that we file a ticket only if there is another bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants