Edit: introduce CodyTaskState.Reverting and FixupTask.retryID #4149
Conversation
I am introducing an automation on top of Fixup and need to be able to
track when a Fixup for a region is done. "retry" is actually an "undo"
on the old FixupTask and then a new FixupTask is created. Before this
commit there was no way to link the old task to the new retried task.
This commit introduces a field on FixupTask which records the new task's
ID. Additionally a "Reverting" state is introduced which occurs while
undoing a task before it enters a terminal state. We then adjust "retry"
to only set the terminal state once the new FixupTask is created. This
makes it possible to set the retryID field before updating the task a
terminal state.
Test Plan: Added the following debug line to FixupController.createTask
task.onDidStateChange(state => {
console.log('onDidStateChange', task.id, state, task.retryID)
})
First did an edit followed by an undo. Expectation is that for the undo
we now have a Reverting state introduced:
onDidStateChange lwszk Working undefined
onDidStateChange lwszk Applying undefined
onDidStateChange lwszk Formatting undefined
onDidStateChange lwszk Applied undefined
onDidStateChange lwszk Reverting undefined
onDidStateChange lwszk Finished undefined
Secondly did a edit followed by a retry. Expectation is we enter
reverting state, create the new task, finish the old task with retryID
set, then continue with the new task:
onDidStateChange lwszqu Working undefined
onDidStateChange lwszqu Applying undefined
onDidStateChange lwszqu Formatting undefined
onDidStateChange lwszqu Applied undefined
onDidStateChange lwszqu Reverting undefined
onDidStateChange lwtj Working undefined
onDidStateChange lwtj Applying undefined
onDidStateChange lwszqu Finished lwtj
onDidStateChange lwtj Formatting undefined
onDidStateChange lwtj Applied undefined
|
FYI I may be going about this completely the wrong way. I am experimenting with a workflow which takes a bunch of github PR inline comments and automatically guides the user through using those comments to run fixup on each region. I was struggling to understand when a user was finally done with the fixup on one that I create. This was the approach I settled on as reasonable, so extracted it out. Very happy with any and all feedback :) I am a TS noob. |
There was a problem hiding this comment.
On the whole, I love this, but it is going to need coordination on the JetBrains side. Can you decide whether you want to make use of task.diff.bufferText I mentioned inline, and then coordinate with me and/or @steveyegge to make sympathetic changes in JetBrains when this is really ready to land?
| * @returns the terminal state to set for the task. | ||
| * | ||
| * TODO: It is possible the original code is out of date if the user edited | ||
| * it whilst the fixup was running. Handle this case better. Possibly take a |
There was a problem hiding this comment.
We have that, see task.diff.bufferText and look forwards and backwards from here if you have doubts: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody@1d3f66514768816b3531843d9bae5c579d08f06d/-/blob/vscode/src/non-stop/diff.ts?L263:88-263:92
There was a problem hiding this comment.
FYI this TODO is from the original code, sorry my editor reflowed the paragraph.
|
Could you add some tests for this, too, maybe cribbing from https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody@1d3f66514768816b3531843d9bae5c579d08f06d/-/blob/vscode/test/e2e/command-edit.test.ts for end-to-end tests, https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody@1d3f66514768816b3531843d9bae5c579d08f06d/-/blob/agent/src/index.test.ts for "agent" (with our stub implementation of the VSCode API), or https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody@1d3f66514768816b3531843d9bae5c579d08f06d/-/blob/vscode/src/non-stop/diff.test.ts (unit tests.) You don't need to write all of these kinds, just use the right tool for the job. |
There was a problem hiding this comment.
On the whole, I love this, but it is going to need coordination on the JetBrains side.
FYI I am pausing the experiment that motivated this PR while I do work on OpenCtx. But in general the concept introduced here seems useful to anyone automating on top of Fixup so happy to get this in now. Your call if you would rather not do it given that.
Can you decide whether you want to make use of
task.diff.bufferTextI mentioned inline.
I responded inline, but that is an old TODO I accidently reflowed. I don't have enough familarity with how everything fits together to know if at this part of the code we can remove the TODO. Can I?
| * @returns the terminal state to set for the task. | ||
| * | ||
| * TODO: It is possible the original code is out of date if the user edited | ||
| * it whilst the fixup was running. Handle this case better. Possibly take a |
There was a problem hiding this comment.
FYI this TODO is from the original code, sorry my editor reflowed the paragraph.
I am introducing an automation on top of Fixup and need to be able to track when a Fixup for a region is done. "retry" is actually an "undo" on the old FixupTask and then a new FixupTask is created. Before this commit there was no way to link the old task to the new retried task.
This commit introduces a field on FixupTask which records the new task's ID. Additionally a "Reverting" state is introduced which occurs while undoing a task before it enters a terminal state. We then adjust "retry" to only set the terminal state once the new FixupTask is created. This makes it possible to set the retryID field before updating the task a terminal state.
Test Plan
Added the following debug line to FixupController.createTask
First did an edit followed by an undo. Expectation is that for the undo we now have a Reverting state introduced:
Secondly did a edit followed by a retry. Expectation is we enter reverting state, create the new task, finish the old task with retryID set, then continue with the new task: