Skip to content

feat(folders): add resource pinning and generalize the folders contract - #6014

Merged
waleedlatif1 merged 3 commits into
stagingfrom
feat/generic-folders-pinning
Jul 28, 2026
Merged

feat(folders): add resource pinning and generalize the folders contract#6014
waleedlatif1 merged 3 commits into
stagingfrom
feat/generic-folders-pinning

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Adds per-user pinning for workflows, files, knowledge bases, and tables — new pinned_item table (migration 0271), /api/pinned-items routes, React Query hooks, and a shared PinButton
  • Pin toggle wired into Tables, Knowledge, and Files, with pinned-first ordering that holds across every sort column and survives desc inversion
  • Adds the generic folder table (migration 0272) with an idempotent, collision-aware backfill from workflow_folder and workspace_file_folders — table only, no cutover yet
  • Moves folderSchema to the generic vocabulary: adds resourceType, renames archivedAtdeletedAt, drops the unused color/isExpanded

Notes for review

  • The folder cutover is deliberately not in this PR. lib/folders/queries.ts holds a documented adapter that reads the legacy workflow_folder table and emits the generic shape, so reads and writes still agree. The remaining work is a one-file query swap plus repointing ~170 refs, which needs a resourceType='workflow' filter added per query — not a mechanical replace.
  • Migration 0272 was dry-run against production data (read-only): 0 unique-index violations, 0 orphaned parents, 0 cross-workspace parents, 0 id overlap between source tables, and 0 dangling references across 4,945 workflows and 5,942 files. ~35k rows backfilled.
  • color/isExpanded were verified to have zero consumers before removal.
  • Two existing tests changed intentionally: renaming a folder to "" now returns 400 (it previously returned 200 and silently discarded the rename), and the prefetch test no longer asserts a color default.
  • Adds bounds that were missing: folder name 1–255, reorder array 1–1000, and typed deletedItems/restoredItems instead of z.unknown().

Type of Change

  • New feature

Testing

2638 tests passing across folders, workflows, pinned-items, uploads, and workspace UI. Typecheck clean, check:migrations, check:api-validation, and lint all pass. Migration verified against production data read-only; UI not yet manually exercised.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Jul 28, 2026 8:06pm

Request Review

@cursor

cursor Bot commented Jul 28, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Large schema migration and breaking folder API shape changes affect all folder clients; pinning is scoped to the session user but POST requires workspace access and resource existence checks.

Overview
Adds per-user pinning for workflows, files, knowledge bases, and tables: pinned_item storage, /api/pinned-items (list/create/unpin by resource), React Query hooks with optimistic updates, and a shared PinButton. Tables, Knowledge, and Files list pages pin from the name column via a new endAdornment on Resource rows; pinned rows sort first regardless of column or direction.

Folder API contract moves toward a generic model: responses go through toFolderApi (resourceType, deletedAt instead of archivedAt; color / isExpanded removed from create/update/duplicate). List folders accepts optional resourceType (non-workflow types return empty until cutover). Stricter validation: trimmed empty/whitespace names return 400; reorder and cascade delete/restore payloads are typed.

Migration 0272 introduces the generic folder table (with backfill from workflow and file folder tables) and pinned_item; writers still hit legacy workflow_folder via the adapter in lib/folders/queries.ts. collectDescendantFolderIds is shared for folder subtree walks.

Reviewed by Cursor Bugbot for commit cae72a6. Configure here.

Comment thread apps/sim/app/api/folders/route.ts
Comment thread apps/sim/app/api/folders/route.ts
Comment thread packages/db/migrations/0272_generic_folders.sql Outdated
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds per-user resource pinning and a generic folder table (migration-only, no cutover), while adapting folder API responses to the new contract.

  • New pinned_item table/API/hooks/PinButton with pinned-first list ordering for files, knowledge bases, and tables
  • Migration 0272 creates generic folder + backfill from legacy tables; self-referential parent FK added after backfill
  • Folder wire schema uses resourceType/deletedAt; list and create/update/duplicate paths emit toFolderApi

Confidence Score: 5/5

Safe to merge; prior folder contract and migration concerns are addressed in the current HEAD.

Mutation folder responses go through toFolderApi, and migration 0272 adds the self-referential parent FK only after both backfills complete.

Important Files Changed

Filename Overview
apps/sim/app/api/folders/route.ts Create response now wraps the new folder with toFolderApi so it matches folderSchema.
apps/sim/app/api/folders/[id]/route.ts Update response uses toFolderApi; drops unused color/isExpanded body fields.
apps/sim/app/api/folders/[id]/duplicate/route.ts Duplicate response uses toFolderApi for contract-aligned client parsing.
apps/sim/lib/folders/queries.ts Shared toFolderApi adapter maps legacy workflow_folder rows to the generic wire shape.
packages/db/migrations/0272_generic_folders_and_pinning.sql Parent FK is created after both backfills; trigger/backfill comments document statement-end FK behavior.
apps/sim/app/api/pinned-items/route.ts List/create pinned items with workspace permission checks and active-resource filtering.
apps/sim/components/pin-button.tsx Shared pin toggle wired into resource list rows with hover/focus visibility.

Reviews (5): Last reviewed commit: "fix(db): guard the folder backfill so a ..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 force-pushed the feat/generic-folders-pinning branch from 9ab72ac to 0629f41 Compare July 28, 2026 19:28
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/folders/route.ts
Comment thread apps/sim/lib/api/contracts/folders.ts Outdated
Comment thread apps/sim/lib/api/contracts/folders.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread packages/db/migrations/0272_generic_folders_and_pinning.sql
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread packages/db/migrations/0272_generic_folders_and_pinning.sql
Adds per-user pinning for workflows, files, knowledge bases, and tables,
and moves the folders API onto a generic, resource-typed contract ahead of
the shared `folder` table cutover.

- new `pinned_item` and `folder` tables in a single migration, with an
  idempotent, collision-aware backfill from `workflow_folder` and
  `workspace_file_folders`
- `/api/pinned-items` routes, React Query hooks, and a shared `PinButton`
- pin toggle wired into Tables, Knowledge, and Files with pinned-first
  ordering across every sort column
- `folderSchema` gains `resourceType`, renames `archivedAt` to `deletedAt`,
  and drops the unused `color`/`isExpanded` fields
- every folder route emits the shape through one shared `toFolderApi`
  adapter; fields the routes cannot yet honor are not accepted
The file ends in CONCURRENTLY index builds under a COMMIT, so a failure
there replays the whole file from the top — but the statements above were
not idempotent, which would have wedged the deploy on retry. Every one is
now idempotent, matching 0250_workspace_forking.sql: CREATE TYPE and every
ADD CONSTRAINT in a duplicate_object handler, IF NOT EXISTS on tables,
columns and indexes, IF EXISTS on the constraint drops, OR REPLACE on the
function, and a drop-then-create for the trigger. Both backfills already
carried ON CONFLICT (id) DO NOTHING.
ON CONFLICT (id) only covers primary-key collisions, not the folder name
unique index. On a replay, a folder created in the source table between
runs could be handed a name a previously-migrated row already holds, which
would abort the migration and wedge the deploy after a CONCURRENTLY
failure. Each backfill now no-ops once rows of its resourceType exist —
correct in both directions, since the INSERT is all-or-nothing inside the
migration transaction. Adopting folders written during the rollout stays
the job of the separate post-drain catch-up pass.
@waleedlatif1
waleedlatif1 force-pushed the feat/generic-folders-pinning branch from 9434e52 to cae72a6 Compare July 28, 2026 20:02
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cae72a6. Configure here.

@waleedlatif1
waleedlatif1 merged commit cb3611b into staging Jul 28, 2026
20 checks passed
@waleedlatif1
waleedlatif1 deleted the feat/generic-folders-pinning branch July 28, 2026 22:10
waleedlatif1 added a commit that referenced this pull request Jul 29, 2026
…ing unit

Audited as the unit that actually ships — #6014 (pinning), #6025 (workflow folders onto the
generic table), #6037 (the engine) and this branch together against `main`, rather than this
branch against `staging`.

Authorization
- `PUT /api/folders/reorder` still operated on archived folders. `getFolderLockStatus` skips
  archived rows, so `assertFolderMutable` there was a guaranteed no-op — meaning a locked folder
  became freely reparentable the moment its parent was deleted. This branch closed exactly that
  hole on `PUT /api/folders/[id]` and left its sibling open, then widened reorder to three
  resource trees. Reordering an archived folder is also wrong on its own terms:
  `collectArchivedSubtreeIds` walks the cascade by parent, so moving a branch out of an archived
  subtree silently drops it from that folder's restore

Data loss in the purge
- `batchDeleteByWorkspaceAndTimestamp` forwarded its eligibility predicate only into the SELECT,
  never the DELETE. The `folder` target runs through it, so a restore committing between the two
  statements had its folder hard-deleted anyway — taking the placement of the children the
  restore had just brought back. The workflow purge re-checks for precisely this reason and the
  knowledge-base sweep gained the same guard earlier in this branch; `folder` had neither. The
  wrapper now re-asserts eligibility on the DELETE for every caller

Wire boundary
- `toPinnedItemApi` had no return type, so `pinned_item.resource_type` — plain `text` by design,
  against a closed contract enum — was never checked. Annotating it surfaced the real hole
  immediately: during a rolling deploy an older pod can read a pin a newer one wrote, and
  returning it would fail response validation and take the WHOLE list down rather than the one
  row. Unknown kinds are now narrowed out explicitly at the boundary instead of surviving by
  accident on a filter whose stated job is something else

Interaction
- The knowledge-base FOLDER move still compared against the snapshot taken when the menu opened.
  The resource move on that page and both Tables handlers were fixed earlier in this branch; this
  was the last one

Operational
- 0274's "re-run after drain" instruction needed a precondition. Cleanup hard-deletes from
  `folder` but never from `workspace_file_folders`, so a re-run after a purge would reinstate
  every purged file folder as a soft-deleted phantom whose files are already gone. Retention is
  far longer than any drain, so running it promptly is sufficient — but the instruction said
  nothing about ordering

Accuracy
- Four comments named symbols that do not exist or no longer apply: `useFolderCreateWithDedup`
  (never existed — it is `nextUntitledFolderName`), `collectActiveSubtreeIds`,
  `restoreFolderCascade` as the live restore path, and a claim that a server
  `createSearchParamsCache` reads the shared folder param. The shared param doc also now admits
  Files declares its own `?folderId=` rather than claiming to be the single declaration
- `FOLDER_RESOURCES.file` is unreachable at runtime — `servedFolderResourceTypeSchema` does not
  serve `'file'` — and a reader would reasonably assume otherwise. Says so, and says what routing
  file folders through the generic engine would bypass
- `pinnedItem.resourceType`'s inline comment was missing `'folder'`
- `folders.test.ts` fixtures still carried `color` and `isExpanded`, both deliberately dropped
  from the generic table — pre-consolidation rows masquerading as folders
- `getFolders` in the folder cache had one caller, in its own file
waleedlatif1 added a commit that referenced this pull request Jul 29, 2026
…bles folders (#6045)

* feat(folders): cut file folders over, and give knowledge bases and tables folders

Builds on the generic resourceType-driven folder engine to finish the migration
and extend it to two more resource trees.

File-folder cutover
- Repoints all 30 `workspace_file_folders` query sites onto `folder` scoped to
  `resourceType = 'file'`, id-keyed lookups included — a caller can hand a
  workflow folder's id to a file-folder endpoint, and only the predicate stops it
- Adds the missing restore-conflict handling: a file folder whose name was taken
  while it was archived now deduplicates against the RESOLVED parent (restore
  re-roots when the original parent is still archived) instead of being
  permanently unrestorable
- The forking mapper's `resourceType` predicate lives in the `leftJoin`
  condition, not the WHERE, where it would silently make the outer join inner
- `servedFolderResourceTypeSchema` gains `file`; the soft-delete sweep enumerates
  all four types rather than dropping its predicate

Knowledge Base and Tables folders
- `knowledge_base.folder_id` and `user_table_definitions.folder_id` are now
  written and served; both create/move paths admit a folder only through
  `findActiveFolder`, scoped to workspace AND resourceType
- Restoring a knowledge base whose folder is still archived re-roots it rather
  than filing it somewhere the page never renders; a workspace move re-roots for
  the same reason
- Both pages get folder rows, breadcrumbs, inline rename on rows and on the
  breadcrumb, move-to submenus, pin/unpin, and drag-a-row-onto-a-folder

Shared folder UI
- One `components/folders/` module now backs Knowledge, Tables, and Files:
  `folderBreadcrumbItems` (data for `Resource.Header`, which owns the crumb
  chrome), `folderRow`, `folderRowId`/`parseFolderedRowId`, `buildMoveOptions` /
  `buildDescendantIndex` / `renderMoveOptions`, `nextUntitledFolderName`,
  `FolderContextMenu`, `useFolderNavigation`, `useFolderRowDragDrop`
- Deletes the per-page copies: `files/move-options.tsx` and the Tables-local
  folder context menu

Recently Deleted
- Restores folders of every type, not just workflow folders, driven by one
  declarative table rather than a branch per resource

Folder pins resolve against `folder` unscoped: file, knowledge-base, and table
folders are all pinnable and all live there, and a folder id addresses exactly
one row.

* fix(folders): close the folder-engine audit findings

Data loss
- Migration 0274 catches up file folders created AFTER 0272. That backfill is guarded
  by `WHERE NOT EXISTS (… resource_type = 'file')`, so it fires once; every file folder
  created between it and this cutover exists only in `workspace_file_folders`. Without
  the catch-up they vanish from Files on deploy and their contents become unreachable

Authorization
- Archived folders were mutable, which bypassed the folder lock: the folder routes and
  `updateFolder` filtered id + resourceType but never `deletedAt`, while
  `getFolderLockStatus` does — so an archived-but-locked folder reported unlocked. Lock a
  subfolder, delete its parent, and any write-level member could rename and reparent it.
  PUT and the engine now require an active row; DELETE deliberately still does not, because
  it reuses an archived folder's own `deletedAt` to make a partial cascade retryable
- `v1/admin/workflows/import` wrote `folderId` with no validation at all. Migration 0272
  dropped the FK, so it accepted a knowledge-base folder, another workspace's folder, or
  garbage — and the workflow then rendered under no sidebar node while still executing,
  billing, and escaping the folder delete cascade. It now runs the same
  `assertFolderInWorkspace` + `assertFolderMutable` pair as `v1/workflows/import`

Restore no longer loses state
- A folder restore ran a bare `archivedAt = null` over its workflows, while the delete had
  routed through `archiveWorkflow` — which also disables schedules, webhooks, chats, and MCP
  tools. Restoring a folder reported success while every schedule stayed disabled forever
  and every chat 404'd. The workflow tree now restores through `restoreWorkflow`, like the
  knowledge-base and table trees already did. Deployment state is still deliberately not
  revived, matching the single-workflow restore
- Those canonical restores re-root a resource whose folder is archived — correct on its own,
  catastrophic inside a cascade, which restores children BEFORE the folder rows and would
  therefore dump every child at the workspace root. `resolveRestoredFolderId` takes the
  subtree being restored and exempts it. `restoreTable` gained the re-root check it lacked

Correctness
- `getFolderPath` in `lib/folders/tree.ts` walked `parentId` with no `visited` set, so a
  cycle in the client cache — reachable through `useReorderFolders`' unchecked optimistic
  write — hung the tab. Its twin already had one. Dead `buildFolderMap` deleted
- The sidebar's "New folder" inside a folder hardcoded the name, so the second one 409'd,
  was swallowed with only a log line, and the user got no folder and no message. It now
  names through the existing `generateSubfolderName` and surfaces failures
- `chunkedBatchDelete` deleted by id with no re-check, so a resource restored between the
  SELECT and the DELETE was hard-deleted anyway. Callers now re-assert their soft-delete
  predicate on the DELETE
- Recently Deleted and the `restore_resource` tool cover every folder tree, not just
  workflow folders, so deleted knowledge-base and table folders are recoverable

Cleanup
- The folder duplicate route stops steering control flow through `throw new Error('literal')`
  matched by string, reuses the engine's `nextFolderSortOrder` instead of recomputing it, and
  names its workflow scope once
- `servedFolderResourceTypeSchema` documents that its default applies only to an omitted
  value; a present-but-unrecognized one is rejected, never coerced to `workflow`
- The `workspace_file_folders` comment described the table as unread and unwritten and
  invited a DROP. It was live until this cutover, and it is now the rollback copy — the
  comment says so, and names what must be true before anyone drops it
- Pinned rows carry a small non-interactive pin glyph again; they sort to the top and had
  nothing explaining why since the inline pin button was removed

Tests
- New `lib/folders/lifecycle.test.ts` covers create/update/delete/restore, the re-root, and
  the 23505 mappings; `cleanup-soft-deletes.test.ts` now pins the folder target's
  resourceType predicate, which is what keeps the sweep off a not-yet-cut-over type

* improvement(folders): show the pin glyph on Files rows and pin the deploy contract

Files rows were left without the pin indicator the other two lists gained, so a pinned
file or folder still sorted to the top of the Files page with nothing explaining why.

Adds `lib/api/contracts/folders.test.ts`, which pins the three cases that together ARE
the rolling-deploy contract: an omitted `resourceType` defaults to `workflow` (so a client
predating the field keeps working), a present-but-unrecognized one is rejected rather than
coerced, and every served tree is accepted.

* fix(folders): close the findings from the full-diff audit

Migration 0274 reconciles instead of catching up
- The deployed manager writes `workspace_file_folders` EXCLUSIVELY, so `folder`'s file rows
  are frozen at the 0272 snapshot: every rename, move, delete, and restore since then lives
  only in the legacy table. An insert-only catch-up left all of that diverged — renames
  reverted, deleted folders came back as active phantoms, and a stale-active row could hold
  the name a newer folder legitimately took
- That last case was silent: the bare `ON CONFLICT DO NOTHING` swallows a NAME violation as
  readily as an id one, so the newer folder was discarded and every file inside it stranded.
  The conflict target is now `(id)`, so a name collision — which would mean the source
  violated its own unique index — fails loudly instead
- Reconciles by parking mirrored names first, then upserting, so no transient state can
  collide with the partial unique index. Both statements are idempotent, and the header says
  what nothing else would: this has to be run again after the deploy drains, because old pods
  keep writing the legacy table until they are gone

`/api/folders` no longer serves file folders
- Adding `'file'` to `servedFolderResourceTypeSchema` opened a second writer over the same
  rows. It bypassed the `workspace_file_folders:${workspaceId}` advisory lock that makes the
  manager's check-then-write pairs atomic, and it accepted names containing `/`, `\`, `.` and
  `..`, which the file surface forbids because a folder name becomes a path segment — a
  folder named `a/b` is then unreachable by path forever, and breadcrumbs resolve it as two
  segments. The storage cutover never needed a second API, so there isn't one

Conflicts that returned 500
- `v1/admin/workspaces/[id]/import` created its folder with an unserialized SELECT-then-
  INSERT, so two imports sharing a folder path failed the whole import. The name is
  server-chosen, so it now adopts whichever folder won, like `ensureWorkspaceFileFolderPath`
- Folder duplicate never caught 23505. `newId` is client-supplied, so replaying a duplicate
  whose response was lost hit the primary key and returned 500 instead of a 409 the caller
  can act on

Performance
- `knowledgeKeys` moved to `hooks/queries/utils/knowledge-keys.ts`. Reading it from
  `hooks/queries/kb/knowledge` pulled a ~1000-line hook module — and the `@sim/emcn` barrel
  behind it — into `hooks/queries/folders`, which three server prefetches import. That is the
  same import edge the navigation-speed audit measured at 11.8MB per workspace route;
  `tableKeys` already lived in a keys-only module for exactly this reason

Merge drop
- Knowledge never got the chrome Tables did: its prefetch now fetches the folder tree
  alongside the bases, so the list does not paint ungrouped and a `?folderId=` deep link does
  not render an empty breadcrumb, and its loading fallback declares the "New folder" action so
  the header does not shift on hydration

Comments corrected to match reality
- The shared folder row and context menu claimed Files as a consumer; Files still builds its
  own row and routes folders through `FileRowContextMenu`, which the TSDoc now says
- `schema.ts` prescribed a row COUNT comparison before dropping the legacy table. The
  divergence 0274 repairs is in row CONTENTS, which a count check passes straight through

* fix(folders): address the review round and the full-PR audit

Reverts the workflow restore hook — it was the wrong fix
- Greptile flagged that restoring a folder leaves schedules disabled and webhooks/chats
  inactive. The hook added last round routed workflows through `restoreWorkflow` to address
  it, but `restoreDependents` ALREADY clears exactly those columns, in bulk, inside the
  restore transaction and matched on the archive timestamp. The hook bought nothing and cost
  a per-workflow read/transaction/read OUTSIDE that transaction — roughly 1600 round trips
  for a folder of 200 workflows — plus a window where the workflows were active and the
  folder was not, and it made `restoreDependents` dead code
- What no restore path can undo is the state archive OVERWRITES: `status: 'disabled'` with
  `nextRunAt` cleared, `isActive: false`. Archive does not record what those were, so
  restoring them to a constant would re-enable a schedule the user had disabled and re-run a
  completed one. Left explicit — redeploy re-activates a schedule — matching deployment state,
  which restore also deliberately leaves off. Documented on the config rather than guessed
- Kept the genuine bug the review surfaced underneath it: `restoreWorkflow` un-archived every
  dependent by `workflowId` alone, resurrecting a webhook or chat the user had archived days
  earlier. Now matched on the workflow's own `archivedAt`, which is the semantics this feature
  documents

Bugbot: orphaned knowledge bases vanished from every view
- Knowledge filtered on a raw `folderId === currentFolderId` and never re-rooted a base whose
  folder is missing or archived, so a base restored on its own — or left behind by a partial
  cascade — was invisible everywhere. Tables already fell those back to the root

A dead `?folderId=` is now healed instead of being a dead end
- A bookmark to a deleted folder rendered the root title over an empty grid, hid everything
  actually at the root, and — worse — left every create/upload action targeting the dead id,
  filing new resources somewhere nothing could reach. `useFolderNavigation` clears it once the
  folder list resolves, which fixes Files, Knowledge, and Tables at once

Parity gaps found by auditing against Files
- Tables sorted folders newest-first on a clean URL while Files and Knowledge sorted A→Z: its
  sort params are defaulted, so the raw column was never null. Reads `activeSort` now
- On Tables you could not delete the folder you were standing in — its own row is not in the
  list, and the crumb menu offered only Rename, which also made the step-out branch
  unreachable dead code
- A failed knowledge-base move was logged and never surfaced, so a rejected move — from the
  submenu or from a drag — looked like nothing happened
- A drag begun in another mount of the page could never be dropped: `onDragOver` bailed before
  `preventDefault`, so no drop event ever fired and the `dataTransfer` fallback in `onDrop`
  was unreachable. External/foreign drags are still ignored, so an OS file dropped on the list
  cannot navigate the tab away

Files: a folder cycle crashed the page
- The folder-size roll-up recursed with no `visited` guard, so a parent/child cycle in the
  cached tree — reachable through the optimistic folder-move write — recursed until the stack
  blew and the page went blank. Also indexes children once instead of re-scanning per node

Migration 0274 is now atomic
- 0272 ends with an embedded COMMIT for its CONCURRENTLY index builds, so this file is not
  guaranteed to run inside drizzle's batch transaction. Wrapped in a DO block so the parking
  pass cannot commit without the reconcile, which would leave every mirrored folder holding a
  placeholder name
- Validated with a read-only dry run: no active name duplicates, no orphaned or
  cross-workspace parents, and no id collisions with `workflow_folder`, so the insert cannot
  trip the unique index, the FKs, or the resource-type trigger

Smaller corrections
- Knowledge indexed its members for the owner comparator instead of two linear scans per
  comparison, and logs a load failure from an effect rather than the render body
- Files uses the shared root sentinel instead of a bare `'__root__'` in two places
- `restore_resource` documents that its two new folder types are unreachable until the enum in
  the copilot service's tool catalog — a different repository — widens
- Corrected a forking comment that still described file folders as a separate table

* docs(folders): record the 0272 + 0274 dry-run results on the migration

A read-only dry run materialises the complete post-0272 `folder` table from both source
tables and asserts every constraint it declares: no NULL names, no primary-key collisions
between the two source tables, no unique-index violations, no resource-type or workspace
trigger violations, and no parent/user/workspace FK violations. 0272's dedup renames only
workflow folders, never file folders — the latter is what lets pass 2 write raw names.

* fix(folders): close the findings from the line-by-line audit of the PR

Data integrity
- Workspace fork copied `folderId` verbatim onto the child's tables and knowledge bases. The
  column carries no workspace, so a forked row pointed at a folder owned by the SOURCE
  workspace — invisible in the fork, and mutated from under it when the source later deleted
  that folder (`ON DELETE SET NULL`). Latent until now because nothing wrote a non-null
  `folderId` for those two resources; this PR is what makes it reachable. Forked resources land
  at the root, like forked files already did

Interaction bugs
- Clearing a dead `?folderId=` wrote through the params' default `history: 'push'`, so Back
  returned to the dead URL, which healed and pushed again — the Back button was dead on that
  page. It replaces now: opening a folder is navigation, correcting a URL that never pointed
  anywhere is not
- That same heal keyed off `isLoading`, which is false for a DISABLED query, false for an
  ERRORED one, and false while `keepPreviousData` is showing the previous workspace's folders.
  In all three the list is empty or stale, so a transient fetch failure — or a workspace switch
  — threw away a perfectly good open folder. Gated on `isSuccess && !isPlaceholderData`
- "New folder" while a search was active created the folder but never showed it: the search
  filters folders too, so the new row did not match, the rename field never appeared, and the
  create read as a no-op. Both pages clear the search so the thing just created is on screen
- The drag ghost was removed only by `dragend`, which fires on the source row. The table is
  virtualized, so scrolling the source out of view mid-drag unmounted it, the event never
  arrived, and the ghost stayed on the page with every row stuck at drag opacity. Cleaned up on
  unmount as the backstop
- A drag begun in another mount highlighted every folder as a valid target — including the
  dragged folder itself — then silently did nothing on drop. It only highlights when the source
  is known and was actually checked

Correctness
- The folder-duplicate route caught 23505 across the WHOLE handler, so a unique violation
  raised while copying the workflows inside the folder was relabelled "a folder with this name
  already exists". Scoped to the folder INSERT, which is the only place the two conflicts the
  caller can act on (client-supplied `newId` replay, concurrent name take) actually arise
- The optimistic folder create derived its sort order from the workspace's WORKFLOWS regardless
  of resource type. Only the workflow tree interleaves folders and resources in one ordering
  space, so a knowledge-base or table folder was placed against an unrelated one
- A table archived between `checkAccess` and the move surfaced as a 500 with a "not found"
  body; it is a 404
- `FOLDER_RESOURCE_TYPE_BY_RESTORABLE` was a `Partial` record, so adding a folder tree without a
  mapping would compile and silently restore into the workflow tree. Total record now
- Recently Deleted paired query results to folder trees by ARRAY POSITION; reordering the const
  would have filed knowledge folders under Tables with no type error. Keyed by type
- The knowledge move's no-op guard compared against the snapshot taken when the menu opened
  rather than the live row

Performance — the module split now actually does what it claimed
- `knowledge-keys.ts` justified itself as keeping server prefetches off the ~1000-line
  `kb/knowledge` module, but `knowledge/prefetch.ts` still imported its stale-time constant from
  exactly that module, whose first line pulls the `@sim/emcn` barrel. Worse, this PR had ADDED
  the same class of edge to four prefetches via `FOLDER_LIST_STALE_TIME`/`mapFolder`
- `FOLDER_LIST_STALE_TIME`, `mapFolder`, and `KNOWLEDGE_BASE_LIST_STALE_TIME` now live beside
  their key factories, so all four server prefetches import only keys-and-constants modules.
  `mapFolder` keeps its explicit field list rather than a spread, so the cached shape cannot
  drift from a client fetch

Honesty in comments
- The KB retention `deleteFilter` narrows the restore race but does not close it — documents
  hard-deleted by `onBatch` do not come back, so a restore landing mid-batch returns an emptied
  base. Said so
- A failed folder restore leaves children active while their folders are still archived. They
  are reachable (both lists fall an unresolvable `folderId` back to the root) but they sit at
  the root until the restore is retried. Said so

Product policy made visible
- Restore deliberately does not re-enable schedules, webhooks, or chats, because archive
  overwrites their state without recording it. A deliberately-disabled schedule or webhook is a
  common state rather than an edge case, so reactivating to a constant would be wrong more often
  than right. Recently Deleted now says so at the moment of restore instead of leaving it to be
  discovered

Smaller
- Pin glyph: dropped the doubled gap, added `role='img'` so the state is announced
- "Move here" carries the folder glyph its sibling leaves do
- `useMoveTable` had stolen `useDeleteTable`'s doc block
- Dead `.returning` column and a `?? null` on a non-nullable param in `moveTableToFolder`
- Tables reads one sort source for both blocks, matching Knowledge
- New test pins that `updateFolder` refuses an archived folder — the guard that stops a delete
  from unlocking its locked subfolders

* fix(tables): re-read live placement before skipping a move as a no-op

`handleMoveTable` and `handleMoveFolder` decided "already there, nothing to do" from
`activeTable`/`activeFolder`, which are snapshots taken when the context menu opened. A refetch
or a concurrent move since then makes that comparison stale, so the write the user just asked
for is silently skipped and the row stays where it was. Both now compare against the live list,
matching the knowledge-base move.

* chore(folders): drop production figures from migration and code comments

This repository is public. The 0274 header and the Recently Deleted policy comment carried
concrete counts and ratios taken from the production database, which is operational detail that
does not belong in open source. Both now state the property that matters — what was asserted,
and why disabled-on-restore is the safe default — without the underlying numbers.

* fix(folders): close the findings from auditing the whole release landing unit

Audited as the unit that actually ships — #6014 (pinning), #6025 (workflow folders onto the
generic table), #6037 (the engine) and this branch together against `main`, rather than this
branch against `staging`.

Authorization
- `PUT /api/folders/reorder` still operated on archived folders. `getFolderLockStatus` skips
  archived rows, so `assertFolderMutable` there was a guaranteed no-op — meaning a locked folder
  became freely reparentable the moment its parent was deleted. This branch closed exactly that
  hole on `PUT /api/folders/[id]` and left its sibling open, then widened reorder to three
  resource trees. Reordering an archived folder is also wrong on its own terms:
  `collectArchivedSubtreeIds` walks the cascade by parent, so moving a branch out of an archived
  subtree silently drops it from that folder's restore

Data loss in the purge
- `batchDeleteByWorkspaceAndTimestamp` forwarded its eligibility predicate only into the SELECT,
  never the DELETE. The `folder` target runs through it, so a restore committing between the two
  statements had its folder hard-deleted anyway — taking the placement of the children the
  restore had just brought back. The workflow purge re-checks for precisely this reason and the
  knowledge-base sweep gained the same guard earlier in this branch; `folder` had neither. The
  wrapper now re-asserts eligibility on the DELETE for every caller

Wire boundary
- `toPinnedItemApi` had no return type, so `pinned_item.resource_type` — plain `text` by design,
  against a closed contract enum — was never checked. Annotating it surfaced the real hole
  immediately: during a rolling deploy an older pod can read a pin a newer one wrote, and
  returning it would fail response validation and take the WHOLE list down rather than the one
  row. Unknown kinds are now narrowed out explicitly at the boundary instead of surviving by
  accident on a filter whose stated job is something else

Interaction
- The knowledge-base FOLDER move still compared against the snapshot taken when the menu opened.
  The resource move on that page and both Tables handlers were fixed earlier in this branch; this
  was the last one

Operational
- 0274's "re-run after drain" instruction needed a precondition. Cleanup hard-deletes from
  `folder` but never from `workspace_file_folders`, so a re-run after a purge would reinstate
  every purged file folder as a soft-deleted phantom whose files are already gone. Retention is
  far longer than any drain, so running it promptly is sufficient — but the instruction said
  nothing about ordering

Accuracy
- Four comments named symbols that do not exist or no longer apply: `useFolderCreateWithDedup`
  (never existed — it is `nextUntitledFolderName`), `collectActiveSubtreeIds`,
  `restoreFolderCascade` as the live restore path, and a claim that a server
  `createSearchParamsCache` reads the shared folder param. The shared param doc also now admits
  Files declares its own `?folderId=` rather than claiming to be the single declaration
- `FOLDER_RESOURCES.file` is unreachable at runtime — `servedFolderResourceTypeSchema` does not
  serve `'file'` — and a reader would reasonably assume otherwise. Says so, and says what routing
  file folders through the generic engine would bypass
- `pinnedItem.resourceType`'s inline comment was missing `'folder'`
- `folders.test.ts` fixtures still carried `color` and `isExpanded`, both deliberately dropped
  from the generic table — pre-consolidation rows masquerading as folders
- `getFolders` in the folder cache had one caller, in its own file

* fix(folders): gate the orphan fallback on a resolved folder index, not a loading flag

The URL heal was fixed to use `isSuccess && !isPlaceholderData`, but the list filters that
decide "this resource's folderId does not resolve, so show it at the root" were left on
`isLoading` — the same footgun, one layer down.

`isLoading` is false for a disabled query (no workspaceId), false for an errored one, and —
because `useFolders` sets `keepPreviousData` — false while the previous workspace's folders are
still on screen during a switch. In all three the index is empty or belongs to another
workspace, so every foldered knowledge base and table was treated as an orphan and dragged to
the root, or filtered out of a deep-linked folder entirely. A transient fetch failure was enough
to make a folder look empty.

`useFolderNavigation` now exposes `foldersResolved` instead of `isLoading`, so the heal and both
list filters share one signal and the footgun is no longer reachable from the hook's surface.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant