chore: fix more linter issues#47
Conversation
…pe safety in Stripe webhook handling
|
@AyanavaKarmakar is attempting to deploy a commit to the Databuddy Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update consists of widespread stylistic and consistency changes across the codebase, primarily converting single-line conditional statements into explicit block statements with braces. Numerous unused imports and variables are removed or renamed, and some function or parameter names are adjusted to clarify intentional non-use. Several files receive improved type annotations or parameter typings, particularly in Stripe webhook handling. No core logic, error handling, or control flow is altered; all modifications aim to enhance readability, maintainability, and type safety. Changes
Sequence Diagram(s)No sequence diagrams generated as changes are primarily stylistic, involve type/interface improvements, or minor refactoring without affecting control flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (14)
apps/database/components/sql-editor.tsx (2)
76-93: Potential stale-state bug in_addToHistory
_addToHistorycloses overqueryHistoryfrom the render in which the function was created.
If the callback is executed afterqueryHistoryhas already been updated by another render, the slice will operate on a stale array and silently drop newer entries.A simple, React-safe pattern is to pass an updater function to
setQueryHistory:-const _addToHistory = (query: string, success: boolean, duration?: number, error?: string) => { - const newEntry: QueryHistory = { … }; - const updatedHistory = [newEntry, ...queryHistory.slice(0, 49)]; - saveQueryHistory(updatedHistory); -}; +const _addToHistory = (query: string, success: boolean, duration?: number, error?: string) => { + const newEntry: QueryHistory = { … }; + setQueryHistory(prev => { + const next = [newEntry, ...prev.slice(0, 49)]; + localStorage.setItem('sql-query-history', JSON.stringify(next)); + return next; + }); +};
76-81: lucide icons violate project icon-set policyLike other components, this file still imports many icons from
lucide-react(CheckCircle,Clock,Copy, etc.).
Guidelines mandate using Phosphor icons instead. Please replace them across the file for consistency.apps/dashboard/components/ui/carousel.tsx (1)
6-6: Icon library violation: Use Phosphor icons instead of Lucide.The file imports from
lucide-reactwhich violates the coding guidelines. According to the project standards, only Phosphor icons should be used.Consider updating in a future PR:
-import { ArrowLeft, ArrowRight } from 'lucide-react'; +import { ArrowLeftIcon, ArrowRightIcon } from '@phosphor-icons/react';And update the usage:
-<ArrowLeft /> +<ArrowLeftIcon weight="fill" /> -<ArrowRight /> +<ArrowRightIcon weight="fill" />apps/dashboard/stores/jotai/filterAtoms.ts (1)
1-3: Replace date-fns with Dayjs to honour project guidelinesThe repo’s coding-guidelines explicitly forbid date-fns and mandate Dayjs. All four helpers imported here (
differenceInDays,format,isValid,subDays) have direct Dayjs equivalents or simple one-liners.-import { differenceInDays, format, isValid, subDays } from 'date-fns'; +import dayjs from 'dayjs'; +import isBetween from 'dayjs/plugin/isBetween'; +dayjs.extend(isBetween);Then:
-const initialStartDate = subDays(new Date(), 30); -const diffDays = differenceInDays(newRange.endDate, newRange.startDate); -format(startDate, 'yyyy-MM-dd') +const initialStartDate = dayjs().subtract(30, 'day').toDate(); +const diffDays = dayjs(newRange.endDate).diff(dayjs(newRange.startDate), 'day'); +dayjs(startDate).format('YYYY-MM-DD')Keeping date-fns here violates the style-lint checks and will re-introduce warnings next run.
apps/dashboard/app/(auth)/login/verification-needed/page.tsx (2)
4-4: Replacelucide-reactwith Phosphor icons.Project guidelines explicitly forbid
lucide-react.
Switch to@phosphor-icons/reactand use Icon-suffixed names:-import { AlertCircle, ChevronLeft, Loader2 } from 'lucide-react'; +import { WarningCircleIcon, CaretLeftIcon, SpinnerIcon } from '@phosphor-icons/react';Update the JSX usages accordingly (
<WarningCircleIcon …/>,<CaretLeftIcon …/>,<SpinnerIcon …/>).
Doing this consistently avoids future lint failures and keeps visual consistency.
42-60: Tailwind radius variants violate the “always rounded” rule.
rounded-xl(Line 42) androunded-lg(Line 60) contradict the coding-guideline: “always use rounded, never rounded-xl/-md”.-<div className="… rounded-xl border …"> +<div className="… rounded border …"> -<div className="rounded-lg border border-amber-200 …"> +<div className="rounded border border-amber-200 …">Please sweep the file (and neighbouring ones) for variant-suffixes to avoid repetitive churn later.
apps/dashboard/app/(auth)/login/forgot/page.tsx (2)
4-4:lucide-reactmust be removed.Same policy breach as in the verification page. Replace with Phosphor equivalents:
-import { ChevronLeft, Loader2 } from 'lucide-react'; +import { CaretLeftIcon, SpinnerIcon } from '@phosphor-icons/react';Adjust JSX accordingly.
44-70: Radius variants conflict with styling guideline.
rounded-xl(Line 44) androunded-lg(Line 70) should be plainrounded.-… overflow-hidden rounded-xl border … +… overflow-hidden rounded border … -<div className="rounded-lg border border-blue-100 …"> +<div className="rounded border border-blue-100 …">Refactor before styles diverge further.
apps/dashboard/app/(main)/organizations/[slug]/components/settings-tab.tsx (2)
40-42: Avoidany; define a proper organization type.Using
anydefeats type-safety and violates the TS guideline. Create an explicit interface and import it here, e.g.:export interface Organization { id: string; name: string; slug: string; // …other fields… } interface SettingsTabProps { organization: Organization; }
72-93:updateOrganizationis not awaited → optimistic toast could fire on failure.
handleSaveis synchronous but calls the (presumably async)updateOrganizationwithoutawait, yet shows a success toast immediately. If the mutation rejects, the user still sees “updated successfully”.-const handleSave = () => { +const handleSave = async () => { … - try { - updateOrganization({ + try { + await updateOrganization({Ensure the hook actually returns a promise or expose a callback to capture success/error.
apps/dashboard/app/(auth)/login/magic-sent/page.tsx (1)
45-45:rounded-xlradius must be plainrounded.-<div className="relative mx-auto … rounded-xl border …"> +<div className="relative mx-auto … rounded border …">Please align with the styling guideline.
apps/dashboard/app/(main)/websites/[id]/revenue/page.tsx (1)
69-73: Index used as React key – leads to unstable reconciliationReact keys must be stable between renders; using
ibreaks that rule and defeats list diffing.-{[...new Array(4)].map((_, i) => ( +{Array.from({ length: 4 }, (_, idx) => `metric-skel-${idx}`).map((id) => ( ... - key={`${i + 1}-metrics-skeleton`} + key={id}(The switch to
new Arrayis a no-op; the original[...Array(4)]was fine.)apps/dashboard/app/(main)/websites/[id]/page.tsx (1)
137-144: Remove the dead_handleDateRangeChangecallbackThe logic is duplicated inline in the
<DateRangePicker onChange={…}>below, so this memoised callback is never invoked. Keeping it around just increases bundle size.-const _handleDateRangeChange = useCallback( - (range: DayPickerRange | undefined) => { - if (range?.from && range?.to) { - setDateRangeAction({ startDate: range.from, endDate: range.to }); - } - }, - [setDateRangeAction] -);Either wire it up to the picker or delete it.
apps/database/components/data-table-view.tsx (1)
16-30: Critical: Replace Lucide icons with Phosphor icons.This file extensively uses Lucide icons, which violates the project's coding guidelines. According to the retrieved learnings, the project should "Don't use lucide for icons, ONLY use phosphor icons, use width='duotone' for most, but for arrows use fill, for plus icons don't add width".
All Lucide icon imports should be replaced with their Phosphor equivalents:
-import { - ArrowUpDown, - Binary, - Calendar, - Columns, - Edit, - Eye, - EyeOff, - Hash, - MoreHorizontal, - SortAsc, - SortDesc, - Trash2, - Type, -} from 'lucide-react'; +import { + ArrowsDownUpIcon, + BinaryIcon, + CalendarIcon, + ColumnsIcon, + PencilIcon, + EyeIcon, + EyeSlashIcon, + HashIcon, + DotsThreeIcon, + SortAscendingIcon, + SortDescendingIcon, + TrashIcon, + TextTIcon, +} from '@phosphor-icons/react';And update all icon usages throughout the component accordingly.
🧹 Nitpick comments (18)
apps/basket/src/hooks/auth.ts (2)
18-20: Centralising regexes is 👍 — add theiflag for full case-insensitivity
Both constants are a welcome readability & perf win. However, callers may invokeisValidDomainFormat()with a mixed-case string (it’s exported). Adding theiflag keeps the match correct without relying on callers to lowercase first.-const REGEX_WWW_PREFIX = /^www\./; -const REGEX_DOMAIN_LABEL = /^[a-zA-Z0-9-]+$/; +const REGEX_WWW_PREFIX = /^www\./i; +const REGEX_DOMAIN_LABEL = /^[a-z0-9-]+$/i;
165-168:hostname.replace(REGEX_WWW_PREFIX, '')ignores numeric “www” prefixes
www2.example.com,www01.example.com, etc., are fairly common and semantically the same aswww.. Consider broadening the pattern to strip anywww\d*.prefix:-const REGEX_WWW_PREFIX = /^www\./i; +const REGEX_WWW_PREFIX = /^www\d*\./i;This keeps behaviour unchanged for plain
www.while covering additional real-world cases.apps/dashboard/lib/canvas-utils.ts (1)
30-31: Remove the now-superfluous_centerX/_centerYconstantsThese constants are still computed but never referenced, so they only add byte-code and keep the “unused-var” suppression hack around. Deleting them is the cleanest option and keeps the function free of dead code.
- const _centerX = image.naturalWidth / 2; - const _centerY = image.naturalHeight / 2;apps/dashboard/components/charts/metrics-constants.ts (1)
1-1: Consider migrating away fromlucide-reactfor future consistencyThroughout the dashboard,
.tsxcomponents are already switching to Phosphor icons per guidelines. Even though this is a.tshelper, it still couples downstream components to Lucide. When you next touch the chart primitives, consider exporting Phosphor equivalents so the icon stack stays uniform.apps/dashboard/components/layout/user-menu.tsx (1)
30-32: Duplicate initials logic – extract to shared utilThe initials-building code now appears both here and in
components/auth/user-button.tsx. Extracting it tolib/get-user-initials.ts(or similar) removes duplication and centralises future tweaks (e.g., handling middle names).apps/dashboard/lib/performance-utils.ts (1)
35-39:fastPagesthreshold off-by-one compared to scoring logic
fastPagesis incremented whenloadTime < FAST, whereas the score treatsloadTime <= FASTas a perfect score (Lines 42-44).
For consistency the equality case should be counted as fast:-if (loadTime < SCORE_THRESHOLDS.FAST) { +if (loadTime <= SCORE_THRESHOLDS.FAST) {Otherwise a page loading exactly at the
FASTthreshold is “perfect” for the score but not for the fast-page count.apps/dashboard/lib/utils.ts (1)
18-34: Minor: guard negative durations
formatDurationdoesn’t guard against negativeseconds. A quick defensive check avoids odd output:+if (seconds <= 0) return '0s';apps/database/components/table-card.tsx (1)
51-51: Minor perf nit: avoid the extraparseFloat.
Number.parseFloat(x.toFixed(2))converts a string back to a number only to string-interpolate it again. Drop the round-trip:-return `${Number.parseFloat((bytes / k ** i).toFixed(2))} ${sizes[i]}`; +return `${(bytes / k ** i).toFixed(2)} ${sizes[i]}`;apps/dashboard/app/(main)/websites/[id]/_components/utils/code-generators.ts (2)
15-18: Remove unused_apiUrlvariableThe variable is dead-code after renaming; keeping it around only to silence the unused-var rule adds noise.
- const _apiUrl = isLocalhost - ? 'http://localhost:4000' - : 'https://basket.databuddy.cc';
20-30: Duplication in option-filtering logicThe three nearly identical
.filter()blocks can be extracted into a small helper to keep these generators DRY:+function buildMeaningfulOptions(opts: TrackingOptions) { + return Object.entries(opts).filter(([k, v]) => { + const def = ACTUAL_LIBRARY_DEFAULTS[k as keyof TrackingOptions]; + return v !== def && !(typeof v === 'boolean' && !v && !def); + }); +}Then reuse in all three generators.
Also applies to: 55-65, 100-110
apps/dashboard/app/(main)/websites/[id]/errors/_components/errors-page-content.tsx (1)
55-69: Remove unused helper functions
_removeFilterand_clearFiltersare no longer referenced after the linter rename. Keeping dead code adds noise and risks it drifting out of sync with the actual filter logic.- // Remove a filter - const _removeFilter = (filterToRemove: DynamicQueryFilter) => { - setActiveFilters((prev) => - prev.filter( - (f) => - !( - f.field === filterToRemove.field && f.value === filterToRemove.value - ) - ) - ); - }; - - // Clear all filters - const _clearFilters = () => { - setActiveFilters([]); - };apps/dashboard/app/(main)/sandbox/reddit-mentions/hooks/use-reddit-mentions.ts (1)
152-160: Centralise the repetitiveresult.errorchecksEach API handler repeats the same 3-line pattern:
const result = await apiRequest<…>(…); if (result.error) { throw new Error(result.error); } return result.data || …Extracting a small helper (e.g.
assertSuccess(result)) trims ~20 lines, prevents copy-paste divergence, and keeps each handler focused on its own mapping logic.Also applies to: 181-187, 195-202, 234-239
apps/dashboard/app/(main)/sandbox/reddit-mentions/page.tsx (1)
239-242: Delete unused_handleExport
_handleExportis defined but never called (export is performed inline in the button handler). Safely remove to avoid unused-symbol linter noise.apps/dashboard/app/(main)/organizations/[slug]/components/settings-tab.tsx (1)
138-156: Multiplerounded-lgclasses conflict with the “always rounded” rule.Replace all
rounded-lgoccurrences withroundedto stay consistent.
A codemod across the component (and repository) will prevent repetitive manual edits.Also applies to: 164-181, 282-289
apps/dashboard/app/(main)/revenue/_components/tabs/settings-tab.tsx (1)
53-53: Consider optimizing the dependency array for better performance.While adding
isMaskedSecretto the dependency array is correct since the function is used within the effect, this will cause the effect to re-run on every render because the function is recreated each time.Consider one of these optimizations:
Option 1: Move the function outside the component
+const isMaskedSecret = (secret: string) => secret.includes('*'); + export function RevenueSettingsTab({ revenueConfig, }: { revenueConfig: ReturnType<typeof useRevenueConfig>; }) { const [webhookSecret, setWebhookSecret] = useState(''); const [isLiveMode, setIsLiveMode] = useState(revenueConfig.isLiveMode); - // Check if the webhook secret is masked (contains asterisks) - const isMaskedSecret = (secret: string) => secret.includes('*');Option 2: Wrap in useCallback
+import { useEffect, useState, useCallback } from 'react'; // Check if the webhook secret is masked (contains asterisks) - const isMaskedSecret = (secret: string) => secret.includes('*'); + const isMaskedSecret = useCallback((secret: string) => secret.includes('*'), []);apps/dashboard/app/(main)/websites/[id]/_components/utils/ui-components.tsx (1)
176-179: Guard clause reads clearer than nested returnTiny readability nit – invert the condition to shorten indentation:
-if (!showTooltip) { - return content; -} +if (showTooltip) { + … +} +return content;Feel free to ignore if you prefer the current style.
apps/database/components/data-table.tsx (1)
210-218: Minor nit – single-letter power operator is clearer-return `${Number.parseFloat((bytes / k ** i).toFixed(2))} ${sizes[i]}`; +return `${Number.parseFloat((bytes / (k ** i)).toFixed(2))} ${sizes[i]}`;Adds parentheses to silence potential precedence lint complaints.
apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-section.tsx (1)
116-126: Potential redundancy in scroll effects.Both
useEffecthooks (lines 116-120 and 122-126) now have empty dependency arrays and perform similar scrolling behavior. The first scrolls withbehavior: 'auto'and the second withbehavior: 'smooth'. Consider if both are necessary or if they can be consolidated.useEffect(() => { if (bottomRef.current) { bottomRef.current.scrollIntoView({ behavior: 'auto' }); } }, []); -useEffect(() => { - if (bottomRef.current) { - bottomRef.current.scrollIntoView({ behavior: 'smooth' }); - } -}, []);
|
@ me when it's all done |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
apps/dashboard/components/bits/Iridiscence.tsx (1)
57-63: Component name ≠ filename → import failures on case-sensitive systems
export default function Iridescence(with an “e”) lives in a file namedIridiscence.tsx(without the “e”).
On macOS/Linux this mismatch will breakimport Iridiscence from './Iridiscence'or vice-versa, depending on which spelling callers choose.-// Option A – keep filename, fix component -export default function Iridescence({ +export default function Iridiscence({or rename the file to
Iridescence.tsx(preferred for correct spelling).Failing to align the two will surface as a “module not found” error at runtime / build time.
apps/dashboard/app/(main)/revenue/_components/onboarding/configuration-summary.tsx (1)
20-32:CheckCircleIconshould default toduotoneweight per icon rulesTeam convention (see “01-MUST-DO”) says: “use weight='duotone' for most icons; use
fillonly for arrows; do not specify width for plus icons”.
CheckCircleIconis currently rendered withfillin both places.-<CheckCircleIcon - className="h-5 w-5 text-green-500" - size={20} - weight="fill" -/> +<CheckCircleIcon + className="h-5 w-5 text-green-500" + size={20} + weight="duotone" +/> ... -<CheckCircleIcon className="h-4 w-4" size={16} weight="fill" /> +<CheckCircleIcon className="h-4 w-4" size={16} weight="duotone" />Switching to
duotonekeeps us 100 % compliant with the house iconography spec.Also applies to: 74-76
apps/dashboard/app/(main)/billing/hooks/use-billing.ts (1)
19-35:_isActionLoadingis written but never read – consider removing or exposing it
_isActionLoadingis toggled viasetIsActionLoading, but the state value itself is never consumed or returned by the hook.
Either:
- Expose it to consumers of
useBillingso they can render a spinner, or- Drop the state entirely if the UI no longer needs it.
-const [_isActionLoading, setIsActionLoading] = useState(false); +const [isActionLoading, setIsActionLoading] = useState(false); ... return { isLoading, + isActionLoading, // make it available to callers onUpgrade: handleUpgrade, ... }
♻️ Duplicate comments (15)
apps/dashboard/lib/geo.ts (1)
17-19: Avoidanyin public coordinate types
any[][][]drops compile-time checks and violates the project guideline againstany.
Use a concrete numeric type or introduce a reusable alias:- coordinates: any[][][]; + coordinates: number[][][];(or)
export type Coordinates3D = number[][][]; // … coordinates: Coordinates3D;Same change applies to both
Subdivisions.geometryandCountry.geometry.Also applies to: 33-36
apps/dashboard/app/not-found.tsx (1)
3-3: Replace lucide-react icons with Phosphor icons as per project guidelines.
The project’s “01-MUST-DO” rules explicitly forbidlucide-react; use Phosphor with the prescribed weights instead. This was already pointed out in an earlier review and remains unresolved.-import { ArrowLeft, Home } from 'lucide-react'; +import { ArrowLeftIcon, HouseIcon } from '@phosphor-icons/react';Icon usage updates later in the file:
- <Home className="mr-2 h-4 w-4" /> + <HouseIcon className="mr-2 h-4 w-4" weight="duotone" /> … - <ArrowLeft className="mr-2 h-4 w-4" /> + <ArrowLeftIcon className="mr-2 h-4 w-4" weight="fill" />apps/dashboard/components/layout/theme-toggle.tsx (2)
20-22: Stylistic improvement looks good, but the critical browser compatibility bug remains unfixed.The expansion from single-line to multi-line if statement with braces improves code consistency and readability, which aligns with the linting improvements in this PR.
However, the critical logic bug identified in the previous review still exists: execution continues to line 23 even when
document.startViewTransitionis undefined, causing a TypeError in unsupported browsers.
19-24: Critical: Browser fallback still broken - execution falls through to undefined API call.The logic bug from the previous review remains unfixed. When
document.startViewTransitionis not available, the code callsswitchTheme()but continues executingdocument.startViewTransition(switchTheme)on line 23, causing a TypeError.Apply this fix to prevent the fallthrough:
const toggleTheme = () => { if (!document.startViewTransition) { switchTheme(); + return; } document.startViewTransition(switchTheme); };apps/dashboard/app/(main)/websites/[id]/map/page.tsx (1)
180-191: Previous key-related concern still unaddressed.While the array instantiation was updated from
Array(6).fill(0)tonew Array(6).fill(0), the previously identified issue with using array indices in React keys (Line 183:key={country-skeleton-${i + 1}}) remains unresolved. Using array indices can break React's reconciliation when list lengths change.Consider the previously suggested improvements:
- Use
Array.from({ length: 6 })for cleaner array creation- Replace index-based keys with constant strings since these are static placeholders
apps/dashboard/app/(main)/organizations/[slug]/components/teams-tab.tsx (1)
12-12: Address the type safety issue withorganization: any.This issue was previously flagged but remains unresolved. The component still uses
anytype for the organization prop, which reduces type safety. Based on the database schema, consider using a proper type:-export function TeamsTab({ organization }: { organization: any }) { +export function TeamsTab({ organization }: { organization?: { id?: string } }) {apps/dashboard/components/error-boundary.tsx (1)
3-3: Replace lucide icons with phosphor icons per project guidelinesThis file still uses
AlertTriangleandRefreshCwfrom lucide-react, violating the project guideline that requires using only phosphor icons.Replace the imports and usage:
-import { AlertTriangle, RefreshCw } from 'lucide-react'; +import { WarningCircleIcon, ArrowClockwiseIcon } from '@phosphor-icons/react';-<AlertTriangle className="h-5 w-5" /> +<WarningCircleIcon size={20} weight="duotone" />-<RefreshCw className="h-3.5 w-3.5" /> +<ArrowClockwiseIcon size={14} weight="duotone" />Also applies to: 44-44, 69-69
apps/dashboard/lib/autumn/pricing-table-content.tsx (1)
1-1: Replaceanywith a proper product interface
productis typed asany, which defeats type-safety and violates the repo guideline: "Don't use any type".
Create a sharedProductinterface (or import an existing one) and use it here.apps/dashboard/app/(main)/websites/[id]/page.tsx (1)
88-88: Drop the unused setter instead of prefixing with "_"As noted in previous reviews,
_setCurrentDateRangeStateis never referenced. Destructure only what you actually use:-const [currentDateRange, _setCurrentDateRangeState] = useAtom(dateRangeAtom); +const [currentDateRange] = useAtom(dateRangeAtom);apps/dashboard/app/(main)/organizations/[slug]/components/invite-member-dialog.tsx (1)
33-35: Style improvement aligns with codebase consistency.Expanding the single-line conditional to a multi-line block statement improves readability and aligns with the consistent formatting pattern applied across the codebase.
However, the previous review concerns about type safety and user feedback remain unaddressed:
- Component props still use
anytype (Line 24) instead of a proper interface- Silent return provides no user feedback when email validation fails
apps/dashboard/app/(auth)/login/magic-sent/page.tsx (1)
4-4: Replace lucide icons with Phosphor equivalents.Based on the project's coding guidelines, lucide icons should be replaced with Phosphor icons. The existing past review comment about this change is still applicable.
apps/dashboard/app/(auth)/register/page.tsx (1)
97-97: Error parameter renaming follows established pattern.The renaming of error parameters to
_errorin all three catch blocks is consistent with the pattern used across other authentication pages and properly signals intentional non-use to the linter.Also applies to: 121-121, 147-147
apps/database/components/table-card.tsx (1)
3-3: Replace lucide icons with Phosphor equivalents.The existing past review comment about replacing lucide icons with Phosphor icons is still applicable. All the imported lucide icons should be replaced with their Phosphor counterparts according to the project guidelines.
apps/dashboard/app/(main)/organizations/components/organizations-tab.tsx (1)
72-72: Preserve error context when deletion fails.This change only renames the error variable but doesn't address the underlying issue of losing error context for debugging organization deletion failures.
apps/database/app/page.tsx (1)
48-48: Potential infinite re-render issue with useEffect dependency.Adding
loadDatato the dependency array could cause infinite re-renders sinceloadDatais redefined on every component render. Consider wrappingloadDatawithuseCallbackto prevent this issue.+ import { useCallback } from 'react'; - const loadData = async () => { + const loadData = useCallback(async () => { setLoading(true); // ... rest of the function - }; + }, []);
🧹 Nitpick comments (20)
apps/basket/src/utils/ip-geo.test.ts (2)
367-367: Avoid the double type-cast workaround
null as unknown as stringside-steps the type system and makes the intent harder to read. If the goal is simply to exercise the “invalid input” branch, consider one of the options below instead of the two-step cast:-const result = await getGeoLocation(null as unknown as string); +// @ts-expect-error – deliberately passing null to verify defensive handling +const result = await getGeoLocation(null);or expose
getGeoLocationwith the broader type (ip: string | null | undefined) so the test doesn’t need casts at all.
439-447: Minor: guard against a single rejected promiseBecause
Promise.allrejects as soon as one promise fails, a transient network/database glitch ingetGeoLocationcould fail the entire test suite even when the other IPs succeed.If you want the test to proceed and collect all results,
Promise.allSettledis safer:-const geoResults = await Promise.all(testIPs.map((ip) => getGeoLocation(ip))); +const geoResults = await Promise.allSettled( + testIPs.map((ip) => getGeoLocation(ip)) +); +for (const r of geoResults) { + expect(r.status).toBe('fulfilled'); +}Not critical, but worth considering.
apps/database/app/api/database/stats/route.ts (1)
7-40: Run the four ClickHouse queries in parallel to cut response latency.The
tableStats,databases,systemInfo, and (best-effort)memoryInfoqueries are completely independent yet execute serially, so the route waits for the slowest sum of their runtimes. Issuing them concurrently can slash p95 latency, especially on busy DB nodes.-// Get table statistics -const tableStats = await chQuery(`...`); - -// Get database list with sizes -const databases = await chQuery(`...`); - -// Get system information -const systemInfo = await chQuery(`...`); - -// Try to get memory usage (may not be available in all versions) -let memoryInfo: unknown[] = []; -try { - memoryInfo = await chQuery(`...`); -} catch (error) { - console.warn('Memory metrics not available:', error); -} +const [ + tableStats, + databases, + systemInfo, +] = await Promise.all([ + chQuery(`/* tables */`), + chQuery(`/* databases */`), + chQuery(`/* system */`), +]); + +// Try to get memory usage (may not be available in all versions) +let memoryInfo: unknown[] = []; +try { + memoryInfo = await chQuery(`/* memory */`); +} catch (error) { + console.warn('Memory metrics not available:', error); +}No behavioural change—just better throughput.
apps/dashboard/app/(main)/revenue/_components/onboarding/configuration-summary.tsx (2)
21-26: Addaria-hiddenoraria-labelfor better a11yThe icons convey status but already have adjacent text. Without an accessible name screen-readers may announce the raw component name. Two options:
- Treat them as decorative:
-<CheckCircleIcon … /> +<CheckCircleIcon aria-hidden="true" focusable="false" … />
- Provide an explicit label:
-<WarningCircleIcon … /> +<WarningCircleIcon aria-label="Configuration warning" role="img" … />Pick one approach and apply consistently.
Also applies to: 27-32, 74-76
43-47: Potential secret leakage – consider masking the webhook tokenRendering the raw
webhookTokenexposes the full secret in the DOM and browser history (copy-paste, screenshots). If operators only need to verify existence rather than the full string, show a masked “••••abcd” version instead.-<p className="font-mono">{webhookToken || 'Not configured'}</p> +<p className="font-mono"> + {webhookToken ? `${webhookToken.slice(0, 4)}•••${webhookToken.slice(-4)}` : 'Not configured'} +</p>Helps prevent accidental credential leaks during demos or screen-sharing.
apps/dashboard/app/(main)/websites/[id]/test/components/minimal-table.tsx (1)
225-238: Guard against state updates after unmount by clearing the timeout
handleTabChangeschedules asetTimeoutthat calls severalsetStatefunctions.
If the component unmounts (oractiveTabchanges again) before the 150 ms timer fires, React will emit a “state update on unmounted component” warning and may cause a memory leak in DEV builds.A lightweight fix is to store the timer id in a ref and clear it in an effect cleanup.
@@ const [expandedRows, setExpandedRows] = useState<Set<string>>(new Set()); +const transitionTimer = useRef<NodeJS.Timeout>(); @@ (tabId: string) => { if (tabId === activeTab) { return; } - setIsTransitioning(true); - setTimeout(() => { + setIsTransitioning(true); + if (transitionTimer.current) clearTimeout(transitionTimer.current); + transitionTimer.current = setTimeout(() => { setActiveTab(tabId); … setIsTransitioning(false); - }, 150); + }, 150); }, [activeTab] ); +useEffect(() => { + return () => { + if (transitionTimer.current) clearTimeout(transitionTimer.current); + }; +}, []);This prevents orphaned timers and keeps dev-mode noise down.
apps/database/app/sql/page.tsx (1)
35-49: Minor: avoid double-trimming and send a clean query string to the API.
query.trim()is computed three separate times (early-return,disabled, and infetch).
It’s inexpensive but still redundant; more importantly, the raw (possibly whitespace-padded) string is what gets posted to/api/database/query.- const executeQuery = async () => { - if (!query.trim()) { - return; - } + const executeQuery = async () => { + const trimmedQuery = query.trim(); + if (!trimmedQuery) { + return; + } … - body: JSON.stringify({ query }), + body: JSON.stringify({ query: trimmedQuery }), …This single
trimmedQueryvariable prevents accidental “\n” submissions and removes duplicated calls.
Impact is low but it tightens the UI-API contract.apps/basket/src/hooks/auth.ts (3)
18-20: Broaden the “www” prefix regex for edge-cases
REGEX_WWW_PREFIXonly matches the literalwww.. Hosts such aswww1.example.comorwww2.—often used behind load-balancers/CDNs—will slip through and fail normalisation. A one-line tweak makes the rule more tolerant while remaining anchored to the start of the hostname:-const REGEX_WWW_PREFIX = /^www\./; +// Accepts "www.", "www1.", "www2.", … but still only at the very beginning +const REGEX_WWW_PREFIX = /^www\d*\./;
165-168: Consider trimming a trailing dot after hostname extractionRarely, a fully-qualified domain arrives with a trailing dot (
example.com.).
new URL().hostnamepreserves that dot, so the subsequentreplace(REGEX_WWW_PREFIX, '')leaves it intact and the value fails later validation.A tiny post-processing step keeps
normalizeDomainresilient:const hostname = new URL(urlString).hostname; -const finalDomain = hostname.replace(REGEX_WWW_PREFIX, ''); +const finalDomain = hostname + .replace(REGEX_WWW_PREFIX, '') + .replace(/\.$/, ''); // strip trailing dot if present
219-223: Consolidate label checks into a single regex for clarity & speed
isValidDomainFormatfirst appliesREGEX_DOMAIN_LABEL, then performs two extra string checks to forbid leading/trailing hyphens.
Those constraints can live inside the regex itself, eliminating the extra branches:-const REGEX_DOMAIN_LABEL = /^[a-zA-Z0-9-]+$/; +// Disallow leading/trailing hyphens while still permitting internal ones +const REGEX_DOMAIN_LABEL = /^(?!-)[a-zA-Z0-9-]{1,63}(?<!-)$/; ... -if ( - !REGEX_DOMAIN_LABEL.test(label) || - label.startsWith('-') || - label.endsWith('-') -) { +if (!REGEX_DOMAIN_LABEL.test(label)) { return false; }Besides being marginally faster, the intent is captured in one place.
Feel free to ignore if you prefer the current explicit checks for readability.apps/dashboard/components/charts/metrics-constants.ts (1)
109-110: Move late import to the top for consistencyThe
formatDurationimport appears after executable code. While top-level imports are hoisted, keeping all imports grouped at the beginning improves readability and avoids churn from auto-import tools.-... -]; - -import { formatDuration } from '@/lib/utils'; +]; + +// utilities +import { formatDuration } from '@/lib/utils';apps/dashboard/lib/canvas-utils.ts (1)
30-31: Consider removing unused variables entirely.The underscore prefix correctly indicates these variables are intentionally unused. However, since
_centerXand_centerYare calculated but never referenced, consider removing them entirely to reduce code complexity.- const _centerX = image.naturalWidth / 2; - const _centerY = image.naturalHeight / 2;apps/database/components/sql-editor.tsx (1)
76-76: Consider removing unused function instead of prefixing with underscore.While prefixing with underscore indicates the function is unused, consider removing the entire
_addToHistoryfunction if it's not needed. This would reduce code bloat and improve maintainability.- const _addToHistory = ( - query: string, - success: boolean, - duration?: number, - error?: string - ) => { - const newEntry: QueryHistory = { - id: Date.now().toString(), - query: query.trim(), - timestamp: new Date(), - duration, - success, - error, - }; - - const updatedHistory = [newEntry, ...queryHistory.slice(0, 49)]; // Keep last 50 queries - saveQueryHistory(updatedHistory); - };apps/dashboard/app/(main)/websites/[id]/page.tsx (1)
137-144: Remove unused callback functionThe
_handleDateRangeChangefunction is defined but never used. The DateRangePicker on line 355 uses an inline callback instead. Consider removing this unused function entirely:-const _handleDateRangeChange = useCallback( - (range: DayPickerRange | undefined) => { - if (range?.from && range?.to) { - setDateRangeAction({ startDate: range.from, endDate: range.to }); - } - }, - [setDateRangeAction] -);apps/dashboard/app/(main)/sandbox/reddit-mentions/page.tsx (1)
239-241: Good practice: Indicating unused function.Prefixing the unused function with underscore clearly indicates intentional non-use. Consider removing this function if it's no longer needed, or implement the export functionality if it should be used.
apps/dashboard/app/(main)/revenue/_components/tabs/settings-tab.tsx (1)
53-53: Consider optimizing function placement and dependencies.While adding
isMaskedSecretto the dependency array is technically correct, since it's defined inside the component, it could cause unnecessary effect re-runs. Consider movingisMaskedSecretoutside the component since it's a pure utility function, or wrap it inuseCallbackto stabilize its reference.+// Move outside component since it's a pure utility function +const isMaskedSecret = (secret: string) => secret.includes('*'); + export function RevenueSettingsTab({ revenueConfig, }: { revenueConfig: ReturnType<typeof useRevenueConfig>; }) { const [webhookSecret, setWebhookSecret] = useState(''); const [isLiveMode, setIsLiveMode] = useState(revenueConfig.isLiveMode); - // Check if the webhook secret is masked (contains asterisks) - const isMaskedSecret = (secret: string) => secret.includes('*'); // Initialize webhook secret only if it's not masked useEffect(() => { const currentSecret = revenueConfig.webhookSecret || ''; if (!isMaskedSecret(currentSecret)) { setWebhookSecret(currentSecret); } - }, [revenueConfig.webhookSecret, isMaskedSecret]); + }, [revenueConfig.webhookSecret]);apps/dashboard/hooks/use-dynamic-query.ts (2)
291-323: Unused loop index – remove_indexparam for clarityThe map callback receives
_indexthat is not referenced.
Drop the parameter to avoid misleading “unused” identifiers and reduce linter noise.-return query.data.results.map((result, _index) => { +return query.data.results.map((result) => {
393-397: Bubble fetch failure details to callerYou convert non-OK responses to a generic
Error('Failed to fetch query options').
Consider includingres.statusor server-supplied message to aid debugging:-throw new Error('Failed to fetch query options'); +throw new Error(`Failed to fetch query options – ${res.status} ${res.statusText}`);apps/dashboard/public/databuddy.js (2)
892-897: Same compatibility concern forentries.at(-1)
PerformanceEntry[]does not guaranteeArray.prototype.atsupport in all browsers.
Useentries[entries.length - 1]for wider compatibility.
440-448: Superfluous empty catch blocks – consider logging behind a debug flagSeveral
catch (_e) {}blocks swallow unexpected errors silently, making field debugging hard.
At minimum, log whenprocess.env.NODE_ENV === 'development'to aid diagnosis.
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/tracking-setup-tab.tsx (1)
92-105: Single sharedcopiedstate causes wrong visual feedback across multipleCodeBlocks
Because everyCodeBlockreceives the samecopiedboolean, copying one snippet flips the icon for all code blocks for 2 seconds. Users viewing another tab (npm vs script) will see an incorrect green checkmark.Consider scoping the copied state per snippet instead of globally:
-const [copied, setCopied] = useState(false); +const [copiedId, setCopiedId] = useState<string | null>(null); … -const handleCopyCode = (code: string) => { - navigator.clipboard.writeText(code); - setCopied(true); +const handleCopyCode = (id: string, code: string) => { + navigator.clipboard.writeText(code); + setCopiedId(id); toast.success('Tracking code copied to clipboard'); - setTimeout(() => setCopied(false), 2000); + setTimeout(() => setCopiedId(null), 2000); };Pass a stable
id(e.g.'tracking-script','npm-install', …) to eachCodeBlockand derive itscopiedprop viacopied={copiedId === id}.
This avoids confusing UX and keeps the component fully self-contained.apps/dashboard/components/analytics/animated-loading.tsx (1)
3-9: Replace remaining lucide icons with phosphor iconsThe file still imports multiple lucide icons (
ActivitySquare,ArrowRight,Loader2,Server,Users) which violates project guidelines requiring phosphor icons exclusively. While the unusedDatabaseimport was removed, the remaining icons need to be replaced with their phosphor equivalents.Replace the lucide imports with phosphor equivalents:
-import { - ActivitySquare, - ArrowRight, - Loader2, - Server, - Users, -} from 'lucide-react'; +import { + Activity, + ArrowRightIcon, + SpinnerIcon, + ServerIcon, + UsersIcon, +} from '@phosphor-icons/react';And update the corresponding JSX usage to use proper phosphor icon props with
weight="duotone"orweight="fill"for arrows.
♻️ Duplicate comments (19)
apps/dashboard/components/charts/metrics-constants.ts (1)
1-1: The lucide-react import issue remains unresolvedWhile
LineChartwas removed, the core issue identified in previous reviews persists: the project guidelines mandate using Phosphor icons instead of lucide-react. The remaining imports (Eye,MousePointer,TrendingUp,Users) still violate the established icon standards.apps/dashboard/components/error-boundary.tsx (1)
3-3: Replace lucide icons with phosphor icons per project guidelinesThis issue was previously identified but remains unresolved. The project guidelines require using phosphor icons exclusively.
apps/dashboard/app/not-found.tsx (1)
3-3: Replace lucide-react with phosphor icons.This file still violates the coding guidelines by using lucide-react icons. The guidelines explicitly state: "Don't use lucide for icons, ONLY use phosphor icons, use width='duotone' for most, but for arrows use fill".
Replace the lucide-react imports with phosphor icons:
-import { ArrowLeft, Home } from 'lucide-react'; +import { ArrowLeftIcon, HouseIcon } from '@phosphor-icons/react';And update the icon usage in the JSX:
-<Home className="mr-2 h-4 w-4" /> +<HouseIcon className="mr-2 h-4 w-4" weight="duotone" />-<ArrowLeft className="mr-2 h-4 w-4" /> +<ArrowLeftIcon className="mr-2 h-4 w-4" weight="fill" />apps/dashboard/components/layout/theme-toggle.tsx (1)
20-24: Critical bug still present - missing return statement.While the stylistic change to use block braces is good, the critical issue identified in previous reviews remains unfixed. When
document.startViewTransitionis unavailable, execution continues to line 23, causing aTypeErrorin browsers without View Transition API support.Add the missing return statement:
if (!document.startViewTransition) { switchTheme(); + return; } document.startViewTransition(switchTheme);apps/dashboard/lib/geo.ts (1)
18-18: LGTM! Improved type safety addresses previous feedback.The change from
any[][][]tonumber[][][]for coordinate arrays enhances type safety and directly addresses the previous review comment about avoidinganyin public types. This provides compile-time safety for coordinate data which should indeed be numeric.Also applies to: 35-35
apps/dashboard/public/databuddy.js (1)
893-896: Browser compatibility concern withentries.at(-1)The use of
.at(-1)may cause issues in older browsers. This is a known concern from previous reviews.apps/dashboard/app/(auth)/login/magic-sent/page.tsx (1)
4-4: Icon migration from Lucide to Phosphor remains incomplete.The imports still use
lucide-reacticons (ChevronLeft,Loader2,MailCheck) which should be replaced with Phosphor icons according to project guidelines.apps/database/app/sql/page.tsx (1)
3-3: Replace lucide-react icons with Phosphor icons.This is a duplicate of a previous comment - the project rules require using Phosphor icons instead of lucide-react.
apps/dashboard/app/(main)/websites/[id]/map/page.tsx (1)
180-191: Avoid using array index for React keys.This duplicates a previous review comment - using array indices in React keys can break reconciliation when the list changes.
apps/dashboard/app/(auth)/register/page.tsx (1)
122-122: Consider logging errors for better observability.This relates to a previous review comment - while renaming to
_errorsatisfies linting, these catch blocks still discard valuable debugging information that could help with troubleshooting.Also applies to: 148-148
apps/dashboard/app/(main)/organizations/components/organizations-tab.tsx (1)
72-75: Error context partially preserved but user still sees generic message.While logging the error to console is helpful for debugging, the past review comment specifically requested forwarding the actual error message to the user via
toast.error. The current implementation still shows a generic message to users.apps/database/components/table-card.tsx (1)
3-3: Settings import removed, but lucide icons still need replacement.Good cleanup removing the unused
Settingsimport. However, the existing issue with lucide icons remains unaddressed.apps/dashboard/lib/autumn/pricing-table-content.tsx (1)
3-3: Excellent type safety improvementThe change from
anytoProducttype significantly improves type safety and addresses the previous review feedback about avoidinganytypes. This will provide compile-time checking and help prevent property access errors.apps/dashboard/app/(main)/organizations/[slug]/components/invite-member-dialog.tsx (2)
24-32: Excellent type safety improvementThe replacement of
anywith a properly defined inline interface significantly improves type safety and addresses the previous review feedback. The interface clearly defines the expected props with appropriate types.
41-43: Formatting improved, but UX issue remainsThe expansion to explicit block statement follows the PR's consistent formatting pattern. However, the underlying UX concern from the previous review persists - the silent return provides no user feedback when the email field is empty. Consider disabling the "Send Invitation" button when
inviteEmail.trim().length === 0or showing validation feedback.apps/dashboard/app/(main)/sandbox/reddit-mentions/hooks/use-reddit-mentions.ts (4)
155-157: Guard againstsuccess === false, not onlyerrorThe conditional formatting improvement is good, but this still has the same critical error handling issue flagged in previous reviews. The code should check
!result.successinstead of onlyresult.errorto handle cases whereapiRequestreturns{ success: false, error: undefined }.-if (result.error) { +if (!result.success) { throw new Error(result.error ?? 'Request failed'); }
182-184: Same error handling issue as noted above.Good formatting improvement, but the same
!result.successcheck should be applied here for consistent error handling.
198-200: Consistent formatting with same error handling concern.The block statement formatting is an improvement, but the error handling logic should also be updated per the previous review comments.
235-237: Final instance of the same error handling pattern.Good stylistic improvement, but this also needs the
!result.successcheck for consistency with proper error handling.
🧹 Nitpick comments (7)
apps/dashboard/app/(main)/websites/[id]/_components/tabs/tracking-setup-tab.tsx (1)
146-158: Refresh button triggers toast without visual loading state
handleRefreshinvalidates the TRPC query and immediately shows “Checking tracking status…”. If the network is slow or errors out, the user has no indication of progress/failure.Optional polish:
- Maintain a local
isRefreshingflag that disables the button and spins the icon while the query refetches.- Catch errors from
invalidate(returns a promise) and surface them viatoast.error.This improves perceived responsiveness and accessibility for screen-reader users.
apps/basket/src/polyfills/compression.js (1)
32-40: Factor out duplicated format-selection logicBoth constructors repeat the same
if‒elsechain. Extracting a shared helper (or lookup table) removes duplication and guarantees the two classes stay consistent.+const compressionFactories = { + 'deflate': () => zlib.createDeflate(), + 'deflate-raw': () => zlib.createDeflateRaw(), + 'gzip': () => zlib.createGzip(), +}; + +function getCompressionHandle(format) { + const factory = compressionFactories[format]; + if (!factory) throw new TypeError(`Unsupported format: ${format}`); + return factory(); +}- let handle; - if (format === 'deflate') { - handle = zlib.createDeflate(); - } else if (format === 'gzip') { - handle = zlib.createGzip(); - } else if (format === 'deflate-raw') { - handle = zlib.createDeflateRaw(); - } else { - throw new TypeError(`Unsupported CompressionStream format: ${format}`); - } - make(this, handle); + make(this, getCompressionHandle(format));The same approach can be mirrored for
DecompressionStream.apps/database/components/sql-editor.tsx (1)
76-93: Consider removing unused function entirely.The function
_addToHistoryis prefixed with an underscore to indicate it's unused, but it's never called anywhere in the component. Consider removing it entirely rather than keeping dead code.- // Add query to history - const _addToHistory = ( - query: string, - success: boolean, - duration?: number, - error?: string - ) => { - const newEntry: QueryHistory = { - id: Date.now().toString(), - query: query.trim(), - timestamp: new Date(), - duration, - success, - error, - }; - - const updatedHistory = [newEntry, ...queryHistory.slice(0, 49)]; // Keep last 50 queries - saveQueryHistory(updatedHistory); - };apps/dashboard/app/(main)/revenue/_components/tabs/settings-tab.tsx (1)
53-53: Consider optimizing the dependency array.Adding
isMaskedSecretto the dependency array will cause the effect to run on every render since the function reference changes each time. SinceisMaskedSecretis a pure function that doesn't depend on props or state, consider either:
- Moving it outside the component:
+const isMaskedSecret = (secret: string) => secret.includes('*'); + export function RevenueSettingsTab({ - // Check if the webhook secret is masked (contains asterisks) - const isMaskedSecret = (secret: string) => secret.includes('*');
- Or wrapping it in
useCallback:- const isMaskedSecret = (secret: string) => secret.includes('*'); + const isMaskedSecret = useCallback((secret: string) => secret.includes('*'), []);Then remove it from the dependency array since it won't change.
apps/dashboard/app/(auth)/login/magic/page.tsx (1)
41-44: Consider logging the error for better observability.While renaming to
_errorsatisfies linting requirements, it discards valuable debugging information. Consider logging the error before showing the user-friendly toast message.-} catch (_error) { +} catch (error) { + console.error('Magic link login error:', error); setIsLoading(false); toast.error('Failed to send magic link. Please try again.'); }apps/dashboard/app/(main)/organizations/[slug]/components/teams-tab.tsx (1)
12-12: Type safety improved but may not match database schema.Good improvement over the
anytype! However, according to the database schema inpackages/db/src/drizzle/schema.ts(lines 594-611), theorganization.idfield is defined as a non-null primary key. Consider using{ id: string }instead of optional properties to better reflect the actual data structure.-export function TeamsTab({ organization }: { organization?: { id?: string } }) { +export function TeamsTab({ organization }: { organization?: { id: string } }) {apps/dashboard/app/(main)/sandbox/reddit-mentions/page.tsx (1)
239-241: Consider removing unused function or refactoring inline exportThe
_handleExportfunction is defined but never used. The export functionality is implemented inline at line 313. Consider either:
- Removing this unused function entirely
- Refactoring the inline export call to use this helper function
This would eliminate dead code and improve maintainability.
|
@izadoesdev ready for review |
|
mind fixing the last of the conflicts? @AyanavaKarmakar |
✅ Actions performedReview triggered.
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (6)
apps/dashboard/app/(main)/websites/[id]/test/components/minimal-table.tsx (1)
335-338: Potential accessibility regression: missing role="tablist"The removal of
role="tablist"from the nav element may impact accessibility. ARIA best practices recommend that elements withrole="tab"should be contained within a parent element withrole="tablist"to properly establish the tab navigation structure for assistive technologies.Consider restoring the role attribute:
<nav aria-label="Data view options" className="flex gap-0.5 rounded-lg bg-muted/40 p-0.5" + role="tablist" >This ensures screen readers can properly announce the tab group and provides the expected keyboard navigation context.
apps/dashboard/components/analytics/animated-loading.tsx (1)
3-9: Replace lucide-react icons with phosphor icons per project guidelines.The retrieved learnings indicate this project should "ONLY use phosphor icons" and not use lucide icons. This file imports multiple icons from
lucide-reactwhich violates the established guidelines.Consider replacing the lucide imports:
-import { - ActivitySquare, - ArrowRight, - Loader2, - Server, - Users, -} from 'lucide-react'; +import { + Activity, + ArrowRight, + CircleNotch, + Server, + Users, +} from '@phosphor-icons/react';Note: You may need to adjust the icon names to match phosphor equivalents and add appropriate
weightprops (typically "duotone" except for arrows which use "fill").apps/dashboard/app/(main)/sandbox/reddit-mentions/hooks/use-reddit-mentions.ts (1)
78-87:headersbuilt withUser-Agentare overwritten by the later spread ofoptionsBecause
...optionsis spread after the explicitheadersobject, anyheaders
property insideoptionswill overwrite the one that containsUser-Agent,
dropping the custom header silently.- headers: { - 'Content-Type': 'application/json', - 'User-Agent': 'Databuddy-Dashboard/1.0', - ...options.headers, - }, - signal: controller.signal, - ...options, + signal: controller.signal, + ...options, + headers: { + 'Content-Type': 'application/json', + 'User-Agent': 'Databuddy-Dashboard/1.0', + ...(options.headers ?? {}), + },apps/dashboard/app/(main)/websites/[id]/page.tsx (1)
137-144: Remove the unused function entirely instead of prefixing.The
_handleDateRangeChangefunction is never used in this component. TheDateRangePickeron lines 351-364 has its own inlineonChangehandler. Consider removing this dead code entirely.- const _handleDateRangeChange = useCallback( - (range: DayPickerRange | undefined) => { - if (range?.from && range?.to) { - setDateRangeAction({ startDate: range.from, endDate: range.to }); - } - }, - [setDateRangeAction] - );apps/dashboard/app/(auth)/login/page.tsx (1)
26-35: Remove unused function or integrate it into the component.The
_handleLastUsedfunction is never called in the component, making it dead code. Either remove it entirely or integrate it into the component's functionality (perhaps call it in auseEffectafterlastUsedis loaded).- const _handleLastUsed = () => { - if (lastUsed === 'github') { - handleGithubLogin(); - } else if (lastUsed === 'google') { - handleGoogleLogin(); - } else if (lastUsed === 'email') { - // Focus email input - document.getElementById('email')?.focus(); - } - };Alternatively, if this function should be used, add it to a
useEffect:useEffect(() => { setLastUsed(localStorage.getItem('lastUsedLogin')); + // Auto-trigger last used method after loading + const lastMethod = localStorage.getItem('lastUsedLogin'); + if (lastMethod) { + // Add logic here or call the function + } }, []);apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-section.tsx (1)
122-126: Ensure the chat auto-scrolls when new messages arriveThe two existing
useEffecthooks both use an empty dependency array ([]), so they only run once on mount. That means incoming messages won’t trigger another scroll to bottom, degrading the UX. Update the effect that callsscrollIntoViewto depend on your message list (e.g.messagesormessages.length).• File:
apps/dashboard/app/(main)/websites/[id]/assistant/components/chat-section.tsx
• Around lines 122–126Suggested change:
- useEffect(() => { - if (bottomRef.current) { - bottomRef.current.scrollIntoView({ behavior: 'smooth' }); - } - }, []); + useEffect(() => { + if (bottomRef.current) { + bottomRef.current.scrollIntoView({ behavior: 'smooth' }); + } + }, [messages]);Optionally you can use
[messages.length]if you only care about additions.
♻️ Duplicate comments (13)
apps/basket/src/polyfills/compression.js (1)
14-23: Unsupportedformatvalues still silently fall back to raw deflateThe earlier review (see past comments) already pointed out that defaulting to
zlib.createDeflateRaw()/createInflateRaw()when the caller supplies an invalidformatviolates the WHATWG Streams Compression spec and masks bugs.
The refactor kept the same behaviour; the constructors should reject anything other than"gzip" | "deflate" | "deflate-raw"with aTypeError.} else if (format === 'gzip') { handle = zlib.createGzip(); - } else { - handle = zlib.createDeflateRaw(); + } else if (format === 'deflate-raw') { + handle = zlib.createDeflateRaw(); + } else { + throw new TypeError( + `Unsupported CompressionStream format: ${format}` + ); }Apply the analogous change to the
DecompressionStreamconstructor (replacecreateInflateRaw()/createGunzip()accordingly).
Until this is fixed, unknown formats will be processed with the wrong algorithm, potentially corrupting data.Also applies to: 31-40
apps/dashboard/components/charts/metrics-constants.ts (1)
1-1:lucide-reactimport violates repo icon guidelines – switch to Phosphor iconsLong-term rules for this repository forbid
lucide-reactand require Phosphor React icons (…Icon, correctweight). Please migrate the imports (and downstream usages) accordingly and drop thelucide-reactdependency.-import { Eye, MousePointer, TrendingUp, Users } from 'lucide-react'; +import { + EyeIcon, + CursorIcon, + TrendingUpIcon, + UsersIcon, +} from '@phosphor-icons/react';After updating the import, adjust each icon usage:
• EyeIcon & UsersIcon → add
weight="duotone"
• CursorIcon & TrendingUpIcon (arrow icons) → addweight="fill"(Tagging as duplicate because this was raised in the previous review round and remains unresolved.)
apps/dashboard/lib/geo.ts (1)
18-18: Excellent type safety improvement!The change from
any[][][]tonumber[][][]for coordinates properly addresses the previous review feedback and aligns with project guidelines for type safety. Geographic coordinates are indeed numeric values, making this type annotation both accurate and meaningful.Also applies to: 35-35
apps/dashboard/components/error-boundary.tsx (1)
3-3: Lucide icons still need to be replaced with phosphor icons.This file still imports
AlertTriangleandRefreshCwfromlucide-react, which violates project guidelines requiring the use of phosphor icons only.apps/database/app/sql/page.tsx (1)
3-3: The Lucide icons import issue remains unresolved.The import from 'lucide-react' still violates the project rule requiring Phosphor icons. This should be replaced with '@phosphor-icons/react' imports using names ending with 'Icon'.
apps/dashboard/app/(main)/websites/[id]/errors/_components/errors-page-content.tsx (1)
55-64: Remove unused functions entirely.These functions are still unused after being renamed with underscore prefixes. As identified in previous reviews, both
_removeFilterand_clearFiltersare never invoked and should be removed entirely to clean up dead code.- // Remove a filter - const _removeFilter = (filterToRemove: DynamicQueryFilter) => { - setActiveFilters((prev) => - prev.filter( - (f) => - !( - f.field === filterToRemove.field && f.value === filterToRemove.value - ) - ) - ); - }; - - // Clear all filters - const _clearFilters = () => { - setActiveFilters([]); - };Also applies to: 67-69
apps/dashboard/app/(main)/sandbox/reddit-mentions/hooks/use-reddit-mentions.ts (1)
152-158: Still guarding only onerrorinstead of genericsuccess === false
This was raised in a previous review and remains unresolved.When the backend returns
{ success: false, error: undefined }the current check
treats the call as success and propagatesundefineddata.-if (result.error) { - throw new Error(result.error); -} +if (!result.success) { + throw new Error(result.error ?? 'Request failed'); +}Apply the same change to
getHealth,refresh, andgetAnalytics.Also applies to: 181-184, 198-200, 235-237
apps/dashboard/public/databuddy.js (1)
1358-1359: Browser compatibility concern with biome-ignore comment.The comment indicates awareness of browser compatibility issues with the
.at()method. Based on past review feedback, this method is not supported in all target browsers and can cause TypeError in older Safari/Firefox versions.The current implementation correctly uses the backward-compatible approach
scripts[scripts.length - 1], which is good. However, verify that other uses of.at(-1)in this file (like line 893) are intentional and have appropriate fallbacks if needed.apps/dashboard/app/(auth)/login/magic-sent/page.tsx (1)
4-4: Still using lucide-react icons instead of Phosphor icons.This file continues to import and use icons from
lucide-react(ChevronLeft,Loader2,MailCheck) despite the coding guidelines requiring Phosphor icons exclusively. This was flagged in previous reviews but remains unaddressed.The retrieved learnings clearly state: "Don't use lucide for icons, ONLY use phosphor icons, use weight='duotone' for most, but for arrows use fill, for plus icons don't add weight"
-import { ChevronLeft, Loader2, MailCheck } from 'lucide-react'; +import { CaretLeftIcon, SpinnerIcon, EnvelopeSimpleOpenIcon } from '@phosphor-icons/react';And update the JSX usage accordingly:
ChevronLeft→CaretLeftIconwithweight="fill"Loader2→SpinnerIconwithweight="duotone"MailCheck→EnvelopeSimpleOpenIconwithweight="duotone"apps/dashboard/app/(main)/organizations/[slug]/components/invite-member-dialog.tsx (1)
44-46: Stylistic improvement but silent return issue persists.The explicit block formatting is good for consistency. However, the silent return when email is empty still provides no user feedback, as noted in previous reviews.
apps/dashboard/app/not-found.tsx (1)
3-3: Good cleanup but coding guideline violation remains.Removing the unused
BarChartimport is good. However, the file still violates coding guidelines by using lucide-react icons instead of phosphor icons, as noted in previous reviews.apps/dashboard/app/(auth)/register/page.tsx (1)
122-122: Inconsistent error handling pattern.These catch blocks still swallow errors by renaming to
_error, while line 98 properly logs them. Apply the same error logging pattern for consistency and better observability.-} catch (_error) { +} catch (error) { + console.error('resend-verification-email', error); toast.error('Failed to send verification email. Please try again later.');-} catch (_error) { +} catch (error) { + console.error('social-login', error); toast.error('Something went wrong');Also applies to: 148-148
apps/dashboard/app/(main)/websites/[id]/map/page.tsx (1)
180-191: Array index still used in React keys despite previous feedback.The array creation syntax change is fine, but the existing issue with using array index in React keys remains unaddressed from previous reviews.
Using
ias part of the key breaks React's reconciliation when list length changes. Consider using a constant string or UUID for these static skeleton elements:-{new Array(6).fill(0).map((_, i) => ( +{Array.from({ length: 6 }, (_, idx) => ( <div className="flex items-center justify-between p-3" - key={`country-skeleton-${i + 1}`} + key={`country-skeleton-${idx}`} >
🧹 Nitpick comments (10)
apps/dashboard/app/(main)/websites/[id]/revenue/page.tsx (1)
69-69: Consider standardizing array instantiation patterns throughout the file.While this change is good, there's inconsistency within the same file:
- Line 69:
[...new Array(4)](updated)- Line 92:
Array.from({ length: 4 })(different pattern)- Line 334:
[...new Array(5)](already using new Array)Consider standardizing to one approach for consistency.
apps/database/app/api/database/stats/route.ts (2)
7-32: Run the three read-only queries in parallel to shave off latencyThe three independent
chQuerycalls (tableStats,databases,systemInfo) plus the optionalmemoryInfoandrecentQueriescan be executed concurrently.
With the current sequential awaits, total latency ≈ Σ(query RTT). UsingPromise.allmakes it ≈ max(query RTT) which is a big win for an edge API route.-// Get table statistics -const tableStats = await chQuery(`...`); - -// Get database list with sizes -const databases = await chQuery(`...`); - -// Get system information -const systemInfo = await chQuery(`...`); +const [tableStats, databases, systemInfo] = await Promise.all([ + chQuery(`...table statistics query...`), + chQuery(`...database list query...`), + chQuery(`...system info query...`), +]);You can wrap the optional parts as well:
-try { - memoryInfo = await chQuery(`...`); -} catch (error) { - ... -} +const [memoryInfo = [], recentQueries = defaultRecent] = await Promise.allSettled([ + chQuery(`...memory...`), + chQuery(`...recent queries...`) +]).then(r => + r.map(p => (p.status === 'fulfilled' ? p.value : [])) +);This keeps the same error-handling semantics while delivering a faster endpoint.
Also applies to: 41-77
79-98: Add explicit result typing & runtime guardsThe object literals assume the ClickHouse column names and types never change. If the query or DB version drifts, this handler could throw at runtime (e.g.,
tableStats[0]isundefined). Add a small helper for safe extraction and strong typing:interface OverviewRow { total_tables: number; total_rows: bigint; total_bytes: bigint; } function firstOrDefault<T>(rows: unknown[], fallback: T): T { return (Array.isArray(rows) && rows.length > 0 ? rows[0] : fallback) as T; } // usage const overview = firstOrDefault<OverviewRow>(tableStats, { total_tables: 0, total_rows: 0n, total_bytes: 0n, });Benefits:
• Compile-time checks on field names
• Fewer optional-chaining / default-value fallbacks scattered through the body
• Early detection of schema drift during CI type-checks.apps/dashboard/app/layout.tsx (1)
134-137: Load external script over HTTPS and pin with SRIProtocol-relative URLs (
//…) fall back to plain HTTP when the dev server itself is served overhttp://, which opens a MITM window. Switching to an explicithttps://URL (and ideally adding anintegrityhash) eliminates the risk and avoids mixed-content warnings. A light performance win can also be gained by deferring the load withstrategy="lazyOnload".- <Script - crossOrigin="anonymous" - src="//unpkg.com/react-scan/dist/auto.global.js" - /> + <Script + src="https://unpkg.com/react-scan/dist/auto.global.js" + integrity="sha384-<computed-hash>" + crossOrigin="anonymous" + strategy="lazyOnload" + />apps/dashboard/components/ui/sidebar.tsx (1)
90-94: Drop the meaninglessreturnfor clearer intent
toggleSidebarcurrently returns the (undefined) result of a state-setter. Returning this value serves no purpose and may confuse readers into thinking the return value is used.-const toggleSidebar = React.useCallback(() => { - return isMobile ? setOpenMobile((open) => !open) : setOpen((open) => !open); -}, [isMobile, setOpen]); +const toggleSidebar = React.useCallback(() => { + if (isMobile) { + setOpenMobile((prev) => !prev); + } else { + setOpen((prev) => !prev); + } +}, [isMobile, setOpen]);apps/dashboard/app/(main)/websites/[id]/_components/utils/code-generators.ts (1)
15-17: Misleading variable name with underscore prefix.The variable
apiUrlwas renamed to_apiUrl, but the underscore prefix typically indicates an unused variable. This could be misleading if the variable is actually used elsewhere in the code or will be used in the future.Consider using a more descriptive name without the underscore prefix if this variable serves a purpose, or remove it entirely if it's truly unused.
apps/dashboard/app/(main)/billing/hooks/use-billing.ts (1)
26-30: Remove the unused_resultvariable to avoid dead-code warnings
_resultis never read afterawait attach().
Either remove the assignment or handle the returned session URL.- const _result = await attach({ + await attach({apps/dashboard/app/(main)/sandbox/reddit-mentions/page.tsx (1)
239-241: Unused_handleExporthelperThe helper wraps
exportMutation.mutatebut is never invoked. Delete it or wire
it to the “Export CSV” button to avoid dead code and future confusion.apps/dashboard/hooks/use-dynamic-query.ts (1)
291-293: No-op underscore rename only silences the linterRenaming
indexto_indexwithout removing it from the parameter list keeps
creating the array element but the variable is unused. Consider dropping the
parameter entirely:-return query.data.results.map((result, _index) => { +return query.data.results.map((result) => {apps/dashboard/app/(auth)/login/magic/page.tsx (1)
41-41: Good naming convention for unused parameter.The underscore prefix correctly indicates the error parameter is intentionally unused. Consider logging the error for debugging purposes:
} catch (_error) { + console.error('Magic link error:', _error); setIsLoading(false); toast.error('Failed to send magic link. Please try again.'); }
|
@izadoesdev should be good now |
2 participants
Description
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests
Documentation
No user-facing features or functionality were changed.