Skip to content

Commit 6db325c

Browse files
fix(REV-008): delete the dead GroupConfig; spec the token finding as REV-009
Stream D2/D3. GroupConfig keyed on 'net.noodl.visual.group', which is not a node type — the real one is 'Group' — so it had never applied to anything. Deleted rather than repointed, and tara's ImageConfig not ported, because activating either would stamp var(--surface), var(--space-4) and friends onto new nodes and nothing defines those. That is the wider finding, written up as REV-009: the style-token system was never connected. StyleTokensInjector is not constructed anywhere, so no tokens reach a preview; the ten tokens that did ship use a different vocabulary from the one ElementConfigs references, which exists only in the phase-9 design docs. And ElementConfigs is live — every new Text node is persisted with fontFamily: var(--font-sans) and four more values that resolve to nothing. ElementConfigRegistry.test had the same identifier confusion: its TEXT_TYPE was 'net.noodl.visual.text', matching no config, so its 'no sizes' assertion passed for the wrong reason. Now split into a registered-type case ('Text') and an unregistered-type case. 705 specs, 0 failures. typecheck:editor clean.
1 parent 6ee68cf commit 6db325c

7 files changed

Lines changed: 189 additions & 82 deletions

File tree

dev-docs/reviews/MERGE-NOTES-cline-dev-tara.md

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,15 @@ and the three `net.noodl.controls.*` configs are correct.
5555
something to smuggle into a merge commit. Porting it and fixing `GroupConfig`
5656
should be a small deliberate task.
5757

58+
**Resolved in REV-008 (2026-07-22): both deleted, neither ported.** Repointing the
59+
identifiers would have activated configs whose variants reference `var(--surface)`,
60+
`var(--space-4)` and similar — and nothing defines those. The token system they
61+
were written against was never wired up: `StyleTokensInjector` is not constructed
62+
anywhere, and the tokens that *did* ship use a different vocabulary. `GroupConfig`
63+
is gone from the tree; `ImageConfig` stays recoverable from `d67ee72` if the wider
64+
question is ever settled. See
65+
[REV-009](../tasks/phase-12-reanimation/REV-009-STYLE-TOKENS-NEVER-WIRED.md).
66+
5867
## Other resolutions
5968

6069
**`LocalProjectsModel.ts`** (the only genuine code conflict). The "create project
@@ -100,10 +109,19 @@ Merged cleanly, no conflicts, independent of the ElementConfigs fight:
100109
`packages/noodl-editor/tsconfig.json` but not in the root `tsconfig.json`, plus a
101110
`LauncherPageMetaData` export missing from noodl-core-ui. Worth a follow-up.
102111

103-
## Not verified
104-
105-
The merge is structurally sound and the suite is green, but the editor was not
106-
launched against it. StyleTokens and the embedded template system have no
107-
automated coverage — the suite exercises neither. Someone should create a project
108-
from the launcher and confirm the hello-world template and token injection behave
109-
before this is relied on.
112+
## Not verified — resolved by REV-008
113+
114+
The merge was structurally sound and the suite green, but the editor had not been
115+
launched against it, and StyleTokens and the embedded template system had no
116+
automated coverage. REV-008 (2026-07-22) closed that:
117+
118+
- **Embedded template system — works.** `tests/models/EmbeddedTemplate.test.ts`
119+
covers it: the provider writes a `project.json` with components, a root
120+
component, and a `graph` object on each component (the missing-`graph` crash in
121+
LEARNINGS.md). `runtimeVersion` round-trips as `react19`, so the hand-resolved
122+
combination in `newProject` — tara's template flow plus cline-dev's react19 —
123+
is now guarded.
124+
- **StyleTokens — does not work.** `StyleTokensInjector` is never constructed, so
125+
no tokens are ever injected, and the tokens that shipped use a different
126+
vocabulary from the one ElementConfigs references. Written up as
127+
[REV-009](../tasks/phase-12-reanimation/REV-009-STYLE-TOKENS-NEVER-WIRED.md).
Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
# REV-009: The style-token system is not connected to anything
2+
3+
## Metadata
4+
5+
| Field | Value |
6+
|-------|-------|
7+
| **ID** | REV-009 |
8+
| **Phase** | Phase 12 — Reanimation (Revival Horizon 0) |
9+
| **Priority** | 🟡 Medium — nothing is visibly broken, but a shipped subsystem does nothing and a live one writes invalid CSS |
10+
| **Difficulty** | 🟡 Medium — the code is small; the decision is a product one |
11+
| **Estimated Time** | 1–2 days |
12+
| **Prerequisites** | None. Found during REV-008 Stream D1. |
13+
| **Branch** | `task/rev-009-style-tokens` |
14+
| **Recommended executor** | 🟠 **Opus 4.8** — the work is small but the call on what the design system *is* is not mechanical |
15+
16+
## Objective
17+
18+
Decide whether the STYLE-001 / STYLE-002 design-token system is finished or
19+
retired, and make the code say so. Today it is neither: half of it is dead code,
20+
the other half is live and writing CSS values that resolve to nothing.
21+
22+
## What was found
23+
24+
REV-008 set out to verify the StyleTokens work that arrived with the
25+
`cline-dev-tara` merge — flagged in
26+
[MERGE-NOTES-cline-dev-tara.md](../../reviews/MERGE-NOTES-cline-dev-tara.md) as
27+
having zero automated coverage. It does not work, for three independent reasons.
28+
29+
### 1. The injector is never constructed
30+
31+
`packages/noodl-viewer-react/src/style-tokens-injector.ts` exports
32+
`StyleTokensInjector`, whose constructor injects a `<style>` element of CSS
33+
custom properties into the viewer document. Nothing anywhere calls `new
34+
StyleTokensInjector(...)`:
35+
36+
```bash
37+
grep -rn "new StyleTokensInjector" packages/ # no matches
38+
```
39+
40+
Its only importer is itself. **No tokens are ever injected into a preview.**
41+
42+
### 2. The two halves use different token vocabularies
43+
44+
Even if it were wired up, the names would not line up.
45+
46+
| Source | Tokens |
47+
|---|---|
48+
| `StyleTokensInjector.getDefaultTokens()` and `models/StyleTokens/DefaultTokens.ts` | `--primary --background --foreground --border --space-sm --space-md --space-lg --radius-md --shadow-sm --shadow-md` (10) |
49+
| `models/ElementConfigs/configs/*.ts` | `--font-sans --text-base --text-xs --font-normal --leading-normal --foreground --primary --primary-foreground --secondary --radius-md --space-3 --surface --space-4 --border-1 --border-subtle --radius-lg --shadow-md --muted …` |
50+
51+
The overlap is `--foreground`, `--primary`, `--radius-md`, `--shadow-md`. The
52+
vocabulary the configs actually use — the Tailwind-style scale with `--space-4`,
53+
`--font-sans`, `--surface` — appears **only in the phase-9 task documents**
54+
(`dev-docs/tasks/phase-9-styles-overhaul/STYLE-001-token-system-enhancement/README.md`),
55+
never in shipped code. STYLE-002 was written against the token set STYLE-001's
56+
design document promised; STYLE-001 shipped a different, smaller one.
57+
58+
### 3. ElementConfigs is live and stamping unresolvable values
59+
60+
This is the part that is not merely dead. `NodePicker.utils.ts` calls
61+
`ElementConfigRegistry.applyDefaults(node, type.name)` on every node creation, and
62+
`TextConfig` is keyed on `'Text'`, which **is** the real node type. So every new
63+
Text node gets written into `project.json` with:
64+
65+
```js
66+
fontFamily: 'var(--font-sans)',
67+
fontSize: 'var(--text-base)',
68+
fontWeight: 'var(--font-normal)',
69+
lineHeight: 'var(--leading-normal)',
70+
color: 'var(--foreground)',
71+
```
72+
73+
None of which are defined at runtime. The three `net.noodl.controls.*` configs
74+
are equally live and equally affected.
75+
76+
Note this is *not* purely cosmetic: the values are persisted into the user's
77+
project file, so retiring the tokens later means those projects still carry them.
78+
79+
### Already actioned in REV-008
80+
81+
`GroupConfig` keyed on `net.noodl.visual.group`, which is not a node type — the
82+
real one is `Group`. It had therefore never applied to anything, and was deleted
83+
rather than repointed, precisely because activating it would have stamped the
84+
undefined `var(--surface)` / `var(--space-4)` vocabulary onto the most-used node
85+
in the product. Tara's `ImageConfig` had the same defect and was not ported.
86+
`TextConfig`'s own identifier is correct, which is why it is live.
87+
88+
## The decision to make
89+
90+
Three coherent end states. Pick one deliberately; the current state is none of
91+
them.
92+
93+
**A. Finish it.** Instantiate `StyleTokensInjector` in the viewer, and reconcile
94+
the vocabularies — most likely by expanding `DefaultTokens.ts` to the full scale
95+
the phase-9 design document specifies, since that is what the configs already
96+
expect. Largest option, and the one that makes the property panel's variant and
97+
size dropdowns actually mean something.
98+
99+
**B. Retire STYLE-002's tokenisation, keep the layout fixes.** Strip the `var(…)`
100+
defaults from `TextConfig` and the control configs, leaving the parts that are
101+
real bug fixes — `TextConfig` documents `width: auto` + flex participation as
102+
fixing text pushing siblings off-screen in row layouts. Delete
103+
`style-tokens-injector.ts` and the StyleTokens model, or leave them clearly
104+
marked unused.
105+
106+
**C. Retire the whole thing.** Remove ElementConfigs, its registry, and the
107+
property-panel variant/size UI that consumes it, along with the StyleTokens
108+
model. Smallest surface left behind.
109+
110+
## Steps
111+
112+
1. Confirm the finding in a running preview: create a project, add a Text node,
113+
and read `getComputedStyle` on the rendered element via
114+
`npm run cdp -- eval --target=viewer`. Expect the `var()` references to
115+
resolve to nothing. (REV-008 established this statically; a live confirmation
116+
costs one dev-loop launch and removes all doubt.)
117+
2. Make the call above. Record it here.
118+
3. Implement, with regression tests — `tests/models/ElementConfigRegistry.test.ts`
119+
and `tests/models/EmbeddedTemplate.test.ts` are the existing surface.
120+
4. If option A: verify token injection end-to-end in a preview, not just in a
121+
unit test. That is the specific gap that let this sit unnoticed.
122+
123+
## Success criteria
124+
125+
- [ ] One of A/B/C chosen, with the reasoning recorded
126+
- [ ] No shipped code references a CSS custom property that nothing defines
127+
- [ ] No exported class that nothing constructs
128+
- [ ] `npm run test:ci` green, `npm run typecheck:editor` clean
129+
- [ ] If tokens survive: injection verified in a running preview via CDP
130+
131+
## References
132+
133+
- `packages/noodl-viewer-react/src/style-tokens-injector.ts` — the uninstantiated injector
134+
- `packages/noodl-editor/src/editor/src/models/StyleTokens/DefaultTokens.ts` — the 10 shipped tokens
135+
- `packages/noodl-editor/src/editor/src/models/ElementConfigs/configs/` — the configs, and the vocabulary they expect
136+
- `packages/noodl-editor/src/editor/src/views/NodePicker/NodePicker.utils.ts` — where `applyDefaults` fires
137+
- [MERGE-NOTES-cline-dev-tara.md](../../reviews/MERGE-NOTES-cline-dev-tara.md) — how this arrived unverified
138+
- [REV-008-DEV-LOOP-HARDENING.md](./REV-008-DEV-LOOP-HARDENING.md) — Stream D, which surfaced it
139+
- `dev-docs/tasks/phase-9-styles-overhaul/` — the original STYLE-001/002 design intent

packages/noodl-editor/src/editor/src/models/ElementConfigs/ElementConfigRegistry.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
import { ButtonConfig } from './configs/ButtonConfig';
1616
import { CheckboxConfig } from './configs/CheckboxConfig';
17-
import { GroupConfig } from './configs/GroupConfig';
1817
import { TextConfig } from './configs/TextConfig';
1918
import { TextInputConfig } from './configs/TextInputConfig';
2019
import { ElementConfig, ResolvedVariant, VariantConfig } from './ElementConfigTypes';
@@ -176,11 +175,17 @@ function getSizeNames(nodeType: string): string[] {
176175

177176
// ------------------------------------------------------------------
178177
// Register all built-in configs
178+
//
179+
// GroupConfig used to be here. It keyed on 'net.noodl.visual.group', which is
180+
// not a node type — the real one is 'Group' (packages/noodl-viewer-react/src/
181+
// nodes/visual/group.js) — so it never matched anything and had never applied
182+
// to a single node. Deleted rather than repointed in REV-008: activating it
183+
// would stamp var(--surface), var(--space-4) and friends onto every new Group
184+
// node, and nothing defines those. See REV-009.
179185
// ------------------------------------------------------------------
180186

181187
register(ButtonConfig);
182188
register(CheckboxConfig);
183-
register(GroupConfig);
184189
register(TextConfig);
185190
register(TextInputConfig);
186191

packages/noodl-editor/src/editor/src/models/ElementConfigs/configs/GroupConfig.ts

Lines changed: 0 additions & 65 deletions
This file was deleted.

packages/noodl-editor/src/editor/src/models/ElementConfigs/configs/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,5 @@
44

55
export { ButtonConfig } from './ButtonConfig';
66
export { CheckboxConfig } from './CheckboxConfig';
7-
export { GroupConfig } from './GroupConfig';
87
export { TextConfig } from './TextConfig';
98
export { TextInputConfig } from './TextInputConfig';

packages/noodl-editor/src/editor/src/models/ElementConfigs/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,5 @@ export type { NodeModelLike } from './ElementConfigRegistry';
99
// Config objects (useful for testing or direct access)
1010
export { ButtonConfig } from './configs/ButtonConfig';
1111
export { CheckboxConfig } from './configs/CheckboxConfig';
12-
export { GroupConfig } from './configs/GroupConfig';
1312
export { TextConfig } from './configs/TextConfig';
1413
export { TextInputConfig } from './configs/TextInputConfig';

packages/noodl-editor/tests/models/ElementConfigRegistry.test.ts

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,15 @@ function makeNode(): NodeModelLike {
1717
}
1818

1919
const BUTTON_TYPE = 'net.noodl.controls.button';
20-
const TEXT_TYPE = 'net.noodl.visual.text';
21-
const GROUP_TYPE = 'net.noodl.visual.group';
20+
21+
// Registered, but defines no sizes. Note the identifier is bare 'Text', not
22+
// 'net.noodl.visual.text' — this spec had the latter, which matches no node type
23+
// and no config, so the "no sizes" assertion below was passing for the wrong
24+
// reason. Same class of bug as the deleted GroupConfig. REV-008.
25+
const TEXT_TYPE = 'Text';
26+
27+
// Deliberately not a registered type, for the unknown-type paths.
28+
const UNREGISTERED_TYPE = 'net.noodl.visual.group';
2229

2330
// ---------------------------------------------------------------------------
2431
// applyVariant
@@ -87,10 +94,15 @@ describe('ElementConfigRegistry.applySize', () => {
8794
expect(node.parameters['_size']).toBe('lg');
8895
});
8996

90-
it('is a no-op for a node type with no sizes', () => {
97+
it('is a no-op for a registered type with no sizes', () => {
98+
const node = makeNode();
99+
ElementConfigRegistry.applySize(node, TEXT_TYPE, 'sm');
100+
expect(node.parameters).toEqual({});
101+
});
102+
103+
it('is a no-op for an unregistered type', () => {
91104
const node = makeNode();
92-
// Group has no sizes defined
93-
ElementConfigRegistry.applySize(node, GROUP_TYPE, 'sm');
105+
ElementConfigRegistry.applySize(node, UNREGISTERED_TYPE, 'sm');
94106
expect(node.parameters).toEqual({});
95107
});
96108

@@ -118,7 +130,7 @@ describe('ElementConfigRegistry.getSizeNames', () => {
118130
});
119131

120132
it('returns empty array for node types with no sizes', () => {
121-
expect(ElementConfigRegistry.getSizeNames(GROUP_TYPE)).toEqual([]);
133+
expect(ElementConfigRegistry.getSizeNames(UNREGISTERED_TYPE)).toEqual([]);
122134
expect(ElementConfigRegistry.getSizeNames(TEXT_TYPE)).toEqual([]);
123135
});
124136

0 commit comments

Comments
 (0)