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.

Sign up for GitHub

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

Jump to bottom

fix: copilot chat extension does not work #335

Closed
wants to merge 9 commits into from
Closed

fix: copilot chat extension does not work #335

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
99cf176
fix: avoid using `silent` when getting a session
vitaliy-guliy Mar 18, 2024
fccb597
fix: extension should get a session
vitaliy-guliy Mar 18, 2024
e5571b9
fix: extension should get a session
vitaliy-guliy Mar 18, 2024
c594221
fix: extension should get a session
vitaliy-guliy Mar 18, 2024
cfd91f9
fix: add rebase rule
vitaliy-guliy Mar 18, 2024
4884edd
Merge branch 'main' into copilot-chat
vitaliy-guliy Mar 18, 2024
c8ff33d
fix: add changes to CHANGELOG.md
vitaliy-guliy Mar 18, 2024
741b96a
Merge branch 'main' into copilot-chat
vitaliy-guliy Mar 20, 2024
12304a0
Merge branch 'main' into copilot-chat
vitaliy-guliy Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion .rebase/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -2,6 +2,12 @@

The file to keep a list of changed files which will potentionaly help to resolve rebase conflicts.

#### @vitaliy-guliy
https://github.com/che-incubator/che-code/pull/335

- code/src/vs/workbench/api/browser/mainThreadAuthentication.ts
---

#### @vitaliy-guliy
https://github.com/che-incubator/che-code/pull/331

Expand All @@ -28,7 +34,6 @@ https://github.com/che-incubator/che-code/pull/337/commits/875893566c2acd0bb7031
- code/src/vs/base/common/network.ts
---


#### @benoitf
https://github.com/che-incubator/che-code/commit/eed0a5213ba1b29b810d53f6365aaa2294165845#diff-2735bf66f14ee64b9ce6fdc30355a5e3085ae96a791cd01d65843a8dcef7c166

Expand Down
6 changes: 6 additions & 0 deletions .rebase/replace/code/src/vs/workbench/api/browser/mainThreadAuthentication.ts.json
View file
Open in desktop
@@ -0,0 +1,6 @@
[
{
"from": "// We may need to prompt because we don't have a valid session",
"by": "// We may need to prompt because we don't have a valid session\\\n\\\t\\\tif (options.silent) {\\\n\\\t\\\t\\\toptions.silent = false;\\\n\\\t\\\t\\\toptions.createIfNone = true;\\\n\\\t\\\t}\\\n"
}
]
5 changes: 5 additions & 0 deletions code/src/vs/workbench/api/browser/mainThreadAuthentication.ts
View file
Open in desktop
Expand Up @@ -152,6 +152,11 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
}

// We may need to prompt because we don't have a valid session
if (options.silent) {
options.silent = false;
options.createIfNone = true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@vitaliy-guliy
I tested this solution - it works for me - I'm able to use Github Copilot Chat extension,
thank you!

At the same time it's still unclear how it works on VS Code side without these changes.
Also I detected some side-effects of the current solution:

  • let's say I don't have Copilot and Copilot extensions
  • It asks me to sign in using GitHub directly after starting a workspace
  • without these changes the system asks to sign in using GitHub only when I'm using some command that requires it (git clone for example)

Another use case is - Sign Out:

  • with these changes the system asks to Sign In directly after Sign Out for all extensions, like: I do Sign Out => 3 dialogs are shown to Sign in for 3 extensions

Without these changes:
https://github.com/che-incubator/che-code/assets/5676062/87a7f967-6621-4e26-b52d-04fb0e1e4cb3

With these changes:
https://github.com/che-incubator/che-code/assets/5676062/702d9f6b-29f4-4cf4-9c57-e069f5fe3a13

Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK for me to merge the PR as is if we don't have another solution that allows to avoid those side-effects - they are insignificant from my point of view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, user is asked to grant the access when the extension wants to get a session.
But is seems user is asked only once at the workspace startup.
It should not be very annoying in case you want to use extension like copilot.
From the other side, clicking the button each time when creating a workspace could make a user a bit nervous.

I created another PR to test a bit different behavior #339

Copy link
Contributor

Choose a reason for hiding this comment

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

But is seems user is asked only once at the workspace startup.

I also described another use case above:
I want to Sign Out => 3 dialogs are shown to Sign in for 3 extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure users are practicing signing out in a real life.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yes, it's a case

// modal flows
if (options.createIfNone || options.forceNewSession) {
const providerName = this.authenticationService.getLabel(providerId);
Expand Down