Right-click file operations, and an icon in the taskbar

apps/desktop/electron/main/icon-path.test.ts

@@ -0,0 +1,44 @@
+import { readFileSync } from 'node:fs'
+import { join } from 'node:path'
+import { describe, expect, it } from 'vitest'
+import { resolveIconPath, WM_CLASS } from './icon-path.js'
+
+const PACKAGED = '/opt/DiskPush/resources/icon.png'
+const CHECKOUT = '/home/you/diskpush/apps/desktop/resources/icon.png'
+
+describe('resolveIconPath', () => {
+  it('prefers the packaged copy when it is there', () => {
+    expect(resolveIconPath([PACKAGED, CHECKOUT], () => true)).toBe(PACKAGED)
+  })
+
+  it('falls back to the checkout, which is the only copy in a dev run', () => {
+    expect(resolveIconPath([PACKAGED, CHECKOUT], (path) => path === CHECKOUT)).toBe(CHECKOUT)
+  })
+
+  it('returns undefined rather than a path to nothing', () => {
+    // BrowserWindow given a missing icon path logs nothing and shows no icon,
+    // so a guess is indistinguishable from the bug this is fixing.
+    expect(resolveIconPath([PACKAGED, CHECKOUT], () => false)).toBeUndefined()
+  })
+})
+
+describe('StartupWMClass', () => {
+  /**
+   * A taskbar matches a window to its launcher by comparing the window's
+   * WM_CLASS with the .desktop file's StartupWMClass. The app pins WM_CLASS to
+   * WM_CLASS above; these two files are the other half of that agreement, and
+   * a rename that touched only one of them would silently un-group the window
+   * again — the exact symptom, with nothing failing.
+   */
+  const root = join(import.meta.dirname, '..', '..', '..', '..')
+
+  it('is what the installer writes into its desktop entry', () => {
+    const installer = readFileSync(join(root, 'scripts', 'install.sh'), 'utf8')
+    expect(installer).toContain(`StartupWMClass=${WM_CLASS}`)
+  })
+
+  it('is what electron-builder writes into the packaged entry', () => {
+    const manifest = JSON.parse(readFileSync(join(root, 'apps', 'desktop', 'package.json'), 'utf8'))
+    expect(manifest.build.linux.desktop.entry.StartupWMClass).toBe(WM_CLASS)
+  })
+})

apps/desktop/electron/main/icon-path.ts

@@ -0,0 +1,32 @@
+/**
+ * Where the window icon lives, which differs between a packaged app and a
+ * checkout.
+ *
+ * Packaged, electron-builder copies resources/icon.png to `extraResources`, so
+ * it sits beside the app under `process.resourcesPath`. Run from a checkout,
+ * `process.resourcesPath` is Electron's own resources directory and holds
+ * nothing of ours, so the repo copy is the fallback.
+ *
+ * Kept apart from index.ts, and given its own `exists`, so the ordering can be
+ * tested without Electron and without touching a disk.
+ */
+export function resolveIconPath(
+  candidates: readonly string[],
+  exists: (path: string) => boolean,
+): string | undefined {
+  // Undefined rather than a guess: BrowserWindow given a path to nothing logs
+  // no error and shows no icon, which is indistinguishable from not asking.
+  return candidates.find((candidate) => exists(candidate))
+}
+
+/**
+ * The WM_CLASS the window reports, pinned rather than inherited.
+ *
+ * A Linux taskbar matches a window to its launcher by comparing WM_CLASS with
+ * the .desktop file's StartupWMClass. Left alone, Chromium derives WM_CLASS
+ * from the executable name, so it changes with `executableName` and differs
+ * between the deb, the AppImage and a dev run — and a StartupWMClass written
+ * to match one of those is wrong for the others. Naming it here means the
+ * desktop entries can all state the same value.
+ */
+export const WM_CLASS = 'DiskPush'

apps/desktop/electron/main/index.ts

@@ -1,9 +1,10 @@
-import { readFileSync } from 'node:fs'
+import { existsSync, readFileSync } from 'node:fs'
 import { readFile } from 'node:fs/promises'
 import { join, normalize } from 'node:path'
 import { fileURLToPath } from 'node:url'
 import { app, BrowserWindow, protocol, shell } from 'electron'
 import { contentTypeFor, resolveBundlePath } from './bundle-path.js'
+import { resolveIconPath, WM_CLASS } from './icon-path.js'
 import { contentSecurityPolicy, inlineScriptHashes } from './csp.js'
 import { registerIpc } from './ipc.js'
 import { checkForUpdates } from './services/updater.js'
@@ -23,6 +24,22 @@ const here = join(fileURLToPath(import.meta.url), '..')
 const isDev = !app.isPackaged && process.env.DISKPUSH_DEV === '1'
 const DEV_URL = 'http://localhost:3210'
 
+/**
+ * Pinned before the app is ready, because Chromium reads it when it creates
+ * the window: a taskbar matches WM_CLASS against a .desktop file's
+ * StartupWMClass, and left to itself Chromium derives WM_CLASS from the
+ * executable name — different for the deb, the AppImage and a dev run.
+ */
+app.commandLine.appendSwitch('class', WM_CLASS)
+
+/** The window icon, which is also what a taskbar draws for the running app. */
+function iconPath(): string | undefined {
+  return resolveIconPath(
+    [join(process.resourcesPath, 'icon.png'), join(here, '..', '..', 'resources', 'icon.png')],
+    existsSync,
+  )
+}
+
 /**
  * The exported renderer is served over a real scheme rather than loaded from
  * file://.
@@ -88,13 +105,18 @@ function serveBundle(): void {
 }
 
 function createWindow(): BrowserWindow {
+  const icon = iconPath()
   const window = new BrowserWindow({
     width: 1360,
     height: 860,
     minWidth: 960,
     minHeight: 600,
     backgroundColor: '#0a0c10',
     title: 'DiskPush',
+    // Without this the window carries no _NET_WM_ICON and the taskbar draws a
+    // placeholder, which is what an app launched directly rather than from its
+    // .desktop file always looked like.
+    ...(icon ? { icon } : {}),
     webPreferences: {
       preload: join(here, '..', 'preload', 'index.cjs'),
       // The renderer gets no Node, no remote module, and its own sandbox. It

apps/desktop/electron/main/ipc.ts

@@ -1,18 +1,19 @@
-import { readFile } from 'node:fs/promises'
-import { readdir, stat } from 'node:fs/promises'
+import { lstat, mkdir, open, readdir, readFile, rename, rm, stat, unlink } from 'node:fs/promises'
 import { homedir } from 'node:os'
-import { isAbsolute, join, resolve } from 'node:path'
+import { isAbsolute, join, posix, resolve } from 'node:path'
 import { ipcMain, shell, type IpcMainInvokeEvent } from 'electron'
-import { probeConnection, parseSshConfig, sshConfigConnections } from '@diskpush/ssh-core'
+import { probeConnection, parseSshConfig, sshConfigConnections, type SftpBrowser } from '@diskpush/ssh-core'
 import { z } from 'zod'
 import {
   ConnectionInputSchema,
+  CreateEntryRequestSchema,
+  DeleteEntryRequestSchema,
   ExternalUrlSchema,
   IPC,
   JobIdSchema,
   PathSchema,
   RemotePathRequestSchema,
-  RenameRequestSchema,
+  RenameEntryRequestSchema,
   TransferRequestSchema,
   type IpcResult,
 } from '../shared/contract.js'
@@ -51,6 +52,32 @@ function resolveLocalPath(input: string): string {
   return isAbsolute(expanded) ? expanded : resolve(expanded)
 }
 
+/** Whether a path exists, without making the caller catch ENOENT. */
+async function exists(path: string): Promise<boolean> {
+  try {
+    await lstat(path)
+    return true
+  } catch {
+    return false
+  }
+}
+
+/**
+ * Runs `fn` against an SFTP browser for `connectionId` and always closes it.
+ *
+ * Every remote mutation had its own copy of this, and a `finally` that is
+ * written six times is a `finally` that eventually is not.
+ */
+async function withBrowser<T>(connectionId: string | undefined, fn: (browser: SftpBrowser) => Promise<T>): Promise<T> {
+  if (!connectionId) throw new Error('That operation needs a server.')
+  const browser = await browserFor(await requireConnection(connectionId))
+  try {
+    return await fn(browser)
+  } finally {
+    browser.close()
+  }
+}
+
 export function registerIpc(): void {
   // --- connections ---------------------------------------------------------
 
@@ -164,42 +191,87 @@ export function registerIpc(): void {
     }
   })
 
-  handle(IPC.fsMkdirRemote, RemotePathRequestSchema, async ({ connectionId, path }) => {
-    const connection = await requireConnection(connectionId)
-    const browser = await browserFor(connection)
-    try {
-      await browser.mkdir(path)
-      return true
-    } finally {
-      browser.close()
-    }
+  /**
+   * The mutating operations, local and remote.
+   *
+   * Each takes a directory and a bare entry name and joins them here, so the
+   * renderer names a thing inside the folder it is showing rather than handing
+   * the main process a path to act on. `resolveLocalPath` still expands `~`,
+   * but it is applied to the directory only.
+   */
+  handle(IPC.fsMkdirLocal, CreateEntryRequestSchema, async ({ directory, name }) => {
+    await mkdir(join(resolveLocalPath(directory), name))
+    return true
   })
 
-  handle(IPC.fsRenameRemote, RenameRequestSchema, async ({ connectionId, from, to }) => {
-    const connection = await requireConnection(connectionId)
-    const browser = await browserFor(connection)
-    try {
-      await browser.rename(from, to)
+  handle(IPC.fsCreateFileLocal, CreateEntryRequestSchema, async ({ directory, name }) => {
+    // `wx` fails when the file exists rather than truncating it.
+    const handle = await open(join(resolveLocalPath(directory), name), 'wx')
+    await handle.close()
+    return true
+  })
+
+  handle(IPC.fsRenameLocal, RenameEntryRequestSchema, async ({ directory, from, to }) => {
+    const root = resolveLocalPath(directory)
+    const target = join(root, to)
+    // Renaming onto an existing name silently destroys it, so refuse. There is
+    // an unavoidable race here; it narrows a footgun rather than closing it.
+    if (await exists(target)) throw new Error(`“${to}” already exists here.`)
+    await rename(join(root, from), target)
+    return true
+  })
+
+  handle(IPC.fsDeleteLocal, DeleteEntryRequestSchema, async ({ directory, name, isDirectory }) => {
+    const target = join(resolveLocalPath(directory), name)
+    const stats = await lstat(target)
+    // A symlink to a directory reports as a directory to the caller; deleting
+    // it must still unlink the link rather than recurse into what it points at.
+    if (stats.isSymbolicLink()) {
+      await unlink(target)
       return true
-    } finally {
-      browser.close()
     }
+    if (stats.isDirectory() !== isDirectory) throw new Error('That item changed on disk; refresh and try again.')
+    if (isDirectory) await rm(target, { recursive: true })
+    else await unlink(target)
+    return true
   })
 
-  handle(
-    IPC.fsDeleteRemote,
-    z.object({ connectionId: z.string().min(1), path: PathSchema, isDirectory: z.boolean() }),
-    async ({ connectionId, path, isDirectory }) => {
-      const connection = await requireConnection(connectionId)
-      const browser = await browserFor(connection)
-      try {
-        if (isDirectory) await browser.rmdir(path)
-        else await browser.unlink(path)
+  handle(IPC.fsMkdirRemote, CreateEntryRequestSchema, async ({ connectionId, directory, name }) =>
+    withBrowser(connectionId, async (browser) => {
+      await browser.mkdir(posix.join(directory, name))
+      return true
+    }),
+  )
+
+  handle(IPC.fsCreateFileRemote, CreateEntryRequestSchema, async ({ connectionId, directory, name }) =>
+    withBrowser(connectionId, async (browser) => {
+      await browser.createFile(posix.join(directory, name))
+      return true
+    }),
+  )
+
+  handle(IPC.fsRenameRemote, RenameEntryRequestSchema, async ({ connectionId, directory, from, to }) =>
+    withBrowser(connectionId, async (browser) => {
+      await browser.rename(posix.join(directory, from), posix.join(directory, to))
+      return true
+    }),
+  )
+
+  handle(IPC.fsDeleteRemote, DeleteEntryRequestSchema, async ({ connectionId, directory, name, isDirectory }) =>
+    withBrowser(connectionId, async (browser) => {
+      const target = posix.join(directory, name)
+      const stats = await browser.stat(target)
+      if (stats.type === 'symlink') {
+        await browser.unlink(target)
         return true
-      } finally {
-        browser.close()
       }
-    },
+      if ((stats.type === 'directory') !== isDirectory) {
+        throw new Error('That item changed on the server; refresh and try again.')
+      }
+      if (isDirectory) await browser.removeRecursive(target)
+      else await browser.unlink(target)
+      return true
+    }),
   )
 
   // --- transfers -----------------------------------------------------------

apps/desktop/electron/preload/index.ts

@@ -24,11 +24,20 @@ const api = {
     homeLocal: () => call<string>(IPC.fsHomeLocal),
     listLocal: (path: string) => call(IPC.fsListLocal, { path }),
     listRemote: (connectionId: string, path: string) => call(IPC.fsListRemote, { connectionId, path }),
-    mkdirRemote: (connectionId: string, path: string) => call(IPC.fsMkdirRemote, { connectionId, path }),
-    renameRemote: (connectionId: string, from: string, to: string) =>
-      call(IPC.fsRenameRemote, { connectionId, from, to }),
-    deleteRemote: (connectionId: string, path: string, isDirectory: boolean) =>
-      call(IPC.fsDeleteRemote, { connectionId, path, isDirectory }),
+    // Mutations name an entry inside a directory; the main process joins them.
+    mkdir: (directory: string, name: string, connectionId?: string) =>
+      call<boolean>(connectionId ? IPC.fsMkdirRemote : IPC.fsMkdirLocal, { connectionId, directory, name }),
+    createFile: (directory: string, name: string, connectionId?: string) =>
+      call<boolean>(connectionId ? IPC.fsCreateFileRemote : IPC.fsCreateFileLocal, { connectionId, directory, name }),
+    rename: (directory: string, from: string, to: string, connectionId?: string) =>
+      call<boolean>(connectionId ? IPC.fsRenameRemote : IPC.fsRenameLocal, { connectionId, directory, from, to }),
+    remove: (directory: string, name: string, isDirectory: boolean, connectionId?: string) =>
+      call<boolean>(connectionId ? IPC.fsDeleteRemote : IPC.fsDeleteLocal, {
+        connectionId,
+        directory,
+        name,
+        isDirectory,
+      }),
   },
   transfers: {
     preview: (request: unknown) => call(IPC.transfersPreview, request),