Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Google Search Console (GSC) integration: OAuth callback and token storage (encrypted), DB schema and ClickHouse tables, TRPC gsc router and UI/screens, background worker/cron sync and queue, plus supporting encryption, migrations, and date/interval UI updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User
participant API as API Server
participant Google as Google OAuth
participant Postgres as Postgres/Prisma
Browser->>API: GET /gsc/initiate
API->>Browser: set cookies (state, code_verifier, projectId), redirect to Google
Browser->>Google: Authorize
Google->>Browser: Redirect /gsc/callback?code&state
Browser->>API: GET /gsc/callback?code&state
API->>API: validate state vs cookie, read code_verifier
API->>Google: Exchange code via googleGsc
Google->>API: tokens (access, refresh)
API->>Postgres: upsert gsc_connections with encrypted tokens
API->>Browser: clear cookies, redirect to project GSC settings
sequenceDiagram
participant Cron as Scheduler
participant Worker as Worker
participant Postgres as Postgres
participant Google as Google API
participant ClickHouse as ClickHouse
Cron->>Worker: trigger gscSyncAllJob
Worker->>Postgres: list projects with siteUrl
loop per project
Worker->>Worker: enqueue gscProjectSync(projectId)
Worker->>Postgres: load connection (refresh if needed)
Worker->>Google: fetch search analytics for window
Google->>Worker: return metrics
Worker->>ClickHouse: insert gsc_daily / gsc_pages_daily / gsc_queries_daily
Worker->>Postgres: update lastSyncedAt, lastSyncStatus
end
Note over Worker,Postgres: on error update lastSyncError and status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (7)
apps/start/src/components/time-window-picker.tsx (1)
48-59: IncompleteuseCallbackdependency array.The
handleCustomcallback referencesonStartDateChange,onEndDateChange,onChange, andonIntervalChange, but onlystartDateandendDateare listed in the dependency array. This could cause stale closures if any of the callback props change.♻️ Suggested fix
- }, [startDate, endDate]); + }, [startDate, endDate, onStartDateChange, onEndDateChange, onChange, onIntervalChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/time-window-picker.tsx` around lines 48 - 59, The useCallback for handleCustom currently only lists startDate and endDate but closes over pushModal, onStartDateChange, onEndDateChange, onChange, and onIntervalChange; update the dependency array of handleCustom to include pushModal, onStartDateChange, onEndDateChange, onChange, and onIntervalChange (in addition to startDate and endDate) so the callback updates when any of those props/values change.apps/start/src/routes/_app.$organizationId.$projectId.settings._tabs.gsc.tsx (1)
292-298: Avoidas anytype cast.The
variant={syncStatusVariant as any}cast bypasses type safety. Consider typingsyncStatusVariantto match the Badge's accepted variant values, or use a type assertion to the specific union type.♻️ Suggested fix
- variant={syncStatusVariant as any} + variant={syncStatusVariant}And update the variant type definition at lines 240-245:
const syncStatusVariant = connection.lastSyncStatus === 'success' ? 'success' : connection.lastSyncStatus === 'error' ? 'destructive' - : 'secondary'; + : 'secondary' as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.settings._tabs.gsc.tsx around lines 292 - 298, The Badge is being passed variant={syncStatusVariant as any}, which loses type safety; change the type of syncStatusVariant (the variable computed earlier) to the Badge component's accepted variant union (or use a narrow type assertion) instead of any — e.g. declare syncStatusVariant as BadgeProps['variant'] or the specific union used by Badge and remove the cast, so the variant prop accepts syncStatusVariant without as any; update the variant type definition where syncStatusVariant is computed to match the Badge's variant type.packages/trpc/src/routers/gsc.ts (1)
266-270: Consider extracting the previous-period date calculation.The previous-period calculation pattern (computing
prevStart/prevEndfrom current range) is duplicated ingetSearchEngines,getAiEngines, andgetPreviousOverview. A shared helper would reduce duplication.♻️ Suggested helper
function getPreviousPeriod(startDate: string, endDate: string) { const startMs = new Date(startDate).getTime(); const duration = new Date(endDate).getTime() - startMs; const prevEnd = new Date(startMs - 1); const prevStart = new Date(prevEnd.getTime() - duration); const fmt = (d: Date) => d.toISOString().slice(0, 10); return { prevStart: fmt(prevStart), prevEnd: fmt(prevEnd) }; }Also applies to: 315-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/gsc.ts` around lines 266 - 270, Extract the duplicated previous-period date logic into a single helper (e.g., getPreviousPeriod) and replace the duplicated blocks in getSearchEngines, getAiEngines, and getPreviousOverview with calls to that helper; the helper should accept startDate and endDate (strings), compute prevStart and prevEnd as ISO YYYY-MM-DD strings (same formatting as fmt in the diff) and return them so callers use the returned prevStart/prevEnd instead of recomputing startMs/duration/prevEnd/prevStart/ fmt locally.apps/start/src/routes/_app.$organizationId.$projectId.seo.tsx (2)
128-134: Consider addingenabledflag for consistency.Unlike other queries that check
!!isConnected,searchEnginesQueryandaiEnginesQuerywill run even when GSC is not connected. If these queries depend on GSC data or should only run when connected, add theenabledoption for consistency with other queries.♻️ Suggested change
const searchEnginesQuery = useQuery( - trpc.gsc.getSearchEngines.queryOptions({ projectId, ...dateInput }) + trpc.gsc.getSearchEngines.queryOptions( + { projectId, ...dateInput }, + { enabled: !!isConnected } + ) ); const aiEnginesQuery = useQuery( - trpc.gsc.getAiEngines.queryOptions({ projectId, ...dateInput }) + trpc.gsc.getAiEngines.queryOptions( + { projectId, ...dateInput }, + { enabled: !!isConnected } + ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.seo.tsx around lines 128 - 134, searchEnginesQuery and aiEnginesQuery are missing the enabled guard and will run even when GSC is not connected; modify the useQuery calls that use trpc.gsc.getSearchEngines.queryOptions({ projectId, ...dateInput }) and trpc.gsc.getAiEngines.queryOptions({ projectId, ...dateInput }) to include an enabled flag (e.g., enabled: !!isConnected) so they only execute when the isConnected boolean is true, merging the enabled option with the existing queryOptions call.
227-241: CTR calculation uses simple average instead of weighted average.The current implementation averages individual day CTRs:
(totals.ctr / n) * 100. For aggregate CTR, the mathematically accurate formula would betotalClicks / totalImpressions * 100, which weights by traffic volume.This may be intentional for comparing trends, but if accurate aggregate CTR is desired:
♻️ Alternative weighted calculation
const totals = sumOverview(overview); const prevTotals = sumOverview(prevOverview); - const n = Math.max(overview.length, 1); - const pn = Math.max(prevOverview.length, 1); + + const avgCtr = totals.impressions > 0 ? (totals.clicks / totals.impressions) * 100 : 0; + const prevAvgCtr = prevTotals.impressions > 0 ? (prevTotals.clicks / prevTotals.impressions) * 100 : 0; + const avgPosition = overview.length > 0 ? totals.position / overview.length : 0; + const prevAvgPosition = prevOverview.length > 0 ? prevTotals.position / prevOverview.length : 0;Then use
avgCtrandprevAvgCtrfor the CTR metric card.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.seo.tsx around lines 227 - 241, The sumOverview reducer correctly totals clicks and impressions but CTR must be computed as a weighted rate, not averaged per-row: replace the current use of (totals.ctr / n) * 100 and (prevTotals.ctr / pn) * 100 with CTR = totals.clicks === 0 || totals.impressions === 0 ? 0 : (totals.clicks / totals.impressions) * 100 (and similarly for prevTotals), using the existing symbols sumOverview, totals, prevTotals, overview and prevOverview; ensure you guard against division by zero and use these computed avgCtr / prevAvgCtr values where the CTR metric card expects them.apps/start/src/components/page/pages-insights.tsx (1)
204-216: Deduplication may not keep highest impact entry.The current dedup keeps the first occurrence of each
(page, type)pair, but results aren't sorted by impact before deduplication. This means a lower-impact duplicate might be kept if it appears first in the iteration.♻️ Sort before deduplication to keep highest impact
+ // Sort by impact descending before deduplication + results.sort((a, b) => b.impact - a.impact); + // Dedupe by (page, type), keep highest impact const seen = new Set<string>(); const deduped = results.filter((r) => { const key = `${r.page}::${r.type}`; if (seen.has(key)) { return false; } seen.add(key); return true; }); - return deduped.sort((a, b) => b.impact - a.impact); + return deduped;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/page/pages-insights.tsx` around lines 204 - 216, The deduplication currently keeps the first seen (page,type) from results which may be lower impact; before the filter that creates seen/deduped, sort results by impact descending (or build a Map keyed by `${r.page}::${r.type}` that retains the max impact) so the highest-impact entry for each (page,type) is encountered/kept; update the logic around results, seen, and deduped to operate on the impact-sorted array (or use a reduce to pick max) so the final return still sorts by impact and returns the true highest-impact deduplicated list.packages/db/src/gsc.ts (1)
21-30: Consider adding timeouts to external API calls.The
fetchcalls to Google APIs lack timeout handling. If the Google API is slow or unresponsive, these calls could hang indefinitely, potentially causing worker jobs to stall.♻️ Add timeout using AbortController
+const FETCH_TIMEOUT_MS = 30_000; + +function fetchWithTimeout(url: string, options: RequestInit): Promise<Response> { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); + return fetch(url, { ...options, signal: controller.signal }).finally(() => clearTimeout(timeout)); +} + async function refreshGscToken( refreshToken: string ): Promise<{ accessToken: string; expiresAt: Date }> { const params = new URLSearchParams({ client_id: process.env.GOOGLE_CLIENT_ID ?? '', client_secret: process.env.GOOGLE_CLIENT_SECRET ?? '', refresh_token: refreshToken, grant_type: 'refresh_token', }); - const res = await fetch('https://oauth2.googleapis.com/token', { + const res = await fetchWithTimeout('https://oauth2.googleapis.com/token', { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, body: params.toString(), });Apply similar changes to
listGscSitesandqueryGscSearchAnalytics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/gsc.ts` around lines 21 - 30, The fetch calls that obtain/refresh GSC tokens and call Google APIs (used in the function that posts to 'https://oauth2.googleapis.com/token' as well as listGscSites and queryGscSearchAnalytics) need request timeouts: create an AbortController for each fetch, pass controller.signal to fetch, and set a timer (e.g., via setTimeout) to call controller.abort() after a configurable timeout (e.g., 5–10s); ensure you clear the timeout on success and catch the abort error to throw a clear timeout-specific Error so callers can handle it.
🤖 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/controllers/gsc-oauth-callback.controller.ts`:
- Around line 84-86: The success path clears PKCE/state cookies via
reply.clearCookie('gsc_oauth_state'), reply.clearCookie('gsc_code_verifier'),
and reply.clearCookie('gsc_project_id') but the error paths (e.g., where the
code exchange or DB write fail) do not; update the error handling branches
(catch blocks or early-return branches that handle token exchange failures and
database write failures) to also call those same reply.clearCookie(...) calls so
stale PKCE/state material is removed on failure—apply the same fix to the other
error locations noted around the existing clearCookie calls (the block
referenced as lines 92–95).
- Around line 19-22: The LogError calls in gsc-oauth-callback.controller.ts are
attaching raw OAuth artifacts (req.query, query.error, state, storedState);
remove any direct inclusion of these sensitive values from the thrown LogError
metadata and instead pass only non-sensitive indicators (e.g., boolean flags
like hasError, hasState, hasStoredState) or sanitized/masked versions (e.g.,
replace values for keys "code", "state", "error" with "<redacted>"). Locate the
LogError usages (the throw new LogError(...) statements and any places that
include req.query, query.error, state, or storedState) and update their metadata
to exclude raw OAuth tokens or codes while preserving useful troubleshooting
flags.
In `@apps/start/src/components/page/gsc-breakdown-table.tsx`:
- Line 55: The header builds a plural by naively appending "s" to breakdownLabel
(rendered in gsc-breakdown-table.tsx as Top {breakdownLabel.toLowerCase()}s),
which yields "querys" for type === 'page'; fix by computing the correct plural
form before rendering (e.g., derive a pluralLabel variable or helper that maps
'query' -> 'queries' for the case when props.type === 'page' or when
breakdownLabel === 'query'), and use that pluralLabel (lowercased) in the <h3>
instead of appending "s".
In `@apps/start/src/components/page/gsc-cannibalization.tsx`:
- Around line 31-35: The component accepts startDate and endDate but they are
not passed into the TRPC call; update the useQuery invocation that calls
trpc.gsc.getCannibalization.queryOptions to include startDate and endDate
alongside projectId, range and interval (e.g., pass { projectId, range: range as
any, interval: interval as any, startDate, endDate }) so custom-range views
query the correct bounds while preserving the existing options object (including
placeholderData/keepPreviousData).
- Around line 52-58: The current page can be out-of-range when items shrink;
clamp the page whenever items or pageSize change so
paginatedItems/rangeStart/rangeEnd never render an empty list; after computing
pageCount (from items.length and pageSize) add a useEffect that, when [items,
pageSize] change, calls the state updater (e.g., setPage) to set page =
Math.max(0, Math.min(page, pageCount - 1)) so pageCount, paginatedItems,
rangeStart and rangeEnd remain consistent; reference pageCount, paginatedItems,
page, pageSize and setPage when making this change.
In `@apps/start/src/components/pages/page-sparkline.tsx`:
- Around line 71-78: The Tooltiper content uses awkward phrasing; update the
conditional strings in the Tooltiper inside page-sparkline.tsx to use "Upward
trend" instead of "Upgoing trend" and "Downward trend" instead of "Downgoing
trend" (the conditional based on the trend variable: trend === '↑' ? ... : trend
=== '↓' ? ... : ...); keep the default "Stable trend" unchanged.
- Around line 98-110: The component currently treats pending/error as empty by
coercing query.data to [], so update the render logic around useQuery
(trpc.event.pageTimeseries.queryOptions) and LazyComponent/SparklineBars to
distinguish states: use query.isLoading to render a loading placeholder (e.g.,
skeleton/spinner with same size), use query.isError (and query.error) to render
an error state or message, and only pass query.data (or [] only when data is
actually present but empty) into SparklineBars; ensure you reference the
existing symbols useQuery, trpc.event.pageTimeseries.queryOptions, query.data,
LazyComponent, and SparklineBars when making the changes.
- Around line 39-66: The current fixed minimum bar width causes overflow for
large data sets; update the bar sizing logic in the page-sparkline component so
bars are computed to always fit the SVG width: compute barW = Math.floor((width
- gap*(total-1))/total) (remove the unconditional Math.max(2,...)), then if barW
< 1 progressively reduce gap (e.g., set gap = 1 then gap = 0) and recompute barW
until it fits or barW >= 1; ensure x positioning still uses i * (barW + gap) and
keep the height/clamping logic for barH unchanged. This touches the
variables/expressions barW, gap, width, total and the mapping that renders
<rect> elements.
In `@apps/start/src/components/pages/table/columns.tsx`:
- Around line 99-126: The cell renderer computes pct using prev before checking
for prev === 0, risking division by zero; update the cell implementation (the
cell arrow function that uses previousMap and row.original.sessions) to first
handle prev == null and prev === 0 cases (returning the "—" or "new" UI) and
only calculate pct = ((row.original.sessions - prev) / prev) * 100 when prev >
0, preserving the existing UI branches and class logic (isPos, percentage
formatting) but avoiding computing pct for prev === 0.
In `@apps/start/src/components/pages/table/index.tsx`:
- Around line 27-31: The UI is truncating results by hardcoding take: 1000 in
the client query and then only searching the in-memory slice, causing missing
rows and an incomplete gscMap; update the code that constructs
trpc.event.pages.queryOptions (the pagesQuery usage) to remove the fixed take:
1000 and instead implement proper server-side pagination or an unbounded fetch
(or looped fetch using the cursor) so the full dataset is retrieved, move the
search/filter logic off the client-side slice and into the server query (or
apply it across the full fetched set), and ensure the gscMap aggregation logic
uses the full pages result rather than the truncated in-memory slice so no pages
or their GSC data are lost.
In `@apps/start/src/modals/date-ranger-picker.tsx`:
- Around line 79-82: The code uses a non-null assertion on
getDefaultIntervalByDates(...) when setting interval, which can blow up if that
function returns undefined; update the interval assignment in
date-ranger-picker.tsx (the call to getDefaultIntervalByDates with
startDate.toISOString()/endDate.toISOString()) to remove the non-null assertion
and provide a safe fallback (e.g., a sensible default interval or computed
fallback) using a nullish coalescing or conditional so interval is never
undefined.
In `@apps/start/src/modals/gsc-details.tsx`:
- Around line 96-101: The modal currently uses pagesTimeseriesQuery
(trpc.event.pagesTimeseries) and then filters client-side, causing O(all pages)
fetches and missing data; change the query to call trpc.event.pageTimeseries (or
the existing pageTimeseries endpoint) with the selected page identifier (use the
same projectId + dateInput + selected page slug/id) so the server returns
timeseries for that single page, and update any usages (e.g., the data passed
into PageViewsChart and the logic around pagesTimeseriesQuery) to consume the
new pageTimeseriesQuery result instead of filtering pagesTimeseries client-side.
- Around line 28-55: The tooltip created by createChartTooltip (used via
TooltipProvider / GscTooltip) assumes GscChartData has clicks and impressions
and will throw when used for GscViewsChart rows shaped { date, views }; update
the tooltip to guard against missing fields or create a separate tooltip for
views: check the first item (const item = data[0]) and branch rendering based on
which keys exist (e.g., if item.views render a Views row using
item.views.toLocaleString(), else render Clicks/Impressions), ensuring you never
call .toLocaleString() on undefined and update any other tooltip usages noted in
the file (the other tooltip code block around the GscViewsChart) to use the
appropriate tooltip component or guarded rendering.
In `@apps/start/src/modals/page-details.tsx`:
- Around line 22-32: The current URL-parse fallback incorrectly leaves origin =
value and path = "/", so path-only inputs (e.g., "/docs/foo") are treated as
origins; in the try/catch inside the type === 'page' block in page-details.tsx,
change the fallback so that on catch you assign path = value (and set origin to
a sensible default such as window.location.origin or an empty string) instead of
leaving origin as value; update the variables referenced by PageViewsChart so
the chart receives the correct origin and path for path-only values.
In `@packages/db/code-migrations/12-add-gsc.ts`:
- Around line 69-80: The migration is using __filename (in the fs.writeFileSync
path.join call) but __filename isn't defined in ESM; add imports for
fileURLToPath from 'node:url' and dirname from 'node:path' at the top of the
file and create a __filename value from import.meta.url (e.g., using
fileURLToPath(import.meta.url)); ensure these declarations are placed after the
existing imports so the fs.writeFileSync(path.join(__filename.replace('.ts',
'.sql')), ...) call can use the defined __filename without throwing a
ReferenceError.
In `@packages/db/src/gsc.ts`:
- Around line 406-411: The CTR merge is using an unweighted average
(existing.ctr = (existing.ctr + row.ctr) / 2) which skews results when merging
more than two variants; instead compute CTR from aggregated clicks and
impressions: after updating existing.clicks and existing.impressions using
row.clicks and row.impressions (the same block that updates existing.clicks and
existing.impressions in the entry.pages merge), set existing.ctr =
existing.clicks / existing.impressions (handle zero impressions safely) so the
CTR is a true weighted average; update the merge logic around
entry.pages.find(...) where existing.* and row.* are used.
- Around line 285-305: Refactor the raw SQL calls in getGscOverview,
getGscPages, and getGscQueries to use the custom ClickHouse Query builder from
./packages/db/src/clickhouse/query-builder.ts: replace originalCh.query({ query:
`...`, query_params: {...} }) with a Query(...) chain (e.g., new
Query().select(...).from('gsc_daily').where(...).groupBy('date').orderBy('date',
'ASC').toSQL()) and pass the generated SQL and parameters to originalCh.query;
ensure you preserve the same selected columns/aggregations (dateExpr,
sum(clicks) as clicks, etc.), the same WHERE placeholders/parameters (projectId,
startDate, endDate), and the JSONEachRow format and return result.json()
unchanged. Ensure imports reference the Query builder and update parameter
passing to match the builder's toSQL() output.
In `@packages/trpc/src/routers/gsc.ts`:
- Around line 343-358: The ClickHouse queries in gsc.ts currently use raw
chQuery with string interpolation (the two chQuery calls that fetch engines and
prevResult using TABLE_NAMES.sessions and where(fmt(prevStart), fmt(prevEnd))) —
replace these with the project's ClickHouse query builder from
./packages/db/src/clickhouse/query-builder.ts (same approach as
getSearchEngines): construct the SELECT, WHERE, GROUP BY, ORDER BY, LIMIT and
the count(*) prev query via the builder API, pass the built SQL and parameters
to chQuery (or the builder's execute helper) instead of interpolating values,
and keep use of helpers like where, fmt, prevStart/prevEnd to populate builder
parameters to avoid raw string templates.
- Around line 273-294: The ClickHouse queries in the gsc router currently use
raw chQuery with string-interpolated SQL (see chQuery calls that reference
TABLE_NAMES.sessions and variables like input.projectId, startDate, endDate,
fmt(prevStart), fmt(prevEnd)), which violates the guideline—refactor both
queries to use the clix query builder exported from
./packages/db/src/clickhouse/query-builder.ts (imported as clix) and
parameterize values instead of string interpolation; replace the SELECT ...
GROUP BY ... LIMIT 10 and the SELECT count(*) ... previous-period query with
equivalent clix-built queries (using
clix.select/from/where/groupBy/orderBy/limit and clix.select/count with where
filters) passing projectId and date bounds as parameters to avoid inline
interpolation.
---
Nitpick comments:
In `@apps/start/src/components/page/pages-insights.tsx`:
- Around line 204-216: The deduplication currently keeps the first seen
(page,type) from results which may be lower impact; before the filter that
creates seen/deduped, sort results by impact descending (or build a Map keyed by
`${r.page}::${r.type}` that retains the max impact) so the highest-impact entry
for each (page,type) is encountered/kept; update the logic around results, seen,
and deduped to operate on the impact-sorted array (or use a reduce to pick max)
so the final return still sorts by impact and returns the true highest-impact
deduplicated list.
In `@apps/start/src/components/time-window-picker.tsx`:
- Around line 48-59: The useCallback for handleCustom currently only lists
startDate and endDate but closes over pushModal, onStartDateChange,
onEndDateChange, onChange, and onIntervalChange; update the dependency array of
handleCustom to include pushModal, onStartDateChange, onEndDateChange, onChange,
and onIntervalChange (in addition to startDate and endDate) so the callback
updates when any of those props/values change.
In `@apps/start/src/routes/_app`.$organizationId.$projectId.seo.tsx:
- Around line 128-134: searchEnginesQuery and aiEnginesQuery are missing the
enabled guard and will run even when GSC is not connected; modify the useQuery
calls that use trpc.gsc.getSearchEngines.queryOptions({ projectId, ...dateInput
}) and trpc.gsc.getAiEngines.queryOptions({ projectId, ...dateInput }) to
include an enabled flag (e.g., enabled: !!isConnected) so they only execute when
the isConnected boolean is true, merging the enabled option with the existing
queryOptions call.
- Around line 227-241: The sumOverview reducer correctly totals clicks and
impressions but CTR must be computed as a weighted rate, not averaged per-row:
replace the current use of (totals.ctr / n) * 100 and (prevTotals.ctr / pn) *
100 with CTR = totals.clicks === 0 || totals.impressions === 0 ? 0 :
(totals.clicks / totals.impressions) * 100 (and similarly for prevTotals), using
the existing symbols sumOverview, totals, prevTotals, overview and prevOverview;
ensure you guard against division by zero and use these computed avgCtr /
prevAvgCtr values where the CTR metric card expects them.
In
`@apps/start/src/routes/_app`.$organizationId.$projectId.settings._tabs.gsc.tsx:
- Around line 292-298: The Badge is being passed variant={syncStatusVariant as
any}, which loses type safety; change the type of syncStatusVariant (the
variable computed earlier) to the Badge component's accepted variant union (or
use a narrow type assertion) instead of any — e.g. declare syncStatusVariant as
BadgeProps['variant'] or the specific union used by Badge and remove the cast,
so the variant prop accepts syncStatusVariant without as any; update the variant
type definition where syncStatusVariant is computed to match the Badge's variant
type.
In `@packages/db/src/gsc.ts`:
- Around line 21-30: The fetch calls that obtain/refresh GSC tokens and call
Google APIs (used in the function that posts to
'https://oauth2.googleapis.com/token' as well as listGscSites and
queryGscSearchAnalytics) need request timeouts: create an AbortController for
each fetch, pass controller.signal to fetch, and set a timer (e.g., via
setTimeout) to call controller.abort() after a configurable timeout (e.g.,
5–10s); ensure you clear the timeout on success and catch the abort error to
throw a clear timeout-specific Error so callers can handle it.
In `@packages/trpc/src/routers/gsc.ts`:
- Around line 266-270: Extract the duplicated previous-period date logic into a
single helper (e.g., getPreviousPeriod) and replace the duplicated blocks in
getSearchEngines, getAiEngines, and getPreviousOverview with calls to that
helper; the helper should accept startDate and endDate (strings), compute
prevStart and prevEnd as ISO YYYY-MM-DD strings (same formatting as fmt in the
diff) and return them so callers use the returned prevStart/prevEnd instead of
recomputing startMs/duration/prevEnd/prevStart/ fmt locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5a17e7e-742c-4b6a-8da5-a8cb84dc9809
📒 Files selected for processing (49)
apps/api/src/controllers/gsc-oauth-callback.controller.tsapps/api/src/index.tsapps/api/src/routes/gsc-callback.router.tsapps/start/src/components/chat/chat-report.tsxapps/start/src/components/overview/overview-range.tsxapps/start/src/components/page/gsc-breakdown-table.tsxapps/start/src/components/page/gsc-cannibalization.tsxapps/start/src/components/page/gsc-clicks-chart.tsxapps/start/src/components/page/gsc-ctr-benchmark.tsxapps/start/src/components/page/gsc-position-chart.tsxapps/start/src/components/page/page-views-chart.tsxapps/start/src/components/page/pages-insights.tsxapps/start/src/components/pages/page-sparkline.tsxapps/start/src/components/pages/table/columns.tsxapps/start/src/components/pages/table/index.tsxapps/start/src/components/report-chart/common/serie-icon.urls.tsapps/start/src/components/report-chart/report-editor.tsxapps/start/src/components/sidebar-project-menu.tsxapps/start/src/components/time-window-picker.tsxapps/start/src/components/ui/calendar.tsxapps/start/src/components/ui/data-table/data-table.tsxapps/start/src/hooks/use-format-date-interval.tsapps/start/src/modals/date-ranger-picker.tsxapps/start/src/modals/gsc-details.tsxapps/start/src/modals/index.tsxapps/start/src/modals/page-details.tsxapps/start/src/routeTree.gen.tsapps/start/src/routes/_app.$organizationId.$projectId.pages.tsxapps/start/src/routes/_app.$organizationId.$projectId.seo.tsxapps/start/src/routes/_app.$organizationId.$projectId.settings._tabs.gsc.tsxapps/start/src/routes/_app.$organizationId.$projectId.settings._tabs.tsxapps/worker/src/boot-cron.tsapps/worker/src/boot-workers.tsapps/worker/src/jobs/cron.tsapps/worker/src/jobs/gsc.tspackages/auth/server/oauth.tspackages/auth/src/oauth.tspackages/db/code-migrations/12-add-gsc.tspackages/db/index.tspackages/db/prisma/migrations/20260306133001_gsc/migration.sqlpackages/db/prisma/schema.prismapackages/db/src/clickhouse/client.tspackages/db/src/encryption.tspackages/db/src/gsc.tspackages/db/src/services/pages.service.tspackages/queue/src/queues.tspackages/trpc/src/root.tspackages/trpc/src/routers/event.tspackages/trpc/src/routers/gsc.ts
💤 Files with no reviewable changes (1)
- packages/auth/server/oauth.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
apps/start/src/components/pages/table/index.tsx (1)
45-53:⚠️ Potential issue | 🟠 MajorRemove the hardcoded 10k cap from GSC enrichment.
Line 52 still truncates
gsc.getPagesto 10,000 rows, so larger properties will silently lose SEO metrics for some table rows. Please page through the full result set, or change the API to return GSC data only for the current page set instead of relying on a fixed client-side ceiling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/pages/table/index.tsx` around lines 45 - 53, The code sets a hardcoded cap via limit: 10_000 in the gscPagesQuery call (trpc.gsc.getPages.queryOptions) which truncates results; remove the fixed limit and either implement proper paging (call trpc.gsc.getPages repeatedly with cursor/nextPage until all pages are accumulated on the client) or change the consumer to request only the current table page (pass page/offset and pageSize into trpc.gsc.getPages.queryOptions) so the API returns just the visible rows; update gscPagesQuery usage and any state/accumulation logic to support cursor/offset-based pagination instead of the 10_000 ceiling.apps/start/src/modals/gsc-details.tsx (1)
179-189:⚠️ Potential issue | 🟡 MinorThe card still promises sessions, but only views are rendered.
This section reduces
pageTimeseriesto a singleviewsseries, and the tooltip/chart only render that metric. Either add sessions here or rename the card so it matches what users actually see.✏️ Minimal fix
- <h3 className="mb-4 font-medium text-sm">Views & Sessions</h3> + <h3 className="mb-4 font-medium text-sm">Views</h3>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/gsc-details.tsx` around lines 179 - 189, The card header promises "Views & Sessions" but the rendered data only supplies views; update the mapping for pageTimeseries to include sessions (e.g., pass data={pageTimeseries.map(r => ({ date: r.date, views: r.pageviews, sessions: r.sessions }))}) and ensure GscViewsChart (and its tooltip/series props) are updated to consume and display the sessions series; alternatively, if you prefer the minimal fix, change the heading text from "Views & Sessions" to "Views" to match the current GscViewsChart output.
🧹 Nitpick comments (1)
apps/start/src/components/page/gsc-breakdown-table.tsx (1)
39-42: Keep the breakdown rows strongly typed.These
Record<string, string | number>/unknown[]assertions discard the concretequeries/pagescontracts already returned by the GSC endpoints, so field drift here degrades to runtimeundefinedrendering instead of a compile-time error. Prefer a typed row union or let the query result types flow through directly.Also applies to: 73-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/page/gsc-breakdown-table.tsx` around lines 39 - 42, The breakdownRows declaration is using broad Record<string, string|number> and unknown[] assertions which discard the real GSC response types; change it to preserve and use the concrete types from the query results (e.g., define a union type like BreakdownRow = QueryRow | PageRow or import the response interfaces from the hooks/types and declare breakdownRows as BreakdownRow[]), then derive the value from pageQuery.data?.queries and queryQuery.data?.pages without casting to unknown—use optional chaining with the correct typed properties and default to an empty array; apply the same typed-fix pattern to the other occurrences referenced (lines 73-138) to keep compile-time type safety.
🤖 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/start/src/components/page/gsc-breakdown-table.tsx`:
- Around line 37-42: The active query's failures are being swallowed because
breakdownRows defaults to [] — add an explicit error branch that checks the
active query's isError (use pageQuery.isError when type === 'page' and
queryQuery.isError when type !== 'page') before rendering the table; if isError
is true, return or render an error state/message (surface the API/auth error)
instead of falling through to the empty-data rendering, and ensure isLoading
(the existing isLoading variable) still short-circuits while requests are
in-flight; update gsc-breakdown-table component logic around isLoading,
breakdownRows, pageQuery, and queryQuery accordingly (same change for the other
similar blocks noted in the file).
In `@apps/start/src/components/pages/table/index.tsx`:
- Around line 98-123: The table's loading state currently only uses pagesQuery,
so update both the useTable call and the DataTable loading prop to reflect all
dependent queries: compute a combined flag like const isLoading =
pagesQuery.isLoading || previousPagesQuery.isLoading || gscPagesQuery.isLoading,
then pass loading: isLoading into useTable(...) (where useTable is called) and
set DataTable's loading={isLoading} to ensure cells for comparison/GSC remain in
a loading state until all queries finish.
In `@apps/start/src/modals/gsc-details.tsx`:
- Around line 128-147: The fallback for pageOrigin/pagePath in the URL-parsing
block (the try/catch that sets pageOrigin and pagePath) currently uses
window.location.origin when value isn't an absolute URL; change this so relative
paths are resolved against the project's canonical site URL (from the project
metadata) instead of window.location.origin. Update the catch branch to obtain
the project's canonical origin (e.g., project.siteUrl or equivalent) and combine
it with the relative path value to produce origin and path, leaving the enabled
condition for the pageTimeseries useQuery unchanged; ensure you reference the
same symbols pageOrigin and pagePath so downstream trpc.event.pageTimeseries
receives the corrected origin.
- Around line 205-206: The heading uses naive string concatenation ("Top
{breakdownLabel.toLowerCase()}s") which produces incorrect plurals like
"querys"; replace this with a proper pluralization approach: update the <h3>
that references breakdownLabel to call a pluralization helper (e.g.,
pluralize(breakdownLabel.toLowerCase()) or a small utility function
pluralizeLabel) so the rendered text becomes "Top queries" (and handles other
irregular forms) instead of appending "s" directly.
In `@packages/db/src/gsc.ts`:
- Around line 287-292: The SQL SELECT uses avg(ctr) and avg(position) which
produce incorrect aggregates; replace avg(ctr) with a recomputed ratio using
sum(clicks)/nullif(sum(impressions),0) and replace avg(position) with an
impressions-weighted position computed as sum(position *
impressions)/nullif(sum(impressions),0). Update every occurrence of avg(ctr) and
avg(position) in the GSC query blocks (the SELECT that uses ${dateExpr} as date
and the other similar SELECTs mentioned) so CTR and position are derived from
summed totals and multiply/divide by impressions to avoid bias.
- Around line 285-303: The raw ClickHouse reads currently call originalCh.query
with inline SQL (see the originalCh.query invocation, dateExpr, query_params and
format:'JSONEachRow' targeting gsc_daily); replace these ad-hoc SQL strings by
using the shared ClickHouse query builder and functions (the query-builder and
query-functions modules) so the query is constructed via the repository layer
(use the same pattern as other DB reads), passing parameters through the builder
instead of query_params and keeping the JSONEachRow output handling; apply the
same refactor to the other occurrences noted (around lines 321-340 and 529-548)
so all GSC reads route through the shared builder layer rather than calling
originalCh.query with raw SQL.
- Around line 403-412: When merging rows into an existing page entry in
packages/db/src/gsc.ts (look for entry.pages and the merge block referencing
existing, row.impressions, row.clicks, row.position), replace the Math.min logic
with an impressions-weighted position update: compute the weighted average using
the pre-merge existing.impressions and row.impressions (guarding for zero total
impressions), then update existing.impressions and existing.clicks (as you
already do) and recompute existing.ctr from merged totals; ensure you use the
correct impression totals when calculating the weighted position so the merged
position reflects impressions-weighted ranking rather than the single best rank.
In `@packages/trpc/src/routers/gsc.ts`:
- Around line 414-416: The code currently calls getGscCannibalization.clear(...)
immediately before every read, forcing the 4-hour cache to miss; remove that
unconditional cache invalidation from the hot path in
packages/trpc/src/routers/gsc.ts and instead implement a temporary rollout gate:
either remove the clear call entirely or wrap it behind a short-lived cache
version/flag check (e.g., check a CACHE_ROLLOUT_VERSION or feature flag) so only
requests that opt into the rollout run clear, and keep
getGscCannibalization(input.projectId, startDate, endDate) as the normal fast
path.
- Around line 280-281: The SQL predicates compare a DateTime column to a
date-only string (e.g. "AND created_at >= '${startDate}' AND created_at <=
'${endDate}'"), which drops rows on the end date; change these predicates to use
a half-open range or date-cast instead: replace "created_at <= '${endDate}'"
with "created_at < '${endDate_plus_one_day}'" (compute endDate + 1 day in the
same code that builds the query) or, alternatively, compare the date portion
like "DATE(created_at) <= '${endDate}'". Update the same pattern occurrences
reported (the shown fragment and the other occurrences around the other SQL
blocks) so current- and previous-period totals include the full end day.
- Around line 272-293: The two chQuery calls that build raw SQL strings (the one
returning engines and the one returning prevResult) splice request values like
input.projectId, startDate, endDate, prevStart and prevEnd directly into SQL;
replace these raw strings with the shared ClickHouse query builder and helper
functions (from packages/db/src/clickhouse/query-builder.ts and
query-functions.ts) so inputs are expressed via the builder API and proper
quoting/expressions are used; update the Promise.all entries where chQuery is
called (and the similar raw queries at the later block around lines 343-358) to
construct the same SELECT, WHERE, GROUP BY, ORDER BY and LIMIT clauses using the
builder helpers instead of string interpolation, keeping TABLE_NAMES.sessions
and the same selected columns/aggregations.
- Around line 95-98: The gsc_project_id cookie is unsigned and can be tampered
with; update the OAuth flow to either HMAC-sign gsc_project_id when creating it
in initiateOAuth (e.g., setCookie('gsc_project_id', signedValue, ...)) and
verify the signature in the OAuth callback before using it, or instead persist
the projectId server-side keyed by the generated state (store state -> projectId
in short-lived session/DB in initiateOAuth and read/consume it in the callback).
Ensure you reference and update the code that sets cookies (gsc_oauth_state,
gsc_code_verifier, gsc_project_id) and the callback logic that reads
gsc_project_id so the code verifies signature or retrieves the server-side
binding and rejects mismatches or missing records.
---
Duplicate comments:
In `@apps/start/src/components/pages/table/index.tsx`:
- Around line 45-53: The code sets a hardcoded cap via limit: 10_000 in the
gscPagesQuery call (trpc.gsc.getPages.queryOptions) which truncates results;
remove the fixed limit and either implement proper paging (call
trpc.gsc.getPages repeatedly with cursor/nextPage until all pages are
accumulated on the client) or change the consumer to request only the current
table page (pass page/offset and pageSize into trpc.gsc.getPages.queryOptions)
so the API returns just the visible rows; update gscPagesQuery usage and any
state/accumulation logic to support cursor/offset-based pagination instead of
the 10_000 ceiling.
In `@apps/start/src/modals/gsc-details.tsx`:
- Around line 179-189: The card header promises "Views & Sessions" but the
rendered data only supplies views; update the mapping for pageTimeseries to
include sessions (e.g., pass data={pageTimeseries.map(r => ({ date: r.date,
views: r.pageviews, sessions: r.sessions }))}) and ensure GscViewsChart (and its
tooltip/series props) are updated to consume and display the sessions series;
alternatively, if you prefer the minimal fix, change the heading text from
"Views & Sessions" to "Views" to match the current GscViewsChart output.
---
Nitpick comments:
In `@apps/start/src/components/page/gsc-breakdown-table.tsx`:
- Around line 39-42: The breakdownRows declaration is using broad Record<string,
string|number> and unknown[] assertions which discard the real GSC response
types; change it to preserve and use the concrete types from the query results
(e.g., define a union type like BreakdownRow = QueryRow | PageRow or import the
response interfaces from the hooks/types and declare breakdownRows as
BreakdownRow[]), then derive the value from pageQuery.data?.queries and
queryQuery.data?.pages without casting to unknown—use optional chaining with the
correct typed properties and default to an empty array; apply the same typed-fix
pattern to the other occurrences referenced (lines 73-138) to keep compile-time
type safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9613aab7-7f4f-44b0-b664-bf318d8d516f
📒 Files selected for processing (12)
apps/api/src/controllers/gsc-oauth-callback.controller.tsapps/start/src/components/page/gsc-breakdown-table.tsxapps/start/src/components/page/gsc-cannibalization.tsxapps/start/src/components/pages/page-sparkline.tsxapps/start/src/components/pages/table/columns.tsxapps/start/src/components/pages/table/index.tsxapps/start/src/modals/gsc-details.tsxapps/start/src/modals/page-details.tsxpackages/db/src/gsc.tspackages/db/src/services/pages.service.tspackages/trpc/src/routers/event.tspackages/trpc/src/routers/gsc.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/start/src/modals/page-details.tsx
- apps/start/src/components/pages/table/columns.tsx
- packages/trpc/src/routers/event.ts
- apps/start/src/components/page/gsc-cannibalization.tsx
- apps/start/src/components/pages/page-sparkline.tsx
- packages/db/src/services/pages.service.ts
Comment on lines
+37
to
+42
| const isLoading = type === 'page' ? pageQuery.isLoading : queryQuery.isLoading; | ||
|
|
||
| const breakdownRows: Record<string, string | number>[] = | ||
| type === 'page' | ||
| ? ((pageQuery.data as { queries?: unknown[] } | undefined)?.queries ?? []) as Record<string, string | number>[] | ||
| : ((queryQuery.data as { pages?: unknown[] } | undefined)?.pages ?? []) as Record<string, string | number>[]; |
Contributor
There was a problem hiding this comment.
Surface request failures instead of showing an empty breakdown.
If the active GSC query fails, breakdownRows falls back to [] and this renders the same as a legitimate “no data” state. That will hide auth/API failures from users. Please add an explicit isError branch for the active query before rendering the table.
🩹 Suggested guard
- const isLoading = type === 'page' ? pageQuery.isLoading : queryQuery.isLoading;
+ const activeQuery = type === 'page' ? pageQuery : queryQuery;
+ const isLoading = activeQuery.isLoading;
...
- return (
+ if (activeQuery.isError) {
+ return (
+ <div className="card overflow-hidden">
+ <div className="border-b p-4">
+ <h3 className="font-medium text-sm">Top {pluralLabel}</h3>
+ </div>
+ <div className="p-4 text-sm text-muted-foreground">
+ Failed to load {pluralLabel}. Retry or reconnect Google Search Console.
+ </div>
+ </div>
+ );
+ }
+
+ return (Also applies to: 53-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/components/page/gsc-breakdown-table.tsx` around lines 37 - 42,
The active query's failures are being swallowed because breakdownRows defaults
to [] — add an explicit error branch that checks the active query's isError (use
pageQuery.isError when type === 'page' and queryQuery.isError when type !==
'page') before rendering the table; if isError is true, return or render an
error state/message (surface the API/auth error) instead of falling through to
the empty-data rendering, and ensure isLoading (the existing isLoading variable)
still short-circuits while requests are in-flight; update gsc-breakdown-table
component logic around isLoading, breakdownRows, pageQuery, and queryQuery
accordingly (same change for the other similar blocks noted in the file).
Comment on lines
+98
to
+123
| const { table } = useTable({ | ||
| columns, | ||
| data: rawData, | ||
| loading: pagesQuery.isLoading, | ||
| pageSize: 50, | ||
| name: 'pages', | ||
| }); | ||
|
|
||
| return ( | ||
| <> | ||
| <DataTableToolbarContainer> | ||
| <AnimatedSearchInput | ||
| placeholder="Search pages" | ||
| value={search ?? ''} | ||
| onChange={setSearch} | ||
| /> | ||
| <div className="flex items-center gap-2"> | ||
| <OverviewRange /> | ||
| <OverviewInterval /> | ||
| <DataTableViewOptions table={table} /> | ||
| </div> | ||
| </DataTableToolbarContainer> | ||
| <DataTable | ||
| table={table} | ||
| loading={pagesQuery.isLoading} | ||
| empty={{ |
Contributor
There was a problem hiding this comment.
Include the dependent queries in the table loading state.
The table stops loading as soon as pagesQuery resolves, even if previousPagesQuery or gscPagesQuery are still in flight. That can render empty comparison / GSC cells as if the data were final.
Suggested change
+ const isTableLoading =
+ pagesQuery.isLoading ||
+ connectionQuery.isLoading ||
+ previousPagesQuery.isLoading ||
+ (isGscConnected && gscPagesQuery.isLoading);
+
const { table } = useTable({
columns,
data: rawData,
- loading: pagesQuery.isLoading,
+ loading: isTableLoading,
pageSize: 50,
name: 'pages',
});
...
<DataTable
table={table}
- loading={pagesQuery.isLoading}
+ loading={isTableLoading}📝 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 { table } = useTable({ | |
| columns, | |
| data: rawData, | |
| loading: pagesQuery.isLoading, | |
| pageSize: 50, | |
| name: 'pages', | |
| }); | |
| return ( | |
| <> | |
| <DataTableToolbarContainer> | |
| <AnimatedSearchInput | |
| placeholder="Search pages" | |
| value={search ?? ''} | |
| onChange={setSearch} | |
| /> | |
| <div className="flex items-center gap-2"> | |
| <OverviewRange /> | |
| <OverviewInterval /> | |
| <DataTableViewOptions table={table} /> | |
| </div> | |
| </DataTableToolbarContainer> | |
| <DataTable | |
| table={table} | |
| loading={pagesQuery.isLoading} | |
| empty={{ | |
| const isTableLoading = | |
| pagesQuery.isLoading || | |
| connectionQuery.isLoading || | |
| previousPagesQuery.isLoading || | |
| (isGscConnected && gscPagesQuery.isLoading); | |
| const { table } = useTable({ | |
| columns, | |
| data: rawData, | |
| loading: isTableLoading, | |
| pageSize: 50, | |
| name: 'pages', | |
| }); | |
| return ( | |
| <> | |
| <DataTableToolbarContainer> | |
| <AnimatedSearchInput | |
| placeholder="Search pages" | |
| value={search ?? ''} | |
| onChange={setSearch} | |
| /> | |
| <div className="flex items-center gap-2"> | |
| <OverviewRange /> | |
| <OverviewInterval /> | |
| <DataTableViewOptions table={table} /> | |
| </div> | |
| </DataTableToolbarContainer> | |
| <DataTable | |
| table={table} | |
| loading={isTableLoading} | |
| empty={{ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/components/pages/table/index.tsx` around lines 98 - 123, The
table's loading state currently only uses pagesQuery, so update both the
useTable call and the DataTable loading prop to reflect all dependent queries:
compute a combined flag like const isLoading = pagesQuery.isLoading ||
previousPagesQuery.isLoading || gscPagesQuery.isLoading, then pass loading:
isLoading into useTable(...) (where useTable is called) and set DataTable's
loading={isLoading} to ensure cells for comparison/GSC remain in a loading state
until all queries finish.
Comment on lines
+128
to
+147
| const { origin: pageOrigin, path: pagePath } = | ||
| type === 'page' | ||
| ? (() => { | ||
| try { | ||
| const url = new URL(value); | ||
| return { origin: url.origin, path: url.pathname + url.search }; | ||
| } catch { | ||
| return { | ||
| origin: typeof window !== 'undefined' ? window.location.origin : '', | ||
| path: value, | ||
| }; | ||
| } | ||
| })() | ||
| : { origin: '', path: '' }; | ||
|
|
||
| const pageTimeseriesQuery = useQuery( | ||
| trpc.event.pageTimeseries.queryOptions( | ||
| { projectId, ...dateInput, origin: pageOrigin, path: pagePath }, | ||
| { enabled: type === 'page' && !!pagePath } | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't fall back to the dashboard origin for page analytics.
If value isn't an absolute URL, this code queries event.pageTimeseries with window.location.origin, which points at the OpenPanel app, not the tracked site. That can show unrelated metrics for the selected page instead of failing safely.
🛠️ Safer fallback
? (() => {
try {
const url = new URL(value);
return { origin: url.origin, path: url.pathname + url.search };
} catch {
- return {
- origin: typeof window !== 'undefined' ? window.location.origin : '',
- path: value,
- };
+ return { origin: '', path: '' };
}
})()
: { origin: '', path: '' };
const pageTimeseriesQuery = useQuery(
trpc.event.pageTimeseries.queryOptions(
{ projectId, ...dateInput, origin: pageOrigin, path: pagePath },
- { enabled: type === 'page' && !!pagePath }
+ { enabled: type === 'page' && !!pageOrigin && !!pagePath }
)
);If relative paths are expected here, resolve them from the project's canonical site URL instead of window.location.origin.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/modals/gsc-details.tsx` around lines 128 - 147, The fallback
for pageOrigin/pagePath in the URL-parsing block (the try/catch that sets
pageOrigin and pagePath) currently uses window.location.origin when value isn't
an absolute URL; change this so relative paths are resolved against the
project's canonical site URL (from the project metadata) instead of
window.location.origin. Update the catch branch to obtain the project's
canonical origin (e.g., project.siteUrl or equivalent) and combine it with the
relative path value to produce origin and path, leaving the enabled condition
for the pageTimeseries useQuery unchanged; ensure you reference the same symbols
pageOrigin and pagePath so downstream trpc.event.pageTimeseries receives the
corrected origin.
Comment on lines
+205
to
+206
| <h3 className="font-medium text-sm"> | ||
| Top {breakdownLabel.toLowerCase()}s |
Contributor
There was a problem hiding this comment.
Pluralize this heading explicitly.
Top {breakdownLabel.toLowerCase()}s renders Top querys in page mode.
✏️ Proposed fix
- <h3 className="font-medium text-sm">
- Top {breakdownLabel.toLowerCase()}s
- </h3>
+ <h3 className="font-medium text-sm">
+ {type === 'page' ? 'Top queries' : 'Top pages'}
+ </h3>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/modals/gsc-details.tsx` around lines 205 - 206, The heading
uses naive string concatenation ("Top {breakdownLabel.toLowerCase()}s") which
produces incorrect plurals like "querys"; replace this with a proper
pluralization approach: update the <h3> that references breakdownLabel to call a
pluralization helper (e.g., pluralize(breakdownLabel.toLowerCase()) or a small
utility function pluralizeLabel) so the rendered text becomes "Top queries" (and
handles other irregular forms) instead of appending "s" directly.
Comment on lines
+27
to
+30
| if (!res.ok) { | ||
| const text = await res.text(); | ||
| throw new Error(`Failed to refresh GSC token: ${text}`); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/db/src/gsc.tsRepository: Openpanel-dev/openpanel
Length of output: 18416
Only mark token_expired for explicit auth failures; let transient errors bubble separately.
The catch block at lines 61–72 marks the connection as expired for ANY refresh failure, including transient network errors and Google 5xx responses. This causes false reconnect prompts and noisy state. Parse Google's error response in refreshGscToken to distinguish auth failures (e.g., invalid_grant) from transient errors; only classify the former as token_expired and let other failures propagate for retry handling.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (7)
packages/trpc/src/routers/gsc.ts (3)
95-98:⚠️ Potential issue | 🔴 CriticalSign the
gsc_project_idcookie or use server-side state binding.The
gsc_project_idcookie is unsigned, allowing an attacker to modify it before completing OAuth to bind GSC credentials to a different project. Either HMAC-sign the cookie value or store the project binding server-side keyed by the OAuth state parameter.🔒 Proposed fix using state-based binding
Consider encoding the projectId into the state parameter itself (signed), so the callback can extract it without relying on an unsigned cookie:
+import { createHmac } from 'crypto'; + +function signState(state: string, projectId: string): string { + const payload = `${state}:${projectId}`; + const sig = createHmac('sha256', process.env.COOKIE_SECRET ?? '') + .update(payload) + .digest('hex') + .slice(0, 16); + return `${payload}:${sig}`; +} const state = Arctic.generateState(); const codeVerifier = Arctic.generateCodeVerifier(); + const signedState = signState(state, input.projectId); const url = googleGsc.createAuthorizationURL(state, codeVerifier, [Then in the callback, parse and verify the signed state to extract projectId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/gsc.ts` around lines 95 - 98, The gsc_project_id cookie is unsigned and can be tampered with; fix by moving project binding into the signed state or storing it server-side keyed by the OAuth state instead of using ctx.setCookie('gsc_project_id', ...). When creating the OAuth request in the function that sets ctx.setCookie('gsc_oauth_state', state, ...) and ctx.setCookie('gsc_code_verifier', codeVerifier, ...), include the projectId inside the signed state (or persist { state -> projectId } in a server-side store keyed by the same state) and remove the unsigned ctx.setCookie('gsc_project_id', ...); then update the OAuth callback handler to verify and parse the signed state (or look up projectId by state) to recover the trusted projectId for completing the flow.
272-294:⚠️ Potential issue | 🟠 MajorReplace raw SQL with query builder; fix date boundary.
These queries use string interpolation instead of the required query builder, and
created_at <= '${endDate}'excludes most of the last day (compares DateTime to midnight).As per coding guidelines: "When writing ClickHouse queries, always use the custom query builder".
🛠️ Proposed fix for date boundary
For the date boundary specifically, use
< nextDayinstead of<= endDate:+ const nextDay = new Date(new Date(endDate).getTime() + 86400000) + .toISOString().slice(0, 10); + const [engines, [prevResult]] = await Promise.all([ chQuery<{ name: string; sessions: number }>( `SELECT referrer_name as name, count(*) as sessions FROM ${TABLE_NAMES.sessions} WHERE project_id = '${input.projectId}' AND referrer_type = 'search' AND created_at >= '${startDate}' - AND created_at <= '${endDate}' + AND created_at < '${nextDay}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/gsc.ts` around lines 272 - 294, The two chQuery calls (the one returning engines and the one returning prevResult) should be rewritten to use the project's ClickHouse query builder instead of raw string interpolation; replace the interpolated TABLE_NAMES.sessions and string SQL with builder calls that bind input.projectId and the date ranges safely, and avoid SQL injection. Also fix the date boundary: change any "created_at <= endDate" usage to use a strict upper bound of the next day (e.g., created_at < nextDay(endDate)) or the query-builder equivalent so the entire final day is included; apply the same change for prevStart/prevEnd (and use fmt only if the builder requires formatted dates). Ensure you update the Promise.all usage to call the builder-based queries returning the same shapes used by engines and prevResult.
337-358:⚠️ Potential issue | 🟠 MajorSame issues: use query builder and fix date boundaries.
Same concerns as
getSearchEngines: raw SQL with string interpolation and<= endDatedate boundary issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/gsc.ts` around lines 337 - 358, The current code builds raw SQL via string interpolation in the local where function (used by chQuery against TABLE_NAMES.sessions) and uses a <= endDate boundary; change to a parameterized query builder instead of interpolating projectId and aiNames (avoid injecting aiNames, projectId, fmt(prevStart)/fmt(prevEnd) directly), passing those values as parameters to chQuery or your ClickHouse client, and adjust the date boundary to use an exclusive end (e.g., created_at >= start AND created_at < end + INTERVAL 1 DAY or compute end as endOfDayExclusive) so the end date is not double-counted; update the two chQuery calls that reference where(...) (and any fmt(...) usage) to use the new parameterized builders.packages/db/src/gsc.ts (4)
285-304:⚠️ Potential issue | 🟠 MajorUse the shared ClickHouse query builder and fix CTR/position aggregation.
This query bypasses the required query builder layer and uses
avg(ctr)andavg(position)which produce mathematically incorrect results when aggregating ratios. CTR should be recomputed from totals, and position should be impression-weighted.As per coding guidelines: "When writing ClickHouse queries, always use the custom query builder from
./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts".🛠️ Proposed SQL fix for correct aggregation
SELECT ${dateExpr} as date, sum(clicks) as clicks, sum(impressions) as impressions, - avg(ctr) as ctr, - avg(position) as position + if(sum(impressions) > 0, sum(clicks) / sum(impressions), 0) as ctr, + if(sum(impressions) > 0, sum(position * impressions) / sum(impressions), 0) as position FROM gsc_daily🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/gsc.ts` around lines 285 - 304, The query in originalCh.query in gsc.ts bypasses the shared ClickHouse query builder and incorrectly uses avg(ctr) and avg(position); replace this raw SQL with the project's query builder (import from ./packages/db/src/clickhouse/query-builder.ts and ./packages/db/src/clickhouse/query-functions.ts) and change CTR/position aggregation to compute CTR as sum(clicks)/sum(impressions) and impression-weighted position as sum(position * impressions)/sum(impressions) while keeping dateExpr, gsc_daily, project_id and the same WHERE/GROUP BY/ORDER BY semantics; ensure you pass query_params { projectId, startDate, endDate } and return result.json() as before.
533-553:⚠️ Potential issue | 🟠 MajorSame aggregation issues: use query builder and fix CTR/position.
Same concerns as the other ClickHouse queries: bypasses query builder and uses incorrect
avg(ctr)/avg(position)aggregation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/gsc.ts` around lines 533 - 553, The raw SQL in the function using originalCh.query bypasses the project's query builder and incorrectly aggregates ctr and position with avg(ctr)/avg(position); replace this raw query with the query builder call consistent with other functions and compute CTR and position as weighted metrics: use clicks_sum = sum(clicks), impressions_sum = sum(impressions), ctr = clicks_sum / NULLIF(impressions_sum, 0) (or equivalent to avoid div-by-zero), and position = sum(position * impressions) / NULLIF(impressions_sum, 0); keep grouping by query and ordering by clicks_sum, and pass the same params (projectId, startDate, endDate, limit) through the builder instead of embedding them in a raw template. Ensure you update the function that currently calls originalCh.query to use the query builder and the new aggregated expressions.
321-341:⚠️ Potential issue | 🟠 MajorSame aggregation issues: use query builder and fix CTR/position.
Same concerns as
getGscOverview: bypasses query builder and uses incorrectavg(ctr)/avg(position)aggregation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/gsc.ts` around lines 321 - 341, The query in the block using originalCh.query bypasses the query builder and incorrectly aggregates avg(ctr)/avg(position); change this to use the project's ClickHouse query builder (replace originalCh.query call) and compute CTR and position from totals: select sum(clicks) as clicks, sum(impressions) as impressions, (sum(clicks) / nullIf(sum(impressions), 0)) as ctr, and weighted position as (sum(position * impressions) / nullIf(sum(impressions), 0)) as position, keep GROUP BY page / ORDER BY clicks DESC / LIMIT {limit} and pass parameters via the builder rather than inline query_params. Ensure you reference and update the call site that currently invokes originalCh.query in this module (the function around this diff) to use the query builder API.
61-73:⚠️ Potential issue | 🟠 MajorDistinguish auth failures from transient errors when marking token status.
The catch block marks the connection as
token_expiredfor any refresh failure, including transient network errors and Google 5xx responses. This causes false reconnect prompts. Parse the error response to distinguish auth failures (e.g.,invalid_grant) from transient errors; only classify the former astoken_expired.🛡️ Proposed fix
} catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Failed to refresh token'; + const isAuthError = errorMessage.includes('invalid_grant') || + errorMessage.includes('Token has been expired or revoked'); + await db.gscConnection.update({ where: { projectId }, data: { - lastSyncStatus: 'token_expired', - lastSyncError: - error instanceof Error ? error.message : 'Failed to refresh token', + lastSyncStatus: isAuthError ? 'token_expired' : 'sync_error', + lastSyncError: errorMessage, }, }); + + if (!isAuthError) { + throw error; // Let transient errors bubble up for retry + } throw new Error( 'GSC token has expired or been revoked. Please reconnect Google Search Console.' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/gsc.ts` around lines 61 - 73, The catch block currently sets lastSyncStatus to 'token_expired' for any error; change the logic in the catch around db.gscConnection.update (projectId) to inspect the thrown error (e.g., check error.code, error.response?.status, and error.response?.data or error.message for OAuth errors like 'invalid_grant' or explicit auth error fields) and only set lastSyncStatus: 'token_expired' and the specific lastSyncError when the error clearly indicates an auth/refresh failure; for other transient/network/5xx errors set a different status (e.g., 'failed' or 'retry') and persist the actual error details, then rethrow the original error so upstream can retry. Ensure you reference the same update call (db.gscConnection.update) and preserve projectId when updating.
🧹 Nitpick comments (1)
packages/db/src/gsc.ts (1)
126-155: Consider adding rate limiting or exponential backoff for large paginated fetches.The pagination loop makes unbounded sequential API calls without delay. For projects with large datasets, this could trigger Google's rate limits, causing 429 errors mid-sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/gsc.ts` around lines 126 - 155, The pagination loop in the while (true) block can hit Google rate limits; add an exponential-backoff retry strategy around the fetch call used with url/accessToken (inside the while loop that advances startRow by rowLimit) so transient 429/5xx/network errors are retried with increasing delays (and jitter) up to a capped number of attempts, honouring a Retry-After header when present, and only throw after max retries; on success reset retry state and continue pagination, and implement a small sleep utility (Promise-based) to pause between retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/db/src/gsc.ts`:
- Around line 285-304: The query in originalCh.query in gsc.ts bypasses the
shared ClickHouse query builder and incorrectly uses avg(ctr) and avg(position);
replace this raw SQL with the project's query builder (import from
./packages/db/src/clickhouse/query-builder.ts and
./packages/db/src/clickhouse/query-functions.ts) and change CTR/position
aggregation to compute CTR as sum(clicks)/sum(impressions) and
impression-weighted position as sum(position * impressions)/sum(impressions)
while keeping dateExpr, gsc_daily, project_id and the same WHERE/GROUP BY/ORDER
BY semantics; ensure you pass query_params { projectId, startDate, endDate } and
return result.json() as before.
- Around line 533-553: The raw SQL in the function using originalCh.query
bypasses the project's query builder and incorrectly aggregates ctr and position
with avg(ctr)/avg(position); replace this raw query with the query builder call
consistent with other functions and compute CTR and position as weighted
metrics: use clicks_sum = sum(clicks), impressions_sum = sum(impressions), ctr =
clicks_sum / NULLIF(impressions_sum, 0) (or equivalent to avoid div-by-zero),
and position = sum(position * impressions) / NULLIF(impressions_sum, 0); keep
grouping by query and ordering by clicks_sum, and pass the same params
(projectId, startDate, endDate, limit) through the builder instead of embedding
them in a raw template. Ensure you update the function that currently calls
originalCh.query to use the query builder and the new aggregated expressions.
- Around line 321-341: The query in the block using originalCh.query bypasses
the query builder and incorrectly aggregates avg(ctr)/avg(position); change this
to use the project's ClickHouse query builder (replace originalCh.query call)
and compute CTR and position from totals: select sum(clicks) as clicks,
sum(impressions) as impressions, (sum(clicks) / nullIf(sum(impressions), 0)) as
ctr, and weighted position as (sum(position * impressions) /
nullIf(sum(impressions), 0)) as position, keep GROUP BY page / ORDER BY clicks
DESC / LIMIT {limit} and pass parameters via the builder rather than inline
query_params. Ensure you reference and update the call site that currently
invokes originalCh.query in this module (the function around this diff) to use
the query builder API.
- Around line 61-73: The catch block currently sets lastSyncStatus to
'token_expired' for any error; change the logic in the catch around
db.gscConnection.update (projectId) to inspect the thrown error (e.g., check
error.code, error.response?.status, and error.response?.data or error.message
for OAuth errors like 'invalid_grant' or explicit auth error fields) and only
set lastSyncStatus: 'token_expired' and the specific lastSyncError when the
error clearly indicates an auth/refresh failure; for other transient/network/5xx
errors set a different status (e.g., 'failed' or 'retry') and persist the actual
error details, then rethrow the original error so upstream can retry. Ensure you
reference the same update call (db.gscConnection.update) and preserve projectId
when updating.
In `@packages/trpc/src/routers/gsc.ts`:
- Around line 95-98: The gsc_project_id cookie is unsigned and can be tampered
with; fix by moving project binding into the signed state or storing it
server-side keyed by the OAuth state instead of using
ctx.setCookie('gsc_project_id', ...). When creating the OAuth request in the
function that sets ctx.setCookie('gsc_oauth_state', state, ...) and
ctx.setCookie('gsc_code_verifier', codeVerifier, ...), include the projectId
inside the signed state (or persist { state -> projectId } in a server-side
store keyed by the same state) and remove the unsigned
ctx.setCookie('gsc_project_id', ...); then update the OAuth callback handler to
verify and parse the signed state (or look up projectId by state) to recover the
trusted projectId for completing the flow.
- Around line 272-294: The two chQuery calls (the one returning engines and the
one returning prevResult) should be rewritten to use the project's ClickHouse
query builder instead of raw string interpolation; replace the interpolated
TABLE_NAMES.sessions and string SQL with builder calls that bind input.projectId
and the date ranges safely, and avoid SQL injection. Also fix the date boundary:
change any "created_at <= endDate" usage to use a strict upper bound of the next
day (e.g., created_at < nextDay(endDate)) or the query-builder equivalent so the
entire final day is included; apply the same change for prevStart/prevEnd (and
use fmt only if the builder requires formatted dates). Ensure you update the
Promise.all usage to call the builder-based queries returning the same shapes
used by engines and prevResult.
- Around line 337-358: The current code builds raw SQL via string interpolation
in the local where function (used by chQuery against TABLE_NAMES.sessions) and
uses a <= endDate boundary; change to a parameterized query builder instead of
interpolating projectId and aiNames (avoid injecting aiNames, projectId,
fmt(prevStart)/fmt(prevEnd) directly), passing those values as parameters to
chQuery or your ClickHouse client, and adjust the date boundary to use an
exclusive end (e.g., created_at >= start AND created_at < end + INTERVAL 1 DAY
or compute end as endOfDayExclusive) so the end date is not double-counted;
update the two chQuery calls that reference where(...) (and any fmt(...) usage)
to use the new parameterized builders.
---
Nitpick comments:
In `@packages/db/src/gsc.ts`:
- Around line 126-155: The pagination loop in the while (true) block can hit
Google rate limits; add an exponential-backoff retry strategy around the fetch
call used with url/accessToken (inside the while loop that advances startRow by
rowLimit) so transient 429/5xx/network errors are retried with increasing delays
(and jitter) up to a capped number of attempts, honouring a Retry-After header
when present, and only throw after max retries; on success reset retry state and
continue pagination, and implement a small sleep utility (Promise-based) to
pause between retries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 282d8ab9-8a79-454f-8589-dcc0f49aa6b2
📒 Files selected for processing (2)
packages/db/src/gsc.tspackages/trpc/src/routers/gsc.ts
Contributor
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)
packages/trpc/src/trpc.ts (1)
36-42:⚠️ Potential issue | 🟠 MajorRequire a non-empty cookie secret for signed cookies to work reliably.
The code at line 40 now passes
options.signedtores.setCookie(), enabling signed cookies. However,apps/api/src/index.ts:143registers the Fastify cookie plugin withsecret: process.env.COOKIE_SECRET ?? ''. WhenCOOKIE_SECRETis unset, the empty secret string breaks cookie integrity verification downstream.This impacts the GSC OAuth flow:
packages/trpc/src/routers/gsc.ts:95-98sets signed cookies forgsc_oauth_state,gsc_code_verifier, andgsc_project_id, which are then unsigned inapps/api/src/controllers/gsc-oauth-callback.controller.ts:48-52. Without a real secret, these integrity guarantees fail.Either enforce a non-empty
COOKIE_SECRETenvironment variable or use unsigned cookies for OAuth state management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/trpc.ts` around lines 36 - 42, The setCookie implementation allows signed cookies but doesn't ensure a real cookie secret is configured; update the setCookie function so it only passes signed: true to res.setCookie when a non-empty secret is present (e.g., check process.env.COOKIE_SECRET or the app's COOKIE_OPTIONS.secret) and otherwise force signed: false (or throw during app startup to enforce a secret), ensuring OAuth cookies set in packages/trpc/src/routers/gsc.ts (gsc_oauth_state, gsc_code_verifier, gsc_project_id) are not marked signed when no secret exists; adjust the call site in setCookie (res.setCookie(..., { signed: ... })) accordingly and keep COOKIE_OPTIONS merged as before.
🤖 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/controllers/gsc-oauth-callback.controller.ts`:
- Around line 142-143: The code currently uses a non-null assertion for
DASHBOARD_URL/NEXT_PUBLIC_DASHBOARD_URL when building dashboardUrl and in
redirectWithError, which can produce runtime errors if neither env var is set;
update the logic in the scope where dashboardUrl is computed (and in
redirectWithError) to explicitly check for the presence of
process.env.DASHBOARD_URL or process.env.NEXT_PUBLIC_DASHBOARD_URL, throw or
return a clear error if both are missing, or provide a safe fallback URL (e.g.,
"/") before calling new URL or performing redirects, and remove the non-null
assertion to ensure runtime safety (refer to the dashboardUrl constant and the
redirectWithError function).
---
Outside diff comments:
In `@packages/trpc/src/trpc.ts`:
- Around line 36-42: The setCookie implementation allows signed cookies but
doesn't ensure a real cookie secret is configured; update the setCookie function
so it only passes signed: true to res.setCookie when a non-empty secret is
present (e.g., check process.env.COOKIE_SECRET or the app's
COOKIE_OPTIONS.secret) and otherwise force signed: false (or throw during app
startup to enforce a secret), ensuring OAuth cookies set in
packages/trpc/src/routers/gsc.ts (gsc_oauth_state, gsc_code_verifier,
gsc_project_id) are not marked signed when no secret exists; adjust the call
site in setCookie (res.setCookie(..., { signed: ... })) accordingly and keep
COOKIE_OPTIONS merged as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0930c732-37ad-4e70-a7a1-2168d2f78a4d
📒 Files selected for processing (4)
apps/api/src/controllers/gsc-oauth-callback.controller.tspackages/trpc/src/routers/gsc.tspackages/trpc/src/trpc.tspackages/validation/src/types.validation.ts
Comment on lines
+142
to
+143
| const dashboardUrl = | ||
| process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL!; |
Contributor
There was a problem hiding this comment.
Non-null assertion on environment variable could cause runtime errors.
If neither DASHBOARD_URL nor NEXT_PUBLIC_DASHBOARD_URL is set, this results in undefined, causing the redirect to fail. The same issue exists in redirectWithError where new URL(undefined) would throw.
🛡️ Proposed fix to add validation or fallback
+ const dashboardUrl =
+ process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL;
+ if (!dashboardUrl) {
+ throw new LogError('DASHBOARD_URL environment variable is not configured');
+ }
- const dashboardUrl =
- process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL!;
const redirectUrl = `${dashboardUrl}/${project.organizationId}/${projectIdStr}/settings/gsc`;And similarly in redirectWithError:
function redirectWithError(reply: FastifyReply, error: LogError | unknown) {
+ const baseUrl = process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL;
+ if (!baseUrl) {
+ // Fallback to a generic error response if URL is not configured
+ return reply.status(500).send({ error: 'Server configuration error' });
+ }
- const url = new URL(
- process.env.DASHBOARD_URL || process.env.NEXT_PUBLIC_DASHBOARD_URL!
- );
+ const url = new URL(baseUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/controllers/gsc-oauth-callback.controller.ts` around lines 142 -
143, The code currently uses a non-null assertion for
DASHBOARD_URL/NEXT_PUBLIC_DASHBOARD_URL when building dashboardUrl and in
redirectWithError, which can produce runtime errors if neither env var is set;
update the logic in the scope where dashboardUrl is computed (and in
redirectWithError) to explicitly check for the presence of
process.env.DASHBOARD_URL or process.env.NEXT_PUBLIC_DASHBOARD_URL, throw or
return a clear error if both are missing, or provide a safe fallback URL (e.g.,
"/") before calling new URL or performing redirects, and remove the non-null
assertion to ensure runtime safety (refer to the dashboardUrl constant and the
redirectWithError function).
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.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements