feat(folders): generalize folders and add pinning across workflows, files, knowledge bases, and tables#5562
feat(folders): generalize folders and add pinning across workflows, files, knowledge bases, and tables#5562waleedlatif1 wants to merge 15 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Adds Folder reorder and duplicate get stricter validation: all-or-nothing batches, same- Admin export/import and types switch to the generic Reviewed by Cursor Bugbot for commit 06b7898. Configure here. |
|
@greptile-apps review |
|
@cursor review |
Greptile SummaryThis PR generalizes folders and pinning across the workspace resources. The main changes are:
Confidence Score: 4/5This is close, but the workflow folder delete path should be fixed before merging.
apps/sim/lib/workflows/orchestration/folder-lifecycle.ts Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@greptile-apps review |
|
@cursor review |
waleedlatif1
force-pushed
the
generic-folders-pinned
branch
from
8c608ff to
8e355c3
Compare
|
@greptile-apps review |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 8e355c3. Configure here.
|
@greptile-apps review |
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 7685991. Configure here.
…t-delete race Proactively audited the whole codebase for the two bug classes Greptile has found repeatedly this session (missing in-transaction lock recheck; target parent lock-checked but not re-verified as still active) rather than waiting for another round to find each remaining instance. Found and fixed 9 sites: 1. Greptile P1 (new finding, on last round's own fix): the just-added workflow performUpdateFolder transaction checks the target parent's LOCK state but not whether it's still ACTIVE -- assertFolderMutable's lock walker treats a deleted parent as unlocked (it stops the walk when the row is missing rather than erroring), so a parent deleted between the earlier assertFolderParentValid precheck and this transaction could still have the moved folder's parentId written to point at it. Added a FOR UPDATE + isNull(deletedAt) recheck, race-free once the lock is held. Found and fixed the identical gap in the sibling generic kb/table performUpdateFolder in folders/orchestration.ts, which had the exact same shape from the same earlier round. 2. performDeleteResourceFolder (kb/table folder delete) -- wrapped in a transaction but never checked the folder's own lock state at all before cascading the delete. Added assertFolderMutable inside the tx. 3. deleteKnowledgeBase -- already row-locks via SELECT ... FOR UPDATE but never re-invoked assertResourceMutable after acquiring it. Added the recheck bound to tx. 4. deleteTable -- pre-check only, no transaction and no recheck at all. Wrapped the write in a transaction with an in-tx recheck. 5. archiveWorkspaceFileFolderRecursive (file-folder delete) -- transaction existed but never checked lock state. Added assertFolderMutable inside the tx, matching the sibling restoreWorkspaceFileFolder in the same file. 6. bulkArchiveWorkspaceFileItems (file/folder bulk delete) -- same gap. Since this function already acquires the workspace-scoped pg_advisory_xact_lock as its first step (which every other file-folder mutation in this file also acquires first), it's already fully serialized against concurrent mutations in the same workspace -- unlike the reorder batch fix, a simple per-id recheck loop is safe here without a closure-based ordered lock. 7. renameWorkspaceFile -- no transaction, no recheck. Wrapped in a transaction with the same "unless unlocking" in-tx recheck pattern used at the other lock-toggle-capable rename/update sites. 8. restoreWorkspaceFile -- no lock check anywhere. Added a direct `.locked` check on the already-fetched soft-deleted row (matches the pattern used by the other resource restore paths, since the generic lock engine only reads active rows and can't see a soft-deleted resource's own flag). 9. performDeleteFolder/deleteFolderRecursively (workflow folder delete) -- entirely non-transactional recursive cascade with zero lock checks. archiveWorkflowsByIdsInWorkspace (called mid-cascade) doesn't accept a `tx`, so wrapping the whole recursive cascade in one transaction isn't a clean fix without a larger, riskier change to that shared function. Applied the narrower, still-real improvement: a tightly-scoped transaction around just each folder's own lock recheck + its own delete write, closing the race for that specific row even though the cascade as a whole still isn't one atomic unit. Full local verification passed (typecheck, full vitest suite 11437 tests, check:api-validation, lint all clean, 302 tests across the folders/locking surface specifically).
|
@greptile-apps review |
|
@cursor review |
Cursor: creating a knowledge_base or table folder inserted after only a pre-request assertMutable on the parent -- the generic update path in the same module already re-checks the parent's lock and active state inside the write transaction, but the create branch never got the same treatment. Extended the proactive sweep from the last round to cover create, since the same two bug classes apply there too: wrapped the kb/table generic performCreateFolder and the workflow-specific performCreateFolder in a transaction, re-checking the target parent's lock state (assertFolderMutable) and, since that lock walker treats a deleted parent as unlocked rather than erroring, a FOR UPDATE + isNull(deletedAt) recheck for active state -- both bound to tx, both race-free once the lock is held. Also added the missing assertFolderMutable check to the file-folder create path (createWorkspaceFileFolder), which already re-verified the target parent's existence/active state under the workspace-scoped advisory lock but never checked its lock state at all. While wiring this up, found and fixed two related gaps the last round's sweep introduced: performCreateFileFolder's and performDeleteFileFolder's catch blocks didn't rethrow ResourceLockedError, so the lock checks added last round (and this round) would have had their exceptions swallowed into a generic 500 instead of the route's 423 handling -- verified every other touched catch block already has this rethrow. Updated the folder-create route test's transaction mock to support the new FOR UPDATE chain. Full local verification passed (typecheck, full vitest suite 11437 tests, check:api-validation, lint all clean, 302 tests across the folders/locking surface).
|
@greptile-apps review |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 9f28f57. Configure here.
Greptile P1: the cycle-check fix's closure walk discovers ancestors with plain reads before the FOR UPDATE lock is acquired. If a concurrent transaction reparents one of those discovered members to point outside the closure in that gap, the locked read reveals a parentId the cycle walk has no data for. The walk's `?? null` fallback treated "not in closure" the same as "reached root", so it could silently stop short of a real cycle instead of detecting it -- e.g. A moves under B, the locked read shows a concurrently-committed B -> C, and C already points back to A; since C was never discovered by the pre-lock walk, the walk incorrectly terminates at B instead of continuing to C and finding the cycle back to A. A fully general fix would need to lock-and-expand the closure in rounds until it's provably closed under FOR UPDATE, which reintroduces real deadlock-ordering complexity. Took the simpler, still-correct path instead: after acquiring the lock, verify every locked row's parentId is either null or already a member of the locked closure. If not, the closure went stale between discovery and locking -- fail the whole batch (errorCode: conflict, 409) rather than risk running the cycle walk on data it can't fully vouch for. The client can retry, and a second attempt's closure walk will see the now-settled state. Added a regression test reproducing the exact scenario Greptile described. Full local verification passed (typecheck, full vitest suite 11438 tests, check:api-validation, lint all clean, 303 tests across the folders/locking surface).
|
@greptile-apps review |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 93c6995. Configure here.
…r orchestration - resource-lock.ts: fix ancestor-walk short-circuit that let a locked ancestor go unnoticed when bypassing a direct lock on the target itself (affects assertFolderMutableUnlessUnlocking/assertResourceMutableUnlessUnlocking) - workflow.ts: thread the inherited flag through WorkflowLockedError (was always false) - folders/orchestration.ts: recheck cycle-freedom inside the transaction for kb/table folder reparents (was only checked against a stale pre-tx read); add missing recordAudit + 409-on-duplicate-name for kb/table folder create/update - workflows/persistence/duplicate.ts: assertTargetFolderMutable now scopes by resourceType='workflow' and row-locks the ancestor chain (was letting a workflow duplicate reparent into a file/kb/table folder) - table/service.ts: wrap renameTable's lock checks and write in a transaction (was running with no transaction and no FOR UPDATE recheck) - workflow-lifecycle.ts: wrap performUpdateWorkflow in a transaction with an in-tx lock recheck (was relying solely on a non-transactional route pre-check) - knowledge/service.ts: recheck updateKnowledgeBase's lock inside the transaction after acquiring the row lock - workflows/orchestration/folder-lifecycle.ts: check the target folder's own lock before cascading into children (was checking only after) - workspace-files/orchestration/file-folder-lifecycle.ts: propagate the real errorCode from performRestoreFolder instead of hardcoding not_found - uploads/contexts/workspace/workspace-file-folder-manager.ts: fix missing import of assertFolderMutableUnlessUnlocking (broke typecheck) - hooks/queries/folders.ts: invalidate the correct resource-type-scoped list on folder delete/restore instead of always invalidating workflow lists - knowledge.tsx: folder rows now honor the active sort column/direction - background/cleanup-soft-deletes.ts: hard-delete retention now covers file/table/knowledge_base folders, not just workflow folders - workspace-vfs.ts: surface archived table/knowledge_base folders under recently-deleted/ so they can be discovered and restored - table API routes: include folderId/locked in create and detail responses - schema.mock.ts: add missing folderId/locked to workspaceFiles/knowledgeBase/ userTableDefinitions mocks - add regression test coverage for the resource-lock ancestor-walk fix and update table service tests for the new transactional renameTable call shape
|
@greptile-apps review |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 0711c61. Configure here.
…older update Cleanup pass over the remaining audit findings (dead code, stale comments, duplicated logic, minor hygiene), plus two real bugs caught along the way: - workflow folder-lifecycle.ts: performUpdateFolder had the same stale cycle-check race already fixed in the generic kb/table path -- the circular-reference check ran against an unlocked pre-transaction snapshot, so two concurrent moves (A under B, B under A) could each pass and commit a persisted cycle. Re-check inside the transaction now, matching the generic path. (Caught by Greptile on the post-audit re-review.) - folders/orchestration.ts: performUpdateFileFolder was swallowing ResourceLockedError into a generic 500 instead of propagating it for the route's 423 handling, unlike its create/delete/restore siblings. - knowledge/[id]/route.ts: the admin-lock-permission check for PUT only verified admin on the KB's *current* workspace, even though workspaceId and locked can change in the same request -- now checks both current and target workspace. Other fixes: extracted duplicated lock+recheck-parent logic in folders/orchestration.ts into a shared helper; removed dead `validUpdates` aliases; removed dead `color` field from workflow folder params; fixed stale comments claiming `locked`/file-folder locking is workflow-only or unused (the generic lock engine treats all four resourceTypes uniformly); added resourceType scoping to log folder-descendant expansion; fixed an O(n*m) descendant-collection loop in the files-download route to build its parent->children index once; added typed TableInvalidFolderError instead of string-matching error messages; fixed a hardcoded SQL column literal in pinned-items lookups; deduplicated FolderResourceType between the folders contract and the folders store; derived a magic query-key array index from the key factory instead of a literal; moved a render-time ref mutation in files.tsx into an effect; memoized a per-render Object.fromEntries in knowledge.tsx; fixed recently-deleted.tsx passing no resourceType on file folder restore (silently defaulted to workflow); aligned a stray eslint-disable in tables.tsx with its file/knowledge siblings. Fixed a knowledge-route test whose hand-rolled db mock had no `transaction` stub (needed after createKnowledgeBase was wrapped in a transaction to close its own folder-lock TOCTOU window) and whose beforeEach was clobbering that stub every test.
|
@greptile-apps review |
|
@cursor review |
There was a problem hiding this comment.
✅ 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 06b7898. Configure here.
1 participant
Summary
workflowFolder/workspaceFileFolderwith one genericfoldertable (resourceTypediscriminator: workflow/file/knowledge_base/table) plus a new polymorphicpinnedItemtable for per-user pinning of any resource, including folders themselvesworkflow/workspace_filesFKs, addsfolderIdtoknowledge_base/user_table_definitions; a DB trigger plus app-levelassertFolderParentValidenforce same-workspace/same-resourceType parenting on every write path/api/foldersroute + new/api/pinned-itemsroutes, contracts, and React Query hooks shared by all four resource types, replacing the previous duplicated per-resource folder CRUDType of Change
Testing
Tested manually end-to-end (folder create/rename/delete/restore/reorder for workflows, files, knowledge bases, and tables; pinning/unpinning across all resource types; migration verified against a live Postgres instance including trigger rejection of cross-workspace/cross-type parenting). Full test suite passing, typecheck/lint/API-validation clean.
Checklist