Agentic UI: Allow cancelling an in-flight push or pull - #4510
Conversation
bcotrim
changed the title
📊 Performance Test ResultsComparing abdec3a vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
| enabled, | ||
| label: enabled | ||
| ? __( 'Cancel pull' ) | ||
| : __( 'Pull can not be cancelled while importing changes to your local site' ), |
There was a problem hiding this comment.
I think we can probably tighten the line height here if the sentence splits in two lines:
katinthehatsite
left a comment
katinthehatsite
left a comment
There was a problem hiding this comment.
Generally, the changes look good to me 👍
I noticed one thing:
- start and cancel push
- observe the the header says
Push cancelled - wait for a couple of minutes
- observe that the header now says
Pushed 2 mins agoeven though the push was cancelled
I am wondering if the Pushed 2 mins ago should appear in this case at all, what do you think?
At the moment it is preserving whatever message it was before the push/cancel. Cancel does not update it? Was that the behavior you saw? I pushed some fixes, can you take another look please? |
So what seems to happen is that if you push, it shows the pushed x amount of time ago, then you try to push again and cancel, it will show |
| */ | ||
| const SYNC_ABORT_CONTROLLERS = new Map< string, AbortController >(); | ||
|
|
||
| // Agentic push/pull resolve with this instead of rejecting when the user cancels. |
There was a problem hiding this comment.
I think the type below is pretty self-explanatory
| remoteSiteId: number; | ||
| }; | ||
|
|
||
| // Stopping is best-effort: the push/pull mutation itself rejects with a |
There was a problem hiding this comment.
I think this can also be removed as it is already visible from the code
| remoteSiteId, | ||
| } ); | ||
| } | ||
| // See `pullSiteFromLive` — same registry, so `cancelSyncOperation` covers both UIs. |
There was a problem hiding this comment.
the template literal on the next line already shows this, I think
| { sitePath: site.path, remoteSiteId, options, signal: release.signal } | ||
| ); | ||
| } catch ( error ) { | ||
| // See the pull route — a cancel is reported, not raised as a 500. |
There was a problem hiding this comment.
The comment restates the code pattern so I would drop it as well
katinthehatsite
left a comment
katinthehatsite
left a comment
There was a problem hiding this comment.
I think the functionality works as expected 👍 I left some minor suggestions regarding cleaning up the comments.
…ncel # Conflicts: # apps/ui/src/components/site-dropdown/main-view.tsx
3 participants
Related issues
How AI was used in this PR
Written with Claude Code, then verified end to end against real pushes and pulls
Proposed Changes
You can now stop an in-flight sync from the site dropdown, with the same boundary Classic uses: a push can be cancelled until the remote import is initiated, a pull until the CLI starts writing your local site. Past that point the button is disabled and the panel says why, because stopping there would leave a half-imported site. Cancelling reports "Push/Pull cancelled" rather than an error, and is logged as such.
Testing Instructions
[Sync] Pull cancelled by userwith no error.studioCLI process survives a cancel (ps aux | grep "main.mjs pull").Pre-merge Checklist