Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds example env vars and multiple Next.js App Router API proxy routes for API key and guardrails operations, introduces a Safety Guardrails UI with prompt-editor integration (guardrails persisted in config blobs), new validator components, and sidebar navigation for guardrails. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant UI as Safety Guardrails UI
participant NextAPI as Next.js API Route
participant Backend as Guardrails Backend
User->>Browser: open Safety Guardrails UI / edit validators
Browser->>UI: UI requests (uses X-API-KEY or GUARDRAILS_TOKEN)
UI->>NextAPI: call proxy endpoint (/api/... or /api/apikeys/verify)
rect rgba(100,150,200,0.5)
Note over NextAPI: validate headers/env<br/>forward request to backend
end
NextAPI->>Backend: proxied request (headers/query/body)
Backend-->>NextAPI: response (JSON or empty)
rect rgba(100,150,200,0.5)
Note over NextAPI: parse/handle empty body<br/>return backend status + data
end
NextAPI->>UI: proxied response (propagated status & data)
UI->>Browser: render validators / navigate back with query params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
nishika26
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
app/api/guardrails/validators/configs/[config_id]/route.ts (2)
81-83: Fix inconsistent indentation.Line 82 has extra leading whitespace before the comment.
♻️ Proposed fix
try { - // Get query parameters + // Get query parameters const { searchParams } = new URL(request.url);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/guardrails/validators/configs/`[config_id]/route.ts around lines 81 - 83, The comment inside the try block that precedes URL parsing is over-indented; fix the inconsistent indentation by aligning the comment with the surrounding statements so it matches the try block's indentation level (adjust the line containing "// Get query parameters" immediately above "const { searchParams } = new URL(request.url);" to remove the extra leading whitespace).
68-68: Remove redundant local variable.
backendUrlis already declared at module scope (line 4). This redeclaration shadows it unnecessarily.♻️ Proposed fix
export async function DELETE( request: Request, { params }: { params: Promise<{ config_id: string }> } ) { const { config_id } = await params; - const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8001';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/guardrails/validators/configs/`[config_id]/route.ts at line 68, There is an unnecessary redeclaration of the backendUrl variable in the route where const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8001' shadows the module-scope backendUrl; remove this local declaration and use the existing module-level backendUrl instead (update any references in the route handler to rely on the top-level backendUrl variable).app/api/apikeys/verify/route.ts (1)
31-36: Simplify redundant conditional.Both branches return the same structure. The
if (!response.ok)check can be removed.♻️ Proposed simplification
- // Return the response with the same status code - if (!response.ok) { - return NextResponse.json(data, { status: response.status }); - } - - return NextResponse.json(data, { status: response.status }); + return NextResponse.json(data, { status: response.status });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/apikeys/verify/route.ts` around lines 31 - 36, The conditional checking response.ok is redundant because both branches return the same NextResponse.json(data, { status: response.status }); remove the if (!response.ok) block and replace it with a single direct return statement using NextResponse.json(data, { status: response.status }); so only one return remains; reference the existing response and data variables and the NextResponse.json call when making the change in route.ts.app/api/guardrails/validators/configs/route.ts (1)
56-62: Simplify redundant conditional.Both branches return the same structure with
NextResponse.json(data, { status: response.status }).♻️ Proposed simplification
- // Return the response with the same status code - if (!response.ok) { - console.error('[POST /api/guardrails/validators/configs] Backend error:', response.status, data); - return NextResponse.json(data, { status: response.status }); - } - - return NextResponse.json(data, { status: response.status }); + if (!response.ok) { + console.error('[POST /api/guardrails/validators/configs] Backend error:', response.status, data); + } + return NextResponse.json(data, { status: response.status });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/guardrails/validators/configs/route.ts` around lines 56 - 62, Redundant conditional: remove the duplicate return branches and always return NextResponse.json(data, { status: response.status }); before returning, if response.ok is false log the error as currently done (keep the console.error call referencing response.status and data). Update the code around the response handling in route.ts to drop the if/else and replace with a single return NextResponse.json(...) while preserving the error logging when !response.ok.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 1-3: The .env.example file is missing a trailing newline; update
the file so it ends with a single blank line (i.e., ensure there is a newline
character after the last line containing GUARDRAIL_TOKEN) to satisfy POSIX
newline conventions for files like NEXT_PUBLIC_BACKEND_URL and GUARDRAIL_TOKEN.
- Line 3: Rename the environment variable in .env.example from GUARDRAIL_TOKEN
to GUARDRAILS_TOKEN so it matches the usage in the route handlers; update the
token name in the file to the plural form referenced by the route modules
(app/api/guardrails/validators/configs/route.ts and
app/api/guardrails/validators/configs/[config_id]/route.ts) to prevent the token
from being undefined at runtime.
In `@app/api/apikeys/verify/route.ts`:
- Line 3: The default backend URL in this file uses the variable backendUrl set
to 'http://localhost:8000', which is inconsistent with other route handlers;
update the default to 'http://localhost:8001' by modifying the backendUrl
initialization in route.ts so it matches the rest of the PR and local
development environment.
In `@app/api/guardrails/ban_lists/`[ban_list_id]/route.ts:
- Line 22: The console.log messages in route.ts use inconsistent path names
("ban_list" vs "ban_lists"); update all logging strings in this file that
reference the route (e.g., the console.log at the shown diff and the other logs
referenced around the same handler) to use the plural "ban_lists" to match the
actual route path; search for occurrences of "ban_list" in this module
(including the logs called near the forwarding, response, and error messages)
and replace them with "ban_lists" so all console/debug statements are consistent
with the route.
- Around line 75-76: The console.log call that prints the full request body in
the PUT handler (the line containing "console.log('[PUT
/api/guardrails/ban_list/[ban_list_id]] Body:'...") should not emit full request
bodies in production; remove this unconditional logging and either (a) restrict
it to non-production environments (check process.env.NODE_ENV !== 'production'
before logging), or (b) replace it with a structured logger that redacts
sensitive fields or logs only necessary metadata (e.g., ban_list_id and
operation type). Update the PUT route handler to use one of these approaches so
full request payloads are not written to production logs.
In `@app/api/guardrails/ban_lists/route.ts`:
- Line 18: Log messages in route handler use the singular string '[GET
/api/guardrails/ban_list]' which is inconsistent with the actual route path
'ban_lists'; update all occurrences of the literal 'ban_list' in console.log
(and any other logging calls) within this file to 'ban_lists' so logs match the
route (search for the exact string '[GET /api/guardrails/ban_list]' and similar
variants and replace with '[GET /api/guardrails/ban_lists]'); apply the same
change to the other reported log statements in this file to keep naming
consistent.
- Line 70: Remove the console.log that prints the entire request body in the
POST /api/guardrails/ban_list handler and replace it with a safe log or no-op:
either use the existing structured logger (e.g., processLogger.debug) to log
only non-sensitive metadata (request id, user id, IP) or mask/omit sensitive
fields from the body before logging; alternatively gate the detailed logging
behind a non-production check (NODE_ENV !== 'production') and ensure the symbol
"body" in the route handler is not fully serialized in production.
In `@app/api/guardrails/validators/configs/`[config_id]/route.ts:
- Around line 103-104: In the DELETE handler in route.ts replace the direct call
to response.json() with the same empty-response-safe pattern used elsewhere:
call response.text(), check if the returned text is non-empty, parse it to JSON
only when non-empty, and then return NextResponse.json(parsedData, { status:
response.status }) (or return NextResponse.text('', { status: response.status })
/ an empty JSON response when text is empty) so a 204/empty body won't throw a
SyntaxError; update the block that currently does const data = await
response.json(); return NextResponse.json(data, { status: response.status });
accordingly.
In `@app/api/guardrails/validators/configs/route.ts`:
- Around line 35-36: The route handler for POST
/api/guardrails/validators/configs currently logs the full request body
(console.log('[POST /api/guardrails/validators/configs] Body:',
JSON.stringify(body, null, 2))) which can expose sensitive data; remove that
body logging or wrap it behind a debug flag check (e.g., only log when
process.env.NODE_ENV !== 'production' or a dedicated DEBUG flag) while keeping
minimal non-sensitive logs like the url; locate the console.log lines in
route.ts and replace the direct body log with a conditional log guarded by the
chosen env/debug variable referencing the body and url variables.
---
Nitpick comments:
In `@app/api/apikeys/verify/route.ts`:
- Around line 31-36: The conditional checking response.ok is redundant because
both branches return the same NextResponse.json(data, { status: response.status
}); remove the if (!response.ok) block and replace it with a single direct
return statement using NextResponse.json(data, { status: response.status }); so
only one return remains; reference the existing response and data variables and
the NextResponse.json call when making the change in route.ts.
In `@app/api/guardrails/validators/configs/`[config_id]/route.ts:
- Around line 81-83: The comment inside the try block that precedes URL parsing
is over-indented; fix the inconsistent indentation by aligning the comment with
the surrounding statements so it matches the try block's indentation level
(adjust the line containing "// Get query parameters" immediately above "const {
searchParams } = new URL(request.url);" to remove the extra leading whitespace).
- Line 68: There is an unnecessary redeclaration of the backendUrl variable in
the route where const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL ||
'http://localhost:8001' shadows the module-scope backendUrl; remove this local
declaration and use the existing module-level backendUrl instead (update any
references in the route handler to rely on the top-level backendUrl variable).
In `@app/api/guardrails/validators/configs/route.ts`:
- Around line 56-62: Redundant conditional: remove the duplicate return branches
and always return NextResponse.json(data, { status: response.status }); before
returning, if response.ok is false log the error as currently done (keep the
console.error call referencing response.status and data). Update the code around
the response handling in route.ts to drop the if/else and replace with a single
return NextResponse.json(...) while preserving the error logging when
!response.ok.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b3d6867-6b23-4bf3-9bf6-137bc2e8c5ab
📒 Files selected for processing (6)
.env.exampleapp/api/apikeys/verify/route.tsapp/api/guardrails/ban_lists/[ban_list_id]/route.tsapp/api/guardrails/ban_lists/route.tsapp/api/guardrails/validators/configs/[config_id]/route.tsapp/api/guardrails/validators/configs/route.ts
.env.example
Outdated
Comment on lines
+1
to
+3
| NEXT_PUBLIC_BACKEND_URL= | ||
|
|
||
| GUARDRAIL_TOKEN= No newline at end of file |
There was a problem hiding this comment.
Add trailing newline for POSIX compliance.
Per static analysis, the file is missing a blank line at the end. This is a minor formatting issue but good practice for POSIX-compliant text files.
Proposed fix
NEXT_PUBLIC_BACKEND_URL=
-GUARDRAIL_TOKEN=
+GUARDRAILS_TOKEN=
+📝 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
| NEXT_PUBLIC_BACKEND_URL= | |
| GUARDRAIL_TOKEN= | |
| NEXT_PUBLIC_BACKEND_URL= | |
| GUARDRAILS_TOKEN= | |
Suggested change
| NEXT_PUBLIC_BACKEND_URL= | |
| GUARDRAIL_TOKEN= | |
| NEXT_PUBLIC_BACKEND_URL= | |
| GUARDRAIL_TOKEN= | |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 3-3: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 1 - 3, The .env.example file is missing a trailing
newline; update the file so it ends with a single blank line (i.e., ensure there
is a newline character after the last line containing GUARDRAIL_TOKEN) to
satisfy POSIX newline conventions for files like NEXT_PUBLIC_BACKEND_URL and
GUARDRAIL_TOKEN.
| @@ -0,0 +1,44 @@ | |||
| import { NextRequest, NextResponse } from 'next/server'; | |||
|
|
|||
| const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8000'; | |||
There was a problem hiding this comment.
Inconsistent default backend port.
This file defaults to http://localhost:8000, while all other route handlers in this PR default to http://localhost:8001. This inconsistency could cause confusion during local development.
Proposed fix for consistency
-const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8000';
+const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8001';📝 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
| const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8000'; | |
| const backendUrl = process.env.NEXT_PUBLIC_BACKEND_URL || 'http://localhost:8001'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/apikeys/verify/route.ts` at line 3, The default backend URL in this
file uses the variable backendUrl set to 'http://localhost:8000', which is
inconsistent with other route handlers; update the default to
'http://localhost:8001' by modifying the backendUrl initialization in route.ts
so it matches the rest of the PR and local development environment.
| try { | ||
| const url = `${backendUrl}/api/v1/guardrails/ban_lists/${ban_list_id}`; | ||
|
|
||
| console.log('[GET /api/guardrails/ban_lists/[ban_list_id]] Forwarding to:', url); |
There was a problem hiding this comment.
Inconsistent log paths within the same file.
Line 22 uses ban_lists (plural) but lines 31, 37, 40 use ban_list (singular). Standardize to plural for consistency with the route path.
Proposed fix: Standardize to plural form
Apply consistent naming across all log statements. For example:
- console.log('[GET /api/guardrails/ban_list/[ban_list_id]] Backend response status:', response.status, response.statusText);
+ console.log('[GET /api/guardrails/ban_lists/[ban_list_id]] Backend response status:', response.status, response.statusText);Similar fixes needed for lines 37, 40, 74-76, 86, 88, 92, 95, 126, 136, 142, 145.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/ban_lists/`[ban_list_id]/route.ts at line 22, The
console.log messages in route.ts use inconsistent path names ("ban_list" vs
"ban_lists"); update all logging strings in this file that reference the route
(e.g., the console.log at the shown diff and the other logs referenced around
the same handler) to use the plural "ban_lists" to match the actual route path;
search for occurrences of "ban_list" in this module (including the logs called
near the forwarding, response, and error messages) and replace them with
"ban_lists" so all console/debug statements are consistent with the route.
Comment on lines
+75
to
+76
| console.log('[PUT /api/guardrails/ban_list/[ban_list_id]] Body:', JSON.stringify(body, null, 2)); | ||
|
|
There was a problem hiding this comment.
Avoid logging full request body in production.
Same concern as other routes—logging complete request bodies could expose sensitive data in production logs.
Proposed fix
console.log('[PUT /api/guardrails/ban_lists/[ban_list_id]] Forwarding to:', url);
- console.log('[PUT /api/guardrails/ban_list/[ban_list_id]] Body:', JSON.stringify(body, null, 2));📝 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
| console.log('[PUT /api/guardrails/ban_list/[ban_list_id]] Body:', JSON.stringify(body, null, 2)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/ban_lists/`[ban_list_id]/route.ts around lines 75 - 76,
The console.log call that prints the full request body in the PUT handler (the
line containing "console.log('[PUT /api/guardrails/ban_list/[ban_list_id]]
Body:'...") should not emit full request bodies in production; remove this
unconditional logging and either (a) restrict it to non-production environments
(check process.env.NODE_ENV !== 'production' before logging), or (b) replace it
with a structured logger that redacts sensitive fields or logs only necessary
metadata (e.g., ban_list_id and operation type). Update the PUT route handler to
use one of these approaches so full request payloads are not written to
production logs.
|
|
||
| const url = `${backendUrl}/api/v1/guardrails/ban_lists`; | ||
|
|
||
| console.log('[GET /api/guardrails/ban_list] Forwarding to:', url); |
There was a problem hiding this comment.
Inconsistent log path: singular vs plural.
Log messages use ban_list (singular) but the actual route path is ban_lists (plural). This inconsistency could cause confusion when debugging.
Proposed fix: Use consistent plural naming
- console.log('[GET /api/guardrails/ban_list] Forwarding to:', url);
+ console.log('[GET /api/guardrails/ban_lists] Forwarding to:', url);Apply similar fixes to other log statements in this file (lines 29, 35, 39, 69, 70, 82, 88, 92).
📝 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
| console.log('[GET /api/guardrails/ban_list] Forwarding to:', url); | |
| console.log('[GET /api/guardrails/ban_lists] Forwarding to:', url); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/ban_lists/route.ts` at line 18, Log messages in route
handler use the singular string '[GET /api/guardrails/ban_list]' which is
inconsistent with the actual route path 'ban_lists'; update all occurrences of
the literal 'ban_list' in console.log (and any other logging calls) within this
file to 'ban_lists' so logs match the route (search for the exact string '[GET
/api/guardrails/ban_list]' and similar variants and replace with '[GET
/api/guardrails/ban_lists]'); apply the same change to the other reported log
statements in this file to keep naming consistent.
| const url = `${backendUrl}/api/v1/guardrails/ban_lists`; | ||
|
|
||
| console.log('[POST /api/guardrails/ban_list] Forwarding to:', url); | ||
| console.log('[POST /api/guardrails/ban_list] Body:', JSON.stringify(body, null, 2)); |
There was a problem hiding this comment.
Avoid logging full request body in production.
Same concern as the validators/configs route—logging complete request bodies could expose sensitive data.
Proposed fix
console.log('[POST /api/guardrails/ban_lists] Forwarding to:', url);
- console.log('[POST /api/guardrails/ban_list] Body:', JSON.stringify(body, null, 2));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/ban_lists/route.ts` at line 70, Remove the console.log
that prints the entire request body in the POST /api/guardrails/ban_list handler
and replace it with a safe log or no-op: either use the existing structured
logger (e.g., processLogger.debug) to log only non-sensitive metadata (request
id, user id, IP) or mask/omit sensitive fields from the body before logging;
alternatively gate the detailed logging behind a non-production check (NODE_ENV
!== 'production') and ensure the symbol "body" in the route handler is not fully
serialized in production.
Comment on lines
+103
to
+104
| const data = await response.json(); | ||
| return NextResponse.json(data, { status: response.status }); |
There was a problem hiding this comment.
Handle empty responses to avoid JSON parse errors.
The DELETE handler uses response.json() directly, which will throw a SyntaxError if the backend returns an empty body (e.g., 204 No Content). Other handlers in this PR use .text() first to handle this case safely.
🐛 Proposed fix: Use consistent empty-response handling
- const data = await response.json();
- return NextResponse.json(data, { status: response.status });
+ // Handle empty responses (204 No Content, etc.)
+ const text = await response.text();
+ const data = text ? JSON.parse(text) : { success: true };
+ return NextResponse.json(data, { status: response.status });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/validators/configs/`[config_id]/route.ts around lines 103
- 104, In the DELETE handler in route.ts replace the direct call to
response.json() with the same empty-response-safe pattern used elsewhere: call
response.text(), check if the returned text is non-empty, parse it to JSON only
when non-empty, and then return NextResponse.json(parsedData, { status:
response.status }) (or return NextResponse.text('', { status: response.status })
/ an empty JSON response when text is empty) so a 204/empty body won't throw a
SyntaxError; update the block that currently does const data = await
response.json(); return NextResponse.json(data, { status: response.status });
accordingly.
Comment on lines
+35
to
+36
| console.log('[POST /api/guardrails/validators/configs] Forwarding to:', url); | ||
| console.log('[POST /api/guardrails/validators/configs] Body:', JSON.stringify(body, null, 2)); |
There was a problem hiding this comment.
Avoid logging full request body in production.
Logging the complete request body could expose sensitive configuration data in production logs. Consider removing or reducing verbosity, or gating behind a debug flag.
Proposed fix: Remove body logging or gate behind debug mode
console.log('[POST /api/guardrails/validators/configs] Forwarding to:', url);
- console.log('[POST /api/guardrails/validators/configs] Body:', JSON.stringify(body, null, 2));📝 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
| console.log('[POST /api/guardrails/validators/configs] Forwarding to:', url); | |
| console.log('[POST /api/guardrails/validators/configs] Body:', JSON.stringify(body, null, 2)); | |
| console.log('[POST /api/guardrails/validators/configs] Forwarding to:', url); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/validators/configs/route.ts` around lines 35 - 36, The
route handler for POST /api/guardrails/validators/configs currently logs the
full request body (console.log('[POST /api/guardrails/validators/configs]
Body:', JSON.stringify(body, null, 2))) which can expose sensitive data; remove
that body logging or wrap it behind a debug flag check (e.g., only log when
process.env.NODE_ENV !== 'production' or a dedicated DEBUG flag) while keeping
minimal non-sensitive logs like the url; locate the console.log lines in
route.ts and replace the direct body log with a conditional log guarded by the
chosen env/debug variable referencing the body and url variables.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/configurations/prompt-editor/page.tsx (2)
147-158:⚠️ Potential issue | 🟠 MajorPreserve
completion.typein the initial URL-load path.
handleLoadConfigByIdkeepsconfig.type, but this branch rebuilds the blob without it. Deep-linking directly to a non-text config will fall back to'text'on the next save.Suggested fix
const loadedConfigBlob: any = { completion: { provider: targetConfig.provider as any, + type: targetConfig.type, params: { model: targetConfig.modelName, instructions: targetConfig.instructions, temperature: targetConfig.temperature, tools: targetConfig.tools || [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 147 - 158, The loadedConfigBlob construction currently omits completion.type so non-text configs lose their type on save; update the blob creation in the handleLoadConfigById path to include completion.type (e.g., set loadedConfigBlob.completion.type = targetConfig.type or preserve existing config.type) when present, using the same source as other completion fields (targetConfig or the loaded config) so deep-linked non-text configs don't revert to 'text'.
528-557:⚠️ Potential issue | 🟠 MajorUse the config blob as the save source of truth for guardrail ids.
currentConfigBlobalready contains the selectedinput_guardrails/output_guardrails, but save rebuilds them fromsavedValidators, which is async-derived. Saving before those lookups settle can serialize empty or partial arrays and remove guardrails unintentionally.Suggested fix
- const inputGuardrails = savedValidators - .filter(v => (v.config?.stage || 'input') === 'input' && v.validator_config_id && v.enabled !== false) - .map(v => ({ validator_config_id: v.validator_config_id! })); - - const outputGuardrails = savedValidators - .filter(v => v.config?.stage === 'output' && v.validator_config_id && v.enabled !== false) - .map(v => ({ validator_config_id: v.validator_config_id! })); + const inputGuardrails = (currentConfigBlob as any).input_guardrails ?? []; + const outputGuardrails = (currentConfigBlob as any).output_guardrails ?? [];If
enabledneeds to affect persistence here, project that state back intocurrentConfigBlobwhen it changes, then still save from the blob.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 528 - 557, The save currently rebuilds input_guardrails/output_guardrails from the async-derived savedValidators (used in inputGuardrails/outputGuardrails) which can be stale and cause accidental removal; instead use the guardrail arrays already present on currentConfigBlob as the source of truth (currentConfigBlob.input_guardrails and currentConfigBlob.output_guardrails) when constructing configBlob, and if UI toggles change a validator's enabled state ensure that change is projected back into currentConfigBlob (e.g., update currentConfigBlob.savedValidators or its input/output guardrail lists on toggle) so saves always persist the intended guardrail ids rather than relying on savedValidators settling.
🧹 Nitpick comments (3)
app/lib/useConfigs.ts (1)
117-120: Consider adding proper types for guardrails fields.The
(blob as any)cast bypasses type safety. If theConfigBlobtype inapp/configurations/prompt-editor/types.tshas been updated to includeinput_guardrailsandoutput_guardrails, you can remove this cast. Otherwise, consider extending the type definition to avoid runtime surprises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/useConfigs.ts` around lines 117 - 120, Replace the unsafe (blob as any) cast by updating types and using them: add input_guardrails and output_guardrails to the ConfigBlob type in app/configurations/prompt-editor/types.ts (or create an extended interface) so useConfigs.ts can access blob.input_guardrails and blob.output_guardrails with proper typing; then update the code that declares inputGuardrails, outputGuardrails, and guardrailsCount to read from blob.input_guardrails and blob.output_guardrails without casting, ensuring defaulting to [] remains in place for runtime safety.app/components/prompt-editor/ValidatorListPane.tsx (1)
17-17: TODO comment: Add validator tags.This TODO indicates a planned enhancement. Would you like me to open an issue to track this feature?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ValidatorListPane.tsx` at line 17, Remove the vague inline TODO in the ValidatorListPane component and instead create a tracked issue for the "add validator tags" enhancement; then update the comment in ValidatorListPane (the ValidatorListPane React component) to reference the new issue ID or URL (e.g., "TODO: track in ISSUE-123" or GitHub issue link) so future work is discoverable and triaged.app/components/prompt-editor/ConfigEditorPane.tsx (1)
598-598: Inconsistent use of hardcoded color value.Lines 598, 615, and 638 use hardcoded
'#000000'instead ofcolors.text.primary. This breaks consistency with the app's color system and could cause issues if the color scheme changes.Proposed fix
- <span className="text-xs font-semibold" style={{ color: '#000000' }}>File Search</span> + <span className="text-xs font-semibold" style={{ color: colors.text.primary }}>File Search</span>Apply similar changes to lines 615 and 638.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/prompt-editor/ConfigEditorPane.tsx` at line 598, Replace the hardcoded color '#000000' used in the span elements inside ConfigEditorPane.tsx with the theme token colors.text.primary; specifically update the span that renders "File Search" and the other two spans that use className "text-xs font-semibold" and style={{ color: '#000000' }} to style={{ color: colors.text.primary }}; if colors is not in scope in this file, import or access the existing theme/colors object used elsewhere in the component so all three spans consistently use colors.text.primary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 1-4: Remove trailing whitespace from the first two lines,
normalize the assignment for NEXT_PUBLIC_GUARDRAILS_URL by removing spaces
around the = so it reads exactly NEXT_PUBLIC_GUARDRAILS_URL=, ensure all other
keys (NEXT_PUBLIC_BACKEND_URL, GUARDRAILS_TOKEN) use the same no-space format
(KEY=) and add a final newline at the end of the file so the file ends with a
single newline character.
In `@app/api/guardrails/validators/configs/route.ts`:
- Line 49: Remove or gate the full-response console.log in the GET handler for
"/api/guardrails/validators/configs" (the line that prints JSON.stringify(data))
and do the same for the POST handler that logs response data; either delete
those console.log calls or wrap them behind a debug flag (e.g. check NODE_ENV
!== 'production' or a dedicated ENABLE_DEBUG_LOGS env var) so sensitive
validator configuration is not written to production logs, and ensure both
handlers use the same gated logging approach.
In `@app/configurations/prompt-editor/page.tsx`:
- Around line 331-359: The isLoadingFromUrl ref is set true earlier but not
cleared on early returns; update the function containing getApiKey() so that
before each early return you set isLoadingFromUrl.current = false (specifically
in the "no apiKey" branch after getApiKey(), in the "!verifyResponse.ok" branch
after verifyResponse, and in the branch where organizationId or projectId are
missing), and also add a finally block (or ensure the outer try/catch) that sets
isLoadingFromUrl.current = false to guarantee it’s cleared on errors or success.
- Around line 400-422: Compute the new inputGuardrails and outputGuardrails from
validValidators but only commit the update if all fetches succeeded (i.e.,
fetchedValidators has no nulls) to make the hydrate atomic; then call
setSavedValidators and a single setCurrentConfigBlob that sets both
input_guardrails and output_guardrails in one operation, using the new array
when non-empty and otherwise preserving the previous stage value from prev (so
you never partially overwrite one stage when fetches were partial or a stage is
empty). Reference: fetchedValidators, validValidators, setSavedValidators,
setCurrentConfigBlob, inputGuardrails, outputGuardrails.
- Around line 195-298: The useEffect loadValidators currently depends on the
entire currentConfigBlob which causes unnecessary re-runs and N+1 network calls;
instead derive and depend only on the guardrail-related fields. Update the
effect to extract input_guardrails and output_guardrails from currentConfigBlob
(e.g. compute a memoized key like inputGuardrailsJson/outputGuardrailsJson using
JSON.stringify or a shallow-compare) and use those derived values in the effect
dependency array; keep existing logic in loadValidators (references:
currentConfigBlob, useEffect, loadValidators, isLoadingFromUrl,
setSavedValidators, validator_config_id) but only trigger when the
guardrail-derived values change.
In `@app/configurations/safety-guardrails/page.tsx`:
- Around line 999-1001: The current flow deletes an existing validator by
calling handleRemoveValidator(editingValidatorIndex) before saving the
replacement, creating a race where the validator can be lost if the save fails;
change this to either update in place (call a PATCH/PUT API such as an
updateValidator or handleUpdateValidator with editingValidatorIndex and the new
data) or, if update API is unavailable, perform the save first (await the
create/save API call), then on success call
handleRemoveValidator(editingValidatorIndex); wrap the create-then-delete
sequence in a try/catch and implement rollback (delete the newly created entry
or restore the original) on failure so no data is lost.
- Around line 169-173: The useEffect that runs when selectedValidator or
editingValidatorId change calls fetchBanLists but does not list it in the
dependency array; add fetchBanLists to the dependency array of the useEffect
and, if fetchBanLists is defined inline or recreated on every render, wrap it
with useCallback (or move it to stable scope) to prevent stale closures or
infinite re-renders; update references to the useEffect, fetchBanLists,
selectedValidator, and editingValidatorId accordingly so React's exhaustive-deps
is satisfied.
---
Outside diff comments:
In `@app/configurations/prompt-editor/page.tsx`:
- Around line 147-158: The loadedConfigBlob construction currently omits
completion.type so non-text configs lose their type on save; update the blob
creation in the handleLoadConfigById path to include completion.type (e.g., set
loadedConfigBlob.completion.type = targetConfig.type or preserve existing
config.type) when present, using the same source as other completion fields
(targetConfig or the loaded config) so deep-linked non-text configs don't revert
to 'text'.
- Around line 528-557: The save currently rebuilds
input_guardrails/output_guardrails from the async-derived savedValidators (used
in inputGuardrails/outputGuardrails) which can be stale and cause accidental
removal; instead use the guardrail arrays already present on currentConfigBlob
as the source of truth (currentConfigBlob.input_guardrails and
currentConfigBlob.output_guardrails) when constructing configBlob, and if UI
toggles change a validator's enabled state ensure that change is projected back
into currentConfigBlob (e.g., update currentConfigBlob.savedValidators or its
input/output guardrail lists on toggle) so saves always persist the intended
guardrail ids rather than relying on savedValidators settling.
---
Nitpick comments:
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Line 598: Replace the hardcoded color '#000000' used in the span elements
inside ConfigEditorPane.tsx with the theme token colors.text.primary;
specifically update the span that renders "File Search" and the other two spans
that use className "text-xs font-semibold" and style={{ color: '#000000' }} to
style={{ color: colors.text.primary }}; if colors is not in scope in this file,
import or access the existing theme/colors object used elsewhere in the
component so all three spans consistently use colors.text.primary.
In `@app/components/prompt-editor/ValidatorListPane.tsx`:
- Line 17: Remove the vague inline TODO in the ValidatorListPane component and
instead create a tracked issue for the "add validator tags" enhancement; then
update the comment in ValidatorListPane (the ValidatorListPane React component)
to reference the new issue ID or URL (e.g., "TODO: track in ISSUE-123" or GitHub
issue link) so future work is discoverable and triaged.
In `@app/lib/useConfigs.ts`:
- Around line 117-120: Replace the unsafe (blob as any) cast by updating types
and using them: add input_guardrails and output_guardrails to the ConfigBlob
type in app/configurations/prompt-editor/types.ts (or create an extended
interface) so useConfigs.ts can access blob.input_guardrails and
blob.output_guardrails with proper typing; then update the code that declares
inputGuardrails, outputGuardrails, and guardrailsCount to read from
blob.input_guardrails and blob.output_guardrails without casting, ensuring
defaulting to [] remains in place for runtime safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a958863-1344-46de-b479-12ceb9849ecf
📒 Files selected for processing (13)
.env.exampleapp/api/guardrails/ban_lists/[ban_list_id]/route.tsapp/api/guardrails/ban_lists/route.tsapp/api/guardrails/validators/configs/[config_id]/route.tsapp/api/guardrails/validators/configs/route.tsapp/coming-soon/guardrails/page.tsxapp/components/Sidebar.tsxapp/components/prompt-editor/ConfigEditorPane.tsxapp/components/prompt-editor/ValidatorListPane.tsxapp/configurations/prompt-editor/page.tsxapp/configurations/prompt-editor/types.tsapp/configurations/safety-guardrails/page.tsxapp/lib/useConfigs.ts
💤 Files with no reviewable changes (1)
- app/coming-soon/guardrails/page.tsx
✅ Files skipped from review due to trivial changes (3)
- app/components/Sidebar.tsx
- app/configurations/prompt-editor/types.ts
- app/api/guardrails/ban_lists/route.ts
Comment on lines
+1
to
+4
| NEXT_PUBLIC_BACKEND_URL= | ||
| NEXT_PUBLIC_GUARDRAILS_URL = | ||
|
|
||
| GUARDRAILS_TOKEN= No newline at end of file |
There was a problem hiding this comment.
Fix .env.example formatting issues.
Static analysis flagged several formatting problems:
- Lines 1-2 have trailing whitespace
- Line 2 has spaces around the
=sign (should beNEXT_PUBLIC_GUARDRAILS_URL=) - Missing trailing newline at end of file
Some environment variable parsers are sensitive to these formatting issues.
Proposed fix
-NEXT_PUBLIC_BACKEND_URL=
-NEXT_PUBLIC_GUARDRAILS_URL =
+NEXT_PUBLIC_BACKEND_URL=
+NEXT_PUBLIC_GUARDRAILS_URL=
GUARDRAILS_TOKEN=
+📝 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
| NEXT_PUBLIC_BACKEND_URL= | |
| NEXT_PUBLIC_GUARDRAILS_URL = | |
| GUARDRAILS_TOKEN= | |
| NEXT_PUBLIC_BACKEND_URL= | |
| NEXT_PUBLIC_GUARDRAILS_URL= | |
| GUARDRAILS_TOKEN= | |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 1-1: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 1-1: [TrailingWhitespace] Trailing whitespace detected
(TrailingWhitespace)
[warning] 2-2: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 2-2: [TrailingWhitespace] Trailing whitespace detected
(TrailingWhitespace)
[warning] 4-4: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 1 - 4, Remove trailing whitespace from the first
two lines, normalize the assignment for NEXT_PUBLIC_GUARDRAILS_URL by removing
spaces around the = so it reads exactly NEXT_PUBLIC_GUARDRAILS_URL=, ensure all
other keys (NEXT_PUBLIC_BACKEND_URL, GUARDRAILS_TOKEN) use the same no-space
format (KEY=) and add a final newline at the end of the file so the file ends
with a single newline character.
| const text = await response.text(); | ||
| const data = text ? JSON.parse(text) : { success: true, data: [] }; | ||
|
|
||
| console.log('[GET /api/guardrails/validators/configs] Backend response data:', JSON.stringify(data, null, 2)); |
There was a problem hiding this comment.
Avoid logging full response data in production.
Similar to the request body logging concern, logging full response data at lines 49 and 115 could expose sensitive validator configuration in production logs. Consider removing these logs or gating them behind a debug flag.
Proposed fix
- console.log('[GET /api/guardrails/validators/configs] Backend response data:', JSON.stringify(data, null, 2));
+ // Avoid logging full response data in production
+ if (process.env.NODE_ENV !== 'production') {
+ console.log('[GET /api/guardrails/validators/configs] Backend response data:', JSON.stringify(data, null, 2));
+ }Apply similar changes to line 115 for the POST handler.
📝 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
| console.log('[GET /api/guardrails/validators/configs] Backend response data:', JSON.stringify(data, null, 2)); | |
| // Avoid logging full response data in production | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.log('[GET /api/guardrails/validators/configs] Backend response data:', JSON.stringify(data, null, 2)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/guardrails/validators/configs/route.ts` at line 49, Remove or gate
the full-response console.log in the GET handler for
"/api/guardrails/validators/configs" (the line that prints JSON.stringify(data))
and do the same for the POST handler that logs response data; either delete
those console.log calls or wrap them behind a debug flag (e.g. check NODE_ENV
!== 'production' or a dedicated ENABLE_DEBUG_LOGS env var) so sensitive
validator configuration is not written to production logs, and ensure both
handlers use the same gated logging approach.
Comment on lines
+195
to
+298
| // Load validators from config blob | ||
| useEffect(() => { | ||
| const loadValidators = async () => { | ||
| // Skip if we're currently loading from URL parameters | ||
| if (isLoadingFromUrl.current) { | ||
| console.log('[ValidatorLoad] Skipping - loading from URL'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Check if the config blob has guardrails | ||
| const inputGuardrails = (currentConfigBlob as any).input_guardrails || []; | ||
| const outputGuardrails = (currentConfigBlob as any).output_guardrails || []; | ||
|
|
||
| if (inputGuardrails.length === 0 && outputGuardrails.length === 0) { | ||
| setSavedValidators([]); | ||
| return; | ||
| } | ||
|
|
||
| const apiKey = getApiKey(); | ||
| if (!apiKey) { | ||
| console.error('[ValidatorLoad] No API key found'); | ||
| setSavedValidators([]); | ||
| return; | ||
| } | ||
|
|
||
| // Get organization_id and project_id | ||
| const verifyResponse = await fetch('/api/apikeys/verify', { | ||
| method: 'GET', | ||
| headers: { | ||
| 'X-API-KEY': apiKey, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| if (!verifyResponse.ok) { | ||
| console.error('[ValidatorLoad] Failed to verify API key'); | ||
| setSavedValidators([]); | ||
| return; | ||
| } | ||
|
|
||
| const verifyData = await verifyResponse.json(); | ||
| const organizationId = verifyData.data?.organization_id; | ||
| const projectId = verifyData.data?.project_id; | ||
|
|
||
| if (!organizationId || !projectId) { | ||
| console.error('[ValidatorLoad] Could not retrieve organization or project ID'); | ||
| setSavedValidators([]); | ||
| return; | ||
| } | ||
|
|
||
| const queryParams = new URLSearchParams({ | ||
| organization_id: organizationId, | ||
| project_id: projectId, | ||
| }); | ||
|
|
||
| // Fetch all validators from both input and output guardrails | ||
| const allGuardrails = [ | ||
| ...inputGuardrails.map((g: any) => ({ ...g, stage: 'input' })), | ||
| ...outputGuardrails.map((g: any) => ({ ...g, stage: 'output' })), | ||
| ]; | ||
|
|
||
| const validatorPromises = allGuardrails.map(async (guardrail: any) => { | ||
| try { | ||
| const response = await fetch( | ||
| `/api/guardrails/validators/configs/${guardrail.validator_config_id}?${queryParams.toString()}` | ||
| ); | ||
|
|
||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| if (data.success && data.data) { | ||
| // Map backend response to validator format | ||
| return { | ||
| id: data.data.type, | ||
| name: data.data.type.replace(/_/g, ' ').replace(/\b\w/g, (l: string) => l.toUpperCase()), | ||
| description: 'Configured guardrail', | ||
| enabled: data.data.is_enabled !== undefined ? data.data.is_enabled : true, | ||
| validator_config_id: guardrail.validator_config_id, | ||
| config: { | ||
| stage: guardrail.stage, | ||
| type: data.data.type, | ||
| ...data.data, | ||
| } | ||
| }; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`[ValidatorLoad] Failed to fetch validator ${guardrail.validator_config_id}:`, error); | ||
| } | ||
| return null; | ||
| }); | ||
|
|
||
| const fetchedValidators = await Promise.all(validatorPromises); | ||
| const validValidators = fetchedValidators.filter(v => v !== null); | ||
| console.log('[ValidatorLoad] Loaded validators from config blob:', validValidators.length, validValidators); | ||
| setSavedValidators(validValidators); | ||
| } catch (e) { | ||
| console.error('Failed to load validators:', e); | ||
| setSavedValidators([]); | ||
| } | ||
| }; | ||
|
|
||
| loadValidators(); | ||
| }, [currentConfigBlob]); |
There was a problem hiding this comment.
Narrow the validator-hydration dependency.
This effect depends on the whole currentConfigBlob, so changing unrelated config fields replays /api/apikeys/verify and every validator fetch. With guardrails attached, normal editing turns into repeated N+1 requests.
Suggested fix
- }, [currentConfigBlob]);
+ }, [
+ (currentConfigBlob as any).input_guardrails,
+ (currentConfigBlob as any).output_guardrails,
+ ]);📝 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
| // Load validators from config blob | |
| useEffect(() => { | |
| const loadValidators = async () => { | |
| // Skip if we're currently loading from URL parameters | |
| if (isLoadingFromUrl.current) { | |
| console.log('[ValidatorLoad] Skipping - loading from URL'); | |
| return; | |
| } | |
| try { | |
| // Check if the config blob has guardrails | |
| const inputGuardrails = (currentConfigBlob as any).input_guardrails || []; | |
| const outputGuardrails = (currentConfigBlob as any).output_guardrails || []; | |
| if (inputGuardrails.length === 0 && outputGuardrails.length === 0) { | |
| setSavedValidators([]); | |
| return; | |
| } | |
| const apiKey = getApiKey(); | |
| if (!apiKey) { | |
| console.error('[ValidatorLoad] No API key found'); | |
| setSavedValidators([]); | |
| return; | |
| } | |
| // Get organization_id and project_id | |
| const verifyResponse = await fetch('/api/apikeys/verify', { | |
| method: 'GET', | |
| headers: { | |
| 'X-API-KEY': apiKey, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| if (!verifyResponse.ok) { | |
| console.error('[ValidatorLoad] Failed to verify API key'); | |
| setSavedValidators([]); | |
| return; | |
| } | |
| const verifyData = await verifyResponse.json(); | |
| const organizationId = verifyData.data?.organization_id; | |
| const projectId = verifyData.data?.project_id; | |
| if (!organizationId || !projectId) { | |
| console.error('[ValidatorLoad] Could not retrieve organization or project ID'); | |
| setSavedValidators([]); | |
| return; | |
| } | |
| const queryParams = new URLSearchParams({ | |
| organization_id: organizationId, | |
| project_id: projectId, | |
| }); | |
| // Fetch all validators from both input and output guardrails | |
| const allGuardrails = [ | |
| ...inputGuardrails.map((g: any) => ({ ...g, stage: 'input' })), | |
| ...outputGuardrails.map((g: any) => ({ ...g, stage: 'output' })), | |
| ]; | |
| const validatorPromises = allGuardrails.map(async (guardrail: any) => { | |
| try { | |
| const response = await fetch( | |
| `/api/guardrails/validators/configs/${guardrail.validator_config_id}?${queryParams.toString()}` | |
| ); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| if (data.success && data.data) { | |
| // Map backend response to validator format | |
| return { | |
| id: data.data.type, | |
| name: data.data.type.replace(/_/g, ' ').replace(/\b\w/g, (l: string) => l.toUpperCase()), | |
| description: 'Configured guardrail', | |
| enabled: data.data.is_enabled !== undefined ? data.data.is_enabled : true, | |
| validator_config_id: guardrail.validator_config_id, | |
| config: { | |
| stage: guardrail.stage, | |
| type: data.data.type, | |
| ...data.data, | |
| } | |
| }; | |
| } | |
| } | |
| } catch (error) { | |
| console.error(`[ValidatorLoad] Failed to fetch validator ${guardrail.validator_config_id}:`, error); | |
| } | |
| return null; | |
| }); | |
| const fetchedValidators = await Promise.all(validatorPromises); | |
| const validValidators = fetchedValidators.filter(v => v !== null); | |
| console.log('[ValidatorLoad] Loaded validators from config blob:', validValidators.length, validValidators); | |
| setSavedValidators(validValidators); | |
| } catch (e) { | |
| console.error('Failed to load validators:', e); | |
| setSavedValidators([]); | |
| } | |
| }; | |
| loadValidators(); | |
| }, [currentConfigBlob]); | |
| // Load validators from config blob | |
| useEffect(() => { | |
| const loadValidators = async () => { | |
| // Skip if we're currently loading from URL parameters | |
| if (isLoadingFromUrl.current) { | |
| console.log('[ValidatorLoad] Skipping - loading from URL'); | |
| return; | |
| } | |
| try { | |
| // Check if the config blob has guardrails | |
| const inputGuardrails = (currentConfigBlob as any).input_guardrails || []; | |
| const outputGuardrails = (currentConfigBlob as any).output_guardrails || []; | |
| if (inputGuardrails.length === 0 && outputGuardrails.length === 0) { | |
| setSavedValidators([]); | |
| return; | |
| } | |
| const apiKey = getApiKey(); | |
| if (!apiKey) { | |
| console.error('[ValidatorLoad] No API key found'); | |
| setSavedValidators([]); | |
| return; | |
| } | |
| // Get organization_id and project_id | |
| const verifyResponse = await fetch('/api/apikeys/verify', { | |
| method: 'GET', | |
| headers: { | |
| 'X-API-KEY': apiKey, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| if (!verifyResponse.ok) { | |
| console.error('[ValidatorLoad] Failed to verify API key'); | |
| setSavedValidators([]); | |
| return; | |
| } | |
| const verifyData = await verifyResponse.json(); | |
| const organizationId = verifyData.data?.organization_id; | |
| const projectId = verifyData.data?.project_id; | |
| if (!organizationId || !projectId) { | |
| console.error('[ValidatorLoad] Could not retrieve organization or project ID'); | |
| setSavedValidators([]); | |
| return; | |
| } | |
| const queryParams = new URLSearchParams({ | |
| organization_id: organizationId, | |
| project_id: projectId, | |
| }); | |
| // Fetch all validators from both input and output guardrails | |
| const allGuardrails = [ | |
| ...inputGuardrails.map((g: any) => ({ ...g, stage: 'input' })), | |
| ...outputGuardrails.map((g: any) => ({ ...g, stage: 'output' })), | |
| ]; | |
| const validatorPromises = allGuardrails.map(async (guardrail: any) => { | |
| try { | |
| const response = await fetch( | |
| `/api/guardrails/validators/configs/${guardrail.validator_config_id}?${queryParams.toString()}` | |
| ); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| if (data.success && data.data) { | |
| // Map backend response to validator format | |
| return { | |
| id: data.data.type, | |
| name: data.data.type.replace(/_/g, ' ').replace(/\b\w/g, (l: string) => l.toUpperCase()), | |
| description: 'Configured guardrail', | |
| enabled: data.data.is_enabled !== undefined ? data.data.is_enabled : true, | |
| validator_config_id: guardrail.validator_config_id, | |
| config: { | |
| stage: guardrail.stage, | |
| type: data.data.type, | |
| ...data.data, | |
| } | |
| }; | |
| } | |
| } | |
| } catch (error) { | |
| console.error(`[ValidatorLoad] Failed to fetch validator ${guardrail.validator_config_id}:`, error); | |
| } | |
| return null; | |
| }); | |
| const fetchedValidators = await Promise.all(validatorPromises); | |
| const validValidators = fetchedValidators.filter(v => v !== null); | |
| console.log('[ValidatorLoad] Loaded validators from config blob:', validValidators.length, validValidators); | |
| setSavedValidators(validValidators); | |
| } catch (e) { | |
| console.error('Failed to load validators:', e); | |
| setSavedValidators([]); | |
| } | |
| }; | |
| loadValidators(); | |
| }, [ | |
| (currentConfigBlob as any).input_guardrails, | |
| (currentConfigBlob as any).output_guardrails, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/configurations/prompt-editor/page.tsx` around lines 195 - 298, The
useEffect loadValidators currently depends on the entire currentConfigBlob which
causes unnecessary re-runs and N+1 network calls; instead derive and depend only
on the guardrail-related fields. Update the effect to extract input_guardrails
and output_guardrails from currentConfigBlob (e.g. compute a memoized key like
inputGuardrailsJson/outputGuardrailsJson using JSON.stringify or a
shallow-compare) and use those derived values in the effect dependency array;
keep existing logic in loadValidators (references: currentConfigBlob, useEffect,
loadValidators, isLoadingFromUrl, setSavedValidators, validator_config_id) but
only trigger when the guardrail-derived values change.
Comment on lines
+331
to
+359
| const apiKey = getApiKey(); | ||
| if (!apiKey) { | ||
| console.error('[ValidatorURL] No API key found'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Get organization_id and project_id | ||
| const verifyResponse = await fetch('/api/apikeys/verify', { | ||
| method: 'GET', | ||
| headers: { | ||
| 'X-API-KEY': apiKey, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| if (!verifyResponse.ok) { | ||
| console.error('[ValidatorURL] Failed to verify API key'); | ||
| return; | ||
| } | ||
|
|
||
| const verifyData = await verifyResponse.json(); | ||
| const organizationId = verifyData.data?.organization_id; | ||
| const projectId = verifyData.data?.project_id; | ||
|
|
||
| if (!organizationId || !projectId) { | ||
| console.error('[ValidatorURL] Could not retrieve organization or project ID'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Clear isLoadingFromUrl on the early-return paths.
After Line 311 sets this ref to true, the no apiKey, !verifyResponse.ok, and missing org/project branches return without resetting it. From that point on, the config-blob hydrate keeps skipping validator loads until a full refresh.
Suggested fix
const apiKey = getApiKey();
if (!apiKey) {
console.error('[ValidatorURL] No API key found');
+ isLoadingFromUrl.current = false;
return;
}
@@
if (!verifyResponse.ok) {
console.error('[ValidatorURL] Failed to verify API key');
+ isLoadingFromUrl.current = false;
return;
}
@@
if (!organizationId || !projectId) {
console.error('[ValidatorURL] Could not retrieve organization or project ID');
+ isLoadingFromUrl.current = false;
return;
}📝 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
| const apiKey = getApiKey(); | |
| if (!apiKey) { | |
| console.error('[ValidatorURL] No API key found'); | |
| return; | |
| } | |
| try { | |
| // Get organization_id and project_id | |
| const verifyResponse = await fetch('/api/apikeys/verify', { | |
| method: 'GET', | |
| headers: { | |
| 'X-API-KEY': apiKey, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| if (!verifyResponse.ok) { | |
| console.error('[ValidatorURL] Failed to verify API key'); | |
| return; | |
| } | |
| const verifyData = await verifyResponse.json(); | |
| const organizationId = verifyData.data?.organization_id; | |
| const projectId = verifyData.data?.project_id; | |
| if (!organizationId || !projectId) { | |
| console.error('[ValidatorURL] Could not retrieve organization or project ID'); | |
| return; | |
| } | |
| const apiKey = getApiKey(); | |
| if (!apiKey) { | |
| console.error('[ValidatorURL] No API key found'); | |
| isLoadingFromUrl.current = false; | |
| return; | |
| } | |
| try { | |
| // Get organization_id and project_id | |
| const verifyResponse = await fetch('/api/apikeys/verify', { | |
| method: 'GET', | |
| headers: { | |
| 'X-API-KEY': apiKey, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| if (!verifyResponse.ok) { | |
| console.error('[ValidatorURL] Failed to verify API key'); | |
| isLoadingFromUrl.current = false; | |
| return; | |
| } | |
| const verifyData = await verifyResponse.json(); | |
| const organizationId = verifyData.data?.organization_id; | |
| const projectId = verifyData.data?.project_id; | |
| if (!organizationId || !projectId) { | |
| console.error('[ValidatorURL] Could not retrieve organization or project ID'); | |
| isLoadingFromUrl.current = false; | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/configurations/prompt-editor/page.tsx` around lines 331 - 359, The
isLoadingFromUrl ref is set true earlier but not cleared on early returns;
update the function containing getApiKey() so that before each early return you
set isLoadingFromUrl.current = false (specifically in the "no apiKey" branch
after getApiKey(), in the "!verifyResponse.ok" branch after verifyResponse, and
in the branch where organizationId or projectId are missing), and also add a
finally block (or ensure the outer try/catch) that sets isLoadingFromUrl.current
= false to guarantee it’s cleared on errors or success.
Comment on lines
+400
to
+422
| const fetchedValidators = await Promise.all(validatorPromises); | ||
| const validValidators = fetchedValidators.filter(v => v !== null); | ||
|
|
||
| console.log('[ValidatorURL] Fetched validators from URL:', validValidators.length, validValidators); | ||
|
|
||
| if (validValidators.length > 0) { | ||
| setSavedValidators(validValidators); | ||
|
|
||
| // IMPORTANT: Update the currentConfigBlob with the new validators | ||
| // This ensures the changes are detected and can be saved as a new version | ||
| const inputGuardrails = validValidators | ||
| .filter(v => (v.config?.stage || 'input') === 'input' && v.validator_config_id) | ||
| .map(v => ({ validator_config_id: v.validator_config_id })); | ||
|
|
||
| const outputGuardrails = validValidators | ||
| .filter(v => v.config?.stage === 'output' && v.validator_config_id) | ||
| .map(v => ({ validator_config_id: v.validator_config_id })); | ||
|
|
||
| setCurrentConfigBlob(prev => ({ | ||
| ...prev, | ||
| ...(inputGuardrails.length > 0 ? { input_guardrails: inputGuardrails } : {}), | ||
| ...(outputGuardrails.length > 0 ? { output_guardrails: outputGuardrails } : {}), | ||
| })); |
There was a problem hiding this comment.
Make the URL hydrate update atomic and stage-complete.
This branch accepts partial fetch success and then only overwrites non-empty stages. If one lookup fails—or the new selection now only has input or only output validators—you can end up with a mixed blob containing stale or missing guardrails.
Suggested fix
const fetchedValidators = await Promise.all(validatorPromises);
const validValidators = fetchedValidators.filter(v => v !== null);
+
+ if (validValidators.length !== validatorIds.length) {
+ console.error('[ValidatorURL] Incomplete validator hydrate; keeping previous guardrails');
+ isLoadingFromUrl.current = false;
+ return;
+ }
@@
- setCurrentConfigBlob(prev => ({
- ...prev,
- ...(inputGuardrails.length > 0 ? { input_guardrails: inputGuardrails } : {}),
- ...(outputGuardrails.length > 0 ? { output_guardrails: outputGuardrails } : {}),
- }));
+ setCurrentConfigBlob(prev => {
+ const updated = { ...prev };
+
+ if (inputGuardrails.length > 0) {
+ (updated as any).input_guardrails = inputGuardrails;
+ } else {
+ delete (updated as any).input_guardrails;
+ }
+
+ if (outputGuardrails.length > 0) {
+ (updated as any).output_guardrails = outputGuardrails;
+ } else {
+ delete (updated as any).output_guardrails;
+ }
+
+ return updated;
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/configurations/prompt-editor/page.tsx` around lines 400 - 422, Compute
the new inputGuardrails and outputGuardrails from validValidators but only
commit the update if all fetches succeeded (i.e., fetchedValidators has no
nulls) to make the hydrate atomic; then call setSavedValidators and a single
setCurrentConfigBlob that sets both input_guardrails and output_guardrails in
one operation, using the new array when non-empty and otherwise preserving the
previous stage value from prev (so you never partially overwrite one stage when
fetches were partial or a stage is empty). Reference: fetchedValidators,
validValidators, setSavedValidators, setCurrentConfigBlob, inputGuardrails,
outputGuardrails.
Comment on lines
+169
to
+173
| useEffect(() => { | ||
| if ((selectedValidator === 'ban-list' || editingValidatorId === 'ban-list')) { | ||
| fetchBanLists(); | ||
| } | ||
| }, [selectedValidator, editingValidatorId]); |
There was a problem hiding this comment.
Missing dependency in useEffect.
The fetchBanLists function is called inside the effect but not listed in the dependency array. This could cause stale closure issues or React's exhaustive-deps warning.
Proposed fix
useEffect(() => {
if ((selectedValidator === 'ban-list' || editingValidatorId === 'ban-list')) {
fetchBanLists();
}
- }, [selectedValidator, editingValidatorId]);
+ }, [selectedValidator, editingValidatorId, fetchBanLists]);Note: You may also need to wrap fetchBanLists in useCallback to avoid infinite re-renders.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/configurations/safety-guardrails/page.tsx` around lines 169 - 173, The
useEffect that runs when selectedValidator or editingValidatorId change calls
fetchBanLists but does not list it in the dependency array; add fetchBanLists to
the dependency array of the useEffect and, if fetchBanLists is defined inline or
recreated on every render, wrap it with useCallback (or move it to stable scope)
to prevent stale closures or infinite re-renders; update references to the
useEffect, fetchBanLists, selectedValidator, and editingValidatorId accordingly
so React's exhaustive-deps is satisfied.
Comment on lines
+999
to
+1001
| if (editingValidatorIndex !== null) { | ||
| handleRemoveValidator(editingValidatorIndex); | ||
| } |
There was a problem hiding this comment.
Potential race condition when updating validator.
When editing an existing validator, the code deletes it first (line 1000) then saves the new version (line 1003). Since handleRemoveValidator is async, this creates a brief window where the validator is deleted but not yet re-added. If an error occurs during save, the validator would be lost.
Consider updating in place or using optimistic updates with rollback.
Proposed fix approach
Instead of delete-then-create, consider:
- Update in place if the backend supports PUT/PATCH for validator configs
- Or perform the save first, then delete the old one on success
- Or wrap both operations in a try-catch with rollback on failure
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/configurations/safety-guardrails/page.tsx` around lines 999 - 1001, The
current flow deletes an existing validator by calling
handleRemoveValidator(editingValidatorIndex) before saving the replacement,
creating a race where the validator can be lost if the save fails; change this
to either update in place (call a PATCH/PUT API such as an updateValidator or
handleUpdateValidator with editingValidatorIndex and the new data) or, if update
API is unavailable, perform the save first (await the create/save API call),
then on success call handleRemoveValidator(editingValidatorIndex); wrap the
create-then-delete sequence in a try/catch and implement rollback (delete the
newly created entry or restore the original) on failure so no data is lost.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/configurations/prompt-editor/page.tsx (3)
400-422:⚠️ Potential issue | 🟠 MajorEnsure atomicity when hydrating validators from URL.
The code filters out failed fetches (
validValidators = fetchedValidators.filter(v => v !== null)) and proceeds with partial results. This can leave the config in an inconsistent state with only some validators applied while others silently failed.🛠️ Suggested fix: abort on partial failure
const fetchedValidators = await Promise.all(validatorPromises); const validValidators = fetchedValidators.filter(v => v !== null); + // Abort if any validator fetch failed to maintain atomicity + if (validValidators.length !== validatorIds.length) { + console.error('[ValidatorURL] Incomplete validator fetch; aborting update'); + isLoadingFromUrl.current = false; + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 400 - 422, The current code accepts partial validator fetch results by filtering fetchedValidators into validValidators and proceeding, which can leave the config inconsistent; update the logic around validatorPromises/fetchedValidators to treat any failed fetch (null) as an overall failure: if fetchedValidators includes null, do not call setSavedValidators or setCurrentConfigBlob, instead surface/log the error and return/abort so no partial input_guardrails/output_guardrails are applied; keep the existing construction of inputGuardrails/outputGuardrails and the setCurrentConfigBlob call only in the success path where all fetchedValidators are non-null.
331-359:⚠️ Potential issue | 🟠 MajorClear
isLoadingFromUrlon all early-return paths.After Line 311 sets
isLoadingFromUrl.current = true, the early returns at lines 333-334 (no apiKey), 348-350 (!verifyResponse.ok), and 357-359 (missing org/project) exit without resetting the flag. This leaves the flag stuck attrue, blocking subsequent validator loads.🛠️ Suggested fix
const apiKey = getApiKey(); if (!apiKey) { console.error('[ValidatorURL] No API key found'); + isLoadingFromUrl.current = false; return; } // ... if (!verifyResponse.ok) { console.error('[ValidatorURL] Failed to verify API key'); + isLoadingFromUrl.current = false; return; } // ... if (!organizationId || !projectId) { console.error('[ValidatorURL] Could not retrieve organization or project ID'); + isLoadingFromUrl.current = false; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 331 - 359, The code sets isLoadingFromUrl.current = true before calling getApiKey() but several early-return paths (when apiKey is falsy after getApiKey(), when verifyResponse.ok is false after the fetch to /api/apikeys/verify, and when verifyData lacks organization_id or project_id) return without resetting isLoadingFromUrl.current; update the logic in the function that calls getApiKey/does the fetch so that every early-return branch (the apiKey check, the verifyResponse.ok check, and the organizationId/projectId check) clears isLoadingFromUrl.current = false before returning, ensuring the flag is always reset (refer to isLoadingFromUrl, getApiKey, verifyResponse, verifyData, organizationId, projectId to locate the branches).
195-298:⚠️ Potential issue | 🟠 MajorNarrow the dependency to guardrails-specific fields.
This effect still depends on the entire
currentConfigBlob, causing re-execution and network requests whenever any config field changes. Extract and memoize the guardrails fields to prevent unnecessary API calls.🛠️ Suggested fix
+ // Memoize guardrails to avoid effect re-runs on unrelated config changes + const inputGuardrailsKey = JSON.stringify((currentConfigBlob as any).input_guardrails || []); + const outputGuardrailsKey = JSON.stringify((currentConfigBlob as any).output_guardrails || []); useEffect(() => { const loadValidators = async () => { // ... existing logic }; loadValidators(); - }, [currentConfigBlob]); + }, [inputGuardrailsKey, outputGuardrailsKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 195 - 298, The effect loadValidators currently depends on the whole currentConfigBlob causing unnecessary runs; extract and memoize the guardrail fields (input_guardrails and output_guardrails) from currentConfigBlob using useMemo (e.g., memoizedGuardrails = useMemo(() => ({ input: currentConfigBlob?.input_guardrails || [], output: currentConfigBlob?.output_guardrails || [] }), [currentConfigBlob?.input_guardrails, currentConfigBlob?.output_guardrails])) and then change the useEffect dependency from [currentConfigBlob] to depend on those memoized values (and any other true external deps), keeping loadValidators, setSavedValidators and the same fetch logic intact.
🧹 Nitpick comments (3)
app/configurations/prompt-editor/page.tsx (3)
87-89: Define a proper type forsavedValidatorsinstead ofany[].Using
any[]loses all type-safety benefits. Consider defining aValidatorConfiginterface matching the validator structure returned from the API.♻️ Suggested type definition
interface ValidatorConfig { id: string; name: string; description: string; enabled: boolean; validator_config_id: string; config: { stage: 'input' | 'output'; type: string; [key: string]: unknown; }; } const [savedValidators, setSavedValidators] = useState<ValidatorConfig[]>([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 87 - 89, Replace the any[] usage for savedValidators with a concrete type: define a ValidatorConfig interface that matches the API shape (fields like id, name, description, enabled, validator_config_id and a config object with stage/type and extra keys) and then change the useState call to useState<ValidatorConfig[]>([]) so savedValidators and setSavedValidators are strongly typed; update any usages of savedValidators in functions or components to respect the new properties and adjust imports/types if necessary.
257-285: Extract duplicate validator-fetch logic into a shared helper.The mapping logic in both
loadValidators(lines 267-278) andfetchValidatorsFromUrl(lines 380-391) is nearly identical. Extracting this to a helper function improves maintainability and prevents divergence.♻️ Suggested extraction
const mapValidatorResponse = ( data: any, validatorConfigId: string, stage: string = 'input' ) => ({ id: data.type, name: data.type.replace(/_/g, ' ').replace(/\b\w/g, (l: string) => l.toUpperCase()), description: 'Configured guardrail', enabled: data.is_enabled !== undefined ? data.is_enabled : true, validator_config_id: validatorConfigId, config: { stage, type: data.type, ...data, } });Also applies to: 370-398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 257 - 285, The duplicate mapping logic used in loadValidators and fetchValidatorsFromUrl should be extracted into a shared helper (e.g., mapValidatorResponse) that accepts (data, validatorConfigId, stage='input') and returns the normalized validator object; replace the inline mapping in both async blocks in loadValidators and fetchValidatorsFromUrl with calls to that helper, preserving the current behavior for id, name (underscore -> title case), description, enabled defaulting to true, validator_config_id, and config merging stage/type/...data so future changes only need to be made in one place.
165-177: Remove debugconsole.logstatements before merging.Lines 165, 174, and 177 contain debug logs that should be removed or converted to a proper logging utility with appropriate log levels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/configurations/prompt-editor/page.tsx` around lines 165 - 177, Remove the debug console.log calls around the saved config guardrail handling: eliminate the console.log('[DEBUG] Found guardrails in saved config'), console.log('[DEBUG] No guardrails found in saved config'), and console.log('[DEBUG] Final loadedConfigBlob:', loadedConfigBlob) statements in the block that references savedConfig and loadedConfigBlob; if logging is required, replace them with the project's logging utility at an appropriate level (e.g., logger.debug) and avoid printing sensitive blob contents directly. Ensure the guardrail assignment logic that sets loadedConfigBlob.input_guardrails and loadedConfigBlob.output_guardrails remains unchanged and that no other console.* calls remain in this function.
🤖 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/configurations/prompt-editor/page.tsx`:
- Around line 324-328: The current arbitrary setTimeout used to reset
isLoadingFromUrl.current is fragile; remove the timeout and instead reset the
flag deterministically after the related state updates (e.g., right after
updating savedValidators or inside a useEffect/useLayoutEffect that depends on
savedValidators) so the flag clears only when the config blob processing is
complete; update both occurrences (the block referencing
isLoadingFromUrl.current near the ValidatorURL handler and the similar block
around savedValidators) to perform the reset synchronously post-state-change or
in an effect cleanup to avoid race conditions.
---
Duplicate comments:
In `@app/configurations/prompt-editor/page.tsx`:
- Around line 400-422: The current code accepts partial validator fetch results
by filtering fetchedValidators into validValidators and proceeding, which can
leave the config inconsistent; update the logic around
validatorPromises/fetchedValidators to treat any failed fetch (null) as an
overall failure: if fetchedValidators includes null, do not call
setSavedValidators or setCurrentConfigBlob, instead surface/log the error and
return/abort so no partial input_guardrails/output_guardrails are applied; keep
the existing construction of inputGuardrails/outputGuardrails and the
setCurrentConfigBlob call only in the success path where all fetchedValidators
are non-null.
- Around line 331-359: The code sets isLoadingFromUrl.current = true before
calling getApiKey() but several early-return paths (when apiKey is falsy after
getApiKey(), when verifyResponse.ok is false after the fetch to
/api/apikeys/verify, and when verifyData lacks organization_id or project_id)
return without resetting isLoadingFromUrl.current; update the logic in the
function that calls getApiKey/does the fetch so that every early-return branch
(the apiKey check, the verifyResponse.ok check, and the organizationId/projectId
check) clears isLoadingFromUrl.current = false before returning, ensuring the
flag is always reset (refer to isLoadingFromUrl, getApiKey, verifyResponse,
verifyData, organizationId, projectId to locate the branches).
- Around line 195-298: The effect loadValidators currently depends on the whole
currentConfigBlob causing unnecessary runs; extract and memoize the guardrail
fields (input_guardrails and output_guardrails) from currentConfigBlob using
useMemo (e.g., memoizedGuardrails = useMemo(() => ({ input:
currentConfigBlob?.input_guardrails || [], output:
currentConfigBlob?.output_guardrails || [] }),
[currentConfigBlob?.input_guardrails, currentConfigBlob?.output_guardrails]))
and then change the useEffect dependency from [currentConfigBlob] to depend on
those memoized values (and any other true external deps), keeping
loadValidators, setSavedValidators and the same fetch logic intact.
---
Nitpick comments:
In `@app/configurations/prompt-editor/page.tsx`:
- Around line 87-89: Replace the any[] usage for savedValidators with a concrete
type: define a ValidatorConfig interface that matches the API shape (fields like
id, name, description, enabled, validator_config_id and a config object with
stage/type and extra keys) and then change the useState call to
useState<ValidatorConfig[]>([]) so savedValidators and setSavedValidators are
strongly typed; update any usages of savedValidators in functions or components
to respect the new properties and adjust imports/types if necessary.
- Around line 257-285: The duplicate mapping logic used in loadValidators and
fetchValidatorsFromUrl should be extracted into a shared helper (e.g.,
mapValidatorResponse) that accepts (data, validatorConfigId, stage='input') and
returns the normalized validator object; replace the inline mapping in both
async blocks in loadValidators and fetchValidatorsFromUrl with calls to that
helper, preserving the current behavior for id, name (underscore -> title case),
description, enabled defaulting to true, validator_config_id, and config merging
stage/type/...data so future changes only need to be made in one place.
- Around line 165-177: Remove the debug console.log calls around the saved
config guardrail handling: eliminate the console.log('[DEBUG] Found guardrails
in saved config'), console.log('[DEBUG] No guardrails found in saved config'),
and console.log('[DEBUG] Final loadedConfigBlob:', loadedConfigBlob) statements
in the block that references savedConfig and loadedConfigBlob; if logging is
required, replace them with the project's logging utility at an appropriate
level (e.g., logger.debug) and avoid printing sensitive blob contents directly.
Ensure the guardrail assignment logic that sets
loadedConfigBlob.input_guardrails and loadedConfigBlob.output_guardrails remains
unchanged and that no other console.* calls remain in this function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db6cf035-5f65-4367-8d9b-2ad313438434
📒 Files selected for processing (3)
app/components/Sidebar.tsxapp/configurations/prompt-editor/page.tsxapp/lib/useConfigs.ts
✅ Files skipped from review due to trivial changes (1)
- app/components/Sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/useConfigs.ts
Comment on lines
+324
to
+328
| setTimeout(() => { | ||
| isLoadingFromUrl.current = false; | ||
| console.log('[ValidatorURL] Reset loading flag'); | ||
| }, 500); | ||
| return; |
There was a problem hiding this comment.
Replace arbitrary setTimeout with a deterministic approach.
Using a 500ms timeout to reset isLoadingFromUrl.current is fragile—the config blob effect may fire before or after this window depending on render timing. Consider using a useEffect cleanup or a more deterministic synchronization mechanism.
🛠️ Alternative: reset immediately after state updates
- // Reset flag after a longer delay to ensure config blob updates are complete
- setTimeout(() => {
- isLoadingFromUrl.current = false;
- console.log('[ValidatorURL] Reset loading flag');
- }, 500);
+ // Reset immediately - React batches state updates within the same event loop
+ isLoadingFromUrl.current = false;If this causes a race, consider using a useLayoutEffect or moving the flag reset to a useEffect that depends on savedValidators.
Also applies to: 437-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/configurations/prompt-editor/page.tsx` around lines 324 - 328, The
current arbitrary setTimeout used to reset isLoadingFromUrl.current is fragile;
remove the timeout and instead reset the flag deterministically after the
related state updates (e.g., right after updating savedValidators or inside a
useEffect/useLayoutEffect that depends on savedValidators) so the flag clears
only when the config blob processing is complete; update both occurrences (the
block referencing isLoadingFromUrl.current near the ValidatorURL handler and the
similar block around savedValidators) to perform the reset synchronously
post-state-change or in an effect cleanup to avoid race conditions.
Ishankoradia
approved these changes
Mar 19, 2026
nishika26
requested review from
Ishankoradia
and removed request for
Ishankoradia
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
2 participants
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.
Target issue is #61
Summary by CodeRabbit
New Features
Chores
Summary by CodeRabbit
Release Notes
New Features
User Interface