[refactor] simplify logic (less useEffect, useRef, and move code)

[severo]

This PR is pure refactoring to make the logic simpler to follow.

• use ref callbacks, instead of the useRef + useEffect combo, as it's simpler to understand what happens
• move the keyboard event handlers from CellNavigationProvider to Scroller and Slice (where it makes sense to define them)
• move the code used to focus the first cell (if focus is passed) from Slice to CellNavigationProvider (where it makes more sense) + remove useEffect + add tests
• pass scrollRowIntoView from Scroller to Slice. It's called when navigating the cells with keyboard (replaces shouldScroll and its useEffects)

[Copilot]

Pull request overview

This refactoring PR simplifies the component logic by replacing useEffect + useRef patterns with ref callbacks, relocating keyboard event handlers from CellNavigationProvider to their logical locations (Scroller and Slice), and introducing a scrollRowIntoView function to replace the imperative shouldScroll state flag.

Key changes:

• Replaced useEffect + useRef combinations with ref callbacks throughout components (Cell, ColumnHeader, RowHeader, TableCorner, Scroller)
• Moved keyboard navigation logic from CellNavigationProvider to Slice component
• Introduced ScrollerContext to provide scrollRowIntoView function for programmatic scrolling
• Added comprehensive tests for focus behavior and cell navigation boundary conditions

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/providers/CellNavigationProvider.tsx	Removed keyboard handlers, simplified context API, added focus prop to control initial focus behavior
src/contexts/CellNavigationContext.ts	Updated context interface to expose colCount/rowCount directly and removed keyboard handler functions
src/contexts/ScrollerContext.ts	New context to provide scrollRowIntoView function
src/hooks/useCellFocus.ts	Converted from useEffect to ref callback pattern, returns focusCellIfNeeded callback
src/components/HighTable/Scroller.tsx	Converted to ref callback, added scrollRowIntoView implementation, moved scroll keyboard handler here
src/components/HighTable/Slice.tsx	Added keyboard navigation logic previously in CellNavigationProvider, integrated scrollRowIntoView
src/components/HighTable/Wrapper.tsx	Passes focus prop through to CellNavigationProvider
src/components/Cell/Cell.tsx	Updated to use focusCellIfNeeded ref callback
src/components/ColumnHeader/ColumnHeader.tsx	Combined focusCellIfNeeded with ref management in callback
src/components/RowHeader/RowHeader.tsx	Updated to use focusCellIfNeeded ref callback
src/components/TableCorner/TableCorner.tsx	Converted to ref callback pattern with size tracking
src/components/ColumnMenu/ColumnMenu.tsx	Minor formatting and removed optional chaining on focusFirstCell
src/providers/RowsAndColumnsProvider.tsx	Removed obsolete comment
test/providers/CellNavigationProvider.test.tsx	Restructured tests, added comprehensive focus behavior tests

Comments suppressed due to low confidence (1)

src/components/HighTable/Scroller.tsx:159

• The ref callback function doesn't return a cleanup function to properly handle component unmounting or ref changes. When the viewport element changes or the component unmounts, the event listeners and ResizeObserver will remain attached to the old element, causing a memory leak. The cleanup function should be returned to properly unregister all listeners and observers.

  const viewportRef = useCallback((viewport: HTMLDivElement) => {
    // Use arrow functions to get correct viewport type (not null)
    // eslint-disable-next-line func-style
    const updateViewportSize = () => {
      setViewportWidth(viewport.clientWidth)
      // recompute the rows range if the height has changed
      computeAndSetRowsRange(viewport)
    }
    // eslint-disable-next-line func-style
    const handleScroll = () => {
      // TODO(SL): throttle? see https://github.com/hyparam/hightable/pull/347
      // recompute the rows range if the scroll position changed
      computeAndSetRowsRange(viewport)
    }

    // run once
    updateViewportSize()
    handleScroll()

    // set scrollToTop function
    if ('scrollTo' in viewport) {
      setScrollToTop(() => {
        // ^ we need to use a setter function, we cannot set a function as a value
        return (top: number) => {
          viewport.scrollTo({ top })
        }
      })
    } else {
      // scrollTo does not exist in jsdom, used in the tests
      setScrollToTop(undefined)
    }

    // listeners
    const resizeObserver = 'ResizeObserver' in window
      ? new window.ResizeObserver(([entry]) => {
          if (!entry) {
            console.warn('ResizeObserver entry is not available.')
            return
          }
          updateViewportSize()
        })
      // for jsdom
      : undefined
    resizeObserver?.observe(viewport)
    viewport.addEventListener('scroll', handleScroll)

    return () => {
      resizeObserver?.unobserve(viewport)
      resizeObserver?.disconnect()
      viewport.removeEventListener('scroll', handleScroll)
    }
  }, [setViewportWidth, computeAndSetRowsRange])

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.