Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,25 @@ concurrency:
jobs:
check:
runs-on: ubuntu-latest
strategy:
# Test the oldest Node we claim to support (package.json engines)
# alongside current, so "works on my machine" can't hide an engines
# violation.
matrix:
node-version: [22, 24]
steps:
# Actions are pinned to commit SHAs (supply-chain hardening); the
# trailing comment records the human-readable version. v6 runs on the
# Node 24 action runtime, clearing the Node 20 deprecation warning.
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
with:
# No step here pushes or calls the API; don't leave the token in
# .git/config for the rest of the job.
persist-credentials: false

- uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version: 24
node-version: ${{ matrix.node-version }}
cache: npm

- run: npm ci
Expand All @@ -42,3 +52,16 @@ jobs:
- run: npm run coverage

- run: npm run build

# Merge gate: branch protection requires a single status named "check".
# Reporting that name from an aggregate job (instead of the matrix legs,
# whose names carry the Node version) keeps the required-check name
# stable no matter how the matrix changes.
ci-ok:
name: check
needs: check
if: always()
runs-on: ubuntu-latest
steps:
- name: Fail unless every matrix leg succeeded
run: test "${{ needs.check.result }}" = "success"
6 changes: 4 additions & 2 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ jobs:
# version twice, so re-running this workflow for an existing tag will
# fail here — bump the version instead.
- name: Sign the Firefox .xpi with Mozilla (AMO)
if: env.AMO_JWT_ISSUER != ''
if: env.AMO_JWT_ISSUER != '' && env.AMO_JWT_SECRET != ''
run: |
version="$(node -p "require('./manifest.json').version")"
mkdir -p /tmp/xpi-src /tmp/xpi-signed
Expand All @@ -75,7 +75,9 @@ jobs:
# Upload the .zip to the Chrome Web Store and publish. Google's
# review still happens asynchronously on their side afterwards.
- name: Publish to the Chrome Web Store
if: env.CWS_EXTENSION_ID != ''
if: >-
env.CWS_EXTENSION_ID != '' && env.CWS_CLIENT_ID != '' &&
env.CWS_CLIENT_SECRET != '' && env.CWS_REFRESH_TOKEN != ''
env:
EXTENSION_ID: ${{ env.CWS_EXTENSION_ID }}
CLIENT_ID: ${{ env.CWS_CLIENT_ID }}
Expand Down
36 changes: 36 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,42 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- Release packaging omitted `checks.js`, so published `.zip`/`.xpi` builds
shipped without the status-checks helpers the content script depends on.
The packaged file list is now derived from `manifest.json` (plus popup
`<script>` tags) instead of being hand-maintained, missing files fail the
build, and a packaging test verifies the archive contents in CI.
- The status-checks indicator now refreshes when GitHub updates a check's
label in place (e.g. running → successful) instead of waiting for
unrelated DOM changes.
- The popup now reflects a preference change made via the in-page toggle
while it is open.
- A corrupted stored preference value no longer desyncs the toggle label
from the actual sort order; anything unrecognized falls back to newest.

- Status-word matching in the checks indicator uses word boundaries, so a
check _named_ e.g. `failover-suite` or `cancellation-service` that passed
is no longer misreported as failing.

### Added

- Accessible labels (`aria-label`) on the injected toggle button and
status-checks indicator.
- 128px toolbar icon variants wired into the action icon set.
- The popup follows the browser's light/dark preference instead of always
rendering dark.

### Changed

- The per-page GitHub DOM selectors moved from `content.js` into a new
`pages.js` module, and `background.js` became importable under Node —
both are now covered by unit tests (79 tests total), including the page
structures for the Conversation, modern Commits, and legacy Commits
views.
- CI runs on both the minimum supported Node (22) and current (24).

## [1.1.0] - 2026-06-04

### Added
Expand Down
28 changes: 18 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,22 @@ source files in this repo as-is. To make a change, edit the source, reload
the extension at `chrome://extensions` (or re-install the temporary add-on
in Firefox), and refresh a PR page.

| File | Purpose |
| --------------- | ----------------------------------------------------- |
| `manifest.json` | Extension definition (Manifest V3, Chrome + Firefox) |
| `constants.js` | Shared constants (storage key, order values) |
| `reorder.js` | Pure DOM-reordering helpers (unit-tested) |
| `content.js` | Runs on PR pages; reorders comments, draws the toggle |
| `background.js` | Keeps the toolbar icon in sync with the current tab |
| `popup.html` | Toolbar popup UI |
| `popup.js` | Popup behavior; reads/writes saved preference |
| File | Purpose |
| --------------- | --------------------------------------------------------- |
| `manifest.json` | Extension definition (Manifest V3, Chrome + Firefox) |
| `constants.js` | Shared constants (storage key, order values) |
| `reorder.js` | Pure DOM-reordering helpers (unit-tested) |
| `checks.js` | PR status-checks detection helpers (unit-tested) |
| `pages.js` | Per-page GitHub DOM selectors and targets (unit-tested) |
| `content.js` | Runs on PR pages; wires storage, observers, and the UI |
| `background.js` | Keeps the toolbar icon in sync with the tab (unit-tested) |
| `popup.html` | Toolbar popup UI |
| `popup.js` | Popup behavior; reads/writes saved preference |

When GitHub ships a UI change that breaks reordering, the selectors to
update live in `pages.js` (timeline/commits targets) and `checks.js`
(status-checks box); each has jsdom tests with fixtures mirroring the real
page structure, so start by updating the fixture to match the new DOM.

### Dev tooling (optional)

Expand All @@ -134,7 +141,8 @@ extension — they only help when contributing.
npm install # install dev dependencies
npm run lint # ESLint
npm run format # Prettier (writes)
npm test # Vitest unit tests for reorder.js
npm run typecheck # tsc --checkJs over the extension sources
npm test # Vitest unit tests (reorder, checks, constants, packaging)
npm run build # produce the .zip / .xpi (version read from manifest.json)
```

Expand Down
129 changes: 80 additions & 49 deletions background.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,56 +8,87 @@
// We use chrome.tabs events instead of messages from the content script
// so the icon updates correctly even when the user navigates to a
// non-PR URL (where the content script doesn't run).
//
// Structured like the other modules: the logic lives in a factory so
// Node tests can import and exercise it against a stubbed chrome API,
// while listener registration stays synchronous at load time (an MV3
// service-worker requirement — events fired while the worker was asleep
// are only redelivered to listeners registered during startup).

const ACTIVE_PATH = { 16: "icon-16.png", 48: "icon-48.png" };
const DISABLED_PATH = { 16: "icon-16-disabled.png", 48: "icon-48-disabled.png" };

// Matches any PR page the extension is active on:
// /owner/repo/pull/N (Conversation)
// /owner/repo/pull/N/commits (Commits)
// All other sub-paths (/files, /checks, deep diff anchors) leave the
// toolbar icon in the disabled state.
const ACTIVE_URL_RE =
/^https:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+(?:\/commits)?\/?(?:[?#].*)?$/;

/** @param {unknown} url */
function isActiveUrl(url) {
return typeof url === "string" && ACTIVE_URL_RE.test(url);
}

/**
* @param {number | undefined} tabId
* @param {string | undefined} url
*/
function setIconForTab(tabId, url) {
const path = isActiveUrl(url) ? ACTIVE_PATH : DISABLED_PATH;
chrome.action.setIcon({ tabId, path }).catch(() => {
// Tab may have been closed between the event and our call; ignore.
});
}

// Initial state on install / browser start: walk existing tabs.
function syncAllTabs() {
chrome.tabs.query({}, (tabs) => {
for (const tab of tabs) setIconForTab(tab.id, tab.url);
});
}

chrome.runtime.onInstalled.addListener(syncAllTabs);
chrome.runtime.onStartup.addListener(syncAllTabs);

// URL changes — covers both full page loads and pushState/replaceState soft nav.
chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
if (changeInfo.url || changeInfo.status === "complete") {
setIconForTab(tabId, changeInfo.url || tab.url);
(function (root, factory) {
const api = factory();
if (typeof module !== "undefined" && module.exports) {
module.exports = api;
} else {
Object.assign(root, api);
}

if (typeof chrome !== "undefined" && chrome.runtime) {
chrome.runtime.onInstalled.addListener(api.syncAllTabs);
chrome.runtime.onStartup.addListener(api.syncAllTabs);
chrome.tabs.onUpdated.addListener(api.handleTabUpdated);
chrome.tabs.onActivated.addListener(api.handleTabActivated);
}
})(typeof globalThis !== "undefined" ? globalThis : this, function () {
const ACTIVE_PATH = { 16: "icon-16.png", 48: "icon-48.png", 128: "icon-128.png" };
const DISABLED_PATH = {
16: "icon-16-disabled.png",
48: "icon-48-disabled.png",
128: "icon-128-disabled.png",
};

// Matches any PR page the extension is active on:
// /owner/repo/pull/N (Conversation)
// /owner/repo/pull/N/commits (Commits)
// All other sub-paths (/files, /checks, deep diff anchors) leave the
// toolbar icon in the disabled state.
const ACTIVE_URL_RE =
/^https:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+(?:\/commits)?\/?(?:[?#].*)?$/;

/** @param {unknown} url */
function isActiveUrl(url) {
return typeof url === "string" && ACTIVE_URL_RE.test(url);
}

/**
* @param {number | undefined} tabId
* @param {string | undefined} url
*/
function setIconForTab(tabId, url) {
const path = isActiveUrl(url) ? ACTIVE_PATH : DISABLED_PATH;
chrome.action.setIcon({ tabId, path }).catch(() => {
// Tab may have been closed between the event and our call; ignore.
});
}

// Initial state on install / browser start: walk existing tabs.
function syncAllTabs() {
chrome.tabs.query({}, (tabs) => {
for (const tab of tabs) setIconForTab(tab.id, tab.url);
});
}

// URL changes — covers both full page loads and pushState/replaceState soft nav.
/**
* @param {number} tabId
* @param {chrome.tabs.OnUpdatedInfo} changeInfo
* @param {chrome.tabs.Tab} tab
*/
function handleTabUpdated(tabId, changeInfo, tab) {
if (changeInfo.url || changeInfo.status === "complete") {
setIconForTab(tabId, changeInfo.url || tab.url);
}
}

// Tab activation — make sure the toolbar reflects the now-focused tab
// even if it was opened/loaded in the background.
/** @param {{ tabId: number }} activeInfo */
function handleTabActivated({ tabId }) {
chrome.tabs.get(tabId, (tab) => {
if (chrome.runtime.lastError) return;
setIconForTab(tabId, tab.url);
});
}
});

// Tab activation — make sure the toolbar reflects the now-focused tab
// even if it was opened/loaded in the background.
chrome.tabs.onActivated.addListener(({ tabId }) => {
chrome.tabs.get(tabId, (tab) => {
if (chrome.runtime.lastError) return;
setIconForTab(tabId, tab.url);
});
return { isActiveUrl, setIconForTab, syncAllTabs, handleTabUpdated, handleTabActivated };
});
17 changes: 14 additions & 3 deletions checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@
.filter(Boolean);
}

// Status-phrase patterns. Word boundaries plus explicit word forms keep
// a check *named* e.g. "failover-suite" or "cancellation-service" from
// tripping the failure branch when its label reads "... successful in 2m"
// (bare substring matching used to misreport those as failing). A check
// name that itself contains a standalone status word (e.g. "error-pages")
// is still ambiguous — labels are all GitHub's accessible rows give us.
const FAILING_RE =
/\b(fail(?:ed|ing|ure|s)?|error(?:ed|s)?|timed out|cancel(?:l?ed)?|denied|action required)\b/i;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plural failures not detected

Low Severity

The new word-boundary FAILING_RE no longer treats check row labels that use the plural word failures as failing, whereas the previous substring fail matcher did. The conversation checks indicator can stay passing or unknown when a label only reports failures in that form.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7eca596. Configure here.

const RUNNING_RE = /\b(in[ _]progress|pending|queued|running|waiting|expected)\b/i;
const PASSING_RE = /\b(success(?:ful)?|passed|passing)\b/i;

// Aggregate the check-row labels into a coarse { key, label, color }.
// Precedence: any failure -> failing; else any in-flight -> running;
// else any success -> passing; else unknown. Checked in that order so a
Expand All @@ -55,13 +66,13 @@
/** @param {RegExp} re */
const any = (re) => list.some((l) => re.test(l));

if (any(/(fail|error|timed out|cancel|denied|action required)/i)) {
if (any(FAILING_RE)) {
return { key: "failing", label: "✗ Checks failing", color: "#d1242f" };
}
if (any(/(in progress|in_progress|pending|queued|running|waiting|expected)/i)) {
if (any(RUNNING_RE)) {
return { key: "running", label: "• Checks running", color: "#9a6700" };
}
if (any(/(success|passed|passing)/i)) {
if (any(PASSING_RE)) {
return { key: "passing", label: "✓ Checks passing", color: "#1a7f37" };
}
return { key: "unknown", label: "• Checks status", color: "#1f6feb" };
Expand Down
20 changes: 14 additions & 6 deletions constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,18 @@
Object.assign(root, factory());
}
})(typeof globalThis !== "undefined" ? globalThis : this, function () {
return {
// chrome.storage.local key holding the user's order preference.
STORAGE_KEY: "prCommentOrder",
// Allowed values for that preference.
ORDER: { NEWEST: "newest", OLDEST: "oldest" },
};
// chrome.storage.local key holding the user's order preference.
const STORAGE_KEY = "prCommentOrder";
// Allowed values for that preference.
const ORDER = { NEWEST: "newest", OLDEST: "oldest" };

// Coerce whatever came out of storage into a valid order value. Storage
// is a system boundary: a stale or corrupted value must not leave the
// button label saying one thing while the sort does another.
/** @param {unknown} value */
function normalizeOrder(value) {
return value === ORDER.OLDEST ? ORDER.OLDEST : ORDER.NEWEST;
}

return { STORAGE_KEY, ORDER, normalizeOrder };
});
Loading