Skip to content

Fix useFind duplicate documents on dep change and stabilize doc references#474

Open
vuphanse wants to merge 1 commit intometeor:masterfrom
Favro:fix-useFind-duplication
Open

Fix useFind duplicate documents on dep change and stabilize doc references#474
vuphanse wants to merge 1 commit intometeor:masterfrom
Favro:fix-useFind-duplication

Conversation

@vuphanse
Copy link

@vuphanse vuphanse commented Mar 4, 2026

Summary
useFind can produce duplicate documents in the rendered output when its dependency list changes and a collection mutation occurs between the new observer setup and the old observer teardown.

The cause
The previous implementation used a custom useSyncEffect hook that relied on useMemo to run the effect (including observer setup) synchronously during render, while deferring the previous observer's cleanup to useEffect:

  const useSyncEffect = (effect, deps) => {
    const [cleanup, timeoutId] = useMemo(() => {
      const cleanup = effect();
      const timeoutId = setTimeout(cleanup, 1000);
      return [cleanup, timeoutId];
    }, deps);
    useEffect(() => {
      clearTimeout(timeoutId);
      return cleanup;
    }, [cleanup]);
  };

When deps change, the following sequence occurs:

  1. useMemo re-runs during render -- creates a new observer synchronously.
  2. The old observer's cleanup is deferred to useEffect's cleanup phase, which hasn't run yet.
  3. Any collection mutation that happens in this gap (e.g. inside a useLayoutEffect in the same component or a child) is seen by both observers, each dispatching an addedAt action and producing a duplicate entry.

This test case can reproduce the issue:

  Tinytest.addAsync(
    'useFind - handles cursor recreation without duplicating documents',
    async function (test, completed) {
      const container = document.createElement('div')
      document.body.appendChild(container)

      const TestDocs = new Mongo.Collection(null)
      TestDocs.insert({ id: 'a', label: 'a' })

      const Test = () => {
        const [rerendered, setRerendered] = useState(false)
        const docs = useFind(() => TestDocs.find({}, { sort: { id: 1 } }), [rerendered])

        useEffect(() => {
          setRerendered(true)
        }, [])

        useLayoutEffect(() => {
          if (!rerendered || TestDocs.findOne({ id: 'b' })) {
            return
          }

          TestDocs.insert({ id: 'b', label: 'b' })
        }, [rerendered])

        return (
          <div>
            {docs && docs.map(doc => (
              <div key={doc.id} data-testid="doc-id">{doc.id}</div>
            ))}
          </div>
        )
      }

      ReactDOM.render(<Test />, container)

      const getIds = () => Array.from(container.querySelectorAll('[data-testid=\"doc-id\"]')).map(node => node.textContent)

      await waitFor(() => {
        const ids = getIds()
        if (ids.length !== 2) {
          throw new Error('Expected two documents')
        }
        if (ids[0] !== 'a' || ids[1] !== 'b') {
          throw new Error('Unexpected document order')
        }
      }, { container, timeout: 500 })

      test.equal(getIds(), ['a', 'b'], 'Documents should not duplicate when deps change')

      document.body.removeChild(container)
      completed()
    }
  )

The fix

  • Replace useSyncEffect with useLayoutEffect + cleanupRef:
    Instead of relying on useMemo for synchronous effect execution and useEffect for deferred cleanup, the new implementation uses a single useLayoutEffect that immediately stops the previous observer before setting up the new one. Because useLayoutEffect fires synchronously after DOM mutations but before the browser paints, the old observer is always torn down before the new one starts observing. There is no window where two observers coexist.

  • Preserve document references across cursor recreation (mergeInitialData):
    When deps change and a new cursor is created, fetchData produces fresh document objects even if the underlying data is identical. Dispatching these via refresh would cause memoized child components to re-render unnecessarily. mergeInitialData compares the new data against the current state by _id using EJSON.equals. For documents that haven't changed, it reuses the previous object reference. If nothing changed at all (same count, same order, same content), it skips the dispatch entirely. This preserves referential stability for React.memo consumers.

Test coverage

  • Reuses document references when deps recreate the cursor -- memoized items don't re-render when data is unchanged.
  • Recreating cursor with modified documents re-renders memoized items -- switching collections with same _ids but different content correctly triggers re-renders.
  • Different sort order updates ordering -- changing sort direction via deps reorders output.
  • Different projection re-renders memoized items -- changing transform/projection triggers re-renders when doc shape changes.
  • Handles cursor recreation without duplicating documents -- mutations during the gap between observer swap don't produce duplicates.

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2026

CLA assistant check
All committers have signed the CLA.

@vuphanse vuphanse force-pushed the fix-useFind-duplication branch from d110cb6 to 6951415 Compare March 4, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants