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

Connect My Computer: Agent compatibility fixes #32477

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Sep 25, 2023

As Rafał noticed, the proxy version is not immediately available, and during that period we erroneously show an error about incompatible agent.
This PR adds changes isAgentCompatible boolean to agentCompatibility union with three values: unknown, compatible and incompatible. The error should be shown only if the value is incompatible.

Additionally, I changed the red dot in the status indicator to a warning icon to make readable for users with red-green confusion.
image

Copy link
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Works well. Also, I really like the warning symbol instead of red dot

@ibeckermayer
Copy link
Contributor

Nit: Have we tried an opaque version of these where the internal background of the item is opaque white? (The corner poking through is grating on my soul.)
image

@gzdunek
Copy link
Contributor Author

gzdunek commented Sep 26, 2023

@ibeckermayer I agree that it is not beautiful, but AFAIK we can't make the internal icon background opaque without modifying the svg (and that would break cases where we don't need the background).


if (!isAgentConfigured) {
if (!isAgentConfigured || isAgentCompatibilityUnkown) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason behind the check for agent compatibility here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, we don't need it.

BTW, I realized that this condition !isAgentConfigured causes the indicator to be not shown during the setup.
I assume this is expected? I'm asking because I remember that I used your patch for this.

Copy link
Member

Choose a reason for hiding this comment

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

I made a draft PR with a reply to this comment. #32638

I also added one improvement there to flip autoStart to false if the auto start fails.

Let me know what you think and I'll merge it to this branch.

@gzdunek gzdunek requested a review from ravicious September 27, 2023 08:11
# Conflicts:
#	web/packages/teleterm/src/ui/Documents/DocumentsRenderer.tsx
@gzdunek gzdunek enabled auto-merge September 27, 2023 12:56
@gzdunek gzdunek added this pull request to the merge queue Sep 27, 2023
Merged via the queue into master with commit cc8fb14 Sep 27, 2023
@gzdunek gzdunek deleted the gzdunek/agent-compatibility-fixes branch September 27, 2023 13:54
@public-teleport-github-review-bot

@gzdunek See the table below for backport results.

Branch Result
branch/v14 Failed

gzdunek added a commit that referenced this pull request Sep 27, 2023
* Allow agent compatibility state to be 'unknown'

* Show warning icon instead of read dot as the status indicator

* Add a comment

* Remove unnecessary check for agent compatibility

* Avoid creating portal if topBarContainerRef is not present

* Check compatibility when autostarting, not before autostart

* Flip autoStart to false on failed autostart

* Don't show Alert from CompatibilityError if current action has failed

* Run prettier

---------

Co-authored-by: Rafał Cieślak <[email protected]>
(cherry picked from commit cc8fb14)
github-merge-queue bot pushed a commit that referenced this pull request Sep 28, 2023
* Allow agent compatibility state to be 'unknown'

* Show warning icon instead of read dot as the status indicator

* Add a comment

* Remove unnecessary check for agent compatibility

* Avoid creating portal if topBarContainerRef is not present

* Check compatibility when autostarting, not before autostart

* Flip autoStart to false on failed autostart

* Don't show Alert from CompatibilityError if current action has failed

* Run prettier

---------

Co-authored-by: Rafał Cieślak <[email protected]>
(cherry picked from commit cc8fb14)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants