Uh oh!
There was an error while loading. Please reload this page.
plugin-dashboard: hoist the "no rows yet" empty to module scope in ObjectDataTable and ObjectPivotTable - #5566
Merged
Conversation
ObjectDataTable and ObjectPivotTable both spelled the resolved row list as `Array.isArray(rawData) ? rawData : []`, evaluating a fresh array literal on every render. In ObjectDataTable that value keys the `derivedColumns` memo; in ObjectPivotTable it is handed to PivotTable and keys its cross-tabulation memo. Same module-scope frozen empty data-table.tsx already uses (#4618, PR #4623). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
… swept-out findings Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 21, 2026 12:59
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes#4629
ObjectDataTableandObjectPivotTableboth spelled the resolved row list asArray.isArray(rawData) ? rawData : [], evaluating a fresh array literal on everyrender. Both now fall back to a module-scope
Object.freeze([])— the same shapedata-table.tsxadopted for its ownEMPTY_ROWSunder #4618 / PR #4623, applied tothe
provider: 'object'siblings. No second remedy was invented: nouseMemowrapperaround the conditional, no change to any memo's dependency list.
Verification
Gate union run at HEAD
c8fd1be97(the final commit), all green:pnpm exec turbo run lint --filter=@object-ui/plugin-dashboardTasks: 2 successful, 2 total·0 errors, 360 warningspnpm exec turbo run type-check --filter=@object-ui/plugin-dashboardTasks: 13 successful, 13 total(incl.tsc --noEmit && tsc -p tsconfig.test.json)pnpm exec vitest run packages/plugin-dashboard/Test Files 72 passed (72)·Tests 653 passed (653)node scripts/check-control-bytes.mjscheck-control-bytes: OK (scanned 4620 tracked text file(s))node scripts/check-changeset-presence.mjs4 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)node scripts/check-changeset-no-major.mjsNo changeset declares a major bump.node scripts/check-changeset-fixed.mjsAll workspace packages are in the changeset fixed group.node scripts/check-lint-coverage.mjslint coverage: 46/46 packages linted, 0 with outstanding errorsnode scripts/check-type-check-coverage.mjstest type-check coverage: 41/41 packages compile their tests, 0 declared debtpnpm check:self-importNo package names itself inside its own src/.pnpm check:phantom-depsEvery in-scope import is declared by the package that publishes it.Declared narrowing of
pnpm lint. Repo-widepnpm lint(turbo across 46 packages)was narrowed to this package's own declared
linttask. Three pieces of evidence, notan assertion: (1) the population is eslint's own config resolution — the package script
is literally
eslint ., no hand-picked file list; (2) 102 files linted, read from--format jsonoutput length,0 errors / 360 warnings; (3) the narrowing can excludenothing, because
eslint.config.jsenables no type-aware linting (noparserOptions.project, noprojectService, no*TypeCheckedpreset) and none of theseven rules in
eslint-rules/reads any file other than the one under lint (noreadFileSync/globSync/readdirSync), so this diff cannot move the verdict on anyfile outside the 102.
check-lint-coverage.mjsindependently reports 46/46 packageslinted with 0 outstanding errors repo-wide.
The ESLint warning, and why clearing it is not the evidence
On
origin/main,ObjectDataTable.tsx:310:9reported exactly what the card quoted:It is gone on this branch (measured both states in one trap-guarded run; the mutation
was confirmed on disk by anchored
grep -con the reverted text, not by an editor'sexit code). But a suppression would clear it too, so the identity is pinned directly.
ObjectPivotTablehad no such warning onorigin/main— itsfinalDatareaches nohook in that file, it flows to
PivotTablethrough props. The sweep therefore found asite the linter could not have flagged.
Tests: what they assert, and the reverse verification
Nothing renders wrong before or after, so a "the table renders correctly" assertion is
green against the broken code. The two new suites assert the two forms the churn takes:
ObjectDataTable.stableEmptyRows.test.tsx— recompute count. OnebuildFieldMetacall per declared column per memo evaluation, counted through a partial mock of
../recordFields. A load-bearing pre-assertion (toBe(2)at mount) keeps it frompassing vacuously, and it also asserts the table is not rendered in that window.
ObjectPivotTable.stableEmptyRows.test.tsx— direct identity. The array handed toPivotTablemust be one and the same object across host re-renders, and frozen.Reverse verification, directions written before the run and both matched:
ObjectDataTablerecompute countexpected 8 to be 2ObjectPivotTableidentityexpected 4 to be 1The fix was committed first so the revert had a restore point; the tree was restored
byte-identically afterwards (
git diff --exit-code HEADclean). The script carried atrap … EXIT INT TERMrestore, so a foreground-capSIGTERMmid-mutation could nothave left the tree mutated for later measurements.
Sweep disposition — every site measured, none silently skipped
ObjectDataTable.tsx:310finalDataderivedColumns(dep list:417); ESLint-reportedObjectPivotTable.tsx:154finalDataPivotTableasschema.data, keys its cross-tab memoObjectDataTable.tsx:162colscomputeLookupExpand, a module-scope pure function, not a component body;colsnever escapes the callPivotTable.tsx:176+ thedata: rawData = []destructuring defaultorigin/main: "The 'data' conditional could make the dependencies of useMemo Hook (at line 241) change on every render"DashboardFilterBar.tsx:300.then()callback; feedssetDynamicOptions(pairOptionRows(...)), not a per-render literalDashboardWithConfig.tsx:111-112selectedWidgetConfig = React.useMemo(...)(deps[selectedWidgetId, configVersion])DatasetWidget.tsx:400-401useMemoDatasetWidget.tsx:529.then(); asetStatepayloadObjectMetricWidget.tsx:253useCallbackbody; local to the callObjectDataTable.tsx:162— the dispatch listed this as in-surface "same defect shape";measured otherwise and left alone. It sits inside
export function computeLookupExpand,which is not a component body.
colsis consumed only within that call (.length,.map) and never returned; the function's own result is a freshArray.from(out)onevery call regardless, and its single call site (
:263) is a fetch-effect body, not adependency list. By the card's own criterion — "only ever safe when nothing memoizes or
synchronizes on the result" — nothing does. Hoisting a shared empty there would imply a
render-identity concern that does not exist.
Two premise corrections, both measured
1. "The whole pre-fetch window" is too wide.
rawDataisboundData || schema.data || fetchedData, andfetchedDatais auseStateseeded with[]—an array with a stable identity from the first render. So the plain pre-fetch window
was never the churning case. The churn needs a truthy non-array: a provider-config
data, or abindpath that resolves to an object. This narrows the exposure; it doesnot change the fix, since the identity bug and the ESLint warning are unconditional.
2. The render-loop question — confirmed impossible, by a stronger argument than the
card's. In
ObjectDataTablethe fallback fires ifffinalData.length === 0, andthat is exactly the condition under which the component takes an early return (loading /
error / no-datasource / empty state) and never renders
SchemaRendererat all. So thechurn does not merely fail to feed a
setState— it never reachesdata-tablein thefirst place. Observation-class grading stands.
ObjectPivotTablehas no unconditionalempty-state early return, so there the churn did escape into
PivotTable's memo, whichis why it is the site with the direct identity assertion.
Filed out of scope (unassigned, not fixed here)
PivotTable's owndatafallback re-runs the cross-tabulation memo on every render — the third site of #4629's class, outside that card's file surface #5562 —PivotTable's own two per-render literals. finding(plugin-dashboard):PivotTable's owndatafallback re-runs the cross-tabulation memo on every render — the third site of #4629's class, outside that card's file surface #5562 remains open; the object-boundpath through
ObjectPivotTableis handled by this PR, the direct-use path is not.useSafeFieldLabelreturns a FRESH object every render — measured 4 distinct identities in 4 renders, re-keying every consumer memo it feeds #5564 — outside an i18next instance,useSafeFieldLabelreturns a fresh object everyrender (measured: 4 distinct identities in 4 renders, 1 with an instance). It re-keys
derivedColumnsindependently of this fix, which is why theObjectDataTablesuiterenders inside a real
I18nProviderand says so. finding(i18n): outside an i18next instanceuseSafeFieldLabelreturns a FRESH object every render — measured 4 distinct identities in 4 renders, re-keying every consumer memo it feeds #5564 is untouched here.Generated by Claude Code