Redesign the dashboard entirely#207
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
apps/dashboard/app/(auth)/register/page.tsx (1)
541-550: Remove unnecessary/conflicting CSS classes from the Link.The Link component has several classes that don't apply correctly in this context:
flex-1has no effect since the parent<p>is not a flex containerh-autoandp-0are button-like utilities that don't make sense for an inline linktext-rightconflicts with the inline text flow<Link - className="h-auto flex-1 cursor-pointer p-0 text-right font-medium text-[13px] text-accent-foreground transition-colors duration-200 hover:text-accent-foreground/60" + className="font-medium text-[13px] text-accent-foreground transition-colors duration-200 hover:text-accent-foreground/60" href={ callbackUrl ? `/login?callback=${encodeURIComponent(callbackUrl)}` : "/login" } >apps/api/src/lib/clickhouse-backup.ts (1)
178-184: Advisory: Consider parameterized queries for INSERT statements.While
backupNameis timestamp-derived and safe,lastBackupPathoriginates from a previous database query. If the database were compromised, this could enable SQL injection. Consider using parameterized queries consistently throughout the file, similar to the improvement made ingetBackupMetadata.The same applies to the error message escaping on line 245—
replace(/'/g, "\\'")may not handle all edge cases (e.g., backslashes in error messages). ClickHouse's parameterized queries would be more robust:await clickHouseOG.command({ query: ` INSERT INTO internal.databuddy_backups (backup_name, backup_path, backup_type, base_backup_path, status) VALUES ({name:String}, {path:String}, {type:String}, {base:String}, 'started') `, query_params: { name: backupName, path: s3Url, type: isIncremental ? "incremental" : "full", base: lastBackupPath ?? "", }, });apps/api/src/query/builders/profiles.ts (1)
55-55: Inconsistent page_views calculation.The
visitor_sessionsCTE counts all events as page views usingCOUNT(*), while theprofile_sessionsquery (line 212) correctly filters to onlyscreen_viewevents usingcountIf(event_name = 'screen_view'). This creates the same inconsistency issue as the unique_pages metric.Apply this diff to align with the corrected logic:
- COUNT(*) as page_views, + countIf(e.event_name = 'screen_view') as page_views,apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx (2)
951-951: Avoidas anytype assertion.Using
as anybypasses TypeScript's type checking. Consider fixing the type incompatibility betweenpagesTabsand theDataTablecomponent's expectedtabsprop type, either by adjusting thepagesTabstype or updating the component's type definitions.
978-987: Fix device type mapping: "laptop" should map to "desktop", not "mobile".The mapping incorrectly converts "laptop" display name to "mobile" device_type value. Laptops are desktop-class devices and should map to "desktop" for correct filtering behavior. Users selecting laptop devices will currently get mobile device analytics instead.
Change line 980 from
laptop: "mobile"tolaptop: "desktop".apps/dashboard/app/(main)/organizations/settings/api-keys/api-key-settings.tsx (1)
56-63: Tighten query typing to avoid theas ApiKeyRowItem[]cast
datais currently cast toApiKeyRowItem[]via(data ?? []) as ApiKeyRowItem[], which hides mismatches between the RPC response andApiKeyRowItem. Prefer wiringuseQueryso it’s correctly typed up front, e.g. by:
- Ensuring
orpc.apikeys.list.queryOptionsis generic and returns aUseQueryOptions<ApiKeyRowItem[]>, or- Calling
useQuery<ApiKeyRowItem[]>({ queryKey, queryFn, ... })explicitly without a loose cast.This keeps the RPC layer and UI in sync and lets TS catch shape changes.
Also applies to: 65-66
apps/dashboard/app/(main)/organizations/settings/api-keys/api-key-row.tsx (1)
14-24: EnsureApiKeyRowItemdate fields match the actual RPC payload
ApiKeyRowItemdeclares:revokedAt: Date | null; expiresAt: string | null; createdAt: Date;If
orpc.apikeys.list(or the underlying API) returns plain JSON, these are typically ISO strings, notDateinstances. In that case, the current typing is misleading and can hide serialization/hydration issues.Consider either:
- Typing them as
string | null(and lettingdayjsaccept strings), or- Normalizing to real
Dateobjects at the boundary (and making sure the RPC layer reflects that in its types).Aligning the type here with the actual transport avoids subtle bugs later.
apps/dashboard/app/(main)/billing/components/billing-header.tsx (1)
5-36: Billing header refactor to sharedPageHeaderlooks goodThe
usePathname→PAGE_TITLES/DEFAULT_TITLEmapping is clean, and delegating layout toPageHeaderimproves consistency. One small nit:PageHeaderunconditionally overrides the iconweightto"fill", soweight="duotone"onCreditCardIconhere is effectively ignored; consider dropping it (or adding a configurableiconWeightprop toPageHeader) to avoid confusion.apps/api/src/schemas/query-schemas.ts (1)
8-24: Keep filter operator definitions DRY between schema and query types
FilterSchema.opandFilterType["op"]now mirror the new operator set, but they duplicate the string literals already represented byFilterOperatorinapps/api/src/query/types.ts. To reduce risk of drift on future changes, consider importing and reusing a shared type (or deriving the union from a single source of truth) instead of repeating the literals here.Also applies to: 82-86
apps/dashboard/app/(main)/websites/[id]/audience/page.tsx (1)
37-45: Remove unusedwebsiteDataprop.The
websiteDataprop is passed toWebsiteAudienceTabbut the component definition (audience-tab.tsx lines 65-72) doesn't accept it. The component only destructures:websiteId,dateRange,isRefreshing,setIsRefreshing,filters, andaddFilter. SincewebsiteIdis already provided,websiteDatais unnecessary and should be removed from the props being passed.apps/dashboard/app/(main)/organizations/components/organization-provider.tsx (1)
103-103:InviteMemberDialogcannot be opened in this component.The
showInviteMemberDialogstate is defined and the dialog is rendered with proper props binding, but there is no code path that ever callssetShowInviteMemberDialog(true). Unlike theCreateOrganizationDialogwhich has a clear trigger, theInviteMemberDialogremains unreachable. Either add a button or action to open it, or remove it if not yet implemented.apps/dashboard/app/(main)/websites/[id]/_components/tabs/audience-tab.tsx (1)
418-439: Minor: Inconsistent indentation in skeleton rendering.The closing
</div>tags at lines 436-439 appear to have inconsistent indentation compared to their opening tags, which may affect readability.apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-history-sheet.tsx (1)
148-165: Consider using a cleaner download approach.The export handler manually creates and removes anchor elements. This works but could be simplified.
const handleExportChat = async (websiteId: string, websiteName?: string) => { try { const exportData = await chatDB.exportChat(websiteId); const blob = new Blob([JSON.stringify(exportData, null, 2)], { type: "application/json", }); const url = URL.createObjectURL(blob); const a = document.createElement("a"); a.href = url; a.download = `chat-${websiteName || websiteId}-${new Date().toISOString().split("T")[0]}.json`; - document.body.appendChild(a); a.click(); - document.body.removeChild(a); URL.revokeObjectURL(url); } catch (error) { console.error("Failed to export chat:", error); } };Modern browsers don't require appending the anchor to the DOM before clicking it.
apps/api/src/query/index.ts (1)
16-27: Avoid duplicated operator literals between schema and types.The added operators look correct, but the
opliterals are now hard‑coded here and (viaFilterOperators) elsewhere. Consider centralizing them in a sharedconst(e.g.,const FILTER_OPS = [...] as const) and reusing that both forFilterOperatorsand thisz.enumto prevent future drift.apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (2)
16-22: Remove unused Table imports.These Table components are imported but no longer used after the refactor to div-based layout.
-import { - Table, - TableBody, - TableCell, - TableHead, - TableHeader, - TableRow, -} from "@/components/ui/table";
178-189: Dead code: This condition can never be true.If
sortedBreakdown.length === 0, the early return at line 92-100 would have already rendered theEmptyStatecomponent and exited. This block is unreachable.Remove the dead code:
- {sortedBreakdown.length === 0 && ( - <div className="py-12 text-center"> - <TableIcon - className="mx-auto mb-4 size-12 text-muted-foreground" - weight="duotone" - /> - <h3 className="font-semibold text-lg text-foreground">No Event Data</h3> - <p className="mt-1 text-muted-foreground text-sm"> - No events found for the selected period - </p> - </div> - )}apps/api/src/query/builders/errors.ts (1)
152-226: GuarderrorRateagainst division by zero inerror_summary.
error_summarycomputes:ROUND((err.error_count / ts.total) * 100, 2) as errorRateIf
total_sessions(ts.total) is 0 for the selected window, this will divide by zero and likely fail the query or emit infinities/NaN.Consider wrapping the expression with a conditional:
- ROUND((err.error_count / ts.total) * 100, 2) as errorRate + ROUND(if(ts.total = 0, 0, (err.error_count / ts.total) * 100), 2) as errorRateThis keeps the shape of the result stable and avoids runtime errors on low‑traffic or newly onboarded sites.
♻️ Duplicate comments (40)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx (1)
963-963: Redundant Tailwind classlg:grid-cols-1.
lg:grid-cols-1is redundant sincegrid-cols-1already applies to all breakpoints until overridden.-<div className="grid grid-cols-1 gap-4 lg:grid-cols-1 xl:grid-cols-2 2xl:grid-cols-3"> +<div className="grid grid-cols-1 gap-4 xl:grid-cols-2 2xl:grid-cols-3">apps/dashboard/app/(main)/layout.tsx (2)
3-7: Confirm auth strategy for(main)and avoid relying on implicitReactnamespaceTwo things to double‑check here:
- This layout now renders
childrenunconditionally with no guard. If(main)is meant to stay protected, please confirm there is middleware or a higher‑level layout enforcing auth; otherwise, this effectively exposes the dashboard publicly. If it’s intentionally public, a short comment noting where auth is enforced would help future readers.- You’re using
React.ReactNodein the prop type but don’t importReactorReactNode. Depending on your TS/JSX config this can fail type‑checking. Safer to import the type explicitly and drop theReact.namespace:-import { Sidebar } from "@/components/layout/sidebar"; - -export default function MainLayout({ - children, -}: { - children: React.ReactNode; -}) { +import { Sidebar } from "@/components/layout/sidebar"; +import type { ReactNode } from "react"; + +export default function MainLayout({ + children, +}: { + children: ReactNode; +}) {
9-16: Reduce nestedh-screen/overflow to avoid double scroll and mobile vh quirksYou currently have three stacked
h-screencontainers withoverflow-hiddenon the outer andoverflow-y-autoon the inner. This tends to cause awkward scrolling (especially on mobile Safari) and makes layout debugging harder. You can keep the same visual layout with a single full‑height shell and one scroll area, e.g.:- <div className="h-screen overflow-hidden text-foreground"> - <Sidebar /> - <div className="relative h-screen pl-0 md:pl-76 lg:pl-84"> - <div className="h-screen overflow-y-auto overflow-x-hidden pt-16 md:pt-0"> - {children} - </div> - </div> - </div> + <div className="min-h-screen text-foreground"> + <Sidebar /> + <div className="relative pl-0 md:pl-76 lg:pl-84"> + <div className="h-screen overflow-y-auto overflow-x-hidden pt-16 md:pt-0"> + {children} + </div> + </div> + </div>This keeps the sidebar/content padding but avoids nested viewport heights and competing overflow declarations.
apps/dashboard/app/(auth)/login/forgot/page.tsx (1)
68-68: Button background override may bypass variant styling.The
bg-primaryclassName override on the Button component may conflict with the Button's built-in variant system for hover, focus, and disabled states. Consider using the Button's variant API instead of direct className overrides.apps/dashboard/app/(auth)/layout.tsx (2)
35-36: Fix Tailwind important modifier syntax and verify Button variant.Two issues:
- The
px-0!syntax is incorrect - the!should come before the utility:!px-0- The variant change to "ghost" is appropriate for this secondary navigation action
Apply this diff to fix the syntax:
- className="px-0! text-white/50 hover:bg-transparent hover:text-white/80" + className="!px-0 text-white/50 hover:bg-transparent hover:text-white/80"
38-38: Missinggroupclass for icon hover effect.The icon uses
group-hover:translate-x-[-4px]but the wrapping Button (line 34-40) lacks thegroupclass, so the hover translation won't trigger.Add
groupto the Button's className:<Button - className="!px-0 text-white/50 hover:bg-transparent hover:text-white/80" + className="group !px-0 text-white/50 hover:bg-transparent hover:text-white/80" variant="ghost"apps/dashboard/app/(auth)/login/page.tsx (2)
157-180: Nested interactive elements issue already flagged.This nested
ButtoninsideLinkcreates invalid HTML and accessibility problems, as detailed in the previous review comment. The suggested fixes (usingasChildpattern orrouter.push) should be applied.
265-280: Text wrapping issue already flagged.The use of
text-nowraponflex-1elements can cause horizontal overflow on narrow viewports, as noted in the previous review comment. Consider removingtext-nowrapor making it responsive withlg:text-nowrap.apps/dashboard/app/(main)/websites/[id]/_components/utils/technology-helpers.tsx (1)
197-207: Confirm custom gradient and accent utility classes are defined and themed correctlyThe updated
getColorClassbranches now rely on custom utilities likegreen-angled-rectangle-gradient,blue-angled-rectangle-gradient,amber-angled-rectangle-gradient,badge-angled-rectangle-gradient, andbg-accent-brighter, plus new border colors.Please double-check that:
- These classes are actually defined in your Tailwind config / global CSS and are included in the compiled dashboard bundle.
- Their colors follow the same token/contrast strategy as the existing
text-*/dark:text-*usage so badges remain legible in both light and dark themes.If design has already validated these utilities, no code change is needed. If more components adopt these variants, consider centralizing the class strings into a small map (e.g., variant → className) to avoid repeating long literals.
apps/dashboard/app/(main)/billing/components/usage-row.tsx (1)
80-84: Restore icon margin on “Bonus” badge for consistency with other badgesThe
GiftIconin the “Bonus” badge is now the only badge icon without a right margin, whileWarningIconandLightningIconboth useclassName="mr-1". This will likely make the “Bonus” label appear tighter than “Overage” / “Unlimited”.Consider restoring the margin for visual consistency:
- {feature.hasExtraCredits && ( - <Badge variant="secondary"> - <GiftIcon size={10} weight="fill" /> - Bonus - </Badge> - )} + {feature.hasExtraCredits && ( + <Badge variant="secondary"> + <GiftIcon className="mr-1" size={10} weight="fill" /> + Bonus + </Badge> + )}apps/dashboard/app/(main)/settings/layout.tsx (1)
1-6: ImportReactNodeinstead of usingReact.ReactNodewithout a React import
SettingsLayoutPropsreferencesReact.ReactNode, but this file doesn’t importReactorReactNode. In this codebase, other layouts typically import theReactNodetype directly, and relying on a globalReactnamespace can cause type errors.Align it with the prevailing pattern:
+import type { ReactNode } from "react"; + type SettingsLayoutProps = { - children: React.ReactNode; + children: ReactNode; }; export default function SettingsLayout({ children }: SettingsLayoutProps) { return <div className="h-full overflow-y-auto">{children}</div>; }apps/api/src/query/types.ts (1)
13-21: Document howcontainsvsstarts_withdiffer despite both mapping toLIKE
contains,not_contains, andstarts_withall map to SQLLIKE/NOT LIKE, so the semantic difference is entirely in how values are formatted (e.g.,%value%vsvalue%). A brief comment here would make this clearer and keep it in sync with the builder logic that applies the wildcards.Example:
export const FilterOperators = { eq: "=", ne: "!=", + // contains/not_contains use %value%, starts_with uses value% contains: "LIKE", not_contains: "NOT LIKE", starts_with: "LIKE", in: "IN", not_in: "NOT IN", } as const;Also double-check that all filter-building code paths format values consistently with this contract.
#!/bin/bash # Spot-check where filter operators are applied to SQL and how LIKE patterns are built. rg -nP "contains|not_contains|starts_with" apps/api/src/query -C4apps/dashboard/app/(main)/settings/_components/chart-preview.tsx (2)
65-83: Extract duplicate gradient definition.The
linearGradientdefinition is duplicated between the "bar" case (lines 65-83) and "area" case (lines 117-135). Extract this into a shared helper function to reduce duplication and maintenance burden.+ const renderGradient = () => ( + <defs> + <linearGradient id={`gradient-${chartId}`} x1="0" x2="0" y1="0" y2="1"> + <stop offset="0%" stopColor="var(--color-primary)" stopOpacity={0.4} /> + <stop offset="100%" stopColor="var(--color-primary)" stopOpacity={0} /> + </linearGradient> + </defs> + );Then replace the duplicated
<defs>blocks with{renderGradient()}in both cases.Also applies to: 117-135
155-169: Inconsistent styling for default/"composed" case.The default case renders a
BarChartwith solid fill (fill="var(--color-primary)"), while the explicit "bar" case uses a gradient fill. Since "composed" is a validchartTypevalue, users passing it will get visually different output than "bar". Either implement a distinct "composed" visualization or align the styling.apps/dashboard/app/(main)/organizations/settings/danger/transfer-assets.tsx (1)
171-171: Inconsistent vertical spacing between website lists.Personal websites list uses
space-y-2(line 171) while Organization websites list usesspace-y-1(line 227). This creates visual asymmetry between the two symmetric sections.- <div className="flex-1 space-y-1 overflow-y-auto"> + <div className="flex-1 space-y-2 overflow-y-auto">Also applies to: 227-227
apps/dashboard/app/(main)/organizations/components/organizations-list.tsx (1)
169-183: Consider movingCreateOrganizationDialogoutsideRightSidebar.The dialog is rendered as a child of
RightSidebar. While most dialog implementations use portals and should work correctly, placing it as a sibling afterRightSidebarprovides cleaner component hierarchy and avoids potential z-index or stacking context issues.<RightSidebar className="gap-4 p-5"> <Button className="w-full" onClick={() => setShowCreateOrganizationDialog(true)} > <PlusIcon size={16} /> New Organization </Button> - <CreateOrganizationDialog - isOpen={showCreateOrganizationDialog} - onClose={() => setShowCreateOrganizationDialog(false)} - /> <RightSidebar.DocsLink /> <RightSidebar.Tip description="Click on an organization to switch to it. The active organization is used across the dashboard." /> </RightSidebar> + <CreateOrganizationDialog + isOpen={showCreateOrganizationDialog} + onClose={() => setShowCreateOrganizationDialog(false)} + /> </div>apps/dashboard/app/(main)/settings/account/sections/two-factor-dialog.tsx (2)
231-242: Add error handling for clipboard operations.
navigator.clipboard.writeTextcan reject if permissions are denied or the API is unavailable. Without a try/catch, the promise rejection goes unhandled and the user gets no feedback.const handleCopyBackupCodes = useCallback(async () => { - const codesText = backupCodes.join("\n"); - await navigator.clipboard.writeText(codesText); - setCopiedBackup(true); - toast.success("Backup codes copied to clipboard"); - setTimeout(() => setCopiedBackup(false), 2000); + try { + const codesText = backupCodes.join("\n"); + await navigator.clipboard.writeText(codesText); + setCopiedBackup(true); + toast.success("Backup codes copied to clipboard"); + setTimeout(() => setCopiedBackup(false), 2000); + } catch { + toast.error("Failed to copy to clipboard"); + } }, [backupCodes]); const handleCopySecret = useCallback(async () => { - await navigator.clipboard.writeText(secret); - toast.success("Secret key copied to clipboard"); + try { + await navigator.clipboard.writeText(secret); + toast.success("Secret key copied to clipboard"); + } catch { + toast.error("Failed to copy to clipboard"); + } }, [secret]);
474-481: Use backup code as key instead of array index.Since backup codes are unique strings, using the code itself as the key is more semantically correct and avoids potential issues if codes are regenerated during the same render cycle.
-{backupCodes.map((code, i) => ( +{backupCodes.map((code) => ( <code className="rounded bg-background px-2 py-1 text-center font-mono text-sm" - key={i} + key={code} > {code} </code> ))}Also applies to: 545-552
apps/api/src/query/expressions.ts (1)
740-746: The quantile source encoding remains fragile.As noted in the past review, encoding
sourceas"level)(column"(e.g.,"0.50)(metric_value") to produce correct SQL is non-obvious and error-prone. The typedagg.quantile()andagg.quantileIf()methods (lines 164-166) provide a safer API, butcompileAggregatestill relies on this fragile convention.Consider adding validation to detect malformed quantile sources, or document this convention clearly if it must remain.
apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx (2)
76-92: Suspense boundaries remain ineffective.This issue was previously flagged: both
ConsumptionChartandUsageBreakdownTablereceiveisLoadingprops and handle loading states internally. Since data fetching happens in the parent viauseQuery(lines 47-56), these Suspense boundaries won't trigger their fallbacks.
97-97: Remove trailing whitespace.Line 97 contains trailing whitespace, which was previously flagged but remains unaddressed.
apps/api/src/query/simple-builder.ts (1)
62-62: **Fix ClickHousequeryString()function parameter: useurlinstead ofpath.**ClickHouse'squeryString()function signature isqueryString(url)— the parameter should be a URL, notpath. ThequeryString()function takes a URL string as input and returns the query parameters as a clean string.If the
pathcolumn contains full URLs (including protocol), this may work, but the parameter name is semantically incorrect. Ifpathonly contains the path portion (e.g.,/page/sub?foo=bar), this will still extract query strings correctly since ClickHouse's URL functions are maximally simplified.Verify what the
pathcolumn contains in your analytics tables:#!/bin/bash # Check what data the path column typically contains ast-grep --pattern 'path' rg -n "path" apps/api/src/types/tables.ts -A2 -B2apps/api/src/query/builders/custom-events.ts (1)
251-295: Missing LIMIT clause could return unbounded results.Unlike
custom_events_by_pathwhich has a default limit of 50, this builder has no LIMIT clause. For long date ranges, this could return a large number of rows (one per day), potentially causing performance issues.Apply this diff to add a limit:
custom_events_trends: { customSql: ( websiteId: string, startDate: string, endDate: string, _filters?: Filter[], _granularity?: TimeUnit, _limit?: number, _offset?: number, _timezone?: string, filterConditions?: string[], filterParams?: Record<string, Filter["value"]> ) => { + const limit = _limit ?? 1000; const combinedWhereClause = filterConditions?.length ? `AND ${filterConditions.join(" AND ")}` : ""; return { sql: ` SELECT toDate(timestamp) as date, COUNT(*) as total_events, COUNT(DISTINCT event_name) as unique_event_types, COUNT(DISTINCT anonymous_id) as unique_users FROM ${Analytics.custom_event_spans} WHERE client_id = {websiteId:String} AND timestamp >= toDateTime({startDate:String}) AND timestamp <= toDateTime(concat({endDate:String}, ' 23:59:59')) AND event_name != '' ${combinedWhereClause} GROUP BY toDate(timestamp) ORDER BY date ASC + LIMIT {limit:UInt32} `, params: { websiteId, startDate, endDate, + limit, ...filterParams, }, }; },apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (1)
113-113: Double border between items.The parent container uses
divide-y divide-border(line 113) which adds borders between children, but each item also hasborder-b border-border(line 141). This creates duplicate bottom borders.Remove the redundant border from items:
<div key={item.event_category} - className="group flex items-center gap-4 border-b border-border px-4 py-3 transition-colors hover:bg-accent/50 sm:px-6 sm:py-4" + className="group flex items-center gap-4 px-4 py-3 transition-colors hover:bg-accent/50 sm:px-6 sm:py-4" >Also applies to: 141-141
apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx (1)
312-316: Bug:subtitleprop does not exist onNoticeBanner.Based on the
NoticeBannercomponent definition (from relevant code snippets atapps/dashboard/app/(main)/websites/_components/notice-banner.tsx), it acceptsdescription, notsubtitle. The status text will not render.Apply this diff to fix the prop name:
<NoticeBanner className="w-full" icon={ isSetup ? ( <CheckIcon className="size-4" weight="duotone" /> ) : ( <WarningCircleIcon className="size-4" weight="duotone" /> ) } - subtitle={ + description={ isSetup ? "Data is being collected successfully" : "Install the tracking script to start collecting data" } title={isSetup ? "Tracking Active" : "Tracking Not Setup"} >apps/dashboard/app/(main)/settings/appearance/page.tsx (3)
126-157: Addaria-pressedfor accessibility on theme buttons.The theme selection buttons lack
aria-pressed, so screen readers won't communicate which theme is currently selected.<button className={cn( "flex flex-col items-center gap-2 rounded border p-4 transition-colors", isActive ? "border-primary bg-primary/5" : "border-border hover:bg-accent" )} key={id} onClick={() => setTheme(id)} type="button" + aria-pressed={isActive} >
94-106: Potential hydration mismatch withuseTheme().The
themevalue fromnext-themesisundefinedduring SSR and initial client render until the component mounts. This causes hydration mismatches when renderingthemedirectly (line 400) or comparing it in theme button active states (line 124).Add a mounted state check to guard theme-dependent rendering:
-import { useState } from "react"; +import { useEffect, useState } from "react"; ... export default function AppearanceSettingsPage() { const { theme, setTheme } = useTheme(); + const [mounted, setMounted] = useState(false); const { preferences, updateLocationPreferences, updateAllPreferences } = useAllChartPreferences(); + + useEffect(() => { + setMounted(true); + }, []);Then guard theme comparisons with
mounted &&or render a placeholder until mounted.
308-386: Invalid nesting of interactive elements inside<button>.Placing
<Select>components inside a<button>element creates invalid HTML. Per the HTML spec, buttons cannot contain interactive content. This causes accessibility issues—screen readers may not properly announce the nested controls, and keyboard navigation may behave unexpectedly.Restructure to use a non-interactive row container with a separate clickable region for the location selection:
-<button +<div className={cn( - "grid w-full grid-cols-[1fr_5.5rem_7.5rem] items-center gap-3 px-4 py-2.5 text-left transition-colors", + "grid w-full grid-cols-[1fr_5.5rem_7.5rem] items-center gap-3 px-4 py-2.5 transition-colors", i < CHART_LOCATIONS.length - 1 && "border-b", isActive && "bg-accent/50" )} key={location} - onClick={() => setPreviewLocation(location)} - type="button" > - <div className="flex items-center gap-2"> + <button + className="flex items-center gap-2 text-left" + onClick={() => setPreviewLocation(location)} + type="button" + > <Icon ... /> <span ...> {CHART_LOCATION_LABELS[location]} </span> - </div> + </button> <Select ...> - <SelectTrigger onClick={(e) => e.stopPropagation()} ...> + <SelectTrigger ...> ... </Select> ... -</button> +</div>apps/api/src/query/builders/vitals.ts (1)
72-111: Unused parameters_filtersand_granularityin vitals_by_page.The parameters
_filtersand_granularityare declared but never used in the query. Sincecustomizable: true, these should either be implemented to support dynamic filtering or removed if not needed.Apply this diff to remove unused parameters:
vitals_by_page: { customSql: ( websiteId: string, startDate: string, endDate: string, - _filters?, - _granularity?, _limit?: number ) => {Or implement filter/granularity support to justify
customizable: true.apps/dashboard/app/(main)/settings/account/page.tsx (4)
56-63: Handle edge case where name could be empty string.If
nameis an empty string,split(" ")returns[""], and accessingn[0]on an empty string returnsundefined, causingundefined.toUpperCase()to throw.Apply this diff to handle empty strings:
function getInitials(name: string): string { + if (!name.trim()) return "?"; return name .split(" ") + .filter(Boolean) .map((n) => n[0]) .join("") .toUpperCase() .slice(0, 2); }
77-102: Add minimum password length validation for consistency.The
ChangePasswordDialogonly validates that passwords match but doesn't enforce minimum length, while theTwoFactorDialogenforces an 8-character minimum. This inconsistency could allow weak passwords.Apply this diff:
const changePasswordMutation = useMutation({ mutationFn: async () => { if (newPassword !== confirmPassword) { throw new Error("Passwords do not match"); } + if (newPassword.length < 8) { + throw new Error("Password must be at least 8 characters"); + } const result = await authClient.changePassword({
227-241: All provider buttons show loading state when any link is pending.When
linkSocial.isPendingis true, all unconnected provider buttons are disabled and would show spinners. Track which provider is being linked to show the spinner only on the relevant button.Track the linking provider:
+const [linkingProvider, setLinkingProvider] = useState<SocialProvider | null>(null); const linkSocial = useMutation({ mutationFn: async (provider: SocialProvider) => { + setLinkingProvider(provider); const result = await authClient.linkSocial({ provider, callbackURL: window.location.href, });Then in the JSX, check
linkingProvider === providerinstead oflinkSocial.isPendingfor the spinner.
589-596: Tip content mentions email but email cannot be changed.The tip advises keeping email up to date, but the email field is disabled (line 351). Consider updating to actionable advice.
<p className="text-muted-foreground text-xs"> - Keep your email up to date to ensure you receive important - notifications about your account. + Enable two-factor authentication and connect multiple login + methods to keep your account secure. </p>apps/api/src/types/tables.ts (1)
1-10: Still missinganalytics.outgoing_linksmapping inTableFieldsMap.
Analyticsexposesoutgoing_links, butTableFieldsMapdoesn’t define a corresponding key, so code using the map can’t get strong typing for that table.You can address this by importing the outgoing‑links row type and wiring it into the map:
import type { AnalyticsEvent, BlockedTraffic, CustomEventSpan, CustomEventsHourlyAggregate, + CustomOutgoingLink, ErrorHourlyAggregate, ErrorSpanRow, WebVitalsHourlyAggregate, WebVitalsSpan, } from "@databuddy/db"; export type TableFieldsMap = { "analytics.events": keyof AnalyticsEvent; "analytics.error_spans": keyof ErrorSpanRow; "analytics.error_hourly": keyof ErrorHourlyAggregate; "analytics.web_vitals_spans": keyof WebVitalsSpan; "analytics.web_vitals_hourly": keyof WebVitalsHourlyAggregate; "analytics.custom_event_spans": keyof CustomEventSpan; "analytics.custom_events_hourly": keyof CustomEventsHourlyAggregate; "analytics.blocked_traffic": keyof BlockedTraffic; + "analytics.outgoing_links": keyof CustomOutgoingLink; };Also applies to: 12-35
apps/api/src/query/builders/performance.ts (2)
216-227: LEFT JOINs combined withWHERE e.* != ''behave as INNER JOINs.In the
web_vitals_by_*builders you use:LEFT JOIN ${Analytics.events} e ON (...) WHERE wv.client_id = ... AND ... AND e.browser_name != '' -- / e.country / e.os_name / e.regionThe
WHERE e.* != ''predicate null‑filters the right side, so these are effectivelyINNER JOINs and will drop vitals rows without matching events. If that’s intentional, it’s clearer to declareINNER JOIN; if you meant to preserve unmatched vitals, move the non‑empty checks into theJOINcondition or useCOALESCEwith a default label.This mirrors an earlier nitpick on the same pattern in this file.
Also applies to: 262-273, 309-320, 355-366
205-205: Avoid redundantany()on columns already inGROUP BY.You’re selecting:
any(e.browser_name) as name any(e.country) as name any(e.os_name) as name CONCAT(any(e.region), ', ', any(e.country)) as namewhile also grouping by those same columns. The
any()wrappers aren’t needed and can be simplified to the grouped columns directly:- any(e.browser_name) as name, + e.browser_name as name, - any(e.country) as name, + e.country as name, - any(e.os_name) as name, + e.os_name as name, - CONCAT(any(e.region), ', ', any(e.country)) as name, + CONCAT(e.region, ', ', e.country) as name,This keeps the intent clearer without changing semantics.
Also applies to: 251-251, 298-298, 344-344
apps/dashboard/app/(main)/websites/[id]/errors/_components/error-detail-modal.tsx (4)
52-82: LocalCopyButtonstill duplicates existing docs CopyButtonYou’re re‑implementing a
CopyButtonhere while there’s already one inapps/docs/components/docs/copy-button.tsx. If the divergence is intentional (e.g., section-aware copy state), consider either extracting a shared primitive (with variant props for analytics/docs) or adding a short comment to explain why this version is local to avoid future drift.
110-121: Clear copy timeout on unmount to avoid state updates after unmountThe
setTimeoutclearingcopiedSectionmay fire after the modal unmounts, causing a state update on an unmounted component and a small memory leak. Track the timeout in a ref and clear it on unmount and before scheduling a new one.-import { useState } from "react"; +import { useEffect, useRef, useState } from "react"; @@ export const ErrorDetailModal = ({ error, isOpen, onClose, }: ErrorDetailModalProps) => { const [copiedSection, setCopiedSection] = useState<CopiedSection>(null); + + const copyTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + return () => { + if (copyTimeoutRef.current) { + clearTimeout(copyTimeoutRef.current); + } + }; + }, []); @@ const copyToClipboard = async (text: string, section: CopiedSection) => { try { await navigator.clipboard.writeText(text); setCopiedSection(section); toast.success("Copied to clipboard"); - setTimeout(() => setCopiedSection(null), 2000); + if (copyTimeoutRef.current) { + clearTimeout(copyTimeoutRef.current); + } + copyTimeoutRef.current = setTimeout(() => { + setCopiedSection(null); + copyTimeoutRef.current = null; + }, 2000); } catch (err) {
149-205: Normalize indentation ofquickActionsconditional blocksThe
ifblocks buildingquickActionsare flush-left while surrounding code is indented, which hurts readability and looks like a formatting glitch. Align them with the rest of the function body:-If (error.path) { - quickActions.push({ + if (error.path) { + quickActions.push({ key: "copy-url", // ... - }); -} + }); + } @@ -If (isAbsoluteUrl) { - quickActions.push({ + if (isAbsoluteUrl) { + quickActions.push({ key: "open-page", // ... - }); -} + }); + } @@ -If (error.session_id) { - quickActions.push({ + if (error.session_id) { + quickActions.push({ key: "copy-session", // ... - }); -} + }); + } @@ -If (error.stack) { - quickActions.push({ + if (error.stack) { + quickActions.push({ key: "copy-stack", // ... - }); -} + }); + }
207-232: TightencontextRowstyping to removeas CopiedSectionand ensuretextis always a string
section={row.copySection as CopiedSection}bypasses type safety, andcopyValuecan be inferred asstring | undefinedwhileCopyButton.textisstring. You can makecontextRowsfully typed and remove the cast while guaranteeingtextis always a string:- const contextRows = [ + const contextRows = [ { key: "url", label: "Page URL", value: error.path || "—", icon: <LinkIcon className="h-4 w-4 shrink-0 text-muted-foreground" weight="duotone" />, - copySection: error.path ? "url" : null, - copyValue: error.path, + copySection: error.path ? ("url" as const) : null, + copyValue: error.path ?? "", }, { key: "session", label: "Session ID", value: error.session_id || "—", icon: <HashIcon className="h-4 w-4 shrink-0 text-muted-foreground" weight="duotone" />, - copySection: error.session_id ? "session" : null, - copyValue: error.session_id, + copySection: error.session_id ? ("session" as const) : null, + copyValue: error.session_id ?? "", }, { key: "user", label: "User ID", value: error.anonymous_id || "—", icon: <UserIcon className="h-4 w-4 shrink-0 text-muted-foreground" weight="duotone" />, - copySection: error.anonymous_id ? "user" : null, - copyValue: error.anonymous_id, + copySection: error.anonymous_id ? ("user" as const) : null, + copyValue: error.anonymous_id ?? "", }, - ]; + ] satisfies { + key: string; + label: string; + value: string; + icon: ReactNode; + copySection: CopiedSection; + copyValue: string; + }[]; @@ - {row.copySection && row.copyValue && ( + {row.copySection && row.copyValue && ( <CopyButton ariaLabel={`Copy ${row.label}`} copiedSection={copiedSection} onCopy={copyToClipboard} - section={row.copySection as CopiedSection} + section={row.copySection} text={row.copyValue} /> )}This keeps runtime behavior the same but removes the need for assertions and aligns types with
CopyButton.Also applies to: 396-403
| max_backup_bandwidth = 104857600, | ||
| s3_use_virtual_addressing = false${baseBackupSetting} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making max_backup_bandwidth configurable.
The hardcoded 100 MiB/s (104857600 bytes) bandwidth limit is reasonable, but different environments may have varying network constraints. Consider exposing this as an optional environment variable with this as the default.
+// At top of file or in env schema
+const BACKUP_BANDWIDTH = env.BACKUP_BANDWIDTH ?? 104857600; // 100 MiB/s default
+
// In backupQuery settings
- max_backup_bandwidth = 104857600,
+ max_backup_bandwidth = ${BACKUP_BANDWIDTH},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/lib/clickhouse-backup.ts around lines 202-203 the backup config
hardcodes max_backup_bandwidth = 104857600; make this value configurable via an
environment variable (e.g. MAX_BACKUP_BANDWIDTH) while preserving 104857600 as
the default. Change the config generation to read
process.env.MAX_BACKUP_BANDWIDTH, parse it to an integer with a safe fallback to
104857600 and a simple validation (positive integer), and then inject that
computed value into the generated config instead of the literal. Also update any
environment typing/schema (if present) and add a comment documenting the env var
and units (bytes/sec).
|
|
||
| SELECT | ||
| ce.id, | ||
| generateUUIDv4() as id, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full context around line 161 in sessions.ts
cd apps/api/src/query/builders && wc -l sessions.tsRepository: databuddy-analytics/Databuddy
Length of output: 88
🏁 Script executed:
# Check the context around line 161 to understand the custom_event_spans query
sed -n '150,175p' apps/api/src/query/builders/sessions.tsRepository: databuddy-analytics/Databuddy
Length of output: 862
🏁 Script executed:
# Search for event ID usage patterns - looking for references to event.id, events[].id, etc.
rg -nP --type=ts --type=tsx '\b(event.*\.id|events\[.*\]\.id|evt\.id|ce\.id)\b' -C3 apps/Repository: databuddy-analytics/Databuddy
Length of output: 100
🏁 Script executed:
# Search for generateUUIDv4 usage to understand the intent behind the change
rg -nP --type=ts --type=tsx 'generateUUIDv4' -C2 apps/Repository: databuddy-analytics/Databuddy
Length of output: 100
🏁 Script executed:
# Search for custom_event_spans references to understand the table schema
rg -nP --type=ts --type=tsx 'custom_event_spans' -C3 apps/Repository: databuddy-analytics/Databuddy
Length of output: 100
🏁 Script executed:
# Search for event ID usage patterns without tsx flag
rg -nP --type=ts 'event.*\.id|events\[.*\]\.id|evt\.id|ce\.id' -C3 apps/Repository: databuddy-analytics/Databuddy
Length of output: 2497
🏁 Script executed:
# Search for generateUUIDv4 usage to understand intent
rg -nP --type=ts 'generateUUIDv4' -C2 apps/Repository: databuddy-analytics/Databuddy
Length of output: 397
🏁 Script executed:
# Look for custom_event_spans references
rg -nP --type=ts 'custom_event_spans' -C3 apps/Repository: databuddy-analytics/Databuddy
Length of output: 4304
🏁 Script executed:
# Search for any frontend/client code that might reference event IDs
rg -nP --type=ts 'event.*id|eventId|event_id' -C2 apps/ | head -100Repository: databuddy-analytics/Databuddy
Length of output: 9767
🏁 Script executed:
# Check if there are any tests or documentation about event ID stability
rg -nP --type=ts '(track.*event|dedup|referential|event.*id|stable.*id)' -C2 apps/ | head -100Repository: databuddy-analytics/Databuddy
Length of output: 6058
🏁 Script executed:
# Search for CustomEventSpan type definition
rg -nP --type=ts 'CustomEventSpan|interface.*CustomEvent' -A 10 apps/Repository: databuddy-analytics/Databuddy
Length of output: 8398
🏁 Script executed:
# Look for where custom_event_spans is populated/inserted
rg -nP --type=ts '(INSERT INTO|custom_event_spans|custom events)' apps/ -C 3Repository: databuddy-analytics/Databuddy
Length of output: 6758
🏁 Script executed:
# Check if ce.id exists or needs to be generated
grep -r "ce\.id\|custom_event.*\.id" apps/ --include="*.ts" -C 3Repository: databuddy-analytics/Databuddy
Length of output: 55
🏁 Script executed:
# Look for the original query to see what ce.id was before
rg -nP --type=ts 'session_events|SELECT.*FROM.*custom_event' -B 5 -A 15 apps/api/src/query/builders/Repository: databuddy-analytics/Databuddy
Length of output: 6923
🏁 Script executed:
# Find the @databuddy/db package definition for CustomEventSpan
fd -t f -e ts 'CustomEventSpan|types' packages/db/src/ 2>/dev/null | head -20Repository: databuddy-analytics/Databuddy
Length of output: 55
🏁 Script executed:
# Search for CustomEventSpan type definition
rg -nP 'export.*CustomEventSpan|interface CustomEventSpan|type CustomEventSpan' -A 15 packages/Repository: databuddy-analytics/Databuddy
Length of output: 3437
🏁 Script executed:
# Look for database schema or migration files
fd -t f -e sql -o -e ts 'schema|migration' packages/db/ 2>/dev/null | head -20Repository: databuddy-analytics/Databuddy
Length of output: 55
🏁 Script executed:
# Check if there's an id field in the database schema
rg -nP 'custom_event|id.*String|UUID' packages/db/ -A 3 -B 1 | head -100Repository: databuddy-analytics/Databuddy
Length of output: 6603
🏁 Script executed:
# Get the full custom_event_spans table schema definition
sed -n '361,383p' packages/db/src/clickhouse/schema.tsRepository: databuddy-analytics/Databuddy
Length of output: 832
Remove generateUUIDv4() and use a stable event identifier instead.
The custom_event_spans table has no id column, so generateUUIDv4() as id generates a new UUID on each query execution. This means the same custom event receives a different ID every time it's queried, breaking any expectation of stable event identifiers. The frontend and deduplication logic in security.ts rely on consistent event IDs. Either store the client-provided eventId from the intake payload or generate IDs deterministically to match the regular events table behavior.
🤖 Prompt for AI Agents
In apps/api/src/query/builders/sessions.ts around line 161, remove the call to
generateUUIDv4() as id because the custom_event_spans table has no id column and
generating a new UUID per query breaks stable identifiers; instead, return the
client-provided eventId field from the intake payload (or compute a
deterministic ID the same way the regular events table does) so the query emits
a stable event id used by the frontend and deduplication logic—update the SELECT
to use the existing eventId column (or the deterministic generator) and remove
generateUUIDv4().
| <Loader2 className="h-4 w-4 animate-spin" /> | ||
| Sending reset link... |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Restore spacing between loading icon and text.
The Loader2 icon has no margin separating it from the text "Sending reset link...", which may reduce readability. Consider adding mr-2 to the icon classes for consistent visual spacing.
Apply this diff to restore the spacing:
- <Loader2 className="h-4 w-4 animate-spin" />
+ <Loader2 className="mr-2 h-4 w-4 animate-spin" />📝 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.
| <Loader2 className="h-4 w-4 animate-spin" /> | |
| Sending reset link... | |
| <Loader2 className="mr-2 h-4 w-4 animate-spin" /> | |
| Sending reset link... |
🤖 Prompt for AI Agents
In apps/dashboard/app/(auth)/login/forgot/page.tsx around lines 74-75, the
Loader2 icon is missing spacing from the adjacent "Sending reset link..." text;
update the icon's className to include spacing (e.g., add "mr-2") so there is
consistent margin between the spinner and the text for improved readability.
| function getFieldLabel(field: string): string { | ||
| return filterOptions.find((o) => o.value === field)?.label ?? field; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate helper function across filter components.
getFieldLabel is duplicated in saved-filters-menu.tsx, filters-section.tsx, and save-filter-dialog.tsx. Consider extracting this to a shared utility (e.g., in @/hooks/use-filters alongside getOperatorLabel).
-function getFieldLabel(field: string): string {
- return filterOptions.find((o) => o.value === field)?.label ?? field;
-}
+import { getFieldLabel } from "@/hooks/use-filters";Then add to apps/dashboard/hooks/use-filters.ts:
export function getFieldLabel(field: string): string {
return filterOptions.find((o) => o.value === field)?.label ?? field;
}🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx
around lines 35-37, the getFieldLabel helper is duplicated across multiple
filter components; extract it into the shared hooks file
apps/dashboard/hooks/use-filters.ts and replace the local implementation with an
import. Specifically, add the exported getFieldLabel function to use-filters.ts
(same logic as currently in this file), update saved-filters-menu.tsx to import
{ getFieldLabel } from '@/hooks/use-filters', and remove the in-file duplicate;
repeat the same replacement in filters-section.tsx and save-filter-dialog.tsx so
all components use the shared utility.
| <div className="flex items-center justify-center rounded border bg-sidebar py-12"> | ||
| <div className="text-center"> | ||
| <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-primary border-b-2" /> | ||
| <p className="text-sidebar-foreground/70 text-sm"> | ||
| Loading performance data... | ||
| </p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Incomplete spinner border styling.
The spinner element has border-b-2 for the animated portion but only border-primary for the rest, which doesn't specify border width. This may result in an invisible or inconsistent spinner appearance.
- <div className="flex items-center justify-center rounded border bg-sidebar py-12">
- <div className="text-center">
- <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-primary border-b-2" />
+ <div className="flex items-center justify-center rounded border bg-sidebar py-12">
+ <div className="text-center">
+ <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-2 border-primary/30 border-t-primary" />📝 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.
| <div className="flex items-center justify-center rounded border bg-sidebar py-12"> | |
| <div className="text-center"> | |
| <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-primary border-b-2" /> | |
| <p className="text-sidebar-foreground/70 text-sm"> | |
| Loading performance data... | |
| </p> | |
| </div> | |
| </div> | |
| <div className="flex items-center justify-center rounded border bg-sidebar py-12"> | |
| <div className="text-center"> | |
| <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-2 border-primary/30 border-t-primary" /> | |
| <p className="text-sidebar-foreground/70 text-sm"> | |
| Loading performance data... | |
| </p> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
In apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx
around lines 566 to 573, the spinner uses border-primary but only sets
border-b-2 for width which can make the non-animated borders invisible or
inconsistent; update the spinner markup/classes so all borders have a defined
width (e.g., use border-2) and make the non-animated sides transparent (e.g.,
border-transparent) while keeping the animated segment using border-primary and
the same width (e.g., border-b-2 or border-2 with a differing side color) to
ensure a consistent visible spinner.
| "w-full cursor-pointer rounded border bg-sidebar p-4 text-left transition-all hover:shadow-sm"; | ||
| const activeClasses = | ||
| filter === "fast" | ||
| ? "bg-green-50 ring-1 ring-green-500/20 border-green-500 dark:bg-green-950/20" | ||
| : "bg-red-50 ring-1 ring-red-500/20 border-red-500 dark:bg-red-950/20"; | ||
| const inactiveClasses = "hover:bg-muted/50 hover:border-primary/50"; | ||
| const inactiveClasses = "hover:bg-sidebar/80 hover:border-primary/50"; | ||
| const badgeClasses = |
There was a problem hiding this comment.
Styling updates look good; guard against totalPages === 0 in percentage math
The move to bg-sidebar/text-sidebar-foreground and the adjusted spacing aligns this card with the new theme and doesn’t affect behavior.
However, the percentage calculations:
- Line 150:
percentage={(summary.fastPages / summary.totalPages) * 100} - Line 161:
percentage={(summary.slowPages / summary.totalPages) * 100}
will yield NaN or Infinity when summary.totalPages is 0, which is likely in “no data yet” scenarios and will render as NaN%/Infinity%.
Consider a small guard, for example:
- <FilterButton
+ const fastPercentage =
+ summary.totalPages > 0
+ ? (summary.fastPages / summary.totalPages) * 100
+ : 0;
+ const slowPercentage =
+ summary.totalPages > 0
+ ? (summary.slowPages / summary.totalPages) * 100
+ : 0;
+
+ <FilterButton
activeFilter={activeFilter}
@@
- percentage={(summary.fastPages / summary.totalPages) * 100}
+ percentage={fastPercentage}
@@
- <FilterButton
+ <FilterButton
@@
- percentage={(summary.slowPages / summary.totalPages) * 100}
+ percentage={slowPercentage}This keeps the UI stable when there are no pages to report on.
Also applies to: 71-79, 103-141, 143-163
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance/_components/performance-summary-card.tsx
around lines 48-54 (also review 71-79, 103-141, 143-163), the percentage
calculations use summary.totalPages as the divisor and will produce NaN/Infinity
when totalPages === 0; change each percentage expression to guard against zero
by returning 0 (or another safe default) when summary.totalPages is falsy (e.g.,
totalPages === 0) and otherwise compute (num/totalPages)*100, ensuring the UI
renders a stable 0% instead of NaN/Infinity.
| const fullErrorInfo = `Error: ${error.message} | ||
| ${error.stack ? `\nStack Trace:\n${error.stack}` : ""} | ||
|
|
||
| Context: | ||
| • URL: ${error.path} | ||
| • Session: ${error.session_id || "Unknown"} | ||
| • User: ${error.anonymous_id} | ||
| • Time: ${formatDateTimeSeconds(error.timestamp)} | ||
| • Browser: ${error.browser_name || "Unknown"} | ||
| • OS: ${error.os_name || "Unknown"} | ||
| • Device: ${error.device_type || "Unknown"} | ||
| • Location: ${locationLabel}`; |
There was a problem hiding this comment.
Recheck which fields are included in fullErrorInfo / “Copy All” for PII
fullErrorInfo currently includes URL, session ID, anonymous user ID, timestamp, browser, OS, device, location, and (via Metadata) IP/user agent are visible elsewhere. Copying this whole blob to the clipboard makes it easy to paste PII into tickets or chats.
If you have stricter privacy constraints, consider:
- Excluding or masking IP / user agent from the “Copy All” payload, or
- Making “Copy All” obviously labeled as including user-identifiable metadata so teams know where it’s safe to paste.
Also applies to: 500-511
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/errors/_components/error-detail-modal.tsx
around lines 128-139 (also applies to lines ~500-511), fullErrorInfo currently
includes user-identifiable fields; remove or mask PII by excluding session_id,
anonymous_id, IP address and user-agent (and consider masking query params in
URL), replacing those values with a fixed token like "<redacted>" (or omit the
lines entirely) and ensure the "Copy All" action uses the masked payload;
additionally update the UI to make the "Copy All" button clearly
labeled/tooltiped that it contains user-identifiable metadata if you choose to
keep any non-redacted metadata.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| }: GoalItemProps) { | ||
| const conversionRate = analytics?.overall_conversion_rate ?? 0; | ||
| const totalUsers = analytics?.total_users_entered ?? 0; | ||
| const completions = analytics?.total_users_completed ?? 0; |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| AND timestamp >= toDateTime({startDate:String}) | ||
| AND timestamp <= toDateTime(concat({endDate:String}, ' 23:59:59')) | ||
| AND path != '' | ||
| GROUP BY path |
There was a problem hiding this comment.
Bug: GROUP BY mismatch causes incorrect path aggregation
The web_vitals_by_page query has a mismatch between the SELECT and GROUP BY clauses. The SELECT normalizes the path with decodeURLComponent(CASE WHEN trimRight(path(path), '/') = '' THEN '/' ELSE trimRight(path(path), '/') END) as name, but the GROUP BY uses raw path. This causes paths like /about and /about/ to be grouped separately but display with the same normalized name in the output, resulting in duplicate rows and incorrect metric aggregation. The similar query vitals_by_page in vitals.ts correctly uses GROUP BY page (the alias), which this query should follow.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestion:
The empty state for Web Vitals is shown when data loading fails, instead of only showing it when there's legitimately no data. When an error occurs, users see both the error banner and the "No data yet" message, which is confusing.
View Details
📝 Patch Details
diff --git a/apps/dashboard/app/(main)/websites/[id]/vitals/page.tsx b/apps/dashboard/app/(main)/websites/[id]/vitals/page.tsx
index 61808bc8..5d757014 100644
--- a/apps/dashboard/app/(main)/websites/[id]/vitals/page.tsx
+++ b/apps/dashboard/app/(main)/websites/[id]/vitals/page.tsx
@@ -251,7 +251,7 @@ export default function VitalsPage() {
</div>
)}
- {!isLoading && overviewData.length === 0 && (
+ {!isLoading && !isError && overviewData.length === 0 && (
<div className="rounded border bg-card p-8 text-center">
<HeartbeatIcon
className="mx-auto size-12 text-muted-foreground/40"
Analysis
Empty state shown alongside error banner in Web Vitals page
What fails: The empty state message "No Web Vitals data yet" is displayed at the same time as the error banner from WebsitePageHeader when a query fails, creating a confusing UX with duplicate error messaging.
How to reproduce:
- Navigate to the Web Vitals page for a website (
/websites/[id]/vitals) - Trigger an API failure (network error, server error, or invalid request)
- Observe both the error banner and "No Web Vitals data yet" message displayed simultaneously
What happened: When the query fails, isError becomes true and overviewData is empty (defaults to []). The condition !isLoading && overviewData.length === 0 evaluates to true even during error state, causing the empty state to render alongside the error banner.
Expected: Only the error banner should be displayed when there's an error. The empty state should only appear when loading is complete AND there's no error AND data is genuinely absent.
Root cause: Line 254 of apps/dashboard/app/(main)/websites/[id]/vitals/page.tsx was missing an !isError check in the condition. The correct pattern is already used in other pages like apps/dashboard/app/(main)/websites/page.tsx (line 132) and apps/dashboard/app/(main)/websites/[id]/users/page.tsx.
Fix applied: Added !isError check to the empty state condition: {!isLoading && !isError && overviewData.length === 0 && (
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (1)
173-186: Unreachable code block.This fallback block is never rendered because:
- Line 83 returns early if
!usageData?.eventTypeBreakdown?.lengthsortedBreakdownis derived fromeventTypeBreakdown(line 95), so if we reach this point,sortedBreakdown.lengthwill always be > 0Consider removing this dead code:
- {sortedBreakdown.length === 0 && ( - <div className="py-12 text-center"> - <TableIcon - className="mx-auto mb-4 size-12 text-muted-foreground" - weight="duotone" - /> - <h3 className="font-semibold text-foreground text-lg"> - No Event Data - </h3> - <p className="mt-1 text-muted-foreground text-sm"> - No events found for the selected period - </p> - </div> - )}apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx (1)
205-217: Critical: Double-toggle bug caused by nested click handlers.The card's
onClick={onToggle}(line 207) and the Switch'sonCheckedChange={onToggle}(line 216) both fire when clicking the switch, toggling the state twice and canceling the intended action. Users will perceive the switch as non-functional.Solution: Remove the card's
onClickhandler and rely solely on the Switch:<div - className="cursor-pointer rounded border bg-card p-4 transition-all duration-200 hover:bg-accent-brighter" - onClick={onToggle} + className="rounded border bg-card p-4 transition-all duration-200 hover:bg-accent-brighter" >If you want the entire card to be clickable, add a click handler that stops propagation from the Switch:
<div className="cursor-pointer rounded border bg-card p-4 transition-all duration-200 hover:bg-accent-brighter" onClick={onToggle} > <div className="flex items-start justify-between pb-3"> <div className="min-w-0 flex-1 space-y-1 pr-3"> <div className="text-left font-medium text-sm">{title}</div> <div className="text-left text-muted-foreground text-xs leading-relaxed"> {description} </div> </div> - <Switch checked={isEnabled} onCheckedChange={onToggle} /> + <div onClick={(e) => e.stopPropagation()}> + <Switch checked={isEnabled} onCheckedChange={onToggle} /> + </div> </div>apps/dashboard/app/(main)/organizations/settings/danger/transfer-assets.tsx (1)
25-53: Add ARIA state to expose selection while keeping new visualsThe updated card/selection styling looks good. Since
WebsiteItemrepresents a selectable/toggle state, consider exposing this to assistive tech, e.g.:<button aria-pressed={selected} // or role="option" aria-selected={selected} if used in a listbox pattern >This keeps the new styling while making the selection state accessible.
apps/api/src/lib/clickhouse-backup.ts (2)
218-228: Prefer parameterized queries for UPDATE statement.Similar to the INSERT statement, this UPDATE uses string interpolation. For consistency and defense-in-depth, use parameterized queries.
await clickHouseOG.command({ - query: ` + await clickHouseOG.query({ + query: ` ALTER TABLE internal.databuddy_backups UPDATE status = 'completed', - size_bytes = ${metadata?.size_bytes ?? 0}, - compressed_size_bytes = ${metadata?.compressed_size_bytes ?? 0}, - num_files = ${metadata?.num_files ?? 0} - WHERE backup_name = '${backupName}' + size_bytes = {sizeBytes:UInt64}, + compressed_size_bytes = {compressedSizeBytes:UInt64}, + num_files = {numFiles:UInt64} + WHERE backup_name = {backupName:String} `, - }); + query_params: { + sizeBytes: metadata?.size_bytes ?? 0, + compressedSizeBytes: metadata?.compressed_size_bytes ?? 0, + numFiles: metadata?.num_files ?? 0, + backupName, + }, + });
191-204: S3 credentials in backup queries will be logged in ClickHouse's query log.Lines 191-204 and 410-417 embed S3 credentials directly into BACKUP and RESTORE queries. ClickHouse records all executed queries in
system.query_logby default without masking sensitive information. Anyone with database access can retrieve credentials from the query log. Use parameterized queries where ClickHouse supports them, or implement server-side query masking via ClickHouse configuration (query_masking_rulesin users.xml) to redact credentials before logging.apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx (1)
649-656:today*values are under-counted for hourly granularityUsing
.findonanalytics.events_by_datereturns only the first matching entry for today, which under‑countspageviews,visitors, andsessionswhengranularity === "hourly"(multiple hourly rows per day). The “X today” descriptions will be wrong in that mode.Aggregate across all entries for today instead of taking the first:
- const todayDate = dayjs().format("YYYY-MM-DD"); - const todayEvent = analytics.events_by_date.find( - (event: MetricPoint) => dayjs(event.date).format("YYYY-MM-DD") === todayDate - ); - const todayVisitors = todayEvent?.visitors ?? 0; - const todaySessions = todayEvent?.sessions ?? 0; - const todayPageviews = todayEvent?.pageviews ?? 0; + const todayDate = dayjs().format("YYYY-MM-DD"); + const todayEvents = analytics.events_by_date.filter( + (event: MetricPoint) => dayjs(event.date).format("YYYY-MM-DD") === todayDate + ); + + const todayVisitors = todayEvents.reduce((sum, event) => { + const visitors = + (event.visitors as number | undefined) ?? + (event.unique_visitors as number | undefined) ?? + 0; + return sum + visitors; + }, 0); + + const todaySessions = todayEvents.reduce( + (sum, event) => sum + ((event.sessions as number | undefined) ?? 0), + 0 + ); + + const todayPageviews = todayEvents.reduce( + (sum, event) => sum + ((event.pageviews as number | undefined) ?? 0), + 0 + );This keeps daily ranges correct while fixing hourly ranges.
Also applies to: 796-804, 809-809, 818-818
apps/api/src/query/utils.ts (2)
26-34: Inline cast inshouldParseReferrersis fine, but a bit noisyAccessing
type/namevia repeated{ type?: string; name?: string }casts works, but is slightly verbose; if this grows, consider a small helper type or local narrowing to avoid duplicating the cast. Not a blocker.
203-210:buildWhereClausecan return invalidWHERE ()when all conditions are filtered outIf
conditionsis non-empty but every entry matchesUNSAFE_SQL,safeends up empty and the function returnsWHERE (), which is invalid SQL. Safer to short-circuit whensafe.length === 0and return an empty string.export function buildWhereClause(conditions?: string[]): string { if (!conditions?.length) { return ""; } const safe = conditions.filter((c) => !UNSAFE_SQL.test(c)); - return `WHERE (${safe.join(" AND ")})`; + if (!safe.length) { + return ""; + } + return `WHERE (${safe.join(" AND ")})`; }apps/basket/src/lib/producer.ts (2)
229-247: Bounded re-buffering on flush failure is a solid improvementThe new
bufferHardMaxcheck avoids unbounded growth when a table flush fails and explicitly tracks dropped events with logging, which tightens backpressure behavior. One optional refinement: whenthis.buffer.length + events.lengthslightly exceedsbufferHardMax, you currently drop the whole batch; if retaining as many events as possible matters, consider re-queueing up to the remaining capacity and only dropping the overflow.
210-216: Passabort_signalto the ClickHouse insert call to makeflushTimeouteffectiveThe
AbortControllercreated at line 212 is never wired into theclickHouse.insert()call. The@clickhouse/clientinsert API accepts anabort_signalparameter (part of BaseQueryParams), so the timeout should be passed asabort_signal: controller.signalin the options object at line 221–225. Without this, the timeout mechanism has no effect on slow inserts.apps/dashboard/app/(auth)/login/page.tsx (1)
232-248: Consider enhancing focus indication for the password toggle button.The password show/hide button uses the ghost variant, which may not provide sufficient visual distinction when focused vs. when the input field is focused. Users navigating by keyboard might find it difficult to determine whether the button or the input has focus.
Consider adding explicit focus ring styles or using a variant with more distinct focus states.
apps/api/src/query/builders/profiles.ts (2)
153-173:total_pageviewsinprofile_detailcurrently counts all events, not page views.Here:
COUNT(DISTINCT session_id) as total_sessions, COUNT(*) as total_pageviews,
COUNT(*)includes all event types for the visitor, sototal_pageviewswill disagree with page‑view metrics elsewhere that are based onscreen_viewonly (e.g.,profile_sessions.page_views).Either:
- Rename this to
total_eventsif that’s the intent, or- Restrict it to page views to keep semantics aligned, e.g.:
countIf(event_name = 'screen_view') as total_pageviews,
38-47: Apply time window constraints tovisitor_sessionsto matchvisitor_profiles.The
visitor_sessionsCTE (lines 79-82) lacks time constraints, so sessions and their associated data (session_start,session_end,page_views,unique_pages) can extend outside the requested date range. AddAND e.time >= toDateTime({startDate:String})andAND e.time <= toDateTime({endDate:String})to the WHERE clause.Additionally,
combinedWhereClauseis reused in both CTEs without explicit column aliases, which can cause ambiguity for filters referencing columns that exist in bothanalytics.eventsand derived data (e.g.,country,region). Consider using explicitly qualified aliases in filter conditions or separate condition sets for the profile-level vs session-level filtering.
♻️ Duplicate comments (17)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx (1)
565-572: Spinner border styling remains incomplete (duplicate issue).The spinner still uses
border-primary border-b-2, which only renders the bottom border with width while other borders remain invisible. This was flagged in a previous review but hasn't been addressed.Apply the previously suggested fix:
- <div className="flex items-center justify-center rounded border bg-sidebar py-12"> - <div className="text-center"> - <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-primary border-b-2" /> + <div className="flex items-center justify-center rounded border bg-sidebar py-12"> + <div className="text-center"> + <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-2 border-primary/30 border-t-primary" />apps/dashboard/app/(main)/organizations/settings/danger/danger-zone-settings.tsx (1)
158-158: Inconsistent icon margin spacing (previously reported).The
SignOutIconhere is still missing themr-2className that theTrashIconhas on line 149, causing inconsistent spacing in the button layout.Apply this diff to fix the spacing:
- <SignOutIcon size={14} /> + <SignOutIcon className="mr-2" size={14} />apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx (1)
76-92: Suspense boundaries are ineffective with current data fetching approach.This was flagged in a previous review. Since data fetching uses
useQuery(notuseSuspenseQuery), the components render immediately and handle loading states internally via theisLoadingprop. The Suspense fallbacks will never trigger.Either:
- Remove the Suspense wrappers if internal loading states are sufficient
- Refactor to use
useSuspenseQueryto leverage Suspense properlyapps/dashboard/app/(main)/websites/[id]/_components/tabs/audience-tab.tsx (1)
219-230: Performance issue: Column definitions still not memoized (previously flagged).This issue was already identified in a previous review. The
displayNamesobject and all five column arrays (countryColumns,regionColumns,cityColumns,timezoneColumns,languageColumns) are recreated on every render. Since these are included in thegeographicTabsdependency array (lines 296-300), the tabs will unnecessarily recompute on every render, defeating the purpose ofuseMemo.Apply the previously suggested fix:
+ const displayNames = useMemo( + () => + typeof window !== "undefined" + ? new Intl.DisplayNames([navigator.language || "en"], { + type: "language", + }) + : null, + [] + ); - const displayNames = - typeof window !== "undefined" - ? new Intl.DisplayNames([navigator.language || "en"], { - type: "language", - }) - : null; - - const countryColumns = createGeoColumns({ type: "country" }); - const regionColumns = createGeoColumns({ type: "region" }); - const cityColumns = createGeoColumns({ type: "city" }); - const timezoneColumns = createTimezoneColumns(); - const languageColumns = createLanguageColumns(displayNames); + const countryColumns = useMemo(() => createGeoColumns({ type: "country" }), []); + const regionColumns = useMemo(() => createGeoColumns({ type: "region" }), []); + const cityColumns = useMemo(() => createGeoColumns({ type: "city" }), []); + const timezoneColumns = useMemo(() => createTimezoneColumns(), []); + const languageColumns = useMemo( + () => createLanguageColumns(displayNames), + [displayNames] + );apps/api/src/query/expressions.ts (3)
781-783: Invalid ClickHouse syntax in quantile fallback.The fallback
quantile(0.50)(*)is invalid—ClickHouse quantile functions require a specific numeric column, not*. This will cause runtime errors whensourceis missing.case "quantile": // source should be "level)(column" e.g., "0.50)(metric_value" - return source ? `quantile(${source})` : "quantile(0.50)(*)"; + if (!source) { + throw new Error("quantile requires a source in format 'level)(column'"); + } + return `quantile(${source})`;
799-800: Invalid ClickHouse syntax in quantileIf fallback.Same issue as
quantile—the fallbackquantile(0.50)(*)is invalid ClickHouse syntax.case "quantileIf": - return source ? `quantile(${source})` : "quantile(0.50)(*)"; + if (!source) { + throw new Error("quantileIf requires a source in format 'level)(column'"); + } + return `quantile(${source})`;
742-748: Fragile source encoding for quantileIf with condition.The pattern of encoding
sourceas"level)(column"(e.g.,"0.50)(metric_value") is non-obvious and error-prone. Consider adding a dedicatedlevelproperty toFieldDefinitionTypefor quantile aggregates, or at minimum add validation to detect malformed inputs.case "quantile": case "quantileIf": - // For quantile with condition, source should be "level)(column" - // e.g., source = "0.50)(metric_value" produces quantileIf(0.50)(metric_value, condition) - return source - ? `quantileIf(${source}, ${condition})` - : `quantileIf(0.50)(1, ${condition})`; + // Validate source format for quantile + if (!source || !source.includes(")(")) { + throw new Error( + `quantileIf requires source in format 'level)(column', got: ${source}` + ); + } + return `quantileIf(${source}, ${condition})`;apps/api/src/query/index.ts (1)
17-25: Operator set reduction already flagged - no new concerns.The operator changes from legacy operators to the new set (
eq,ne,contains,not_contains,starts_with,in,not_in) were previously identified as a breaking API change. The implementation is consistent with the updatedFilterOperatorsintypes.ts.apps/api/src/query/builders/sessions.ts (1)
161-161: Non-deterministic ID generation already flagged.Using
generateUUIDv4() as idfor custom events generates a new UUID on each query execution, meaning the same event receives different IDs across queries. This was previously flagged as a major issue. Consider using a deterministic ID based on event properties or storing/retrieving the client-providedeventId.apps/api/src/query/builders/performance.ts (1)
163-181: GROUP BY mismatch causes incorrect path aggregation — still present.The SELECT normalizes the path (line 164) with
trimRightanddecodeURLComponent, but the GROUP BY (line 181) uses rawpath. This causes paths like/about/and/aboutto be grouped separately but display with the same normalized name, leading to duplicate rows and incorrect metric aggregation.Apply this diff to fix:
- GROUP BY path + GROUP BY nameOr equivalently, group by the same expression:
- GROUP BY path + GROUP BY decodeURLComponent(CASE WHEN trimRight(path(path), '/') = '' THEN '/' ELSE trimRight(path(path), '/') END)apps/dashboard/app/(main)/organizations/settings/websites/website-settings.tsx (1)
164-164: Pluralization helper already suggested in previous review.The inline pluralization logic here was previously flagged for extraction to a reusable helper function.
apps/dashboard/app/(main)/organizations/components/general-settings.tsx (1)
18-21: Fix nullableorganization.slughandling to avoid runtime errors and inconsistent change detection.
organization.slugis nullable per the schema, butslugstate and validation assume a non-null string:
const [slug, setSlug] = useState(organization.slug);hasChangescomparesslug !== organization.slug.handleSavecallsslug.trim().- The slug
<Input>usesvalue={slug}.If an org has
slug === nulland the user only edits the name,hasChangesbecomes true,handleSaveis invoked, andslug.trim()will throw. The same nullable value also makes change detection and the input value type less clear.Recommend normalizing to a string and aligning
hasChangeswith that normalization:- const [slug, setSlug] = useState(organization.slug); + const [slug, setSlug] = useState(organization.slug ?? ""); // ... - const hasChanges = name !== organization.name || slug !== organization.slug; + const hasChanges = + name !== organization.name || slug !== (organization.slug ?? "");This keeps the input controlled with a string, prevents
trimonnull, and makes the “unsaved changes” banner behavior predictable when migrating orgs that don’t yet have a slug.Also applies to: 24-36, 37-53, 99-105
apps/api/src/query/builders/profiles.ts (1)
23-47: Alignunique_pages/page_viewssemantics inprofile_listwithprofile_sessions.In
profile_list:
- Line 30:
COUNT(DISTINCT path) as unique_pagescounts all paths, regardless ofevent_name.- Line 56:
COUNT(DISTINCT e.path) as unique_pagesdoes the same, whilepage_viewsisCOUNT(*)across all events.In
profile_sessionsyou’ve already normalized bothpage_viewsandunique_pagesto only considerscreen_viewevents, so these endpoints will now disagree on the same conceptual metrics for the same visitor/session, which is a data integrity issue.To keep metrics consistent, consider updating
profile_listto match theprofile_sessionslogic, for example:- COUNT(DISTINCT path) as unique_pages, + COUNT(DISTINCT CASE WHEN event_name = 'screen_view' THEN path ELSE NULL END) as unique_pages, ... - COUNT(*) as page_views, - COUNT(DISTINCT e.path) as unique_pages, + countIf(e.event_name = 'screen_view') as page_views, + COUNT(DISTINCT CASE WHEN e.event_name = 'screen_view' THEN e.path ELSE NULL END) as unique_pages,Also applies to: 49-56
apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx (1)
307-352: NestedFormFieldfor operator inside value field render prop (same prior nit)The
operatorFormFieldis nested inside thevalueFormField’s render callback, which is unconventional and makes the structure harder to follow.As suggested previously, you can render
operatorandvalueas siblings inside the same flex container:-<FormField - name="value" - render={({ field }) => ( - <FormItem> - <FormControl> - <div className="flex"> - <FormField name="operator" render={...} /> - <Input {...field} /> - </div> - </FormControl> - <FormMessage /> - </FormItem> - )} -/> +<FormItem> + <FormLabel className="text-xs">Value</FormLabel> + <div className="flex"> + <FormField + control={form.control} + name="operator" + render={({ field: operatorField }) => ( + <Select + value={operatorField.value} + onValueChange={operatorField.onChange} + > + {/* trigger/content */} + </Select> + )} + /> + <FormField + control={form.control} + name="value" + render={({ field }) => ( + <Input autoFocus className="rounded-l-none text-sm" {...field} /> + )} + /> + </div> + <FormMessage className="text-xs" /> +</FormItem>This keeps each field isolated and easier to reason about.
apps/dashboard/app/(main)/websites/[id]/_components/filters/filters-section.tsx (1)
20-22:getFieldLabelhelper duplicated across filter componentsThis
getFieldLabelimplementation is repeated in at leastfilters-section.tsx,save-filter-dialog.tsx, andsaved-filters-menu.tsx.Consider extracting it once, e.g. into
@/hooks/use-filtersnext togetOperatorLabel:// apps/dashboard/hooks/use-filters.ts import { filterOptions } from "@databuddy/shared/lists/filters"; export function getFieldLabel(field: string): string { return filterOptions.find((o) => o.value === field)?.label ?? field; }Then replace local helpers with:
-import { filterOptions } from "@databuddy/shared/lists/filters"; +import { getFieldLabel } from "@/hooks/use-filters";And remove the in-file
getFieldLabeldefinitions.apps/dashboard/app/(main)/websites/[id]/_components/filters/save-filter-dialog.tsx (1)
37-39:getFieldLabelduplicated (same as other filter components)This helper repeats the same logic already present in the other filter files.
Recommend centralizing
getFieldLabel(e.g. in@/hooks/use-filters) and importing it here instead of re-defining it locally, as outlined in the comment onfilters-section.tsx.apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx (1)
35-37:getFieldLabelduplicated againSame helper as in
filters-section.tsxandsave-filter-dialog.tsx.Please centralize this helper (e.g. in
@/hooks/use-filters) and import it here, removing the local implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (44)
apps/api/src/index.ts(1 hunks)apps/api/src/lib/api-key.test.ts(1 hunks)apps/api/src/lib/clickhouse-backup.ts(6 hunks)apps/api/src/query/batch-executor.ts(1 hunks)apps/api/src/query/builders/custom-events.ts(10 hunks)apps/api/src/query/builders/engagement.ts(1 hunks)apps/api/src/query/builders/performance.ts(2 hunks)apps/api/src/query/builders/profiles.ts(5 hunks)apps/api/src/query/builders/sessions.ts(3 hunks)apps/api/src/query/expressions.ts(11 hunks)apps/api/src/query/index.ts(2 hunks)apps/api/src/query/screen-resolution-to-device-type.ts(2 hunks)apps/api/src/query/simple-builder.ts(11 hunks)apps/api/src/query/types.ts(2 hunks)apps/api/src/query/utils.ts(7 hunks)apps/api/src/routes/query.ts(8 hunks)apps/api/src/schemas/query-schemas.ts(2 hunks)apps/api/src/types/tables.ts(1 hunks)apps/basket/src/lib/event-service.ts(4 hunks)apps/basket/src/lib/producer.ts(2 hunks)apps/dashboard/app/(auth)/layout.tsx(1 hunks)apps/dashboard/app/(auth)/login/page.tsx(4 hunks)apps/dashboard/app/(main)/billing/components/usage-row.tsx(1 hunks)apps/dashboard/app/(main)/billing/cost-breakdown/components/consumption-chart.tsx(11 hunks)apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx(4 hunks)apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx(1 hunks)apps/dashboard/app/(main)/billing/hooks/use-billing.ts(1 hunks)apps/dashboard/app/(main)/billing/types/billing.ts(1 hunks)apps/dashboard/app/(main)/billing/utils/feature-usage.ts(1 hunks)apps/dashboard/app/(main)/organizations/components/general-settings.tsx(3 hunks)apps/dashboard/app/(main)/organizations/invitations/invitation-list.tsx(2 hunks)apps/dashboard/app/(main)/organizations/settings/danger/danger-zone-settings.tsx(8 hunks)apps/dashboard/app/(main)/organizations/settings/danger/transfer-assets.tsx(6 hunks)apps/dashboard/app/(main)/organizations/settings/websites/website-settings.tsx(5 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/filters-section.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/save-filter-dialog.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx(8 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/audience-tab.tsx(11 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx(6 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx(3 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance/_components/web-vitals-chart.tsx(3 hunks)apps/dashboard/app/(main)/websites/[id]/_components/website-page-header.tsx(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (26)
apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx (5)
packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)apps/dashboard/hooks/use-funnels.ts (2)
AutocompleteData(68-78)useAutocompleteData(364-372)apps/dashboard/lib/utils.ts (1)
cn(5-7)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)apps/dashboard/hooks/use-filters.ts (1)
operatorOptions(4-10)
apps/dashboard/app/(main)/websites/[id]/_components/filters/filters-section.tsx (5)
packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)apps/dashboard/stores/jotai/filterAtoms.ts (2)
dynamicQueryFiltersAtom(284-284)removeDynamicFilterAtom(315-329)apps/dashboard/hooks/use-saved-filters.ts (1)
useSavedFilters(108-409)apps/dashboard/hooks/use-filters.ts (1)
getOperatorLabel(33-35)
apps/dashboard/app/(main)/organizations/invitations/invitation-list.tsx (1)
packages/db/src/drizzle/schema.ts (1)
invitation(138-175)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx (1)
apps/dashboard/components/table/data-table.tsx (1)
DataTable(83-236)
apps/api/src/query/builders/sessions.ts (1)
apps/api/src/types/tables.ts (1)
Analytics(13-23)
apps/api/src/query/index.ts (2)
apps/api/src/query/types.ts (1)
QueryRequest(230-242)apps/api/src/query/simple-builder.ts (1)
SimpleQueryBuilder(206-715)
apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx (1)
apps/dashboard/app/(main)/websites/_components/notice-banner.tsx (1)
NoticeBanner(5-52)
apps/dashboard/app/(main)/organizations/settings/websites/website-settings.tsx (3)
apps/dashboard/app/(main)/organizations/components/empty-state.tsx (1)
EmptyState(17-62)apps/dashboard/app/(main)/websites/[id]/_components/utils/ui-components.tsx (1)
EmptyState(169-185)apps/dashboard/components/right-sidebar.tsx (1)
RightSidebar(16-27)
apps/api/src/query/builders/custom-events.ts (2)
apps/api/src/types/tables.ts (1)
Analytics(13-23)apps/api/src/query/types.ts (2)
Filter(41-49)TimeUnit(35-35)
apps/api/src/query/simple-builder.ts (1)
apps/api/src/query/screen-resolution-to-device-type.ts (1)
DeviceType(1-8)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx (3)
apps/dashboard/hooks/use-chart-preferences.ts (1)
useChartPreferences(99-118)apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance/_utils/performance-utils.ts (1)
formatNumber(14-22)apps/dashboard/lib/formatters.ts (1)
formatNumber(40-46)
apps/dashboard/app/(auth)/login/page.tsx (4)
apps/dashboard/components/ui/button.tsx (1)
Button(59-59)apps/docs/components/ui/separator.tsx (1)
Separator(28-28)apps/dashboard/components/ui/label.tsx (1)
Label(24-24)apps/dashboard/components/ui/input.tsx (1)
Input(95-95)
apps/dashboard/app/(main)/billing/types/billing.ts (1)
apps/dashboard/app/(main)/billing/hooks/use-billing.ts (1)
CustomerWithPaymentMethod(21-21)
apps/dashboard/app/(main)/organizations/components/general-settings.tsx (3)
apps/dashboard/hooks/use-organizations.ts (1)
Organization(457-459)apps/dashboard/app/(main)/organizations/components/organization-logo-uploader.tsx (1)
OrganizationLogoUploader(32-272)apps/dashboard/components/right-sidebar.tsx (1)
RightSidebar(16-27)
apps/dashboard/app/(main)/websites/[id]/_components/filters/save-filter-dialog.tsx (3)
packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)apps/dashboard/hooks/use-filters.ts (1)
getOperatorLabel(33-35)
apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx (4)
apps/dashboard/hooks/use-saved-filters.ts (1)
SavedFilter(8-14)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)apps/dashboard/hooks/use-filters.ts (1)
getOperatorLabel(33-35)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/audience-tab.tsx (5)
apps/dashboard/components/table/rows/geo-row.tsx (1)
createGeoColumns(29-98)apps/dashboard/components/table/rows/timezone-row.tsx (1)
createTimezoneColumns(28-88)apps/dashboard/components/table/rows/language-row.tsx (1)
createLanguageColumns(23-89)apps/dashboard/components/table/data-table.tsx (1)
DataTable(83-236)apps/dashboard/components/ui/skeleton.tsx (1)
Skeleton(13-13)
apps/dashboard/app/(main)/billing/hooks/use-billing.ts (1)
apps/dashboard/app/(main)/billing/utils/feature-usage.ts (1)
calculateFeatureUsage(56-96)
apps/dashboard/app/(main)/billing/utils/feature-usage.ts (1)
apps/dashboard/app/(main)/billing/cost-breakdown/utils/billing-utils.ts (1)
calculateOverageCost(9-25)
apps/api/src/query/utils.ts (2)
apps/api/src/query/types.ts (1)
SimpleQueryConfig(174-228)apps/api/src/query/screen-resolution-to-device-type.ts (1)
mapScreenResolutionToDeviceType(102-107)
apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx (3)
apps/dashboard/components/ui/skeleton.tsx (1)
Skeleton(13-13)apps/dashboard/app/(main)/billing/cost-breakdown/components/consumption-chart.tsx (1)
ConsumptionChart(41-398)apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (1)
UsageBreakdownTable(56-189)
apps/dashboard/app/(auth)/layout.tsx (1)
apps/dashboard/components/ui/button.tsx (1)
Button(59-59)
apps/api/src/types/tables.ts (1)
packages/db/src/clickhouse/schema.ts (9)
AnalyticsEvent(580-643)ErrorSpanRow(439-451)ErrorHourlyAggregate(456-466)WebVitalsSpan(478-486)WebVitalsHourlyAggregate(491-502)CustomEventSpan(546-554)CustomEventsHourlyAggregate(559-567)BlockedTraffic(504-530)CustomOutgoingLink(569-578)
apps/dashboard/app/(main)/billing/components/usage-row.tsx (2)
apps/dashboard/components/ui/badge.tsx (1)
Badge(54-54)apps/docs/components/ui/badge.tsx (1)
Badge(50-50)
apps/api/src/lib/api-key.test.ts (2)
apps/api/src/lib/api-key.ts (13)
ApiKeyRow(7-7)isApiKeyPresent(28-32)extractSecret(34-46)getEffectiveScopes(84-92)hasKeyScope(94-103)hasKeyAnyScope(105-114)hasKeyAllScopes(116-125)resolveEffectiveScopesForWebsite(127-132)hasWebsiteScope(134-140)hasWebsiteAnyScope(142-148)hasWebsiteAllScopes(150-156)hasGlobalAccess(158-164)getAccessibleWebsiteIds(166-177)packages/sdk/src/core/flags/browser-storage.ts (1)
isExpired(82-87)
apps/api/src/query/builders/profiles.ts (1)
apps/api/src/query/types.ts (2)
Filter(41-49)TimeUnit(35-35)
🪛 ast-grep (0.40.0)
apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx
[warning] 104-104: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vercel Agent Review
- GitHub Check: Cursor Bugbot
| // Record backup start | ||
| await clickHouseOG.command({ | ||
| query: ` | ||
| INSERT INTO internal.databuddy_backups (backup_name, backup_path, backup_type, base_backup_path, status) | ||
| VALUES ('${backupName}', '${s3Url}', '${isIncremental ? "incremental" : "full"}', '${lastBackupPath ?? ""}', 'started') | ||
| `, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
SQL injection risk: Use parameterized queries for INSERT statements.
While backupName and s3Url are derived from controlled sources, string interpolation in SQL is fragile. If lastBackupPath (retrieved from the database) were ever compromised, this could lead to SQL injection.
Consider using parameterized queries consistently, as done in getBackupMetadata.
- await clickHouseOG.command({
- query: `
- INSERT INTO internal.databuddy_backups (backup_name, backup_path, backup_type, base_backup_path, status)
- VALUES ('${backupName}', '${s3Url}', '${isIncremental ? "incremental" : "full"}', '${lastBackupPath ?? ""}', 'started')
- `,
- });
+ await clickHouseOG.query({
+ query: `
+ INSERT INTO internal.databuddy_backups (backup_name, backup_path, backup_type, base_backup_path, status)
+ VALUES ({backupName:String}, {s3Url:String}, {backupType:String}, {baseBackupPath:String}, 'started')
+ `,
+ query_params: {
+ backupName,
+ s3Url,
+ backupType: isIncremental ? "incremental" : "full",
+ baseBackupPath: lastBackupPath ?? "",
+ },
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/lib/clickhouse-backup.ts around lines 178 to 184, the INSERT
uses string interpolation which risks SQL injection; change it to a
parameterized query like the pattern used in getBackupMetadata: prepare the
INSERT with placeholders for backup_name, backup_path, backup_type,
base_backup_path and status, pass [backupName, s3Url, isIncremental ?
"incremental" : "full", lastBackupPath ?? "", "started"] (or equivalent) as the
parameters array, and execute via clickHouseOG.command with the params instead
of embedding values directly.
| // Mark backup as failed | ||
| try { | ||
| await clickHouseOG.command({ | ||
| query: ` | ||
| ALTER TABLE internal.databuddy_backups | ||
| UPDATE status = 'failed', error = '${errorMessage.replace(/'/g, "\\'")}' | ||
| WHERE backup_name = '${backupName}' | ||
| `, | ||
| }); | ||
| } catch { } | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| timestamp: timestampStr, | ||
| }; | ||
| } | ||
| }); | ||
| } catch {} |
There was a problem hiding this comment.
SQL injection vulnerability in error message handling.
The error message escaping (errorMessage.replace(/'/g, "\\'")) is insufficient. Error messages can contain arbitrary content including SQL metacharacters beyond single quotes (e.g., backslashes, semicolons). A crafted error message could potentially break out of the string context.
Use parameterized queries or at minimum sanitize more thoroughly.
try {
- await clickHouseOG.command({
- query: `
+ await clickHouseOG.query({
+ query: `
ALTER TABLE internal.databuddy_backups
- UPDATE status = 'failed', error = '${errorMessage.replace(/'/g, "\\'")}'
- WHERE backup_name = '${backupName}'
+ UPDATE status = 'failed', error = {errorMsg:String}
+ WHERE backup_name = {backupName:String}
`,
- });
+ query_params: {
+ errorMsg: errorMessage,
+ backupName,
+ },
+ });
} catch {}🤖 Prompt for AI Agents
In apps/api/src/lib/clickhouse-backup.ts around lines 240 to 249, the
backup-failed UPDATE builds SQL by interpolating errorMessage and backupName
(with only naive single-quote escaping) which creates an SQL injection risk;
replace this interpolation with a parameterized query (bind variables) so the
driver sends the error message and backup name as parameters instead of
embedding them in the SQL string, or if the client has no parameter support,
escape all SQL metacharacters robustly and use a proper library escaping
function; update the command call to use the parameter API (passing backupName
and errorMessage as bound parameters) and remove the manual replace logic.
| }; | ||
| } | ||
| }); | ||
| } catch {} |
There was a problem hiding this comment.
Empty catch block silently swallows errors.
If updating the backup status to 'failed' itself fails, there's no logging or indication. This could leave the backup record in 'started' status indefinitely.
- } catch {}
+ } catch (updateError) {
+ logger.warn({ updateError, backupName }, "Failed to update backup status to failed");
+ }📝 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.
| } catch {} | |
| try { | |
| // ... update backup status to 'failed' ... | |
| } catch (updateError) { | |
| logger.warn({ updateError, backupName }, "Failed to update backup status to failed"); | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/lib/clickhouse-backup.ts around line 249 the empty catch block
swallows errors when attempting to update the backup status to 'failed'; change
it to capture the caught error and log it (including backup id/context and the
error stack/message) using the module's logger (or console.error if no logger
available), and avoid silently swallowing the exception — either rethrow after
logging or ensure the failure is recorded via a secondary error record so the
backup isn't left stuck in 'started' state.
| const result = await clickHouseOG.query({ | ||
| query: ` | ||
| SELECT backup_name, backup_path | ||
| FROM internal.databuddy_backups | ||
| WHERE timestamp < now() - INTERVAL ${retentionDays} DAY | ||
| AND status = 'completed' | ||
| ORDER BY timestamp ASC | ||
| `, | ||
| }); | ||
| }); | ||
|
|
||
| const oldBackups = await result.json<{ | ||
| backup_name: string; | ||
| backup_path: string; | ||
| }>(); | ||
| const oldBackups = await result.json<{ | ||
| backup_name: string; | ||
| backup_path: string; | ||
| }>(); | ||
|
|
||
| if (oldBackups.data.length === 0) { | ||
| logger.info("No backups to clean up"); | ||
| return { success: true, deletedCount: 0 }; | ||
| } | ||
| if (oldBackups.data.length === 0) { | ||
| logger.info("No backups to clean up"); | ||
| return { success: true, deletedCount: 0 }; | ||
| } | ||
|
|
||
| logger.info( | ||
| `Found ${oldBackups.data.length} backups to clean up (tracking table only, manual R2 cleanup required)` | ||
| ); | ||
| logger.info( | ||
| `Found ${oldBackups.data.length} backups to clean up (tracking table only, manual R2 cleanup required)` | ||
| ); | ||
|
|
||
| // Delete from tracking table | ||
| await clickHouseOG.command({ | ||
| query: ` | ||
| // Delete from tracking table | ||
| await clickHouseOG.command({ | ||
| query: ` | ||
| DELETE FROM internal.databuddy_backups | ||
| WHERE timestamp < now() - INTERVAL ${retentionDays} DAY | ||
| `, | ||
| }); | ||
|
|
||
| logger.info( | ||
| `Cleaned up ${oldBackups.data.length} old backups from tracking table` | ||
| ); | ||
|
|
||
| return { | ||
| success: true, | ||
| deletedCount: oldBackups.data.length, | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| logger.error({ error }, `Backup cleanup failed: ${errorMessage}`); | ||
|
|
||
| return { | ||
| success: false, | ||
| deletedCount: 0, | ||
| error: errorMessage, | ||
| }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validate retentionDays parameter to prevent unexpected SQL behavior.
While retentionDays has a default of 30 and TypeScript types it as number, JavaScript runtime doesn't enforce this. If a non-numeric value is passed, the generated SQL could be malformed or exploitable.
export async function cleanupOldBackups(retentionDays = 30): Promise<{
success: boolean;
deletedCount: number;
error?: string;
}> {
+ // Validate and sanitize retentionDays
+ const days = Math.max(1, Math.min(365, Math.floor(Number(retentionDays) || 30)));
+
try {
- logger.info(`Starting backup cleanup (retention: ${retentionDays} days)`);
+ logger.info(`Starting backup cleanup (retention: ${days} days)`);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/lib/clickhouse-backup.ts around lines 327 to 357, the
retentionDays value is interpolated directly into SQL which can produce
malformed or unsafe queries if a non-numeric value is passed; validate and
sanitize retentionDays before building the query by coercing it to a safe
integer (e.g., const days = Number(retentionDays); if (!Number.isFinite(days) ||
Number.isNaN(days) || !Number.isInteger(days) || days <= 0) throw new
Error('Invalid retentionDays');) or use parseInt with similar checks, then use
that sanitized integer in the template; alternatively default to 30 when
validation fails and log the correction — do not interpolate unchecked user
input into the SQL.
| const database = options.database ?? "analytics"; | ||
| const restoreTarget = options.tables | ||
| ? `TABLE ${options.tables.map((t) => `${database}.${t}`).join(", ")}` | ||
| : `DATABASE ${database}`; | ||
|
|
||
| const restoreQuery = ` | ||
| RESTORE ${restoreTarget} | ||
| FROM S3('${backupPath}', '${env.R2_ACCESS_KEY_ID}', '${env.R2_SECRET_ACCESS_KEY}') | ||
| SETTINGS | ||
| async = false, | ||
| s3_max_redirects = 10 | ||
| s3_max_redirects = 10, | ||
| s3_use_virtual_addressing = false | ||
| `; |
There was a problem hiding this comment.
SQL injection vulnerability: backupPath, database, and tables are directly interpolated.
The backupPath parameter and optional tables/database values are interpolated directly into the SQL query without validation. An attacker could inject malicious SQL through these parameters.
Add input validation for all parameters:
export async function restoreBackup(
backupPath: string,
options: {
tables?: string[];
database?: string;
} = {}
): Promise<BackupResult> {
const timestamp = new Date().toISOString();
+
+ // Validate backupPath format (should be an S3 URL)
+ if (!/^https?:\/\/[^'"\s]+$/.test(backupPath)) {
+ return {
+ success: false,
+ error: "Invalid backup path format",
+ timestamp,
+ };
+ }
+
+ // Validate database and table names (alphanumeric and underscores only)
+ const identifierRegex = /^[a-zA-Z_][a-zA-Z0-9_]*$/;
+ const database = options.database ?? "analytics";
+ if (!identifierRegex.test(database)) {
+ return {
+ success: false,
+ error: "Invalid database name",
+ timestamp,
+ };
+ }
+ if (options.tables?.some((t) => !identifierRegex.test(t))) {
+ return {
+ success: false,
+ error: "Invalid table name",
+ timestamp,
+ };
+ }Committable suggestion skipped: line range outside the PR's diff.
| function filtersMatch( | ||
| a: DynamicQueryFilter[], | ||
| b: DynamicQueryFilter[] | ||
| ): boolean { | ||
| if (a.length !== b.length) return false; | ||
| return a.every((f1, i) => { | ||
| const f2 = b[i]; | ||
| return ( | ||
| f2 && | ||
| f1.field === f2.field && | ||
| f1.operator === f2.operator && | ||
| JSON.stringify(f1.value) === JSON.stringify(f2.value) | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
filtersMatch is order-sensitive and relies on JSON.stringify for value comparison
filtersMatch requires arrays to match in order and compares value via JSON.stringify. This is fine if:
- filter application always preserves order, and
- you don’t expect type-coercion equivalence (e.g.
"1"vs1) to count as equal.
If order shouldn’t matter, or you want a stricter structural comparison, consider normalizing (e.g. sorting by field/operator/value) before comparison instead of comparing by index. Otherwise, a comment explaining the order dependency would help future readers.
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx
around lines 39-53, filtersMatch is order-sensitive and uses JSON.stringify to
compare values; change it to normalize both arrays before comparing by sorting
their filters deterministically (e.g., by field, then operator, then a stable
string/serialized representation of value) and then compare lengths and
corresponding items using strict equality for field/operator and a deep-equality
check for value (or a stable JSON serialization) so order no longer matters and
value comparisons are robust.
| <div className="flex items-center justify-between px-2 py-1.5"> | ||
| <span className="font-medium text-xs">Saved Filters</span> | ||
| <Button | ||
| className="h-6 text-xs" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| onDeleteAll(); | ||
| }} | ||
| size="sm" | ||
| variant="ghost" | ||
| > | ||
| Clear all | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: close menu when “Clear all” is clicked
The “Clear all” button calls onDeleteAll but leaves the dropdown open. Given this action usually opens a confirmation dialog, you might want to close the menu for a cleaner UX.
onClick={(e) => {
e.preventDefault();
- onDeleteAll();
+ onDeleteAll();
+ setOpen(false);
}}📝 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.
| <div className="flex items-center justify-between px-2 py-1.5"> | |
| <span className="font-medium text-xs">Saved Filters</span> | |
| <Button | |
| className="h-6 text-xs" | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| onDeleteAll(); | |
| }} | |
| size="sm" | |
| variant="ghost" | |
| > | |
| Clear all | |
| </Button> | |
| </div> | |
| <div className="flex items-center justify-between px-2 py-1.5"> | |
| <span className="font-medium text-xs">Saved Filters</span> | |
| <Button | |
| className="h-6 text-xs" | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| onDeleteAll(); | |
| setOpen(false); | |
| }} | |
| size="sm" | |
| variant="ghost" | |
| > | |
| Clear all | |
| </Button> | |
| </div> |
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx
around lines 90 to 103, the “Clear all” button invokes onDeleteAll but leaves
the dropdown open; update the click handler to also close the menu (call the
provided onClose prop or the menu state setter) after invoking onDeleteAll — if
onDeleteAll returns a Promise await it first and then call onClose to ensure any
confirmation completes before closing; keep the preventDefault and preserve
button props.
| createPageTimeColumns, | ||
| createReferrerColumns, | ||
| } from "@/components/table/rows"; | ||
| import { useChartPreferences } from "@/hooks/use-chart-preferences"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
useChartPreferences integration into StatCard looks solid
Importing useChartPreferences("overview-stats") and passing chartType/chartStepType through to all StatCard instances is consistent and keeps mini‑charts driven by user prefs. If you want the main “Traffic Trends” chart to respect the same preferences, consider threading these props into MetricsChartWithAnnotations as a follow‑up.
Also applies to: 125-125, 872-876
🤖 Prompt for AI Agents
apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx lines
31, 125, and 872-876: you already import/use useChartPreferences at line 31 for
StatCard — also wire the returned chartType and chartStepType into the main
“Traffic Trends” chart by passing them as props to the
MetricsChartWithAnnotations instance(s) rendered around lines 125 and 872-876;
if MetricsChartWithAnnotations doesn’t accept those props yet, update its prop
types/signature to accept chartType and chartStepType and forward them to the
internal chart component so the main chart respects the same user preferences as
the StatCard mini-charts.
| const showSubtitleSkeleton = isLoading && !description; | ||
|
|
||
| if (showSubtitleSkeleton) { | ||
| return ( | ||
| <div className="h-5 sm:h-6"> | ||
| <Skeleton className="h-4 w-48" /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Subtitle skeleton and truncation behavior are coherent; double‑check loading semantics
The showSubtitleSkeleton = isLoading && !description guard plus the fixed-height wrappers and truncation classes give you stable layout and good line clamping. Just be aware that while isLoading is true and description is falsy, any provided subtitle (string or node) will be hidden in favor of the skeleton; ensure that matches the intended UX for callers that might pass a static subtitle while data loads.
Also applies to: 76-81, 86-87
🤖 Prompt for AI Agents
In apps/dashboard/app/(main)/websites/[id]/_components/website-page-header.tsx
around lines 64-71 (and similarly at 76-81, 86-87), the current guard
showSubtitleSkeleton = isLoading && !description will hide any passed subtitle
while loading; update the condition and rendering so the skeleton only replaces
content when there is no explicit subtitle provided (e.g., render skeleton when
isLoading && !description && !subtitle or prefer showing a static subtitle prop
even during loading), and add a short comment documenting the chosen UX decision
so callers know whether passing a static subtitle should override the loading
skeleton.
| <div className="space-y-4"> | ||
| <div className="border-b p-3 sm:p-4"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/dashboard/app/\(main\)/websites/\[id\]/_components/website-page-header.tsxRepository: databuddy-analytics/Databuddy
Length of output: 152
🏁 Script executed:
cat -n apps/dashboard/app/\(main\)/websites/\[id\]/_components/website-page-header.tsx | sed -n '140,260p'Repository: databuddy-analytics/Databuddy
Length of output: 4286
Add disabled={isRefreshing} to the error card retry button and optionally add aria-hidden to the error icon
The header refresh button correctly uses disabled={isRefreshing} (line 203), but the retry button in the error card (line 244) lacks this flag, which could allow concurrent invocations if both are visible. Additionally, the header icon has aria-hidden="true" (line 174), but the error card icon (line 230) is rendered without it—consider adding the same treatment if it's meant to be decorative.
🤖 Prompt for AI Agents
In apps/dashboard/app/(main)/websites/[id]/_components/website-page-header.tsx
around lines 154-155 and referencing the error card area (lines ~230-244), the
retry button in the error card is missing disabled={isRefreshing} (allowing
concurrent invocations when the header refresh button already disables during
refresh) and the error card icon lacks aria-hidden="true" (inconsistent with the
decorative header icon). Update the retry button JSX to pass
disabled={isRefreshing} and add aria-hidden="true" to the error card icon
element so both controls behave/accessibly consistent with the header.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/basket/src/lib/event-service.ts (1)
579-586: Remove duplicate and contradictory JSDoc comments.Two consecutive JSDoc blocks describe the same function with contradictory implementations:
- First block (lines 579–581): "Transforms and groups metrics before storing"
- Second block (lines 583–586): "Each metric is stored as a separate row - no aggregation"
The function code creates individual span rows without grouping, so the first description is stale. Remove the first comment block and keep only the second accurate one.
-/** - * Insert individual vital metrics (v2.x format) - * Transforms and groups metrics before storing - */ /** * Insert individual vital metrics (v2.x format) as spans * Each metric is stored as a separate row - no aggregation */apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx (2)
63-66: XSS vulnerability in error fallback.The fallback path directly interpolates
codeinto HTML without escaping. IfcodeToHtmlfails andcodecontains malicious content like<script>alert('xss')</script>, it will be rendered as executable HTML.Apply this diff to escape the code content in the fallback:
} catch (error) { console.error("Error highlighting code:", error); - setHighlightedCode(`<pre><code>${code}</code></pre>`); + const escaped = code + .replace(/&/g, "&") + .replace(/</g, "<") + .replace(/>/g, ">"); + setHighlightedCode(`<pre><code>${escaped}</code></pre>`); }
205-216: Bug: Double toggle on Switch click.Clicking the
Switchwill trigger both itsonCheckedChangeand the parentdiv'sonClick, causing the option to toggle twice (effectively no change).Stop event propagation when the switch is clicked:
- <Switch checked={isEnabled} onCheckedChange={onToggle} /> + <Switch + checked={isEnabled} + onCheckedChange={onToggle} + onClick={(e) => e.stopPropagation()} + />apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (1)
173-186: Unreachable code block.This fallback is dead code. When
!usageData?.eventTypeBreakdown?.lengthis true, the function returns early at lines 83–91. Otherwise,sortedBreakdownis derived fromeventTypeBreakdownand will never be empty at this point.Remove the unreachable block:
- {sortedBreakdown.length === 0 && ( - <div className="py-12 text-center"> - <TableIcon - className="mx-auto mb-4 size-12 text-muted-foreground" - weight="duotone" - /> - <h3 className="font-semibold text-foreground text-lg"> - No Event Data - </h3> - <p className="mt-1 text-muted-foreground text-sm"> - No events found for the selected period - </p> - </div> - )}apps/api/src/lib/clickhouse-backup.ts (1)
218-228: Consider using parameterized query for consistency.While
backupNameis internally generated and numeric values are safe, using parameterized queries (like ingetBackupMetadata) would be more consistent and robust against future refactoring.
♻️ Duplicate comments (46)
apps/api/src/query/batch-executor.ts (1)
123-136: Unresolved: Silent data loss insplitResults(flagged in past review).A prior review flagged that line 132 silently discards rows with a
__query_idxnot present in theindicesmap. This issue remains unresolved in the current diff—the optional chainingbyIndex.get(__query_idx)?.push(rest)will still drop unexpected indices without warning or error.While this should not occur if the union query is correctly constructed, any data flow mismatch (e.g., a builder bug, schema drift, or edge case) would cause undetected data loss.
Please confirm:
- Whether this was intentionally deferred and is tracked as a separate issue.
- Or, if you'd like me to propose a fix (e.g., warn on unexpected indices or throw an error for early detection).
If this is critical for data integrity, consider adding validation:
for (const { __query_idx, ...rest } of rows) { - byIndex.get(__query_idx)?.push(rest); + const bucket = byIndex.get(__query_idx); + if (bucket) { + bucket.push(rest); + } else { + console.warn(`Unexpected __query_idx: ${__query_idx}`, rest); + } }apps/dashboard/app/(main)/organizations/settings/danger/transfer-assets.tsx (2)
171-171: Spacing inconsistency resolved.The past review comment flagging inconsistent spacing between Personal (
space-y-2) and Organization (space-y-1) lists has been addressed. Both now usespace-y-2.
227-227: Spacing now consistent.Organization list spacing now matches Personal list spacing (
space-y-2), completing the fix for the previously flagged inconsistency.apps/dashboard/app/(main)/organizations/invitations/invitation-list.tsx (2)
23-26: DeriveInvitationToCancelfromInvitationto avoid driftRather than duplicating
id/Invitationso it stays in sync if the source changes:-type InvitationToCancel = { - id: string; - email: string; -}; +type InvitationToCancel = Pick<Invitation, "id" | "email">;
43-60: Strongly typestatusConfigby invitation status and avoidasassertionYou can key
statusConfigonInvitation["status"]to remove the type assertion and get compile‑time coverage if new statuses are added:- const statusConfig = { + const statusConfig: Record< + Invitation["status"], + { label: string; className: string } + > = { pending: { label: "Pending", className: "border-amber-500/20 bg-amber-500/10 text-amber-600", }, accepted: { label: "Accepted", className: "border-green-500/20 bg-green-500/10 text-green-600", }, expired: { label: "Expired", className: "border-muted bg-muted text-muted-foreground", }, }; - const status = - statusConfig[invitation.status as keyof typeof statusConfig] ?? - statusConfig.expired; + const status = + statusConfig[invitation.status] ?? statusConfig.expired;If the backend can emit additional statuses (e.g. cancelled/revoked), consider adding explicit entries here and adjusting the
"Expires"/"Expired"subtitle (lines 71–74) so badges and copy stay consistent.Also applies to: 71-74
apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx (1)
76-92: Suspense boundaries are ineffective withuseQuery.This was flagged in a previous review. Since data fetching uses
useQuery(notuseSuspenseQuery) and both child components handle their own loading states viaisLoadingprop, these Suspense boundaries will never trigger their fallbacks.Either:
- Remove the Suspense wrappers (simpler, since internal loading states are sufficient), or
- Refactor to use
useSuspenseQuerywithin the child componentsapps/dashboard/app/(main)/organizations/settings/danger/danger-zone-settings.tsx (1)
158-158: Inconsistent icon spacing (still unresolved).As noted in a previous review, the
SignOutIconis missing themr-2className that theTrashIcon(line 149) has, resulting in inconsistent button layouts.Apply this diff:
- <SignOutIcon size={14} /> + <SignOutIcon className="mr-2" size={14} />apps/dashboard/app/(main)/billing/utils/feature-usage.ts (1)
98-109: Resolve:formatCompactNumberstill doesn't handleInfinityinput.This issue was flagged in a previous review and remains unresolved. If
Number.POSITIVE_INFINITYis passed to this function (which can occur whencalculateFeatureUsagesetslimittoPOSITIVE_INFINITYfor unlimited features), the function will return"InfinityB"instead of a user-friendly representation.Apply this diff to add an early guard:
export function formatCompactNumber(num: number): string { + if (!Number.isFinite(num)) { + return "∞"; + } if (num >= 1_000_000_000) { return `${(num / 1_000_000_000).toFixed(1)}B`; }apps/dashboard/app/(auth)/login/page.tsx (1)
157-170: The disabled state does not prevent Link navigation.When using
asChild, the Button component merges its props with the Link child. However, thedisabledattribute is not valid on<a>elements, so while the button may appear disabled visually, the Link remains fully clickable and keyboard-navigable during the loading state.Apply this diff to fix the issue by removing the Link and using router navigation:
- <div className="relative lg:col-span-2"> - <Button - asChild - className="w-full" - disabled={isLoading} - size="lg" - type="button" - variant="secondary" - > - <Link href="/login/magic"> - <SparkleIcon className="size-4" /> - Sign in with Magic Link - </Link> - </Button> + <div className="relative lg:col-span-2"> + <Button + className="w-full" + disabled={isLoading} + onClick={() => !isLoading && router.push('/login/magic')} + size="lg" + type="button" + variant="secondary" + > + <SparkleIcon className="size-4" /> + Sign in with Magic Link + </Button> {lastUsed === "magic" && (apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx (3)
39-53:filtersMatchis order-sensitive.This comparison requires arrays to match in exact order. If filter application doesn't guarantee order preservation, consider normalizing (sorting by field/operator/value) before comparison.
35-37: DuplicategetFieldLabelhelper across filter components.This helper is duplicated in
saved-filters-menu.tsx,filters-section.tsx, andsave-filter-dialog.tsx. Consider extracting to a shared utility alongsidegetOperatorLabelin@/hooks/use-filters.
90-103: Consider closing menu when "Clear all" is clicked.The "Clear all" button triggers a confirmation dialog via
onDeleteAll()but leaves the dropdown open. Closing the menu would provide cleaner UX.apps/dashboard/app/(main)/websites/[id]/_components/filters/filters-section.tsx (4)
63-71: Removing a filter by value removes all identical entries.
handleRemoveFilterlooks up the filter by index butremoveFilter(filter)removes all entries matching that(field, operator, value)triple. If duplicate filters are possible, consider exposing a remove-by-index atom instead.
75-91: Saved filter mutations treated as synchronous.
saveFilter,updateFilter, etc. are used as if they return synchronous{ success: boolean }. If these become async,isSavingwon't correctly track in-flight state. Consider usingasync/awaitwith try/finally to properly manage loading states.
112-119: Delete handlers also assume synchronous mutations.Same concern applies to
handleConfirmDeleteandhandleConfirmDeleteAll. ThesetIsDeleting(false)calls happen immediately, not after the mutation completes.Also applies to: 160-165
20-22: DuplicategetFieldLabelhelper.Same helper exists in
saved-filters-menu.tsxandsave-filter-dialog.tsx. Extract to a shared utility.apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx (4)
57-61: Whitespace-only values can bypass validation.The schema allows whitespace-only strings like
" "to pass.min(1), butonSubmittrims before sending. Apply.trim()at the schema level to validate the trimmed value:const filterFormSchema = z.object({ field: z.string().min(1, "Please select a field"), operator: z.enum(["eq", "ne", "contains", "not_contains", "starts_with"]), - value: z.string().min(1, "Value is required"), + value: z.string().trim().min(1, "Value is required"), });Then remove the
.trim()call inonSubmit(line 181).
87-132: Duplicate suggestions may cause React key warnings.If
autocompleteDatacontains duplicates, buttons will have identicalkey={suggestion}values. Deduplicate before rendering:+const uniqueSuggestions = Array.from(new Set(suggestions)); const filteredSuggestions = searchValue.trim() - ? suggestions + ? uniqueSuggestions .filter((s) => s.toLowerCase().includes(searchValue.toLowerCase())) .slice(0, MAX_SUGGESTIONS) - : suggestions.slice(0, MAX_SUGGESTIONS); + : uniqueSuggestions.slice(0, MAX_SUGGESTIONS);
188-219: Error state blocks filter creation entirely.When
isErroris true, users cannot add filters at all. Consider falling back to the normal flow with empty suggestions and a non-blocking warning banner, so users can still create filters manually.
307-352: Nested FormField structure is unconventional.The operator
FormFieldis nested inside the valueFormField's render prop. While functional, consider moving the operator field to a sibling position for clearer component structure.apps/dashboard/app/(main)/organizations/settings/websites/website-settings.tsx (3)
110-133: LGTM!Clean early return pattern for loading and error states. The
isEmptyvariable improves readability.Note: A previous review suggested extracting the inline props type to a shared interface if reused elsewhere.
141-146: Verify EmptyStateiconprop type.The external
EmptyStatecomponent destructuresicon: Iconand renders it as<Icon className="..." size={48} weight="duotone" />, which expects a component reference. However, the code passes a JSX element<GlobeIcon weight="duotone" />.If the imported
@/components/empty-statehas the same signature, this would cause a runtime error. Verify the expected prop type matches the usage.Additionally,
variant="minimal"was flagged in a previous review as potentially invalid—please confirm the component supports this variant.#!/bin/bash # Verify EmptyState component signature and variant support cat $(fd -t f 'empty-state.tsx' apps/dashboard/components --max-depth 1) 2>/dev/null || echo "File not found in expected location" # Check if there's an EmptyStateProps type that clarifies icon type rg -A 10 "interface EmptyStateProps" apps/dashboard/components/
156-171: LGTM!Good use of
RightSidebarcompound components and conditional rendering.Note: A previous review suggested extracting the inline pluralization logic on line 164 to a reusable helper.
apps/dashboard/app/(main)/websites/[id]/_components/filters/save-filter-dialog.tsx (3)
28-36: Normalize and validate the name on the trimmed value via Zod.trim()Right now the schema validates the raw string length, but
onSubmittrims before using the value. This means whitespace‑padded (or whitespace‑only) names can appear valid to the form (isValid) while submitting a shorter trimmed value than the min length implies.You can push normalization into the schema and keep the rest of the code simpler:
-const formSchema = z.object({ - name: z - .string() - .min(2, "Name must be at least 2 characters") - .max(100, "Name is too long"), -}); +const formSchema = z.object({ + name: z + .string() + .trim() + .min(2, "Name must be at least 2 characters") + .max(100, "Name is too long"), +});Then
data.nameinonSubmitis already trimmed, so the extra.trim()becomes unnecessary (can be removed or left as a no‑op):-const onSubmit = (data: FormData) => { - const trimmed = data.name.trim(); +const onSubmit = (data: FormData) => { + const trimmed = data.name;Also applies to: 85-87
37-39: ExtractgetFieldLabelinto a shared helper to avoid duplicationThis helper is (per earlier reviews) duplicated in other filter‑related components. Consider moving it to a shared utility (e.g., a small
filterHelpers.tsin a common UI or hooks folder) and importing it here and in the other call sites:// shared export function getFieldLabel( field: string, options = filterOptions, ): string { return options.find((o) => o.value === field)?.label ?? field; }Then in this file you can import
getFieldLabelinstead of redefining it.
80-83: HandleDialog’sonOpenChangeboolean instead of always treating it as “close”
handleCloseignores theopenboolean passed by the Dialog’sonOpenChange, so any future attempt to open the dialog via that callback would immediately trigger a reset/close as well. Safer to only run the close/reset path whenopen === false:-const handleClose = () => { - form.reset(); - onClose(); -}; +const handleClose = () => { + form.reset(); + onClose(); +}; ... - <Dialog onOpenChange={handleClose} open={isOpen}> + <Dialog + open={isOpen} + onOpenChange={(open) => { + if (!open) { + handleClose(); + } + }} + >Cancel can still call
handleClosedirectly, but Dialog’s internal triggers and overlay/ESC behavior will now distinguish between open and close events correctly.Also applies to: 101-103
apps/api/src/lib/clickhouse-backup.ts (4)
178-184: SQL injection risk persists in INSERT statement.This issue was previously flagged. While the values are internally generated, using parameterized queries (as demonstrated in
getBackupMetadata) would be more robust and consistent.
240-249: SQL injection and silent error handling issues persist.These issues were previously flagged:
- The error message escaping is insufficient for SQL safety
- The empty catch block silently swallows failures
Apply the parameterized query pattern and add error logging as suggested in prior reviews.
327-357:retentionDaysvalidation issue persists.This was previously flagged. The parameter is interpolated directly into SQL without validation.
405-417: SQL injection risks persist;s3_use_virtual_addressingaddition is good.The input validation concerns for
backupPath,database, andtableswere previously flagged and remain unaddressed. The addition ofs3_use_virtual_addressing = falseis appropriate for path-style S3 URLs.apps/dashboard/app/(auth)/layout.tsx (1)
33-41: Fix Tailwind!importantsyntax and avoid nestingButtoninsideLink.
px-0!is invalid Tailwind syntax; it should be!px-0(or drop the!if not needed).- Also, wrapping a
Button(likely rendering a<button>) directly insideLink(which renders an<a>) produces invalid<a><button>…</button></a>markup. Prefer styling theLinkas a button, or using aButtonthat renders theLinkas its child.Example refactor if your
ButtonsupportsasChild:- <Link className="relative z-10" href="https://www.databuddy.cc"> - <Button - className="group px-0! text-white/50 hover:bg-transparent hover:text-white/80" - variant="ghost" - > - <CaretLeftIcon className="size-4 transition-transform duration-200 group-hover:translate-x-[-4px]" /> - Back - </Button> - </Link> + <Button + asChild + className="relative z-10 group !px-0 text-white/50 hover:bg-transparent hover:text-white/80" + variant="ghost" + > + <Link href="https://www.databuddy.cc"> + <CaretLeftIcon className="size-4 transition-transform duration-200 group-hover:translate-x-[-4px]" /> + Back + </Link> + </Button>apps/dashboard/app/(main)/organizations/components/general-settings.tsx (2)
19-19: Normalize nullableorganization.slugto a string and trim once to avoid runtime errors and stale “unsaved changes”.
organization.slugis nullable at the DB level; if it comes through asnull, initializing state with it and later callingslug.trim()will throw at runtime, and passingnullto<Input value>is also problematic. You can also simplify change detection by normalizing once:- const [slug, setSlug] = useState(organization.slug); + const [slug, setSlug] = useState(organization.slug ?? ""); - const hasChanges = name !== organization.name || slug !== organization.slug; + const normalizedOrgSlug = organization.slug ?? ""; + const hasChanges = name !== organization.name || slug !== normalizedOrgSlug; const handleSave = async () => { - if (!name.trim()) { + const trimmedName = name.trim(); + const trimmedSlug = slug.trim(); + + if (!trimmedName) { toast.error("Name is required"); return; } - if (!slug.trim()) { + if (!trimmedSlug) { toast.error("Slug is required"); return; } @@ - await updateOrganizationAsync({ - organizationId: organization.id, - data: { name: name.trim(), slug: slug.trim() }, - }); + await updateOrganizationAsync({ + organizationId: organization.id, + data: { name: trimmedName, slug: trimmedSlug }, + }); + // Keep local state in sync with what we actually saved + setName(trimmedName); + setSlug(trimmedSlug);This guarantees
slugis always a string, prevents.trim()from ever running onnull, keeps the Input happy, and ensures the “unsaved changes” footer clears once the server-providedorganizationprop reflects the saved values.Also applies to: 35-35, 37-45, 47-52, 104-105
107-110: Avoid/nullURLs in previews and prefer the cleaned slug state for sidebar/URL display.Using
organization.slugdirectly can render/nullfor legacy data and doesn’t reflect in‑progress edits; you already have the cleanedslugstate:- <p className="text-muted-foreground text-xs"> - Used in URLs:{" "} - <code className="rounded bg-muted px-1 py-0.5 text-xs"> - /{slug} - </code> - </p> + <p className="text-muted-foreground text-xs"> + Used in URLs:{" "} + <code className="rounded bg-muted px-1 py-0.5 text-xs"> + /{slug || ""} + </code> + </p> @@ - <RightSidebar.InfoCard - description={`/${organization.slug}`} - icon={BuildingsIcon} - title={organization.name} - /> + const effectiveSlug = slug || organization.slug || ""; + + <RightSidebar.InfoCard + description={effectiveSlug ? `/${effectiveSlug}` : "/"} + icon={BuildingsIcon} + title={organization.name} + />This prevents
/nullfrom ever showing and makes the sidebar URL preview track the currently edited, cleaned slug.Also applies to: 141-149
apps/dashboard/app/(main)/websites/[id]/_components/website-page-header.tsx (2)
229-230: Addaria-hiddento the error card icon for consistency.The header icon at line 174 includes
aria-hidden="true", but the error card icon at line 230 doesn't have this attribute. For consistency with decorative icons, add the same treatment.This was flagged in a previous review.
<div className="rounded-full border border-destructive/10 bg-destructive/5 p-3"> - {icon} + {cloneElement(icon, { + ...icon.props, + "aria-hidden": "true", + })} </div>
241-250: Adddisabled={isRefreshing}to the error card retry button.The header refresh button at line 203 correctly uses
disabled={isRefreshing}, but the retry button in the error card lacks this flag. This could allow concurrent refresh invocations if both buttons are visible.This was flagged in a previous review.
<Button className="cursor-pointer select-none gap-2 rounded transition-all duration-300 hover:border-primary/20 hover:bg-primary/10" + disabled={isRefreshing} onClick={onRefreshAction} size="sm" variant="outline" > + <ArrowClockwiseIcon className={isRefreshing ? "animate-spin" : ""} size={16} /> - <ArrowClockwiseIcon className="h-4 w-4" size={16} /> Retry </Button>apps/api/src/query/expressions.ts (3)
742-748: The source encoding convention for quantile remains fragile.The pattern of encoding
sourceas"level)(column"(e.g.,"0.50)(metric_value") to produce correct SQL is non-obvious and error-prone. This was flagged in a previous review.Consider extending
FieldDefinitionTypeto support a dedicatedlevelfield for quantile aggregates, or add validation to detect malformed quantile sources.
781-783: Quantile fallback uses invalid ClickHouse syntax.The fallback
quantile(0.50)(*)on line 783 is invalid. ClickHouse quantile functions require a specific numeric column expression—*is not valid. Either provide a sensible default column or throw an error whensourceis missing.This issue was flagged in a previous review and remains unaddressed.
- return source ? `quantile(${source})` : "quantile(0.50)(*)"; + if (!source) { + throw new Error("quantile aggregate requires a source column"); + } + return `quantile(${source})`;
799-800: Same invalid quantile fallback issue for quantileIf.Line 800 has the same invalid
quantile(0.50)(*)fallback when source is missing.- return source ? `quantile(${source})` : "quantile(0.50)(*)"; + if (!source) { + throw new Error("quantileIf aggregate requires a source column"); + } + return `quantile(${source})`;apps/api/src/query/builders/profiles.ts (1)
213-213: LGTM for this line, but inconsistency remains in other queries.The updated calculation correctly filters unique pages to only count
screen_viewevents. However, as noted in a previous review, the same metric is calculated differently at:
- Line 30 (
visitor_profilesCTE):COUNT(DISTINCT path)— no filter- Line 56 (
visitor_sessionsCTE):COUNT(DISTINCT e.path)— no filterThis inconsistency will cause different values for the same conceptual metric across API responses.
Apply these fixes for consistency:
Line 30:
- COUNT(DISTINCT path) as unique_pages, + COUNT(DISTINCT CASE WHEN event_name = 'screen_view' THEN path ELSE NULL END) as unique_pages,Line 56:
- COUNT(DISTINCT e.path) as unique_pages, + COUNT(DISTINCT CASE WHEN e.event_name = 'screen_view' THEN e.path ELSE NULL END) as unique_pages,apps/api/src/query/index.ts (1)
17-25: Operator set reduction already flagged.This breaking change (removal of
gt,gte,lt,lte,like,ilike,notLike,isNull,isNotNulland renamingnotIntonot_in) was noted in a prior review. Ensure API consumers are notified.apps/api/src/query/builders/sessions.ts (1)
161-161:generateUUIDv4()produces non-deterministic IDs on each query execution.This concern was raised in a prior review. The
custom_event_spanstable has noidcolumn, so generating a new UUID per query means the same custom event receives a different ID every time it's queried. If the frontend or any deduplication logic relies on stable event identifiers, this will break that expectation. Consider using a deterministic hash of event attributes or the client-providedeventIdif available.apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx (1)
565-572: Incomplete spinner border styling.This was flagged in a prior review. The spinner uses
border-primary border-b-2but doesn't specify a width for the non-animated borders, which may cause an invisible or inconsistent spinner appearance.Apply this diff:
- <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-primary border-b-2" /> + <div className="mx-auto mb-4 h-8 w-8 animate-spin rounded-full border-2 border-primary/30 border-t-primary" />apps/api/src/query/builders/custom-events.ts (1)
251-298: Missingcustomizable: trueflag oncustom_events_trends.This was flagged in a prior review. The
custom_events_trendsbuilder lacks thecustomizable: trueflag present on sibling builders, which may prevent clients from customizing this query type.Apply this diff:
custom_events_trends: { customSql: ( // ... function body unchanged ), timeField: "timestamp", allowedFilters: ["path", "event_name"], + customizable: true, },apps/dashboard/app/(main)/websites/[id]/_components/tabs/audience-tab.tsx (1)
219-230: Column definitions should be memoized to prevent unnecessary re-renders.The
displayNamesand column creators (createGeoColumns,createTimezoneColumns,createLanguageColumns) are called on every render, creating new array references. Since these are ingeographicTabs's dependency array (lines 296-301), the tabs will recalculate on every render.This was flagged in a previous review but remains unaddressed.
apps/api/src/query/builders/performance.ts (2)
262-272: LEFT JOIN with WHERE clause behaves as INNER JOIN.The
LEFT JOINon line 263 combined withWHERE e.country != ''on line 272 effectively filters out all rows where the join didn't match (sincee.countrywould be NULL). This makes it semantically equivalent to an INNER JOIN.If the intent is to exclude rows without country data, use explicit
INNER JOINfor clarity. If the intent is to include unmatched vitals with a fallback, move the filter into the JOIN condition.- LEFT JOIN ${Analytics.events} e ON ( + INNER JOIN ${Analytics.events} e ON ( wv.session_id = e.session_id AND wv.client_id = e.client_id AND abs(dateDiff('second', wv.timestamp, e.time)) < 60 + AND e.country != '' ) WHERE wv.client_id = {websiteId:String} AND wv.timestamp >= toDateTime({startDate:String}) AND wv.timestamp <= toDateTime(concat({endDate:String}, ' 23:59:59')) - AND e.country != ''
164-181: GROUP BY mismatch causes incorrect path aggregation.The SELECT uses a normalized path expression that removes trailing slashes and decodes URL components, but GROUP BY uses raw
path. This causes/aboutand/about/to be grouped separately while displaying the same normalized name, resulting in duplicate rows.Apply this diff to fix the GROUP BY:
- GROUP BY path + GROUP BY decodeURLComponent(CASE WHEN trimRight(path(path), '/') = '' THEN '/' ELSE trimRight(path(path), '/') END)Or use an alias approach:
- decodeURLComponent(CASE WHEN trimRight(path(path), '/') = '' THEN '/' ELSE trimRight(path(path), '/') END) as name, + decodeURLComponent(CASE WHEN trimRight(path(path), '/') = '' THEN '/' ELSE trimRight(path(path), '/') END) as page, ... - GROUP BY path + GROUP BY page ... - COUNT(*) as measurements + COUNT(*) as measurements, + page as name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (44)
apps/api/src/index.ts(1 hunks)apps/api/src/lib/api-key.test.ts(1 hunks)apps/api/src/lib/clickhouse-backup.ts(6 hunks)apps/api/src/query/batch-executor.ts(1 hunks)apps/api/src/query/builders/custom-events.ts(10 hunks)apps/api/src/query/builders/engagement.ts(1 hunks)apps/api/src/query/builders/performance.ts(2 hunks)apps/api/src/query/builders/profiles.ts(5 hunks)apps/api/src/query/builders/sessions.ts(3 hunks)apps/api/src/query/expressions.ts(11 hunks)apps/api/src/query/index.ts(2 hunks)apps/api/src/query/screen-resolution-to-device-type.ts(2 hunks)apps/api/src/query/simple-builder.ts(11 hunks)apps/api/src/query/types.ts(2 hunks)apps/api/src/query/utils.ts(7 hunks)apps/api/src/routes/query.ts(8 hunks)apps/api/src/schemas/query-schemas.ts(2 hunks)apps/api/src/types/tables.ts(1 hunks)apps/basket/src/lib/event-service.ts(4 hunks)apps/basket/src/lib/producer.ts(2 hunks)apps/dashboard/app/(auth)/layout.tsx(1 hunks)apps/dashboard/app/(auth)/login/page.tsx(4 hunks)apps/dashboard/app/(main)/billing/components/usage-row.tsx(1 hunks)apps/dashboard/app/(main)/billing/cost-breakdown/components/consumption-chart.tsx(11 hunks)apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx(4 hunks)apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx(1 hunks)apps/dashboard/app/(main)/billing/hooks/use-billing.ts(1 hunks)apps/dashboard/app/(main)/billing/types/billing.ts(1 hunks)apps/dashboard/app/(main)/billing/utils/feature-usage.ts(1 hunks)apps/dashboard/app/(main)/organizations/components/general-settings.tsx(3 hunks)apps/dashboard/app/(main)/organizations/invitations/invitation-list.tsx(2 hunks)apps/dashboard/app/(main)/organizations/settings/danger/danger-zone-settings.tsx(8 hunks)apps/dashboard/app/(main)/organizations/settings/danger/transfer-assets.tsx(6 hunks)apps/dashboard/app/(main)/organizations/settings/websites/website-settings.tsx(5 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/filters-section.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/save-filter-dialog.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx(8 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/audience-tab.tsx(11 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx(6 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx(3 hunks)apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance/_components/web-vitals-chart.tsx(3 hunks)apps/dashboard/app/(main)/websites/[id]/_components/website-page-header.tsx(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (26)
apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx (6)
packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)apps/dashboard/hooks/use-funnels.ts (2)
AutocompleteData(68-78)useAutocompleteData(364-372)apps/dashboard/lib/utils.ts (1)
cn(5-7)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)apps/dashboard/components/ui/skeleton.tsx (1)
Skeleton(13-13)apps/dashboard/hooks/use-filters.ts (1)
operatorOptions(4-10)
apps/dashboard/app/(main)/billing/cost-breakdown/page.tsx (3)
apps/dashboard/components/ui/skeleton.tsx (1)
Skeleton(13-13)apps/dashboard/app/(main)/billing/cost-breakdown/components/consumption-chart.tsx (1)
ConsumptionChart(41-398)apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (1)
UsageBreakdownTable(56-189)
apps/dashboard/app/(main)/billing/hooks/use-billing.ts (1)
apps/dashboard/app/(main)/billing/utils/feature-usage.ts (1)
calculateFeatureUsage(56-96)
apps/dashboard/app/(main)/websites/[id]/_components/filters/save-filter-dialog.tsx (3)
packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)apps/dashboard/hooks/use-filters.ts (1)
getOperatorLabel(33-35)
apps/api/src/query/builders/sessions.ts (1)
apps/api/src/types/tables.ts (1)
Analytics(13-23)
apps/dashboard/app/(main)/billing/cost-breakdown/components/usage-breakdown-table.tsx (4)
apps/dashboard/components/ui/skeleton.tsx (1)
Skeleton(13-13)apps/dashboard/components/empty-state.tsx (1)
EmptyState(43-260)apps/dashboard/app/(main)/billing/cost-breakdown/utils/billing-utils.ts (1)
calculateOverageCost(9-25)apps/dashboard/components/ui/badge.tsx (1)
Badge(54-54)
apps/api/src/lib/clickhouse-backup.ts (2)
packages/env/src/api.ts (1)
env(31-33)packages/db/src/clickhouse/client.ts (1)
clickHouseOG(37-40)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance-tab.tsx (2)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance/_components/performance-summary-card.tsx (1)
PerformanceSummaryCard(92-166)apps/dashboard/components/table/data-table.tsx (1)
DataTable(83-236)
apps/api/src/query/simple-builder.ts (1)
apps/api/src/query/screen-resolution-to-device-type.ts (1)
DeviceType(1-8)
apps/dashboard/app/(main)/organizations/settings/websites/website-settings.tsx (3)
apps/dashboard/hooks/use-organizations.ts (1)
Organization(457-459)apps/dashboard/app/(main)/organizations/components/empty-state.tsx (1)
EmptyState(17-62)apps/dashboard/components/right-sidebar.tsx (1)
RightSidebar(16-27)
apps/api/src/query/utils.ts (2)
apps/api/src/query/types.ts (1)
SimpleQueryConfig(174-228)apps/api/src/query/screen-resolution-to-device-type.ts (1)
mapScreenResolutionToDeviceType(102-107)
apps/api/src/query/builders/custom-events.ts (2)
apps/api/src/types/tables.ts (1)
Analytics(13-23)apps/api/src/query/types.ts (2)
Filter(41-49)TimeUnit(35-35)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx (4)
apps/dashboard/hooks/use-chart-preferences.ts (1)
useChartPreferences(99-118)apps/dashboard/components/analytics/index.tsx (1)
EventLimitIndicator(5-5)apps/dashboard/lib/formatters.ts (1)
formatNumber(40-46)packages/db/src/drizzle/schema.ts (1)
chartType(631-631)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/performance/_components/web-vitals-chart.tsx (1)
apps/dashboard/components/table/data-table.tsx (1)
DataTable(83-236)
apps/dashboard/app/(main)/billing/types/billing.ts (1)
apps/dashboard/app/(main)/billing/hooks/use-billing.ts (1)
CustomerWithPaymentMethod(21-21)
apps/dashboard/app/(main)/organizations/settings/danger/danger-zone-settings.tsx (3)
apps/dashboard/app/(main)/websites/_components/notice-banner.tsx (1)
NoticeBanner(5-52)apps/dashboard/components/right-sidebar.tsx (1)
RightSidebar(16-27)apps/dashboard/components/ui/delete-dialog.tsx (1)
DeleteDialog(29-113)
apps/dashboard/app/(main)/billing/cost-breakdown/components/consumption-chart.tsx (3)
apps/dashboard/components/empty-state.tsx (1)
EmptyState(43-260)apps/dashboard/components/ui/button.tsx (1)
Button(59-59)apps/dashboard/lib/utils.ts (1)
cn(5-7)
apps/api/src/query/index.ts (3)
apps/api/src/query/types.ts (1)
QueryRequest(230-242)apps/api/src/query/builders/index.ts (1)
QueryBuilders(15-29)apps/api/src/query/simple-builder.ts (1)
SimpleQueryBuilder(206-715)
apps/api/src/query/builders/performance.ts (2)
apps/api/src/query/types.ts (2)
Filter(41-49)TimeUnit(35-35)apps/api/src/types/tables.ts (1)
Analytics(13-23)
apps/dashboard/app/(main)/billing/utils/feature-usage.ts (1)
apps/dashboard/app/(main)/billing/cost-breakdown/utils/billing-utils.ts (1)
calculateOverageCost(9-25)
apps/dashboard/app/(main)/websites/[id]/_components/filters/saved-filters-menu.tsx (4)
apps/dashboard/hooks/use-saved-filters.ts (1)
SavedFilter(8-14)packages/shared/src/types/api.ts (1)
DynamicQueryFilter(19-30)packages/shared/src/lists/filters.ts (1)
filterOptions(1-12)apps/dashboard/hooks/use-filters.ts (1)
getOperatorLabel(33-35)
apps/api/src/lib/api-key.test.ts (1)
apps/api/src/lib/api-key.ts (13)
ApiKeyRow(7-7)isApiKeyPresent(28-32)extractSecret(34-46)getEffectiveScopes(84-92)hasKeyScope(94-103)hasKeyAnyScope(105-114)hasKeyAllScopes(116-125)resolveEffectiveScopesForWebsite(127-132)hasWebsiteScope(134-140)hasWebsiteAnyScope(142-148)hasWebsiteAllScopes(150-156)hasGlobalAccess(158-164)getAccessibleWebsiteIds(166-177)
apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx (1)
apps/dashboard/app/(main)/websites/_components/notice-banner.tsx (1)
NoticeBanner(5-52)
apps/dashboard/app/(main)/organizations/components/general-settings.tsx (4)
apps/dashboard/hooks/use-organizations.ts (1)
Organization(457-459)apps/dashboard/app/(main)/organizations/components/organization-logo-uploader.tsx (1)
OrganizationLogoUploader(32-272)packages/db/src/drizzle/schema.ts (1)
organization(531-542)apps/dashboard/components/right-sidebar.tsx (1)
RightSidebar(16-27)
apps/dashboard/app/(main)/websites/[id]/_components/website-page-header.tsx (2)
apps/dashboard/components/ui/skeleton.tsx (1)
Skeleton(13-13)apps/dashboard/lib/utils.ts (1)
cn(5-7)
apps/api/src/query/expressions.ts (1)
apps/api/src/query/types.ts (1)
TimeUnit(35-35)
🪛 ast-grep (0.40.0)
apps/dashboard/app/(main)/websites/[id]/_components/shared/tracking-components.tsx
[warning] 104-104: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Vercel Agent Review
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging errors before returning null.
While returning null on error is a safe fallback (forces full backup), adding a debug/warn log would help diagnose issues with the backup chain lookup.
- } catch {
- return null;
+ } catch (error) {
+ logger.warn({ error }, "Failed to get last backup path, falling back to full backup");
+ return null;
}📝 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.
| } catch { | |
| return null; | |
| } | |
| } catch (error) { | |
| logger.warn({ error }, "Failed to get last backup path, falling back to full backup"); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/lib/clickhouse-backup.ts around lines 77 to 79, the catch block
currently swallows errors and returns null; change it to log the caught error
(at debug or warn level) before returning null so failures in the backup chain
lookup are recorded for diagnostics. Capture the exception in the catch clause,
call the module's logger (or inject/require one if missing) to log a clear
message including the error stack/details and context, then return null as
before.
| const baseBackupSetting = isIncremental | ||
| ? `,base_backup = S3('${lastBackupPath}', '${env.R2_ACCESS_KEY_ID}', '${env.R2_SECRET_ACCESS_KEY}')` | ||
| : ""; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the getLastBackupPath function
rg -n "getLastBackupPath" --type ts -A 10Repository: databuddy-analytics/Databuddy
Length of output: 1747
🏁 Script executed:
#!/bin/bash
# Check for S3 URL validation patterns
rg -n "s3.*url.*validat|backup.*path.*validat|validateUrl|URL.*validat" --type ts -iRepository: databuddy-analytics/Databuddy
Length of output: 1247
🏁 Script executed:
#!/bin/bash
# Check where lastBackupPath is used and how it's constructed
rg -n "lastBackupPath" --type ts -B 2 -A 2Repository: databuddy-analytics/Databuddy
Length of output: 1731
🏁 Script executed:
#!/bin/bash
# Get more context around the backup insertion and see what format backup_path should be
cat -n apps/api/src/lib/clickhouse-backup.ts | sed -n '165,210p'Repository: databuddy-analytics/Databuddy
Length of output: 2186
🏁 Script executed:
#!/bin/bash
# Check if there are any validation or sanitization functions used for the SQL queries in this file
rg -n "escape|sanitize|parameterize|prepared" apps/api/src/lib/clickhouse-backup.tsRepository: databuddy-analytics/Databuddy
Length of output: 186
SQL injection vulnerability: unescaped parameters in INSERT query at line 182.
The backup function interpolates backupName, s3Url, and lastBackupPath directly into the INSERT query without escaping. This allows injection through any of these parameters. While the original concern about lastBackupPath at line 188 is technically correct, the root cause is the unescaped INSERT at line 182. An attacker controlling backupName could inject a malicious backup_path into the database, which would then be used in the S3() call on subsequent backups. Use parameterized queries or proper SQL escaping for all parameters at line 182 and line 193.
| customSql: ( | ||
| websiteId: string, | ||
| startDate: string, | ||
| endDate: string, | ||
| _filters?: Filter[], | ||
| _granularity?: TimeUnit, | ||
| _limit?: number | ||
| ) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused _filters and _granularity parameters across multiple builders.
Multiple web vitals builders accept _filters?: Filter[] and _granularity?: TimeUnit parameters but never use them. The customSql queries don't apply any filter logic from these parameters.
Either remove the unused parameters or implement filter/granularity handling within the SQL generation.
Also applies to: 193-200, 239-246, 286-293, 332-339
🤖 Prompt for AI Agents
In apps/api/src/query/builders/performance.ts around lines 152-159 (and also
apply same change at 193-200, 239-246, 286-293, 332-339): the `_filters?:
Filter[]` and `_granularity?: TimeUnit` parameters are declared but never used
in the customSql builders; either remove these unused parameters from the
function signatures (and update any callers) or implement their handling inside
the SQL generation by applying filter conditions derived from `_filters` to the
WHERE clause and mapping `_granularity` to appropriate time-binning (e.g.,
date_trunc or GROUP BY intervals) so the generated SQL respects filters and
granularity. Ensure TypeScript types stay consistent and run tests/compile after
the change.
| FROM ${Analytics.web_vitals_spans} wv | ||
| LEFT JOIN ${Analytics.events} e ON ( | ||
| wv.session_id = e.session_id | ||
| AND wv.client_id = e.client_id | ||
| AND abs(dateDiff('second', wv.timestamp, e.time)) < 60 | ||
| ) | ||
| WHERE | ||
| wv.client_id = {websiteId:String} | ||
| AND wv.timestamp >= toDateTime({startDate:String}) | ||
| AND wv.timestamp <= toDateTime(concat({endDate:String}, ' 23:59:59')) | ||
| AND e.os_name != '' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same LEFT JOIN issue in web_vitals_by_os.
The LEFT JOIN with WHERE e.os_name != '' has the same semantic issue as web_vitals_by_country. Apply the same fix: either use INNER JOIN or move the filter to the JOIN condition.
🤖 Prompt for AI Agents
In apps/api/src/query/builders/performance.ts around lines 309 to 319, the LEFT
JOIN to Analytics.events combined with the WHERE clause filtering e.os_name !=
'' effectively turns the LEFT JOIN into an INNER JOIN and may drop rows
unexpectedly; fix this by either changing the LEFT JOIN to an INNER JOIN if you
intend to require a matching event, or keep the LEFT JOIN and move the e.os_name
!= '' predicate into the ON clause so that rows without matching events are
preserved (choose one approach and apply consistently).
| export function normalizeGranularity( | ||
| unit: TimeUnit | undefined | ||
| unit: TimeUnit | undefined | ||
| ): Granularity | undefined { | ||
| if (!unit) { | ||
| return; | ||
| } | ||
| if (unit === "hourly") { | ||
| return "hour"; | ||
| } | ||
| if (unit === "daily") { | ||
| return "day"; | ||
| } | ||
| return unit as Granularity; | ||
| if (!unit) { | ||
| return; | ||
| } | ||
| if (unit === "hourly") { | ||
| return "hour"; | ||
| } | ||
| if (unit === "daily") { | ||
| return "day"; | ||
| } | ||
| return unit as Granularity; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding exhaustive validation for TimeUnit.
The as Granularity cast at line 265 assumes all remaining TimeUnit values (after handling "hourly"/"daily") are valid Granularity values. If TimeGranularity contains keys that don't exist in Granularity, this could silently pass invalid values.
Consider adding runtime validation or using a type assertion function:
- return unit as Granularity;
+ const validGranularities: Granularity[] = ["minute", "hour", "day", "week", "month"];
+ if (validGranularities.includes(unit as Granularity)) {
+ return unit as Granularity;
+ }
+ return undefined;🤖 Prompt for AI Agents
In apps/api/src/query/expressions.ts around lines 253-266, the function uses a
blind cast "return unit as Granularity" which can silently accept TimeUnit
values that are not valid Granularity; replace the cast with runtime validation:
enumerate the allowed Granularity values (or use an exhaustive switch over
TimeUnit), map/translate any aliases ("hourly" -> "hour", "daily" -> "day"), and
only return the value if it exists in the Granularity set (otherwise return
undefined or throw a clear error). Ensure the validation is explicit so
non-matching TimeUnit values are rejected instead of being cast through.
| disabled={isDeleting} | ||
| onClick={handleDelete} | ||
| <DeleteDialog | ||
| confirmDisabled={confirmText !== organization.name} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ensure user guidance matches validation strictness.
The confirmDisabled validation (line 177) uses exact case-sensitive matching, but the guidance text (lines 204-206) doesn't emphasize this requirement. If you keep strict matching, consider updating the guidance text to clarify: "Type exactly {organization.name} to confirm deletion."
<p className="text-muted-foreground text-xs">
- Type <span className="font-medium">{organization.name}</span> to
- confirm deletion
+ Type <span className="font-medium">{organization.name}</span> exactly to
+ confirm deletion (case-sensitive)
</p>Also applies to: 203-206
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/organizations/settings/danger/danger-zone-settings.tsx
around line 177, the confirmDisabled check uses a case-sensitive exact match
(confirmText !== organization.name) but the guidance text shown to users (lines
~203-206) does not state that matching is exact; update the guidance copy to
explicitly instruct users to "Type exactly {organization.name} to confirm
deletion." and ensure any interpolation/escaping matches the current
organization.name variable and i18n approach used in the file so the message
clearly conveys the case-sensitive requirement.
| const { id } = useParams(); | ||
| const websiteId = id as string; |
There was a problem hiding this comment.
Unsafe type assertion for websiteId.
useParams() returns string | string[] | undefined for dynamic segments. The assertion id as string could fail if the route param is missing or an array.
const { id } = useParams();
-const websiteId = id as string;
+const websiteId = typeof id === "string" ? id : "";Consider also disabling the button or showing an error state when websiteId is invalid.
📝 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.
| const { id } = useParams(); | |
| const websiteId = id as string; | |
| const { id } = useParams(); | |
| const websiteId = typeof id === "string" ? id : ""; |
🤖 Prompt for AI Agents
In apps/dashboard/app/(main)/websites/[id]/_components/filters/add-filters.tsx
around lines 399-400, avoid the unsafe assertion `id as string`; validate the
param instead (check typeof id === 'string' and that it's not empty) and only
use a typed websiteId when valid; if id is undefined or an array, set websiteId
to null/undefined or derive an error state and update the UI to disable the
submit/add button and show a validation message or error banner; optionally
narrow useParams with a typed generic or helper that returns a safe string, but
the core fix is to replace the direct assertion with runtime validation and
guard UI/actions when websiteId is invalid.
| const { id } = useParams(); | ||
| const websiteId = id as string; |
There was a problem hiding this comment.
Unsafe type assertion for websiteId.
Same issue as in add-filters.tsx. useParams() returns string | string[] | undefined, and the assertion id as string could fail silently.
const { id } = useParams();
-const websiteId = id as string;
+const websiteId = typeof id === "string" ? id : "";🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/_components/filters/filters-section.tsx
around lines 37-38, the code unsafely asserts useParams().id to string; replace
the type assertion with a safe runtime check: if id is undefined, throw or
redirect; if Array, pick the relevant element (e.g., id[0]) or join if intended;
finally narrow the type so websiteId is a true string (or update the function
signature to accept undefined/string[]), ensuring you handle all cases instead
of using `as string`.
| const form = useForm<FormData>({ | ||
| resolver: zodResolver(formSchema), | ||
| defaultValues: { name: "" }, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reconsider using formState.isValid with default useForm mode for disabling “Save”
disabled={... || !form.formState.isValid} combined with the default useForm config (mode: "onSubmit") can keep the Save button disabled until after a submit cycle, which is a bit awkward UX for a simple single‑field form.
If you want real‑time validity gating, explicitly opt into change‑based validation:
-const form = useForm<FormData>({
- resolver: zodResolver(formSchema),
- defaultValues: { name: "" },
-});
+const form = useForm<FormData>({
+ resolver: zodResolver(formSchema),
+ defaultValues: { name: "" },
+ mode: "onChange",
+});That way formState.isValid tracks the Zod schema (with .trim() applied) as the user types, and the Save button enables as soon as the name becomes valid.
Also applies to: 188-190
| <DialogDescription className="text-muted-foreground text-xs"> | ||
| {isEditing | ||
| ? `Update the name for "${editingFilter?.name}"` | ||
| : `Save ${filters.length} filter${filters.length === 1 ? "" : "s"} for later`} | ||
| </DialogDescription> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten copy when there are no filters to avoid “Save 0 filters for later”
When filters.length === 0, the description still says Save 0 filters for later while the preview area shows “No filters applied”. Minor UX nit, but you could special‑case the description for the empty state to keep the messaging consistent, e.g.:
- <DialogDescription className="text-muted-foreground text-xs">
- {isEditing
- ? `Update the name for "${editingFilter?.name}"`
- : `Save ${filters.length} filter${filters.length === 1 ? "" : "s"} for later`}
- </DialogDescription>
+ <DialogDescription className="text-muted-foreground text-xs">
+ {isEditing
+ ? `Update the name for "${editingFilter?.name}"`
+ : filters.length === 0
+ ? "You don’t have any filters to save yet"
+ : `Save ${filters.length} filter${filters.length === 1 ? "" : "s"} for later`}
+ </DialogDescription>Also applies to: 123-153
| }; | ||
| }, | ||
| timeField: "timestamp", | ||
| customizable: true, |
There was a problem hiding this comment.
Bug: Web vitals queries ignore filters despite being customizable
The vitals_by_page and multiple web_vitals_* queries are marked with customizable: true but their customSql function signatures don't include the filterConditions and filterParams parameters that SimpleQueryBuilder.compile() passes. When users pass filters to these queries, the filters are silently ignored because the generated SQL never includes them. Compare with custom_events which properly accepts filterConditions?: string[] and filterParams?: Record<string, Filter["value"]> and uses them to build a combinedWhereClause applied to the SQL.
Additional Locations (1)
| "utm_source", | ||
| "utm_medium", | ||
| "utm_campaign", | ||
| ] as const; |
There was a problem hiding this comment.
Bug: Global filters allow columns missing from target tables
The new GLOBAL_ALLOWED_FILTERS list permits filtering on columns like country, device_type, browser_name, os_name, referrer, and utm_* even when querying tables that lack these columns. For example, the error_spans table only has client_id, anonymous_id, session_id, timestamp, path, message, filename, lineno, colno, stack, and error_type. Similarly, web_vitals_spans lacks these columns. When a user applies a globally-allowed filter like country to table-based queries against these tables (e.g., error_types, error_trends), the generated SQL will reference a non-existent column, causing a ClickHouse runtime error.
Additional Locations (1)
| AND timestamp >= toDateTime({startDate:String}) | ||
| AND timestamp <= toDateTime(concat({endDate:String}, ' 23:59:59')) | ||
| AND path != '' | ||
| GROUP BY path |
There was a problem hiding this comment.
Bug: GROUP BY mismatch causes incorrect page grouping
The web_vitals_by_page query has a mismatch between its SELECT and GROUP BY expressions. The SELECT transforms the path using decodeURLComponent(CASE WHEN trimRight(path(path), '/') = '' THEN '/' ELSE trimRight(path(path), '/') END) as name, but the GROUP BY uses just the raw path column. This means rows with different raw paths (e.g., /page?a=1 and /page?b=2) will be in separate groups but display the same transformed name (/page), causing duplicate entries with split metrics. The correct pattern, used in top_pages and vitals_by_page, is to GROUP BY the same transformed expression or its alias.
| "utm_source", | ||
| "utm_medium", | ||
| "utm_campaign", | ||
| ] as const; |
There was a problem hiding this comment.
Bug: Global filters allow columns missing from span tables
The new GLOBAL_ALLOWED_FILTERS list bypasses per-query allowedFilters restrictions, but several query builders use tables that don't have these columns. For example, error_types, error_trends, and similar queries use error_spans directly without joining events. The error_spans table lacks columns like browser_name, country, device_type, os_name, referrer, and UTM fields. When users filter these queries by a globally allowed column that doesn't exist in the underlying table, ClickHouse will return a column-not-found error at runtime.
Additional Locations (1)
3 participants
Pull Request
Description
Please include a summary of the change and which issue is fixed. Also include relevant motivation and context.
Checklist
Note
Major dashboard UI overhaul with new components and API key UX, adds active users and refined analytics (filters, funnels/goals), and updates SDK/tracker for better vitals/error handling.
components/ui/*(badges, buttons, tables, sheets, dialogs, tooltips, inputs, tabs, etc.).RightSidebar,ChartTooltip,DeleteDialog,LineSlider,SegmentedControl,KeyboardShortcuts,TableEmptyState.api-key-detail-dialog,api-key-list) and organizations provider touseQueries.ignoreHistoricData.contains,not_contains,starts_with,in,not_in) and mapping.2.3.0; refines script injection (excludes server-only props) and README.Written by Cursor Bugbot for commit 949003d. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.