ronantakizawa/codereview-bench huggingface dataset:
Repository: payloadcms/payload
File: test/joins/int.spec.ts (TypeScript)

Code before:
      joins: {
        documentsAndFolders: {
          limit: 100_000,
          sort: 'name',
          where: {
            and: [
              {
                relationTo: {
                  equals: 'payload-folders',
                },
              },
              {
                folderType: {
                  in: ['folderPoly1'],
                },
              },
            ],
          },
        },
      },
    })

    expect(findFolder?.docs[0]?.documentsAndFolders?.docs).toHaveLength(1)
  })

  it('should not throw an error with where query using EXISTS / EQUALS operators on hasMany select fields', async () => {
    await payload.delete({ collection: 'payload-folders', where: {} })
    const folderDoc = await payload.create({
      collection: 'payload-folders',
      data: {
        name: 'scopedFolder',
        folderType: ['folderPoly1', 'folderPoly2'],
      },
    })

    await payload.create({
      collection: 'payload-folders',
      data: {
        name: 'childFolder',
        folderType: ['folderPoly1'],
        folder: folderDoc.id,
      },
    })

    const findFolder = await payload.find({
      collection: 'payload-folders',
      where: {
        id: {
          equals: folderDoc.id,
        },
      },

Review comment (suggestion):
I would not describe it as "not throw an error" and instead describe the type of query that works. 
I'd write something like "Should query where 'exists' and 'equals' operators for hasMany select fields"

Code after:
      joins: {
        documentsAndFolders: {
          limit: 100_000,
          sort: 'name',
          where: {
            and: [
              {
                relationTo: {
                  equals: 'payload-folders',
                },
              },
              {
                folderType: {
                  in: ['folderPoly1'],
                },
              },
            ],
          },
        },
      },
    })

    expect(findFolder?.docs[0]?.documentsAndFolders?.docs).toHaveLength(1)
  })

  it('should query where with exists for hasMany select fields', async () => {
    await payload.delete({ collection: 'payload-folders', where: {} })
    const folderDoc = await payload.create({
      collection: 'payload-folders',
      data: {
        name: 'scopedFolder',
        folderType: ['folderPoly1', 'folderPoly2'],
      },
    })

    await payload.create({
      collection: 'payload-folders',
      data: {
        name: 'childFolder',
        folderType: ['folderPoly1'],
        folder: folderDoc.id,
      },
    })

    const findFolder = await payload.find({
      collection: 'payload-folders',
      where: {
        id: {
          equals: folderDoc.id,
        },
      },
Repository: payloadcms/payload
File: packages/ui/src/providers/RouteCache/index.tsx (TypeScript)

Code before:
  const clearAfterPathnameChange = useRef(false)

  const clearRouteCache = useCallback(() => {
    router.refresh()
  }, [router])

  useEffect(() => {
    const handlePopState = () => {
      // Calling `router.refresh()` directly here doesn't work. Probably fires too early.
      // Instead, need to set a flag that we can check on the next pathname change.
      clearAfterPathnameChange.current = true
    }

    window.addEventListener('popstate', handlePopState)

    return () => {
      window.removeEventListener('popstate', handlePopState)
    }
  }, [router])

  useEffect(() => {
    if (cachingEnabled || clearAfterPathnameChange.current) {
      clearAfterPathnameChange.current = false
      clearRouteCache()
    }
  }, [pathname, clearRouteCache, cachingEnabled])

  return <Context value={{ cachingEnabled, clearRouteCache }}>{children}</Context>
}

export const useRouteCache = () => use(Context)


Review comment (suggestion):
I think this is a good candidate for `useEffectEvent` to make sure this runs more predictably and only if pathname changes

Code after:
  const pathname = usePathname()
  const router = useRouter()

  /**
   * Next.js caches pages in memory in their {@link https://nextjs.org/docs/app/guides/caching#client-side-router-cache Client-side Router Cache},
   * causing forward/back browser navigation to show stale data from a previous visit.
   * The problem is this bfcache-like behavior has no opt-out, even if the page is dynamic, requires authentication, etc.
   * The workaround is to force a refresh when navigating via the browser controls.
   * This should be removed if/when Next.js supports this natively.
   */
  const clearAfterPathnameChange = useRef(false)

  const clearRouteCache = useCallback(() => {
    router.refresh()
  }, [router])

  useEffect(() => {
    const handlePopState = () => {
      clearAfterPathnameChange.current = true
    }

    window.addEventListener('popstate', handlePopState)

    return () => {
      window.removeEventListener('popstate', handlePopState)
    }
  }, [router])

  const handlePathnameChange = useEffectEvent((pathname: string) => {
    if (cachingEnabled || clearAfterPathnameChange.current) {
      clearAfterPathnameChange.current = false
      clearRouteCache()
    }
  })

  useEffect(() => {
    handlePathnameChange(pathname)
  }, [pathname])

  return <Context value={{ cachingEnabled, clearRouteCache }}>{children}</Context>
}

export const useRouteCache = () => use(Context)
Repository: payloadcms/payload
File: templates/with-cloudflare-d1/next.config.ts (TypeScript)

Code before:
import { withPayload } from '@payloadcms/next/withPayload'
import { initOpenNextCloudflareForDev } from '@opennextjs/cloudflare'

initOpenNextCloudflareForDev({ environment: process.env.CLOUDFLARE_ENV })

/** @type {import('next').NextConfig} */
const nextConfig = {
  // Your Next.js config here
  webpack: (webpackConfig: any) => {
    webpackConfig.resolve.extensionAlias = {
      '.cjs': ['.cts', '.cjs'],
      '.js': ['.ts', '.tsx', '.js', '.jsx'],
      '.mjs': ['.mts', '.mjs'],
    }

    return webpackConfig
  },
}

export default withPayload(nextConfig, { devBundleServerPackages: false })


Review comment (suggestion):
Despite this being the recommended way to do in OpenNext, as we're using a custom getCloudflareContextFromWrangler() in the payload.config.ts, we no longer need this one.

Payload doesn't load the next.config when inside the CLI (e.g. npx payload migrate), so we can't rely on just the Next.js initialization to handle the Cloudflare bindings

Code after:
import { withPayload } from '@payloadcms/next/withPayload'

/** @type {import('next').NextConfig} */
const nextConfig = {
  // Your Next.js config here
  webpack: (webpackConfig: any) => {
    webpackConfig.resolve.extensionAlias = {
      '.cjs': ['.cts', '.cjs'],
      '.js': ['.ts', '.tsx', '.js', '.jsx'],
      '.mjs': ['.mts', '.mjs'],
    }

    return webpackConfig
  },
}

export default withPayload(nextConfig, { devBundleServerPackages: false })
Repository: payloadcms/payload
File: packages/next/src/utilities/slugify.ts (TypeScript)

Code before:

  if (!req.user) {
    throw new UnauthorizedError()
  }

  const docConfig = collectionSlug
    ? req.payload.collections[collectionSlug]?.config
    : globalSlug
      ? req.payload.config.globals.find((g) => g.slug === globalSlug)
      : null

  const { field } = getFieldByPath({
    config: req.payload.config,
    fields: flattenAllFields({ fields: docConfig?.fields || [] }),
    path: path || '',
  })

  const CustomField = field.admin?.components?.Field

  // This is required for type safety, rip
  const customSlugify =
    typeof CustomField === 'object' &&
    'serverProps' in CustomField &&
    'slugify' in CustomField.serverProps
      ? (CustomField.serverProps.slugify as Slugify)
      : undefined

  const slugify = customSlugify || defaultSlugify

  const result = await slugify(val)

  return result
}


Review comment (suggestion):
@denolfe told me that the reason we don't use Zod or Valibot was simply because they weren't popular when Payload was created. Perhaps it's time to start adopting them, at least incrementally for cases like these? Using a validator is more readable and less prone to errors.

Code after:
> = async (args) => {
  const { collectionSlug, data, globalSlug, path, req, valueToSlugify } = args

  if (!req.user) {
    throw new UnauthorizedError()
  }

  const docConfig = collectionSlug
    ? req.payload.collections[collectionSlug]?.config
    : globalSlug
      ? req.payload.config.globals.find((g) => g.slug === globalSlug)
      : null

  if (!docConfig) {
    throw new Error()
  }

  const { field } = getFieldByPath({
    config: req.payload.config,
    fields: flattenAllFields({ fields: docConfig.fields }),
    path,
  })

  const customSlugify = (
    typeof field?.custom?.slugify === 'function' ? field.custom.slugify : undefined
  ) as Slugify

  const result = customSlugify
    ? await customSlugify({ data, req, valueToSlugify })
    : defaultSlugify(valueToSlugify)

  return result
}
Repository: payloadcms/payload
File: packages/richtext-lexical/src/field/rscEntry.tsx (TypeScript)

Code before:
  }
  if (args.admin?.hideAddBlockButton) {
    admin.hideAddBlockButton = true
  }
  if (args.admin?.hideDraggableBlockElement) {
    admin.hideDraggableBlockElement = true
  }

  const props: LexicalRichTextFieldProps = {
    clientFeatures,
    featureClientImportMap,
    featureClientSchemaMap, // TODO: Does client need this? Why cant this just live in the server
    field: args.clientField as RichTextFieldClient,
    forceRender: args.forceRender,
    initialLexicalFormState,
    lexicalEditorConfig: args.sanitizedEditorConfig.lexical,
    path,
    permissions: args.permissions,
    readOnly: args.readOnly,
    renderedBlocks: args.renderedBlocks,
    schemaPath,
  }
  if (Object.keys(admin).length) {
    props.admin = admin
  }

  for (const key in props) {
    if (props[key as keyof LexicalRichTextFieldProps] === undefined) {
      delete props[key as keyof LexicalRichTextFieldProps]
    }
  }

  return <RichTextField {...props} />
}


Review comment (suggestion):
Do not unnecessarily send `featureClientImportMap` to client if it's empty. We do the same for other props

Code after:
  }
  if (args.admin?.hideAddBlockButton) {
    admin.hideAddBlockButton = true
  }
  if (args.admin?.hideDraggableBlockElement) {
    admin.hideDraggableBlockElement = true
  }

  const props: LexicalRichTextFieldProps = {
    clientFeatures,
    featureClientSchemaMap, // TODO: Does client need this? Why cant this just live in the server
    field: args.clientField as RichTextFieldClient,
    forceRender: args.forceRender,
    initialLexicalFormState,
    lexicalEditorConfig: args.sanitizedEditorConfig.lexical,
    path,
    permissions: args.permissions,
    readOnly: args.readOnly,
    renderedBlocks: args.renderedBlocks,
    schemaPath,
  }
  if (Object.keys(admin).length) {
    props.admin = admin
  }
  if (Object.keys(featureClientImportMap).length) {
    props.featureClientImportMap = featureClientImportMap
  }

  for (const key in props) {
    if (props[key as keyof LexicalRichTextFieldProps] === undefined) {
      delete props[key as keyof LexicalRichTextFieldProps]
    }
  }

  return <RichTextField {...props} />
}
Repository: payloadcms/payload
File: packages/richtext-lexical/src/features/migrations/lexicalPluginToLexical/nodes/unknownConvertedNode/index.tsx (TypeScript)

Code before:
}

export type SerializedUnknownConvertedNode = Spread<
  {
    data: UnknownConvertedNodeData
  },
  SerializedLexicalNode
>

const Component = React.lazy(() =>
  import('./Component.js').then((module) => ({
    default: module.UnknownConvertedNodeComponent,
  })),
)

/** @noInheritDoc */
export class UnknownConvertedNode extends DecoratorNode<JSX.Element> {
  __data: UnknownConvertedNodeData

  constructor({ data, key }: { data: UnknownConvertedNodeData; key?: NodeKey }) {
    super(key)
    this.__data = data
  }

  static override clone(node: UnknownConvertedNode): UnknownConvertedNode {
    return new UnknownConvertedNode({
      data: node.__data,
      key: node.__key,
    })
  }

  static override getType(): string {
    return 'unknownConverted'
  }

  static override importJSON(serializedNode: SerializedUnknownConvertedNode): UnknownConvertedNode {
    const node = $createUnknownConvertedNode({ data: serializedNode.data })
    return node
  }

  canInsertTextAfter(): true {
    return true
  }

  canInsertTextBefore(): true {
    return true
  }

  override createDOM(config: EditorConfig): HTMLElement {
    const element = document.createElement('span')
    addClassNamesToElement(element, 'unknownConverted')

Review comment (suggestion):
I don't think these changes in clone are necessary. It's already handled by `$applyNodeReplacement`. The Lexical codebase doesn't use this pattern.

Code after:
}

export type SerializedUnknownConvertedNode = Spread<
  {
    data: UnknownConvertedNodeData
  },
  SerializedLexicalNode
>

const Component = React.lazy(() =>
  import('./Component.js').then((module) => ({
    default: module.UnknownConvertedNodeComponent,
  })),
)

/** @noInheritDoc */
export class UnknownConvertedNode extends DecoratorNode<JSX.Element> {
  __data: UnknownConvertedNodeData

  constructor({ data, key }: { data: UnknownConvertedNodeData; key?: NodeKey }) {
    super(key)
    this.__data = data
  }

  static override clone(node: UnknownConvertedNode): UnknownConvertedNode {
    return new this({
      data: node.__data,
      key: node.__key,
    })
  }

  static override getType(): string {
    return 'unknownConverted'
  }

  static override importJSON(serializedNode: SerializedUnknownConvertedNode): UnknownConvertedNode {
    const node = $createUnknownConvertedNode({ data: serializedNode.data })
    return node
  }

  canInsertTextAfter(): true {
    return true
  }

  canInsertTextBefore(): true {
    return true
  }

  override createDOM(config: EditorConfig): HTMLElement {
    const element = document.createElement('span')
    addClassNamesToElement(element, 'unknownConverted')
Repository: payloadcms/payload
File: packages/plugin-mcp/src/collections/createApiKeysCollection.ts (TypeScript)

Code before:
      defaultValue: true,
      label: camelCasedName,
    }
  })

  return {
    slug: 'payload-mcp-api-keys',
    admin: {
      group: 'MCP',
      useAsTitle: 'label',
    },
    auth: {
      disableLocalStrategy: true,
      useAPIKey: true,
    },
    fields: [
      {
        name: 'user',
        type: 'relationship',
        admin: {
          description: 'The user that the API key is associated with.',
        },
        relationTo:
          typeof pluginOptions.userCollection === 'string'
            ? pluginOptions.userCollection
            : pluginOptions.userCollection?.slug || 'users',
        required: true,
      },
      {
        name: 'label',
        type: 'text',
        admin: {
          description: 'A useful label for the API key.',
        },
      },
      {
        name: 'description',
        type: 'text',
        admin: {
          description: 'The purpose of the API key.',
        },
      },

      ...addEnabledCollectionTools(collections),

      ...(customTools.length > 0
        ? [
            {
              type: 'collapsible' as const,
              admin: {
                position: 'sidebar' as const,

Review comment (security):
This could be improved. Since `pluginOptions.userCollection` is an optional field I am guessing most users won't have it configured. I would want the same fallback handling as we do for the admin user sanitization: https://github.com/payloadcms/payload/blob/91e64b3937765dfcf75011ae34f3c8710b0c9e81/packages/payload/src/config/sanitize.ts#L57

Always need to go for lowest amount of config possible and sensibly defaulting as we can.

Code after:
      },
      defaultValue: true,
      label: camelCasedName,
    }
  })

  const customResourceFields =
    pluginOptions.mcp?.resources?.map((resource) => {
      const camelCasedName = toCamelCase(resource.name)
      return {
        name: camelCasedName,
        type: 'checkbox' as const,
        admin: {
          description: resource.description,
        },
        defaultValue: true,
        label: camelCasedName,
      }
    }) || []

  const customPromptFields =
    pluginOptions.mcp?.prompts?.map((prompt) => {
      const camelCasedName = toCamelCase(prompt.name)
      return {
        name: camelCasedName,
        type: 'checkbox' as const,
        admin: {
          description: prompt.description,
        },
        defaultValue: true,
        label: camelCasedName,
      }
    }) || []

  const userCollection = pluginOptions.userCollection
    ? typeof pluginOptions.userCollection === 'string'
      ? pluginOptions.userCollection
      : pluginOptions.userCollection.slug
    : 'users'

  return {
    slug: 'payload-mcp-api-keys',
    admin: {
      group: 'MCP',
      useAsTitle: 'label',
    },
    auth: {
      disableLocalStrategy: true,
      useAPIKey: true,
    },
    fields: [
Repository: payloadcms/payload
File: packages/plugin-mcp/src/endpoints/mcp.ts (TypeScript)

Code before:
      const sha256APIKeyIndex = crypto
        .createHmac('sha256', payload.secret)
        .update(apiKey || '')
        .digest('hex')

      const apiKeyConstraints = [
        {
          apiKeyIndex: {
            equals: sha256APIKeyIndex,
          },
        },
      ]

      const where: Where = {
        or: apiKeyConstraints,
      }

      const { docs } = await payload.find({
        collection: 'payload-mcp-api-keys',
        limit: 1,
        pagination: false,
        where,
      })

      if (docs.length === 0) {
        throw new AuthenticationError()
      }

      if (useVerboseLogs) {
        payload.logger.info('[payload-mcp] API Key is valid')
      }

      return docs[0] as MCPAccessSettings
    }

    const mcpAccessSettings = pluginOptions.overrideAuth
      ? await pluginOptions.overrideAuth(req, getDefaultMcpAccessSettings)
      : await getDefaultMcpAccessSettings()

    const handler = getMCPHandler(pluginOptions, mcpAccessSettings, req)
    const request = createRequestFromPayloadRequest(req)
    return await handler(request)
  }
  return mcpHandler
}


Review comment (bug):
This is going to give the message: 'The email or password provided is incorrect.' though, I don't think that is right as we're dealing with API keys, right?

Unauthorized error will also give a 401 Unauthorized, with message 'you must be logged in to make this request.'
If neither are what you need then you might need to introduce translations into your plugin.

Code after:
      if (apiKey === null) {
        throw new UnauthorizedError()
      }

      const sha256APIKeyIndex = crypto
        .createHmac('sha256', payload.secret)
        .update(apiKey || '')
        .digest('hex')

      const apiKeyConstraints = [
        {
          apiKeyIndex: {
            equals: sha256APIKeyIndex,
          },
        },
      ]

      const where: Where = {
        or: apiKeyConstraints,
      }

      const { docs } = await payload.find({
        collection: 'payload-mcp-api-keys',
        limit: 1,
        pagination: false,
        where,
      })

      if (docs.length === 0) {
        throw new UnauthorizedError()
      }

      if (useVerboseLogs) {
        payload.logger.info('[payload-mcp] API Key is valid')
      }

      return docs[0] as MCPAccessSettings
    }

    const mcpAccessSettings = pluginOptions.overrideAuth
      ? await pluginOptions.overrideAuth(req, getDefaultMcpAccessSettings)
      : await getDefaultMcpAccessSettings()

    const handler = getMCPHandler(pluginOptions, mcpAccessSettings, req)
    const request = createRequestFromPayloadRequest(req)
    return await handler(request)
  }
  return mcpHandler
}
Repository: payloadcms/payload
File: packages/payload/src/collections/operations/findByID.ts (TypeScript)

Code before:
      where: fullWhere,
    }

    // execute only if there's a custom ID and potentially overwriten access on id
    if (req.payload.collections[collectionConfig.slug]!.customIDType) {
      await validateQueryPaths({
        collectionConfig,
        overrideAccess,
        req,
        where,
      })
    }

    // /////////////////////////////////////
    // Find by ID
    // /////////////////////////////////////

    if (!findOneArgs.where?.and?.[0]?.id) {
      throw new NotFound(t)
    }

    // Query the document first
    const docFromDB = await req.payload.db.findOne(findOneArgs)

    // Return NotFound when access control denies
    if (!docFromDB && !args.data && !overrideAccess && accessResult !== true) {
      if (!disableErrors) {
        throw new NotFound(req.t)
      }
      return null!
    }

    // Use provided data or queried doc
    let result: DataFromCollectionSlug<TSlug> =
      (args.data as DataFromCollectionSlug<TSlug>) ?? docFromDB!

    if (!result) {
      if (!disableErrors) {
        throw new NotFound(req.t)
      }

      return null!
    }

    // /////////////////////////////////////
    // Include Lock Status if required
    // /////////////////////////////////////

    if (includeLockStatus && id) {
      let lockStatus: (JsonObject & TypeWithID) | null = null


Review comment (suggestion):
Feels like we just need 1 check `!docFromDB`, it either exists or it does not. Then you can remove L188-L194.

Code after:
      where: fullWhere,
    }

    // execute only if there's a custom ID and potentially overwriten access on id
    if (req.payload.collections[collectionConfig.slug]!.customIDType) {
      await validateQueryPaths({
        collectionConfig,
        overrideAccess,
        req,
        where,
      })
    }

    // /////////////////////////////////////
    // Find by ID
    // /////////////////////////////////////

    if (!findOneArgs.where?.and?.[0]?.id) {
      throw new NotFound(t)
    }

    const docFromDB = await req.payload.db.findOne(findOneArgs)

    if (!docFromDB && !args.data) {
      if (!disableErrors) {
        throw new NotFound(req.t)
      }
      return null!
    }

    let result: DataFromCollectionSlug<TSlug> =
      (args.data as DataFromCollectionSlug<TSlug>) ?? docFromDB!

    // /////////////////////////////////////
    // Include Lock Status if required
    // /////////////////////////////////////

    if (includeLockStatus && id) {
      let lockStatus: (JsonObject & TypeWithID) | null = null

      try {
        const lockDocumentsProp = collectionConfig?.lockDocuments

        const lockDurationDefault = 300 // Default 5 minutes in seconds
        const lockDuration =
          typeof lockDocumentsProp === 'object' ? lockDocumentsProp.duration : lockDurationDefault
        const lockDurationInMilliseconds = lockDuration * 1000

        const lockedDocument = await req.payload.find({
          collection: lockedDocumentsCollectionSlug,
          depth: 1,