Uh oh!
There was an error while loading. Please reload this page.
refactor(plugin-dashboard): drop the unreachable DatasetWidget arm from DashboardRenderer's self-contained branch - #5604
Conversation
…om the self-contained branch `isSelfContained` is `widget.type === 'metric' && !datasetBound`, and the `isSelfContained` arm of `renderedNode` forked on `datasetBound` a second time. The `datasetBound` side of that inner fork could never execute: reaching it required `isSelfContained`, which requires `!datasetBound`. Render `SchemaRenderer` unconditionally there and name the invariant in a comment so the arm is not re-added by the next person mirroring this shape. The reachable fork in the Card branch — the one that gives a dataset-bound metric its title and border chrome — is untouched, and behaviour is unchanged. Fixes#4620 Co-Authored-By: Claude <noreply@anthropic.com>
✅ 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
commented
Aug 21, 2026
PM review — ACCEPT (card #4620)Gates. 22 named check runs read individually for The reachability proof is a proof, not a restatementThe card's line anchors had drifted, and re-reading them at their new numbers would only have re-asserted what the card already claimed. What makes this sound is the step past that: That closure is the difference between a reachability argument and a guess, and it is what I could not have taken on faith from the card. The dispatch's stop condition was correctly evaluated rather than assumed away: the Coverage named, not assumedI asked for an honest "not found" if the Card-path coverage did not exist. It does, and it is named: Better still, that pin was shown to bite: forcing the reachable Card fork to its No new test, correctly. A pin asserting that deleted code does not run would only re-state the deletion. Independent corroboration I did not ask forCI's own bundle bot settles a question the reachability argument leaves open — whether the arm was emitted at all, or had already been tree-shaken so that deleting it changed nothing. Three PRs of mine ran concurrently off the same merge-base
The arm was real emitted code. The deletion is a real deletion. Fence held exactly — Not fixed here, recorded elsewhere: the write path ate this PR's report-comment marker and appended a duplicate footer to the body. Both are logged on #5581 with the reasoning for leaving the body alone; neither affects this diff. Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#4620
Deletion-class change: removes a JSX branch that cannot execute. Behaviour is unchanged.
Reachability, re-derived on current
origin/mainLine numbers in the card had drifted; these are the two lines as read at merge-base
7a28e1e3f, inpackages/plugin-dashboard/src/DashboardRenderer.tsx:const isSelfContained = widget.type === 'metric' && !datasetBound;{datasetBoundopening the inner fork inside theisSelfContainedarm ofrenderedNode(which begins at :882)datasetBoundis bound exactly once for this scope, at :570 (const datasetBound = !!widget.dataset;), insiderenderWidgetwhich opens at :545. It is aconst, never shadowed and never reassigned — its only other reads were :842, :850, :888 and :917. So reaching the truthy side of the :888 fork requiresdatasetBoundtrue, while reaching the enclosing arm at all requiresisSelfContainedtrue, which requiresdatasetBoundfalse. The arm was unreachable by construction.The guard on
isSelfContainedstill carries its&& !datasetBoundclause, i.e. the shape has not changed since the card was filed, so this stayed a deletion rather than becoming a behaviour question.The change
The self-contained arm now renders
SchemaRendererunconditionally, with a comment naming the invariant (isSelfContainedimplies!datasetBound) so the next person mirroring this shape does not re-add the dead limb — that re-addition is the cost the card documents, already paid once by the PR that mirrored this fork ontoDashboardGridLayout.The reachable fork in the Card branch (now at :917) is untouched. It is correct and it is the one giving a dataset-bound metric its title and border chrome.
DatasetWidgetandeffectiveWidgetboth remain referenced by that Card-branch fork, so nothing became unused and the module graph is unchanged.Evidence: the Card path is pinned by existing coverage, and the pin bites
The card asserted that existing dataset-bound-metric coverage already pins that such a widget takes the Card path. Verified rather than taken on trust — the test is:
packages/plugin-dashboard/src/__tests__/DashboardRenderer.filters.test.tsxat :162, "injects the merged filter into a dataset widget's runtimeFilter". It rendersDashboardRendererwith{ id: 'w1', type: 'metric', dataset: 'sales', values: ['revenue'], filter: { stage: 'won' } }(:171) and assertsqueryDatasetwas called with the mergedruntimeFilter.isSelfContainedis false for that widget, so the assertion can only pass by way of the Card-branch fork.Reverse-verification, to show the pin actually bites rather than merely mentioning a dataset metric — the reachable Card-branch fork was forced to its
SchemaRendererside on disk and the file restored viatrap ... EXIT INT TERM:Test Files 1 passed (1)·Tests 5 passed (5)Test Files 1 failed (1)·Tests 5 failed (5), the named test among themgit status --porcelainempty,git diff --statemptyBoth legs were proven on disk with anchored
grep -cin both directions (injected marker 1 then 0; the removed guard line 0 then 1) plusgit diff --stat. No rebuild was needed for this ablation: the test reaches the subject through the relative source specifier../DashboardRenderer, not through the package'sexports/dist.No new test is added. A test asserting that deleted code does not run would only re-state the deletion.
Gates — all at
901f369bc, exit codes captured before any pipepnpm --filter @object-ui/plugin-dashboard type-checktsc --noEmit && tsc -p tsconfig.test.json)pnpm --filter @object-ui/plugin-dashboard lint365 problems (0 errors, 365 warnings)— warnings all pre-existingpnpm exec vitest run packages/plugin-dashboard/(repo root)Test Files 73 passed (73)·Tests 657 passed (657)check-control-bytes.mjscheck-control-bytes: OK (scanned 4654 tracked text file(s); skipped 85 binary).check-changeset-presence.mjs1 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major.mjsNo changeset declares a major bump.check-changeset-fixed.mjsAll workspace packages are in the changeset fixed group.check-type-check-coverage.mjstype-check coverage: 45/46 via type-check … 0 errors outstandingcheck-lint-coverage.mjslint coverage: 46/46 packages linted, 0 with outstanding errors (0 total).The dependency closure was built first (
pnpm --filter '@object-ui/plugin-dashboard^...' build) — the first type-check run in the fresh worktree failed with module-resolution errors that read exactly like breakage but were only unbuilt dependencies.Declared narrowing. Gate set re-derived from
.github/workflows/against the actual diff rather than from the dispatch list. Left to CI: the repo-widepnpm lintandpnpm type-check,pnpm check(CLI self-check), the phantom-deps / self-import / esm-specifiers / spec-symbols / action-forward-parity / i18n gates, the two docs type gates (which carry no path filter and so run on every PR), and Bundle Analysis (triggered bypackages/**). The narrowing cannot hide a failure from this diff: it changes one JSX expression inside one function, adds and removes no import, no export, no string literal and not()call site, so the module graph the bundle and eager-closure gauges measure is byte-for-byte the same set of edges, and the i18n and docs gates read surfaces this diff does not touch. The package's own type-check and full test suite — the two gates that can actually see this edit — were run in full, not narrowed.Generated by Claude Code
Generated by Claude Code