-
Notifications
You must be signed in to change notification settings - Fork 0
Review 4377
PR #4377 · author AKnassa (external contributor) · opened 2026-07-26 · reviewed 2026-08-27
| HEAD REVIEWED | 36f240dce941a9b788ba6605cc8f39864c841b0f |
| Head state at review |
OPEN, mergeable: CONFLICTING, mergeStateStatus: DIRTY
|
| LOOP VERSION | 1.6.0 |
| AUDIT RUBRIC | 1.13 |
| Lane | full |
| Gates | 2 (gate 1 fail — verdict had no carrier; gate 2 pass) |
| Judgement | request changes |
| Author can proceed | yes — 5 acceptance criteria, all checkable |
Every number below is re-runnable from a named command over a named tree.
| # | measurement | result |
|---|---|---|
| 1 |
vitest run --project ui packages/vega packages/charts at head 36f240d
|
6 of 12 files fail — all on Failed to resolve import "@astryxdesign/core"; 48 tests passed. packages/core/dist absent. |
| 2 | same, with main's landed bare-specifier alias applied to the head config |
9 files / 79 tests pass, no build performed |
| 3 |
vega routed into ui
|
3 files / 30 tests pass (VegaChart 18, schema 8, vegaLiteConfig 4) |
| 4 | net-new core tests at head 36f240d
|
8 files / 106 tests pass |
| 5 | net-new tests on a correctly merged tree (current main merged in; ui/node lists resolved to core, lab, charts, richtext, vega; the 8 add/add files resolved to main's copies) |
4 tests fail / 132 pass across 11 files — 2 in useTooltip.test.tsx, 2 in TableHeaderCell.test.tsx
|
| 6 | drift of the branch | base fe8657a1 (2026-07-24); 815 commits on main since; 16 of 66 tested source files changed |
| 7 | conflict surface | 9 files, of which 8 are add/add on test files — i.e. not mechanical |
| 8 | assertion quality of the added tests | 465 tests · 1,117 assertions · toBe 268, toHaveAttribute 237, toHaveLength 133, toHaveBeenCalled 106; ~6% weak shapes. Not a tautological test dump. |
Nothing was pushed to the PR, and no code was pushed anywhere. The merge in
measurement 5 was local and disposable, made only to answer whether the tests
still hold against current main.
#4377 test: route charts/vega into the ui vitest project and cover the untested viz packages and hooks (#4295) by AKnassa (bucket: external contributor — read permission, absent from ENGOWNERS and DESIGNOWNERS)
36f240dce941a9b788ba6605cc8f39864c841b0f
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: fast-lane condition "no merge conflict" fails — mergeable: CONFLICTING, mergeStateStatus: DIRTY. Also 69 files and a root build-config change, so "one runtime decision of ≤50 lines" fails too.
WHY 1: packages/charts and packages/vega component tests could not run. The root vitest.config.ts ui project (jsdom + StyleX babel + jest-dom) listed only core and lab; every other package fell through to the node project, which deliberately does not extend the root config.
WHY 2: A contributor writing a component test in those packages gets no DOM, no StyleX compilation and no jest-dom matchers, so the test fails for reasons unrelated to the code under test — and reads as "this package cannot be tested".
WHY 3: Two shipped visualization packages therefore carried no regression net at all; vega had no test script and zero test files.
USER-FACING PROBLEM: a builder changing charts or vega had nothing that would catch a break, and no working path to add one.
PROBLEM SEVERITY: missing capability — evidenced: git ls-tree origin/main -- packages/vega/ shows zero *.test.* files; the PR base's ui include list contains only core and lab.
The diagnosis was correct at the PR's base commit (fe8657a1, 2026-07-24). It is now substantially stale — see IMPACT.
VERDICT: note — the problem was real and correctly diagnosed, but main has since solved the larger half of it.
The change teaches the root test config that two more packages need a browser-like environment, then adds test files across those packages and a set of previously-uncovered hooks. Responsibility sits in the root vitest.config.ts, which is the single place that decides which environment a package's tests run in. It solves the stated problem by moving charts and vega from the bare-node project into the jsdom one.
SOLUTION (2 decisions · ~10 runtime lines of 9,948 total lines)
- Route
packages/chartsandpackages/vegainto theuiproject and out of thenodeproject's include (4 added lines invitest.config.ts, plus a rewritten explanatory comment). - Add 66 test files (~9,900 lines) across
charts,vega,labandcore, plus atest/test:watchscript pair inpackages/vega/package.json.
BURDEN: low — no runtime state, no Effects, no listeners, no timers, no public surface, no dependency. The maintenance burden is 66 test files, which is the point of the PR rather than a cost of it. BURDEN MATCH: proportionate — 10 lines of config buys a working test environment for two packages.
Decision 2 carries a proven defect. Against a correctly merged tree (current origin/main merged into this head, ui include and node exclude each resolved to core, lab, charts, richtext, vega, the 8 add/add test files resolved to main's copies so only this PR's net-new tests are under measurement), 4 of the PR's own tests fail out of a 136-test sample:
| failing test | asserts | current main does |
|---|---|---|
useTooltip.test.tsx:144 "never shows on hover when the device reports no hover capability" |
onShow not called |
calls it — touchTrigger defaults to auto, which opens on tap where there is no hover |
useTooltip.test.tsx:274 "leaves a controlled tooltip open on Escape" |
onHide not called |
calls it — dismissal now routes through the shared useLayerDismissal
|
TableHeaderCell.test.tsx:199 |
border-right-width: var(--border-width) |
paints borderInlineEndWidth
|
TableHeaderCell.test.tsx:210 |
same physical declaration | same |
Second, independent confirmation (R16g), not a re-reading of the same run:
-
Executed —
vitest run --project uiover the merged tree: 2 files failed, 4 tests failed, 132 passed. -
Read —
packages/core/src/Table/TableHeaderCell.tsx:94declaresborderInlineEndWidth, andpackages/core/src/Tooltip/useTooltip.doc.mjs:73documentstouchTriggeras'auto' | 'tap' | 'none'with "auto opens on tap unless the trigger performs an action of its own". Both showmain's behavior was replaced deliberately, so these are the PR's tests being stale rather thanmainhaving regressed.
VERDICT: BLOCKS — 4 of this PR's tests assert behavior main has intentionally replaced, and fail on a correctly merged tree.
OWNER: root vitest.config.ts — the only place that partitions packages between the ui and node projects.
TIER 1: reuses the existing ui/node project split and the existing root StyleX/jsdom/jest-dom setup. No new mechanism.
TIER 2: none.
SEAMS: the ui/node partition is the seam; the invariant is that no package falls through both lists.
BEHAVIOR UNIT: not applicable — declarative config, no behavior code.
| seam | driven result |
|---|---|
ui project resolves charts' bare @astryxdesign/core import |
fails at the PR head — vitest run --project ui packages/charts → 6 of 12 files fail, Failed to resolve import "@astryxdesign/core" from packages/charts/src/Chart.tsx. packages/core/dist absent. |
same seam, with main's landed alias applied |
passes — 9 files / 79 tests, no build performed |
vega routed into ui
|
passes — 3 files / 30 tests (18 + 8 + 4) |
| no package falls through both project lists | at risk on merge — see BREAKING |
The PR resolves charts' bare-specifier import by depending on a build side-effect from a different vitest project (vitest.config.ts:90-92 at head: "The node project's globalSetup builds core, so a cold pnpm test with no dist on disk still passes"). main instead added a resolve alias mapping the bare specifier to source, so no build is needed. main's mechanism is the better one and it has landed.
VERDICT: note — the chosen mechanism is weaker than the landed alternative, but it is not broken under full pnpm test.
Reaches contributors to charts, vega, and the covered core/lab modules — nobody using the shipped packages. No runtime code changes.
The material impact is that main has moved 815 commits since this branch's base (2026-07-24) and has already absorbed the larger half of the PR:
-
#4382 (landed 2026-07-30, four days after this PR opened) routed
packages/chartsinto theuiproject — the same change decision 1 makes — and additionally added the bare-specifier resolve alias. -
#4678 (2026-08-14) added
packages/richtextto the same two lists. - Eight of the PR's 66 test files now already exist on
main, written independently by five other people while this sat:
| file | added on main | by | main tests | this PR's tests |
|---|---|---|---|---|
packages/charts/src/Chart.test.tsx |
2026-07-30 | Baraa Hamodi | 4 | 7 |
packages/lab/src/Chart/Chart.test.tsx |
2026-07-30 | Baraa Hamodi | 4 | 4 |
packages/lab/src/Radial/RadialChart.test.tsx |
2026-07-30 | Baraa Hamodi | 6 | 3 |
packages/lab/src/ThreeD/ThreeDChart.test.tsx |
2026-07-30 | Baraa Hamodi | 15 | 3 |
packages/lab/src/SVGIcon/SVGIcon.test.tsx |
2026-08-04 | Baraa Hamodi | 4 | 8 |
packages/core/src/Field/InputClearButton.test.tsx |
2026-08-13 | freddymeta | 11 | 7 |
packages/core/src/hooks/useScrollLock.test.ts |
2026-08-13 | alex-js-ltd | 6 | 10 |
packages/core/src/Typeahead/TypeaheadItem.test.tsx |
2026-08-26 | Cindy Zhang | 2 | 10 |
packages/vega is the part nobody else has touched: still zero tests on main, and still absent from both project lists.
VERDICT: note — 8 of 66 files duplicated, and the config change's charts half superseded; the vega half is unduplicated and still wanted.
No public API change. No barrel export added, removed or altered; no component prop or type touched.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
packages/vega/package.json "test": "vitest run --root ../.."
|
package-local script |
packages/charts already ships the same script |
n/a | ok |
+ |
packages/vega/package.json "test:watch": "vitest --root ../.."
|
package-local script | same | n/a | ok |
+ |
packages/core/src/__tests__/stylexDeclarations.ts |
internal test helper, not exported from the package barrel | joins fastRoleQueries.ts, forcedColors.ts, stubMatchMedia.ts in the same directory |
n/a | ok |
OSSIFICATION: none of the three is public surface. The two package scripts match a class that already exists (packages/charts ships the identical pair), and the test helper joins three landed siblings in packages/core/src/__tests__/. Cost of being wrong is a renamed script in a package's own package.json — near zero.
VERDICT: clear
No theming change. No themeProps, no theme target added, renamed or removed, no CSS variable, no token. The only styling-adjacent line in the diff is a test assertion about a border declaration (TableHeaderCell.test.tsx:199), which reads style output and does not define it.
VERDICT: clear
BEHAVIOR: no — no runtime code path changes. git diff --name-only shows 66 *.test.* files plus vitest.config.ts, packages/vega/package.json and one test helper; no component, hook or style source is touched.
API: no — evidenced in the API table above.
VISUAL: no — nothing rendered by the shipped packages changes.
THEME: no — evidenced in THEMING.
One breaking risk lives in the merge, not in the diff. main's config now lists core, lab, charts, richtext in both the ui include and the node exclude. This PR's version lists core, lab, charts, vega. A resolver who takes this PR's side on the ui include but main's side on the node exclude drops packages/richtext from the ui include while leaving it in the node exclude — richtext then matches neither project and its tests stop running silently, with CI still green. packages/richtext currently has 1 test file, so the blast radius is small, but the failure mode is silent.
VERDICT: note — no breaking change in the diff; one silent-coverage-loss trap in the conflict resolution, sized at 1 file.
EFFECTS: zero added, changed, kept or moved. No Effect, listener, observer, timer or rAF appears in the diff (grep -cE "useEffect|addEventListener|setTimeout|requestAnimationFrame" over added runtime lines = 0; the only hits are inside test bodies).
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
| none | — | — | — | — | — |
RENDER: no reachable change — no runtime code modified.
LISTENERS/OBSERVERS: none added.
LAYOUT: none.
BUNDLE: no dependency added, no lockfile change (pnpm-lock.yaml absent from the changed-file list). Both new package.json entries are scripts, not dependencies.
Test-suite cost: moving charts and vega into ui makes their files pay per-file jsdom instantiation. Measured on the merged tree, charts (9 files / 79 tests) took 16.9s wall and vega (3 files / 30 tests) ~0.11s of test time. This is the cost the PR intends to buy, not a regression.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: the diff contains no rendering path. Of 69 changed files, 66 are *.test.*, and the other three are vitest.config.ts, packages/vega/package.json (two script keys) and packages/core/src/__tests__/stylexDeclarations.ts (a test helper). No component source, no .stylex.ts, no CSS, no token, no story. There is no pixel this change can move, so there is no before/after to capture and no visual gate to read.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
Nothing in the diff touches accessibility or internationalization surface: no aria-*, no role, no focus management, no live region, no user-visible string, no locale key, no physical/logical CSS property in shipped code.
-
A18 (axe baseline) — auto-covered;
.github/a11y-baseline.jsonis not in the changed-file list, so no baseline entry is bought. -
I5 (catalog keys) — auto-covered;
packages/core/locales/en.jsonis untouched, so no key is added or removed. -
I8/I9 (logical properties) — checked by me, and this is the one place the slot has something to say.
TableHeaderCell.test.tsx:199and:210assert the physical declarationborder-right-width: var(--border-width).mainnow paints that edge with the logicalborderInlineEndWidth(packages/core/src/Table/TableHeaderCell.tsx:94). The test is not shipped code, so it breaks no RTL user — but it encodes the pre-logical-property shape and fails against currentmain. - Everything else in §1/§9: not applicable — no shipped surface touched.
VERDICT: note — no user-reachable a11y or i18n effect; one test assertion encodes a physical property main has already migrated away from.
| slot | verdict |
|---|---|
| PROBLEM | note — real, correctly diagnosed, now substantially stale |
| SOLUTION | BLOCKS — 4 tests assert behavior main replaced; fail on a correctly merged tree |
| ARCHITECTURE | note — weaker mechanism than the landed alternative |
| IMPACT | note — 8 of 66 files duplicated; charts half superseded |
| API | clear |
| THEMING | clear |
| BREAKING | note — silent-coverage-loss trap in the conflict resolution, sized at 1 file |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | note — one test asserts a physical property main has migrated |
GOAL: partly met — measured. The vega half of the goal is met and unduplicated: routing vega into ui yields 3 files / 30 tests passing on a correctly merged tree, and vega still has zero tests on main. The charts half was met by #4382 on 2026-07-30. Against a correctly merged tree, 4 of the PR's tests fail (2 in useTooltip.test.tsx, 2 in TableHeaderCell.test.tsx) because they assert behavior main has since intentionally replaced.
CARRIER (R16e): the verdict is carried by one finding — the 4 tests that fail on a correctly merged tree (SOLUTION BLOCKS). Everything else below is context that shapes the ask; none of it is a charge against the author.
DISPOSITION:
-
4 tests failing against current
main→ blocks now. Author-actionable, proven twice, isolated to two files. -
chartsconfig half superseded by #4382 → not a charge — our latency. The branch opened 2026-07-26 andmainmoved 815 commits under it. It is the reason the PR must be narrowed, not a defect the author introduced. - 8 duplicate test files → not a charge — our latency, same cause. Per-file keep/drop is a maintainer call; in four of the eight
mainhas more tests and in four this PR does. - Bare-specifier mechanism weaker than
main's alias → accepted; resolved for free by rebasing ontomain, which already carries the alias. -
richtextsilent-drop trap → accepted with a named acceptance criterion (the mergeduiinclude must listcore, lab, charts, richtext, vega).
ADVICE: proven existing pattern — rebase onto main and keep main's vitest.config.ts, adding only the two vega lines. Driven: applying exactly that resolution on a local merge produced 0 conflicts and 132 of 136 sampled tests passing, with the 4 known failures isolated to the two stale files.
AUTHOR CAN PROCEED: yes — acceptance criteria: (1) rebase onto current main; (2) the ui include and node exclude each list core, lab, charts, richtext, vega; (3) drop or reconcile the 8 test files that now exist on main; (4) update the 2 useTooltip and 2 TableHeaderCell assertions to current behavior; (5) pnpm test green.
WORST OUTCOME: "A contributor resolves this conflict the obvious way, packages/richtext silently stops being tested, and CI stays green." → note (sized at 1 file, and prevented by acceptance criterion 2)
JUDGEMENT NEEDED: none
request changes
-
[carries the verdict] Four tests assert behavior
mainhas since replaced → whoever merges this gets a red suite:useTooltip.test.tsx:144and:274predatetouchTrigger/useLayerDismissal;TableHeaderCell.test.tsx:199and:210assertborder-right-widthwhere main paintsborderInlineEndWidth·packages/core/src/Table/TableHeaderCell.tsx:94 -
[context, not a charge] The
chartshalf of the config change already landed, with a better mechanism → a contributor rebasing this branch hits a conflict invitest.config.tswhose correct resolution is "keep main's file, add two lines"; the PR's own bare-specifier approach (depend on thenodeproject's globalSetup buildingcore) is superseded by main's resolve alias ·vitest.config.ts:90-92at head -
[context, not a charge] Eight test files now exist on
main, written by five other people → whoever resolves this must decide per file which suite to keep; in four of the eightmainhas more tests, in four this PR has more ·packages/lab/src/ThreeD/ThreeDChart.test.tsx(main 15 tests, PR 3)
Thanks for this, and I'm sorry it sat so long — that delay is the whole problem with it now.
Your diagnosis was right: charts and vega really were falling through to the node project, where a component test can't work. The tests themselves are good, and the vega suites are the shape I want.
But main moved under you. #4382 routed charts into ui on 30 July and fixed the bare @astryxdesign/core import by aliasing it to source, so that half is already in. Eight of your files now collide with tests other people wrote for the same modules.
vega is still untested and still wanted. Could you rebase and narrow this to that? Two useTooltip and two TableHeaderCell assertions need updating for current behaviour.
If you'd rather talk it through, we're in Discord.
[Reviewed by Robohands]
-
vitest.config.ts:90-92— main already aliases the bare@astryxdesign/corespecifier to source, so nodistbuild is needed here. -
packages/core/src/Table/TableHeaderCell.test.tsx:199— main paints this edge withborderInlineEndWidth; this asserts the physical property. -
packages/core/src/Tooltip/useTooltip.test.tsx:144— main'stouchTriggerdefault opens on tap where there is no hover.
- The PR's 66 test files carry 465 tests and 1,117 assertions, dominated by
toBe(268),toHaveAttribute(237),toHaveLength(133) andtoHaveBeenCalled(106); only ~6% are weak shapes. This is not a tautological test dump — worth saying to the author if the rebase ask lands badly. -
packages/chartsshipped atestscript pointing at a runner that would have rejected its own component tests, which is a neat detail in the PR body and true. - The PR body is unusually well written and states its rationale explicitly, so R1j (unrecorded change) has nothing to catch here.
TIME total 25m setup 3m worktree cut at 36f240d + clone-seeded install; warm main NOT reused as a checkout — read via the bare repo instead, so no second main worktree was created reading 6m kit (brief, presentation, measurement harness, critic rule index), both vitest configs, PR body, 815-commit drift analysis, conflict enumeration measuring 7m 5 vitest runs + 1 local merge; 2 runs discarded (see waste) writing 4m round-1 presentation, gate 1, round-2 presentation, gate 2 publishing 3m fresh wiki clone, mirror-integrity check, record + index row, serialized push waste 2m two invalid measurements, both caught before they reached a finding: (1) a zsh word-splitting slip made an 8-file source overlay a silent no-op that looked like a clean pass; (2) the corrected partial overlay produced 22 failures that were artifacts of modules postdating the PR base (useLocale, useTouchTrigger, useLayerDismissal, getStandaloneShortWeekdayNames), not real incompatibilities. The valid answer came from a real local merge. Repeatable lesson: to ask "does this stale branch still work", merge — never overlay a subset of files.
| phase | wall |
|---|---|
| setup + rules | 3m |
| build / server | 0m — no build taken; the alias probe replaced a core build, and no Storybook or browser was started |
| browser / a11y | 0m — no visual claim in the diff |
| focused tests | 7m |
| code + history analysis | 6m |
| critique + wiki | 7m |
| CI wait | 0m |
- Whether CI passes on a rebased branch. I ran a focused sample (charts, vega, and the drifted core files), never the full suite, per the run's constraints.
- Whether the 4 stale assertions are cheap to update or reveal a behavior disagreement. I confirmed
main'stouchTriggersemantics are documented and intentional (Tooltip.doc.mjs:73), so I treated them as the PR's tests being stale rather than main having regressed — but I did not drive the tooltip in a browser.
Not posted — this review is a read-only draft. The run was dispatched read-only on the PR and the code branch, so nothing was posted to GitHub and no code was pushed.