Conversation
We cannot call a prop function during rendering. It must be in an effect, or an event callback.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the component architecture by converting DataProvider from a React context provider to a custom hook (useData), removing the ErrorProvider and Wrapper components, and inlining the Wrapper logic directly into HighTable. The refactoring improves code organization by making data flow more explicit through props rather than implicit through context.
Changes:
- Converted DataProvider to useData hook that returns dataId, version, and numRows
- Removed ErrorProvider and ErrorContext, passing onError as explicit props to components that need it
- Removed Wrapper component and merged its logic into HighTable component
- Updated all affected providers to receive data, numRows, and onError as props instead of consuming them from context
- Refactored RowsAndColumnsProvider to use useEffect pattern instead of state-based fetch management
- Updated CellNavigationProvider to accept numRows and optional dataId props
- Migrated tests from DataProvider.test.tsx to useData.test.ts with appropriate adjustments
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/useData.ts | New hook that manages data frame state (dataId, version, numRows) by subscribing to data frame events |
| test/hooks/useData.test.ts | New test file for useData hook, migrated from DataProvider tests |
| src/components/HighTable/HighTable.tsx | Now includes the former Wrapper component logic and uses useData hook |
| src/components/HighTable/Slice.tsx | Updated to receive data, numRows, and version as props |
| src/providers/CellNavigationProvider.tsx | Updated to accept numRows and dataId as props instead of using DataContext |
| src/providers/SelectionProvider.tsx | Updated to accept onError prop instead of using ErrorContext |
| src/providers/ScrollProvider.tsx | Updated to accept numRows as prop |
| src/providers/RowsAndColumnsProvider.tsx | Refactored to use useEffect for fetching, accepts data/numRows/onError as props, uses experimental useEffectEvent |
| test/providers/CellNavigationProvider.test.tsx | Updated tests to pass numRows and dataId props directly |
| test/providers/DataProvider.test.tsx | Deleted (tests migrated to useData.test.ts) |
| src/contexts/DataContext.ts | Deleted (replaced by explicit prop passing) |
| src/contexts/ErrorContext.ts | Deleted (replaced by explicit prop passing) |
| src/providers/ErrorProvider.tsx | Deleted (onError now passed as prop) |
| src/components/HighTable/Wrapper.tsx | Deleted (logic merged into HighTable.tsx) |
Comments suppressed due to low confidence (1)
src/hooks/useData.ts:8
- The JSDoc comment mentions "through the DataContext" but DataContext has been removed in this refactoring. The comment should be updated to reflect that this is now a hook that returns data frame state.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove Wrapper, by changing DataProvider to a hook (useData), and removing ErrorProvider.
Also: