Rank the "Latest" pill by the newest commit, not the newest touch - #285
Open
juanmaguitar wants to merge 3 commits into
Open
Rank the "Latest" pill by the newest commit, not the newest touch#285juanmaguitar wants to merge 3 commits into
juanmaguitar wants to merge 3 commits into
Conversation
The pill was decided by GitHub's `updated_at`, which a comment, a label change or an upstream force-push bumps just as hard as a push. On Trac #62064 a force-push of trunk restamped ~2,580 open pull requests inside one window; the two on that ticket landed 19 seconds apart in the sweep, and the app crowned the November 2024 patch that no longer applies over the April 2026 one that does. Pull requests are now dated by their newest commit. The extra cost is one request per lookup against the shared unauthenticated quota, kept small by a bound: `updated_at` is never earlier than the last commit, so on the list already sorted by it, it bounds every row below. The walk stops as soon as a resolved commit date beats the next row's bound — one extra request on a normal ticket — and stops at four regardless. When the walk cannot finish, or the top two are within an hour, there is no pill: the rows still carry their dates, and a wrong claim sends a contributor into an apply that cannot succeed. Closes #281 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reuse cached commit dates instead of re-spending the walk. `search/issues` and the core API have separate unauthenticated allowances, so "the search answers, the commit lookups 403" is the ordinary state on a shared IP — and without reuse that Refresh overwrote a ranking the contributor could already read with an unranked list. A push moves `updated_at`, so a row whose stamp has not changed has not had new code pushed to it either, and its cached date still stands. Discard a commit dated ahead of now. As the running best it would clear every remaining bound at once, ending the walk and declaring the ranking complete on the strength of the one date that is wrong — the highest confidence exactly where there is least. Move the row's date label into src/renderer/pr-date-label.cjs. It chooses between two strings a contributor reads and two sources, which is a decision the suite cannot reach inside index.jsx. It is also the one place allowed to fall back to `updatedAt` after latest-patch.cjs was changed never to, and that asymmetry is worth a test. Correct two comments that claimed more than the code does: the lookup cap counts requests, not pull requests, and an unresolved row is known to be older than the newest resolved one, not than every row above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pill vanished from the ordinary ticket. The walk leaves a row undated on purpose once its stamp is below a resolved commit date — that is how it gets away with one lookup — and reports the ranking complete, correctly. pickLatest then read any undated row as unknown and refused to answer. So the cheap path the walk exists to produce was exactly the path that showed no pill, and the "latest is a patch file" note went with it. Both halves were green in isolation; nothing drove one into the other. An undated pull request now competes as an upper bound on the runner-up rather than disqualifying the answer, which collapses the two cases into one comparison. A row the walk ruled out carries a stamp far below the winner, so the pill shows. A row it never reached carries the stamp that has not been ruled out — in a force-push sweep, at or above the winner — so the near-tie check withholds the pill on its own. A row with no date at all is unbounded and still takes the answer with it. Also from the review: the horizon check now covers cached dates, which a backwards clock correction could otherwise leave in the future, winning the pill and never being re-read since a dated row is one the walk skips. It gained ten minutes of tolerance, so a machine a few minutes out does not throw away a just-pushed commit. And the second request a paginated pull request spends is now checked against the cap, which counts requests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Why
The Latest pill is meant to point a contributor at the freshest fix so they can try that one first. It was ranked by GitHub's
updated_at, which a comment, a label change or a bot sweep bumps just as hard as a push.On Trac #62064 that broke outright. An upstream force-push of trunk restamped roughly 2,580 open pull requests inside one window; the two on that ticket landed 19 seconds apart in that sweep, and the app crowned the one whose newest code is from November 2024 — which no longer applies — over the one from April 2026, which does. The contributor followed the pill into an apply that could not succeed.
It is not an edge case needing a tie-break. Any ticket with more than one open pull request had its pill decided by the internal ordering of an unrelated bulk event upstream. See #281.
What changes
Root cause:
updated_atis a "most recently touched" stamp, and the ranking treated it as "newest code". The caveat was written down when the ranking was built and accepted as a fair trade for a comment. It does not survive an event that touches every pull request at once — that does not nudge the ordering, it replaces it with noise.Pull requests are now dated by their most recent commit. An unresolved one does not compete at all: falling back to
updated_atwould be falling back to the bug.The cost, and what keeps it small.
search/issues— the one request the list is built from — carries no commit date, so this costs a request per pull request against the shared unauthenticated quota. Three things bound it:updated_atis never earlier than the last commit, so on the list already sorted by it, it is a per-row upper bound. The walk stops as soon as a resolved commit date beats the next row's bound. A ticket with one pull request, or with a clear winner, costs one extra request. Only a ticket caught inside a fresh force-push sweep — every bound identical — walks further.updated_at, so a row whose stamp has not changed since it was cached has not had new code pushed to it either. A Refresh re-spends nothing.Where it stays quiet. No pill when the walk could not finish, and none when the top two commits are within an hour of each other — that margin is a coin flip, and the rows carry their dates for the contributor to read. Each row now says
last commit 12 Apr 2026rather thanupdated 6 Jul 2026, so the row and the pill stop contradicting each other.Deliberately not in this PR: authenticating the lookups to get a larger quota, and pre-resolving dates in the background. Both are real options if the cap turns out to bite; neither is needed to fix the bug.
How to test this
Platforms: any — no paths, spawning or line endings are touched.
Starting state: a site with Trac #62064 linked, and the patch panel open. Run the app from a terminal with
WP_DEV_ENV_HTTP_LOG=1set so the GitHub requests are visible.last commitwith an April 2026 date — three rows below where the pill used to be. Every row readslast commit …, notupdated ….trunktoday the pill points at the November 2024 one, whose apply fails because the file it touches has moved on ("No longer applies" hides a patch that is already in the tree — sometimes in the same file as one that is not #226)./pulls/*/commitslines in the terminal. Expected: at most four, and on a ticket with an obvious winner exactly one./pulls/*/commitsrequests at all — the dates come back from the cache — and the list and pill are unchanged.What must not have happened:
Covered by
test/latest-patch.test.cjs—pickLatest: the force-push stamps do not decide it; the newest commit does (issue #281)reproduces #62064's exact two stamps and commit dates, and fails on the old code. So doesfetchLinkedPrs ranks by newest commit, not by an upstream force-push stampintest/github-prs.test.cjs. Both checked againsttrunk.Risks and limitations
Self-review, two rounds: 5 findings then 4 — all
[fix here]items fixed in both, one follow-up deferred. The second round caught a 🔴 that would have shipped this PR removing the pill from the ordinary ticket; detail in the collapsed block below.updated_athas not moved. A comment moves it too, so this sometimes re-fetches a date it did not need to — the safe side of the trade.Related
Closes #281
Fixes the failed apply described in #226
Follow-up to #109, where the pill and the pull request list came from
Design decisions and alternatives considered
Why
pulls/{n}/commitsand not something cheaper.GET /repos/…/pulls/{n}carries no commit date.head.repo.pushed_atis the fork's last push to any branch, so it is wrong for this. Resolvinghead.shaand thencommits/{sha}is two requests, not one. A ref trick — askingcommits/refs%2Fpull%2FN%2Fhead— might be one request, but it is undocumented, and this file already refuses undocumented GitHub behaviour on the.diffroutes for the same reason.Why a bound rather than "resolve the top three". A fixed top-N is simpler and predictable, but it spends requests on tickets with an obvious winner and still misses the true winner on a ticket with four or more pull requests — the exact shape of the bug. The bound is exact where it terminates and costs less in the common case. The cap is what keeps the pathological case survivable.
Why no pill instead of a tie-break rule. A tie-break on noise is still noise, wearing a more confident face. The rows carry their dates; a contributor reading two dates is better served than one following a pill into a dead end.
What was deliberately left alone. The
updatedAtsort inparseLinkedPrsstays — it is the ordering the bound needs. It is now documented as such, so it does not get "simplified" into the display order later.Review outcome (required — see AGENTS.md)
Run twice per
.github/instructions/code-review.instructions.md, each time with the judgement pass dispatched to a subagent given only the diff and the standard.npm run lintclean,npm test853 pass / 0 fail.Round two — 3 [fix here] · 1 [follow-up]
All five round-one findings verified resolved. Three new, all fixed:
correctness · 🔴 · [fix here] ·
src/latest-patch.cjs— the pill disappeared from the ordinary ticket. The walk leaves a row undated on purpose once its stamp falls below a resolved commit date — that is how it gets away with one lookup — and reports the ranking complete, correctly.pickLatestthen read any undated row as unknown and refused to answer, so the cheap path the walk exists to produce was exactly the path that showed no pill, and the "latest is a patch file" note went with it. Reproduced end to end before fixing: a two-PR ticket with an unambiguous winner andrankComplete: truereturnednull.The blanket rule was mine, added so a pre-change cache would degrade safely. Replaced by one comparison: an undated pull request competes as an upper bound on the runner-up rather than disqualifying the answer. A ruled-out row's stamp sits far below the winner, so the pill shows; an unreached row's stamp sits at or above it in a force-push sweep, so the near-tie check withholds the pill on its own. No flag distinguishes them.
The test finding matters as much: both halves were green in isolation while the feature was broken between them, because nothing drove
fetchLinkedPrsoutput intopickLatest. Four seam tests now do.correctness · 🔵 · [fix here] ·
src/github-prs.js— the horizon check from round one covered fetched dates but not cached ones, so a backwards clock correction could leave a future date on a row where it would win the pill and never be re-read (a dated row is one the walk skips). It also gained ten minutes of tolerance, so a machine a few minutes out does not discard a legitimately just-pushed commit.performance · 🔵 · [fix here] ·
src/github-prs.js— the cap was checked before a lookup, but a paginated pull request spends two requests inside one, so the real ceiling was five against a comment that argues at length that it counts requests. The second page is now checked too, and refuses rather than falling back to the first page — whose newest commit is the oldest part of the pull request.Deferred: architecture · 🔵 · [follow-up] ·
src/main.js— if a pull request'supdatedAtmoves for a non-code reason while the commits quota is spent, its cached date is dropped, the refetch fails, and the store is overwritten with that row undated. The good date is gone from disk rather than merely unused. The fix is a policy choice — keep the last-known date, marked stale — rather than a bug in what shipped, so it is a follow-up.Round one — 2 [fix here] · 3 [follow-up]
Both
[fix here]fixed, 2 of 3 follow-ups also fixed:src/main.js— a partially-ranked result overwrote a complete cached ranking.search/issuesand the core API have separate unauthenticated allowances, so "search works, commit lookups 403" is the ordinary state on a shared Contributor Day IP; the failed request destroyed the evidence rather than merely failing to add to it. Fixed by passing the cached list back intofetchLinkedPrsand reusing dates for rows whoseupdatedAthas not moved — which also resolves the re-spend follow-up below.src/renderer/index.jsx— the date label became a two-branch decision insideindex.jsx, unreachable by the suite. Moved tosrc/renderer/pr-date-label.cjswithtest/pr-date-label.test.cjs, per the §1 invariant.src/github-prs.js— a future-dated commit both won the pill and silently truncated the walk, since as the running best it clears every remaining bound at once. Fixed here rather than deferred: it reported the highest confidence exactly where it had the least. A date ahead of now is now discarded.src/patch-sources.cjs— a comment claimed unresolved rows were "provably older than everything above them"; the walk only proves they are older than the newest resolved one. Comment corrected to state what the code actually establishes. The ordering itself is unchanged, and the pill is unaffected either way.Round one raised no style or process notes beyond these: the ones it did —
MAX_COMMIT_LOOKUPScounting requests rather than pull requests, anditem.numberinterpolated into a URL path without the digit-strippingfetchPrDiffuses — were both addressed in the doc comment and judged consistency-not-exposure respectively (the source is api.github.com over TLS).Implementation notes
src/github-prs.js—fetchPrCommitDate(one page of 100, followingLink rel="last"once for a longer pull request; a failed second page discards the first page's stale answer rather than using it) andresolveCommitDates(the cache pre-pass, then the bounded walk). GitHub truncates the commit list at 250, so a pull request longer than that resolves to the newest of the 250 it lists.src/patch-sources.cjs—orderByCommitDatefor display order. Resolved rows first by commit date; the rest follow inupdatedAtorder, which is not a claim about them.src/latest-patch.cjs—prDateMsreadscommitDate;pickLatestgainedprRankCompleteand the near-tie rule. An undated pull request contributes itsupdatedAtas an upper bound on the runner-up and never as the answer, which is also what makes a list restored from a pre-change cache degrade safely rather than needing a migration.src/main.js—rankCompleteis stored with the items and returned on both the fresh and the fallback path. The store shape change is purely additive.Screenshots or recording
Not captured — see the first limitation above. The visible change is small and stated precisely in How to test this: the pill moves to the April 2026 pull request, and each row's date line reads
last commit …instead ofupdated …. Worth a reviewer confirming on a real #62064 rather than taking a still frame from me.