ronantakizawa/codereview-bench huggingface dataset:
Repository: strapi/strapi
File: packages/core/core/src/services/content-source-maps.ts (TypeScript)

Code before:
const ENCODABLE_TYPES = [
  'string',
  'text',
  'richtext',
  'uid',
  'enumeration',
  'email',
  'date',
  'datetime',
  'time',
  'timestamp',
];

const EXCLUDED_FIELDS = [
  'id',
  'documentId',
  'locale',
  'localizations',
  'created_by',
  'updated_by',
  'created_at',
  'updated_at',
  'publishedAt',
];

const createContentSourceMapsService = (strapi: Core.Strapi) => {
  return {
    encodeField(text: string, key: string): string {
      const res = vercelStegaCombine(text, {
        // TODO: smarter metadata than just the key
        key,
      });
      return res;
    },

    async encodeEntry(options: {
      entryRootId: any;
      entryRootModel: string;
      entryData: any;
      model: any;
    }): Promise<any> {
      const { entryRootId, entryRootModel, entryData, model } = options;

      if (typeof entryData !== 'object' || entryData === null || entryData === undefined) {
        return entryData;
      }

      const encodedData = { ...entryData };

      await Promise.all(
        Object.keys(entryData).map(async (key) => {

Review comment (suggestion):
shouldn't we encode all but relations, components & dzs? 
```suggestion
const ENCODABLE_TYPES = [
      "string",
      "text",
      "richtext",
      "integer",
      "blocks",
      "biginteger",
      "float",
      "decimal",
      "date",
      "time",
      "datetime",
      "timestamp",
      "boolean",
      "enumeration",
      "json",
      "media",
      "email",
      "password",
      "uid",
];
```

Code after:

const ENCODABLE_TYPES = [
  'string',
  'text',
  'richtext',
  'biginteger',
  'date',
  'time',
  'datetime',
  'timestamp',
  'boolean',
  'enumeration',
  'json',
  'media',
  'email',
  'password',
  'uid',
  /**
   * We cannot modify the response shape, so types that aren't based on string cannot be encoded:
   * - json: object
   * - blocks: object, will require a custom implementation in a dedicated PR
   * - integer, float and decimal: number
   * - boolean: boolean (believe it or not)
   */
];

// TODO: use a centralized store for these fields that would be shared with the CM and CTB
const EXCLUDED_FIELDS = [
  'id',
  'documentId',
  'locale',
  'localizations',
  'created_by',
  'updated_by',
  'created_at',
  'updated_at',
  'publishedAt',
];

interface EncodingInfo {
  data: any;
  schema: Struct.Schema;
}

const createContentSourceMapsService = (strapi: Core.Strapi) => {
  return {
    encodeField(text: string, key: string): string {
      const res = vercelStegaCombine(text, {
        // TODO: smarter metadata than just the key
        key,
      });
Repository: strapi/strapi
File: packages/core/core/src/services/content-source-maps.ts (TypeScript)

Code before:

      return traverseEntity(
        ({ key, value, attribute }, { set }) => {
          if (!attribute || EXCLUDED_FIELDS.includes(key)) {
            return;
          }

          if (ENCODABLE_TYPES.includes(attribute.type) && typeof value === 'string') {
            set(key, this.encodeField(value, key) as any);
          }
        },
        {
          schema: model,
          getModel: (uid: string) => strapi.getModel(uid as any),
        },
        entryData
      );
    },

    async encodeSourceMaps(options: {
      data: any;
      contentType: Struct.ContentTypeSchema;
      rootId?: any;
      rootModel?: string;
    }): Promise<any> {
      const { data, contentType, rootId, rootModel } = options;

      try {
        if (Array.isArray(data)) {
          return await Promise.all(
            data.map((item) => this.encodeSourceMaps({ data: item, contentType }))
          );
        }

        if (typeof data !== 'object' || data === null) {
          return data;
        }

        const actualRootId = rootId || data.id;
        const actualRootModel = rootModel || contentType.uid;

        return await this.encodeEntry({
          entryRootId: actualRootId,
          entryRootModel: actualRootModel,
          entryData: data,
          model: contentType,
        });
      } catch (error) {
        strapi.log.error('Error encoding source maps:', error);
        return data;
      }

Review comment (suggestion):
how come you only check null here but above for entry data you also checked undefined

Code after:
        ({ key, value, attribute }, { set }) => {
          if (!attribute || EXCLUDED_FIELDS.includes(key)) {
            return;
          }

          if (ENCODABLE_TYPES.includes(attribute.type) && typeof value === 'string') {
            set(key, this.encodeField(value, key) as any);
          }
        },
        {
          schema,
          getModel: (uid) => strapi.getModel(uid as UID.Schema),
        },
        data
      );
    },

    async encodeSourceMaps({ data, schema }: EncodingInfo): Promise<any> {
      try {
        if (Array.isArray(data)) {
          return await Promise.all(
            data.map((item) => this.encodeSourceMaps({ data: item, schema }))
          );
        }

        if (typeof data !== 'object' || data === null) {
          return data;
        }

        return await this.encodeEntry({ data, schema });
      } catch (error) {
        strapi.log.error('Error encoding source maps:', error);
        return data;
      }
    },
  };
};

export { createContentSourceMapsService };
Repository: strapi/strapi
File: packages/core/content-type-builder/admin/src/components/AIChat/UploadCodeModal.tsx (TypeScript)

Code before:

    return files;
  };

  const handleDrop = async (e: React.DragEvent<HTMLDivElement>) => {
    e.preventDefault();
    setDragOver(false);

    if (!e.dataTransfer?.items) {
      return;
    }

    // For folder upload, process directories recursively
    if (importType === 'folder') {
      const items = e.dataTransfer.items;
      const allFiles: File[] = [];

      for (let i = 0; i < items.length; i++) {
        const item = items[i];
        // Use webkitGetAsEntry to access the file system entry
        const entry = item.webkitGetAsEntry?.();

        if (entry) {
          if (entry.isDirectory) {
            const dirEntry = entry as FileSystemDirectoryEntry;
            const files = await readDirectory(dirEntry);
            allFiles.push(...files);
          } else if (entry.isFile) {
            const fileEntry = entry as FileSystemFileEntry;
            const file = await new Promise<File>((resolve, reject) => {
              fileEntry.file(resolve, reject);
            });
            allFiles.push(file);
          }
        }
      }

      if (allFiles.length > 0) {
        onAddFiles(allFiles);
      }
    } else {
      // For zip files, just import them regularly
      if (e.dataTransfer.files) {
        onAddFiles(Array.from(e.dataTransfer.files));
      }
    }
  };

  return (
    <Flex
      position="relative"

Review comment (suggestion):
```suggestion
            const files = await readDirectory(entry as FileSystemDirectoryEntry);
```

Code after:

    return files;
  };

  const handleDrop = async (e: React.DragEvent<HTMLDivElement>) => {
    e.preventDefault();
    setDragOver(false);

    if (!e.dataTransfer?.items) {
      return;
    }

    // For folder upload, process directories recursively
    if (importType === 'folder') {
      const items = e.dataTransfer.items;
      const allFiles: File[] = [];

      for (let i = 0; i < items.length; i++) {
        const item = items[i];
        // Use webkitGetAsEntry to access the file system entry
        const entry = item.webkitGetAsEntry?.();

        if (entry) {
          if (entry.isDirectory) {
            const files = await readDirectory(entry as FileSystemDirectoryEntry);
            allFiles.push(...files);
          } else if (entry.isFile) {
            const file = await new Promise<File>((resolve, reject) => {
              (entry as FileSystemFileEntry).file(resolve, reject);
            });
            allFiles.push(file);
          }
        }
      }

      if (allFiles.length > 0) {
        onAddFiles(allFiles);
      }
    } else {
      // For zip files, just import them regularly
      if (e.dataTransfer.files) {
        onAddFiles(Array.from(e.dataTransfer.files));
      }
    }
  };

  return (
    <Flex
      position="relative"
      cursor="pointer"
      width="100%"
Repository: strapi/strapi
File: packages/core/content-type-builder/admin/src/components/AIChat/UploadCodeModal.tsx (TypeScript)

Code before:
    e.preventDefault();
    setDragOver(false);

    if (!e.dataTransfer?.items) {
      return;
    }

    // For folder upload, process directories recursively
    if (importType === 'folder') {
      const items = e.dataTransfer.items;
      const allFiles: File[] = [];

      for (let i = 0; i < items.length; i++) {
        const item = items[i];
        // Use webkitGetAsEntry to access the file system entry
        const entry = item.webkitGetAsEntry?.();

        if (entry) {
          if (entry.isDirectory) {
            const dirEntry = entry as FileSystemDirectoryEntry;
            const files = await readDirectory(dirEntry);
            allFiles.push(...files);
          } else if (entry.isFile) {
            const fileEntry = entry as FileSystemFileEntry;
            const file = await new Promise<File>((resolve, reject) => {
              fileEntry.file(resolve, reject);
            });
            allFiles.push(file);
          }
        }
      }

      if (allFiles.length > 0) {
        onAddFiles(allFiles);
      }
    } else {
      // For zip files, just import them regularly
      if (e.dataTransfer.files) {
        onAddFiles(Array.from(e.dataTransfer.files));
      }
    }
  };

  return (
    <Flex
      position="relative"
      cursor="pointer"
      width="100%"
      height="100%"
      minHeight="140px"
      borderStyle="dashed"

Review comment (suggestion):
```suggestion
              (entry as FileSystemFileEntry).file(resolve, reject);
```

Code after:
    e.preventDefault();
    setDragOver(false);

    if (!e.dataTransfer?.items) {
      return;
    }

    // For folder upload, process directories recursively
    if (importType === 'folder') {
      const items = e.dataTransfer.items;
      const allFiles: File[] = [];

      for (let i = 0; i < items.length; i++) {
        const item = items[i];
        // Use webkitGetAsEntry to access the file system entry
        const entry = item.webkitGetAsEntry?.();

        if (entry) {
          if (entry.isDirectory) {
            const files = await readDirectory(entry as FileSystemDirectoryEntry);
            allFiles.push(...files);
          } else if (entry.isFile) {
            const file = await new Promise<File>((resolve, reject) => {
              (entry as FileSystemFileEntry).file(resolve, reject);
            });
            allFiles.push(file);
          }
        }
      }

      if (allFiles.length > 0) {
        onAddFiles(allFiles);
      }
    } else {
      // For zip files, just import them regularly
      if (e.dataTransfer.files) {
        onAddFiles(Array.from(e.dataTransfer.files));
      }
    }
  };

  return (
    <Flex
      position="relative"
      cursor="pointer"
      width="100%"
      height="100%"
      minHeight="140px"
      borderStyle="dashed"
      borderColor={dragOver ? 'primary500' : error ? 'danger600' : 'neutral200'}
      background={dragOver ? 'primary100' : error ? 'danger100' : 'neutral100'}
Repository: strapi/strapi
File: packages/core/core/src/services/session-manager.ts (TypeScript)

Code before:
      sessionId,
      deviceId,
      origin,
      expiresAt,
    });

    const payload: Omit<RefreshTokenPayload, 'iat' | 'exp'> = {
      userId,
      sessionId,
      type: 'refresh',
    };

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

    return { token, sessionId };
  }

  validateAccessToken(
    token: string
  ): { isValid: true; payload: AccessTokenPayload } | { isValid: false; payload: null } {
    try {
      const payload = jwt.verify(token, this.config.jwtSecret, {
        algorithms: [this.config.algorithm ?? 'HS256'],
      }) as TokenPayload;

      // Ensure this is an access token
      if (!payload || payload.type !== 'access') {
        return { isValid: false, payload: null };
      }

      return { isValid: true, payload };
    } catch (err) {
      return { isValid: false, payload: null };
    }
  }

  async validateRefreshToken(token: string): Promise<ValidateRefreshTokenResult> {
    try {
      const verifyOptions: VerifyOptions = {
        algorithms: [this.config.algorithm ?? 'HS256'],
      };

      const payload = jwt.verify(
        token,
        this.config.jwtSecret,
        verifyOptions
      ) as RefreshTokenPayload;


Review comment (suggestion):
```suggestion
        algorithms: [this.config.algorithm ?? DEFAULT_ALGORITHM],
```

Just so we don't have a string everywhere

Code after:
      userId,
      sessionId,
      deviceId,
      origin,
      expiresAt,
    });

    const payload: Omit<RefreshTokenPayload, 'iat' | 'exp'> = {
      userId,
      sessionId,
      type: 'refresh',
    };

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

    return { token, sessionId };
  }

  validateAccessToken(
    token: string
  ): { isValid: true; payload: AccessTokenPayload } | { isValid: false; payload: null } {
    try {
      const payload = jwt.verify(token, this.config.jwtSecret, {
        algorithms: [this.config.algorithm ?? DEFAULT_ALGORITHM],
      }) as TokenPayload;

      // Ensure this is an access token
      if (!payload || payload.type !== 'access') {
        return { isValid: false, payload: null };
      }

      return { isValid: true, payload };
    } catch (err) {
      return { isValid: false, payload: null };
    }
  }

  async validateRefreshToken(token: string): Promise<ValidateRefreshTokenResult> {
    try {
      const verifyOptions: VerifyOptions = {
        algorithms: [this.config.algorithm ?? DEFAULT_ALGORITHM],
      };

      const payload = jwt.verify(
        token,
        this.config.jwtSecret,
        verifyOptions
      ) as RefreshTokenPayload;
Repository: strapi/strapi
File: packages/core/admin/server/src/controllers/validation/schema.ts (TypeScript)

Code before:
  z.literal(4),
  z.literal(6),
  z.literal(8),
  z.literal(12),
]);

export const UserLayoutSchema = z.object({
  version: z.number().int().min(0),
  order: z.array(WidgetUIDSchema).max(100),
  widths: z.record(WidgetUIDSchema, WidthSchema),
  updatedAt: z.string().datetime(),
}).strict();

export type UserLayout = z.infer<typeof UserLayoutSchema>;

// PATCH accepts partial changes
export const UserLayoutWriteSchema = z.object({
  order: z.array(WidgetUIDSchema).max(100).optional(),
  widths: z.record(WidgetUIDSchema, WidthSchema).optional(),
})
  .strict()
  .refine((v) => v.order || v.widths, {
    message: "At least one of 'order' or 'widths' is required",
  });

export type UserLayoutWrite = z.infer<typeof UserLayoutWriteSchema>;

Review comment (suggestion):
Unrelated but do you have prettier configured in your IDE? It should normally add an empty new line at the end of files with our config

Code after:
  .object({
    uid: z.string().nonempty(),
    width: WidthSchema,
  })
  .strict();

export const HomepageLayoutSchema = z
  .object({
    version: z.number().int().min(1),
    widgets: z.array(WidgetEntrySchema).max(100),
    updatedAt: z.string().datetime(),
  })
  .strict();

export type HomepageLayout = z.infer<typeof HomepageLayoutSchema>;

export const HomepageLayoutWriteSchema = z
  .object({
    version: z.number().int().min(1).optional(),
    widgets: z.array(WidgetEntrySchema).max(100),
    updatedAt: z.string().datetime().optional(),
  })
  .strict();

export type HomepageLayoutWrite = z.infer<typeof HomepageLayoutWriteSchema>;
Repository: strapi/strapi
File: packages/core/admin/server/src/controllers/homepage.ts (TypeScript)

Code before:
import type { Context } from 'koa';
import { getService } from '../utils';
import { UserLayout, UserLayoutWrite } from './validation/schema';

export default {
  async getKeyStatistics(): Promise<{
    data: Awaited<ReturnType<typeof homepageService.getKeyStatistics>>;
  }> {
    const homepageService = getService('homepage');
    return { data: await homepageService.getKeyStatistics() };
  },
  async getUserLayout(ctx: Context): Promise<{ data: UserLayout } | null> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;
    if (!userId) ctx.throw(401, 'Unauthorized');

    const data = await homepageService.getUserLayout(userId);
    if (!data) return null;
    return { data };
  },
  async updateUserLayout(ctx: Context): Promise<{ data: UserLayout }> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;
    if (!userId) ctx.throw(401, 'Unauthorized');

    const body = ctx.request.body as UserLayoutWrite;
    const data = await homepageService.updateUserLayout(userId, body);
    return { data };
  },
};


Review comment (security):
I don't think this check is necessary here. You can trust the `admin::isAuthenticatedAdmin` you used when defining the route to keep away unauthenticated requests

Code after:
import type { Context } from 'koa';
import { getService } from '../utils';
import { HomepageLayout, HomepageLayoutWrite } from './validation/schema';

export default {
  async getKeyStatistics(): Promise<{
    data: Awaited<ReturnType<typeof homepageService.getKeyStatistics>>;
  }> {
    const homepageService = getService('homepage');
    return { data: await homepageService.getKeyStatistics() };
  },
  async getHomepageLayout(ctx: Context): Promise<{ data: HomepageLayout | null }> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;

    const data = await homepageService.getHomepageLayout(userId);
    return { data };
  },
  async updateHomepageLayout(ctx: Context): Promise<{ data: HomepageLayout }> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;

    const body = ctx.request.body as HomepageLayoutWrite;
    const data = await homepageService.updateHomepageLayout(userId, body);
    return { data };
  },
};
Repository: strapi/strapi
File: packages/core/admin/server/src/controllers/homepage.ts (TypeScript)

Code before:
import type { Context } from 'koa';
import { getService } from '../utils';
import { UserLayout, UserLayoutWrite } from './validation/schema';

export default {
  async getKeyStatistics(): Promise<{
    data: Awaited<ReturnType<typeof homepageService.getKeyStatistics>>;
  }> {
    const homepageService = getService('homepage');
    return { data: await homepageService.getKeyStatistics() };
  },
  async getUserLayout(ctx: Context): Promise<{ data: UserLayout } | null> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;
    if (!userId) ctx.throw(401, 'Unauthorized');

    const data = await homepageService.getUserLayout(userId);
    if (!data) return null;
    return { data };
  },
  async updateUserLayout(ctx: Context): Promise<{ data: UserLayout }> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;
    if (!userId) ctx.throw(401, 'Unauthorized');

    const body = ctx.request.body as UserLayoutWrite;
    const data = await homepageService.updateUserLayout(userId, body);
    return { data };
  },
};


Review comment (suggestion):
I would personally prefer to the object with data when it's null as well. I think having the same response shape in all cases makes things easier in the frontend

Code after:
import type { Context } from 'koa';
import { getService } from '../utils';
import { HomepageLayout, HomepageLayoutWrite } from './validation/schema';

export default {
  async getKeyStatistics(): Promise<{
    data: Awaited<ReturnType<typeof homepageService.getKeyStatistics>>;
  }> {
    const homepageService = getService('homepage');
    return { data: await homepageService.getKeyStatistics() };
  },
  async getHomepageLayout(ctx: Context): Promise<{ data: HomepageLayout | null }> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;

    const data = await homepageService.getHomepageLayout(userId);
    return { data };
  },
  async updateHomepageLayout(ctx: Context): Promise<{ data: HomepageLayout }> {
    const homepageService = getService('homepage');
    const userId = ctx.state.user?.id;

    const body = ctx.request.body as HomepageLayoutWrite;
    const data = await homepageService.updateHomepageLayout(userId, body);
    return { data };
  },
};
Repository: strapi/strapi
File: packages/core/admin/server/src/content-types/session.ts (TypeScript)

Code before:
export default {
  collectionName: 'admin_sessions',
  info: {
    name: 'Session',
    description: 'Admin user session management',
    singularName: 'session',
    pluralName: 'sessions',
    displayName: 'Session',
  },
  pluginOptions: {
    'content-manager': {
      visible: false,
    },
    'content-type-builder': {
      visible: false,
    },
  },
  attributes: {
    userId: {
      type: 'string',
      required: true,
      configurable: false,
      private: true,
    },
    sessionId: {
      type: 'string',
      unique: true,

Review comment (suggestion):
`strapi_` is a guaranteed restricted prefix, I don't think `admin_` is. Also, this is generic sessions, not just admin, so this makes it more clear

Code after:
export default {
  collectionName: 'strapi_sessions',
  info: {
    name: 'Session',
    description: 'Session Manager storage',
    singularName: 'session',
    pluralName: 'sessions',
    displayName: 'Session',
  },
  options: {
    draftAndPublish: false,
  },
  pluginOptions: {
    'content-manager': {
      visible: false,
    },
    'content-type-builder': {
      visible: false,
    },
    i18n: {
      localized: false,
    },
  },
  attributes: {
    userId: {
      type: 'string',
      required: true,