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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesRequest ID propagation and display
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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/error-request-id.spec.tsOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON src/components/Notification.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 |
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
The dashboard reads the request id from a cross-origin response, so the browser only surfaces the header to JS when the response lists it in Access-Control-Expose-Headers. Mirror a CORS-enabled management server in the mock so the test exercises the dashboard logic rather than the header being filtered out cross-origin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016B2yLBeDK3kvX2FvhqcWk8
Add a notification-title testid to the toast and target it with getByTestId in the request-id spec, following the repo convention of selecting by data-testid rather than brittle visible text.
2 participants
Issue ticket number and link
N/A — UX improvement.
Summary
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 it can be quoted in support tickets. It is shown only when the header is present and readable, so older servers are unaffected.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, covering every promise-based mutation toast via 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
Documentation
Select exactly one:
Surfacing an existing management API response header in the error toast is a small UX addition with no new user-facing configuration or workflow to document.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
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
The dashboard reads
X-Request-Idfrom the fetch response. When the dashboard and management API are on different origins (e.g.app.netbird.io→api.netbird.io), the browser only exposes a non-safelisted response header to JS if the server lists it inAccess-Control-Expose-Headers. I verified this locally: withoutAccess-Control-Expose-Headers: X-Request-Id,response.headers.get("X-Request-Id")returnsnulleven though the body is readable (this is what the e2e mock now mirrors).Action needed: confirm the management server adds
X-Request-IdtoAccess-Control-Expose-Headers, otherwise this feature silently no-ops on cross-origin deployments. Same-origin deployments are unaffected. I could not verify the current management CORS config from this repo.🤖 Generated with Claude Code
https://claude.ai/code/session_016B2yLBeDK3kvX2FvhqcWk8
Summary by CodeRabbit