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

Feat: display preview to AI fix #445

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Conversation

MichaelAquilina
Copy link
Contributor

@MichaelAquilina MichaelAquilina commented Mar 26, 2024

Description

Add a new display preview when generating AI Fixes instead of them being applied to the user's code immediately.

The changes can be applied by the user with the "Apply Fix" button if they are happy with the changes being suggested.

If clicked, the updated code will be temporarily highlighted until either:

  • 30 seconds have passed
  • the user saves the file
  • the user changes some text in the file

Checklist

  • Tests added and all succeed
  • Linted
  • CHANGELOG.md updated
  • README.md updated, if user-facing

Screenshots / GIFs

image

Screencast.from.2024-04-05.10-13-39.webm
Screencast.from.2024-04-17.16-47-21.webm

@MichaelAquilina MichaelAquilina force-pushed the feat/display-ai-fix branch 2 times, most recently from 88f2892 to a95a72e Compare March 27, 2024 15:13
@MichaelAquilina MichaelAquilina changed the title Feat/display ai fix Feat: display preview to AI fix Apr 2, 2024
@MichaelAquilina
Copy link
Contributor Author

CHANGELOG.md updated
README.md updated, if user-facing

Not sure what the convention for these two are if someone can help :) I'm guessing merging to main wont release a new version immediately so not sure what exact contents to put in CHANGELOG.md

For README.md's case, I dont see any case where it needs to be updated - possibly because AI Fix is still an opt in feature? But I could be wrong :)

@Arvi3d
Copy link
Contributor

Arvi3d commented Apr 10, 2024

IMHO, error handling is a pretty big deal considering the state of Code Fix AI services.
In that improvements document, it's not even priority#1. From a UX point of view, I disagree with that.

But also, this is a tech debt, so I'm going to leave it for @bastiandoetsch to decide and unblock it from my side.

@Arvi3d
Copy link
Contributor

Arvi3d commented Apr 10, 2024

@MichaelAquilina could you please also add a task for application workflow.
What we talked about in a call. Whether we remove the suggestion or introduce an additional stage to it. We must give a feedback to developer, that he/she successfully applied the fix.

@Arvi3d Arvi3d dismissed their stale review April 10, 2024 17:10

wrote in a comment below. this PR is pretty big and my comment might be considered not vital to it right now

@MichaelAquilina
Copy link
Contributor Author

IMHO, error handling is a pretty big deal considering the state of Code Fix AI services. In that improvements document, it's not even priority#1. From a UX point of view, I disagree with that.

But also, this is a tech debt, so I'm going to leave it for @bastiandoetsch to decide and unblock it from my side.

@bastiandoetsch might have to confirm this for me, but from what I can tell - these errors are actually originating from the Snyk language server not visual studio code. So fixing them would involve making separate changes in the language server (which is out of scope of this PR at the very least)

constructor() {}
async deserializeWebviewPanel(webviewPanel: vscode.WebviewPanel): Promise<void> {
// we want to make sure the panel is closed on startup
webviewPanel.dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bastiandoetsch I tried making this dispose on extension deactivation but it did not seem to work.

This solution I came up with here feels a bit hacky, but it does work as expected.

Please let me know if you can think of a better way to make sure the panel is closed if the user restarts their editor 🙏🏼

@MichaelAquilina MichaelAquilina force-pushed the feat/display-ai-fix branch 3 times, most recently from a62afe6 to 16d2e89 Compare April 11, 2024 14:49
@MichaelAquilina
Copy link
Contributor Author

Hi @bastiandoetsch. Is there anything I can help with moving this along? I guess next stage is get the PR approved and prepare it for release?

@bastiandoetsch
Copy link
Contributor

Didn't manage to review it, yet. It's high on my todo list, though!

Copy link
Contributor

@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.

Error handling still seems to need some improvement - I would expect the executeCommand failed message to not be displayed, if the patch retrieval fails (lower right corner). WDYT?

image

@MichaelAquilina
Copy link
Contributor Author

Error handling still seems to need some improvement - I would expect the executeCommand failed message to not be displayed, if the patch retrieval fails (lower right corner). WDYT?

so I thought this was something that would need to change on the language server, but thinking about it, we show the Oh snap message so I must have concluded this wrongly 🤔

@bastiandoetsch
Copy link
Contributor

Error handling still seems to need some improvement - I would expect the executeCommand failed message to not be displayed, if the patch retrieval fails (lower right corner). WDYT?

so I thought this was something that would need to change on the language server, but thinking about it, we show the Oh snap message so I must have concluded this wrongly 🤔

No you haven't, the HTTP 500 triggers the language server command handler to popup the dialog. The Oh Snap! message uses a different call/methodology. We can filter it though - I'll take care of that in the getAutofixDiffs command. If we had better HTTP codes on the backend, it would be easier, as we don't have any chance to report real errors, if we filter on the language server side.

@MichaelAquilina MichaelAquilina force-pushed the feat/display-ai-fix branch 2 times, most recently from 3fc3f80 to 3c2085c Compare April 16, 2024 13:36
CHANGELOG.md Outdated Show resolved Hide resolved
@MichaelAquilina MichaelAquilina force-pushed the feat/display-ai-fix branch 6 times, most recently from a95fc69 to 7f4af0d Compare April 18, 2024 09:15
@MichaelAquilina MichaelAquilina merged commit b866d3c into main Apr 19, 2024
4 checks passed
@MichaelAquilina MichaelAquilina deleted the feat/display-ai-fix branch April 19, 2024 10:00
@Arvi3d
Copy link
Contributor

Arvi3d commented Apr 19, 2024

👏 👏 👏

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.

5 participants