Skip to content

fix(forking): make the trigger URL preview reflect the user's actual picks - #6290

Merged
icecrasher321 merged 1 commit into
stagingfrom
fix/fork-trigger-url-picks
Aug 5, 2026
Merged

fix(forking): make the trigger URL preview reflect the user's actual picks#6290
icecrasher321 merged 1 commit into
stagingfrom
fix/fork-trigger-url-picks

Conversation

@icecrasher321

Copy link
Copy Markdown
Collaborator

Summary

Address comments on trigger url UI exposure. The choice resolution is a pure module mirroring resolveForkTriggerPaths (offered-paths guard, first-claim-wins), so the preview and the server agree by construction rather than by two hand-kept implementations.

Type of Change

  • Bug fix

Testing

N/A

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)

…picks

Two Cursor findings on #6272, both in the preview layer - the sync's write path
was correct in each case, but the UI stated an outcome that did not match it.

- The heads-up and overwrite confirm read `triggerUrlChanges` straight off the
  diff, which the server computes with its DEFAULT resolution before the user
  chooses anything. Selecting "Generate new URL" for a trigger that would have
  adopted a URL therefore killed that URL with no warning, in the one modal whose
  job is to state irreversible consequences (it also over-warned in the reverse
  case). The diff now returns the RAW retiring set and the client subtracts the
  live choices, so the rows, the heads-up and the confirm cannot disagree.
- The picker let two triggers select the same retiring URL and showed both as
  keeping it. Two blocks cannot serve one path (`path_deployment_unique`) and the
  resolver awards it to the first slot, so the loser silently got a new URL. A
  path another row claimed is now disabled and named, and each row displays its
  RESOLVED outcome rather than its raw pick.

The choice resolution is a pure module mirroring `resolveForkTriggerPaths`
(offered-paths guard, first-claim-wins), so the preview and the server agree by
construction rather than by two hand-kept implementations.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 5, 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 5, 2026 5:31pm

Request Review

@cursor

cursor Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes fork sync preview contracts and webhook URL UX; incorrect resolution could mislead users about which external URLs stop working, though behavior is covered by new unit tests mirroring server logic.

Overview
Fixes fork sync UI where Heads up, overwrite confirm, and Trigger URL rows could disagree about which webhook URLs stop being served when users change adoption choices.

The fork diff API now returns retiringTriggerUrls (all URLs retired before adoption) instead of a post-default triggerUrlChanges list. The client derives which URLs actually die from the user’s current picks.

A new trigger-choices module mirrors server resolveForkTriggerPaths: offered-path validation, first-claim-wins for contested paths, and forkDyingTriggerUrls for the live warning set. use-fork-sync wires this into triggerUrlChanges; TriggerMappingRow shows resolved outcomes via triggerChoiceFor and disables adopt options already taken by another row (triggerPathOwnersFor).

Reviewed by Cursor Bugbot for commit a9b167c. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR moves trigger-URL retirement resolution into live client state so preview rows and confirmation warnings reflect current adoption picks.

  • Adds pure helpers for resolving first-claim-wins trigger choices, path ownership, and dying URLs.
  • Returns raw retiring URLs from the fork-diff endpoint and derives final warnings in the hook.
  • Updates trigger selection UI and adds resolver and contract tests.
  • The response-field rename needs a compatibility bridge to preserve warnings during mixed-version rollout.

Confidence Score: 4/5

The mixed-version response-field rename can suppress irreversible trigger-URL retirement warnings and should be fixed before merging.

The client and server each recognize only one side of the renamed diff-response field, so version-skewed requests parse the warning list as empty; the remaining resolver behavior otherwise matches the server’s first-claim-wins semantics.

Files Needing Attention: apps/sim/lib/api/contracts/workspace-fork.ts, apps/sim/app/api/workspaces/[id]/fork/diff/route.ts, apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts

Important Files Changed

Filename Overview
apps/sim/lib/api/contracts/workspace-fork.ts Renames the diff response field to expose raw retiring URLs, but does not preserve wire compatibility across mixed-version rollout.
apps/sim/app/api/workspaces/[id]/fork/diff/route.ts Returns the raw retiring trigger set instead of the server’s default resolution; newly added comments violate the mandated comment format.
apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.ts Implements offered-path validation and deterministic first-claim-wins resolution matching the server algorithm.
apps/sim/ee/workspace-forking/components/fork-sync/use-fork-sync.ts Derives warnings and row outcomes from current choices, but relies exclusively on the newly renamed response field.
apps/sim/ee/workspace-forking/components/fork-sync/fork-sync-view.tsx Displays resolved outcomes and disables paths owned by another row; some new explanatory comments violate repository guidance.
apps/sim/ee/workspace-forking/components/fork-sync/trigger-choices.test.ts Covers defaults, explicit choices, invalid offers, contested paths, dying URLs, and owner labels.
apps/sim/lib/api/contracts/workspace-fork.test.ts Updates contract expectations for the new field but does not exercise old/new field interoperability.

Sequence Diagram

sequenceDiagram
  participant UI as Fork Sync UI
  participant Diff as Fork Diff API
  participant Resolver as Client Choice Resolver
  participant Promote as Promote API
  Diff-->>UI: retiringTriggerUrls + triggerMappings
  UI->>Resolver: mappings + live user picks
  Resolver-->>UI: resolved paths + dying URLs
  UI->>Promote: triggerMappingOverrides
  Promote->>Promote: rebuild plan and resolve paths
  Promote-->>UI: applied trigger URL changes
Loading

Reviews (1): Last reviewed commit: "fix(forking): make the trigger URL previ..." | Re-trigger Greptile

Comment thread apps/sim/lib/api/contracts/workspace-fork.ts
Comment thread apps/sim/app/api/workspaces/[id]/fork/diff/route.ts

@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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a9b167c. Configure here.

@icecrasher321
icecrasher321 merged commit 402f862 into staging Aug 5, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/fork-trigger-url-picks branch August 5, 2026 18:59
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