Skip to content

fix: prevent rect mutation during tag bin probing #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gminor7 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: prevent rect mutation during tag bin probing #67

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/maxrects-packer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,12 @@ export class MaxRectsPacker<T extends IRectangle = Rectangle> {
}

// still in the same tag group
if (testBin.add(rect) === undefined) {
const testRect = { ...rect, rot:false, data: { ...rect.data } }
if (testBin.add(testRect) === undefined) {
Comment on lines +167 to +168
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Spread breaks placement test for Rectangle class instances

{ ...rect } on a Rectangle instance only copies the private backing fields set directly in the constructor (_width, _height, _x, _y, _rot, etc.) — not the prototype getters (width, height, x, y). As a result, testRect.width is undefined, and place() calls findNode(undefined + padding, ...) which returns undefined every time. testBin.add(testRect) therefore always returns undefined, causing every existing bin to be rejected during probing and all rects to be placed in new bins instead of being packed together.

The fix works correctly when callers pass plain objects implementing IRectangle (the pattern shown in all existing tests), but silently breaks bin-sharing for users who pass Rectangle class instances. To cover both cases, extract the getter values explicitly:

const testRect: IRectangle = {
    width: rect.width,
    height: rect.height,
    x: rect.x,
    y: rect.y,
    rot: false,
    data: rect.data ? { ...rect.data } : rect.data,
    tag: (rect as any).tag,
};

// IMPORTANT:
+ // Use a cloned rect for test placement to avoid mutating
+ // the original rect (especially `rot`) during bin probing
Comment on lines +170 to +171
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Git diff markers accidentally committed into source

Lines 170–171 contain a literal + character (a git diff marker) in the middle of the comment text, e.g. + // Use a cloned rect.... These should be plain comment lines.

Suggested change
+ // Use a cloned rect for test placement to avoid mutating
+ // the original rect (especially `rot`) during bin probing
// Use a cloned rect for test placement to avoid mutating
// the original rect (especially `rot`) during bin probing


// add the rects that could fit into the bins already
// do addArray()
this.sort(rects.slice(currentIdx, i), this.options.logic).forEach(r => bin.add(r));
Expand Down