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

Fixup: Improve error handling #1376

Merged
merged 7 commits into from
Oct 30, 2023
Merged

Fixup: Improve error handling #1376

merged 7 commits into from
Oct 30, 2023

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Oct 12, 2023

Description

Improves how we handle errors for fixups:

  • Errors from the LLM are no longer considered valid code that can be applied, so we don't get weird error message kinda code being added.
  • Instead of discarding the fixup immediately on error, we show the error code lens.
  • When the error code lens is clicked, it now shows an error modal in the editor, rather than just focusing the chat which will be empty.

Before

FixupErrorBefore.mov

After

FixupErrorAfter.mov

Test plan

Tested locally

  • Error from LLM
  • Forced error from controller (respins)
  • Checked happy paths work as usual

@umpox
Copy link
Contributor Author

umpox commented Oct 12, 2023

@toolmantim Would appreciate your 👀 on this UX flow.

Right now we treat most errors from Cody as valid fixups, which means we can apply some invalid code sometimes.

This changes it so we have an actual error state.

This thing I wasn't sure about is: Is this visible enough? We could also show a notification immediately after the error happens, so users don't just miss the error.

@umpox umpox marked this pull request as ready for review October 12, 2023 10:58
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Love the pop up on the code lense click!

I found an error that is not being handled:

  1. Right now recipe level error is not being handled and the task will continue to spin indefinitely:
image
  1. Fixup with no updates from Cody is not returning error?
image

We should probably update the action items for fixup task with errors in the tree view as well?

image

@toolmantim might have some updates for this but here is the old figma design for fixup for reference: https://www.figma.com/file/9112BsKsJc1BpO2j8XYwLL/%F0%9F%A4%96-Cody-VS-Code-%5BMain%5D?type=design&node-id=2799-13359&mode=design&t=8KXSY8PA3YuFzIoj-0

@umpox
Copy link
Contributor Author

umpox commented Oct 23, 2023

@abeatrix Thanks for testing, sorry didn't get round to this - fixing now!

Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

Confirmed:

  • /doc and /edit works as before.
  • show error properly when selected code excesses limit
    image
  • fixup without suggestion retruns error as well

@umpox umpox merged commit cea2f99 into main Oct 30, 2023
14 checks passed
@umpox umpox deleted the tr/fixup-error-message branch October 30, 2023 11:26
@toolmantim
Copy link
Contributor

Nice!

@toolmantim
Copy link
Contributor

@umpox I can't think of a better option than putting the error message in the confirm like you've done. The only other option I could think of were a native VS Code notification, but that should trigger as soon as it happens, and you probably still want something on click. I think confirm is fine for now, and we can improve as we go.

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.

None yet

3 participants