feat(core): improve byok ux - #15303
Merged
Merged
Conversation
github-actions
Bot
added
mod:i18n
Contributor
📝 WalkthroughWalkthroughChangesBYOK private endpoint support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BYOKModal
participant ByokSettingsQuery
participant ByokService
participant runProviderProbe
participant Provider
BYOKModal->>ByokSettingsQuery: Request privateEndpointSupported
ByokSettingsQuery->>ByokService: Resolve workspace BYOK settings
ByokService-->>BYOKModal: Return endpoint capabilities
BYOKModal->>ByokService: Test provider configuration
ByokService->>runProviderProbe: Pass provider, credentials, endpoint, capability
runProviderProbe->>Provider: GET models endpoint
Provider-->>runProviderProbe: HTTP response
runProviderProbe-->>BYOKModal: Success or probe error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/frontend/core/src/__tests__/auth-native-sign-in.spec.ts (2)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary computed property syntax.
The property name
config$is a valid identifier and does not require computed property brackets.♻️ Proposed refactor
- ['config$']: { + config$: {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/core/src/__tests__/auth-native-sign-in.spec.ts` at line 49, Update the object literal in the auth native sign-in test to use the direct config$ property syntax instead of the unnecessary computed-property brackets, preserving its existing value and behavior.
15-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove redundant parameterization or utilize the platform parameter.
The test uses
test.each(['android', 'ios'])but does not accept or use the platform argument in its callback. Consequently, the exact same test runs twice without verifying any platform-specific logic or state.If the environment needs to be mocked per platform (e.g., injecting the platform into a service or global config), pass the parameter to the closure (like
async (platform) => { ... }) and configure the dependency injection accordingly. Otherwise, simplify the test to a single execution.♻️ Proposed refactor (simplifying to a single test)
- test.each(['android', 'ios'])( - 'waits for the %s session bootstrap before completing', - async () => { + test('waits for the native session bootstrap before completing', async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/frontend/core/src/__tests__/auth-native-sign-in.spec.ts` around lines 15 - 18, Remove the redundant test.each(['android', 'ios']) parameterization around the session bootstrap test and make it a single test, unless platform-specific setup is required. If retaining parameterization, accept the platform argument in the callback and use it to configure the relevant mocked dependency or state.packages/backend/server/src/__tests__/copilot/byok-probe.spec.ts (1)
11-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the probe failure path.
Both tests only cover the
response.okpath.providerProbeFailureMessage(probe.ts Lines 79-94) status-code mapping and theBadRequestExceptionthrow on non-ok responses aren't exercised here, so a regression in that mapping wouldn't be caught.♻️ Suggested additional test
test('provider probe surfaces provider rejection as BadRequestException', async t => { const fetch = Sinon.stub< Parameters<typeof safeFetch>, ReturnType<typeof safeFetch> >().resolves(new Response('{}', { status: 401 })); await t.throwsAsync( () => runProviderProbe(fetch, ByokProvider.openai, 'secret', null, false), { message: 'Provider rejected the BYOK key.' } ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/server/src/__tests__/copilot/byok-probe.spec.ts` around lines 11 - 53, Add a test covering the non-ok response path of runProviderProbe: stub safeFetch to return a 401 response, invoke the probe with no private target, and assert it throws BadRequestException with the mapped message “Provider rejected the BYOK key.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/backend/server/src/__tests__/copilot/byok-probe.spec.ts`:
- Around line 11-53: Add a test covering the non-ok response path of
runProviderProbe: stub safeFetch to return a 401 response, invoke the probe with
no private target, and assert it throws BadRequestException with the mapped
message “Provider rejected the BYOK key.”
In `@packages/frontend/core/src/__tests__/auth-native-sign-in.spec.ts`:
- Line 49: Update the object literal in the auth native sign-in test to use the
direct config$ property syntax instead of the unnecessary computed-property
brackets, preserving its existing value and behavior.
- Around line 15-18: Remove the redundant test.each(['android', 'ios'])
parameterization around the session bootstrap test and make it a single test,
unless platform-specific setup is required. If retaining parameterization,
accept the platform argument in the callback and use it to configure the
relevant mocked dependency or state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44858eeb-e576-4828-8d58-ba55ea47a93b
📒 Files selected for processing (22)
.docker/selfhost/schema.jsonpackages/backend/server/src/__tests__/copilot/byok-probe.spec.tspackages/backend/server/src/plugins/copilot/byok/probe.tspackages/backend/server/src/plugins/copilot/byok/resolver.tspackages/backend/server/src/plugins/copilot/byok/service.tspackages/backend/server/src/plugins/copilot/config.tspackages/backend/server/src/schema.gqlpackages/common/graphql/src/graphql/index.tspackages/common/graphql/src/graphql/workspace-byok-settings.gqlpackages/common/graphql/src/schema.tspackages/frontend/admin/src/config.jsonpackages/frontend/core/src/__tests__/auth-native-sign-in.spec.tspackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/add-key-modal.tsxpackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/index.css.tspackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/index.spec.tsxpackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/index.tsxpackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/metadata.spec.tspackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/metadata.tspackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/byok/types.tspackages/frontend/i18n/src/i18n.gen.tspackages/frontend/i18n/src/resources/en.jsonpackages/frontend/i18n/src/resources/zh-Hans.json
Deploying blocksuite-docs with
|
| Latest commit: |
4d498a3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2a0e3b5.blocksuite-docs.pages.dev |
| Branch Preview URL: | https://darksky-improvve-byok-settin.blocksuite-docs.pages.dev |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #15303 +/- ##
==========================================
+ Coverage 60.72% 61.04% +0.32%
==========================================
Files 3311 3312 +1
Lines 191936 191988 +52
Branches 28277 28335 +58
==========================================
+ Hits 116548 117196 +648
+ Misses 71600 71012 -588
+ Partials 3788 3780 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #15265
PR Dependency Tree
This tree was auto-generated by Charcoal