Review nits from #80 and #81: error page links and copy, Appearance return, browser tests open
On branch chore/review-nits
Branch chore/review-nits, cut from origin/main 83c5d16. Two commits: c5ed5ea, d595a02.
What changed, and how each one is tested
- The 500 page for a GET kept "Go to Oficina". Before, "Try again" (which points at the URL that just failed) replaced it. Now "Try again" sits beside it, in estilo-error's second link slot (
ErrorPage.Back). Test:TestServerErrorKeepsTheWayHome. Before the fix it failed withno Go to Oficina link to /: <p rst-error-cta><a rst-btn="primary" href="/help/open-source?x=1">Try again</a>(the id.* case failed the same way). - Appearance
return_tois now the page's path only for a GET that drew a 200, and/otherwise.pageForandaccountnow take the status. Test:TestAppearanceReturnsOnlyToARealPage. Before the fix it failed withreturn_to "/no/such/page", want "/"andreturn_to "/appearance", want "/", and the POST-drawn 200, the GET 503 and the GET 500 all failed withreturn_to "/help/open-source". - Hostile
return_tocases added toTestAppearanceSetsTheTheme://evil,/%09/evil,/%5c/evil,/%5C/evil,https://evil,https:%2F%2Feviland/%0d%0a/evil, each posted as a raw form body, must 303 to/. These already passed. To show the test can catch a regression, I temporarily replaced the validator estilo actually calls with a bare/-prefix check. That validator issessions.SafeReturnfromgithub.com/carlosframework/rastrillo, the old path, because estilo's chrome.go still imports it. Five cases then failed, for examplereturn_to=//evil: 303 to "//evil", want 303 to /. The replace was reverted afterwards. - Error copy for every status. 400, 401, 403, 405, 413, 429 and 503 now have their own words, taken from rastrillo's
locales/en.tomlwhere it has some. Any other status gets a general "Something's not right" page. Test:TestErrorPageHasCopyForEveryStatus, which checks that each status has a heading and body, that headings are distinct, and that 418, 409 and 502 get the fallback. Before the fix it failed withNNN: no headingfor every status except 404 and 500. - assertion.html comment. On main, §9.3 now appears only once. But the comment still cited it for both the hidden card and the bar/sky placeholder, and one line ran long. §9.3 of
2026-09-23-shared-chrome.mdis the hidden-card rule only. The placeholder comes from layout.html's hop handling (r3 C2), and the comment now says so. This is a comment-only change, so there is no test. - Browser tests on a Mac. rastrillo's harness only knows Playwright's Linux layout. A new finder,
playwrightChrome, also knowschrome-mac*/Google Chrome for Testing.app/...and the olderChromium.app, and picks the newest revision. The browser tests setRASTRILLO_CHROMEfrom it when the variable is unset and no Chromium is on PATH.make test-browsernow runs the whole package with-tags browserinstead of-run 'OnTap'. Test:TestPlaywrightChromeFindsEveryCacheLayout, in an untagged file so the plain gate runs it. With only the Linux layout (the harness's behaviour) it failedgot ""for all three macOS cases. Before this change,make test-browserfailed withno chromium found. It now passes, andTestNoZoomOnTapreally drives Chromium (about 21s).
The team-home lede and the footer "Source"/"Home" link are untouched, as asked.
Gate
make test passes, except that test-race stops on internal/provisioner on macOS ("build constraints exclude all Go files"; the package is //go:build linux). That happens on main too. The rest of test-race was run by hand and passed. make test-browser passes.
Review: not yet reviewed. A Fable whole-branch review is required before merge (AGENTS.md).
LLM
Fixes the six non-blocking findings from Fable's reviews of #80/#81. Every fix with a regression test was shown failing first. Needs a Fable review before merge.