Skip to content

Separate State and DOM#452

Merged
severo merged 13 commits intomasterfrom
simplify
Mar 3, 2026
Merged

Separate State and DOM#452
severo merged 13 commits intomasterfrom
simplify

Conversation

@severo
Copy link
Copy Markdown
Contributor

@severo severo commented Mar 3, 2026

See #449

The internal state is handled with contexts (even if it creates some sort of "Providers hell" - still manageable, no need for an external state library).

Now, the state and the DOM are clearly separated.

Replace useData hook with a DataProvider, to reduce the number of rerenders.

Everything (state and DOM) is recreated when a new data frame is passed. It was already the case before. An improvement might be to adapt the internal state to the new data frame, but I'm not sure we need to add this complexity, if changing the data prop is not a common action. I'll work on it in a future PR.
Note that the "DOM" component still has some internal state, it's not stateless, but it's a cleaner way to separate the global state from the elements.


Note that (even if I don't really understand what FPS exactly measures in React Scan), the FPS are increasing. Beware: it's not a scientific experiment, take with care, and see if you reproduce.

before:

Screencast.From.2026-03-03.16-23-05.mp4

after:

Screencast.From.2026-03-03.16-22-34.mp4

(I don't reproduce anymore :) )

severo added 3 commits March 3, 2026 14:57
Also: change the logic a bit -> the whole component is recreated on data
frame change. It's simpler (before we were only recreating part of the
providers, but it was more complex and ended in rerendering everything).
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

This PR refactors HighTable to separate internal state management from DOM rendering by moving dataframe-derived state into dedicated contexts (DataVersionContext, NumRowsContext) via a new DataProvider, reducing prop-drilling and aiming to limit unnecessary re-renders/unmounts.

Changes:

  • Replace the former useData hook approach with a new DataProvider + DataContext (version/numRows via context).
  • Update providers/components (Selection/Scroll/CellNavigation/Slice/Wrapper) to consume numRows/version from context instead of props.
  • Restructure HighTable into State and DOM subtrees and add tests around remounting and the new provider behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/providers/SelectionProvider.test.tsx Adjusts tests to provide numRows via NumRowsContext.Provider.
test/providers/DataProvider.test.tsx Adds new test coverage for DataProvider version/numRows behavior.
test/providers/CellNavigationProvider.test.tsx Adjusts tests to provide numRows via NumRowsContext.Provider.
test/hooks/useData.test.ts Removes tests for the deleted useData hook.
src/providers/SelectionProvider.tsx Switches from numRows prop to useNumRows() context consumption.
src/providers/ScrollProvider.tsx Switches from numRows prop to useNumRows() context consumption.
src/providers/DataProvider.tsx Introduces DataProvider that publishes version and numRows via context.
src/providers/CellNavigationProvider.tsx Switches from numRows prop to useNumRows() context consumption.
src/contexts/DataContext.ts Adds DataVersionContext/NumRowsContext and hooks.
src/components/HighTable/Wrapper.tsx Switches from numRows prop to useNumRows() context consumption.
src/components/HighTable/Slice.tsx Switches from numRows/version props to useNumRows()/useDataVersion().
src/components/HighTable/HighTable.tsx Splits State/DOM and attempts to remount state when dataframe instance changes.
src/components/HighTable/HighTable.test.tsx Adds coverage to validate remount behavior when dataframe instance changes.
Comments suppressed due to low confidence (2)

src/providers/DataProvider.tsx:15

  • The JSDoc for DataProvider says it “Handles the viewport size (width and height) state”, but this provider actually manages version/numRows derived from the dataframe. Please update the comment to reflect the actual responsibility to avoid misleading future changes.
    src/providers/DataProvider.tsx:42
  • DataProvider initializes numRows from the initial data prop, but when a new dataframe instance is passed, neither numRows nor version are reset/synchronized to the new data value. Because the effect only attaches listeners, consumers can observe stale numRows/version after a data prop change. Consider resetting version to 0 and numRows to data.numRows when data changes (e.g., inside the [data] effect).

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

Comment thread test/providers/DataProvider.test.tsx Outdated
Comment thread src/components/HighTable/HighTable.tsx Outdated
severo and others added 2 commits March 3, 2026 11:29
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 14 out of 14 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/providers/DataProvider.tsx:15

  • DataProvider’s JSDoc appears to be copied from the viewport-size provider (it describes viewport width/height), but this component actually tracks version and numRows from the dataframe and exposes them via DataVersionContext/NumRowsContext. Please update the docstring to reflect the actual behavior to avoid misleading future changes.
    src/providers/DataProvider.tsx:25
  • DataProvider doesn’t reset/sync its internal version and numRows state when the data prop changes to a different dataframe instance (it only swaps event listeners). If a parent rerenders this provider with a new data, consumers can keep seeing the old numRows/version until events fire. Consider explicitly resetting state in a useEffect keyed on data (e.g., set numRows to data.numRows and reset version), or document/enforce that DataProvider must be remounted on data changes.

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

Comment thread src/components/HighTable/HighTable.tsx Outdated
Comment thread src/providers/SelectionProvider.tsx Outdated
severo and others added 2 commits March 3, 2026 12:03
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 14 out of 14 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/providers/DataProvider.tsx:15

  • The JSDoc above DataProvider describes viewport size state/contexts, but this provider actually tracks version and numRows from the dataframe and exposes them via DataVersionContext/NumRowsContext. Please update the comment so it matches the current behavior to avoid misleading future changes.
    src/providers/DataProvider.tsx:42
  • When the data prop changes, this effect re-wires event listeners but the provider state (version, numRows) is not reset to match the new dataframe instance. That can leave consumers seeing stale values until the new dataframe emits events. Consider resetting version/numRows when data changes (e.g., inside this effect before registering listeners, or in a separate useEffect keyed on data).

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

Comment thread src/components/HighTable/HighTable.tsx 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 14 out of 14 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/providers/DataProvider.tsx:19

  • DataProvider doesn’t reset version/numRows when the data prop changes; it only re-subscribes to the new eventTarget. If a different dataframe instance is passed without remounting this component, numRows can stay stuck on the previous dataframe until a numrowschange event fires (and version won’t reset), so the provided context becomes incorrect. Consider resetting state in a useEffect when data changes, or explicitly require/ensure DataProvider is keyed by data.
    src/providers/DataProvider.tsx:15
  • The docblock above DataProvider appears to describe viewport size state, but this provider manages dataframe-derived state (version and numRows) and exposes them via DataVersionContext/NumRowsContext. Please update the comment to match the actual responsibility to avoid misleading maintainers.

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

Comment thread src/components/HighTable/HighTable.tsx Outdated
Comment thread test/providers/DataProvider.test.tsx
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 14 out of 14 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/providers/DataProvider.tsx:23

  • DataProvider keeps version and numRows in local state but doesn’t reset/synchronize them when the data prop changes. Because useState(data.numRows) only applies on first mount, rerendering DataProvider with a different dataframe instance will continue to expose the previous dataframe’s numRows/version until the new dataframe emits events. Consider updating state in response to data changes (e.g., set numRows from the new dataframe immediately and reset version), or explicitly document/encode the requirement that the component is always remounted on data changes.

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

Comment thread test/providers/DataProvider.test.tsx 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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 15 out of 15 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 src/providers/DataProvider.tsx
@severo severo merged commit 9cd1309 into master Mar 3, 2026
5 checks passed
@severo severo deleted the simplify branch March 3, 2026 20:33
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