SiegeEngineers / aoe2cm2

Captains Mode for Age of Empires II
https://aoe2cm.net
51 stars 25 forks source link

Added support for live refresh of recent drafts list #117

Closed Taloth closed 8 months ago

Taloth commented 9 months ago

Implements https://github.com/SiegeEngineers/aoe2cm2/issues/116

Changes

Todo


Been working on this on&off since the issue was accepted.

My initial version was implemented purely in the Spectate component. But after some thought I wanted it integrated in the redux layer. My redux experience is limited, so I hope this was the right way. The hover/pause is still done in the component state since imo that had more to do with the view. But technically even that can be moved to redux if needed.

I did add fontawesome for some icons, lemme know if that's ok.

Merging the sockets Finally, my thought is that the drafting process shouldn't use a query param in the socket.io connection, but instead send a 'JOIN_DRAFT' message, and 'LEAVE_DRAFT' message to disconnect. That would allow us to use a single socket for everything. (An alternative would be to use an dynamic namespace, but i'm not sure if that's necessary.) There are some edge-cases there to consider:

  1. Server shouldn't forcible disconnect socket in case of invalid state, but instead send a message back to notify.
  2. In case of socket.io disconnection, the UI should become aware of it so it can rejoin the draft.

I didn't want to touch that code since I was afraid of breaking literally the most important feature of the site. But lemme know if I should try doing that.

Taloth commented 9 months ago

Added tests for the socketio.

@HSZemi Do you have time to review it?

There's also the question if the two sockets should be merged, but there are those edgecases that I think I can't oversee the consequences of. Lemme know if you want something done with that or are happy with the current compromise.

HSZemi commented 9 months ago

Thanks for taking the time and effort to implement this! :yellow_heart:

Some thoughts:

  1. I think adding FontAwesome is not necessary. You can probably use mdi-react icons like we do in ReplayControls.tsx.
  2. Speaking of the auto update button: Maybe that could indicate the action that will be taken on hover? That means show an X when the button is hovered and auto update is currently active, and vice versa.
  3. In general, I would like to have visitors connected to the websockets as little as possible, to minimize the number of concurrent connections. It seems like you are currently not disconnecting users from the lobby when they navigate to another site? I know that also does not happen properly with the draft socket, but I have not yet figured out why that is.
  4. Is there a benefit in merging the two client-side sockets? Currently, there is a clear division of responsibilities between the two, which I rather like.
  5. I believe you still need to filter out drafts for hidden preset ids, like in DraftsStore.ts#103.
Taloth commented 9 months ago

Thx for checking it out.

  1. I'll check out mdi-react, I didn't know about it.
  2. Ok, I get the idea, I'll update it so it swaps the icon on hover. Not sure if that's more clear, but i'm no UX guy so what do I know.
  3. The lobby socket opens when ppl visit the spectate page since we don't need it earlier. The reason I kept the lobby connected is that when ppl go to a draft, and then use the 'back' button, they are sent back to the spectate page. And I preferred if it kept an up to date list in the background rather than resetting it. This is especially relevant when they forget to 'ctrl' click Watch to open a draft in a new tab. What I could do is put a timer on it, like if ppl are not in spectate for like 5 or 15 minutes, disconnect them, but that also means resetting the list when they eventually do go back. But I'm fine if you want me to terminate that socket immediately, I'm not sure how much pressure a socket connection actually has on the server.
  4. Fair question, there's something to be said for both approaches. I considered that the socket would be a connection between client and server and handle all communication between the two without the need to reconnect. And basically join the right rooms on demand. It also opens up the possibility to add more stuff to it in the future. It's also why I added the spectate_drafts message, rather than dropping ppl in the lobby room on socket connection. But there's no hard reason why that should be done that way. Truth be told, I'm not an expert in this field and I mainly wanted to acknowledge that some stuff wasn't up to standard.
  5. Ah right, I'll add a test for those and then fix the code.
HSZemi commented 8 months ago

There is one scenario I can think of where disconnecting the lobby connection would be beneficial: Casters or fans who open the draft of the set that is being casted, then keep it open for the duration of the games. Players will probably use the preset or draft link directly and not interact with the Spectate page, and people randomly clicking through drafts will probably not remain on the draft page for long.

Taloth commented 8 months ago

@HSZemi Updated it according to our discussion.

I'm fairly happy with the timeout approach now, by simply keeping track of the subscribe count I don't have to keep track of the timeoutId and clear it.

After testing I feel like inverting the icon on the hover on the Auto Update button isn't that good, coz it becomes a bit too jittery when ppl move mouse from the table to the button, that single mouse movement causes the icon to change twice, first from pause to active and then to inactive coz of the hover. But i'll leave that for you to judge, I don't mind either way.

HSZemi commented 8 months ago

Looks good to me!

Regarding the button: I think it's fine as it currently is. You could even make it grayed out when the auto update is disabled, and fully visible when the autoupdate is active. However, I think we could even remove the button altogether, and just keep the auto update always on. What do you think?

A timeout of 15 minutes seems a bit high to me. I had a look at the drafts from September and October 2023, and the draft durations look like this: grafik 90 % of drafts take less than 4 minutes (10,213 / 11,345). Therefore I think 5 minutes should be plenty.

Taloth commented 8 months ago

Regarding removing the button. The button serves multiple purposes: It tells ppl that it's auto refreshing so they don't spam refresh the page, it shows the current state (since it pauses when you hover over the table to avoid misclicks), and it allows someone to be like "don't refresh, I need this list". If you remove the button, you'd need another indicator in it's place. Note that this is also the reason why I made the button is light gray, it's primarily an indicator "Hey, this list is getting auto updated".

I had it at 5 minutes timeout. Whether it's 5 minutes or 15 minutes, I don't think it matters in terms of performance. If ppl visit via draft link, they're not getting the lobby socket. If they visit via the spectate list, then they're somewhat likely to return there, and in the past they'd refresh repeatedly and hit the api. The bigger concern would be people that keep the spectate list open for longer periods of time, and to deal with that you'd either have to set a hard limit or detect 'activity', but I'd suggest monitoring traffic (site, api and socket.io) to see whether the site load changes. So while I don't mind changing it back to 5 minutes, I just don't think it matters.

HSZemi commented 8 months ago

I see your point. Let's keep the button as is, and reduce the timeout to 5 minutes then.

Taloth commented 8 months ago

Done

HSZemi commented 8 months ago

Marvellous, thanks! Added in a9243e79f01220404b5e7c9f6a9be3a326d35d9a and deployed soon™