Implement temporary last step of Connect My Computer flow in Discover#34452
Implement temporary last step of Connect My Computer flow in Discover#34452
Conversation
ravicious
added
backport/branch/v14
merge-for-v14
no-changelog
| export const TestConnection = (props: AgentStepProps) => { | ||
| const { userService } = useTeleport(); | ||
| const abortController = useRef<AbortController>(); | ||
| const [reloadUserAttempt, reloadUser] = useAsync( |
There was a problem hiding this comment.
I used useAsync instead of Suspense because when I wanted to understand how Suspense works in the existing components that we have, I realized that Suspense is currently undocumented in the official docs and depends on using some kind of an abstraction over it. We seem to be using Suspense through that undocumented API and I couldn't find any abstraction on top of that in our project. As I was tight on time, I opted to use what I know to achieve the same result versus potentially running into issues which would be hard to debug and understand without documentation.
Let me know if I should rewrite it to use Suspense. I can do it once I get around to the connection test. But tbh, I don't see the value of using Suspense without any abstractions – it's significantly more code which, to my eye, also seems more complex and I don't understand what benefits we get in return.
There was a problem hiding this comment.
i am curious where you got that you had to use suspense? the only suspense we use is with useJoinTokenSuspender as far as i know (it's for join scripts). tbh, it was pretty painful to use suspense because it was a completely different way of thinking/structuring your code
There was a problem hiding this comment.
Yep, that was the hook I saw. I saw it first in DownloadScript so I assumed that this is the way to do things if we want something to happen before a step gets rendered.
I think using Suspense there would be easier if:
- It was hidden behind some kind of abstraction so that the hook itself doesn't have to implement all this fancy stuff with throwing promises etc.
- The hook usage was scoped as tightly as possible to the component that's actually going to use this data so that we don't have to go out of our way with the
Templatecomponent.
But those are just my observations based on the existing code, I've never actually used Suspense so I don't know what the best practices are really.
|
@kimlisa @rudream I'd appreciate if you could find time to review this today. It's scheduled to release in 14.2 on Monday, so next week I'd like to address any feedback and merge it. I believe it's good enough to be released, unless there's anything critical I think any major requests could be addressed after the release – I'm going to continue working on this guided flow anyway. |
| }); | ||
|
|
||
| const [showHint, setShowHint] = useState(false); | ||
| const { agentMeta, updateAgentMeta, nextStep } = props; |
There was a problem hiding this comment.
non-blocker nit: discover work started by prop drilling (me), then ryan introduced context. so ideally, we should be getting these props by useDiscover going forward. i was going to at some point get rid of AgentStepProps
| export const TestConnection = (props: AgentStepProps) => { | ||
| const { userService } = useTeleport(); | ||
| const abortController = useRef<AbortController>(); | ||
| const [reloadUserAttempt, reloadUser] = useAsync( |
There was a problem hiding this comment.
i am curious where you got that you had to use suspense? the only suspense we use is with useJoinTokenSuspender as far as i know (it's for join scripts). tbh, it was pretty painful to use suspense because it was a completely different way of thinking/structuring your code
|
Thanks for testing it Lisa! This feature depends on so many things (the Web UI, Connect, custom protocol URL handling by the OS and the browser, our CDN, configuring and running an agent locally) and it always feel very brittle to me, so I'm glad you didn't run into any problems.
Good idea, I didn't catch that the flow for a server already does that. |
This makes sure that the list of logins in the node is up to date.
ravicious
force-pushed
the
ravicious/refresh-cert
branch
from
7e56164 to
a1e678e
Compare
|
@ravicious See the table below for backport results.
|
|
@ravicious See the table below for backport results.
|
3 participants
Closes #32192.
Figma
This PR implements the last step of the Connect My Computer flom in Discover. It's a temporary version of it – after next week I'm going to work on adding an actual connection test.
Shortcuts taken in the last step
Connect My Computer creates a special role for each user (
connect-my-computer-<username>) which contains a valid login for the device where Connect My Computer was set up on. If the user set up Connect My Computer on multiple devices which use different system usernames, this role is going to contain multiple valid logins.Originally the idea was to read the logins from that role and show only them in MenuLogin – or not even show MenuLogin at all if there's only a single valid role.
However, I assumed that there exists a way to read the details of a role in the Web UI. There's no such API today, the only way to achieve that is to read the YAML from the role which we don't want to do. idk why I assumed that, Connect doesn't have such an API on the frontend as well!
Once I get around to adding an actual connection test, I'm going to add an endpoint which reads the logins from the Connect My Computer role. In the meantime, I'm just going to show all usable logins.
Reloading user in polling
This PR also addresses an edge case I described in #34401. If polling in the previous step reaches a "timeout" where we show the hint, we should reload the user session. If they finished the setup in Connect, the new role in their role list might allow them to see the newly added node which they would not be able to see otherwise.
For the deep link to work, you need to compile master Connect locally:
But you can also just download the current v14 of Connect, open the app manually and then open Connect My Computer through the top left icon.