Move commit to new branch when dropping onto NewStackDropzone#13191
Move commit to new branch when dropping onto NewStackDropzone#13191cbjeukendrup wants to merge 1 commit intogitbutlerapp:masterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
Enables dropping a commit onto the “outside lanes” / NewStackDropzone to create a new stack (branch) and move the commit there, aligning commit drag-and-drop behavior with the new-stack workflow in the Desktop app.
Changes:
- Add support for
CommitDropDatainOutsideLaneDzHandler.accepts()andondrop(). - Implement
ondropCommitData()to create a new stack and callstackService.moveCommit(), showing a warning toast for illegal moves.
| async ondropCommitData(data: CommitDropData) { | ||
| const stack = await this.stackService.newStackMutation({ | ||
| projectId: this.projectId, | ||
| branch: { name: undefined }, | ||
| }); | ||
|
|
||
| const illegalAction = await this.stackService.moveCommit({ | ||
| projectId: this.projectId, | ||
| targetStackId: ensureValue(stack.id), | ||
| commitId: data.commit.id, | ||
| sourceStackId: data.stackId, | ||
| }); | ||
|
|
||
| if (illegalAction) { | ||
| const message = getMoveCommitIllegalActionMessage(illegalAction); | ||
| showToast({ | ||
| style: "warning", | ||
| title: "Cannot move commit", | ||
| message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
ondropCommitData() creates a new stack before attempting moveCommit(). If moveCommit() returns an illegalAction, the commit is not moved but the newly created stack remains, which can leave users with an unintended empty stack. Consider preflighting the move before creating the stack, or cleaning up the newly created stack/branch when illegalAction is returned.
There was a problem hiding this comment.
This is also true, but more complicated.
We cannot 'preflight' it as suggested, but we can delete the branch if the move operation fails.
Let's do that please
| async ondropCommitData(data: CommitDropData) { | ||
| const stack = await this.stackService.newStackMutation({ | ||
| projectId: this.projectId, | ||
| branch: { name: undefined }, | ||
| }); | ||
|
|
||
| const illegalAction = await this.stackService.moveCommit({ | ||
| projectId: this.projectId, | ||
| targetStackId: ensureValue(stack.id), | ||
| commitId: data.commit.id, | ||
| sourceStackId: data.stackId, | ||
| }); |
There was a problem hiding this comment.
When moving a commit to a new stack, the UI selection for the source lane can still reference data.commit.id (and the new stack isn’t selected), since moveCommit has no StackService side-effect here. This can leave the preview/selection pointing at a commit that no longer exists in the source stack. Consider clearing the source lane selection if it matches the moved commit (as MoveCommitDzHandler does) and/or selecting the moved commit in the newly created stack.
There was a problem hiding this comment.
This is not a bad note.
I think we should at the very least unselect the commit.
Move commit doesn't return a commit map with the new commit ID, but in the meantime, we should clear the stale selection
estib-vega
left a comment
estib-vega
left a comment
There was a problem hiding this comment.
I reeeeally want this. 'tearing off' commits would be super cool.
Only echoed the copilot commits with some extra context.
Future steps from my side
It probably makes sense to add a first-class API for this operation, so that it's atomically handled and can be rolled-back in a single op. Plus the added benefit of having this in the CLI & TUI as well
| async ondropCommitData(data: CommitDropData) { | ||
| const stack = await this.stackService.newStackMutation({ | ||
| projectId: this.projectId, | ||
| branch: { name: undefined }, | ||
| }); | ||
|
|
||
| const illegalAction = await this.stackService.moveCommit({ | ||
| projectId: this.projectId, | ||
| targetStackId: ensureValue(stack.id), | ||
| commitId: data.commit.id, | ||
| sourceStackId: data.stackId, | ||
| }); |
There was a problem hiding this comment.
This is not a bad note.
I think we should at the very least unselect the commit.
Move commit doesn't return a commit map with the new commit ID, but in the meantime, we should clear the stale selection
| async ondropCommitData(data: CommitDropData) { | ||
| const stack = await this.stackService.newStackMutation({ | ||
| projectId: this.projectId, | ||
| branch: { name: undefined }, | ||
| }); | ||
|
|
||
| const illegalAction = await this.stackService.moveCommit({ | ||
| projectId: this.projectId, | ||
| targetStackId: ensureValue(stack.id), | ||
| commitId: data.commit.id, | ||
| sourceStackId: data.stackId, | ||
| }); | ||
|
|
||
| if (illegalAction) { | ||
| const message = getMoveCommitIllegalActionMessage(illegalAction); | ||
| showToast({ | ||
| style: "warning", | ||
| title: "Cannot move commit", | ||
| message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This is also true, but more complicated.
We cannot 'preflight' it as suggested, but we can delete the branch if the move operation fails.
Let's do that please
|
Thanks for the review! I will work on these suggestions, but might not get to it until the end of the week or so, so please bear with me :) |
No description provided.