Handle agent-network settings defaults response - #739
Conversation
The management API now answers the settings GET with a defaults object before the account is bootstrapped (empty cluster/subdomain/endpoint) instead of a JSON null body. Normalize that shape to the existing null settings signal so every consumer keeps a single not-bootstrapped check, while still tolerating the null body and 404 from older backends.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe provider now normalizes missing agent-network endpoints and 404 responses as unbootstrapped settings. End-to-end tests cover default-only, ChangesAgent network settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProvidersPage
participant AIProvidersProvider
participant SettingsAPI
ProvidersPage->>AIProvidersProvider: Load agent-network settings
AIProvidersProvider->>SettingsAPI: Request settings
SettingsAPI-->>AIProvidersProvider: Defaults, null, 404, or configured settings
AIProvidersProvider-->>ProvidersPage: Null or normalized settings
ProvidersPage-->>ProvidersPage: Render empty state or configured endpoint
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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-settings-shapes.spec.tsOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON src/modules/agent-network/AIProvidersProvider.tsxOops! Something went wrong! :( ESLint: 9.39.3 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 providers page must render the connect-first empty state for every unbootstrapped settings shape the management API has answered with over time (defaults object with an empty endpoint, JSON null body, 404) and the endpoint badge once bootstrapped. The settings route is mocked per test so the spec is independent of the backend build and of account state left behind by other suites.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
e2e/tests/agent-network-settings-shapes.spec.ts (5)
80-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
navigateTofor the providers navigation.
page.goto()followed bypage.keyboard.press("Escape")does not apply the setup-modal dismissal and scroll-lock cleanup provided bynavigateTo. Replace this pair withnavigateTo(page, "/agent-network/providers").As per coding guidelines, use
navigateTo(page, path)instead of directpage.goto().Suggested change
- await page.goto("/agent-network/providers"); - await page.keyboard.press("Escape"); + await navigateTo(page, "/agent-network/providers");🤖 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/agent-network-settings-shapes.spec.ts` around lines 80 - 84, In the setup flow returning the page and close handler, replace the direct page.goto("/agent-network/providers") plus Escape keypress with navigateTo(page, "/agent-network/providers"), preserving the existing login and return behavior.Source: Coding guidelines
108-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a serial test suite.
Change
test.describetotest.describe.serial. This follows the required E2E suite structure.As per coding guidelines, organize test specs with
test.describe.serial().🤖 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/agent-network-settings-shapes.spec.ts` around lines 108 - 109, Update the suite declaration around UNBOOTSTRAPPED_SHAPES from test.describe to test.describe.serial, preserving the existing suite body and test structure.Source: Coding guidelines
80-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWait for the settings response before asserting the rendered state.
Start
page.waitForResponse()before navigation. MatchGETrequests to/api/agent-network/settingsand set a timeout. Await the response before returning fromopenProvidersPage.Without this wait, assertions can observe the initial UI before the settings hook consumes the mock. The PR objective is to verify each response shape.
As per coding guidelines, wait for specific API responses with
page.waitForResponse()before assertions.🤖 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/agent-network-settings-shapes.spec.ts` around lines 80 - 84, Update openProvidersPage to start page.waitForResponse before navigating to /agent-network/providers, matching the GET /api/agent-network/settings request with a timeout. Await that response after navigation and before returning the page so assertions observe the rendered state after the mocked settings response is consumed.Source: Coding guidelines
115-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
data-testidselectors for state assertions.Replace the
page.getByText()locators withpage.getByTestId()locators for the empty state and endpoint. If the components do not expose test IDs, add optionaldata-testid?: stringprops and use stable kebab-case IDs.As per coding guidelines, Playwright specs must use
page.getByTestId()for element selection.Also applies to: 131-132
🤖 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/agent-network-settings-shapes.spec.ts` around lines 115 - 116, Replace the page.getByText() assertions in the agent network settings test, including the additional assertions at the referenced later range, with page.getByTestId() using stable kebab-case IDs for the empty state and bootstrapped endpoint. If those components lack test IDs, add optional data-testid props and pass the corresponding IDs through their render paths.Source: Coding guidelines
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
dashboardAsOwnerfor agent-network spec authentication.
e2e/tests/**/*.spec.tsmust use custom fixtures instead of raw-page logins. Replace the localBrowser+owner.json+loginToApp(page, "owner")flow with an injecteddashboardAsOwnerpage, and move thee2e/fixtures/auth/owner.jsonfeature-flag logic into the fixture lifecycle.🤖 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/agent-network-settings-shapes.spec.ts` around lines 15 - 16, The agent-network spec should use the custom dashboardAsOwner fixture instead of raw Browser/Page authentication. Update the test setup and test signatures to inject dashboardAsOwner, remove the Browser and loginToApp imports/usages, and relocate the owner.json feature-flag setup into the fixture lifecycle while preserving the existing test behavior.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-settings-shapes.spec.ts`:
- Around line 68-84: Update openProvidersPage so browser context cleanup is
handled during setup: wrap context creation and subsequent initialization in
try/catch or try/finally, close the context if mockSettingsResponse, loginToApp,
navigation, or other setup steps fail, then rethrow the original error; preserve
the existing close callback on successful setup.
In `@src/modules/agent-network/AIProvidersProvider.tsx`:
- Around line 542-545: Update the settings memo in AIProvidersProvider around
settingsFromAPI so it returns null whenever the request has a 404 error, even if
keepPreviousData supplies stale data. Include notFound in the predicate and add
both error and notFound to the memo dependency array.
---
Nitpick comments:
In `@e2e/tests/agent-network-settings-shapes.spec.ts`:
- Around line 80-84: In the setup flow returning the page and close handler,
replace the direct page.goto("/agent-network/providers") plus Escape keypress
with navigateTo(page, "/agent-network/providers"), preserving the existing login
and return behavior.
- Around line 108-109: Update the suite declaration around UNBOOTSTRAPPED_SHAPES
from test.describe to test.describe.serial, preserving the existing suite body
and test structure.
- Around line 80-84: Update openProvidersPage to start page.waitForResponse
before navigating to /agent-network/providers, matching the GET
/api/agent-network/settings request with a timeout. Await that response after
navigation and before returning the page so assertions observe the rendered
state after the mocked settings response is consumed.
- Around line 115-116: Replace the page.getByText() assertions in the agent
network settings test, including the additional assertions at the referenced
later range, with page.getByTestId() using stable kebab-case IDs for the empty
state and bootstrapped endpoint. If those components lack test IDs, add optional
data-testid props and pass the corresponding IDs through their render paths.
- Around line 15-16: The agent-network spec should use the custom
dashboardAsOwner fixture instead of raw Browser/Page authentication. Update the
test setup and test signatures to inject dashboardAsOwner, remove the Browser
and loginToApp imports/usages, and relocate the owner.json feature-flag setup
into the fixture lifecycle while preserving the existing test behavior.
🪄 Autofix (Beta)
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: 52cbb84a-0ed6-46b3-b919-be8c83e22e4c
📒 Files selected for processing (2)
e2e/tests/agent-network-settings-shapes.spec.tssrc/modules/agent-network/AIProvidersProvider.tsx
useFetchApi keeps the previous SWR data alongside the error, so a later 404 could expose stale settings through the memo; it now yields null when the error is a 404 while still riding out transient failures on the kept data. The wire-shapes spec closes its browser context when a setup step throws before the close callback is handed to the test.
* Handle agent-network settings defaults response The management API now answers the settings GET with a defaults object before the account is bootstrapped (empty cluster/subdomain/endpoint) instead of a JSON null body. Normalize that shape to the existing null settings signal so every consumer keeps a single not-bootstrapped check, while still tolerating the null body and 404 from older backends. * Pin the settings wire-shape normalization in a Playwright spec The providers page must render the connect-first empty state for every unbootstrapped settings shape the management API has answered with over time (defaults object with an empty endpoint, JSON null body, 404) and the endpoint badge once bootstrapped. The settings route is mocked per test so the spec is independent of the backend build and of account state left behind by other suites. * Address review: no stale settings on 404, close context on setup failure useFetchApi keeps the previous SWR data alongside the error, so a later 404 could expose stale settings through the memo; it now yields null when the error is a 404 while still riding out transient failures on the kept data. The wire-shapes spec closes its browser context when a setup step throws before the close callback is handed to the test.
2 participants
Issue ticket number and link
NET-1465 — companion to netbirdio/netbird#7026, where the management API's settings GET now answers the unbootstrapped state with a defaults object (empty
cluster/subdomain/endpoint) instead of a JSONnullbody.useAgentNetworkSettingsnormalizes that shape to the existingsettings === nullsignal, so every consumer keeps a single not-bootstrapped check. Thenullbody and 404 from older backends still normalize the same way, so the dashboard works against both server generations.created_at/updated_atbecome optional in the API type since they are absent until a row is persisted.Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
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
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests