Dev tool redesign#1409
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughDev tool UI, placement, and lifecycle were overhauled: trigger moved to a 36×36 corner-docked square with quadrant snapping; placement model changed from side+offset → corner; trigger creation returns element+cleanup; overview/console tabs altered; singleton mounting/cleanup and an opt-out devTool flag were added. ChangesDev Tool UI & Lifecycle Overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Stack Auth template dev tool UX/behavior and improves reliability around mounting, trigger positioning, and some backend AI docs-tool error handling.
Changes:
- Add a
devTool?: booleanconstructor option to allow disabling the dev tool indicator/mounting in browser-like dev environments. - Redesign the dev tool trigger to be icon-only with corner snapping (plus state migrations and updated tests/CSS/layout).
- Make docs AI tool failures throw (with structured capture) so the AI SDK can surface tool errors instead of silently returning fallback strings.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/template/src/lib/stack-app/apps/interfaces/client-app.ts | Adds the devTool?: boolean option to the client app constructor options. |
| packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts | Gates dev tool mounting on devTool !== false and includes devTool in client JSON serialization. |
| packages/template/src/dev-tool/index.ts | Fixes StrictMode double-invoke cleanup behavior by scoping cleanup to the current mount call. |
| packages/template/src/dev-tool/dev-tool-trigger-position.ts | Replaces side+offset snapping with corner-based snapping and simplified clamping logic. |
| packages/template/src/dev-tool/dev-tool-trigger-position.test.ts | Updates/expands tests to cover corner snapping, corner resolution, resize anchoring, and clamping. |
| packages/template/src/dev-tool/dev-tool-styles.ts | Updates trigger styling (icon-only) and shifts Overview tab layout to a single-column design. |
| packages/template/src/dev-tool/dev-tool-core.ts | Implements the redesigned trigger behavior (corner placement, migrations, resize handling), updates tabs, and refactors Overview content. |
| apps/backend/src/lib/ai/tools/docs.ts | Changes docs-tools error handling to throw on transport/HTTP/parse/API errors (with captureError tags). |
| .claude/CLAUDE-KNOWLEDGE.md | Documents the env-var gate for whether the Dashboard tab iframes or links out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case 'top-right': { | ||
| return { left: viewport.width - triggerSize.width - m, top: m }; | ||
| } |
| const defaultPos = (): Position => ({ | ||
| left: window.innerWidth - 76 - 16, | ||
| top: window.innerHeight - 36 - 16, | ||
| left: window.innerWidth - triggerSize.width - 16, | ||
| top: window.innerHeight - triggerSize.height - 16, | ||
| }); |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/lib/ai/tools/docs.ts (1)
28-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the
fetchcall to prevent indefinitely blocked backend threads.If the docs service hangs, this call blocks the backend request thread indefinitely. The AI SDK forwards abort signals from
generateText/streamTextto the toolexecutefunction via its second parameter, specifically to abort long-running computations or forward them tofetchcalls inside tools. Theexecutecallbacks increateDocsToolsdon't yet accept or forward that signal, so a belt-and-suspenders fix is to combine forwarding the signal with a localAbortControllertimeout:⏱️ Proposed fix
- let res: Response; + let res: Response; + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(new Error("Docs tools request timed out after 30s")), 30_000); try { res = await fetch(`${base}/api/internal/docs-tools`, { method: "POST", headers: { "Content-Type": "application/json", Accept: "application/json, text/event-stream", }, body: JSON.stringify(action), + signal: controller.signal, }); } catch (err) { captureError("docs-tools-transport-error", err instanceof Error ? err : new Error(String(err))); throw new Error("Stack Auth docs tools are temporarily unavailable. Please try again later."); + } finally { + clearTimeout(timeout); }You would also need to pass the
AbortSignalfrom the tool'sexecutecontext intopostDocsToolActionso it can be combined with the local timeout signal (e.g. viaAbortSignal.any([signal, controller.signal])in environments that support it).🤖 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 `@apps/backend/src/lib/ai/tools/docs.ts` around lines 28 - 41, The fetch in postDocsToolAction can hang forever; modify postDocsToolAction (and its callers such as createDocsTools.execute) to accept an AbortSignal parameter, create a local AbortController with a short timeout, combine that controller.signal with the incoming signal (use AbortSignal.any([incomingSignal, timeoutController.signal]) when available or manually listen to abort events), pass the combined signal into fetch via the request's signal option, clear the timeout on completion, and propagate abort errors appropriately; ensure you reference postDocsToolAction and the createDocsTools.execute call sites so the signal is forwarded through the tool execution path.
🧹 Nitpick comments (3)
packages/template/src/dev-tool/dev-tool-trigger-position.test.ts (1)
8-103: 💤 Low valueConsider
toMatchInlineSnapshotfor the position assertions.Several assertions use
toEqual({ left: …, top: … })with explicit derived numbers (e.g.,1000 - 36 - 16). Switching them totoMatchInlineSnapshotwould be in line with the project's preference for inline snapshots in tests and avoid duplicating the expected geometry math in each case.As per coding guidelines, "When writing tests, prefer .toMatchInlineSnapshot over other selectors, if possible. Check snapshot-serializer.ts to see how snapshots are formatted and how non-deterministic values are handled".
🤖 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 `@packages/template/src/dev-tool/dev-tool-trigger-position.test.ts` around lines 8 - 103, Replace explicit numeric expectation objects in the tests with inline snapshots: for each assertion calling resolveTriggerPosition or clampTriggerPosition or getSnappedTriggerPlacement (e.g., expect(pos).toEqual({ left: 1000 - 36 - 16, top: 700 - 36 - 16 }) and similar), change the matcher to expect(...).toMatchInlineSnapshot() and update the snapshot content by running the test runner to record the snapshot; ensure you convert all occurrences in this file (tests referencing resolveTriggerPosition, getSnappedTriggerPlacement, clampTriggerPosition) so they follow the project's snapshot format and serializer.packages/template/src/dev-tool/dev-tool-core.ts (1)
2310-2322: 💤 Low value
closePanelisOpenupdate duplicatestogglePanel's update.When called from
togglePanel(line 2326) thestate.update({ isOpen: false })runs twice in quick succession, triggering listeners twice. Not a correctness issue, but you could either drop the explicitstate.updatehere and havetogglePanelsolely own the toggle, or haveclosePanelalways own it and lettogglePanelsimply callclosePanel(). The latter is probably cleaner and avoids divergence if another caller forgets to update state before invokingclosePanel.🤖 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 `@packages/template/src/dev-tool/dev-tool-core.ts` around lines 2310 - 2322, closePanel currently calls state.update({ isOpen: false }) but togglePanel also updates state, causing duplicate listener invocations; make closePanel the single owner of closing state by keeping state.update({ isOpen: false }) inside closePanel and removing the duplicate state.update call from togglePanel (and any other callers that call closePanel), so togglePanel should simply call closePanel() without updating state itself; ensure all paths that close the panel call closePanel() so the single state update is preserved.apps/backend/src/lib/ai/tools/docs.ts (1)
49-55: ⚡ Quick winReplace
as DocsToolHttpResulttype cast with Zod runtime validation.
res.json()returnsany, soas DocsToolHttpResultgives false compile-time confidence without any runtime guarantee. An unexpected response shape will silently pass through. Sincezodis already imported, a schema parse is the idiomatic fix.♻️ Proposed refactor
Add a Zod schema at module scope (near the
DocsToolHttpResulttype):+const DocsToolHttpResultSchema = z.object({ + content: z + .array(z.object({ type: z.string(), text: z.string().optional() })) + .optional(), + isError: z.boolean().optional(), +}); type DocsToolHttpResult = { content?: Array<{ type: string, text?: string }>, isError?: boolean, };Then replace the cast:
- let data: DocsToolHttpResult; try { - data = (await res.json()) as DocsToolHttpResult; + data = DocsToolHttpResultSchema.parse(await res.json()); } catch (err) { captureError("docs-tools-parse-error", err instanceof Error ? err : new Error(String(err))); throw new Error("Stack Auth docs tools returned an invalid response. Please try again later."); }As per coding guidelines, "Do NOT use
as/any/type casts or anything else like that to bypass the type system."🤖 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 `@apps/backend/src/lib/ai/tools/docs.ts` around lines 49 - 55, Replace the unsafe cast of res.json() to DocsToolHttpResult with Zod runtime validation: define a Zod schema for DocsToolHttpResult at module scope (near the existing type), then call schema.parse or schema.safeParse on the parsed JSON instead of using "as DocsToolHttpResult"; on parse failure call captureError("docs-tools-parse-error", error) (preserving the existing error handling pattern) and throw the same user-facing Error, otherwise assign the validated result to the local variable data so all downstream uses (data, DocsToolHttpResult) are strongly validated at runtime.
🤖 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 `@apps/backend/src/lib/ai/tools/docs.ts`:
- Line 44: Do not silently swallow errors on reading the response body: replace
the chained .catch(() => "") on res.text() with an explicit try/catch so any
read error is captured and recorded (e.g., let errBody = ""; try { errBody =
await res.text(); } catch (e) { errBody = `<unreadable body: ${String(e)}>`; /*
log e via your logger or include in thrown Error */ }). Ensure you include the
caught error details (String(e) or e.message) in the subsequent log or thrown
error instead of discarding it; reference res.text() and the errBody variable
when making this change.
---
Outside diff comments:
In `@apps/backend/src/lib/ai/tools/docs.ts`:
- Around line 28-41: The fetch in postDocsToolAction can hang forever; modify
postDocsToolAction (and its callers such as createDocsTools.execute) to accept
an AbortSignal parameter, create a local AbortController with a short timeout,
combine that controller.signal with the incoming signal (use
AbortSignal.any([incomingSignal, timeoutController.signal]) when available or
manually listen to abort events), pass the combined signal into fetch via the
request's signal option, clear the timeout on completion, and propagate abort
errors appropriately; ensure you reference postDocsToolAction and the
createDocsTools.execute call sites so the signal is forwarded through the tool
execution path.
---
Nitpick comments:
In `@apps/backend/src/lib/ai/tools/docs.ts`:
- Around line 49-55: Replace the unsafe cast of res.json() to DocsToolHttpResult
with Zod runtime validation: define a Zod schema for DocsToolHttpResult at
module scope (near the existing type), then call schema.parse or
schema.safeParse on the parsed JSON instead of using "as DocsToolHttpResult"; on
parse failure call captureError("docs-tools-parse-error", error) (preserving the
existing error handling pattern) and throw the same user-facing Error, otherwise
assign the validated result to the local variable data so all downstream uses
(data, DocsToolHttpResult) are strongly validated at runtime.
In `@packages/template/src/dev-tool/dev-tool-core.ts`:
- Around line 2310-2322: closePanel currently calls state.update({ isOpen: false
}) but togglePanel also updates state, causing duplicate listener invocations;
make closePanel the single owner of closing state by keeping state.update({
isOpen: false }) inside closePanel and removing the duplicate state.update call
from togglePanel (and any other callers that call closePanel), so togglePanel
should simply call closePanel() without updating state itself; ensure all paths
that close the panel call closePanel() so the single state update is preserved.
In `@packages/template/src/dev-tool/dev-tool-trigger-position.test.ts`:
- Around line 8-103: Replace explicit numeric expectation objects in the tests
with inline snapshots: for each assertion calling resolveTriggerPosition or
clampTriggerPosition or getSnappedTriggerPlacement (e.g., expect(pos).toEqual({
left: 1000 - 36 - 16, top: 700 - 36 - 16 }) and similar), change the matcher to
expect(...).toMatchInlineSnapshot() and update the snapshot content by running
the test runner to record the snapshot; ensure you convert all occurrences in
this file (tests referencing resolveTriggerPosition, getSnappedTriggerPlacement,
clampTriggerPosition) so they follow the project's snapshot format and
serializer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0eef83ca-97b0-4a48-ba1c-67221ff88e48
📒 Files selected for processing (9)
.claude/CLAUDE-KNOWLEDGE.mdapps/backend/src/lib/ai/tools/docs.tspackages/template/src/dev-tool/dev-tool-core.tspackages/template/src/dev-tool/dev-tool-styles.tspackages/template/src/dev-tool/dev-tool-trigger-position.test.tspackages/template/src/dev-tool/dev-tool-trigger-position.tspackages/template/src/dev-tool/index.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/interfaces/client-app.ts
Greptile SummaryThis PR redesigns the Stack Auth dev tool trigger (icon-only square button with corner snapping instead of a draggable pill with text), simplifies the Overview tab (removes project info, changelog, and SDK version cards), adds a
Confidence Score: 3/5Not safe to merge as-is due to a P1 stale-state regression in refreshUser and several P2 issues in error handling. One clear P1 (stale user UI on unexpected errors — a behavioral regression from the old blanket-catch pattern) and two P2s lower the score below 4. The trigger redesign, StrictMode fix, and position-snapping logic are solid and well-tested. dev-tool-core.ts (refreshUser stale state, loadAuthMethods skeleton pills, isBestEffortOverviewError Safari gap) and apps/backend/src/lib/ai/tools/docs.ts (isError throw semantics). Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isBestEffortOverviewError(error: unknown) { | ||
| if (error instanceof DOMException && error.name === 'AbortError') { | ||
| return true; | ||
| } | ||
| if (error instanceof Error) { | ||
| return error.message.includes('Failed to fetch') || error.message.includes('NetworkError'); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isBestEffortOverviewError matches "Failed to fetch" (Chrome) and "NetworkError" (Firefox), but Safari raises TypeError: Load failed for network failures. A Safari developer using the dev tool while the local backend is starting up will hit a non-best-effort path: loadAuthMethods re-throws (silently swallowed by runAsynchronously, leaving skeleton pills forever), and refreshUser re-throws without resetting currentUser. The card comment explicitly mentions "while the local Stack backend is still booting" as the target scenario, so Safari is a real gap.
Consider adding error.message.includes('Load failed') or using a more browser-agnostic approach such as catching TypeError with no message-content check when the request was clearly a fetch call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/template/src/dev-tool/dev-tool-core.ts
Line: 628-636
Comment:
**Safari network errors not classified as best-effort**
`isBestEffortOverviewError` matches `"Failed to fetch"` (Chrome) and `"NetworkError"` (Firefox), but Safari raises `TypeError: Load failed` for network failures. A Safari developer using the dev tool while the local backend is starting up will hit a non-best-effort path: `loadAuthMethods` re-throws (silently swallowed by `runAsynchronously`, leaving skeleton pills forever), and `refreshUser` re-throws without resetting `currentUser`. The card comment explicitly mentions "while the local Stack backend is still booting" as the target scenario, so Safari is a real gap.
Consider adding `error.message.includes('Load failed')` or using a more browser-agnostic approach such as catching `TypeError` with no message-content check when the request was clearly a fetch call.
How can I resolve this? If you propose a fix, please make it concise.| @@ -382,12 +404,23 @@ function createTrigger(onClick: () => void): HTMLElement { | |||
| const logoSpan = h('span', { className: 'sdt-trigger-logo' }); | |||
| setHtml(logoSpan, STACK_LOGO_SVG); | |||
| btn.appendChild(logoSpan); | |||
There was a problem hiding this comment.
loadAuthMethods leave skeleton pills forever
If app.getProject() throws a non-best-effort error, loadAuthMethods re-throws. Since the caller is runAsynchronously, the error is silently swallowed and authGrid keeps showing skeleton pills with no feedback. Unlike refreshUser, this function is only called once (on mount), so there is no retry — the pills stay indefinitely. Consider ensuring the catch block always renders a fallback error message and reserves the re-throw only for errors that should crash the component (which isn't the case here for a dev tool overlay).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/template/src/dev-tool/dev-tool-core.ts
Line: 383-406
Comment:
**Unexpected errors in `loadAuthMethods` leave skeleton pills forever**
If `app.getProject()` throws a non-best-effort error, `loadAuthMethods` re-throws. Since the caller is `runAsynchronously`, the error is silently swallowed and `authGrid` keeps showing skeleton pills with no feedback. Unlike `refreshUser`, this function is only called once (on mount), so there is no retry — the pills stay indefinitely. Consider ensuring the `catch` block always renders a fallback error message and reserves the re-throw only for errors that should crash the component (which isn't the case here for a dev tool overlay).
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/template/src/dev-tool/dev-tool-core.ts (1)
775-806:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkeleton pills remain forever if
loadAuthMethodshits a non-best-effort error.
authGrid.innerHTML = ''runs only after the awaitedgetProject()resolves. In the catch branch, the fallback<div>Could not load auth methods</div>is set only for best-effort errors; non-best-effort errors re-throw before any UI update, so the three skeleton pills built at L770–772 stay visible indefinitely while the real error is only surfaced throughrunAsynchronouslylogging.This same shape repeats in
refreshUser(L862–892): a non-best-effort throw on the first poll keeps the Identity card stuck on "Loading…" / "?". Either render the fallback UI before re-throwing, or render it in afinally-equivalent path so users always see a deterministic post-load state.♻️ Suggested fix for `loadAuthMethods` (apply the same pattern to `refreshUser`)
async function loadAuthMethods() { try { const project = await app.getProject(); authGrid.innerHTML = ''; // …build pills… } catch (error) { + authGrid.innerHTML = '<div style="font-size:11px;color:var(--sdt-text-tertiary)">Could not load auth methods</div>'; if (!isBestEffortOverviewError(error)) { throw error; } - authGrid.innerHTML = '<div style="font-size:11px;color:var(--sdt-text-tertiary)">Could not load auth methods</div>'; } }🤖 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 `@packages/template/src/dev-tool/dev-tool-core.ts` around lines 775 - 806, loadAuthMethods leaves skeleton pills if getProject() throws a non-best-effort error because authGrid.innerHTML = '' runs only after the await; move the UI-reset and/or failure placeholder to always run on error: clear authGrid (authGrid.innerHTML = '') before awaiting app.getProject() or add a finally/catch path that sets the fallback message into authGrid before re-throwing non-best-effort errors; apply the same change to refreshUser so both functions always replace the skeleton pills with either the real pills or the "Could not load ..." fallback even when throwing non-best-effort errors (references: loadAuthMethods, refreshUser, authGrid, app.getProject, isBestEffortOverviewError).
🤖 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 `@packages/template/src/dev-tool/dev-tool-core.ts`:
- Around line 628-636: The network-error detection in isBestEffortOverviewError
is too narrow and misses Safari/WebKit fetch failures (they surface as
TypeError: "Load failed"); update isBestEffortOverviewError to also treat
TypeError instances as best-effort network errors (or at least check for
TypeError with message containing "Load failed"), while keeping the existing
checks for DOMException AbortError and Error messages ("Failed to fetch",
"NetworkError"); modify the function isBestEffortOverviewError to return true
for error instanceof TypeError or when Error.message contains those
network-related substrings so Safari paths are tolerated like Chrome/Firefox.
---
Outside diff comments:
In `@packages/template/src/dev-tool/dev-tool-core.ts`:
- Around line 775-806: loadAuthMethods leaves skeleton pills if getProject()
throws a non-best-effort error because authGrid.innerHTML = '' runs only after
the await; move the UI-reset and/or failure placeholder to always run on error:
clear authGrid (authGrid.innerHTML = '') before awaiting app.getProject() or add
a finally/catch path that sets the fallback message into authGrid before
re-throwing non-best-effort errors; apply the same change to refreshUser so both
functions always replace the skeleton pills with either the real pills or the
"Could not load ..." fallback even when throwing non-best-effort errors
(references: loadAuthMethods, refreshUser, authGrid, app.getProject,
isBestEffortOverviewError).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba88192c-94c5-4634-8ddb-52e71192b4c0
📒 Files selected for processing (4)
.claude/CLAUDE-KNOWLEDGE.mdpackages/template/src/dev-tool/dev-tool-core.tspackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/interfaces/client-app.ts
✅ Files skipped from review due to trivial changes (1)
- .claude/CLAUDE-KNOWLEDGE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
| function isBestEffortOverviewError(error: unknown) { | ||
| if (error instanceof DOMException && error.name === 'AbortError') { | ||
| return true; | ||
| } | ||
| if (error instanceof Error) { | ||
| return error.message.includes('Failed to fetch') || error.message.includes('NetworkError'); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What error message and class does fetch() throw on network failure in Safari vs Chrome vs Firefox?
💡 Result:
Fetch network failure errors (what you see from fetch rejection / error.message) differ by browser: 1) Safari (WebKit) - Error class: TypeError [1] - Error message: "Load failed" [1][2] 2) Chrome (Chromium) - Error class: TypeError [3] - Error message: "Failed to fetch" [3] 3) Firefox (Gecko) - Error class: TypeError [4] - Error message: "NetworkError when attempting to fetch resource." [4] Note: fetch rejects on network failure (or CORS/misconfiguration that prevents the request from completing) and uses a generic error value for the rejection reason; the exact message is implementation/browser-specific. [5]
Citations:
- 1: https://stackoverflow.com/questions/71280168/javascript-typeerror-load-failed-error-when-calling-fetch-on-ios
- 2: https://stackoverflow.com/questions/78862195/fetch-request-load-failed-error-in-safari-on-ios-devices-stimulus
- 3: https://stackoverflow.com/questions/45383874/fetch-api-yields-typeerror-failed-to-fetch
- 4: TypeError: NetworkError when attempting to fetch resource - firefox issue JakeChampion/fetch#310
- 5: https://stackoverflow.com/questions/61113344/fetch-api-how-to-determine-if-an-error-is-a-network-error
🏁 Script executed:
# Confirm the function exists at the specified lines and check surrounding context
sed -n '625,640p' packages/template/src/dev-tool/dev-tool-core.tsRepository: hexclave/stack-auth
Length of output: 823
🏁 Script executed:
# Search for usage of isBestEffortOverviewError
rg -n 'isBestEffortOverviewError' packages/template/src/dev-tool/dev-tool-core.ts -A 2 -B 2Repository: hexclave/stack-auth
Length of output: 562
🏁 Script executed:
# Check if there are any other catch handlers or error patterns related to fetch
rg -n 'catch.*error|Failed to fetch|LoadError|NetworkError' packages/template/src/dev-tool/dev-tool-core.ts -A 3 | head -60Repository: hexclave/stack-auth
Length of output: 672
Brittle network error detection — Safari paths slip through.
The isBestEffortOverviewError check matches Failed to fetch (Chrome/Edge) and NetworkError (Firefox), but Safari and WebKit-based browsers raise TypeError: Load failed, which is not matched. In Safari, a transient backend boot or offline blip during getProject() or getUser() will be classified as severe, re-thrown before clearing the loading state, and surface via runAsynchronously's logger — while the same condition is silently tolerated on Chromium/Firefox.
Consider broadening the error detection to cover Safari's error message, or checking by error type (all browsers raise TypeError for fetch network failures).
🛡️ Suggested fix
function isBestEffortOverviewError(error: unknown) {
if (error instanceof DOMException && error.name === 'AbortError') {
return true;
}
+ // `fetch` raises a TypeError for network-layer failures across browsers.
+ if (error instanceof TypeError) {
+ return true;
+ }
if (error instanceof Error) {
- return error.message.includes('Failed to fetch') || error.message.includes('NetworkError');
+ const m = error.message;
+ return m.includes('Failed to fetch')
+ || m.includes('NetworkError')
+ || m.includes('Load failed')
+ || m.includes('network connection');
}
return false;
}📝 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.
| function isBestEffortOverviewError(error: unknown) { | |
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| return true; | |
| } | |
| if (error instanceof Error) { | |
| return error.message.includes('Failed to fetch') || error.message.includes('NetworkError'); | |
| } | |
| return false; | |
| } | |
| function isBestEffortOverviewError(error: unknown) { | |
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| return true; | |
| } | |
| // `fetch` raises a TypeError for network-layer failures across browsers. | |
| if (error instanceof TypeError) { | |
| return true; | |
| } | |
| if (error instanceof Error) { | |
| const m = error.message; | |
| return m.includes('Failed to fetch') | |
| || m.includes('NetworkError') | |
| || m.includes('Load failed') | |
| || m.includes('network connection'); | |
| } | |
| return false; | |
| } |
🤖 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 `@packages/template/src/dev-tool/dev-tool-core.ts` around lines 628 - 636, The
network-error detection in isBestEffortOverviewError is too narrow and misses
Safari/WebKit fetch failures (they surface as TypeError: "Load failed"); update
isBestEffortOverviewError to also treat TypeError instances as best-effort
network errors (or at least check for TypeError with message containing "Load
failed"), while keeping the existing checks for DOMException AbortError and
Error messages ("Failed to fetch", "NetworkError"); modify the function
isBestEffortOverviewError to return true for error instanceof TypeError or when
Error.message contains those network-related substrings so Safari paths are
tolerated like Chrome/Firefox.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| top: clamp(placement.offset, 0, bounds.maxTop), | ||
| }; | ||
| case 'top-right': { | ||
| return { left: viewport.width - triggerSize.width - m, top: m }; |
There was a problem hiding this comment.
resolveTriggerPosition returns negative left/top values on small viewports, placing trigger off-screen
| return true; | ||
| } | ||
| if (error instanceof Error) { | ||
| return error.message.includes('Failed to fetch') || error.message.includes('NetworkError'); |
There was a problem hiding this comment.
| return error.message.includes('Failed to fetch') || error.message.includes('NetworkError'); | |
| return error.message.includes('Failed to fetch') || error.message.includes('NetworkError') || error.message.includes('Load failed'); |
Safari network errors ("Load failed") are not matched by isBestEffortOverviewError, causing them to propagate and hide auth methods instead of gracefully handling network timeouts
| authGrid.innerHTML = '<div style="font-size:11px;color:var(--sdt-text-tertiary)">Could not load config</div>'; | ||
| }) | ||
| ); | ||
| } catch (error) { |
There was a problem hiding this comment.
Unexpected errors in loadAuthMethods are re-thrown and swallowed by runAsynchronously, leaving skeleton pills forever without error feedback to user
| left: window.innerWidth - 76 - 16, | ||
| top: window.innerHeight - 36 - 16, | ||
| left: window.innerWidth - triggerSize.width - 16, | ||
| top: window.innerHeight - triggerSize.height - 16, |
There was a problem hiding this comment.
defaultPos function is unused dead code - default trigger placement is hard-coded instead
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/template/src/dev-tool/dev-tool-core.ts`:
- Around line 870-874: The "Auth method active" check currently hardcodes ok:
true; change it to compute its value from the actual auth methods state returned
by loadAuthMethods (or the cached authMethods result) so the checklist reflects
reality: call or subscribe to loadAuthMethods (or use the existing authMethods
variable/state), derive a boolean like authMethods.someEnabled, and set the
check's ok accordingly, and ensure the checks array (or UI that renders it) is
updated after loadAuthMethods resolves/cached so the "Auth method active" row
changes when methods are loaded; refer to the checks array and
loadAuthMethods/authMethods/currentUser identifiers when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2169524-72bb-41d9-bbbb-b9ca46689867
📒 Files selected for processing (3)
.claude/CLAUDE-KNOWLEDGE.mdpackages/template/src/dev-tool/dev-tool-core.tspackages/template/src/dev-tool/dev-tool-styles.ts
✅ Files skipped from review due to trivial changes (1)
- .claude/CLAUDE-KNOWLEDGE.md
| const checks = [ | ||
| { ok: !!projectId && projectId !== 'default', label: 'Project' }, | ||
| { ok: true, label: 'Provider' }, | ||
| { ok: !!currentUser, label: 'Auth' }, | ||
| { ok: !!projectId && projectId !== 'default', label: 'Project configured', hint: null }, | ||
| { ok: true, label: 'Auth method active', hint: null }, | ||
| { ok: !!currentUser, label: 'Sign in a test user', hint: 'Use \u201cQuick Sign In\u201d above \u2192' }, | ||
| ]; |
There was a problem hiding this comment.
Hard-coded ok: true makes the "Auth method active" check meaningless.
The setup checklist always reports this as passing regardless of project configuration. If a project genuinely has all auth methods disabled (e.g. credential, magic link, passkey, OAuth all off, sign-up off), the checklist will still indicate setup is complete on this dimension.
Consider deriving this from the same project config that loadAuthMethods already fetches (e.g. cache the result and update the checklist when methods load), so the row reflects reality instead of acting as a placeholder.
🤖 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 `@packages/template/src/dev-tool/dev-tool-core.ts` around lines 870 - 874, The
"Auth method active" check currently hardcodes ok: true; change it to compute
its value from the actual auth methods state returned by loadAuthMethods (or the
cached authMethods result) so the checklist reflects reality: call or subscribe
to loadAuthMethods (or use the existing authMethods variable/state), derive a
boolean like authMethods.someEnabled, and set the check's ok accordingly, and
ensure the checks array (or UI that renders it) is updated after loadAuthMethods
resolves/cached so the "Auth method active" row changes when methods are loaded;
refer to the checks array and loadAuthMethods/authMethods/currentUser
identifiers when making the change.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation