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

Code before:
      cursor = nextCursor;
    } while (cursor !== 0);
  }

  async scanAndCountSetMembers(scanPattern: string): Promise<number> {
    if (!this.isRedisCache()) {
      throw new Error(
        'scanAndCountSetMembers is only supported with Redis cache',
      );
    }

    const redisClient = (this.cache as RedisCache).store.client;
    let cursor = 0;
    let totalCount = 0;

    do {
      const result = await redisClient.scan(cursor, {
        MATCH: `${this.namespace}:${scanPattern}`,
        COUNT: 100,
      });

      cursor = result.cursor;
      const keys = result.keys;

      if (keys.length > 0) {
        const counts = await Promise.all(
          keys.map((key) => redisClient.sCard(key)),
        );

        totalCount += counts.reduce((sum, count) => sum + count, 0);
      }
    } while (cursor !== 0);

    return totalCount;
  }

  async acquireLock(key: string, ttl = 1000): Promise<boolean> {
    if (!this.isRedisCache()) {
      throw new Error('acquireLock is only supported with Redis cache');
    }

    const redisClient = (this.cache as RedisCache).store.client;

    const result = await redisClient.set(this.getKey(key), 'lock', {
      NX: true,
      PX: ttl,
    });

    return result === 'OK';
  }


Review comment (suggestion):
Actually my bad, multi is blocking here. I'm not sure this is a good idea after all and we probably prefer the multiple round trips 🤔 

Code after:
      cursor = nextCursor;
    } while (cursor !== 0);
  }

  async scanAndCountSetMembers(scanPattern: string): Promise<number> {
    if (!this.isRedisCache()) {
      throw new Error(
        'scanAndCountSetMembers is only supported with Redis cache',
      );
    }

    const redisClient = (this.cache as RedisCache).store.client;
    let cursor = 0;
    let totalCount = 0;

    do {
      const result = await redisClient.scan(cursor, {
        MATCH: `${this.namespace}:${scanPattern}`,
        COUNT: 100,
      });

      cursor = result.cursor;
      const keys = result.keys;

      if (keys.length > 0) {
        const pipeline = redisClient.multi();

        for (const key of keys) {
          pipeline.sCard(key);
        }

        const results = await pipeline.exec();

        for (const result of results) {
          if (result instanceof Error) {
            throw result;
          }
          totalCount += result as number;
        }
      }
    } while (cursor !== 0);

    return totalCount;
  }

  async acquireLock(key: string, ttl = 1000): Promise<boolean> {
    if (!this.isRedisCache()) {
      throw new Error('acquireLock is only supported with Redis cache');
    }

    const redisClient = (this.cache as RedisCache).store.client;
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/metadata-modules/cron-trigger/crons/jobs/cron-trigger.cron.job.ts (TypeScript)

Code before:
      select: ['id'],
    });

    const now = new Date();

    for (const activeWorkspace of activeWorkspaces) {
      const cronTriggers = await this.cronTriggerRepository.find({
        where: {
          workspaceId: activeWorkspace.id,
        },
        select: ['id', 'settings', 'workspaceId'],
        relations: ['serverlessFunction'],
      });

      for (const cronTrigger of cronTriggers) {
        const settings = cronTrigger.settings;

        if (!isDefined(settings.pattern)) {
          continue;
        }

        if (!shouldRunNow(settings.pattern, now)) {
          continue;
        }

        await this.messageQueueService.add<ServerlessFunctionTriggerJobData>(
          ServerlessFunctionTriggerJob.name,
          {
            serverlessFunctionId: cronTrigger.serverlessFunction.id,
            workspaceId: cronTrigger.workspaceId,
            payload: {},
          },
          { retryLimit: 3 },
        );
      }
    }
  }
}


Review comment (suggestion):
that seems to be the expected type - it is what is used in call-database-event-trigger-jobs

Code after:
      select: ['id'],
    });

    const now = new Date();

    for (const activeWorkspace of activeWorkspaces) {
      const cronTriggers = await this.cronTriggerRepository.find({
        where: {
          workspaceId: activeWorkspace.id,
        },
        select: ['id', 'settings', 'workspaceId'],
        relations: ['serverlessFunction'],
      });

      for (const cronTrigger of cronTriggers) {
        const settings = cronTrigger.settings;

        if (!isDefined(settings.pattern)) {
          continue;
        }

        if (!shouldRunNow(settings.pattern, now)) {
          continue;
        }

        await this.messageQueueService.add<ServerlessFunctionTriggerJobData[]>(
          ServerlessFunctionTriggerJob.name,
          [
            {
              serverlessFunctionId: cronTrigger.serverlessFunction.id,
              workspaceId: cronTrigger.workspaceId,
              payload: {},
            },
          ],
          { retryLimit: 3 },
        );
      }
    }
  }
}
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-field/ui/hooks/useClearField.ts (TypeScript)

Code before:
          foundFieldMetadataItem.settings?.relationType ===
            RelationType.ONE_TO_MANY;

        if (isOneToManyRelation) {
          return;
        }

        const fieldName = fieldDefinition.metadata.fieldName;

        const emptyFieldValue = generateEmptyFieldValue({
          fieldMetadataItem: foundFieldMetadataItem,
        });

        set(
          recordStoreFamilySelector({ recordId, fieldName }),
          emptyFieldValue,
        );

        const isManyToOneRelation =
          isRelation &&
          foundFieldMetadataItem.settings?.relationType ===
            RelationType.MANY_TO_ONE;

        const updateFieldName = isManyToOneRelation
          ? getForeignKeyNameFromRelationFieldName(fieldName)
          : fieldName;

        updateRecord?.({
          variables: {
            where: { id: recordId },
            updateOneRecordInput: {
              [updateFieldName]: emptyFieldValue,
            },
          },
        });
      },
    [recordId, fieldDefinition, updateRecord],
  );

  return clearField;
};


Review comment (suggestion):
this fixes the sentry - we were trying to update `people.company` instead of `people.companyId`

Code after:
          foundFieldMetadataItem.settings?.relationType ===
            RelationType.ONE_TO_MANY;

        if (shouldSkipClearingBecauseInvolvesMultipleRecords) {
          return;
        }

        const fieldName = fieldDefinition.metadata.fieldName;

        const emptyFieldValue = generateEmptyFieldValue({
          fieldMetadataItem: foundFieldMetadataItem,
        });

        set(
          recordStoreFamilySelector({ recordId, fieldName }),
          emptyFieldValue,
        );

        const isManyToOneRelation =
          isRelation &&
          foundFieldMetadataItem.settings?.relationType ===
            RelationType.MANY_TO_ONE;

        const updateFieldName = isManyToOneRelation
          ? getForeignKeyNameFromRelationFieldName(fieldName)
          : fieldName;

        if (isManyToOneRelation) {
          set(
            recordStoreFamilySelector({ recordId, fieldName: updateFieldName }),
            emptyFieldValue,
          );
        }

        updateRecord?.({
          variables: {
            where: { id: recordId },
            updateOneRecordInput: {
              [updateFieldName]: emptyFieldValue,
            },
          },
        });
      },
    [recordId, fieldDefinition, updateRecord],
  );

  return clearField;
};
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/validator-utils/validate-emails-primary-email-subfield-or-throw.util.ts (TypeScript)

Code before:
import { msg } from '@lingui/core/macro';
import { isNonEmptyString, isNull } from '@sniptt/guards';
import { z } from 'zod';

import {
  CommonQueryRunnerException,
  CommonQueryRunnerExceptionCode,
} from 'src/engine/api/common/common-query-runners/errors/common-query-runner.exception';

export const validateEmailsPrimaryEmailSubfieldOrThrow = (
  value: unknown,
  fieldName: string,
): string | null => {
  if (isNull(value)) return null;

  if (typeof value !== 'string') {
    const inspectedValue = inspect(value);

    throw new CommonQueryRunnerException(
      `Invalid string value ${inspectedValue} for email field "${fieldName}"`,
      CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA,
      { userFriendlyMessage: msg`Invalid value: "${inspectedValue}"` },
    );
  }

  if (!z.email().trim().safeParse(value).success && isNonEmptyString(value)) {
    const inspectedValue = inspect(value);

    throw new CommonQueryRunnerException(
      `Invalid string value ${inspectedValue} for email field "${fieldName}"`,
      CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA,
      { userFriendlyMessage: msg`Invalid value: "${inspectedValue}"` },
    );
  }

  return value;
};


Review comment (suggestion):
this one important - when testing on UI and trying to remove an email, it sends empty string to the backend and then throws an error

Code after:
import { msg } from '@lingui/core/macro';
import { isNonEmptyString, isNull } from '@sniptt/guards';
import { z } from 'zod';

import {
  CommonQueryRunnerException,
  CommonQueryRunnerExceptionCode,
} from 'src/engine/api/common/common-query-runners/errors/common-query-runner.exception';

export const validateEmailsPrimaryEmailSubfieldOrThrow = (
  value: unknown,
  fieldName: string,
): string | null => {
  if (isNull(value)) return null;

  if (typeof value !== 'string') {
    const inspectedValue = inspect(value);

    throw new CommonQueryRunnerException(
      `Invalid string value ${inspectedValue} for email field "${fieldName}"`,
      CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA,
      { userFriendlyMessage: msg`Invalid value: "${inspectedValue}"` },
    );
  }

  if (
    !z.email({ pattern: z.regexes.unicodeEmail }).safeParse(value).success &&
    isNonEmptyString(value)
  ) {
    const inspectedValue = inspect(value);

    throw new CommonQueryRunnerException(
      `Invalid string value ${inspectedValue} for email field "${fieldName}"`,
      CommonQueryRunnerExceptionCode.INVALID_ARGS_DATA,
      { userFriendlyMessage: msg`Invalid value: "${inspectedValue}"` },
    );
  }

  return value;
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/pages/onboarding/internal/ChooseYourPlanContent.tsx (TypeScript)

Code before:
          ))}
        </StyledChooseTrialContainer>
      )}
      <MainButton
        title={t`Continue`}
        onClick={handleCheckoutSession}
        width={200}
        Icon={() => isSubmitting && <Loader />}
        disabled={isSubmitting}
      />
      <StyledLinkGroup>
        <ClickToActionLink onClick={signOut}>
          <Trans>Log out</Trans>
        </ClickToActionLink>
        <span />
        <ClickToActionLink
          href={calendarBookingPageId ? AppPath.BookCall : CAL_LINK}
          target={calendarBookingPageId ? '_self' : '_blank'}
          rel={calendarBookingPageId ? '' : 'noreferrer'}
        >
          <Trans>Book a Call</Trans>
        </ClickToActionLink>
        <span />
        {currentPlanKey === BillingPlanKey.PRO ? (
          <ClickToActionLink
            href={AppPath.PlanRequired.concat('?plan=enterprise')}
          >
            <Trans>Organization plan</Trans>
          </ClickToActionLink>
        ) : (
          <ClickToActionLink href={AppPath.PlanRequired.concat('?plan=pro')}>
            <Trans>Pro plan</Trans>
          </ClickToActionLink>
        )}
      </StyledLinkGroup>
    </>
  );
};


Review comment (suggestion):
this concatenation does not seem to be enough, we should create a link with a full billingSessionState

<img width="1512" height="911" alt="Image" src="https://github.com/user-attachments/assets/c89953c9-51c6-4a50-998d-4a60c231f8c0" />

Code after:
              key={trialPeriod.duration}
            >
              <TrialCard
                duration={trialPeriod.duration}
                withCreditCard={trialPeriod.isCreditCardRequired}
              />
            </CardPicker>
          ))}
        </StyledChooseTrialContainer>
      )}
      <MainButton
        title={t`Continue`}
        onClick={handleCheckoutSession}
        width={200}
        Icon={() => isSubmitting && <Loader />}
        disabled={isSubmitting}
      />
      <StyledLinkGroup>
        <ClickToActionLink onClick={signOut}>
          <Trans>Log out</Trans>
        </ClickToActionLink>
        <span />
        <ClickToActionLink
          href={calendarBookingPageId ? AppPath.BookCall : CAL_LINK}
          target={calendarBookingPageId ? '_self' : '_blank'}
          rel={calendarBookingPageId ? '' : 'noreferrer'}
        >
          <Trans>Book a Call</Trans>
        </ClickToActionLink>
        <span />
        {currentPlanKey === BillingPlanKey.PRO ? (
          <ClickToActionLink href={planChangeLink(BillingPlanKey.ENTERPRISE)}>
            <Trans>Organization plan</Trans>
          </ClickToActionLink>
        ) : (
          <ClickToActionLink href={planChangeLink(BillingPlanKey.PRO)}>
            <Trans>Pro plan</Trans>
          </ClickToActionLink>
        )}
      </StyledLinkGroup>
    </>
  );
};
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/metadata-modules/front-component/front-component.service.ts (TypeScript)

Code before:
      throw new WorkspaceMigrationBuilderException(
        validateAndBuildResult,
        'Multiple validation errors occurred while destroying front component',
      );
    }

    return existingFlatFrontComponent;
  }

  async findByIdOrThrow(
    id: string,
    workspaceId: string,
  ): Promise<FrontComponentDTO> {
    const frontComponent = await this.findById(id, workspaceId);

    if (!isDefined(frontComponent)) {
      throw new FrontComponentException(
        'Front component not found',
        FrontComponentExceptionCode.FRONT_COMPONENT_NOT_FOUND,
      );
    }

    return frontComponent;
  }

  async getBuiltComponentStream({
    frontComponentId,
    workspaceId,
  }: {
    frontComponentId: string;
    workspaceId: string;
  }): Promise<Readable> {
    const frontComponent = await this.findByIdOrThrow(
      frontComponentId,
      workspaceId,
    );

    const application = await this.applicationService.findOneApplicationOrThrow(
      {
        id: frontComponent.applicationId,
        workspaceId,
      },
    );

    return this.fileStorageService.readFile_v2({
      workspaceId,
      applicationUniversalIdentifier: application.universalIdentifier,
      fileFolder: FileFolder.BuiltFrontComponent,
      resourcePath: frontComponent.builtComponentPath,
    });
  }

Review comment (suggestion):
Note, not related to this PR:
to me we should really separate:

1) core-modules/front-component.service.ts (low level fc logic)
2) metadata-modules/front-component-api.service (high level service to serve the API)

like we have for logic function
IMO metadata-modules should become our highl level api service layer and everything else should be moved to core-modules

Code after:
      throw new WorkspaceMigrationBuilderException(
        validateAndBuildResult,
        'Multiple validation errors occurred while destroying front component',
      );
    }

    return existingFlatFrontComponent;
  }

  async findByIdOrThrow(
    id: string,
    workspaceId: string,
  ): Promise<FrontComponentDTO> {
    const frontComponent = await this.findById(id, workspaceId);

    if (!isDefined(frontComponent)) {
      throw new FrontComponentException(
        'Front component not found',
        FrontComponentExceptionCode.FRONT_COMPONENT_NOT_FOUND,
      );
    }

    return frontComponent;
  }

  async getBuiltComponentStream({
    frontComponentId,
    workspaceId,
  }: {
    frontComponentId: string;
    workspaceId: string;
  }): Promise<Readable> {
    const frontComponent = await this.findByIdOrThrow(
      frontComponentId,
      workspaceId,
    );

    const application = await this.applicationService.findOneApplicationOrThrow(
      {
        id: frontComponent.applicationId,
        workspaceId,
      },
    );

    return this.fileStorageService.readFile({
      workspaceId,
      applicationUniversalIdentifier: application.universalIdentifier,
      fileFolder: FileFolder.BuiltFrontComponent,
      resourcePath: frontComponent.builtComponentPath,
    });
  }
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/action-menu/actions/components/ActionOpenSidePanelPage.tsx (TypeScript)

Code before:
  pageIcon,
  onClick,
  shouldResetSearchState = false,
}: {
  page: CommandMenuPages;
  pageTitle: MessageDescriptor;
  pageIcon: IconComponent;
  onClick?: () => void;
  shouldResetSearchState?: boolean;
}) => {
  const actionConfig = useContext(ActionConfigContext);

  const { navigateCommandMenu } = useNavigateCommandMenu();
  const { openAskAIPage } = useOpenAskAIPageInCommandMenu();

  const setCommandMenuSearchState = useSetRecoilState(commandMenuSearchState);

  if (!actionConfig) {
    return null;
  }

  const handleClick = () => {
    onClick?.();

    if (page === CommandMenuPages.AskAI) {
      openAskAIPage({ resetNavigationStack: false });
    } else {
      navigateCommandMenu({
        page,
        pageTitle: t(pageTitle),
        pageIcon,
      });
    }

    if (shouldResetSearchState) {
      setCommandMenuSearchState('');
    }
  };

  return <ActionDisplay onClick={handleClick} />;
};


Review comment (bug):
This is also not very pretty (inserted a edge case within a component that's broadly used). Have you considered other options?

Code after:
  onClick,
  shouldResetSearchState = false,
}: {
  page: CommandMenuPages;
  pageTitle: MessageDescriptor;
  pageIcon: IconComponent;
  onClick?: () => void;
  shouldResetSearchState?: boolean;
}) => {
  const actionConfig = useContext(ActionConfigContext);

  const { navigateCommandMenu } = useNavigateCommandMenu();

  const setCommandMenuSearchState = useSetRecoilState(commandMenuSearchState);

  if (!actionConfig) {
    return null;
  }

  const handleClick = () => {
    onClick?.();

    navigateCommandMenu({
      page,
      pageTitle: t(pageTitle),
      pageIcon,
    });

    if (shouldResetSearchState) {
      setCommandMenuSearchState('');
    }
  };

  return <ActionDisplay onClick={handleClick} />;
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/pages/settings/ai/constants/SettingsAiModelProviders.ts (TypeScript)

Code before:
import {
  IconAnthropic,
  IconBrandOpenai,
  IconBrandX,
  IconRobot,
  type IconComponent,
} from 'twenty-ui/display';

export const PROVIDER_CONFIG: Record<
  string,
  { label: string; Icon: IconComponent }
> = {
  OPENAI: { label: 'OpenAI', Icon: IconBrandOpenai },
  ANTHROPIC: { label: 'Anthropic', Icon: IconAnthropic },
  XAI: { label: 'xAI', Icon: IconBrandX },
  GROQ: { label: 'Groq', Icon: IconRobot },
  OPENAI_COMPATIBLE: { label: 'OpenAI Compatible', Icon: IconBrandOpenai },
  NONE: { label: '', Icon: IconRobot },
};


Review comment (suggestion):
I feel like in a followup PR we should differentiate the "Model by" (OpenAI, Anthropic) from "served via" (Azure, Bedrock, OpenAI, Anthropic etc). Let's adapt the data model for this and show this in cards that appear on hover. In the code it could be the model publisher vs model inference provider.

Code after:
import {
  IconBrandAnthropic,
  IconBrandGoogle,
  IconBrandGroq,
  IconBrandMistral,
  IconBrandOpenai,
  IconBrandXai,
  IconRobot,
  type IconComponent,
} from 'twenty-ui/display';

export const PROVIDER_CONFIG: Record<
  string,
  { label: string; Icon: IconComponent }
> = {
  OPENAI: { label: 'OpenAI', Icon: IconBrandOpenai },
  ANTHROPIC: { label: 'Anthropic', Icon: IconBrandAnthropic },
  XAI: { label: 'xAI', Icon: IconBrandXai },
  GOOGLE: { label: 'Google', Icon: IconBrandGoogle },
  GROQ: { label: 'Groq', Icon: IconBrandGroq },
  MISTRAL: { label: 'Mistral', Icon: IconBrandMistral },
  NONE: { label: '', Icon: IconRobot },
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/utils/convertPageLayoutDraftToUpdateInput.ts (TypeScript)

Code before:
export const convertPageLayoutDraftToUpdateInput = (
  pageLayoutDraft: DraftPageLayout,
): UpdatePageLayoutWithTabsInput => {
  return {
    name: pageLayoutDraft.name,
    type: pageLayoutDraft.type,
    objectMetadataId: pageLayoutDraft.objectMetadataId ?? null,
    tabs: pageLayoutDraft.tabs.map((tab) => ({
      id: tab.id,
      title: tab.title,
      position: tab.position,
      widgets: tab.widgets
        .filter((widget) => !isDynamicRelationWidget(widget))
        .map((widget) => ({
          id: widget.id,
          pageLayoutTabId: widget.pageLayoutTabId,
          title: widget.title,
          type: widget.type,
          objectMetadataId: widget.objectMetadataId ?? null,
          gridPosition: {
            row: widget.gridPosition.row,
            column: widget.gridPosition.column,
            rowSpan: widget.gridPosition.rowSpan,
            columnSpan: widget.gridPosition.columnSpan,
          },
          position: {
            layoutMode: PageLayoutTabLayoutMode.GRID,
            row: widget.gridPosition.row,
            column: widget.gridPosition.column,
            rowSpan: widget.gridPosition.rowSpan,
            columnSpan: widget.gridPosition.columnSpan,
          },
          configuration: widget.configuration ?? null,
        })),
    })),
  };
};


Review comment (suggestion):
This function is called when updating dashboards and record page layouts. I think we should add a guard not to send layoutMode=GRID for record page layouts.

Code after:
export const convertPageLayoutDraftToUpdateInput = (
  pageLayoutDraft: DraftPageLayout,
): UpdatePageLayoutWithTabsInput => {
  return {
    name: pageLayoutDraft.name,
    type: pageLayoutDraft.type,
    objectMetadataId: pageLayoutDraft.objectMetadataId ?? null,
    tabs: pageLayoutDraft.tabs.map((tab) => ({
      id: tab.id,
      title: tab.title,
      position: tab.position,
      widgets: tab.widgets
        .filter((widget) => !isDynamicRelationWidget(widget))
        .map((widget) => ({
          id: widget.id,
          pageLayoutTabId: widget.pageLayoutTabId,
          title: widget.title,
          type: widget.type,
          objectMetadataId: widget.objectMetadataId ?? null,
          gridPosition: {
            row: widget.gridPosition.row,
            column: widget.gridPosition.column,
            rowSpan: widget.gridPosition.rowSpan,
            columnSpan: widget.gridPosition.columnSpan,
          },
          position: {
            layoutMode:
              widget.position?.layoutMode ?? PageLayoutTabLayoutMode.GRID,
            row: widget.gridPosition.row,
            column: widget.gridPosition.column,
            rowSpan: widget.gridPosition.rowSpan,
            columnSpan: widget.gridPosition.columnSpan,
          },
          configuration: widget.configuration ?? null,
        })),
    })),
  };
};