fix(forking): stop a parent re-pick blanking a dependent's stored target value - #6787
fix(forking): stop a parent re-pick blanking a dependent's stored target value#6787waleedlatif1 wants to merge 3 commits into
Conversation
…get value A dependent selector (a sheet under a spreadsheet, a label under a mailbox) is invalidated when its parent is re-picked, because the stored child no longer exists under the new parent. That invalidation was recorded by writing an empty string into the in-session override map — the same value the user's own "clear this field" produces. The two are not the same thing, and the map is submitted verbatim and written into the target workflow's configuration, so an invalidated field cleared the target's real stored value. The sharpest case is an undo. Re-pick a parent away from its original target, then back. The parent nets out unchanged, so nothing is remapped and the remap's own clearing pass never runs — but the child is still blank, and that blank lands on a value the user never touched, with nothing in the UI showing it happened. Record the invalidation with a distinct marker instead. It reads as blank in the selector, the in-block chain context, and the Sync gate, so a required invalidated field still blocks Sync and still renders; but it is omitted from the submitted payload rather than sent as empty, so no override is written and the target keeps what it had. A blank the user picked themselves is still submitted and still clears the target. Also: skip the cascade entirely when a re-pick selects the value the field already had, since the selector fires its change handler either way. Fork file copy: a file whose name is already taken in a reused target folder is de-duplicated with the same allocator the ordinary upload path uses, rather than colliding with the folder-name unique index and being dropped from the fork with its blob deleted. Adds hook-level coverage for the submitted payload, which had none.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Fork file blob copy de-duplicates names in reused target folders via Adds Reviewed by Cursor Bugbot for commit d636d5e. Configure here. |
Greptile SummaryThe PR distinguishes automatic dependent invalidation from intentional clearing, restores dependent values when provider changes are undone, de-duplicates copied file names, and preserves edits made during asynchronous Save or Sync operations.
Confidence Score: 5/5The PR appears safe to merge. The previously reported loss of newer edits is fixed because every content-changing setter creates a new state object and successful Save or Sync requests clear state only when it is still the exact submitted snapshot; no blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/ee/workspace-forking/components/fork-sync/dependent-value.ts | Introduces distinct invalidation state and restores descendant values only when their complete provider chain returns to baseline. |
| apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx | Supplies atomic previous-value and baseline context when applying dependent selector changes. |
| apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts | Builds dependent payloads from the new state model and preserves newer edits by conditionally clearing exact submitted snapshots. |
| apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.test.tsx | Covers dependent payload semantics and verifies that edits made during pending Save and Sync requests survive completion. |
| apps/sim/ee/workspace-forking/lib/copy/copy-files.ts | Allocates collision-free names before copying files into reused target folders. |
Reviews (2): Last reviewed commit: "fix(forking): preserve edits during fork..." | Re-trigger Greptile
|
Carrying over a Cursor Bugbot finding from #6783, where these files lived before the split.
The mechanism is real — undo is A→B then B→A, and on the second pick Before this PR the cascade wrote a literal
That flip is the fix. With only markers present the payload is byte-identical to the stored mapping, so there is nothing to save — 'Save enabled' was the destructive path this PR closes. The escape is one click and on-screen: a required field reading Worth noting the obvious fix is wrong: deleting the marker on undo would restore |
* fix(workflows,connectors): close pre-merge audit findings Recover subblock values orphaned by the id renames in this release, and stop truncated knowledge-base listings from reporting themselves complete. - Add operation-scoped subblock id migrations so a saved workflow's stored value survives a rename. Cloudflare create/update DNS record, ServiceNow read record, and Okta deactivate/delete previously lost their stored value: the create path substituted a seeded default (an A record where the user chose CNAME, and unproxied where they chose proxied), and the update path silently no-opped while reporting success. A migration is used rather than a legacy-id fallback so no subblock id carries two value spaces at runtime. - Webflow, Zendesk: a listing that stops for a reason the connector cannot rule out now reports as capped instead of exhausted. A malformed envelope, an unfollowable continuation link, or an absent collection list previously read as a complete listing and let deletion reconciliation hard-delete every document past the truncation point. - Sentry: pin the listing window in the request rather than inheriting the server default, so the range cannot silently narrow into hard deletes. - Fork sync: a parent re-pick no longer writes a blank over a hidden optional dependent's stored target value, and a required field stays on screen once it is filled. Add hook-level coverage for the submitted payload. - Fork file copy: a file whose name is already taken in a reused target folder is de-duplicated instead of dropped. - Delete an orphaned Shopify OAuth route that built a credential from unsigned cookies. It had no writer, no caller, and no inbound link. - Tailwind: drop two content globs that scanned 5.4k files to emit one unused rule, keeping the ones that fix brand tile icon color. - Correct the API route-count baseline, add an Evernote docs redirect, align library copy with the language rules, and fix a stale turbo filter. * fix(connectors,forking): trim the audit fixes to their minimum A legitimacy review found several changes closed no live defect, and two introduced problems of their own. - Zendesk: narrow the cursor fix to a signal change. Treating a missing meta envelope as truncation had also made the walk follow links.next and keep paginating, and the ticket cursor has no page-depth valve, so a source advertising a next page with no meta could loop without terminating. The page-fetch set now matches the previous behavior; only the flag is new. - Zendesk: drop the search next_page branch. The existing count check already caps every case where a missing key could lose documents. - Webflow: drop the empty-collections flag. The sync engine already blocks the first sync on an empty listing and reconciles only when a second sync agrees, which handles a transient fault better and still removes documents when a source is genuinely emptied. The flag short-circuited that and suppressed reconciliation permanently. Restore the previous loud failure on a non-array envelope, and drop the unreachable collection-id filter. - Webflow: soften a docstring that claimed pagination.total is always present. It is documented optional, so its absence proves nothing either way and treating it as unprovable truncation is the fail-safe reading. - Sentry: drop the pinned statsPeriod. Sentry's issue search floors every query at 90 days in the executor regardless of the request, and the endpoint this release moved away from hit the same floor, so there was no window to close. Keep the tests and the docstring recording that. - Fork copy: drop the renamed counter, which no caller reads. - Repair check-block-registry, which stopped exempting migrated subblock ids when the migration map became an array — `in` was testing array indices. - Drop mdx from a Tailwind content glob that emits nothing, and loosen an exact compiled-SQL assertion to the invariant it was pinning. * fix(migrations): keep a ServiceNow write body off the read projection Review findings from the first round. - A legacy ServiceNow block can hold a Create/Update Record JSON body under `fields` while its stored operation is Read Records: the id served both value spaces before the rename, and a subblock value is not cleared when the operation changes. The scoped migration moved that body onto `readFields`, where it would reach the wire as sysparm_fields. Migration entries can now carry a `whenValue` predicate for the case where the stored operation alone cannot separate two value spaces, and the ServiceNow entry uses it to move only a plausible comma-separated projection. - Type the fork copy test harness instead of using `any`, without weakening it: every predicate shape it does not model still throws rather than matching. - Correct the dependent-omission comments. Omitting a parent-invalidated field preserves the target's stored value on Save and across an undo, where the parent nets out unchanged; on a Sync the written state is source-derived, so what it prevents there is an explicit blank reaching the fields the remap's clearing pass does not cover, nested tool params in particular. Okta's migration scope is left as-is: `okta_remove_user_from_app` and the sendEmail split shipped in the same release, so no saved block can hold legacy state for it, and widening the scope would promote an activation-era value onto the deactivation switch. Tests document the boundary. * chore(forking): move the fork-sync changes to their own PR The dependent-omission fix and the fork file-copy de-duplication are reviewed separately in #6787. They are the only changes here that overlap #6776, and they carry their own design tradeoff, so they should not ride along with the unrelated audit fixes in this PR. * fix(migrations): separate a ServiceNow write body from a projection by parsing The guard tested for a `{` or `[` prefix, so a stored scalar body — `true`, `"short_description"`, `42` — read as a field list and was promoted onto `readFields`, where it would go out as sysparm_fields. A Create/Update Record body is JSON and a projection is a bare comma-separated field list, which is never valid JSON, so parsing is the whole test rather than a guess at its opening character. Ambiguity still resolves to "not a projection", leaving the value where the Create/Update control owns it. * test(connectors,credentials): tie two assertions to what they actually prove - Webflow: a non-array collections envelope reaching `for...of` throws, which is the intended loud failure. Assert the spec-mandated TypeError plus a single request and no write-back, rather than matching V8's wording. - Credentials: the second guard test cannot observe "not deleted" — the proxy driver replays canned rows — so name it for what it does verify, that the reference check carries no workspace predicate and an empty RETURNING logs nothing. Making the driver decide the outcome would fake the database. - Drop `vi.importActual`; a plain `drizzle-orm/pg-proxy` import works now that `drizzle-orm` is un-mocked. * fix(migrations): identify a ServiceNow projection by its own shape Recognising a write body was the wrong way round. A saved body is not always well-formed: it can be a half-typed draft or carry an unquoted block reference, so neither "opens with a brace" nor "fails to parse as JSON" identifies one — and a body misread as a projection is moved to readFields with its original key dropped, losing the draft. Match the projection instead: a comma-separated list of ServiceNow field names, which are word characters plus the dot of a dotted walk. A brace, quote, colon, angle bracket or interior space fails that shape. Parsing then removes the bare scalars that satisfy it by accident.
|
@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 d636d5e. Configure here.
Summary
Notes for review
Split out of #6783 so it can be reviewed on its own.
Complements #6776 rather than competing with it: that PR scoped the cascade and added the edit affordance (visibility); this one fixes what gets submitted (payload).
sameDependencyScopeis a no-op for two top-level subblocks, so the blank-write path survives it — and #6776 widened the cascade to nested tool params, which is exactly the path the remap's clearing pass doesn't cover.The marker is a sentinel string because the override map is typed
Record<string, string>and this needs a third state. The NUL prefix keeps it disjoint from every real selector value; it never leaves memory (submittedDependentValuereturnsundefinedfor it). Widening the map tostring | nullwould remove the sentinel but is a broader change across the view and the hook — worth doing separately if preferred.Type of Change
Testing
ee/workspace-forking: 39 files / 593 tests passing.bun run type-checkandbun run lintclean.Each behavior is covered by a test that fails without it — verified by reverting each change in place and confirming the suite went red for the right reason before restoring.
Checklist