fix(codegen): parse rem token lengths - #313
Conversation
|
I have enough context to write the review. Review:
|
| Check | Result |
|---|---|
unsafe outside plumb-cdp |
✓ none |
unwrap/expect in library code |
✓ pre-existing #[allow] scoped to #[cfg(test)] mod only |
println!/eprintln! |
✓ none |
SystemTime/Instant |
✓ none |
todo!/unimplemented!/dbg! |
✓ none |
HashMap/HashSet in observable output |
✓ none |
| Layer violation | ✓ no Cargo.toml change |
| New dep with restricted license | ✓ no new deps |
| MCP tool added without protocol test | ✓ not applicable |
Issues
ts_tokens.rs:682–684 — em is silently treated as rem
.strip_suffix("rem")
.or_else(|| trimmed.strip_suffix("em"))em is relative to the current element's computed font size, not the root's. Treating them identically at * 16.0 is a pragmatic approximation that holds for top-level design-token files, but it will silently misinterpret nested em values (e.g. line-height: 1.5em in a component token). No comment explains the assumption. A future reader will read this as "intentional equivalence" and replicate the pattern in the wrong context.
Minimum fix: a short comment on the or_else branch, e.g.:
// em in token files is treated as root-relative (= rem) — component-scoped
// em values are not expected in design token exports.
.or_else(|| trimmed.strip_suffix("em"))ts_tokens.rs:678 — case-insensitive px is an undocumented behaviour expansion
The to_ascii_lowercase() call was added to support REM/EM, but it also silently broadens px parsing: "16PX" now parses where it previously returned None. The commit message only mentions rem. There is no test for "1PX", "1REM", "1EM" (uppercase), so the case-fold path for px is untested and the expansion is invisible to future reviewers.
ts_tokens.rs:908–945 — test has no isolation for the em path and no edge cases
The added test mixes rem and em in one object, making it impossible to know if a regression dropped em support (both values would have to fail for the radii assertion to catch it). Desirable additions:
- An
em-only assertion. "0rem"→Some(0)."rem"(no digit) →None.- A case-insensitive variant (
"0.5EM","16PX") to cover the folding path explicitly.
Minor (non-blocking)
decimal_to_u32_value at ts_tokens.rs:704 is a private helper extracted from decimal_to_u32. The extraction is clean. No issues.
Verdict: REQUEST_CHANGES
Summary
remandemstring lengths in literal TypeScript/JavaScript token modulesDogfood
packages/theme/src/tokens/radius.tsusesradius.base = "0.75rem".plumb init --from apps/webdid not infer any radius scale from that module.[radius] scale = [12].Validation
cargo fmt --all -- --checkcargo test -p plumb-codegencargo test -p plumb-cli --test init_fromcargo clippy -p plumb-codegen -p plumb-cli --all-targets --all-features -- -D warningscargo build -p plumb-cliRUSTDOCFLAGS='-Dwarnings' cargo doc --workspace --no-depsjust validatePLUMB_CHROME="/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" scripts/noise-scoreboard.sh --liveScoreboard