refactor: remove folder/workspace from vsCodeCliArgs (#4932)

* refactor: remove folder/workspace from vsCodeCliArgs

Since we handle this in the vscode.ts route, we no longer need to pass it to VS
Code as a CLI arg since it's deprecated on that side.

* feat(vscode): redirect to folder from cli

* Update src/node/routes/vscode.ts

Co-authored-by: Asher <[email protected]>

* fixup!: update _: type

* fixup!: move vars to lower if block

* fixup!: share redirect block

* fixup!: mmove req.query.ew block into if

* fixup!: refactor vscode tests

* refactor: make vscode.ts logic easier to read

* fixup!: fix broken tests and clean up logic

* chore: upgrade vscode version

* fixup!: delete unnecessary if closed block

* Update src/node/routes/vscode.ts

Co-authored-by: Asher <[email protected]>

* fixup!: rename to FOLDER_OR_WORKSPACE_WAS_CLOSED

Co-authored-by: Asher <[email protected]>

Author: jsjoeio

src/node/cli.ts

@@ -3,15 +3,7 @@ import { promises as fs } from "fs"
 import yaml from "js-yaml"
 import * as os from "os"
 import * as path from "path"
-import {
-  canConnect,
-  generateCertificate,
-  generatePassword,
-  humanPath,
-  paths,
-  isNodeJSErrnoException,
-  isFile,
-} from "./util"
+import { canConnect, generateCertificate, generatePassword, humanPath, paths, isNodeJSErrnoException } from "./util"
 
 const DEFAULT_SOCKET_PATH = path.join(os.tmpdir(), "vscode-ipc")
 
@@ -448,7 +440,7 @@ export interface DefaultedArgs extends ConfigArgs {
   "extensions-dir": string
   "user-data-dir": string
   /* Positional arguments. */
-  _: []
+  _: string[]
 }
 
 /**
@@ -770,25 +762,9 @@ export const shouldOpenInExistingInstance = async (args: UserProvidedArgs): Prom
  * Convert our arguments to VS Code server arguments.
  */
 export const toVsCodeArgs = async (args: DefaultedArgs): Promise<CodeServerLib.ServerParsedArgs> => {
-  let workspace = ""
-  let folder = ""
-  if (args._.length) {
-    const lastEntry = path.resolve(args._[args._.length - 1])
-    const entryIsFile = await isFile(lastEntry)
-    if (entryIsFile && path.extname(lastEntry) === ".code-workspace") {
-      workspace = lastEntry
-    } else if (!entryIsFile) {
-      folder = lastEntry
-    }
-    // Otherwise it is a regular file.  Spawning VS Code with a file is not yet
-    // supported but it can be done separately after code-server spawns.
-  }
-
   return {
     "connection-token": "0000",
     ...args,
-    workspace,
-    folder,
     "accept-server-license-terms": true,
     /** Type casting. */
     help: !!args.help,

src/node/routes/vscode.ts

@@ -1,12 +1,13 @@
 import { logger } from "@coder/logger"
 import * as express from "express"
+import * as path from "path"
 import { WebsocketRequest } from "../../../typings/pluginapi"
 import { logError } from "../../common/util"
 import { toVsCodeArgs } from "../cli"
 import { isDevMode } from "../constants"
 import { authenticated, ensureAuthenticated, redirect, self } from "../http"
 import { SocketProxyProvider } from "../socket"
-import { loadAMDModule } from "../util"
+import { isFile, loadAMDModule } from "../util"
 import { Router as WsRouter } from "../wsRouter"
 import { errorHandler } from "./errors"
 
@@ -25,6 +26,9 @@ export class CodeServerRouteWrapper {
 
   private $root: express.Handler = async (req, res, next) => {
     const isAuthenticated = await authenticated(req)
+    const NO_FOLDER_OR_WORKSPACE_QUERY = !req.query.folder && !req.query.workspace
+    // Ew means the workspace was closed so clear the last folder/workspace.
+    const FOLDER_OR_WORKSPACE_WAS_CLOSED = req.query.ew
 
     if (!isAuthenticated) {
       const to = self(req)
@@ -33,25 +37,38 @@ export class CodeServerRouteWrapper {
       })
     }
 
-    const { query } = await req.settings.read()
-    if (query) {
-      // Ew means the workspace was closed so clear the last folder/workspace.
-      if (req.query.ew) {
-        delete query.folder
-        delete query.workspace
-      }
+    if (NO_FOLDER_OR_WORKSPACE_QUERY && !FOLDER_OR_WORKSPACE_WAS_CLOSED) {
+      const settings = await req.settings.read()
+      const lastOpened = settings.query || {}
+      // This flag disables the last opened behavior
+      const IGNORE_LAST_OPENED = req.args["ignore-last-opened"]
+      const HAS_LAST_OPENED_FOLDER_OR_WORKSPACE = lastOpened.folder || lastOpened.workspace
+      const HAS_FOLDER_OR_WORKSPACE_FROM_CLI = req.args._.length > 0
+      const to = self(req)
+
+      let folder = undefined
+      let workspace = undefined
 
       // Redirect to the last folder/workspace if nothing else is opened.
-      if (
-        !req.query.folder &&
-        !req.query.workspace &&
-        (query.folder || query.workspace) &&
-        !req.args["ignore-last-opened"] // This flag disables this behavior.
-      ) {
-        const to = self(req)
+      if (HAS_LAST_OPENED_FOLDER_OR_WORKSPACE && !IGNORE_LAST_OPENED) {
+        folder = lastOpened.folder
+        workspace = lastOpened.workspace
+      } else if (HAS_FOLDER_OR_WORKSPACE_FROM_CLI) {
+        const lastEntry = path.resolve(req.args._[req.args._.length - 1])
+        const entryIsFile = await isFile(lastEntry)
+        const IS_WORKSPACE_FILE = entryIsFile && path.extname(lastEntry) === ".code-workspace"
+
+        if (IS_WORKSPACE_FILE) {
+          workspace = lastEntry
+        } else if (!entryIsFile) {
+          folder = lastEntry
+        }
+      }
+
+      if (folder || workspace) {
         return redirect(req, res, to, {
-          folder: query.folder,
-          workspace: query.workspace,
+          folder,
+          workspace,
         })
       }
     }

test/unit/node/cli.test.ts

@@ -726,29 +726,6 @@ describe("toVsCodeArgs", () => {
   it("should convert empty args", async () => {
     expect(await toVsCodeArgs(await setDefaults(parse([])))).toStrictEqual({
       ...vscodeDefaults,
-      folder: "",
-      workspace: "",
-    })
-  })
-
-  it("should convert with workspace", async () => {
-    const workspace = path.join(await tmpdir(testName), "test.code-workspace")
-    await fs.writeFile(workspace, "foobar")
-    expect(await toVsCodeArgs(await setDefaults(parse([workspace])))).toStrictEqual({
-      ...vscodeDefaults,
-      workspace,
-      folder: "",
-      _: [workspace],
-    })
-  })
-
-  it("should convert with folder", async () => {
-    const folder = await tmpdir(testName)
-    expect(await toVsCodeArgs(await setDefaults(parse([folder])))).toStrictEqual({
-      ...vscodeDefaults,
-      folder,
-      workspace: "",
-      _: [folder],
     })
   })
 
@@ -757,8 +734,6 @@ describe("toVsCodeArgs", () => {
     await fs.writeFile(file, "foobar")
     expect(await toVsCodeArgs(await setDefaults(parse([file])))).toStrictEqual({
       ...vscodeDefaults,
-      folder: "",
-      workspace: "",
       _: [file],
     })
   })

test/unit/node/routes/vscode.test.ts

@@ -1,19 +1,9 @@
 import { promises as fs } from "fs"
-import { Response } from "node-fetch"
 import * as path from "path"
 import { clean, tmpdir } from "../../../utils/helpers"
 import * as httpserver from "../../../utils/httpserver"
 import * as integration from "../../../utils/integration"
 
-interface WorkbenchConfig {
-  folderUri?: {
-    path: string
-  }
-  workspaceUri?: {
-    path: string
-  }
-}
-
 describe("vscode", () => {
   let codeServer: httpserver.HttpServer | undefined
 
@@ -39,7 +29,7 @@ describe("vscode", () => {
       expect(resp.status).toBe(200)
       const html = await resp.text()
       const url = new URL(resp.url) // Check there were no redirections.
-      expect(url.pathname + decodeURIComponent(url.search)).toBe(route)
+      expect(url.pathname + url.search).toBe(route)
       switch (route) {
         case "/":
         case "/vscode/":
@@ -52,59 +42,33 @@ describe("vscode", () => {
     }
   })
 
-  /**
-   * Get the workbench config from the provided response.
-   */
-  const getConfig = async (resp: Response): Promise<WorkbenchConfig> => {
-    expect(resp.status).toBe(200)
-    const html = await resp.text()
-    const match = html.match(/<meta id="vscode-workbench-web-configuration" data-settings="(.+)">/)
-    if (!match || !match[1]) {
-      throw new Error("Unable to find workbench configuration")
-    }
-    const config = match[1].replace(/&quot;/g, '"')
-    try {
-      return JSON.parse(config)
-    } catch (error) {
-      console.error("Failed to parse workbench configuration", config)
-      throw error
-    }
-  }
-
-  it("should have no default folder or workspace", async () => {
-    codeServer = await integration.setup(["--auth=none"], "")
-
-    const config = await getConfig(await codeServer.fetch("/"))
-    expect(config.folderUri).toBeUndefined()
-    expect(config.workspaceUri).toBeUndefined()
-  })
-
-  it("should have a default folder", async () => {
-    const defaultDir = await tmpdir(testName)
-    codeServer = await integration.setup(["--auth=none", defaultDir], "")
+  it("should redirect to the passed in workspace using human-readable query", async () => {
+    const workspace = path.join(await tmpdir(testName), "test.code-workspace")
+    await fs.writeFile(workspace, "")
+    codeServer = await integration.setup(["--auth=none", workspace], "")
 
-    // At first it will load the directory provided on the command line.
-    const config = await getConfig(await codeServer.fetch("/"))
-    expect(config.folderUri?.path).toBe(defaultDir)
-    expect(config.workspaceUri).toBeUndefined()
+    const resp = await codeServer.fetch("/")
+    const url = new URL(resp.url)
+    expect(url.pathname).toBe("/")
+    expect(url.search).toBe(`?workspace=${workspace}`)
   })
 
-  it("should have a default workspace", async () => {
-    const defaultWorkspace = path.join(await tmpdir(testName), "test.code-workspace")
-    await fs.writeFile(defaultWorkspace, "")
-    codeServer = await integration.setup(["--auth=none", defaultWorkspace], "")
+  it("should redirect to the passed in folder using human-readable query", async () => {
+    const folder = await tmpdir(testName)
+    codeServer = await integration.setup(["--auth=none", folder], "")
 
-    // At first it will load the workspace provided on the command line.
-    const config = await getConfig(await codeServer.fetch("/"))
-    expect(config.folderUri).toBeUndefined()
-    expect(config.workspaceUri?.path).toBe(defaultWorkspace)
+    const resp = await codeServer.fetch("/")
+    const url = new URL(resp.url)
+    expect(url.pathname).toBe("/")
+    expect(url.search).toBe(`?folder=${folder}`)
   })
 
   it("should redirect to last query folder/workspace", async () => {
     codeServer = await integration.setup(["--auth=none"], "")
 
     const folder = await tmpdir(testName)
     const workspace = path.join(await tmpdir(testName), "test.code-workspace")
+    await fs.writeFile(workspace, "")
     let resp = await codeServer.fetch("/", undefined, {
       folder,
       workspace,
@@ -118,21 +82,32 @@ describe("vscode", () => {
       resp = await codeServer.fetch(route)
       const url = new URL(resp.url)
       expect(url.pathname).toBe(route)
-      expect(decodeURIComponent(url.search)).toBe(`?folder=${folder}&workspace=${workspace}`)
+      expect(url.search).toBe(`?folder=${folder}&workspace=${workspace}`)
       await resp.text()
     }
 
     // Closing the folder should stop the redirecting.
     resp = await codeServer.fetch("/", undefined, { ew: "true" })
     let url = new URL(resp.url)
     expect(url.pathname).toBe("/")
-    expect(decodeURIComponent(url.search)).toBe("?ew=true")
+    expect(url.search).toBe("?ew=true")
     await resp.text()
 
     resp = await codeServer.fetch("/")
     url = new URL(resp.url)
     expect(url.pathname).toBe("/")
-    expect(decodeURIComponent(url.search)).toBe("")
+    expect(url.search).toBe("")
+    await resp.text()
+  })
+
+  it("should do nothing when nothing is passed in", async () => {
+    codeServer = await integration.setup(["--auth=none"], "")
+
+    let resp = await codeServer.fetch("/", undefined)
+
+    expect(resp.status).toBe(200)
+    const url = new URL(resp.url)
+    expect(url.search).toBe("")
     await resp.text()
   })
 
@@ -141,6 +116,8 @@ describe("vscode", () => {
 
     const folder = await tmpdir(testName)
     const workspace = path.join(await tmpdir(testName), "test.code-workspace")
+    await fs.writeFile(workspace, "")
+
     let resp = await codeServer.fetch("/", undefined, {
       folder,
       workspace,
@@ -152,7 +129,7 @@ describe("vscode", () => {
     resp = await codeServer.fetch("/")
     const url = new URL(resp.url)
     expect(url.pathname).toBe("/")
-    expect(decodeURIComponent(url.search)).toBe("")
+    expect(url.search).toBe("")
     await resp.text()
   })
 })

vendor/package.json

@@ -7,6 +7,6 @@
     "postinstall": "./postinstall.sh"
   },
   "devDependencies": {
-    "code-oss-dev": "coder/vscode#0fd21e4078ac1dddb26be024f5d4224a4b86da93"
+    "code-oss-dev": "coder/vscode#bd734e3d9f21b1bce4dabab2514177e90c090ee6"
   }
 }

vendor/yarn.lock

@@ -274,9 +274,9 @@ clone-response@^1.0.2:
   dependencies:
     mimic-response "^1.0.0"
 
-code-oss-dev@coder/vscode#0fd21e4078ac1dddb26be024f5d4224a4b86da93:
+code-oss-dev@coder/vscode#bd734e3d9f21b1bce4dabab2514177e90c090ee6:
   version "1.63.0"
-  resolved "https://codeload.github.com/coder/vscode/tar.gz/%3Cspan%20class="x x-first x-last">0fd21e4078ac1dddb26be024f5d4224a4b86da93"
+  resolved "https://codeload.github.com/coder/vscode/tar.gz/%3Cspan%20class="x x-first x-last">bd734e3d9f21b1bce4dabab2514177e90c090ee6"
   dependencies:
     "@microsoft/applicationinsights-web" "^2.6.4"
     "@parcel/watcher" "2.0.3"