fix(custom-views): Fix infinite rerendering upon unsaved changes

[MichaelSun48]

This PR fixes an infinite rerendering bug that caused the issue stream to crash. This was caused by a circular dependency in a useEffect. Fix is to remove those dependencies.

[leeandher]

Could this still cause the same issue since were calling tabListState.setSelectedKey() inside which will cause tabListState to update and refire the hook?

[MichaelSun48]

Good point - intuitively I feel like it should too, but in my testing that doesn't occur. I'm pretty sure this is because this useEffect's behavior depends on the url query, and if tabListState.setSelectedKey() is called, it changes the query in a way such that the subsequent time this useEffect runs, it would not result in tabListState.setSelectedKey() being called again. Sorta like reaching a state of equilibrium after one unnecessary rerender if that makes sense?

I think you've still got a point that leaving tabListState as a dependency is more likely to cause issues down the line, even if its stable now, so I'll remove it in the next commit.

[leeandher]

Few more things before a green stamp!

• Rather than passing pathname to navigate, I'd just use the current location. I think theres a search params hook in v6 react router but that might not be available yet, so this is the next best. It'll prevent weird bugs if we ever use this component on a different page, or change the path or something.

const location = useLocation()
navigate({...location, query: {...location.query, modifed: True}})

• If the entire page is crashing when the component fails, and it's only supplementary to the data on the page as a whole, it would probably be worth wrapping it in an ErrorBoundary. This is always true, but especially so for these changes in high traffic areas!

[MichaelSun48]

• Rather than passing pathname to navigate, I'd just use the current location. I think theres a search params hook in v6 react router but that might not be available yet, so this is the next best. It'll prevent weird bugs if we ever use this component on a different page, or change the path or something.

@leeandher
Good idea! I think I'd prefer to make this change in a separate PR since this PR prevents the issue stream from crashing, and I'd want to spend some extra time testing after changing all of the navigates to what you suggested. I added your ErrorBoundary suggestion in the last commit though.

[scttcper]

sometimes for things you want to only happen on init it's easier to use a ref that enforces it is only applied once