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: State issue on refresh #310

Merged
merged 4 commits into from
Apr 25, 2024
Merged

Conversation

cmaddox5
Copy link
Contributor

Asana task: ad-hoc

When a refresh occurs on the Edit Pending page, the places render correctly but the hook does not fetch existing screens correctly. I think this is due to hook execution timing. The hook fetching existing screens has already run by the time the state has been updated with the places from location.state. Making selectedPlaces.length part of the dep array fixes this. #308 removes the Remove Location button so this won't happen unless the page is being loaded. The Edit page state will continue to look at location state for the selectedPlaces and the New page state will look at the list passed in through props.

@cmaddox5 cmaddox5 requested review from a team and digitalcora and removed request for a team April 23, 2024 14:23
@digitalcora
Copy link
Contributor

I'm very much a stranger to the context here — is the expectation basically that selectedPlaces may be null or empty on initial render, then some time later it will be populated with values once, and not change again after that? If so I think using .length as the dependency makes sense, though maybe we should add a comment that this is the expectation. If not (that is, if the content of selectedPlaces may change other than going from "none" to "some"), I'd think this could still have a bug in some cases, namely when the contents change but the length stays the same.

@cmaddox5
Copy link
Contributor Author

I'm very much a stranger to the context here — is the expectation basically that selectedPlaces may be null or empty on initial render, then some time later it will be populated with values once, and not change again after that? If so I think using .length as the dependency makes sense, though maybe we should add a comment that this is the expectation. If not (that is, if the content of selectedPlaces may change other than going from "none" to "some"), I'd think this could still have a bug in some cases, namely when the contents change but the length stays the same.

Yep, your comment is correct. There are two ways to get to this page

  1. from the previous step in the workflow. In this way, selectedPlaces is populated before the first render because it comes from props.
  2. From the Edit Pending button on the Pending tab. In this way, selectedPlaces is populated as soon as this setter is executed which will be after first render.

In both scenarios, selectedPlaces is guaranteed to not change again after #308 is merged. The user will have to leave the page to change the list. The comment is a good idea, will do that now.

Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

🚀

@cmaddox5 cmaddox5 merged commit 32c16aa into permanent-configuration Apr 25, 2024
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/hook-refresh-issue branch April 25, 2024 13:59
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