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

✨ Create in-editor diff view #79

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Conversation

rszwajko
Copy link
Contributor

@rszwajko rszwajko commented Oct 22, 2024

Use built-in vscode.diff editor to compare the current code(pointed via
file URI) with proposed changes(pointed via custom konveyorMemFs Uri).

Key points:

  1. proposed changes (diffs) are applied to current code base and
    are written to in-memory file system. Late they can be accessed
    via custom URI scheme konveyorMemFs.
  2. both left and right side of the diff is editable
  3. available commands grouped using konveyor (global)
    or konveyor.diffView prefix(diff view specific):
    a) applyAll - apply all fixes
    b) revertAll - discard local changes to the proposed fixes
    c) applyFile/revertFile - as above but on a single file
    d) copyDiff/copyPath - convenience commands (single file scope)
    e) next/prev - keyboard navigation (for now) using the same key
    bindings as reference-view extension (f4 and shift+f4)
    f) viewFix - wrapper around vscode.diff that uses konveyorMemFs

Sources:

  1. tree view is based on built-in reference-view extension. Only
    file-item nodes are used. Support for history items and dynamic model
    changes have been dropped. However the original architecture is there
    and should allow adding more advance features in the future.
  2. in-memory file system is taken as-it-is from fsprovider-sample
  3. the approach (using hidden file tree for fixes) originates from
    Continue's DiffManager
  4. check/discard icons taken from source-control-sample

Reference-Url: https://github.com/continuedev/continue/blob/e7fe5994ffc6a3f45ad358ced7d6890deb145d96/extensions/vscode/src/diff/horizontal.ts#L34
Reference-Url: https://github.com/microsoft/vscode/tree/main/extensions/references-view
Reference-Url: https://github.com/microsoft/vscode-extension-samples/tree/main/fsprovider-sample
Reference-Url: https://github.com/microsoft/vscode-extension-samples/tree/main/source-control-sample

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

This is really cool to see what is possible. Thank you for getting this up so quickly.

I'm still trying to digest all that is going on here. My initial thought was that we would develop the mock response from the kai/rpc-server::getCodeplanAgentSolution and then look at how those change(s) get rendered, like diff viewer.

Things I don't yet understand:

  • Do we need the tree view to navigate these things?
  • How the result of our analyzerClient's getSolution for a specific incident is going to integrate with what is demonstrated here.

Things I'm uneasy about:

  • Would prefer all of the commands to be registered in commands.ts. I really don't want them spread about the repository.
  • The number of new commands being added. I would have thought we could have something like konveyor.acceptFix and konveyor.rejectFix and that is it.
  • We are generating all of these file changes at startup...shouldn't this be tied to some user action? This is where I had thought the mocks would come into play...You call "Generate Fix" and you get changes that you can optional accept or reject.

@rszwajko
Copy link
Contributor Author

rszwajko commented Oct 23, 2024

  • Do we need the tree view to navigate these things?

No. Actually we use the tree as a list. However it's easier (and future-proof) to use tree view. The extensions I've followed (reference-view and SCM) use this approach. In SCM (i.e. git) you can switch between tree and list but technically it's the same tree.

  • How the result of our analyzerClient's getSolution for a specific incident is going to integrate with what is demonstrated here.

The solutions need to be written to in-memory file system. Right now I'm using static mock data from vscode/src/diffView/suggestions.

This approach (EDIT: hidden file tree) originates from Continue's DiffManager but instead of real file system we are using in-memory one borrowed from fsprovider-sample

  • Would prefer all of the commands to be registered in commands.ts. I really don't want them spread about the repository.

All commands are already listed in package.json. We could duplicate that in commands.ts but this will require imports from all modules.

  • The number of new commands being added. I would have thought we could have something like konveyor.acceptFix and konveyor.rejectFix and that is it.

Most of the commands are related to navigation. Copy* were inherited from reference-view - the seem to be commonly used in VS Code in file views.

  • We are generating all of these file changes at startup...shouldn't this be tied to some user action? This is where I had thought the mocks would come into play...You call "Generate Fix" and you get changes that you can optional accept or reject.

Yep. Right now we are using static mock data. As soon as we will have better mocks we can switch.

@rszwajko
Copy link
Contributor Author

image

@rszwajko rszwajko changed the title ✨ Create diff view ✨ Create in-editor diff view Oct 23, 2024
@rszwajko rszwajko marked this pull request as ready for review October 23, 2024 19:24
@rszwajko
Copy link
Contributor Author

@djzager
the current solution is not fully functional as it uses mock data however this will be improved in follow-up issues. First one is #68 which should provide us better mocks.
If you prefer to put some fixes already in this PR then please comment.

@rszwajko rszwajko requested a review from djzager October 23, 2024 19:30
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

I am a hard no on this PR as it is currently constructed. To reiterate what I have said on Slack, this is what I think the main user flow through our extension should look in mid-November:

  1. User installs our extension and is presented with a walkthrough where they can configure the analyzer, configure kai, sources+targets, etc.
  2. User starts the server and we initiate the client/server communication with either the analyzer or kai
  3. User calls konveyor.runAnalysis where we run an analysis based on configuration, push results to the Problems (as vscode.Diagnostics) and save those RuleSets to our extensions active workspace state.
  4. User can view and interact with the results of an analysis in either our webview OR the problems panel. All that cool filter/sort/search, whatever that lets you navigate the results.
  5. User selects an individual incident of a particular violation and they call konveyor.getSolution, and we make a call to kai (assuming genAi is enabled) to get a solution. The result we get back will be a bunch of changes that kai wants to make to the repository for that solution.
  6. With a specific "solution" returned, the user can navigate (maybe breadcrumbs? and state management so we can come back) the changes to be made. This is where I feel the biggest disconnect. We should primarily be driving this user experience through our singular webview (this is what we can take with us when we go to implement this thing in another editor like jetbrains or eclipse). So we make it possible for the user to "see" the changes to be made in our singular webview and they can accept OR reject them. Additionally, if you click on the code block we will open that specific file in the editor for you and show you that diff inline where the user can accept OR reject a specific change.

This PR deals with a very specific interaction inside of No.6 above, I want a function to be callable from the webview that shows a specific fix in its context (If you want to do accept/reject fix from #72 ... go for it), but the main feature addition in this pull request should be viewing a specific fix in the editor in context.

vscode/src/extension.ts Outdated Show resolved Hide resolved
vscode/src/diffView/fileModel.ts Show resolved Hide resolved
vscode/package.json Show resolved Hide resolved
Use built-in vscode.diff editor to compare the current code(pointed via
file URI) with proposed changes(pointed via custom konveyorMemFs Uri).

Key points:
1. proposed changes are written to in-memory file system and accessed
   via custom URI scheme konveyorMemFs
2. both left and right side of the diff is editable
3. available commands grouped using konveyor.diffView prefix:
   a) applyAll - apply all fixes
   b) revertAll - discard local changes to the proposed fixes
   c) applyFile/revertFile - as above but on a single file
   d) copyDiff/copyPath - convenience commands (single file scope)
   d) next/prev - keyboard navigation (for now) using the same key
      bindings as reference-view extension (f4 and shift+f4)
4. this version uses static mock data from src/diffView/resolutions

Sources:
1. tree view is based on built-in reference-view extension. Only
   file-item nodes are used. Support for history items and dynamic model
   changes have been dropped. However the original architecture is there
   and should allow adding more advance features in the future.
2. in-memory file system is taken as-it-is from fsprovider-sample
3. the approach (using hidden file tree for fixes) originates from
   Continue's DiffManager
4. check/discard icons taken from source-control-sample

Reference-Url: https://github.com/continuedev/continue/blob/e7fe5994ffc6a3f45ad358ced7d6890deb145d96/extensions/vscode/src/diff/horizontal.ts#L34
Reference-Url: https://github.com/microsoft/vscode/tree/main/extensions/references-view
Reference-Url: https://github.com/microsoft/vscode-extension-samples/tree/main/fsprovider-sample
Reference-Url: https://github.com/microsoft/vscode-extension-samples/tree/main/source-control-sample
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Refactor the diff view code to integrate with existing extension
structure.

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
@rszwajko rszwajko marked this pull request as ready for review November 7, 2024 12:09
@rszwajko rszwajko requested a review from djzager November 7, 2024 12:13
@rszwajko
Copy link
Contributor Author

rszwajko commented Nov 7, 2024

Screencast.from.2024-11-07.13-19-57.mp4

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM.

@ibolton336 ibolton336 merged commit d2faf02 into konveyor:main Nov 11, 2024
11 checks passed
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