I don't think SMTP config can be blank.

From the types, it could be tho, but you shared this message a few minutes ago:

We can't assume SMTP will always be configured. We should have the same check here.

Done!

For this part, I think it'd be better to swap as for satisfies. That way, you can't accidentally pass in too many properties, and you don't get runtime/compile-time drift

I also tried adding the type parameter to the .map callback, but that didn't raise any complaints about extra properties

Tagging this for now while I look through the other files. I don't fully understand the point of this function, and at the very least, I think that it should be redefined as a type predicate

This is just easier to use IMO.

Type predicate:

const isNotificationMethod = (v: string): v is NotificationMethod => {}
// Usage
values.map(v => {
   if(!isNotificationMethod(v)) {
      throw new Error("v is not valid") 
   }
   return <Component method={method} />
})

Cast function

const castNotificationMethod = (v: string): NotificationMethod => {}
// Usage
values.map(v => {
   const method = castNotificationMethod(v)
   return <Component method={method} />
})

Another case where I think as can be swapped for satisfies

In this case, as is intentional. I want to be able to use partial data and not have to pass more than 100+ values.

Yeah, sorry. Misread that

Do you think this might be a good use case for useSearchParamsKey? It'd have to be changed a little bit to account for the || instead of the ??, though

I don't think it would make too much difference since I'm using regukar links to update the search params. I'm using || instead of ?? to grab empty strings as well.

Could this have a .sort tacked on to make sure there's no risk of the UI elements jumping around? Mainly worried that as the objects gets updated over time, the entries array could serialize the values in different orders across re-renders

Going to implement this in the selectTemplatesByGroup

Not a big deal, but could this changed so that we check the array index and conditionally choose not to render out the divider if it's the last one? I know we have the last-child styling, but it feels like it could be a bit more fragile over time

I think the tables could use a little more spacing between them. They have the borders, so there's no real risk of them getting confused for each other, but they feel a little tense right now

I think this is a bit confusing from a user standpoint

When I see a parent switch that's off, I assume the whole group is off

I think I would expect this behavior:

1. When the parent switch is off, all child switches are off
2. When the parent switch is on, at least one child switch is on
3. Switching the parent switch from on to off turns all child switches off
4. Switching the parent switch from off to on turns all child switches on
5. Switching a child switch to on will switch the parent to on if the parent isn't already on
6. If a child switch is the only one that's on, and it's flipped off, the parent switch flips off, too

I also think the current email icons are a bit misleading, because before I tried interacting with them, I expected them to be buttons. As in, clicking one would send off an email in some way

Not sure what a good change would be. Maybe something badge-like?

Related to the first comment, I see how this can be confusing. I will put more thought into it. I just would rather do it in a different PR to avoid this one getting longer than it is.

I also think the current email icons are a bit misleading, because before I tried interacting with them, I expected them to be buttons. As in, clicking one would send off an email in some way

I can try a badge and see how it looks like

Yeah, I can deal with a slightly confusing UI (or at least confusing from my point of view) as long as it works predictably

I'm not sure what the best alternative is, but this feels like a code smell to me. Feels like it's breaking out of React's one-way data binding because even though there's shared state between the container and the individual switches, the parent container has little say in how the state is allowed to change. I don't think this is big enough of an edge case to justify breaking React conventions

My gut feeling is also that, just to reduce coupling, any handler like onToggle should always be a void function

I don't think this is big enough of an edge case to justify breaking React conventions

Is there a place where I can read about this convention?

My gut feeling is also that, just to reduce coupling, any handler like onToggle should always be a void function

I don't know if I agree with this... what are the pros and cons?

Eg. An onChange handler returns an event and not a void. I think it is pretty common to have handlers returning data.

To be clear, onChange and all the event handlers receive an event as input, but they don't return anything. This is the type React has:

type React.EventHandler<E extends SyntheticEvent<any>> = (event: E) => void;

In real HTML, you can have the event handlers return booleans to affect event bubbling behavior, but React shaves that off for the synthetic events.

Like, if I have a component like this:

function MyComponent () {
  const [color, setColor] = useState("blue");
  return <button onClick={() => setColor("red"}>Click me</button>;
}

I don't think anyone would expect the button to have any way to affect MyComponent's state, beyond the onClick. But even then, MyComponent is in full control of the event handler definition – it fully prescribes how its own state is allowed to change. MyComponent has a dependency on button, but everything related to its state is self-contained in the one component. button doesn't have direct access to the state setter, so all it can do is follow the very restricted set of instructions it's given

But with the switches, the main state is being split across four different places: the RQ cache living outside the UI tree, the parent page component, the individual switches, and the mutation key factory function. It felt like I had to jump through a lot of hoops (and two different files) to figure out exactly how the state was changing

I don't think there's a way to fully consolidate the state in one place, just because we do need RQ, but four places for one main piece of state feels like a bit much to me

My preference would be to have the switches be "dumb" and just call an event handler when clicked. Then put all the state related to the functionality inside the parent page component

But as far as conventions around one-way data-binding/unidirectional data flow, I tried looking on React.dev, and I'm surprised there wasn't too much dedicated information on it. There's Thinking in React, particularly parts 4 and 5, but I think it's the sort of thing that's not explicitly shouted out, even though it informs pretty much every lesson on the site

The main reason I know about all this is because I just happen to know that React was designed around the flux architecture pattern. Facebook has an archive page describing what's it's about

I think I better understand your concerns now. Thanks for explaining them to me. I will fix it.

Done!

Bruno, you're so good at your job. This was the first thing I thought to check, knowing how easy it would be to miss adding it here. 😄 Should it be behind the same experiment check as in the other sidebar tho?

Thanks @aslilac ❤️. Yes, it definitely needs to be under the experiment.

Yeah, sorry. Misread that