ronantakizawa/codereview-bench huggingface dataset:
Repository: pixijs/pixijs
File: src/scene/container/__tests__/getGlobalAlpha.test.ts (TypeScript)

Code before:
import { Container } from '../Container';

describe('getGlobalAlpha', () =>
{
    describe('with no arguments', () =>
    {
        it('should default to skipUpdate = false', () =>
        {
            const parent = new Container();

            const container = new Container();

            container.alpha = 0.5;

            parent.addChild(container);

            parent.alpha = 0.5;

            expect(container.getGlobalAlpha()).toBe(0.25);
        });
    });

    describe('with skipUpdateTransform = false', () =>
    {
        it('should return container alpha when no parent exists', () =>
        {
            const container = new Container();

            container.alpha = 0.5;

            expect(container.getGlobalAlpha(false)).toBe(0.5);
        });

        it('should multiply alpha with single parent', () =>
        {
            const parent = new Container();
            const container = new Container();

            parent.alpha = 0.5;

            container.alpha = 0.5;
            container.parent = parent;

Review comment (suggestion):
```suggestion
            const parent = new Container({ alpha: 0.5 });
            const container = new Container({ alpha: 0.5 });
            parent.addChild(container);
```


Code after:
import { Container } from '../Container';

describe('getGlobalAlpha', () =>
{
    describe('with no arguments', () =>
    {
        it('should default to skipUpdate = false', () =>
        {
            const parent = new Container({ alpha: 0.5 });
            const container = new Container({ alpha: 0.5 });

            parent.addChild(container);

            expect(container.getGlobalAlpha()).toBe(0.25); // 0.5 * 0.5
        });
    });

    describe('with skipUpdateTransform = false', () =>
    {
        it('should return container alpha when no parent exists', () =>
        {
            const container = new Container({ alpha: 0.5 });

            expect(container.getGlobalAlpha(false)).toBe(0.5);
        });

        it('should multiply alpha with single parent', () =>
        {
            const parent = new Container({ alpha: 0.5 });
            const container = new Container({ alpha: 0.5, parent });

            expect(container.getGlobalAlpha(false)).toBe(0.25); // 0.5 * 0.5
        });

        it('should multiply alpha through multiple parents', () =>
        {
            const grandParent = new Container({ alpha: 0.5 });
            const parent = new Container({ alpha: 0.5, parent: grandParent });
            const container = new Container({ alpha: 0.5, parent });

            expect(container.getGlobalAlpha(false)).toBe(0.125); // 0.5 * 0.5 * 0.5
        });
Repository: pixijs/pixijs
File: src/culling/Culler.ts (TypeScript)

Code before:
import { Matrix, Rectangle } from '..';
import { Bounds } from '../scene/container/bounds/Bounds';
import { getGlobalBounds } from '../scene/container/bounds/getGlobalBounds';

import type { Container } from '../scene/container/Container';

const tempBounds = new Bounds();
const tempRectangle = new Rectangle();

/**
 * A rectangle-like object that contains x, y, width, and height properties.
 * @example
 * const rect = { x: 0, y: 0, width: 100, height: 100 };
 * @category utils
 * @advanced
 */
export type RectangleLike = {x: number, y: number, width: number, height: number};

/**
 * The Culler class is responsible for managing and culling containers.
 * Culling optimizes rendering performance by skipping objects outside the visible area.
 *
 * > [!IMPORTANT] culling is not always a golden bullet, it can be more expensive than rendering
 * > objects that are not visible, so it is best used in scenarios where you have many objects
 * > that are not visible at the same time, such as in large scenes or games with many sprites.
 * @example

Review comment (bug):
Unit tests are failing because this import created a circular dependency. 

Let's fix it with direct imports:
```ts
import { Matrix } from '../maths/matrix/Matrix';
import { Rectangle } from '../maths/shapes/Rectangle';
```


Code after:
import { Matrix } from '../maths/matrix/Matrix';
import { Rectangle } from '../maths/shapes/Rectangle';
import { Bounds } from '../scene/container/bounds/Bounds';
import { getGlobalBounds } from '../scene/container/bounds/getGlobalBounds';

import type { Container } from '../scene/container/Container';

const tempBounds = new Bounds();
const tempMatrix = new Matrix();
const tempRectangle = new Rectangle();

/**
 * A rectangle-like object that contains x, y, width, and height properties.
 * @example
 * const rect = { x: 0, y: 0, width: 100, height: 100 };
 * @category utils
 * @advanced
 */
export type RectangleLike = {x: number, y: number, width: number, height: number};

/**
 * The Culler class is responsible for managing and culling containers.
 * Culling optimizes rendering performance by skipping objects outside the visible area.
 *
 * > [!IMPORTANT] culling is not always a golden bullet, it can be more expensive than rendering
 * > objects that are not visible, so it is best used in scenarios where you have many objects
Repository: pixijs/pixijs
File: src/scene/text-html/BatchableHTMLText.ts (TypeScript)

Code before:
        super();

        // Next step is to make canvasTextSystem a GLOBAL object.
        // so this is ok for now..
        this._renderer = renderer;

        renderer.runners.resolutionChange.add(this);
    }

    /** Handles resolution changes for the HTML text. If the text has auto resolution enabled, it triggers a view update. */
    public resolutionChange()
    {
        const text = this.renderable as HTMLText;

        if (text._autoResolution)
        {
            text.onViewUpdate();
        }
    }

    /** Destroys the BatchableHTMLText instance. Returns the texture promise to the renderer and cleans up references. */
    public destroy()
    {
        this._renderer.htmlText.getReferenceCount(this.currentKey) === null
            ? this._renderer.htmlText.returnTexturePromise(this.texturePromise)
            : this._renderer.htmlText.decreaseReferenceCount(this.currentKey);
        this._renderer.runners.resolutionChange.remove(this);
        this.texturePromise = null;
        (this._renderer as null) = null;
    }
}


Review comment (suggestion):
nit, just to be a little less verbose.

```suggestion
        const { htmlText } = this._renderer;
        htmlText.getReferenceCount(this.currentKey) === null
            ? htmlText.returnTexturePromise(this.texturePromise)
            : htmlText.decreaseReferenceCount(this.currentKey);
```

Code after:
        super();

        // Next step is to make canvasTextSystem a GLOBAL object.
        // so this is ok for now..
        this._renderer = renderer;

        renderer.runners.resolutionChange.add(this);
    }

    /** Handles resolution changes for the HTML text. If the text has auto resolution enabled, it triggers a view update. */
    public resolutionChange()
    {
        const text = this.renderable as HTMLText;

        if (text._autoResolution)
        {
            text.onViewUpdate();
        }
    }

    /** Destroys the BatchableHTMLText instance. Returns the texture promise to the renderer and cleans up references. */
    public destroy()
    {
        const { htmlText } = this._renderer;

        htmlText.getReferenceCount(this.currentKey) === null
            ? htmlText.returnTexturePromise(this.texturePromise)
            : htmlText.decreaseReferenceCount(this.currentKey);
        this._renderer.runners.resolutionChange.remove(this);
        this.texturePromise = null;
        (this._renderer as null) = null;
    }
}
Repository: pixijs/pixijs
File: src/math-extras/pointExtras.ts (TypeScript)

Code before:
        outPoint.y = onto.y * normalizedScalarProjection;

        return outPoint;
    },
    reflect<T extends PointData>(normal: PointData, outPoint?: T): T
    {
        if (!outPoint)
        {
            outPoint = new Point() as PointData as T;
        }

        // Given an incident vector i and a normal vector n, returns the reflection vector r = i - 2 * dot(i, n) * n

        const dotProduct = (this.x * normal.x) + (this.y * normal.y);

        outPoint.x = this.x - (2 * dotProduct * normal.x);
        outPoint.y = this.y - (2 * dotProduct * normal.y);

        return outPoint;
    },
    rotate<T extends PointData>(radians: number, outPoint?: T): T
    {
        if (!outPoint)
        {
            outPoint = new Point() as PointData as T;
        }

        const cosTheta = Math.cos(radians);
        const sinTheta = Math.sin(radians);

        outPoint.x = (this.x * cosTheta) - (this.y * sinTheta);
        outPoint.y = (this.x * sinTheta) + (this.y * cosTheta);

        return outPoint;
    }
};


Review comment (suggestion):
```suggestion
        outPoint ??= new Point() as PointData as T;
```


Code after:
        outPoint.y = onto.y * normalizedScalarProjection;

        return outPoint;
    },
    reflect<T extends PointData>(normal: PointData, outPoint?: T): T
    {
        if (!outPoint)
        {
            outPoint = new Point() as PointData as T;
        }

        // Given an incident vector i and a normal vector n, returns the reflection vector r = i - 2 * dot(i, n) * n

        const dotProduct = (this.x * normal.x) + (this.y * normal.y);

        outPoint.x = this.x - (2 * dotProduct * normal.x);
        outPoint.y = this.y - (2 * dotProduct * normal.y);

        return outPoint;
    },
    rotate<T extends PointData>(radians: number, outPoint?: T): T
    {
        outPoint ??= new Point() as PointData as T;

        const cosTheta = Math.cos(radians);
        const sinTheta = Math.sin(radians);

        outPoint.x = (this.x * cosTheta) - (this.y * sinTheta);
        outPoint.y = (this.x * sinTheta) + (this.y * cosTheta);

        return outPoint;
    }
};
Repository: pixijs/pixijs
File: src/rendering/batcher/shared/Batcher.ts (TypeScript)

Code before:
        }
    }

    /**
     * Destroys the batch and its resources.
     * @param options - destruction options
     * @param options.shader - whether to destroy the associated shader
     */
    public destroy(options: {shader?: boolean} = {})
    {
        if (this.batches === null) return;

        for (let i = 0; i < this.batches.length; i++)
        {
            returnBatchToPool(this.batches[i]);
        }

        this.batches = null;
        this.geometry.destroy(true);
        this.geometry = null;

        if (this.shader && options.shader)
        {
            this.shader.destroy();
            this.shader = null;
        }

        for (let i = 0; i < this._elements.length; i++)
        {
            if (this._elements[i]) this._elements[i]._batch = null;
        }

        this._elements = null;

        this.indexBuffer = null;

        this.attributeBuffer.destroy();
        this.attributeBuffer = null;
    }
}



Review comment (suggestion):
```suggestion
        if (options.shader)
        {
            this.shader?.destroy();
            this.shader = null;
        }
```

Code after:
        }
    }

    /**
     * Destroys the batch and its resources.
     * @param options - destruction options
     * @param options.shader - whether to destroy the associated shader
     */
    public destroy(options: {shader?: boolean} = {})
    {
        if (this.batches === null) return;

        for (let i = 0; i < this.batches.length; i++)
        {
            returnBatchToPool(this.batches[i]);
        }

        this.batches = null;
        this.geometry.destroy(true);
        this.geometry = null;

        if (options.shader)
        {
            this.shader?.destroy();
            this.shader = null;
        }

        for (let i = 0; i < this._elements.length; i++)
        {
            if (this._elements[i]) this._elements[i]._batch = null;
        }

        this._elements = null;

        this.indexBuffer = null;

        this.attributeBuffer.destroy();
        this.attributeBuffer = null;
    }
}
Repository: pixijs/pixijs
File: src/rendering/renderers/gpu/shader/utils/__tests__/extractAttributesFromGpuProgram.test.ts (TypeScript)

Code before:
  @location(0) color: vec3<f32>,
  @location(1) uv: vec2<f32>
) -> @location(0) vec4<f32> {


  return  textureSample(myTexture, mySampler, uv) * vec4(color, 1.);
};
`;

const shaderCodeStruct = `
struct VertexInput {
    @location(0) position: vec2f,
    @location(1) uv: vec2<f32>,
    @location(2) color: vec4<f32>,
    @builtin(vertex_index) vertexIndex: u32,
};

@vertex
fn mainVert(input: VertexInput) -> @builtin(position) vec4<f32> {
    return vec4(input.position, 0.0, 1.0);
}
`;

describe('extractAttributesFromGpuProgram', () =>
{
    it('should extract attributes from inline @location decorators', async () =>
    {
        const extractedAttributeData = extractAttributesFromGpuProgram({
            source: shaderCodeInline,
            entryPoint: 'mainVert'
        });

        expect(extractedAttributeData).toEqual({
            aPosition: {
                format: 'float32x2',
                instance: false,
                offset: 0,
                location: 0,
                start: 0,
                stride: 8
            },
            aColor: {
                format: 'float32x3',
                instance: false,
                offset: 0,
                location: 1,
                start: 0,
                stride: 12
            },
            aUV: {
                format: 'float32x2',

Review comment (refactor):
The test function is marked as async but doesn't contain any await statements or asynchronous operations. The async keyword should be removed as it's unnecessary and could be misleading.
```suggestion
    it('should extract attributes from inline @location decorators', () =>
```

Code after:
  @location(0) color: vec3<f32>,
  @location(1) uv: vec2<f32>
) -> @location(0) vec4<f32> {


  return  textureSample(myTexture, mySampler, uv) * vec4(color, 1.);
};
`;

const shaderCodeStruct = `
struct VertexInput {
    @location(0) position: vec2f,
    @location(1) uv: vec2<f32>,
    @location(2) color: vec4<f32>,
    @builtin(vertex_index) vertexIndex: u32,
};

@vertex
fn mainVert(input: VertexInput) -> @builtin(position) vec4<f32> {
    return vec4(input.position, 0.0, 1.0);
}
`;

describe('extractAttributesFromGpuProgram', () =>
{
    it('should extract attributes from inline @location decorators', () =>
    {
        const extractedAttributeData = extractAttributesFromGpuProgram({
            source: shaderCodeInline,
            entryPoint: 'mainVert'
        });

        expect(extractedAttributeData).toEqual({
            aPosition: {
                format: 'float32x2',
                instance: false,
                offset: 0,
                location: 0,
                start: 0,
                stride: 8
            },
            aColor: {
                format: 'float32x3',
                instance: false,
                offset: 0,
                location: 1,
                start: 0,
                stride: 12
            },
            aUV: {
                format: 'float32x2',
Repository: pixijs/pixijs
File: src/assets/loader/parsers/textures/loadVideoTextures.ts (TypeScript)

Code before:
            const onCanPlay = async () =>
            {
                const base = new VideoSource({ ...options, resource: videoElement });

                cleanup();

                if (asset.data.preload)
                {
                    await preloadVideo(videoElement);
                }

                resolve(createTexture(base, loader, url));
            };

            const onError = (event: Event | ErrorEvent) =>
            {
                cleanup();
                reject(event);
            };

            function cleanup(): void
            {
                videoElement.removeEventListener('canplay', onCanPlay);
                videoElement.removeEventListener('error', onError);
                sourceElement.removeEventListener('error', onError);
            }

            if (options.preload && !options.autoPlay)
            {
                videoElement.load();
            }

            videoElement.addEventListener('canplay', onCanPlay);
            videoElement.addEventListener('error', onError);
            sourceElement.addEventListener('error', onError);
            videoElement.appendChild(sourceElement);
        });
    },

    unload(texture: Texture): void
    {
        texture.destroy(true);
    }
} satisfies LoaderParser<Texture, LoadVideoData>;


Review comment (suggestion):
nit, we prefer fat arrows for inline functions to maintain a single context.

```suggestion
            const cleanup = (): void =>
            {
                videoElement.removeEventListener('canplay', onCanPlay);
                videoElement.removeEventListener('error', onError);
                sourceElement.removeEventListener('error', onError);
            };
```


Code after:
            if (options.preload && !options.autoPlay)
            {
                videoElement.load();
            }

            videoElement.addEventListener('canplay', onCanPlay);
            videoElement.addEventListener('error', onError);
            sourceElement.addEventListener('error', onError);
            videoElement.appendChild(sourceElement);

            async function onCanPlay()
            {
                const base = new VideoSource({ ...options, resource: videoElement });

                cleanup();

                if (asset.data.preload)
                {
                    await preloadVideo(videoElement);
                }

                resolve(createTexture(base, loader, url));
            }

            function onError(event: Event | ErrorEvent)
            {
                cleanup();
                reject(event);
            }

            function cleanup()
            {
                videoElement.removeEventListener('canplay', onCanPlay);
                videoElement.removeEventListener('error', onError);
                sourceElement.removeEventListener('error', onError);
            }
        });
    },

    unload(texture: Texture): void
    {
        texture.destroy(true);
    }
} satisfies LoaderParser<Texture, LoadVideoData>;
Repository: pixijs/pixijs
File: tests/visual/scenes/text-html/html-text-dynamic-destroy.scene.ts (TypeScript)

Code before:
            style: {
                fill: '#FFC700',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            }
        });

        container.addChild(text);

        let text2 = new HTMLText({
            text: 'Hello 2',
            style: {
                fill: 'green',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            },
            y: 40
        });

        container.addChild(text2);
        scene.addChild(container);

        renderer.render(scene);
        await new Promise((resolve) => setTimeout(resolve, 250));
        container.destroy({ children: true });

        container = new Container();

        text = new HTMLText({
            text: 'World1',
            style: {
                fill: '#FFC700',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            }
        });

        container.addChild(text);

        text2 = new HTMLText({
            text: 'World2',
            style: {
                fill: 'green',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            },
            y: 40

Review comment (refactor):
As a follow up would be good to replace this Promise with built in setTimeout from node's `timer/promises`? Would really help simplify these waits.

Code after:
            style: {
                fill: '#FFC700',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            }
        });

        container.addChild(text);

        let text2 = new HTMLText({
            text: 'Hello 2',
            style: {
                fill: 'green',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            },
            y: 40
        });

        container.addChild(text2);
        scene.addChild(container);

        renderer.render(scene);
        await new Promise((resolve) => setTimeout(resolve, 350));
        container.destroy({ children: true });

        container = new Container();

        text = new HTMLText({
            text: 'World1',
            style: {
                fill: '#FFC700',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            }
        });

        container.addChild(text);

        text2 = new HTMLText({
            text: 'World2',
            style: {
                fill: 'green',
                fontSize: 32,
                fontFamily: 'Arial',
                fontWeight: 'bold'
            },
            y: 40
Repository: plait-board/drawnix
File: packages/drawnix/src/plugins/freehand/with-freehand-erase.ts (TypeScript)

Code before:
            );
            
            if (elementsToRemove.length > 0) {
                CoreTransforms.removeElements(board, elementsToRemove);
            }
        }
    };

    const complete = () => {
        if (isErasing) {
            deleteMarkedElements();
            isErasing = false;
            elementsToDelete.clear();
            laserPointer.destroy();
        }
    };

    board.pointerDown = (event: PointerEvent) => {
        const isEraserPointer = PlaitBoard.isInPointer(board, [FreehandShape.eraser]);

        if (isEraserPointer && isDrawingMode(board)) {
            isErasing = true;
            elementsToDelete.clear();
            const currentPoint: Point = [event.x, event.y];
            checkAndMarkFreehandElementsForDeletion(currentPoint);
            laserPointer.init(document.querySelector(`#${LASER_POINTER_ID}`) as HTMLCanvasElement);
            return;
        }

        pointerDown(event);
    };

    board.pointerMove = (event: PointerEvent) => {
        if (isErasing) {
            throttleRAF(board, 'with-freehand-erase', () => {
                const currentPoint: Point = [event.x, event.y];
                checkAndMarkFreehandElementsForDeletion(currentPoint);
            });
            return;
        }

        pointerMove(event);
    };

    board.pointerUp = (event: PointerEvent) => {
        if (isErasing) {
            complete();
            return;
        }

        pointerUp(event);

Review comment (suggestion):
If we just directly write this in `init()` function, then we don't need to import `LASER_POINTER_ID` from `lazer-pointer`

Code after:
            );
            
            if (elementsToRemove.length > 0) {
                CoreTransforms.removeElements(board, elementsToRemove);
            }
        }
    };

    const complete = () => {
        if (isErasing) {
            deleteMarkedElements();
            isErasing = false;
            elementsToDelete.clear();
            laserPointer.destroy();
        }
    };

    board.pointerDown = (event: PointerEvent) => {
        const isEraserPointer = PlaitBoard.isInPointer(board, [FreehandShape.eraser]);

        if (isEraserPointer && isDrawingMode(board)) {
            isErasing = true;
            elementsToDelete.clear();
            const currentPoint: Point = [event.x, event.y];
            checkAndMarkFreehandElementsForDeletion(currentPoint);
            laserPointer.init(board);
            return;
        }

        pointerDown(event);
    };

    board.pointerMove = (event: PointerEvent) => {
        if (isErasing) {
            throttleRAF(board, 'with-freehand-erase', () => {
                const currentPoint: Point = [event.x, event.y];
                checkAndMarkFreehandElementsForDeletion(currentPoint);
            });
            return;
        }

        pointerMove(event);
    };

    board.pointerUp = (event: PointerEvent) => {
        if (isErasing) {
            complete();
            return;
        }

        pointerUp(event);