-
Notifications
You must be signed in to change notification settings - Fork 403
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
feat: support VSCode API: AuthenticationGetSessionOptions.account #4224
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. Walkthrough此拉取请求对 Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
packages/core-common/src/types/authentication.ts (2)
50-53
: 简化getSessions
方法的参数类型声明在
getSessions
方法中,参数scopes
被声明为scopes?: string[] | undefined
。由于?
已表示参数是可选的,| undefined
是多余的。建议简化类型声明为scopes?: string[]
。建议的修改:
getSessions( - scopes?: string[] | undefined, + scopes?: string[], account?: AuthenticationSessionAccountInformation, ): Promise<ReadonlyArray<AuthenticationSession>>;
84-84
: 参数命名不一致:user
与account
在
IAuthenticationService
接口的getSessions
方法中,参数命名为user
,而在IAuthenticationProvider
接口中,相应的参数命名为account
。建议统一参数命名为account
,以保持一致性,避免混淆。packages/extension/src/browser/vscode/api/main.thread.authentication.ts (2)
99-102
: 简化getSessions
方法的参数传递在
getSessions
方法中,参数scopes
和account
可以直接传递给_proxy.$getSessions
,无需创建额外的对象。建议的修改:
return this._proxy.$getSessions(this.id, scopes, { account });改为:
return this._proxy.$getSessions(this.id, scopes, account);
256-260
: 在$getAccounts
方法中添加错误处理当前
$getAccounts
方法中没有对getSessions
方法可能的错误进行处理。建议添加错误处理,以提高代码的健壮性。建议的修改:
return this.authenticationService .getSessions(providerId) .then((sessions) => sessions.map((session) => session.account)) + .catch((error) => { + this.logger.error(`Failed to get accounts for provider ${providerId}:`, error); + throw error; + });packages/core-browser/src/authentication/authentication.service.ts (1)
440-440
: 捕获异常时的错误消息应更加具体在捕获异常时,错误消息中包含了
err.message
,但未对err
进行类型检查。在某些情况下,err
可能不是Error
类型,直接访问message
可能导致异常。建议进行类型检查或提供默认错误消息。建议的修改:
- throw new Error(`No authentication provider '${providerId}' is currently registered, error messge: ${err.message}`); + const errorMessage = err instanceof Error ? err.message : String(err); + throw new Error(`No authentication provider '${providerId}' is currently registered. Error message: ${errorMessage}`);packages/extension/src/hosted/api/vscode/ext.host.authentication.ts (3)
63-65
: 新增账户管理功能实现getAccounts 方法的实现看起来合理,但建议:
- 添加错误处理机制
- 考虑添加参数验证
建议改进实现:
getAccounts(providerId: string): Thenable<AuthenticationSessionAccountInformation[]> { + if (!providerId) { + throw new Error('providerId 不能为空'); + } return extHostAuthentication.getAccounts(providerId); },
236-243
: 优化会话获取逻辑$getSessions 方法的实现需要注意:
- 需要处理 scopes 和 options 为可选参数的情况
- 建议添加结果缓存机制以提高性能
建议改进实现:
$getSessions( providerId: string, scopes: string[], options: AuthenticationProviderSessionOptions, ): Promise<ReadonlyArray<AuthenticationSession>> { const providerData = this._authenticationProviders.get(providerId); if (providerData) { + const cacheKey = `${providerId}-${scopes?.join(',')}-${JSON.stringify(options)}`; + // TODO: 实现缓存机制 return Promise.resolve(providerData.provider.getSessions(scopes, options)); }
Line range hint
178-190
: 修复重复的提供者注册检查在 registerAuthenticationProvider 方法中发现重复的检查代码:
if (!this._providers.find((p) => p.id === id)) { this._providers.push({ id, label, }); }删除重复的检查代码块。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core-browser/src/authentication/authentication.service.ts
(5 hunks)packages/core-common/src/types/authentication.ts
(2 hunks)packages/extension/src/browser/vscode/api/main.thread.authentication.ts
(8 hunks)packages/extension/src/common/vscode/authentication.ts
(3 hunks)packages/extension/src/hosted/api/vscode/ext.host.authentication.ts
(5 hunks)packages/types/vscode/typings/vscode.authentication.d.ts
(5 hunks)
🔇 Additional comments (8)
packages/core-common/src/types/authentication.ts (2)
35-38
: 接口 AuthenticationSessionAccountInformation
定义合理
添加了用于表示身份验证会话账户信息的接口,包含 id
和 label
属性,设计合理。
40-42
: 接口 AuthenticationProviderSessionOptions
定义合理
新增的接口包含可选的 account
属性,为身份验证提供者提供更灵活的会话选项。
packages/types/vscode/typings/vscode.authentication.d.ts (3)
109-112
: 在 AuthenticationGetSessionOptions
中添加 account
属性
新增的 account
属性允许在获取会话时指定特定的账户信息,提供了更精细的会话控制。
173-183
: 定义 AuthenticationProviderSessionOptions
接口
添加了新的接口,用于在身份验证提供者中传递会话选项,包括可选的 account
属性,设计合理。
201-201
: 🛠️ Refactor suggestion
getSessions
方法中 options
参数可选性不一致
在 AuthenticationProvider
接口中,getSessions
方法的 options
参数是可选的(options?: AuthenticationProviderSessionOptions
),但在 createSession
方法中,options
参数是必需的(options: AuthenticationProviderSessionOptions
)。为保持一致性,建议在 createSession
方法中将 options
参数设置为可选。
建议的修改:
- createSession(scopes: string[], options: AuthenticationProviderSessionOptions): Thenable<AuthenticationSession>;
+ createSession(scopes: string[], options?: AuthenticationProviderSessionOptions): Thenable<AuthenticationSession>;
Likely invalid or redundant comment.
packages/extension/src/browser/vscode/api/main.thread.authentication.ts (1)
244-249
: 确保 $login
方法的参数与接口定义一致
在 $login
方法中,options
参数被添加,但需要确认此更改与接口定义一致,并且在所有相关调用中都正确传递了 options
参数。
请运行以下脚本以检查 $login
方法的实现和调用:
✅ Verification successful
Let me gather more context about the implementation and interface definitions.
让我再获取一些关于 $login
方法的具体实现细节。
$login
方法的参数与接口定义完全一致
经过验证,$login
方法的实现在以下几个关键位置都保持了一致的参数签名:
IMainThreadAuthentication
接口定义IExtHostAuthentication
接口定义MainThreadAuthentication
类的实现ExtHostAuthentication
类的实现
所有实现都使用了相同的三个参数:
providerId: string
scopes: string[]
options: AuthenticationProviderSessionOptions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查 `$login` 方法的参数一致性
# 查找 `$login` 方法的定义
rg -A 5 'function \$login\('
# 查找对 `$login` 的所有调用,确保传递了 `options` 参数
rg '\$login\('
Length of output: 575
Script:
#!/bin/bash
# 检查 $login 方法的完整实现和接口定义
ast-grep --pattern '$login($$$) {
$$$
}'
# 查找相关的接口定义
rg -B 2 -A 5 'interface.*Authentication'
Length of output: 14133
Script:
#!/bin/bash
# 检查 $login 方法的完整实现
rg -B 5 -A 10 '\$login\(' packages/extension/src/browser/vscode/api/main.thread.authentication.ts packages/extension/src/common/vscode/authentication.ts packages/extension/src/hosted/api/vscode/ext.host.authentication.ts
Length of output: 8087
packages/extension/src/common/vscode/authentication.ts (2)
63-67
: 验证接口变更的兼容性
IExtHostAuthentication 接口的变更需要确保:
- $getSessions 方法新增的可选参数不会影响现有实现
- $login 方法的 options 参数在所有实现中都得到正确处理
Also applies to: 69-69
✅ Verification successful
Let me analyze the implementation to verify the interface changes. I can see the implementation of $getSessions
and $login
in the ExtHostAuthentication class, but let me check for any other implementations to ensure completeness.
接口变更已得到正确实现
经过验证,IExtHostAuthentication 接口只有一个实现类 ExtHostAuthentication,且该实现类:
- 完整支持 $getSessions 方法的新增可选参数 options
- $login 方法正确处理了 options 参数
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 检查接口实现
ast-grep --pattern 'class $_ implements IExtHostAuthentication {
$$$
}'
Length of output: 23371
Script:
#!/bin/bash
# Search for other implementations of IExtHostAuthentication
rg "implements IExtHostAuthentication" --type ts
# Search for any other $getSessions implementations
ast-grep --pattern '$getSessions($_, $_, $_) {
$$$
}'
Length of output: 247
3-3
: 验证 AuthenticationProviderSessionOptions 的使用
新增的 AuthenticationProviderSessionOptions 类型在 selectSession 方法中被使用,这是一个重要的接口变更。请确保所有实现类都正确处理了这个新参数。
Also applies to: 28-28
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4224 +/- ##
==========================================
- Coverage 53.99% 53.99% -0.01%
==========================================
Files 1616 1616
Lines 98129 98136 +7
Branches 20080 20080
==========================================
+ Hits 52986 52987 +1
- Misses 37503 37509 +6
Partials 7640 7640
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core-common/src/types/authentication.ts (1)
71-77
: 建议优化参数顺序以提高可用性
requestNewSession
方法的参数顺序可以优化:
- 建议将
options
参数移到最后,作为可选参数- 相关参数应该组合在一起,例如
extensionId
和extensionName
应该相邻建议的修改:
- requestNewSession( - providerId: string, - scopes: string[], - options: AuthenticationProviderSessionOptions, - extensionId: string, - extensionName: string, - ): Promise<void>; + requestNewSession( + providerId: string, + scopes: string[], + extensionId: string, + extensionName: string, + options: AuthenticationProviderSessionOptions, + ): Promise<void>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core-common/src/types/authentication.ts
(2 hunks)packages/extension/src/common/vscode/authentication.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/src/common/vscode/authentication.ts
🔇 Additional comments (3)
packages/core-common/src/types/authentication.ts (3)
35-42
: 接口定义清晰且符合预期!
新增的两个接口设计合理:
AuthenticationSessionAccountInformation
与现有的AuthenticationSession
中的账户结构保持一致AuthenticationProviderSessionOptions
通过可选属性提供了良好的扩展性
50-55
: 方法签名更新需要保持一致性
在 IAuthenticationProvider
接口中,login
方法的 options
参数是必需的,但在 IAuthenticationService
接口中是可选的。建议统一这个参数的可选性。
83-92
: 方法签名更新符合 VSCode API 规范
getSessions
和 login
方法的更新:
- 新增的参数增强了会话管理的灵活性
- 保持了与 VSCode API 的兼容性
- 参数类型定义清晰
Types
Background or solution
Changelog
Summary by CodeRabbit
Summary by CodeRabbit
$getAccounts
,可获取指定提供者的账户信息。