ronantakizawa/codereview-bench huggingface dataset:
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/core-modules/file/services/file-metadata.service.ts (TypeScript)

Code before:
    filename,
    mimeType,
    workspaceId,
  }: {
    file: Buffer;
    filename: string;
    mimeType: string;
    workspaceId: string;
  }): Promise<FileDTO> {
    const { files } = await this.fileUploadService.uploadFile({
      file,
      filename,
      mimeType,
      fileFolder: FileFolder.File,
      workspaceId,
    });

    if (!files.length) {
      throw new Error('Failed to upload file');
    }

    const createdFile = this.fileRepository.create({
      path: files[0].path,
      size: file.length,
      workspaceId,
    });

    const savedFile = await this.fileRepository.save(createdFile);

    return savedFile;
  }

  async deleteFileById(
    id: string,
    workspaceId: string,
  ): Promise<FileDTO | null> {
    const file = await this.fileRepository.findOne({
      where: { id, workspaceId },
    });

    if (!file) {
      return null;
    }

    const { folderPath, filename } = extractFolderPathAndFilename(file.path);

    try {
      if (file.path) {
        await this.fileService.deleteFile({
          folderPath,
          filename,

Review comment (suggestion):
Same, not a great fan of removing type, this let MIME type inference to the caller (through the extension) or some header parsing in the file before serving it. Again we apparently were ok for that but just wanted to share my thoughts

Code after:
    filename,
    mimeType,
    workspaceId,
  }: {
    file: Buffer;
    filename: string;
    mimeType: string;
    workspaceId: string;
  }): Promise<FileDTO> {
    const { files } = await this.fileUploadService.uploadFile({
      file,
      filename,
      mimeType,
      fileFolder: FileFolder.File,
      workspaceId,
    });

    if (!files.length) {
      throw new Error('Failed to upload file');
    }

    const createdFile = this.fileRepository.create({
      path: files[0].path,
      size: file.length,
      workspaceId,
    });

    const savedFile = await this.fileRepository.save(createdFile);

    return savedFile;
  }

  async deleteFileById(
    id: string,
    workspaceId: string,
  ): Promise<FileDTO | null> {
    const file = await this.fileRepository.findOne({
      where: { id, workspaceId },
    });

    if (!file) {
      return null;
    }

    const { folderPath, filename } = extractFolderPathFilenameAndTypeOrThrow(
      file.path,
    );

    try {
      if (file.path) {
        await this.fileService.deleteFile({
Repository: twentyhq/twenty
File: packages/twenty-server/src/utils/image.ts (TypeScript)

Code before:
    if (bufferLength === 0) {
      throw new Error('Received empty response from image URL');
    }

    const contentType = response.headers['content-type'];
    if (contentType && !contentType.startsWith('image/')) {
      throw new Error(
        `Invalid content type: expected image/*, got ${contentType}`,
      );
    }

    return Buffer.from(response.data, 'binary');
  } catch (error) {
    const axiosError = error as AxiosError;
    const axiosResponse = axiosError.response;
    if (axiosResponse) {
      throw new Error(
        `Failed to fetch image: HTTP ${axiosResponse.status} from ${url}`,
      );
    }
    
    if (error instanceof Error) {
      if (
        axiosError.code === 'ECONNABORTED' ||
        error.message.includes('timeout')
      ) {
        throw new Error(`Request timeout while fetching image from URL: ${url}`);
      }
      if (
        axiosError.code === 'ENOTFOUND' ||
        axiosError.code === 'ECONNREFUSED' ||
        error.message.includes('ENOTFOUND') ||
        error.message.includes('ECONNREFUSED')
      ) {
        throw new Error(`Failed to connect to image URL: ${url}`);
      }
      throw new Error(`Failed to fetch image from URL: ${error.message}`);
    }
    throw new Error(`Failed to fetch image from URL: ${url}`);
  }
};


Review comment (bug):
why do we have axiosError here ? It should be filter in previous condition no ?

Code after:
    if (bufferLength === 0) {
      throw new Error('Received empty response from image URL');
    }

    const contentType = response.headers['content-type'];

    if (contentType && !contentType.startsWith('image/')) {
      throw new Error(
        `Invalid content type: expected image/*, got ${contentType}`,
      );
    }

    return Buffer.from(response.data, 'binary');
  } catch (error) {
    const axiosError = error as AxiosError;
    const axiosResponse = axiosError.response;

    if (axiosResponse) {
      throw new Error(
        `Failed to fetch image: HTTP ${axiosResponse.status} from ${url}`,
      );
    }

    if (error instanceof Error) {
      if (
        axiosError.code === 'ECONNABORTED' ||
        error.message.includes('timeout')
      ) {
        throw new Error(
          `Request timeout while fetching image from URL: ${url}`,
        );
      }
      if (
        axiosError.code === 'ENOTFOUND' ||
        axiosError.code === 'ECONNREFUSED' ||
        error.message.includes('ENOTFOUND') ||
        error.message.includes('ECONNREFUSED')
      ) {
        throw new Error(`Failed to connect to image URL: ${url}`);
      }
      throw new Error(`Failed to fetch image from URL: ${error.message}`);
    }
    throw new Error(`Failed to fetch image from URL: ${url}`);
  }
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-inline-cell/property-box/components/PropertyBox.tsx (TypeScript)

Code before:
import isPropValid from '@emotion/is-prop-valid';
import styled from '@emotion/styled';

interface PropertyBoxProps {
  children: React.ReactNode;
  className?: string;
  dataTestId?: string;
}

const StyledPropertyBoxContainer = styled('div', {
  shouldForwardProp: isPropValid,
})`
  align-self: stretch;
  border-radius: ${({ theme }) => theme.border.radius.sm};
  display: flex;
  flex-direction: column;
  gap: ${({ theme }) => theme.spacing(2)};
  padding-top: ${({ theme }) => theme.spacing(3)};
  padding-bottom: ${({ theme }) => theme.spacing(3)};
  padding-left: ${({ theme }) => theme.spacing(3)};
  padding-right: ${({ theme }) => theme.spacing(2)};
`;

export const PropertyBox = ({
  children,
  className,
  dataTestId,
}: PropertyBoxProps) => (
  <StyledPropertyBoxContainer className={className} data-testid={dataTestId}>
    {children}
  </StyledPropertyBoxContainer>
);


Review comment (suggestion):
The layoutRenderingContext is obtained from useLayoutRenderingContext which uses createRequiredContext, meaning it will throw an error if the context is missing rather than return undefined. Therefore, the optional chaining on layoutRenderingContext is unnecessary and potentially misleading. You can safely access layoutRenderingContext.layoutType and layoutRenderingContext.isLegacyRecordShowPage directly without optional chaining.
```suggestion
    layoutRenderingContext.layoutType === PageLayoutType.RECORD_PAGE &&
    !layoutRenderingContext.isLegacyRecordShowPage;
```

Code after:
  align-self: stretch;
  border-radius: ${({ theme }) => theme.border.radius.sm};
  display: flex;
  flex-direction: column;
  gap: ${({ theme }) => theme.spacing(2)};
  padding-top: ${({ theme }) => theme.spacing(3)};
  padding-bottom: ${({ theme }) => theme.spacing(3)};
  padding-left: ${({ theme, noHorizontalPadding }) =>
    noHorizontalPadding ? 0 : theme.spacing(3)};
  padding-right: ${({ theme, noHorizontalPadding }) =>
    noHorizontalPadding ? 0 : theme.spacing(2)};
`;

/**
 * TODO: Remove noHorizontalPadding logic once the traditional record show page is removed.
 */
export const PropertyBox = ({
  children,
  className,
  dataTestId,
}: PropertyBoxProps) => {
  const layoutRenderingContext = useLayoutRenderingContext();

  const isInRecordPageLayout =
    layoutRenderingContext?.layoutType === PageLayoutType.RECORD_PAGE &&
    !layoutRenderingContext?.isLegacyRecordShowPage;

  return (
    <StyledPropertyBoxContainer
      className={className}
      data-testid={dataTestId}
      noHorizontalPadding={isInRecordPageLayout}
    >
      {children}
    </StyledPropertyBoxContainer>
  );
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-field-list/record-detail-section/components/RecordDetailRecordsListItemContainer.tsx (TypeScript)

Code before:
import styled from '@emotion/styled';

const StyledListItem = styled.div`
  align-items: center;
  justify-content: space-between;
  gap: ${({ theme }) => theme.spacing(1)};
  display: flex;
  height: ${({ theme }) => theme.spacing(10)};
  padding-left: ${({ theme }) => theme.spacing(3)};
  padding-right: ${({ theme }) => theme.spacing(2)};
`;

export { StyledListItem as RecordDetailRecordsListItemContainer };


Review comment (suggestion):
The layoutRenderingContext is obtained from useLayoutRenderingContext which uses createRequiredContext, meaning it will throw an error if the context is missing rather than return undefined. Therefore, the optional chaining on layoutRenderingContext is unnecessary and potentially misleading. You can safely access layoutRenderingContext.layoutType and layoutRenderingContext.isLegacyRecordShowPage directly without optional chaining.
```suggestion
    layoutRenderingContext.layoutType === PageLayoutType.RECORD_PAGE &&
    !layoutRenderingContext.isLegacyRecordShowPage;
```

Code after:
  gap: ${({ theme }) => theme.spacing(1)};
  display: flex;
  height: ${({ theme }) => theme.spacing(10)};
  padding-left: ${({ theme, noHorizontalPadding }) =>
    noHorizontalPadding ? 0 : theme.spacing(3)};
  padding-right: ${({ theme, noHorizontalPadding }) =>
    noHorizontalPadding ? 0 : theme.spacing(2)};
`;

type RecordDetailRecordsListItemContainerProps = {
  children: React.ReactNode;
  className?: string;
};

/**
 * TODO: Remove noHorizontalPadding logic once the traditional record show page is removed.
 */
export const RecordDetailRecordsListItemContainer = ({
  children,
  className,
}: RecordDetailRecordsListItemContainerProps) => {
  const layoutRenderingContext = useLayoutRenderingContext();

  const isInRecordPageLayout =
    layoutRenderingContext?.layoutType === PageLayoutType.RECORD_PAGE &&
    !layoutRenderingContext?.isLegacyRecordShowPage;

  return (
    <StyledListItem
      className={className}
      noHorizontalPadding={isInRecordPageLayout}
    >
      {children}
    </StyledListItem>
  );
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/fields/utils/filterAndOrderFieldsFromConfiguration.ts (TypeScript)

Code before:
  );

  const sectionsWithFields = sortedSections
    .map((section) => {
      const sectionFieldIds = new Set(
        section.fields.map((f) => f.fieldMetadataId),
      );

      const visibleFields = availableFieldMetadataItems
        .filter((fieldMetadataItem) => sectionFieldIds.has(fieldMetadataItem.id))
        .filter((fieldMetadataItem) => {
          const fieldConfig = section.fields.find(
            (f) => f.fieldMetadataId === fieldMetadataItem.id,
          );

          if (!isDefined(fieldConfig)) {
            return false;
          }

          return evaluateWidgetVisibility({
            conditionalDisplay: fieldConfig.conditionalDisplay,
            context,
          });
        });

      if (visibleFields.length === 0) {
        return null;
      }

      return {
        id: section.id,
        title: section.title,
        position: section.position,
        fields: visibleFields,
      };
    })
    .filter(isDefined);

  return sectionsWithFields;
};


Review comment (performance):
The nested filter with a find operation creates O(n*m) complexity where n is the number of available fields and m is the number of section fields. Consider creating a Map from section.fields to store fieldConfig by fieldMetadataId, similar to how the previous implementation used fieldMetadataItemsMap. This would reduce the complexity to O(n) and improve performance, especially for sections with many fields.

Code after:
  );

  const sectionsWithFields = sortedSections
    .map((section) => {
      const sectionFieldIds = new Set(
        section.fields.map((f) => f.fieldMetadataId),
      );

      const visibleFields = availableFieldMetadataItems
        .filter((fieldMetadataItem) =>
          sectionFieldIds.has(fieldMetadataItem.id),
        )
        .filter((fieldMetadataItem) => {
          const fieldConfig = section.fields.find(
            (f) => f.fieldMetadataId === fieldMetadataItem.id,
          );

          if (!isDefined(fieldConfig)) {
            return false;
          }

          return evaluateWidgetVisibility({
            conditionalDisplay: fieldConfig.conditionalDisplay,
            context,
          });
        });

      if (visibleFields.length === 0) {
        return null;
      }

      return {
        id: section.id,
        title: section.title,
        position: section.position,
        fields: visibleFields,
      };
    })
    .filter(isDefined);

  return sectionsWithFields;
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/widget-card/components/WidgetCardContent.tsx (TypeScript)

Code before:
import { css } from '@emotion/react';
import styled from '@emotion/styled';
import { type WidgetCardVariant } from '~/modules/page-layout/widgets/types/WidgetCardVariant';

const StyledWidgetCardContent = styled.div<{ variant: WidgetCardVariant }>`
  box-sizing: border-box;
  display: grid;
  grid-template-columns: minmax(0, 1fr);
  height: 100%;
  overflow: hidden;

  ${({ theme, variant }) => {
    if (variant === 'dashboard') {
      return css`
        padding: ${theme.spacing(2)};
      `;
    }

    if (variant === 'record-page') {
      return css`
        border: 1px solid ${theme.border.color.medium};
        border-radius: ${theme.border.radius.md};
        padding: ${theme.spacing(2)};
      `;
    }
  }}
`;

export { StyledWidgetCardContent as WidgetCardContent };


Review comment (suggestion):
The `:not(:empty)` pseudo-class selector will not work as intended here. The WidgetCardContent always contains child elements (either ErrorBoundary or StyledNoAccessContainer), so it will never be considered empty by CSS, even when the actual widget content is empty. The :empty selector only matches elements with no children at all, not even text nodes or whitespace. This means the margin-top will always be applied when hasHeader is true, which defeats the purpose of this change.
```suggestion
      margin-top: ${theme.spacing(2)};
    `}
```

Code after:
import { css } from '@emotion/react';
import styled from '@emotion/styled';
import { type WidgetCardVariant } from '~/modules/page-layout/widgets/types/WidgetCardVariant';

const StyledWidgetCardContent = styled.div<{
  variant: WidgetCardVariant;
  hasHeader: boolean;
}>`
  box-sizing: border-box;
  display: grid;
  grid-template-columns: minmax(0, 1fr);
  height: 100%;
  overflow: hidden;

  ${({ theme, hasHeader }) =>
    hasHeader &&
    css`
      :not(:empty) {
        margin-top: ${theme.spacing(2)};
      }
    `}

  ${({ theme, variant }) => {
    if (variant === 'dashboard') {
      return css`
        padding: ${theme.spacing(2)};
      `;
    }

    if (variant === 'record-page') {
      return css`
        border: 1px solid ${theme.border.color.medium};
        border-radius: ${theme.border.radius.md};
        padding: ${theme.spacing(2)};
      `;
    }
  }}
`;

export { StyledWidgetCardContent as WidgetCardContent };
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/core-modules/billing/services/billing-subscription.service.ts (TypeScript)

Code before:
    return {
      handleUnpaidInvoiceStripeSubscriptionId:
        billingSubscription.stripeSubscriptionId,
    };
  }

  async getWorkspaceEntitlements(
    workspaceId: string,
  ): Promise<BillingEntitlementEntity[]> {
    const isBillingEnabled = this.twentyConfigService.get('IS_BILLING_ENABLED');

    if (!isBillingEnabled) {
      return [];
    }

    const entitlements = await this.billingEntitlementRepository.find({
      where: { workspaceId },
    });

    return entitlements;
  }

  async getWorkspaceEntitlementByKey(
    workspaceId: string,
    key: BillingEntitlementKey,
  ): Promise<boolean> {
    const entitlement = await this.billingEntitlementRepository.findOneBy({
      workspaceId,
      key,
      value: true,
    });

    return entitlement?.value ?? false;
  }

  async endTrialPeriod(workspace: WorkspaceEntity) {
    const billingSubscription = await this.getCurrentBillingSubscriptionOrThrow(
      { workspaceId: workspace.id },
    );

    if (billingSubscription.status !== SubscriptionStatus.Trialing) {
      throw new BillingException(
        'Billing subscription is not in trial period',
        BillingExceptionCode.BILLING_SUBSCRIPTION_NOT_IN_TRIAL_PERIOD,
      );
    }

    const hasPaymentMethod = await this.stripeCustomerService.hasPaymentMethod(
      billingSubscription.stripeCustomerId,
    );


Review comment (suggestion):
***Polish:*** Unless I'm mistaken this sould be a `Partial<Record<BillingEntitlementKey, BillingEntitlementEntity>>`

Code after:

    return {
      handleUnpaidInvoiceStripeSubscriptionId:
        billingSubscription.stripeSubscriptionId,
    };
  }

  async getWorkspaceEntitlements(
    workspaceId: string,
  ): Promise<BillingEntitlementDTO[]> {
    const isBillingEnabled = this.twentyConfigService.get('IS_BILLING_ENABLED');
    const hasValidEnterpriseKey = isDefined(
      this.twentyConfigService.get('ENTERPRISE_KEY'),
    );

    const entitlements = await this.billingEntitlementRepository.find({
      where: { workspaceId },
    });

    const entitlementsByKey = entitlements.reduce(
      (acc, entitlement) => {
        acc[entitlement.key] = entitlement;

        return acc;
      },
      {} as Record<BillingEntitlementKey, BillingEntitlementEntity>,
    );

    return Object.values(BillingEntitlementKey).map((key) => ({
      key,
      value:
        hasValidEnterpriseKey &&
        (!isBillingEnabled || (entitlementsByKey[key]?.value ?? false)),
    }));
  }

  async getWorkspaceEntitlementByKey(
    workspaceId: string,
    key: BillingEntitlementKey,
  ): Promise<boolean> {
    const entitlement = await this.billingEntitlementRepository.findOneBy({
      workspaceId,
      key,
      value: true,
    });

    return entitlement?.value ?? false;
  }

  async endTrialPeriod(workspace: WorkspaceEntity) {
    const billingSubscription = await this.getCurrentBillingSubscriptionOrThrow(
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/workspace-event-emitter/utils/wrap-async-iterator-with-lifecycle.ts (TypeScript)

Code before:
    next: () => iterator.next(),
    return: async () => {
      let result: IteratorResult<T>;

      try {
        await cleanup();
      } finally {
        result = (await iterator.return?.()) ?? {
          done: true,
          value: undefined,
        };
      }

      return result;
    },
    throw: async (error) => {
      await cleanup();
      if (iterator.throw) {
        return iterator.throw(error);
      }
      throw error;
    },
    [Symbol.asyncIterator]() {
      return this;
    },
  };
}


Review comment (bug):
Race condition: The only way to clear the interval is to call cleanup.
Since you start the interval before the first iteration, if something happens between the interval start and the iterator start, the interval might never be cleaned up and call onHeartbeat forever. The fix would be to lazily call startInterval in the first next().

Code after:
    }
  };

  return {
    next: async () => {
      if (!isDefined(heartbeatInterval)) {
        startHeartbeat();
      }

      let result: IteratorResult<T>;

      try {
        result = await iterator.next();
      } catch (error) {
        await cleanup();

        throw error;
      }

      if (result.done) {
        await cleanup();
      }

      return result;
    },
    return: async () => {
      let result: IteratorResult<T>;

      try {
        await cleanup();
      } finally {
        result = (await iterator.return?.()) ?? {
          done: true,
          value: undefined,
        };
      }

      return result;
    },
    throw: async (error) => {
      await cleanup();
      if (iterator.throw) {
        return iterator.throw(error);
      }
      throw error;
    },
    [Symbol.asyncIterator]() {
      return this;
    },
  };
}
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/command-menu/pages/ask-ai/components/CommandMenuAskAIPage.tsx (TypeScript)

Code before:
import styled from '@emotion/styled';
import { AIChatTab } from '@/ai/components/AIChatTab';
import { useIsMobile } from '@/ui/utilities/responsive/hooks/useIsMobile';

const StyledContainer = styled.div<{ isMobile: boolean }>`
  height: ${({ theme, isMobile }) => {
    const mobileOffset = isMobile ? theme.spacing(13) : '0px';

    return `calc(100% - ${mobileOffset})`;
  }};
  width: 100%;
`;

export const CommandMenuAskAIPage = () => {
  const isMobile = useIsMobile();

  return (
    <StyledContainer isMobile={isMobile}>
      <AIChatTab />
    </StyledContainer>
  );
};


Review comment (suggestion):
I feel this adaptation should be handled at command menu level for all pages

Code after:
import styled from '@emotion/styled';
import { AIChatTab } from '@/ai/components/AIChatTab';

const StyledContainer = styled.div`
  height: 100%;
  width: 100%;
`;

export const CommandMenuAskAIPage = () => {
  return (
    <StyledContainer>
      <AIChatTab />
    </StyledContainer>
  );
};