Repository navigation
fix(hub): show untriaged notes only for the current head - #680
Conversation
|
Triage: Pass No changes needed from you. A maintainer will review this pull request. Notes:
|
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
Restricts the "Couldn't triage" note so it only appears when recorded against the pull request's current head, as reported by the open-pulls listing. This stops stale-head notes after a push and keeps closed pull requests from reappearing when a repository listing fails. The one real defect is that a failed or refused listing now drops notes for open pull requests whose head is still current.
| if (row.status === "waiting") waiting.set(pullKey(pull), { ...pull, since: row.firstSeenAt, headSha: row.headSha }); | ||
| const text = row.error === undefined ? undefined : untriagedText(row.error, pull.source); | ||
| if (text !== undefined) untriaged.set(pullKey(pull), { ...pull, untriaged: text }); | ||
| if (text !== undefined && row.headSha === headOf(open, pull)) untriaged.set(pullKey(pull), { ...pull, untriaged: text }); |
There was a problem hiding this comment.
P2 · Open pull requests lose their untriaged note whenever their repository's listing failed or was refused
Why it is wrong: the note is kept only when row.headSha equals the head from the open-pulls listing. When a repository's listing errors or is refused, headsOf returns nothing and that repository is absent from open, so headOf returns undefined and every note for its open pull requests is dropped. The description only asks to stop closed pull requests from coming back through notes, so this goes further than intended. Previously an open pull request with a current-head failed run kept its note during a rate-limited listing.
Trigger: a repository's listing returns the error "rate limited" (or the member's host credential is refused with no kept listing), and an open pull request there has a failed run recorded on its current head. The note disappears until the listing recovers, even though the stored head is still current.
Suggested fix: when no listing is available for the repository, fall back to the last kept head or keep the note rather than dropping it, and only exclude notes when the listing positively shows the pull request is closed or the head differs. For example, keep the note when headOf(open, pull) is undefined and the repository's listing is in error, and add a test covering a rate-limited listing with a current-head failed run.
| const [latest, listed, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), listingOf(member, connected), deps.managerRuns(tenantId, [pull])]); | ||
| const notes = await notesOf(tenantId, [record], openHeads(listed, [record], [])); |
There was a problem hiding this comment.
P3 · Single pull request read now lists open pulls for every connected repository
Why it is wrong: the single pull request branch only needs the head of one pull request, but it now awaits listingOf(member, connected), which may request open pulls for every connected repository on each read. That adds host API cost and latency and can hit rate limits. This only matters if listingOf is not cached across requests.
Trigger: a GET with ?repo=&number= in a workspace with many connected repositories while the listing cache is cold or expired.
Suggested fix: fetch the head for just the requested repository, or confirm that listingOf is cached per member and reused here. For example, query the single pull request's head directly instead of the full listing, or add a test or note confirming the cache covers this path.
A failure note shows only when its head sha is the pull request's current head. The list compares it with the open-pulls listing; a repository whose listing failed keeps its notes, but a note alone never lists a pull request there, so closed pull requests no longer come back. The single pull request read asks the host for that one pull request's head instead of listing every repository.
f9e1648 to
c2a7895
Compare
There was a problem hiding this comment.
Shows a "Couldn't triage" note only when it matches the pull request's current head, comparing against the open-pulls listing and asking the host for a single pull's head on the single-pull read. The main concern is that the single-pull route now makes a host API call on every request, even when no note needs checking, and an open pull with an empty head sha can have its note wrongly dropped.
| if (!record) return failure(404, "not_found", "That repository is not connected."); | ||
| const pull = { repo: query.repo, number: query.number, source: sourceOf(record) }; | ||
| const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record]), deps.managerRuns(tenantId, [pull])]); | ||
| const [latest, head, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), headOn(tenantId, record, pull.number), deps.managerRuns(tenantId, [pull])]); |
There was a problem hiding this comment.
P2 · Single-pull read calls the host for every request, even when there is no note to check
Why it is wrong: headOn is awaited unconditionally in the single-pull route, so every poll of one pull request now costs a host API read. The head is only needed to decide whether a stored untriaged note is still current, so the call is wasted when the pull has no note. For Bitbucket, getPr also fetches mergeability, diff stats and the author association, which adds rate-limit pressure on a route that previously made no host call.
Trigger: the portal polls GET /triage-verdicts?repo=...&number=... for a pull request that has a verdict or no note. Each poll now runs openHeadFor against the host.
Fix: only resolve the head when a note exists. For example:
const latest = await deps.verdicts.latestFor(tenantId, pull);
const head = latest?.untriaged ? await headOn(tenantId, record, pull.number) : undefined;
| try { | ||
| const read = await deps.openHeadFor(tenantId); | ||
| if (!read) return undefined; | ||
| return (await read(record, number))?.headSha ?? null; |
There was a problem hiding this comment.
P3 · Open pull request with no head sha is reported as closed, dropping its note
Why it is wrong: openHeadOf in tenant-open-heads returns undefined both for a closed pull request and for an open one whose sha is empty. headOn maps undefined to null, and the single-pull path treats null as closed and drops the note. The host never said the pull is closed, so the note should be kept.
Trigger: the host returns an open pull request with an empty or missing head sha on the single-pull read. The note for that pull request is removed.
Fix: distinguish "closed" from "unknown" in the return value, and keep the note when the host did not confirm the pull is closed. For example, return undefined for unknown and a distinct value for closed, rather than collapsing both to null:
return (await read(record, number))?.headSha ?? undefined;
and make the closed case explicit in openHeadOf.
…the host gives no sha
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Ready
Triage: Waiting for CI
labels
There was a problem hiding this comment.
Limits the "Couldn't triage" note to the pull request's current head, keeps notes when a repository listing fails, and makes the single-pull read ask the host for only that pull's head. The main defect is that the single-pull read keeps a note the list view drops for the same pull request, so the two views disagree; a second issue is an uncached host call on every poll of a pull with a note.
| const key = pullKey(pull); | ||
| const note = notes.untriaged.get(key); | ||
| const head = note === undefined ? undefined : await headOn(tenantId, record, pull.number); | ||
| const untriaged = new Map(note !== undefined && (head === undefined || head === note.headSha) ? [[key, note]] : []); |
There was a problem hiding this comment.
P2 · Single-pull read keeps a note the list drops for the same pull request
The list view drops a note when the pull request is missing from the open listing, and openHeadsOf/openHeadOf also skip open pulls that have no sha. The single read treats an open pull with no head (headOn returns undefined) as a reason to keep the note. So the same stored note appears on the single read and disappears from the list.
Trigger: a note exists for a pull the host reports as open without a usable sha. The list omits it, but GET ?repo=...&number=... shows "Couldn't triage" for it. The test currently pins this behaviour (#14 is absent from the list but kept on the single read).
Fix: make both paths use the same rule. Either drop the note on the single read when the head is unknown, or keep it in the list too. For example:
const untriaged = new Map(note !== undefined && head !== undefined && head === note.headSha ? [[key, note]] : []);
then update the test to match.
| const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record], () => true), deps.managerRuns(tenantId, [pull])]); | ||
| const key = pullKey(pull); | ||
| const note = notes.untriaged.get(key); | ||
| const head = note === undefined ? undefined : await headOn(tenantId, record, pull.number); |
There was a problem hiding this comment.
P2 · Uncached host API call on every single-pull read that has a note
headOn calls the host for the pull's head on every request to the single-pull route whenever a note exists. Nothing caches the result, and the call runs before the response is built, so a matching etag does not avoid it. A portal polling one pull request makes one host request per poll, which uses rate limit and adds latency.
Trigger: the portal repeatedly polls ?repo=X&number=N for a pull that has a stored "Couldn't triage" note.
Fix: reuse the existing open-head read or cache the head per pull for a short TTL, and only call the host when the cache misses. Check the etag before the host call if the etag can be derived from stored data.
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Ready
Triage: Waiting for CI
labels
There was a problem hiding this comment.
Limits the "Couldn't triage" note to pull requests whose current head matches the head the note was recorded for, so stale notes from earlier pushes no longer appear. Notes are kept when a repository listing fails, and a single pull-request read is meant to ask the host only for that pull request's head. The single-read path currently lists every connected repository instead, which is the main defect.
| const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record], () => true), deps.managerRuns(tenantId, [pull])]); | ||
| const key = pullKey(pull); | ||
| const note = notes.untriaged.get(key); | ||
| const current = note !== undefined && noteCurrent(openHeads(await listingOf(member, connected), [record], []), note, note.headSha); |
There was a problem hiding this comment.
P2 · Single read lists every connected repository instead of asking for one PR's head
Why it is wrong: The description says a single pull-request read asks the host only for that pull request's head, through the existing open-head read. The code instead calls listingOf(member, connected), which lists open pulls across every connected repository. That makes a single-PR lookup as expensive as a full multi-repo listing, and it can miss the head if another repository's listing is slow or fails.
Triggering scenario: Fetch ?repo=X&number=N for a pull request that has a stored untriaged note, in a tenant with many connected repositories, outside the 30s cache window. Every such request triggers a full multi-repo host listing and counts against the rate limit.
Suggested fix: Use the existing open-head read for just this pull request, for example by passing repo and number to that read, and keep the note when the host cannot answer. Keep the multi-repo listing only for the list view, and add a test asserting that the single read requests only its own pull request's head.
A "Couldn't triage" note now shows only when it was recorded for the pull request's current head. The list compares the note with the open-pulls listing, so a note from an earlier head no longer shows after a new push. When a repository's listing failed, its notes are kept, but a note alone never lists a pull request there, so closed pull requests no longer come back. The single pull request read asks the host for that one pull request's head, through the hub's existing open-head read, instead of listing every repository; when the host can't answer, the note is kept. No docs needed a change: the user manual already describes the note on open pull requests only.
Verified with
bun run check,bun run lintandbun --conditions=intx-src test apps/hub/src/github/triage-verdictsagainst local Postgres (5 pass). The test covers a failed run on a pushed-over head, a failed run on a closed pull request in a repository GitHub could not list, a failed run on an open pull request with a manager's run in that repository, and a single read that asks the host only for its own pull request's head. Each new assertion fails on the code before it and passes here.Closes CL-11592