ronantakizawa/codereview-bench huggingface dataset:
Repository: playcanvas/engine
File: scripts/esm/first-person-controller.mjs (JavaScript)

Code before:
     * @attribute
     * @title Velocity Damping Ground
     * @description The damping applied to the velocity when on the ground.
     * @type {number}
     */
    velocityDampingGround = 0.99;

    /**
     * @attribute
     * @title Velocity Damping Air
     * @description The damping applied to the velocity when in the air.
     * @type {number}
     */
    velocityDampingAir = 0.99925;

    /**
     * @attribute
     * @title Jump Force
     * @description The force applied when jumping.
     * @type {number}
     */
    jumpForce = 600;

    /**
     * The joystick event name for the UI position for the base and stick elements.
     * The event name is appended with the side: 'left' or 'right'.
     *
     * @attribute
     * @title Joystick Base Event Name
     * @type {string}
     */
    joystickEventName = 'joystick';

    initialize() {
        // check camera
        if (!this._camera) {
            throw new Error('FirstPersonController requires a camera component');
        }

        // check collision and rigidbody
        if (!this.entity.collision) {
            this.entity.addComponent('collision', {
                type: 'capsule',
                radius: 0.5,
                height: 2
            });
        }
        if (!this.entity.rigidbody) {
            this.entity.addComponent('rigidbody', {
                type: 'dynamic',
                mass: 100,

Review comment (suggestion):
```suggestion
     * The event name is appended with the side: ':left' or ':right'.
```

Code after:
     * @attribute
     * @title Velocity Damping Ground
     * @description The damping applied to the velocity when on the ground.
     * @type {number}
     */
    velocityDampingGround = 0.99;

    /**
     * @attribute
     * @title Velocity Damping Air
     * @description The damping applied to the velocity when in the air.
     * @type {number}
     */
    velocityDampingAir = 0.99925;

    /**
     * @attribute
     * @title Jump Force
     * @description The force applied when jumping.
     * @type {number}
     */
    jumpForce = 600;

    /**
     * The joystick event name for the UI position for the base and stick elements.
     * The event name is appended with the side: ':left' or ':right'.
     *
     * @attribute
     * @title Joystick Base Event Name
     * @type {string}
     */
    joystickEventName = 'joystick';

    initialize() {
        // check camera
        if (!this._camera) {
            throw new Error('FirstPersonController requires a camera component');
        }

        // check collision and rigidbody
        if (!this.entity.collision) {
            this.entity.addComponent('collision', {
                type: 'capsule',
                radius: 0.5,
                height: 2
            });
        }
        if (!this.entity.rigidbody) {
            this.entity.addComponent('rigidbody', {
                type: 'dynamic',
                mass: 100,
Repository: playcanvas/engine
File: src/scene/gsplat-unified/gsplat-unified-sorter.js (JavaScript)

Code before:
        // reuse order data
        if (orderData.length === this.bufferLength) {
            this.availableOrderData.push(orderData);
        }
    }

    destroy() {
        this._destroyed = true;
        this.worker.terminate();
        this.worker = null;
    }

    /**
     * Adds or removes centers from the sorter.
     *
     * @param {number} id - The id of the centers.
     * @param {Float32Array|null} centers - The centers buffer.
     */
    setCenters(id, centers) {

        if (centers) { // add

            if (!this.centersSet.has(id)) {
                this.centersSet.add(id);

                // clone centers buffer - requireed when mutliple workers sort the same splat resource
                const centersBuffer = centers.buffer.slice();

                // post centers to worker
                this.worker.postMessage({
                    command: 'addCenters',
                    id: id,
                    centers: centersBuffer
                }, [centersBuffer]);
            }

        } else { // remove

            if (this.centersSet.has(id)) {
                this.centersSet.delete(id);

                // post centers to worker
                this.worker.postMessage({
                    command: 'removeCenters',
                    id: id
                });
            }
        }
    }

    /**

Review comment (suggestion):
```suggestion
                // clone centers buffer - required when multiple workers sort the same splat resource
```

Code after:
        // reuse order data
        if (orderData.length === this.bufferLength) {
            this.availableOrderData.push(orderData);
        }
    }

    destroy() {
        this._destroyed = true;
        this.worker.terminate();
        this.worker = null;
    }

    /**
     * Adds or removes centers from the sorter.
     *
     * @param {number} id - The id of the centers.
     * @param {Float32Array|null} centers - The centers buffer.
     */
    setCenters(id, centers) {

        if (centers) { // add

            if (!this.centersSet.has(id)) {
                this.centersSet.add(id);

                // clone centers buffer - required when multiple workers sort the same splat resource
                const centersBuffer = centers.buffer.slice();

                // post centers to worker
                this.worker.postMessage({
                    command: 'addCenters',
                    id: id,
                    centers: centersBuffer
                }, [centersBuffer]);
            }

        } else { // remove

            if (this.centersSet.has(id)) {
                this.centersSet.delete(id);

                // post centers to worker
                this.worker.postMessage({
                    command: 'removeCenters',
                    id: id
                });
            }
        }
    }

    /**
Repository: playcanvas/engine
File: scripts/esm/blurred-planar-reflection.mjs (JavaScript)

Code before:
     * - Higher values = sharper/quicker fade to white
     * - Lower values = more gradual fade
     * - fadeStrength = 1: linear fade
     *
     * @attribute
     * @range [0.1, 5]
     * @precision 2
     * @step 0.1
     */
    fadeStrength = 1.0;

    /**
     * Controls how reflection fades based on viewing angle.
     * Higher values = more fade when looking straight down, stronger reflection at grazing angles.
     *
     * @attribute
     * @range [0.1, 1]
     * @precision 2
     * @step 0.1
     */
    angleFade = 0.5;

    /**
     * Height range for distance-based blur calculation.
     *
     * @attribute
     */
    heightRange = 10.0;

    /**
     * Background color to fade reflections into.
     *
     * @attribute
     */
    fadeColor = new Color(0.5, 0.5, 0.5, 1);

    /**
     * @private
     */
    _sceneColorMap = false;

    /**
     * Enable scene color map on reflection cameras. Required for materials with
     * refraction/transmission effects (e.g. glass lenses).
     *
     * @attribute
     * @type {boolean}
     */
    set sceneColorMap(value) {
        this._sceneColorMap = value;
        // Apply to existing cameras if they exist

Review comment (suggestion):
What about this to disallow negative numbers:
```js
    /**
     * Height range for distance-based blur calculation.
     *
     * @attribute
     * @range [0]
     */
    heightRange = 10;
```

Code after:
     * - Higher values = sharper/quicker fade to white
     * - Lower values = more gradual fade
     * - fadeStrength = 1: linear fade
     *
     * @attribute
     * @range [0.1, 5]
     * @precision 2
     * @step 0.1
     */
    fadeStrength = 1.0;

    /**
     * Controls how reflection fades based on viewing angle.
     * Higher values = more fade when looking straight down, stronger reflection at grazing angles.
     *
     * @attribute
     * @range [0.1, 1]
     * @precision 2
     * @step 0.1
     */
    angleFade = 0.5;

    /**
     * Height range for distance-based blur calculation.
     *
     * @attribute
     * @range [0]
     */
    heightRange = 1;

    /**
     * Background color to fade reflections into.
     *
     * @attribute
     */
    fadeColor = new Color(0.5, 0.5, 0.5, 1);

    /**
     * @private
     */
    _sceneColorMap = false;

    /**
     * Enable scene color map on reflection cameras. Required for materials with
     * refraction/transmission effects (e.g. glass lenses).
     *
     * @attribute
     * @type {boolean}
     */
    set sceneColorMap(value) {
        this._sceneColorMap = value;
Repository: playcanvas/engine
File: src/scene/shader-lib/wgsl/chunks/gsplat/vert/gsplatCommon.js (JavaScript)

Code before:
export default /* wgsl */`
#include "gsplatHelpersVS"
#include "gsplatFormatVS"
#include "gsplatStructsVS"
#include "gsplatDeclarationsVS"
#include "gsplatModifyVS"
#include "gsplatEvalSHVS"
#include "gsplatQuatToMat3VS"
#include "gsplatReadVS"
#include "gsplatSourceVS"
#include "gsplatCenterVS"
#include "gsplatCornerVS"
#include "gsplatOutputVS"

// modify the gaussian corner so it excludes gaussian regions with alpha less than 1/255
fn clipCorner(corner: ptr<function, SplatCorner>, alpha: f32) {
    let clip: half = half(min(1.0, sqrt(-log(1.0 / (255.0 * alpha))) / 2.0));
    corner.offset = corner.offset * f32(clip);
    corner.uv = corner.uv * clip;
}
`;


Review comment (suggestion):
should this whole line use half precision, instead of just the result. Currently we just cast result to half and cast it back to float on the next line

Code after:
export default /* wgsl */`
#include "gsplatHelpersVS"
#include "gsplatFormatVS"
#include "gsplatStructsVS"
#include "gsplatDeclarationsVS"
#include "gsplatModifyVS"
#include "gsplatEvalSHVS"
#include "gsplatQuatToMat3VS"
#include "gsplatReadVS"
#include "gsplatSourceVS"
#include "gsplatCenterVS"
#include "gsplatCornerVS"
#include "gsplatOutputVS"

// modify the gaussian corner so it excludes gaussian regions with alpha less than 1/255
fn clipCorner(corner: ptr<function, SplatCorner>, alpha: half) {
    let clip = min(half(1.0), sqrt(log(half(255.0) * alpha)) * half(0.5));
    corner.offset = corner.offset * f32(clip);
    corner.uv = corner.uv * clip;
}
`;
Repository: plotly/dash
File: dash/_utils.py (Python)

Code before:
def interpolate_str(template, **data):
    s = template
    for k, v in data.items():
        key = '{' + k + '}'
        s = s.replace(key, v)
    return s


class AttributeDict(dict):
    """
    Dictionary subclass enabling attribute lookup/assignment of keys/values.

    For example::
        >>> m = AttributeDict({'foo': 'bar'})
        >>> m.foo
        'bar'
        >>> m.foo = 'not bar'
        >>> m['foo']
        'not bar'
    ``AttributeDict`` objects also provide ``.first()`` which acts like
    ``.get()`` but accepts multiple keys as arguments, and returns the value of
    the first hit, e.g.::
        >>> m = AttributeDict({'foo': 'bar', 'biz': 'baz'})
        >>> m.first('wrong', 'incorrect', 'foo', 'biz')
        'bar'
    """

    def __setattr__(self, key, value):
        self[key] = value

Review comment (suggestion):
This is probably too brittle... in modern Javascript you could easily write something like `setConfig({config})` and have someone paste that into `index`

Code after:
def interpolate_str(template, **data):
    s = template
    for k, v in data.items():
        key = '{%' + k + '%}'
        s = s.replace(key, v)
    return s


class AttributeDict(dict):
    """
    Dictionary subclass enabling attribute lookup/assignment of keys/values.

    For example::
        >>> m = AttributeDict({'foo': 'bar'})
        >>> m.foo
        'bar'
        >>> m.foo = 'not bar'
        >>> m['foo']
        'not bar'
    ``AttributeDict`` objects also provide ``.first()`` which acts like
    ``.get()`` but accepts multiple keys as arguments, and returns the value of
    the first hit, e.g.::
        >>> m = AttributeDict({'foo': 'bar', 'biz': 'baz'})
        >>> m.first('wrong', 'incorrect', 'foo', 'biz')
        'bar'
    """

    def __setattr__(self, key, value):
        self[key] = value
Repository: plotly/dash
File: dash-renderer/src/components/core/DocumentTitle.react.js (JavaScript)

Code before:
import {connect} from 'react-redux';
import {Component} from 'react';
import PropTypes from 'prop-types';

class DocumentTitle extends Component {
    constructor(props) {
        super(props);
        const {update_title} = props.config;
        this.state = {
            update_title,
        };
    }

    UNSAFE_componentWillReceiveProps(props) {
        if (this.state.update_title && props.isLoading) {
            document.title = this.state.update_title;
        }
    }

    shouldComponentUpdate() {
        return false;
    }

    render() {
        return null;
    }
}

DocumentTitle.propTypes = {
    isLoading: PropTypes.bool.isRequired,
    config: PropTypes.shape({update_title: PropTypes.string}),
};

export default connect(state => ({
    isLoading: state.isLoading,
    config: state.config,
}))(DocumentTitle);


Review comment (suggestion):
While this allows the clientside callback to set the value, it seems to break existing tests / behavior. Would a better verification be to only revert back to `inititalTitle` if the value is equal to `update_title` instead? Possibly the same chekc for `update_title` - only update if the value is currently `initialTitle`.

Code after:
import {connect} from 'react-redux';
import {Component} from 'react';
import PropTypes from 'prop-types';

class DocumentTitle extends Component {
    constructor(props) {
        super(props);
        const {update_title} = props.config;
        this.state = {
            title: document.title,
            update_title,
        };
    }

    UNSAFE_componentWillReceiveProps(props) {
        if (!this.state.update_title) {
            // Let callbacks or other components have full control over title
            return;
        }
        if (props.isLoading) {
            this.setState({title: document.title});
            if (this.state.update_title) {
                document.title = this.state.update_title;
            }
        } else {
            if (document.title === this.state.update_title) {
                document.title = this.state.title;
            } else {
                this.setState({title: document.title});
            }
        }
    }

    shouldComponentUpdate() {
        return false;
    }

    render() {
        return null;
    }
}

DocumentTitle.propTypes = {
Repository: plotly/dash
File: tests/integration/test_csp.py (Python)

Code before:
import pytest
import flask_talisman
from selenium.common.exceptions import NoSuchElementException

import dash
import dash_core_components as dcc
import dash_html_components as html
from dash.dependencies import Input, Output


@contextlib.contextmanager
def does_not_raise():
    yield


@pytest.mark.parametrize(
    "add_hashes, hash_algorithm, expectation",
    [
        (False, None, pytest.raises(NoSuchElementException)),
        (True, "sha256", does_not_raise()),
        (True, "sha384", does_not_raise()),
        (True, "sha512", does_not_raise()),
        (True, "sha999", pytest.raises(ValueError)),
    ],
)
def test_csp_hashes_inline_scripts(dash_duo, add_hashes, hash_algorithm, expectation):
    app = dash.Dash(__name__)

    app.layout = html.Div(
        [dcc.Input(id="input_element", type="text"), html.Div(id="output_element")]
    )

    app.clientside_callback(
        """
        function(input) {
            return input;
        }
        """,
        Output("output_element", "children"),
        [Input("input_element", "value")],
    )

    with expectation:
        csp = {
            "default-src": "'self'",
            "script-src": ["'self'"]
            + (app.csp_hashes(hash_algorithm) if add_hashes else []),
        }

        flask_talisman.Talisman(
            app.server, content_security_policy=csp, force_https=False

Review comment (suggestion):
```suggestion
def test_incs001_csp_hashes_inline_scripts(dash_duo, add_hashes, hash_algorithm, expectation):
```

Just adding a test case ID for local targeting :)

Code after:
import pytest
import flask_talisman
from selenium.common.exceptions import NoSuchElementException

import dash
import dash_core_components as dcc
import dash_html_components as html
from dash.dependencies import Input, Output


@contextlib.contextmanager
def does_not_raise():
    yield


@pytest.mark.parametrize(
    "add_hashes, hash_algorithm, expectation",
    [
        (False, None, pytest.raises(NoSuchElementException)),
        (True, "sha256", does_not_raise()),
        (True, "sha384", does_not_raise()),
        (True, "sha512", does_not_raise()),
        (True, "sha999", pytest.raises(ValueError)),
    ],
)
def test_incs001_csp_hashes_inline_scripts(
    dash_duo, add_hashes, hash_algorithm, expectation
):
    app = dash.Dash(__name__)

    app.layout = html.Div(
        [dcc.Input(id="input_element", type="text"), html.Div(id="output_element")]
    )

    app.clientside_callback(
        """
        function(input) {
            return input;
        }
        """,
        Output("output_element", "children"),
        [Input("input_element", "value")],
    )

    with expectation:
        csp = {
            "default-src": "'self'",
            "script-src": ["'self'"]
            + (app.csp_hashes(hash_algorithm) if add_hashes else []),
        }
Repository: plotly/dash
File: @plotly/dash-test-components/src/components/MyPersistedComponent.js (JavaScript)

Code before:
        this.onKeyPress = this.onKeyPress.bind(this);
        this.setInputValue = this.setInputValue.bind(this);
        this.setPropValue = this.setPropValue.bind(this);
    }

    UNSAFE_componentWillReceiveProps(nextProps) {
        const {value} = this.input.current;
        const valueAsNumber = convert(value);
        this.setInputValue(
            isNil(valueAsNumber) ? value : valueAsNumber,
            nextProps.value
        );
        if (this.props.type !== 'number') {
            this.setState({value: nextProps.value});
        }
    }

    componentDidMount() {
        const {value} = this.input.current;
        const valueAsNumber = convert(value);
        this.setInputValue(
            isNil(valueAsNumber) ? value : valueAsNumber,
            this.props.value
        );
    }

    UNSAFE_componentWillMount() {
        if (this.props.type !== 'number') {
            this.setState({value: this.props.value});
        }
    }

    render() {
        const valprops =
            this.props.type === 'number' ? {} : {value: this.state.value};
        const {loading_state} = this.props;
        return (
            <input
                data-dash-is-loading={
                    (loading_state && loading_state.is_loading) || undefined
                }
                ref={this.input}
                onChange={this.onChange}
                onKeyPress={this.onKeyPress}
                {...valprops}
                {...omit(
                    [
                        'debounce',
                        'value',
                        'n_submit',
                        'n_submit_timestamp',

Review comment (suggestion):
As we had discussed, this is copied over from `dcc.Input`. A lot of these props are not really useful for the test either. Try and keep the surface area for this component to the minimum you need to do the tests you need, nothing more.

Code after:

    componentDidMount() {
        const {value} = this.input.current;
        this.setInputValue(
            value,
            this.props.value
        );
    }

    UNSAFE_componentWillMount() {
        this.setState({value: this.props.value})
    }

    render() {
        const valprops = {value: this.state.value}
        return (
            <input
                ref={this.input}
                onChange={this.onChange}
                onKeyPress={this.onKeyPress}
                {...valprops}
                {...omit(
                    [
                        'value',
                        'setProps',
                    ],
                    this.props
                )}
            />
        );
    }

    setInputValue(base, value) {
        base = NaN;

        if (!isEquivalent(base, value)) {
            this.input.current.value = value
        }
    }

    setPropValue(base, value) {
        if (!isEquivalent(base, value)) {
            this.props.setProps({value});
        }
    }

    onEvent() {
        const {value} = this.input.current;
        this.props.setProps({value})
    }
Repository: plotly/dash
File: tests/integration/callbacks/test_basic_callback.py (Python)

Code before:
                    ),
                ],
            ),
            html.Button("n", id="n"),
            DelayedEventComponent(id="d"),
            html.Div("Output init", id="output"),
        ]
    )

    @app.callback(
        Output("output", "children"),
        Input("n", "n_clicks"),
        Input("d", "n_clicks"),
        Input("async", "value"),
    )
    def content(n, d, v):
        return json.dumps([n, d, v])

    dash_duo.start_server(app)

    wait.until(lambda: dash_duo.find_element("#output").text == '[null, null, "A"]', 3)
    dash_duo.wait_for_element("#d").click()

    wait.until(
        lambda: dash_duo.find_element("#output").text == '[null, 1, "A"]', 3,
    )

    dash_duo.wait_for_element("#n").click()
    wait.until(
        lambda: dash_duo.find_element("#output").text == '[1, 1, "A"]', 3,
    )

    dash_duo.wait_for_element("#d").click()
    wait.until(
        lambda: dash_duo.find_element("#output").text == '[1, 2, "A"]', 3,
    )


Review comment (suggestion):
Simplified version of the original reproducible example involving the DataTable inside a dcc.Tab and the dcc.Graph, all inside a non-click reactive top-level component -- does not do away with all real components as it would be tedious but used custom made components for the key behavior.

Code after:
    @app.callback(
        Output("output", "children"),
        Input("n", "n_clicks"),
        Input("d", "n_clicks"),
        Input("async", "value"),
    )
    def content(n, d, v):
        return json.dumps([n, d, v])

    dash_duo.start_server(app)

    wait.until(lambda: dash_duo.find_element("#output").text == '[null, null, "A"]', 3)
    dash_duo.wait_for_element("#d").click()

    wait.until(
        lambda: dash_duo.find_element("#output").text == '[null, 1, "A"]', 3,
    )

    dash_duo.wait_for_element("#n").click()
    wait.until(
        lambda: dash_duo.find_element("#output").text == '[1, 1, "A"]', 3,
    )

    dash_duo.wait_for_element("#d").click()
    wait.until(
        lambda: dash_duo.find_element("#output").text == '[1, 2, "A"]', 3,
    )