[Feat]: Credential Management UI for providers#76
Open
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete credentials management system featuring API routes for CRUD operations, type definitions for credentials and providers, utility functions for API communication, and a full-featured settings page with form and provider selection components. The Sidebar navigation is extended to include the new Settings section. Changes
Sequence DiagramsequenceDiagram
participant User
participant CredentialsPage as CredentialsPage
participant ProviderList as ProviderList
participant CredentialForm as CredentialForm
participant APIRoute as /api/credentials/*
participant Backend as Backend API
User->>CredentialsPage: Load credentials settings page
CredentialsPage->>CredentialsPage: Load API key from localStorage
CredentialsPage->>APIRoute: GET /api/credentials/
APIRoute->>Backend: Forward with X-API-KEY
Backend-->>APIRoute: Return credentials list
APIRoute-->>CredentialsPage: { status, data }
CredentialsPage->>ProviderList: Render with providers & credentials
User->>ProviderList: Click provider
ProviderList->>CredentialsPage: onSelect(provider)
CredentialsPage->>CredentialForm: Render with selected provider & form state
User->>CredentialForm: Fill in credential fields
CredentialForm->>CredentialsPage: onValueChange(fieldKey, value)
CredentialsPage->>CredentialsPage: Update formValues state
User->>CredentialForm: Click Save/Update
CredentialsPage->>CredentialsPage: Validate all required fields
CredentialsPage->>APIRoute: POST/PATCH /api/credentials/ with credential data
APIRoute->>Backend: Forward credential submission with X-API-KEY
Backend-->>APIRoute: { status, data }
APIRoute-->>CredentialsPage: Success response
CredentialsPage->>APIRoute: GET /api/credentials/ (refresh)
APIRoute->>Backend: Fetch updated credentials
Backend-->>APIRoute: Updated credentials list
APIRoute-->>CredentialsPage: { status, data }
CredentialsPage->>CredentialsPage: Update credentials state, reset form
CredentialsPage->>User: Show success toast
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
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: 6
🧹 Nitpick comments (6)
app/lib/apiClient.ts (2)
3-3: Consider using a server-only environment variable.
NEXT_PUBLIC_BACKEND_URLexposes the backend URL to the client bundle. SinceapiClientruns exclusively in server route handlers, a non-prefixed env var (e.g.,BACKEND_URL) would keep the internal URL private.Suggested change
-const BACKEND_URL = process.env.NEXT_PUBLIC_BACKEND_URL || "http://localhost:8000"; +const BACKEND_URL = process.env.BACKEND_URL || "http://localhost:8000";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/apiClient.ts` at line 3, The constant BACKEND_URL in app/lib/apiClient.ts uses NEXT_PUBLIC_BACKEND_URL which leaks the URL to the client; change it to read a server-only env var (e.g., BACKEND_URL) instead of the NEXT_PUBLIC_ prefixed one, update the declaration of BACKEND_URL to use process.env.BACKEND_URL with the same fallback, and ensure any call sites that import BACKEND_URL or rely on apiClient continue to work with the new name.
26-27: Guard against non-JSON error responses.If the backend returns a non-JSON response (e.g., HTML error page on 502/504),
response.json()will throw, masking the actual error. Consider defensive parsing:Suggested approach
// 204 No Content has no body - const data = response.status === 204 ? null : await response.json(); + let data = null; + if (response.status !== 204) { + const text = await response.text(); + try { + data = JSON.parse(text); + } catch { + data = { error: text || `Backend returned ${response.status}` }; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/apiClient.ts` around lines 26 - 27, The current parsing line "const data = response.status === 204 ? null : await response.json()" can throw if the response body is non-JSON (HTML error pages), so change it to defensively parse: if status is 204 set data to null, otherwise attempt response.json() inside a try/catch; on JSON parse failure fall back to await response.text() (or include the raw text) and include that fallback along with response.status in the error/return so callers see meaningful info. Update the parsing around the "response" and "data" variables accordingly and ensure any thrown error includes both status and the fallback text.app/settings/credentials/page.tsx (4)
170-182: Add accessible label to sidebar toggle button.The toggle button lacks an accessible name for screen reader users. Add an
aria-labelattribute to describe the button's action.♻️ Proposed fix
<button onClick={() => setSidebarCollapsed(!sidebarCollapsed)} className="p-1.5 rounded-md" style={{ color: colors.text.secondary }} + aria-label={sidebarCollapsed ? "Expand sidebar" : "Collapse sidebar"} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` around lines 170 - 182, The sidebar toggle button lacks an accessible name; update the JSX button (the element using onClick={() => setSidebarCollapsed(!sidebarCollapsed)} and referencing sidebarCollapsed) to include an aria-label that describes the action (e.g., "Collapse sidebar" or "Expand sidebar" based on sidebarCollapsed) so screen readers can announce its purpose; ensure the label text changes or use a single descriptive label like "Toggle sidebar" to reflect the button's functionality.
49-65: Duplicated form population logic with handleCancel.The logic in this effect (lines 49-65) is nearly identical to
handleCancel(lines 132-146). Consider extracting a shared helper function to reduce duplication and ensure consistent behavior.♻️ Suggested refactor
const populateForm = (provider: ProviderDef, existing: Credential | null) => { if (existing) { setExistingCredential(existing); setIsActive(existing.is_active); const populated: Record<string, string> = {}; provider.fields.forEach((f) => { populated[f.key] = existing.credential[f.key] || ""; }); setFormValues(populated); } else { setExistingCredential(null); setIsActive(true); const blank: Record<string, string> = {}; provider.fields.forEach((f) => { blank[f.key] = ""; }); setFormValues(blank); } setVisibleFields(new Set()); };Then use
populateForm(selectedProvider, existing)in both the effect andhandleCancel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` around lines 49 - 65, The form population logic in the useEffect duplicates the logic in handleCancel; extract a shared helper (e.g., populateForm(provider: ProviderDef, existing: Credential | null)) that contains the setExistingCredential, setIsActive, building of populated/blank Record<string,string> and setFormValues, and the setVisibleFields(new Set()); call getExistingForProvider(selectedProvider, credentials) in the effect and call populateForm(selectedProvider, existing) there and replace the matching block in handleCancel to call populateForm(selectedProvider, existing) as well so both use the same implementation.
125-126: Avoidanytype for error handling.Using
err: anybypasses TypeScript's type safety. Preferunknownwith type narrowing for safer error handling.♻️ Proposed fix
- } catch (err: any) { - toast.error(err.message || "Failed to save credentials"); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : "Failed to save credentials"; + toast.error(message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` around lines 125 - 126, Replace the catch clause that types the error as `any` with `unknown` and narrow it before using its message: change `catch (err: any)` to `catch (err: unknown)` and then check `if (err instanceof Error)` to call `toast.error(err.message)` otherwise call `toast.error(String(err) || "Failed to save credentials")`; update the catch block surrounding the `toast.error` call so all branches produce a safe string.
14-14: Consider moving shared exports to a dedicated module.Importing
APIKeyandSTORAGE_KEYfrom a page file creates tight coupling between pages. These should live in a shared location likeapp/lib/types/orapp/lib/constants.tsto improve maintainability and avoid circular dependency risks as the app grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` at line 14, APIKey and STORAGE_KEY are exported from a page file which couples pages; extract these exports into a dedicated shared module (e.g., a new "shared constants/types" module), export APIKey and STORAGE_KEY from there, update the import sites (including the current credentials page and the original keystore page) to import from the new shared module instead of the page, and remove the original exports from the page file so the page no longer serves as a shared module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/credentials/`[credential_id]/route.ts:
- Around line 17-32: The PATCH handler currently calls request.json() before the
try block which can throw on malformed JSON; move the await request.json() call
into the try block inside the PATCH function so JSON parsing errors are caught
by the catch, then use the parsed body in the apiClient call (keep
apiClient(request, `/api/v1/credentials/${credential_id}`, { method: 'PATCH',
body: JSON.stringify(body) }) and the existing NextResponse.json return logic)
and ensure the catch returns the 500 JSON error as before.
- Around line 34-45: The DELETE handler currently always returns 204; instead
call apiClient(request, `/api/v1/credentials/${credential_id}`, { method:
'DELETE' }) and capture its Response (from DELETE), then forward that response's
status and body to the client (e.g., return a NextResponse with the backend
response body and backend response.status; for 204/empty responses ensure you
return null body with the same status). Keep the try/catch around this call and
only return 500 with the error message when an exception is thrown. Ensure you
update references in the DELETE function and use the captured response's status
instead of hardcoding 204.
In `@app/api/credentials/route.ts`:
- Around line 13-24: In POST, move the await request.json() call inside the try
block so malformed JSON is caught; inside the try, first await request.json(),
then call apiClient(request, "/api/v1/credentials/", ...) and return
NextResponse.json(data, { status }); in the catch block detect JSON parse errors
and return a 400 with a clear error message (otherwise keep returning 500 for
other errors) so both request.json() and apiClient failures are handled; update
references to POST, request.json(), apiClient, and NextResponse accordingly.
In `@app/components/settings/credentials/CredentialForm.tsx`:
- Around line 104-121: The visibility toggle button (onToggleVisibility and
field.key) is missing an accessible name; update the button to include a dynamic
aria-label that reflects state (e.g., aria-label={`Hide ${field.key} value`}
when visible is true, otherwise aria-label={`Show ${field.key} value`}) so
screen readers can announce the control and its action; keep existing onClick,
visible logic, and other attributes but ensure the aria-label text includes the
field key or descriptive text to uniquely identify which credential is being
toggled.
In `@app/settings/credentials/page.tsx`:
- Around line 208-210: Replace the plain anchor used for "Keystore" with Next.js
client-side navigation by importing the Link component (import Link from
"next/link") and using the Link component instead of the <a> tag for the element
that renders "Keystore" (the current anchor in the credentials settings page).
Ensure the same className and inline style (colors.text.primary) are applied to
the Link so styling is preserved and remove the plain <a> to avoid a full page
reload.
- Around line 43-46: The useEffect that runs when apiKeys changes references
loadCredentials from outer scope but doesn't list it as a dependency, causing a
stale closure; fix by either (A) moving the loadCredentials function definition
inside the useEffect so it captures the latest apiKeys, or (B) memoizing
loadCredentials with useCallback and including it in the dependency array
(useCallback deps should include apiKeys and any other used state/props), then
update the useEffect dependencies to [apiKeys, loadCredentials]; ensure
consistent references to loadCredentials and apiKeys to avoid exhaustive-deps
warnings.
---
Nitpick comments:
In `@app/lib/apiClient.ts`:
- Line 3: The constant BACKEND_URL in app/lib/apiClient.ts uses
NEXT_PUBLIC_BACKEND_URL which leaks the URL to the client; change it to read a
server-only env var (e.g., BACKEND_URL) instead of the NEXT_PUBLIC_ prefixed
one, update the declaration of BACKEND_URL to use process.env.BACKEND_URL with
the same fallback, and ensure any call sites that import BACKEND_URL or rely on
apiClient continue to work with the new name.
- Around line 26-27: The current parsing line "const data = response.status ===
204 ? null : await response.json()" can throw if the response body is non-JSON
(HTML error pages), so change it to defensively parse: if status is 204 set data
to null, otherwise attempt response.json() inside a try/catch; on JSON parse
failure fall back to await response.text() (or include the raw text) and include
that fallback along with response.status in the error/return so callers see
meaningful info. Update the parsing around the "response" and "data" variables
accordingly and ensure any thrown error includes both status and the fallback
text.
In `@app/settings/credentials/page.tsx`:
- Around line 170-182: The sidebar toggle button lacks an accessible name;
update the JSX button (the element using onClick={() =>
setSidebarCollapsed(!sidebarCollapsed)} and referencing sidebarCollapsed) to
include an aria-label that describes the action (e.g., "Collapse sidebar" or
"Expand sidebar" based on sidebarCollapsed) so screen readers can announce its
purpose; ensure the label text changes or use a single descriptive label like
"Toggle sidebar" to reflect the button's functionality.
- Around line 49-65: The form population logic in the useEffect duplicates the
logic in handleCancel; extract a shared helper (e.g., populateForm(provider:
ProviderDef, existing: Credential | null)) that contains the
setExistingCredential, setIsActive, building of populated/blank
Record<string,string> and setFormValues, and the setVisibleFields(new Set());
call getExistingForProvider(selectedProvider, credentials) in the effect and
call populateForm(selectedProvider, existing) there and replace the matching
block in handleCancel to call populateForm(selectedProvider, existing) as well
so both use the same implementation.
- Around line 125-126: Replace the catch clause that types the error as `any`
with `unknown` and narrow it before using its message: change `catch (err: any)`
to `catch (err: unknown)` and then check `if (err instanceof Error)` to call
`toast.error(err.message)` otherwise call `toast.error(String(err) || "Failed to
save credentials")`; update the catch block surrounding the `toast.error` call
so all branches produce a safe string.
- Line 14: APIKey and STORAGE_KEY are exported from a page file which couples
pages; extract these exports into a dedicated shared module (e.g., a new "shared
constants/types" module), export APIKey and STORAGE_KEY from there, update the
import sites (including the current credentials page and the original keystore
page) to import from the new shared module instead of the page, and remove the
original exports from the page file so the page no longer serves as a shared
module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 745c7718-27f1-4beb-811a-b80c652e455f
📒 Files selected for processing (9)
app/api/credentials/[credential_id]/route.tsapp/api/credentials/route.tsapp/components/Sidebar.tsxapp/components/settings/credentials/CredentialForm.tsxapp/components/settings/credentials/ProviderList.tsxapp/lib/apiClient.tsapp/lib/types/credentials.tsapp/lib/utils.tsapp/settings/credentials/page.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/settings/credentials/page.tsx (1)
54-58:⚠️ Potential issue | 🟡 MinorMissing dependency in useEffect causes stale closure.
loadCredentialsreferencesapiKeysfrom the outer scope but isn't listed as a dependency. Consider wrappingloadCredentialsinuseCallbackwith[apiKeys]as dependencies and adding it to this effect's dependency array, or inline the fetch logic within the effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` around lines 54 - 58, The useEffect that calls loadCredentials while depending on apiKeys can create a stale closure because loadCredentials is not included in the dependency array; update the code so the effect either inlines the fetch logic that references apiKeys or wrap loadCredentials with useCallback([...]) using apiKeys as a dependency and then add loadCredentials to the useEffect dependency list; ensure the symbols mentioned (useEffect, loadCredentials, apiKeys, useCallback) are updated accordingly so React sees correct dependencies and avoids stale closures.
🧹 Nitpick comments (1)
app/settings/credentials/page.tsx (1)
95-96: Consider adding user-facing error feedback on fetch failure.Currently, errors are logged to the console but the user receives no notification when credentials fail to load. They may incorrectly assume no credentials exist.
♻️ Proposed fix
} catch (err) { console.error("Failed to load credentials:", err); + toast.error("Failed to load credentials"); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` around lines 95 - 96, The catch block in app/settings/credentials/page.tsx only does console.error("Failed to load credentials:", err) so users get no feedback; update the error handling in the same async loader (the function that fetches credentials inside the Credentials page component) to set a UI-visible error state (e.g., setLoadError or setFetchError) or invoke the existing toast/notification API, include the err message for context, and render a user-facing banner or error panel in the Credentials page that explains the failure and offers a retry action; replace the silent console.error with this state update/notification so the component displays the error to the user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/settings/credentials/page.tsx`:
- Around line 54-58: The useEffect that calls loadCredentials while depending on
apiKeys can create a stale closure because loadCredentials is not included in
the dependency array; update the code so the effect either inlines the fetch
logic that references apiKeys or wrap loadCredentials with useCallback([...])
using apiKeys as a dependency and then add loadCredentials to the useEffect
dependency list; ensure the symbols mentioned (useEffect, loadCredentials,
apiKeys, useCallback) are updated accordingly so React sees correct dependencies
and avoids stale closures.
---
Nitpick comments:
In `@app/settings/credentials/page.tsx`:
- Around line 95-96: The catch block in app/settings/credentials/page.tsx only
does console.error("Failed to load credentials:", err) so users get no feedback;
update the error handling in the same async loader (the function that fetches
credentials inside the Credentials page component) to set a UI-visible error
state (e.g., setLoadError or setFetchError) or invoke the existing
toast/notification API, include the err message for context, and render a
user-facing banner or error panel in the Credentials page that explains the
failure and offers a retry action; replace the silent console.error with this
state update/notification so the component displays the error to the user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af99d9f9-c497-4095-87b3-8f06a3aefd1b
📒 Files selected for processing (3)
app/api/credentials/[credential_id]/route.tsapp/api/credentials/route.tsapp/settings/credentials/page.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/api/credentials/route.ts
- app/api/credentials/[credential_id]/route.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/settings/credentials/page.tsx (1)
55-58:⚠️ Potential issue | 🟡 MinorStabilize
loadCredentialsusage in the effect dependency chain.Line 57 calls
loadCredentials()from outer scope, but the effect only depends onapiKeys. This is the same stale-closure/exhaustive-deps issue raised earlier.♻️ Proposed fix
-import { useState, useEffect } from "react"; +import { useState, useEffect, useCallback } from "react"; @@ - useEffect(() => { - if (apiKeys.length === 0) return; - loadCredentials(); - }, [apiKeys]); + useEffect(() => { + if (apiKeys.length === 0) return; + loadCredentials(); + }, [apiKeys, loadCredentials]); @@ - const loadCredentials = async () => { + const loadCredentials = useCallback(async () => { setIsLoading(true); try { const data = await apiFetch<{ data?: Credential[] } | Credential[]>( "/api/credentials", apiKeys[0].key, ); setCredentials(Array.isArray(data) ? data : data.data || []); } catch (err) { console.error("Failed to load credentials:", err); } finally { setIsLoading(false); } - }; + }, [apiKeys]);Quick verification:
#!/bin/bash rg -nU -C2 'useEffect\(\(\) => \{\s*if \(apiKeys.length === 0\) return;\s*loadCredentials\(\);\s*\}, \[apiKeys\]\);|const loadCredentials = async' app/settings/credentials/page.tsxAlso applies to: 83-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/settings/credentials/page.tsx` around lines 55 - 58, The effect uses loadCredentials from outer scope but only lists apiKeys, causing a stale-closure; fix by stabilizing loadCredentials (either wrap the existing loadCredentials function in useCallback with dependencies it uses, or move its async body into the useEffect) and then include loadCredentials in the effect dependency array so the hook reads useEffect(() => { if (apiKeys.length === 0) return; loadCredentials(); }, [apiKeys, loadCredentials]);; ensure the same change is applied for the other occurrence around lines 83-96 so useEffect and loadCredentials remain consistent.app/api/credentials/route.ts (1)
13-24:⚠️ Potential issue | 🟡 MinorReturn
400for malformed JSON bodies instead of generic500.Line 15 and Line 28 can throw on invalid JSON, but both handlers currently return
500. That misclassifies client input errors.♻️ Proposed fix
export async function POST(request: NextRequest) { try { const body = await request.json(); @@ - } catch (e: any) { - return NextResponse.json({ error: e.message }, { status: 500 }); + } catch (e: unknown) { + const message = e instanceof Error ? e.message : "Unknown error"; + const status = e instanceof SyntaxError ? 400 : 500; + return NextResponse.json({ error: message }, { status }); } } @@ export async function PATCH(request: NextRequest) { try { const body = await request.json(); @@ - } catch (e: any) { - return NextResponse.json({ error: e.message }, { status: 500 }); + } catch (e: unknown) { + const message = e instanceof Error ? e.message : "Unknown error"; + const status = e instanceof SyntaxError ? 400 : 500; + return NextResponse.json({ error: message }, { status }); } }Also applies to: 26-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/credentials/route.ts` around lines 13 - 24, The POST handler currently treats any error from await request.json() as a 500; update POST (and any other handler in this file that calls request.json) to detect malformed JSON by catching a SyntaxError (or checking error.name === 'SyntaxError') thrown by request.json and return NextResponse.json({ error: 'Malformed JSON' }, { status: 400 }) for that case, while preserving the existing 500 response for other errors; locate the POST function and any other functions that call request.json and implement this targeted error handling around the request.json call and in the catch block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/credentials/`[provider]/route.ts:
- Around line 12-13: The dynamic provider value is interpolated raw into backend
paths (e.g. `/api/v1/credentials/provider/${provider}`) which can break routes
when provider contains reserved URL characters; update the code that builds
these backend URLs (where `provider` is used) to call
encodeURIComponent(provider) before interpolation so the proxied path is always
a valid URL-encoded segment (apply the same change for the second occurrence of
the backend path construction in this file).
In `@app/lib/apiClient.ts`:
- Around line 20-24: The current headers merge places ...(options.headers) last
which allows callers to override "X-API-KEY" and "Content-Type"; update the
merge so the fixed headers win (e.g. move ...(options.headers ?? {}) before the
fixed entries or explicitly assign 'X-API-KEY' and 'Content-Type' after the
spread) in the headers object in app/lib/apiClient.ts (apply the same change to
both occurrences around lines 20-24 and 45-49) so callers cannot override apiKey
or content-type.
In `@app/settings/credentials/page.tsx`:
- Around line 197-201: The sidebar toggle button (the element that calls
setSidebarCollapsed(!sidebarCollapsed) and reads sidebarCollapsed) is missing an
accessible name; add an accessible label (e.g., aria-label="Toggle sidebar" or
aria-label that reflects state like aria-label={sidebarCollapsed ? "Expand
sidebar" : "Collapse sidebar"}) to the button so screen readers can identify its
purpose; optionally also add aria-pressed or title to reflect the collapsed
state for additional accessibility.
---
Duplicate comments:
In `@app/api/credentials/route.ts`:
- Around line 13-24: The POST handler currently treats any error from await
request.json() as a 500; update POST (and any other handler in this file that
calls request.json) to detect malformed JSON by catching a SyntaxError (or
checking error.name === 'SyntaxError') thrown by request.json and return
NextResponse.json({ error: 'Malformed JSON' }, { status: 400 }) for that case,
while preserving the existing 500 response for other errors; locate the POST
function and any other functions that call request.json and implement this
targeted error handling around the request.json call and in the catch block.
In `@app/settings/credentials/page.tsx`:
- Around line 55-58: The effect uses loadCredentials from outer scope but only
lists apiKeys, causing a stale-closure; fix by stabilizing loadCredentials
(either wrap the existing loadCredentials function in useCallback with
dependencies it uses, or move its async body into the useEffect) and then
include loadCredentials in the effect dependency array so the hook reads
useEffect(() => { if (apiKeys.length === 0) return; loadCredentials(); },
[apiKeys, loadCredentials]);; ensure the same change is applied for the other
occurrence around lines 83-96 so useEffect and loadCredentials remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33ecfa72-6c8a-4f9a-b35c-99d9a8abed11
📒 Files selected for processing (4)
app/api/credentials/[provider]/route.tsapp/api/credentials/route.tsapp/lib/apiClient.tsapp/settings/credentials/page.tsx
Comment on lines
+12
to
+13
| `/api/v1/credentials/provider/${provider}`, | ||
| ); |
There was a problem hiding this comment.
Encode dynamic provider before building backend path.
Line 12 and Line 28 interpolate provider directly. If it contains reserved URL characters, the proxied backend route can be malformed or misrouted.
♻️ Proposed fix
export async function GET(
request: NextRequest,
{ params }: { params: Promise<{ provider: string }> },
) {
const { provider } = await params;
+ const providerPath = encodeURIComponent(provider);
try {
const { status, data } = await apiClient(
request,
- `/api/v1/credentials/provider/${provider}`,
+ `/api/v1/credentials/provider/${providerPath}`,
);
return NextResponse.json(data, { status });
} catch (e: any) {
return NextResponse.json({ error: e.message }, { status: 500 });
}
}
@@
export async function DELETE(
request: NextRequest,
{ params }: { params: Promise<{ provider: string }> },
) {
const { provider } = await params;
+ const providerPath = encodeURIComponent(provider);
try {
const { status, data } = await apiClient(
request,
- `/api/v1/credentials/provider/${provider}`,
+ `/api/v1/credentials/provider/${providerPath}`,
{ method: "DELETE" },
);Also applies to: 28-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/credentials/`[provider]/route.ts around lines 12 - 13, The dynamic
provider value is interpolated raw into backend paths (e.g.
`/api/v1/credentials/provider/${provider}`) which can break routes when provider
contains reserved URL characters; update the code that builds these backend URLs
(where `provider` is used) to call encodeURIComponent(provider) before
interpolation so the proxied path is always a valid URL-encoded segment (apply
the same change for the second occurrence of the backend path construction in
this file).
Comment on lines
+20
to
+24
| headers: { | ||
| "X-API-KEY": apiKey, | ||
| "Content-Type": "application/json", | ||
| ...(options.headers ?? {}), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nU --type=ts --type=tsx 'api(Client|Fetch)\([\s\S]{0,250}headers\s*:' appRepository: ProjectTech4DevAI/kaapi-frontend
Length of output: 103
🏁 Script executed:
# First, let's verify the file exists and view the problematic code
cat -n app/lib/apiClient.ts | head -60Repository: ProjectTech4DevAI/kaapi-frontend
Length of output: 2034
🏁 Script executed:
# Search for calls to apiClient and apiFetch with custom headers (fixing the file type filter)
rg -n 'api(Client|Fetch)\(' app --type ts | head -20Repository: ProjectTech4DevAI/kaapi-frontend
Length of output: 805
🏁 Script executed:
# Check the call sites in credentials page
sed -n '120,140p' app/settings/credentials/page.tsxRepository: ProjectTech4DevAI/kaapi-frontend
Length of output: 755
🏁 Script executed:
# Check the call sites in credentials route
cat -n app/api/credentials/route.tsRepository: ProjectTech4DevAI/kaapi-frontend
Length of output: 1498
🏁 Script executed:
# Check the call sites in provider route
cat -n app/api/credentials/\[provider\]/route.tsRepository: ProjectTech4DevAI/kaapi-frontend
Length of output: 1412
Prevent options.headers from overriding X-API-KEY and Content-Type.
Lines 20-24 and 45-49 use object spread to merge caller headers last, allowing X-API-KEY to be replaced. This weakens the security guarantees of these helpers. While current call sites don't exploit this, the design flaw should be fixed defensively.
🔒 Proposed hardening
export async function apiClient(
request: NextRequest | Request,
endpoint: string,
options: RequestInit = {},
) {
const apiKey = request.headers.get("X-API-KEY") || "";
+ const headers = new Headers(options.headers);
+ headers.set("Content-Type", "application/json");
+ headers.set("X-API-KEY", apiKey);
const response = await fetch(`${BACKEND_URL}${endpoint}`, {
...options,
- headers: {
- "X-API-KEY": apiKey,
- "Content-Type": "application/json",
- ...(options.headers ?? {}),
- },
+ headers,
});
@@
export async function apiFetch<T>(
url: string,
apiKey: string,
options: RequestInit = {},
): Promise<T> {
+ const headers = new Headers(options.headers);
+ headers.set("Content-Type", "application/json");
+ headers.set("X-API-KEY", apiKey);
+
const res = await fetch(url, {
...options,
- headers: {
- "X-API-KEY": apiKey,
- "Content-Type": "application/json",
- ...(options.headers ?? {}),
- },
+ headers,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/lib/apiClient.ts` around lines 20 - 24, The current headers merge places
...(options.headers) last which allows callers to override "X-API-KEY" and
"Content-Type"; update the merge so the fixed headers win (e.g. move
...(options.headers ?? {}) before the fixed entries or explicitly assign
'X-API-KEY' and 'Content-Type' after the spread) in the headers object in
app/lib/apiClient.ts (apply the same change to both occurrences around lines
20-24 and 45-49) so callers cannot override apiKey or content-type.
Comment on lines
+197
to
+201
| <button | ||
| onClick={() => setSidebarCollapsed(!sidebarCollapsed)} | ||
| className="p-1.5 rounded-md" | ||
| style={{ color: colors.text.secondary }} | ||
| > |
There was a problem hiding this comment.
Add an accessible label to the icon-only sidebar toggle button.
The button at Line 197 has no accessible name, so assistive tech users can’t identify its purpose.
♿ Proposed fix
<button
+ type="button"
onClick={() => setSidebarCollapsed(!sidebarCollapsed)}
+ aria-label={sidebarCollapsed ? "Expand sidebar" : "Collapse sidebar"}
+ aria-expanded={!sidebarCollapsed}
className="p-1.5 rounded-md"
style={{ color: colors.text.secondary }}
>📝 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.
Suggested change
| <button | |
| onClick={() => setSidebarCollapsed(!sidebarCollapsed)} | |
| className="p-1.5 rounded-md" | |
| style={{ color: colors.text.secondary }} | |
| > | |
| <button | |
| type="button" | |
| onClick={() => setSidebarCollapsed(!sidebarCollapsed)} | |
| aria-label={sidebarCollapsed ? "Expand sidebar" : "Collapse sidebar"} | |
| aria-expanded={!sidebarCollapsed} | |
| className="p-1.5 rounded-md" | |
| style={{ color: colors.text.secondary }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/settings/credentials/page.tsx` around lines 197 - 201, The sidebar toggle
button (the element that calls setSidebarCollapsed(!sidebarCollapsed) and reads
sidebarCollapsed) is missing an accessible name; add an accessible label (e.g.,
aria-label="Toggle sidebar" or aria-label that reflects state like
aria-label={sidebarCollapsed ? "Expand sidebar" : "Collapse sidebar"}) to the
button so screen readers can identify its purpose; optionally also add
aria-pressed or title to reflect the collapsed state for additional
accessibility.
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.
Issue: #74
Summary
Summary by CodeRabbit