In D9638#282182, @ashoat wrote:

Can you clarify why? Wouldn't we want to make this check everywhere?

There's no strong reason, I'm ultimately happy to include it on the server. My thinking was as follows:

There are two callsites of isInvalidPinSource on the server:

• fetchDerivedMessages in message-fetchers
• toggleMessagePinForThread in thread-updaters (this is the endpoint when pinning / unpinning a message)

For fetchDerivedMessages, I weighed the tradeoff of adding some additional complexity to the fetchDerivedMessages code of having to fetch all of the threadInfos associated with the message ids. I ultimately decided against adding another query on the database since it's impossible for a client to pin a message that begins a sidebar.

For toggleMessagePinForThread, we already have to fetch the thread info in the updater code. This change actually wouldn't be too bad, it would just require moving the call to fetchServerThreadInfos earlier and passing it in and fixing the types, but I thought to keep the call sites consistent (so we can definitively say threadInfo will be provided to isInvalidSidebarSource from the client, but not from the server). So if I change fetchDerivedMessages, I'll change this too.

I'm ok to include the threadInfo parameter from the server for a matter of consistency everywhere, I estimate it should be a short change (just will take some time testing fetchDerivedMessages) so it won't derail any time estimates further. I'll go ahead and work on that change and then follow through by updating the diff here

Interesting that toggleMessagePinForThread is missing the threadInfo.sourceMessageID !== item.messageInfo.id check. I guess that can't happen, since it only takes a messageID, and the parent channel gets fetched from that (rather than the corresponding sidebar).

Still, it might be a good idea to add this to toggleMessagePinForThread. Besides the benefits of consistently checking the same condition in as many places as possible, you also get a bonus for moving the fetchServerThreadInfos call earlier. I think that lets you replace the isInvalidPinSource check there with canToggleMessagePin, and drop the checkThreadPermission entirely. (You'd need to update the types on canToggleMessagePin I think.)

On the other hand, adding an extra query to fetchDerivedMessages probably isn't worth it. And like you said, that functions handles fetching rather than setting data, so the need to validate isn't as high.

That said, I don't love that the thread is currently an optional parameter to isInvalidPinSource. It's ambiguous to the caller whether it needs to be provided or not.

Could you instead introduce two variants of the function with two different names? That way you could use naming to communicate the differences between the situations in which each function should be called.

(toggleMessagePinForThread changes can probably be in a separate diff)