feat: warn in doctor when a stylesheet link skips asset() - #1244
Conversation
A page or layout that hand-writes <link rel="stylesheet" href="/public/..."> serves it at an un-versioned url, so a CDN keeps the pre-deploy bytes for the whole TTL. That shipped a visible regression on webjs.dev: the edge served a tailwind.css built before the deploy, so a new page rendered with its content edge to edge and its grid collapsed, because the cached css lacked the arbitrary-value utilities that page introduced. asset() (#1194) already fixes this, but it is opt-in and the <link> is hand-written HTML, so it is easy to omit. Rails and Remix close the same gap at authoring time (a helper over a digest manifest, a hashed url from the build graph), never by rewriting a rendered document. This advisory does the same: it reads the author's source, names file:line, and rewrites nothing. Scoped to rel=stylesheet. An icon is a legitimate deliberate non-mark (the website leaves its favicons bare so the SEO tests parse the hrefs literally), and a rel=preload must stay unversioned or its hint could never match the request a CSS url() actually makes.
Documents the new check on the two surfaces that describe it: the CLI's doctor row, which enumerates every check, and the skill's built-ins caching section, next to asset() itself, where an author reading about the helper learns that forgetting it is caught. Both state the scope and the reason it reads source rather than rewriting output, so the #1196 rejection is not re-litigated later.
The scan looked ahead for rel=...stylesheet anywhere in the tag, which matched the string inside ANOTHER attribute's value. That flagged the two shapes the check most needs to leave alone: the canonical async-CSS <link rel="preload" ... onload="this.rel='stylesheet'">, where the advised asset() fix would actively break the preload because the versioned hint could never match the unversioned request, and a data-rel="stylesheet" sitting on a genuine rel="icon". Parse the tag's attributes instead. Each quoted value is consumed as one unit, so it can never be re-scanned as if it held an attribute of its own, and rel now means the rel attribute. The fast bail was also case-sensitive while the scanner was /i, so a file whose only tag was <LINK> was skipped before the scanner could see it. Regression tests cover the onload swap, the data-rel near-miss, the uppercase tag, and a > inside a quoted value.
Four defects from review, all verified against a fixture first. The check flagged hrefs asset() provably cannot fingerprint. resolveAssetUrl returns a path carrying a query or a .. UNCHANGED, so an author who wrapped one saw the warning stay: doctor --strict could never go green. A hand-rolled ?v= cache-buster is the likeliest thing an author who has not adopted asset() already wrote, so this was the common case. Mirror resolveAssetUrl's own refusals instead. The check was silently inert for every webjs.basePath app, which writes /myapp/public/app.css because the author supplies the prefix themselves. Those apps have the MORE ceremony to forget, so read the key and strip it. A commented-out tag emits nothing but was flagged. Blank HTML comments before scanning, length-preserving so reported line numbers still point at the real source. The docs site taught the exact markup the check now warns about, and two enumerations of doctor's checks (the configuration page and the help banner) had gone stale.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review: rel was being read as a substring, not an attribute
Two real problems in the scan, both now fixed.
The lookahead searched for rel=...stylesheet anywhere in the tag, so it matched the string sitting inside ANOTHER attribute value. That flagged the canonical async CSS idiom, <link rel="preload" as="style" href="/public/app.css" onload="this.rel=stylesheet">, which is the one shape the check most needs to leave alone: applying the advised asset() fix there versions the hint so it can never match the unversioned request, and the file downloads twice. A data-rel="stylesheet" on a genuine rel="icon" tripped it too, since the word boundary matches after a hyphen. I confirmed both against a fixture before touching anything. Fixed in 8a71d071 by parsing the tag attributes, so each quoted value is consumed as one unit and can never be rescanned as though it held attributes of its own.
The fast bail was also case sensitive while the scanner carried /i, so a file whose only tag was <LINK> was skipped before the scanner could ever see it. Same commit.
Regression tests cover the onload swap, the data-rel near miss, the uppercase tag, and a > inside a quoted value.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review: the check was advising fixes that cannot work
Four findings, all reproduced against a fixture first, all fixed in ff561e75.
The worst one: the check flagged hrefs that asset() provably cannot fingerprint. resolveAssetUrl returns a path carrying a query or a .. unchanged, so an author who took the advice and wrapped /public/app.css?v=3 would see the warning stay put forever, and doctor --strict could never go green. A hand rolled ?v= cache buster is the likeliest thing an author who has not adopted asset() already wrote, so that was the common case rather than a corner. The check now mirrors resolveAssetUrl own refusals, so every flagged href is one the fix actually resolves.
It was also silently inert for every webjs.basePath app. Under a sub path the author writes /myapp/public/app.css themselves, the check only knew /public/, and those are precisely the apps carrying the extra ceremony to forget. It reads the key now and strips the prefix before the test.
A commented out tag was flagged as though it were live markup. Comments are blanked before scanning, length preserving so the reported line numbers still point at the real source.
Last one is the docs. The styling page taught the exact markup this check now warns about, so anyone copying the documented layout would get a warning naming their own file. Fixed, and the two enumerations of what doctor checks (the configuration page and the help banner) were stale, so both list it now.
One thing I deliberately did NOT change. The broader concern that a page which DISPLAYS a tag is treated the same as one that EMITS it holds only for an unescaped sample, and an unescaped <link> in a page template really does emit a live tag into the rendered HTML, so flagging it is correct rather than a false positive. The commented out case was the genuine miss and that is the part I fixed.
Six defects from two more review reads, each reproduced against a fixture before being touched. Scope was wrong three ways. basePath was read as 'must start with a slash', while normalizeBasePath trims and PREPENDS it, so an app configured "myapp" stayed silently inert: the exact defect the previous commit claimed to close. The walk descended into _private folders the router never routes. And only page and layout were scanned, missing the always-shipped error boundaries and global-error, which renders its own head and is the likeliest place outside the root layout to hand-write a stylesheet link. The refusal set was applied to the raw href while resolveAssetUrl decodes first, so /public/%2e%2e/x raised a warning asset() can never clear and /%70ublic/app.css was silently missed. Only HTML comments were blanked, but a tag is commented out in a .ts page with // or a block comment, so that warning could only be cleared by deleting a comment. All three forms are blanked now, still length-preserving, and the // matcher will not fire on a https:// url. Docs: the root AGENTS.md line never listed the advisory, and the two surfaces updated last commit had gone stale against the widened scope.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review: the basePath fix did not actually cover the documented config forms
Four findings on the fix commits, all reproduced first, all fixed in 22677118.
The basePath handling was the embarrassing one. I read the key as "must start with a slash", but normalizeBasePath trims and PREPENDS the slash, so "myapp", "/myapp" and "/myapp/" are all one base path. An app configured the first way stayed exactly as inert as before, which is the defect the commit claimed to close. It is a faithful port now, including the fail safe rejects for .., a scheme, a backslash, whitespace and a //host prefix, and the tests pin all three accepted forms plus the rejects.
The refusal set was applied to the raw href while resolveAssetUrl decodes first, so /public/%2e%2e/x raised a warning asset() can never clear and /%70ublic/app.css was silently missed. It now mirrors that function in its own order: strip the base path, cut at ? and #, decode, then judge.
The walk also descended into _-prefixed folders. The router drops any route with such a segment, so that markup is never routed and never rendered, and advising on it is advice about dead code.
Last one was the root AGENTS.md CLI line, which never listed the advisory. Worth noting that line was already a lossy summary (it omits the static-asset freshness check too), so it is a summary rather than a maintained enumeration.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review: the check only saw half the modules that render, and half the comments
A whole diff read. Three real findings, all fixed in 22677118, plus one correction to the body.
Only HTML comments were blanked before scanning. A tag gets commented out in a .ts page with // or a block comment, so the common form was still read as live markup, and that warning could only be cleared by deleting a comment. All three forms are blanked now, still length preserving so the reported line is right, and the // matcher will not fire inside a https:// url.
Scope missed the boundary modules. Only page and layout were scanned, but error, not-found, forbidden, unauthorized and loading always ship, and global-error renders its OWN doctype and head with no framework splice, which makes it the likeliest place outside the root layout to hand write a stylesheet link. All of them are covered now.
The percent encoded traversal case was the same divergence the other read caught, fixed in the same commit.
The body test plan had also gone stale (it still claimed five new cases and an old pass count). Corrected.
Two behaviour defects and four stale claims. The widened module set added global-error and global-not-found, but router.js registers both only at the app root (dir === '.'), so a nested one is never routed and warning on it is advice about dead code, the same class the _private skip exists to avoid. The // comment blanking was a flat regex guarded only against https://. It fired on a protocol-relative href="//cdn/x.css" and blanked the rest of the line, so a layout mixing a CDN sheet with a local one went silently inert: exactly the shape this check targets. Comment blanking is a quote-aware walk now, because the two comment families live in opposite contexts. An HTML comment must be blanked INSIDE the template literal holding the tags, a JS comment only OUTSIDE a string. The recorded rationale for skipping a query or .. href was also wrong. It claimed asset() could never clear the warning, but this check reads the SOURCE shape, so a wrapped href is an unquoted hole and does clear it. The real reason is that the wrap is a runtime no-op there, so it would clear the warning without improving the caching. Remaining edits are the scope wording that widening left stale: the user-facing pass message, three in-file contract lines, the docs-site configuration page, and a built-ins sentence that still said this was a convention rather than something a tool checks.
Two lexers, two bugs. The flat // regex blanked the rest of any line
holding a protocol-relative url. The quote-tracking walk that replaced it
broke on the framework's most common idiom, a nested html`` inside a ${}
hole: one quote char cannot model nesting, so the inner backtick read as
closing the outer template and inverted string/code polarity for the rest
of the file. An unbalanced apostrophe in nested template text then
desynchronized everything after it and resurrected dead markup as a
finding the author could only clear by deleting a comment.
The check never needed to lex JavaScript. It needs one answer, is this
tag commented out, and two local stateless tests give it: an HTML comment
is unambiguous so scan backwards for the nearest <!-- against the nearest
-->, and a JS comment is judged from the tag's own line prefix, which is
how commenting one out is actually written. No cross-line state, so there
is nothing left to desynchronize.
The residual gap is a tag commented out mid-line after real code, which
stays reported. Rare, and it fails toward reporting rather than toward
the silent inertness both lexers produced.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review: the comment walk breaks on nested templates, so I stopped lexing
One finding, and it was in code I had already rewritten once.
blankComments tracked string context with a single quote char, which cannot model a nested ``html...``` inside a ${}` hole, the most common idiom in the framework. The inner backtick read as CLOSING the outer template, so string and code polarity inverted for everything after it. I reproduced the consequence: an unbalanced apostrophe in nested template text (`it's`) desynchronized the rest of the file, and a genuinely commented-out tag further down came back as a finding the author could only clear by deleting a comment. That is the exact un-clearable advice this check is supposed to never give.
Fixed in aec7881a, by removing the lexer rather than repairing it. Two attempts at lexing this file produced two bugs of the same family, which is the signal that the approach was wrong: the check never needed to understand JavaScript. It needs one answer, is this tag commented out, and two local stateless tests give it. An HTML comment is unambiguous, so scan backwards for the nearest <!-- against the nearest -->. A JS comment is judged from the tag own line prefix, which is how commenting one out is actually written. No cross-line state, so there is nothing left to desynchronize.
The residual gap is a tag commented out mid-line after real code, which stays reported. Rare, and it fails toward reporting rather than toward the silent inertness both lexers produced.
Worth recording for whoever touches this next: the repo already ships a hardened masker for this class in packages/server/src/js-scan.js, with a differential fuzz test against a real TypeScript parse. It is not on the public export surface, which is why this check did not use it. If this scan ever needs real lexical awareness, export that rather than growing a third hand-rolled one here.
isCommentedOut already had a correct stateless test for HTML comments (the nearest opener beats the nearest closer), and I simply never wrote the symmetric one for /* */. Judging a block comment from the tag's own line prefix instead missed any multi-line block whose interior lines carry no marker, which is what an editor's toggle-block-comment produces, and that false warning was unclearable except by deleting the comment. Neither <!-- nor /* nests, so the backward test is exact for both and needs no state, which is what the two deleted lexers got wrong. A // line comment has no closer, so it stays judged from the tag's own line. This is smaller than either lexer it replaces, and it covers every shape the earlier attempts broke on: a protocol-relative url, a nested html`` in a hole, an unbalanced apostrophe, and now the un-starred block.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review: the block-comment case, and the simpler fix I had walked past
One finding on the previous commit, and it was real: an un-starred multi-line block comment produced a false warning. Judging a block comment from the tag OWN line prefix cannot see the interior lines, which carry no marker, and that is exactly what an editor toggle-block-comment writes. The author could only clear it by deleting the comment, and since --strict fails the exit on warnings it would have blocked CI with no source-level fix.
Fixed in 27d93fed, and the fix is one line. isCommentedOut already carried a correct stateless test for HTML comments, the nearest opener beating the nearest closer, and I had simply never written the symmetric one for /* */. It holds for the same reason: neither delimiter nests, so the backward test is exact and needs no state, which is precisely what the two lexers before it got wrong. A // has no closer so it stays judged from its own line.
Recording the misjudgement, because it is the useful part. After three rounds on this one predicate I argued a fourth attempt was too risky and proposed dropping the feature. That was wrong reasoning. The earlier failures all came from ADDING machinery, a line-blanking regex and then a stateful walk. This change removes a special case and makes the function symmetric, so it is the opposite kind of edit, and the result is smaller than either thing it replaced.
Re-ran every shape that broke an earlier attempt: a protocol-relative url beside a local sheet, a nested ``html```` inside a ${} hole, an unbalanced apostrophe, the un-starred block, a JSDoc-style block, and balanced CSS comments inside a `<style>`. All correct.
The JSDoc still described block comments as judged from the tag's own line prefix, which is the behaviour the previous commit replaced, and the body repeated it. Stated once, accurately, with the narrowed residual gap (a // trailing real code on the same line). Kept the do-not-lex warning and the pointer at js-scan's masker, since that is what the next person needs to not repeat the two failed attempts. Module header said page/layout, stale since the module set widened.
|
Design rationale: why this is an advisory and not the SSR rewrite the issue asked for #1095 was filed before The automatic approach was already built and rejected. #1196 matched asset urls in the assembled HTML, and two deep-review rounds found six major defects, five of them one bug: at that layer framework output and author data are indistinguishable, so the matcher kept rewriting things it did not own (a custom element reactive prop, a rendered code sample, a I did build the narrowed version first, scoped to hoisted The check against other frameworks pointed the same way. Rails resolves a logical name through a digest manifest inside So the gap |
|
Reach limit: doctor does not run in CI, so this is findable rather than unmissable Recording a real limitation of what shipped, because the next person to hit a stale stylesheet at the edge will want to know why this check did not stop it.
The consequence is concrete. This advisory only fires when somebody types the command, so it would NOT have caught the webjs.dev regression on its own. It makes the mistake findable, not unmissable.
|
|
Closing ledger: every review finding and where it landed Six review rounds ran on this PR, each a fresh reader over the diff. None of them left an open thread, because the findings were recorded in the review summaries rather than as inline comments, so there is no per-line trail to walk. That is a real loss of granularity, and this comment is the substitute: every finding, with the commit that resolved it.
Nothing was rejected and nothing was deferred, so there is no follow-up owed from the review itself. The one thing worth carrying forward is the shape of findings 5, 10, 14, 15 and 16. All five are the same sub-problem, deciding whether a matched tag is commented out, and each fix but the last introduced the next failure. Two of those attempts tried to lex the file. The version that holds does not lex at all: an unclosed |
1 participant
Closes #1095
webjs doctornow warns when a page or layout hand-writes<link rel="stylesheet" href="/public/...">withoutasset(), naming the file and line. That url is un-versioned, so a CDN keeps serving the pre-deploy bytes for the whole TTL, which is exactly the regression #1095 caught in production on webjs.dev.Why an advisory rather than the automatic rewrite the issue proposed
#1095 was filed before
asset()(#1194) existed, and asked the framework to fingerprint author-written<link>hrefs during SSR. That approach was already built and rejected. #1196 matched asset urls in the assembled HTML, and two review rounds found six major defects, five of them one bug: at that layer framework output and author data are indistinguishable, so the matcher kept rewriting things it did not own (a custom element's reactive prop, a rendered code sample, arel=preloadhint, a data-drivensrcpointing at/.env).I built the narrowed version first, scoped to hoisted
<link>tags with an allowlistedreland routed throughresolveAssetUrl. It worked, and it still re-made the choice the framework deliberately stopped making. Concretely it fingerprinted the website's favicons, which are left bare on purpose.Rails and Remix both point the other way. Rails resolves a logical name through a digest manifest in
stylesheet_link_tagand rewrites urls automatically only inside CSS, where the grammar is unambiguous. Remix takes the hashed url from the build graph via an import and surfaces it throughlinks(). Neither scans a rendered document.asset()is the same shape, and the gap it leaves is purely ergonomic: in Rails the helper is the only idiomatic way to write the tag, so forgetting it is nearly impossible, whereas here the<link>is hand-written HTML. This closes that gap where the author's meaning is unambiguous, and rewrites nothing.Scope
Flags a
<link rel="stylesheet">whose href is a static, quoted, root-absolute/public/...literal, in anyapp/**route module that renders markup:pageandlayout, the always-shippederror/not-found/forbidden/unauthorized/loadingboundaries, andglobal-error(which writes its own<head>, so it is the likeliest place outside the root layout to hand-write one). Honorswebjs.basePath, normalized exactly as the framework does. Skips_privatefolders the router never routes.It mirrors
resolveAssetUrl's own refusals, in its order (strip base path, cut at?/#, decode, then judge), so it never raises a warningasset()cannot clear. Deliberately left alone:rel="icon", a legitimate deliberate non-mark (the website's favicons stay bare so the SEO repo-health tests parse the hrefs literally)rel="preload", which must stay unversioned or its hint could never match the request a CSSurl()actually makes, including the async-CSSonload="this.rel='stylesheet'"idiomhref=${expr}hole, undecidable from source and exactly the marked shape<!-- -->,/* */,//)WARN only. An un-versioned stylesheet still serves correctly, it just caches badly.
Test plan
test/cli/doctor.test.mjs: 77/77 pass, 22 new caseswebsite166/166npm test, differentially against the fork point: the failing set is byte-identical to baseline (this worktree's symlinkednode_modulescannot loadws), so none of it is from this branch. CI is the real signal.Deciding whether a tag is commented out
Worth recording, since it took three rounds and the answer is smaller than any attempt at it. The check must not report a tag that is commented out. Two attempts tried to lex the file and both shipped bugs a stateless test cannot have:
html`...`inside a${}hole (one quote char cannot model nesting), so an unbalanced apostrophe in template text desynchronized the rest of the fileisCommentedOutdoes not lex. A delimited comment is decided by an unclosed opener behind the tag, the same backward test for<!--and/*, exact because neither nests and covering a multi-line block whose interior lines carry no marker. A//has no closer, so it is decided from the tag's own line. Five lines, no state, nothing to desynchronize.Residual gap, accepted: a tag behind a
//trailing real code on the same line stays reported. It fails toward reporting rather than toward the silent inertness both lexers produced.If this ever genuinely needs lexical awareness, export
redactStringsAndTemplatesfrom@webjsdev/server(src/js-scan.js, differentially fuzz-tested against a real TypeScript parse) rather than growing a third hand-rolled one.Docs surfaces
packages/cli/AGENTS.md, thewebjs doctorrow that enumerates the checksreferences/built-ins.md, alongside theasset()entry, so an author reading about the helper learns that forgetting it is caughtdoctor.jsmodule header, which enumerates the preconditions doctor verifiesscripts/sync-scaffold-skill.mjs, so there is no second copy to syncdoctor.jsreads files throughnode:fsonly, with no runtime-sensitive surface.