-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5528
bhamodi · open, request changes drafted · reviewed at b1145cf4595 · view on GitHub
Verdict: request changes — the crash it fixes is real, and the guard is narrower than the parser it is guarding
The review asks for two things: type the querySelector call in the new test, because build, build-storybook and lint are all red on it at this head, and widen or replace the ident regex, because ^[a-zA-Z][\w-]*$ rejects type names Chromium accepts. It did not merge because three CI checks are red on the PR's own new test, and because a consumer whose tokenizer emits a non-ASCII or leading-underscore type loses colour it has today while the PR's Notes say nothing changes.
A builder passes tokenizer to CodeBlock (CodeBlock.tsx:448) or to CodeEditor (CodeEditor.tsx:156), or calls the exported chunked and flat highlight functions (index.ts:30-33, reachable from the core barrel), with a token stream from a server-side tokenizer. If a type is not a CSS identifier — a TextMate-style scope such as keyword.control.sql, which is what a VS Code grammar emits — insertRule at highlightRanges.ts:82 throws, and the code block is replaced by an error. Driven in real Chromium at this head.
The PR body has the rationale but not the outcome: it says "rendering degrades gracefully rather than erroring", which is true and does not tell a reader their whole code block is currently gone. The changelog will not say what was fixed.
(1 decision · 7 runtime lines of 54)
- A token type must match
^[a-zA-Z][\w-]*$to get a dynamic::highlight()rule.
Before the component writes a colour rule for a token type it has not seen, it checks the type's name against a pattern. Names shaped like plain identifiers get a rule; anything else is skipped, so that text is still highlighted but takes the colour of the surrounding code rather than its own. The check lives beside the line that builds the rule text, which solves the crash because the browser now only ever receives rule text it can parse. One decision, tracing to the stated problem, nothing piggybacked and nothing to split.
Placement is right, and it is the strongest thing about the PR. Every seam funnels through createHighlightResolver (highlightRanges.ts:93), so one guard covers all of them — including the lab package's CodeEditor, which the rule text names in its own selector (.astryx-codeeditor, :83) and which calls the flat highlight function bare at CodeEditor.tsx:325 and inside a .then() at :352, where a throw is an unhandled rejection nothing catches. A fix one layer up in either component would have missed the other. Driven: a name-shaped type gets its rule and its colour; a dotted scope gets no rule and the block renders; a non-ASCII type gets no rule and loses its colour; the CodeEditor seam was read at those two lines rather than driven, since it shares the fixed resolver.
- A builder whose tokenizer emits dotted scope names — today the code block is replaced by an error screen; after this it renders, with those tokens in the body colour. A strict improvement, and the reason to take this.
-
A builder whose tokenizer emits non-ASCII or leading-underscore type names — today those colour correctly, since Chromium inserts
astryx-キーワードandastryx-_privatewithout complaint; after this the colour silently goes flat. New, and the PR's Notes assert the opposite: "No behavior change for … any custom tokenizer using ordinary type names." -
Everyone using the built-in tokenizer — nothing at all. All 13 types are pre-seeded into
registeredHighlightTypes(highlightRanges.ts:38) and return before the guard is reached.
Landing it newly exposes nothing: no other component takes a custom token stream, and the two that do share the fixed function.
No API change. Nothing added, removed or re-typed; the new regex constant is module-private and never exported.
No new theme targets, no raw colours, no themeable surface removed — the style grep over the three changed files returns 0.
Not fully n/a, though, and this is the half a structural grep cannot see: the guard decides whether --color-syntax-<type> is ever read for a custom type. A theme setting --color-syntax- on a non-ASCII type name resolves today and stops resolving after this.
The diff freezes nothing. It does narrow which --color-syntax-<type> custom properties are ever read, which is theme surface in practice and is carried above.
- API — no. No signature, default, export or type changes.
- Visual — yes, for one class. A custom tokenizer emitting a non-ASCII or leading-underscore type loses its syntax colour, and there is a frame pair for it.
- Theme — yes, the same class: that type's custom property stops resolving.
- Behaviour — empty, loading, error, disabled, controlled-vs-uncontrolled and boundary states are not reachable, since the diff adds no state, no default and no prop. The one state it moves through is unknown token type, walked in three variants.
The far side of the bound is the regex itself, so both sides were driven through a real CSS parser rather than at the default: accepted (sql-keyword, _private, キーワード, x--y) and rejected (keyword.control.sql, x), *, the test's own hostile string).
Why this is the review and not a footnote, since the reachability is narrow enough to argue about: the affected population is unmeasured and is deliberately not the argument. It is the review because one spelling of a type name works and another silently fails, and because the Notes tell a consumer the opposite of what happens. Absent either, it would have been a note.
Zero effects added, moved or removed. The diff adds one regex test and one early return.
ensureDynamicHighlightType runs once per distinct token type per apply pass rather than per token, because createHighlightResolver caches by type (highlightRanges.ts:93-99). Accepted types are added to the module-level registry and short-circuit on later passes; rejected types are not, because the guard returns at :61 before the add at :62, so a rejected type re-tests its regex once per pass. Distinct types in a code block are a handful. No listeners, no observers, no layout reads, no new dependency, no bundle delta. No number was put on the regex, because there is no N here that grows with anything a user can grow.
Six frames, captured in real Chromium against a Storybook dev server at this head, in the bare story iframe rather than the Storybook manager, and every one opened with the read tool. BEFORE and the try/catch build are the same worktree with highlightRanges.ts edited and reverted — same install, same browser, one session: the dotted scope before and after, the non-ASCII type before and after, the name-shaped control before and after, and the non-ASCII type under the alternative.
| difference | intentional? | source |
|---|---|---|
| dotted type: error screen → rendered code block | intentional | the PR body: "its ranges still register and paint with currentColor, so rendering degrades gracefully rather than erroring"
|
| non-ASCII type: magenta keywords → black keywords | unintentional | no sentence in the body covers it, and the Notes assert the opposite. This is the finding |
| name-shaped type: identical | intentional | unchanged path |
The alternative was built and driven, not proposed. With the regex removed and insertRule wrapped in try/catch, in the same worktree, all three stories re-run:
| case | the PR's regex | try/catch |
|---|---|---|
dotted scope keyword.control.sql
|
renders, uncoloured | renders, uncoloured |
non-ASCII キーワード
|
colour lost | colour kept |
sql-keyword |
coloured | coloured |
| the PR's own 4 tests, jsdom | 4/4 pass | 4/4 pass |
What was looked for against it and not found: a retry storm — the registry add runs before the try, so a failed type is recorded and never retried, which is better than the regex path that re-tests every pass; a second throw site — createElement, appendChild and reading .sheet do not throw, so the catch can only swallow a parse failure; and a silence regression — a swallowed parse error is exactly as quiet as the regex's early return. What was not established is that try/catch is the shape Cindy wants, which is why the review asks rather than prescribes.
One instrument check, because the result was surprising: the "arbitrary CSS" the code comment at :59 defends against is not reachable through insertRule, which parses exactly one rule. Every structural type threw, and every escaped variant the parser accepted produced exactly one declaration, color — escapes stay escaped.
Nothing rendered is added, removed or re-roled, and no string is introduced. aria-|role=|useTranslator|t('@astryx over the three changed files → 0.
- Automated coverage — the a11y job is skipped on this head, read from the check runs rather than assumed, so there is no axe signal to read and none is owed. The baseline file is untouched: the PR is not buying silence.
- In the browser — not reachable, and checked to exactly this extent: per run, the rendered frame, the highlight key list and the dynamic stylesheet's rule list were compared, and across before and after the same keys are registered and the same elements are on screen, so nothing focusable is added or removed. The DOM was not diffed, so the claim is "no element added or removed", not byte identity.
- Strings — no new key, no new user-visible or AT-visible string.
- Direction — the diff writes no CSS property, and the RTL job skips for the same reason.
One thing here is an i18n consequence, and it is the same finding rather than a second one: ^[a-zA-Z][\w-]*$ is ASCII-only, so a tokenizer whose token vocabulary is not Latin loses its colours. \w in a non-unicode JavaScript regex is [A-Za-z0-9_], while CSS custom-idents accept U+0080 and above, which is why the Japanese ident inserts cleanly.
request changes. The goal is met — the crash the PR exists to fix is gone, measured at this head in real Chromium: with the guard reverted, the dotted scope leaves no code block in the DOM and a parse error on screen; with it restored, the block renders and nothing throws.
Four slots wrote down one thing — the regex is narrower than the CSS parser — and they compound into one finding rather than four; that is what the review spends itself on. And one finding arrived in no slot at all: CI is red, and all three red checks are the PR's own new test. It is not a performance or visual defect, it belongs to the change's mechanics, and it is the thing that literally prevents merge.
1. document.querySelector('style[data-astryx-highlight-dynamic]')
returns Element
→ build, build-storybook and lint are all red at this head, so the PR
cannot merge; typing the call clears all three
· highlightRanges.test.ts:88
2. ^[a-zA-Z][\w-]*$ is narrower than the CSS parser
→ a builder whose tokenizer emits `_private` or a non-ASCII type has
coloured code blocks today and flat ones after the upgrade, with the
changeset saying nothing changed
· highlightRanges.ts:47
Two things were accepted rather than asked. The changeset ships the mechanism and never the crash — whoever reads the changelog cannot tell a crash was fixed — and the author is already pushing again for two real asks while editing that file, so a third round trip for one clause is not worth their evening; it is a wording gap rather than API or design debt. And the code comment at :59 overstates the threat as "arbitrary CSS", which is folded into the second finding's wording, because it disappears with the mechanism question and raising it separately would be churn.
Nobody is stuck, nothing is unreachable, no data is lost, so this is a repairable defect inside the fix rather than something for a human to rule on, and the ask is small either way. No design call is involved: no new prop, export, accepted value, default or theme target.
Three things found and not spent on the author: the exported batch function is called nowhere in the repo, pre-existing and not the contributor's to carry; and the PR's own test passes identically against both mechanisms, 4 of 4 each, because jsdom's CSS parser accepts rule text Chromium rejects — so the test cannot distinguish them and proves nothing about a real browser.
Not verified: whether any consumer today ships a tokenizer with a non-ASCII or leading-underscore type name — the narrowing is proven, its blast radius is not — and Firefox and WebKit, since every parser result here is Chromium, so the mismatch could be wider or narrower elsewhere.
Thanks for this — I reproduced the crash: a tokenizer emitting
keyword.control.sqltakes the whole code block down today, and the guard fixes it.Typecheck and lint fail on the new test, so
build,build-storybookandlintare red.querySelectorreturnsElement:document.querySelector<HTMLStyleElement>('style[data-astryx-highlight-dynamic]')The bigger one:
^[a-zA-Z][\w-]*$is narrower than the CSS parser. Chromium insertsastryx-キーワードandastryx-_privatefine, so a tokenizer using either loses its colours after this — and the Notes say nothing changes. The injection it defends against doesn't reach either:insertRuleparses one rule, so every structural type I tried threw, and every type it accepted produced onlycolor.I ran the alternative: dropping the regex and wrapping
insertRulein try/catch fixes the crash and keeps those colours. Would that work here?If you'd rather talk it through with someone, we're in Discord.
Inline: packages/core/src/CodeBlock/highlightRanges.test.ts:88 — querySelector returns Element here, so .sheet doesn't exist. Might need <HTMLStyleElement>. · highlightRanges.ts:47 — Hmm, Chromium takes _private and non-ASCII idents. This rejects both.
Whoever posts publishes the four frames — the dotted-scope pair and the non-ASCII pair — to the assets branch on the fork, never to facebook/astryx, and embeds them: both visual sentences in the comment are unverified to a reader without them. If the upload fails, post anyway and say the frames could not be attached.
One review, four gate passes.
-
Gate 1 — failed on eight counts. The remedy was proposed rather than built: a fix designed while reading the diff, handed to a contributor with the review's confidence and none of its evidence. The class was never enumerated — the evidence proved the finding for
CodeBlockand never asked who else was on this path, while the rule text the diff builds names the editor's own selector, the tell sitting in the diff. Anchors were verified at head but their text was not pasted into the draft, leaving the next reader a number to re-fetch instead of a string to compare. Two audits were run mentally and not recorded, and a pass that is not written down reads identically to a step that was skipped. The frames existed and had been opened, but nothing told whoever posts to publish and embed them. The deliberate omission of a full-review link was silent, so it was indistinguishable from a forgotten one. The inline voice was declarative where hers hedges. And the time numbers did not add up. - Gate 2 — three more. An a11y claim said the DOM was byte-identical when what had been captured was the frame, the highlight keys and the rule list — a claim one notch stronger than its evidence, in the slot graded hardest on that distinction. The built alternative was asserted better without saying what had been looked for against it and not found. And the block was asserted twice without ever being tested against the overstatement it could be: the draft admitted under "what I could not verify" that the blast radius is unproven, and then blocked on it, without reconciling the two.
- Gate 3 — one, and it was gate 2's correction overshooting. Several slots ran past the four-sentence ceiling, the two worst being the paragraphs gate 2 had asked for. On a reader who does not scroll, that is the same defect as the overclaim was: the slot stops being a summary.
- Gate 4 — clean. Nothing else grew in the trim; the comment was unaffected by rounds 2, 3 and 4 and posts as drafted.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines, and the frames are unpublished. The presentation's merge trailer is a hold whose content is the red build — three CI checks failing on the PR's own new test — with the regex finding beside it. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5528) line goes in before the attribution.
HEAD REVIEWED: c16568b50f6ec156a7f56961f41cb4341afc22e5
LOOP VERSION: 1.5.0
AUDIT RUBRIC: 1.13
Baseline: e5e83effd87469c11a9818c9cbfb6e49d32bcca2
Outcome: request changes — both prior blockers are fixed; exact-head lint and the stale PR body remain.
| earlier ask | current head |
|---|---|
Type the dynamic-style querySelector
|
satisfied — querySelector<HTMLStyleElement>; build and build-storybook pass |
| Replace the narrow regex with identifier escaping | satisfied — both generated names use CSS.escape; parser rejection is contained |
The focused test fails 1/4 against origin/main at the unescaped insertRule and passes 4/4 at this head. Exact-head lint alone fails in changed code: highlightRanges.test.ts:22 wraps an already-string value in String().
The runtime fix is one decision at the existing shared owner, with no API, theme target, state, Effect, listener, or dependency change. CodeBlock’s chunked path and CodeEditor’s flat path both reach the same resolver.
The PR description has not moved with the code: it still says a regex allowlist skips non-name-shaped types, while this head escapes and preserves those names. The author can proceed by removing the redundant conversion and updating the description.
Manual exact-head frames are required before a later approval: the acceptance claim is visible, and the green Stable visual regression job has no CodeBlock story with a custom tokenizer. This static round did not start Storybook or Chromium; it did not need that evidence to establish the current lint block.
Thanks — the two earlier blockers are fixed at this head: the typed
querySelectorleaves build/Storybook green, andCSS.escapereplaces the narrow allowlist. The focused regression test fails againstmainand passes 4/4 here.Two small pieces remain: lint rejects the no-op
String(value)inhighlightRanges.test.ts:22, and the PR body still describes the removed regex/skip behavior. Could you remove that conversion and update the body to match the escaped-name behavior? If you'd rather talk it through with someone, we're in Discord.[Reviewed by Robohands]
- Focused Vitest: current 4/4; baseline 3/4 with the expected parser throw.
- Focused ESLint: reproduces the redundant conversion; exact-head strict CI treats it as the sole lint error.
- Exact-head CI read: build, build-storybook, test, pr-a11y, pr-rtl, and Stable visual regression pass.
- Two critic passes; gate files are session-local under
review-artifacts/pr-5528/. - Wall time: setup/rules 2m; install/build/server 1m; browser/a11y 0m; focused tests 1m; code/history 2m; critique+wiki 12m (including 10m stale-lock wait); CI wait 0m.
No public PR action was taken.