Codecov Report

Merging#1186 (19c7b54) intomain (021e4cd) willincrease coverage by0.14%.
The diff coverage is69.20%.

@@            Coverage Diff             @@##             main    #1186      +/-   ##==========================================+ Coverage   65.65%   65.80%   +0.14%==========================================  Files         269      272       +3       Lines       17366    17872     +506       Branches      164      192      +28     ==========================================+ Hits        11402    11760     +358- Misses       4772     4882     +110- Partials     1192     1230      +38

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update021e4cd...19c7b54. Read thecomment docs.

Have you considered instead of capturing scroll back just reconnecting the tty to the new websocket and forcing the shell or ncurses app to refresh by sending a resize signal? Would be way simpler, and we can show the reconnection similar to how iterm or vscode does it

This actually is how VS Code does it!

@deansheather do you feel like this is complex right now? We do just store a reconnection token, much like VS Code.

@kylecarbs my suggestion is to not use a scroll back buffer at all, but when they reconnect the client would keep the existing history it has locally and just write a "Restored terminal session" message to the terminal, then force a screen refresh for interactive apps (like the shell and vim etc.) using sigwinch. We would not need to worry about scrollback buffers which I think are weird and complex anyways

How would that work if data occurred during disconnect? Or if they refresh the window?

The current case handles both seamlessly, and could even allow terminal sharing just with a link.

@kylecarbs my suggestion is how vscode does it I think. Obviously having a scrollback is nicer, but I think it adds to much complexity and is prone to weird terminal bugs rather than a simpler and more robust solution.

Terminal sharing is still possible but when new clients connect they won't have scrollback which I think is fine.

VS Code has a buffer like the current implementation, but maybe they also do what you mention.

What bugs do you foresee? Have you tried it out?

We can add scrollback later if customers request it, but for most cases users just don't want to do lose their shell history or running processes or vim session etc and the output (as long as we do the refresh thing) isn't a huge issue IMO

@presleyp I appreciate the thorough review!

I'm going to merge, but@deansheather please feel free to follow up if my message doesn't resolve your concerns! It appears to be a solid web-terminal as-is, but if we can make it even better I'm of course happy to do so! 😄🪄