-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5521
bhamodi · open, approve posted · reviewed at 1dd147f0c96 · view on GitHub
Verdict: approve — the loop merges it
The CI comment's own structure is assembled by pasting report-file strings into markdown, so a value carrying a newline or a pipe becomes structure rather than text. Reproduced at main: an esmSize of "1 KB |\n| ROW | INJECTED |" renders a fake extra row inside the component's metrics table, and a helpUrl of javascript:alert(1) renders as a live [Learn more] link. The person is whoever reads the bot comment to decide whether a PR is safe — they get a table with a row nobody generated.
Who can put such a value there today: nobody by accident. Measured — across 173 story files, 294 story titles and 1,226 story names on main, zero contain any of | [ ] ` * _ ~ < >. This is the reachable-but-unexercised case, where the vector is a contributor authoring a story title or a component name.
(3 sub-decisions, one judgement · ~60 runtime lines of 255)
-
inline()/num()applied at every report-field interpolation in the three files — the fix. - The two values that land inside a code span or an href are allowlist-reduced rather than escaped — axe rule id to
[\w-], Storybook story path to[a-z0-9_-]. -
safeUrl()drops a link whose target is not a plain absolute http(s) URL;extLinkpercent-encodes quotes in the href.
The three are spellings of the same boundary and cannot ship apart, so the count is one judgement rather than three. Anything the comment prints that came out of a report file is now treated as words rather than as formatting: a report string is flattened to one line and its markdown/HTML punctuation neutered, a numeric field is forced to be a number, and a link is drawn only when its target really is a plain https:// address. The generator's own literals remain the only source of headings, tables and <details>.
The escaping lives at the interpolation sites in .github/scripts/generate-pr-comment.js, which owns the comment's structure. buildA11ySection and buildVisualSection are also called by test-pr-enrichment.js and weekly-a11y-summary.js, and both inherit the fix for free because it lives in the shared libs rather than in the caller. One existing helper was checked before calling this duplication: escapeHtml() at .github/scripts/visual-gate/lib/report.mjs:18 does a different job — it escapes for an HTML document, where inline() escapes for markdown inside a comment and also one-lines — so consolidating them would be the wrong consolidation.
Before any of it was run, the seven changed files were read end to end, because the head is a fork's: no package.json script change, no dependency, no lockfile change, no workflow change, nothing reading env vars, ~/.npmrc or ssh keys, no network call. Pure string functions plus vitest files.
Invisible to everyone today. Measured on a realistic fixture — a new component, a modified component, two a11y violations, two visual changes — the generated comment is byte-identical before and after, except that a value containing & now emits &, which GitHub renders as &. No real story name in the repo contains &.
Nothing reaches an end user of Astryx and nothing reaches a builder: .github/ is outside every workspace glob (pnpm-workspace.yaml lists apps/* and packages/*), so none of this ships in a package. The audience is the person reading a PR comment, and the change is what they don't see — a report string can no longer add a row, a heading, or a link. Landing it newly exposes nothing; it closes a hole rather than half of one.
No API change. No package file is touched; nothing here is reachable from any barrel.
n/a — no styles, no theme targets, no rendered class names. The style grep over the seven changed files returns 0. No new theme targets.
Nothing published ossifies. For the internal addition: .github/scripts/lib/report-text.js exports inline, num, safeUrl, and its only consumers are the two sibling libs and the generator, all inside .github/scripts, so nothing outside the repo can import it. The class it serves has three existing members already — the two format libs and the generator — plus two indirect ones in test-pr-enrichment.js and weekly-a11y-summary.js. It is not a one-case helper.
Consumer who can be broken: none — .github/ is outside pnpm-workspace.yaml's apps/* / packages/*, so nothing here is published.
- API — no. Nothing exported, nothing published.
- Visual — no, in the product. In the bot comment, only for values no repo file currently contains.
- Theme — no.
- Behaviour — the states are the report-file shapes, walked against both builds:
| state | before | after |
|---|---|---|
| normal values | table renders | byte-identical (measured, realistic fixture) |
missing field (undefined/null) |
N/A |
N/A — inline(null) is '', so || 'N/A' still fires |
numeric field that is genuinely 0
|
N/A |
0 — inline(0) is '0', which is truthy |
numeric field that is a string ("1234") |
1234 (String.toLocaleString is a no-op) |
1,234 — num() makes it a number first |
| newline / pipe in a string | breaks the table | one cell |
non-URL helpUrl
|
live link | link dropped, text kept |
| empty / no violations / no changes | unchanged paths, not touched by the diff | same |
Rows 3 and 4 are the only behaviour changes on non-hostile input, and both are corrections rather than regressions. ci: is an honest commit type: seven files, all under .github/scripts/, no changeset.
Zero effects — a Node script, no React, no listeners, no observers, no layout. Bundle: nothing ships, .github/ being outside the workspace, which is also what the PR's own Analysis comment reports ("No component packages changed.").
Cost of the change itself: three regex passes per interpolated field on a comment built once per CI run. Not measured, and said rather than called negligible — the reason it was not measured is that the whole comment is a few hundred interpolations on a job that already spent minutes building Storybook, and no user waits on it. No perf test required: no O(N) work on a user-facing flow.
No frames: not renderable — CI script code, nothing in the product renders differently. The equivalent artefact is the generated comment text, captured before and after from both builds in one session, main's three files restored from origin/main into a scratch dir and the generator run against the same fixtures.
Realistic fixture, one line differs:
- | Button | Axes & Grids | stone | dark | 98 |
+ | Button | Axes & Grids | stone | dark | 98 |Intentional, and its source is the PR's own sentence: "report fields render as data". Renders identically on GitHub. Everything else is byte-for-byte the same.
Hostile fixture — esmSize: "1 KB |\n| ROW | INJECTED |":
BEFORE (main) AFTER (this PR)
| Bundle Size (ESM) | 1 KB | | Bundle Size (ESM) | 1 KB \| \| ROW \| INJECTED \| |
| ROW | INJECTED | | | Bundle Size (CJS) | 2 KB |
| Bundle Size (CJS) | 2 KB |
Hostile verdict reason — "line one\n## FAKE HEADING\n\n**Status:** all clear":
BEFORE **Status:** Skipped — line one
## FAKE HEADING
**Status:** all clear
AFTER **Status:** Skipped — line one ## FAKE HEADING \*\*Status:\*\* all clear
Hostile helpUrl: "javascript:alert(1)":
BEFORE - Rule: `x` · Affects 1/1 stories · [Learn more](javascript:alert(1))
AFTER - Rule: `x` · Affects 1/1 stories
One harness error, recorded: the first before/after run reported "byte-identical" because zsh does not word-split an unquoted $ARGS, so both invocations got zero flags and both produced a 362-byte stub. Caught by checking the output size before believing the diff.
Nothing here is touched, and this is what was checked.
-
aria-|role=|useTranslator|t('@astryxover the seven changed files → 0. No roles, no ARIA, no catalog strings, nopackages/core/locales/en.jsonentry. - The a11y report is data this code prints; the audit itself (
pr-a11y,.github/a11y-baseline.json) is untouched and the baseline file is not in the diff — nobody is buying silence. - No direction, no logical properties, no glyphs.
- Not driven in a browser, and nothing here needs to be: no rendered output exists.
approve and merge. Low-risk on all four counts: no new API surface, no behaviour regression (walked in the breaking table, measured on a realistic fixture), no perf or resource regression, nothing grew. Read against the slot bodies rather than their labels, the two behaviour changes — a genuine 0 now printing as 0, a numeric string getting thousands separators — are both the field printing what it actually is. Nothing describes someone stuck, unreachable, unheard, or losing data.
1. getStorybookLink drops characters where Storybook replaces them with a dash
→ someone clicking "View in Storybook" on the bot comment for a component
whose story title has a space lands on Storybook's "story not found" — but
that link is already broken today (the space survives into the href), so
this is not a regression, and `[^a-z0-9]+ → -` on the line the PR is
already editing fixes it · generate-pr-comment.js:58
Not blocking, and measured rather than asserted: over all 173 story files, 9 have a title whose id changes under the new regex — Core/Control Size Comparison, Core/Layer Dismissal, Core/Icon/Size Theming, Core/Themes/MediaTheme Auto, Lab/Chart Interactions, Lab/Chart Interactions/Coordinated Views, Stocks price progression 2000-2010, and two where getStoryTitle's first-title:-in-the-file regex has picked up a fixture string rather than the meta title (Schedule.stories.tsx → "Company all hands", TableTree.stories.tsx → "VP Engineering"). Storybook's sanitize maps every non-alphanumeric run to -, so the correct id for all nine has dashes; before this PR the space survives and after it the space is deleted. Both wrong, neither worse.
CI: 5 workflow runs on this head, all success, and the new tests are genuinely in the run — vitest.config.ts's node project includes .github/scripts/**/*.test.{ts,tsx,mjs}, and CI's test job runs the full suite for any non-docsite PR. Run locally too: 25 passed, matching the PR's test plan exactly. The repo's needs:code-review label is on this PR because review-signal.yml:299 adds it for a community contribution, not for any high-risk area — the gate working, not a finding.
Three things found and not spent on the author: inline(x) || 'N/A' turns a genuine 0 into "0" where main printed N/A (propsCount, linesOfCode, fileCount), more accurate rather than worth a line; safeUrl() is applied to --visual-report-url while extLink only quote-encodes the other three URLs, but all four come from the workflow rather than a report file, so the inconsistency reaches nobody; and inline()'s markdown backslash-escapes show as literal backslashes inside <summary>, on hostile values only.
Not verified: whether any of the nine spaced story titles is actually reachable as a component's storyTitle in a real run — getStoryTitle prefers an exact <Name>.stories.tsx and every component's match was not enumerated. It does not change the finding, since both spellings are broken either way. And the per-run cost of the three regex passes, as above.
Thanks — good catch, and the tests are the right shape. One nit inline, on a link that was already broken; happy to take it as is.
[Reviewed by Robohands]
Inline: .github/scripts/generate-pr-comment.js:58 — Storybook dashes these rather than dropping them; [^a-z0-9]+ → - also fixes titles with spaces.
One review, two gate passes.
- Gate 1 — failed on three counts. The inline asked the author to change a line whose symptom is inherited debt and never said so — the private brief established it plainly ("that link is already broken today"), the text the author reads did not, and the summary had eleven words of headroom to carry it. "Spaced titles" was a compressed referent a reader who had not just read the diff could not resolve. And the time slot still read "(filled at the end of the run)", which is an unfinished slot.
- Gate 2 — clean. All three closed and each re-checked rather than trusted: the summary now says "on a link that was already broken", "spaced titles" became "titles with spaces", and the time slot is filled with its waste named. The head was re-fetched immediately before the pass and was unmoved; the one anchor was opened at head and its line text pasted so the next check is a string comparison.
Posted as drafted, with a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5521) line appended before the attribution.