docs(adr): record the visualization fold decision (DLS-1022) - #855
Open
gamegee wants to merge 34 commits into
Open
docs(adr): record the visualization fold decision (DLS-1022)#855gamegee wants to merge 34 commits into
gamegee wants to merge 34 commits into
Conversation
Adds ADR 0002 covering the decision to fold the standalone visualization libraries into ui-react / ui-rnative as a component category exposed via a subpath, following the Coinbase CDS model. The two viz packages were never independent: they import back into their parent (13 statements on web, 34 on RN), ui-rnative-visualization already declares an implicit dependency on ui-rnative, web chart stories are already served from the ui-react Storybook, and the two trees have begun to fork -- 6 of 9 files under utils/ now differ. The ADR records the decision, a verified inventory of 25 blockers, the PR sequence and the acceptance criteria. Its central finding is a build asymmetry that would let the new subpath typecheck, autocomplete, pass every CI job and publish cleanly while 404-ing at runtime with zero chart CSS: vite-plugin-dts emits declarations for every file in the TS program, while rollup (preserveModules + treeshake 'smallest') emits JS only from declared entries. The effect is already visible in the checked-in build, where dist/test-setup.d.ts exists with no matching .js. Docs only -- no change under libs/*/src, so no version plan is required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Three corrections to ADR 0002 after verifying against `npm pack --dry-run`: - The vendored `dist/node_modules/**` DOES ship. The ADR claimed npm never packs a node_modules directory and that the published package therefore carries dangling relative imports. That is wrong: npm's exclusion applies to the package root, not to a nested directory under an included path. `npm pack --dry-run` lists 66 such entries, 332 KB of a 1.2 MB unpacked package. The real defect is a double-ship, not a broken package -- consumers receive a vendored copy of eleven packages alongside the ones they install, three of which (cva, clsx, tailwind-merge) are declared as peerDependencies. - Reanimated range decided: `^4.1.0`, ui-rnative's narrower constraint. - Documents the inner `Components/` segment decision with measured counts. 158 imports cross that boundary (78 web, 80 RN), not the 36 first estimated, which counted only the three-level `config` specifiers. Records the three options and recommends dissolving the segment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The recorded plan cannot meet its CSS-isolation criterion and contains several inaccurate packaging instructions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Records the decision and migration plan for folding visualization packages into the primary UI libraries.
Changes:
- Defines the target subpath architecture and migration sequence.
- Documents packaging risks, blockers, and acceptance criteria.
File summaries
| File | Description |
|---|---|
docs/adr/0002-visualization-fold.md |
Adds the visualization-fold ADR. |
Review details
Suppressed comments (1)
docs/adr/0002-visualization-fold.md:282
- This acceptance step cannot install anything because
npm pack --dry-runnever writes the tarballs. Require a realnpm packinvocation so the consumer-install check can actually run.
paragraph whose code block was already removed, now reading as if it
describes the viz import) · `libs/ui-react/ai-rules/RULES.md:29` · **add**
- Files reviewed: 1/1 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+112
to
+114
| **The guard (non-negotiable):** run `npm pack --dry-run` on both packages and | ||
| install the tarballs into a scratch app that imports one chart. Ninety seconds, | ||
| and it surfaces the entire class. |
Comment on lines
+158
to
+161
| 7. **`libs/ui-react/tsconfig.lib.json`: exclude `**/__stories__/**`** (and the | ||
| RN twin), or story fixtures ship to consumers. There is **no `.npmignore` | ||
| anywhere in the repo** — packaging is governed solely by `files` arrays, and | ||
| `libs/ui-rnative/package.json` ships all of `src`. |
Comment on lines
+242
to
+246
| 25. **Consumer-facing docs — the only items that reach users.** | ||
| `SetupTailwind.mdx:77-81` (delete) and `:83-88` (an orphaned "legacy path" | ||
| paragraph whose code block was already removed, now reading as if it | ||
| describes the viz import) · `libs/ui-react/ai-rules/RULES.md:29` · **add** | ||
| the `/visualization` entry point to both `ai-rules/RULES.md` files. Both |
| `build.lib.entry` (`vite.config.ts:51-57`) declares exactly two: `index` and | ||
| `lib/Components/symbols/index`. | ||
|
|
||
| This asymmetry is already observable in the checked-in build: |
Comment on lines
+103
to
+104
| **No job in this repo imports a Lumen subpath other than `/symbols`.** There is | ||
| no `npm pack` check, no publint, no consumer-install smoke test. |
|
|
||
| ### Docs, tooling, agent guidance | ||
|
|
||
| 22. `sonar-project.properties:6` (blocker); `pr.yml:123,127,139,140` (no-op cleanup). |
- Landing path: fold onto the current layout. The `src/lib` flatten is not a prerequisite, which removes three blockers -- `@source "./dist/lib"` needs no change and the ui-react mdx glob already covers the new tree. - Inner `Components/` segment: dissolved. Corrected the count from 158 to 155 crossing imports (78 web, 77 RN); the 3 RN `'../utils'` specifiers resolve to the component-local `CartesianChart/utils.ts`, so a textual sed would silently repoint them. The rewrite must be resolution-aware. - The `"./*"` wildcard is deleted rather than out-ranked. It resolves to `dist/lib/Components/core/*/index.js` and all 52 core components ship `index.d.ts` with no `index.js`, so every per-component subpath already throws ERR_MODULE_NOT_FOUND in the published package. It is undocumented, so removing it breaks nothing that works. - RN chart stories are enabled with explicit `rnative-*` ids, and the RN StoryDecorator is deleted rather than relocated: `preview.tsx` already applies a brand-aware provider, while the viz decorator hardcoded ledgerLiveThemes. The web decorator is still relocated -- it adds padding the global one does not, and dropping it would cause 8 gratuitous Chromatic diffs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ponents/ Step 1 of the DLS-1022 fold. Pure `git mv`: libs/ui-react-visualization/src/lib becomes libs/ui-react/src/lib/Components/visualization, and the nine chart folders are lifted out of the inner Components/ directory so charts sit at Components/visualization/LineChart/, consistent with Components/core/Button/. The inner Components/index.ts is removed here; its six exports are folded into the new visualization barrel in the next commit. Imports are left broken on purpose -- they are re-depthed in step 3 so that the move stays reviewable as a rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 2 of the DLS-1022 fold. The viz package's public API was src/index.ts, which re-exported ./lib/Components AND ./lib/utils -- and src/index.ts sits outside the src/lib/** tree that step 1 moved, so the relocated folder had no barrel at all. The new Components/visualization/index.ts is the file the ./visualization export key will point at. It reproduces the previous surface exactly: the six public chart components plus ./utils, so the chart-math types (CategoricalScale, ChartScaleFunction, DrawingArea, Series, ...) stay public. Dropping ./utils here would have been a second breaking change hidden inside the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…omponents/ Step 3 of the DLS-1022 fold. Dissolving the inner Components/ directory moved every chart folder up one level, so the 78 specifiers reaching the shared config.ts and utils/ each lose one `../`: '../../config' -> '../config' (24) '../../../config' -> '../../config' (13) '../../utils/...' -> '../utils/...' (28) '../../../utils/...'-> '../../utils/...' (13) The rewrite is resolution-aware rather than textual, which matters: every chart folder has its own component-local utils.ts, so a blind sed on `../utils` would silently repoint those at the shared module instead of leaving them alone. Each candidate is only rewritten when the stripped form resolves to a real file inside visualization/; anything that already resolves is left untouched. 78 specifiers across 51 files, zero unresolved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 4 of the DLS-1022 fold. The 13 statements importing `@ledgerhq/lumen-ui-react` and `@ledgerhq/lumen-ui-react/symbols` from inside the moved tree are now relative. Two of them named several symbols at once, so they expand to 17 import statements in total. Left as bare specifiers these would be self-referencing package imports: rollup resolves them through the workspace symlink into libs/ui-react/dist and inlines a SECOND copy of the library into the chart chunks, giving duplicate ThemeProvider and disabled-context instances -- charts would silently ignore the theme and disabled state the host provides. It would also make the build depend on a possibly stale dist. Targets are the component barrels rather than the package root, so the module graph stays tight: core/ThemeProvider, core/Button, core/AmountDisplay, core/SegmentedControl, core/Trend, symbols, and src/utils/useControllableState (which lives outside Components/, hence the deeper prefix). Import order restored with eslint --fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cifiers Step 5 of the DLS-1022 fold. StoryDecorator.tsx moves from the doomed viz lib into libs/ui-react/.storybook/. It stays rather than being dropped because it adds `p-32` padding that the global withStorybookProviders decorator does not -- removing it would produce eight gratuitous Chromatic diffs on stories that are otherwise unchanged. Its own ThemeProvider import now uses a relative path, matching the sibling Decorator.tsx convention rather than the package specifier. All 11 .storybook/* specifiers in the moved tree are recomputed from each file's real location rather than nudged by a fixed number of levels: the 8 StoryDecorator imports sat at two different depths, and the 3 DocTable imports in MDX previously crossed the lib boundary via '../../ui-react/.storybook/...', which no longer exists. main.ts drops the two cross-lib globs. No widening is needed -- the existing '../src/lib/**/*.mdx' and '../src/**/*.stories' globs already reach the relocated tree, and .storybook/** is already a Tailwind content root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 6 of the DLS-1022 fold. Adds the "./visualization" export key -- an explicit conditions object with `types` first -- so charts are reachable at @ledgerhq/lumen-ui-react/visualization without ever entering the main barrel. Removes "./*": "./dist/lib/Components/core/*/index.js". That pattern was already non-functional: rollup elides pure re-export barrels under treeshake:'smallest', so every core component ships index.d.ts with no index.js and @ledgerhq/lumen-ui-react/Button has been throwing ERR_MODULE_NOT_FOUND in the published package. All 52 were affected. It appears in no README, no ai-rules, nothing consumer-facing, so removing it breaks nothing that works -- and leaving it would have silently captured /visualization ahead of the explicit key. d3-scale and d3-shape become real dependencies, and @types/d3-scale and @types/d3-shape are dependencies rather than dev, because utils/types.ts re-exports d3-scale types through the public barrel. d3-array is dropped entirely -- verified unused in the moved tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 7 of the DLS-1022 fold -- the step that decides whether the subpath is real. vite-plugin-dts emits a .d.ts for every file in the TypeScript program, while rollup with preserveModules + treeshake:'smallest' emits JS only for modules reachable from a declared entry. Since charts deliberately stay out of src/index.ts, the ./visualization key added in step 6 would otherwise point at a declaration file with no runtime: tsc green, IDE autocomplete working, `nx build` exit 0, and ERR_MODULE_NOT_FOUND for the consumer. Tailwind would fail in the same stroke, because class names live only in emitted JS. Declaring 'lib/Components/visualization/index' is what closes that gap. externalize /^d3-/ and internmap so d3 is required at runtime rather than vendored into dist/node_modules/. The viz package today does the opposite and ships 66 vendored files, 332 KB of a 1.2 MB unpacked package, including three packages it declares as peerDependencies. @ledgerhq/lumen-ui-react and its /symbols subpath are externalized as tripwires: after step 4 nothing inside the package should import it by name, and anything that slips back in now fails loudly at build instead of silently inlining a second copy of the library. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 8 of the DLS-1022 fold. The exclude list already covers *.stories.tsx by pattern, but the __stories__ folders also hold plain modules -- cryptoChartData.ts and chartStoryFixtures.ts -- which no pattern matched. They therefore entered the TypeScript program and vite-plugin-dts emitted declarations for them; the viz package's checked-in dist confirms it, carrying cryptoChartData.d.ts and chartStoryFixtures.d.ts. Nothing else stops them: there is no .npmignore anywhere in this repo, so the `files` array is the only packaging gate. tsconfig.lib.json drives the library build only; Storybook resolves through its own config, so the stories themselves are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 9 of the DLS-1022 fold. The two '../ui-react-visualization/**' content globs existed so the Storybook build could scan chart classes across the lib boundary. The charts now live under ./src, which the first glob already covers. tailwind.css is deliberately untouched: charts landed at dist/lib/Components/visualization/, so the shipped `@source "./dist/lib"` still reaches them. That is one of the three blockers the current-layout baseline removed. Left in place: the './lib/**/*.stories' glob, which is already dead (libs/ui-react/lib does not exist). Unrelated to this fold; noted for a separate cleanup rather than widened here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 10 of the DLS-1022 fold. @ledgerhq/lumen-ui-react-visualization is gone -- hard cut, no shim. Its only remaining source files were src/index.ts (replaced by the barrel authored in step 2) plus styles.css and test-setup.ts, both byte-identical to ui-react's own, so nothing was salvaged. Also removed: the root tsconfig.json project reference, and the AGENTS.md Libraries row and prose entry. The AGENTS.md edit is not optional -- scripts/check-agent-docs-drift.mjs enforces a bijection between that table and the libs/* folders on disk, so the agent-drift job fails without it. Note the drift check is only partial: several skills still name the package and pass silently. They are handled in the docs PR. npm install left an `"extraneous": true` stub for the deleted workspace, which is removed by hand along with its node_modules link entry. The lockfile already carried the same phantom for a `libs/ui-core` deleted long ago; that one is pre-existing and left alone rather than widened into this commit. The published package is retired with `npm deprecate` after merge -- that operates on already-published versions and needs no source, so deleting first is correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes step 3 of the DLS-1022 fold. The step 3 rewrite only handled specifiers pointing OUT of the dissolved directory -- component files reaching the shared config.ts and utils/. It missed the reverse direction: three modules under utils/ import types back into the chart components via '../../Components/Axis'. Those keep their depth (utils/domain/ is still two levels below the visualization root) but lose the now-nonexistent Components/ path segment. Caught by tsc, which also reported the knock-on implicit-any errors in ticks.ts and domain.test.ts where the broken import degraded BaseAxisProps to any. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes step 8 of the DLS-1022 fold. Excluding **/__stories__/** from tsconfig.lib.json stopped the fixtures shipping, but it also removed them from the only project that contained them: tsconfig.tools.json includes just *.stories.*, *.mdx, *.figma.* and .storybook/**, and previously resolved cryptoChartData.ts and chartStoryFixtures.ts through its project reference to tsconfig.lib.json. With that reference no longer carrying them they belonged to no project at all, and tsc raised TS6307. Adding them to the tools include gives the clean split the exclusion was after: fixtures typecheck with the stories, and stay out of the published build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes step 5 of the DLS-1022 fold. Swapping the package specifier for a relative path moved the ThemeProvider import into a different import/order group, which eslint --fix had not been run over because the file was moved rather than edited in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes step 10 of the DLS-1022 fold. scripts/check-agent-docs-drift.mjs fails the agent-drift job on four counts beyond the AGENTS.md table: component-styling and component-testing each cite libs/ui-react-visualization as a repo path and carry a `paths:` auto-attach glob that now matches no file. These were scheduled for the docs PR, but CI enforces them here, so they land with the deletion. The remaining stale references -- release-plan, component-anatomy, code-connect, component-mdx, pr-review and the rnative styling reference -- are NOT caught by the drift check (it only validates backtick-quoted paths that start with a known prefix, and those name the package rather than the path). They stay in the docs PR. react.md's visualization note is rewritten rather than deleted: the exception it describes still exists, it just now lives at src/lib/Components/visualization/ behind the ./visualization subpath. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…omponents/ Step 1 of the DLS-1022 fold, React Native side -- mirrors the ui-react move. libs/ui-rnative-visualization/src/lib becomes libs/ui-rnative/src/lib/Components/visualization, and the nine chart folders are lifted out of the inner Components/ directory so charts sit beside core/, internal/, primitives/, animations/ and symbols/. The inner Components/index.ts is removed here; its six exports move into the new visualization barrel in the next commit. Imports are left broken on purpose so this commit stays reviewable as a pure rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 2 of the DLS-1022 fold, React Native side. The viz package's public API was src/index.ts, which sits outside the src/lib/** tree moved in step 1, so the relocated folder had no barrel. Components/visualization/index.ts is what the ./visualization export key points at. It reproduces the previous surface exactly -- the six public chart components plus ./utils -- so the chart-math types stay public. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… Components/ Step 3 of the DLS-1022 fold, React Native side. 80 specifiers rewritten: the 77 that reach the shared config.ts and utils/ each lose one `../`, and the 3 under utils/ that point back into the chart components lose the now-nonexistent Components/ path segment. Both directions are handled here, unlike the ui-react pass where the inward ones needed a follow-up. The rewrite is resolution-aware rather than textual. That matters most on this side: the three `'../utils'` specifiers under CartesianChart/RevealAnimation/ resolve to the component-local CartesianChart/utils.ts, not the shared module, and a blind sed would have silently repointed them at the wrong file -- one of the few edits in this migration that tsc would NOT catch. They are left untouched, along with 252 other already-correct specifiers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…paths Step 4 of the DLS-1022 fold, React Native side. The 34 statements importing `@ledgerhq/lumen-ui-rnative` from inside the moved tree are now relative; several named multiple symbols, so they expand to 42 import statements. Targets follow where each symbol actually lives rather than the package root: core/ThemeProvider, primitives/Box, primitives/Text, src/styles (useTheme, useStyleSheet), src/lib/utils (RuntimeConstants, useControllableState) and src/lib/Haptics (triggerHapticFeedback). The 35th occurrence is deliberately left alone: Point/types.ts:63 sits inside a JSDoc @example, where `@ledgerhq/lumen-ui-rnative` is still the correct specifier for a consumer reading the docs. This matters more on React Native than on web. bob transpiles per-file and would happily publish a package that imports itself, so unlike the web build there is no bundler step to catch a leftover self-reference. Import order restored with eslint --fix. The remaining lint errors are the missing d3 / gesture-handler / worklets dependencies, added in step 6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rator Step 5 of the DLS-1022 fold, React Native side. Unlike ui-react, the decorator is deleted rather than relocated: .storybook/preview.tsx already applies withProvidersDecorator with a `brand` global, while the viz decorator hardcoded ledgerLiveThemes. Dropping it un-pins the charts from ledger-live and puts them on the brand switcher. The four stories have never been snapshotted, so there is no Chromatic baseline to disturb -- but they do warrant a visual check. Three stories keep their sizing <View>; Scrubber's decorator wrapped nothing else, so its decorators block is removed entirely. The four metas gain explicit ids -- rnative-point, rnative-linechart, rnative-referenceline, rnative-scrubber. Without them these stories would enter Storybook with title-derived `visualization-*` ids, breaking the platform-prefix contract that .claude/agents/sync-doc-links.md depends on. The eight web chart stories already carry `react-*` ids, which is why they needed no equivalent change. main.ts drops the commented-out cross-lib glob: the existing '../src/lib/**' pattern already matches the relocated stories, so these four become visible for the first time -- excluding them would have required writing a negated glob on purpose. The '@ledgerhq/lumen-ui-rnative' self-alias and its optimizeDeps exclusion go too; with charts now inside the package they would resolve from libs/ui-rnative/src back into libs/ui-rnative/src, a genuine cycle in the Storybook Vite graph. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 6 of the DLS-1022 fold, React Native side. The ./visualization key mirrors ./symbols with all four conditions, and its `react-native` condition points at TypeScript SOURCE rather than dist. That is load-bearing: apps/app-sandbox-rnative/metro.config.js enables package exports, so a types/import-only key would compile in CI and die on device. There is no wildcard export on this package to remove. d3-scale and d3-shape become dependencies, with @types/d3-scale and @types/d3-shape as dependencies rather than dev because utils/types.ts re-exports d3-scale types through the public barrel. d3-array is dropped -- verified unused. react-native-gesture-handler and react-native-worklets move up from the viz package's peers, marked optional via peerDependenciesMeta since only chart consumers need them. Note .npmrc sets legacy-peer-deps=true, so npm gives no local signal if these are wrong -- they were verified by grepping the moved tree. Reanimated stays at ui-rnative's ^4.1.0; the viz package's looser >=4.0.0 is discarded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…path Step 8 of the DLS-1022 fold. The RN sandbox is the only app that consumed the visualization package -- app-sandbox-react never did. Three block components (LineCharts, DonutCharts, Legends) now import from @ledgerhq/lumen-ui-rnative/visualization, and the workspace dependency plus both project references are dropped. tsconfig.app.json matters beyond tsc here: the @nx/js/typescript plugin reads those references to build project-graph edges, so a stale one is not merely a compile error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 7 of the DLS-1022 fold -- the React Native counterpart to ui-react's Vite
entry. bob compiles the whole src tree, so no build entry is needed here; what
the charts do need is the viz lib's test setup.
jest.config.ts: transformIncludePatterns is the union of both lists, gaining
@react-native/js-polyfills, react-native-gesture-handler, react-native-worklets,
d3-.* and internmap. d3-scale and d3-shape are pure ESM, so without the
transform the folded chart suites fail outright. Note the existing
'(jest-)?react-native' entry does NOT cover react-native-worklets: the negative
lookahead requires a '/' after the alternative.
jest.setup.ts: adds the react-native-svg, react-native-gesture-handler and
react-native-worklets mocks plus useReducedMotion. Two things had to be
reconciled rather than copied:
- The svg mock was incomplete. It omitted Mask and RadialGradient, which the
pre-existing Icon and RadialGradient suites use, so merging it verbatim broke
five tests in components unrelated to charts.
- Its components were plain functions rendering bare host elements, and
react-test-renderer leaves refs to those null -- real react-native-svg exports
resolve to a component instance. Icon asserts ref.current is non-null, so the
mock components are now classes. The rendered tree still carries the svg
element names the chart tests query by.
ui-rnative's evaluating `useAnimatedStyle: (cb) => cb()` is kept over the viz
lib's `() => ({})`. The chart suites were written against the non-evaluating
variant, so this was the one merge that could have changed assertions silently;
all 94 suites and 1278 tests pass against the evaluating one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 9 of the DLS-1022 fold. @ledgerhq/lumen-ui-rnative-visualization is gone -- hard cut, no shim. Its only remaining source file was src/index.ts, replaced by the barrel authored in step 2; the rest was build and test configuration, whose useful parts were merged in step 7. Also removed: the root tsconfig.json project reference, the AGENTS.md Libraries row and prose entry, and the `extraneous: true` lockfile stub npm leaves behind along with its node_modules link. The component-styling and component-testing skills are updated here rather than in the docs PR because scripts/check-agent-docs-drift.mjs fails the agent-drift job on them -- each cited the lib path and carried a now-dead `paths:` auto-attach glob. Both visualization exceptions they document still hold; they just moved to Components/visualization/ behind the subpath, so the notes are rewritten rather than dropped. The stale references the drift check does NOT catch (release-plan, component-anatomy, code-connect, component-mdx, pr-review) remain for the docs PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lves Step 1 of PR 3 for DLS-1022. Adds three exclusions to the Button-only bundle check: Components/visualization/, node_modules/d3-, and internmap. Charts ship at the ./visualization subpath, so none of them may reach a bundle that imports only the main barrel. Verified against the current build: 0 of each across 51 modules. Adds a CSS section, which this harness never had. It read only index-*.js.map and ignored the 162 KB stylesheet built alongside it. That matters because Lumen ships `@source "./dist/lib"` inside its own tailwind.css, and Tailwind emits a utility only for classes it has actually seen -- so if that glob ever stops matching, every Lumen utility silently disappears from the consumer's CSS with no build error. The stylesheet's own comment documents this failure mode; nothing in CI covered it. The assertion requires bg-accent, text-on-accent and body-2-semi-bold, none of which the fixture's own markup writes, so they can only come from scanning Lumen's compiled dist. A minimum size guards the same thing coarsely. Note this deliberately does NOT assert that chart utilities are absent from the CSS. Tailwind scans the whole dist, so the stylesheet is the full design-system sheet regardless of what a consumer imports -- charts add a handful of utilities to 162 KB. Isolation is a JS-bundle property here, not a CSS one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 2 of PR 3 for DLS-1022. A second Vite build imports LineChart from @ledgerhq/lumen-ui-react/visualization and asserts the opposite of the first one: the chart modules and both d3 packages must be PRESENT. The two fixtures are only meaningful together. `main` alone would pass happily if ./visualization resolved to nothing at all -- an absent module satisfies an exclusion check by accident -- and `visualization` alone would pass even if charts leaked into every other bundle. Together they also make the d3 path fragments self-verifying: the exclusion in `main` cannot be a typo, because the identical fragment must match in `visualization`. verify-bundle.mjs is parameterised with --profile and --dist, and now finds the JS map and CSS by extension rather than an `index-` prefix, since Vite names chunks after the entry html. Verified by simulating the failure the ADR was written to prevent: removing the visualization entry from libs/ui-react/vite.config.ts leaves `nx build` green and still emits index.d.ts, while index.js is absent -- and the `main` profile happily reports SUCCESS. The new fixture is the only thing that fails, with "Rollup failed to resolve import @ledgerhq/lumen-ui-react/visualization". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 3 of PR 3 for DLS-1022. Adds scripts/check-package-exports.mjs, wired into the bundle-checks CI job. For each publishable lib it collects every relative path advertised by the package -- across all export conditions, plus main/module/types/react-native -- and asserts each one is present in `npm pack --dry-run`. Pattern targets like design-core's "./symbols/*" are satisfied by at least one match, so a wildcard whose whole target tree was never emitted fails instead of passing because it names no concrete file. That is what the removed ./* wildcard on ui-react would have tripped. npm pack is the source of truth rather than the filesystem, because it also applies the `files` allowlist: a target that exists on disk but is filtered out of the tarball fails just as hard for a consumer. This generalises the guard beyond charts -- it covers every subpath of every package, forever. It immediately found a real bug, which is fixed here: utils-shared declared no `files` field, so npm fell back to .gitignore, which ignores dist/. The package has been publishing only src/index.ts while its exports pointed at ./dist/index.js. Every other lib declares `files`; this one is now consistent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The visualization fixture wrote to tests/treeShaking/dist-visualization, a name matched by neither .gitignore's `dist` entry nor eslint.shared.mjs's globalIgnorePatterns. ESLint therefore walked the minified bundle and its source map on every run, which hung app-sandbox-react:lint rather than failing it. Writing to dist/visualization instead puts the output under an already-ignored path, so the .gitignore addition is reverted and the shared eslint config needs no new entry. Build order is unaffected: the main build empties dist/ before the subpath build creates the subdirectory. Also fixes three real lint errors in verify-bundle.mjs that the hang was masking, including inverting a negated condition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gamegee
force-pushed
the
DLS-1022/move-visualization-into-main-package
branch
from
September 2, 2026 18:51
9e5045b to
3e426cb
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
Step 1 of PR 5 for DLS-1022 — the surfaces users actually see. Both ai-rules directories are listed in their package's `files` array, so RULES.md ships inside the npm tarball and is what a consumer's agent reads. - Both RULES.md files gain the `/visualization` entry point alongside the existing `/symbols` line, so agents stop reaching for the deleted package. - The web RULES.md no longer tells consumers to add a second stylesheet import. That instruction now breaks their CSS build outright: the package it names is gone, and `@ledgerhq/lumen-ui-react/tailwind.css` already covers charts because @source scans the whole dist. - SetupTailwind.mdx drops the same instruction, and with it an orphaned paragraph left behind by #819: "This is the legacy path…" described a Tailwind v3 `content` glob whose code block had already been removed, so it read as if it were describing the charts import directly above it. The doc states at the top that it assumes v4, so there is nothing for it to refer to. - The bug report template drops the two deleted packages; charts are now reported against the UI kit that contains them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 2 of PR 5 for DLS-1022. - sonar-project.properties: removes both libs from sonar.sources, the RN one from cpd.exclusions, and both lcov report paths. The charts are still analysed — they now sit under libs/ui-react/src and libs/ui-rnative/src, which are already listed. - pr.yml: the lcov path-rewriting loops and the coverage artifact list no longer name the deleted libs. These were no-ops rather than failures (each is guarded by a file-existence test), but they hid the fact that two of the six coverage reports would never appear again. - .vscode/settings.json: eslint.workingDirectories pointed at two directories that no longer exist. This one is silent rather than loud — ESLint's flat config falls back, so contributors get subtly different in-editor results from CI. Observed but deliberately not changed: sonar.exclusions and sonar.coverage.exclusions still say `**/Symbols/**`, while the folder has been lowercase `symbols/` since the tier restructure in #854. That predates this fold, and altering Sonar exclusions shifts quality-gate metrics, so it wants its own change rather than riding along here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step 3 of PR 5 for DLS-1022 — the stale references the drift check cannot see. scripts/check-agent-docs-drift.mjs only validates backtick-quoted paths starting with a known prefix, so everything here passed CI while documenting two packages that no longer exist. - sync-doc-links.md: three hardcoded `nx run-many -p …` invocations named the deleted project. Nx errors on an unknown project, so these would have failed outright the next time the agent ran. - release-plan/SKILL.md: drops both rows from the path->package table this skill exists to be the single source of, and replaces a filename-convention example that used a slug no longer valid. - component-anatomy/SKILL.md: the two viz rows are replaced by a note under the table, since charts now follow their host lib's row with two exceptions -- some web stories in `__stories__/`, and no figma or RN `.mdx` coverage. Also updates the review-check example that cited the deleted lib. - pr-review/SKILL.md: the visualization exception is now keyed on the folder rather than the package. - CONTRIBUTING.md: the repo tree no longer shows two libs that are gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ADR said `npm deprecate` needs no source, so deleting first is correct. True about repo state, but silent about release timing -- and that gap matters. Verified against the registry: the published @ledgerhq/lumen-ui-react@0.1.56 declares only ".", "./*", "./package.json", "./symbols" and "./tailwind.css". The /visualization subpath does not exist in any published version yet. A deprecation notice is advice consumers act on immediately, so running it before the release would point every one of them at a specifier that resolves to nothing -- the exact ERR_MODULE_NOT_FOUND this ADR was written to prevent, delivered on purpose. Records the ordering (merge -> nx release -> confirm the subpath resolves in the published tarball -> deprecate) and the two exact commands, with the shipped version left as a placeholder to fill from the actual release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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
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.
What
Adds
docs/adr/0002-visualization-fold.md— the architecture decision record for folding the standalone visualization libraries intoui-react/ui-rnativeas a component category exposed via a subpath, following the Coinbase CDS model.Docs only. No change under
libs/*/src, so no version plan is required.Why
The two viz packages were never independent:
"@ledgerhq/lumen-ui-react": "0.1.56"exactly.ui-rnative-visualizationalready declaresimplicitDependencies: ["@ledgerhq/lumen-ui-rnative"].libs/ui-react/.storybook/main.ts:11-12globs across the lib boundary); the RN equivalent is commented out, so RN chart stories ship nowhere.utils/differ, and the test files differ only byvitestvs@jest/globals.Charts are a category of the design system, not a product of their own.
What the ADR records
The decision, a verified inventory of 25 blockers, the PR sequence, and acceptance criteria.
The finding that motivated writing it down: the new subpath would typecheck, autocomplete, pass every CI job and publish cleanly — and 404 at runtime with zero chart CSS.
vite-plugin-dtsemits a.d.tsfor every file in the TypeScript program, while rollup (preserveModules+treeshake: 'smallest') emits.jsonly for modules reachable from a declared entry — andvite.config.ts:51-57declares exactly two. Adding the"./visualization"export key without a matching Vite entry meanstypesresolves andimportdoes not.This is already observable in the checked-in build:
Nothing in CI catches it: no job imports any Lumen subpath except
/symbols, and there is nonpm packor install smoke test. Hence acceptance criterion #1 — pack the tarballs and install them into a scratch app that imports one chart.Other verified blockers include the viz
dist/node_modules/vendoring 11 packages (three of them declared as peerDependencies and bundled anyway), leftover self-imports inlining a second copy of the library (duplicateThemeProvider/ disabled-context instances → charts silently ignore host theme), the moved tree having no barrel (src/index.tssits outside the movedsrc/lib/**), andcheck:agent-docsgoing green whilerelease-plan/SKILL.mdstill documents the deleted packages.Notes for reviewers
src/lib/Components/{core,internal,symbols}). The ADR states the delta to apply if thesrc/libflatten lands first.^4.1.0vs>=4.0.0), and whether theComponents/segment survives insidevisualization/(it must, or 36 three-level relative imports re-depth).__stories__/, re-enabling RN chart stories.vercel.json.🤖 Generated with Claude Code