virtual icon indicating copy to clipboard operation
virtual copied to clipboard

Error: Maximum update depth exceeded

Open Atw-Lee opened this issue 10 months ago • 11 comments

Describe the bug

I want to display the table in the dropdown。

I used the antd UI library

But they had a problem

Your minimal, reproducible example

https://codesandbox.io/p/devbox/infallible-smoke-8tc6d6?file=%2Fsrc%2Fmain.tsx%3A104%2C32

Steps to reproduce

click select

Don't use rowVirtualizer measureElement is normal

Expected behavior

Normal table rendering

How often does this bug happen?

None

Screenshots or Videos

No response

Platform

  • OS: [ macOS 14.6.1 ] - Browser: [Chrome 132.0.6834.160 ]

tanstack-virtual version

^3.12.1

TypeScript version

No response

Additional context

No response

Terms & Code of Conduct

  • [x] I agree to follow this project's Code of Conduct
  • [x] I understand that if my bug cannot be reliable reproduced in a debuggable environment, it will probably not be fixed and this issue may even be closed.

Atw-Lee avatar Feb 08 '25 09:02 Atw-Lee

@piecyk This is very annoying bug. Help is much appreciated.

creage avatar Mar 12 '25 09:03 creage

In my case overriding the default measureElement function by specifying it in the options object made the issue go away. My overridden version simply returns the fixed height of the element (In my case it's a table with fixed height anyways).

measureElement: () => 40

My suspicion: the default version of this function in the lib uses element.getBoundingClientRect() to get the dimensions of the element. I think this can be pretty slow in certain circumstances. https://github.com/TanStack/virtual/blob/main/packages/virtual-core/src/index.ts#L255

When items don't have the same dimensions: I think, somehow IntersectionObserver (https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserverEntry/boundingClientRect) could be used to fix the performance problems causing this issue, however the solution would probably be a breaking change. measureElement expects us to return the measurement immediately. The solution with IntersectionObserver on the other hand would be asynchronous.

dmezei avatar Mar 19 '25 21:03 dmezei

@dmezei If the element has a fixed height, you don’t need to override measureElement. Instead, you can just avoid passing ref={virtualizer.measureElement}. However, if explicitly overriding it with a fixed height resolved the issue for you, that suggests there might be something else affecting measurement in your setup.

piecyk avatar Mar 20 '25 09:03 piecyk

@creage can you provide a small example to reproduce the bug on your end? That would help in diagnosing the issue more effectively.

piecyk avatar Mar 20 '25 09:03 piecyk

@piecyk The example in the description. Launch Chrome DevTools there, and click on the dropdown. It will crash with error:

Uncaught Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at checkForNestedUpdates (chunk-ZUX4WFZD.js?v=30649b61:19659:19)
    at scheduleUpdateOnFiber (chunk-ZUX4WFZD.js?v=30649b61:18533:11)
    at dispatchReducerAction (chunk-ZUX4WFZD.js?v=30649b61:12351:15)
    at Object.onChange (@tanstack_react-virtual.js?v=3998eb52:829:9)
    at Virtualizer.notify (@tanstack_react-virtual.js?v=3998eb52:319:65)
    at Virtualizer.resizeItem (@tanstack_react-virtual.js?v=3998eb52:594:14)
    at Virtualizer._measureElement (@tanstack_react-virtual.js?v=3998eb52:572:14)
    at Virtualizer.measureElement (@tanstack_react-virtual.js?v=3998eb52:607:12)
    at ref (main.tsx:311:37)
    at commitAttachRef (chunk-ZUX4WFZD.js?v=30649b61:17279:28)

The way we workaround it for now is by providing custom measureElement function, that will fallback to estimateSize when measuring of target item fails.

creage avatar Mar 20 '25 09:03 creage

The way we workaround it for now is by providing custom measureElement function, that will fallback to estimateSize when measuring of target item fails.

Yes, the issue here is that on the first render, the elements are positioned off-screen or hidden. Without a defined height, the virtualizer will continue rendering items in the list until they fit within the viewport.

One way to fix this is by checking if the element has a valid height and width. If it does, return the measured size; otherwise, fallback to the cached value:

measureElement: (element, _roEntry, instance) => {
  const cachedSize =
    instance.measurementsCache[instance.indexFromElement(element)].size;
  const rect = element.getBoundingClientRect();
  if (rect.height > 0 && rect.width > 0) {
    return Math.round(rect.height);
  }
  return cachedSize;
},

There were previous discussions about including this in the core, and overall, it should be configurable. For example, if an element in the list can be collapsed, we may want to explicitly set its size to 0.

piecyk avatar Mar 20 '25 11:03 piecyk

@piecyk

We encountered a similar issue in our app. Oddly enough passing the function reference directly into the ref attribute solved the problem.

@Atw-Lee I was able to confirm the same fix worked for your sandbox by changing your code to do this

// change this to a function
    measureElement: (element) => {
      return typeof window !== 'undefined' &&
      navigator.userAgent.indexOf('Firefox') === -1
        ? element => element?.getBoundingClientRect().height
        : undefined
    },

// pass the function reference directly in rather than wrapped in an anonymous function
ref={rowVirtualizer.measureElement} 

unsure why this happens, but the fix in our app was similar

stv8 avatar Jul 03 '25 21:07 stv8

@stv8 you should not wrap the ref with an function as it's known Caveats with callback refs

If the ref callback is defined as an inline function, it will get called twice during updates, first with null and then again with the DOM element. This is because a new instance of the function is created with each render, so React needs to clear the old ref and set up the new one. You can avoid this by defining the ref callback as a bound method on the class, but note that it shouldn’t matter in most cases.

piecyk avatar Jul 10 '25 06:07 piecyk

yep I actually stumbled on this related blog last night which explains the problem as well.

I was trying to find documentation on the react site which mentioned those caveats but this was the best I could find.

https://tkdodo.eu/blog/avoiding-use-effect-with-callback-refs


With that knowledge, what stops us from focussing the input right inside the callback ref, where we have direct access to the node?

<input
  ref={(node) => {
    node?.focus()
  }}
  defaultValue="Hello world"
/>

Well, a tiny detail does: React will run this function after every render. So unless we are fine with focussing our input that often (which we are likely not), we have to tell React to only run this when we want to.

Luckily, React uses referential stability to check if the callback ref should be run or not. That means if we pass the same ref(erence, pun intended) to it, execution will be skipped.

stv8 avatar Jul 10 '25 14:07 stv8

@stv8 it's on legacy page, don't know if there is something on new docs https://legacy.reactjs.org/docs/refs-and-the-dom.html#caveats-with-callback-refs

piecyk avatar Jul 10 '25 15:07 piecyk

I can reproduce this issue on Mobile Safari (iOS 17.5 and iOS 18.6). It seems the problem is about the intensive usage of getBoundingClientRect?

In my case since all my elements have more or less the same height, I implemented a way to calculate an average height of the first N items, and I use it as a fallback:

// Remember to clear these refs if the list items change, on useEffect
//...
    measureElement: (element) => {
      const fallbackHeight = 100

      if (typeof window === 'undefined') {
        return fallbackHeight
      }

      // Recalculate average height if we have enough items, up to a max of N items
      if (measuredHeightsRef.current.length > 0 && measuredHeightsRef.current.length <= measuredHeightsMaxItems) {
        measuredHeightsAverageRef.current =
          measuredHeightsRef.current.reduce((a, b) => a + b, 0) / measuredHeightsRef.current.length
      }

      const rect = element.getBoundingClientRect()
      const avgOrFallback = measuredHeightsAverageRef.current ?? fallbackHeight

      if (rect.height > 0 && rect.width > 0) {
        const roundedHeight = Math.round(rect.height)

        if (roundedHeight <= 0) {
          return avgOrFallback
        }

        if (measuredHeightsRef.current.length < measuredHeightsMaxItems) {
          measuredHeightsRef.current.push(roundedHeight)
        }
        return roundedHeight
      }
      return avgOrFallback
    },
//...

I asked GPT5 to analyse the library and suggested some changes. I dont know if this could help the maintainers or if it makes sense (I dont fully understand how all the parts of the lib works):

  • Proposed changes (safe defaults; minimal impact)

    • react package:
      • In useVirtualizerBase → resolvedOptions.onChange:
        • Remove immediate rerenders (incl. flushSync).
        • Coalesce rerenders to a single requestAnimationFrame (rAF) per frame with a boolean guard.
        • Pseudocode:
          const scheduledRef = useRef(false)
          onChange: (instance) => {
            if (scheduledRef.current) return
            scheduledRef.current = true
            const win = instance.targetWindow ?? window
            ;(win.requestAnimationFrame ?? ((cb) => setTimeout(cb, 0)))(() => {
              scheduledRef.current = false
              rerender()
            })
          }
          
    • core package:
      • In maybeNotify callback:
        • Do not call notify immediately. Batch to one rAF per frame:
          if (!this._notifyScheduled) {
            this._notifyScheduled = true
            raf(() => {
              this._notifyScheduled = false
              this.notify(isScrolling)
            })
          }
          
      • In observeElementRect handler:
        • Accumulate latest rect and apply once per frame before maybeNotify:
          this._rectPending = rect
          if (!this._rectScheduled) {
            this._rectScheduled = true
            raf(() => {
              this.scrollRect = this._rectPending
              this._rectPending = undefined
              this._rectScheduled = false
              this.maybeNotify()
            })
          }
          
      • In observeElementOffset handler:
        • Accumulate { offset, isScrolling } and apply once per frame before maybeNotify:
          this._offsetPending = { offset, isScrolling }
          if (!this._offsetScheduled) {
            this._offsetScheduled = true
            raf(() => {
              const p = this._offsetPending
              this._offsetPending = undefined
              this._offsetScheduled = false
              if (!p) return
              this.scrollAdjustments = 0
              this.scrollDirection = p.isScrolling ? (this.getScrollOffset() < p.offset ? 'forward' : 'backward') : null
              this.scrollOffset = p.offset
              this.isScrolling = p.isScrolling
              this.maybeNotify()
            })
          }
          
  • Rationale

    • Scroll events, ResizeObserver callbacks, and measureElement can fire multiple times per frame; each currently can trigger maybeNotify → onChange → React rerender. Coalescing to one rAF-per-frame breaks the re-entrant update chain and removes the update-depth loop, especially on Safari/mobile.
  • Optional (API toggle)

    • Add a boolean option (default: true) to enable rAF batching:
      • react: useAnimationFrameOnChange
      • core: reuse useAnimationFrameWithResizeObserver or a new useAnimationFrameOnNotify
    • Document: slight frame-latency trades for stability under bursty events.
  • Outcome

    • Implementing the above eliminated the loop in our reproduction and aligns with reports in this issue.

itsjavi avatar Aug 14 '25 08:08 itsjavi