Skip to content

fix(fork): preserve folder structure across a fork edge for files, tables, and knowledge bases - #6752

Merged
waleedlatif1 merged 3 commits into
stagingfrom
fix/fork-sync-folder-structure
Aug 16, 2026
Merged

fix(fork): preserve folder structure across a fork edge for files, tables, and knowledge bases#6752
waleedlatif1 merged 3 commits into
stagingfrom
fix/fork-sync-folder-structure

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fork sync only mirrored workflow folders into the target. Copied files, tables, and knowledge bases were written with a hardcoded folderId: null, so every push/pull flattened them into the target root and lost the source's grouping.
  • resolveForkFolderMapping already did the work (prune to folders holding copied content plus ancestors, reuse same-named target folders, remap parentId) but was pinned to resourceType: 'workflow' on both reads and the folder-ceiling check. Parameterized it by resource type and run it per family.
  • The four folder-bearing families own disjoint trees and folder ids are globally unique, so the per-family maps merge cleanly for the sim:folder/<id> content rewrite — which previously resolved only for workflow folders.
  • BlobCopyTask.targetFolderId is optional so file-copy jobs queued by an earlier deploy replay at the root exactly as before.

Applies to fork create and to both sync directions — push and pull share the same code path.

Scope: forward-only. Resources already flattened by earlier syncs stay where they are. An earlier revision of this PR included a sync-time re-home pass to heal them; it was removed after review — folder_id IS NULL cannot distinguish legacy flattening from a user's deliberate move to the root, so it would re-apply on every sync and fight that placement indefinitely. Doing it correctly needs a one-time marker per edge (a migration), which belongs in its own change.

Type of Change

  • Bug fix

Testing

  • 539 tests pass across 37 files in ee/workspace-forking; bun run type-check clean; all 27 check:audits pass
  • Verified the new tests can fail: reverting the file folderId remap turns the file-placement test red

Notes for review

  • Folder creation now happens in three more collections, so assertFolderCollectionHasRoom can refuse a sync whose target is at the 10k-per-collection cap. It only fires when new folders are genuinely needed, the promote route already maps that error cleanly, and it matches existing workflow behavior.

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 Aug 16, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 16, 2026 12:32am

Request Review

@cursor

cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches fork transaction folder inserts and resource placement across four folder collections; behavior is forward-only and folder-ceiling checks can refuse syncs at cap, matching existing workflow-folder behavior.

Overview
Fork create and promote/sync no longer flatten copied files, tables, and knowledge bases to the target root. Each family now mirrors the source folder subtree (content folders plus ancestors, pruned branches omitted) via the same logic workflows already used.

resolveForkFolderMapping is parameterized by resourceType (workflow, file, table, knowledge_base) so each disjoint folder tree is mapped separately. Copied rows get remapped folderId values; unmapped folders still fall back to root.

Files: planForkFileCopies returns folderIdMap and sets optional targetFolderId on blob tasks; finalize writes that folder instead of always null. Older queued blob jobs without targetFolderId still finalize at root.

Fork create / promote: workflow, file, table, and KB folder maps are merged into one map for sim:folder/<id> rewriting in copied markdown/skills and related content refs.

Reviewed by Cursor Bugbot for commit a07e14b. Configure here.

Comment thread apps/sim/ee/workspace-forking/lib/promote/rehome-mapped.ts Outdated
@greptile-apps

greptile-apps Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extends fork folder preservation from workflows to files, tables, and knowledge bases in create and promote flows.

  • Parameterizes folder mapping by resource family.
  • Assigns mirrored target folders to copied resources and file-copy tasks.
  • Merges all folder maps for content-reference rewriting.
  • Adds coverage for file and knowledge-base folder placement.

Confidence Score: 4/5

The PR should not merge until the outstanding dangling-parent mapping defect is fixed, because copied content can be assigned to an unrelated target folder.

The shared mapper still converts an unresolved soft-deleted parent to a root-level mapping key and may reuse an unrelated same-named target folder; the new file, table, and knowledge-base paths consume that mapping directly.

Files Needing Attention: apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts

Important Files Changed

Filename Overview
apps/sim/ee/workspace-forking/lib/copy/copy-workflows.ts Generalizes the shared folder-subtree mapper to operate on each folder-bearing resource type.
apps/sim/ee/workspace-forking/lib/copy/copy-files.ts Mirrors selected files’ folder subtree and carries the resolved target folder into deferred metadata finalization.
apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts Mirrors table and knowledge-base folder trees and assigns remapped folder IDs to inserted resources.
apps/sim/ee/workspace-forking/lib/create-fork.ts Combines workflow, file, table, and knowledge-base folder maps during initial fork creation.
apps/sim/ee/workspace-forking/lib/promote/copy-unmapped.ts Combines per-family folder maps when rewriting references for unmapped resources copied during synchronization.
apps/sim/ee/workspace-forking/lib/promote/promote.ts Explicitly scopes the existing promote-time workflow folder mapping to the workflow tree.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  S[Selected source resources] --> W[Map workflow folders]
  S --> F[Map file folders]
  S --> T[Map table folders]
  S --> K[Map knowledge-base folders]
  W --> U[Merge folder ID maps]
  F --> U
  T --> U
  K --> U
  U --> P[Place copied resources in target folders]
  U --> R[Rewrite sim:folder references]
Loading

Reviews (3): Last reviewed commit: "fix(fork): drop the sync-time re-home pa..." | Re-trigger Greptile

Comment thread apps/sim/ee/workspace-forking/lib/promote/rehome-mapped.ts Outdated
Comment thread apps/sim/ee/workspace-forking/lib/promote/rehome-mapped.ts Outdated
…s, and KBs

Only workflow folders were mirrored into the target workspace on fork
create and on sync. Copied files, tables, and knowledge bases were written
with a hardcoded `folderId: null`, so a push or pull flattened them all
into the target root and lost the source's grouping — visible as a fork
sync that drops folder structure when copying files to the parent.

`resolveForkFolderMapping` already did the real work (prune to folders
holding copied content plus ancestors, reuse same-named target folders,
remap parentId), but was pinned to `resourceType: 'workflow'` on both
reads and on the folder-ceiling check. Parameterize it by resource type
and run it per family, threading the resulting map into each copy instead
of nulling. The four folder-bearing families own disjoint trees and folder
ids are globally unique, so the per-family maps merge cleanly for the
`sim:folder/<id>` content rewrite, which previously resolved only for
workflow folders.

Existing forks are healed on their next sync rather than by a migration:
`rehomeFlattenedForkResources` re-homes mapped files/tables/KBs whose
target `folder_id` is still NULL — the exact signature of the old
flattening — so a placement chosen in the target is never overwritten and
the pass converges to a no-op.

`BlobCopyTask.targetFolderId` is optional so tasks queued by an earlier
deploy replay at the root exactly as before.
…y rows

Self-review of the folder-transit change surfaced two scaling problems in
the re-home pass, both of which grow with the size of the fork edge rather
than the size of the sync:

- The resource lookups built `IN (...)` lists straight from the edge's
  mapping rows, so a large fork could hand Postgres a list approaching the
  bind-parameter ceiling and a pathological query plan. Page them at 500,
  matching the paging the rest of the fork copy already uses.
- The pass re-read the whole edge mapping via `getEdgeMappingRows`, which
  the promote plan had already loaded in the same transaction — a second
  full load of identical rows. Expose them on `ForkPromotePlan` and pass
  them in, which also drops a mock from the re-home tests.

Also tally moved rows from `returning()` rather than the planned batch
size, so the log line reports what the `folder_id IS NULL` guard actually
wrote instead of what was attempted.
…rd-only

Review surfaced three findings and every one of them was in the re-home
pass, none in the forward-looking fix:

- It keyed mapping orientation off `direction`, but the promote route
  resolves the edge from whichever workspace the caller is acting in, so a
  caller in the PARENT pushing to its child is `direction: 'push'` with the
  parent as source. The plan derives this as
  `sourceWorkspaceId === edge.parentWorkspaceId` for exactly that reason.
- Moving a file into a mirrored folder can violate
  `workspace_files_workspace_folder_name_active_unique`, which would abort
  the whole promote transaction and take the workflow sync down with it.
- `folder_id IS NULL` cannot distinguish "flattened by the old copy" from
  "the user moved this to the root", so the pass re-applied on every sync
  and would fight a deliberate placement indefinitely.

The first two are fixable; the third is not without a one-time marker per
edge, which means a migration. A heal that re-applies forever is worse
than no heal, so remove the pass entirely rather than ship it half-right.
Folder structure now transits correctly from this point forward, which is
the actual reported bug; healing already-flattened resources can be a
separate change with a marker to make it run exactly once.

Reverts the `ForkPromotePlan.mappingRows` field with it — it existed only
to feed this pass.
@waleedlatif1
waleedlatif1 force-pushed the fix/fork-sync-folder-structure branch from 68bc46e to a07e14b Compare August 16, 2026 00:32
@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 a07e14b. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Re: the remaining "Dangling parent aliases root folder" finding on copy-workflows.ts (L143-152) — I dug into this rather than waving it off, and I believe it should stay as-is. Three independent reasons:

1. The state it describes isn't reachable through the delete path. It requires an active folder whose parent is soft-deleted. archiveFolderCascade (lib/folders/cascade.ts) stamps the entire descendant subtree in a single UPDATE ... WHERE id IN (folderIds) with one shared timestamp, so a parent folder and its child folders are always soft-deleted together — there's no window between them. (The documented partial-failure window in that file is about child resources walked one at a time, not child folders.) And on a hard delete, folder.parentId is ON DELETE SET NULL, so the child's parentId becomes NULL and it is genuinely root-level — reusing a same-named target root folder is then exactly right, not a mis-placement.

2. The suggested remedy would throw. A "distinct re-rooted copy" cannot exist: folder_workspace_resource_parent_name_active_unique is a partial unique index on (workspace_id, resource_type, coalesce(parent_id, ''), name) WHERE deleted_at IS NULL. Two active root folders with the same name in the same collection are forbidden, so creating a distinct one would raise a unique violation and abort the entire fork/sync transaction — strictly worse than the current behavior.

3. Reuse-by-name is the mapper's deliberate policy, not a quirk of this branch. The same ${mappedParentId ?? ''}::${name} lookup is what makes the ordinary case work (source root Reports maps onto target root Reports instead of duplicating it). The dangling-parent path re-roots first and then applies that same policy consistently.

This is also pre-existing behavior that predates the PR — it shipped for workflow folders and is unchanged here; this PR only parameterizes the mapper by resource_type so files, tables, and knowledge bases stop being flattened to the root. Tightening the reuse rule would change shipped workflow fork placement for a state the delete path does not produce, which is risk without benefit.

Happy to change it if you disagree — but the fix would need to be "re-root the content to the target root (folder_id = NULL)" rather than "create a distinct folder," since the index rules the latter out.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
waleedlatif1 merged commit 76318e4 into staging Aug 16, 2026
31 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/fork-sync-folder-structure branch August 16, 2026 00:51
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