fix(frontend): centralize cookie credentials in fetchWrapper and route callers through it - #322
Open
chriswritescode-dev wants to merge 1 commit into
Open
Conversation
…e callers through it
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAuthentication, file, speech, and event-stream requests now use shared frontend API wrappers with consistent credentials, timeout, cancellation, and error handling. New helpers and tests cover authentication, file operations, STT/TTS behavior, and browser transport. Configuration documentation describes same-origin and cookie constraints. ChangesFrontend request and API centralization
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
frontend/src/contexts/TTSContext.tsx (1)
135-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile string-matching for fallback error messages.
isFallbackcompareserr.messageagainst hardcoded strings ('Request failed','An error occurred') produced elsewhere in the fetch stack. If that wording changes, the 401/429/5xx friendly messages silently stop applying and users see the raw generic string.♻️ Suggested approach: key off status instead of message text
if (err instanceof FetchError) { const status = err.statusCode ?? 0 - const backendMessage = err.message - const isFallback = backendMessage === 'Request failed' || backendMessage === 'An error occurred' || !backendMessage - let errorMessage = backendMessage - if (isFallback) { - if (status === 401) errorMessage = 'Invalid API key' - else if (status === 429) errorMessage = 'Rate limit exceeded' - else if (status >= 500) errorMessage = 'Service unavailable' - } + let errorMessage = err.message + if (status === 401) errorMessage = 'Invalid API key' + else if (status === 429) errorMessage = 'Rate limit exceeded' + else if (status >= 500) errorMessage = 'Service unavailable' throw new Error(errorMessage) }Adjust if a distinct backend-provided message should still take precedence for these statuses.
🤖 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 `@frontend/src/contexts/TTSContext.tsx` around lines 135 - 146, Update the FetchError handling in the TTS error path to determine friendly 401, 429, and 5xx messages from statusCode rather than matching hardcoded err.message strings. Preserve distinct backend-provided messages where they should take precedence, while ensuring generic or absent messages still receive the status-specific fallback.
🤖 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 `@docs/configuration/authentication.md`:
- Line 180: Update the cross-site authentication note to accurately reflect the
settings in the auth configuration: `advanced.useSecureCookies` does not
override the default `SameSite=Lax`, so state that cross-site access is
unsupported unless the cookie configuration explicitly uses `sameSite: 'none'`
with `secure: true`. Clarify whether that configuration is supported, without
implying that CORS or client credentials alone enables cross-site cookies.
In `@frontend/src/api/authInfo.ts`:
- Around line 34-39: Propagate fetchWrapper failures from listUserPasskeys
instead of converting them to an empty array; update
frontend/src/components/settings/AccountSettings.tsx lines 27-31 to render React
Query’s error state for failed loads, and update
frontend/src/api/authInfo.test.ts lines 83-91 to assert rejection/error handling
rather than an empty-list fallback.
In `@frontend/src/api/files.ts`:
- Around line 90-96: Update writeFileEntry and renameFileEntry to pass timeout:
0 in their fetchWrapperVoid request options, disabling the default timeout for
both file mutations while preserving their existing methods, headers, and
bodies.
---
Nitpick comments:
In `@frontend/src/contexts/TTSContext.tsx`:
- Around line 135-146: Update the FetchError handling in the TTS error path to
determine friendly 401, 429, and 5xx messages from statusCode rather than
matching hardcoded err.message strings. Preserve distinct backend-provided
messages where they should take precedence, while ensuring generic or absent
messages still receive the status-specific fallback.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 234e849e-ff82-4c66-912a-b5ef3382ebfa
📒 Files selected for processing (21)
docs/configuration/authentication.mddocs/configuration/environment.mdfrontend/src/api/authInfo.test.tsfrontend/src/api/authInfo.tsfrontend/src/api/fetchWrapper.test.tsfrontend/src/api/fetchWrapper.tsfrontend/src/api/files.test.tsfrontend/src/api/files.tsfrontend/src/api/stt.tsfrontend/src/api/tts.test.tsfrontend/src/api/tts.tsfrontend/src/components/file-browser/FileBrowser.tsxfrontend/src/components/file-browser/FilePreview.tsxfrontend/src/components/settings/AccountSettings.tsxfrontend/src/contexts/AuthContext.tsxfrontend/src/contexts/TTSContext.tsxfrontend/src/hooks/useSTT.test.tsxfrontend/src/hooks/useSTT.tsfrontend/src/lib/auth-loaders.tsfrontend/src/lib/opencode-event-stream/browserTransport.test.tsfrontend/src/lib/opencode-event-stream/browserTransport.ts
| 1. Check AUTH_SECRET is persistent across restarts | ||
| 2. Verify cookies aren't being blocked | ||
| 3. Check AUTH_SECURE_COOKIES setting | ||
| 4. If accessing from a different site (cross-site, not just cross-origin), note that CORS alone is not sufficient: the auth config in `backend/src/auth/index.ts` sets no `sameSite` override, so better-auth's `SameSite=Lax` default prevents the session cookie from being attached to cross-site requests regardless of `credentials: 'include'` on the client. The app is designed same-origin (`VITE_API_URL` defaults to `''`); cross-site access requires changing `sameSite` to `'none'` with `secure: true`, which is an open question rather than a supported configuration. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'sameSite|defaultCookieAttributes|cookieOptions|cookies' backend/src/auth/index.tsRepository: chriswritescode-dev/opencode-manager
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== backend/src/auth/index.ts (exists? + line count) ==\n'
if [ -f backend/src/auth/index.ts ]; then
wc -l backend/src/auth/index.ts
echo
cat -n backend/src/auth/index.ts | sed -n '1,240p'
else
echo "missing"
fi
printf '\n== backend auth-related search ==\n'
rg -n -C3 'sameSite|secure: true|defaultCookieAttributes|cookieOptions|sessionCookie|cookie.*site|better-auth|createAuth|authConfig' backend/src -g '!**/node_modules/**'Repository: chriswritescode-dev/opencode-manager
Length of output: 7645
Clarify the cross-site auth note: backend/src/auth/index.ts only sets advanced.useSecureCookies; it does not override sameSite, so the default SameSite=Lax behavior still blocks cross-site session cookies. If cross-site access isn’t supported, say that explicitly; otherwise document the required sameSite: 'none' + secure: true settings.
🤖 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 `@docs/configuration/authentication.md` at line 180, Update the cross-site
authentication note to accurately reflect the settings in the auth
configuration: `advanced.useSecureCookies` does not override the default
`SameSite=Lax`, so state that cross-site access is unsupported unless the cookie
configuration explicitly uses `sameSite: 'none'` with `secure: true`. Clarify
whether that configuration is supported, without implying that CORS or client
credentials alone enables cross-site cookies.
Comment on lines
+34
to
+39
| export async function listUserPasskeys(): Promise<Passkey[]> { | ||
| try { | ||
| return await fetchWrapper<Passkey[]>(`${API_BASE_URL}/api/auth/passkey/list-user-passkeys`) | ||
| } catch { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not present passkey-load failures as an empty passkey list.
Catching FetchError values—including 401/500, timeout, and the new 499 cancellation—in listUserPasskeys makes Account Settings display “No passkeys registered” for failed requests.
frontend/src/api/authInfo.ts#L34-L39: propagatefetchWrapperfailures instead of returning[].frontend/src/components/settings/AccountSettings.tsx#L27-L31: render React Query’s error state rather than the empty-list message when loading fails.frontend/src/api/authInfo.test.ts#L83-L91: assert rejection/error handling instead of an empty-list fallback.
📍 Affects 3 files
frontend/src/api/authInfo.ts#L34-L39(this comment)frontend/src/components/settings/AccountSettings.tsx#L27-L31frontend/src/api/authInfo.test.ts#L83-L91
🤖 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 `@frontend/src/api/authInfo.ts` around lines 34 - 39, Propagate fetchWrapper
failures from listUserPasskeys instead of converting them to an empty array;
update frontend/src/components/settings/AccountSettings.tsx lines 27-31 to
render React Query’s error state for failed loads, and update
frontend/src/api/authInfo.test.ts lines 83-91 to assert rejection/error handling
rather than an empty-list fallback.
Comment on lines
+90
to
+96
| export async function writeFileEntry(path: string, type: 'file' | 'folder', content?: string): Promise<void> { | ||
| await fetchWrapperVoid(getFileApiUrl(path), { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ type, content }), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Disable the default timeout for all file mutations.
writeFileEntry and renameFileEntry inherit the wrapper’s 30-second timeout, unlike upload and delete. A slow mutation can be reported as failed while the server completes it, leaving the UI stale and retries unreliable. Add timeout: 0 to both calls.
Proposed fix
await fetchWrapperVoid(getFileApiUrl(path), {
method: 'PUT',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ type, content }),
+ timeout: 0,
})
await fetchWrapperVoid(getFileApiUrl(oldPath), {
method: 'PATCH',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ newPath }),
+ timeout: 0,
})Also applies to: 102-108
🤖 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 `@frontend/src/api/files.ts` around lines 90 - 96, Update writeFileEntry and
renameFileEntry to pass timeout: 0 in their fetchWrapperVoid request options,
disabling the default timeout for both file mutations while preserving their
existing methods, headers, and bodies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Centralize
credentials: 'include'in fetchWrapper (with caller-signal abort propagation) and route all frontend auth-cookie fetches through it, removing per-callcredentials/inline-fetch duplication across auth-info, files, TTS/STT, account settings, auth context, file browser, and the opencode event stream transport. Documents the same-origin deployment constraint and the deprecated CORS_ORIGIN / split-origin open question.Summary
Type of Change
Checklist
pnpm lintpasses locallypnpm typecheckpasses locallySummary by CodeRabbit