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

✨ Add server status component #127

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Dec 4, 2024

  • Disable analysis and getSolution if server is not running
  • Move server status and prevent blocking analysis sidebar when server is starting
  • Fix webview ready message to follow "type" convention
  • Change startAnalysis to runAnalysis across all files
Screen.Recording.2024-12-03.at.8.08.05.PM.mov

@ibolton336 ibolton336 requested a review from a team as a code owner December 4, 2024 01:18
@ibolton336 ibolton336 force-pushed the analyzer-start-server-2 branch from cc34c3f to be8724f Compare December 4, 2024 01:21
Copy link
Contributor

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Looks good !

vscode/src/client/analyzerClient.ts Outdated Show resolved Hide resolved
vscode/src/KonveyorGUIWebviewViewProvider.ts Outdated Show resolved Hide resolved
vscode/src/client/analyzerClient.ts Show resolved Hide resolved
@ibolton336 ibolton336 force-pushed the analyzer-start-server-2 branch from 3f11638 to 368de80 Compare December 4, 2024 16:55
@ibolton336 ibolton336 force-pushed the analyzer-start-server-2 branch from 368de80 to 7787daa Compare December 4, 2024 17:03
@rszwajko
Copy link
Contributor

rszwajko commented Dec 4, 2024

@ibolton336
I wonder if we really need to surface the server state - we could keep it internal and start it automatically if user requests analysis to be run.
Looking at the wireframes we might even drop the button (I understand it would then be run automatically )

@sjd78
Copy link
Member

sjd78 commented Dec 4, 2024

I like having explicit status for the server available. Back on the wireframes, I thought of it more as a status bar item with wireframe 10 (described but not drawn -- it is the blue bar in the status area in the wireframe image below).

Wireframe 14 has the progression from not configured forward...
image

In terms of UX, the server should manage itself like any other language server would. If things are configured when the extension initializes, just start the server up right away. The user changes the configs? Restart the server. The extensions gets reloaded? Stop and start the server again.

For our purposes, manually starting and stopping the server can make a lot of sense as the start and initalize code can be changing. Keeping those actions around and having a full set of server actions start, restart, and stop that can be hit from the command pallet is very vscode UX consistent. Showing server status in the status bar is also far more vscode UX consistent.

@djzager
Copy link
Member

djzager commented Dec 4, 2024

In terms of UX, the server should manage itself like any other language server would. If things are configured when the extension initializes, just start the server up right away. The user changes the configs? Restart the server. The extensions gets reloaded? Stop and start the server again.

I agree with this being the longterm goal for the extension.

@ibolton336 ibolton336 requested a review from djzager December 4, 2024 20:08
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.

ACK. I think we can iterate on this server status as we getting working on more systems more reliably in the coming weeks.

@ibolton336 ibolton336 merged commit 5ed6df1 into konveyor:main Dec 4, 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.

4 participants