ronantakizawa/codereview-bench huggingface dataset:
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/fields/components/FieldsWidgetSectionContainer.tsx (TypeScript)

Code before:
}: FieldsWidgetSectionContainerProps) => {
  const [isExpanded, setIsExpanded] = useState(true);
  const theme = useTheme();

  const handleToggleSection = () =>
    setIsExpanded((previousIsExpanded) => !previousIsExpanded);

  return (
    <StyledFieldsWidgetSectionContainer>
      <StyledHeader onClick={handleToggleSection}>
        <StyledTitleLabel>{title}</StyledTitleLabel>
        {isExpanded ? (
          <IconChevronUp
            size={theme.icon.size.md}
            stroke={theme.icon.stroke.sm}
            color={theme.font.color.tertiary}
          />
        ) : (
          <IconChevronDown
            size={theme.icon.size.md}
            stroke={theme.icon.stroke.sm}
            color={theme.font.color.tertiary}
          />
        )}
      </StyledHeader>
      <AnimatedExpandableContainer isExpanded={isExpanded}>
        {children}
      </AnimatedExpandableContainer>
    </StyledFieldsWidgetSectionContainer>
  );
};


Review comment (suggestion):
The animation happens when the component mounts. If I remember well, there is an option to ask the library not to animate on mount.

https://github.com/user-attachments/assets/48754d97-a931-48b1-95bc-900877a5a511

Code after:
type FieldsWidgetSectionContainerProps = {
  children: React.ReactNode;
  title: string;
};

export const FieldsWidgetSectionContainer = ({
  children,
  title,
}: FieldsWidgetSectionContainerProps) => {
  const [isExpanded, setIsExpanded] = useState(true);
  const theme = useTheme();

  const handleToggleSection = () =>
    setIsExpanded((previousIsExpanded) => !previousIsExpanded);

  return (
    <StyledFieldsWidgetSectionContainer>
      <StyledHeader onClick={handleToggleSection}>
        <StyledTitleLabel>{title}</StyledTitleLabel>
        <StyledChevronIcon
          isExpanded={isExpanded}
          size={theme.icon.size.md}
          stroke={theme.icon.stroke.sm}
        />
      </StyledHeader>
      <AnimatedExpandableContainer
        isExpanded={isExpanded}
        initial={false}
        mode="fit-content"
      >
        {children}
      </AnimatedExpandableContainer>
    </StyledFieldsWidgetSectionContainer>
  );
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/fields/components/FieldsWidgetSectionContainer.tsx (TypeScript)

Code before:
type FieldsWidgetSectionContainerProps = {
  children: React.ReactNode;
  title: string;
};

export const FieldsWidgetSectionContainer = ({
  children,
  title,
}: FieldsWidgetSectionContainerProps) => {
  const [isExpanded, setIsExpanded] = useState(true);
  const theme = useTheme();

  const handleToggleSection = () =>
    setIsExpanded((previousIsExpanded) => !previousIsExpanded);

  return (
    <StyledFieldsWidgetSectionContainer>
      <StyledHeader onClick={handleToggleSection}>
        <StyledTitleLabel>{title}</StyledTitleLabel>
        {isExpanded ? (
          <IconChevronUp
            size={theme.icon.size.md}
            stroke={theme.icon.stroke.sm}
            color={theme.font.color.tertiary}
          />
        ) : (
          <IconChevronDown
            size={theme.icon.size.md}
            stroke={theme.icon.stroke.sm}
            color={theme.font.color.tertiary}
          />
        )}
      </StyledHeader>
      <AnimatedExpandableContainer isExpanded={isExpanded}>
        {children}
      </AnimatedExpandableContainer>
    </StyledFieldsWidgetSectionContainer>
  );
};


Review comment (suggestion):
I feel it would be better to rotate the icon instead of going from chevron up to chevron down. This is a pattern we widely use in the codebase, I guess you can take inspiration from there.

https://github.com/user-attachments/assets/75598165-de55-490e-8241-e1ae768a56d5

Code after:
const StyledChevronIcon = styled(IconChevronDown)<{ isExpanded: boolean }>`
  color: ${({ theme }) => theme.font.color.tertiary};
  transform: ${({ isExpanded }) =>
    isExpanded ? 'rotate(180deg)' : 'rotate(0deg)'};
  transition: ${({ theme }) =>
    `transform ${theme.animation.duration.normal}s ease`};
`;

type FieldsWidgetSectionContainerProps = {
  children: React.ReactNode;
  title: string;
};

export const FieldsWidgetSectionContainer = ({
  children,
  title,
}: FieldsWidgetSectionContainerProps) => {
  const [isExpanded, setIsExpanded] = useState(true);
  const theme = useTheme();

  const handleToggleSection = () =>
    setIsExpanded((previousIsExpanded) => !previousIsExpanded);

  return (
    <StyledFieldsWidgetSectionContainer>
      <StyledHeader onClick={handleToggleSection}>
        <StyledTitleLabel>{title}</StyledTitleLabel>
        <StyledChevronIcon
          isExpanded={isExpanded}
          size={theme.icon.size.md}
          stroke={theme.icon.stroke.sm}
        />
      </StyledHeader>
      <AnimatedExpandableContainer
        isExpanded={isExpanded}
        initial={false}
        mode="fit-content"
      >
        {children}
      </AnimatedExpandableContainer>
    </StyledFieldsWidgetSectionContainer>
  );
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/hooks/usePageLayoutWithRelationWidgets.ts (TypeScript)

Code before:
    firstTab.id,
  );

  return {
    ...layout,
    tabs: layout.tabs.map((tab) => {
      if (tab.id === firstTab.id) {
        return {
          ...tab,
          widgets: [...tab.widgets, ...relationWidgets],
        };
      }
      return tab;
    }),
  };
};

const computePageLayoutWithRelationWidgets = (
  basePageLayout: PageLayout,
  boxedRelationFieldMetadataItems: FieldMetadataItem[],
): PageLayout => {
  return injectRelationWidgetsIntoLayout(
    basePageLayout,
    boxedRelationFieldMetadataItems,
  );
};

export const usePageLayoutWithRelationWidgets = (
  basePageLayout: PageLayout | undefined,
): PageLayout | undefined => {
  const { targetRecordIdentifier, layoutType } = useLayoutRenderingContext();

  const { boxedRelationFieldMetadataItems } = useFieldListFieldMetadataItems({
    objectNameSingular: targetRecordIdentifier?.targetObjectNameSingular ?? '',
  });

  if (!isDefined(basePageLayout)) {
    return undefined;
  }

  const isRecordPage = layoutType === PageLayoutType.RECORD_PAGE;

  if (!isRecordPage) {
    return basePageLayout;
  }

  return computePageLayoutWithRelationWidgets(
    basePageLayout,
    boxedRelationFieldMetadataItems,
  );
};

Review comment (suggestion):
The computePageLayoutWithRelationWidgets function is a simple wrapper that only calls injectRelationWidgetsIntoLayout. This intermediate function adds no value and can be removed for simplicity. Consider calling injectRelationWidgetsIntoLayout directly in the hook.

Code after:
    firstTab.id,
  );

  return {
    ...layout,
    tabs: layout.tabs.map((tab) => {
      if (tab.id === firstTab.id) {
        return {
          ...tab,
          widgets: [...tab.widgets, ...relationWidgets],
        };
      }
      return tab;
    }),
  };
};

export const usePageLayoutWithRelationWidgets = (
  basePageLayout: PageLayout | undefined,
): PageLayout | undefined => {
  const { targetRecordIdentifier, layoutType } = useLayoutRenderingContext();

  const { boxedRelationFieldMetadataItems } = useFieldListFieldMetadataItems({
    objectNameSingular: targetRecordIdentifier?.targetObjectNameSingular ?? '',
  });

  if (!isDefined(basePageLayout)) {
    return undefined;
  }

  const isRecordPage = layoutType === PageLayoutType.RECORD_PAGE;

  if (!isRecordPage) {
    return basePageLayout;
  }

  return injectRelationWidgetsIntoLayout(
    basePageLayout,
    boxedRelationFieldMetadataItems,
  );
};
Repository: twentyhq/twenty
File: packages/twenty-shared/src/application/objectManifestType.ts (TypeScript)

Code before:
import { type FieldManifest } from '@/application';
import { type RelationFieldManifest } from '@/application/relationFieldManifestType';
import { type SyncableEntityOptions } from '@/application/syncableEntityOptionsType';

export type ObjectManifest = SyncableEntityOptions & {
  nameSingular: string;
  namePlural: string;
  labelSingular: string;
  labelPlural: string;
  description?: string;
  icon?: string;
  fields: FieldManifest[];
  relations?: RelationFieldManifest[];
};


Review comment (suggestion):
I think I would prefer 
```suggestion
fields: (FieldManifest | RelationFieldManifest)[] |;
```

Code after:
import { type FieldManifest } from '@/application';
import { type RelationFieldManifest } from '@/application/relationFieldManifestType';
import { type SyncableEntityOptions } from '@/application/syncableEntityOptionsType';

export type ObjectManifest = SyncableEntityOptions & {
  nameSingular: string;
  namePlural: string;
  labelSingular: string;
  labelPlural: string;
  description?: string;
  icon?: string;
  fields: (FieldManifest | RelationFieldManifest)[];
};
Repository: twentyhq/twenty
File: packages/twenty-server/src/modules/messaging/message-import-manager/drivers/imap/services/imap-find-sent-folder.service.ts (TypeScript)

Code before:
        };
      }
    }

    if (regexCandidateFolders.length > 0) {
      this.logger.log(
        `Using first regex candidate sent folder: ${regexCandidateFolders[0].path} (no messages found in any regex candidate)`,
      );

      const folder = regexCandidateFolders[0];

      return {
        name: folder.name,
        path: folder.path,
      };
    }

    return null;
  }

  private async getFolderMessageCount(
    client: ImapFlow,
    folderPath: string,
  ): Promise<number> {
    try {
      const lock = await client.getMailboxLock(folderPath);

      try {
        const status = await client.status(folderPath, {
          messages: true,
        });

        const messageCount = status?.messages;

        this.logger.debug(
          `Folder "${folderPath}" has ${messageCount} messages`,
        );

        return isNumber(messageCount) ? messageCount : 0;
      } finally {
        lock.release();
      }
    } catch (error) {
      this.logger.warn(
        `Error checking folder "${folderPath}": ${error.message}`,
      );

      return 0;
    }
  }
}

Review comment (suggestion):
@neo773 could you explain when you do this kind of change? why don't we need the lock anymore? why was it introduced in the first place.

This helps for the review

Code after:
        };
      }
    }

    if (regexCandidateFolders.length > 0) {
      this.logger.log(
        `Using first regex candidate sent folder: ${regexCandidateFolders[0].path} (no messages found in any regex candidate)`,
      );

      const folder = regexCandidateFolders[0];

      return {
        name: folder.name,
        path: folder.path,
      };
    }

    return null;
  }

  private async getFolderMessageCount(
    client: ImapFlow,
    folderPath: string,
  ): Promise<number> {
    try {
      const status = await client.status(folderPath, {
        messages: true,
      });

      const messageCount = status?.messages;

      this.logger.debug(`Folder "${folderPath}" has ${messageCount} messages`);

      return isNumber(messageCount) ? messageCount : 0;
    } catch (error) {
      this.logger.warn(
        `Error checking folder "${folderPath}": ${error.message}`,
      );

      return 0;
    }
  }
}
Repository: twentyhq/twenty
File: packages/twenty-server/src/modules/messaging/message-import-manager/drivers/imap/utils/create-sync-cursor.util.ts (TypeScript)

Code before:
import { type MailboxState } from './extract-mailbox-state.util';
import { type ImapSyncCursor } from './parse-sync-cursor.util';

export const createSyncCursor = (
  messages: { uid: number }[],
  previousCursor: ImapSyncCursor | null,
  mailboxState: MailboxState,
): ImapSyncCursor => {
  const { uidValidity, highestModSeq } = mailboxState;
  const lastSeenUid = previousCursor?.highestUid ?? 0;

  const highestUid =
    messages.length > 0
      ? Math.max(...messages.map((message) => message.uid))
      : lastSeenUid;

  return {
    highestUid,
    uidValidity,
    ...(highestModSeq ? { modSeq: highestModSeq.toString() } : {}),
  };
};


Review comment (refactor):
ok, we usually avoid the for loop in the code base for reduce API in case we want to extract one aggregate (here the max). This avoid mutating variable (here let highestUid)

Code after:
import { type MailboxState } from './extract-mailbox-state.util';
import { type ImapSyncCursor } from './parse-sync-cursor.util';

export const createSyncCursor = (
  messageUids: number[],
  previousCursor: ImapSyncCursor | null,
  mailboxState: MailboxState,
): ImapSyncCursor => {
  const { uidValidity, highestModSeq } = mailboxState;
  const lastSeenUid = previousCursor?.highestUid ?? 0;

  let highestUid = lastSeenUid;

  for (let i = 0; i < messageUids.length; i++) {
    if (messageUids[i] > highestUid) {
      highestUid = messageUids[i];
    }
  }

  return {
    highestUid,
    uidValidity,
    ...(highestModSeq ? { modSeq: highestModSeq.toString() } : {}),
  };
};
Repository: twentyhq/twenty
File: packages/twenty-ui/src/theme/constants/FontDark.ts (TypeScript)

Code before:
import { COLOR_DARK } from '@ui/theme/constants/ColorsDark';
import { FONT_COMMON } from './FontCommon';
import { GRAY_SCALE_DARK } from './GrayScaleDark';

export const FONT_DARK = {
  color: {
    primary: GRAY_SCALE_DARK.gray12,
    secondary: GRAY_SCALE_DARK.gray11,
    tertiary: GRAY_SCALE_DARK.gray9,
    light: GRAY_SCALE_DARK.gray8,
    extraLight: GRAY_SCALE_DARK.gray7,
    inverted: GRAY_SCALE_DARK.gray1,
    danger: COLOR_DARK.red,
    indigo: COLOR_DARK.blue9,
  },
  ...FONT_COMMON,
};


Review comment (suggestion):
We already use these colors, try maybe theme.color.blue? etc ; I don't think we need to introduce these

Code after:
import { COLOR_DARK } from '@ui/theme/constants/ColorsDark';
import { FONT_COMMON } from './FontCommon';
import { GRAY_SCALE_DARK } from './GrayScaleDark';

export const FONT_DARK = {
  color: {
    primary: GRAY_SCALE_DARK.gray12,
    secondary: GRAY_SCALE_DARK.gray11,
    tertiary: GRAY_SCALE_DARK.gray9,
    light: GRAY_SCALE_DARK.gray8,
    extraLight: GRAY_SCALE_DARK.gray7,
    inverted: GRAY_SCALE_DARK.gray1,
    danger: COLOR_DARK.red,
  },
  ...FONT_COMMON,
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-table/components/RecordTableDeactivateRecordTableRowEffect.tsx (TypeScript)

Code before:
import { isCommandMenuOpenedState } from '@/command-menu/states/isCommandMenuOpenedState';
import { useActiveRecordTableRow } from '@/object-record/record-table/hooks/useActiveRecordTableRow';
import { useListenToSidePanelClosing } from '@/ui/layout/right-drawer/hooks/useListenToSidePanelClosing';
import { useRecoilCallback } from 'recoil';

export const RecordTableDeactivateRecordTableRowEffect = () => {
  const { deactivateRecordTableRow } = useActiveRecordTableRow();

  const deactivateRowIfMenuClosed = useRecoilCallback(
    ({ snapshot }) =>
      () => {
        const isCommandMenuOpened = snapshot
          .getLoadable(isCommandMenuOpenedState)
          .getValue();

        if (!isCommandMenuOpened) {
          deactivateRecordTableRow();
        }
      },
    [deactivateRecordTableRow],
  );

  useListenToSidePanelClosing(() => {
    queueMicrotask(deactivateRowIfMenuClosed);
  });

  return null;
};


Review comment (refactor):
This is more or less the same as doing a setTimeout 0, it would be better to try to refactor this to make it synchronous rather than using an effect

Code after:
import { useActiveRecordTableRow } from '@/object-record/record-table/hooks/useActiveRecordTableRow';
import { useListenToSidePanelClosing } from '@/ui/layout/right-drawer/hooks/useListenToSidePanelClosing';

export const RecordTableDeactivateRecordTableRowEffect = () => {
  const { deactivateRecordTableRow } = useActiveRecordTableRow();

  useListenToSidePanelClosing(() => {
    deactivateRecordTableRow();
  });

  return null;
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/sse-db-event/components/SSEProviderEffect.tsx (TypeScript)

Code before:
import { SseClientContext } from '@/sse-db-event/contexts/SseClientContext';
import { ON_EVENT_SUBSCRIPTION } from '@/sse-db-event/graphql/subscriptions/OnEventSubscription';
import { sseEventStreamIdState } from '@/sse-db-event/states/sseEventStreamIdState';
import { dispatchObjectRecordEventsWithQueryIds } from '@/sse-db-event/utils/dispatchObjectRecordEventsWithQueryIds';
import { useIsFeatureEnabled } from '@/workspace/hooks/useIsFeatureEnabled';
import { isNonEmptyString } from '@sniptt/guards';
import { type ExecutionResult, print } from 'graphql';
import { useContext, useEffect } from 'react';
import { useRecoilState } from 'recoil';
import { isDefined } from 'twenty-shared/utils';
import { v4 } from 'uuid';
import { type EventSubscription, FeatureFlagKey } from '~/generated/graphql';

export const SSEProviderEffect = () => {
  const sseClient = useContext(SseClientContext);
  const isSseDbEventsEnabled = useIsFeatureEnabled(
    FeatureFlagKey.IS_SSE_DB_EVENTS_ENABLED,
  );

  const [sseEventStreamId, setSseEventStreamId] = useRecoilState(
    sseEventStreamIdState,
  );

  useEffect(() => {
    if (!isSseDbEventsEnabled || !isDefined(sseClient)) {
      return;
    }

    if (!isNonEmptyString(sseEventStreamId)) {
      setSseEventStreamId(v4());
      return;
    }

    const unsubscribe = sseClient.subscribe(
      {
        query: print(ON_EVENT_SUBSCRIPTION),
        variables: {
          eventStreamId: sseEventStreamId,

Review comment (suggestion):
if you update that you might update SSEQuerySubscribeEffect isn't it?
(I would change none of those 2 files personally, change in `packages/twenty-front/src/modules/sse-db-event/components/SSEProvider.tsx` is enough to me)

Code after:
import { SseClientContext } from '@/sse-db-event/contexts/SseClientContext';
import { ON_EVENT_SUBSCRIPTION } from '@/sse-db-event/graphql/subscriptions/OnEventSubscription';
import { sseEventStreamIdState } from '@/sse-db-event/states/sseEventStreamIdState';
import { dispatchObjectRecordEventsWithQueryIds } from '@/sse-db-event/utils/dispatchObjectRecordEventsWithQueryIds';
import { isNonEmptyString } from '@sniptt/guards';
import { type ExecutionResult, print } from 'graphql';
import { useContext, useEffect } from 'react';
import { useRecoilState } from 'recoil';
import { isDefined } from 'twenty-shared/utils';
import { v4 } from 'uuid';
import { type EventSubscription } from '~/generated/graphql';

export const SSEProviderEffect = () => {
  const sseClient = useContext(SseClientContext);

  const [sseEventStreamId, setSseEventStreamId] = useRecoilState(
    sseEventStreamIdState,
  );

  useEffect(() => {
    if (!isDefined(sseClient)) {
      return;
    }

    if (!isNonEmptyString(sseEventStreamId)) {
      setSseEventStreamId(v4());
      return;
    }

    const unsubscribe = sseClient.subscribe(
      {
        query: print(ON_EVENT_SUBSCRIPTION),
        variables: {
          eventStreamId: sseEventStreamId,
        },
      },
      {
        next: (
Repository: twentyhq/twenty
File: packages/twenty-sdk/src/cli/services/generate.service.ts (TypeScript)

Code before:
    return this.client.query(request);
  }

  mutation<R extends MutationGenqlSelection>(request: R & { __name?: string }) {
    return this.client.mutation(request);
  }

  async uploadFile(
    fileBuffer: Buffer,
    filename: string,
    contentType: string = 'application/octet-stream',
  ): Promise<{ path: string; token: string }> {
    const form = new FormData();
    const fileFolder = "Attachment";

    form.append('operations', JSON.stringify({
      query: \`mutation UploadFile($file: Upload!, $fileFolder: FileFolder) {
        uploadFile(file: $file, fileFolder: $fileFolder) { path token }
      }\`,
      variables: { file: null, fileFolder },
    }));
    form.append('map', JSON.stringify({ '0': ['variables.file'] }));
    form.append('0', new Blob([fileBuffer], { type: contentType }), filename);


    const response = await fetch(\`\${process.env.TWENTY_API_URL}/graphql\`, {
      method: 'POST',
      headers: {
        Authorization: \`Bearer \${process.env.TWENTY_API_KEY}\`,
      },
      body: form,
    });

    const result = await response.json();

    if (result.errors) {
      throw new GenqlError(result.errors, result.data);
    }

    return result.data.uploadFile;
  }
}

`;

    await fs.appendFile(join(output, 'index.ts'), twentyClientContent);
  }
}


Review comment (suggestion):
should not be process.env.TWENTY_API_URL but should be `merged` (see in Twenty constructor)

Code after:
    return this.client.query(request);
  }

  mutation<R extends MutationGenqlSelection>(request: R & { __name?: string }) {
    return this.client.mutation(request);
  }

  async uploadFile(
    fileBuffer: Buffer,
    filename: string,
    contentType: string = 'application/octet-stream',
    fileFolder: string = 'Attachment',
  ): Promise<{ path: string; token: string }> {
    const form = new FormData();

    form.append('operations', JSON.stringify({
      query: \`mutation UploadFile($file: Upload!, $fileFolder: FileFolder) {
        uploadFile(file: $file, fileFolder: $fileFolder) { path token }
      }\`,
      variables: { file: null, fileFolder },
    }));
    form.append('map', JSON.stringify({ '0': ['variables.file'] }));
    form.append('0', new Blob([fileBuffer], { type: contentType }), filename);


    const response = await fetch(\`\${process.env.TWENTY_API_URL}/graphql\`, {
      method: 'POST',
      headers: {
        Authorization: \`Bearer \${process.env.TWENTY_API_KEY}\`,
      },
      body: form,
    });

    const result = await response.json();

    if (result.errors) {
      throw new GenqlError(result.errors, result.data);
    }

    return result.data.uploadFile;
  }
}

`;

    await fs.appendFile(join(output, 'index.ts'), twentyClientContent);
  }
}