fix(fork): preserve folder structure across a fork edge for files, tables, and knowledge bases - #6752
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Files: Fork create / promote: workflow, file, table, and KB folder maps are merged into one map for Reviewed by Cursor Bugbot for commit a07e14b. Configure here. |
Greptile SummaryThis PR extends fork folder preservation from workflows to files, tables, and knowledge bases in create and promote flows.
Confidence Score: 4/5The 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
|
| 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]
Reviews (3): Last reviewed commit: "fix(fork): drop the sync-time re-home pa..." | Re-trigger Greptile
…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.
68bc46e to
a07e14b
Compare
|
@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 a07e14b. Configure here.
|
Re: the remaining "Dangling parent aliases root folder" finding on 1. The state it describes isn't reachable through the delete path. It requires an active folder whose parent is soft-deleted. 2. The suggested remedy would throw. A "distinct re-rooted copy" cannot exist: 3. Reuse-by-name is the mapper's deliberate policy, not a quirk of this branch. The same 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 Happy to change it if you disagree — but the fix would need to be "re-root the content to the target root ( |
Summary
folderId: null, so every push/pull flattened them into the target root and lost the source's grouping.resolveForkFolderMappingalready did the work (prune to folders holding copied content plus ancestors, reuse same-named target folders, remapparentId) but was pinned toresourceType: 'workflow'on both reads and the folder-ceiling check. Parameterized it by resource type and run it per family.sim:folder/<id>content rewrite — which previously resolved only for workflow folders.BlobCopyTask.targetFolderIdis 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 NULLcannot 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
Testing
ee/workspace-forking;bun run type-checkclean; all 27check:auditspassfolderIdremap turns the file-placement test redNotes for review
assertFolderCollectionHasRoomcan 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