ronantakizawa/codereview-bench huggingface dataset:
Repository: supabase/supabase
File: apps/studio/components/interfaces/Database/Triggers/TriggerSheet.tsx (TypeScript)

Code before:
              onClick={() => onClose()}
            >
              Cancel
            </Button>
            <Button form={formId} htmlType="submit" loading={isCreating || isUpdating}>
              {isEditing ? 'Save' : 'Create'} trigger
            </Button>
          </SheetFooter>

          <ConfirmationModal
            visible={isClosingPanel}
            title="Discard changes"
            confirmLabel="Discard"
            onCancel={() => setIsClosingPanel(false)}
            onConfirm={() => {
              setIsClosingPanel(false)
              onClose()
            }}
          >
            <p className="text-sm text-foreground-light">
              There are unsaved changes. Are you sure you want to close the panel? Your changes will
              be lost.
            </p>
          </ConfirmationModal>
        </SheetContent>
      </Sheet>

      <ChooseFunctionForm
        visible={showFunctionSelector}
        setVisible={setShowFunctionSelector}
        onChange={(fn) => {
          form.setValue('function_name', fn.name)
          form.setValue('function_schema', fn.schema)
        }}
      />
    </>
  )
}


Review comment (suggestion):
Surprised that this was missing from this, I wonder how many users feel to unsaved changes by clicking away 🙈 

Code after:
              htmlType="reset"
              disabled={isCreating || isUpdating}
              onClick={onClose}
            >
              Cancel
            </Button>
            <Button form={formId} htmlType="submit" loading={isCreating || isUpdating}>
              {isEditing ? 'Save' : 'Create'} trigger
            </Button>
          </SheetFooter>

          <ConfirmationModal
            visible={isClosingPanel}
            title="Discard changes"
            confirmLabel="Discard"
            onCancel={() => setIsClosingPanel(false)}
            onConfirm={() => {
              setIsClosingPanel(false)
              onClose()
            }}
          >
            <p className="text-sm text-foreground-light">
              There are unsaved changes. Are you sure you want to close the panel? Your changes will
              be lost.
            </p>
          </ConfirmationModal>
        </SheetContent>
      </Sheet>

      <ChooseFunctionForm
        visible={showFunctionSelector}
        setVisible={setShowFunctionSelector}
        onChange={(fn) => {
          form.setValue('function_name', fn.name)
          form.setValue('function_schema', fn.schema)
        }}
      />
    </>
  )
}
Repository: supabase/supabase
File: apps/studio/pages/project/[ref]/auth/templates/[templateId].tsx (TypeScript)

Code before:
        {
          label: 'Emails',
          href: `/project/${ref}/auth/templates`,
        },
      ]}
      secondaryActions={[
        <DocsButton
          key="docs"
          href={`${DOCS_URL}/guides/local-development/customizing-email-templates#authemailtemplate${templateIdForDocs}`}
        />,
      ]}
    >
      <ScaffoldContainer bottomPadding>
        {!isPermissionsLoaded ? (
          <ScaffoldSection isFullWidth>
            <GenericSkeletonLoader />
          </ScaffoldSection>
        ) : (
          <ScaffoldSection isFullWidth>
            <Card>
              <TemplateEditor template={template} />
            </Card>
          </ScaffoldSection>
        )}
      </ScaffoldContainer>
    </PageLayout>
  )
}

TemplatePage.getLayout = (page) => (
  <DefaultLayout>
    <AuthLayout>{page}</AuthLayout>
  </DefaultLayout>
)

export default TemplatePage


Review comment (suggestion):
This will be fleshed out to include a`ScaffoldHeader`, and there will be other conditional `ScaffoldSection` instances in the future.

Code after:
      title={template.title}
      subtitle={template.purpose || 'Configure and customize email templates.'}
      breadcrumbs={[
        {
          label: 'Emails',
          href: `/project/${ref}/auth/templates`,
        },
      ]}
      secondaryActions={[
        <DocsButton
          key="docs"
          href={`${DOCS_URL}/guides/local-development/customizing-email-templates#authemailtemplate${templateIdForDocs}`}
        />,
      ]}
    >
      <ScaffoldContainer bottomPadding>
        {!isPermissionsLoaded ? (
          <ScaffoldSection isFullWidth>
            <GenericSkeletonLoader />
          </ScaffoldSection>
        ) : (
          <ScaffoldSection isFullWidth>
            <Card>
              <TemplateEditor template={template} />
            </Card>
          </ScaffoldSection>
        )}
      </ScaffoldContainer>
    </PageLayout>
  )
}

TemplatePage.getLayout = (page) => (
  <DefaultLayout>
    <AuthLayout>{page}</AuthLayout>
  </DefaultLayout>
)

export default TemplatePage
Repository: supabase/supabase
File: packages/ui-patterns/src/CommandMenu/api/CommandMenu.tsx (TypeScript)

Code before:
'use client'
import { VisuallyHidden } from '@radix-ui/react-visually-hidden'
import { AlertTriangle, ArrowLeft } from 'lucide-react'
import type { HTMLAttributes, MouseEvent, PropsWithChildren, ReactElement, ReactNode } from 'react'
import { Children, cloneElement, forwardRef, isValidElement, useEffect, useMemo } from 'react'
import { ErrorBoundary } from 'react-error-boundary'

import { useBreakpoint } from 'common'
import useDragToClose from 'common/hooks/useDragToClose'
import {
  Button,
  Command_Shadcn_,
  Dialog,
  DialogContent,
  DialogDescription,
  DialogTitle,
  cn,
} from 'ui'

import { useCurrentPage, usePageComponent, usePopPage } from './hooks/pagesHooks'
import { useQuery, useSetQuery } from './hooks/queryHooks'
import {
  useCommandMenuOpen,
  useCommandMenuSize,
  useSetCommandMenuOpen,
  useSetupCommandMenuTouchEvents,
} from './hooks/viewHooks'

Review comment (suggestion):
Radix insists on a DialogTitle and DialogDescription for accessibility. This VisuallyHidden makes that error go away. It allows folks with screen-readers to get those contents without the visual result changing.

Code after:
'use client'
import { VisuallyHidden } from '@radix-ui/react-visually-hidden'
import { AlertTriangle, ArrowLeft, Command, Search } from 'lucide-react'
import type { HTMLAttributes, MouseEvent, PropsWithChildren, ReactElement, ReactNode } from 'react'
import { Children, cloneElement, forwardRef, isValidElement, useEffect, useMemo } from 'react'
import { ErrorBoundary } from 'react-error-boundary'

import { useBreakpoint } from 'common'
import useDragToClose from 'common/hooks/useDragToClose'
import {
  Button,
  Command_Shadcn_,
  Dialog,
  DialogContent,
  DialogDescription,
  DialogTitle,
  cn,
} from 'ui'

import { useCurrentPage, usePageComponent, usePopPage } from './hooks/pagesHooks'
import { useQuery, useSetQuery } from './hooks/queryHooks'
import { useCommandMenuTelemetryContext } from './hooks/useCommandMenuTelemetryContext'
import {
  useCommandMenuOpen,
  useCommandMenuSize,
  useSetCommandMenuOpen,
  useSetupCommandMenuTouchEvents,
Repository: supabase/supabase
File: packages/common/telemetry-constants.ts (TypeScript)

Code before:
 * User clicked the assistant button in the assistant variation.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/editor (NewTab)
 */
export interface TableQuickstartAssistantOpenedEvent {
  action: 'table_quickstart_assistant_opened'
  properties: {
    /**
     * Whether the assistant chat was successfully created
     */
    chatCreated: boolean
  }
  groups: TelemetryGroups
}

/**
 * User toggled the inline editor setting in account preferences.
 *
 * @group Events
 * @source studio
 * @page /dashboard/account/preferences
 */
export interface InlineEditorSettingToggledEvent {
  action: 'inline_editor_setting_toggled'
  properties: {
    /**
     * Whether the inline editor was enabled or disabled
     */
    enabled: boolean
  }
  groups: Partial<TelemetryGroups>
}

/**
 * @hidden
 */
export type TelemetryEvent =
  | SignUpEvent
  | SignInEvent
  | ConnectionStringCopiedEvent
  | ApiDocsOpenedEvent
  | ApiDocsCodeCopyButtonClickedEvent
  | CronJobCreatedEvent
  | CronJobUpdatedEvent
  | CronJobDeletedEvent
  | CronJobCreateClickedEvent
  | CronJobUpdateClickedEvent
  | CronJobDeleteClickedEvent
  | CronJobHistoryClickedEvent

Review comment (suggestion):
Could we use clicked as the verb of choice instead of toggled? We aim to re-use verbs if they're similar in functionality. 

Code after:
 * User clicked the assistant button in the assistant variation.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/editor (NewTab)
 */
export interface TableQuickstartAssistantOpenedEvent {
  action: 'table_quickstart_assistant_opened'
  properties: {
    /**
     * Whether the assistant chat was successfully created
     */
    chatCreated: boolean
  }
  groups: TelemetryGroups
}

/**
 * User toggled the inline editor setting in account preferences.
 *
 * @group Events
 * @source studio
 * @page /dashboard/account/preferences
 */
export interface InlineEditorSettingClickedEvent {
  action: 'inline_editor_setting_clicked'
  properties: {
    /**
     * Whether the inline editor was enabled or disabled
     */
    enabled: boolean
  }
  groups: Partial<TelemetryGroups>
}

/**
 * User clicked the save destination button in add log drains sheet.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/settings/log-drains (LogDrainDestinationSheetForm)
 */
export interface LogDrainSaveButtonClickedEvent {
  action: 'log_drain_save_button_clicked'
  properties: {
    /**
     * Type of the destination saved
     */
    destination: 'webhook' | 'datadog' | 'loki' | 'sentry'
  }
  groups: TelemetryGroups
Repository: supabase/supabase
File: apps/studio/lib/password-strength.ts (TypeScript)

Code before:
import { DEFAULT_MINIMUM_PASSWORD_STRENGTH, PASSWORD_STRENGTH } from 'lib/constants'

export async function passwordStrength(value: string) {
  // [Alaister]: Lazy load zxcvbn to avoid bundling it with the main app (it's pretty chunky)
  const zxcvbn = await import('zxcvbn').then((module) => module.default)

  let message: string = ''
  let warning: string = ''
  let strength: number = 0

  if (value && value !== '') {
    if (value.length > 99) {
      message = `${PASSWORD_STRENGTH[0]} Maximum length of password exceeded`
      warning = `Password should be less than 100 characters`
    } else {
      const result = zxcvbn(value)
      const resultScore = result?.score ?? 0

      const score = (PASSWORD_STRENGTH as any)[resultScore]
      const suggestions = result.feedback?.suggestions ? result.feedback.suggestions.join(' ') : ''

      message = `${score} ${suggestions}`
      strength = resultScore

      // warning message for anything below 4 strength :string
      if (resultScore < DEFAULT_MINIMUM_PASSWORD_STRENGTH) {
        warning = `${
          result?.feedback?.warning ? result?.feedback?.warning + '.' : ''
        } You need a stronger password.`
      }
    }
  }

  return {
    message,
    warning,
    strength,
  }
}


Review comment (suggestion):
```suggestion
      const suggestions = result.feedback?.suggestions?.join(' ') ?? ''
```

tiny nit

Code after:
import { DEFAULT_MINIMUM_PASSWORD_STRENGTH, PASSWORD_STRENGTH } from 'lib/constants'

export async function passwordStrength(value: string) {
  // [Alaister]: Lazy load zxcvbn to avoid bundling it with the main app (it's pretty chunky)
  const zxcvbn = await import('zxcvbn').then((module) => module.default)

  let message: string = ''
  let warning: string = ''
  let strength: number = 0

  if (value && value !== '') {
    if (value.length > 99) {
      message = `${PASSWORD_STRENGTH[0]} Maximum length of password exceeded`
      warning = `Password should be less than 100 characters`
    } else {
      const result = zxcvbn(value)
      const resultScore = result?.score ?? 0

      const score = (PASSWORD_STRENGTH as any)[resultScore]
      const suggestions = result.feedback?.suggestions?.join(' ') ?? ''

      message = `${score} ${suggestions}`
      strength = resultScore

      // warning message for anything below 4 strength :string
      if (resultScore < DEFAULT_MINIMUM_PASSWORD_STRENGTH) {
        warning = `${
          result?.feedback?.warning ? result?.feedback?.warning + '.' : ''
        } You need a stronger password.`
      }
    }
  }

  return {
    message,
    warning,
    strength,
  }
}
Repository: supabase/supabase
File: apps/studio/components/layouts/LogsLayout/LogsSidebarMenuV2.tsx (TypeScript)

Code before:
          />
        )}
        {savedQueries.map((query) => (
          <SavedQueriesItem item={query as any} key={query.id} /> // kemal: i know, i know, temp any fix.
        ))}
      </SidebarCollapsible>

      <FeaturePreviewSidebarPanel
        className="mx-4 mt-4"
        title="Capture your logs"
        description="Send logs to your preferred observability or storage platform."
        illustration={
          <div className="flex items-center gap-4">
            {LOG_DRAIN_TYPES.map((type) =>
              React.cloneElement(type.icon, { height: 20, width: 20 })
            )}
          </div>
        }
        actions={
          <Button asChild type="default">
            <Link href={`/project/${ref}/settings/log-drains`}>Go to Log Drains</Link>
          </Button>
        }
      />
    </div>
  )
}


Review comment (suggestion):
I believe this component is for the old logs, right? If so, this feedback can likely be ignored since I understand we’re moving away from it:

- This panel looks a bit funny next to the centered text in the aside above it
- Maybe a `Separator` above this would help make it not look like a child of _Queries_

Code after:
          />
        )}
        {savedQueries.map((query) => (
          <SavedQueriesItem item={query as any} key={query.id} /> // kemal: i know, i know, temp any fix.
        ))}
      </SidebarCollapsible>

      <Separator className="my-4" />

      <FeaturePreviewSidebarPanel
        className="mx-4 mt-4"
        title="Capture your logs"
        description="Send logs to your preferred observability or storage platform."
        illustration={
          <div className="flex items-center gap-4">
            {LOG_DRAIN_TYPES.map((type) =>
              React.cloneElement(type.icon, { height: 20, width: 20 })
            )}
          </div>
        }
        actions={
          <Button asChild type="default">
            <Link href={`/project/${ref}/settings/log-drains`}>Go to Log Drains</Link>
          </Button>
        }
      />
    </div>
  )
}
Repository: supabase/supabase
File: packages/common/telemetry-constants.ts (TypeScript)

Code before:
 * User clicked the assistant button in the assistant variation.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/editor (NewTab)
 */
export interface TableQuickstartAssistantOpenedEvent {
  action: 'table_quickstart_assistant_opened'
  properties: {
    /**
     * Whether the assistant chat was successfully created
     */
    chatCreated: boolean
  }
  groups: TelemetryGroups
}

/**
 * User clicked the save destination button in add log drains sheet.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/settings/log-drains (LogDrainDestinationSheetForm)
 */
export interface LogDrainDestinationSavedEvent {
  action: 'log_drain_destination_saved'
  properties: {
    /**
     * Type of the destination saved
     */
    destination: 'webhook' | 'datadog' | 'loki' | 'sentry'
  }
  groups: TelemetryGroups
}

/**
 * User confirmed addition of log drain destination.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/settings/log-drains (LogDrains)
 */
export interface LogDrainDestinationConfirmedEvent {
  action: 'log_drain_destination_confirmed'
  properties: {
    /**
     * Type of the destination confirmed
     */
    destination: 'webhook' | 'datadog' | 'loki' | 'sentry'
  }
  groups: TelemetryGroups

Review comment (suggestion):
as these are all buttons, can you end it with save_button_clicked instead? To make it consistent with the other actions. 

Code after:
 * User clicked the assistant button in the assistant variation.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/editor (NewTab)
 */
export interface TableQuickstartAssistantOpenedEvent {
  action: 'table_quickstart_assistant_opened'
  properties: {
    /**
     * Whether the assistant chat was successfully created
     */
    chatCreated: boolean
  }
  groups: TelemetryGroups
}

/**
 * User clicked the save destination button in add log drains sheet.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/settings/log-drains (LogDrainDestinationSheetForm)
 */
export interface LogDrainSaveButtonClickedEvent {
  action: 'log_drain_save_button_clicked'
  properties: {
    /**
     * Type of the destination saved
     */
    destination: 'webhook' | 'datadog' | 'loki' | 'sentry'
  }
  groups: TelemetryGroups
}

/**
 * User confirmed addition of log drain destination.
 *
 * @group Events
 * @source studio
 * @page /dashboard/project/{ref}/settings/log-drains (LogDrains)
 */
export interface LogDrainConfirmButtonSubmittedEvent {
  action: 'log_drain_confirm_button_submitted'
  properties: {
    /**
     * Type of the destination confirmed
     */
    destination: 'webhook' | 'datadog' | 'loki' | 'sentry'
  }
  groups: TelemetryGroups
Repository: supabase/supabase
File: apps/studio/lib/error-reporting.ts (TypeScript)

Code before:
    return
  }
}

function handleError(error: Error, context: string) {
  const errorMessage = error.message
  if (!errorMessage) {
    return
  }

  captureMessage({
    message: `[CRITICAL][${context}] Failed: ${errorMessage}`,
    context,
  })
}

function handleUnknownError(error: unknown, context: string) {
  const hasMessage =
    error && typeof error === 'object' && 'message' in error && typeof error.message === 'string'

  if (hasMessage) {
    captureMessage({
      message: `[CRITICAL][${context}] Failed: ${error.message}`,
      context,
    })
  }
}

function captureMessage({ message, context }: CaptureMessageOptions) {
  if (WHITELIST_ERRORS.some((whitelisted) => message.includes(whitelisted))) {
    return
  }
  Sentry.captureMessage(`[CRITICAL][${context}] ${message}`)
}


Review comment (suggestion):
Normally I hate this, but Typescript doesn't safely determine error.message is a string if its assigned :(

Code after:
    return
  }
}

function handleError(error: Error, context: string) {
  const message = error.message
  if (!message) {
    return
  }

  captureMessage({
    message,
    context,
  })
}

function handleUnknownError(error: unknown, context: string) {
  if (
    error &&
    typeof error === 'object' &&
    'message' in error &&
    typeof error.message === 'string'
  ) {
    captureMessage({
      message: error.message,
      context,
    })
  }
}

function captureMessage({ message, context }: CaptureMessageOptions) {
  if (WHITELIST_ERRORS.some((whitelisted) => message.includes(whitelisted))) {
    return
  }
  Sentry.captureMessage(`[CRITICAL][${context}] ${message} Failed: ${message}`)
}
Repository: supabase/supabase
File: apps/studio/components/interfaces/Database/ETL/ComingSoon.tsx (TypeScript)

Code before:
      <ReplicationStaticMockup projectRef={projectRef} />
    </ReactFlowProvider>
  )
}

const ReplicationStaticMockup = ({ projectRef }: { projectRef: string }) => {
  const nodes = useMemo(() => STATIC_NODES, [])
  const edges = useMemo(() => STATIC_EDGES, [])

  const { resolvedTheme } = useTheme()

  const backgroundPatternColor =
    resolvedTheme === 'dark' ? 'rgba(255, 255, 255, 0.3)' : 'rgba(0, 0, 0, 0.4)'

  const nodeTypes = useMemo(
    () => ({
      primary: PrimaryNode,
      replica: ReplicaNode,
      blank: BlankNode,
      cta: () => CTANode({ projectRef }),
    }),
    [projectRef]
  )

  return (
    <div className="relative border-t h-full w-full relative">
      <ReactFlow
        fitView
        fitViewOptions={{ minZoom: 0.9, maxZoom: 0.9 }}
        className="instance-configuration"
        zoomOnPinch={false}
        zoomOnScroll={false}
        nodesDraggable={true}
        nodesConnectable={false}
        zoomOnDoubleClick={false}
        edgesFocusable={false}
        edgesUpdatable={false}
        nodes={nodes}
        edges={edges}
        nodeTypes={nodeTypes}
        proOptions={{ hideAttribution: true }}
      >
        <Background color={backgroundPatternColor} />
      </ReactFlow>
    </div>
  )
}

const PrimaryNode = ({
  data,
}: {

Review comment (suggestion):
```suggestion
    <div className="relative border-t h-full w-full">
```

Code after:
      <ReplicationStaticMockup projectRef={projectRef} />
    </ReactFlowProvider>
  )
}

const ReplicationStaticMockup = ({ projectRef }: { projectRef: string }) => {
  const nodes = useMemo(() => STATIC_NODES, [])
  const edges = useMemo(() => STATIC_EDGES, [])

  const { resolvedTheme } = useTheme()

  const backgroundPatternColor =
    resolvedTheme === 'dark' ? 'rgba(255, 255, 255, 0.3)' : 'rgba(0, 0, 0, 0.4)'

  const nodeTypes = useMemo(
    () => ({
      primary: PrimaryNode,
      replica: ReplicaNode,
      blank: BlankNode,
      cta: () => CTANode({ projectRef }),
    }),
    [projectRef]
  )

  return (
    <div className="relative border-t h-full w-full">
      <ReactFlow
        fitView
        fitViewOptions={{ minZoom: 0.9, maxZoom: 0.9 }}
        className="instance-configuration"
        zoomOnPinch={false}
        zoomOnScroll={false}
        nodesDraggable={true}
        nodesConnectable={false}
        zoomOnDoubleClick={false}
        edgesFocusable={false}
        edgesUpdatable={false}
        nodes={nodes}
        edges={edges}
        nodeTypes={nodeTypes}
        proOptions={{ hideAttribution: true }}
      >
        <Background color={backgroundPatternColor} />
      </ReactFlow>
    </div>
  )
}

const PrimaryNode = ({
  data,
}: {