fix(redesign): repair the hero selectors and make the canary able to catch the next drift - #145
Merged
Merged
Conversation
RoyalRoad renumbered the redesign's chapter hero headings: the chapter
title went h3 -> h1 and the fiction title h4 -> h2. Both selectors were
pinned to the old levels, so on the beta layout:
- chapterTitle matched nothing, and every recap failed with "Could not
find the previous chapter's title";
- fictionTitle silently landed on the AUTHOR's h4 instead. That one
produced no error at all — the recap was titled with the author's
name, and findFictionOverviewUrl()'s closest("a") walk pointed the
blurb fetch at /profile/<id> rather than the fiction.
Match on role instead, which survives the next renumber: the chapter
title is the hero's only heading that is not inside a link, and the
fiction title is the heading inside the hero's link to the fiction.
Scoping the latter to that anchor also fixes the overview URL by
construction rather than by luck of document order.
The drift above shipped unnoticed for a day because the canary could not
see it and would not have said so if it could. Two gaps, both closed:
Coverage. getChapterPageSelectors()/getFictionPageSelectors() default to
FALLBACK_ADAPTER, so the canary only ever checked the legacy layout — the
redesign's selectors were never tested against the live site at all. It
now loops over ADAPTERS and asks RoyalRoad for each layout in turn via
the beta-ui-v2 cookie, asserting first that the requested layout is what
was actually served so a withdrawn beta cannot masquerade as drift.
Cadence. It ran on pull_request only, and RoyalRoad changes its markup on
its own schedule rather than ours. Added a daily cron; a failing
scheduled run opens (or comments on) one labelled issue, which is closed
automatically when the canary goes green again — a red X on a cron run
notifies nobody.
Two further hardening changes fall out of the same bug. Presence checks
cannot see a selector that matches a real but WRONG element, so the two
identity selectors are now checked for what they resolve to: the chapter
title against the page's <title>, and the fiction title's closest("a")
against /fiction/. Under the old selectors that second test fails while
"fictionTitle selector exists" still passes. And selectors must now match
exactly one element unless listed in MAY_REPEAT, since matching several
is how the extension's querySelector starts reading the wrong one.
The canary no longer loads the extension, so the workflow's build step is
gone with it — one less way for this job to fail for reasons that have
nothing to do with selectors.
… colour
Audit of every remaining selector on both layouts against the live site,
after the hero-heading drift showed what the fragile ones look like. Each
replacement below was verified across six different fictions.
Legacy:
chapterTitle h1.font-white -> .fic-header h1
fictionTitle h2.font-white -> .fic-header a[href*='/fiction/'] :is(h1..h6)
closeButton a four-deep nth-child chain -> #settings .modal-footer
button[data-dismiss='modal']
reportPlacement div.col-lg-3:nth-child(3) -> div:has(> a[href^='/report/chapter/'])
Redesign:
prevChapterBtn now matches EITHER RoyalRoad's data-vt-direction='prev'
or the Font Awesome arrow, so losing one hook is survivable
togglePlacement [class*='grid-cols-2']:has(#chapterSelect) — the same
element prepareMounts() resolves, so the override hint and
the behaviour finally agree, and it stops matching the
duplicate nav bar below the chapter
closeButton RoyalRoad's data-rr-dialog-close, not "the direct child
that happens to be a button"
font-white and the nth-child chains work today but say nothing about what
they select; the anchor-scoped fiction title additionally makes
findFictionOverviewUrl()'s closest("a") walk correct by construction
rather than by luck of nesting — the legacy layout had the same latent
author-heading trap the redesign fell into, one markup change away.
Deliberately unchanged: blurbLabels still leans on .font-red-sunglo, which
appears ~60 times on an overview page, because RoyalRoad gives the
content-warning box no id, data attribute or semantic class to aim at.
Fixtures now carry a .fic-header block with the author heading present,
mirroring the real page, so a title selector that drifts onto the author
fails in the suite. togglePlacement leaves the canary's MAY_REPEAT
allowlist: it matches exactly one element on both layouts now.
Two ways the scheduled run's issue automation reported the wrong thing. The close step ran on success(), but the canary calls test.skip() on every test when RoyalRoad is unreachable, and Playwright exits 0 on an all-skipped run (verified: stats.expected 0, exit 0). So an outage during the 05:17 cron auto-closed a still-valid drift issue with "the canary is green again", having checked nothing. It now runs the JSON reporter alongside list and closes only when stats.expected is above zero. The report step ran on failure() of ANY prior step, so a lockfile drift or a Playwright download hiccup filed "RoyalRoad's layout drifted" and sent someone into the adapters after a CI problem. Both steps now key off steps.canary.outcome instead of the job's.
…yout guard bite browser.newContext() does not inherit the project's `use` options — Playwright applies those in the context/page fixtures, which a shared per-describe navigation can't use. The canary was therefore fetching royalroad.com as HeadlessChrome rather than the Desktop Chrome the config asks for, so anything the site varies on the user agent would have surfaced as a selector drift on a required check. The "RoyalRoad served this layout" test claimed to guard the tests below it, but Playwright tests are independent and nothing skipped on its failure: a renamed cookie would have failed it AND every selector test with a "drifted" message, which is the exact masquerade the split exists to prevent. The check now also runs in beforeAll and sets a flag the selector tests skip on, while the test itself stays reportable. Also close the context on the paths that never hand a page back — the caller closes contexts through the page it was given, so an outage leaked one context per describe per retry until the worker exited.
prepareMounts() never read selectors.togglePlacement: it resolved the nav bar from a hardcoded #chapterSelect walk with a #chapterHeroData fallback, so resolveToggle() never returned null and the base mount was dead code. A user whose toggle placement broke could edit the key in Advanced Settings — which the settings UI offers precisely so a layout change can be worked around without an extension update — and watch nothing happen. The configured selector is now asked first, with the #chapterSelect walk as the fallback for when it stops matching. Both resolve to the same inner grid on today's markup, so the rendered placement is unchanged.
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
RoyalRoad renumbered the redesign's chapter hero headings, which broke every recap on the beta layout. The canary that should have caught it was only checking the legacy layout. This fixes the drift and closes the gaps that let it ship unnoticed.
Changes
chapterTitlematched nothing ("Could not find the previous chapter's title");fictionTitlesilently landed on the author's heading, pointing the blurb fetch at/profile/<id>.togglePlacementnow actually moves the toggle on the redesign —prepareMounts()never read the key. Rendered placement unchanged.beta-ui-v2cookie, runs daily, and files one labelled issue on drift.Testing
Live canary green on this branch: 25 passed, 1 skipped (redesign
blurbLabels, intentionally unconfigured). Unit, e2e and Prettier green.