New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Edit: introduce CodyTaskState.Reverting and FixupTask.retryID #4149
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.bufferText
I 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: