ronantakizawa/codereview-bench huggingface dataset:
Repository: twentyhq/twenty
File: packages/twenty-server/src/database/commands/upgrade-version-command/1-16/1-16-backfill-updated-by-field.command.ts (TypeScript)

Code before:
      });
    }

    if (fieldsToCreate.length === 0) {
      this.logger.log(
        `All objects already have updatedBy field for workspace ${workspaceId}`,
      );

      return;
    }

    this.logger.log(
      `Found ${fieldsToCreate.length} objects that need updatedBy field`,
    );

    if (options.dryRun) {
      this.logger.log(
        `[DRY RUN] Would create updatedBy field for ${fieldsToCreate.length} objects`,
      );

      return;
    }

    for (const { objectMetadataId, standardId, label } of fieldsToCreate) {
      try {
        await this.fieldMetadataService.createManyFields({
          createFieldInputs: [
            {
              objectMetadataId,
              name: 'updatedBy',
              type: FieldMetadataType.ACTOR,
              label: 'Updated by',
              description: 'The user who last updated the record',
              icon: 'IconUserCircle',
              isNullable: true,
              isUIReadOnly: true,
              isCustom: false,
              isSystem: false,
              isActive: true,
              standardId,
            },
          ],
          workspaceId,
        });

        this.logger.log(
          `Successfully created updatedBy field for object ${label} (${objectMetadataId})`,
        );
      } catch (error) {
        this.logger.error(
          `Failed to create updatedBy field for object ${label} (${objectMetadataId}): ${error.message}`,

Review comment (performance):
***Suggestion:*** We could avoid making n operation but only 1 for all fields to be created instead of for n objects. This implemen will be very costy in production involving n cache invalidation

Code after:
        objectMetadataId: flatObjectMetadata.id,
        name: 'updatedBy',
        type: FieldMetadataType.ACTOR,
        label: 'Updated by',
        description: 'The user who last updated the record',
        icon: 'IconUserCircle',
        isNullable: true,
        isUIReadOnly: true,
        isCustom: false,
        isSystem: false,
        isActive: true,
        standardId: updatedByStandardId,
        universalIdentifier,
        applicationId,
      });
    }

    if (createFieldInputs.length === 0) {
      this.logger.log(
        `All objects already have updatedBy field for workspace ${workspaceId}`,
      );

      return;
    }

    this.logger.log(
      `Found ${createFieldInputs.length} objects that need updatedBy field`,
    );

    if (options.dryRun) {
      this.logger.log(
        `[DRY RUN] Would create updatedBy field for ${createFieldInputs.length} objects`,
      );

      return;
    }

    try {
      await this.fieldMetadataService.createManyFields({
        createFieldInputs,
        workspaceId,
        isSystemBuild: true,
      });
    } catch (error) {
      this.logger.error(
        `Failed to create many fields \n ${JSON.stringify(error, null, 2)}`,
      );
      throw error;
    }

    this.logger.log(
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/hooks/useIsCurrentWidgetLastOfTab.ts (TypeScript)

Code before:
  );

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

  const tab = currentPageLayout.tabs.find((tab) =>
    tab.widgets.some((widget) => widget.id === widgetId),
  );

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

  const visibleWidgets = isPageLayoutInEditMode
    ? tab.widgets
    : filterVisibleWidgets({
        widgets: tab.widgets,
        context: buildWidgetVisibilityContext({ isMobile, isInRightDrawer }),
      });

  if (visibleWidgets.length === 0) {
    return false;
  }

  const lastWidget = visibleWidgets[visibleWidgets.length - 1];

  return lastWidget.id === widgetId;
};


Review comment (suggestion):
```suggestion
  const lastWidget = visibleWidgets.at(-1);
```

Code after:
  );

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

  const tab = currentPageLayout.tabs.find((tab) =>
    tab.widgets.some((widget) => widget.id === widgetId),
  );

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

  const visibleWidgets = isPageLayoutInEditMode
    ? tab.widgets
    : filterVisibleWidgets({
        widgets: tab.widgets,
        context: buildWidgetVisibilityContext({ isMobile, isInRightDrawer }),
      });

  if (visibleWidgets.length === 0) {
    return false;
  }

  const lastWidget = visibleWidgets.at(-1);

  return lastWidget?.id === widgetId;
};
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/api/mcp/services/mcp-protocol.service.ts (TypeScript)

Code before:

    if (!userWorkspaceId) {
      throw new HttpException(
        'User workspace ID missing',
        HttpStatus.FORBIDDEN,
      );
    }

    const roleId = await this.userRoleService.getRoleIdForUserWorkspace({
      workspaceId,
      userWorkspaceId,
    });

    if (!roleId) {
      throw new HttpException('Role ID missing', HttpStatus.FORBIDDEN);
    }

    return roleId;
  }

  private buildAuthContext(
    workspace: WorkspaceEntity,
    userWorkspaceId?: string,
    apiKey?: ApiKeyEntity,
  ): WorkspaceAuthContext {
    if (isDefined(apiKey)) {
      return {
        user: null,
        apiKey,
        application: null,
        workspace,
        workspaceMemberId: undefined,
        userWorkspaceId: undefined,
      } as WorkspaceAuthContext;
    }

    if (userWorkspaceId) {
      return {
        user: null,
        apiKey: null,
        application: null,
        workspace,
        workspaceMemberId: undefined,
        userWorkspaceId,
      } as WorkspaceAuthContext;
    }

    return {
      user: null,
      apiKey: null,
      application: null,

Review comment (security):
I feel like we could simply return
```typescript
  private buildAuthContext(
    workspace: WorkspaceEntity,
    userWorkspaceId?: string,
    apiKey?: ApiKeyEntity,
  ): WorkspaceAuthContext {
    return {
      user: null,
      apiKey: apiKey ?? null,
      application: null,
      workspace,
      workspaceMemberId: undefined,
      userWorkspaceId: userWorkspaceId ?? undefined,
    };
  }
```
the conditions don't bring much or am I missing something? Do you think we could have both api and userWorkspace and you want to make sure your object only contains apiKey if it's there?

Code after:

    if (!roleId) {
      throw new HttpException('Role ID missing', HttpStatus.FORBIDDEN);
    }

    return roleId;
  }

  private buildAuthContext(
    workspace: WorkspaceEntity,
    userWorkspaceId?: string,
    apiKey?: ApiKeyEntity,
  ): WorkspaceAuthContext {
    return {
      user: null,
      apiKey: apiKey ?? null,
      application: null,
      workspace,
      workspaceMemberId: undefined,
      userWorkspaceId: userWorkspaceId ?? undefined,
    } as WorkspaceAuthContext;
  }

  async handleMCPCoreQuery(
    { id, method, params }: JsonRpc,
    {
      workspace,
      userWorkspaceId,
      apiKey,
    }: {
      workspace: WorkspaceEntity;
      userWorkspaceId?: string;
      apiKey: ApiKeyEntity | undefined;
    },
  ): Promise<Record<string, unknown>> {
    try {
      await this.checkAiEnabled(workspace.id);

      if (method === 'initialize') {
        return this.handleInitialize(id);
      }

      if (method === 'ping') {
        return wrapJsonRpcResponse(
          id,
          {
            result: {},
          },
          true,
        );
      }
Repository: twentyhq/twenty
File: packages/twenty-server/src/engine/core-modules/application/application.service.ts (TypeScript)

Code before:
    workspaceId: string;
  }) {
    return this.applicationRepository.findOne({
      where: {
        universalIdentifier,
        workspaceId,
      },
    });
  }

  async findTwentyStandardApplicationOrThrow(workspaceId: string): Promise<{
    application: ApplicationEntity;
    workspace: WorkspaceEntity;
  }> {
    const workspace = await this.workspaceRepository.findOne({
      where: { id: workspaceId },
    });

    if (!isDefined(workspace)) {
      throw new ApplicationException(
        `Could not find workspace ${workspaceId}`,
        ApplicationExceptionCode.APPLICATION_NOT_FOUND,
      );
    }

    const application = await this.applicationRepository.findOne({
      where: {
        universalIdentifier: TWENTY_STANDARD_APPLICATION.universalIdentifier,
        workspaceId,
      },
    });

    if (!isDefined(application)) {
      throw new ApplicationException(
        `Could not find Twenty Standard Application for workspace ${workspaceId}`,
        ApplicationExceptionCode.APPLICATION_NOT_FOUND,
      );
    }

    return { application, workspace };
  }

  async createTwentyStandardApplication(
    {
      workspaceId,
      skipCacheInvalidation = false,
    }: {
      workspaceId: string;
      skipCacheInvalidation?: boolean;
    },
    queryRunner?: QueryRunner,

Review comment (performance):
We prefer to use the cache when it's available. (this will go through the local (pod) cache instead of querying the DB). Tradeoff here is you'll have to filter yourself and not use pg filter (which is most likely more efficient) but we are talking about applications here so probably 2-3 max. Up to you!

Code after:
    workspaceId: string;
  }) {
    return this.applicationRepository.findOne({
      where: {
        universalIdentifier,
        workspaceId,
      },
    });
  }

  async findTwentyStandardApplicationOrThrow(workspaceId: string): Promise<{
    application: ApplicationEntity;
    workspace: WorkspaceEntity;
  }> {
    const workspace = await this.workspaceRepository.findOne({
      where: { id: workspaceId },
    });

    if (!isDefined(workspace)) {
      throw new ApplicationException(
        `Could not find workspace ${workspaceId}`,
        ApplicationExceptionCode.APPLICATION_NOT_FOUND,
      );
    }

    const { twentyStandardFlatApplication } =
      await this.findWorkspaceTwentyStandardAndCustomApplicationOrThrow({
        workspace,
      });

    return {
      application: twentyStandardFlatApplication as ApplicationEntity,
      workspace,
    };
  }

  async createTwentyStandardApplication(
    {
      workspaceId,
      skipCacheInvalidation = false,
    }: {
      workspaceId: string;
      skipCacheInvalidation?: boolean;
    },
    queryRunner?: QueryRunner,
  ) {
    const twentyStandardApplication = await this.create(
      {
        ...TWENTY_STANDARD_APPLICATION,
        serverlessFunctionLayerId: null,
        workspaceId,
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx (TypeScript)

Code before:
      formConfig.reset(undefined, { keepValues: true });
    }

    navigate(SettingsPath.ObjectDetail, {
      objectNamePlural: objectNamePluralForRedirection,
    });

    const previousNamePlural = objectMetadataItem.namePlural;
    const updatedNamePlural = updatedObject?.data?.updateOneObject.namePlural;

    if (!updatedNamePlural) return;

    setNavigationMemorizedUrl((prev) => {
      if (!prev) return prev;

      const objectRouteRegex = new RegExp(
        `^/objects/${previousNamePlural}(/|\\?|$)`,
      );

      if (!objectRouteRegex.test(prev)) {
        return prev;
      }

      return prev.replace(
        new RegExp(`^/objects/${previousNamePlural}`),
        `/objects/${updatedNamePlural}`,
      );
    });
  };

  const updateObjectMetadata = async (
    formValues: SettingsDataModelObjectAboutFormValues,
  ) => {
    const updatePayload = { ...formValues };

    if (!objectMetadataItem.isCustom) {
      const {
        nameSingular: _nameSingular,
        namePlural: _namePlural,
        isLabelSyncedWithName: _isLabelSyncedWithName,
        ...payloadWithoutNames
      } = updatePayload;

      return await updateOneObjectMetadataItem({
        idToUpdate: objectMetadataItem.id,
        updatePayload: payloadWithoutNames,
      });
    }

    return await updateOneObjectMetadataItem({
      idToUpdate: objectMetadataItem.id,

Review comment (refactor):
A new RegExp object is created for the replacement operation even though a similar regex was already created for testing on line 112. Consider reusing the regex pattern or extracting it to a variable to improve maintainability and avoid duplicating the regex pattern definition.

Code after:
      });
    } else {
      formConfig.reset(undefined, { keepValues: true });
    }

    navigate(SettingsPath.ObjectDetail, {
      objectNamePlural: objectNamePluralForRedirection,
    });

    const updatedObjectNamePlural =
      updatedObject?.data?.updateOneObject.namePlural;

    if (!isDefined(updatedObjectNamePlural)) {
      return;
    }

    setNavigationMemorizedUrl((previousNavigationMemorizedUrl) =>
      computeUpdatedNavigationMemorizedUrlAfterObjectNamePluralChange(
        previousNavigationMemorizedUrl,
        objectMetadataItem.namePlural,
        updatedObjectNamePlural,
      ),
    );
  };

  const updateObjectMetadata = async (
    formValues: SettingsDataModelObjectAboutFormValues,
  ) => {
    const updatePayload = { ...formValues };

    if (!objectMetadataItem.isCustom) {
      const {
        nameSingular: _nameSingular,
        namePlural: _namePlural,
        isLabelSyncedWithName: _isLabelSyncedWithName,
        ...payloadWithoutNames
      } = updatePayload;

      return await updateOneObjectMetadataItem({
        idToUpdate: objectMetadataItem.id,
        updatePayload: payloadWithoutNames,
      });
    }

    return await updateOneObjectMetadataItem({
      idToUpdate: objectMetadataItem.id,
      updatePayload,
    });
  };

  return (
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/activities/inline-cell/hooks/useUpdateActivityTargetFromCell.ts (TypeScript)

Code before:
          activityTargetsAfterUpdate = [
            ...activityTargetWithTargetRecords.map((activityTarget) => {
              return activityTarget.activityTarget;
            }),
            activityTarget as NoteTarget | TaskTarget,
          ];

          await createOneActivityTarget({
            ...activityTarget,
            [targetObjectName]: undefined,
            [pickedObjectMetadataItem.nameSingular]: undefined,
            [`${pickedObjectMetadataItem.nameSingular}Id`]: morphItem.recordId,
          } as Partial<NoteTarget | TaskTarget>);
        }

        setActivityFromStore((currentActivity) => {
          if (!isDefined(currentActivity)) {
            return null;
          }

          return {
            ...currentActivity,
            [`${targetObjectName}Targets`]: activityTargetsAfterUpdate,
          };
        });
      },
    [
      activityId,
      activityObjectNameSingular,
      createOneActivityTarget,
      deleteOneActivityTarget,
      setActivityFromStore,
    ],
  );

  return { updateActivityTargetFromCell };
};


Review comment (suggestion):
***remark:*** out of scope we might wanna expose `computeMorphOrRelationFieldJoinColumnName` in twenty-shared

Code after:
                  id: v4(),
                  createdAt: new Date().toISOString(),
                  updatedAt: new Date().toISOString(),
                  __typename: 'NoteTarget',
                  noteId: activityId,
                  note: {
                    id: activityId,
                    __typename: 'Note',
                    createdAt: new Date().toISOString(),
                    updatedAt: new Date().toISOString(),
                  },
                  [targetFieldName]: targetRecord,
                };

          activityTargetsAfterUpdate = [
            ...activityTargetWithTargetRecords.map((activityTarget) => {
              return activityTarget.activityTarget;
            }),
            activityTarget as NoteTarget | TaskTarget,
          ];

          await createOneActivityTarget({
            ...activityTarget,
            [targetObjectName]: undefined,
            [targetFieldName]: undefined,
            [`${targetFieldName}Id`]: morphItem.recordId,
          } as Partial<NoteTarget | TaskTarget>);
        }

        setActivityFromStore((currentActivity) => {
          if (!isDefined(currentActivity)) {
            return null;
          }

          return {
            ...currentActivity,
            [`${targetObjectName}Targets`]: activityTargetsAfterUpdate,
          };
        });
      },
    [
      activityId,
      activityObjectNameSingular,
      createOneActivityTarget,
      deleteOneActivityTarget,
      setActivityFromStore,
    ],
  );

  return { updateActivityTargetFromCell };
};
Repository: twentyhq/twenty
File: packages/twenty-server/src/modules/connected-account/refresh-tokens-manager/drivers/microsoft/services/microsoft-api-refresh-tokens.service.ts (TypeScript)

Code before:
        throw new ConnectedAccountRefreshAccessTokenException(
          'No response received from Microsoft token endpoint',
          ConnectedAccountRefreshAccessTokenExceptionCode.INVALID_REFRESH_TOKEN,
        );
      }

      const freshRefreshToken = this.extractMicrosoftRefreshTokenFromCache();

      return {
        accessToken: response.accessToken,
        refreshToken: freshRefreshToken,
      };
    } catch (error) {
      if (error instanceof ConnectedAccountRefreshAccessTokenException) {
        throw error;
      }

      throw parseMsalError(error);
    }
  }

  /**
   * Extracts the refresh token from the MSAL token cache.
   * @see https://github.com/duolingo/metasearch/blob/3d782bba8c0068461acb442d89e7d555df5d0025/src/oauth.microsoft.ts#L42-L44
   */
  private extractMicrosoftRefreshTokenFromCache(): string {
    const tokenCache = JSON.parse(this.msalClient.getTokenCache().serialize());
    const refreshTokenKey = Object.keys(tokenCache.RefreshToken)[0];

    return tokenCache.RefreshToken[refreshTokenKey].secret;
  }
}


Review comment (performance):
While this is not ideal, it should do for now as the serialized token cache is an complex object, and it's not backwards compatible with our current approach which only stores the raw `refresh_token`, this would involve creating migration strategies and the other alternative, our old approach (raw HTTP request) wasn't that good either

Code after:
          ConnectedAccountRefreshAccessTokenExceptionCode.INVALID_REFRESH_TOKEN,
        );
      }

      return {
        accessToken: response.accessToken,
        refreshToken: this.extractRefreshTokenFromCache(msalClient),
      };
    } catch (error) {
      if (error instanceof ConnectedAccountRefreshAccessTokenException) {
        throw error;
      }

      throw parseMsalError(error);
    }
  }

  private extractRefreshTokenFromCache(
    msalClient: ConfidentialClientApplication,
  ): string {
    const tokenCache = JSON.parse(msalClient.getTokenCache().serialize());
    const refreshTokenKey = Object.keys(tokenCache.RefreshToken)[0];

    return tokenCache.RefreshToken[refreshTokenKey].secret;
  }
}
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/object-record/record-calendar/components/RecordCalendarAddNew.tsx (TypeScript)

Code before:
  );

  const calendarFieldMetadataItem = objectMetadataItem.fields.find(
    (field) => field.id === recordIndexCalendarFieldMetadataId,
  );

  const isCalendarFieldReadOnly = calendarFieldMetadataItem
    ? isFieldMetadataReadOnlyByPermissions({
        objectPermissions,
        fieldMetadataId: calendarFieldMetadataItem.id,
      })
    : false;

  if (
    hasAnySoftDeleteFilterOnView ||
    !hasObjectUpdatePermissions ||
    !calendarFieldMetadataItem ||
    isCalendarFieldReadOnly
  ) {
    return null;
  }

  return (
    <StyledButton
      onClick={async () => {
        closeCommandMenu();
        await createNewIndexRecord({
          [calendarFieldMetadataItem.name]: cardDate
            .toZonedDateTime(userTimezone)
            .toInstant()
            .toString(),
        });
      }}
      variant="tertiary"
      Icon={() => <IconPlus size={theme.icon.size.sm} />}
    />
  );
};


Review comment (suggestion):
I feel:
- this should actually be part of createNewIndexRecord
- and we should call it only if the view is set to navigate to a new record page

Code after:
    (field) => field.id === recordIndexCalendarFieldMetadataId,
  );

  const isCalendarFieldReadOnly = calendarFieldMetadataItem
    ? isFieldMetadataReadOnlyByPermissions({
        objectPermissions,
        fieldMetadataId: calendarFieldMetadataItem.id,
      })
    : false;

  if (
    hasAnySoftDeleteFilterOnView ||
    !hasObjectUpdatePermissions ||
    !calendarFieldMetadataItem ||
    isCalendarFieldReadOnly
  ) {
    return null;
  }

  return (
    <StyledButton
      onClick={async () => {
        await createNewIndexRecord({
          [calendarFieldMetadataItem.name]: cardDate
            .toZonedDateTime(userTimezone)
            .toInstant()
            .toString(),
        });
      }}
      variant="tertiary"
      Icon={() => <IconPlus size={theme.icon.size.sm} />}
    />
  );
};
Repository: twentyhq/twenty
File: packages/twenty-front/src/modules/page-layout/widgets/graph/utils/compareDimensionValues.ts (TypeScript)

Code before:
};

export const compareDimensionValues = ({
  rawValueA,
  rawValueB,
  formattedValueA,
  formattedValueB,
  direction,
  fieldType,
  subFieldName,
}: CompareDimensionValuesParams): number => {
  const applyDirection = (comparison: number) =>
    direction === 'ASC' ? comparison : -comparison;

  if (isDefined(fieldType)) {
    if (isFieldMetadataDateKind(fieldType)) {
      const dateA = parseDate(rawValueA, fieldType);
      const dateB = parseDate(rawValueB, fieldType);
      if (isDefined(dateA) && isDefined(dateB)) {
        return applyDirection(Temporal.PlainDate.compare(dateA, dateB));
      }
    }

    if (fieldType === FieldMetadataType.CURRENCY) {
      if (subFieldName === 'amountMicros') {
        if (isDefined(rawValueA) && isDefined(rawValueB)) {
          return applyDirection(Number(rawValueA) - Number(rawValueB));
        }
      }
      return applyDirection(formattedValueA.localeCompare(formattedValueB));
    }

    if (isFieldMetadataNumericKind(fieldType)) {
      if (isDefined(rawValueA) && isDefined(rawValueB)) {
        return applyDirection(Number(rawValueA) - Number(rawValueB));
      }
    }
  }

  return applyDirection(formattedValueA.localeCompare(formattedValueB));
};


Review comment (suggestion):
feels like subFieldName could be typed if we are comparing it like this?
I see its typed as a string

Code after:
  subFieldName?: CompositeFieldSubFieldName;
};

export const compareDimensionValues = ({
  rawValueA,
  rawValueB,
  formattedValueA,
  formattedValueB,
  direction,
  fieldType,
  subFieldName,
}: CompareDimensionValuesParams): number => {
  const applyDirection = (comparison: number) =>
    direction === 'ASC' ? comparison : -comparison;

  if (isDefined(fieldType)) {
    if (isFieldMetadataDateKind(fieldType)) {
      const dateA = parseDate(rawValueA, fieldType);
      const dateB = parseDate(rawValueB, fieldType);
      if (isDefined(dateA) && isDefined(dateB)) {
        return applyDirection(Temporal.PlainDate.compare(dateA, dateB));
      }
    }

    if (fieldType === FieldMetadataType.CURRENCY) {
      if (subFieldName === 'amountMicros') {
        if (isDefined(rawValueA) && isDefined(rawValueB)) {
          return applyDirection(Number(rawValueA) - Number(rawValueB));
        }
      }
      return applyDirection(formattedValueA.localeCompare(formattedValueB));
    }

    if (isFieldMetadataNumericKind(fieldType)) {
      if (isDefined(rawValueA) && isDefined(rawValueB)) {
        return applyDirection(Number(rawValueA) - Number(rawValueB));
      }
    }
  }

  return applyDirection(formattedValueA.localeCompare(formattedValueB));
};