-
Notifications
You must be signed in to change notification settings - Fork 349
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
Fix inline-edit prompts chat building #6003
Conversation
@@ -97,8 +97,7 @@ export class EditProvider { | |||
onTurnComplete: async () => { | |||
typewriter.close() | |||
typewriter.stop() | |||
void this.handleResponse(text, false) | |||
return Promise.resolve() | |||
return this.handleResponse(text, false) |
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.
@abeatrix any concerns on awaiting here handleResponse?
@@ -157,7 +156,7 @@ export class EditProvider { | |||
break | |||
} | |||
case 'complete': { | |||
void multiplexer.notifyTurnComplete() | |||
await multiplexer.notifyTurnComplete() |
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.
Same question here, I haven't seen anything suspicious in multiplexer implementation but maybe I'm missing here something important
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.
I have no idea what sideeffects this will have. Will have to dig into the code for it later. Just approving to unblock. Please get an approval from @abeatrix. I'll try to check the code myself soon, if it doesn't get approved.
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.
The code look good to me but I might be missing some context on why it was implemented this way. I'm afk rn so if you can check and confirm the PR that made this change did not make this change on purpose.
The only thing I can think of that could be affected by this change is the code lenses state, so if you can confirm they are working as expected for both JetBrains and VS Code, I think this should be safe.
I approved to unblock but i think it'd be safer if we could move it to Fixup controller like I mentioned in the other PR, where we could update them accordingly when we change the state of the task, wdyt? |
Is this fix effective? What's are the messages in the stream?
|
This change ensures that all vital fields on the task object will be updated when we resolve
This is exactly right, the original problem was that task object doesn't get updated in time when we resolve the original edit flow. I think this change should be okay, because if we take a look at the |
What how does waiting for |
So, consumers send a command and expect a task object with a completed state. Previously, the edit command returned a task object with still inProgressState (with not properly updated task fields); this awaiting ensures that the consumer will get a proper "finished" task object. You can check the 'executeEdit' method in the
If I understood this correctly, the first option is our case here |
I've noticed that on the main when you run the inline prompt, the chat doesn't have any different messages from the actual inline edit task. Apparently, it looks like there is a race condition between task execution and logic that populates chat with diffs. The main problem is that at the moment when we run populate with chat messages,
task.diff
has an undefined value.This PR tries to fix this problem by awaiting the task finish state.
Test plan
prompt-creation-v2
flag enabled already)