Skip to content

[refactor] pass the data frame methods in a context#456

Merged
severo merged 5 commits intomasterfrom
simplify
Mar 4, 2026
Merged

[refactor] pass the data frame methods in a context#456
severo merged 5 commits intomasterfrom
simplify

Conversation

@severo
Copy link
Copy Markdown
Contributor

@severo severo commented Mar 4, 2026

see #449

context instead of props, to avoid rerenders.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors HighTable to stop passing the data object through component props and instead expose data-frame methods via a dedicated React context, aiming to reduce unnecessary re-renders (per #449).

Changes:

  • Introduce DataContext + useData() and split out method-only typing (DataFrameMethods) in DataContext.ts.
  • Wrap the table subtree with DataContext.Provider and stop threading data through HighTableDOMSlice.
  • Update useFetchCells and Slice to consume data/numRows from context rather than props.

Reviewed changes

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

Show a summary per file
File Description
src/providers/DataProvider.tsx Adds DataContext.Provider and refines typing for the keyed provider.
src/contexts/DataContext.ts Defines DataContext, useData(), and method/metadata type splits for the data frame.
src/hooks/useFetchCells.ts Switches to reading data and numRows from contexts.
src/components/HighTable/Slice.tsx Removes data prop dependency; reads methods from useData() and updates call sites.
src/components/HighTable/HighTable.tsx Stops passing data through DOM/Slice; passes data only where still needed (state providers).
Comments suppressed due to low confidence (2)

src/components/HighTable/Slice.tsx:21

  • SliceProps still includes numRowsPerPage, but Slice doesn't read or forward that prop anywhere (it's only referenced in this type). Keeping unused props in the public signature makes it harder to tell what actually affects rendering—consider removing it from SliceProps (or wiring it up if it should influence behavior).
type SliceProps = Pick<HighTableProps, 'numRowsPerPage' | 'onDoubleClickCell' | 'onError' | 'onKeyDownCell' | 'onMouseDownCell' | 'overscan' | 'renderCellContent' | 'stringify'>

src/components/HighTable/HighTable.tsx:26

  • The comment says “The DataProvider is remounted on data change”, but the implementation actually keys/remounts KeyedDataProvider (a child) while DataProvider itself stays mounted. Updating this comment would avoid confusion when reasoning about what does/doesn’t reset across data changes (especially now that DataContext.Provider sits outside the keyed subtree).
    // The DataProvider is remounted on data change, so everything is recreated.
    // TODO(SL): if this becomes a performance issue, we can revisit this behavior, and update the
    // state more granularly.
    <DataProvider data={data}>
      <State data={data} {...props}>

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

Comment thread src/components/HighTable/Slice.tsx
Comment thread src/contexts/DataContext.ts Outdated
Comment thread src/contexts/DataContext.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


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

Comment thread test/providers/DataProvider.test.tsx
@severo severo merged commit 4c45cf9 into master Mar 4, 2026
9 checks passed
@severo severo deleted the simplify branch March 4, 2026 10:47
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