ronantakizawa/codereview-bench huggingface dataset:
Repository: strapi/strapi
File: packages/core/admin/admin/src/components/UnstableGuidedTour/Step.tsx (TypeScript)

Code before:
  children: React.ReactNode;
  showStepCount?: boolean;
  showSkip?: boolean;
};

type WithActionsProps = {
  children?: undefined;
  showStepCount?: boolean;
  showSkip?: boolean;
};

type StepProps = WithChildren | WithIntl;
type ActionsProps = WithActionsChildren | WithActionsProps;

type Step = {
  Root: React.ForwardRefExoticComponent<
    React.ComponentProps<typeof Popover.Content> & { withArrow?: boolean }
  >;
  Title: (props: StepProps) => React.ReactNode;
  Content: (props: StepProps) => React.ReactNode;
  Actions: (props: ActionsProps & { to?: string }) => React.ReactNode;
};

const GUIDED_TOUR_TRACKING_EVENTS = {
  contentTypeBuilder: {
    create: 'didCreateGuidedTourCollectionType' as EventWithoutProperties['name'],
  },
  contentManager: {
    create: 'didCreateGuidedTourEntry' as EventWithoutProperties['name'],
  },
  apiTokens: {
    create: 'didGenerateGuidedTourApiTokens' as EventWithoutProperties['name'],
  },
  strapiCloud: {
    create: '' as EventWithoutProperties['name'],
  },
};

const ActionsContainer = styled(Flex)`
  border-top: ${({ theme }) => `1px solid ${theme.colors.neutral150}`};
`;

/**
 * TODO:
 * We should probably move all arrow styles + svg to the DS
 */
const PopoverArrow = styled(Popover.Arrow)`
  fill: ${({ theme }) => theme.colors.neutral0};
  transform: translateY(-16px) rotate(-90deg);
`;


Review comment (suggestion):
I would add `start: 'didClickGuidedTourHomepageContentManager'` and then use the constant in the overview so all tracking is centralized

Code after:
type WithIntl = {
  children?: undefined;
  id: MessageDescriptor['id'];
  defaultMessage: MessageDescriptor['defaultMessage'];
  withArrow?: boolean;
};

type WithActionsChildren = {
  children: React.ReactNode;
  showStepCount?: boolean;
  showSkip?: boolean;
};

type WithActionsProps = {
  children?: undefined;
  showStepCount?: boolean;
  showSkip?: boolean;
};

type StepProps = WithChildren | WithIntl;
type ActionsProps = WithActionsChildren | WithActionsProps;

type Step = {
  Root: React.ForwardRefExoticComponent<
    React.ComponentProps<typeof Popover.Content> & { withArrow?: boolean }
  >;
  Title: (props: StepProps) => React.ReactNode;
  Content: (props: StepProps) => React.ReactNode;
  Actions: (props: ActionsProps & { to?: string } & FlexProps) => React.ReactNode;
};

const ActionsContainer = styled(Flex)`
  border-top: ${({ theme }) => `1px solid ${theme.colors.neutral150}`};
`;

/**
 * TODO:
 * We should probably move all arrow styles + svg to the DS
 */
const PopoverArrow = styled(Popover.Arrow)`
  fill: ${({ theme }) => theme.colors.neutral0};
  transform: translateY(-16px) rotate(-90deg);
`;

export const StepCount = ({ tourName }: { tourName: ValidTourName }) => {
  const state = unstableUseGuidedTour('GuidedTourPopover', (s) => s.state);
  const currentStep = state.tours[tourName].currentStep + 1;
  // TODO: Currently all tours do not count their last step, but we should find a way to make this more smart
  const displayedLength = state.tours[tourName].length - 1;

  return (
Repository: strapi/strapi
File: packages/core/core/src/middlewares/cors.ts (TypeScript)

Code before:
      'The strapi::cors middleware no longer supports the `enabled` option. Using it' +
        ' to conditionally enable CORS might cause an insecure default. To disable strapi::cors, remove it from' +
        ' the exported array in config/middleware.js'
    );
  }

  return koaCors({
    async origin(ctx) {
      if (!ctx.get('Origin')) {
        return '*';
      }

      let originList: string | string[];

      if (typeof origin === 'function') {
        originList = await origin(ctx);
      } else {
        originList = origin;
      }

      if (Array.isArray(originList)) {
        return originList.includes(ctx.get('Origin')) ? ctx.get('Origin') : '';
      }

      const parsedOrigin = originList.split(',').map((origin) => origin.trim());
      if (parsedOrigin.length > 1) {
        return parsedOrigin.includes(ctx.get('Origin')) ? ctx.get('Origin') : '';
      }

      return originList;
    },
    exposeHeaders: expose,
    maxAge,
    credentials,
    allowMethods: methods,
    allowHeaders: headers,
    keepHeadersOnError,
  });
};


Review comment (suggestion):
Just as a comment for posterity, this seems to be standard practice for undefined origins in settings (I think koa does the same thing), and it seemed better to err on the side of not causing breaking changes.

Code after:
  requestOrigin: string | undefined,
  configuredOrigin:
    | string
    | string[]
    | ((ctx: any) => string | string[] | Promise<string | string[]>),
  ctx?: any
): Promise<string> => {
  if (!requestOrigin) {
    return '*';
  }

  let originList: string | string[];

  if (typeof configuredOrigin === 'function') {
    originList = await configuredOrigin(ctx);
  } else {
    originList = configuredOrigin;
  }

  // Normalize originList into an array
  let normalizedOrigins: string[];
  if (Array.isArray(originList)) {
    normalizedOrigins = originList;
  } else if (originList === undefined || originList === null) {
    // Handle undefined/null - treat as wildcard
    normalizedOrigins = ['*'];
  } else {
    // Handle comma-separated string of origins
    normalizedOrigins = originList.split(',').map((origin) => origin.trim());
  }

  // Check if wildcard is in the normalized origins
  if (normalizedOrigins.includes('*')) {
    return requestOrigin;
  }

  // Check if request origin is in the normalized origins
  return normalizedOrigins.includes(requestOrigin) ? requestOrigin : '';
};

export const cors: Core.MiddlewareFactory<Config> = (config) => {
  const { origin, expose, maxAge, credentials, methods, headers, keepHeadersOnError } = {
    ...defaults,
    ...config,
  };

  if (config.enabled !== undefined) {
    strapi.log.warn(
      'The strapi::cors middleware no longer supports the `enabled` option. Using it' +
        ' to conditionally enable CORS might cause an insecure default. To disable strapi::cors, remove it from' +
        ' the exported array in config/middleware.js'
Repository: strapi/strapi
File: packages/cli/create-strapi-app/src/create-strapi.ts (TypeScript)

Code before:

    try {
      await trackUsage({ event: 'willStartServer', scope });

      await execa(packageManager, ['run', 'develop'], {
        stdio: 'inherit',
        cwd: rootPath,
        env: {
          FORCE_COLOR: '1',
        },
      });
    } catch (error) {
      if (typeof error === 'string' || error instanceof Error) {
        await trackUsage({
          event: 'didNotStartServer',
          scope,
          error,
        });
      }

      logger.fatal('Failed to start your Strapi application');
    }
  }

  if (isABTestEnabled) {
    await trackUsage({ event: 'didEnabledABTest', scope });
  }
}

async function runInstall({ rootPath, packageManager }: Scope) {
  // include same cwd and env to ensure version check returns same version we use below
  const { envArgs, cmdArgs } = await getInstallArgs(packageManager, {
    cwd: rootPath,
    env: {
      ...process.env,
      NODE_ENV: 'development',
    },
  });

  const options: execa.Options = {
    cwd: rootPath,
    stdio: 'inherit',
    env: {
      ...process.env,
      ...envArgs,
      NODE_ENV: 'development',
    },
  };

  const proc = execa(packageManager, cmdArgs, options);


Review comment (suggestion):
I would change the event name here to `didEnableABTest` just makes more sense grammatically
but overall the code looks good

Code after:

    try {
      await trackUsage({ event: 'willStartServer', scope });

      await execa(packageManager, ['run', 'develop'], {
        stdio: 'inherit',
        cwd: rootPath,
        env: {
          FORCE_COLOR: '1',
        },
      });
    } catch (error) {
      if (typeof error === 'string' || error instanceof Error) {
        await trackUsage({
          event: 'didNotStartServer',
          scope,
          error,
        });
      }

      logger.fatal('Failed to start your Strapi application');
    }
  }

  if (isABTestEnabled) {
    await trackUsage({ event: 'didEnableABTest', scope });
  }
}

async function runInstall({ rootPath, packageManager }: Scope) {
  // include same cwd and env to ensure version check returns same version we use below
  const { envArgs, cmdArgs } = await getInstallArgs(packageManager, {
    cwd: rootPath,
    env: {
      ...process.env,
      NODE_ENV: 'development',
    },
  });

  const options: execa.Options = {
    cwd: rootPath,
    stdio: 'inherit',
    env: {
      ...process.env,
      ...envArgs,
      NODE_ENV: 'development',
    },
  };

  const proc = execa(packageManager, cmdArgs, options);
Repository: strapi/strapi
File: packages/core/core/src/services/session-manager.ts (TypeScript)

Code before:
      return { error: 'invalid_refresh_token' };
    }

    const payload: Omit<AccessTokenPayload, 'iat' | 'exp'> = {
      userId: validation.userId!,
      sessionId: validation.sessionId!,
      type: 'access',
    };

    const token = jwt.sign(payload, this.config.jwtSecret, {
      expiresIn: this.config.accessTokenLifespan,
    });

    return { token };
  }
}

const createDatabaseProvider = (db: Database, contentType: string): SessionProvider => {
  return new DatabaseSessionProvider(db, contentType);
};

const createSessionManager = ({ db, config }: { db: Database; config: SessionManagerConfig }) => {
  const provider = createDatabaseProvider(db, 'admin::session');
  return new SessionManager(provider, config);
};

export { createSessionManager, createDatabaseProvider };


Review comment (suggestion):

```suggestion
      algorithm: 'HS256',
      expiresIn: this.config.accessTokenLifespan,
```

Better if we explicitly specify this everywhere (and then easier to find/replace if we go with a different algorithm before release)

Code after:
            // If we cannot recover payload safely, skip cleanup
          }
        }
        return { isValid: false };
      }

      throw error;
    }
  }

  async generateAccessToken(refreshToken: string): Promise<{ token: string } | { error: string }> {
    const validation = await this.validateRefreshToken(refreshToken);

    if (!validation.isValid) {
      return { error: 'invalid_refresh_token' };
    }

    const payload: Omit<AccessTokenPayload, 'iat' | 'exp'> = {
      userId: validation.userId!,
      sessionId: validation.sessionId!,
      type: 'access',
    };

    const token = jwt.sign(payload, this.config.jwtSecret, {
      algorithm: this.config.algorithm ?? 'HS256',
      expiresIn: this.config.accessTokenLifespan,
    });

    return { token };
  }
}

const createDatabaseProvider = (db: Database, contentType: string): SessionProvider => {
  return new DatabaseSessionProvider(db, contentType);
};

const createSessionManager = ({ db, config }: { db: Database; config: SessionManagerConfig }) => {
  const provider = createDatabaseProvider(db, 'admin::session');
  return new SessionManager(provider, config);
};

export { createSessionManager, createDatabaseProvider };
Repository: strapi/strapi
File: packages/core/core/src/services/metrics/middleware.ts (TypeScript)

Code before:
import type { Core } from '@strapi/types';
import type { Sender } from './sender';

interface State {
  expires: number;
  counter: number;
}

function nextResetDate(): number {
  return Date.now() + 24 * 60 * 60 * 1000; // Now + 24 hours.
}

const createMiddleware = ({ sendEvent }: { sendEvent: Sender }) => {
  const state: State = {
    expires: nextResetDate(),
    counter: 0,
  };

  const middleware: Core.MiddlewareHandler = async (ctx, next) => {
    const { url, method } = ctx.request;

    if (
      !url.includes('.') &&
      url.includes('api/') &&
      ['GET', 'PUT', 'POST', 'DELETE'].includes(method)
    ) {
      if (Date.now() > state.expires) {
        state.expires = nextResetDate();
        state.counter = 0;
      }

      // Send max. 1000 events per day.
      if (state.counter < 1000) {
        sendEvent('didReceiveRequest', { eventProperties: { url: ctx.request.url } });

        // Increase counter.
        state.counter += 1;
      }
    }

    await next();
  };

  return middleware;
};

export default createMiddleware;


Review comment (suggestion):
```suggestion
      url.includes(strapi.config.get('api.rest.prefix')) &&
```

Code after:
import type { Core } from '@strapi/types';
import type { Sender } from './sender';

interface State {
  expires: number;
  counter: number;
}

function nextResetDate(): number {
  return Date.now() + 24 * 60 * 60 * 1000; // Now + 24 hours.
}

const createMiddleware = ({ sendEvent, strapi }: { sendEvent: Sender; strapi: Core.Strapi }) => {
  const state: State = {
    expires: nextResetDate(),
    counter: 0,
  };

  const middleware: Core.MiddlewareHandler = async (ctx, next) => {
    const { url, method } = ctx.request;

    if (
      !url.includes('.') &&
      url.includes(strapi.config.get('api.rest.prefix')) &&
      ['GET', 'PUT', 'POST', 'DELETE'].includes(method)
    ) {
      if (Date.now() > state.expires) {
        state.expires = nextResetDate();
        state.counter = 0;
      }

      // Send max. 1000 events per day.
      if (state.counter < 1000) {
        sendEvent('didReceiveRequest', { eventProperties: { url: ctx.request.url } });

        // Increase counter.
        state.counter += 1;
      }
    }

    await next();
  };

  return middleware;
};

export default createMiddleware;
Repository: strapi/strapi
File: packages/core/admin/server/src/routes/users.ts (TypeScript)

Code before:
    method: 'GET',
    path: '/users/me',
    handler: 'authenticated-user.getMe',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'PUT',
    path: '/users/me',
    handler: 'authenticated-user.updateMe',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'GET',
    path: '/users/me/permissions',
    handler: 'authenticated-user.getOwnPermissions',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'GET',
    path: '/users/me/getAiToken',
    handler: 'authenticated-user.getAiToken',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'POST',
    path: '/users',
    handler: 'user.create',
    config: {
      policies: [
        'admin::isAuthenticatedAdmin',
        { name: 'admin::hasPermissions', config: { actions: ['admin::users.create'] } },
      ],
    },
  },
  {
    method: 'GET',
    path: '/users',
    handler: 'user.find',
    config: {
      policies: [
        'admin::isAuthenticatedAdmin',
        { name: 'admin::hasPermissions', config: { actions: ['admin::users.read'] } },
      ],

Review comment (suggestion):
```suggestion
    path: '/users/me/ai-token',
```


Code after:
    method: 'GET',
    path: '/users/me',
    handler: 'authenticated-user.getMe',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'PUT',
    path: '/users/me',
    handler: 'authenticated-user.updateMe',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'GET',
    path: '/users/me/permissions',
    handler: 'authenticated-user.getOwnPermissions',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'GET',
    path: '/users/me/ai-token',
    handler: 'authenticated-user.getAiToken',
    config: {
      policies: ['admin::isAuthenticatedAdmin'],
    },
  },
  {
    method: 'POST',
    path: '/users',
    handler: 'user.create',
    config: {
      policies: [
        'admin::isAuthenticatedAdmin',
        { name: 'admin::hasPermissions', config: { actions: ['admin::users.create'] } },
      ],
    },
  },
  {
    method: 'GET',
    path: '/users',
    handler: 'user.find',
    config: {
      policies: [
        'admin::isAuthenticatedAdmin',
        { name: 'admin::hasPermissions', config: { actions: ['admin::users.read'] } },
      ],
Repository: strapi/strapi
File: packages/core/admin/shared/contracts/users.ts (TypeScript)

Code before:
    body: BaseRequestBody;
  }

  export interface Response {
    data: SanitizedAdminUser;
    error?:
      | errors.ApplicationError
      | errors.ValidationError<'ValidationError', { currentPassword: ['Invalid credentials'] }>
      | errors.YupValidationError;
  }
}

/**
 * GET /users/me/getAiToken - Get an AI token for the current admin user
 */
export declare namespace GetAiToken {
  export interface Request {
    query: {};
    body: {};
  }

  export interface Response {
    data: {
      token: string;
      expiresAt?: string;
      projectId?: string;
    };
    error?: errors.ApplicationError;
  }
}

/**
 * GET /users/me/permissions - Get the permissions of the current admin user
 */
export declare namespace GetOwnPermissions {
  export interface Request {
    query: {};
    body: {};
  }

  export interface Response {
    data: Permission[];
    error?: errors.ApplicationError;
  }
}


Review comment (suggestion):
```suggestion
```
I don't think there's a reason to expect projectId (although technically I think it's not considered private, just unneccessary)

Code after:
    body: BaseRequestBody;
  }

  export interface Response {
    data: SanitizedAdminUser;
    error?:
      | errors.ApplicationError
      | errors.ValidationError<'ValidationError', { currentPassword: ['Invalid credentials'] }>
      | errors.YupValidationError;
  }
}

/**
 * GET /users/me/getAiToken - Get an AI token for the current admin user
 */
export declare namespace GetAiToken {
  export interface Request {
    query: {};
    body: {};
  }

  export interface Response {
    data: {
      token: string;
      expiresAt?: string;
    };
    error?: errors.ApplicationError;
  }
}

/**
 * GET /users/me/permissions - Get the permissions of the current admin user
 */
export declare namespace GetOwnPermissions {
  export interface Request {
    query: {};
    body: {};
  }

  export interface Response {
    data: Permission[];
    error?: errors.ApplicationError;
  }
}
Repository: strapi/strapi
File: packages/core/core/src/core-api/routes/validation/utils.ts (TypeScript)

Code before:
        .replace('Document', '')
        .replace('Entry', '')
        .replace(/([A-Z])/g, ' $1')
        .trim();
      strapi.log.info(`📝 Generating validation schema for "${schemaName}"`);
    }

    // Temporary any placeholder before replacing with the actual schema type
    // Used to prevent infinite loops in cyclical data structures
    safeGlobalRegistrySet(id, z.any());

    // Generate the actual schema using the callback
    const schema = callback();

    // Replace the placeholder with the real schema
    safeGlobalRegistrySet(id, schema);

    // Show completion for user content only
    if (!isBuiltInSchema) {
      const fieldCount = Object.keys((schema as any)?._def?.shape || {}).length || 0;
      const schemaName = transformedId
        .replace('Document', '')
        .replace('Entry', '')
        .replace(/([A-Z])/g, ' $1')
        .trim();
      strapi.log.info(`   ✅ "${schemaName}" schema created with ${fieldCount} fields`);
    }

    return schema;
  } catch (error) {
    strapi.log.error(`Schema creation failed: Failed to create schema ${id}`);

    throw error;
  }
};


Review comment (bug):
I would keep all of these new `info` logs as `debug` too, possibly even lower (`silly`?) since it's weird to see these on every startup. If it only happened once it would make sense, but since it happens every time it's just kind of annoying 😆

Code after:
      strapi.log.debug(`📝 Generating validation schema for "${schemaName}"`);
    }

    // Temporary any placeholder before replacing with the actual schema type
    // Used to prevent infinite loops in cyclical data structures
    safeGlobalRegistrySet(id, z.any());

    // Generate the actual schema using the callback
    const schema = callback();

    // Replace the placeholder with the real schema
    safeGlobalRegistrySet(id, schema);

    // Show completion for user content only
    if (!isBuiltInSchema) {
      const fieldCount = Object.keys((schema as any)?._def?.shape || {}).length || 0;
      const schemaName = transformedId
        .replace('Document', '')
        .replace('Entry', '')
        .replace(/([A-Z])/g, ' $1')
        .trim();
      strapi.log.debug(`   ✅ "${schemaName}" schema created with ${fieldCount} fields`);
    }

    return schema;
  } catch (error) {
    strapi.log.error(`Schema creation failed: Failed to create schema ${id}`);

    throw error;
  }
};
Repository: strapi/strapi
File: packages/core/core/src/core-api/routes/validation/utils.ts (TypeScript)

Code before:
        .replace('Document', '')
        .replace('Entry', '')
        .replace(/([A-Z])/g, ' $1')
        .trim();
      strapi.log.info(`📝 Generating validation schema for "${schemaName}"`);
    }

    // Temporary any placeholder before replacing with the actual schema type
    // Used to prevent infinite loops in cyclical data structures
    safeGlobalRegistrySet(id, z.any());

    // Generate the actual schema using the callback
    const schema = callback();

    // Replace the placeholder with the real schema
    safeGlobalRegistrySet(id, schema);

    // Show completion for user content only
    if (!isBuiltInSchema) {
      const fieldCount = Object.keys((schema as any)?._def?.shape || {}).length || 0;
      const schemaName = transformedId
        .replace('Document', '')
        .replace('Entry', '')
        .replace(/([A-Z])/g, ' $1')
        .trim();
      strapi.log.info(`   ✅ "${schemaName}" schema created with ${fieldCount} fields`);
    }

    return schema;
  } catch (error) {
    strapi.log.error(`Schema creation failed: Failed to create schema ${id}`);

    throw error;
  }
};


Review comment (bug):
Makes sense, but maybe we don't need the extra info in the log? Could keep it as ?

strapi.log.debug(`Schema ${schemaName} successfully created and registered`);

Code after:
      strapi.log.debug(`📝 Generating validation schema for "${schemaName}"`);
    }

    // Temporary any placeholder before replacing with the actual schema type
    // Used to prevent infinite loops in cyclical data structures
    safeGlobalRegistrySet(id, z.any());

    // Generate the actual schema using the callback
    const schema = callback();

    // Replace the placeholder with the real schema
    safeGlobalRegistrySet(id, schema);

    // Show completion for user content only
    if (!isBuiltInSchema) {
      const fieldCount = Object.keys((schema as any)?._def?.shape || {}).length || 0;
      const schemaName = transformedId
        .replace('Document', '')
        .replace('Entry', '')
        .replace(/([A-Z])/g, ' $1')
        .trim();
      strapi.log.debug(`   ✅ "${schemaName}" schema created with ${fieldCount} fields`);
    }

    return schema;
  } catch (error) {
    strapi.log.error(`Schema creation failed: Failed to create schema ${id}`);

    throw error;
  }
};