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: ai fix html [IDE-375] #527

Merged
merged 3 commits into from
Jun 10, 2024
Merged

feat: ai fix html [IDE-375] #527

merged 3 commits into from
Jun 10, 2024

Conversation

teodora-sandu
Copy link
Contributor

@teodora-sandu teodora-sandu commented May 31, 2024

Description

Moves over the common HTML and styling from VSCode so that eventually it can be used to render this panel in IntelliJ. In snyk/vscode-extension#467 we can see the changes needed to add extra styling and javascript for VSCode and in snyk/snyk-intellij-plugin#541 we completely hide this panel for IntelliJ.

Checklist

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

🚨After having merged, please update the CLI go.mod to pull in latest language server.

Screenshots / GIFs

See screenshots in linked PRs.

@teodora-sandu teodora-sandu changed the title feat: ai fix html feat: ai fix html [IDE-375] May 31, 2024
@@ -600,8 +947,9 @@ <h2 class="example-fixes-header">Fixed Code Examples</h2>
};

// Event listeners for main and nested tabs
// TODO: move this into the HTML as an onclick
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can simplify a lot of this code by just adding an onclick attribute to these elements and in IntelliJ and VSCode we only implement the functions that these elements use. The changes are already complicated enough in this PR and its other two in our IDEs so I'd like to do that in a follow-up if there's time.

Copy link
Contributor

Choose a reason for hiding this comment

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

The mark-up is 😍 , thank you!
It seems that there are some issues with the listeners injected from the IDE? You can see that error when switching between issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did you manage to test this view? The consistent ignores backend is broken at the moment (it was broken on Friday and I explained in the PR description and it seems still broken today), so I haven't been able to test this part of the UI (where we have an ignored issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

In dev, everything is working fine 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, did we say that is the issue is ignored, we don't show AI Fixes? I'm not sure now and I can't find references to this case.

Copy link
Contributor Author

@teodora-sandu teodora-sandu Jun 10, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks it does work in dev indeed! I've pushed a fix in snyk/vscode-extension@b21e406

Copy link
Contributor

@cat2608 cat2608 left a comment

Choose a reason for hiding this comment

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

All rendering looks awesome! Just the JS seems to not work sometimes. I'm approving it so we can continue it in later iterations since you said you'd like to inline the onclicks :)

@teodora-sandu
Copy link
Contributor Author

Just the JS seems to not work sometimes.

Which JS is that @cat2608 ? I probably will raise a ticket to add the onclicks because I think the manual testing of the feature is more important than code improvements at this moment, so if there are parts of the JS that don't work maybe I can work on those instead

@cat2608
Copy link
Contributor

cat2608 commented Jun 10, 2024

@teodora-sandu this was my setup when testing your change and when I left my comment here

  • LS: feat/ai-fix-html at aec12f5
  • vscode-plugin: feat/ai-fix-html at eb5c2aee5a60eea04288c0721d0909a1c5357688
event-listeners-error.mp4

Now I can see that you have added b21e4068514cf60caa82c48230e71b0c7c87c734 in vscode-plugin which reads fix: add event listener conditionally. I tested it again and the issue about the event listeners has gone! 🙌

The only thing remaining is the Fix Action itself:

action-missing.mp4

@teodora-sandu
Copy link
Contributor Author

Ah the error you're seeing is a backend error, the call to get the fix diffs seems to return null. The AI fixes were not working on Friday and they seem to still be slightly broken today. I will cover that in https://snyksec.atlassian.net/browse/IDE-380. If you apply the patch I provide in the PR description it does work correctly, because it's using the old backend

@teodora-sandu teodora-sandu merged commit cd4af4f into main Jun 10, 2024
15 checks passed
@teodora-sandu teodora-sandu deleted the feat/ai-fix-html branch June 10, 2024 13:25
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.

2 participants