ronantakizawa/codereview-bench huggingface dataset:
Repository: payloadcms/payload
File: packages/ui/src/elements/CloseModalOnRouteChange/index.tsx (TypeScript)

Code before:
'use client'

import { useModal } from '@faceless-ui/modal'
import { usePathname } from 'next/navigation.js'
import { useEffect } from 'react'

export function CloseModalOnRouteChange() {
  const { closeAllModals } = useModal()
  const pathname = usePathname()

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

  return null
}


Review comment (performance):
Does this run on mount? If so, we should probably prevent that using a ref, see example below.

We should also be using `useEffectEvent` here, so that we only react to changes to `pathname`. [Example](https://github.com/payloadcms/payload/blob/main/packages/ui/src/providers/RouteCache/index.tsx#L56).

```ts
const initialRenderRef = useRef(true)

useEffect(() => {
  if (initialRenderRef.current) {
    initialRenderRef.current = false
    return
  }
  
  closeAllModalsEffectEvent(pathname)
}, [pathname])
```

Code after:
'use client'

import { useModal } from '@faceless-ui/modal'
import { usePathname } from 'next/navigation.js'
import { useEffect, useRef } from 'react'

import { useEffectEvent } from '../../hooks/useEffectEvent.js'

export function CloseModalOnRouteChange() {
  const { closeAllModals } = useModal()
  const pathname = usePathname()

  const closeAllModalsEffectEvent = useEffectEvent(() => {
    closeAllModals()
  })

  const initialRenderRef = useRef(true)

  useEffect(() => {
    if (initialRenderRef.current) {
      initialRenderRef.current = false
      return
    }

    closeAllModalsEffectEvent()
  }, [pathname])

  return null
}
Repository: payloadcms/payload
File: packages/payload/src/versions/saveVersion.ts (TypeScript)

Code before:
    let errorMessage: string | undefined

    if (collection) {
      errorMessage = `There was an error while saving a version for the ${typeof collection.labels.singular === 'string' ? collection.labels.singular : collection.slug} with ID ${id}.`
    }
    if (global) {
      errorMessage = `There was an error while saving a version for the global ${typeof global.label === 'string' ? global.label : global.slug}.`
    }
    payload.logger.error({ err, msg: errorMessage })
    return undefined!
  }

  const max = getVersionsMax(collection || global!)

  if (createNewVersion && max > 0) {
    await enforceMaxVersions({
      id,
      collection,
      global,
      max,
      payload,
      req,
    })
  }
  if (returning === false) {
    return {}
  }

  let createdVersion = (result as any).version

  createdVersion = sanitizeInternalFields(createdVersion)
  createdVersion.id = (result as any).parent

  return createdVersion
}


Review comment (suggestion):
I'd rather this return `null` though the return type is likely not going to allow for this unless we overload the type based on signature to avoid a breaking change.

Code after:
          global,
          payload,
          publishSpecificLocale,
          req,
          select,
        })
      }
    }
  } catch (err) {
    let errorMessage: string | undefined

    if (collection) {
      errorMessage = `There was an error while saving a version for the ${typeof collection.labels.singular === 'string' ? collection.labels.singular : collection.slug} with ID ${id}.`
    }
    if (global) {
      errorMessage = `There was an error while saving a version for the global ${typeof global.label === 'string' ? global.label : global.slug}.`
    }
    payload.logger.error({ err, msg: errorMessage })
    return undefined!
  }

  const max = getVersionsMax(collection || global!)

  if (createNewVersion && max > 0) {
    await enforceMaxVersions({
      id,
      collection,
      global,
      max,
      payload,
      req,
    })
  }
  if (returning === false) {
    return null
  }

  let createdVersion = (result as any).version

  createdVersion = sanitizeInternalFields(createdVersion)
  createdVersion.id = (result as any).parent

  return createdVersion
}
Repository: payloadcms/payload
File: packages/richtext-lexical/src/utilities/applyBaseFilterToFields.ts (TypeScript)

Code before:
        const hidden = admin?.hidden
        if (typeof hidden === 'function' && hidden({ user } as { user: TypedUser })) {
          return false
        }

        // Apply baseFilter (with backwards compatibility for baseListFilter)
        const baseFilter = admin?.baseFilter ?? admin?.baseListFilter
        const baseFilterResult = await baseFilter?.({
          limit: 0,
          page: 1,
          req,
          sort: 'id',
        })

        // If no baseFilter, return original result
        if (!baseFilterResult) {
          return originalResult
        }

        // If original result is true, just return the baseFilter
        if (originalResult === true) {
          return baseFilterResult
        }

        // Combine original and baseFilter results
        return {
          and: [originalResult, baseFilterResult],
        }
      }

      return relationshipField
    }

    // Recursively process nested fields
    if ('fields' in field && field.fields) {
      return {
        ...field,
        fields: applyBaseFilterToFields(field.fields, config),
      }
    }

    // Handle tabs
    if (field.type === 'tabs' && 'tabs' in field) {
      return {
        ...field,
        tabs: field.tabs.map((tab) => ({
          ...tab,
          fields: applyBaseFilterToFields(tab.fields, config),
        })),
      }
    }

Review comment (refactor):
Can we use the `combineWhereConstraints([originalResult, baseFilterResult], 'and')` helper function from payload?

Code after:

        // Check if collection is hidden
        const hidden = admin?.hidden
        if (typeof hidden === 'function' && hidden({ user } as { user: TypedUser })) {
          return false
        }

        // Apply baseFilter (with backwards compatibility for baseListFilter)
        const baseFilter = admin?.baseFilter ?? admin?.baseListFilter
        const baseFilterResult = await baseFilter?.({
          limit: 0,
          page: 1,
          req,
          sort: 'id',
        })

        // If no baseFilter, return original result
        if (!baseFilterResult) {
          return originalResult
        }

        // If original result is true, just return the baseFilter
        if (originalResult === true) {
          return baseFilterResult
        }

        // Combine original and baseFilter results
        return combineWhereConstraints([originalResult, baseFilterResult], 'and')
      }

      return relationshipField
    }

    // Recursively process nested fields
    if ('fields' in field && field.fields) {
      return {
        ...field,
        fields: applyBaseFilterToFields(field.fields, config),
      }
    }

    // Handle tabs
    if (field.type === 'tabs' && 'tabs' in field) {
      return {
        ...field,
        tabs: field.tabs.map((tab) => ({
          ...tab,
          fields: applyBaseFilterToFields(tab.fields, config),
        })),
      }
    }
Repository: payloadcms/payload
File: packages/payload/src/collections/operations/local/find.ts (TypeScript)

Code before:
   *
   * This argument has no effect unless `trash` is enabled on the collection.
   * @default false
   */
  trash?: boolean
  /**
   * If you set `overrideAccess` to `false`, you can pass a user to use against the access control checks.
   */
  user?: Document
  /**
   * A filter [query](https://payloadcms.com/docs/queries/overview)
   */
  where?: Where
}

export async function findLocal<
  TSlug extends CollectionSlug,
  TSelect extends SelectFromCollectionSlug<TSlug>,
  TDraft extends boolean = false,
>(
  payload: Payload,
  options: { draft?: TDraft } & Options<TSlug, TSelect>,
): Promise<
  PaginatedDocs<
    TDraft extends true
      ? GeneratedTypes extends { strictDraftTypes: true }
        ? DraftTransformCollectionWithSelect<TSlug, TSelect>
        : TransformCollectionWithSelect<TSlug, TSelect>
      : TransformCollectionWithSelect<TSlug, TSelect>
  >
> {
  const {
    collection: collectionSlug,
    currentDepth,
    depth,
    disableErrors,
    draft = false,
    includeLockStatus,
    joins,
    limit,
    overrideAccess = true,
    page,
    pagination = true,
    populate,
    select,
    showHiddenFields,
    sort,
    trash = false,
    where,
  } = options


Review comment (suggestion):
Small oversight, just pushed a fix - we should take the types from `PayloadTypes` instead of `GeneratedTypes` where possible

Code after:
   *
   * This argument has no effect unless `trash` is enabled on the collection.
   * @default false
   */
  trash?: boolean
  /**
   * If you set `overrideAccess` to `false`, you can pass a user to use against the access control checks.
   */
  user?: Document
  /**
   * A filter [query](https://payloadcms.com/docs/queries/overview)
   */
  where?: Where
}

export async function findLocal<
  TSlug extends CollectionSlug,
  TSelect extends SelectFromCollectionSlug<TSlug>,
  TDraft extends boolean = false,
>(
  payload: Payload,
  options: { draft?: TDraft } & Options<TSlug, TSelect>,
): Promise<
  PaginatedDocs<
    TDraft extends true
      ? PayloadTypes extends { strictDraftTypes: true }
        ? DraftTransformCollectionWithSelect<TSlug, TSelect>
        : TransformCollectionWithSelect<TSlug, TSelect>
      : TransformCollectionWithSelect<TSlug, TSelect>
  >
> {
  const {
    collection: collectionSlug,
    currentDepth,
    depth,
    disableErrors,
    draft = false,
    includeLockStatus,
    joins,
    limit,
    overrideAccess = true,
    page,
    pagination = true,
    populate,
    select,
    showHiddenFields,
    sort,
    trash = false,
    where,
  } = options
Repository: payloadcms/payload
File: packages/payload/src/admin/views/dashboard.ts (TypeScript)

Code before:
import type { PayloadRequest } from '../../index.js'

export enum EntityType {
  collection = 'collections',
  global = 'globals',
}

export type WidgetServerProps = {
  req: PayloadRequest
  widgetData?: Record<string, unknown>
  widgetSlug: string
}


Review comment (security):
I don't remember about `permissions` and `cookies`, but isn't `locale` at least already included in `req`?

Code after:
import type { InitReqResult } from '../../index.js'

export enum EntityType {
  collection = 'collections',
  global = 'globals',
}

export type WidgetServerProps = {
  widgetData?: Record<string, unknown>
  widgetSlug: string
} & Pick<InitReqResult, 'cookies' | 'locale' | 'permissions' | 'req'>
Repository: penpot/penpot
File: frontend/src/app/main/data/workspace/viewport.cljs (ClojureScript)

Code before:
              (update* local)
              (initialize state local)))]

    (ptk/reify ::initialize-viewport
      ptk/UpdateEvent
      (update [_ state]
        (update state :workspace-local
                (fn [local]
                  (setup state local)))))))

(defn calculate-centered-viewbox
  "Updates the viewbox coordinates for a given center position"
  [local position]
  (let [vbox (:vbox local)
        nw   (/ (:width vbox) 2)
        nh   (/ (:height vbox) 2)
        nx   (- (:x position) nw)
        ny   (- (:y position) nh)]
    (update local :vbox assoc :x nx :y ny)))

(defn update-viewport-position-center
  [position]

  (dm/assert!
   "expected a point instance for `position` param"
   (gpt/point? position))

  (ptk/reify ::update-viewport-position-center
    ptk/UpdateEvent
    (update [_ state]
      (update state :workspace-local
              (fn [local]
                (calculate-centered-viewbox local position))))))

(defn update-viewport-position
  [{:keys [x y] :or {x identity y identity}}]

  (dm/assert!
   "expected function for `x`"
   (fn? x))
  (dm/assert!
   "expected function for `y`"
   (fn? y))

  (ptk/reify ::update-viewport-position
    ptk/UpdateEvent
    (update [_ state]
      (update-in state [:workspace-local :vbox]
                 (fn [vbox]
                   (-> vbox
                       (update :x x)

Review comment (suggestion):
We are deprecating `dm/assert!`, so consider using that instead:

```clojure
(assert (gpt/point? position) "expected a point instance for `position` param")
```

Code after:
              (update* local)
              (initialize state local)))]

    (ptk/reify ::initialize-viewport
      ptk/UpdateEvent
      (update [_ state]
        (update state :workspace-local
                (fn [local]
                  (setup state local)))))))

(defn calculate-centered-viewbox
  "Updates the viewbox coordinates for a given center position"
  [local position]
  (let [vbox (:vbox local)
        nw   (/ (:width vbox) 2)
        nh   (/ (:height vbox) 2)
        nx   (- (:x position) nw)
        ny   (- (:y position) nh)]
    (update local :vbox assoc :x nx :y ny)))

(defn update-viewport-position-center
  [position]
  (assert (gpt/point? position) "expected a point instance for `position` param")

  (ptk/reify ::update-viewport-position-center
    ptk/UpdateEvent
    (update [_ state]
      (update state :workspace-local calculate-centered-viewbox position))))

(defn update-viewport-position
  [{:keys [x y] :or {x identity y identity}}]

  (dm/assert!
   "expected function for `x`"
   (fn? x))
  (dm/assert!
   "expected function for `y`"
   (fn? y))

  (ptk/reify ::update-viewport-position
    ptk/UpdateEvent
    (update [_ state]
      (update-in state [:workspace-local :vbox]
                 (fn [vbox]
                   (-> vbox
                       (update :x x)
                       (update :y y)))))))

(defn update-viewport-size
  [resize-type {:keys [width height] :as size}]
  (ptk/reify ::update-viewport-size
Repository: penpot/penpot
File: frontend/src/app/main/data/workspace/tokens/typography.cljs (ClojureScript)

Code before:
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS INC

(ns app.main.data.workspace.tokens.typography
  (:require
   [potok.v2.core :as ptk]))

(defn set-base-font-size
  [base-font-size]
  (ptk/reify ::set-base-font-size
    ptk/UpdateEvent
    (update [_ state]
      (let [file-id (:current-file-id state)]
        (assoc-in state [:files file-id :data :options :base-font-size] base-font-size)))))

Review comment (suggestion):
I would add in `common.types.file` a function `set-base-font-size`, and leave here
```clojure
(update-in state [:files file-id :data] ctf/set-base-font-size base-font-size)
```

Code after:
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS INC

(ns app.main.data.workspace.tokens.typography
  (:require
   [app.common.data.macros :as dm]
   [app.common.files.changes-builder :as pcb]
   [app.main.data.changes :as dch]
   [beicon.v2.core :as rx]
   [potok.v2.core :as ptk]))

(defn set-base-font-size [base-font-size]
  (ptk/reify ::set-base-font-size
    ptk/WatchEvent
    (watch [it state _]
      (let [file-id (dm/get-in state [:workspace :current-file-id])
            file-data (dm/get-in state [:files file-id :data])
            changes (-> (pcb/empty-changes it)
                        (pcb/with-file-data file-data)
                        (pcb/set-base-font-size base-font-size))]
        (rx/of
         (dch/commit-changes changes))))))
Repository: penpot/penpot
File: frontend/src/app/render_wasm/api/texts.cljs (ClojureScript)

Code before:
   :oriya       #"[\u0B00-\u0B7F]"
   :mongolian   #"[\u1800-\u18AF]"
   :syriac      #"[\u0700-\u074F]"
   :tifinagh    #"[\u2D30-\u2D7F]"
   :coptic      #"[\u2C80-\u2CFF]"
   :ol-chiki    #"[\u1C50-\u1C7F]"
   :vai         #"[\uA500-\uA63F]"
   :shavian     #"[\u10450-\u1047F]"
   :osmanya     #"[\u10480-\u104AF]"
   :runic       #"[\u16A0-\u16FF]"
   :old-italic  #"[\u10300-\u1032F]"
   :brahmi      #"[\u11000-\u1107F]"
   :modi        #"[\u11600-\u1165F]"
   :sora-sompeng #"[\u110D0-\u110FF]"
   :bamum       #"[\uA6A0-\uA6FF]"
   :meroitic    #"[\u10980-\u1099F]"})


(defn contains-emoji? [text]
  (boolean (re-find emoji-pattern text)))

(defn contains-languages [text]
  (->> unicode-ranges
       (filter (fn [[_ pattern]] (re-find pattern text)))
       (map first)
       set))

Review comment (performance):
I think this can be implemented with reduction what has better performance in this case and generates less GC pressure:

```clojure
(defn get-languages [text]
  (reduce-kv (fn [result lang pattern]
               (if (re-find pattern text)
                 (conj result lang)
                 result))
             #{}
             unicode-ranges))
```

Also, i suggest change the name

Code after:
   :oriya       #"[\u0B00-\u0B7F]"
   :mongolian   #"[\u1800-\u18AF]"
   :syriac      #"[\u0700-\u074F]"
   :tifinagh    #"[\u2D30-\u2D7F]"
   :coptic      #"[\u2C80-\u2CFF]"
   :ol-chiki    #"[\u1C50-\u1C7F]"
   :vai         #"[\uA500-\uA63F]"
   :shavian     #"[\u10450-\u1047F]"
   :osmanya     #"[\u10480-\u104AF]"
   :runic       #"[\u16A0-\u16FF]"
   :old-italic  #"[\u10300-\u1032F]"
   :brahmi      #"[\u11000-\u1107F]"
   :modi        #"[\u11600-\u1165F]"
   :sora-sompeng #"[\u110D0-\u110FF]"
   :bamum       #"[\uA6A0-\uA6FF]"
   :meroitic    #"[\u10980-\u1099F]"})


(defn contains-emoji? [text]
  (boolean (re-find emoji-pattern text)))

(defn get-languages [text]
  (reduce-kv (fn [result lang pattern]
               (if (re-find pattern text)
                 (conj result lang)
                 result))
             #{}
             unicode-ranges))
Repository: penpot/penpot
File: frontend/src/app/plugins/local_storage.cljs (ClojureScript)

Code before:
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS INC

(ns app.plugins.local-storage
  (:require
   [app.common.data.macros :as dm]
   [app.common.exceptions :as ex]
   [app.plugins.register :as r]
   [app.plugins.utils :as u]
   [app.util.globals :as g]
   [app.util.object :as obj]
   [cuerdas.core :as str]))

(defonce ^:private local-storage
  (ex/ignoring (unchecked-get g/global "localStorage")))

(defn prefix-key
  [plugin-id key]
  (dm/str plugin-id "/" key))

(defn local-storage-proxy
  [plugin-id]
  (obj/reify {:name "LocalStorageProxy"}
    :$plugin {:enumerable false :get (fn [] plugin-id)}

    :getItem
    (fn [key]
      (cond
        (not (r/check-permission plugin-id "allow:localstorage"))
        (u/display-not-valid :getItem "Plugin doesn't have 'allow:localstorage' permission")

        (not (string? key))
        (u/display-not-valid :getItem "The key must be a string")

        :else
        (.getItem ^js local-storage (prefix-key plugin-id key))))

    :setItem
    (fn [key value]
      (cond
        (not (r/check-permission plugin-id "allow:localstorage"))
        (u/display-not-valid :setItem "Plugin doesn't have 'allow:localstorage' permission")

        (not (string? key))

Review comment (suggestion):
I think you can follow a similar scheme as we already using on penpot. I suggest changing this to:

`(dm/str "penpot-plugins:" plugin-id "/" key)`

Code after:
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS INC

(ns app.plugins.local-storage
  (:require
   [app.common.data.macros :as dm]
   [app.common.exceptions :as ex]
   [app.plugins.register :as r]
   [app.plugins.utils :as u]
   [app.util.globals :as g]
   [app.util.object :as obj]
   [cuerdas.core :as str]))

(defonce ^:private local-storage
  (ex/ignoring (unchecked-get g/global "localStorage")))

(defn prefix-key
  [plugin-id key]
  (dm/str "penpot-plugins:" plugin-id "/" key))

(defn local-storage-proxy
  [plugin-id]
  (obj/reify {:name "LocalStorageProxy"}
    :$plugin {:enumerable false :get (fn [] plugin-id)}

    :getItem
    (fn [key]
      (cond
        (not (r/check-permission plugin-id "allow:localstorage"))
        (u/display-not-valid :getItem "Plugin doesn't have 'allow:localstorage' permission")

        (not (string? key))
        (u/display-not-valid :getItem "The key must be a string")

        :else
        (.getItem ^js local-storage (prefix-key plugin-id key))))

    :setItem
    (fn [key value]
      (cond
        (not (r/check-permission plugin-id "allow:localstorage"))
        (u/display-not-valid :setItem "Plugin doesn't have 'allow:localstorage' permission")

        (not (string? key))
Repository: penpot/penpot
File: render-wasm/src/render.rs (Rust)

Code before:
    active: bool,
}

impl FocusMode {
    pub fn new() -> Self {
        FocusMode {
            shapes: Vec::new(),
            active: false,
        }
    }

    pub fn clear(&mut self) {
        self.shapes.clear();
        self.active = false;
    }

    pub fn set_shapes(&mut self, shapes: Vec<Uuid>) {
        self.shapes = shapes;
        self.active = false;
    }

    /// Returns `true` if the given shape ID should be focused.
    /// If the `shapes` list is empty, focus applies to all shapes.
    pub fn should_focus(&self, id: &Uuid) -> bool {
        self.shapes.is_empty() || self.shapes.contains(id)
    }

    pub fn enter(&mut self, id: &Uuid) {
        if !self.active && self.should_focus(id) {
            self.active = true;
        }
    }

    pub fn exit(&mut self, id: &Uuid) {
        if self.active && self.should_focus(id) {
            self.active = false;
        }
    }

    pub fn is_active(&self) -> bool {
        self.active
    }

    pub fn reset(&mut self) {
        self.active = false;
    }
}

pub(crate) struct RenderState {
    gpu_state: GpuState,
    pub options: RenderOptions,

Review comment (suggestion):
This means that even the focus mode has not been "activated", at the beginning the focus active value is `true`, I guess this is ok, but just adding the note to take it into account

Code after:
///   render contexts.
/// - `is_active()` returns whether the current shape is being rendered in focus.
pub struct FocusMode {
    shapes: Vec<Uuid>,
    active: bool,
}

impl FocusMode {
    pub fn new() -> Self {
        FocusMode {
            shapes: Vec::new(),
            active: false,
        }
    }

    pub fn clear(&mut self) {
        self.shapes.clear();
        self.active = false;
    }

    pub fn set_shapes(&mut self, shapes: Vec<Uuid>) {
        self.shapes = shapes;
    }

    /// Returns `true` if the given shape ID should be focused.
    /// If the `shapes` list is empty, focus applies to all shapes.
    pub fn should_focus(&self, id: &Uuid) -> bool {
        self.shapes.is_empty() || self.shapes.contains(id)
    }

    pub fn enter(&mut self, id: &Uuid) {
        if !self.active && self.should_focus(id) {
            self.active = true;
        }
    }

    pub fn exit(&mut self, id: &Uuid) {
        if self.active && self.should_focus(id) {
            self.active = false;
        }
    }

    pub fn is_active(&self) -> bool {
        self.active
    }

    pub fn reset(&mut self) {
        self.active = false;
    }
}