fix: improve import error UX with server-reachability probe#24
fix: improve import error UX with server-reachability probe#24damienriehl wants to merge 1 commit intomainfrom
Conversation
- Add 5-min XHR timeout for large file uploads - Probe API root on XHR error to distinguish "server down" from "server returned error without CORS headers" (e.g. unhandled 500) - Move import error out of FileUpload into a separate alert banner - Clear import error when user selects a new file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFile upload error handling was enhanced with timeout detection and improved network diagnostics. The project creation page's error display was refactored to show errors in a dedicated alert panel instead of through the FileUpload component prop. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/projects/new/page.tsx (1)
320-328: Announce the import failure banner to assistive tech.This new panel is visually clear, but it will not be announced automatically when it appears. Adding
role="alert"makes the failure much easier to notice for screen-reader users.Suggested change
- <div className="mt-3 flex items-start gap-2 rounded-lg border border-red-200 bg-red-50 p-3 text-sm text-red-700 dark:border-red-800 dark:bg-red-900/20 dark:text-red-400"> + <div + role="alert" + className="mt-3 flex items-start gap-2 rounded-lg border border-red-200 bg-red-50 p-3 text-sm text-red-700 dark:border-red-800 dark:bg-red-900/20 dark:text-red-400" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/projects/new/page.tsx` around lines 320 - 328, The import failure banner currently rendered when importError is truthy isn't announced to screen readers; update the container div that renders the banner (the JSX block checking importError) to include an accessibility announcement attribute such as role="alert" (you may also add aria-live="assertive" if desired) so screen readers automatically announce the error when the banner appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/api/client.ts`:
- Around line 198-199: The probe uses probe.ok (2xx-only) to set
serverReachable, which treats resolved non-2xx responses as unreachable; change
the logic in the probe block (where fetch(`${API_BASE}/`, ...) returns probe) to
treat any resolved Response as proof of reachability by setting serverReachable
= true when fetch resolves (and only mark unreachable on thrown network errors).
Update the code around the probe variable and serverReachable assignment to
reflect this (or alternatively point the probe at a dedicated health endpoint
that guarantees 2xx), ensuring the fetch rejection is the only condition that
sets serverReachable = false.
---
Nitpick comments:
In `@app/projects/new/page.tsx`:
- Around line 320-328: The import failure banner currently rendered when
importError is truthy isn't announced to screen readers; update the container
div that renders the banner (the JSX block checking importError) to include an
accessibility announcement attribute such as role="alert" (you may also add
aria-live="assertive" if desired) so screen readers automatically announce the
error when the banner appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b503b12e-0d1c-43e5-ac17-dca9f7012be2
📒 Files selected for processing (2)
app/projects/new/page.tsxlib/api/client.ts
| const probe = await fetch(`${API_BASE}/`, { method: "HEAD", mode: "cors" }); | ||
| serverReachable = probe.ok; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the Fetch API resolve for HTTP 404/500 responses, and is Response.ok only true for 2xx statuses?
💡 Result:
Yes, the Fetch API resolves (does not reject) its Promise for HTTP 404 and 500 responses. These are considered valid HTTP responses, not network errors. The Promise only rejects on actual network failures (e.g., no connection, DNS failure). Yes, Response.ok is true only for 2xx status codes (200-299). It is false for 4xx, 5xx, and other non-successful statuses.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Response/ok
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Response
- 4: https://stackoverflow.com/questions/39297345/fetch-resolves-even-if-404
- 5: https://stackoverflow.com/questions/77257252/why-does-the-fetch-treat-a-404-response-as-a-response-without-an-error
Use any resolved fetch response to detect reachability, not just 2xx status codes.
When fetch() resolves successfully, the server is reachable—regardless of the HTTP status code. Checking probe.ok (true only for 2xx responses) will incorrectly classify a reachable server returning 401, 404, 405, or 500 as unreachable, which undermines the error handler's goal to distinguish between network failures and API errors.
Either accept any resolved response as proof of reachability:
- const probe = await fetch(`${API_BASE}/`, { method: "HEAD", mode: "cors" });
- serverReachable = probe.ok;
+ await fetch(`${API_BASE}/`, { method: "HEAD", mode: "cors" });
+ serverReachable = true;Or probe a dedicated health endpoint that guarantees a 2xx response.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const probe = await fetch(`${API_BASE}/`, { method: "HEAD", mode: "cors" }); | |
| serverReachable = probe.ok; | |
| await fetch(`${API_BASE}/`, { method: "HEAD", mode: "cors" }); | |
| serverReachable = true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/api/client.ts` around lines 198 - 199, The probe uses probe.ok (2xx-only)
to set serverReachable, which treats resolved non-2xx responses as unreachable;
change the logic in the probe block (where fetch(`${API_BASE}/`, ...) returns
probe) to treat any resolved Response as proof of reachability by setting
serverReachable = true when fetch resolves (and only mark unreachable on thrown
network errors). Update the code around the probe variable and serverReachable
assignment to reflect this (or alternatively point the probe at a dedicated
health endpoint that guarantees 2xx), ensuring the fetch rejection is the only
condition that sets serverReachable = false.
1 participant
Summary
Context
Importing an 18MB OWL file showed "Could not reach the server" even though the server was running — the actual cause was a 500 from a missing DB migration whose response lacked CORS headers, triggering the XHR
errorevent instead ofload.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes