48a09586 - Referral landing follow-ups from the #30 review - #32
Merged
Conversation
Two follow-ups from the review of #30. ?lang=en without a code flipped html lang and og:locale to English while the shell text stayed German — crawlers snapshot exactly those bytes. shareTitle and shareDescription now fall back to generic English copy. Without JavaScript the loading state sat there for ever: nothing resolves the code. Both shells now hide it and say why, and a Playwright case with javaScriptEnabled:false asserts the served bytes.
The no-JS hint was German only — the same defect this PR fixes, since ?lang=en flips the document to English. It is bilingual now, and its <style> moved into a head <noscript> where the spec allows it. The English fallback said "your code" where I18N.en says "this code", so the crawler snapshot and the JS-rendered page disagreed. The kind guard in shareTitle went back to the front: reordering it made shareTitle(null, null, 'en') render as an invitation. Adds the end-to-end assertion the helper tests were missing, and corrects the Playwright comment: getByText genuinely does not reach into <noscript> in this version, which is why the assertion reads page.content().
The follow-up review found the codeless English fallback only on the
function module: public/js/lib/invite-core.js kept its own shareTitle and
shareDescription and now answered a codeless ?lang=en landing differently.
Nothing in production calls that file's HTML injectors, so no other test
would have gone red. Both helpers now match the function module byte for
byte, and a matrix test pins them against each other so the next drift
fails the suite instead of the next review.
The injectors there stay as they are: parseCodeFromLocation returns null
without a code, so their codeless branch is unreachable and forcing parity
would mean changing a parser the browser page actually runs.
Three smaller points from the same review:
- og:image:alt described the picture, so the codeless English title no
longer lands there. Without a code the image is the generic og.png the
shell already labels "RealUnit".
- The JS-less hint told the visitor to open the link in the app, but the
only control that does so is hidden without JavaScript. It now names
what is actually reachable: enable JavaScript and reload, or use the
store links below.
- The no-JS test asserted on page.content(), which a rule like
"noscript section { display: none }" would leave green while the page
stayed blank. It now asserts visibility. Measured on this Playwright
version: role and CSS locators do reach into <noscript>, getByText does
not, so the earlier comment was too broad and is corrected.
npm run check: exit 0, 161 tests, coverage thresholds met.
playwright behavior.spec.mjs --project=desktop-chromium: 100 passed,
1 skipped. Both new guards verified by mutation: hiding the noscript
section and letting the mirror drift each turn their test red.
…full text The logic lane found the notice only half guarded. The German paragraph was asserted by its heading alone, and nothing anywhere checked that the notice stays inside <noscript>: dropping that wrapper would show every visitor a warning about JavaScript being off, and the existing case runs with scripting disabled, where the wrapper's children are parsed as ordinary markup either way — so it would have stayed green. Both paragraphs are now asserted by their full text, the lang attributes are pinned, and a second case with JavaScript enabled requires the wrapper to be present and the heading to be absent. Verified by mutation: removing the <noscript> wrapper turns the new case red, and shortening the German copy turns the existing one red. npm run check: exit 0, 161 tests. playwright behavior.spec.mjs --project=desktop-chromium: 101 passed, 1 skipped.
…sent toHaveText matches textContent, which display:none leaves untouched, so the previous revision would have stayed green on a rule that hid either paragraph while the visitor saw nothing. The heading kept its toBeVisible check, so the whole-section case was still caught; a rule aimed at one paragraph was not. Both paragraphs now assert visibility before their text.
…reuse Both modules say in a comment that their codeless English copy is the same string as I18N.en, so the crawler snapshot and the JS-rendered page cannot disagree. Until now the tests pinned hand-written literals on both sides, so editing the catalogue alone — which public/invite/invite.js reads at runtime — would have split the two while the whole suite stayed green. Verified by mutation: changing only I18N.en['doc.desc'] turns the new test red and nothing else.
The conformity lane asked for this and it is the right call. Both landings now have a visible state this PR introduces — the <noscript> panel — and the visual matrix could not express it: every view renders with scripting on, so nothing would have caught the panel breaking, and CONTRIBUTING listed variants that no longer matched the matrix. Views get an optional noJs flag, visual.spec.mjs passes it into the browser context, and settle skips the font-readiness question when there is no page context to ask. Baselines are generated by the pinned container in CI, not on this host, so the committed PNGs match what the gate compares against.
Six PNGs, two views across three viewports, taken from the CI run at 78977e1 that failed with "a snapshot doesn't exist". Generating them on this host would have produced arm64 macOS text rendering and turned the gate red against the amd64 Linux container it actually compares in. The invite and promo baselines are byte-identical by design: without scripting both pages show the same panel, and holding them to the same image is what keeps the two from drifting apart.
Collaborator
Author
|
EN. Four review verdicts on this head, two vendors × two dimensions, all at DE. Vier Prüfurteile auf diesem Stand, zwei Anbieter × zwei Dimensionen, alle vollständig und ohne Befund. Dahin waren es vier Runden mit fünf Funden — allesamt an Code aus diesem PR, und jeder ist jetzt durch einen Test abgesichert, der ohne die Korrektur rot wird. Die CI ist an genau diesem Commit grün, geprüft je Check-Lauf statt über den Branch-Namen. Rounds and findings
An earlier round also caught a copy divergence the tests could not see: both modules claim in a comment to reuse Reviews ran read-only against a frozen checkout at the exact SHA, after one lane correctly reported that it had been reading a working tree while a commit landed underneath it. Every guard verified by mutationEach of these turns exactly one test red, measured at the committed SHA rather than on a working copy:
One correction worth recordingAn earlier revision of this PR reported a green local run while CI was red at the same head. The test asserted a string that had never existed in the shipped HTML: the test host carried an older wording than the commit, so the green number belonged to bytes that were never pushed. That is the same class of false claim this PR series was opened to fix. Verification now runs against a checkout of the committed SHA, and CI is read per check-run at that SHA. |
TaprootFreakAI
marked this pull request as ready for review
September 9, 2026 02:54
TaprootFreak
approved these changes
Sep 9, 2026
TaprootFreak
pushed a commit
that referenced
this pull request
Sep 9, 2026
* fix(invite): report a rewritten landing as found, not as 404 Measured against the deploy, which is what #30 left open and #32 promised to settle with one curl: curl -sI https://realunit.app/invite/AB12CD -> HTTP/2 404 The body is correct — the landing shell with the rewritten meta tags — but the status is not. _routes.json sends /invite/<code> to the Function, and the asset lookup behind context.next() resolves the code-less shell through the _redirects 200-rewrite while keeping the not-found status of the path that was asked for. The middleware copied that status through. Browsers render a 404 body, so a person following the link sees the right page. Share crawlers do not: they drop a 404 before reading the tags the middleware just wrote, which is the entire reason the rewrite exists. Every invite and promo link shared today therefore previews as nothing. The promotion is guarded on the landing marker, so a broken deploy that serves the site's own 404 page on these paths keeps saying 404 instead of looking healthy, and the reason phrase is dropped rather than left reading "Not Found". functions/_middleware.js was outside coverage while it decided the status every crawler sees, which is how this shipped unnoticed. It is measured now, at 100%, with the entry point's own test file. npm run check: exit 0, 173 tests. Verified by mutation: reverting the promotion turns three tests red, and removing the marker guard turns the two that protect a genuine 404 red. The other half of the same open question is now answered too: _headers do apply to Function-served requests. CSP, X-Frame-Options and Referrer-Policy are all present on https://realunit.app/invite/AB12CD. * docs(invite): describe the status promotion where the rules are written The conformity lane found the documentation and the config describing a narrower change than the one that landed, which is the same drift #30 had to correct in this file. - CONTRIBUTING said public/js/lib was the only code with a coverage gate and the gate table named only that path; functions/lib and the Function entry point carry one too. - The ground rule described the middleware as a pure byte rewrite with "nothing else transformed", which no longer holds now that it also decides the status a crawler sees. - The vitest comment still claimed the measured surface was side-effect-free browser logic only. - The file header of the middleware did not mention the promotion at all. Two lanes independently found the same test gap: the promoted responses asserted their status but not their reason phrase, so reverting the ternary would have stayed green at 100% branch coverage. Both promotion tests now assert the empty phrase, and the untouched-404 test asserts it keeps "Not Found". npm run check: exit 0, 173 tests, functions/_middleware.js at 100%. Verified by mutation: keeping the reason phrase on promotion turns the two promotion tests red. * test(invite): anchor the status promotion to the shipped pages The conformity lane found the middleware test building its own landing shell and its own 404 page. The promotion keys on a marker that has to be in the landings and must never be in the site's 404 page, and a synthetic fixture keeps passing after someone moves that marker — production would answer 404 again with nothing going red, which is exactly how this shipped. The test now reads public/invite/index.html, public/promo/index.html and public/404.html, and asserts the marker contract against them directly. npm run check: exit 0, 174 tests. Verified by mutation on the real files: renaming the marker in the landing turns two tests red, and adding it to the 404 page turns two others red. * fix(invite): answer HEAD like GET, and prove the headers survive The logic lane caught the fix stopping at GET. That is the method this bug was measured with: `curl -sI` sends HEAD, so the very command quoted as the proof would still have read 404 after the fix, and a link checker that sends HEAD first would still see the link as dead. HEAD has to answer with the same status as GET. HEAD is now handled alongside GET and returns no body of its own. A HEAD whose body the platform withheld keeps the status it came with: without a body there is no way to tell the landing shell from the site's own 404 page, and guessing from the path alone would make a broken deploy look healthy. The same lane found the header pass-through untested. The fixture now carries the Cache-Control rule public/_headers sets for these paths, and the promotion test asserts it survives the response being rebuilt. Also recorded in the marker comment: two other pages carry the same id. It does not matter, because the caller has already established the path is a landing, so the only open question is shell versus 404 page. npm run check: exit 0, 176 tests, functions/_middleware.js at 100%. Verified by mutation: dropping HEAD from the guard, returning a body for HEAD, and keeping content-length instead of cache-control each turn one test red. * test(invite): assert the HEAD response's headers and its absent body The logic lane found the HEAD path asserting only status, reason phrase and an empty text(). Two regressions would have stayed green: keeping content-length for HEAD alone, and returning an empty body instead of no body, which text() cannot tell apart. The HEAD case now asserts body === null and the same three headers the GET case does. npm run check: exit 0, 176 tests. Verified by mutation: an empty body for HEAD and a HEAD-only content-length each turn one test red. * docs(invite): carry the status promotion into the README too The conformity lane found README describing the middleware as byte injection only, and its testing section naming public/js/lib as the sole measured surface. CONTRIBUTING names README as the architecture reference, so leaving it behind recreates the drift the last PR had to correct. Both passages now match what the code does. * fix(invite): decide the HEAD status from a GET-equivalent lookup The logic lane found the HEAD path resting on an assumption instead of a fact. The status has to be decided from the body, because the marker in it is what tells the landing shell from the site's own 404 page — and a HEAD response has no body. The previous revision kept the status when the body was missing, so if the platform hands the Function nothing for HEAD, the same link would have read as found by GET and as dead by the HEAD a link checker sends first. That contradicted the comment right above it. HEAD now asks for the GET-equivalent through context.next() and answers with that status and those headers, without a body. Nothing is left to the platform, and the outcome no longer waits on a deploy to be known. The same lane found the fixture carrying no security headers, so a middleware that dropped one would not have failed a value assertion anywhere. The fixture now carries what public/_headers sets for these paths, and both the GET and the HEAD case assert every one of them survives. npm run check: exit 0, 176 tests, functions/_middleware.js at 100%. Verified by mutation: dropping the GET-equivalent lookup, deleting a security header, and returning the GET body to a non-HTML HEAD each turn one test red. * docs(invite): make the comments say what is actually true Four accuracy findings from the conformity lane, all of them places where a comment claimed more than it should: - The "Browser JS and unit coverage" section still read as if everything outside public/js/lib were covered by Playwright alone. Two surfaces carry their own gate and are not page scripts at all. - The test fixture called its Content-Security-Policy value "what public/_headers sets". It is a short stand-in for a much longer policy; only the cache-control value is the real one. - "The contract the two tests above rely on" sat above two tests that do not use the marker at all. - "the tags written just above" was copied out of the middleware header, where it had a referent. In the test it had none. npm run check: exit 0, 176 tests. * test(invite): take the header values from public/_headers, and cover HEAD on a 404 Two low findings from the conformity lane. The fixture hand-copied a shortened Content-Security-Policy while calling it what public/_headers sets. It now reads the real values out of that file, so the fixture cannot drift from production and the pass-through assertion is always about the shipped policy. Measured, and written into the comment so the next reader is not misled: this checks the pass-through, not the policy — changing _headers moves both sides together by design, and a middleware that drops a header is what turns red. The marker guard was only exercised on GET. There is now a HEAD case against the real 404 page, so a regression that evaluates the guard for GET alone has somewhere to fail. npm run check: exit 0, 177 tests. Verified by mutation: deleting referrer-policy in the middleware and weakening the marker guard each turn a test red. * fix(invite): strip Range from the GET-equivalent a HEAD triggers The logic lane found the synthetic GET inheriting every header of the HEAD it came from, Range and If-Range included. A HEAD carrying Range would have come back as 206 Partial Content, whose body is not the landing shell: the status would neither have been promoted nor meant what the client asked for. Both headers are now dropped and everything else the client sent is kept. The test asserts exactly that, so a regression that forwards Range, or one that throws the client's headers away altogether, has somewhere to fail. The same lane found the non-HTML HEAD branch asserting status and body but not headers, though it rebuilds the response too. It now checks them. npm run check: exit 0, 178 tests, functions/_middleware.js at 100%. Verified by mutation: keeping Range, dropping the client headers, and building the body-less response with empty headers each turn one test red. * docs(invite): stop repeating a phrase that has no referent The conformity lane caught me reintroducing, in the middleware header, the exact wording I had just corrected in the test file: "the tags written just above", where nothing is above. It now says what it means. The three places that mention the functions/lib ratchet now say where it is set, instead of calling it documented and leaving the reader to find it. npm run check: exit 0, 178 tests. * test(invite): pin the promotion to 404 and nothing near it The logic lane found that widening the condition from 404 to the whole 4xx range would have passed the entire suite. Only the 404 Pages produces for these paths is an artefact; a 403 or a 410 means what it says, even when the body happens to be the shell — promoting those would turn a refusal or a withdrawn code into a healthy page. landingStatus now pins 403, 410 and 451, and the middleware has a case that sends a refusal through untouched. npm run check: exit 0, 179 tests. Verified by mutation: widening the condition to `status >= 400 && status < 500` turns both new tests red. * fix(invite): drop Range for a plain GET too, not only for HEAD The logic lane found the previous commit cleaning the request on one path only. A real GET carrying Range would still have been forwarded with it, and this pass rewrites the whole document: a partial representation would be injected into a fragment and returned under a Content-Range describing the bytes before the rewrite. Both methods now ask for the same thing — the whole document as a GET — so the two paths share one helper and one comment instead of drifting apart again. Measured against the deploy before changing anything: curl -sI -H 'Range: bytes=0-99' https://realunit.app/invite/AB12CD -> 404, and the body is the full 4162 bytes, not a fragment Pages answers a Range request on these paths with the whole document today, so this is a guard rather than a live fix, and the comment says so. npm run check: exit 0, 180 tests, functions/_middleware.js at 100%. Verified by mutation: cleaning only the HEAD path, and keeping Range at all, each turn tests red. * fix(invite): drop the headers that described the bytes before the rewrite The logic lane found ETag and Last-Modified passed through unchanged. They describe the document the asset server produced, not the one this pass sends, and a stale validator is worse than none: a conditional request would be answered 304 against a document the client never received. Content-Range gets the same treatment for the same reason. Measured against the deploy before changing anything: Pages sets neither ETag nor Last-Modified on these paths today, so this is a guard rather than a live fix, and the comment says so. The refusal case now also asserts the body passes through, not just the status. The conformity lane found the Range behaviour missing from both architecture documents; CONTRIBUTING and README now say that GET and HEAD are resolved internally as one full GET. The measurement note in the code also says what was actually measured — the body of a Range GET, not only its headers, which is what the previous commit message implied. npm run check: exit 0, 181 tests, functions/_middleware.js at 100%. Verified by mutation: dropping only content-length, and building the header set empty, each turn tests red. * fix(invite): make the GET-equivalent unconditional, and never pair a body with 304 The logic lane found the helper promising more than it delivered. It ruled out 206 by dropping Range, but forwarded the conditional headers untouched, so the answer could still come back as 304 or 412 — bodies that are not the landing shell either, on a path where the status is decided from the body. Worse, a 304 would have crashed the request: the previous revision paired the rewritten string with that status, and constructing a Response with a body and a null-body status throws. A conditional request would have become a 500 instead of being passed on. Both are closed. Every header that could make the answer something other than the full document is dropped, and 204, 205 and 304 are answered without a body. The same lane found that re-methoding a request that carries a body throws, which would have aborted the middleware rather than answering. That case now falls back to rebuilding from the URL, which loses the platform's request metadata but keeps the request answerable, and a test drives it. npm run check: exit 0, 184 tests, functions/_middleware.js at 100%. Verified by mutation: narrowing the header list back to Range, treating only HEAD as body-less, and removing the fallback each turn a test red. * fix(invite): drop the stale Content-Encoding, and stop saying "everything" The logic lane left this as an open question rather than a claim, and the measurement settled it: curl -sSI --compressed https://realunit.app/invite/AB12CD -> content-encoding: gzip These paths really are served gzipped. The rewrite always hands on a decoded string, so a Content-Encoding inherited from the asset response would label it as something it is not, and a body labelled gzip that is not gzip does not render at all. Whether the header is on the object the Function sees or only added by the edge afterwards is not knowable from here — dropping it costs nothing in the second case and prevents a blank page in the first. The conformity lane found the comment above that list, and the test title, saying "everything that described the bytes" while the code covers a named set. That is the same overclaim this series has now corrected four times. Both name the five headers instead, and the helper's comment names the six request headers RFC 9110 defines rather than claiming to cover every possible one. npm run check: exit 0, 184 tests, functions/_middleware.js at 100%. Verified by mutation: keeping Content-Encoding turns the test red. * docs(invite): stop crediting the dev-server with the middleware's behaviour The conformity lane found the README attributing the status promotion, the HEAD parity and the unconditional full GET to both injection paths, because the sentence followed a bracket naming the Function and the dev-server together. scripts/dev-server.mjs has none of it. While correcting that I checked the file rather than trusting the finding, and the first correction I wrote was wrong too: the dev-server does answer 404, on its own not-found path. What is true is narrower — it shares the injection and answers HEAD without a body, but has no promotion, no marker guard and no header stripping, because its routing serves the landings as 200 and never produces the not-found status the promotion exists to correct. Both documents now say that, and both name the conditional validators the helper strips alongside Range. The logic lane added two small ones, both taken: - The integrity digests of RFC 9530 and its predecessors describe the bytes this pass replaces, so they join the stale list. Not observed on this deploy; the same guard as the validators. - The body-carrying-GET fallback asserted the method and URL but not the headers, so rebuilding it with the client's untouched headers would have stayed green. It now checks that the cleaned set is what gets forwarded. npm run check: exit 0, 184 tests. Verified by mutation: dropping a digest from the list, and forwarding the raw headers in the fallback, each turn a test red. * docs(invite): say what the rewrite invalidates, not what the headers are Two precision findings from the conformity lane, both the same reflex this series keeps catching: a comment claiming a little more than the code holds. - The stale-header comment called ETag and Last-Modified byte descriptions. They are validators of the selected representation, and a weak ETag explicitly does not promise byte equality. The reason they go is that rewriting the representation invalidates them, which is what it now says. - A test was titled "keeps the request headers but drops Range" while the helper drops four conditional headers as well; its comment said everything else is kept. The title and comment now describe that case, and point at the case that covers the conditional ones. npm run check: exit 0, 184 tests. * fix(invite): declare UTF-8, refuse to rewrite a partial answer, clean the fallback Four findings from the logic lane, after it downgraded two of its own and rejected two more as unsupported. - response.text() decodes as UTF-8 and a string body is encoded as UTF-8, so an origin charset carried through would have been a lie. The rewritten answer now declares text/html; charset=utf-8 itself. - A 206 cannot be rewritten: the body is a fragment and the range metadata would stop matching it. Range is stripped before the lookup, so this only fires for an origin that answers 206 unasked, but the guard is one line. - The body-carrying-GET fallback kept the headers that described that body, so the rebuilt request announced a body it does not carry. - The non-HTML branch deliberately drops only content-length, because nothing was rewritten there and the validators still hold. No test showed that, so a regression widening it would have passed. It is asserted now. The comment on the non-HTML branch also says what it previously left out: the request has already lost Range and the conditional headers by then, which is harmless only because these paths serve HTML alone. While verifying, one of my own mutation checks came back green and I chased it rather than accepting it. The check had silently not applied — the script's output was piped away, so "pattern not found" was invisible, and the two assertions it was meant to test had landed in the wrong test because the same line occurs twice in the file. Both are corrected, and the check now fails as it should. npm run check: exit 0, 186 tests, functions/_middleware.js at 100%. Verified by mutation: dropping the charset, removing the partial-answer guard, keeping the body headers in the fallback, and widening the non-HTML strip each turn a test red. * docs(invite): separate the two claims about Content-Encoding, and ask instead of catch Three findings from the conformity lane. The comment on the stale-header list argued in one breath that dropping Content-Encoding is "not hypothetical" because the deploy serves gzip, and that it "costs nothing if it was never on the object". Those are two different layers, and put together they contradict each other. Split: the encoding goes because response.text() decoded the body, whatever the origin declared; whether the header was ever on the object is a separate question, and the gzip seen on the deploy is applied by the edge after this Function runs. Only the ETag and Last-Modified sentence rests on a measurement. A test titled "passed through untouched" checked status and body but not headers, while its two siblings with the same word do. It checks them now. The fallback used a bare catch, so any constructor failure would have been rerouted through it while the comment named one cause. It asks whether the request has a body instead, and anything else still surfaces. npm run check: exit 0, 186 tests, functions/_middleware.js at 100%. Verified by mutation: mangling a header on the non-HTML branch, and taking the re-method path unconditionally, each turn a test red. * fix(invite): let a body-less answer keep the length it correctly had The conformity lane pointed out that the helper's own name for what it drops was wrong. On the paths that reach it nothing was rewritten, so the origin's Content-Length describes exactly the representation a GET would have returned — RFC 9110 wants a HEAD to carry it. Calling it stale and deleting it lost correct information. It is kept now, and the tests assert it on both branches that use the helper. The asFullGet docstring also claimed everything else the client sent is kept, while the fallback right below it drops the body, the headers that described it and the request metadata a URL cannot rebuild. It names that exception. npm run check: exit 0, 186 tests, functions/_middleware.js at 100%. Verified by mutation: deleting the length again turns two tests red. * fix(invite): hand on what this pass has no business rewriting Three findings from the logic lane. A 304 was going through the rewrite branch, where its ETag was stripped and a Content-Type set. That is the one validator a 304 has to keep — it is what the client was matched on. 204, 205, 304 and 206 are now one set of statuses handed on as they came: none of them carries a representation this pass could rewrite, and their headers still describe what the origin meant. The special case that kept a body away from them disappears with it. The rewrite is also gated on the body being a landing shell. On a broken deploy serving the site's own 404 page under these paths, the status was already left alone, but the page's title was still rewritten into an invitation — a description of something the visitor is not looking at. The stale-header strip runs for every rewritten answer, not only a promoted one, but only the promotion test proved it. Coupling the strip to the status would have left a 403 or 410 carrying a length from before the rewrite, and no test would have noticed. The refusal case asserts it now. npm run check: exit 0, 186 tests, functions/_middleware.js at 100%. Verified by mutation: coupling the strip to 404, shrinking the pass-through set back to 206, and removing the shell guard each turn a test red. * fix(invite): read the media type as a media type, and leave other encodings alone Two production findings from the logic lane, and two test gaps. The content type was matched with includes('text/html'). That is case-sensitive, so an origin answering Text/HTML would have been treated as something else and never rewritten; and it is a substring test, so application/json; profile="text/html" would have been read as HTML and had an invitation injected into it. It is parsed as a media type now. The charset was ignored. response.text() decodes as UTF-8 whatever the header says, so a body in another encoding would have come back as replacement characters and left here labelled UTF-8. Such an answer is handed on untouched instead. The old test asserted the opposite — that an ISO-8859-1 answer gets relabelled — and it went red, correctly; it now covers what the rewrite really guarantees, which is that a rewritten answer says UTF-8 even when the origin left it out. The two test gaps: the partial-answer case asserted only status and body, and the non-HTML pass-through named a few headers by hand. The first now uses the landing shell as its fixture, so the guard has something to protect and a mutation that lets HEAD through goes red; the second compares the whole header set, so a regression that drops one nobody thought to name is caught. npm run check: exit 0, 188 tests, functions/_middleware.js at 100%. Verified by mutation: the substring match, the missing charset guard, letting HEAD past the partial guard, and dropping a header in the body-less branch each turn a test red. * fix(invite): hand an untouched answer on as it came, not rebuilt from its text Seven findings across both conformity and logic lanes. The branch that decides not to rewrite was rebuilding the response from the decoded string while keeping the origin's headers — the same mismatch the rewrite branch strips headers to avoid, just without the stripping. It reads from a clone now, so an answer this pass declines to touch is handed on exactly as it came, body and headers both. Accept-Ranges joins the stale list: after a rewrite there are no ranges of the original bytes left to offer. The docblock describing the status promotion had ended up above isLandingShell when that predicate was extracted. It sits above landingStatus, which is what it describes. Four test gaps, three of them the same shape — asserting that a header is absent afterwards while the fixture never set it, which proves nothing: - the refusal case now sends a real Content-Encoding before checking it is gone; - the stale-header case now sends a real Accept-Ranges; - the media-type lookalike now carries the landing shell as its body, so a substring match would actually rewrite it and be caught; - the two remaining "passed through untouched" cases assert the body, as their siblings already did. Charset parsing is exercised across the shapes it arrives in: quoted, unspaced, differently cased, and two encodings that must be handed on instead. npm run check: exit 0, 189 tests, functions/_middleware.js at 100%. Verified by mutation: reading without the clone, keeping Accept-Ranges, stripping only on a promoted status, and matching the media type as a substring each turn a test red. * fix(invite): read the Content-Type parameters with quotes respected The logic lane found the charset regex not quote-aware. A quoted parameter value may contain a semicolon, so `text/html; foo="x;charset=utf-8"; charset=iso-8859-1` matched the charset embedded in the quoted value and missed the real one behind it — the answer would have been decoded as UTF-8, rewritten and relabelled, which is exactly what the charset check exists to prevent. The parameters are split with quotes respected now, and every charset present has to be UTF-8: two that disagree mean hands off rather than picking one. The 206 fixture described a range that could not exist — `bytes 0-9826/4162`, because .length counts UTF-16 units while a Content-Length counts bytes, and the total had been left over from an earlier fixture. It uses real byte counts. npm run check: exit 0, 189 tests, functions/_middleware.js at 100%. Verified by mutation: dropping quote-awareness, taking the first charset instead of requiring all, and leaving the quotes on the value each turn a test red. * test(invite): make the untouched-404 cases tell the fix from the bug The logic lane found both of them unable to distinguish the current code from the one it replaced. Their fixture never set an encoding or a validator, and the page's own bytes are the same either way, so rebuilding the answer from its decoded text instead of handing on the origin's would have passed. Both now receive an answer carrying Content-Encoding and an ETag, and require the very object the origin returned — a rebuilt look-alike fails. The HEAD case compares the whole header set, since only the body may be missing. Two smaller ones from the conformity lane: a test titled for the answer being the full document checked only the forwarded request, and now checks both; and the two documents called the four request headers "conditional validators", which is the RFC's name for ETag and Last-Modified, not for these. npm run check: exit 0, 189 tests. Verified by mutation: rebuilding the untouched answer from its text turns both cases red. * test(invite): prove the foreign encoding survives, with real bytes The conformity lane found the test claiming more than it showed. Its body was the same JS string every other fixture uses, and a string body is always encoded as UTF-8 — so it only ever proved that the guard flips on a foreign charset label, never that a body in another encoding comes through intact. It now sends real ISO-8859-1 bytes, with 0xE9 for 'é', which is not valid UTF-8 at all, and carries the landing marker so that nothing but the charset guard stands between it and the rewrite. The answer is compared byte for byte. Verified by mutation: removing the charset guard turns it red, because the byte comes back as U+FFFD and the answer leaves as different bytes. npm run check: exit 0, 189 tests. * fix(invite): honour a quoted-pair when splitting the Content-Type The logic lane found the parser toggling on every quote, escaped or not. RFC 9110 lets a backslash escape the next character inside a quoted value, so `foo="a\";charset=utf-8"; charset=iso-8859-1` was split in the wrong place and the wrong charset won. The escape is consumed now, and a quoted value has its escapes resolved rather than only its outer quotes stripped. Two test gaps from the same lane: - The disagreeing-charset case was only tried one way round, so a change from "all must be UTF-8" to "the last one wins" would have passed. Both orders are covered. - Two pass-through branches asserted their headers but not that the answer is the origin's own object, while the 404 branch beside them does. A rebuild at either would have gone unnoticed; both now require the same object and carry an encoding and a validator to prove it. npm run check: exit 0, 189 tests, functions/_middleware.js at 100%. Verified by mutation: dropping the escape handling, letting the last charset win, and rebuilding the non-HTML answer each turn a test red. * test(invite): count bytes as bytes, pin og:url, and finish the identity checks Six findings from the conformity lane, all of them places where a comment or an assertion was looser than what it stood next to. - Two comments called a HEAD answer "handed on untouched" or "the same object". A body-less answer has to be a new object; only the GET path can be the origin's own. Both now say which is which. - The fixture builder set Content-Length from .length while a comment three lines above explained that .length counts UTF-16 units and these pages carry multi-byte characters. Every fixture and assertion uses real byte counts now. - An assertion checked for a URL string that og:url and twitter:url share, so a defect in the og:url replacement alone would have passed. It pins the whole tag. - "The German shell must be left exactly as it is" was not true: the pass adds og:site_name regardless of language. The claim is narrowed to the copy, which is what the test checks. - The last of the four pass-through branches still lacked the identity check its siblings got, and has it now. npm run check: exit 0, 189 tests. Verified by mutation: breaking the og:url replacement alone, and rebuilding the answer on the non-GET/HEAD branch, each turn a test red. * test(invite): pin the parameter name's case and the marker's exact shape Two mutation gaps from the logic lane, which also found the quoted-pair bug independently — that one was already fixed in 9ef5048. - Every charset test used a lower-case parameter name, so dropping the case-folding would have passed while `Charset=` stopped being recognised. Both an upper-case UTF-8 and an upper-case foreign charset are covered. - The landing marker is matched as a substring, so widening it from `id="state-loading"` to `state-loading` would have passed too — and a page that merely mentions the words would then have been promoted from 404 to 200 and rewritten. A page that mentions them without the attribute is now a case. npm run check: exit 0, 190 tests. Verified by mutation: removing the case-folding and widening the marker each turn a test red. * test(invite): require the origin's own answer on every pass-through branch Two test-strength findings from the decisive round; both lanes reported nothing that changes behaviour for a user or a crawler. The already-found case asserted its status but not that the rewrite ran, so a change that handed a 200 answer straight through would have passed. My first attempt at fixing that was itself trivially true — it checked for a tag the shipped shell already carries, and the mutation stayed green. Measured what the rewrite actually changes on that URL: it normalises the canonical and the two URL tags to the form without a trailing slash. That is what the test checks now. The identity check that three pass-through branches carry was missing from the other two, so a rebuild at either would have gone unnoticed. All five require the origin's own object on the GET path. npm run check: exit 0, 190 tests. Verified by mutation: skipping the rewrite on an already-found answer, and rebuilding the body-less pass-through, each turn tests red. * docs(invite): state the precondition deviation instead of leaving it implicit The decisive round came back with nothing behaviour-affecting from three of the four lanes. The fourth raised two, and this is how they stand. The first is real and is now written down where it happens rather than left for the next reader to discover: stripping the conditional request headers means a client sending `If-None-Match: *` gets 200 with the whole document where RFC 9110 §13.1 calls for 304, and `If-Match` never yields 412. These paths emit neither ETag nor Last-Modified, so a client only reaches that case by sending a precondition it was never given one for, and the answer it gets is the current representation — never wrong, only larger than it had to be. Evaluating preconditions properly means minting a validator for the rewritten document, which is a larger change than this one and belongs in its own. The second is not this PR's: shouldRewriteItunesBanner and parseLandingFromUrl carry two different lists of asset extensions, so a referral code ending in .json is skipped entirely while one ending in .html loses its code. Both functions predate this change. It is written into the PR body as a follow-up. Three test-strength points from the same round are closed: an unterminated quoted charset, the reason phrase on a passed-on HEAD, and the boundary of the marker check spelled out as a documented limit rather than left unstated. npm run check: exit 0, 190 tests. Verified by mutation: dropping the closing-quote check and blanking the reason phrase each turn a test red. * test(invite): the last pass-through branch, and the third URL tag Two test-strength points from the confirmation round; it reported nothing that changes behaviour. The branch for a path the rewrite does not own was the one pass-through left without an identity check, so a rebuild there would have gone unnoticed while the body and length stayed the same. It requires the origin's own object now, like the other five. The proof that an already-found landing really is rewritten checked the canonical and og:url but not twitter:url, though the rewrite normalises all three. A regression in that one replacement alone would have passed. npm run check: exit 0, 190 tests. Verified by mutation: rebuilding the answer on the not-owned branch, and breaking the twitter:url replacement alone, each turn a test red. * docs(invite): put the RFC deviation where contributors read, not only in the code Three test-strength and documentation points from the confirmation round, which again reported nothing that changes behaviour. The Range case asserted the forwarded request but discarded the answer, so a regression that stripped the header and still treated the response as partial would have passed. It asserts the answer too. The charset matrix collapsed every case to a status. It now checks what each outcome means: a rewritten answer is promoted, declares UTF-8 and carries the injected title; an untouched one keeps the origin's status, type and bytes. The deliberate deviation from RFC 9110 §13.1 was stated in the code but not in either document, though both describe the surrounding behaviour. Contributors read those first, so both now say it and why. npm run check: exit 0, 190 tests. * docs(invite): separate the two kinds of precondition in the reasoning The confirmation round accepted the deviation but caught the justification being too broad. A tag or date condition would indeed need a validator minted for the rewritten document. A wildcard one would not: `If-Match: *` and `If-None-Match: *` ask only whether a current representation exists, which is already known here. They are dropped for uniformity, not out of necessity, and the comment now says which is which. The behaviour is unchanged; only the stated reason was wrong for half of it. npm run check: exit 0, 190 tests. * test(invite): pin the two hand-off URLs and the path boundary The last two mutation gaps from the confirmation round. The Android and iOS alternate links were asserted by attribute name only, so swapping the two would have passed while Android users were sent to the iOS hand-off and back. Both hrefs are pinned now. shouldRewriteItunesBanner matches its prefixes at a segment boundary, but no test used a path that merely starts with the same letters, so dropping the slash would have gone unnoticed and pulled /invitee and /promotion into the rewrite. Four negative cases cover it. npm run check: exit 0, 190 tests. Verified by mutation: swapping the two hand-off URLs, and widening the prefix match, each turn a test red. * fix(invite): recognise a landing shell by two marks, not one The release round raised four points, and this closes all of them. The one that touches behaviour: the shell was recognised by a single substring, so an error document carrying `id="state-loading"` anywhere — a comment, a script, a copied snippet — would have been promoted to 200 and dressed in an invitation's metadata. The shipped 404 page does not carry it, and a contract test pinned that, but nothing covered an error document the site does not ship, such as an edge error or a challenge page. Recognition now needs both that id and `aria-busy="true"`, which the two landings carry and nothing else arrives at by accident. The case that used to be a documented boundary is now simply handled: such a page keeps its 404 and its own body. Three coverage gaps for behaviour that was already correct: - `/promo` exactly was never tested, so deleting that clause would have stopped the promo landing being rewritten without a red test. - The replace branch of the alternate-link upsert had never run: no fixture carried an existing link. Both attribute orders are covered, and the old href has to be gone rather than joined by a second tag. - The iPad Twitter app card was emitted and asserted nowhere. npm run check: exit 0, 191 tests. Verified by mutation: dropping the second mark, removing the exact `/promo` clause, giving the iPad card a wrong URL, and making the upsert always append each turn a test red. * docs(invite): say two marks everywhere the code now needs two The round on the previous commit found only its own after-effects, and this clears them. Both documents still described the guard as keying on one mark. That is the drift this series keeps correcting, one commit after introducing it: they name both now. The same singular sat in the middleware comment, in the test file's header and in two test titles. Two claims of mine were wrong rather than merely stale: - The commit that added the replace-branch test said it covered both attribute orders. Both fixtures put the attribute before the href, so the branch for the other order had still never run. A third fixture covers it, and disabling that branch now turns the test red. - The Latin-1 test said its body carried the landing marker so that only the charset guard stood between it and the rewrite. With two marks required and only one present, that was no longer true — the marks check would have blocked it as well. The body carries both now, so the test isolates what it claims to. npm run check: exit 0, 191 tests. * docs(invite): the body is never stale; it is the status that diverges "Never wrong, only larger than it had to be" claimed too much. The body a client gets is always the current representation, so that half holds. The status is the part that deviates: RFC 9110 puts 304 and 412 at SHOULD, and this pass answers 200 instead. The comment now separates the two. npm run check: exit 0, 191 tests. * test(invite): the marks are attributes, not words The last open point from the logic lane. Nothing in the suite carried both words without the attribute syntax, so loosening the marks from `id="state-loading"` / `aria-busy="true"` to bare `state-loading` / `aria-busy` would have passed — and any page mentioning both in prose would then have been promoted to 200 and dressed as an invitation. A page that does exactly that is now a case, and it keeps its 404 and its body. npm run check: exit 0, 191 tests. Verified by mutation: loosening the marks to bare words turns it red. * docs(invite): a comment that its own next line disproves The last point from the round on 880311e. The comment still called the attribute-in-a-comment case a boundary the check cannot see, while the assertion three lines below asserts a 404 because the second mark closes it. It says what is true now, and what it used to be. npm run check: exit 0, 191 tests. * docs(invite): say what the marker check does, and pin what makes it enough The marker check is a substring test. Four comments and two doc paragraphs claimed more than that, or more than the code around them holds: - two comments called a pass-through "passed on as they are" and "handed on exactly as it came", which is true of a GET and not of a HEAD, whose answer is rebuilt without a body three lines below; - the landingStatus JSDoc used "rewrite" for both the tag injection that has already happened and the status promotion it performs itself; - the isLandingShell JSDoc said two marks together are "not something another page arrives at by accident", which the check does not establish: it never asks whether the marks share an element; - CONTRIBUTING and README said these paths emit no validator, which holds for a rewritten landing and not for a pass-through, where the origin's ETag is kept and a test already pins it. What actually makes two substrings enough is a property of the shipped site, so it is now asserted instead of described: of every HTML page under public/, only the two the Function is routed to read as a landing shell. The account-merge and Aktionariat shells carry the id and no aria-busy, the 404 page carries neither. The test goes red the day one of them gains the second mark, which is the day the guard would have to become a real parse. * docs(invite): correct three claims, and pin the inventory the new test rests on Three statements from the previous commit did not hold, and one measurement was missing: - CONTRIBUTING said the marks are carried by the landings and by "no page served under these paths", which contradicts itself: the landings are the pages served under these paths. It now says no other page the site ships. - The isLandingShell JSDoc credited the function with establishing that no other shipped page reaches both marks. The function performs a substring test and establishes nothing about other files; the property is held by the test, which the docstring now says. - The path gate excludes twelve asset-like suffixes, and a review read that as leaving /invite/AB.JSON without the promotion. Measured on the deploy instead of argued: such a path is answered with the site's own 404 page, not with the landing shell, so the promotion would refuse it on the marks in any case. What a code like that loses is the landing itself, which is the follow-up #30 left open. The gate now says so. The repo-wide test pinned only three of the six shipped pages by name, so a walker that quietly stopped finding the others would still have passed: the filtered list would simply have had nothing left to disagree with. It now pins the whole inventory first, and the doubled-slash and /invites path boundaries are asserted rather than reasoned about. --------- Co-authored-by: TaprootFreakAI <315477232+TaprootFreakAI@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EN. Follow-up to #30, which was merged after a full double review. The five defects that could credit a referral to the wrong person were fixed there; this PR carries the two remaining user-visible ones — an English locale served with German copy, and a landing page that spins for ever without JavaScript — plus a third the review of this PR uncovered: the browser-side copy of the share helpers had drifted from the one production serves. Everything else the review found is listed below with its evidence, unfixed and deliberately so.
DE. Nachtrag zu #30, der nach vollständigem Doppelreview gemergt wurde. Die fünf Fehler, die eine Empfehlung dem Falschen hätten gutschreiben können, sind dort behoben; dieser PR bringt die zwei verbliebenen sichtbaren Mängel — englische Seite mit deutschem Text und eine Landingpage, die ohne JavaScript ewig lädt — plus einen dritten, den erst das Review dieses PR fand: die Browser-Kopie der Share-Helfer war von der produktiv ausgelieferten abgewichen. Alles Übrige aus dem Review steht unten mit Beleg, bewusst unbehoben.
What this PR changes
?lang=enwithout a code served German copy under an English locale.injectShareLocaleHtmlflipshtml langandog:localewheneverlang=en, butshareTitleandshareDescriptionreturnednullwithout a code, so the shell's German<title>and description survived. Crawlers snapshot exactly those bytes. Both now fall back to generic English copy, worded identically toI18N.enso the crawler snapshot and the JS-rendered page do not disagree. German is unchanged, because German is the shell's language and there is nothing to replace.og:image:altdeliberately does not take the codeless title: alt text describes the picture, and without a code the picture is the genericog.pngthe shell already labelsRealUnit.Without JavaScript both landings sat in the loading state for ever.
Nothing resolves the code, and there was no
<noscript>. Both shells now hide#state-loadingfrom a<noscript>in the head —<style>is metadata content and is only conformant there — and explain the situation in both languages, since?lang=enmay already have flipped the document. The copy names only what is actually reachable without JavaScript: enable it and reload, or use the store links below. An earlier draft told the visitor to open the link in the app, but the control that does so (#ok-open) ishiddenin that state.The browser mirror had drifted from the module production serves.
public/js/lib/invite-core.jscarries a second copy ofshareTitleandshareDescription. Nothing in production calls that file's HTML injectors, so the divergence would not have failed a single test. Both helpers now matchfunctions/lib/itunes-banner.jsbyte for byte, and a matrix test over kind × code × lang pins them against each other, so the next drift fails the suite instead of the next review. The injectors there are left alone on purpose:parseCodeFromLocationreturnsnullwithout a code, so their codeless branch is unreachable and forcing parity would mean changing a parser the browser page actually runs. Both modules also state in a comment that their English fallback is the same string asI18N.en, whichpublic/invite/invite.jsreads at runtime; that claim is now pinned by a test too, so editing the catalogue alone cannot split the crawler snapshot from the rendered page.The JS-less panel is now a view in the screenshot matrix.
Every view rendered with scripting on, so the matrix could not express the state this PR introduces and nothing would have caught the panel breaking. Views take an optional
noJsflag,tests/visual.spec.mjspasses it into the browser context, andsettleskips the font-readiness question when there is no page context to ask. The six baselines were rendered by the pinned container in CI, not on the arm64 host that would have produced different text metrics. The invite and promo baselines are byte-identical by design: without scripting both pages show the same panel, and holding them to the same image is what keeps the two from drifting.Verification
npm run check(format, HTML validation,check:site, unit tests + coverage): exit 0, 162 tests, thresholds met.npx playwright test behavior.spec.mjs --project=desktop-chromium: 101 passed, 1 skipped.QualityandScreenshotsboth green, includingcheck:visual, verified per check-run against the head SHA rather than against a branch name.noscript section { display: none }; hiding only the English paragraph; removing the<noscript>wrapper; shortening the German copy; letting either share helper drift from the function module; editingI18N.en['doc.desc']without touching the fallback literals.javaScriptEnabled: false: role and CSS locators do reach into<noscript>,getByTextdoes not — its text engine skips that subtree. An earlier revision of this PR asserted onpage.content(), which a display rule would have left green while the page stayed blank.Found and deliberately not fixed — needs a decision or a deploy to settle
Cannot be settled from this repo — one
curlafter deploy will:_redirectsand_headersapply to requests a Pages Function serves. If they do not,/invite/AB12CD404s and the CSP is absent on exactly the pages that callfetch. Evidence against:_middleware.jscallscontext.next()rather than building its own response, and copies the response headers; production demonstrably serves CSP andX-Frame-Optionstoday. There is no preview deployment for the PR, so this stayed unverified.curl -sI https://realunit.app/invite/AB12CDafter the next deploy answers it.public/_redirects:7-13uses absolute URLs (https://www.realunit.app/…) as redirect sources. Cloudflare Pages expects paths there;scripts/check-site.mjs:617-641actively requires these lines, so the check pins a configuration that may never fire. Affects thewwwAASA/assetlinks guarantee inREADME.md:35-36.Dead weight worth a scope decision:
public/js/lib/invite-core.jshave no production caller at all; onlyapplyItunesBannerFromLocationdoes. They are a second implementation kept alive by tests, and they are the reason 01a03535 - Add invite and promo landing pages #30's coverage badge measured the wrong file. Deleting them is the cleaner answer than mirroring them forever, but it is a scope call for the owner, not a follow-up fix.Gates that pass on shape rather than behaviour:
scripts/check-site.mjs:351-395asserts that strings appear in a file. A middleware that imports the injector but never calls it, or an injector returning the input unchanged, passes. It did not catch anything in this feature.scripts/check-visual.mjs:77-91compares aggregate counts. A test without atoHaveScreenshot()staysexpectedand the total still matches. Pre-existing, but 01a03535 - Add invite and promo landing pages #30 grew the matrix by 115 baselines.functions/lib/itunes-banner.js) and asserted nowhere;check:sitechecks onlyiphoneandgoogleplay, thoughCONTRIBUTING.md:21-22requirestwitter:app:url:ipad. They could be deleted and all four gates would stay green.Behaviour worth a product decision:
capCodeis not idempotent, and the path and query forms decode a different number of times:/invite/%2541yields%41while?code=%2541yieldsA. The same transport-encoded value therefore resolves to two different codes depending on how the link is written.realunit-wallet://…with no store fallback, so iOS without the app installed is a dead end.scripts/dev-server.mjs:97-107hardcodes the invite/promo rewrite, so the whole Playwright suite exercises a different routing mechanism than production.Test coverage still missing (each verified absent): duplicate
?code=A&code=Bin both orders; the idempotency propertynormalize(normalize(x)) === normalize(x); host and scheme variants (realunit.app.attacker.example,https://realunit.app@attacker.example,//attacker.example,javascript:,data:, punycode); code separators other than the slash; boundary lengths 31/32/33 and 10 KB; the app-not-installed hand-off.