perf(ui): single-commit GenericProvider column registration (R1) - #31023
perf(ui): single-commit GenericProvider column registration (R1)#31023harsh-vador wants to merge 1 commit into
Conversation
Nine entity detail tabs push their sorted/filtered/paginated column list into GenericProvider via a useEffect + useState setter. A setter fired from a child effect re-renders the provider after the child commits, so every entity page double-renders on load. The value's only consumer is ColumnDetailPanel's `allColumns`, which is mounted only when a column is selected (user action). Store the child registration in a ref (no re-render), and mirror it into `panelColumns` state only while the panel is open, snapshotting at open time. Load path is now one commit; prev/next navigation over the displayed list is unchanged. No child or context-interface changes. Refs #5446 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
✅ Playwright Results — workflow succeededValidated commit ✅ 550 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 1h 3m 32s ⏱️ Max setup 2m 58s · max shard execution 18m 53s · max shard-job elapsed before upload 22m 22s · reporting 4s 🌐 203.62 requests/attempt · 2.83 app boots/UI scenario · 10.86% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const setDisplayedColumns = useCallback((cols: ColumnOrTask[]) => { | ||
| displayedColumnsRef.current = cols; | ||
| // Keep the open panel's navigation list live; when closed, skip the re-render. | ||
| if (selectedColumnRef.current) { | ||
| setPanelColumns(cols); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
⚠️ Edge Case: Deep-linked column panel loses child's sorted/paginated navigation list
On the columnFqn deep-link path (GenericProvider.tsx:156-164), setSelectedColumn runs but panelColumns is never seeded. Because children register their displayed list via setDisplayedColumns during their mount effect — which fires before the provider's selectedColumnRef.current = selectedColumn effect (line 120-122) commits — the gate at line 109 is still null, so setPanelColumns is skipped and the ref-write is dropped. As a result columnsForPanel falls back to extractedColumns, so prev/next navigation in a deep-linked/shared-URL column panel follows the unsorted, non-paginated order instead of the child's displayed list (previously preserved because displayedColumns was React state). The click-to-open path is unaffected because it snapshots the ref directly. Fix: seed panelColumns when selecting a column on the deep-link path.
Snapshot the child's displayed list into panelColumns when opening the panel via deep link, mirroring openColumnDetailPanel.:
// Sync selected column from prop (deep link)
useEffect(() => {
// If we have a direct columnFqn from props, try to find and select it
if (columnFqn && extractedColumns.length > 0) {
const col = findFieldByFQN(extractedColumns as Column[], columnFqn);
if (col) {
setPanelColumns(
displayedColumnsRef.current.length > 0
? displayedColumnsRef.current
: extractedColumns
);
setSelectedColumn(cleanColumn(col));
}
}
}, [extractedColumns, columnFqn, cleanColumn]);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source
harsh-vador
added
the
safe to test
Jest test CoverageUI tests summary
|
|
1 participant
Problem
Nine entity-detail tabs (
SchemaTable,ModelTab,SearchIndexFieldsTable,TopicSchema,ContainerDataModel,PipelineTaskTab,MlModelFeaturesList,WorksheetColumnsTable,APIEndpointSchema) push their sorted/filtered/paginated/searched column list intoGenericProvidervia auseEffect+useStatesetter. A setter fired from a child effect re-renders the provider after the child commits, so every entity detail page double-renders (2 commits) on load.Fix
displayedColumns' only consumer iscolumnsForPanel→ColumnDetailPanel'sallColumns, which is mounted only when a column is selected (a user action). So the value is never needed on the initial, panel-closed render.useRef(displayedColumnsRef) — the write no longer re-renders the provider.setDisplayedColumnsis now a stableuseCallback; it mirrors into newpanelColumnsstate only while the panel is open (selectedColumnRefgate).openColumnDetailPanelsnapshots the ref at click-time;columnsForPanelreadspanelColumns.Result: load path is one commit; prev/next navigation over the displayed list is byte-for-byte unchanged. No child changes, no context-interface change.
Testing
GenericProvider+ all 6 child specs +ColumnDetailPanel— 10 suites / 127 tests pass, unmodified.tsc --noEmit: no new errors (2 pre-existingWidgetConfig[]layout errors onmainare untouched).lint:baseclean.Part of the UI Quality Audit epic — open-metadata/openmetadata-collate#5442, ticket open-metadata/openmetadata-collate#5446 (R1).
🤖 Generated with Claude Code