Since the only cue for synchronization seems to be when selectedResource changes, could this be redefined with useEffectEvent to make sure useEffect doesn't run too often?

const syncSelectionChange = useEffectEvent((previousResource) => {
  const hasResourcesWithAgents =
    resources.length > 0 &&
    resources[0].agents &&
    resources[0].agents.length > 0;
  if (!previousResource && hasResourcesWithAgents) {
    resourcesNav.set(resourceOptionId(resources[0]));
  }
});

useEffect(() => {
  syncSelectionChange(selectedResource);
}, [syncSelectionChange, selectedResource]);

Even if resources changes by array reference, if the selected resource is still in there, I wouldn't think that should be a cause for making the effect re-run. And because the set function doesn't have memoization, it would cause the effect to run on every render

Though I think there's one more edge case: what would happen if we had a resource selected, but then the resources array becomes completely empty? Would we need to set things back to an empty string?

Overall, though, I think the effect logic is what's pushing me to think that a custom hook might be justified, mainly because it's relying on URL syncs via React Router. We can't really get away with derived values or inline state syncs because we'd have to talk to an outside system during the renders

I do think there's value in abstracting over those syncs and giving you a guarantee that things will "just work". I'm just wondering if it's worth testing the hook itself, or if it's better to treat it like an implementation detail, and test the whole component

Yes, this would be the ideal scenario but rendering the workspace component and testing this behavior requires some server events mocking which I would like to avoid. I rather have a simple solution like this for now and only go for more complex ones if it is needed.

Though I think there's one more edge case: what would happen if we had a resource selected, but then the resources array becomes completely empty? Would we need to set things back to an empty string?

Hm... I think this would not happen because when resources are returned, we can expect at least one resource. I think Coder does not allow workspace creations without a resource but if it does, I think it is not a huge concern right now.

And because the set function doesn't have memoization, it would cause the effect to run on every render

I would not care too much about effects running on every render if it is not going to mess up with the state or cause a loop. I would rather optimize it to be ok running on every render one million times.

I said all of that but after some thought I think the useEffectEvent is a good use case for this as you suggested, and I appreciate you providing the code for refactoring for that ❤️

I think the isSelected change is an improvement, but I'm wondering if the implementation is getting a little too tied to how useResourcesNav is set up.

What do you think of turning isSelected into just a boolean, and making the parent component call the function to get the right prop to pass in?

We need to know what is the selected resource and since this is not just an attribute but a "calculation", I think this interface makes things easier to extend.

Do you think it makes sense to add testing logic to check that the URL is syncing correctly on renders/re-renders, too?

I think it is too much since we don't care too much about the URL but what the hook is returning. What use case could justify this test?

It's more for making sure that the value we're getting back isn't getting out of sync with the URL. I don't think the current implementation has any real risks of that, but the way I see the hook, it does two things:

• Exposes the selection values
• Fires side effects to sync the selections with the URL

And we're only testing one of those things