processing / p5.js-web-editor

The p5.js Editor is a website for creating p5.js sketches, with a focus on making coding accessible and inclusive for artists, designers, educators, beginners, and anyone else! You can create, share, or remix p5.js sketches without needing to download or configure anything.
https://editor.p5js.org
GNU Lesser General Public License v2.1
1.38k stars 1.32k forks source link

fix: infinite api requests when the verification token is invalid #2736

Closed aryanas159 closed 9 months ago

aryanas159 commented 10 months ago

Fixes #2652

Changes: Because of the continuous prop changes the useEffect was executing infinitely, I have added a state that allows only one API call to check where the token in invalid or not. I have verified that this pull request:

https://github.com/processing/p5.js-web-editor/assets/114330931/b22e4dd2-3463-4d95-8d79-5d4c3ff98636

lindapaiste commented 10 months ago

Changes: Because of the continuous prop changes the useEffect was executing infinitely

Yikes!

I have added a state that allows only one API call to check where the token in invalid or not.

This is definitely an improvement and should solve the problem. It feels like a bit of a band-aid solution though. You've identified that the core problem is having props in the useEffect dependencies, which causes the effect to run on every re-render. We can change the dependencies to [location, props.verifyEmailConfirmation] so that it depends only on the one function and not on the props object. I'm not sure if that works though because I'm not sure if props.verifyEmailConfirmation is a stable reference, especially since we're using bindActionCreators (which is completely unnecessary). Maybe it's time to clean up this component and bring it into the modern age. See the checklist in https://github.com/processing/p5.js-web-editor/pull/2405#issuecomment-1718326111. The following code will definitely only re-execute the effect if the token changes.

const { t } = useTranslation();

const location = useLocation();

const browserHistory = useHistory();

const dispatch = useDispatch();

const emailVerificationTokenState = useSelector(
  (state) => state.user.emailVerificationTokenState
);

const verificationToken = useMemo(() => {
  const searchParams = new URLSearchParams(location.search);
  return searchParams.get('t');
}, [location.search]);

useEffect(() => {
  if (verificationToken) {
    dispatch(verifyEmailConfirmation(verificationToken));
  }
}, [dispatch, verificationToken]);
aryanas159 commented 10 months ago

@lindapaiste Great suggestion, I have made the following changes you listed in https://github.com/processing/p5.js-web-editor/pull/2405#issuecomment-1718326111. It works well since the dependency array contains the verification token which is memoized there are no infinite loops. Let me know if these kinds of changes are required for other components as well.

aryanas159 commented 9 months ago

@lindapaiste, could you please review the changes?

lindapaiste commented 9 months ago

I went ahead and committed my suggestion because I want to get this bug fixed.

As a side note, it's weird that the message that we show when the token is invalid is "Something went wrong." and not the "Token is invalid or has expired." message that we get back from the API. But that's something to address in a separate issue.