Bootstrap Agent Network settings via the explicit POST - #746
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds explicit account-level Agent Network settings bootstrap before first provider creation. It updates settings models and requests, handles uninitialized and concurrent bootstrap states, and strengthens Agent Network and service-user E2E coverage. ChangesAgent Network settings bootstrap
Service-user E2E navigation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant AIProviderModal
participant AIProvidersProvider
participant AgentNetworkSettingsAPI
AIProviderModal->>AIProvidersProvider: bootstrapAgentNetworkSettings(proxyAddress)
AIProvidersProvider->>AgentNetworkSettingsAPI: create settings with proxy_address
AgentNetworkSettingsAPI-->>AIProvidersProvider: success or 409
AIProvidersProvider->>AgentNetworkSettingsAPI: refresh settings after 409
AIProvidersProvider-->>AIProviderModal: bootstrap result
AIProviderModal->>AIProvidersProvider: addProvider()
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
e2e/tests/agent-network-kimi-provider.spec.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: Converting circular structure to JSON e2e/tests/agent-network-settings-shapes.spec.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: Converting circular structure to JSON e2e/tests/team-service-users.spec.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: Converting circular structure to JSON 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 |
The management API replaced the implicit settings bootstrap (a bootstrap_cluster hint on the first provider create) with an explicit POST /agent-network/settings carrying the proxy cluster address; the settings response now carries endpoint + proxy_address + dedicated instead of cluster + subdomain, and the endpoint is assigned as a server-allocated label beneath the proxy address. The wizard keeps its UX — auto-picking a validated cluster and gating the first create on having one — but now bootstraps the settings row with an explicit POST right before the first provider create. A bootstrap failure keeps the wizard open and surfaces the error instead of being swallowed server-side, and a 409 (concurrent bootstrap won) is treated as success since the row exists either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The settings PUT now follows the REST API's full-replace convention: every field is required, including endpoint and proxy_address. The backend compares the identity fields against the stored row and rejects a mismatch with 422 instead of leaving them out of the schema, so the update path now echoes the loaded settings' identity verbatim — stored values only, never user input, which keeps the rejection unreachable from the UI. A save before the settings row exists fails fast with a notification; the backend would 404 that PUT anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4e47121 to
b59d7d7
Compare
…bootstrap on old backends Two unrelated Playwright failures on this branch. The service-users suite reverted its own change and then blamed the UI. Returning to the list by clicking the nav entry resolves while the client-side navigation is still in flight, so the next test's row click was computed against the page being left behind and landed on the wrong row. Both role PUTs went to the regular user, the second undoing the first, and the assertions two steps later failed on a list that was telling the truth. Nothing checked either the target or the status of the PUT, so the mis-edit passed silently. Land on the list deterministically, confirm the user page really belongs to the user being opened before editing it, verify the role pick landed, and pin the PUT to the open user's id with a status assertion — so a mis-click fails where it happens instead of corrupting account state. The Kimi suite timed out at 30s because the first provider create now bootstraps the settings row first, and POST /agent-network/settings does not exist before netbirdio/netbird#7085. The wizard never got to the provider create, and the wait for it hung. Probe the endpoint off the settings GET shape and skip, as the suite already does for a catalog without kimi_api, plus race a rejected bootstrap so the status is reported rather than a bare timeout. Also update the settings wire-shape fixtures to the new contract; they still described cluster + subdomain and only passed by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/tests/team-service-users.spec.ts (1)
101-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
data-testidselectors for navigation and row selection.
returnToServiceUserListselects the breadcrumb withgetByRole.openServiceUserselects a table row withlocator("tr"). Replace both interactive selectors withpage.getByTestId()selectors. Add the required test IDs to the UI if they do not exist.As per coding guidelines, use
data-testidselectors viapage.getByTestId()for element selection in Playwright tests.🤖 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 `@e2e/tests/team-service-users.spec.ts` around lines 101 - 125, Replace the role-based navigation selector in returnToServiceUserList and the generic row locator in openServiceUser with page.getByTestId() selectors. Add the corresponding data-testid attributes to the Service Users navigation link and user rows if they are not already present, while preserving the existing URL and visibility checks.Source: Coding guidelines
🤖 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.
Inline comments:
In `@e2e/tests/agent-network-kimi-provider.spec.ts`:
- Around line 146-149: Replace the role-based selector for the “Connect
Provider” action in this test with page.getByTestId() using the component’s
existing test ID; if none exists, add an appropriate kebab-case data-testid to
the button and target it from the test.
- Around line 129-145: Update the bootstrap failure response predicate in the
provider creation race so HTTP 409 responses are excluded from failure matching,
while continuing to reject other non-2xx responses. Preserve the existing
timeout and pending-promise behavior for responses that do not match.
In `@e2e/tests/agent-network-settings-shapes.spec.ts`:
- Around line 26-27: Correct the comment describing UNBOOTSTRAPPED_DEFAULTS to
identify the fixture as unbootstrapped settings, while retaining the notes about
present values, empty endpoint/proxy_address, and absent timestamps.
---
Nitpick comments:
In `@e2e/tests/team-service-users.spec.ts`:
- Around line 101-125: Replace the role-based navigation selector in
returnToServiceUserList and the generic row locator in openServiceUser with
page.getByTestId() selectors. Add the corresponding data-testid attributes to
the Service Users navigation link and user rows if they are not already present,
while preserving the existing URL and visibility checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dbd4f8f6-526f-45bb-858b-143911fc2663
📒 Files selected for processing (4)
e2e/helpers/api.tse2e/tests/agent-network-kimi-provider.spec.tse2e/tests/agent-network-settings-shapes.spec.tse2e/tests/team-service-users.spec.ts
…ser specs Do not treat a 409 from the settings bootstrap as a failure in the Kimi provider race. The wizard accepts it — a concurrent bootstrap won, so the row exists — and goes on to create the provider, so matching it would fail a run that was about to succeed. Other agent-network suites run in the second worker against the same account, which is exactly where that race comes from. Select the breadcrumb by its existing breadcrumb-item test id rather than by role, per the e2e selector guidance. Reflow the unbootstrapped fixture comment: "before the account is bootstrapped" wrapped across lines and read as though the fixture were the bootstrapped one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adopts the new Agent Network settings contract: - The management API replaced the implicit settings bootstrap — a `bootstrap_cluster` hint on the first provider create — with an explicit POST /agent-network/settings carrying the proxy cluster address. The settings response now carries endpoint + proxy_address + dedicated instead of cluster + subdomain. - The provider wizard keeps its UX (auto-picking a validated cluster, gating the first create on having one) but now bootstraps the settings row with the explicit POST right before the first provider create. A bootstrap failure keeps the wizard open and surfaces the error — previously a failed bootstrap was swallowed server-side and the account silently ended up with a provider but no endpoint. A 409 (concurrent bootstrap won) is treated as success, since the row exists either way. - Types updated accordingly; bootstrapAgentNetworkSettings is exposed on the providers context. The onboarding provider step reuses the same wizard, so it is covered by the same change. Without this, the dashboard against a new management silently stops bootstrapping accounts: provider create succeeds (the old hint is an ignored unknown field), but no endpoint is ever assigned.
Adopts the new Agent Network settings contract: - The management API replaced the implicit settings bootstrap — a `bootstrap_cluster` hint on the first provider create — with an explicit POST /agent-network/settings carrying the proxy cluster address. The settings response now carries endpoint + proxy_address + dedicated instead of cluster + subdomain. - The provider wizard keeps its UX (auto-picking a validated cluster, gating the first create on having one) but now bootstraps the settings row with the explicit POST right before the first provider create. A bootstrap failure keeps the wizard open and surfaces the error — previously a failed bootstrap was swallowed server-side and the account silently ended up with a provider but no endpoint. A 409 (concurrent bootstrap won) is treated as success, since the row exists either way. - Types updated accordingly; bootstrapAgentNetworkSettings is exposed on the providers context. The onboarding provider step reuses the same wizard, so it is covered by the same change. Without this, the dashboard against a new management silently stops bootstrapping accounts: provider create succeeds (the old hint is an ignored unknown field), but no endpoint is ever assigned.
3 participants
Important
Depends on netbirdio/netbird#7085 (management: rework Agent Network endpoint identity and settings bootstrap). Draft until that merges — against a management build without it, the settings POST this introduces does not exist.
Adopts the new Agent Network settings contract:
bootstrap_clusterhint on the first provider create — with an explicitPOST /agent-network/settingscarrying the proxy cluster address. The settings response now carriesendpoint+proxy_address+dedicatedinstead ofcluster+subdomain.409(concurrent bootstrap won) is treated as success, since the row exists either way.bootstrapAgentNetworkSettingsis exposed on the providers context. The onboarding provider step reuses the same wizard, so it is covered by the same change.Without this, the dashboard against a new management silently stops bootstrapping accounts: provider create succeeds (the old hint is an ignored unknown field), but no endpoint is ever assigned.
Verified manually end to end against a local management running netbirdio/netbird#7085 (labeled bootstrap through the wizard, endpoint rendered, second create skips the bootstrap, failure path keeps the wizard open).
Issue ticket number and link
Internal (NetBird team) change; ticket to be linked alongside the management PR before this leaves draft.
Documentation
Select exactly one:
Docs for the changed settings API are being handled with the management PR (netbirdio/netbird#7085); nothing dashboard-specific beyond that.
E2E tests
Optional: override the image tags used by the Playwright e2e workflow.
Defaults to
mainwhen omitted.management-cloud-tag: main
reverse-proxy-tag: main
Summary by CodeRabbit
New Features
Bug Fixes