fix(courses): address review — dialog mount, empty-list, rename validation, partial-delete refresh, breadcrumb dedup

Per review (wyuc, CHANGES_REQUESTED):

1. Mount NewFolderDialog/DeleteFolderDialog outside the collapsible Recent
   subtree so they are reachable while it is collapsed; the New-folder
   button now expands the section before opening the dialog.
2. Keep the Recent/folder surface alive when the course list is empty so
   folders remain reachable and a first folder can be created.
3. Enforce folder-name validation (width + uniqueness) on rename as well
   as create, at the storage boundary (FolderNameError) and in the UI.
4. Refresh authoritative state on folder-delete both success and failure,
   so a partial "remove" failure does not leave stale cards/counts.
5. Drop the duplicated folder breadcrumb from the top section header;
   keep a single navigation breadcrumb in the content area.

Add focused storage tests covering rename validation, membership writes,
both deletion modes, and partial-failure propagation.

app/page.tsx

@@ -62,6 +62,7 @@ import {
   renameFolder,
   deleteFolder,
   setStageFolder,
+  FolderNameError,
   type DeleteFolderMode,
 } from '@/lib/utils/stage-storage';
 import type { FolderRecord } from '@/lib/utils/database';
@@ -319,11 +320,19 @@ function HomePage() {
   const handleRenameFolder =
     (folder: FolderRecord) =>
     async (newName: string): Promise<string | null> => {
+      // Empty or unchanged input just exits editing without an error.
+      const trimmed = newName.trim();
+      if (!trimmed || trimmed === folder.name) return null;
       try {
         await renameFolder(folder.id, newName);
-        setFolders((prev) => prev.map((f) => (f.id === folder.id ? { ...f, name: newName } : f)));
+        setFolders((prev) => prev.map((f) => (f.id === folder.id ? { ...f, name: trimmed } : f)));
         return null;
       } catch (err) {
+        if (err instanceof FolderNameError) {
+          if (err.kind === 'duplicate') return t('classroom.folderNameExists');
+          if (err.kind === 'tooLong') return t('classroom.folderWidth', { width: 0, max: 40 });
+          return t('classroom.folderNameHint');
+        }
         log.error('Failed to rename folder:', err);
         return t('classroom.folderRenameFailed');
       }
@@ -334,15 +343,15 @@ function HomePage() {
     if (!target) return;
     try {
       await deleteFolder(target.id, mode);
-      // 'remove' deletes the member courses via deleteStageData, so reload
-      // classrooms; 'ungroup' only changes membership, but reload is cheap and
-      // keeps the list authoritative.
-      await Promise.all([loadFolders(), loadClassrooms()]);
       if (currentFolderId === target.id) setCurrentFolderId(undefined);
     } catch (err) {
       log.error('Failed to delete folder:', err);
       toast.error(t('classroom.folderDeleteFailed'));
     } finally {
+      // Always refresh authoritative state: in 'remove' mode a partial failure
+      // may have durably deleted some courses before throwing, and the UI must
+      // reflect that rather than leaving stale cards/counts behind.
+      await Promise.all([loadFolders(), loadClassrooms()]);
       setDeleteTarget(null);
     }
   };
@@ -925,7 +934,10 @@ function HomePage() {
       </motion.div>
 
       {/* ═══ Recent classrooms — collapsible ═══ */}
-      {classrooms.length > 0 && (
+      {/* Keep the section alive as long as there is anything to show: courses,
+          folders, or — when the flag is on — the "New folder" affordance. This
+          avoids trapping persisted folders when the last course is deleted. */}
+      {(classrooms.length > 0 || effectiveFolders.length > 0 || COURSE_FOLDERS_ENABLED) && (
         <motion.div
           initial={{ opacity: 0 }}
           animate={{ opacity: 1 }}
@@ -945,14 +957,6 @@ function HomePage() {
               >
                 <Clock className="size-3.5" />
                 {t('classroom.recentClassrooms')}
-                {currentFolder && (
-                  <>
-                    <ChevronRight className="size-3 opacity-40" />
-                    <span className="text-foreground/70 truncate max-w-[160px]">
-                      {currentFolder.name}
-                    </span>
-                  </>
-                )}
                 <span className="text-[11px] tabular-nums opacity-60">{classrooms.length}</span>
                 <motion.div
                   animate={{ rotate: recentOpen ? 180 : 0 }}
@@ -1070,7 +1074,10 @@ function HomePage() {
               {COURSE_FOLDERS_ENABLED && !currentFolderId && !isSearching && (
                 <button
                   type="button"
-                  onClick={() => setNewFolderOpen(true)}
+                  onClick={() => {
+                    if (!recentOpen) persistRecentOpen(true);
+                    setNewFolderOpen(true);
+                  }}
                   aria-label={t('classroom.newFolderTitle')}
                   title={t('classroom.newFolderTitle')}
                   className="inline-flex items-center justify-center size-7 rounded-full bg-muted/40 text-muted-foreground ring-1 ring-border/50 hover:bg-muted hover:text-foreground hover:ring-border transition-[background-color,color,box-shadow] cursor-pointer"
@@ -1235,23 +1242,6 @@ function HomePage() {
                         ))}
                       </motion.div>
                     </AnimatePresence>
-
-                    <NewFolderDialog
-                      open={newFolderOpen}
-                      onOpenChange={(open) => {
-                        setNewFolderOpen(open);
-                        if (!open) setCreateAndMoveTarget(null);
-                      }}
-                      folders={effectiveFolders}
-                      onCreate={handleCreateFolder}
-                    />
-                    <DeleteFolderDialog
-                      open={deleteTarget !== null}
-                      onOpenChange={(open) => !open && setDeleteTarget(null)}
-                      folder={deleteTarget}
-                      courseCount={deleteTargetCourseCount}
-                      onConfirm={confirmDeleteFolder}
-                    />
                   </div>
                 )}
               </motion.div>
@@ -1260,6 +1250,29 @@ function HomePage() {
         </motion.div>
       )}
 
+      {/* Folder dialogs — mounted at the top level so they are reachable even
+          while the Recent section is collapsed or the course list is empty. */}
+      {COURSE_FOLDERS_ENABLED && (
+        <>
+          <NewFolderDialog
+            open={newFolderOpen}
+            onOpenChange={(open) => {
+              setNewFolderOpen(open);
+              if (!open) setCreateAndMoveTarget(null);
+            }}
+            folders={effectiveFolders}
+            onCreate={handleCreateFolder}
+          />
+          <DeleteFolderDialog
+            open={deleteTarget !== null}
+            onOpenChange={(open) => !open && setDeleteTarget(null)}
+            folder={deleteTarget}
+            courseCount={deleteTargetCourseCount}
+            onConfirm={confirmDeleteFolder}
+          />
+        </>
+      )}
+
       {/* Footer — flows with content, at the very end */}
       <div className="mt-auto pt-12 pb-4 text-center text-xs text-muted-foreground/40">
         OpenMAIC Open Source Project

lib/utils/stage-storage.ts

@@ -10,6 +10,7 @@ import { ChatSession } from '../types/chat';
 import { db } from './database';
 import type { FolderRecord } from './database';
 import { nanoid } from 'nanoid';
+import { validateFolderName } from './folder-name-validation';
 import {
   ChatStorageLockUnavailableError,
   saveChatSessions,
@@ -897,17 +898,46 @@ export async function listFolders(): Promise<FolderRecord[]> {
   return folders.sort((a, b) => a.order - b.order);
 }
 
+/** Error thrown when a folder name fails validation at the storage boundary. */
+export class FolderNameError extends Error {
+  constructor(
+    message: string,
+    readonly kind: 'empty' | 'tooLong' | 'duplicate',
+  ) {
+    super(message);
+    this.name = 'FolderNameError';
+  }
+}
+
+/** Validate a folder name against the width rule and (optionally) duplicates. */
+function assertFolderName(name: string, existing: FolderRecord[], currentId?: string): void {
+  const result = validateFolderName(name);
+  if (!result.ok) {
+    throw new FolderNameError(
+      result.kind === 'empty' ? 'Folder name must not be empty' : 'Folder name is too long',
+      result.kind,
+    );
+  }
+  const trimmed = name.trim();
+  const clash = existing.some(
+    (f) => f.name.toLowerCase() === trimmed.toLowerCase() && f.id !== currentId,
+  );
+  if (clash) throw new FolderNameError('A folder with this name already exists', 'duplicate');
+}
+
 /**
  * Create a folder. `order` is placed after the current maximum so new folders
- * land at the end of the list.
+ * land at the end of the list. Validates the name (width + uniqueness) at the
+ * storage boundary.
  */
 export async function createFolder(name: string): Promise<FolderRecord> {
   const now = Date.now();
   const existing = await db.folders.toArray();
+  assertFolderName(name, existing);
   const order = existing.reduce((max, folder) => Math.max(max, folder.order), -1) + 1;
   const folder: FolderRecord = {
     id: nanoid(),
-    name,
+    name: name.trim(),
     order,
     createdAt: now,
     updatedAt: now,
@@ -918,13 +948,16 @@ export async function createFolder(name: string): Promise<FolderRecord> {
 }
 
 /**
- * Rename a folder.
+ * Rename a folder. Validates the name (width + uniqueness excluding itself) at
+ * the storage boundary, so the UI invariant cannot be bypassed.
  */
 export async function renameFolder(id: string, name: string): Promise<void> {
   const now = Date.now();
-  const folder = await db.folders.get(id);
+  const existing = await db.folders.toArray();
+  assertFolderName(name, existing, id);
+  const folder = existing.find((f) => f.id === id);
   if (!folder) throw new Error(`Folder not found: ${id}`);
-  await db.folders.put({ ...folder, name, updatedAt: now });
+  await db.folders.put({ ...folder, name: name.trim(), updatedAt: now });
   log.info(`Renamed folder ${id} to "${name}"`);
 }
 

tests/utils/folder-storage.test.ts

@@ -0,0 +1,212 @@
+/* eslint-disable @typescript-eslint/no-explicit-any -- test-only db mocks */
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+
+/**
+ * Folder storage contract: name validation at the boundary, membership writes,
+ * and both folder-deletion modes including partial failure.
+ *
+ * `deleteStageData` (used by deleteFolder('remove')) runs a full cascade, so
+ * its dependencies are mocked the same way the deletion-wiring tests do —
+ * only the folder tables (`folders`, `stageFolders`) use in-memory maps.
+ */
+
+const { folders, memberships, dbMock, mutateDocumentMock } = vi.hoisted(() => {
+  const folders = new Map<string, Record<string, unknown>>();
+  const memberships = new Map<string, Record<string, unknown>>();
+
+  const tableFrom = (store: Map<string, Record<string, unknown>>, idKey: string) => ({
+    put: vi.fn(async (row: Record<string, unknown>) => {
+      store.set(row[idKey] as string, row);
+    }),
+    get: vi.fn(async (id: string) => store.get(id)),
+    delete: vi.fn(async (id: string) => {
+      store.delete(id);
+    }),
+    toArray: vi.fn(async () => [...store.values()]),
+    where: (field: string) =>
+      ({
+        equals: (value: unknown) => ({
+          toArray: vi.fn(async () => [...store.values()].filter((r) => r[field] === value)),
+          delete: vi.fn(async () => {
+            for (const [k, v] of store) if (v[field] === value) store.delete(k);
+          }),
+        }),
+      }) as any,
+  });
+
+  return {
+    folders,
+    memberships,
+    dbMock: {
+      folders: tableFrom(folders, 'id'),
+      stageFolders: tableFrom(memberships, 'stageId'),
+      stages: { delete: vi.fn().mockResolvedValue(undefined) },
+      scenes: {
+        where: () => ({
+          equals: () => ({
+            toArray: vi.fn().mockResolvedValue([]),
+            delete: vi.fn().mockResolvedValue(undefined),
+          }),
+        }),
+      },
+      stageOutlines: { delete: vi.fn().mockResolvedValue(undefined) },
+      playbackState: { delete: vi.fn().mockResolvedValue(undefined) },
+      generatedAgents: {
+        where: () => ({ equals: () => ({ delete: vi.fn().mockResolvedValue(undefined) }) }),
+      },
+      transaction: vi.fn(async (_mode: string, _tables: unknown[], fn: () => Promise<void>) =>
+        fn(),
+      ),
+    },
+    mutateDocumentMock: vi.fn(
+      async (
+        _id: string,
+        fn: (doc: unknown, store: { deleteDocument: () => Promise<void> }) => Promise<void>,
+      ) => fn(undefined, { deleteDocument: vi.fn().mockResolvedValue(undefined) }),
+    ),
+  } as any;
+});
+
+vi.mock('@/lib/utils/database', () => ({ db: dbMock }));
+vi.mock('@/lib/document-store', () => ({
+  accessDocument: vi.fn(),
+  clearCurrentScene: vi.fn().mockResolvedValue(undefined),
+  getDocumentStore: vi.fn(),
+  getLegacyDocumentStore: vi.fn(),
+  loadCurrentScene: vi.fn().mockResolvedValue(null),
+  mutateDocument: mutateDocumentMock,
+  saveCurrentScene: vi.fn().mockResolvedValue(undefined),
+}));
+vi.mock('@/lib/utils/chat-storage', () => ({
+  ChatStorageLockUnavailableError: class extends Error {},
+  saveChatSessions: vi.fn().mockResolvedValue(undefined),
+  loadChatSessions: vi.fn().mockResolvedValue([]),
+  deleteChatSessions: vi.fn().mockResolvedValue(undefined),
+}));
+vi.mock('@/lib/utils/chat-storage-lock', () => ({
+  withRuntimeStorageSharedLock: vi.fn(async (fn: () => Promise<unknown>) => fn()),
+  withRuntimeStorageExclusiveLockUntilSettled: vi.fn(
+    async (fn: (release: (v?: unknown) => void) => Promise<unknown>) => fn(() => {}),
+  ),
+}));
+vi.mock('@/lib/playback/cursor', () => ({ clearCursor: vi.fn() }));
+vi.mock('@/lib/quiz/persistence', () => ({ clearAllForScene: vi.fn() }));
+vi.mock('@/lib/runtime/store', () => ({
+  beginStageRuntimeDeletionSafely: vi.fn(() => ({
+    completion: Promise.resolve(),
+    settlement: Promise.resolve(),
+  })),
+}));
+vi.mock('@/lib/pbl/v2/runtime/drain', () => ({ clearStageDrainWatermarks: vi.fn() }));
+vi.mock('@/lib/store/stage', () => ({
+  clearStoreForDeletedStage: vi.fn(),
+  discardPendingStageChanges: vi.fn(),
+  snapshotPendingStageChangesForDeletion: vi.fn().mockReturnValue([]),
+  restorePendingStageChanges: vi.fn(),
+}));
+vi.mock('@/lib/pbl/v2/runtime/document-persistence', () => ({
+  preparePBLScenesForDocumentPersistence: vi.fn(async (_id: string, scenes: unknown[]) => scenes),
+}));
+
+import {
+  createFolder,
+  renameFolder,
+  deleteFolder,
+  setStageFolder,
+  FolderNameError,
+} from '@/lib/utils/stage-storage';
+
+beforeEach(() => {
+  folders.clear();
+  memberships.clear();
+});
+
+describe('createFolder / renameFolder name validation', () => {
+  it('rejects an empty name on create', async () => {
+    await expect(createFolder('   ')).rejects.toMatchObject({ kind: 'empty' });
+  });
+
+  it('rejects an over-width name on create', async () => {
+    await expect(createFolder('a'.repeat(41))).rejects.toMatchObject({ kind: 'tooLong' });
+  });
+
+  it('rejects a duplicate name (case-insensitive) on create', async () => {
+    await createFolder('Math');
+    await expect(createFolder('math')).rejects.toMatchObject({ kind: 'duplicate' });
+  });
+
+  it('trims the stored name', async () => {
+    const f = await createFolder('  Physics  ');
+    expect(f.name).toBe('Physics');
+  });
+
+  it('rejects an empty name on rename', async () => {
+    const f = await createFolder('Old');
+    await expect(renameFolder(f.id, '  ')).rejects.toMatchObject({ kind: 'empty' });
+  });
+
+  it('rejects a duplicate name on rename, but allows keeping the current name', async () => {
+    const a = await createFolder('Alpha');
+    await createFolder('Beta');
+    await expect(renameFolder(a.id, 'Beta')).rejects.toMatchObject({ kind: 'duplicate' });
+    await expect(renameFolder(a.id, 'Alpha')).resolves.toBeUndefined();
+  });
+});
+
+describe('setStageFolder membership', () => {
+  it('writes a membership row keyed by stageId', async () => {
+    await setStageFolder('stage-1', 'folder-1');
+    const row = await dbMock.stageFolders.get('stage-1');
+    expect(row).toMatchObject({ stageId: 'stage-1', folderId: 'folder-1' });
+  });
+
+  it('moving out stores folderId undefined', async () => {
+    await setStageFolder('stage-1', undefined);
+    const row = await dbMock.stageFolders.get('stage-1');
+    expect(row.folderId).toBeUndefined();
+  });
+});
+
+describe('deleteFolder', () => {
+  it("'ungroup' drops the folder and clears its memberships", async () => {
+    const folder = await createFolder('Group A');
+    await setStageFolder('s1', folder.id);
+    await setStageFolder('s2', folder.id);
+
+    await deleteFolder(folder.id, 'ungroup');
+
+    expect(await dbMock.folders.get(folder.id)).toBeUndefined();
+    expect(await dbMock.stageFolders.get('s1')).toBeUndefined();
+    expect(await dbMock.stageFolders.get('s2')).toBeUndefined();
+  });
+
+  it("'remove' drops the folder and its membership rows", async () => {
+    const folder = await createFolder('Group B');
+    await setStageFolder('s1', folder.id);
+    await setStageFolder('s2', folder.id);
+
+    await deleteFolder(folder.id, 'remove');
+
+    // The full deleteStageData cascade is mocked to no-op, but the folder row
+    // and memberships are dropped by deleteFolder itself.
+    expect(await dbMock.folders.get(folder.id)).toBeUndefined();
+  });
+
+  it("'remove' propagates a member-deletion failure (partial failure surfaces)", async () => {
+    const folder = await createFolder('Group C');
+    await setStageFolder('s1', folder.id);
+    // Force the deletion cascade to throw by making mutateDocument reject.
+    mutateDocumentMock.mockRejectedValueOnce(new Error('boom'));
+    await expect(deleteFolder(folder.id, 'remove')).rejects.toThrow('boom');
+    // Restore the default implementation for subsequent tests.
+    mutateDocumentMock.mockResolvedValueOnce(undefined);
+  });
+});
+
+describe('FolderNameError', () => {
+  it('carries a machine-readable kind', () => {
+    const e = new FolderNameError('msg', 'duplicate');
+    expect(e.kind).toBe('duplicate');
+    expect(e).toBeInstanceOf(Error);
+  });
+});