perf fix sciweave#3
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Route as API Route (GET/POST)
participant Handler as handleRequest(...)
participant DB as Database
Client->>Route: GET /api/sciweave-analytics/{chats|devices|sessions|users}?start&end
Route->>Handler: delegate(request)
Handler->>DB: Query (start/end, aggregations)
DB-->>Handler: Rows
Handler-->>Route: JSON
Route-->>Client: 200 OK
rect rgba(230,245,255,0.5)
note over Client,Route: Replica-compatible POST path
Client->>Route: POST ...?get=true
Route->>Handler: delegate(request)
Handler->>DB: Same query
DB-->>Handler: Rows
Handler-->>Route: JSON
Route-->>Client: 200 OK
end
Client->>Route: POST ... (no ?get=true)
Route-->>Client: 400 Bad Request (requires ?get=true)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/components/molecules/DesciResearchAnalytics.tsx (3)
124-142: Fix 0% trend handling and negative sign display.0 currently shows “No trend…”, and negatives render as “-3% down”. Treat 0 as valid and show absolute value for “down”.
- {props.trend ? ( + {props.trend !== undefined && props.trend !== null ? ( <> - {props.trend > 0 ? ( + {props.trend > 0 ? ( <> Trending up by{" "} <span className="text-green-500">{props.trend}%</span>{" "} <TrendingUp className="h-4 w-4" /> </> ) : ( <> Trending down by{" "} - <span className="text-red-500">{props.trend}%</span>{" "} + <span className="text-red-500">{Math.abs(props.trend)}%</span>{" "} <TrendingDown className="h-4 w-4" /> </> )} </> ) : ( "No trend data available" )}
195-205: Fix date parsing bug in devices chart (Invalid Date).You pre-format date to "dd MMM", then try new Date(value) in tick/tooltip formatters → “Invalid Date”. Keep Date objects in data and format only in formatters.
- const chartData = useMemo(() => { - return props.data.map((item) => ({ - date: formatDate(item.date, "dd MMM"), - mobile: item.mobile, - tablet: item.tablet, - mac: item.mac, - windows: item.windows, - otherDesktops: item.otherDesktops, - unknown: item.unknown, - })); - }, [props.data]); + const chartData = useMemo(() => { + return props.data.map((item) => ({ + date: item.date, // keep as Date + mobile: item.mobile, + tablet: item.tablet, + mac: item.mac, + windows: item.windows, + otherDesktops: item.otherDesktops, + unknown: item.unknown, + })); + }, [props.data]);- <XAxis + <XAxis dataKey="date" tickLine={false} axisLine={false} tickMargin={8} minTickGap={32} - tickFormatter={(value) => { - const date = new Date(value); - return date.toLocaleDateString("en-US", { - month: "short", - day: "numeric", - }); - }} + tickFormatter={(value: Date) => + new Date(value).toLocaleDateString("en-US", { + month: "short", + day: "numeric", + }) + } />- labelFormatter={(value) => { - return new Date(value).toLocaleDateString("en-US", { - month: "short", - day: "numeric", - }); - }} + labelFormatter={(value: Date) => + new Date(value).toLocaleDateString("en-US", { + month: "short", + day: "numeric", + }) + }Also applies to: 308-315, 320-326
418-444: Guard trend calculations against empty arrays and zero baselines.Current code will throw on empty data and can divide by zero.
- const trends = useMemo(() => { - const chatsTrend = - (Number(chats[chats.length - 1].value) - Number(chats[0].value)) / - Number(chats[0].value); - const uniqueUsersTrend = - (Number(uniqueUsers[uniqueUsers.length - 1].value) - - Number(uniqueUsers[0].value)) / - Number(uniqueUsers[0].value); - const userSessionsTrend = - (Number(userSessions[userSessions.length - 1].sessionCount) - - Number(userSessions[0].sessionCount)) / - Number(userSessions[0].sessionCount); - const userSessionsDurationTrend = - (Number( - userSessionsDurationData[userSessionsDurationData.length - 1].value - ) - - Number(userSessionsDurationData[0].value)) / - Number(userSessionsDurationData[0].value); - - return { - chats: Math.round(chatsTrend * 100), - uniqueUsers: Math.round(uniqueUsersTrend * 100), - userSessions: Math.round(userSessionsTrend * 100), - userSessionsDuration: Math.round(userSessionsDurationTrend * 100), - }; - }, [chats, uniqueUsers, userSessions, userSessionsDurationData]); + const trends = useMemo(() => { + const pct = (end: number, start: number) => + start === 0 ? 0 : Math.round(((end - start) / start) * 100); + + const safeFirst = <T>(arr: T[]) => (arr.length ? arr[0] : undefined); + const safeLast = <T>(arr: T[]) => (arr.length ? arr[arr.length - 1] : undefined); + + const c0 = safeFirst(chats)?.value; + const c1 = safeLast(chats)?.value; + const u0 = safeFirst(uniqueUsers)?.value; + const u1 = safeLast(uniqueUsers)?.value; + const s0 = safeFirst(userSessions)?.sessionCount; + const s1 = safeLast(userSessions)?.sessionCount; + const d0 = safeFirst(userSessionsDurationData)?.value; + const d1 = safeLast(userSessionsDurationData)?.value; + + return { + chats: c0 !== undefined && c1 !== undefined ? pct(Number(c1), Number(c0)) : 0, + uniqueUsers: u0 !== undefined && u1 !== undefined ? pct(Number(u1), Number(u0)) : 0, + userSessions: s0 !== undefined && s1 !== undefined ? pct(Number(s1), Number(s0)) : 0, + userSessionsDuration: d0 !== undefined && d1 !== undefined ? pct(Number(d1), Number(d0)) : 0, + }; + }, [chats, uniqueUsers, userSessions, userSessionsDurationData]);src/app/(auth)/metrics/sciweave/page.tsx (1)
133-135: Align client-side date range with server’s exclusive end boundary.Server changed to created_at < $2. Use startOfDay(next day) instead of endOfDay(current), to include the selected “to” day completely and avoid microsecond edge cases.
- const normalizedFrom = startOfDay(fromDate, { in: tz("UTC") }).toISOString(); - const normalizedTo = endOfDay(toDate, { in: tz("UTC") }).toISOString(); + const normalizedFrom = startOfDay(fromDate, { in: tz("UTC") }).toISOString(); + const normalizedTo = startOfDay(addDays(toDate, 1), { in: tz("UTC") }).toISOString();Add missing import:
// at top import { startOfDay, subDays, addDays } from "date-fns";src/app/api/sciweave-analytics/chats/route.ts (1)
19-41: Prevent connection leaks; use pool.query and cast counts.client.release() is skipped on thrown queries → leaked connections. Also COUNT(*) returns text in pg; cast to integer.
- const client = await pool.connect(); - const result = await client.query( + const result = await pool.query( ` SELECT - DATE_TRUNC($3, created_at) AS DATE, - COUNT(*) AS value + DATE_TRUNC($3, created_at) AS date, + COUNT(*)::integer AS value FROM public.search_logs WHERE thread_id IS NULL AND created_at >= $1 AND created_at < $2 ${IS_PROD ? "AND username NOT LIKE '%@desci.com'" : ""} GROUP BY - DATE + date ORDER BY - DATE + date `, [from, to, interval] ); - client.release(); - const data = result.rows as { day: string; value: number }[]; + const data = result.rows as { date: string; value: number }[];src/app/api/sciweave-analytics/users/route.ts (1)
19-41: Prevent connection leaks; use pool.query and cast counts.Same leak risk and text-return issue as chats.
- const client = await pool.connect(); - const result = await client.query( + const result = await pool.query( ` - SELECT - DATE_TRUNC($3, created_at) AS DATE, - COUNT(DISTINCT username) AS value + SELECT + DATE_TRUNC($3, created_at) AS date, + COUNT(DISTINCT username)::integer AS value FROM public.search_logs WHERE thread_id IS NULL AND created_at >= $1 AND created_at < $2 ${IS_PROD ? "AND username NOT LIKE '%@desci.com'" : ""} GROUP BY - DATE + date ORDER BY - DATE + date `, [from, to, interval] ); - client.release(); - const data = result.rows as { day: string; value: number }[]; + const data = result.rows as { date: string; value: number }[];src/app/api/sciweave-analytics/sessions/route.ts (1)
19-28: Prevent connection leaks; use pool.query.Swap to pool.query to avoid leaking clients on errors.
- const client = await pool.connect(); - const result = await client.query( + const result = await pool.query( ` @@ - SELECT - session_value as DATE, - COUNT(*)::integer AS "sessionCount", - ROUND(AVG(session_duration_sec), 2)::integer AS "durationInSeconds" + SELECT + session_value AS date, + COUNT(*)::integer AS "sessionCount", + ROUND(AVG(session_duration_sec))::integer AS "durationInSeconds" @@ - client.release(); - const data = result.rows as { - date: Date; + const data = result.rows as { + date: Date; sessionCount: number; durationInSeconds: number; }[];Also applies to: 106-118, 122-128
src/app/api/sciweave-analytics/devices/route.ts (1)
19-21: Always release the PG client in finally (prevent connection leaks).If anything throws between connect and release, the client is never returned to the pool.
- const client = await pool.connect(); - const result = await client.query( + const client = await pool.connect(); + let result; + try { + result = await client.query( ` ... - [from, to, interval] - ); - client.release(); + [from, to, interval] + ); + } finally { + client.release(); + }Also applies to: 39-39
🧹 Nitpick comments (12)
README.md (1)
6-8: Mention .nvmrc usage to avoid version drift.Since you pinned Node, tell devs to run nvm use before yarn to ensure matching runtime.
```bash -yarn -yarn dev +nvm use +yarn +yarn dev</blockquote></details> <details> <summary>src/components/molecules/DesciResearchAnalytics.tsx (1)</summary><blockquote> `554-555`: **Grammar nit: “each sessions” → “each session”.** ```diff - description="Analysis of devices used in each sessions for the selected period" + description="Analysis of devices used in each session for the selected period"src/app/(auth)/metrics/sciweave/page.tsx (2)
34-45: Make fetches consistent and resilient (match sessions handler).Mirror the robust checks you added to getUserSessionsAnalytics: gate on res.ok and verify array payloads.
- const res = await fetch( + const res = await fetch( `${process.env.NEXT_PUBLIC_BASE_URL}/api/sciweave-analytics/chats?${params.toString()}`, { next: { revalidate: 3600 } } ); - return res.json() as Promise<DataItem[]>; + if (!res.ok) { + console.error("Failed to fetch chats:", res.status, res.statusText); + return []; + } + const data = await res.json(); + return Array.isArray(data) ? (data as DataItem[]) : [];- const res = await fetch( + const res = await fetch( `${process.env.NEXT_PUBLIC_BASE_URL}/api/sciweave-analytics/users?${params.toString()}`, { next: { revalidate: 3600 } } ); - return res.json() as Promise<DataItem[]>; + if (!res.ok) { + console.error("Failed to fetch unique users:", res.status, res.statusText); + return []; + } + const data = await res.json(); + return Array.isArray(data) ? (data as DataItem[]) : [];- const res = await fetch( + const res = await fetch( `${process.env.NEXT_PUBLIC_BASE_URL}/api/sciweave-analytics/devices?${params.toString()}`, { next: { revalidate: 0 } } ); - return res.json() as Promise<DevicesDataItem[]>; + if (!res.ok) { + console.error("Failed to fetch devices:", res.status, res.statusText); + return []; + } + const data = await res.json(); + return Array.isArray(data) ? (data as DevicesDataItem[]) : [];Also applies to: 56-67, 110-121
123-131: Type of searchParams should not be Promise.Next.js App Router passes searchParams as a plain object; the Promise type is misleading and can mask type issues.
-export default async function DesciResearch({ +export default async function DesciResearch({ searchParams, }: { - searchParams: Promise<{ from: string; to: string; interval: string }>; + searchParams: { from?: string; to?: string; interval?: string }; }) { - const { from, to, interval } = await searchParams; + const { from, to, interval } = searchParams;src/app/api/sciweave-analytics/chats/route.ts (1)
42-48: Clarify error message.“chats over time” is fine here, but mirror this precise phrasing across routes for consistency.
src/app/api/sciweave-analytics/users/route.ts (1)
42-48: Correct copy-paste error in log/response text.These should say “unique users over time”.
- console.error("Error fetching chats over time:", error); + console.error("Error fetching unique users over time:", error); @@ - { error: "Failed to fetch chats over time" }, + { error: "Failed to fetch unique users over time" },src/app/api/sciweave-analytics/sessions/route.ts (2)
129-135: Clarify error message.Say “sessions over time” to match the route.
- console.error("Error fetching chats over time:", error); + console.error("Error fetching sessions over time:", error); @@ - { error: "Failed to fetch chats over time" }, + { error: "Failed to fetch sessions over time" },
47-49: Confirm internal email filter logic and excluded list governance.The static excluded array plus IS_PROD filter can drift. Consider sourcing from config or DB.
src/app/api/sciweave-analytics/devices/route.ts (4)
33-33: Don’t drop NULL usernames when excluding internal traffic.
username NOT LIKE ...is NULL when username is NULL, removing those rows. Coalesce to keep them.- ${IS_PROD ? "AND username NOT LIKE '%@desci.com'" : ""} + ${IS_PROD ? "AND COALESCE(username, '') NOT LIKE '%@desci.com'" : ""}
15-17: Add quick input sanity checks (from < to, small max window).Pre-empt bad queries and protect the DB.
const { from, to, interval } = querySchema.parse( Object.fromEntries(searchParams) ); + if (from >= to) { + return NextResponse.json({ error: "'from' must be before 'to'." }, { status: 400 }); + } + // Optional: cap window to e.g. 366 days + if (to.getTime() - from.getTime() > 366 * 24 * 60 * 60 * 1000) { + return NextResponse.json({ error: "Date range too large." }, { status: 400 }); + }
52-55: Fix copy/paste: “chats” → “devices”.Aligns logs and response with this route.
- console.error("Error fetching chats over time:", error); + console.error("Error fetching device analytics over time:", error); @@ - { error: "Failed to fetch chats over time" }, + { error: "Failed to fetch device analytics over time" },
64-76: Consider 405 Method Not Allowed for POST without ?get=true.HTTP 405 is more semantically correct; optionally include Allow header.
- return NextResponse.json( - { error: "POST method requires ?get=true parameter for replica compatibility" }, - { status: 400 } - ); + return NextResponse.json( + { error: "POST method requires ?get=true parameter for replica compatibility" }, + { status: 405 /*, headers: { 'Allow': 'GET, POST' } */ } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.env.example(1 hunks).nvmrc(1 hunks)README.md(1 hunks)src/app/(auth)/metrics/sciweave/page.tsx(1 hunks)src/app/api/sciweave-analytics/chats/route.ts(3 hunks)src/app/api/sciweave-analytics/devices/route.ts(3 hunks)src/app/api/sciweave-analytics/sessions/route.ts(3 hunks)src/app/api/sciweave-analytics/users/route.ts(3 hunks)src/components/molecules/DesciResearchAnalytics.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/app/api/sciweave-analytics/chats/route.ts (3)
src/app/api/sciweave-analytics/devices/route.ts (2)
GET(60-62)POST(64-76)src/app/api/sciweave-analytics/sessions/route.ts (2)
GET(138-140)POST(142-154)src/app/api/sciweave-analytics/users/route.ts (2)
GET(51-53)POST(55-67)
src/app/api/sciweave-analytics/users/route.ts (3)
src/app/api/sciweave-analytics/chats/route.ts (2)
GET(51-53)POST(55-67)src/app/api/sciweave-analytics/devices/route.ts (2)
GET(60-62)POST(64-76)src/app/api/sciweave-analytics/sessions/route.ts (2)
GET(138-140)POST(142-154)
src/app/api/sciweave-analytics/sessions/route.ts (3)
src/app/api/sciweave-analytics/chats/route.ts (2)
GET(51-53)POST(55-67)src/app/api/sciweave-analytics/devices/route.ts (2)
GET(60-62)POST(64-76)src/app/api/sciweave-analytics/users/route.ts (2)
GET(51-53)POST(55-67)
src/app/api/sciweave-analytics/devices/route.ts (3)
src/app/api/sciweave-analytics/chats/route.ts (2)
GET(51-53)POST(55-67)src/app/api/sciweave-analytics/sessions/route.ts (2)
GET(138-140)POST(142-154)src/app/api/sciweave-analytics/users/route.ts (2)
GET(51-53)POST(55-67)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 23-23: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (10)
.nvmrc (1)
1-1: Pin a full Node semver and add a trailing newline in .nvmrc22.15.0 was released Apr 23, 2025, but the latest 22.x as of Sep 11, 2025 is v22.19.0 (Aug 28, 2025). Choose one:
- To pin the intended minor: replace
22.15→22.15.0and add a trailing newline.- To use the current latest 22.x: replace with
22.19.0and add a trailing newline.src/app/(auth)/metrics/sciweave/page.tsx (1)
115-118: Revalidate policy check.Devices endpoint uses revalidate: 0 while others use 3600. If unintentional, align them; if intentional, consider documenting the rationale.
src/app/api/sciweave-analytics/chats/route.ts (2)
51-67: POST get=true gate LGTM.
31-31: Confirm internal email filter logic.Ensure excluding “%@desci.com” in IS_PROD matches product intent; otherwise move to a configurable allowlist/regex.
src/app/api/sciweave-analytics/users/route.ts (2)
51-67: POST get=true gate LGTM.
31-31: Confirm internal email filter logic.Same note as chats route.
src/app/api/sciweave-analytics/sessions/route.ts (2)
146-154: POST get=true gate LGTM.
48-50: End-time exclusive change confirmed.created_at < $2 aligns with UI suggestion to use next-day start. Good.
src/app/api/sciweave-analytics/devices/route.ts (2)
60-62: GET delegator — LGTM.Simple and consistent with sibling routes.
40-48: ```shell
#!/bin/bash
set -euo pipefailecho "=== show src/lib/postgresClient.ts ==="
if [ -f src/lib/postgresClient.ts ]; then
sed -n '1,240p' src/lib/postgresClient.ts || true
else
echo "file not found: src/lib/postgresClient.ts"
fiecho
echo "=== search repo for pg type parser config / pg.types usage ==="
rg -n 'setTypeParser' || true
rg -n 'types.setTypeParser' || true
rg -n 'pg.types' || trueecho
echo "=== search repo for created_at / timestamptz / timestamp usages (migrations/schema) ==="
rg -n -S 'created_at' || true
rg -n -S 'timestamptz' || true
rg -n -S 'timestamp without time zone' || true
rg -n -S 'CREATE TABLE' || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+18
to
+23
| DESCI_RESEARCH_CONNECTION_DATABASE_URL="" | ||
|
|
||
| NEXT_PUBLIC_BASE_URL=http://localhost:3000 No newline at end of file | ||
| NEXT_PUBLIC_BASE_URL=http://localhost:3000 | ||
|
|
||
| # supabase for search_logs sciweave | ||
| DESCI_RESEARCH_DATABASE_URL= No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Resolve env var duplication and add EOF newline.
You have both DESCI_RESEARCH_CONNECTION_DATABASE_URL and DESCI_RESEARCH_DATABASE_URL. This invites misconfiguration.
- Pick one canonical name (prefer the new DESCI_RESEARCH_DATABASE_URL) and remove/alias the other across the codebase.
- Add a blank line at file end (dotenv-linter EndingBlankLine).
Proposed change:
-DESCI_RESEARCH_CONNECTION_DATABASE_URL=""
+## (deprecated) DESCI_RESEARCH_CONNECTION_DATABASE_URL="" # consider removing
@@
-# supabase for search_logs sciweave
-DESCI_RESEARCH_DATABASE_URL=
+# supabase for search_logs sciweave
+DESCI_RESEARCH_DATABASE_URL=
+To verify usage across the repo:
🏁 Script executed:
#!/bin/bash
# Find all references to both env names
rg -nP 'DESCI_RESEARCH_(CONNECTION_)?DATABASE_URL' -C2Length of output: 256
Remove unused DESCI_RESEARCH_CONNECTION_DATABASE_URL from .env.example and add trailing newline
Repo search: only DESCI_RESEARCH_DATABASE_URL is referenced (src/lib/postgresClient.ts). Remove or mark DESCI_RESEARCH_CONNECTION_DATABASE_URL in .env.example and add a single blank line at EOF.
Location: .env.example (lines 18–23)
Proposed change:
-DESCI_RESEARCH_CONNECTION_DATABASE_URL=""
+## (deprecated) DESCI_RESEARCH_CONNECTION_DATABASE_URL="" # consider removing
@@
-# supabase for search_logs sciweave
-DESCI_RESEARCH_DATABASE_URL=
+# supabase for search_logs sciweave
+DESCI_RESEARCH_DATABASE_URL=
+📝 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
| DESCI_RESEARCH_CONNECTION_DATABASE_URL="" | |
| NEXT_PUBLIC_BASE_URL=http://localhost:3000 | |
| \ No newline at end of file | |
| NEXT_PUBLIC_BASE_URL=http://localhost:3000 | |
| # supabase for search_logs sciweave | |
| DESCI_RESEARCH_DATABASE_URL= | |
| ## (deprecated) DESCI_RESEARCH_CONNECTION_DATABASE_URL="" # consider removing | |
| NEXT_PUBLIC_BASE_URL=http://localhost:3000 | |
| # supabase for search_logs sciweave | |
| DESCI_RESEARCH_DATABASE_URL= | |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 18-18: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 23-23: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
In .env.example around lines 18 to 23, remove the unused
DESCI_RESEARCH_CONNECTION_DATABASE_URL entry (it is not referenced anywhere;
only DESCI_RESEARCH_DATABASE_URL is used) and ensure the file ends with a single
trailing newline (add one blank line at EOF if missing); simply delete or
comment out the DESCI_RESEARCH_CONNECTION_DATABASE_URL line and save the file
with a final newline.
Fixes FUNCTION_INVOCATION_TIMEOUT on Vercel by adding maxDuration=300 and streaming the backend response body instead of buffering.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/app/api/download/route.ts (1)
7-35: Consider extracting shared logic to reduce duplication.Both
download/route.tsanddownloadAnalyticsReport/route.tsshare nearly identical fetch-and-stream logic. A shared utility would improve maintainability.Example shared utility
// lib/streamCsvFromBackend.ts export async function streamCsvFromBackend( endpoint: string, cookieHeader: string ): Promise<Response> { const response = await fetch(endpoint, { credentials: "include", headers: { cookie: cookieHeader, "Content-Type": "application/json", }, }); if (!response.ok || !response.body) { return NextResponse.json( { error: "Failed to fetch report" }, { status: response.ok ? 502 : response.status } ); } return new Response(response.body, { headers: { "Content-Type": "text/csv", "Content-Disposition": 'attachment; filename="report.csv"', }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/download/route.ts` around lines 7 - 35, Extract the duplicated fetch-and-stream logic into a shared utility (e.g., streamCsvFromBackend) and have the GET handler in this file call that utility instead of duplicating code; specifically, create streamCsvFromBackend(endpoint: string, cookieHeader: string) that performs the fetch with credentials and cookie header, checks response.ok and response.body, returns a NextResponse.json({ error: "Failed to fetch report" }, { status: response.ok ? 502 : response.status }) on failure, or returns new Response(response.body, { headers: { "Content-Type": "text/csv", "Content-Disposition": 'attachment; filename="report.csv"' } }) on success, then update this file's GET to call streamCsvFromBackend(`${NODES_API_URL}/v1/admin/analytics/csv`, cookies().toString()) and forward the returned Response; also update downloadAnalyticsReport/route.ts to use the same utility to remove duplication and ensure the catch block returns a JSON error message string (not the raw error object).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/download/route.ts`:
- Around line 33-35: The catch block in the download route returns the raw
caught error object which serializes to {} (see the catch in download/route.ts);
update the catch to extract a serializable message instead—use the caught
variable (err) and pass err instanceof Error ? err.message : String(err) (or
similar) into NextResponse.json({ error: ... }, { status: 500 }) so the response
includes the actual error text; locate the catch surrounding NextResponse.json
in download/route.ts and replace the returned error payload accordingly.
- Around line 20-25: The current check returns response.status even when
response.ok is true but response.body is missing, which can be misleading;
update the block that checks response.ok and response.body (the lines using
response.ok, response.body and NextResponse.json) to specifically handle the
"body missing" case by returning a clear error and a 502 (or other appropriate
server-error) status instead of using response.status—ensure the
NextResponse.json call communicates "Missing response body" (or similar) and
uses a fixed 502 status when response.ok is true but response.body is falsy.
In `@src/app/api/downloadAnalyticsReport/route.ts`:
- Around line 34-36: The catch block in the downloadAnalyticsReport route
currently returns the raw Error instance (err) which serializes to an empty
object; change the error payload to a serializable object (e.g., { name:
err.name, message: err.message, stack: err.stack }) and return that via
NextResponse.json instead of err so consumers receive useful error details;
update the catch in route.ts (the handler that uses NextResponse.json) to build
and return this serializable error object.
- Around line 21-26: The current check returns response.status even when
response.ok is true but response.body is missing, which can misleadingly return
200; update the handling in route.ts around the fetch response so that you
distinguish the two cases: if !response.ok return NextResponse.json({...}, {
status: response.status }); else if response.ok but !response.body return
NextResponse.json({ error: "Failed to fetch report" }, { status: 502 });
reference the same response and NextResponse.json calls in this file so the
missing-body case uses a 502 (or other non-200) status instead of returning
response.status.
---
Nitpick comments:
In `@src/app/api/download/route.ts`:
- Around line 7-35: Extract the duplicated fetch-and-stream logic into a shared
utility (e.g., streamCsvFromBackend) and have the GET handler in this file call
that utility instead of duplicating code; specifically, create
streamCsvFromBackend(endpoint: string, cookieHeader: string) that performs the
fetch with credentials and cookie header, checks response.ok and response.body,
returns a NextResponse.json({ error: "Failed to fetch report" }, { status:
response.ok ? 502 : response.status }) on failure, or returns new
Response(response.body, { headers: { "Content-Type": "text/csv",
"Content-Disposition": 'attachment; filename="report.csv"' } }) on success, then
update this file's GET to call
streamCsvFromBackend(`${NODES_API_URL}/v1/admin/analytics/csv`,
cookies().toString()) and forward the returned Response; also update
downloadAnalyticsReport/route.ts to use the same utility to remove duplication
and ensure the catch block returns a JSON error message string (not the raw
error object).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87af05f3-a825-4d23-8a9a-1bb1061b1e62
📒 Files selected for processing (2)
src/app/api/download/route.tssrc/app/api/downloadAnalyticsReport/route.ts
Comment on lines
33
to
35
| } catch (err) { | ||
| return NextResponse.json({ error: err }, { status: 500 }); | ||
| } |
There was a problem hiding this comment.
Same error serialization issue as in downloadAnalyticsReport/route.ts.
Error objects serialize to {}. Extract err.message instead (see comment in the other file for proposed fix).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/download/route.ts` around lines 33 - 35, The catch block in the
download route returns the raw caught error object which serializes to {} (see
the catch in download/route.ts); update the catch to extract a serializable
message instead—use the caught variable (err) and pass err instanceof Error ?
err.message : String(err) (or similar) into NextResponse.json({ error: ... }, {
status: 500 }) so the response includes the actual error text; locate the catch
surrounding NextResponse.json in download/route.ts and replace the returned
error payload accordingly.
Comment on lines
34
to
36
| } catch (err) { | ||
| return NextResponse.json({ error: err }, { status: 500 }); | ||
| } |
There was a problem hiding this comment.
Error objects don't serialize to JSON properly.
Error instances serialize to {} when passed to JSON.stringify. This will produce an unhelpful response like { "error": {} }.
Proposed fix
} catch (err) {
- return NextResponse.json({ error: err }, { status: 500 });
+ return NextResponse.json(
+ { error: err instanceof Error ? err.message : "Internal server error" },
+ { status: 500 }
+ );
}📝 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
| } catch (err) { | |
| return NextResponse.json({ error: err }, { status: 500 }); | |
| } | |
| } catch (err) { | |
| return NextResponse.json( | |
| { error: err instanceof Error ? err.message : "Internal server error" }, | |
| { status: 500 } | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/downloadAnalyticsReport/route.ts` around lines 34 - 36, The catch
block in the downloadAnalyticsReport route currently returns the raw Error
instance (err) which serializes to an empty object; change the error payload to
a serializable object (e.g., { name: err.name, message: err.message, stack:
err.stack }) and return that via NextResponse.json instead of err so consumers
receive useful error details; update the catch in route.ts (the handler that
uses NextResponse.json) to build and return this serializable error object.
Take develop's PROD_FILTER constants, supabase env vars, and yarn.lock.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/api/sciweave-analytics/devices/route.ts (2)
46-49:⚠️ Potential issue | 🟡 MinorCorrect error context in logs/response
This devices endpoint currently reports
"chats over time"errors, which is misleading during incident/debug workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/sciweave-analytics/devices/route.ts` around lines 46 - 49, The error handling in the devices route logs and returns a message referencing "chats over time", which is misleading; update the console.error and the NextResponse.json error message inside the GET handler (or exported route function) to reflect the correct context such as "devices over time" or "devices metrics" (update the console.error("Error fetching chats over time:", error) and the corresponding NextResponse.json({ error: "Failed to fetch chats over time" }, { status: 500 }) to use the correct wording), ensuring the error text matches the endpoint purpose.
13-33:⚠️ Potential issue | 🟠 MajorRelease DB connection in a
finallyblockThe database connection is not released if query execution fails. When
client.query()throws an error (lines 14–32), the exception is caught by the outer try-catch block, butclient.release()on line 33 is skipped, leaving the connection unreleased in the pool and risking pool exhaustion under error load.Suggested fix
- const client = await pool.connect(); - const result = await client.query( + const client = await pool.connect(); + try { + const result = await client.query( ` @@ - [from, to, intervalToDateTrunc(interval)] - ); - client.release(); - const data = result.rows as { + [from, to, intervalToDateTrunc(interval)] + ); + const data = result.rows as { date: Date; mobile: number; tablet: number; mac: number; windows: number; otherDesktops: number; unknown: number; - }[]; - - return NextResponse.json(data); + }[]; + + return NextResponse.json(data); + } finally { + client.release(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/sciweave-analytics/devices/route.ts` around lines 13 - 33, The DB client acquired via pool.connect() in the route handler must be released in a finally block to ensure it is returned to the pool even if client.query(...) throws; refactor the code around pool.connect(), client.query(...) and client.release() so you call client.release() from a finally (or guard with if (client) client.release()) after the try that runs the query (e.g., the block surrounding the SELECT using intervalToDateTrunc), leaving error handling in the existing catch but guaranteeing release.src/app/api/sciweave-analytics/users/route.ts (2)
37-40:⚠️ Potential issue | 🟡 MinorFix route-specific error message
This users endpoint logs/returns
"chats over time"errors, which makes triage misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/sciweave-analytics/users/route.ts` around lines 37 - 40, The error handling in the users API route incorrectly logs and returns a generic "chats over time" message; update the console.error call and the NextResponse.json error message to reflect this users endpoint (e.g., "Failed to fetch users" or "Error fetching users") so logs and responses are accurate; locate the error block in src/app/api/sciweave-analytics/users/route.ts where console.error("Error fetching chats over time:", error) and NextResponse.json({ error: "Failed to fetch chats over time" }, { status: 500 }) are used and replace both strings with a users-specific message while keeping the error object included in the log and preserving the 500 response.
13-34:⚠️ Potential issue | 🟠 MajorEnsure DB client is always released
client.release()only runs on the success path. Ifclient.query(...)throws, the connection is leaked.Suggested fix
- const client = await pool.connect(); - const result = await client.query( + const client = await pool.connect(); + try { + const result = await client.query( ` SELECT DATE_TRUNC($3, created_at) AS DATE, COUNT(DISTINCT username) AS value @@ - [from, to, intervalToDateTrunc(interval)] - ); - client.release(); - const data = result.rows as { day: string; value: number }[]; - return NextResponse.json(data); + [from, to, intervalToDateTrunc(interval)] + ); + const data = result.rows as { day: string; value: number }[]; + return NextResponse.json(data); + } finally { + client.release(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/sciweave-analytics/users/route.ts` around lines 13 - 34, The DB client obtained via pool.connect() is only released on the success path, so wrap the usage of client (the result of pool.connect()) in a try/finally (or use a helper that auto-releases) to guarantee client.release() runs even if client.query(...) throws; update the block around pool.connect(), client.query(..., [from, to, intervalToDateTrunc(interval)]) and client.release() to call client.release() from finally and keep the same query/parameters and PROD_FILTER_AND logic.
♻️ Duplicate comments (1)
src/app/api/sciweave-analytics/devices/route.ts (1)
17-23:⚠️ Potential issue | 🟠 MajorUse NULL-safe aggregate predicates for device counters
Current
SUM(bool::int)expressions can produceNULL-driven undercount/empty totals and are less robust when fields are missing. PreferCOUNT(*) FILTER (WHERE ...)withCOALESCE(...).Suggested fix
- date_trunc($3, created_at) AS DATE, - SUM(((device_info->>'isMobile')::boolean)::int) AS "mobile", - SUM(((device_info->>'isTablet')::boolean)::int) AS "tablet", - SUM(((device_info->>'isDesktop')::boolean AND device_info->>'osName' ILIKE 'mac%')::int) AS "mac", - SUM(((device_info->>'isDesktop')::boolean AND device_info->>'osName' ILIKE 'windows%')::int) AS "windows", - SUM(((device_info->>'isDesktop')::boolean AND device_info->>'osName' NOT ILIKE 'mac%' AND device_info->>'osName' NOT ILIKE 'windows%')::int) AS "otherDesktops", - SUM((NOT((device_info->>'isMobile')::boolean OR (device_info->>'isTablet')::boolean OR (device_info->>'isDesktop')::boolean))::int) AS "unknown" + date_trunc($3, created_at) AS "date", + COUNT(*) FILTER (WHERE (device_info->>'isMobile')::boolean IS TRUE) AS "mobile", + COUNT(*) FILTER (WHERE (device_info->>'isTablet')::boolean IS TRUE) AS "tablet", + COUNT(*) FILTER ( + WHERE COALESCE((device_info->>'isDesktop')::boolean, FALSE) IS TRUE + AND COALESCE(device_info->>'osName', '') ILIKE 'mac%' + ) AS "mac", + COUNT(*) FILTER ( + WHERE COALESCE((device_info->>'isDesktop')::boolean, FALSE) IS TRUE + AND COALESCE(device_info->>'osName', '') ILIKE 'windows%' + ) AS "windows", + COUNT(*) FILTER ( + WHERE COALESCE((device_info->>'isDesktop')::boolean, FALSE) IS TRUE + AND COALESCE(device_info->>'osName', '') NOT ILIKE 'mac%' + AND COALESCE(device_info->>'osName', '') NOT ILIKE 'windows%' + ) AS "otherDesktops", + COUNT(*) FILTER ( + WHERE COALESCE((device_info->>'isMobile')::boolean, FALSE) IS NOT TRUE + AND COALESCE((device_info->>'isTablet')::boolean, FALSE) IS NOT TRUE + AND COALESCE((device_info->>'isDesktop')::boolean, FALSE) IS NOT TRUE + ) AS "unknown"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/sciweave-analytics/devices/route.ts` around lines 17 - 23, Replace the SUM(bool::int) aggregates with NULL-safe COUNT(*) FILTER expressions wrapped in COALESCE to avoid NULLs when device_info keys are missing: for each alias ("mobile","tablet","mac","windows","otherDesktops","unknown") change e.g. SUM(((device_info->>'isMobile')::boolean)::int) AS "mobile" to COALESCE(COUNT(*) FILTER (WHERE (device_info->>'isMobile')::boolean = TRUE), 0) AS "mobile", and similarly use FILTER conditions for tablet, for mac/windows use (device_info->>'isDesktop')::boolean = TRUE AND device_info->>'osName' ILIKE 'mac%' (or 'windows%'), for otherDesktops use desktop TRUE AND osName NOT ILIKE 'mac%' AND NOT ILIKE 'windows%', and for unknown use FILTER (WHERE NOT ((device_info->>'isMobile')::boolean = TRUE OR (device_info->>'isTablet')::boolean = TRUE OR (device_info->>'isDesktop')::boolean = TRUE)) with COALESCE(..., 0); update the SELECT expressions that currently define the "mobile","tablet","mac","windows","otherDesktops","unknown" columns accordingly.
🧹 Nitpick comments (1)
.env.example (1)
20-21: Consider alphabetical ordering of environment variables.The static analysis suggests ordering
SCIWEAVE_SUPABASE_SERVICE_KEYbeforeSCIWEAVE_SUPABASE_URLalphabetically. While not critical, consistent ordering improves readability and reduces merge conflicts.📝 Proposed reordering
-SCIWEAVE_SUPABASE_URL= SCIWEAVE_SUPABASE_SERVICE_KEY= +SCIWEAVE_SUPABASE_URL=As per static analysis: "The SCIWEAVE_SUPABASE_SERVICE_KEY key should go before the SCIWEAVE_SUPABASE_URL key (UnorderedKey)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 20 - 21, The environment variables in .env.example are out of alphabetical order; move SCIWEAVE_SUPABASE_SERVICE_KEY so it appears before SCIWEAVE_SUPABASE_URL to maintain consistent alphabetical ordering. Edit the .env.example file and reorder the two entries (SCIWEAVE_SUPABASE_SERVICE_KEY, then SCIWEAVE_SUPABASE_URL), and ensure any related example env blocks follow the same ordering for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 21: The file `.env.example` ends with the variable declaration
SCIWEAVE_SUPABASE_SERVICE_KEY= but is missing a trailing newline; update the
file to ensure there is a single blank line (newline character) after that line
so the file ends with a newline.
In `@src/app/`(auth)/metrics/sciweave/page.tsx:
- Around line 93-105: Wrap the entire fetch + parsing flow in a try/catch so
network errors and JSON parsing exceptions are handled and return the fallback
[]; specifically surround the call that produces res and the subsequent
res.json() check (the block that checks res.ok, res.statusText,
Array.isArray(data), and returns data as UserSessionsDataItem[]) with try/catch,
log the caught error (including error details) and return [] from the catch
block to ensure callers always receive a safe UserSessionsDataItem[].
---
Outside diff comments:
In `@src/app/api/sciweave-analytics/devices/route.ts`:
- Around line 46-49: The error handling in the devices route logs and returns a
message referencing "chats over time", which is misleading; update the
console.error and the NextResponse.json error message inside the GET handler (or
exported route function) to reflect the correct context such as "devices over
time" or "devices metrics" (update the console.error("Error fetching chats over
time:", error) and the corresponding NextResponse.json({ error: "Failed to fetch
chats over time" }, { status: 500 }) to use the correct wording), ensuring the
error text matches the endpoint purpose.
- Around line 13-33: The DB client acquired via pool.connect() in the route
handler must be released in a finally block to ensure it is returned to the pool
even if client.query(...) throws; refactor the code around pool.connect(),
client.query(...) and client.release() so you call client.release() from a
finally (or guard with if (client) client.release()) after the try that runs the
query (e.g., the block surrounding the SELECT using intervalToDateTrunc),
leaving error handling in the existing catch but guaranteeing release.
In `@src/app/api/sciweave-analytics/users/route.ts`:
- Around line 37-40: The error handling in the users API route incorrectly logs
and returns a generic "chats over time" message; update the console.error call
and the NextResponse.json error message to reflect this users endpoint (e.g.,
"Failed to fetch users" or "Error fetching users") so logs and responses are
accurate; locate the error block in
src/app/api/sciweave-analytics/users/route.ts where console.error("Error
fetching chats over time:", error) and NextResponse.json({ error: "Failed to
fetch chats over time" }, { status: 500 }) are used and replace both strings
with a users-specific message while keeping the error object included in the log
and preserving the 500 response.
- Around line 13-34: The DB client obtained via pool.connect() is only released
on the success path, so wrap the usage of client (the result of pool.connect())
in a try/finally (or use a helper that auto-releases) to guarantee
client.release() runs even if client.query(...) throws; update the block around
pool.connect(), client.query(..., [from, to, intervalToDateTrunc(interval)]) and
client.release() to call client.release() from finally and keep the same
query/parameters and PROD_FILTER_AND logic.
---
Duplicate comments:
In `@src/app/api/sciweave-analytics/devices/route.ts`:
- Around line 17-23: Replace the SUM(bool::int) aggregates with NULL-safe
COUNT(*) FILTER expressions wrapped in COALESCE to avoid NULLs when device_info
keys are missing: for each alias
("mobile","tablet","mac","windows","otherDesktops","unknown") change e.g.
SUM(((device_info->>'isMobile')::boolean)::int) AS "mobile" to COALESCE(COUNT(*)
FILTER (WHERE (device_info->>'isMobile')::boolean = TRUE), 0) AS "mobile", and
similarly use FILTER conditions for tablet, for mac/windows use
(device_info->>'isDesktop')::boolean = TRUE AND device_info->>'osName' ILIKE
'mac%' (or 'windows%'), for otherDesktops use desktop TRUE AND osName NOT ILIKE
'mac%' AND NOT ILIKE 'windows%', and for unknown use FILTER (WHERE NOT
((device_info->>'isMobile')::boolean = TRUE OR
(device_info->>'isTablet')::boolean = TRUE OR
(device_info->>'isDesktop')::boolean = TRUE)) with COALESCE(..., 0); update the
SELECT expressions that currently define the
"mobile","tablet","mac","windows","otherDesktops","unknown" columns accordingly.
---
Nitpick comments:
In @.env.example:
- Around line 20-21: The environment variables in .env.example are out of
alphabetical order; move SCIWEAVE_SUPABASE_SERVICE_KEY so it appears before
SCIWEAVE_SUPABASE_URL to maintain consistent alphabetical ordering. Edit the
.env.example file and reorder the two entries (SCIWEAVE_SUPABASE_SERVICE_KEY,
then SCIWEAVE_SUPABASE_URL), and ensure any related example env blocks follow
the same ordering for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edcb2f5e-d948-4a6b-88cc-6668eeba56c7
📒 Files selected for processing (7)
.env.examplesrc/app/(auth)/metrics/sciweave/page.tsxsrc/app/api/sciweave-analytics/chats/route.tssrc/app/api/sciweave-analytics/devices/route.tssrc/app/api/sciweave-analytics/sessions/route.tssrc/app/api/sciweave-analytics/users/route.tssrc/components/molecules/DesciResearchAnalytics.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/molecules/DesciResearchAnalytics.tsx
- src/app/api/sciweave-analytics/chats/route.ts
- src/app/api/sciweave-analytics/sessions/route.ts
|
|
||
| SCIWEAVE_SUPABASE_URL= | ||
| SCIWEAVE_SUPABASE_SERVICE_KEY= No newline at end of file | ||
| SCIWEAVE_SUPABASE_SERVICE_KEY= |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if .env.example ends with a newline
if [ -s .env.example ]; then
tail -c 1 .env.example | od -An -tc | grep -q '\n' && echo "✓ File ends with newline" || echo "✗ Missing trailing newline"
fiRepository: desci-labs/desci-admin
Length of output: 150
🏁 Script executed:
cat -n .env.example | tail -5Repository: desci-labs/desci-admin
Length of output: 199
Add a trailing newline at the end of the file.
The file still lacks a newline after line 21. Add a single blank line to the end of .env.example.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 21-21: [UnorderedKey] The SCIWEAVE_SUPABASE_SERVICE_KEY key should go before the SCIWEAVE_SUPABASE_URL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 21, The file `.env.example` ends with the variable
declaration SCIWEAVE_SUPABASE_SERVICE_KEY= but is missing a trailing newline;
update the file to ensure there is a single blank line (newline character) after
that line so the file ends with a newline.
Change disabled check from `state?.user !== undefined` to `!!state?.user` so that null/undefined both correctly result in enabled button. Also fix button label from "Send code" to "Verify code" on step 2.
NODE_ENV is always "production" on Vercel, which sets the cookie domain to .desci.com even on vercel.app preview deployments. The browser ignores this cross-domain cookie, so login succeeds server-side but the middleware never sees the auth cookie.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/actions.tsx (1)
46-46:⚠️ Potential issue | 🟡 MinorComment/code mismatch: Comment says "2 hours" but code calculates 30 days.
The expression
1000 * 60 * 60 * 24 * 30 * 1equals 30 days, not 2 hours as stated in the comment. Please verify whether this extended expiry is intentional for the dev cookie.📝 Proposed fix (if 30 days is intentional)
- expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 30 * 1), // 2 hours + expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 30), // 30 days🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/actions.tsx` at line 46, The expires value for the dev cookie (the expires property expression `1000 * 60 * 60 * 24 * 30 * 1`) does not match the inline comment "2 hours"; decide which is intended and fix accordingly: if you want a 2-hour expiry, replace the expression with `1000 * 60 * 60 * 2` for the expires property used when setting the cookie; if 30 days is intended, update the comment to reflect "30 days" (or remove the misleading comment) — locate the expires assignment in the cookie-setting logic in src/app/actions.tsx (the expires property on the cookie object) and make the change there.
🧹 Nitpick comments (1)
src/app/actions.tsx (1)
31-61: Duplicate cookie setting in production.When
NEXT_ENV === "production", the cookie is set twice with identical values:
- Lines 31-37: Sets cookie with
domain: ".desci.com"(since the condition evaluates to true)- Lines 53-61: Sets the same cookie again
Consider removing the redundant production block (lines 53-61) since the first block already handles production correctly after this change.
♻️ Proposed fix to remove duplicate cookie setting
cookies().set(AUTH_COOKIE_FIELDNAME, response.user.token, { path: "/", expires: new Date(Date.now() + 1000 * 60 * 60 * 2), // 2 hours httpOnly: true, secure: process.env.NEXT_ENV === "production", domain: process.env.NEXT_ENV === "production" ? ".desci.com" : undefined, }); if ( AUTH_COOKIE_FIELDNAME === "auth-dev" && process.env.NEXT_ENV === "development" ) { console.log("[cookie]", AUTH_COOKIE_FIELDNAME, process.env.NEXT_ENV); cookies().set("auth", response.user.token, { path: "/", expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 30 * 1), // 30 days httpOnly: true, secure: true, // domain: '.desci.com', }); } - - if (process.env.NEXT_ENV === "production") { - cookies().set(AUTH_COOKIE_FIELDNAME, response.user.token, { - path: "/", - expires: new Date(Date.now() + 1000 * 60 * 60 * 2), // 2 hours - httpOnly: true, - secure: true, - domain: ".desci.com", - }); - } return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/actions.tsx` around lines 31 - 61, Remove the redundant production-only cookies().set call: the initial cookies().set(AUTH_COOKIE_FIELDNAME, response.user.token, {..., domain: process.env.NEXT_ENV === "production" ? ".desci.com" : undefined}) already covers production, so delete the later if (process.env.NEXT_ENV === "production") { cookies().set(AUTH_COOKIE_FIELDNAME, ...) } block; keep the dev-only branch that sets "auth" when AUTH_COOKIE_FIELDNAME === "auth-dev" and NEXT_ENV === "development".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/actions.tsx`:
- Line 33: The inline comment for the expires value mismatches the calculation:
the object property expires uses new Date(Date.now() + 1000 * 60 * 60 * 2) which
computes 2 hours but the comment says "3 hours"; update the code so they match
by either changing the multiplier from 2 to 3 to actually set 3 hours, or change
the comment to "2 hours" to reflect the current calculation (locate the expires
property assignment in actions.tsx to apply the fix).
---
Outside diff comments:
In `@src/app/actions.tsx`:
- Line 46: The expires value for the dev cookie (the expires property expression
`1000 * 60 * 60 * 24 * 30 * 1`) does not match the inline comment "2 hours";
decide which is intended and fix accordingly: if you want a 2-hour expiry,
replace the expression with `1000 * 60 * 60 * 2` for the expires property used
when setting the cookie; if 30 days is intended, update the comment to reflect
"30 days" (or remove the misleading comment) — locate the expires assignment in
the cookie-setting logic in src/app/actions.tsx (the expires property on the
cookie object) and make the change there.
---
Nitpick comments:
In `@src/app/actions.tsx`:
- Around line 31-61: Remove the redundant production-only cookies().set call:
the initial cookies().set(AUTH_COOKIE_FIELDNAME, response.user.token, {...,
domain: process.env.NEXT_ENV === "production" ? ".desci.com" : undefined})
already covers production, so delete the later if (process.env.NEXT_ENV ===
"production") { cookies().set(AUTH_COOKIE_FIELDNAME, ...) } block; keep the
dev-only branch that sets "auth" when AUTH_COOKIE_FIELDNAME === "auth-dev" and
NEXT_ENV === "development".
- Wrap session analytics fetch in try/catch to handle network/parse errors - Separate !ok and !body checks in download routes, return 502 for missing body - Fix mismatched expiry comments in login cookie settings
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.
Description of the Problem / Feature
Explanation of the solution
Instructions on making this work
Describe environment variable changes
Preview of changes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores