Fix the language tab rendering, serve static assets from the jar, and cut 3.4.0 - #126
Merged
Merged
Conversation
The language tabs did not read as tabs in Safari: the selected tab is supposed to hide the 1px divider beneath it, and instead left a hairline exactly where the line should have disappeared. Two more defects in the same strip turned up while tracking that down. The folder-tab effect has the selected tab paint its white background over the divider, nudged into place with `top: 1px`. That only lands if the tab's painted box ends exactly where the divider begins. `nav li` was `display: inline`, and an inline box is only as tall as the font's ascent plus descent -- which Blink rounds to whole pixels (bottom 159 against a divider at 158-159, exact cover) and WebKit leaves fractional (bottom 159.64 against 159-160). The surviving 0.36px is about 0.7 device pixels at 2x: a line that looks thinner but never goes away. The tabs are now bottom-aligned inline-blocks, whose box ends at the line box bottom -- where the divider starts -- in every engine. The divider itself stopped 8px short of each screen edge, being a block inside a body that carries an 8px margin; it now cancels that gutter with -mx-2 and spans the viewport exactly. And the strip container carried min-w-screen, but 100vw includes the scrollbar -- 1665px inside a 1650px viewport -- so the page scrolled sideways by 23px, which also undid the full-bleed rule the moment you scrolled right. Removed; the strip's natural width is identical. The strip is now inset 37px from the left, one tab gap, so the first tab is spaced from the page edge the way the tabs are spaced from each other. PlaywrightTabsTest guards both promises in Chromium and WebKit. Running both engines is the point: the hairline was invisible to Chromium at every font size probed, so a Chromium-only test could not have caught it. These are the first WebKit tests in the suite, and the Java driver already downloads the browser, so they cost nothing extra to run. Also in this release: the toolchain and dependency refresh already in the tree (Gradle 9.7.1, Ktor 3.6.0, Exposed 1.5.0, Flyway 13.7.0, Kotest 6.2.5, Playwright 1.63.0, and others), the .gitattributes line-ending normalization that re-normalized gradlew.bat to CRLF, and release documentation across CHANGELOG.md, RELEASE_NOTES.md, the website release notes, README.md, llms.txt, and DESIGN.md. CLAUDE.md records the two facts this cost a debugging session to learn: never rest a pixel-exact effect on an inline box, and 100vw includes the scrollbar. Verified: 368 tests, 0 failures, 6 skipped; make lint clean; geometry confirmed in WebKit, Chromium, and Firefox. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Every image, icon and Prism file the app references was already packaged in the jar under static/ -- 29 of them, verified against the built artifact -- and yet all of them were fetched from https://static.readingbat.com. Endpoints.STATIC_ROOT was doing two incompatible jobs: it was both the path staticResources() mounts the classpath tree at and the URL prefix pages emit. As an absolute URL it mounted an unreachable route, so the app served none of these files. https://www.readingbat.com/static/white-check.jpg returned the HTML not-found page. Nothing noticed for eight releases because that page answers 200, indistinguishable from a real asset unless you check the content type. git history shows the constant was `/$STATIC` until 1.3.0, when it became the CDN URL -- the serving code predates the CDN and has been dead since. So the constant is deleted rather than redefined, splitting it in two: STATIC_PATH is always the local route path, and the prefix pages emit is a new STATIC_URL_PREFIX property (env var or readingbat.site.staticUrlPrefix) defaulting to it. Point that at a CDN origin and the old behavior returns with no rebuild, while the app keeps serving the files itself either way. Deleting forces every call site to fail loudly and be classified as one or the other, which is the whole point; silently changing a published constant's meaning is how this shipped in the first place. Assets carry a one-year max-age and an ETag, with ConditionalHeaders installed so those become 304s. That is only safe because StaticAssets.urlOf appends ?v=<version> to every URL: the filenames are not content-hashed, so a replaced image would otherwise be stranded behind warm caches for a year. Three things found along the way: - site.webmanifest declared its icons root-relative (/android-chrome-192x192.png) against files that live under icons/ -- 404 from the app, and already 403 on the CDN. Now relative, so they resolve under any prefix. - deflate declared its own condition (minimumSize), and Ktor applies its default content-type exclusions only when neither the plugin nor the encoder declares any -- so deflate opted itself out of the image/video/audio exclusions entirely, and at priority 10.0 it outranked gzip. Every response, PNGs and JPEGs included, went through it. Conditions moved to the plugin level, deflate dropped to 0.9. - The HTTPS redirect's excludePrefix compared a request path against a CDN URL, so it had never matched. Dropped rather than repaired: the assets are same-origin now, and exempting them would invite mixed content. staticAssetRoutes() is shared by the server and the Kotest test module so the two cannot drift again, and Vary: Accept-Encoding is now sent, since Ktor's Compression plugin emits none and static responses are Cache-Control: public. StaticAssetServingTest covers two things that are not the same and had both gone unchecked: that every packaged asset is served as itself (asserting content type and byte length, since status cannot tell an asset from the not-found page), and that every /static URL the pages actually emit resolves -- the latter is what would have caught this, since the route was fine and the emitted URL was not. Verified: 380 tests, 0 failures, 6 skipped; make lint clean; generated CSS unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The 3.4.0 release notes, CHANGELOG and CLAUDE.md already covered both commits on this branch, but the two files aimed at readers arriving cold did not mention that the app no longer needs a CDN. README gains a feature bullet, an architecture line, and STATIC_URL_PREFIX in both the environment-configuration sample and the production variable list. llms.txt gains a Static Assets section describing the contract that is easy to get wrong: build URLs with StaticAssets.urlOf so the configured prefix and the ?v= cache-buster are applied, reserve Endpoints.STATIC_PATH for route registration, and remember that a missing asset answers 200 with the not-found page, so status cannot distinguish it from a real one. Every version string in both files re-checked against the catalog. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
PlaywrightTabsTest failed on ubuntu-latest with "Host system is missing dependencies to run browsers", listing libgtk-4, graphene, gstreamer and flite. Playwright's Java driver downloads the browser binaries but not the OS libraries they need. Chromium happens to run on the runner without them, which is why the existing Playwright specs never surfaced this; WebKit does not, so the spec added for the tab geometry died in beforeSpec and took its four tests with it (378 completed, 2 failed). Installing the deps rather than skipping WebKit when it is unavailable: the whole point of that spec is that the bug it guards against is invisible to Chromium, so a spec that quietly degrades to Chromium-only would be a test that reports success for the one case it exists to catch. The playwright version is read from the version catalog rather than hardcoded, so it cannot drift from the dependency -- the same approach the Makefile uses to derive GRADLE_VERSION. Also corrects CLAUDE.md, which claimed a WebKit spec "costs nothing extra to run" on the strength of the driver downloading all three browsers. That is true of the binaries and false of the system libraries, which is precisely the gap this failure exposed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The dependency-install step reads the playwright version out of the version catalog, and this workflow runs on pull_request -- so on a fork PR that file is attacker-controlled, and the value flows into an npm package spec that `npx --yes` installs and executes. Shell quoting already prevented command injection, but not spec substitution: `playwright@git+https://attacker/repo` is a perfectly legitimate npm spec, as is a tag pointing at an arbitrary publish. Either yields code execution on the runner. So the value is now required to match a literal MAJOR.MINOR.PATCH before npx sees it, and the step fails loudly otherwise. Verified the pattern accepts 1.63.0 and rejects a git URL, a trailing `; curl | sh`, and `latest`. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
pambrose
changed the title
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
1 participant
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.
Summary
Fixes three defects in the language tab strip, refreshes the toolchain and dependencies, and cuts 3.4.0.
The selected tab left a hairline in WebKit. The folder-tab effect has the selected tab paint its white background over the 1px divider below the strip, nudged into place with
#selected { position: relative; top: 1px }. That only lands correctly if the tab's painted box ends exactly where the divider begins.nav liwasdisplay: inline, and an inline box is only as tall as the font's ascent plus descent — which Blink rounds to whole pixels (bottom 159, divider 158–159, exact cover) and WebKit leaves fractional (bottom 159.64, divider 159–160). The surviving 0.36px is ~0.7 device pixels at 2×: a line that looks thinner but never disappears. The tabs are now bottom-aligned inline-blocks, whose box ends at the line box bottom — where the divider starts — in every engine.The divider stopped 8px short of each screen edge. It is a block inside
<body>, which carries an 8px margin. It now carries-mx-2to cancel that gutter and spans the viewport exactly.The page scrolled sideways by 23px. The strip container carried
min-w-screen, and100vwincludes the scrollbar — 1665px inside a 1650px viewport, soscrollWidth(1673) exceededclientWidth(1650). Beyond the stray scrollbar, this undid the fix above: scroll right and the full-width divider ran out again.The strip is inset 37px from the left — one tab gap (the 25px + 6px margins plus the ~5.6px word space between two inline-block tabs) — so the first tab is spaced from the page edge the way the tabs are spaced from each other.
Test plan
PlaywrightTabsTest— four new geometric assertions run in both Chromium and WebKit: the selected tab's painted box must reach past the divider's bottom edge, and the divider must start at 0, end at the viewport width, and leave no horizontal overflow. Written before the fix: it failed in WebKit (159.63961791992188 should be >= 160.0) and passed in Chromium, then passed in both after. A Chromium-only test could not have caught this bug at any font size probed. These are the first WebKit tests in the suite.make lintclean.rule=[0, 1200],clientWidth=1200, no horizontal scroll, first tab at x=51, divider fully covered.Also included
.gitattributes:* text=auto, and thebinaryattribute dropped fromgradlew/*.batwhere it was suppressing both diffs and theeolconversion those same lines requested —gradlew.batis re-normalized to CRLF as a result (82 lines, no content change). The generatedstatic/tailwind.cssand vendoredstatic/prism/**are marked so GitHub collapses them.CHANGELOG.md,RELEASE_NOTES.md, the website release-notes page,README.md,llms.txt, andDESIGN.md.CLAUDE.mdrecords the two transferable lessons — never rest a pixel-exact effect on an inline box, and100vwincludes the scrollbar.gradle.properties3.3.1 → 3.4.0.Note
Constants.ktcarries a commented-outSTATIC_ROOTpointing at the DigitalOcean Spaces CDN — an existing working-tree edit, no behavior change. Say the word if it should come back out.🤖 Generated with Claude Code