Conversation
Fang-
left a comment
Fang-
left a comment
There was a problem hiding this comment.
Some comments. Haven't run this yet. May request additional review from a client dev.
| useEffect(() => { | ||
| console.log('spin: setting up...'); | ||
| let available = false; | ||
| const spin = new EventSource('/~_~/spin', { withCredentials: true }); |
There was a problem hiding this comment.
Worth noting that, with this and the /~_~/slog endpoint, and of course the eyre channel for herm subscriptions, this page now opens three SSE connections to your ship's domain, which iirc browsers limit to some max amount per domain (not per tab/window).
Of course, multi-tab urbiters are already hurting because of this, but this will hurt them even more.
Wonder if it's necessary, or if there even is some way we could demux/unify these connections somehow.
There was a problem hiding this comment.
Maybe just make it toggleable and off by default?
Fang-
left a comment
Fang-
left a comment
There was a problem hiding this comment.
Some new small comments below.
Additionally, there's some work here that's very-nice-to-have, but out of scope for this "initial integration" PR:
- The ctrl-c bug we saw.
- The spinner should key off of this endpoint instead of the unacked-pokes queue, which has proven to be somewhat unreliable for some reason. (Could probably be made to be reliable, but this will be faster and spin even when you're not actively interacting with the terminal.)
- Should have a combined "slog and spin" (slon/slin/slig/spon/spog/spig) endpoint and use that instead, to reduce open SSE connections.
|
|
||
| if (spinStack.length === 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This causes it to not render until we've heard something from the spin endpoint. Should be nearly instant usually, but you can imagine cases (problematic cases) where that takes a hot second or longer. This causes a rough "pop-in" effect, especially given that styling was updated so that the terminal window doesn't hit the bottom of the viewport until this element comes into existence.
Should just render it unconditionally.
| <style> | ||
| body, #root { | ||
| height: 99vh; /* prevent scrollbar on outer frame */ | ||
| height: 97vh; /* prevent scrollbar on outer frame */ |
There was a problem hiding this comment.
If this is accounting for space for the spin stack, the comment should be updated to say so.
Arguably, the styling for .spin-info-bar and its spans should live here with the rest of the styling. Then, might also make more sense to have this do a calc(99vh - xx) or whatever.
3 participants
This adds a %spin stack bar to webterm.