From 4af8446f483be43ae391471e016e8a0762fa6018 Mon Sep 17 00:00:00 2001
From: Asher <ash@coder.com>
Date: Mon, 23 Oct 2023 23:42:39 -0400
Subject: [PATCH] fix: initialize terminal with correct size (#10369)

* Fit once during creation

This does not fix any bugs (that I know of) but we only need to fit once
when the terminal is created, not every time we reconnect.  Granted,
currently we do not support reconnecting without refreshing anyway so it
does not really matter, but this just seems more correct.

Plus now we will not have to pass the fit addon around.

* Pass size when connecting web socket URL

I think this will solve an issue where screen does does not correctly
handle an immediate resize.  It seems to ignore the resize, but even if
you send it again nothing changes, seemingly thinking it is already at
that size?

* Use new struct for decoding reconnecting pty requests

Decoding a JSON message does not touch omitted (or null) fields so once
a message with a resize comes in, every single message from that point
will cause a resize.

I am not sure if this is an actual problem in practice but at the very
least it seems unintentional.
---
 agent/reconnectingpty/reconnectingpty.go     |  2 +-
 site/src/pages/TerminalPage/TerminalPage.tsx | 31 ++++++++++----------
 site/src/utils/terminal.ts                   |  4 +++
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/agent/reconnectingpty/reconnectingpty.go b/agent/reconnectingpty/reconnectingpty.go
index 30b1f44801..280cf62aaa 100644
--- a/agent/reconnectingpty/reconnectingpty.go
+++ b/agent/reconnectingpty/reconnectingpty.go
@@ -196,8 +196,8 @@ func (s *ptyState) waitForStateOrContext(ctx context.Context, state State) (Stat
 // until EOF or an error writing to ptty or reading from conn.
 func readConnLoop(ctx context.Context, conn net.Conn, ptty pty.PTYCmd, metrics *prometheus.CounterVec, logger slog.Logger) {
 	decoder := json.NewDecoder(conn)
-	var req codersdk.ReconnectingPTYRequest
 	for {
+		var req codersdk.ReconnectingPTYRequest
 		err := decoder.Decode(&req)
 		if xerrors.Is(err, io.EOF) {
 			return
diff --git a/site/src/pages/TerminalPage/TerminalPage.tsx b/site/src/pages/TerminalPage/TerminalPage.tsx
index c3844fe051..7c82c82a1f 100644
--- a/site/src/pages/TerminalPage/TerminalPage.tsx
+++ b/site/src/pages/TerminalPage/TerminalPage.tsx
@@ -54,7 +54,6 @@ const TerminalPage: FC = () => {
   const [terminalState, setTerminalState] = useState<
     "connected" | "disconnected" | "initializing"
   >("initializing");
-  const [fitAddon, setFitAddon] = useState<FitAddon | null>(null);
   const [searchParams] = useSearchParams();
   // The reconnection token is a unique token that identifies
   // a terminal session. It's generated by the client to reduce
@@ -125,7 +124,6 @@ const TerminalPage: FC = () => {
       terminal.loadAddon(new CanvasAddon());
     }
     const fitAddon = new FitAddon();
-    setFitAddon(fitAddon);
     terminal.loadAddon(fitAddon);
     terminal.loadAddon(new Unicode11Addon());
     terminal.unicode.activeVersion = "11";
@@ -134,13 +132,21 @@ const TerminalPage: FC = () => {
         handleWebLinkRef.current(uri);
       }),
     );
-    setTerminal(terminal);
+
     terminal.open(xtermRef.current);
-    const listener = () => {
-      // This will trigger a resize event on the terminal.
-      fitAddon.fit();
-    };
+
+    // We have to fit twice here. It's unknown why, but the first fit will
+    // overflow slightly in some scenarios. Applying a second fit resolves this.
+    fitAddon.fit();
+    fitAddon.fit();
+
+    // This will trigger a resize event on the terminal.
+    const listener = () => fitAddon.fit();
     window.addEventListener("resize", listener);
+
+    // Terminal is correctly sized and is ready to be used.
+    setTerminal(terminal);
+
     return () => {
       window.removeEventListener("resize", listener);
       terminal.dispose();
@@ -165,16 +171,10 @@ const TerminalPage: FC = () => {
 
   // Hook up the terminal through a web socket.
   useEffect(() => {
-    if (!terminal || !fitAddon) {
+    if (!terminal) {
       return;
     }
 
-    // We have to fit twice here. It's unknown why, but
-    // the first fit will overflow slightly in some
-    // scenarios. Applying a second fit resolves this.
-    fitAddon.fit();
-    fitAddon.fit();
-
     // The terminal should be cleared on each reconnect
     // because all data is re-rendered from the backend.
     terminal.clear();
@@ -229,6 +229,8 @@ const TerminalPage: FC = () => {
       reconnectionToken,
       workspaceAgent.id,
       command,
+      terminal.rows,
+      terminal.cols,
     )
       .then((url) => {
         if (disposed) {
@@ -289,7 +291,6 @@ const TerminalPage: FC = () => {
     };
   }, [
     command,
-    fitAddon,
     proxy.preferredPathAppURL,
     reconnectionToken,
     terminal,
diff --git a/site/src/utils/terminal.ts b/site/src/utils/terminal.ts
index 52d46feaaf..d27a6efce3 100644
--- a/site/src/utils/terminal.ts
+++ b/site/src/utils/terminal.ts
@@ -5,11 +5,15 @@ export const terminalWebsocketUrl = async (
   reconnect: string,
   agentId: string,
   command: string | undefined,
+  height: number,
+  width: number,
 ): Promise<string> => {
   const query = new URLSearchParams({ reconnect });
   if (command) {
     query.set("command", command);
   }
+  query.set("height", height.toString());
+  query.set("width", width.toString());
 
   const url = new URL(baseUrl || `${location.protocol}//${location.host}`);
   url.protocol = url.protocol === "https:" ? "wss:" : "ws:";