ronantakizawa/codereview-bench huggingface dataset:
Repository: Mintplex-Labs/anything-llm
File: frontend/src/LogoContext.jsx (JavaScript)

Code before:

export function LogoProvider({ children }) {
  const [logo, setLogo] = useState("");
  const [loginLogo, setLoginLogo] = useState("");
  const [isCustomLogo, setIsCustomLogo] = useState(false);
  const DefaultLoginLogo = isLightMode()
    ? DefaultLoginLogoDark
    : DefaultLoginLogoLight;

  async function fetchInstanceLogo() {
    try {
      const { isCustomLogo, logoURL } = await System.fetchLogo();
      if (logoURL) {
        setLogo(logoURL);
        setLoginLogo(isCustomLogo ? logoURL : DefaultLoginLogo);
        setIsCustomLogo(isCustomLogo);
      } else {
        isLightMode() ? setLogo(AnythingLLMDark) : setLogo(AnythingLLM);
        setLoginLogo(DefaultLoginLogo);
        setIsCustomLogo(false);
      }
    } catch (err) {
      isLightMode() ? setLogo(AnythingLLMDark) : setLogo(AnythingLLM);
      setLoginLogo(DefaultLoginLogo);
      setIsCustomLogo(false);
      console.error("Failed to fetch logo:", err);
    }
  }

  useEffect(() => {
    fetchInstanceLogo();
    window.addEventListener(REFETCH_LOGO_EVENT, fetchInstanceLogo);
    return () => {
      window.removeEventListener(REFETCH_LOGO_EVENT, fetchInstanceLogo);
    };
  }, []);

  return (
    <LogoContext.Provider value={{ logo, setLogo, loginLogo, isCustomLogo }}>
      {children}
    </LogoContext.Provider>
  );
}


Review comment (suggestion):
There's an issue here with the light/dark mode logos. On master, it currently switches back and forth on the login screen between the light and dark AnythingLLM logos (switches after theme is switched then a page refresh). When testing this PR, the logo always displayed the dark mode logo.

<img width="638" height="671" alt="Image" src="https://github.com/user-attachments/assets/9f78d343-c264-4e80-8969-cd86a00e9cd2" />

Code after:

export function LogoProvider({ children }) {
  const [logo, setLogo] = useState("");
  const [loginLogo, setLoginLogo] = useState("");
  const [isCustomLogo, setIsCustomLogo] = useState(false);

  async function fetchInstanceLogo() {
    const DefaultLoginLogo = isLightMode()
      ? DefaultLoginLogoDark
      : DefaultLoginLogoLight;
    try {
      const { isCustomLogo, logoURL } = await System.fetchLogo();
      if (logoURL) {
        setLogo(logoURL);
        setLoginLogo(isCustomLogo ? logoURL : DefaultLoginLogo);
        setIsCustomLogo(isCustomLogo);
      } else {
        isLightMode() ? setLogo(AnythingLLMDark) : setLogo(AnythingLLM);
        setLoginLogo(DefaultLoginLogo);
        setIsCustomLogo(false);
      }
    } catch (err) {
      isLightMode() ? setLogo(AnythingLLMDark) : setLogo(AnythingLLM);
      setLoginLogo(DefaultLoginLogo);
      setIsCustomLogo(false);
      console.error("Failed to fetch logo:", err);
    }
  }

  useEffect(() => {
    fetchInstanceLogo();
    window.addEventListener(REFETCH_LOGO_EVENT, fetchInstanceLogo);
    return () => {
      window.removeEventListener(REFETCH_LOGO_EVENT, fetchInstanceLogo);
    };
  }, []);

  return (
    <LogoContext.Provider value={{ logo, setLogo, loginLogo, isCustomLogo }}>
      {children}
    </LogoContext.Provider>
  );
}
Repository: NVlabs/instant-ngp
File: include/neural-graphics-primitives/common.h (C/C++)

Code before:
#	define NGP_PRAGMA_UNROLL _Pragma("unroll")
#	define NGP_PRAGMA_NO_UNROLL _Pragma("unroll 1")
#else
#	define NGP_PRAGMA_UNROLL
#	define NGP_PRAGMA_NO_UNROLL
#endif

#if defined(__CUDACC__) || (defined(__clang__) && defined(__CUDA__))
#	define NGP_HOST_DEVICE __host__ __device__
#else
#	define NGP_HOST_DEVICE
#endif

namespace ngp {

enum class ETrainMode : int {
	NeRF,
	RFL,
	RFLrelax,
};
static constexpr const char* TrainModeStr = "NeRF\0RFL\0RFLrelax\0\0";

enum class EMeshRenderMode : int {
	Off,
	VertexColors,
	VertexNormals,
	FaceIDs,
};

enum class EGroundTruthRenderMode : int {
	Shade,
	Depth,
	NumRenderModes,
};
static constexpr const char* GroundTruthRenderModeStr = "Shade\0Depth\0\0";

enum class ERenderMode : int {
	AO,
	Shade,
	Normals,
	Positions,
	Depth,
	Distortion,
	Cost,
	Slice,
	NumRenderModes,
	EncodingVis, // EncodingVis exists outside of the standard render modes
};
static constexpr const char* RenderModeStr = "AO\0Shade\0Normals\0Positions\0Depth\0Distortion\0Cost\0Slice\0\0";

enum class EPmVizMode : int {

Review comment (suggestion):
In this codebase, PascalCase / camelCase is generally applied to acronyms, so let's use `Nerf`, `Rfl`, and `RflRelaxed` here (and in the python bindings).

Probably worth putting a link to your paper into a comment above this enum to explain what RFL means and where it comes from.

Code after:
#	define NGP_PRAGMA_UNROLL _Pragma("unroll")
#	define NGP_PRAGMA_NO_UNROLL _Pragma("unroll 1")
#else
#	define NGP_PRAGMA_UNROLL
#	define NGP_PRAGMA_NO_UNROLL
#endif

#if defined(__CUDACC__) || (defined(__clang__) && defined(__CUDA__))
#	define NGP_HOST_DEVICE __host__ __device__
#else
#	define NGP_HOST_DEVICE
#endif

namespace ngp {

// Training modes.
// - NeRF: Standard volumetric reconstruction approach
// - RFL (Radiance Field Loss): Promotes surface-like representations
// - RFL-Relaxed: Hybrid approach that maintains NeRF-like volumetric properties while
//   encouraging surface formation, resulting in faster rendering
// For technical details, see: https://rgl.epfl.ch/publications/Zhang2025Radiance
enum class ETrainMode : int {
	Nerf,
	Rfl,
	RflRelax,
};
static constexpr const char* TrainModeStr = "Nerf\0Rfl\0RflRelax\0\0";

enum class EMeshRenderMode : int {
	Off,
	VertexColors,
	VertexNormals,
	FaceIDs,
};

enum class EGroundTruthRenderMode : int {
	Shade,
	Depth,
	NumRenderModes,
};
static constexpr const char* GroundTruthRenderModeStr = "Shade\0Depth\0\0";

enum class ERenderMode : int {
	AO,
	Shade,
	Normals,
	Positions,
	Depth,
	Distortion,
	Cost,
	Slice,
Repository: Physical-Intelligence/openpi
File: src/openpi/training/config.py (Python)

Code before:
import dataclasses

import openpi.training.optimizer as _optimizer


@dataclasses.dataclass
class TrainConfig:
    keep_interval: int = 5000
    lr_schedule: str | _optimizer.ScheduleProvider = "cosine_decay"
    optimizer: str | _optimizer.OptimizerProvider = "adamw"
    load_pretrained_weights: str | None = None
    checkpoint_dir: str = "/tmp/openpi/checkpoints"
    seed: int = 42
    batch_size: int = 16
    num_train_steps: int = 2_000_000
    log_interval: int = 100
    save_interval: int = 1000


Review comment (suggestion):
I don't think the Protocol types will play nicely with tyro -- trying to keep the config as serializable/command-line-overridable as possible (which is why I had dataclass types here before). Although, excessive nesting is also bad, so not sure of a better solution. The problem with having string only is that then you can't override sub-properties, like, e.g., `--config.lr_schedule.peak_lr`.

Code after:
import dataclasses
from typing import Annotated, Union

import tyro

import openpi.training.optimizer as _optimizer


@dataclasses.dataclass(frozen=True)
class TrainConfig:
    keep_interval: int = 5000
    lr_schedule: Annotated[_optimizer.ScheduleFactory, tyro.conf.Suppress] = _optimizer.cosine_decay_schedule
    optimizer: Annotated[_optimizer.OptimizerFactory, tyro.conf.Suppress] = _optimizer.adamw
    load_pretrained_weights: str | None = None
    checkpoint_dir: str = "/tmp/openpi/checkpoints"
    seed: int = 42
    batch_size: int = 16
    num_train_steps: int = 2_000_000
    log_interval: int = 100
    save_interval: int = 1000

    override: bool = False
    resume: bool = False


_CONFIGS = {
    "default": TrainConfig(),
    "large": TrainConfig(batch_size=128),
}


def cli() -> TrainConfig:
    return tyro.cli(
        Union.__getitem__(  # type: ignore
Repository: Physical-Intelligence/openpi
File: packages/openpi-client/src/openpi_client/base_policy.py (Python)

Code before:
import abc
from typing import Dict


class BasePolicy(abc.ABC):
    @abc.abstractmethod
    def infer(self, obs: Dict) -> Dict:
        """Infer actions from observations."""

    @abc.abstractmethod
    def reset(self) -> None:
        """Reset the policy to its initial state."""


Review comment (refactor):
I think `BasePolicy` is also used in the `openpi` package, so lets have this new method not be abstract, and instead have a default implementation that just `pass`es.

Code after:
import abc
from typing import Dict


class BasePolicy(abc.ABC):
    @abc.abstractmethod
    def infer(self, obs: Dict) -> Dict:
        """Infer actions from observations."""

    def reset(self) -> None:
        """Reset the policy to its initial state."""
        pass
Repository: PixiEditor/PixiEditor
File: PixiEditor/Models/Controllers/BitmapManager.cs (C#)

Code before:
                RaisePropertyChanged("SelectedTool");
            }
        }

        public Layer PreviewLayer
        {
            get => _previewLayer;
            set
            {
                _previewLayer = value;
                RaisePropertyChanged("PreviewLayer");
            }
        }

        public Layer ActiveLayer => ActiveDocument.ActiveLayer;

        public Color PrimaryColor { get; set; }

        public int ToolSize
        {
            get => SelectedTool.Toolbar.GetSetting("ToolSize") != null
            ? (int)SelectedTool.Toolbar.GetSetting("ToolSize").Value
            : 1;
            set
            {
                SelectedTool.Toolbar.GetSetting("ToolSize").Value = value;
            }
        }

        public BitmapOperationsUtility BitmapOperations { get; set; }
        public ReadonlyToolUtility ReadonlyToolUtility { get; set; }

        public Document ActiveDocument
        {
            get => _activeDocument;
            set
            {
                _activeDocument = value;
                RaisePropertyChanged("ActiveDocument");
                DocumentChanged?.Invoke(this, new DocumentChangedEventArgs(value));
            }
        }

        private Document _activeDocument;

        private Layer _previewLayer;
        private Tool _selectedTool;

        public BitmapManager()
        {
            MouseController = new MouseMovementController();

Review comment (suggestion):
Using shortcuts on the tool, which doesn't have `ToolSize` raises an error, you should add null checking on the setter.

Code after:
                _selectedTool = value;
                RaisePropertyChanged("SelectedTool");
            }
        }

        public Layer PreviewLayer
        {
            get => _previewLayer;
            set
            {
                _previewLayer = value;
                RaisePropertyChanged("PreviewLayer");
            }
        }

        public Layer ActiveLayer => ActiveDocument.ActiveLayer;

        public Color PrimaryColor { get; set; }

        public int ToolSize
        {
            get => SelectedTool.Toolbar.GetSetting("ToolSize") != null
            ? (int)SelectedTool.Toolbar.GetSetting("ToolSize").Value
            : 1;
            set
            {
                if (SelectedTool.Toolbar.GetSetting("ToolSize") is Setting toolSize)
                {
                    toolSize.Value = value;
                    HighlightPixels(MousePositionConverter.CurrentCoordinates);
                }
            }
        }

        public BitmapOperationsUtility BitmapOperations { get; set; }
        public ReadonlyToolUtility ReadonlyToolUtility { get; set; }

        public Document ActiveDocument
        {
            get => _activeDocument;
            set
            {
                _activeDocument = value;
                RaisePropertyChanged("ActiveDocument");
                DocumentChanged?.Invoke(this, new DocumentChangedEventArgs(value));
            }
        }

        private Document _activeDocument;

        private Layer _previewLayer;
Repository: PixiEditor/PixiEditor
File: PixiEditor/Models/Tools/Tool.cs (C#)

Code before:
﻿using System;
using System.Windows.Input;
using PixiEditor.Helpers;
using PixiEditor.Models.Controllers;
using PixiEditor.Models.Tools.ToolSettings;
using PixiEditor.Models.Tools.ToolSettings.Toolbars;

namespace PixiEditor.Models.Tools
{
    public abstract class Tool : NotifyableObject
    {
        private bool isActive;
        private string actionDisplay = string.Empty;

        public string Name => GetType().Name.Replace("Tool", string.Empty);

        public Type Type => GetType();

        public string ImagePath => $"/Images/{Name}Image.png";

        public bool HideHighlight { get; set; } = false;

        public string Tooltip { get; set; }

        public string ActionDisplay
        {
            get => actionDisplay;
            set
            {
                actionDisplay = value;
                RaisePropertyChanged("ActionDisplay");
            }
        }

        public bool IsActive
        {
            get => isActive;
            set
            {
                isActive = value;

Review comment (suggestion):
This shouldn't be a getter, instead of `Name =>` it should be `Name = `, because maybe someone would want to change the name in the constructor

Code after:
﻿using System;
using System.Windows.Input;
using PixiEditor.Helpers;
using PixiEditor.Models.Controllers;
using PixiEditor.Models.Tools.ToolSettings;
using PixiEditor.Models.Tools.ToolSettings.Toolbars;

namespace PixiEditor.Models.Tools
{
    public abstract class Tool : NotifyableObject
    {
        protected string name;
        private bool isActive;
        private string actionDisplay = string.Empty;

        public string ToolName
        {
            get
            {
                if (string.IsNullOrWhiteSpace(name))
                {
                    return GetType().Name.Replace("Tool", string.Empty);
                }

                return name;
            }
        }

        public string ImagePath => $"/Images/{ToolName}Image.png";

        public bool HideHighlight { get; set; } = false;

        public string Tooltip { get; set; }

        public string ActionDisplay
        {
            get => actionDisplay;
            set
            {
                actionDisplay = value;
Repository: PixiEditor/PixiEditor
File: PixiEditor/Models/Tools/BitmapOperationTool.cs (C#)

Code before:
﻿using System;
using System.Collections.Generic;
using System.Windows.Documents;
using System.Windows.Media;
using PixiEditor.Models.DataHolders;
using PixiEditor.Models.Layers;
using PixiEditor.Models.Position;

namespace PixiEditor.Models.Tools
{
    public abstract class BitmapOperationTool : Tool
    {
        public bool RequiresPreviewLayer { get; set; }

        public bool ClearPreviewLayerOnEachIteration { get; set; } = true;

        public bool UseDefaultUndoMethod { get; set; } = true;
        public virtual bool UsesShift { get => true; }

        private readonly LayerChange[] onlyLayerArr = new LayerChange[] { new LayerChange(BitmapPixelChanges.Empty, Guid.Empty) };

        public abstract LayerChange[] Use(Layer layer, List<Coordinates> mouseMove, Color color);

        protected LayerChange[] Only(BitmapPixelChanges changes, Layer layer)
        {
            onlyLayerArr[0] = new LayerChange(changes, layer);
            return onlyLayerArr;
        }

        protected LayerChange[] Only(BitmapPixelChanges changes, Guid layerGuid)
        {
            onlyLayerArr[0] = new LayerChange(changes, layerGuid);
            return onlyLayerArr;
        }
    }
}

Review comment (suggestion):
You can simply use this

```suggestion
        public virtual bool UsesShift => true;
```


Code after:
﻿using System;
using System.Collections.Generic;
using System.Windows.Documents;
using System.Windows.Media;
using PixiEditor.Models.DataHolders;
using PixiEditor.Models.Layers;
using PixiEditor.Models.Position;

namespace PixiEditor.Models.Tools
{
    public abstract class BitmapOperationTool : Tool
    {
        public bool RequiresPreviewLayer { get; set; }

        public bool ClearPreviewLayerOnEachIteration { get; set; } = true;

        public bool UseDefaultUndoMethod { get; set; } = true;
        public virtual bool UsesShift => true;

        private readonly LayerChange[] onlyLayerArr = new LayerChange[] { new LayerChange(BitmapPixelChanges.Empty, Guid.Empty) };

        public abstract LayerChange[] Use(Layer layer, List<Coordinates> mouseMove, Color color);

        protected LayerChange[] Only(BitmapPixelChanges changes, Layer layer)
        {
            onlyLayerArr[0] = new LayerChange(changes, layer);
            return onlyLayerArr;
        }

        protected LayerChange[] Only(BitmapPixelChanges changes, Guid layerGuid)
        {
            onlyLayerArr[0] = new LayerChange(changes, layerGuid);
            return onlyLayerArr;
        }
    }
}
Repository: PixiEditor/PixiEditor
File: PixiEditor/App.xaml.cs (C#)

Code before:
﻿using PixiEditor.Models.Dialogs;
using PixiEditor.Models.Enums;
using PixiEditor.ViewModels;
using System.Linq;
using System.Windows;

namespace PixiEditor
{
    /// <summary>
    ///     Interaction logic for App.xaml.
    /// </summary>
    public partial class App : Application
    {
        protected override void OnSessionEnding(SessionEndingCancelEventArgs e)
        {
            base.OnSessionEnding(e);

            if (ViewModelMain.Current.BitmapManager.Documents.Any(x => !x.ChangesSaved))
            {
                ConfirmationType confirmation = ConfirmationDialog.Show($"{e.ReasonSessionEnding} with unsaved data. Are you sure?", $"{e.ReasonSessionEnding}");
                e.Cancel = (confirmation == ConfirmationType.Canceled || confirmation == ConfirmationType.No);
            }
        }
    }
}


Review comment (suggestion):
```suggestion
                e.Cancel = confirmation != ConfirmationType.Yes
```

Code after:
﻿using PixiEditor.Models.Dialogs;
using PixiEditor.Models.Enums;
using PixiEditor.ViewModels;
using System.Linq;
using System.Windows;

namespace PixiEditor
{
    /// <summary>
    ///     Interaction logic for App.xaml.
    /// </summary>
    public partial class App : Application
    {
        protected override void OnSessionEnding(SessionEndingCancelEventArgs e)
        {
            base.OnSessionEnding(e);

            if (ViewModelMain.Current.BitmapManager.Documents.Any(x => !x.ChangesSaved))
            {
                ConfirmationType confirmation = ConfirmationDialog.Show($"{e.ReasonSessionEnding} with unsaved data. Are you sure?", $"{e.ReasonSessionEnding}");
                e.Cancel = confirmation != ConfirmationType.Yes;
            }
        }
    }
}
Repository: PixiEditor/PixiEditor
File: src/PixiEditor/Helpers/VersionHelpers.cs (C#)

Code before:
    {
        StringBuilder builder = new(GetCurrentAssemblyVersion().ToString());

        bool isDone = false;

        AppendDevBuild(builder, ref isDone);
        if (isDone)
            return builder.ToString();

        AppendMsixDebugBuild(builder, ref isDone);
        if (isDone)
            return builder.ToString();

        AppendDebugBuild(builder, ref isDone);
        if (isDone || !moreSpecific)
            return builder.ToString();

        AppendSteamBuild(builder, ref isDone);
        if (isDone)
            return builder.ToString();

        AppendMsixBuild(builder, ref isDone);
        if (isDone)
            return builder.ToString();

        AppendReleaseBuild(builder, ref isDone);
        return builder.ToString();
    }

    [Conditional("DEVRELEASE")]
    private static void AppendDevBuild(StringBuilder builder, ref bool done)
    {
        done = true;

        builder.Append(" Dev Build");
    }

    [Conditional("DEBUG")]
    private static void AppendDebugBuild(StringBuilder builder, ref bool done)
    {
        done = true;

        builder.Append(" Debug Build");
    }

    [Conditional("RELEASE")]
    private static void AppendReleaseBuild(StringBuilder builder, ref bool done)
    {
        done = true;

        builder.Append(" Release Build");

Review comment (suggestion):
Wouldn't it be simpler to just call one method and put preprocessors inside:

```csharp
private static void AppendBuild(StringBuilder builder)
{
    string str = "";
#if STEAM
    str = " Steam Build"
#elif MSIX
   str = " MSIX Build"

...etc
}
```

Code after:
    {
        StringBuilder builder = new(GetCurrentAssemblyVersion().ToString());

#if DEVRELEASE
        builder.Append(" Dev Build");
        return builder.ToString();
#elif MSIX_DEBUG
        builder.Append(" MSIX Debug Build");
        return builder.ToString();
#elif DEBUG
        builder.Append(" Debug Build");
        return builder.ToString();
#endif

        if (!moreSpecific)
            return builder.ToString();

#if STEAM
        builder.Append(" Steam Build");
#elif MSIX
        builder.Append(" MSIX Build");
#elif RELEASE
        builder.Append(" Release Build");
#endif
        return builder.ToString();
    }
}
Repository: PixiEditor/PixiEditor
File: src/PixiEditor/Models/DocumentModels/UpdateableChangeExecutors/MagicWandToolExecutor.cs (C#)

Code before:

namespace PixiEditor.Models.DocumentModels.UpdateableChangeExecutors;

internal class MagicWandToolExecutor : UpdateableChangeExecutor
{
    private bool considerAllLayers;
    private bool drawOnMask;
    private List<Guid> memberGuids;
    private SelectionMode mode;

    public override ExecutionState Start()
    {
        var magicWand = ViewModelMain.Current?.ToolsSubViewModel.GetTool<MagicWandToolViewModel>();
        var members = document!.ExtractSelectedLayers(true);

        if (magicWand is null || members.Count == 0)
            return ExecutionState.Error;

        mode = magicWand.SelectMode;
        memberGuids = members;
        considerAllLayers = magicWand.DocumentScope == DocumentScope.AllLayers;
        if (considerAllLayers)
            memberGuids = document!.StructureHelper.GetAllLayers().Select(x => x.GuidValue).ToList();
        var pos = controller!.LastPixelPosition;

        internals!.ActionAccumulator.AddActions(new MagicWand_Action(memberGuids, pos, mode, considerAllLayers));

        return ExecutionState.Success;
    }

    public override void OnLeftMouseButtonUp()
    {
        internals!.ActionAccumulator.AddActions(new ChangeBoundary_Action());
        onEnded!(this);
    }

    public override void ForceStop()
    {
        internals!.ActionAccumulator.AddActions(new ChangeBoundary_Action());
    }
}


Review comment (bug):
you are passing considerAllLayers as the drawOnMask argument (which is completely unused so this isn't a bug, it's just weird)

Code after:

namespace PixiEditor.Models.DocumentModels.UpdateableChangeExecutors;

internal class MagicWandToolExecutor : UpdateableChangeExecutor
{
    private bool considerAllLayers;
    private bool drawOnMask;
    private List<Guid> memberGuids;
    private SelectionMode mode;

    public override ExecutionState Start()
    {
        var magicWand = ViewModelMain.Current?.ToolsSubViewModel.GetTool<MagicWandToolViewModel>();
        var members = document!.ExtractSelectedLayers(true);

        if (magicWand is null || members.Count == 0)
            return ExecutionState.Error;

        mode = magicWand.SelectMode;
        memberGuids = members;
        considerAllLayers = magicWand.DocumentScope == DocumentScope.AllLayers;
        if (considerAllLayers)
            memberGuids = document!.StructureHelper.GetAllLayers().Select(x => x.GuidValue).ToList();
        var pos = controller!.LastPixelPosition;

        internals!.ActionAccumulator.AddActions(new MagicWand_Action(memberGuids, pos, mode));

        return ExecutionState.Success;
    }

    public override void OnLeftMouseButtonUp()
    {
        internals!.ActionAccumulator.AddActions(new ChangeBoundary_Action());
        onEnded!(this);
    }

    public override void ForceStop()
    {
        internals!.ActionAccumulator.AddActions(new ChangeBoundary_Action());
    }
}