Show management API request id in error notifications#698
Conversation
Newer management servers return an X-Request-Id header on every response, including errors. Surface it in the error toast so it can be quoted in support tickets. - api.tsx: read X-Request-Id from the response and attach it to the rejected ErrorResponse (both the JSON-body and status-text paths). - Notification.tsx: render a copyable "Request ID" line whenever an error carries one, covering the promise-based mutation notifications. - ErrorBoundary.tsx: thread the request id through for GET/fetch errors. The line is only shown when the header is present, so older servers that do not send it are unaffected. Adds an e2e spec covering both cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016B2yLBeDK3kvX2FvhqcWk8
📝 WalkthroughWalkthroughAdds propagation of a server ChangesRequest ID propagation and display
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant apiRequest
participant ErrorBoundaryProvider
participant Notification
participant User
Server-->>apiRequest: 500 response + X-Request-Id header
apiRequest->>apiRequest: parse error, extract requestId
apiRequest-->>ErrorBoundaryProvider: reject(error + requestId)
ErrorBoundaryProvider->>Notification: notify({..., requestId})
Notification->>Notification: update requestId state
Notification-->>User: render error toast with Request ID button
User->>Notification: click copy button
Notification->>Notification: copyRequestId() writes to clipboard
Notification-->>User: show copied confirmation icon
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016B2yLBeDK3kvX2FvhqcWk8
![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 (2)
src/components/Notification.tsx (2)
209-209: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRequest ID button doesn't gate on
errorstate.
requestIdcan be set via the initialrequestIdPropregardless of whether the toast represents an error. SinceNotifyProps.requestIdis a public prop, a future non-errornotify()call passingrequestIdwould render the "Request ID:" line on a success toast, which could be confusing.🔧 Suggested tightening
- {!loading && requestId && ( + {!loading && requestId && error && (🤖 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 `@src/components/Notification.tsx` at line 209, The Request ID display in Notification.tsx is gated only by loading and requestId, so a non-error toast could still render it if requestId is provided. Update the conditional around the Request ID block in the Notification component to also require the error state, using the existing Notification/NotifyProps flow and the requestIdProp-derived value so that only error toasts can show the “Request ID:” line.
77-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing the existing
CopyToClipboardTextcomponent instead of hand-rolled clipboard logic.The codebase already has a
CopyToClipboardTextcomponent for this exact copy-to-clipboard + feedback pattern. Reimplementingnavigator.clipboard.writeText+ timedcopiedstate here duplicates that logic.Based on learnings, "In components that use CopyToClipboardText, the message prop should be the notification shown after copying, while the children content is what gets copied to the clipboard."
Also applies to: 209-230
🤖 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 `@src/components/Notification.tsx` around lines 77 - 86, The Notification copy behavior is duplicating clipboard and copied-state logic in copyRequestId instead of using the existing CopyToClipboardText component. Refactor the request ID display in Notification.tsx to wrap the selectable ID with CopyToClipboardText, passing the ID as the copied content and the copy-success text through the message prop, and remove the manual navigator.clipboard.writeText, setCopied, and timeout handling. Apply the same pattern anywhere else in this component that reimplements the same copy flow, using the existing CopyToClipboardText API as the source of truth.
🤖 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/error-request-id.spec.ts`:
- Around line 61-63: The Playwright assertions in the error-request-id spec are
selecting status-code messages with getByText, which is brittle and violates the
test selector convention. Update the assertions in the relevant test block(s) to
use page.getByTestId() instead of page.getByText(), targeting the existing
data-testid on the status/error element(s) in this spec. Apply the same selector
change in the other referenced assertion block so all status-code checks use
stable test IDs.
- Around line 38-52: The test setup in openOwner() is bypassing the shared auth
fixture and creating a context directly, so update the spec to use the
dashboardAsOwner fixture instead of manual browser.newContext/storageState/login
flow. Refactor the test in error-request-id.spec.ts to rely on the custom
fixture from helpers/fixtures.ts and keep the page/close handling aligned with
that fixture pattern, including any other affected call sites in this spec.
---
Nitpick comments:
In `@src/components/Notification.tsx`:
- Line 209: The Request ID display in Notification.tsx is gated only by loading
and requestId, so a non-error toast could still render it if requestId is
provided. Update the conditional around the Request ID block in the Notification
component to also require the error state, using the existing
Notification/NotifyProps flow and the requestIdProp-derived value so that only
error toasts can show the “Request ID:” line.
- Around line 77-86: The Notification copy behavior is duplicating clipboard and
copied-state logic in copyRequestId instead of using the existing
CopyToClipboardText component. Refactor the request ID display in
Notification.tsx to wrap the selectable ID with CopyToClipboardText, passing the
ID as the copied content and the copy-success text through the message prop, and
remove the manual navigator.clipboard.writeText, setCopied, and timeout
handling. Apply the same pattern anywhere else in this component that
reimplements the same copy flow, using the existing CopyToClipboardText API as
the source of truth.
🪄 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
Run ID: 0f8da750-f41d-472b-963b-184c45a15185
📒 Files selected for processing (4)
e2e/tests/error-request-id.spec.tssrc/components/Notification.tsxsrc/contexts/ErrorBoundary.tsxsrc/utils/api.tsx
| async function openOwner( | ||
| browser: Browser, | ||
| ): Promise<{ page: Page; close: () => Promise<void> }> { | ||
| const context = await browser.newContext({ | ||
| storageState: "e2e/fixtures/auth/owner.json", | ||
| }); | ||
| const page = await context.newPage(); | ||
| return { page, close: () => context.close() }; | ||
| } | ||
|
|
||
| test.describe.serial("API error request id @error-handling", () => { | ||
| test("shows the X-Request-Id from a failed request in the error toast", async ({ | ||
| browser, | ||
| }) => { | ||
| const { page, close } = await openOwner(browser); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use dashboardAsOwner fixture instead of manually creating contexts/login.
openOwner() manually spins up a browser.newContext() with storageState and calls loginToApp, bypassing the project's custom auth fixtures.
As per path instructions, "Use custom fixtures (dashboardAsOwner or dashboardAsUser) from helpers/fixtures.ts instead of raw page for test authentication."
Also applies to: 77-80
🤖 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/error-request-id.spec.ts` around lines 38 - 52, The test setup in
openOwner() is bypassing the shared auth fixture and creating a context
directly, so update the spec to use the dashboardAsOwner fixture instead of
manual browser.newContext/storageState/login flow. Refactor the test in
error-request-id.spec.ts to rely on the custom fixture from helpers/fixtures.ts
and keep the page/close handling aligned with that fixture pattern, including
any other affected call sites in this spec.
| await expect( | ||
| page.getByText("Request failed with status code 500").first(), | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use data-testid selectors instead of getByText for these assertions.
The status-code text assertions use page.getByText(...) rather than a data-testid selector, and are also brittle to copy changes.
As per path instructions, "Always use data-testid selectors via page.getByTestId() for element selection in Playwright tests."
Also applies to: 87-89
🤖 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/error-request-id.spec.ts` around lines 61 - 63, The Playwright
assertions in the error-request-id spec are selecting status-code messages with
getByText, which is brittle and violates the test selector convention. Update
the assertions in the relevant test block(s) to use page.getByTestId() instead
of page.getByText(), targeting the existing data-testid on the status/error
element(s) in this spec. Apply the same selector change in the other referenced
assertion block so all status-code checks use stable test IDs.
1 participant
What
Newer management API servers set an
X-Request-Idresponse header (telemetry.RequestIDHeader) on every response, including errors. This PR surfaces that request id in the dashboard's error notification so users can quote it in support tickets.The id is shown only when the header is present, so older servers that don't send it are unaffected (the line simply isn't rendered).
Changes
src/utils/api.tsx—ErrorResponsegains an optionalrequestId;apiRequestreadsX-Request-Idfrom the response headers and attaches it to both error-rejection paths (JSON-body error and the status-text fallback).src/components/Notification.tsx— renders a copyable "Request ID: …" line whenever an error carries one. This covers every promise-based mutation toast through the existing.catch.src/contexts/ErrorBoundary.tsx— threads the request id into the toast for GET/fetch errors.e2e/tests/error-request-id.spec.ts— mocks/networkswith a 500 in isolated browser contexts: header present → the id is shown; header absent → no Request ID line.Screenshots
Fetch/GET error with request id
Mutation error with request id
Older server (no header) — no Request ID line
Notes
X-Request-Idcross-origin (e.g.app.netbird.io→api.netbird.io), the management server must list it inAccess-Control-Expose-Headers. If it isn't exposed,res.headers.get()returnsnulland the feature gracefully no-ops. Please confirm the header is exposed, or add it.tsc --noEmit(clean). Note:npm run lintis currently broken on the base branch because Next.js 16 removednext lintand the repo has no flat ESLint config — unrelated to this change.🤖 Generated with Claude Code
https://claude.ai/code/session_016B2yLBeDK3kvX2FvhqcWk8