DataGrid: split scroll/scroll to position/should focus position handling into hooks

[nstepien]

src/DataGrid.tsx is quite fat, so I'm trying to slim it down.
This improves co-locality too!

[nstepien]

Scroll state now uses useSyncExternalStore instead of 2xuseState+flushSync

[nstepien]

This used to be a useState, but an eslint rule complained about setting state in effect.
Strangely it did not complain when the same code was in DataGrid, maybe because DataGrid is too big and complicated so it confuses static analysis algos.
Though to make sure the effect works, I had to change the selectedPosition.idx dependency to selectedPosition.
But now we 1 fewer state that can trigger re-renders!

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.45%. Comparing base (1e98765) to head (332d1dd).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3986      +/-   ##
==========================================
+ Coverage   97.41%   97.45%   +0.04%     
==========================================
  Files          37       38       +1     
  Lines        1470     1495      +25     
  Branches      473      481       +8     
==========================================
+ Hits         1432     1457      +25     
  Misses         38       38              

Files with missing lines	Coverage Δ
src/DataGrid.tsx	98.85% <100.00%> (-0.07%)	⬇️
src/hooks/useActivePosition.ts	95.91% <100.00%> (+0.91%)	⬆️
src/hooks/useGridDimensions.ts	100.00% <100.00%> (ø)
src/hooks/useScrollState.ts	100.00% <100.00%> (ø)
src/hooks/useScrollToPosition.tsx	100.00% <100.00%> (ø)
src/utils/domUtils.ts	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nstepien]

before: useRef+useLayoutEffect
now: 1 function
Seems to work just as well.
Since it doesn't need any hooks anymore, I've also inlined the component

[amanmahajan7]

So this works because we recreate ref callback on each render?

[nstepien]

Correct.
Although it might not work 100% well with auto-sized columns + smooth scrolling.
With smooth scrolling it may take >1 frame before the grid starts scrolling, so the condition passes immediately.
We can revisit later or I can revert. Not sure this worked well before either.

[amanmahajan7]

Is there a benefit of using a map instead of setting a local state inside the component?

[nstepien]

Are you suggesting we keep using useState instead of useSyncExternalStore?

[nstepien]

function Test() {
  const [resolvers, setResolvers] = useState<PromiseWithResolvers<void>>(() =>
    Promise.withResolvers()
  );

  return (
    <>
      <button type="button" onClick={() => setResolvers(Promise.withResolvers())}>
        New promise
      </button>
      <button type="button" onClick={() => resolvers.resolve()}>
        Resolve
      </button>

      <Suspense fallback="Loading...">
        <Inner promise={resolvers.promise} />
      </Suspense>
    </>
  );
}


function Inner({ promise }: { promise: Promise<void> }) {
  useState(() => {
    console.log('init state');
  });

  use(promise);

  return 'ok';
}

In this example the useState init function in <Inner> is called twice, then once more after the promise resolves. (multiply by two in StrictMode)
Once React commits a render tree to the DOM though, then the init function is not called again.

This also happens when the suspense is triggered deeper in the tree:

function Inner({ promise }: { promise: Promise<void> }) {
  useState(() => {
    console.log('init state');
  });

  // use(promise);

  return (
    <>
      <InnerSide />
      <InnerDeep promise={promise} />
    </>
  );
}

function subscribe() {
  console.log('subscribe');

  return () => {
    console.log('unsubscribe');
  };
}

function getSnapshot() {
  console.log('get snapshot');
}

function InnerSide() {
  useState(() => {
    console.log('init state side');
  });

  useSyncExternalStore(subscribe, getSnapshot);

  return 'side';
}

function InnerDeep({ promise }: { promise: Promise<void> }) {
  useState(() => {
    console.log('init state deep');
  });

  use(promise);

  return 'ok';
}

React only stops re-initializing states when it renders the tree under <Suspense> to the DOM (as I understand it).
So <DataGrid> could be affected if anything suspends the render tree it is in.
Also, subscribe is called just after useLayoutEffects, but it is not cleaned up if the component suspends again.
useTransition and useDeferredValue also trigger concurrent rendering which may affect all this.

All that to say... I guess it's slightly better to have a single WeakMap?

[amanmahajan7]

Does this mean a single WeakMap would be more performant or prevent some edge cases?

[nstepien]

It's simple and potentially avoids states reinitialization due to suspense/activity.
How would you rather this be implemented?

[amanmahajan7]

Not sure. Let's go with what we have.

[amanmahajan7]

Is it better to use state? little concerned that we have to manually handle the dependencies in the effect.

[nstepien]

That's what tests are for!
We have a few tests that fail when the dependency is activePosition.idx instead of activePosition.
I think ref makes sense here.

[nstepien]

Ended up moving this into useActivePosition, and found a more elegant solution

[amanmahajan7]

A few more tiny comments

[amanmahajan7]

Do we not need to reset positionToFocus after focus?

[nstepien]

No, to avoid 1 re-render.
I've added a ref tho to avoid re-focusing elements after the layout effect re-mounts

[amanmahajan7]

Should we add a test? I though we had test to prevent focus stealing

[nstepien]

We don't have any tests for suspense/activity.
I can add one if you want

[amanmahajan7]

I think it is okay. I got little confused on why we were resetting the state previously. The previous deps were [shouldFocusPosition, activePositionIsRow, gridRef]) so it would rerun when activePositionIsRow is changed. But now we only have [positionToFocus, gridRef]) which is more robust

[amanmahajan7]

Does this mean a single WeakMap would be more performant or prevent some edge cases?

[amanmahajan7]

So this works because we recreate ref callback on each render?

[amanmahajan7]

👍