Fix broken release packaging, test the fragile core, and polish to a higher bar - #15
Merged
Merged
Conversation
The hand-maintained INCLUDE list in scripts/build.mjs had fallen out of sync with manifest.json: checks.js was loaded as a content script but never packaged, so published .zip/.xpi builds shipped with the status-checks helpers missing. Derive the file list from the manifest (content scripts, background scripts, icons, popup, and the popup's <script src> tags), fail the build if any referenced file is missing, and add a packaging test that inspects the real archive in CI. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
icon-128-disabled.png existed in the repo but was referenced nowhere; add both 128px variants to the action icons so high-DPI toolbars get a crisp icon and the asset ships for a reason. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
- normalizeOrder() in constants.js coerces whatever comes out of chrome.storage into a valid order value, so a stale or corrupted entry can no longer desync the toggle label from the sort behavior. - The popup mirrors chrome.storage.onChanged, so toggling via the in-page button updates an open popup immediately. - The body observer also watches aria-label mutations: GitHub updates check-row labels in place (running -> successful) without structural changes, which previously left the checks indicator stale. - The injected toggle and checks indicator get plain-text aria-labels; the visible text leans on glyphs and color alone. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
- README: document checks.js in the file table, list the typecheck script, and correct the test command description. - package.json: add repository, homepage, and bugs fields. - CHANGELOG: record the packaging fix and behavior changes under Unreleased. - npm audit fix: bump transitive undici (jsdom) past known advisories. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
The selectors that break when GitHub ships UI changes — the extension's main maintenance burden — lived untested inside content.js. They now live in pages.js (same UMD pattern as the other modules) with jsdom tests whose fixtures mirror the real Conversation, modern React Commits, and legacy Rails Commits structures, plus path-routing tests. content.js shrinks to observer/storage/UI glue. background.js gets the same treatment: logic in a factory that Node tests exercise against a stubbed chrome API, with listener registration kept synchronous at load (an MV3 service-worker requirement). Tests cover URL matching (incl. rejecting /files, /checks, commit permalinks, non-GitHub hosts), icon selection, tab-event handling, and the runtime.lastError path. Both modules join the coverage gate; content.js also stops silently swallowing storage failures at init (logs instead of an unhandled rejection), and the stored-order/checks-state types tighten from string to unions. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
A passing check named e.g. failover-suite or cancellation-service was reported as failing because the failure regex matched bare substrings. Match explicit inflected status phrases behind word boundaries instead, with regression tests for embedded-status-word names and for every inflected form GitHub actually emits. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
Run the matrix on Node 22 (package.json engines floor) and 24 so an engines violation can't slip through, and pass persist-credentials: false to checkout since no CI step needs the token after clone. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
The popup hardcoded GitHub dark-mode colors, which looked out of place for light-mode users. Move the palette to custom properties with a prefers-color-scheme override and declare color-scheme so form controls match. Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
cursor
Bot
changed the title
ShiosOS
marked this pull request as ready for review
July 14, 2026 17:53
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Splitting the check job into a Node 22/24 matrix renamed its check runs
to 'check (22)' / 'check (24)', so the branch-protection rule requiring
a status named 'check' waited forever ('Expected — Waiting for status
to be reported'). Add a merge-gate job that reports the stable 'check'
name and fails unless every matrix leg succeeded.
Co-authored-by: Julius Walton <ShiosOS@users.noreply.github.com>
Merged
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.
Summary
A critical repo-wide review surfaced one release-breaking bug, several correctness gaps, and a structural testing hole. This PR fixes all of them across two passes.
Pass 1 — correctness and hygiene
Release packaging shipped a broken extension (the important one)
manifest.jsonloadschecks.jsas a content script, but the hand-maintainedINCLUDElist inscripts/build.mjsnever picked it up — so every.zip/.xpiproduced bynpm run build(including the tag-triggered release workflow) shipped without the status-checks helpers thatcontent.jsdepends on.Rather than appending the missing filename, the packaged file list is now derived from
manifest.json(content scripts, background scripts, icons, popup, plus the popup's<script src>tags), the build fails loudly if a referenced file is missing on disk, andtest/build.test.mjsruns the real build in CI and asserts the archive contents. This class of bug can't silently recur — proven later in this PR, when the newpages.jsmodule was picked up by the build automatically.Behavior fixes
aria-labels in place (running → successful) without structural DOM changes, so thechildList-only body observer never refreshed the indicator. It now also watchesaria-labelattribute mutations.chrome.storage.onChanged, so toggling via the in-page button left an open popup showing the wrong active state.normalizeOrder()now coerces anything unrecognized tonewestat both read sites.Pass 2 — raising the bar on the core
The fragile heart of the extension is now tested
The GitHub DOM selectors — the code that breaks every time GitHub ships a UI change, and the repo's #1 maintenance burden — lived untested inside
content.js. They now live inpages.js(same UMD pattern as the other modules) with jsdom fixtures mirroring the real Conversation, modern React Commits, and legacy Rails Commits structures, plus URL-routing tests.content.jsshrinks to observer/storage/UI glue.background.jsgot the same treatment: logic in a factory that tests exercise against a stubbedchromeAPI (listener registration stays synchronous at load, per MV3 service-worker rules). Tests cover URL matching — including rejecting/files,/checks, commit permalinks, and non-GitHub hosts — icon selection, tab events, and theruntime.lastErrorpath.Test count goes from 28 to 79, and both modules join the enforced coverage gate (now 96% statements across five modules).
Checks-state false positives
The failure regex matched bare substrings, so a passing check named
failover-suiteorcancellation-servicewas reported as failing. Status words now match as explicit inflected phrases behind word boundaries, with regression tests for embedded-status-word names and every phrase GitHub actually emits.Everything else
init()no longer swallows storage failures as an unhandled rejection; stored-order and checks-state types tightened fromstringto unions.enginesfloor) alongside 24, and checkout usespersist-credentials: false.prefers-color-schemeinstead of forcing dark colors on light-mode users.aria-labels on the injected toggle and indicator, which previously conveyed state via glyphs and color only.icon-128-disabled.pngis wired into the action icon set; README documentschecks.js/pages.jsand the selector-maintenance workflow;package.jsongainsrepository/homepage/bugs;npm audit fixcleared seven undici advisories (dev-only tree); CHANGELOG updated.Verification
format:check,lint,typecheck(stricttsc --checkJs),coverage(79 tests, all thresholds met), andbuildall pass; the built archive was inspected and contains all 15 manifest-referenced files.Not done here (possible follow-ups)
placehold.coREADME screenshot with a real capture.web-ext) for the remainingcontent.js/popup.jsglue and real-GitHub smoke testing.