Updates to cell renaming behavior and UI (#133)

packages/api/server/ws.mts

@@ -7,6 +7,7 @@ import {
   readPackageJsonContentsFromDisk,
   updateCell,
   removeCell,
+  updateCodeCellFilename,
 } from '../session.mjs';
 import { getSecrets } from '../config.mjs';
 import type { SessionType } from '../types.mjs';
@@ -24,20 +25,23 @@ import type {
   TsServerStartPayloadType,
   TsServerStopPayloadType,
   CellDeletePayloadType,
+  CellRenamePayloadType,
+  CellErrorType,
 } from '@srcbook/shared';
 import {
   CellErrorPayloadSchema,
   CellUpdatePayloadSchema,
+  CellUpdatedPayloadSchema,
+  CellRenamePayloadSchema,
+  CellDeletePayloadSchema,
   CellExecPayloadSchema,
   CellStopPayloadSchema,
   DepsInstallPayloadSchema,
   DepsValidatePayloadSchema,
-  CellUpdatedPayloadSchema,
   CellOutputPayloadSchema,
   DepsValidateResponsePayloadSchema,
   TsServerStartPayloadSchema,
   TsServerStopPayloadSchema,
-  CellDeletePayloadSchema,
   TsServerCellDiagnosticsPayloadSchema,
 } from '@srcbook/shared';
 import tsservers from '../tsservers.mjs';
@@ -259,6 +263,37 @@ async function cellStop(payload: CellStopPayloadType) {
   }
 }
 
+function sendCellUpdateError(session: SessionType, cellId: string, errors: CellErrorType[]) {
+  wss.broadcast(`session:${session.id}`, 'cell:error', {
+    sessionId: session.id,
+    cellId: cellId,
+    errors: errors,
+  });
+
+  // Revert the client's optimistic updates with most recent server cell state
+  wss.broadcast(`session:${session.id}`, 'cell:updated', {
+    cell: findCell(session, cellId),
+  });
+}
+
+function reopenFileInTsServer(
+  tsserver: TsServer,
+  session: SessionType,
+  file: { closeFilename: string; openFilename: string; source: string },
+) {
+  // These two are usually the same unless a file is being renamed.
+  const closeFilePath = pathToCodeFile(session.dir, file.closeFilename);
+  const openFilePath = pathToCodeFile(session.dir, file.openFilename);
+
+  // To update a file in tsserver, close and reopen it. I assume performance of
+  // this implementation is worse than calculating diffs and using `change` command
+  // (although maybe not since this is not actually reading or writing to disk).
+  // However, that requires calculating diffs which is more complex and may also
+  // have performance implications, so sticking with the simple approach for now.
+  tsserver.close({ file: closeFilePath });
+  tsserver.open({ file: openFilePath, fileContent: file.source });
+}
+
 async function cellUpdate(payload: CellUpdatePayloadType) {
   const session = await findSession(payload.sessionId);
 
@@ -277,18 +312,55 @@ async function cellUpdate(payload: CellUpdatePayloadType) {
   const result = await updateCell(session, cellBeforeUpdate, payload.updates);
 
   if (!result.success) {
-    wss.broadcast(`session:${session.id}`, 'cell:error', {
-      sessionId: session.id,
-      cellId: payload.cellId,
-      errors: result.errors,
-    });
+    return sendCellUpdateError(session, payload.cellId, result.errors);
+  }
+
+  const cell = result.cell as CodeCellType;
+
+  if (
+    session.metadata.language === 'typescript' &&
+    cell.type === 'code' &&
+    tsservers.has(session.id)
+  ) {
+    const tsserver = tsservers.get(session.id);
 
-    // Revert the client's optimistic updates with most recent server cell state
-    wss.broadcast(`session:${session.id}`, 'cell:updated', {
-      cell: findCell(session, payload.cellId),
+    // This isn't intended for renaming, so the filenames
+    // and their resulting paths are expected to be the same
+    reopenFileInTsServer(tsserver, session, {
+      openFilename: cell.filename,
+      closeFilename: cell.filename,
+      source: cell.source,
     });
 
-    return;
+    requestAllDiagnostics(tsserver, session);
+  }
+}
+
+async function cellRename(payload: CellRenamePayloadType) {
+  const session = await findSession(payload.sessionId);
+
+  if (!session) {
+    throw new Error(`No session exists for session '${payload.sessionId}'`);
+  }
+
+  const cellBeforeUpdate = findCell(session, payload.cellId);
+
+  if (!cellBeforeUpdate) {
+    throw new Error(
+      `No cell exists for session '${payload.sessionId}' and cell '${payload.cellId}'`,
+    );
+  }
+
+  if (cellBeforeUpdate.type !== 'code') {
+    throw new Error(
+      `Cannot rename cell of type '${cellBeforeUpdate.type}'. Only code cells can be renamed.`,
+    );
+  }
+
+  const result = await updateCodeCellFilename(session, cellBeforeUpdate, payload.filename);
+
+  if (!result.success) {
+    return sendCellUpdateError(session, payload.cellId, result.errors);
   }
 
   if (
@@ -299,40 +371,31 @@ async function cellUpdate(payload: CellUpdatePayloadType) {
     const cellAfterUpdate = result.cell as CodeCellType;
     const tsserver = tsservers.get(session.id);
 
-    // These are usually the same. However, if the user renamed the cell, we need to
-    // ensure that we close the old file in tsserver and open the new one.
-    const oldFilePath = pathToCodeFile(session.dir, cellBeforeUpdate.filename);
-    const newFilePath = pathToCodeFile(session.dir, cellAfterUpdate.filename);
-
-    // To update a file in tsserver, close and reopen it. I assume performance of
-    // this implementation is worse than calculating diffs and using `change` command
-    // (although maybe not since this is not actually reading or writing to disk).
-    // However, that requires calculating diffs which is more complex and may also
-    // have performance implications, so sticking with the simple approach for now.
-    tsserver.close({ file: oldFilePath });
-    tsserver.open({ file: newFilePath, fileContent: cellAfterUpdate.source });
-
-    // TODO: Given the amount of differences here and elsewhere when renaming cells,
-    // it's probably worth it at this point to make those separate websocket events.
-    if (oldFilePath !== newFilePath) {
-      // Tsserver can get into a bad state if we don't reload the project after renaming a file.
-      // This consistently happens under the following condition:
-      //
-      // 1. Rename a `a.ts` that is imported by `b.ts` to `c.ts`
-      // 2. Semantic diagnostics report an error in `b.ts` that `a.ts` doesn't exist
-      // 3. Great, all works so far.
-      // 4. Rename `c.ts` back to `a.ts`.
-      // 5. Semantic diagnostics still report an error in `b.ts` that `a.ts` doesn't exist.
-      // 6. This is wrong, `a.ts` does exist.
-      //
-      // If we reload the project, this issue resolves itself.
-      //
-      // NOTE: reloading the project sends diagnostic events without calling `geterr`.
-      // However, it seems to take a while for the diagnostics to be sent, so we still
-      // request it below.
-      //
-      tsserver.reloadProjects();
-    }
+    // This function is specifically for renaming code cells. Thus,
+    // the filenames before and after the update should be different.
+    reopenFileInTsServer(tsserver, session, {
+      closeFilename: cellBeforeUpdate.filename,
+      openFilename: cellAfterUpdate.filename,
+      source: cellAfterUpdate.source,
+    });
+
+    // Tsserver can get into a bad state if we don't reload the project after renaming a file.
+    // This consistently happens under the following condition:
+    //
+    // 1. Rename a `a.ts` that is imported by `b.ts` to `c.ts`
+    // 2. Semantic diagnostics report an error in `b.ts` that `a.ts` doesn't exist
+    // 3. Great, all works so far.
+    // 4. Rename `c.ts` back to `a.ts`.
+    // 5. Semantic diagnostics still report an error in `b.ts` that `a.ts` doesn't exist.
+    // 6. This is wrong, `a.ts` does exist.
+    //
+    // If we reload the project, this issue resolves itself.
+    //
+    // NOTE: reloading the project sends diagnostic events without calling `geterr`.
+    // However, it seems to take a while for the diagnostics to be sent, so we still
+    // request it below.
+    //
+    tsserver.reloadProjects();
 
     requestAllDiagnostics(tsserver, session);
   }
@@ -450,6 +513,7 @@ wss
   .incoming('cell:exec', CellExecPayloadSchema, cellExec)
   .incoming('cell:stop', CellStopPayloadSchema, cellStop)
   .incoming('cell:update', CellUpdatePayloadSchema, cellUpdate)
+  .incoming('cell:rename', CellRenamePayloadSchema, cellRename)
   .incoming('cell:delete', CellDeletePayloadSchema, cellDelete)
   .incoming('deps:install', DepsInstallPayloadSchema, depsInstall)
   .incoming('deps:validate', DepsValidatePayloadSchema, depsValidate)

packages/api/session.mts

@@ -28,6 +28,7 @@ import {
 } from './srcbook/index.mjs';
 import { fileExists } from './fs-utils.mjs';
 import { validFilename } from '@srcbook/shared';
+import { pathToCodeFile } from './srcbook/path.mjs';
 
 const sessions: Record<string, SessionType> = {};
 
@@ -174,27 +175,46 @@ async function updateCodeCell(
   updates: any,
 ): Promise<UpdateResultType> {
   const attrs = CodeCellUpdateAttrsSchema.parse(updates);
+  return updateCellWithRollback(session, cell, { ...attrs }, async (session, updatedCell) => {
+    try {
+      await writeCellToDisk(
+        session.dir,
+        session.metadata,
+        session.cells,
+        updatedCell as CodeCellType,
+      );
+    } catch (e) {
+      console.error(e);
+      return [{ message: 'An error occurred persisting files to disk' }];
+    }
+  });
+}
 
-  // This shouldn't happen but it will cause the code below to fail
-  // when it shouldn't, so here we check if a mistake was made, log it,
-  // and ignore this attribute.
-  if (attrs.filename === cell.filename) {
+/**
+ * Use this to rename a code cell's filename.
+ */
+export async function updateCodeCellFilename(
+  session: SessionType,
+  cell: CodeCellType,
+  filename: string,
+): Promise<UpdateResultType> {
+  if (filename === cell.filename) {
     console.warn(
       `Attempted to update a cell's filename to its existing filename '${cell.filename}'. This is likely a bug in the code.`,
     );
-    delete attrs.filename;
+    return { success: true, cell };
   }
 
-  if (attrs.filename && !validFilename(attrs.filename)) {
+  if (!validFilename(filename)) {
     return {
       success: false,
-      errors: [{ message: `${attrs.filename} is not a valid filename`, attribute: 'filename' }],
+      errors: [{ message: `${filename} is not a valid filename`, attribute: 'filename' }],
     };
   }
 
   const language = session.metadata.language;
 
-  if (attrs.filename && language !== languageFromFilename(attrs.filename)) {
+  if (language !== languageFromFilename(filename)) {
     return {
       success: false,
       errors: [
@@ -206,34 +226,22 @@ async function updateCodeCell(
     };
   }
 
-  if (attrs.filename && (await fileExists(Path.join(session.dir, attrs.filename)))) {
+  if (await fileExists(pathToCodeFile(session.dir, filename))) {
     return {
       success: false,
-      errors: [
-        { message: `A file named '${attrs.filename}' already exists`, attribute: 'filename' },
-      ],
+      errors: [{ message: `A file named '${filename}' already exists`, attribute: 'filename' }],
     };
   }
 
-  const isChangingFilename = !!attrs.filename;
-
-  return updateCellWithRollback(session, cell, { ...attrs }, async (session, updatedCell) => {
+  return updateCellWithRollback(session, cell, { filename }, async (session, updatedCell) => {
     try {
-      const writes = isChangingFilename
-        ? moveCodeCellOnDisk(
-            session.dir,
-            session.metadata,
-            session.cells,
-            updatedCell as CodeCellType,
-            cell.filename,
-          )
-        : writeCellToDisk(
-            session.dir,
-            session.metadata,
-            session.cells,
-            updatedCell as CodeCellType,
-          );
-      await writes;
+      await moveCodeCellOnDisk(
+        session.dir,
+        session.metadata,
+        session.cells,
+        updatedCell as CodeCellType,
+        cell.filename,
+      );
     } catch (e) {
       console.error(e);
       return [{ message: 'An error occurred persisting files to disk' }];

packages/shared/src/schemas/cells.ts

@@ -56,9 +56,10 @@ export const PackageJsonCellUpdateAttrsSchema = z.object({
   source: z.string(),
 });
 
+// filename not allowed here because renaming
+// a file has a separate websocket message.
 export const CodeCellUpdateAttrsSchema = z.object({
-  source: z.string().optional(),
-  filename: z.string().optional(),
+  source: z.string(),
 });
 
 export const CellUpdateAttrsSchema = z.union([

packages/shared/src/schemas/websockets.ts

@@ -18,6 +18,12 @@ export const CellUpdatePayloadSchema = z.object({
   updates: CellUpdateAttrsSchema,
 });
 
+export const CellRenamePayloadSchema = z.object({
+  sessionId: z.string(),
+  cellId: z.string(),
+  filename: z.string(),
+});
+
 export const CellDeletePayloadSchema = z.object({
   sessionId: z.string(),
   cellId: z.string(),

packages/shared/src/types/websockets.ts

@@ -5,6 +5,7 @@ import {
   CellStopPayloadSchema,
   CellUpdatePayloadSchema,
   CellUpdatedPayloadSchema,
+  CellRenamePayloadSchema,
   CellDeletePayloadSchema,
   CellOutputPayloadSchema,
   DepsInstallPayloadSchema,
@@ -20,6 +21,7 @@ export type CellExecPayloadType = z.infer<typeof CellExecPayloadSchema>;
 export type CellStopPayloadType = z.infer<typeof CellStopPayloadSchema>;
 export type CellUpdatePayloadType = z.infer<typeof CellUpdatePayloadSchema>;
 export type CellUpdatedPayloadType = z.infer<typeof CellUpdatedPayloadSchema>;
+export type CellRenamePayloadType = z.infer<typeof CellRenamePayloadSchema>;
 export type CellDeletePayloadType = z.infer<typeof CellDeletePayloadSchema>;
 export type CellOutputPayloadType = z.infer<typeof CellOutputPayloadSchema>;
 

packages/web/src/clients/websocket/index.ts

@@ -12,6 +12,7 @@ import {
   TsServerStopPayloadSchema,
   CellDeletePayloadSchema,
   TsServerCellDiagnosticsPayloadSchema,
+  CellRenamePayloadSchema,
 } from '@srcbook/shared';
 
 import Channel from '@/clients/websocket/channel';
@@ -33,6 +34,7 @@ const OutgoingSessionEvents = {
   'cell:exec': CellExecPayloadSchema,
   'cell:stop': CellStopPayloadSchema,
   'cell:update': CellUpdatePayloadSchema,
+  'cell:rename': CellRenamePayloadSchema,
   'cell:delete': CellDeletePayloadSchema,
   'deps:install': DepsInstallPayloadSchema,
   'deps:validate': DepsValidatePayloadSchema,