Ultracite fixes#154
Conversation
|
@haydenbleasel is attempting to deploy a commit to the Databuddy Team on Vercel. A member of the Team first needs to authorize it. |
|
|
How to use the Graphite Merge QueueAdd the label Main to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
* quick fixxx * Delete packages/db/src/drizzle/meta/0000_snapshot.json * Delete packages/db/src/drizzle/meta/_journal.json
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughWidespread refactors: lint/type annotation changes, regex constant extraction, accessibility/keyboard tweaks, UI attribute standardization, several signature changes (sync vs async), component prop removals, FlagStorage API converted to synchronous, and structured-data pricing adjusted for tiered event overage. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Consumer (app/sdk)
participant FlagStorage as FlagStorage (localStorage)
Note over App,FlagStorage: FlagStorage API converted from async to synchronous
App->>FlagStorage: set(key, value)
FlagStorage-->>App: void
App->>FlagStorage: get(key)
FlagStorage-->>App: value | undefined
App->>FlagStorage: getAll()
FlagStorage->>FlagStorage: scan keys 'db-flag-*', parse, drop expired
FlagStorage-->>App: Record<string, unknown>
App->>FlagStorage: cleanupExpired()
FlagStorage->>FlagStorage: remove expired items
FlagStorage-->>App: void
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (47)
apps/dashboard/components/analytics/map-component.tsx (3)
312-324: Remove dead state/effect:_resolvedHeightis never read.This state and the resize listener do unnecessary work every render; nothing uses
_resolvedHeight. Drop both to cut re-renders and event listeners.- const [_resolvedHeight, setResolvedHeight] = useState<number>(0); - - useEffect(() => { - const updateHeight = () => { - if (containerRef.current) { - setResolvedHeight(containerRef.current.clientHeight); - } - }; - - updateHeight(); - window.addEventListener('resize', updateHeight); - return () => window.removeEventListener('resize', updateHeight); - }, []); + // removed: unused height measurement
159-167: Border color alpha construction produces invalid CSS (rgb(r,g,b, a)).You’re appending an alpha channel to
rgb()via.replace(...), yieldingrgb(r,g,b, a)which isn’t valid. Usergba(r,g,b,a)(or modernrgb(r g b / a)), otherwise Leaflet may ignore the color.Apply this minimal refactor to return raw RGB triplets and build
rgb/rgbastrings explicitly:- const getThemeColors = useCallback(() => { - const isDark = resolvedTheme === 'dark'; - return { - // More saturated blue colors - primary: isDark ? 'rgb(59, 130, 246)' : 'rgb(37, 99, 235)', // blue-500 / blue-600 - muted: isDark ? 'rgb(75, 85, 99)' : 'rgb(156, 163, 175)', // gray-600 / gray-400 - isDark, - }; - }, [resolvedTheme]); + const getThemeColors = useCallback(() => { + const isDark = resolvedTheme === 'dark'; + return { + primaryRgb: isDark ? '59, 130, 246' : '37, 99, 235', // blue-500 / blue-600 + mutedRgb: isDark ? '75, 85, 99' : '156, 163, 175', // gray-600 / gray-400 + isDark, + }; + }, [resolvedTheme]); const getBorderColor = useCallback( ( hasData: boolean, isHovered: boolean, colors: ReturnType<typeof getThemeColors> ) => { if (!hasData) { - return `${colors.muted.replace(')', ', 0.5)')}`; + return `rgba(${colors.mutedRgb}, 0.5)`; } - return isHovered - ? colors.primary - : `${colors.primary.replace(')', ', 0.6)')}`; + return isHovered + ? `rgb(${colors.primaryRgb})` + : `rgba(${colors.primaryRgb}, 0.6)`; }, [] );Also applies to: 169-183
411-423: Remove incorrect ARIA role on non-tab UI.
role="tablist"misrepresents this container to assistive tech; there are notabdescendants or keyboard semantics. Remove the role (or implement full Tabs semantics).- <div + <div className="relative cursor-pointer" onMouseMove={(e) => { if (tooltipContent) { setTooltipPosition({ x: e.clientX, y: e.clientY, }); } }} ref={containerRef} - role="tablist" style={{ height }} >apps/dashboard/hooks/use-dynamic-query.ts (1)
698-715: Replace explicit any with a precise type and fix SSR/relative-URL handling for referrer parsing.Using
anyviolates the repo rule to avoidany, and referencingwindowwill misclassify referrers on SSR or whenreferreris relative (falls back to "direct"). Guardwindow, support relative URLs, and use a union type.Apply this diff within the selected lines:
- let referrerParsed: any = null; + let referrerParsed: ReferrerParsed | null = null; if (session.referrer) { try { - const url = new URL(session.referrer); - referrerParsed = { - type: - url.hostname === window.location.hostname ? 'internal' : 'external', - name: url.hostname, - domain: url.hostname, - }; + const base = + typeof window !== 'undefined' ? window.location.origin : undefined; + const url = base + ? new URL(session.referrer, base) + : new URL(session.referrer); + const currentHost = + typeof window !== 'undefined' ? window.location.hostname : undefined; + const isInternal = currentHost ? url.hostname === currentHost : false; + referrerParsed = { + type: isInternal ? 'internal' : 'external', + name: url.hostname, + domain: url.hostname, + }; } catch { - referrerParsed = { - type: 'direct', - name: 'Direct', - domain: null, - }; + if ( + typeof window !== 'undefined' && + typeof session.referrer === 'string' && + session.referrer.startsWith('/') + ) { + const host = window.location.hostname; + referrerParsed = { type: 'internal', name: host, domain: host }; + } else { + referrerParsed = { type: 'direct', name: 'Direct', domain: null }; + } } }Add this type alias once (e.g., near the top of the file):
type ReferrerParsed = | { type: 'internal' | 'external'; name: string; domain: string } | { type: 'direct'; name: 'Direct'; domain: string | null };apps/dashboard/app/(main)/websites/[id]/_components/tabs/overview-tab.tsx (1)
476-495: Blocker: dayjs plugins are extended after first use (runtime crash risk).
filterFutureEventscallsdayjs.utc(...).tz(...)beforedayjs.extend(utc/timezone)runs, which can throw “tz is not a function” on initial render. Extend plugins at module top (once) and remove late extensions.Apply:
import dayjs from 'dayjs'; import timezone from 'dayjs/plugin/timezone'; import utc from 'dayjs/plugin/utc'; +dayjs.extend(utc); +dayjs.extend(timezone);And remove late extensions:
- dayjs.extend(utc); - dayjs.extend(timezone);Also applies to: 919-923
apps/dashboard/app/(main)/settings/_components/password-form.tsx (2)
169-172: Add autocomplete to password fields.Per guidelines, set appropriate autocomplete tokens so password managers behave correctly.
Apply this diff:
- placeholder="Enter your new password" + placeholder="Enter your new password" + autoComplete="new-password" type={showNewPassword ? 'text' : 'password'} {...field}- placeholder="Confirm your new password" + placeholder="Confirm your new password" + autoComplete="new-password" type={showConfirmPassword ? 'text' : 'password'} {...field}Also applies to: 257-260
178-198: Icon‑only toggle buttons need accessible names (and pressed state).Screen‑reader users currently get no purpose for these controls.
Apply this diff:
- <Button + <Button className="-translate-y-1/2 absolute top-1/2 right-1 h-8 w-8 transform p-0" onClick={() => setShowNewPassword(!showNewPassword)} size="sm" type="button" variant="ghost" + aria-label={showNewPassword ? 'Hide password' : 'Show password'} + aria-pressed={showNewPassword} >- <Button + <Button className="-translate-y-1/2 absolute top-1/2 right-1 h-8 w-8 transform p-0" onClick={() => setShowConfirmPassword(!showConfirmPassword) } size="sm" type="button" variant="ghost" + aria-label={showConfirmPassword ? 'Hide password confirmation' : 'Show password confirmation'} + aria-pressed={showConfirmPassword} >Also applies to: 266-288
apps/dashboard/hooks/use-funnels.ts (2)
268-275: React Hooks misuse: calling a hook insidequeryFn(not top-level).
trpc.funnels.getAnalytics.useQueryis a hook; invoking it insideuseQueries({ queryFn })violates the Rules of Hooks and can crash or misbehave. Use the tRPC client inqueryFninstead.Apply:
- queryFn: () => - // biome-ignore lint/correctness/useHookAtTopLevel: "trpc works this way" - trpc.funnels.getAnalytics.useQuery({ - websiteId, - funnelId, - startDate: dateRange?.start_date, - endDate: dateRange?.end_date, - }), + queryFn: () => + utils.client.funnels.getAnalytics.query({ + websiteId, + funnelId, + startDate: dateRange?.start_date, + endDate: dateRange?.end_date, + }),Add (outside the diff range) at the top of
useFunnelComparison:const utils = trpc.useUtils();
315-322: Same Hooks violation inuseFunnelPerformance. Use the client inqueryFn.Apply:
- queryFn: () => - // biome-ignore lint/correctness/useHookAtTopLevel: "trpc works this way" - trpc.funnels.getAnalytics.useQuery({ - websiteId, - funnelId: funnel.id, - startDate: dateRange?.start_date, - endDate: dateRange?.end_date, - }), + queryFn: () => + utils.client.funnels.getAnalytics.query({ + websiteId, + funnelId: funnel.id, + startDate: dateRange?.start_date, + endDate: dateRange?.end_date, + }),Add (outside the diff range) at the top of
useFunnelPerformance:const utils = trpc.useUtils();apps/dashboard/app/(main)/billing/cost-breakdown/components/consumption-chart.tsx (1)
90-97: Replaceanywith a concrete chart datum type.Strengthen types and keep keys aligned with
EVENT_TYPE_COLORS.Apply:
- const dayData: any = { + const dayData: ChartDatum = { date: new Date(date).toLocaleDateString('en-US', { month: 'short', day: 'numeric', }), fullDate: date, };Add above the component:
type EventKey = keyof typeof EVENT_TYPE_COLORS; type ChartDatum = { date: string; fullDate: string } & Record<EventKey, number>; const EVENT_KEYS = Object.keys(EVENT_TYPE_COLORS) as EventKey[];apps/dashboard/app/(main)/settings/integrations/vercel/_components/project-row.tsx (2)
100-131: Replace any[] with a typed ActionItem[] and fix optional-chaining on issuesTyping this array prevents silent regressions, and
issues?.[0]avoids a potential runtime error whenissuesis undefined.- const getTriageActions = (domainStatus: any) => { - const actions: any[] = []; + const getTriageActions = (domainStatus: any): ActionItem[] => { + type ActionItem = { + label: string; + icon: (props: { className?: string }) => JSX.Element; + action: TriageAction; + }; + const actions: ActionItem[] = []; @@ - if ( - domainStatus.status === 'invalid' && - domainStatus.issues[0]?.includes('Multiple') - ) { + if ( + domainStatus.status === 'invalid' && + domainStatus.issues?.[0]?.includes('Multiple') + ) {
582-605: Fix double-toggle bug and invalid div attribute; use a proper checkbox+labelThe wrapper div both clicks and the checkbox change handler fire, causing a double toggle; and
type="button"on a div is invalid. Rely on the checkbox with a proper label.- <div - className="flex cursor-pointer items-center gap-2" - onClick={handleSelectAll} - onKeyUp={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - handleSelectAll(); - } - }} - type="button" - > - <Checkbox - checked={ - selectedDomains.size === - filteredDomains.length && - filteredDomains.length > 0 - } - className="cursor-pointer" - onCheckedChange={handleSelectAll} - /> - <span className="text-muted-foreground text-xs"> - Select All - </span> - </div> + <div className="flex items-center gap-2"> + <Checkbox + id={`select-all-${project.id}`} + checked={ + selectedDomains.size === filteredDomains.length && + filteredDomains.length > 0 + } + className="cursor-pointer" + onCheckedChange={handleSelectAll} + /> + <label + htmlFor={`select-all-${project.id}`} + className="cursor-pointer text-muted-foreground text-xs" + > + Select All + </label> + </div>packages/rpc/src/lib/vercel-sdk.ts (2)
341-351: Fix:_optionsignored → breaks team/slug scoping. Use and plumb through togetProjectEnvs.This currently fetches envs without scoping, causing incorrect results across teams.
Apply this diff:
-async getProjectEnvByKey( +async getProjectEnvByKey( projectId: string, key: string, - _options: { + options: { teamId?: string; slug?: string; } = {} ): Promise<VercelEnvVar | null> { - const envs = await this.getProjectEnvs(projectId); + const envs = await this.getProjectEnvs(projectId, options); return envs.envs.find((env) => env.key === key) || null; }
353-364: Same issue: plumboptionsto env list and remove underscore.Apply this diff:
-async getProjectEnvsByKey( +async getProjectEnvsByKey( projectId: string, key: string, - _options: { + options: { teamId?: string; slug?: string; } = {} ): Promise<VercelEnvVar[]> { - const envs = await this.getProjectEnvs(projectId); + const envs = await this.getProjectEnvs(projectId, options); return envs.envs.filter((env) => env.key === key); }apps/database/components/data-table.tsx (1)
160-163: Bug:useStateused likeuseEffect(will not compile/run)This should be an effect that resets pagination when filters/sort change. Also include
pageSize.-import { useMemo, useState } from 'react'; +import { useEffect, useMemo, useState } from 'react'; ... -// Reset pagination when filters change -useState(() => { - setCurrentPage(1); -}, [searchTerm, columnFilters, sortColumn, sortDirection]); +// Reset pagination when filters change +useEffect(() => { + setCurrentPage(1); +}, [searchTerm, columnFilters, sortColumn, sortDirection, pageSize]);apps/database/components/sql-editor.tsx (3)
314-324: Replace clickable div with a semantic button (fix focusability and keyboard semantics).A non-interactive div with onClick/onKeyUp isn’t focusable, so keyboard users can’t activate it. Use a native button (type="button") and drop custom key handling; native Enter/Space will work and you’ll satisfy the “no handlers on non-interactive elements” guideline.
- <div - className="cursor-pointer rounded border p-3 transition-colors hover:bg-muted/50" - key={item.id} - onClick={() => loadFromHistory(item.query)} - onKeyUp={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - loadFromHistory(item.query); - } - }} - > + <button + type="button" + className="w-full text-left rounded border p-3 transition-colors hover:bg-muted/50 focus:outline-none focus:ring-2 focus:ring-ring" + key={item.id} + onClick={() => loadFromHistory(item.query)} + > ... - </div> + </button>If you must keep a div, add role="button", tabIndex={0}, and handle Enter/Space on keyDown (not keyUp).
- onKeyUp={(e) => { + onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); loadFromHistory(item.query); } }} + role="button" + tabIndex={0}
352-361: Icon-only “Copy” control needs an accessible name.This button has only an icon. Add aria-label (and optional title) so screen readers announce it.
- <Button + <Button onClick={(e) => { e.stopPropagation(); copyToClipboard(item.query); }} size="sm" variant="ghost" + aria-label="Copy query to clipboard" + title="Copy query to clipboard" >
352-359: Set Button's default to type="button" (or add an explicit type where used).apps/database/components/ui/button.tsx, apps/dashboard/components/ui/button.tsx, apps/docs/components/ui/button.tsx currently spread {...props} without a default type — set a default (e.g. type={props.type ?? 'button'} or destructure type = 'button') so usages like apps/database/components/sql-editor.tsx:352-359 (copy button) don't accidentally submit forms.
apps/dashboard/app/(main)/settings/_components/two-factor-form.tsx (1)
332-338: Security and accessibility concerns with QR code generationThe QR code is generated using an external service (qrserver.com) which could pose security risks for sensitive 2FA setup data. Additionally, the img element lacks proper error handling.
Consider these alternatives:
- Generate QR codes client-side using a library like
qrcode- Add error handling and fallback for the image
- If using external service is necessary, ensure HTTPS and validate the service's security
// Alternative: Client-side QR generation import QRCode from 'qrcode'; // In your component: const [qrCodeDataURL, setQrCodeDataURL] = useState<string>(''); useEffect(() => { if (qrCodeURI) { QRCode.toDataURL(qrCodeURI).then(setQrCodeDataURL); } }, [qrCodeURI]); // Then use: <img src={qrCodeDataURL} ... />apps/dashboard/components/ai-elements/tool.tsx (1)
66-79: CollapsibleTrigger should declare type="button".Avoids accidental form submits when placed inside forms.
- <CollapsibleTrigger + <CollapsibleTrigger + type="button" className={cn( 'flex w-full items-center justify-between gap-4 p-3', className )} {...props} >apps/docs/app/(home)/ambassadors/ambassador-form.tsx (2)
154-159: Replaceanywith a typed API response (aligns with repo rule “Don’t use any”).Define a response type and use it instead of
any. Also cast the parsed JSON to that type.+type AmbassadorSubmitResponse = { + error?: string; + details?: string[] | string; + resetTime?: string | number; +}; @@ - let data: any; + let data: AmbassadorSubmitResponse; try { - data = await response.json(); + data = (await response.json()) as AmbassadorSubmitResponse; } catch (_parseError) { throw new Error('Invalid response from server. Please try again.'); }
34-60: Associate labels with controls and add autocomplete/ARIA parity.Labels aren’t programmatically associated with inputs, and only “Name” has ARIA wiring. Add
htmlForon labels +idon fields; addautocompleteand ARIA for email/textarea.@@ -function FormField({ +function FormField({ label, required = false, children, description, error, -}: { + htmlFor, +}: { label: string; required?: boolean; children: React.ReactNode; description?: string; error?: string; + htmlFor?: string; }) { return ( <div className="space-y-2"> - <Label className="text-foreground"> + <Label className="text-foreground" htmlFor={htmlFor}> {label} {required && <span className="ml-1 text-destructive">*</span>} </Label> {children} @@ - <FormField error={errors.name} label="Full Name" required> + <FormField htmlFor="name" error={errors.name} label="Full Name" required> <Input aria-describedby={ errors.name ? 'name-error' : 'name-description' } aria-invalid={!!errors.name} className={errors.name ? 'border-destructive' : ''} id="name" + autoComplete="name" maxLength={100} name="name" onChange={handleInputChange} placeholder="John Doe" required type="text" value={formData.name} /> @@ - <FormField error={errors.email} label="Email Address" required> + <FormField htmlFor="email" error={errors.email} label="Email Address" required> <Input className={errors.email ? 'border-destructive' : ''} + id="email" + aria-invalid={!!errors.email} + aria-describedby={errors.email ? 'email-error' : undefined} maxLength={255} + autoComplete="email" name="email" onChange={handleInputChange} placeholder="john@example.com" required type="email" value={formData.email} /> + {errors.email && ( + <div className="sr-only" id="email-error"> + {errors.email} + </div> + )} @@ - <FormField + <FormField + htmlFor="whyAmbassador" description="Required field (max 1000 characters)" error={errors.whyAmbassador} label="Why do you want to be a Databuddy ambassador?" required > <Textarea className={errors.whyAmbassador ? 'border-destructive' : ''} + id="whyAmbassador" + aria-invalid={!!errors.whyAmbassador} maxLength={1000} name="whyAmbassador" onChange={handleInputChange} placeholder="I believe in privacy-first analytics and want to help spread awareness about better data practices..." required rows={4} value={formData.whyAmbassador} />Follow the same pattern for xHandle (id + htmlFor + autoComplete="username"), website (id + htmlFor + autoComplete="url" + inputMode="url"), experience/audience (id + htmlFor), and referralSource (id + htmlFor + autoComplete="off").
Also applies to: 262-286, 288-299, 356-375
packages/db/src/clickhouse/client.ts (1)
154-168: Replace deprecated globalescape()and safely quote SQL date literals in toDateescape() is deprecated and doesn't quote SQL — use DATE_REGEX.test and explicit single-quote escaping; update packages/db/src/clickhouse/client.ts (lines 154–168).
export function toDate(str: string, interval?: string) { if (!interval || interval === 'minute' || interval === 'hour') { - if (str.match(DATE_REGEX)) { - return escape(str); - } + // Plain date literal → safely quoted string + if (DATE_REGEX.test(str)) { + return `'${str.replace(/'/g, "''")}'`; + } return str; } - if (str.match(DATE_REGEX)) { - return `toDate(${escape(str.split(' ')[0])})`; - } + if (DATE_REGEX.test(str)) { + const d = str.split(' ')[0]; + return `toDate('${d.replace(/'/g, "''")}')`; + } return `toDate(${str})`; }apps/dashboard/app/(main)/sandbox/api-testing/page.tsx (2)
195-201: Fix hardcoded “200 OK” and surface real status.UI always shows 200 OK, even on errors. Track status and render accordingly.
Apply these diffs:
Add status state:
const [response, setResponse] = useState(''); const [isLoading, setIsLoading] = useState(false); const [copied, setCopied] = useState(false); + const [status, setStatus] = useState<number | null>(null);Reset status on new test:
- setIsLoading(true); + setIsLoading(true); + setStatus(null);Set status on success and error:
- setResponse(JSON.stringify(mockResponse, null, 2)); + setResponse(JSON.stringify(mockResponse, null, 2)); + setStatus(mockResponse.status);-} catch (error) { +} catch (error) { + setStatus(500);Render dynamic badge and text:
- <div className="flex items-center gap-2"> - <Badge className="text-green-600" variant="outline"> - 200 OK - </Badge> - <span className="text-muted-foreground text-sm"> - Response received - </span> - </div> + <div className="flex items-center gap-2"> + <Badge + className={status && status >= 200 && status < 300 ? 'text-green-600' : 'text-red-600'} + variant="outline" + > + {status ?? '—'} + </Badge> + <span className="text-muted-foreground text-sm"> + {status && status >= 200 && status < 300 ? 'Response received' : 'Request failed'} + </span> + </div>Also applies to: 64-65, 65-75, 26-28, 30-31
3-3: Swap Lucide icons for Phosphor (Icon suffix + duotone)Phosphor is already installed (package.json); update apps/dashboard/app/(main)/sandbox/api-testing/page.tsx — import at line 3 and usages around lines 159–163, 182–187.
-import { Check, Copy, Loader2, Send } from 'lucide-react'; +import CheckIcon from '@phosphor-icons/react/Check'; +import CopyIcon from '@phosphor-icons/react/Copy'; +import CircleNotchIcon from '@phosphor-icons/react/CircleNotch'; +import PaperPlaneRightIcon from '@phosphor-icons/react/PaperPlaneRight';- <Loader2 className="h-4 w-4 animate-spin" /> + <CircleNotchIcon className="h-4 w-4 animate-spin" weight="duotone" aria-hidden="true" role="status" />- <Send className="h-4 w-4" /> + <PaperPlaneRightIcon className="h-4 w-4" weight="duotone" aria-hidden="true" />- <Check className="h-4 w-4" /> + <CheckIcon className="h-4 w-4" weight="duotone" aria-hidden="true" />- <Copy className="h-4 w-4" /> + <CopyIcon className="h-4 w-4" weight="duotone" aria-hidden="true" />apps/docs/components/structured-data.tsx (3)
77-77: Replaceany[]with schema.org-typed price specs.Avoid
anyin TS. Useschema-dts’sUnitPriceSpecificationfor strong typing.-const priceSpecs: any[] = []; +const priceSpecs: UnitPriceSpecification[] = [];Add this import near the top of the file:
import type { UnitPriceSpecification } from 'schema-dts';
187-187: Strongly type the JSON-LD graph.Use
Thing[]fromschema-dtsinstead ofany[].-const graph: any[] = []; +const graph: Thing[] = [];Add to the imports:
import type { Thing } from 'schema-dts';
314-319: Emit numeric lowPrice/highPrice (not strings)Google Rich Results expects numeric AggregateOffer.lowPrice/highPrice; .toFixed(2) returns a string. File: apps/docs/components/structured-data.tsx (lines 314–319)
lowPrice: Math.min( ...offers.map((o) => Number(o.price ?? 0)) ).toFixed(2), highPrice: Math.max( ...offers.map((o) => Number(o.price ?? 0)) ).toFixed(2),Remove .toFixed(2) or wrap the result in Number(...) so the JSON‑LD emits numeric values (e.g., Number(...toFixed(2)) or drop toFixed).
apps/dashboard/components/empty-state.tsx (2)
92-110: Remove custom key handler (can double-trigger) and gate the badge by action; simplify button a11y.Buttons already handle Enter/Space. The onKeyDown here can cause duplicate invocations (notably for Space), and tabIndex={0} is redundant. Also avoid rendering a clickable control if no action is provided. Add aria-hidden to the decorative SVG and prefer a contextual aria-label.
- {showPlusBadge && ( - <button - aria-label="Create new item" + {showPlusBadge && action && ( + <button + aria-label={action.label || 'Create new item'} className="-top-2 -right-2 absolute cursor-pointer select-none rounded-full border-2 border-primary/10 bg-background p-2" onClick={(e) => { e.stopPropagation(); - action?.onClick(); + action.onClick(); }} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - action?.onClick(); - } - }} - tabIndex={0} type="button" > - <PlusIcon className="h-6 w-6 text-primary" size={16} /> + <PlusIcon aria-hidden className="h-6 w-6 text-primary" size={16} /> </button> )}Also applies to: 91-91, 108-108
133-136: Ensure a single main landmark per page — don’t set role="main" in reusable components.This component uses role={isMainContent ? 'main' : role}. The repo already contains other
landmarks; remove the main role here or ensure isMainContent is only true when this component is the page’s primary content. Check these locations as examples:
- apps/dashboard/components/empty-state.tsx:135 — role={isMainContent ? 'main' : role}
- apps/dashboard/app/layout.tsx:139 — {children}
- apps/dashboard/app/demo/layout.tsx:265 —
- apps/dashboard/app/(main)/organizations/components/organization-provider.tsx:142, 188, 253 — multiple
- apps/dashboard/app/(main)/settings/page.tsx:170 —
- apps/dashboard/components/ui/sidebar.tsx:308 — SidebarInset renders
apps/database/app/table/[database]/[table]/schema/page.tsx (10)
160-166: Client page must not be async; params typing is wrong (use useParams instead).This is a Client Component; remove
asyncand read route params viauseParams. The currentparams: Promise<...>+awaitwill misbehave.- export default async function TableSchemaPage({ - params, - }: { - params: Promise<{ database: string; table: string }>; - }) { - const { database, table } = await params; + export default function TableSchemaPage() { + const { database, table } = useParams<{ database: string; table: string }>();Also update imports:
- import { useRouter } from 'next/navigation'; - import { useEffect, useState } from 'react'; + import { useParams, useRouter } from 'next/navigation'; + import { useCallback, useEffect, useState } from 'react';
196-221: SQL injection/identifier safety: quote identifiers and strings.User-controlled
database/tableflow into SQL. Introduce helpers and use a fully‑qualified, quoted table name.const tableName = `${database}.${table}`; +// SQL quoting helpers +const qIdent = (id: string) => `"${id.replace(/"/g, '""')}"`; +const qString = (s: string) => `'${s.replace(/'/g, "''")}'`; +const fqTableName = `${qIdent(database)}.${qIdent(table)}`; ... - body: JSON.stringify({ - query: `DESCRIBE TABLE ${tableName}`, - }), + body: JSON.stringify({ + query: `DESCRIBE TABLE ${fqTableName}`, + }),Apply the same pattern throughout all queries in this file.
228-241: Use string literal quoting in WHERE clause.Avoid raw interpolation in SQL string comparisons.
- WHERE database = '${database}' - AND name = '${table}' + WHERE database = ${qString(database)} + AND name = ${qString(table)}
280-289: Sanitize DDL in Add Column.Quote identifiers and escape comments.
- let query = `ALTER TABLE ${tableName} ADD COLUMN ${newColumn.name} ${newColumn.type}`; + let query = `ALTER TABLE ${fqTableName} ADD COLUMN ${qIdent(newColumn.name)} ${newColumn.type}`; ... - if (newColumn.comment) { - query += ` COMMENT '${newColumn.comment}'`; + if (newColumn.comment) { + query += ` COMMENT ${qString(newColumn.comment)}`; }
321-327: Sanitize COMMENT COLUMN.- const query = `ALTER TABLE ${tableName} COMMENT COLUMN ${column.name} '${column.comment}'`; + const query = `ALTER TABLE ${fqTableName} COMMENT COLUMN ${qIdent(column.name)} ${qString(column.comment)}`;
343-353: Sanitize DROP COLUMN.- query: `ALTER TABLE ${tableName} DROP COLUMN ${columnName}`, + query: `ALTER TABLE ${fqTableName} DROP COLUMN ${qIdent(columnName)}`,
367-397: Stats queries: quote identifiers and table, avoid raw interpolation.- const uniqueQuery = `SELECT count(DISTINCT "${column.name}") as unique_count FROM ${tableName}`; + const uniqueQuery = `SELECT count(DISTINCT ${qIdent(column.name)}) as unique_count FROM ${fqTableName}`; ... - const nonDefaultQuery = `SELECT count() as non_default_count FROM ${tableName} WHERE "${column.name}" != ${column.default_expression}`; + const nonDefaultQuery = `SELECT count() as non_default_count FROM ${fqTableName} WHERE ${qIdent(column.name)} != ${column.default_expression}`;Note:
default_expressionis injected as an expression. Consider server-side allow‑listing or validation to reduce abuse risk.
605-619: Avoidany[]for badges; use a typed shape.- const badges: any[] = []; + type BadgeMeta = { label: string; variant: React.ComponentProps<typeof Badge>['variant'] }; + const badges: BadgeMeta[] = [];
3-16: Replace lucide-react imports with Phosphor React iconsapps/database/app/table/[database]/[table]/schema/page.tsx (lines 3–16) — current import:
import { ArrowLeft, BarChart2, Database, Download, Edit, Plus, RefreshCw, Save, Table as TableIcon, Trash2, Upload, X, } from 'lucide-react';Action: replace with default Phosphor React imports, name components with Icon suffix (e.g., ArrowLeftIcon), and apply weight defaults: duotone by default; use fill for arrow icons; omit weight for Plus. Generate a PR-wide codemod to migrate icons across the repo?
563-574: applySchemaChanges must update DEFAULT expressions (set and drop)File: apps/database/app/table/[database]/[table]/schema/page.tsx
Lines: 563-574applySchemaChanges diffs default_expression but doesn't emit ALTERs to set or remove DEFAULT. ClickHouse requires "MODIFY COLUMN ... DEFAULT " to set and "MODIFY COLUMN ... REMOVE DEFAULT" to drop — add handling and use fqTableName / qIdent / qString for proper quoting.
for (const mod of schemaDiff.modified) { if (mod.old.type !== mod.new.type) { queries.push( - `ALTER TABLE ${tableName} MODIFY COLUMN ${mod.new.name} ${mod.new.type}` + `ALTER TABLE ${fqTableName} MODIFY COLUMN ${qIdent(mod.new.name)} ${mod.new.type}` ); } + if (mod.old.default_expression !== mod.new.default_expression) { + if (mod.new.default_expression && mod.new.default_expression.trim()) { + queries.push( + `ALTER TABLE ${fqTableName} MODIFY COLUMN ${qIdent(mod.new.name)} ${mod.new.type} DEFAULT ${mod.new.default_expression}` + ); + } else { + // Drop the DEFAULT + queries.push( + `ALTER TABLE ${fqTableName} MODIFY COLUMN ${qIdent(mod.new.name)} ${mod.new.type} REMOVE DEFAULT` + ); + } + } if (mod.old.comment !== mod.new.comment) { queries.push( - `ALTER TABLE ${tableName} COMMENT COLUMN ${mod.new.name} '${mod.new.comment}'` + `ALTER TABLE ${fqTableName} COMMENT COLUMN ${qIdent(mod.new.name)} ${qString(mod.new.comment)}` ); } }apps/dashboard/app/(main)/websites/[id]/experiments/[experimentId]/results/page.tsx (2)
70-81: Refresh handler is a no-op; wire up refetch or remove the spinner.Currently it toggles
isRefreshingwithout fetching. Either call a real refetch or drop the refreshing UI to avoid misleading UX.Example using the shared helper:
+import { handleDataRefresh } from '../../../_components/utils/analytics-helpers'; ... -const handleRefresh = useCallback(() => { - setIsRefreshing(true); - try { - // Refresh experiment results data - // await refetchExperimentResults(); - } catch (_error) { - // console.error('Failed to refresh experiment results:', error); - } finally { - setIsRefreshing(false); - } -}, [setIsRefreshing]); +const handleRefresh = useCallback(() => { + setIsRefreshing(true); + // TODO: implement refetchExperimentResults() + void handleDataRefresh(true, async () => {/* await refetchExperimentResults() */}, setIsRefreshing); +}, [setIsRefreshing]);I can help thread a real
refetchExperimentResults()into this page or remove the spinner until wired.
127-141: Remove passedexperimentprop — components are zero-argMetricsTable, VariantComparison, and StatisticalDetails are exported as zero-argument functions; remove the
experiment={experiment}props in apps/dashboard/app/(main)/websites/[id]/experiments/[experimentId]/results/page.tsx (lines ~135–139).-<MetricsTable experiment={experiment} /> -<VariantComparison experiment={experiment} /> -<StatisticalDetails experiment={experiment} /> +<MetricsTable /> +<VariantComparison /> +<StatisticalDetails />apps/dashboard/app/(main)/websites/[id]/_components/utils/analytics-helpers.tsx (1)
155-171: Fix trailing-slash regex; current pattern strips the first slash anywhere.
const SLASH_REGEX = /\//;removes the first slash, not just a trailing one.-const SLASH_REGEX = /\//; +const SLASH_REGEX = /\/+$/; // strip trailing slashes only ... - const cleanDomain = domain - .replace(PROTOCOL_REGEX, '') - .replace(SLASH_REGEX, ''); + const cleanDomain = domain + .replace(PROTOCOL_REGEX, '') + .replace(SLASH_REGEX, '');apps/dashboard/app/(main)/settings/_components/timezone-preferences.tsx (2)
26-47: Selected date format is ignored; preview won’t reflect user choice.formatDate drops dateFormat and hardcodes an en-US pattern. Users who pick DD/MM/YYYY or YYYY-MM-DD will still see Jan 15, 2024 in the preview. Respect both dateFormat and timeFormat, and timezone.
Apply:
-function formatDate( - date: Date, - options: { timezone?: string; dateFormat?: string; timeFormat?: string } -) { - const { timezone = getBrowserTimezone(), timeFormat = 'h:mm a' } = options; - try { - const formatter = new Intl.DateTimeFormat('en-US', { - timeZone: timezone, - year: 'numeric', - month: 'short', - day: 'numeric', - hour: timeFormat?.includes('H') ? '2-digit' : 'numeric', - minute: '2-digit', - hour12: !timeFormat?.includes('H'), - }); - return formatter.format(date); - } catch { - return date.toLocaleString(); - } -} +function formatDate( + date: Date, + options: { timezone?: string; dateFormat?: string; timeFormat?: string } +) { + const timezone = options.timezone || getBrowserTimezone(); + const fmt = + [options.dateFormat, options.timeFormat].filter(Boolean).join(' ') || + 'MMM D, YYYY h:mm a'; + // dayjs-based impl suggested below + // return dayjs(date).tz(timezone).format(fmt); + return new Date( + new Intl.DateTimeFormat('en-US', { timeZone: timezone }).formatToParts(date) + ? date + : date + ).toLocaleString(); // temporary; replace with dayjs impl below +}Also add Dayjs timezone for correct formatting (outside diff range):
import dayjs from 'dayjs'; import utc from 'dayjs/plugin/utc'; import tz from 'dayjs/plugin/timezone'; dayjs.extend(utc); dayjs.extend(tz);Then replace the temporary return with the dayjs line (commented above).
299-333: Clickable divs are not accessible. Use button semantics.Static elements with onClick need role and tabIndex, but native is better.
- {zones.map((tz) => ( - <div - className={cn( - 'mx-1 my-0.5 cursor-pointer rounded-md px-3 py-2 text-sm transition-all duration-200', - 'hover:bg-accent hover:text-accent-foreground', - localPreferences.timezone === tz.region && 'bg-primary text-primary-foreground' - )} - key={tz.region} - onClick={() => - setLocalPreferences({ ...localPreferences, timezone: tz.region }) - } - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - setLocalPreferences({ ...localPreferences, timezone: tz.region }); - } - }} - > + {zones.map((tz) => ( + <button + type="button" + className={cn( + 'mx-1 my-0.5 rounded px-3 py-2 text-left text-sm transition-all duration-200', + 'hover:bg-accent hover:text-accent-foreground', + localPreferences.timezone === tz.region && 'bg-primary text-primary-foreground' + )} + key={tz.region} + onClick={() => + setLocalPreferences({ ...localPreferences, timezone: tz.region }) + } + aria-pressed={localPreferences.timezone === tz.region} + > ... - </div> + </button>apps/api/src/routes/custom-sql.ts (1)
579-586: Avoid logging raw query parameters (PII risk).parameters: JSON.stringify(parameters) can leak user data into logs.
- parameters: JSON.stringify(parameters), + parameters: JSON.stringify(Object.keys(parameters)),Or selectively redact sensitive keys.
apps/dashboard/lib/discord-webhook.ts (1)
463-475: Hard-coded Discord webhook token in repo — rotate immediately and remove from bundles.Found live webhook URL in two locations: packages/shared/src/utils/discord-webhook.ts (≈line 467) and apps/dashboard/lib/discord-webhook.ts (≈line 465). packages/shared re-exports this module (packages/shared/src/utils/index.ts). Imports observed are server-only (apps/dashboard/app/actions/users.ts — "use server"; apps/docs/app/api/ambassador/submit/route.ts — server route). No client-side imports detected in the repository search.
Immediate actions:
- Rotate/revoke the exposed webhook in Discord now.
- Remove the hard-coded URL from both files and any copies; do NOT commit the new webhook.
- Move webhook initialization to a server-only module that reads process.env.DATABUDDY_DISCORD_WEBHOOK_URL (never NEXT_PUBLIC) and only instantiates when typeof window === 'undefined'.
- Stop exporting or re-exporting the webhook from shared code that can be imported by client bundles — create a server-only API surface and import it only from server code.
- After fixes, re-run a repo-wide search for "discord-webhooks" / DATABUDDY_WEBHOOK_URL to ensure no remaining occurrences and confirm no client files import the module.
| } | ||
|
|
||
| const results = []; | ||
| const results: any[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type results precisely; avoid any[].
Use a discriminated union that matches the per‑event returns.
- const results: any[] = [];
+ const results: BatchResult[] = [];Add this type (near imports):
type EventKind = 'track' | 'error' | 'web_vitals' | 'custom' | 'outgoing_link';
type SuccessResult = { status: 'success'; type: EventKind; eventId?: string };
type ErrorResult = {
status: 'error';
message: string;
eventType: EventKind;
error?: string;
eventId?: string;
};
type BatchResult = SuccessResult | ErrorResult;🤖 Prompt for AI Agents
In apps/basket/src/routes/basket.ts around line 931, replace the loose
declaration "const results: any[] = []" with a precise discriminated-union-typed
array: add the EventKind/SuccessResult/ErrorResult/BatchResult types near the
imports, type results as BatchResult[], and update all places that push into
results so each pushed object conforms to either SuccessResult (status:
'success', type: EventKind, optional eventId) or ErrorResult (status: 'error',
message, eventType: EventKind, optional error and eventId). Ensure all property
names match the union (e.g., eventType vs type) and narrow any dynamic values to
the EventKind literal union before pushing.
|
|
||
| // Mock response based on endpoint | ||
| let mockResponse; | ||
| let mockResponse: any; |
There was a problem hiding this comment.
Replace any with a typed union.
Using any violates our TypeScript guidelines. Define response types and use a discriminated/union type.
Apply this diff:
- let mockResponse: any;
+ let mockResponse: AnalyticsResponse | GenericResponse;Add these types near the top of the file (outside the component):
type AnalyticsResponse = {
status: number;
data: {
pageviews: number;
visitors: number;
bounce_rate: number;
avg_session_duration: number;
top_pages: { path: string; views: number }[];
};
timestamp: string;
};
type GenericResponse = {
status: number;
message: string;
endpoint: string;
method: string;
timestamp: string;
};🤖 Prompt for AI Agents
In apps/dashboard/app/(main)/sandbox/api-testing/page.tsx around line 37, the
variable "mockResponse" is typed as "any"; add the provided AnalyticsResponse
and GenericResponse type declarations near the top of the file (outside the
component) and replace the "let mockResponse: any;" declaration with a
discriminated union using those types (e.g., let mockResponse: AnalyticsResponse
| GenericResponse;), then ensure any code that reads mockResponse uses the union
safely (narrow by checking fields like data or message/timestamp) so explicit
typing is enforced.
| </div> | ||
| <div className="grid grid-cols-2 gap-2"> | ||
| {backupCodes.map((code, index) => ( | ||
| <code className="rounded bg-muted p-1 text-xs" key={index + code}> |
There was a problem hiding this comment.
Array index in key prop violates React best practices
Using array index in the key prop can cause rendering issues when the list changes. Since backup codes should be unique, use the code itself as the key.
Apply this diff to use a more stable key:
- <code className="rounded bg-muted p-1 text-xs" key={index + code}>
+ <code className="rounded bg-muted p-1 text-xs" key={code}>📝 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.
| <code className="rounded bg-muted p-1 text-xs" key={index + code}> | |
| <code className="rounded bg-muted p-1 text-xs" key={code}> |
🤖 Prompt for AI Agents
In apps/dashboard/app/(main)/settings/_components/two-factor-form.tsx around
line 91, the JSX uses key={index + code} which relies on the array index;
replace this with a stable unique key using the backup code alone (e.g.,
key={code}) or another unique identifier to avoid React re-render bugs when the
list changes. Ensure the chosen value is truly unique per item and remove the
index concatenation.
| className="flex cursor-pointer items-center justify-between p-4" | ||
| onClick={onToggle} | ||
| onKeyUp={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| onToggle(); | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
Make the header row focusable and handle Space on keyDown (not keyUp)
Current div isn’t focusable and Space on keyUp won’t prevent page scroll. Add role, tabIndex, aria-expanded, and handle Enter/Space on keyDown.
- <div
- className="flex cursor-pointer items-center justify-between p-4"
- onClick={onToggle}
- onKeyUp={(e) => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- onToggle();
- }
- }}
- >
+ <div
+ className="flex cursor-pointer items-center justify-between p-4"
+ onClick={onToggle}
+ role="button"
+ tabIndex={0}
+ aria-expanded={isExpanded}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onToggle();
+ }
+ }}
+ >📝 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.
| className="flex cursor-pointer items-center justify-between p-4" | |
| onClick={onToggle} | |
| onKeyUp={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| onToggle(); | |
| } | |
| }} | |
| > | |
| <div | |
| className="flex cursor-pointer items-center justify-between p-4" | |
| onClick={onToggle} | |
| role="button" | |
| tabIndex={0} | |
| aria-expanded={isExpanded} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| onToggle(); | |
| } | |
| }} | |
| > |
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/settings/integrations/vercel/_components/project-row.tsx
around lines 463 to 471, the header div is not keyboard-focusable and it handles
Space on keyUp which doesn't prevent page scrolling; update the element to be
accessible by adding role="button", tabIndex={0}, and aria-expanded={isOpen} (or
appropriate state), remove the onKeyUp handler and implement an onKeyDown
handler that checks for Enter (e.key === 'Enter') and Space (e.key === ' ' or
e.key === 'Spacebar' for legacy), calls e.preventDefault(), and then calls
onToggle(), while keeping the onClick handler intact.
| // Mock data generator | ||
| const generateTimeSeriesData = (startDate: Date, days: number) => { | ||
| const data = []; | ||
| const data: any[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid any[]. Type the time‑series points.
Define a point type and use it here for safer Tooltip/AreaChart bindings.
- const data: any[] = [];
+ type ChartPoint = { date: string; control: number; variant: number };
+ const data: ChartPoint[] = [];📝 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 data: any[] = []; | |
| type ChartPoint = { date: string; control: number; variant: number }; | |
| const data: ChartPoint[] = []; |
🤖 Prompt for AI Agents
In
apps/dashboard/app/(main)/websites/[id]/experiments/[experimentId]/results/_components/conversion-chart.tsx
around line 21, replace the untyped const data: any[] with a proper time-series
point type (e.g. declare a type or interface like Point { time: string | number
| Date; value: number; [optional fields as needed] } and then use const data:
Point[]). Update any references in Tooltip, AreaChart, and other bindings to use
the typed property names (time and value or whatever you choose), and adjust
parsing/formatting (e.g. Date -> number/string) so the chart components receive
the correctly typed fields.
| @@ -1,42 +1,46 @@ | |||
| class FlagStorage { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Introduce FlagValue (JSONValue) and genericize helpers; avoid any/unknown.
Also mark ttl readonly.
Add near the top of the file:
export type JsonValue = string | number | boolean | null | { [k: string]: JsonValue } | JsonValue[];
export type FlagValue = JsonValue;Apply targeted diffs:
- private ttl = 24 * 60 * 60 * 1000; // 24 hours in milliseconds
+ private readonly ttl = 24 * 60 * 60 * 1000; // 24 hours in milliseconds-private getFromLocalStorage(key: string): any {
+private getFromLocalStorage<T extends FlagValue = FlagValue>(key: string): T | null {
try {
const item = localStorage.getItem(`db-flag-${key}`);
if (!item) {
return null;
}
const parsed = JSON.parse(item);
if (parsed.expiresAt) {
if (this.isExpired(parsed.expiresAt)) {
localStorage.removeItem(`db-flag-${key}`);
return null;
}
- return parsed.value;
+ return parsed.value as T;
}
- return parsed;
+ return parsed as T;
} catch {
return null;
}
}-private setToLocalStorage(key: string, value: unknown): void {
+private setToLocalStorage<T extends FlagValue>(key: string, value: T): void {
try {
const item = {
value,
timestamp: Date.now(),
expiresAt: Date.now() + this.ttl,
};
localStorage.setItem(`db-flag-${key}`, JSON.stringify(item));
} catch {}
}Also applies to: 46-76
🤖 Prompt for AI Agents
In packages/sdk/src/react/flags/flag-storage.ts at the top and covering lines 1
and 46-76, introduce the JsonValue and FlagValue types (as exported aliases) and
replace any/unknown uses with FlagValue/JsonValue in the storage helpers; update
helper signatures to be generic over FlagValue (or use FlagValue directly)
instead of any/unknown, and mark ttl properties readonly where defined.
Specifically, add the two type exports near the top, change any
function/variable types that currently use any or unknown to use FlagValue or
JsonValue, make ttl readonly in the relevant interfaces/classes, and adjust
helper generics and return types accordingly so the file is fully typed without
any/unknown.
| get(key: string) { | ||
| return this.getFromLocalStorage(key); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strongly type get; avoid implicit any/unknown and surface nullability.
Return a precise type and thread it through to the backing method.
-get(key: string) {
- return this.getFromLocalStorage(key);
-}
+get<T extends FlagValue = FlagValue>(key: string): T | null {
+ return this.getFromLocalStorage<T>(key);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get(key: string) { | |
| return this.getFromLocalStorage(key); | |
| } | |
| get<T extends FlagValue = FlagValue>(key: string): T | null { | |
| return this.getFromLocalStorage<T>(key); | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/react/flags/flag-storage.ts around lines 4 to 6, the get
method is untyped and returns any/unknown; change its signature to get<T =
unknown>(key: string): T | null and forward that generic to the backing method
(e.g., getFromLocalStorage<T>(key): T | null), updating both method signatures
and any related callers so the precise type and nullability are preserved
through the call chain.
| set(key: string, value: unknown) { | ||
| this.setToLocalStorage(key, value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make set generic; drop unknown per guidelines.
Aligns with typed value flow and prevents accidental non‑JSON values.
-set(key: string, value: unknown) {
- this.setToLocalStorage(key, value);
-}
+set<T extends FlagValue>(key: string, value: T): void {
+ this.setToLocalStorage(key, value);
+}📝 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.
| set(key: string, value: unknown) { | |
| this.setToLocalStorage(key, value); | |
| } | |
| set<T extends FlagValue>(key: string, value: T): void { | |
| this.setToLocalStorage(key, value); | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/react/flags/flag-storage.ts around lines 8 to 10, the set
method currently accepts value: unknown; change it to a generic signature (e.g.
set<T>(key: string, value: T)) and update any internal calls (like
setToLocalStorage) and their signatures to accept the same generic type so types
flow through; ensure callers get typed values and remove the use of unknown
while preserving the JSON-serializable handling already used by localStorage
persistence.
| getAll(): Record<string, unknown> { | ||
| const result: Record<string, unknown> = {}; | ||
| const now = Date.now(); | ||
| Object.keys(localStorage) | ||
| .filter((key) => key.startsWith('db-flag-')) | ||
| .forEach((key) => { | ||
| const flagKey = key.replace('db-flag-', ''); | ||
| try { | ||
| const item = localStorage.getItem(key); | ||
| if (item) { | ||
| const parsed = JSON.parse(item); | ||
| if (parsed.expiresAt && now > parsed.expiresAt) { | ||
| localStorage.removeItem(key); | ||
| } else { | ||
| result[flagKey] = parsed.value || parsed; // Support both new and old format | ||
| } | ||
|
|
||
| const keys = Object.keys(localStorage).filter((key) => | ||
| key.startsWith('db-flag-') | ||
| ); | ||
|
|
||
| for (const key of keys) { | ||
| const flagKey = key.replace('db-flag-', ''); | ||
| try { | ||
| const item = localStorage.getItem(key); | ||
| if (item) { | ||
| const parsed = JSON.parse(item); | ||
| if (parsed.expiresAt && now > parsed.expiresAt) { | ||
| localStorage.removeItem(key); | ||
| } else { | ||
| result[flagKey] = parsed.value || parsed; // Support both new and old format | ||
| } | ||
| } catch {} | ||
| }); | ||
| } | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Fix falsy-value bug and enumerate Storage correctly.
- Bug:
parsed.value || parsedcollapses valid falsy values (false, 0, '') and returns the whole object. - Robustness:
Object.keys(localStorage)is non‑spec and inconsistent; iterate viaStorage.key().
-getAll(): Record<string, unknown> {
- const result: Record<string, unknown> = {};
- const now = Date.now();
-
- const keys = Object.keys(localStorage).filter((key) =>
- key.startsWith('db-flag-')
- );
-
- for (const key of keys) {
- const flagKey = key.replace('db-flag-', '');
- try {
- const item = localStorage.getItem(key);
- if (item) {
- const parsed = JSON.parse(item);
- if (parsed.expiresAt && now > parsed.expiresAt) {
- localStorage.removeItem(key);
- } else {
- result[flagKey] = parsed.value || parsed; // Support both new and old format
- }
- }
- } catch {}
- }
+getAll<T extends FlagValue = FlagValue>(): Record<string, T> {
+ const result: Record<string, T> = {};
+ const now = Date.now();
+ const prefix = 'db-flag-';
+
+ for (let i = localStorage.length - 1; i >= 0; i--) {
+ const key = localStorage.key(i);
+ if (!key || !key.startsWith(prefix)) continue;
+ const flagKey = key.slice(prefix.length);
+ try {
+ const item = localStorage.getItem(key);
+ if (!item) continue;
+ const parsed = JSON.parse(item);
+ const expiresAt = parsed?.expiresAt as number | undefined;
+ const value = (Object.prototype.hasOwnProperty.call(parsed, 'value') ? parsed.value : parsed) as T;
+ if (expiresAt && now > expiresAt) {
+ localStorage.removeItem(key);
+ } else {
+ result[flagKey] = value;
+ }
+ } catch {
+ // Drop corrupt entries
+ localStorage.removeItem(key!);
+ }
+ }
return result;
}📝 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.
| getAll(): Record<string, unknown> { | |
| const result: Record<string, unknown> = {}; | |
| const now = Date.now(); | |
| Object.keys(localStorage) | |
| .filter((key) => key.startsWith('db-flag-')) | |
| .forEach((key) => { | |
| const flagKey = key.replace('db-flag-', ''); | |
| try { | |
| const item = localStorage.getItem(key); | |
| if (item) { | |
| const parsed = JSON.parse(item); | |
| if (parsed.expiresAt && now > parsed.expiresAt) { | |
| localStorage.removeItem(key); | |
| } else { | |
| result[flagKey] = parsed.value || parsed; // Support both new and old format | |
| } | |
| const keys = Object.keys(localStorage).filter((key) => | |
| key.startsWith('db-flag-') | |
| ); | |
| for (const key of keys) { | |
| const flagKey = key.replace('db-flag-', ''); | |
| try { | |
| const item = localStorage.getItem(key); | |
| if (item) { | |
| const parsed = JSON.parse(item); | |
| if (parsed.expiresAt && now > parsed.expiresAt) { | |
| localStorage.removeItem(key); | |
| } else { | |
| result[flagKey] = parsed.value || parsed; // Support both new and old format | |
| } | |
| } catch {} | |
| }); | |
| } | |
| } catch {} | |
| } | |
| getAll<T extends FlagValue = FlagValue>(): Record<string, T> { | |
| const result: Record<string, T> = {}; | |
| const now = Date.now(); | |
| const prefix = 'db-flag-'; | |
| for (let i = localStorage.length - 1; i >= 0; i--) { | |
| const key = localStorage.key(i); | |
| if (!key || !key.startsWith(prefix)) continue; | |
| const flagKey = key.slice(prefix.length); | |
| try { | |
| const item = localStorage.getItem(key); | |
| if (!item) continue; | |
| const parsed = JSON.parse(item); | |
| const expiresAt = parsed?.expiresAt as number | undefined; | |
| const value = (Object.prototype.hasOwnProperty.call(parsed, 'value') ? parsed.value : parsed) as T; | |
| if (expiresAt && now > expiresAt) { | |
| localStorage.removeItem(key); | |
| } else { | |
| result[flagKey] = value; | |
| } | |
| } catch { | |
| // Drop corrupt entries | |
| localStorage.removeItem(key!); | |
| } | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/react/flags/flag-storage.ts around lines 12 to 33, fix two
issues: iterate Storage via its indexed API instead of Object.keys(localStorage)
and preserve valid falsy flag values when reading parsed entries. Replace the
keys collection with a for loop using for (let i = 0; i < localStorage.length;
i++) { const key = localStorage.key(i); } (skip null keys and filter
startsWith('db-flag-')), and change the assignment result[flagKey] =
parsed.value || parsed to use an explicit existence check such as
result[flagKey] = Object.prototype.hasOwnProperty.call(parsed, 'value') ?
parsed.value : parsed so false, 0, and '' are not lost; keep the existing
try/catch around JSON.parse.
| clear(): void { | ||
| const keys = Object.keys(localStorage).filter((key) => | ||
| key.startsWith('db-flag-') | ||
| ); | ||
| for (const key of keys) { | ||
| localStorage.removeItem(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use Storage.key() iteration; Object.keys(localStorage) is unreliable.
Prevents missed/decorated props in some engines.
-clear(): void {
- const keys = Object.keys(localStorage).filter((key) =>
- key.startsWith('db-flag-')
- );
- for (const key of keys) {
- localStorage.removeItem(key);
- }
-}
+clear(): void {
+ const prefix = 'db-flag-';
+ for (let i = localStorage.length - 1; i >= 0; i--) {
+ const key = localStorage.key(i);
+ if (key && key.startsWith(prefix)) {
+ localStorage.removeItem(key);
+ }
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clear(): void { | |
| const keys = Object.keys(localStorage).filter((key) => | |
| key.startsWith('db-flag-') | |
| ); | |
| for (const key of keys) { | |
| localStorage.removeItem(key); | |
| } | |
| } | |
| clear(): void { | |
| const prefix = 'db-flag-'; | |
| for (let i = localStorage.length - 1; i >= 0; i--) { | |
| const key = localStorage.key(i); | |
| if (key && key.startsWith(prefix)) { | |
| localStorage.removeItem(key); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/react/flags/flag-storage.ts around lines 37 to 44, replace
the unreliable Object.keys(localStorage) usage with Storage.key() iteration:
loop over localStorage.length, read each key via localStorage.key(index), filter
keys that start with 'db-flag-' and remove them. To avoid index-shift issues
when removing entries, either collect matching keys first and then remove them,
or iterate the storage indices in reverse while checking localStorage.key(i)
before calling removeItem.
4 participants
This pull request focuses on minor refactoring and code consistency improvements across several frontend components. The main changes include reordering imports, adjusting the placement of props for better readability, and removing unnecessary async/await usage. There are also several updates to ensure consistent usage of data attributes and props order in component definitions.
The most important changes are:
Frontend Component Consistency and Refactoring:
Standardized the order of props and data attributes in multiple UI components such as
Avatar,Badge,Button,Carousel, andDropdownMenuto improve readability and maintainability. This includes consistently placingdata-slotand other attributes after class names and before spreading props. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19]Moved import statements for React and other dependencies to the top of files for consistency in
badge.tsx,button.tsx,dropdown-menu.tsx, andcarousel.tsx. [1] [2] [3] [4]Fixed the order of logic and props in event handlers and effect hooks, such as adding early returns for null checks and moving logic blocks for clarity in
carousel.tsxand other components. [1] [2]Async/Await and Function Refactoring:
async/awaitin several functions, converting them to synchronous functions where possible for improved performance and simplicity. This applies to functions inindex.ts,two-factor-form.tsx,results/page.tsx, andchat-db.ts. [1] [2] apps/dashboard/app/(main)/websites/[id]/experiments/[experimentId]/results/page.tsxL70-R70, apps/dashboard/app/(main)/websites/[id]/assistant/lib/chat-db.tsL42-R42)Code Cleanup:
tool.tsxfor better organization and to remove unused imports. [1] [2]These changes collectively improve code readability, maintainability, and consistency across the codebase.
Summary by CodeRabbit