[select] Uncaught TypeError: Converting circular structure to JSON

[Profesor08]

fixes #4451 [select] Uncaught TypeError: Converting circular structure to JSON

• I have followed (at least) the PR section of the contributing guide.

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@4452
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@4452
  

commit: 5303d9e

[mui-bot]

Bundle size report

Bundle	Parsed size	Gzip size
@base-ui/react	0B(0.00%)	0B(0.00%)

Details of bundle changes

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	5303d9e
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/69c666004d32c30008ceecb4
😎 Deploy Preview	https://deploy-preview-4452--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[romgrk]

I don't remember, would it still work with this in the devDependencies (considering the check only runs in development)?

Feels bad to declare a dependency if it's only used in dev mode.

[Profesor08]

It depends on how bundler works, if bundler has dead code elimination and tree shaking then all be fine. If not, then es-toolkit is commonly used ESM library, so bundle will grow a little, but this is not a big deal, because in any way, there should be implemented something to deeply compare objects.

Also bundle size bot reports 0 changes

[atomiks]

We can just use deepEqual as a util pasted locally:

const defaultPropRef = React.useRef(defaultProp);

React.useEffect(() => {
  if (!isControlled && !deepEqual(defaultPropRef.current, defaultProp)) {
    console.error(
      `Base UI: A component is changing the default ${state} state of an uncontrolled ${name} after being initialized. ` +
        `To suppress this warning opt to use a controlled ${name}.`,
    );
  }
}, [defaultProp, isControlled, name, state]);

[Profesor08]

as alternative solution it is ok

[Profesor08]

After playng a while with this implementation of deepEqual, I can say what I definetely don't like it. Code is weird and unreadable, and it not covering a lot of cases. It is ok for floating-ui and covers their cases, but it is not for public use.

In this example, all values are not equal, but function returns true

const m1 = new Map().set('a', 1).set('b', 2);
const m2 = new Map().set('a', 1).set('b', 2).set('c', 3);

console.log(deepEqual(m1, m2)); // returns true, should be false

console.log(deepEqual(/test1/, /test2/)); // returns true, should be false

console.log(deepEqual(new Date(0), new Date(1))); // returns true, should be false

class A {
  getPrice = () => 100;
}

console.log(deepEqual(new A(), new A())); // returns true, should be false

class B {
  price = 100;
}

class C {
  price = 100;
}

console.log(deepEqual(new B(), new C())); // returns true, should be false

const b = new B();
const c = new C();

console.log(deepEqual(b, c)); // returns true, should be false

isEqual from es-toolkit covers all this cases correctly, also it is used in other parts of this repo.

[atomiks]

Those are fairly niche for a dev warning though. es-toolkit's is more accurate but seems overkill. So we should inline it into useControlled and not expose it given it's not intended for public use

[oliviertassinari]

The Data Grid is most likely going to need a deepEqual logic too, looking at how MUI X Data Grid uses this in a bunch of places. So +1 to have it as an until, to cut down on dependencies.

[romgrk]

If we need a deep equal function in utils, it probably needs to be a generic one like es-tk's one.

Alternatively, I would suggest this function as a safe alternative to JSON.stringify. It handles circular references. It doesn't handle all edge-cases like es-tk's function does, but neither does JSON.stringify:

$ node -p 'JSON.stringify({ test: new Map([[1, 2]]) })'
{"test":{}}

[Profesor08]

Just checked how react handles defaultValue change on <input />, and it not gives any warnings, it allow this prop to be changed on uncontrolled inputs. But it warns then I change behaviour from controlled to uncontrolled and back. So now i'm not sure, if this checks are really usefull. It is rare case, then I use uncontrolled inputs, but when I use, I don't excpect any troubles with them or limitations.

[atomiks]

Codex Review (GPT-5.4)

This is a bug-fix PR that resolves the reported circular-JSON crash for React-element defaults, and the targeted useControlled JSDOM test still passes on the branch. The main remaining issues are that the warning path now re-logs on every rerender after a mismatch appears, and the new helper is being introduced as a public @base-ui/utils entrypoint in a shape that does not look ready to support.

1. Bugs / Issues

1. 🟡 The new warning effect re-logs on every rerender after a mismatch appears (non-blocking)

In packages/utils/src/useControlled.ts, the defaultValue warning effect no longer has a dependency array. Once defaultValue differs from the captured initial value, every subsequent rerender schedules the same warning again, even if defaultValue itself did not change again.

From packages/utils/src/useControlled.ts:

React.useEffect(() => {
  if (!isControlled && !deepEqual(defaultValue, defaultProp)) {
    console.error(
      [
        `Base UI: A component is changing the default ${state} state of an uncontrolled ${name} after being initialized. ` +
          `To suppress this warning opt to use a controlled ${name}.`,
      ].join('\n'),
    );
  }
});

This matters because the old implementation only logged when the tracked value changed. The new version turns that into repeated console noise during unrelated rerenders, which is especially annoying in tests and local debugging.

Fix: Add the dependency gate back, or route this through @base-ui/utils/error so the warning is deduplicated once the mismatch has been detected.

2. ℹ️ If this helper stays public, the entrypoint should be deepEqual, not deepEquals (informational)

packages/utils exports every top-level source file via the wildcard export map, so adding packages/utils/src/deepEquals.ts makes @base-ui/utils/deepEquals part of the public package surface immediately. If the helper is going to remain at the package root, that public path should match the actual symbol name (deepEqual) instead of introducing a pluralized entrypoint for a singular comparator.

From packages/utils/package.json:

"exports": {
  "./store": "./src/store/index.ts",
  "./*": "./src/*.ts"
}

This matters because even a narrow internal helper becomes naming debt once consumers can import it.

Recommendation: Prefer keeping the comparator private to useControlled, but if it stays as a root-level entrypoint, rename the file to deepEqual.ts so the public path matches the export.

3. ℹ️ Drop the top-of-file comment if this helper lands (informational)

The new file starts with an internal implementation note that says the comparator only handles “the comparisons we need”. That kind of comment makes sense for a tightly local helper, but it reads oddly on a file that is being published as a top-level package entrypoint.

From packages/utils/src/deepEquals.ts:

// Fork of `fast-deep-equal` that only does the comparisons we need and compares
// functions

This matters because it advertises a partial, intentionally narrow implementation right on what is currently a public utility surface.

Recommendation: Remove the comment if the file lands, or keep the helper private so the implementation note stays local.

2. Root Cause & Patch Assessment

The patch does address the reported crash on initial render when defaultValue contains a React element, and the new regression test proves that narrow scenario. I also checked the old JSON.stringify behavior directly against the new comparator behavior: RegExp and Map were already collapsed by the old code because both serialized to {}, so that part is not a new regression. Date is the notable semantic difference, because the old check distinguished different dates while the new deepEqual implementation treats them as equal. Even so, those value types feel secondary here; the bigger practical regression is still the repeated warning spam from the now-unbounded effect.

3. Test Coverage Assessment

I ran pnpm test:jsdom useControlled --no-watch, and the touched file passes. The new test in packages/utils/src/useControlled.test.tsx is still narrow, though: it only proves the initial render no longer throws, so it does not protect the changed warning behavior itself.

The highest-value addition here would be a regression that changes a circular/default object after mount and asserts the warning only fires once.

Merge Confidence Score

Overall merge confidence is 3/5. The reported crash looks fixed, but I would still address the repeated-warning behavior before merging, and I would avoid baking the current helper name/comment into the public utils surface unless that exposure is intentional.

[atomiks]

Codex Review (GPT-5.4)

This follow-up addresses most of the earlier review feedback: the helper is now named deepEqual, the top-of-file comment is gone, and restoring the dependency array fixes the repeated-warning issue I called out before. I reviewed the full branch again against a freshly fetched upstream/master and reran pnpm test:jsdom useControlled --no-watch.

1. Remaining Bugs / Issues

1. ℹ️ deepEqual is still being added as a public @base-ui/utils entrypoint (informational)

Because packages/utils exports every top-level file in src/, adding packages/utils/src/deepEqual.ts still publishes @base-ui/utils/deepEqual as a supported entrypoint. The helper still looks specialized to this warning path rather than like a general-purpose utility: it skips React _owner links and compares functions by toString(), which is a fairly surprising public contract to freeze.

From packages/utils/package.json:

"exports": {
  "./store": "./src/store/index.ts",
  "./*": "./src/*.ts"
}

This matters because a narrow bug fix now grows the public utils surface in a way that may be awkward to document, support, or revise later.

Recommendation: Keep the comparator private to useControlled (or under a non-exported internal path) unless the intent is to ship and support it as a stable utility.

2. Root Cause & Patch Assessment

The reported crash on initial render with a React-element defaultValue does look fixed, and the previous rerender-spam issue from the missing dependency array is resolved by switching the effect back to [defaultProp]. At this point the main remaining concern is API shape rather than a concrete behavior problem in the fix itself.

3. Test Coverage Assessment

The test coverage is better than in the previous revision, but it still stops short of the exact branch this patch reworks. One test proves the initial render with a circular React element no longer throws, and another proves primitive defaultValue changes warn on change boundaries. What is still missing is a regression that updates an uncontrolled object-shaped defaultValue containing a React element after mount and asserts that it warns without throwing.

From packages/utils/src/useControlled.test.tsx:

it('does not throw - Converting circular structure to JSON', () => {
  function TestComponentArray() {
    useControlled({
      controlled: undefined,
      default: {
        icon: <span />,
      },
      name: 'TestComponent',
    });
    return null;
  }

  expect(() => {
    render(<TestComponentArray />);
  }).not.toErrorDev();
});

This matters because the new comparator logic is only exercised on the initial-render no-throw path in the current suite. The branch that compares changed object defaults after mount is still effectively inferred rather than directly protected.

Fix: Add one follow-up case that mounts with an object-shaped defaultValue, updates it after mount, and asserts that the warning fires exactly as intended without reintroducing the circular-JSON failure.

4. Previous Review Issues Status

Issue	Status	Notes
Repeated warning on every rerender	Resolved	Restoring the dependency array addresses the earlier console-spam issue
Public entrypoint name deepEquals	Resolved	The file is now named deepEqual.ts
Top-of-file internal comment	Resolved	The comment was removed

Merge Confidence Score

Overall merge confidence is 4/5. This revision is materially better than the previous one, and the original crash fix looks sound from the targeted validation I ran. I would still prefer to keep deepEqual private and add the object-update regression test, but I do not see the removed comparison note as a reason to block merging.

[flaviendelangle]

I don't think this comment is relevant anymore