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

[local editor] Don't poll server status when in read only view #1325

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

Ankush-lastmile
Copy link
Member

@Ankush-lastmile Ankush-lastmile commented Feb 23, 2024

[local editor] Don't poll server status when in read only view

In order to prevent showing status error message when in read only view, this diff makes sure that the aiconfig editor client does not poll the server status when in read only mode.

See #1320 for more context

Testplan

  1. Manually set readonly prop to True
  2. Load local editor (Loaded in prod mode, I first built the editor with yarn build)
  3. kill the python process that is hosting the aiconfig server.
Before After
after before

Note: the killpython command in my testplan is a custom alias I have defined that tries to kill all pid's that have the descriptor "python" in its name.

@Ankush-lastmile Ankush-lastmile marked this pull request as ready for review February 23, 2024 18:56
@Ankush-lastmile Ankush-lastmile changed the title [local editor] Don't poll server status when in read only view [local editor] Don't poll AIConfig server status when in read only view Feb 23, 2024
rholinshead pushed a commit that referenced this pull request Feb 23, 2024
# [2/n] Support Restarting Editor Server

Add an "AIConfig: Restart Active Editor Server" command so that users can restart the server for the active (in-view) editor. As part of this, implement the restart functionality for the EditorServer so that the server process is killed and a new one is started, ensuring the associated extension/webview is properly updated to associate with the new server process. As best effort, also implement an `onDidChangeState` so that we can listen to the editor server status changing and show a progress notification that the server is starting.

This logic will be leveraged in subsequent PR to handle restarting all editor servers when the workspace python interpreter changes.


https://github.com/lastmile-ai/aiconfig/assets/5060851/39739008-abc7-4538-9087-42972c1400aa


Note: The server heartbeat banner shows when restarting because of the existing server status request failing. This should be fixed by #1325

## Testing:
- Restart server and ensure (via logging) that the old one is killed and new one is set up, ensuring prompts run in new one
- Close webview and ensure the server is killed
- Activate restart server command and toggle away then back to webview, ensure it's in readonly until it starts up and becomes editable again when toggle back
In order to prevent showing status error message when in read only view, this diff makes sure that the aiconfig editor client does not poll the server status when in read only mode.

See #1320 for more context

## Testplan
1. Manually set readonly prop to True
2. Load local editor
3. kill the python process that is hosting the aiconfig server.

| Before  | After |
| ------------- | ------------- |
| <img width="1913" alt="after" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/ce3487ba-0174-4ca7-b58e-7e10aed6409d">  | <img width="1909" alt="before" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/cba7b817-d09e-435f-b2e8-da7c13109459"> |

Note: the `killpython` command in my testplan is a custom alias I have defined that tries to kill all pid's that have the descriptor "python" in its name.
@Ankush-lastmile Ankush-lastmile changed the title [local editor] Don't poll AIConfig server status when in read only view [local editor] Don't poll server status when in read only view Feb 25, 2024
@Ankush-lastmile
Copy link
Member Author

Ankush-lastmile commented Feb 25, 2024

  • Moved readOnly check to the beginning of the useEffect() call
  • followed steps in testplan -> works as intended. No changes in test plan

note: the aiconfig-editor package needs to be republished in order to validate if #1320 is needed

@rossdanlm
Copy link
Contributor

UI change for VS Code: Let's have follow up to say "if we're in read-only now, tell them why they can't do changes" -->

  1. because we're currently initializing (VS Code server start up)
  2. we got an error and need to display it now

Copy link
Contributor

@rossdanlm rossdanlm left a comment

Choose a reason for hiding this comment

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

Stamping to unblock, we can follow up more later

@Ankush-lastmile Ankush-lastmile merged commit 0318614 into main Feb 25, 2024
2 checks passed
rholinshead added a commit that referenced this pull request Feb 25, 2024
# Update aiconfig-editor to 0.2.2

Includes the changes from #1325 for not polling server status in
readonly
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