feat(api): update API URL handling in conversations and escalations p…#12
Merged
feat(api): update API URL handling in conversations and escalations p…#12
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds force-dynamic rendering and Supabase-backed session auth to dashboard pages and actions; replaces hard-coded backend URLs with env-driven API_URL; strengthens middleware env validation; introduces ThemeProvider/ThemeToggle and dark-mode styling; adds WhatsApp outbound messaging, recipient lookup, and related repository/API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Middleware as Dashboard Middleware
participant Supabase as Supabase (Auth)
participant Server as Next Server (Dashboard)
participant Backend as Backend API
participant WhatsApp as WhatsApp Graph API
Browser->>Middleware: Request page (cookies)
Middleware->>Supabase: init server client & get session
Supabase-->>Middleware: session or null
alt no user
Middleware-->>Browser: Redirect to /login (unless excluded)
else user present
Browser->>Server: SSR request (cookies forwarded)
Server->>Supabase: get session.access_token
Supabase-->>Server: access_token
Server->>Backend: fetch ${API_URL}/api/... with Authorization: Bearer token
Backend-->>Server: JSON response
Server-->>Browser: Render page with data
opt Sending outbound message via UI
Browser->>Server: POST /api/conversations/:id/messages (with token via server)
Server->>Backend: POST /api/conversations/:id/messages (Authorization: Bearer token)
Backend->>WhatsApp: sendWhatsAppTextMessage (phoneNumberId, accessToken, to, text)
WhatsApp-->>Backend: messageId (or error)
Backend->>Backend: persist message with whatsappMessageId
Backend-->>Server: response (persisted status)
Server-->>Browser: action result (success/error)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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 Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/app/conversations/page.tsx`:
- Around line 10-12: Standardize the dashboard API configuration to use
NEXT_PUBLIC_API_URL everywhere: replace hardcoded URLs and other env var names
(e.g., API_BASE_URL, API_URL) with process.env.NEXT_PUBLIC_API_URL and keep a
single consistent fallback (e.g., 'http://localhost:3001'); update the apiUrl
declaration in apps/dashboard/app/conversations/page.tsx (the const apiUrl and
the fetch call to `${apiUrl}/api/conversations`) and change similar usages in
apps/dashboard/app/conversations/[id]/page.tsx,
apps/dashboard/app/conversations/actions.ts,
apps/dashboard/app/escalations/actions.ts, and apps/dashboard/app/actions.ts to
reference the same variable and fallback, and add the Next.js route directive
("use server" / force-dynamic) to pages that require SSR/real-time fetches so
they don't rely on static optimization.
In `@apps/dashboard/app/escalations/page.tsx`:
- Around line 8-9: The API base URL is hardcoded in actions.ts (const
API_BASE_URL) causing assignOperatorAction and updateEscalationStatusAction to
ignore the environment config; change the constant to read from
NEXT_PUBLIC_API_URL with a localhost fallback so it matches page.tsx (use the
env var plus “/api” when present) and ensure assignOperatorAction and
updateEscalationStatusAction use that updated API_BASE_URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95ba392d-b493-4cfd-a471-51819c392312
📒 Files selected for processing (2)
apps/dashboard/app/conversations/page.tsxapps/dashboard/app/escalations/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/dashboard/middleware.ts (1)
44-54: Consider using consistent path matching for exclusions.The path exclusion logic uses
startsWithfor/loginand/apibut exact match (!==) for/auth/callback. This inconsistency could cause issues if/auth/callbackhas sub-paths (e.g.,/auth/callback/google).♻️ Suggested fix for consistency
if ( !user && !request.nextUrl.pathname.startsWith('/login') && !request.nextUrl.pathname.startsWith('/api') && - request.nextUrl.pathname !== '/auth/callback' + !request.nextUrl.pathname.startsWith('/auth/callback') ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/middleware.ts` around lines 44 - 54, The path exclusion check is inconsistent: it uses startsWith for '/login' and '/api' but an exact !== match for '/auth/callback', which will miss subpaths like '/auth/callback/google'; update the condition that checks request.nextUrl.pathname !== '/auth/callback' to use startsWith (request.nextUrl.pathname.startsWith('/auth/callback')) or consolidate into a single helper/array of excluded prefixes so the user redirect logic (the block that constructs url.clone() and calls NextResponse.redirect) consistently excludes all intended subpaths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/middleware.ts`:
- Around line 13-20: The middleware currently returns supabaseResponse when
supabaseUrl or supabaseKey are missing, which leaves protected routes open;
update the logic in apps/dashboard/middleware.ts so that if either supabaseUrl
or supabaseKey is absent you fail closed in production by returning an error
response (HTTP 500) or redirecting to a configuration/error page, but allow the
existing bypass only when NODE_ENV === 'development' (or a similar explicit dev
check); locate the check that references supabaseUrl, supabaseKey and
supabaseResponse and replace the unconditional return with a conditional: if
missing vars and not in development, return a 500/redirect response with a clear
message, otherwise (dev) continue returning supabaseResponse.
- Around line 57-61: The middleware's catch block currently logs the error and
returns supabaseResponse (allowing requests to proceed unauthenticated); change
that to "fail closed" by replacing the return of supabaseResponse in the catch
with an explicit denial of access — e.g., return an unauthorized response or a
redirect to the sign-in page (use NextResponse.redirect(...) or a 401/403
Response) while keeping the console.error logging; update the catch handling in
apps/dashboard/middleware.ts (referencing the catch block, console.error call,
and supabaseResponse symbol) so authentication errors cannot silently bypass the
middleware.
---
Nitpick comments:
In `@apps/dashboard/middleware.ts`:
- Around line 44-54: The path exclusion check is inconsistent: it uses
startsWith for '/login' and '/api' but an exact !== match for '/auth/callback',
which will miss subpaths like '/auth/callback/google'; update the condition that
checks request.nextUrl.pathname !== '/auth/callback' to use startsWith
(request.nextUrl.pathname.startsWith('/auth/callback')) or consolidate into a
single helper/array of excluded prefixes so the user redirect logic (the block
that constructs url.clone() and calls NextResponse.redirect) consistently
excludes all intended subpaths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 739e4004-037e-4571-950e-e6eddba6f78a
📒 Files selected for processing (1)
apps/dashboard/middleware.ts
Comment on lines
+13
to
+20
| if (!supabaseUrl || !supabaseKey) { | ||
| console.error('Missing Supabase env vars:', { | ||
| hasUrl: !!supabaseUrl, | ||
| hasKey: !!supabaseKey, | ||
| }); | ||
| // Allow request to proceed if auth is unavailable | ||
| return supabaseResponse; | ||
| } |
There was a problem hiding this comment.
Security risk: Bypassing authentication when env vars are missing.
When Supabase environment variables are missing, the middleware allows all requests to proceed without any authentication. This could expose protected routes to unauthenticated users if the environment is misconfigured in production.
Consider either:
- Returning an error response (500) to fail closed rather than open
- Redirecting to an error page explaining the misconfiguration
- At minimum, only allow this bypass in development mode
🛡️ Proposed fix to fail closed
if (!supabaseUrl || !supabaseKey) {
console.error('Missing Supabase env vars:', {
hasUrl: !!supabaseUrl,
hasKey: !!supabaseKey,
});
- // Allow request to proceed if auth is unavailable
- return supabaseResponse;
+ // Fail closed: return error if auth is unavailable in production
+ if (process.env.NODE_ENV === 'production') {
+ return new NextResponse('Authentication service unavailable', { status: 503 });
+ }
+ // In development, allow request to proceed for easier debugging
+ return supabaseResponse;
}📝 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
| if (!supabaseUrl || !supabaseKey) { | |
| console.error('Missing Supabase env vars:', { | |
| hasUrl: !!supabaseUrl, | |
| hasKey: !!supabaseKey, | |
| }); | |
| // Allow request to proceed if auth is unavailable | |
| return supabaseResponse; | |
| } | |
| if (!supabaseUrl || !supabaseKey) { | |
| console.error('Missing Supabase env vars:', { | |
| hasUrl: !!supabaseUrl, | |
| hasKey: !!supabaseKey, | |
| }); | |
| // Fail closed: return error if auth is unavailable in production | |
| if (process.env.NODE_ENV === 'production') { | |
| return new NextResponse('Authentication service unavailable', { status: 503 }); | |
| } | |
| // In development, allow request to proceed for easier debugging | |
| return supabaseResponse; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/middleware.ts` around lines 13 - 20, The middleware currently
returns supabaseResponse when supabaseUrl or supabaseKey are missing, which
leaves protected routes open; update the logic in apps/dashboard/middleware.ts
so that if either supabaseUrl or supabaseKey is absent you fail closed in
production by returning an error response (HTTP 500) or redirecting to a
configuration/error page, but allow the existing bypass only when NODE_ENV ===
'development' (or a similar explicit dev check); locate the check that
references supabaseUrl, supabaseKey and supabaseResponse and replace the
unconditional return with a conditional: if missing vars and not in development,
return a 500/redirect response with a clear message, otherwise (dev) continue
returning supabaseResponse.
Comment on lines
+57
to
+61
| } catch (error) { | ||
| console.error('Middleware auth error:', error); | ||
| // On auth error, allow request to proceed (errors will be caught at component level) | ||
| return supabaseResponse; | ||
| } |
There was a problem hiding this comment.
Security risk: Fail-open error handling bypasses authentication.
When authentication errors occur (network issues, Supabase downtime, etc.), the middleware allows requests to proceed unauthenticated. Combined with the env var check above, there are now two paths where authentication can be silently bypassed.
Relying on component-level error handling for security is fragile—middleware should be the security boundary.
🛡️ Proposed fix to fail closed on auth errors
} catch (error) {
console.error('Middleware auth error:', error);
- // On auth error, allow request to proceed (errors will be caught at component level)
- return supabaseResponse;
+ // Fail closed: redirect to login on auth error for protected routes
+ if (
+ !request.nextUrl.pathname.startsWith('/login') &&
+ !request.nextUrl.pathname.startsWith('/api') &&
+ !request.nextUrl.pathname.startsWith('/auth/callback')
+ ) {
+ const url = request.nextUrl.clone();
+ url.pathname = '/login';
+ url.searchParams.set('error', 'auth_error');
+ return NextResponse.redirect(url);
+ }
+ return supabaseResponse;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/middleware.ts` around lines 57 - 61, The middleware's catch
block currently logs the error and returns supabaseResponse (allowing requests
to proceed unauthenticated); change that to "fail closed" by replacing the
return of supabaseResponse in the catch with an explicit denial of access —
e.g., return an unauthorized response or a redirect to the sign-in page (use
NextResponse.redirect(...) or a 401/403 Response) while keeping the
console.error logging; update the catch handling in apps/dashboard/middleware.ts
(referencing the catch block, console.error call, and supabaseResponse symbol)
so authentication errors cannot silently bypass the middleware.
…iddleware response
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/dashboard/middleware.ts (2)
55-59:⚠️ Potential issue | 🔴 CriticalCatch block still fails open on auth errors.
Returning
supabaseResponsehere allows protected requests through when auth checks fail.🛡️ Suggested fix
} catch (error) { console.error('Middleware auth error:', error); - // On auth error, allow request to proceed (errors will be caught at component level) - return supabaseResponse; + if ( + !request.nextUrl.pathname.startsWith('/login') && + !request.nextUrl.pathname.startsWith('/api') && + request.nextUrl.pathname !== '/auth/callback' + ) { + const url = request.nextUrl.clone(); + url.pathname = '/login'; + url.searchParams.set('error', 'auth_error'); + return NextResponse.redirect(url); + } + return supabaseResponse; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/middleware.ts` around lines 55 - 59, The catch block in the middleware (the catch after creating/validating supabaseResponse) currently returns supabaseResponse which lets protected requests through on auth errors; instead, do not "fail open"—either rethrow the caught error or return an explicit unauthorized/redirect response (e.g., throw error or return NextResponse.redirect/signInResponse) so requests are blocked when auth checks fail; update the catch handling around supabaseResponse to propagate the error or return a proper auth-failure response rather than returning supabaseResponse.
13-20:⚠️ Potential issue | 🔴 CriticalFail-open on missing Supabase env vars still exposes protected routes.
This still allows unauthenticated access when config is missing. Please fail closed in non-development environments.
🔒 Suggested fix
if (!supabaseUrl || !supabaseKey) { console.error('Missing Supabase env vars:', { hasUrl: !!supabaseUrl, hasKey: !!supabaseKey, }); - // Allow request to proceed if auth is unavailable - return supabaseResponse; + if (process.env.NODE_ENV !== 'development') { + return new NextResponse('Authentication service unavailable', { status: 503 }); + } + return supabaseResponse; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/middleware.ts` around lines 13 - 20, The current check in middleware that returns supabaseResponse when supabaseUrl or supabaseKey are missing leaves protected routes open; change it to fail closed in non-development environments by detecting the runtime (e.g., process.env.NODE_ENV !== 'development' or an existing isDev flag) and, when credentials are missing and not in development, return an explicit denied response (HTTP 401/403 or redirect to sign-in) instead of returning supabaseResponse; keep the current permissive behavior only when in development. Ensure you update the branch that references supabaseUrl/supabaseKey and the code path that returns supabaseResponse so the denial happens before any further request handling.
🧹 Nitpick comments (1)
apps/dashboard/middleware.ts (1)
29-33: Consider using explicit block statements inforEachcallbacks for clarity.The arrow functions without braces in these callbacks are valid JavaScript, but using explicit braces improves readability. Here's the suggested refactor:
Suggested improvement
- cookiesToSet.forEach(({ name, value }) => request.cookies.set(name, value)); + cookiesToSet.forEach(({ name, value }) => { + request.cookies.set(name, value); + }); supabaseResponse = NextResponse.next({ request }); - cookiesToSet.forEach(({ name, value, options }) => - supabaseResponse.cookies.set(name, value, options), - ); + cookiesToSet.forEach(({ name, value, options }) => { + supabaseResponse.cookies.set(name, value, options); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/middleware.ts` around lines 29 - 33, The forEach callbacks that call request.cookies.set and supabaseResponse.cookies.set use concise arrow bodies which reduces readability; update the cookiesToSet.forEach callbacks to use explicit block statements with braces and an explicit return (or no return) so each callback is clearly a statement block—specifically modify the cookiesToSet.forEach(({ name, value }) => request.cookies.set(name, value)) and cookiesToSet.forEach(({ name, value, options }) => supabaseResponse.cookies.set(name, value, options)) to use full block arrow functions (e.g., ({...}) => { request.cookies.set(...); }) to improve clarity around the side-effect operations and make it obvious these callbacks perform mutations rather than return values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/dashboard/middleware.ts`:
- Around line 55-59: The catch block in the middleware (the catch after
creating/validating supabaseResponse) currently returns supabaseResponse which
lets protected requests through on auth errors; instead, do not "fail
open"—either rethrow the caught error or return an explicit
unauthorized/redirect response (e.g., throw error or return
NextResponse.redirect/signInResponse) so requests are blocked when auth checks
fail; update the catch handling around supabaseResponse to propagate the error
or return a proper auth-failure response rather than returning supabaseResponse.
- Around line 13-20: The current check in middleware that returns
supabaseResponse when supabaseUrl or supabaseKey are missing leaves protected
routes open; change it to fail closed in non-development environments by
detecting the runtime (e.g., process.env.NODE_ENV !== 'development' or an
existing isDev flag) and, when credentials are missing and not in development,
return an explicit denied response (HTTP 401/403 or redirect to sign-in) instead
of returning supabaseResponse; keep the current permissive behavior only when in
development. Ensure you update the branch that references
supabaseUrl/supabaseKey and the code path that returns supabaseResponse so the
denial happens before any further request handling.
---
Nitpick comments:
In `@apps/dashboard/middleware.ts`:
- Around line 29-33: The forEach callbacks that call request.cookies.set and
supabaseResponse.cookies.set use concise arrow bodies which reduces readability;
update the cookiesToSet.forEach callbacks to use explicit block statements with
braces and an explicit return (or no return) so each callback is clearly a
statement block—specifically modify the cookiesToSet.forEach(({ name, value })
=> request.cookies.set(name, value)) and cookiesToSet.forEach(({ name, value,
options }) => supabaseResponse.cookies.set(name, value, options)) to use full
block arrow functions (e.g., ({...}) => { request.cookies.set(...); }) to
improve clarity around the side-effect operations and make it obvious these
callbacks perform mutations rather than return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c216c3f0-2722-4389-9441-4b7a0fbd7588
📒 Files selected for processing (2)
apps/dashboard/app/actions.tsapps/dashboard/middleware.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/auth.ts`:
- Around line 6-7: The current fallback to the string 'dummy' for
supabaseServiceRoleKey masks misconfiguration; change initialization of
supabaseServiceRoleKey (the const) to require a real env value by removing the
'dummy' default and adding an explicit startup check that throws a clear Error
(or exits) if neither SUPABASE_SERVICE_ROLE_KEY nor SUPABASE_SERVICE_KEY is set;
include the variable name supabaseServiceRoleKey in the error message so the
failure is loud and immediate.
Comment on lines
+6
to
+7
| const supabaseServiceRoleKey = | ||
| process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.SUPABASE_SERVICE_KEY || 'dummy'; |
There was a problem hiding this comment.
Fail fast on missing Supabase service key instead of using 'dummy'.
'dummy' on Line 7 masks misconfiguration and shifts failure into runtime auth/database calls. This should error at startup so the service fails loudly and predictably.
Suggested fix
const supabaseUrl = process.env.SUPABASE_URL || 'http://localhost:54321';
-const supabaseServiceRoleKey =
- process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.SUPABASE_SERVICE_KEY || 'dummy';
+const supabaseServiceRoleKey =
+ process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.SUPABASE_SERVICE_KEY;
+
+if (!supabaseServiceRoleKey) {
+ throw new Error(
+ 'Missing Supabase service key: set SUPABASE_SERVICE_ROLE_KEY (or legacy SUPABASE_SERVICE_KEY).',
+ );
+}📝 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 supabaseServiceRoleKey = | |
| process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.SUPABASE_SERVICE_KEY || 'dummy'; | |
| const supabaseUrl = process.env.SUPABASE_URL || 'http://localhost:54321'; | |
| const supabaseServiceRoleKey = | |
| process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.SUPABASE_SERVICE_KEY; | |
| if (!supabaseServiceRoleKey) { | |
| throw new Error( | |
| 'Missing Supabase service key: set SUPABASE_SERVICE_ROLE_KEY (or legacy SUPABASE_SERVICE_KEY).', | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/auth.ts` around lines 6 - 7, The current fallback to the string
'dummy' for supabaseServiceRoleKey masks misconfiguration; change initialization
of supabaseServiceRoleKey (the const) to require a real env value by removing
the 'dummy' default and adding an explicit startup check that throws a clear
Error (or exits) if neither SUPABASE_SERVICE_ROLE_KEY nor SUPABASE_SERVICE_KEY
is set; include the variable name supabaseServiceRoleKey in the error message so
the failure is loud and immediate.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/app/actions.ts`:
- Around line 23-27: Remove or sanitize the raw console logging of internals:
stop printing process.env.API_URL and full error/session objects directly;
instead gate these logs behind an explicit debug flag or non-production check
(e.g., NODE_ENV !== 'production' or ENABLE_DEBUG_LOGS) and only log sanitized
fields (error.message, error.status, and boolean/session id instead of full
session). Update the console.log calls that reference process.env.API_URL,
session, session?.access_token, and error (and similar logs around those areas)
to use the guarded debug path or a proper logger.debug so production paths do
not expose backend internals.
- Around line 20-30: Replace the overly defensive check around
supabase.auth.getSession() with a proper SSR-safe auth check: call
supabase.auth.getUser() and use its returned user (symbols:
supabase.auth.getUser, user, userError) to gate access—if userError or !user
return null; if you must rely on getSession() keep only the session token guard
(symbols: supabase.auth.getSession, session, access_token) and remove the
`error` check entirely. Ensure subsequent fetches use user.id or, when needing a
token, read session.access_token after confirming session exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1557954-1c47-4b1e-98a7-a2983df1bc28
📒 Files selected for processing (1)
apps/dashboard/app/actions.ts
Comment on lines
+20
to
30
| error, | ||
| } = await supabase.auth.getSession(); | ||
|
|
||
| if (!session?.access_token) { | ||
| console.log('Session error:', error); | ||
| console.log('Has session:', !!session); | ||
| console.log('Has token:', !!session?.access_token); | ||
| console.log('API URL:', process.env.API_URL); | ||
|
|
||
| if (error || !session?.access_token) { | ||
| console.log('Early return — no session'); | ||
| return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Supabase JS (auth.getSession) for SSR/Next.js, can errorbe non-null whiledata.session is still present, and what is the recommended guard pattern before authenticated fetch calls?
💡 Result:
No—per the auth-js implementation, auth.getSession() does not return a non-null error alongside a non-null data.session. If a refresh fails (or another auth error occurs), the function returns { data: { session: null }, error }; if it returns a session, error is null. [1]
Recommended guard pattern (SSR/Next.js) before authenticated fetches:
// Prefer this in SSR to “trust” the user (avoids trusting cookie storage directly)
const { data: { user }, error: userError } = await supabase.auth.getUser()
if (userError || !user) {
// redirect / return 401
}
// If you still need the access token for a manual fetch:
const { data: { session } } = await supabase.auth.getSession() // error should be null here
if (!session?.access_token) {
// redirect / return 401
}
await fetch(url, {
headers: { Authorization: `Bearer ${session.access_token}` },
})Supabase explicitly warns that getSession() reads directly from storage (cookies in SSR) and is not a trusted identity signal—use getUser() (or JWT verification) for SSR authorization decisions. [2] Also, Supabase notes you can call getUser() before getSession() to suppress the “getSession is potentially insecure” warning in SSR scenarios. [3]
Sources: [1] [2] [3]
Remove the redundant error check; use getUser() for SSR authorization instead.
At Line 28, if (error || !session?.access_token) is overly defensive. Per Supabase's auth-js design, error cannot coexist with a valid session—if error is non-null, session is already null. The check is redundant.
More importantly, getSession() is not trusted for SSR authorization (it reads directly from storage). Use getUser() instead, which properly validates the JWT:
const { data: { user }, error: userError } = await supabase.auth.getUser();
if (userError || !user) {
return null;
}Then fetch using user.id or retrieve the session for its token only if needed. If you must use getSession(), simplify the guard to if (!session?.access_token) and drop the error check entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/actions.ts` around lines 20 - 30, Replace the overly
defensive check around supabase.auth.getSession() with a proper SSR-safe auth
check: call supabase.auth.getUser() and use its returned user (symbols:
supabase.auth.getUser, user, userError) to gate access—if userError or !user
return null; if you must rely on getSession() keep only the session token guard
(symbols: supabase.auth.getSession, session, access_token) and remove the
`error` check entirely. Ensure subsequent fetches use user.id or, when needing a
token, read session.access_token after confirming session exists.
Comment on lines
+23
to
+27
| console.log('Session error:', error); | ||
| console.log('Has session:', !!session); | ||
| console.log('Has token:', !!session?.access_token); | ||
| console.log('API URL:', process.env.API_URL); | ||
|
|
There was a problem hiding this comment.
Avoid logging raw API internals in production paths.
Line 26 and Line 45 log environment/backend details directly (API_URL and full error body). That can leak internal info and create noisy logs on hot paths. Keep only sanitized, opt-in debug logs.
Proposed hardening
+ const debugKpiFetch =
+ process.env.NODE_ENV !== 'production' &&
+ process.env.DEBUG_KPI_FETCH === '1';
+
- console.log('Session error:', error);
- console.log('Has session:', !!session);
- console.log('Has token:', !!session?.access_token);
- console.log('API URL:', process.env.API_URL);
+ if (debugKpiFetch) {
+ console.log('Has session:', !!session);
+ console.log('Has token:', !!session?.access_token);
+ }
...
- console.log('Fetching KPIs from:', `${apiUrl}/api/kpis`);
+ if (debugKpiFetch) console.log('Fetching KPIs');
...
- console.log('KPI Response status:', response.status);
+ if (debugKpiFetch) console.log('KPI response status:', response.status);
if (!response.ok) {
- console.log('KPI Response not ok:', await response.text());
+ const body = await response.text();
+ console.error('KPI request failed', {
+ status: response.status,
+ statusText: response.statusText,
+ bodyPreview: body.slice(0, 256),
+ });
return null;
}Also applies to: 35-36, 43-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/actions.ts` around lines 23 - 27, Remove or sanitize the
raw console logging of internals: stop printing process.env.API_URL and full
error/session objects directly; instead gate these logs behind an explicit debug
flag or non-production check (e.g., NODE_ENV !== 'production' or
ENABLE_DEBUG_LOGS) and only log sanitized fields (error.message, error.status,
and boolean/session id instead of full session). Update the console.log calls
that reference process.env.API_URL, session, session?.access_token, and error
(and similar logs around those areas) to use the guarded debug path or a proper
logger.debug so production paths do not expose backend internals.
…ve error handling
…improve error handling
… readability and error logging
…ucturing async methods
…ng and remove unused headers
…and return actions
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)
apps/dashboard/app/login/page.tsx (1)
7-11:⚠️ Potential issue | 🔴 CriticalMake component async and handle
searchParamsas a Promise for Next.js 16.1.6 compatibility.With Next.js 16.1.6,
searchParamsis a Promise and must be awaited. The current synchronous access will fail at runtime.Fix required:
Required changes for Next.js 16
-export default function LoginPage({ +export default async function LoginPage({ searchParams, }: { - searchParams: { message?: string; error?: string }; + searchParams: Promise<{ message?: string; error?: string }>; }) { + const params = await searchParams; return ( <div className="flex h-screen items-center justify-center bg-gray-50 px-4 dark:bg-gray-950"> <div className="w-full max-w-md rounded border border-gray-200 bg-white p-8 text-black shadow dark:border-gray-800 dark:bg-gray-900 dark:text-gray-100"> <h1 className="text-2xl font-bold mb-6 text-center">WA Chat Login</h1> - {searchParams?.error && ( + {params?.error && ( <div className="mb-4 rounded bg-red-100 p-3 text-red-700 dark:bg-red-950/40 dark:text-red-200"> - {searchParams.error} + {params.error} </div> )} - {searchParams?.message && ( + {params?.message && ( <div className="mb-4 rounded bg-green-100 p-3 text-green-700 dark:bg-green-950/40 dark:text-green-200"> - {searchParams.message} + {params.message} </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/login/page.tsx` around lines 7 - 11, Make LoginPage async and await the incoming searchParams Promise: change the component signature for LoginPage to be async (async function LoginPage(...)) and update the prop type for searchParams to Promise<{ message?: string; error?: string }>, then inside LoginPage await searchParams (e.g. const { message, error } = await searchParams) before using them so the component works with Next.js 16.1.6.apps/dashboard/app/conversations/[id]/ConversationActions.tsx (1)
60-79:⚠️ Potential issue | 🟡 MinorDisable the manual reply form while the handoff action is pending.
After Line 28 starts
returnToBot, the textarea and submit button stay interactive until revalidation completes. That leaves a race where an operator can send a manual reply after giving control back to the bot.Suggested fix
<textarea id="content" name="content" + disabled={isPending} rows={2} placeholder="Type your response to the user..." className="w-full rounded-md border border-gray-300 bg-white p-2 text-gray-900 shadow-sm focus:border-indigo-500 focus:ring-indigo-500 focus:outline-none dark:border-gray-700 dark:bg-gray-800 dark:text-gray-100 dark:placeholder:text-gray-400 sm:text-sm" /> </div> <button type="submit" - className="px-4 py-2 bg-indigo-600 text-white rounded hover:bg-indigo-700 text-sm font-medium" + disabled={isPending} + className="px-4 py-2 bg-indigo-600 text-white rounded hover:bg-indigo-700 disabled:opacity-50 text-sm font-medium" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/conversations/`[id]/ConversationActions.tsx around lines 60 - 79, The textarea and submit button should be disabled while the handoff back to the bot is in progress; add a boolean flag (e.g., isReturningToBot or handoffPending) that's set when returnToBot starts and cleared after revalidation, then pass that flag to disable the textarea (id="content") and the Send Manual Reply button and prevent sendMessage from being called while true; reference the existing returnToBot, botActive, sendMessage and conversationId to locate where to set the flag and apply the disabled prop.
♻️ Duplicate comments (1)
apps/dashboard/app/conversations/[id]/page.tsx (1)
5-6:⚠️ Potential issue | 🟠 MajorUse the same API URL resolution and failure handling as the list pages.
This route only reads
process.env.API_URL, and both helpers translate missing config, missing auth, and non-OK responses intonull/[]. In an environment where onlyNEXT_PUBLIC_API_URLis configured—or when the backend returns 401/500—the operator will seeConversation not foundor an empty timeline instead of a real load error.Also applies to: 19-37, 45-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/conversations/`[id]/page.tsx around lines 5 - 6, The page currently reads API URL directly via the apiUrl constant (const apiUrl = process.env.API_URL) which differs from the list pages' resolution and error handling; update this file to use the same API URL resolution helper and failure-to-null behavior used by the list pages (replace the direct env read with the shared resolver used elsewhere), and ensure the fetch/error paths (the logic around where apiUrl is used in the functions spanning the areas noted ~lines 19-37 and 45-62) map missing config, auth failures, and non-OK responses to the same null/empty return semantics so the UI shows a proper load error instead of "Conversation not found" or a blank timeline.
🧹 Nitpick comments (4)
apps/api/src/routes/conversations.ts (2)
82-84: Don't persist'unknown'as the operator ID.
authenticateRequestinapps/api/src/auth.ts:37-90already aborts beforenext()when it cannot populatereq.user.id. Falling back to'unknown'here just masks a broken auth contract and risks writing a synthetic actor into takeover/return/message history. Fail closed instead.🔒 Suggested change
- const operatorId = req.user?.id || 'unknown'; + const operatorId = req.user?.id; + if (!operatorId) { + res.status(401).json({ error: 'Unauthorized' }); + return; + }Also applies to: 94-95, 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.ts` around lines 82 - 84, The code must not persist a synthetic 'unknown' operator id; instead ensure the authenticated user id is present and fail closed: in the conversations route replace the fallback const operatorId = req.user?.id || 'unknown' with a guard that checks req.user and req.user.id and returns a 401/400 or throws an AuthenticationError before calling conversationRepository.takeoverConversation (and similarly update the other occurrences around the takeover/return/message history calls referenced at the other spots). Keep the operatorId strictly sourced from req.user.id and add a clear error response when it's missing.
23-24: Lift the UUID guard out of the timeline route.Only
GET /:id/timelineenforces the new UUID contract right now. The other/:idhandlers below still pass raw path params to the repository, so the same malformed conversation ID can be rejected on one endpoint and leak deeper on another.Also applies to: 63-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.ts` around lines 23 - 24, The UUID check is currently only used inside the GET '/:id/timeline' handler (isUuid), leaving other '/:id' routes to accept malformed IDs; move the UUID guard so it runs for all routes with the :id param. Add an Express param middleware (router.param('id', ...)) or a router.use('/:id', ...) that calls isUuid(value) and returns a 400 error for invalid IDs before reaching handlers (affects handlers referenced by '/:id/timeline' and the other '/:id' routes around lines 63-72). Ensure the middleware uses the same isUuid function and short-circuits request handling when invalid, so repository calls never receive malformed IDs.apps/dashboard/app/escalations/actions.ts (1)
26-35: Keep this fetch helper in sync with the conversations version.
apps/dashboard/app/conversations/actions.tsalready preserves network failures, status codes, and response bodies. This copy only throwsstatusText, so backend details from/assignand/statusdisappear here and the two paths will drift again.Suggested fix
- const res = await fetch(`${API_BASE_URL}/api${endpoint}`, { - ...options, - headers, - }); - - if (!res.ok) { - throw new Error(`API error: ${res.statusText}`); - } + let res: Response; + try { + res = await fetch(`${API_BASE_URL}/api${endpoint}`, { + ...options, + headers, + }); + } catch (error) { + const message = error instanceof Error ? error.message : 'Network error'; + throw new Error(`API network error: ${message}`); + } + + if (!res.ok) { + const body = await res.text().catch(() => ''); + throw new Error(`API error (${res.status}): ${body || res.statusText}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/escalations/actions.ts` around lines 26 - 35, The fetch helper currently throws only res.statusText and drops backend details; update the helper that calls fetch with API_BASE_URL/endpoint to preserve network failures, status codes, and response bodies by: 1) letting network/fetch exceptions bubble up (don’t catch and swallow them), 2) after receiving res (the Response from fetch), if !res.ok read the response body (try res.json(), fallback to res.text()) and throw an Error (or a custom error) that includes res.status, res.statusText and the parsed body/string so callers can inspect backend details; keep usage of API_BASE_URL, endpoint, options, headers, and res intact so it mirrors the conversations/actions.ts implementation.apps/dashboard/app/conversations/[id]/page.tsx (1)
71-73: Fetch summary and timeline in parallel.Once
idis resolved these requests are independent, so the current sequence adds one extra network round-trip to every detail-page render.Suggested change
const { id } = await params; - const summary = await getSummary(id); - const timeline = await getTimeline(id); + const [summary, timeline] = await Promise.all([getSummary(id), getTimeline(id)]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/conversations/`[id]/page.tsx around lines 71 - 73, The code awaits getSummary(id) then getTimeline(id) sequentially causing an extra round-trip; after resolving id from params, call both requests in parallel using Promise.all and destructure the results into summary and timeline (i.e., keep the await params resolution as is, then replace the two awaits with a single await Promise.all([getSummary(id), getTimeline(id)]) and assign the returned array to summary and timeline).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/app/conversations/`[id]/page.tsx:
- Line 102: Replace the invalid Tailwind class on the Conversations page: locate
the div in the Conversations page component (page.tsx) whose className contains
"min-h-100" and either remove that token or change it to the valid Tailwind JIT
form "min-h-[100px]" so the minimum height takes effect.
In `@apps/dashboard/app/conversations/actions.ts`:
- Around line 75-89: The sendMessage action currently swallows failures (in
function sendMessage) by only logging errors, so the UI can't detect or preserve
the draft; update sendMessage to propagate failures back to the caller by either
rethrowing the caught error or returning a standardized ActionResult object
(e.g., { success: boolean, error?: string }) like other mutations, call
revalidatePath(`/conversations/${conversationId}`) only on success, and ensure
consumers of fetchWithAuth receive the error information so the composer can
show an error and keep the draft.
In `@apps/dashboard/app/conversations/page.tsx`:
- Around line 11-16: getConversations is currently returning [] both for
missing/expired session and for API/fetch failures, which collapses auth and
backend errors into a benign "empty queue" state; update the code paths in
getConversations (and the session-check in page.tsx around
createClient()/supabase.auth.getSession()) so that a missing/expired session or
any non-successful fetch response throws or returns an explicit error (or
rejects) instead of returning an empty array, while still returning [] only when
the API responds successfully with an empty conversations list; ensure you
surface/throw the underlying error details for fetch failures and
expired/missing session cases so callers can render an error state or bubble to
monitoring (also apply the same change to the logic referenced in the 23-36
block).
In `@apps/dashboard/app/escalations/actions.ts`:
- Around line 7-18: The API_BASE_URL constant currently falls back to
'http://localhost:3001' which can mask missing config in non-dev environments;
change the initialization of API_BASE_URL so that it uses process.env.API_URL ||
process.env.NEXT_PUBLIC_API_URL, and only fall back to 'http://localhost:3001'
when process.env.NODE_ENV === 'development' (or equivalent dev flag), otherwise
throw a clear error during module initialization; update the fetchApi usage to
rely on this new API_BASE_URL and ensure the thrown error message clearly
indicates a missing API_URL in staging/production so deployments fail fast.
In `@apps/dashboard/app/escalations/page.tsx`:
- Around line 9-14: The current helper silently returns [] on missing session or
fetch errors (in the createClient → supabase.auth.getSession flow), which hides
auth expiry or API failures; change the logic in the page-level loader (around
createClient / supabase.auth.getSession) and the follow-up fetch block (the
other early-return at lines ~21-34) to surface an error instead of returning an
empty array: detect a missing session or any fetch error and throw or return an
error response (e.g., throw new Error or return a server error Response) so the
UI can show an error state rather than the "Inbox Zero" empty copy; update the
code paths referencing createClient, supabase.auth.getSession, and the
escalation fetch to stop returning [] on failures and propagate an explicit
error.
In `@apps/dashboard/app/login/client.tsx`:
- Around line 40-43: The error message rendered from errorMsg is currently only
visual; make it an accessible live region so screen readers announce failures by
adding ARIA attributes to the container (e.g., set role="alert" or
aria-live="assertive" and aria-atomic="true") on the div that renders {errorMsg}
in the login component; keep the existing styling and conditional rendering
logic but add those attributes to the element that contains errorMsg so
assistive tech is automatically notified of new error text.
In `@apps/dashboard/components/theme-toggle.tsx`:
- Around line 11-18: The button currently only has aria-label and swaps icons
(Sun, Moon) but doesn't expose state; update the theme toggle button (the
element using setTheme and isDark) to include an accessible pressed state and
clear destination text: set aria-pressed to the boolean isDark and change
aria-label to a dynamic string like "Switch to light theme" when isDark is true
and "Switch to dark theme" when isDark is false so screen readers get both
action and current/destination state.
---
Outside diff comments:
In `@apps/dashboard/app/conversations/`[id]/ConversationActions.tsx:
- Around line 60-79: The textarea and submit button should be disabled while the
handoff back to the bot is in progress; add a boolean flag (e.g.,
isReturningToBot or handoffPending) that's set when returnToBot starts and
cleared after revalidation, then pass that flag to disable the textarea
(id="content") and the Send Manual Reply button and prevent sendMessage from
being called while true; reference the existing returnToBot, botActive,
sendMessage and conversationId to locate where to set the flag and apply the
disabled prop.
In `@apps/dashboard/app/login/page.tsx`:
- Around line 7-11: Make LoginPage async and await the incoming searchParams
Promise: change the component signature for LoginPage to be async (async
function LoginPage(...)) and update the prop type for searchParams to Promise<{
message?: string; error?: string }>, then inside LoginPage await searchParams
(e.g. const { message, error } = await searchParams) before using them so the
component works with Next.js 16.1.6.
---
Duplicate comments:
In `@apps/dashboard/app/conversations/`[id]/page.tsx:
- Around line 5-6: The page currently reads API URL directly via the apiUrl
constant (const apiUrl = process.env.API_URL) which differs from the list pages'
resolution and error handling; update this file to use the same API URL
resolution helper and failure-to-null behavior used by the list pages (replace
the direct env read with the shared resolver used elsewhere), and ensure the
fetch/error paths (the logic around where apiUrl is used in the functions
spanning the areas noted ~lines 19-37 and 45-62) map missing config, auth
failures, and non-OK responses to the same null/empty return semantics so the UI
shows a proper load error instead of "Conversation not found" or a blank
timeline.
---
Nitpick comments:
In `@apps/api/src/routes/conversations.ts`:
- Around line 82-84: The code must not persist a synthetic 'unknown' operator
id; instead ensure the authenticated user id is present and fail closed: in the
conversations route replace the fallback const operatorId = req.user?.id ||
'unknown' with a guard that checks req.user and req.user.id and returns a
401/400 or throws an AuthenticationError before calling
conversationRepository.takeoverConversation (and similarly update the other
occurrences around the takeover/return/message history calls referenced at the
other spots). Keep the operatorId strictly sourced from req.user.id and add a
clear error response when it's missing.
- Around line 23-24: The UUID check is currently only used inside the GET
'/:id/timeline' handler (isUuid), leaving other '/:id' routes to accept
malformed IDs; move the UUID guard so it runs for all routes with the :id param.
Add an Express param middleware (router.param('id', ...)) or a
router.use('/:id', ...) that calls isUuid(value) and returns a 400 error for
invalid IDs before reaching handlers (affects handlers referenced by
'/:id/timeline' and the other '/:id' routes around lines 63-72). Ensure the
middleware uses the same isUuid function and short-circuits request handling
when invalid, so repository calls never receive malformed IDs.
In `@apps/dashboard/app/conversations/`[id]/page.tsx:
- Around line 71-73: The code awaits getSummary(id) then getTimeline(id)
sequentially causing an extra round-trip; after resolving id from params, call
both requests in parallel using Promise.all and destructure the results into
summary and timeline (i.e., keep the await params resolution as is, then replace
the two awaits with a single await Promise.all([getSummary(id),
getTimeline(id)]) and assign the returned array to summary and timeline).
In `@apps/dashboard/app/escalations/actions.ts`:
- Around line 26-35: The fetch helper currently throws only res.statusText and
drops backend details; update the helper that calls fetch with
API_BASE_URL/endpoint to preserve network failures, status codes, and response
bodies by: 1) letting network/fetch exceptions bubble up (don’t catch and
swallow them), 2) after receiving res (the Response from fetch), if !res.ok read
the response body (try res.json(), fallback to res.text()) and throw an Error
(or a custom error) that includes res.status, res.statusText and the parsed
body/string so callers can inspect backend details; keep usage of API_BASE_URL,
endpoint, options, headers, and res intact so it mirrors the
conversations/actions.ts implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4e4fc1c-4970-4926-ba2a-0cdc3569204d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/api/src/repositories/conversation.tsapps/api/src/routes/conversations.tsapps/dashboard/app/actions.tsapps/dashboard/app/conversations/[id]/ConversationActions.tsxapps/dashboard/app/conversations/[id]/page.tsxapps/dashboard/app/conversations/actions.tsapps/dashboard/app/conversations/page.tsxapps/dashboard/app/escalations/EscalationRow.tsxapps/dashboard/app/escalations/actions.tsapps/dashboard/app/escalations/page.tsxapps/dashboard/app/layout.tsxapps/dashboard/app/login/client.tsxapps/dashboard/app/login/page.tsxapps/dashboard/app/page.tsxapps/dashboard/components/kpi-card.tsxapps/dashboard/components/kpi-dashboard.tsxapps/dashboard/components/theme-provider.tsxapps/dashboard/components/theme-toggle.tsxapps/dashboard/package.json
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/repositories/conversation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/actions.ts
| </div> | ||
|
|
||
| <div className="space-y-4 bg-white p-6 shadow-sm rounded border min-h-100"> | ||
| <div className="min-h-100 space-y-4 rounded border border-gray-200 bg-white p-6 shadow-sm dark:border-gray-800 dark:bg-gray-900"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
configs=$(fd 'tailwind\.config\..*' . -HI)
if [ -n "$configs" ]; then
echo "== Tailwind configs =="
printf '%s\n' "$configs"
echo
echo "== Relevant config entries =="
rg -n "min-h-100|minHeight|spacing|theme\.extend" $configs || true
else
echo "No Tailwind config found"
fi
echo
echo "== Direct usages of min-h-100 =="
rg -n "min-h-100" . || trueRepository: Ryanakml/ChatFuse
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Search for Tailwind config in monorepo root and subdirectories
echo "== Searching for Tailwind configs in repository =="
find . -type f -name "tailwind.config.*" -o -name "tailwindcss.config.*" 2>/dev/null | head -20
echo
echo "== Checking package.json files for Tailwind configuration =="
fd 'package\.json' . -HI | while read pjson; do
if rg -q "tailwind" "$pjson"; then
echo "Found tailwind reference in: $pjson"
cat "$pjson"
fi
done | head -100
echo
echo "== Checking for PostCSS config with Tailwind =="
fd 'postcss\.config|\.postcssrc' . -HI | xargs cat 2>/dev/null || echo "No PostCSS config found"
echo
echo "== Checking for tailwind in any config files =="
rg -l "tailwind" . --type-list | head -20Repository: Ryanakml/ChatFuse
Length of output: 1756
🏁 Script executed:
#!/bin/bash
# Check for CSS files that might define min-h-100
echo "== Searching for CSS files =="
fd '\.(css|scss|sass)$' apps/dashboard -HI | head -10
echo
echo "== Checking CSS files for min-h-100 definition =="
rg -n "min-h-100" apps/dashboard --type-list 2>/dev/null || true
rg -n "min-h-100" . --type css || echo "No CSS file definitions found"
echo
echo "== Context around line 102 in the problematic file =="
head -110 "apps/dashboard/app/conversations/[id]/page.tsx" | tail -20Repository: Ryanakml/ChatFuse
Length of output: 1113
Remove min-h-100 or replace with min-h-[100px].
The min-h-100 class is not defined in Tailwind (no custom config extends it), so it has no effect. If you intended a 100px minimum height, use min-h-[100px] instead. Otherwise, remove it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/conversations/`[id]/page.tsx at line 102, Replace the
invalid Tailwind class on the Conversations page: locate the div in the
Conversations page component (page.tsx) whose className contains "min-h-100" and
either remove that token or change it to the valid Tailwind JIT form
"min-h-[100px]" so the minimum height takes effect.
Comment on lines
+11
to
+16
| const supabase = await createClient(); | ||
| const { | ||
| data: { session }, | ||
| } = await supabase.auth.getSession(); | ||
| if (!session?.access_token) { | ||
| return []; |
There was a problem hiding this comment.
Don't collapse auth/API failures into an empty queue.
getConversations() returns [] when the session is missing and also on fetch errors, so expired auth or backend outages render the same No active conversations. state as a healthy empty queue. That hides production failures from operators; bubble an explicit load error instead.
Also applies to: 23-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/conversations/page.tsx` around lines 11 - 16,
getConversations is currently returning [] both for missing/expired session and
for API/fetch failures, which collapses auth and backend errors into a benign
"empty queue" state; update the code paths in getConversations (and the
session-check in page.tsx around createClient()/supabase.auth.getSession()) so
that a missing/expired session or any non-successful fetch response throws or
returns an explicit error (or rejects) instead of returning an empty array,
while still returning [] only when the API responds successfully with an empty
conversations list; ensure you surface/throw the underlying error details for
fetch failures and expired/missing session cases so callers can render an error
state or bubble to monitoring (also apply the same change to the logic
referenced in the 23-36 block).
Comment on lines
+7
to
+18
| const API_BASE_URL = | ||
| process.env.API_URL || process.env.NEXT_PUBLIC_API_URL || 'http://localhost:3001'; | ||
|
|
||
| // Helper to make API calls to our backend proxying the auth | ||
| async function fetchApi(endpoint: string, options: RequestInit = {}) { | ||
| const supabase = await createClient(); | ||
| const { | ||
| data: { session }, | ||
| } = await supabase.auth.getSession(); | ||
| if (!session?.access_token) { | ||
| throw new Error('Unauthorized: missing access token'); | ||
| } |
There was a problem hiding this comment.
Don’t silently fall back to localhost in server actions.
If API_URL is missing in staging or production, this will target http://localhost:3001 and turn a config mistake into hard-to-diagnose runtime failures. Keep that fallback local-dev only, otherwise fail fast.
Suggested fix
const API_BASE_URL =
- process.env.API_URL || process.env.NEXT_PUBLIC_API_URL || 'http://localhost:3001';
+ process.env.API_URL ??
+ process.env.NEXT_PUBLIC_API_URL ??
+ (process.env.NODE_ENV === 'development' ? 'http://localhost:3001' : undefined);
// Helper to make API calls to our backend proxying the auth
async function fetchApi(endpoint: string, options: RequestInit = {}) {
+ if (!API_BASE_URL) {
+ throw new Error('Missing API base URL');
+ }
const supabase = await createClient();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/escalations/actions.ts` around lines 7 - 18, The
API_BASE_URL constant currently falls back to 'http://localhost:3001' which can
mask missing config in non-dev environments; change the initialization of
API_BASE_URL so that it uses process.env.API_URL ||
process.env.NEXT_PUBLIC_API_URL, and only fall back to 'http://localhost:3001'
when process.env.NODE_ENV === 'development' (or equivalent dev flag), otherwise
throw a clear error during module initialization; update the fetchApi usage to
rely on this new API_BASE_URL and ensure the thrown error message clearly
indicates a missing API_URL in staging/production so deployments fail fast.
Comment on lines
+9
to
+14
| const supabase = await createClient(); | ||
| const { | ||
| data: { session }, | ||
| } = await supabase.auth.getSession(); | ||
| if (!session?.access_token) { | ||
| return []; |
There was a problem hiding this comment.
Avoid showing Inbox Zero on load failures.
This helper returns [] for missing session and any fetch error, which makes auth expiry or API outages look like a healthy empty escalation inbox. That's dangerous for an ops screen; surface an error state instead of the normal empty-state copy.
Also applies to: 21-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/escalations/page.tsx` around lines 9 - 14, The current
helper silently returns [] on missing session or fetch errors (in the
createClient → supabase.auth.getSession flow), which hides auth expiry or API
failures; change the logic in the page-level loader (around createClient /
supabase.auth.getSession) and the follow-up fetch block (the other early-return
at lines ~21-34) to surface an error instead of returning an empty array: detect
a missing session or any fetch error and throw or return an error response
(e.g., throw new Error or return a server error Response) so the UI can show an
error state rather than the "Inbox Zero" empty copy; update the code paths
referencing createClient, supabase.auth.getSession, and the escalation fetch to
stop returning [] on failures and propagate an explicit error.
Comment on lines
40
to
43
| {errorMsg && ( | ||
| <div className="bg-red-50 text-red-600 p-3 rounded border border-red-200 text-sm"> | ||
| <div className="rounded border border-red-200 bg-red-50 p-3 text-sm text-red-600 dark:border-red-900 dark:bg-red-950/40 dark:text-red-300"> | ||
| {errorMsg} | ||
| </div> |
There was a problem hiding this comment.
Announce login failures to screen readers.
This message is only rendered visually right now. Adding a live region here makes failed sign-ins discoverable without forcing users to hunt for new text.
Suggested fix
- <div className="rounded border border-red-200 bg-red-50 p-3 text-sm text-red-600 dark:border-red-900 dark:bg-red-950/40 dark:text-red-300">
+ <div
+ role="alert"
+ className="rounded border border-red-200 bg-red-50 p-3 text-sm text-red-600 dark:border-red-900 dark:bg-red-950/40 dark:text-red-300"
+ >📝 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
| {errorMsg && ( | |
| <div className="bg-red-50 text-red-600 p-3 rounded border border-red-200 text-sm"> | |
| <div className="rounded border border-red-200 bg-red-50 p-3 text-sm text-red-600 dark:border-red-900 dark:bg-red-950/40 dark:text-red-300"> | |
| {errorMsg} | |
| </div> | |
| {errorMsg && ( | |
| <div | |
| role="alert" | |
| className="rounded border border-red-200 bg-red-50 p-3 text-sm text-red-600 dark:border-red-900 dark:bg-red-950/40 dark:text-red-300" | |
| > | |
| {errorMsg} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/login/client.tsx` around lines 40 - 43, The error message
rendered from errorMsg is currently only visual; make it an accessible live
region so screen readers announce failures by adding ARIA attributes to the
container (e.g., set role="alert" or aria-live="assertive" and
aria-atomic="true") on the div that renders {errorMsg} in the login component;
keep the existing styling and conditional rendering logic but add those
attributes to the element that contains errorMsg so assistive tech is
automatically notified of new error text.
Comment on lines
+11
to
+18
| <button | ||
| type="button" | ||
| onClick={() => setTheme(isDark ? 'light' : 'dark')} | ||
| className="inline-flex h-9 w-9 items-center justify-center rounded-md border border-gray-300 bg-white text-gray-700 transition-colors hover:bg-gray-100 dark:border-gray-700 dark:bg-gray-900 dark:text-gray-200 dark:hover:bg-gray-800" | ||
| aria-label="Toggle theme" | ||
| > | ||
| <Sun className="hidden h-4 w-4 dark:block" /> | ||
| <Moon className="h-4 w-4 dark:hidden" /> |
There was a problem hiding this comment.
Expose the current toggle state to assistive tech.
aria-label="Toggle theme" describes the action, but not whether dark mode is currently on. Adding pressed state, and ideally a destination label, gives screen-reader users the same context as the icon swap.
Suggested fix
- aria-label="Toggle theme"
+ aria-label={isDark ? 'Switch to light theme' : 'Switch to dark theme'}
+ aria-pressed={isDark}📝 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 | |
| type="button" | |
| onClick={() => setTheme(isDark ? 'light' : 'dark')} | |
| className="inline-flex h-9 w-9 items-center justify-center rounded-md border border-gray-300 bg-white text-gray-700 transition-colors hover:bg-gray-100 dark:border-gray-700 dark:bg-gray-900 dark:text-gray-200 dark:hover:bg-gray-800" | |
| aria-label="Toggle theme" | |
| > | |
| <Sun className="hidden h-4 w-4 dark:block" /> | |
| <Moon className="h-4 w-4 dark:hidden" /> | |
| <button | |
| type="button" | |
| onClick={() => setTheme(isDark ? 'light' : 'dark')} | |
| className="inline-flex h-9 w-9 items-center justify-center rounded-md border border-gray-300 bg-white text-gray-700 transition-colors hover:bg-gray-100 dark:border-gray-700 dark:bg-gray-900 dark:text-gray-200 dark:hover:bg-gray-800" | |
| aria-label={isDark ? 'Switch to light theme' : 'Switch to dark theme'} | |
| aria-pressed={isDark} | |
| > | |
| <Sun className="hidden h-4 w-4 dark:block" /> | |
| <Moon className="h-4 w-4 dark:hidden" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/components/theme-toggle.tsx` around lines 11 - 18, The button
currently only has aria-label and swaps icons (Sun, Moon) but doesn't expose
state; update the theme toggle button (the element using setTheme and isDark) to
include an accessible pressed state and clear destination text: set aria-pressed
to the boolean isDark and change aria-label to a dynamic string like "Switch to
light theme" when isDark is true and "Switch to dark theme" when isDark is false
so screen readers get both action and current/destination state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/routes/conversations.ts (1)
70-79: Remove or gate debug logging before merging.Lines 71-72 and 79 use
console.logfor debug information that will generate noise in production logs. Consider:
- Removing the debug statements entirely, or
- Gating them behind a
DEBUGenvironment variable, or- Using a proper logging library with configurable log levels.
The UUID validation itself (lines 73-77) and the
console.erroron line 74 are appropriate.♻️ Proposed fix to remove debug logging
try { const conversationId = req.params.id; - console.log('Timeline request received - ID:', conversationId); - console.log('Is valid UUID:', isUuid(conversationId)); if (!isUuid(conversationId)) { console.error('Invalid UUID format:', conversationId); res.status(400).json({ error: `Invalid conversation id: ${conversationId}` }); return; } const timeline = await conversationRepository.getConversationTimeline(conversationId); - console.log('Timeline fetched successfully, items:', timeline.length); res.json(timeline); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.ts` around lines 70 - 79, Remove or gate the debug console.log statements around the timeline handler: eliminate the two console.log calls that print 'Timeline request received - ID:' and 'Is valid UUID:' and the final 'Timeline fetched successfully...' console.log, or wrap them behind a DEBUG env check; keep the UUID validation and the existing console.error. Locate the code using the conversationId variable, the isUuid check, and the call to conversationRepository.getConversationTimeline to make the change, or replace console.log with the project's logger at an appropriate debug level if available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/conversations.ts`:
- Around line 89-90: The code currently falls back to 'unknown' for operatorId
which masks bugs and writes invalid data; instead assert that req.user exists
and use its id directly (set operatorId = req.user.id) so the request fails fast
if authenticateRequest didn't populate the user; update the same pattern in the
other handlers that set operatorId (the routes handling /:id/return and
/:id/messages) and ensure any use of operatorId when updating assigned_to or
emitting agent_events (e.g., in takeoverConversation and related event emission)
uses the asserted req.user.id rather than a string fallback.
---
Nitpick comments:
In `@apps/api/src/routes/conversations.ts`:
- Around line 70-79: Remove or gate the debug console.log statements around the
timeline handler: eliminate the two console.log calls that print 'Timeline
request received - ID:' and 'Is valid UUID:' and the final 'Timeline fetched
successfully...' console.log, or wrap them behind a DEBUG env check; keep the
UUID validation and the existing console.error. Locate the code using the
conversationId variable, the isUuid check, and the call to
conversationRepository.getConversationTimeline to make the change, or replace
console.log with the project's logger at an appropriate debug level if
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73c0e69f-d6a8-49ad-b83f-6e140c8bfae5
📒 Files selected for processing (2)
apps/api/src/routes/conversations.jsapps/api/src/routes/conversations.ts
Comment on lines
+89
to
+90
| // operatorId comes from the authenticated JWT token via authenticateRequest middleware | ||
| const operatorId = req.user?.id || 'unknown'; |
There was a problem hiding this comment.
Avoid falling back to 'unknown' for operator ID.
The authenticateRequest middleware guarantees req.user is populated before reaching these routes (returns 401 otherwise). Falling back to 'unknown' masks potential bugs and stores invalid data in the database:
- The
assigned_tofield would contain'unknown'instead of a valid UUID (per context intakeoverConversation) - Event payloads in
agent_eventswould have invalidoperatorIdvalues, breaking audit trails
Since the middleware guarantees req.user exists, assert it rather than silently degrading:
🐛 Proposed fix to fail fast on missing user
conversationsRouter.post('/:id/takeover', async (req, res) => {
try {
- // operatorId comes from the authenticated JWT token via authenticateRequest middleware
- const operatorId = req.user?.id || 'unknown';
+ // operatorId comes from the authenticated JWT token via authenticateRequest middleware
+ if (!req.user?.id) {
+ res.status(401).json({ error: 'Unauthorized: User ID not available' });
+ return;
+ }
+ const operatorId = req.user.id;
await conversationRepository.takeoverConversation(req.params.id, operatorId);Apply the same pattern to /:id/return and /:id/messages routes.
Also applies to: 101-101, 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/conversations.ts` around lines 89 - 90, The code
currently falls back to 'unknown' for operatorId which masks bugs and writes
invalid data; instead assert that req.user exists and use its id directly (set
operatorId = req.user.id) so the request fails fast if authenticateRequest
didn't populate the user; update the same pattern in the other handlers that set
operatorId (the routes handling /:id/return and /:id/messages) and ensure any
use of operatorId when updating assigned_to or emitting agent_events (e.g., in
takeoverConversation and related event emission) uses the asserted req.user.id
rather than a string fallback.
…endMessage error handling
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/routes/conversations.js (1)
36-52:⚠️ Potential issue | 🟠 MajorInconsistent authentication between endpoints and files.
The
takeoverandreturnendpoints still use header-based authentication (req.header('x-wa-user')), while themessagesendpoint (line 65) usesreq.user?.id. This is also inconsistent with the TypeScript version (conversations.ts) which usesreq.user?.idthroughout.🐛 Proposed fix to use consistent authentication
conversationsRouter.post('/:id/takeover', async (req, res) => { try { - // Note: in a real app, operatorId comes from the authenticated x-wa-user header - const operatorId = req.header('x-wa-user') || 'unknown'; + const operatorId = req.user?.id || 'unknown'; await conversationRepository.takeoverConversation(req.params.id, operatorId); res.json({ success: true }); }conversationsRouter.post('/:id/return', async (req, res) => { try { - const operatorId = req.header('x-wa-user') || 'unknown'; + const operatorId = req.user?.id || 'unknown'; await conversationRepository.returnToBot(req.params.id, operatorId); res.json({ success: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.js` around lines 36 - 52, The takeover and return endpoints use header-based auth (req.header('x-wa-user')) which is inconsistent with messages endpoint and the TypeScript variant; update both routes (conversationsRouter.post('/:id/takeover', ...) and conversationsRouter.post('/:id/return', ...)) to read the operator id from req.user?.id instead, keeping the existing fallback (e.g., 'unknown') if req.user or req.user.id is undefined, and remove reliance on the x-wa-user header so authentication is consistent across endpoints.
♻️ Duplicate comments (1)
apps/dashboard/app/conversations/actions.ts (1)
76-85:⚠️ Potential issue | 🟠 MajorHarden message validation (trim + type guard).
formData.get('content') as stringwithif (!content)allows whitespace-only messages and trusts type too aggressively. Normalize and validate before sending.✉️ Suggested fix
-export async function sendMessage(conversationId: string, formData: FormData): Promise<ActionResult> { - const content = formData.get('content') as string; - if (!content) { +export async function sendMessage(conversationId: string, formData: FormData): Promise<ActionResult> { + const rawContent = formData.get('content'); + if (typeof rawContent !== 'string' || !rawContent.trim()) { return { success: false, error: 'Message content required' }; } + const content = rawContent.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/conversations/actions.ts` around lines 76 - 85, The current message extraction trusts the type and allows whitespace-only content; change how you get and validate the message in the action: retrieve raw = formData.get('content'), ensure it's a string (type guard) then normalize with const content = raw.trim(); if content is empty return the same error object; only then call fetchWithAuth(`${API_BASE}/api/conversations/${conversationId}/messages`, ...) with JSON.stringify({ content }) so whitespace-only or non-string inputs are rejected before sending.
🧹 Nitpick comments (6)
apps/dashboard/app/conversations/actions.ts (1)
51-55: EncodeconversationIdbefore using it in URLs/paths.Interpolating raw
conversationIddirectly into endpoint and revalidation paths is brittle. Encode once and reuse for all three actions.🧩 Suggested fix
export async function takeoverConversation(conversationId: string): Promise<ActionResult> { + const safeConversationId = encodeURIComponent(conversationId); try { - await fetchWithAuth(`${API_BASE}/api/conversations/${conversationId}/takeover`, { + await fetchWithAuth(`${API_BASE}/api/conversations/${safeConversationId}/takeover`, { method: 'POST', }); - revalidatePath(`/conversations/${conversationId}`); + revalidatePath(`/conversations/${safeConversationId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/app/conversations/actions.ts` around lines 51 - 55, The code interpolates raw conversationId into the API endpoint and revalidation path; encode the id once and reuse it for all calls by creating an encoded variable (e.g., const encodedId = encodeURIComponent(conversationId)) and then use `${API_BASE}/api/conversations/${encodedId}/takeover` for fetchWithAuth and `/conversations/${encodedId}` for revalidatePath (and any other places in this file that use conversationId in URLs).apps/api/src/repositories/conversation.ts (2)
448-451: Inconsistent error handling pattern.Plain
Erroris thrown here, but immediately caught and wrapped bytoRepositoryErroron line 470. This double-wrapping is inefficient and may alter the error message. Consider throwing a repository-specific error or returning a more specific error type.♻️ Suggested improvement for consistency
const userId = conversation?.user_id as string | undefined; if (!userId) { - throw new Error(`conversation not found: ${conversationId}`); + throw toRepositoryError( + new Error(`conversation not found: ${conversationId}`), + 'load conversation recipient', + ); }Same pattern applies to line 465 for the phone number check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/repositories/conversation.ts` around lines 448 - 451, The current block throws a plain Error when userId is missing after resolving conversationId (const userId = conversation?.user_id ...), which later gets caught and wrapped by toRepositoryError; replace this with a repository-specific error (e.g., throw new RepositoryError(...) or a typed RepositoryNotFoundError) or return a specific error result so it isn't double-wrapped by toRepositoryError in the calling scope; apply the same change to the phone-number check path (the check at/around the phone number validation) and ensure callers expect the repository error type instead of a generic Error.
434-472: Consider combining queries to reduce database round-trips.The
getConversationRecipientPhonemethod makes two sequential database queries. For performance, consider using a single query with a join.♻️ Single query alternative
async getConversationRecipientPhone(conversationId: string): Promise<string> { const client = getClient(); try { - const { data: conversation, error: conversationError } = await client + const { data, error } = await client .from('conversations') - .select('user_id') + .select('users!inner(phone_number)') .eq('id', conversationId) .maybeSingle(); - if (conversationError) { - throw toRepositoryError(conversationError, 'load conversation recipient'); + if (error) { + throw toRepositoryError(error, 'load conversation recipient phone'); } - - const userId = conversation?.user_id as string | undefined; - if (!userId) { + + if (!data) { throw new Error(`conversation not found: ${conversationId}`); } - const { data: user, error: userError } = await client - .from('users') - .select('phone_number') - .eq('id', userId) - .maybeSingle(); - - if (userError) { - throw toRepositoryError(userError, 'load recipient phone'); - } - - const phoneNumber = user?.phone_number as string | undefined; + const phoneNumber = (data as { users: { phone_number?: string } }).users?.phone_number; if (!phoneNumber || phoneNumber.trim() === '') { - throw new Error(`recipient phone number not found for user: ${userId}`); + throw new Error(`recipient phone number not found for conversation: ${conversationId}`); } return phoneNumber;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/repositories/conversation.ts` around lines 434 - 472, getConversationRecipientPhone currently issues two sequential queries (one to conversations, one to users); combine them into a single joined query to avoid the extra round-trip: query the conversations table and select the related users.phone_number in one call (e.g. use client.from('conversations').select('users(phone_number)').eq('id', conversationId).maybeSingle()), then extract and validate the phone number, keep the same error handling by converting DB errors with toRepositoryError and throwing if phone is missing, and remove the second client.from('users') query and its associated variables (user, userError, phoneNumber).apps/api/src/routes/conversations.ts (3)
71-80: Remove or convert debug logging to structured logging.These
console.logstatements appear to be debug artifacts. The codebase uses a structured logger (loggerfrom@wa-chat/shared). Consider removing these or converting them to use the proper logger for consistency and production readiness.♻️ Suggested fix using structured logger
conversationsRouter.get('/:id/timeline', async (req, res) => { try { const conversationId = req.params.id; - console.log('Timeline request received - ID:', conversationId); - console.log('Is valid UUID:', isUuid(conversationId)); if (!isUuid(conversationId)) { - console.error('Invalid UUID format:', conversationId); res.status(400).json({ error: `Invalid conversation id: ${conversationId}` }); return; } const timeline = await conversationRepository.getConversationTimeline(conversationId); - console.log('Timeline fetched successfully, items:', timeline.length); res.json(timeline); } catch (error) { - console.error('Timeline error:', error); handleRepositoryError(res, error, 'Failed to fetch timeline'); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.ts` around lines 71 - 80, Replace the console.* debug statements in the conversations route with the project structured logger: remove console.log/console.error and use the imported logger from `@wa-chat/shared` instead, e.g., call logger.debug or logger.info for the "Timeline request received" and "Timeline fetched" messages and logger.error for the invalid UUID case (include the conversationId and isValid flag as structured fields). Update the branch that returns a 400 to log via logger.error before sending res.status(400).json, and ensure conversationRepository.getConversationTimeline usage and subsequent logging use the same logger methods.
142-150: Monitor partial success scenarios to prevent data loss.When WhatsApp outbound succeeds but persistence fails, the message is sent to the user but not recorded in the database. While returning
persisted: falseis reasonable, this scenario should be monitored/alerted to prevent data loss. Consider adding structured logging or metrics here.♻️ Add structured logging for monitoring
} catch (persistError) { - console.error('Manual outbound sent but failed to persist message:', persistError); + // Log with structured logger for alerting/monitoring + logger.error({ + event: 'outbound_message.persist_failed', + conversationId: req.params.id, + messageId: outboundResult.messageId, + error: persistError instanceof Error ? persistError.message : String(persistError), + }, 'Manual outbound sent but failed to persist message'); res.json({ success: true, messageId: outboundResult.messageId, persisted: false, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.ts` around lines 142 - 150, In the catch block handling persistence failures after a successful WhatsApp send (the catch with persistError that currently logs with console.error and returns res.json including outboundResult.messageId and persisted:false), replace the ad-hoc console.error with structured logging and emit a monitoring metric/alert so partial-successes are tracked; log a structured object containing messageId (outboundResult.messageId), conversationId/recipient (if available in scope), error details (persistError), and a clear event name (e.g., "message_persist_failure") using the project logger (e.g., processLogger or the app’s logger) and increment or emit a metric/monitoring event (e.g., metrics.increment or telemetry.record) so ops can detect and alert on these incidents while preserving the existing res.json response flow.
24-25: UUID validation excludes newer UUID versions.The regex only validates UUID versions 1-5. If UUID v6 or v7 (which are gaining adoption) are used for conversation IDs, they would be incorrectly rejected.
♻️ More permissive UUID validation
const isUuid = (value: string): boolean => - /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value); + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/conversations.ts` around lines 24 - 25, The isUuid function's regex restricts UUID versions to 1-5 and will reject newer versions; update the regex in isUuid to stop enforcing the version/variant nibbles and instead validate the generic 8-4-4-4-12 hex format (for example replace the current pattern with one that uses [0-9a-f] for those positions, e.g. /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i) so UUID v6/v7 (and future versions) are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/index.js`:
- Around line 499-505: The express.json() middleware is registered after the
/api routes so JSON bodies won't be parsed for routes like the admin-only
endpoint; move the app.use('/api', express.json()) call to above any route
registrations (e.g., before app.post('/api/protected/admin-only',
authenticateRequest, requireRole('admin'), ...)) so that req.body is populated
for handlers and before mounting routers such as conversationsRouter and
kpisRouter.
In `@apps/api/src/repositories/conversation.js`:
- Around line 135-145: The mock addOperatorMessage implementation has a
signature mismatch: function addOperatorMessage(conversationId, operatorId,
content) should accept the fourth parameter whatsappMessageId to match the
TypeScript/type declarations; update the function signature to add
whatsappMessageId and ensure the pushed mockTimeline[conversationId] entry
includes a whatsappMessageId property (and/or uses it when building the id if
desired) so the mockTimeline structure matches the real implementation and
types; locate and update the addOperatorMessage function and the mockTimeline
entries to include the new field.
In `@apps/api/src/services/whatsapp.ts`:
- Line 11: The Graph API version in the WhatsApp messages endpoint is pinned to
v22.0; update the endpoint string in the apps/api src service where the constant
endpoint is defined (variable name: endpoint in whatsapp.ts) to use v25.0
instead of v22.0 so the URL becomes
https://graph.facebook.com/v25.0/${input.phoneNumberId}/messages and ensures
compatibility with the current API version.
In `@apps/dashboard/app/conversations/`[id]/ConversationActions.tsx:
- Line 75: The error paragraph rendering (the JSX that shows actionError in
ConversationActions.tsx) isn't announced to assistive tech; update the element
that renders actionError (the <p> that uses {actionError}) to include an
accessibility live region attribute such as role="alert" or
aria-live="assertive" so screen readers announce the message immediately; ensure
you add the attribute to the same element that renders the variable actionError
in the ConversationActions component.
In `@apps/dashboard/app/conversations/actions.ts`:
- Around line 41-45: The current error path reads the raw response body (body ||
res.statusText) and throws it in the Error, which can leak internal backend
details to the UI; update the failure branch that handles res.ok to stop
forwarding raw res.text() to the thrown Error — replace the thrown message with
a generic, user-safe string (e.g., `API error (${res.status})`) and, if you need
diagnostics, log or attach a sanitized/truncated version of the response body to
server-side logs only; specifically change the block that constructs `details`
and the `throw new Error(...)` call so it no longer exposes the full upstream
response text.
- Around line 35-39: The outbound fetch lacks a timeout and can hang; wrap the
fetch call in an AbortController and timeout logic: create an AbortController,
pass controller.signal into the fetch options where res = await fetch(url, {
...options, headers }), start a setTimeout that calls controller.abort() after a
configurable API_REQUEST_TIMEOUT_MS (e.g., 10_000), clear the timeout after the
fetch resolves, and update the catch block to detect an abort (error.name ===
'AbortError') and throw a clear timeout Error (e.g., `API request timed out`)
instead of the generic network error; reference the fetch invocation and the
existing catch block in actions.ts and add a constant/API_REQUEST_TIMEOUT_MS for
configurability.
---
Outside diff comments:
In `@apps/api/src/routes/conversations.js`:
- Around line 36-52: The takeover and return endpoints use header-based auth
(req.header('x-wa-user')) which is inconsistent with messages endpoint and the
TypeScript variant; update both routes
(conversationsRouter.post('/:id/takeover', ...) and
conversationsRouter.post('/:id/return', ...)) to read the operator id from
req.user?.id instead, keeping the existing fallback (e.g., 'unknown') if
req.user or req.user.id is undefined, and remove reliance on the x-wa-user
header so authentication is consistent across endpoints.
---
Duplicate comments:
In `@apps/dashboard/app/conversations/actions.ts`:
- Around line 76-85: The current message extraction trusts the type and allows
whitespace-only content; change how you get and validate the message in the
action: retrieve raw = formData.get('content'), ensure it's a string (type
guard) then normalize with const content = raw.trim(); if content is empty
return the same error object; only then call
fetchWithAuth(`${API_BASE}/api/conversations/${conversationId}/messages`, ...)
with JSON.stringify({ content }) so whitespace-only or non-string inputs are
rejected before sending.
---
Nitpick comments:
In `@apps/api/src/repositories/conversation.ts`:
- Around line 448-451: The current block throws a plain Error when userId is
missing after resolving conversationId (const userId = conversation?.user_id
...), which later gets caught and wrapped by toRepositoryError; replace this
with a repository-specific error (e.g., throw new RepositoryError(...) or a
typed RepositoryNotFoundError) or return a specific error result so it isn't
double-wrapped by toRepositoryError in the calling scope; apply the same change
to the phone-number check path (the check at/around the phone number validation)
and ensure callers expect the repository error type instead of a generic Error.
- Around line 434-472: getConversationRecipientPhone currently issues two
sequential queries (one to conversations, one to users); combine them into a
single joined query to avoid the extra round-trip: query the conversations table
and select the related users.phone_number in one call (e.g. use
client.from('conversations').select('users(phone_number)').eq('id',
conversationId).maybeSingle()), then extract and validate the phone number, keep
the same error handling by converting DB errors with toRepositoryError and
throwing if phone is missing, and remove the second client.from('users') query
and its associated variables (user, userError, phoneNumber).
In `@apps/api/src/routes/conversations.ts`:
- Around line 71-80: Replace the console.* debug statements in the conversations
route with the project structured logger: remove console.log/console.error and
use the imported logger from `@wa-chat/shared` instead, e.g., call logger.debug or
logger.info for the "Timeline request received" and "Timeline fetched" messages
and logger.error for the invalid UUID case (include the conversationId and
isValid flag as structured fields). Update the branch that returns a 400 to log
via logger.error before sending res.status(400).json, and ensure
conversationRepository.getConversationTimeline usage and subsequent logging use
the same logger methods.
- Around line 142-150: In the catch block handling persistence failures after a
successful WhatsApp send (the catch with persistError that currently logs with
console.error and returns res.json including outboundResult.messageId and
persisted:false), replace the ad-hoc console.error with structured logging and
emit a monitoring metric/alert so partial-successes are tracked; log a
structured object containing messageId (outboundResult.messageId),
conversationId/recipient (if available in scope), error details (persistError),
and a clear event name (e.g., "message_persist_failure") using the project
logger (e.g., processLogger or the app’s logger) and increment or emit a
metric/monitoring event (e.g., metrics.increment or telemetry.record) so ops can
detect and alert on these incidents while preserving the existing res.json
response flow.
- Around line 24-25: The isUuid function's regex restricts UUID versions to 1-5
and will reject newer versions; update the regex in isUuid to stop enforcing the
version/variant nibbles and instead validate the generic 8-4-4-4-12 hex format
(for example replace the current pattern with one that uses [0-9a-f] for those
positions, e.g.
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i) so UUID v6/v7
(and future versions) are accepted.
In `@apps/dashboard/app/conversations/actions.ts`:
- Around line 51-55: The code interpolates raw conversationId into the API
endpoint and revalidation path; encode the id once and reuse it for all calls by
creating an encoded variable (e.g., const encodedId =
encodeURIComponent(conversationId)) and then use
`${API_BASE}/api/conversations/${encodedId}/takeover` for fetchWithAuth and
`/conversations/${encodedId}` for revalidatePath (and any other places in this
file that use conversationId in URLs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86dbee9e-eaad-4d03-ad3a-f763054a9255
📒 Files selected for processing (11)
apps/api/src/index.jsapps/api/src/index.tsapps/api/src/repositories/conversation.d.tsapps/api/src/repositories/conversation.jsapps/api/src/repositories/conversation.tsapps/api/src/routes/conversations.jsapps/api/src/routes/conversations.tsapps/api/src/services/whatsapp.jsapps/api/src/services/whatsapp.tsapps/dashboard/app/conversations/[id]/ConversationActions.tsxapps/dashboard/app/conversations/actions.ts
Comment on lines
+499
to
+505
| app.post('/api/protected/admin-only', authenticateRequest, requireRole('admin'), (req, res) => { | ||
| res.json({ ok: true }); | ||
| }); | ||
| // Parse JSON for API endpoints; webhook uses a dedicated parser to preserve raw body for signature checks. | ||
| app.use('/api', express.json()); | ||
| app.use('/api/conversations', conversationsRouter); | ||
| app.use('/api/kpis', kpisRouter); |
There was a problem hiding this comment.
Middleware ordering bug: JSON parser applied after route definition.
The express.json() middleware on line 503 is registered after the /api/protected/admin-only route on line 499. In Express, middleware only applies to routes defined after it. This means the admin-only route will not parse JSON request bodies—req.body will be undefined or empty for JSON payloads.
Move the JSON parser before any routes that need it.
🛠️ Proposed fix
-app.post('/api/protected/admin-only', authenticateRequest, requireRole('admin'), (req, res) => {
- res.json({ ok: true });
-});
// Parse JSON for API endpoints; webhook uses a dedicated parser to preserve raw body for signature checks.
app.use('/api', express.json());
+
+// Example RBAC-protected endpoint implementation
+app.post('/api/protected/admin-only', authenticateRequest, requireRole('admin'), (req, res) => {
+ res.json({ ok: true });
+});
+
app.use('/api/conversations', conversationsRouter);
app.use('/api/kpis', kpisRouter);📝 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
| app.post('/api/protected/admin-only', authenticateRequest, requireRole('admin'), (req, res) => { | |
| res.json({ ok: true }); | |
| }); | |
| // Parse JSON for API endpoints; webhook uses a dedicated parser to preserve raw body for signature checks. | |
| app.use('/api', express.json()); | |
| app.use('/api/conversations', conversationsRouter); | |
| app.use('/api/kpis', kpisRouter); | |
| // Parse JSON for API endpoints; webhook uses a dedicated parser to preserve raw body for signature checks. | |
| app.use('/api', express.json()); | |
| // Example RBAC-protected endpoint implementation | |
| app.post('/api/protected/admin-only', authenticateRequest, requireRole('admin'), (req, res) => { | |
| res.json({ ok: true }); | |
| }); | |
| app.use('/api/conversations', conversationsRouter); | |
| app.use('/api/kpis', kpisRouter); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/index.js` around lines 499 - 505, The express.json() middleware
is registered after the /api routes so JSON bodies won't be parsed for routes
like the admin-only endpoint; move the app.use('/api', express.json()) call to
above any route registrations (e.g., before
app.post('/api/protected/admin-only', authenticateRequest, requireRole('admin'),
...)) so that req.body is populated for handlers and before mounting routers
such as conversationsRouter and kpisRouter.
Comment on lines
+135
to
+145
| async addOperatorMessage(conversationId, operatorId, content) { | ||
| if (!mockTimeline[conversationId]) mockTimeline[conversationId] = []; | ||
| mockTimeline[conversationId].push({ | ||
| type: 'message', | ||
| id: `msg_op_${Date.now()}`, | ||
| conversationId, | ||
| senderRole: 'agent', | ||
| content, | ||
| createdAt: new Date().toISOString(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Signature mismatch with TypeScript implementation.
The JS mock's addOperatorMessage accepts 3 parameters, but the TypeScript implementation and type declarations expect 4 parameters (including whatsappMessageId). This inconsistency could cause confusion during testing or if the mock is used alongside the real implementation.
🐛 Proposed fix to align signatures
- async addOperatorMessage(conversationId, operatorId, content) {
+ async addOperatorMessage(conversationId, operatorId, content, whatsappMessageId) {
if (!mockTimeline[conversationId]) mockTimeline[conversationId] = [];
mockTimeline[conversationId].push({
type: 'message',
id: `msg_op_${Date.now()}`,
conversationId,
senderRole: 'agent',
content,
+ whatsappMessageId: whatsappMessageId ?? null,
createdAt: new Date().toISOString(),
});
}📝 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
| async addOperatorMessage(conversationId, operatorId, content) { | |
| if (!mockTimeline[conversationId]) mockTimeline[conversationId] = []; | |
| mockTimeline[conversationId].push({ | |
| type: 'message', | |
| id: `msg_op_${Date.now()}`, | |
| conversationId, | |
| senderRole: 'agent', | |
| content, | |
| createdAt: new Date().toISOString(), | |
| }); | |
| } | |
| async addOperatorMessage(conversationId, operatorId, content, whatsappMessageId) { | |
| if (!mockTimeline[conversationId]) mockTimeline[conversationId] = []; | |
| mockTimeline[conversationId].push({ | |
| type: 'message', | |
| id: `msg_op_${Date.now()}`, | |
| conversationId, | |
| senderRole: 'agent', | |
| content, | |
| whatsappMessageId: whatsappMessageId ?? null, | |
| createdAt: new Date().toISOString(), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/repositories/conversation.js` around lines 135 - 145, The mock
addOperatorMessage implementation has a signature mismatch: function
addOperatorMessage(conversationId, operatorId, content) should accept the fourth
parameter whatsappMessageId to match the TypeScript/type declarations; update
the function signature to add whatsappMessageId and ensure the pushed
mockTimeline[conversationId] entry includes a whatsappMessageId property (and/or
uses it when building the id if desired) so the mockTimeline structure matches
the real implementation and types; locate and update the addOperatorMessage
function and the mockTimeline entries to include the new field.
| to: string; | ||
| text: string; | ||
| }): Promise<WhatsAppSendResult> => { | ||
| const endpoint = `https://graph.facebook.com/v22.0/${input.phoneNumberId}/messages`; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest Facebook Graph API version for WhatsApp Business?
💡 Result:
As of March 14, 2026, the latest Facebook/Meta Graph API version used by the WhatsApp Business (Cloud) API is v25.0 (released February 18, 2026). [1], [2]
Sources:
[1] MobileMarketingReads (Feb 19, 2026) (mobilemarketingreads.com)
[2] Social Media Today (Feb 2026) (socialmediatoday.com)
Citations:
- 1: https://mobilemarketingreads.com/meta-overhauls-marketing-api-retires-legacy-reach-metrics-in-push-toward-views-standard/?utm_source=openai
- 2: https://www.socialmediatoday.com/news/meta-updates-marketing-api-to-align-with-latest-ad-shifts/812648/?utm_source=openai
Update Graph API version from v22.0 to v25.0.
The endpoint uses Graph API v22.0, but the latest stable version is v25.0 (released February 18, 2026). Update the endpoint to prevent deprecation and ensure access to current security patches and features.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/services/whatsapp.ts` at line 11, The Graph API version in the
WhatsApp messages endpoint is pinned to v22.0; update the endpoint string in the
apps/api src service where the constant endpoint is defined (variable name:
endpoint in whatsapp.ts) to use v25.0 instead of v22.0 so the URL becomes
https://graph.facebook.com/v25.0/${input.phoneNumberId}/messages and ensures
compatibility with the current API version.
| )} | ||
| </div> | ||
|
|
||
| {actionError && <p className="mb-4 text-sm text-red-600 dark:text-red-400">{actionError}</p>} |
There was a problem hiding this comment.
Make error feedback announced to assistive tech.
Line 75 renders errors visually, but it won’t be reliably announced by screen readers. Add role="alert" (or aria-live="assertive").
♿ Suggested fix
- {actionError && <p className="mb-4 text-sm text-red-600 dark:text-red-400">{actionError}</p>}
+ {actionError && (
+ <p role="alert" className="mb-4 text-sm text-red-600 dark:text-red-400">
+ {actionError}
+ </p>
+ )}📝 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
| {actionError && <p className="mb-4 text-sm text-red-600 dark:text-red-400">{actionError}</p>} | |
| {actionError && ( | |
| <p role="alert" className="mb-4 text-sm text-red-600 dark:text-red-400"> | |
| {actionError} | |
| </p> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/conversations/`[id]/ConversationActions.tsx at line 75,
The error paragraph rendering (the JSX that shows actionError in
ConversationActions.tsx) isn't announced to assistive tech; update the element
that renders actionError (the <p> that uses {actionError}) to include an
accessibility live region attribute such as role="alert" or
aria-live="assertive" so screen readers announce the message immediately; ensure
you add the attribute to the same element that renders the variable actionError
in the ConversationActions component.
Comment on lines
+35
to
+39
| res = await fetch(url, { ...options, headers }); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : 'Network error'; | ||
| throw new Error(`API network error: ${message}`); | ||
| } |
There was a problem hiding this comment.
Add a timeout to outbound API calls.
fetch has no timeout right now. A slow/unresponsive backend can stall this server action and tie up request capacity.
⏱️ Suggested fix
- let res: Response;
+ let res: Response;
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000);
try {
- res = await fetch(url, { ...options, headers });
+ res = await fetch(url, { ...options, headers, signal: controller.signal });
} catch (error) {
const message = error instanceof Error ? error.message : 'Network error';
throw new Error(`API network error: ${message}`);
+ } finally {
+ clearTimeout(timeoutId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/conversations/actions.ts` around lines 35 - 39, The
outbound fetch lacks a timeout and can hang; wrap the fetch call in an
AbortController and timeout logic: create an AbortController, pass
controller.signal into the fetch options where res = await fetch(url, {
...options, headers }), start a setTimeout that calls controller.abort() after a
configurable API_REQUEST_TIMEOUT_MS (e.g., 10_000), clear the timeout after the
fetch resolves, and update the catch block to detect an abort (error.name ===
'AbortError') and throw a clear timeout Error (e.g., `API request timed out`)
instead of the generic network error; reference the fetch invocation and the
existing catch block in actions.ts and add a constant/API_REQUEST_TIMEOUT_MS for
configurability.
Comment on lines
+41
to
+45
| if (!res.ok) { | ||
| const body = await res.text().catch(() => ''); | ||
| const details = body || res.statusText; | ||
| throw new Error(`API error (${res.status}): ${details}`); | ||
| } |
There was a problem hiding this comment.
Avoid surfacing raw backend error bodies to the UI.
The thrown message currently includes full upstream response text. Since this message is returned to the client-facing component, it can leak internal backend details.
🔒 Suggested fix
if (!res.ok) {
const body = await res.text().catch(() => '');
- const details = body || res.statusText;
- throw new Error(`API error (${res.status}): ${details}`);
+ console.error('Upstream API error', { status: res.status, body });
+ throw new Error(`API error (${res.status})`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/app/conversations/actions.ts` around lines 41 - 45, The
current error path reads the raw response body (body || res.statusText) and
throws it in the Error, which can leak internal backend details to the UI;
update the failure branch that handles res.ok to stop forwarding raw res.text()
to the thrown Error — replace the thrown message with a generic, user-safe
string (e.g., `API error (${res.status})`) and, if you need diagnostics, log or
attach a sanitized/truncated version of the response body to server-side logs
only; specifically change the block that constructs `details` and the `throw new
Error(...)` call so it no longer exposes the full upstream response text.
…ture, and setup instructions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
97-100: Make migration execution order explicitLines 97-100 rely on shell glob expansion order. It’s usually sorted, but making ordering explicit improves reproducibility across environments.
Proposed doc fix
-for f in supabase/migrations/*.sql; do +for f in $(ls -1 supabase/migrations/*.sql | sort -V); do echo "Applying $f" psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -f "$f" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 97 - 100, The migration loop using "for f in supabase/migrations/*.sql; do" relies on shell glob expansion order; change it to explicitly sort the migration filenames before applying them so execution order is reproducible. Modify the script that iterates over supabase/migrations/*.sql to pipe or list the filenames through a stable sort (e.g., version-aware sort) and then iterate and call psql "$DATABASE_URL" -v ON_ERROR_STOP=1 -f "$f" on each sorted entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 29-31: The three consecutive README items all start with
“Worker…”, making the list repetitive; edit the three lines so they vary
sentence openings for readability (e.g., change to “Consumes queue jobs…” for
apps/worker/src/queue/consumer.ts, “Extracts inbound payload, upserts
user/conversation, and stores message to Supabase” for the Supabase step, and
“Runs the LangChain pipeline (packages/llm/src/langchain/pipeline.ts):
normalization → retrieval → classification → confidence → routing → tool
execution → composition → policy”)—ensure the meaning remains identical while
avoiding the repeated “Worker” prefix.
- Around line 247-251: Update the README example to avoid port collision by
changing the dashboard fallback command's port from 3002 to 3003; specifically
edit the example command "npm --workspace apps/dashboard run dev -- --port 3002"
to use "--port 3003" so it doesn't conflict with the worker configured on port
3002 (referenced earlier in the README).
---
Nitpick comments:
In `@README.md`:
- Around line 97-100: The migration loop using "for f in
supabase/migrations/*.sql; do" relies on shell glob expansion order; change it
to explicitly sort the migration filenames before applying them so execution
order is reproducible. Modify the script that iterates over
supabase/migrations/*.sql to pipe or list the filenames through a stable sort
(e.g., version-aware sort) and then iterate and call psql "$DATABASE_URL" -v
ON_ERROR_STOP=1 -f "$f" on each sorted entry.
Comment on lines
+29
to
+31
| 5. Worker consumes queue jobs (`apps/worker/src/queue/consumer.ts`) with timeout + retry policies. | ||
| 6. Worker extracts inbound message payload, upserts user/conversation, and stores inbound message to Supabase. | ||
| 7. Worker runs the LangChain pipeline (`packages/llm/src/langchain/pipeline.ts`): normalization → retrieval → classification → confidence → routing → tool execution → composition → policy. |
There was a problem hiding this comment.
Tighten repeated sentence openings for readability
Lines 29-31 repeatedly begin with “Worker…”. Minor style issue, but easy readability win.
🧰 Tools
🪛 LanguageTool
[style] ~31-~31: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... stores inbound message to Supabase. 7. Worker runs the LangChain pipeline (`packages/...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 29 - 31, The three consecutive README items all start
with “Worker…”, making the list repetitive; edit the three lines so they vary
sentence openings for readability (e.g., change to “Consumes queue jobs…” for
apps/worker/src/queue/consumer.ts, “Extracts inbound payload, upserts
user/conversation, and stores message to Supabase” for the Supabase step, and
“Runs the LangChain pipeline (packages/llm/src/langchain/pipeline.ts):
normalization → retrieval → classification → confidence → routing → tool
execution → composition → policy”)—ensure the meaning remains identical while
avoiding the repeated “Worker” prefix.
Comment on lines
+247
to
+251
| If OTEL profile is enabled and port `3000` is taken by Grafana: | ||
|
|
||
| ```bash | ||
| npm --workspace apps/dashboard run dev -- --port 3002 | ||
| ``` |
There was a problem hiding this comment.
Fix dashboard fallback port to avoid collision with worker port
Line 250 suggests --port 3002, but Line 150 already configures the worker on 3002. Following both steps will fail one service bind.
Proposed doc fix
-If OTEL profile is enabled and port `3000` is taken by Grafana:
+If OTEL profile is enabled and port `3000` is taken by Grafana:
```bash
-npm --workspace apps/dashboard run dev -- --port 3002
+npm --workspace apps/dashboard run dev -- --port 3003</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @README.md around lines 247 - 251, Update the README example to avoid port
collision by changing the dashboard fallback command's port from 3002 to 3003;
specifically edit the example command "npm --workspace apps/dashboard run dev --
--port 3002" to use "--port 3003" so it doesn't conflict with the worker
configured on port 3002 (referenced earlier in the README).
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
This was referenced Mar 22, 2026
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.
…ages
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation