Skip to content

Show the favicon as passing when peer review is the only failing check - #369

Merged
roryabraham merged 2 commits into
mainfrom
rory-peer-review-favicon
Aug 7, 2026
Merged

Show the favicon as passing when peer review is the only failing check#369
roryabraham merged 2 commits into
mainfrom
rory-peer-review-favicon

Conversation

@roryabraham

@roryabraham roryabraham commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Fixed Issues

For https://github.com/Expensify/Expensify/issues/668743

Problem

#368 fixed the per-commit indicators in a PR's history, but the tab's favicon has the same problem. GitHub shows a red X in the favicon while a PR's checks are failing, and since the org-wide ruleset started triggering verifyPeerReview.yml, that X is there from the moment the PR opens until someone reviews it

Solution

Point the favicon link at the passing icon GitHub already serves when the only check holding the PR back is one we ignore.

That last part matters. GitHub keeps a favicon per state (favicon-failure, favicon-success, favicon-pending, in both .svg and .png) and swaps its own link.js-site-favicon between them — the SVG link even carries data-base-href for exactly that. So this is a one-attribute change to a URL GitHub is already serving.

  • src/js/lib/checkStatusVerdicts.js (new) — the verdict cache and the classification logic, moved out of commitCheckStatuses.js. The favicon and the commit rows now share one cache and, more to the point, one decision about what counts as failing. Duplicating that would mean two places to get a wrong green wrong.
  • src/js/lib/prFavicon.js (new) — resolves the PR's head commit, asks for its verdict, and swaps the link. Stores the href GitHub had set so it can be put back exactly rather than rebuilt.
  • src/js/lib/api.jsgetPullRequestHeadRefOid(), since the favicon reports on the PR as a whole, which is the state of its head commit.

Carried over from #368: a null verdict leaves whatever GitHub set alone, a verdict of false puts our change back, and the head SHA is re-read after 60s so a push doesn't leave us reporting on the wrong commit. Navigating off the PR drops our marker rather than holding a stale icon over whatever GitHub sets next.

One bug fix worth calling out: the first version ran a pass at init before the token had loaded out of Onyx. getOctokit() memoises for the life of the page, so that single early call cached an unauthenticated client and made every subsequent GraphQL request 403. The init path now touches nothing until the token is there.

Tests

GitHub only swaps in its status favicon for a signed-in session, so these set the failing favicon GitHub would have set and check what the extension does with it. Everything else — the verdict, the head commit lookup, the API — is real.

  1. Run npm run build, then load the dist/ folder as an unpacked extension.
  2. Open prevent live-tail cleanup from clearing RHP route params App#98020, whose head commit fails Check independent approval and nothing else.
  3. Confirm the tab's favicon shows a green check once the checks have reported.
  4. Open Guard FlashList ViewHolderCollection render against stale layout index (web crash) App#98015, whose head commit fails checklist as well.
  5. Confirm the tab's favicon still shows a red X.
  6. Navigate from the PR in step 2 to any non-PR page and confirm the favicon is whatever GitHub sets, not a leftover green check.

GitHub serves a favicon per check state and swaps its own link between them, so
this points the link at the asset GitHub already has rather than drawing a
replacement. Fetching and re-encoding an icon is what leaves a tab showing the
browser's default globe when any step of that chain fails.

The verdict logic moves to its own module so the favicon and the commit rows
share one cache and, more importantly, one decision about what counts as
failing. Duplicating that would be two places to get a wrong green wrong.
@roryabraham
roryabraham marked this pull request as ready for review August 7, 2026 08:36
Base automatically changed from rory-commit-check-statuses to main August 7, 2026 15:33
@NicolasBonet

Copy link
Copy Markdown
Contributor

@roryabraham we need to fix conflicts

@roryabraham

Copy link
Copy Markdown
Contributor Author

@NicolasBonet conflicts resolved

@roryabraham
roryabraham merged commit dcbba40 into main Aug 7, 2026
6 checks passed
@roryabraham
roryabraham deleted the rory-peer-review-favicon branch August 7, 2026 19:34
@os-botify

os-botify Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🚀 Released in version 1.5.53 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants