Skip to content
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: Autofix timeout bug #668

Merged
merged 14 commits into from
Oct 7, 2024
Merged

fix: Autofix timeout bug #668

merged 14 commits into from
Oct 7, 2024

Conversation

BBerabi
Copy link
Contributor

@BBerabi BBerabi commented Sep 17, 2024

Description

Autofix Diffs flow (web view flow) was implemented slightly differently than the code action flow. The diff flow treated an empty list of suggestions as an error and invalid response. This is not the correct behavior because the AI Fix pipeline might run successfully and there can be no valid fixes found.

The code was polling until the suggestion's length was not 0. This causes the returned successful response with empty suggestions to be ignored and the code aggressively keeps polling until the timeout hits. The timeout takes 2 minutes whereas the result is available after several seconds. The timeout also hides the real issue from the user, making the user think something unexpected happened whereas we could not compute any valid fix.

This PR corrects the flow for autofix diffs by

  • adding status into the return value similar to how it is done for code actions
  • corrects the logic in the polling function

Checklist

  • Tests added and all succeed
  • Linted
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

I uploaded a video to my drive showing the incorrect behavior on the IDE.
https://drive.google.com/file/d/1BlHmbkW5WEiSdPQKzmjoU-5IspK20PoZ/vi

and here the corrected behaviour which immediately shows user the result of 0 fixes

ide_no_timeout.mp4

@BBerabi
Copy link
Contributor Author

BBerabi commented Sep 27, 2024

I think the failing test is not related to my changes but because of an RCE vulnerability detected by Snyk Code. Is anyone already looking at it?

Copy link
Contributor

@ShawkyZ ShawkyZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Berkay
It looks good to me besides some nitpicks.

Let's pair on it on Monday. I would like to revisit / understand some stuff here, like why do we have the AutofixStatus struct and if we can simplify/refactor it a bit.

domain/ide/command/code_fix_diffs.go Outdated Show resolved Hide resolved
domain/ide/command/code_fix_diffs.go Outdated Show resolved Hide resolved
infrastructure/code/autofix.go Outdated Show resolved Hide resolved
@ShawkyZ ShawkyZ changed the title Fix/timeout bug fix: Autofix timeout bug Sep 30, 2024
Copy link
Collaborator

@bastiandoetsch bastiandoetsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks - please decide yourself what to update :). Thanks for the PR, Berkay!

@ShawkyZ ShawkyZ merged commit 45039e1 into main Oct 7, 2024
16 of 17 checks passed
@ShawkyZ ShawkyZ deleted the fix/timeout-bug branch October 7, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants