docs(folders): correct four comments that claimed more than the code does (#6063)

A comment pass over this branch found four factual errors, all mine:

- the admin route test justified its column-pinned assertion with reasoning
  copied from lifecycle.test.ts — that condition has no second isNull node, so
  the stated vacuity could not occur there
- 'every clause mirrors one column of the partial unique index' was wrong:
  deletedAt is the index PREDICATE, and name has no clause at all
- 'optimistic client reparents can write a cycle' is unsupported —
  wouldCreateFolderCycle has one caller and no write path bypasses it; the real
  origin is two concurrent reparents each passing the check
- 'restoreFolder runs restoreChildren BEFORE un-archiving' holds only for the
  injected hook path; the in-transaction fallback runs after restoreFolderRows

The rest trims comments that restated the code or duplicated a TSDoc block on
the module under test, and pins three presence-only isNull assertions to the
deletedAt column so they cannot be satisfied by an unrelated nullable filter —
the same fragility that made an earlier assertion here vacuous.

Author: waleedlatif1

apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts

@@ -39,11 +39,6 @@ describe('admin workspace folders GET', () => {
     resetDbChainMock()
   })
 
-  /**
-   * Both the count and the page must exclude soft-deleted folders. Without the filter an operator
-   * inspecting a workspace sees folders that live in Recently Deleted and an inflated total, and
-   * this endpoint disagrees with every user-facing folder list — all of which filter `deletedAt`.
-   */
   it('excludes soft-deleted folders from both the count and the page', async () => {
     queueTableRows(schemaMock.workspace, [{ id: WORKSPACE_ID }])
     queueTableRows(schemaMock.folder, [{ total: 0 }])
@@ -55,8 +50,8 @@ describe('admin workspace folders GET', () => {
     const folderWheres = dbChainMockFns.where.mock.calls.slice(1).map(([where]) => where)
     expect(folderWheres.length).toBeGreaterThanOrEqual(2)
     for (const where of folderWheres) {
-      // Asserted on the COLUMN: `resourceType`/`workspaceId` are eq nodes, so a bare
-      // "some isNull exists" check could pass on an unrelated clause.
+      // Pinned to the column so the assertion stays meaningful if another nullable filter
+      // (e.g. a parent scope) is ever added to this condition.
       expect(
         flattenMockConditions(where).some(
           (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt

apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts

@@ -47,10 +47,9 @@ export const GET = withRouteHandler(
       }
 
       /**
-       * Soft-deleted folders are excluded. Without this the count and the page both include rows
-       * sitting in Recently Deleted, so an operator inspecting a workspace sees phantom folders
-       * and an inflated total — and the two disagree with every user-facing folder list, all of
-       * which filter on `deletedAt`.
+       * Without the `deletedAt` filter the count and the page both include rows sitting in
+       * Recently Deleted, so an operator sees phantom folders and an inflated total, and this
+       * endpoint disagrees with every user-facing folder list.
        */
       const activeWorkflowFolders = and(
         eq(folderTable.workspaceId, workspaceId),

apps/sim/lib/folders/lifecycle.test.ts

@@ -231,13 +231,9 @@ describe('createFolder', () => {
     expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ sortOrder: -3 }))
   })
 
+  // Asserted on the WHERE clauses, not the resulting sortOrder: the mock returns whatever was
+  // queued regardless of the filter, so a sortOrder assertion passes without either clause.
   it('ignores soft-deleted folders and resources when picking the new sortOrder', async () => {
-    /**
-     * `min - 1` means archived rows would ratchet the floor further negative on every delete and
-     * never recover. Both minima must therefore see only rows a user can still see. Asserted on
-     * the WHERE clauses because the mock returns whatever is queued regardless of the filter, so
-     * an assertion on the resulting sortOrder alone would pass without either clause.
-     */
     setConfig({
       resourceType: 'workflow',
       countKey: 'workflows',
@@ -254,8 +250,8 @@ describe('createFolder', () => {
       .map(([where]) => where)
 
     // Assert on the specific COLUMN, not merely that some isNull exists: for a root folder the
-    // parent condition is itself `isNull(parentId)`, so a presence-only check passes with the
-    // soft-delete filter deleted. That made the first version of this test vacuous.
+    // parent condition is itself `isNull(parentId)`, so a presence-only check stays green with
+    // the soft-delete filter deleted.
     expect(
       flattenMockConditions(folderWhere).some(
         (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt

apps/sim/lib/folders/lifecycle.ts

@@ -129,11 +129,9 @@ export async function nextFolderSortOrder(
     : isNull(folderTable.parentId)
 
   /**
-   * Soft-deleted rows are excluded from both minima. This function returns `min - 1` to put a
-   * new folder at the top, so counting archived rows lets every delete ratchet the floor further
-   * negative and never recover — an archived folder at -400 forces the next new folder to -401
-   * forever. Only rows a user can actually see should influence the ordering. The Files path
-   * (`workspace-file-folder-manager`) has always filtered this way.
+   * Both minima exclude soft-deleted rows. This returns `min - 1` to put a new folder at the
+   * top, so counting archived rows lets every delete ratchet the floor further negative and
+   * never recover — an archived folder at -400 pins the next new folder at -401 forever.
    */
   const folderMinPromise = tx
     .select({ minSortOrder: min(folderTable.sortOrder) })

apps/sim/lib/folders/naming.test.ts

@@ -29,11 +29,8 @@ function makeTx(siblingNames: string[]) {
 }
 
 /**
- * The suffix shape is a cross-surface contract: the client's `nextUntitledFolderName` and
- * migration 0272's backfill both produce `"<name> (N)"` starting at (1). A server-side drift
- * either collides on `folder_workspace_resource_parent_name_active_unique` (23505 on a path the
- * user cannot retry) or renders a folder named differently depending on how it was created.
- * Nothing asserted this before — every caller mocks this module out.
+ * Pins the `"<name> (N)"` shape the client and migration 0272 also produce (see `naming.ts`).
+ * Every caller mocks this module out, so nothing else in the suite covers it.
  */
 describe('deduplicateFolderName', () => {
   it('returns the requested name untouched when no sibling holds it', async () => {
@@ -43,8 +40,6 @@ describe('deduplicateFolderName', () => {
   })
 
   it('starts the suffix at (1), not (2)', async () => {
-    // A loop seeded at 2 — the shape of a bug already fixed twice in this feature — yields
-    // "Reports (2)" here and silently diverges from the client and the migration.
     const { tx } = makeTx(['Reports'])
 
     expect(await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')).toBe('Reports (1)')
@@ -73,9 +68,9 @@ describe('deduplicateFolderName', () => {
   })
 
   /**
-   * The sibling query defines the namespace the suffix is chosen within. Every clause below
-   * mirrors one column of the partial unique index — dropping any of them counts the wrong rows
-   * and either inflates the suffix or picks a name that is already taken.
+   * The sibling query defines the namespace the suffix is chosen within, and must match the
+   * scope of the partial unique index — workspace, resourceType, parent, active-only. Drop any
+   * clause and it counts the wrong rows, inflating the suffix or picking a taken name.
    */
   describe('sibling scoping', () => {
     it('scopes to workspace, resourceType, root parent, and active rows', async () => {
@@ -111,6 +106,7 @@ describe('deduplicateFolderName', () => {
 
       await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')
 
+      // Two isNull nodes: the root parent scope and the soft-delete filter.
       expect(
         flattenMockConditions(selectCalls[0].where).filter((n) => n.type === 'isNull')
       ).toHaveLength(2)

apps/sim/lib/folders/queries.test.ts

@@ -43,12 +43,9 @@ describe('folder queries', () => {
   })
 
   /**
-   * These are the id-keyed lookups. Every other query in the feature is already scoped by
-   * workspace + resourceType through a list filter, but these accept a caller-supplied id — so
-   * they are the one place a missing `resource_type` clause silently crosses resource trees,
-   * filing a knowledge base under a table folder where no page will ever render it. Nothing
-   * asserted this before: every caller mocks this module out, so deleting the clause left the
-   * whole suite green.
+   * The only lookup keyed on a caller-supplied folder id, so the `resourceType` clause is the
+   * sole guard against crossing resource trees (rationale in `findActiveFolder`). Every caller
+   * mocks this module out, so deleting that clause used to leave the whole suite green.
    */
   describe('findActiveFolder', () => {
     it('scopes by id, workspace, resourceType, and active state', async () => {
@@ -63,7 +60,13 @@ describe('folder queries', () => {
         true
       )
       // Archived folders are not valid destinations — a row filed under one is unreachable.
-      expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(true)
+      // Pinned to the column so the check cannot be satisfied by some other nullable filter.
+      expect(
+        hasMockCondition(
+          where,
+          (n) => n.type === 'isNull' && n.column === schemaMock.folder.deletedAt
+        )
+      ).toBe(true)
     })
 
     it('returns null when no row matches', async () => {
@@ -100,7 +103,8 @@ describe('folder queries', () => {
     })
 
     it('terminates on a pre-existing cycle above the folder', async () => {
-      // `visited` is what stops this looping forever; optimistic client reparents can write one.
+      // `visited` is what stops this looping forever; concurrent reparents can each pass the
+      // check and land a cycle.
       queueTableRows(schemaMock.folder, [{ parentId: 'b' }])
       queueTableRows(schemaMock.folder, [{ parentId: 'a' }])
 
@@ -115,9 +119,9 @@ describe('folder queries', () => {
   })
 
   /**
-   * The `restoringFolderIds` short-circuit is load-bearing for cascade ordering: `restoreFolder`
-   * runs its `restoreChildren` hook BEFORE un-archiving the folder rows, so a plain "is my folder
-   * active?" check sees them still archived and dumps the entire subtree at the workspace root.
+   * The `restoringFolderIds` short-circuit is load-bearing for cascade ordering: the
+   * `config.restoreChildren` hook path runs before the un-archive transaction, so without the
+   * set a plain "is my folder active?" check dumps the subtree at the workspace root.
    */
   describe('resolveRestoredFolderId', () => {
     it('keeps the folder without querying when it is in the restoring set', async () => {
@@ -155,7 +159,12 @@ describe('folder queries', () => {
       const where = whereAt(0)
       expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true)
       expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true)
-      expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(true)
+      expect(
+        hasMockCondition(
+          where,
+          (n) => n.type === 'isNull' && n.column === schemaMock.folder.deletedAt
+        )
+      ).toBe(true)
       expect(hasMockCondition(where, (n) => n.type === 'isNotNull')).toBe(false)
     })
 
@@ -165,16 +174,16 @@ describe('folder queries', () => {
       await listFoldersForWorkspace('ws-1', 'archived', 'workflow')
 
       const where = whereAt(0)
-      expect(hasMockCondition(where, (n) => n.type === 'isNotNull')).toBe(true)
+      expect(
+        hasMockCondition(
+          where,
+          (n) => n.type === 'isNotNull' && n.column === schemaMock.folder.deletedAt
+        )
+      ).toBe(true)
       expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(false)
     })
   })
 
-  /**
-   * `requestJson` validates responses against the contract, so a route returning a raw row fails
-   * client-side parse AFTER its write has already committed. This normalizer is the single point
-   * that keeps every folder route emitting the same wire shape.
-   */
   describe('toFolderApi', () => {
     it('serializes timestamps to ISO strings and preserves a null deletedAt', () => {
       expect(toFolderApi(ROW)).toMatchObject({