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 account switcher component in the Accounts webview tab #6159

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

pkukielka
Copy link
Contributor

@pkukielka pkukielka commented Nov 20, 2024

Changes

That PR:

  1. Fixes webview crashes when switching accounts with Account tab open
  2. Add Switch Account button on the Accounts tab when accountSwitchingInWebview capability is enabled in client.

Test plan

Testing with jetbrains main:

Fixes https://linear.app/sourcegraph/issue/QA-150

  1. Open Accounts tab
  2. Click Cody icon, then Manage Accounts
  3. Switch the account
  4. Account tab should re-render properly (after a 1-2s delay) showing new account details

Testing with jetbrains migrated to webview account management:

Account switching:

  1. Open Accounts tab
  2. Click Switch Account
  3. Select account you want to switch to from the list
  4. Account tab should re-render properly (after a 1-2s delay) showing new account details

Signing out:

  1. Open Accounts tab
  2. Click Sign Out
  3. You should be moved to the Sign In panel

Account adding:

  1. Open Accounts tab
  2. Click 'Switch Account', and then 'Add another account'
  3. Type url of your endpoint
  4. You should be redirected to the web page to confirm adding the token
  5. Account should be added and displayed as active

Note: If wrong endpoint is provided web redirection will fail, but no error is displayed in the UI.
This definitely can be improved.

Account adding with token:

  1. Open Accounts tab
  2. Click 'Switch Account', and then 'Add another account'
  3. Type url of your endpoint
  4. Click 'Access Token (optional)'
  5. Type incorrect access token and click 'Add and Switch'
  6. You should get error saying Invalid access token.
  7. Type correct access token and click 'Add and Switch'
  8. Account should be added and displayed as active

image
image
image
image

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

Looks like a nice step forward. Some comments inline. TODOs may be appropriate at this point since this will be under heavy change for a while.

const tokenSource = message.value
? 'paste'
: await secretStorage.getTokenSource(serverEndpoint)
const validationResult = await authProvider.validateAndStoreCredentials(
Copy link
Contributor

@dominiccooney dominiccooney Nov 20, 2024

Choose a reason for hiding this comment

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

I know we used to be saving the endpoint here, I assume now we are talking to the network now too... Can we show a spinner and when the result comes back, flip the UX at that point?

Copy link
Contributor Author

@pkukielka pkukielka Nov 22, 2024

Choose a reason for hiding this comment

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

Done.

image

export const AccountSwitcher: React.FC<{ endpoints: string[] }> = ({ endpoints }) => {
const [isOpen, setIsOpen] = useState(false)

const onKeyDownInPopoverContent = useCallback<KeyboardEventHandler<HTMLDivElement>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless this is passed to some memoized thing, it is probably not worth memoizing... particularly since you're not memoizing onOpenChange or the onclick handler...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@danielmarquespt danielmarquespt 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 as a temporary solution until we built the unified design

@mkondratek
Copy link
Contributor

mkondratek commented Nov 20, 2024

When I run it for the first time for the instance I had two accounts logged in (dotcom and enterprise) I saw 3 accounts in the list to switch account

I switched to http one out of curiosity and it presented me the log in screen.
I tried to log in using http://sourcegraph.com - the error happens silently in the logs

After I logged in to dotcom https I see this:

image

@pkukielka
Copy link
Contributor Author

pkukielka commented Nov 20, 2024

Looks like a nice step forward. Some comments inline. TODOs may be appropriate at this point since this will be under heavy change for a while.

Thanks @dominiccooney .
In fact I already have adding and removing accounts flow 85% working so I think I will just push rest of the changes to this PR tomorrow.

@kalanchan
Copy link
Contributor

nice typescript skills!

@pkukielka pkukielka force-pushed the pkukielka/add-account-switcher branch from 9b4bc26 to ecf0cc7 Compare November 25, 2024 12:09
@mkondratek
Copy link
Contributor

lgtm

@pkukielka pkukielka merged commit 2b3163f into main Nov 25, 2024
19 of 20 checks passed
@pkukielka pkukielka deleted the pkukielka/add-account-switcher branch November 25, 2024 16:03
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.

5 participants