fix(blocks-engine): a reference is held to the walk's name rule, and refusals keep the type line - #1885
Conversation
…refusals keep the type line
isReference accepted {base.$private}: it checked emptiness and the forbidden
characters, not the $ prefix the walk treats as reserved. Both now ask
isDtcgName, built from isReservedKey and DTCG_NAME_FORBIDDEN, which read() and
unreadTokenParts also use to tell a field from a name.
readToken returned early for a malformed or unauthorable name, an unusable
stated id and a naming cap before any typeUnread call, so an ignored $type
beside those went unsaid. Every refusal now goes through one routine that
names the ignored type first, crediting no group.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe DTCG walker now shares reserved-key and name validation across reads and references. Token refusals use a common path that reports unusable ChangesDTCG validation consistency
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The DTCG import-report fixes are covered by targeted tests, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/blocks-engine/src/style/dtcg.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/blocks-engine/src/style/dtcg.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9436e9a63e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const walked = names({ | ||
| [key]: { t: { $type: "number", $value: 1 } }, | ||
| }).includes(`${key}.t`); |
There was a problem hiding this comment.
If read() stops routing $private or $root through isReservedKey() and descends into that object, this assertion still passes: names() remains empty because the downstream token-name validation rejects $private.t, while isReference() also returns false. The test therefore does not verify the claimed agreement between reference parsing and group traversal; assert a walk-specific diagnostic or the shared predicate so traversal itself is observable.
AGENTS.md reference: AGENTS.md:L216-L220
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The walk was never observed: a later name check refuses $private.t whether or not read() routed the key aside, so names() came back empty either way. Fixed in d3850b7b1.
Each key is now judged by the walk's own line about it:
- the reserved-field line (
"$private" is a design-token field this site does not read) when the walk routes the key aside; - the malformed-name line (
"a{b" is not a usable name in a design-token file) when it rejects the name.
For each row, the test asserts which of the two lines is present and that the other is absent. So brand passes because the walk wrote neither line, not because a lookup found nothing. It then asserts that {key} is reported as an inheritance exactly when the walk accepted the key.
Break-verified on the committed tree: routing $private as a name (if (isReservedKey(key) && key !== "$private")) now fails this test by name. Engine dtcg.test.ts 144/144; blocks-engine lint and check-types (44/44) pass; the comment check is clean; fallow passes on 3 changed files with nothing introduced.
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-mcp
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
… about each key The agreement test called a key read as a group when its token name came back, and a later name check refuses $private.t whether or not read() routed the key aside, so the walk itself was never observed. Each key is now judged by the walk's reserved-field or malformed-name line, each asserted present.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
Two defects in the DTCG token reader's import report. Both were found after #1867 merged, and both come from one question being answered in more than one place.
1.
$extends: "{base.$private}"was called an inheritanceisReferencejudged each segment of a{group.path}reference by emptiness and the forbidden characters.,{and}. It never checked the$prefix, which the walk reserves for the format's own keys and never reads as a group. So a reference running through$privateor$rootgot the line "inherits from another group" when it names nothing.isReservedKey(key): the one test for "this key is the format's own".read()uses it to route a group's$keys tounreadGroupField, andunreadTokenPartsuses it to tell a token's fields from its children.isDtcgName(segment): a segment that is not blank, not reserved, and free ofDTCG_NAME_FORBIDDEN.isReferencenow asks this for every segment, so a reference can't accept a name the walk would never read.2. A refused token lost the line about its ignored
$typereadTokenreturned early before reachingtypeUnreadin four cases: a name with a forbidden character, a name this site can't author, an unusable stated id, and a name over the cap. A non-string$typebeside any of those went unreported, while the later refusals (no kind, unreadable value, unsafe value) did report it.refuse(...reasons): every exit inreadTokennow goes through this one routine. It names the ignored type first, credits no group, then pushes the refusal.isWritableValue's lines and passes them throughrefuselike every other exit.Tests (engine
dtcg.test.ts, 141 → 144)calls $extends an inheritance only when it names something: adds{base.$private},{$root}and{a.$b}, each expected to get the generic unread-field line. The existing{base}control still gets the inheritance line.reads a key as a group name exactly when a reference may name it(new): an agreement test. Forbrand,$private,$root,a{banda}b, it checks that the walk reads the key as a group exactly when{key}is reported as an inheritance. A control proves both answers occur.is named ahead of every refusal, whichever check refused the token(new): six labelled rows, one per refusal (forbidden character, unauthorable name, unusable id, length cap, no kind, unreadable value). Each row first asserts its own refusal line is present, so it reached that exit. It then asserts the type line is present, comes before the refusal, and credits no group.Verification
Red first: the three tests above failed on
main's reader for the intended reasons.Gates:
dtcg.test.ts144/144; buildertokens-transfer.test.ts58/58pnpm check-types44/44 from the root; lint 2/2FALLOW_AUDIT_BASE=origin/main fallow audit: pass, 3 changed files, every*_introducedat 0check-comment-conventionclean;check-changesetscovers the groupMutants: every mutant is killed on the committed tree, each by the test named for its property, and the files are restored byte-identical after the run. The 11 new ones:
$rule fromisDtcgName;isReferenceits own predicate again;refuse;refuse(malformed name, unauthorable name, unusable id, naming cap, no kind, unreadable value, unsafe value).The earlier rounds' mutants whose target text this change removed were retargeted at the new code and are killed too.
Changeset
a-token-reference-names-only-a-group-the-reader-walks.md: all 26 lockstep packages,patch.Summary by CodeRabbit
Bug Fixes
$-prefixed names and references.$typedeclarations are reported consistently alongside other validation errors.Tests