fix: Resolve "setState during render" React warning in BackgroundRunning - #86
fix: Resolve "setState during render" React warning in BackgroundRunning#86Utkarshpandey0001 wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate the React warning caused by updating BackgroundRunning state during a child Table render, and also introduces shared column-sorting logic used when building gadget datasource field lists.
Changes:
- Replaces
BackgroundRunning’s stored table instance state with auseRefand attempts to sync selected row count via an effect. - Adds
getSortedColumnsutility and applies it when deriving datasource fields inresourcegadgets.tsxandGadgetContext. - Minor formatting/style adjustments in callbacks and a reduce block.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/gadgets/utility.tsx | Adds getSortedColumns and minor formatting; used to reorder datasource fields. |
| src/gadgets/resourcegadgets.tsx | Uses getSortedColumns when computing datasource fields. |
| src/gadgets/backgroundgadgets.tsx | Refactors table instance handling to avoid setState during render (but currently incomplete/incorrect). |
| src/common/GadgetContext/index.tsx | Uses getSortedColumns when preparing datasource fields in context state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return [...columns].sort((a, b) => { | ||
| const idxA = preferredOrder.indexOf(a); | ||
| const idxB = preferredOrder.indexOf(b); | ||
| if (idxA !== -1 && idxB !== -1) return idxA - idxB; | ||
| if (idxA !== -1) return -1; | ||
| if (idxB !== -1) return 1; | ||
| return 0; | ||
| }); | ||
| } | ||
|
|
||
| // Fallback: prioritize k8s columns | ||
| const k8sOrder = ['k8s.node', 'k8s.namespace', 'k8s.podName', 'k8s.containerName']; | ||
| return [...columns].sort((a, b) => { | ||
| const isAK8s = a.startsWith('k8s.'); | ||
| const isBK8s = b.startsWith('k8s.'); | ||
| if (isAK8s && !isBK8s) return -1; | ||
| if (!isAK8s && isBK8s) return 1; | ||
| if (isAK8s && isBK8s) { | ||
| const idxA = k8sOrder.indexOf(a); | ||
| const idxB = k8sOrder.indexOf(b); | ||
| if (idxA !== -1 && idxB !== -1) return idxA - idxB; | ||
| if (idxA !== -1) return -1; | ||
| if (idxB !== -1) return 1; |
There was a problem hiding this comment.
getSortedColumns does preferredOrder.indexOf(...) inside the sort comparator, which makes sorting O(n^2 log n) in the worst case. If column lists can be large, precompute a lookup map (column -> order index) once and use that in the comparator to keep it O(n log n).
| return [...columns].sort((a, b) => { | |
| const idxA = preferredOrder.indexOf(a); | |
| const idxB = preferredOrder.indexOf(b); | |
| if (idxA !== -1 && idxB !== -1) return idxA - idxB; | |
| if (idxA !== -1) return -1; | |
| if (idxB !== -1) return 1; | |
| return 0; | |
| }); | |
| } | |
| // Fallback: prioritize k8s columns | |
| const k8sOrder = ['k8s.node', 'k8s.namespace', 'k8s.podName', 'k8s.containerName']; | |
| return [...columns].sort((a, b) => { | |
| const isAK8s = a.startsWith('k8s.'); | |
| const isBK8s = b.startsWith('k8s.'); | |
| if (isAK8s && !isBK8s) return -1; | |
| if (!isAK8s && isBK8s) return 1; | |
| if (isAK8s && isBK8s) { | |
| const idxA = k8sOrder.indexOf(a); | |
| const idxB = k8sOrder.indexOf(b); | |
| if (idxA !== -1 && idxB !== -1) return idxA - idxB; | |
| if (idxA !== -1) return -1; | |
| if (idxB !== -1) return 1; | |
| const preferredOrderIndex: Record<string, number> = Object.fromEntries( | |
| preferredOrder.map((column, index) => [column, index]) | |
| ); | |
| return [...columns].sort((a, b) => { | |
| const idxA = preferredOrderIndex[a]; | |
| const idxB = preferredOrderIndex[b]; | |
| const hasIdxA = idxA !== undefined; | |
| const hasIdxB = idxB !== undefined; | |
| if (hasIdxA && hasIdxB) return idxA - idxB; | |
| if (hasIdxA) return -1; | |
| if (hasIdxB) return 1; | |
| return 0; | |
| }); | |
| } | |
| // Fallback: prioritize k8s columns | |
| const k8sOrder = ['k8s.node', 'k8s.namespace', 'k8s.podName', 'k8s.containerName']; | |
| const k8sOrderIndex: Record<string, number> = Object.fromEntries( | |
| k8sOrder.map((column, index) => [column, index]) | |
| ); | |
| return [...columns].sort((a, b) => { | |
| const isAK8s = a.startsWith('k8s.'); | |
| const isBK8s = b.startsWith('k8s.'); | |
| if (isAK8s && !isBK8s) return -1; | |
| if (!isAK8s && isBK8s) return 1; | |
| if (isAK8s && isBK8s) { | |
| const idxA = k8sOrderIndex[a]; | |
| const idxB = k8sOrderIndex[b]; | |
| const hasIdxA = idxA !== undefined; | |
| const hasIdxB = idxB !== undefined; | |
| if (hasIdxA && hasIdxB) return idxA - idxB; | |
| if (hasIdxA) return -1; | |
| if (hasIdxB) return 1; |
|
Let me get rid off conflicts |
3a92f37 to
2ab61a9
Compare
|
@illume hi , I have updated the PR to fix those typeScript grid errors and some formatting issues. I got few conflicts that now has been solved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tableInstanceRef.current = table; | ||
| return null; | ||
| }} | ||
| /> |
There was a problem hiding this comment.
This PR is scoped (per title/description) to fixing the React "setState during render" warning in BackgroundRunning, but it also includes a number of unrelated UI/layout refactors (Grid→Box), import reordering, and utility sorting changes. Consider splitting the non-warning-related changes into a separate PR to keep review risk and regression surface area low.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
There was a problem hiding this comment.
Thanks for fixing the TS, grid, formatting and conflict issues. Much appreciated 🎉
Would you mind having a look at the copilot suggestions?
|
@illume Sure , i am going through the reviews and will fix it. Thanks for words. |
8d7bb73 to
3b35b76
Compare
|
Thanks @Utkarshpandey0001 for the changes |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3b35b76 to
6cfef59
Compare
|
@illume this time i have refined the commit as copilot asked to do a little change in backgroundgadgets.tsx to eliminate mutation for safe-behaviour. I fixed it and reviewed it carefully by running it locally. |
fix #85
This PR resolves a problematic React pattern where state updates were being performed during the render phase of the
BackgroundRunning component. Specifically, the renderTopToolbarCustomActions prop in the Table component was being used to directly call setTableInstance and setSelectedCount, which are state setters for the parent component. This violated React's core rendering principles and triggered a "Cannot update a component while rendering a different component" warning in the browser console.
Review:
Reviewers should navigate to the Gadgets section in the sidebar and ensure the "Running Instances" table is visible (this requires an active Inspektor Gadget connection). Open the browser's developer console and verify that no React warnings regarding "setState during render" or "updating a component while rendering" appear when the table renders or when rows are selected.
Test:
The plugin was built and verified successfully using the following command: npm run build.
I also manually verified the fix by temporarily bypassing the connection check to force the table to render in a local environment. I confirmed that the React warning, which was previously visible in the console, disappeared entirely after applying these changes.