Repository navigation
fix(plugin-dashboard): route object-metric's drill through the shared DrillDownDrawer - #8999
Merged
Merged
Conversation
… DrillDownDrawer
`ObjectMetricWidget` hand-rolled its own drill panel — a Radix `Dialog` when
`drillDown.target` was the literal `dialog`, a `Sheet` otherwise, with a
record-list body it built itself. That copy read `enabled`, `target`'s two
in-place arms, `title` and `report`, and had no read site for the rest of the
`DrillDownConfig` the designer inspector offers. Three members were accepted
from the author and discarded on this block alone while acting on every other
block that shares the config: `columns`, `maxRows` and `target: 'navigate'`.
`target: 'navigate'` was the sharpest, because the declared contract states a
CONDITIONAL fallback — "falls back to 'drawer' when none is available" — and
the inline copy fell back even when a host handler was available, reading as
the documented behaviour while being a different one.
Routing through the shared component delivers all three and deletes the
duplicate. Two behaviour changes come with it, both deliberate: the drilled
list loses its search box (the inline panel never opted out of the record
table's default; the shared drawer does, so a metric drill is now the same
self-contained peek as every other drill), and the sheet's widest breakpoint
narrows one step to the shared `lg:max-w-4xl`. The default page size is kept
at the 25 the inline panel hard-coded rather than dropping to `data-table`'s
default of 10, so a config that never authored `maxRows` drills as before.
`drillDown.filter` and `drillDown.mode` are unchanged and still unread here:
the shared drawer honours neither, a metric has no click event for `${event.*}`
to resolve against, and the block's registration promises the drilled list
agrees with the number. objectui#8970 asks for a judgement on those two; this
change does not make it.
The member pin from objectui#8071 slice 9 passes unchanged against the shared
drawer — nothing had to be deleted for these members to become live, which is
what its stated restraint was for.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB
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-steve
marked this pull request as ready for review
September 10, 2026 16:47
This was referenced Sep 10, 2026
This was referenced Sep 10, 2026
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 free
to 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 #8970
ObjectMetricWidgethand-rolled its own drill panel. Triage's ruling was to fix it by routing the block through the sharedDrillDownDrawer, not by re-implementing the dropped members inline — "Two implementations of one drawer is the defect; teaching the copy five more members guarantees a sixth divergence." That is what this does.The member enumeration, re-measured (the card's "five" splits 3 / 2)
DrillDownConfighas eight members. Measured on both implementations rather than taken from the card:DrillDownDrawerenabledisDrillEnabledopentarget—drawer/dialogtarget: 'navigate'navigateOnlytitleresolveDrillTitletitlereportcolumnsmaxRowsfiltermodeDrillDownDrawer" does not survive measurement. Rerouting closes three, not five:modeis not aDrillDownDrawerPropsmember at all. Its only read site in the repo isObjectDataTable.tsx((drillDown?.mode ?? 'record') === 'record'), where a table row decides drill-to-record. Routing through the drawer cannot deliver it.filteris read bycomputeDrillFilterin@object-ui/core, called byObjectChartandObjectPivotTable— not by the drawer, which takes an already-computedfilterprop.Both are exactly the two the filing seat flagged as possibly a deliberate refusal, and said "That is a judgement nobody has made." This PR does not make it. See the open question at the bottom.
Dependency measurement
DrillDownDrawerlives in the same package (packages/plugin-dashboard/src/DrillDownDrawer.tsx), so the new import is relative —./DrillDownDrawer. Its own imports arereact(already a peer dep),@object-ui/componentsand@object-ui/react(already dependencies), and two same-package siblings. No edge added todependencies,peerDependenciesordevDependencies. Not a manual-floor item.The net import surface shrank:
ObjectMetricWidgetno longer importsSheet/Dialogfrom@object-ui/components,SchemaRendererfrom@object-ui/react, or./OpenInListButton.What routing through the shared drawer changed
Nothing broke. The metric tile's own layout is untouched — the hand-rolled panel was a drawer, not a tile constraint. Three differences exist between the two implementations and each is named rather than papered over:
pageSize: 25; the shared drawer passespageSize: maxRows, which isundefinedwhen unauthored and would fall todata-table's default of 10. AMETRIC_DRILL_PAGE_SIZE = 25fallback keeps a config that never namedmaxRowsdrilling exactly as before, while an authoredmaxRowsnow wins.searchable, so it inheriteddata-table's default oftrue; the shared drawer setssearchable: false. This is inherited-default drift, not a constraint the inline version handled, and adopting the shared value makes a metric drill the same self-contained peek as a pivot, chart or dataset drill. Adding asearchableprop to the shared drawer for one caller would be the sixth divergence triage warned about.lg:max-w-5xlto the sharedlg:max-w-4xl. The centreddialogarm is byte-identical between the two and is unchanged.Two latent problems in the inline copy go away with it: the report body built
{ type: 'spec-report', ...report }, which clobbered the inner report's owntypeand never produced the nestedreportlimbReportRendererunwraps; and an explicitly-passeddataSourceprop never reached the drilled table, because the inline path rendered a bare schema and relied on context.Acceptance — each member asserted on its EFFECT
packages/plugin-dashboard/src/__tests__/ObjectMetricWidget.drillRoutedToSharedDrawer-8970.test.tsx, mounted through the registeredobject-metricblock, 7 cases. Every case reads something the drilled list did, never that a prop reached a mock — a test that only checked a drawer appears would have passed against the hand-rolled panel too, which also drew a drawer.target: 'navigate'— with a hostopenRecordList, clicking the tile calls it with the metric's own resolved filter and draws no panel. Two controls: the same host handler withtarget: 'drawer'draws the panel and never navigates (so the row readstarget, not the handler's presence); andtarget: 'navigate'with no host handler still draws the sheet, which is the contract's conditional half —DrillDownConfig.targetpromises the fallback happens only "when none is available", and the bug was that it happened either way.columns—columns: ['name']renders exactly one column header; the control without the member auto-derives all three.maxRows— with twelve records andmaxRows: 2, two body rows render. The control without the member renders all twelve, which also pins the preserved 25 (the shared drawer's own fallback of 10 would show ten of the twelve).The panel is located by
role="dialog", which both implementations satisfy, rather than by the shared drawer'sdata-testid. That is what makes the ablation below legible.Ablation — the fix removed, from the committed state
git checkout BASECOMMIT -- packages/plugin-dashboard/src/ObjectMetricWidget.tsx(BASECOMMIT spelled as a word on purpose — this repo's body sanitizer eats tag-shaped placeholders), proven on disk before running (DrillDownDraweroccurrences 4 to 0,SheetContent0 to 3, on-disk blob hash equal to the base blob and unequal to the HEAD blob), restored by atrapto the HEAD blob and verified bygit diff HEADbeing empty.3 red, 4 green — one red per delivered member, each with its control still green:
target: 'navigate'navigates, no panelexpected "vi.fn()" to be called with arguments: [ 'deal', { stage: 'won' } ]target: 'drawer'controltarget: 'navigate'no-handler fallback controlcolumns: ['name']renders one columnexpected [ 'name', 'stage', 'amount' ] to deeply equal [ 'name' ]maxRows: 2renders 2 rowsexpected 12 to be 2All 13 cases of the objectui#8071 slice 9 member pin pass in both arms. That is the point of the filing seat's restraint: nothing had to be deleted for these members to become live. ⛔ No pin asserting the dead behaviour was added.
Gate table
--filter '@object-ui/plugin-dashboard^...')vitest run packages/plugin-dashboard/vitest run apps/console/src/__tests__/registry-inputs-spec-parity.test.tsturbo run type-check(plugin-dashboard + console)tsc -p tsconfig.test.json, so the new test file is type-checkedturbo run lint(plugin-dashboard + console + root)ObjectMetricWidget.tsxwarnings 16 to 11,as any5 to 0check-control-bytes.mjscheck-changeset-presence.mjscheck-governed-queue-guard.mjs --testcheck:new-line-citationscheck:doc-typescheck:doc-snippetscheck:doc-examplescheck:vi-mock-{specifiers,inherit,override-shape}check:unreferenced-sourcesHeavy runs were serialized through the shared verify lock.
Acceptance notes
ObjectChartis a third implementation of this drawer (packages/plugin-charts/src/ObjectChart.tsx, its ownSheet/Dialogat the tail of the file plus its ownnavigateOnly). The dispatching assumption thatobject-metricwas the only block hand-rolling a drill panel is therefore false. It is not touched here and it drops no members — it readscolumns,maxRows,filterand thenavigatearm itself — so it is named for a follow-up rather than filed as a dropped-metadata finding. Its registration description does understate its own contract (target?: 'drawer' | 'dialog'while the code implementsnavigate). Successor: whoever next consolidates drill panels; objectui#8944 is already open against the same file's drill path.columnsis authored, and as humanized labels when it is not. Both branches callfieldLabel; only the fallback differs (ObjectDataTable.tsxpasses the humanized form on the auto-derive branch and the caller'sheaderon the whitelist branch, and bothDrillDownDrawerandObjectChartset thatheaderto the raw field name). It predates this card, belongs to the shared drawer rather than toobject-metric, and would change headers for four widgets, so it is not fixed here. It is why the new test compares column names case-folded, with the reason written into the helper.Open question left to the card
drillDown.filteranddrillDown.modestill have no read site onobject-metric, and this PR does not add one — the shared drawer honours neither, so the reroute does not settle them. The card's own framing stands: a metric has no click event for${event.*}to interpolate against, and the block's registration promises "the same filter narrows the drill-down list, so the number and the records behind it always agree", which an author-suppliedfilteroverride would break. If refusal is the intent, the remedy is to say so in the registration text or the shared type rather than to implement them. That judgement is a maintainer call, and this PR does not pre-empt it. Note the trailer at the top of this body closes the card on merge, per the dispatching seat's explicit instruction; if the maintainer wants the two-member judgement tracked, it needs its own card, because a closed card drops out of every open-issue sweep. Flagged back to the dispatching seat rather than decided here.Authored by an agent seat via Claude Code; the session reference is
https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB(in prose and as a code span, because the footer form does not survive an edit to this body).Generated by Claude Code