Repository navigation
Conversation
88aad59 to
1088c21
Compare
…t gh, read nothing while composing, name the state icon Addresses the four P2 findings of the review of 1088c21 on PR #1790. A displayed link ending in `/pull/2147483648` parsed as a pull request, so the store admitted it to the shared `gh` query, where `$pN: Int!` cannot hold it. GitHub rejected the whole request with no data, so every pull request shown alongside stayed unknown or stale, and the batch failed the same way a minute later for as long as the link was mounted. `parseGitHubReference` now requires a number a signed 32-bit integer can hold, which is how GitHub numbers them, so such a URL is no reference: Links keeps it as a plain GitHub link, the store never sees it and the panel does not claim it. The desktop host always supplies `gh`, so without a sign-in every poll throws `GhUnavailable` and falls to the public API. Once `publicUntil` lapsed, `readsPublic()` was false at the top of the poll, so the hold guard was skipped, and the fallback read publicly without looking at `publicHeldUntil`: a 403 with a reset an hour out produced a refused fetch every five minutes. The fallback now checks the hold after `gh` throws and reads nothing under it. `gh` is still tried every five minutes, so a later sign-in is picked up, and the public API only once GitHub says it resets. The five-minute public cadence while `gh` is unavailable is kept: that poll is what retries `gh`, and the hold now bounds what it can spend. Composer decorations and previews render links through `MessageLink` with `interactive={false}`, but a renderer component knows only its URL, so typing `/pull/1790` watched `/pull/1`, `/17`, `/179` and `/1790` in turn, spending a public read on each prefix, or with `gh` restarting the batch per keystroke. `LinkInteractiveContext` joins the host's other link contexts in `LinkLabelContext.ts`; `MessageLink` provides its `interactive` prop through it, and `PullLink` shows what the store already knows without watching when it is false. The `LinkRenderer` contract is unchanged. The state was a `title` on a generic span around an `aria-hidden` glyph, so accessibility snapshots held only the link's name and URL. The span is now `role="img"` named by `describePull`, as the status display is, so the tree shows `img "Pull request, merged"` inside the link, while a labelled link keeps its authored name from the anchor's `aria-label`. Tests: a number past Int is no reference, and its neighbours are read in one query without it; a refusal while `gh` is unavailable holds the public API until the reset while `gh` is retried, telling no one; a non-interactive `PullLink` shows cached state without watching, and a composed `MessageLink` with both renderers spends no read; the state is found by role and name through every transition while the link keeps its authored name. Each fails on the previous code: seven refused fetches in 30 minutes, the invalid number in the query's variables, a watched decoration, and no `img` in the tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
f795f21 to
5a091da
Compare
…t gh, read nothing while composing, name the state icon Addresses the four P2 findings of the review of 1088c21 on PR #1790. A displayed link ending in `/pull/2147483648` parsed as a pull request, so the store admitted it to the shared `gh` query, where `$pN: Int!` cannot hold it. GitHub rejected the whole request with no data, so every pull request shown alongside stayed unknown or stale, and the batch failed the same way a minute later for as long as the link was mounted. `parseGitHubReference` now requires a number a signed 32-bit integer can hold, which is how GitHub numbers them, so such a URL is no reference: Links keeps it as a plain GitHub link, the store never sees it and the panel does not claim it. The desktop host always supplies `gh`, so without a sign-in every poll throws `GhUnavailable` and falls to the public API. Once `publicUntil` lapsed, `readsPublic()` was false at the top of the poll, so the hold guard was skipped, and the fallback read publicly without looking at `publicHeldUntil`: a 403 with a reset an hour out produced a refused fetch every five minutes. The fallback now checks the hold after `gh` throws and reads nothing under it. `gh` is still tried every five minutes, so a later sign-in is picked up, and the public API only once GitHub says it resets. The five-minute public cadence while `gh` is unavailable is kept: that poll is what retries `gh`, and the hold now bounds what it can spend. Composer decorations and previews render links through `MessageLink` with `interactive={false}`, but a renderer component knows only its URL, so typing `/pull/1790` watched `/pull/1`, `/17`, `/179` and `/1790` in turn, spending a public read on each prefix, or with `gh` restarting the batch per keystroke. `LinkInteractiveContext` joins the host's other link contexts in `LinkLabelContext.ts`; `MessageLink` provides its `interactive` prop through it, and `PullLink` shows what the store already knows without watching when it is false. The `LinkRenderer` contract is unchanged. The state was a `title` on a generic span around an `aria-hidden` glyph, so accessibility snapshots held only the link's name and URL. The span is now `role="img"` named by `describePull`, as the status display is, so the tree shows `img "Pull request, merged"` inside the link, while a labelled link keeps its authored name from the anchor's `aria-label`. Tests: a number past Int is no reference, and its neighbours are read in one query without it; a refusal while `gh` is unavailable holds the public API until the reset while `gh` is retried, telling no one; a non-interactive `PullLink` shows cached state without watching, and a composed `MessageLink` with both renderers spends no read; the state is found by role and name through every transition while the link keeps its authored name. Each fails on the previous code: seven refused fetches in 30 minutes, the invalid number in the query's variables, a watched decoration, and no `img` in the tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…nd expect the inline read Addresses the two branch defects in the CI notes on #1790, which failed ten browser journeys on both engines in every run. 5a091da gave the state glyph `role="img"` named by `describePull`, inside the host's anchor. A link shown as its URL has no `aria-label`, so its name is computed from its contents and became "Pull request, openhttps://…": no separator, and a name that changed from "Pull request" to "…open…" to "…merged…" as the state was read. Screen readers announced it run together, and `layout.spec.mjs`'s exact-name lookup no longer found the link. The state now describes the link instead. `LinkDescriptionContext` joins the host's link contexts in `LinkLabelContext.ts`. `MessageLink` provides a setter for its live anchor's `aria-description`, the attribute it already sets for an unavailable chat. `PullLink` sets the state's words while shown and clears them on unmount. The glyph's span is `aria-hidden` again, keeping its `title` tooltip, so the name stays the authored label or the URL whatever the state. The `LinkRenderer` contract is unchanged, and a composer decoration has no anchor to describe. The inline link's state read made the panel's GitHub journeys count one more `pulls/N` request than they expected. In the API-error journey, that read hit the fixture's 403 and logged an unexpected console error. These journeys now expect it. Before opening the panel, each waits for the inline read: the two checklist journeys on the link's description "Pull request, open", the error journey on the refused response. Then they expect `pulls/N` twice, or allow the 403 twice. After the refusal, the store holds public reads until GitHub's reset, so the retry adds no third read. Tests: the unit test of the states expects each one's words as the description and none once unmounted; a labelled link keeps its name with the state as its description; and a link shown as its URL is named by the URL alone before and after it is read as merged. All three fail on the previous `PullLink`. `layout.spec.mjs`, `external-links.spec.mjs` and `github-checklist.spec.mjs` pass on Chromium and WebKit (52 cases). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes’s GitHub account.
Thanks for this. It reads cleanly. The URL→gh boundary is sound: exact github.com host, bounded Int32 numbers, and owner/repo/number passed as GraphQL variables. Kalvin's four earlier findings hold at 2a4d563. CI is green.
Recommend fixing before merge (comment only; humans decide):
- Without
gh, pull request states stop updating after the first read (pull-states.ts:177) - The selected Solo row's icon loses its selected-row colour (
GitHub.module.css:404) - At Buzzin', state icons vanish inside neutral insets in your own bubble (
Messages.module.css:3042)
Should fix: without gh, links that keep failing can permanently starve one that has never been read (pull-states.ts:269).
Optional: a doc mismatch on renderer failure, and the glyph shown for failed.
Validation gaps: Mongo's two store findings were reproduced with focused Vitest probes (fake timers, mocked fetch). The CSS findings were traced from source; no in-app check in either theme at Buzzin'. The PR's own human in-app check is still pending.
…t unread first, and keep state icons out of insets Addresses the review of 2a4d563 on PR #1790. Without `gh`, a successful public read set every state's next read to never, so an open, draft or closed pull request kept the state it was first read in for the session: neither polling, returning to the window nor a remount read it again. On `main`, the Solo sidebar read non-merged pull requests publicly every five minutes. Only merged is now settled. Every other public state is read again at `PULL_SLOW_REFRESH_MS`, which is what the store already did while `gh` was unavailable. The 403/429 hold bounds what that spends. A public poll read the first ten due URLs in the order they were first watched. URLs that kept failing, such as private repositories read without `gh`, came round every five minutes and took the slots back. A URL never read was left over each time and moved a minute on, so fifty failing links kept the fifty-first unread indefinitely. Ordering by due time, as the review suggested, does not help, because that minute moves a left-over URL level with or behind the failures it lost to; the new test fails that way too. The store now records when each URL was last read and takes the longest unread first, never read before all. With `gh` the batch is read whole, so only the aliases' order changes. The Solo sidebar's quiet icon read `--icon-subtle`. That role is `var(--text-subtle)` computed at `:root`, so `.navigation-item`'s hover, highlight and selected rebinds of `--text-subtle` no longer reached it, and a selected row showed root grey on the selected fill. `.soloPull` reads `--text-subtle` again. Only a message link's `.pullState` uses `--icon-subtle`. The own bubble rebound the shared icon roles for everything inside it, including neutral insets. At Buzzin', a pull request glyph inside a table or revealed spoiler in your own message took the accent ink on `--surface-panel`: `#fff` on `#fff` in light magenta, iris, cobalt and mono, and `#000` on `#1a1a1a` in dark bee, citrus and mono. This now follows the `--mention-*` pattern. The bubble sets `--message-icon-subtle`, `--message-icon-success` and `--message-icon-categorical-purple`. Both neutral inset rules reset them to `initial`, as they reset mentions. `.pullState` reads each one with the shared role as its fallback. Docs: `plugin-architecture.md` describes the two fallbacks separately: Links when the plugin is disabled, a plain link when the renderer fails. The plugin README makes no such claim. `design-system.md` names the bubble's variables and the inset reset. An unavailable pull request still shows the plain pull request glyph, greyed. Tabler, the only allowed source, has no unavailable variant: its pull request family is open, closed, draft and conflict. The neutral glyphs already exported each mean something else here or elsewhere: circle-dashed is draft, x-circle is closed, and help-circle means help. A new glyph meaning is a designer call under AGENTS.md. The state stays in the link's `aria-description` and its `title`. Tests: the store reads a public pull request again every five minutes through draft and closed, then never once merged. Twelve public links are read again ten and then two at a time. A never-read link behind fifty failing ones is read within five minutes. All three fail on the previous store, and the last also fails with only the first fix or with due-time ordering. `GitHub.module.test.ts` reads the stylesheets, since jsdom does not resolve `var()`. It checks that the sidebar's quiet colour reads only roles that `.navigation-item` rebinds. It also checks that each neutral inset resets every icon variable the own bubble hands down, and that `.pullState` reads each with its shared role as fallback. Both fail on the previous CSS, and the second still fails with only the sidebar fix. The contrast guard already measures the base roles on `--surface-panel`, which is what the inset now shows. It reads only `tokens.css`, so it cannot see the module cascade. A Chromium and WebKit probe of the real stylesheets confirmed the computed colours. At high/magenta/light and high/bee/dark, an own bubble's table and revealed-spoiler icons take the shared green, grey and purple, where they were `#fff` and `#000` before. A selected Solo row's quiet icon takes the row's selected ink, where it was the root grey `rgb(82, 82, 82)` before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One public polling quota issue remains.
…s hourly limit Addresses the review of 128bd1c on PR #1790. Without `gh`, 128bd1c read every non-merged pull request from the public API every five minutes. Six shown pull requests cost 72 reads an hour and ten cost 120, against the 60 GitHub allows an address, before the panel read anything. The 403/429 hold only stopped reads once the limit was spent. Then the links stopped updating until the reset, and an opened panel reported the limit. Public reads now draw on a budget the store keeps: `PUBLIC_READS_PER_POLL` (10) can be spent at once, as on a channel of links first shown, and it refills at `PUBLIC_READS_PER_HOUR` (30). However many pull requests are watched, first reads included, the store spends at most 40 public reads in any hour, so the panel keeps at least 20. The pull requests a poll cannot afford wait a minute, as left-overs already did. They still go longest unread first, so the budget rotates through every shown pull request. Each is still read at most every five minutes, and `gh` reads are unchanged. Tests: twelve public pull requests watched for two hours cost ten reads at once, then 30 an hour, and the last twelve reads are one of each. Under 128bd1c the first hour cost about 140. The never-read-first test now watches thirteen failing links, not fifty. That is enough for some to be unread when the first come round again. It shows a readable link is read before any failing one is tried again. Fifty would take over an hour to read within the budget. Watch-order selection fails it. Not run locally at the author's request; left to CI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account: no further changes requested. The prior polling, fairness and icon-color findings are addressed, and the public-read budget is bounded; Rocket, Mantis and Groot independently reviewed the follow-up, with no fix-introduced defects found. Reviewed head c7e8bf3691f38abd3227cc003b91de91a1caad28 against base 49fac0341e6ffeb1b9cae841d5ed8d96a6734aad, following the prior review. Source-only: tests and app flows were not run, CI was not assessed, and human visual acceptance remains pending.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One new quota-accounting defect remains after the steady-state polling fix.
…a clock set back Addresses the review of c7e8bf3 on PR #1790. The public read budget refilled by the time since it was last counted, as a fraction of a read. With the budget spent, a clock corrected backwards made that fraction negative, and `Math.floor(-0.5)` is -1, so `slice(0, -1)` read every due pull request but the last: seventy links shown after a one-minute correction cost 69 reads at once. Reads are now earned whole, one for every `PUBLIC_READ_EARNED_MS` (two minutes) since the budget was last earned to, and that time moves on by the whole steps earned. The count stays an integer, so no fraction drifts across an hour of polls. A clock set back earns nothing. Its time is earned again once the clock passes where it was. The cadence is unchanged: one read every two minutes once the first ten are spent. Tests: with the first ten spent and the clock a minute back, three links shown read nothing, and are read, with the two left over, one every two minutes once the clock is past where it was. On c7e8bf3 they cost two reads at once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account: no further changes requested. This fixes the backward-clock over-read missed in my previous review; Rocket, Mantis and Groot independently reviewed the follow-up, with no fix-introduced defects found. Reviewed head 046db51b05c94e073acc4cd93ffff820d79be3c0 against base 49fac0341e6ffeb1b9cae841d5ed8d96a6734aad, following the prior review. Source-only: tests and app flows were not run, CI was not assessed, and human visual acceptance remains pending.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One remaining public polling fairness defect; the clock-rollback quota burst is fixed.
…ages GitHub pull request links in messages rendered the Links plugin's static octocat. The GitHub plugin now contributes a `LinkRenderer` (order -1, below Links' default band) for `kind === "pull"` URLs that reuses Links' `LinkLabel` with the pull request's state glyph: open (success), draft and closed (subtle), merged (categorical purple). The plain pull request glyph shows until the state is read, and when it cannot be, so the icon never swaps. Repositories, issues and commits stay with Links, which also stands in when the plugin is disabled or the renderer fails. The batched, visibility-aware state poller moves out of `createSoloPulls` into a URL-keyed `createPullStates(gh)` store shared by every session's sidebar and every inline link, so one pull request shown in both costs one read. Merged stays terminal, closed is read at the slow five-minute cadence, and without `gh` each pull request is read from the public API once. The state icons, words and tone move to `pull-state-view.ts` so the sidebar, panel and inline link agree; closed stays grey on all three. `LinkLabel` gains an optional `icon` override. Docs and the plugin README note the renderer and that Links remains the fallback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…r poll Addresses the review of 4a1f445f3 on the shared pull request state store. A shared `gh` read that threw with no usable data (rate limited, a non-zero exit, the 30 s deadline) recorded nothing, so every URL in the batch stayed due and `finally → later()` armed a 0 ms timer: a spawn-and-fail loop while GitHub was already refusing. The catch now moves every due URL's next read out a minute, keeping what is shown. Without `gh`, every outcome was read once, so a public read that failed for a passing reason (offline at launch, a shared NAT's 403) marked the link unavailable for good. A successful public read is still read once; a failed one is tried again at the slow five-minute cadence until it is read. `refresh` aborted any read in flight, and inline links mount per virtualised row, so scrolling through a channel of PR links restarted the batch on every newly mounted one and spent a rate-limit point on each discarded query. A read in flight is now left to finish; its `finally → later()` picks up what became due meanwhile at once. Aborting is reserved for the last unsubscribe. Pull requests GitHub could not read were retried as one `gh` process each, concurrently, every five minutes. They now stay in the one aliased query with the rest: `readStates` already takes the NOT_FOUND aliases from the error's partial data, so the retry costs no extra process. Only the cadence distinguishes them. Tests cover each: a thrown shared read waits a minute and does not spin, a failed public read is retried on the slow cadence then read once, watching a second URL mid-read neither aborts the first nor delays the second, two unreadable URLs retry inside the shared query, and a hidden window re-arms without reading. Each behaviour test fails on the previous store. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…watched sidebar views Addresses the review of a659ce7a8 on the shared pull request state store. The store keyed each entry on the URL it was given and read the number off its last path segment, but `parseGitHubReference` accepts a query string and a trailing slash, which inline links pass through as pasted. `…/pull/123?x` gave `Number("123?x")`, `NaN`, serialised as `null` for `$pN: Int!`; GitHub rejected the whole query with no data, so every due URL in the batch backed off together and failed the same way a minute later, leaving every PR link in the channel on the plain glyph while that one was shown. `…/pull/123/` gave number 0, NOT_FOUND, and an unavailable icon for a pull request that exists. `watch` and `get` now key on the pull request's canonical URL from `parseGitHubReference`, so the variants of one pull request, and an inline link and the sidebar's normalised link to it, share one entry and one read, and `readStates` only ever sees canonical URLs. A URL that is not a pull request's is not watched and reads as `"failed"` rather than throwing. Re-watching a chat whose link was already known called `follow` but kept the cached `views` entry. The shared store can advance a URL while that chat is unwatched, kept current by another chat or an inline link, and notifies only its current subscribers, so on remount `useSyncExternalStore` read the stale view. Pollable states healed within a minute, but merged is terminal and never re-read, so the sidebar showed open for a merged pull request until a re-read on returning to the window. `follow` now drops the cached view, so `get` reads the store afresh. Tests: a query-string and a trailing-slash variant resolve, cost one query with the canonical numbers alongside a plain URL, and share one state object; a non-pull URL is neither read nor thrown on; a chat re-watched after the store moved to merged shows merged. Each fails on the previous code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses the optional finding of the review of 7aedd9123. `notify()` fired every listener of every watched URL after each poll, not just those whose state the poll recorded. Inline `PullLink`s were unaffected, since `get` returns a stable reference and React bails out, but each Solo sidebar listener drops its cached view on notify, so every shown Solo row with a link rebuilt its view and re-rendered once a minute, including rows whose pull request is merged and was never in the batch. `notify` now takes the `due` URLs and tells only their listeners. The thrown-batch back-off records no state and so, as before, notifies nobody; the public fallback reads the same `due` URLs and notifies them. The first store test, which already polls only the open pull request on its second tick, now checks that the open URL's two listeners hear of it and the merged URL's does not. It fails on the previous store. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…roles The open glyph of a pull request link in a message was painted `--text-success`, a step-12 ink chosen for reading text on fills. Inside a bubble whose body and link inks are also step 12 it clears the bubble at 10:1 but the neighbouring ink at only 1.1–1.5:1, so the eye reads a dark mark, not green; merged read only because purple's hue is far from olive, which does not hold for every accent. The cascade was correct; the role was the wrong tier. Two icon-tier roles join `tokens.css` and the registry, next to `--icon-status-complete` and `--text-categorical-purple`: `--icon-success` (green-11 in both modes) and `--icon-categorical-purple` (purple-11 in both modes). The sidebar's and the inline link's open and merged icons move to them; the panel's chip keeps the text-tier roles, since there green and purple text sits on a tinted fill and needs 4.5:1. Draft and closed stay `--text-subtle`. Measured against the own bubble at the tinted levels across all six accents: green-11 3.89–3.99:1 light and 7.48–7.62:1 dark (Citrus 3.99 / 7.48), purple-11 5.01–5.15:1 light and 6.67–6.79:1 dark (Citrus 5.15 / 6.67); received bubble 4.25 / 7.76 and 5.49 / 6.91; page surfaces 4.33–4.72 / 7.86–11.2 and 5.58–6.08 / 7.01–9.98. Dark green-10 would also clear (4.92–5.27) but green-11 matches `--status-online` and keeps one step in both modes; dark purple-10 clears by only 3.01–3.22, so purple stays at 11. The contrast guard now measures both roles against the page surfaces and, per accent family, against both message bubbles at 3:1. On the high colour level the own bubble is the solid accent step and no single hue clears 3:1 on every family, a gap the previous text roles shared; the guard names it and skips that one pairing rather than hiding it in an exception. Tests move each role one step thinner (green-10, purple-9): both still clear the page, so only the new bubble pairings fail them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ink ink Addresses the review of d077059d4 on the pull request state icons. On the high colour level the own bubble is the solid accent step, and no green or purple clears it on every family: `--icon-success` and `--icon-categorical-purple` measured 1.05–1.69:1 there, so a pull request's state glyph was effectively invisible in your own messages. The guard knew and skipped that one pairing rather than enforcing it. The high level is expressed in `tokens.css` as a global block that swaps the `-own` roles, not an own-bubble-scoped block, so redefining the icon roles there would also repaint received bubbles and the Solo sidebar. The fix follows the existing `-own` pair pattern instead. `--icon-success-own` and `--icon-categorical-purple-own` join the role layer and the registry, pointing at their base role on the neutral and touch levels and at `--text-message-link-own` in both high blocks. `.messageBubble[data-own]` hands the pair to whatever sits inside it by shadowing the two roles, as it already does for links, mentions and selection, so `GitHub.module.css` keeps no host knowledge and the pair covers any state icon rendered in an own bubble. On the high own bubble both roles now measure the link ink's floor: magenta and cobalt 4.50:1, iris 5.04:1, bee 12.88:1, citrus 13.27:1 and mono 17.40:1 light / 16.83:1 dark, with every other family identical in both modes since the accent-9 fill and its ink do not change with the mode. The contrast guard drops the `SOLID_OWN_BUBBLE_LEVEL` skip and measures each base role on the received bubble and its `-own` pair on the own bubble at every level. The test's framing is corrected: purple-9 is two steps thinner, since purple-10 passes the whole guard at 3.01–3.22:1 on the dark bubbles, and purple-9 also fails the dark popover, which the pinned received-bubble pairing does not depend on. A third case points the high-level `-own` pair back at the base role and expects the own-bubble pairing to fail on the high level, so the previously skipped pairing is shown to be guarded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…h the link ink Addresses item 1 of the rendered check of de5abe4a2 on the pull request state icons. Draft and closed pull request glyphs were painted `--text-subtle`, a text role the own bubble does not pair, so on the high colour level, where that bubble is the solid accent step, the grey measured 1.04–2.98:1 on most families in both modes, 1.07:1 on citrus in dark: the glyph of a draft or closed pull request in your own message was effectively invisible while the open and merged glyphs beside it had already moved to their `-own` pair in de5abe4a2. The guard did not measure these two states at all. The fix is the same pair, one tier down. `--icon-subtle`, pointing at `--text-subtle`, and `--icon-subtle-own` join the role layer and the registry next to the two existing icon pairs; the `-own` role is its base on the neutral and touch levels and `--text-message-link-own` in both high blocks. `.messageBubble[data-own]` shadows it alongside the other two, so `GitHub.module.css` only switches the draft/closed/unknown default of `.soloPull` and `.pullState` to `--icon-subtle`; the panel chip's text and every other supporting text keep `--text-subtle`, which is not shadowed for the bubble as a whole. On the high own bubble the role now measures the link ink's floor: magenta and cobalt 4.50:1, iris 5.04:1, bee 12.88:1, citrus 13.27:1 and mono 17.40:1 light / 16.83:1 dark, identical in both modes elsewhere. On the tinted levels it measures 6.44 light / 8.03 dark at neutral and 6.44–6.61 light / 8.30–8.46 dark at touch across the six families, and the base role measures 7.05 light / 8.61 dark on the received bubble. The contrast guard adds `--icon-subtle` to the message icon roles, so it is measured on the page surfaces and the received bubble and its `-own` pair on the own bubble at every family and level. A fourth test case points the high-level pair back at the base role and expects the dark own-bubble pairing to fail; the previous guard passes that stylesheet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…read on window return Addresses the three ranked concerns of the refresh timing and performance assessment of 05be7a7 on the shared pull request state store. Without `gh`, every due pull request was read from the public API at once, one `fetch` each with no cap, against the 60 reads an hour GitHub allows an address. Thirty links in view spent thirty at once; a second channel ran out the hour, after which each failed URL was tried again every five minutes and refused again, starving the panel's public reads too. A public poll now reads at most `PUBLIC_READS_PER_POLL` (10) of the due URLs, in the order they were shown, and moves the rest a minute on rather than leaving them due. `loadGitHubDetails` throws a typed `GitHubRateLimited` for a 403 or 429, with the same message the panel shows, carrying GitHub's `x-ratelimit-reset` or an hour on without it; the store catches it, records nothing for the batch, and holds every public read until that time, as it already holds a thrown `gh` batch for a minute. A pull request shown during the hold waits for it too. With `gh`, the hold ends at the earlier of the reset and `gh`'s next try, so a signed-in `gh` is not kept waiting an hour behind a public refusal. Each poll stored a fresh state object even when nothing changed, so every mounted open or draft link re-rendered once a minute and each Solo sidebar row with a pollable link dropped and rebuilt its view. An unchanged state, with the same `state`, `checks` and `conflicts`, now keeps its object, and only the URLs whose state changed are notified, so `useSyncExternalStore` bails out and the sidebar keeps its view. Returning to the window after a long hidden stretch waited for the hidden tick's re-arm, up to a minute, before reading what had become due. The store now listens for `visibilitychange` while anything is watched and runs the scheduler on return: anything due is read at once, and otherwise the poll keeps its time. The hidden window still looks again later rather than reading. Tests: twelve public links cost ten reads, then the other two a minute later, each once; a refusal with and without a reset header holds every public read, including a link shown meanwhile, until the reset or an hour, telling no one and showing nothing as unavailable; a refusal does not keep `gh` waiting past its retry; a poll reading the same state keeps the object and tells no one, and the sidebar row's view and listener are left alone; coming back after two hidden minutes reads at once, after half a minute with nothing due it does not; the loader's limit error names the reset. The first store test now changes the checks between polls, since an unchanged poll is no longer heard, and the failed-public-read test uses a 502, since a 403 is now a hold. Each new behaviour test fails on the previous store or loader, except the `gh` retry one, which pins the new hold's bound. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…t gh, read nothing while composing, name the state icon Addresses the four P2 findings of the review of 1088c21 on PR #1790. A displayed link ending in `/pull/2147483648` parsed as a pull request, so the store admitted it to the shared `gh` query, where `$pN: Int!` cannot hold it. GitHub rejected the whole request with no data, so every pull request shown alongside stayed unknown or stale, and the batch failed the same way a minute later for as long as the link was mounted. `parseGitHubReference` now requires a number a signed 32-bit integer can hold, which is how GitHub numbers them, so such a URL is no reference: Links keeps it as a plain GitHub link, the store never sees it and the panel does not claim it. The desktop host always supplies `gh`, so without a sign-in every poll throws `GhUnavailable` and falls to the public API. Once `publicUntil` lapsed, `readsPublic()` was false at the top of the poll, so the hold guard was skipped, and the fallback read publicly without looking at `publicHeldUntil`: a 403 with a reset an hour out produced a refused fetch every five minutes. The fallback now checks the hold after `gh` throws and reads nothing under it. `gh` is still tried every five minutes, so a later sign-in is picked up, and the public API only once GitHub says it resets. The five-minute public cadence while `gh` is unavailable is kept: that poll is what retries `gh`, and the hold now bounds what it can spend. Composer decorations and previews render links through `MessageLink` with `interactive={false}`, but a renderer component knows only its URL, so typing `/pull/1790` watched `/pull/1`, `/17`, `/179` and `/1790` in turn, spending a public read on each prefix, or with `gh` restarting the batch per keystroke. `LinkInteractiveContext` joins the host's other link contexts in `LinkLabelContext.ts`; `MessageLink` provides its `interactive` prop through it, and `PullLink` shows what the store already knows without watching when it is false. The `LinkRenderer` contract is unchanged. The state was a `title` on a generic span around an `aria-hidden` glyph, so accessibility snapshots held only the link's name and URL. The span is now `role="img"` named by `describePull`, as the status display is, so the tree shows `img "Pull request, merged"` inside the link, while a labelled link keeps its authored name from the anchor's `aria-label`. Tests: a number past Int is no reference, and its neighbours are read in one query without it; a refusal while `gh` is unavailable holds the public API until the reset while `gh` is retried, telling no one; a non-interactive `PullLink` shows cached state without watching, and a composed `MessageLink` with both renderers spends no read; the state is found by role and name through every transition while the link keeps its authored name. Each fails on the previous code: seven refused fetches in 30 minutes, the invalid number in the query's variables, a watched decoration, and no `img` in the tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…nd expect the inline read Addresses the two branch defects in the CI notes on #1790, which failed ten browser journeys on both engines in every run. 5a091da gave the state glyph `role="img"` named by `describePull`, inside the host's anchor. A link shown as its URL has no `aria-label`, so its name is computed from its contents and became "Pull request, openhttps://…": no separator, and a name that changed from "Pull request" to "…open…" to "…merged…" as the state was read. Screen readers announced it run together, and `layout.spec.mjs`'s exact-name lookup no longer found the link. The state now describes the link instead. `LinkDescriptionContext` joins the host's link contexts in `LinkLabelContext.ts`. `MessageLink` provides a setter for its live anchor's `aria-description`, the attribute it already sets for an unavailable chat. `PullLink` sets the state's words while shown and clears them on unmount. The glyph's span is `aria-hidden` again, keeping its `title` tooltip, so the name stays the authored label or the URL whatever the state. The `LinkRenderer` contract is unchanged, and a composer decoration has no anchor to describe. The inline link's state read made the panel's GitHub journeys count one more `pulls/N` request than they expected. In the API-error journey, that read hit the fixture's 403 and logged an unexpected console error. These journeys now expect it. Before opening the panel, each waits for the inline read: the two checklist journeys on the link's description "Pull request, open", the error journey on the refused response. Then they expect `pulls/N` twice, or allow the 403 twice. After the refusal, the store holds public reads until GitHub's reset, so the retry adds no third read. Tests: the unit test of the states expects each one's words as the description and none once unmounted; a labelled link keeps its name with the state as its description; and a link shown as its URL is named by the URL alone before and after it is read as merged. All three fail on the previous `PullLink`. `layout.spec.mjs`, `external-links.spec.mjs` and `github-checklist.spec.mjs` pass on Chromium and WebKit (52 cases). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…t unread first, and keep state icons out of insets Addresses the review of 2a4d563 on PR #1790. Without `gh`, a successful public read set every state's next read to never, so an open, draft or closed pull request kept the state it was first read in for the session: neither polling, returning to the window nor a remount read it again. On `main`, the Solo sidebar read non-merged pull requests publicly every five minutes. Only merged is now settled. Every other public state is read again at `PULL_SLOW_REFRESH_MS`, which is what the store already did while `gh` was unavailable. The 403/429 hold bounds what that spends. A public poll read the first ten due URLs in the order they were first watched. URLs that kept failing, such as private repositories read without `gh`, came round every five minutes and took the slots back. A URL never read was left over each time and moved a minute on, so fifty failing links kept the fifty-first unread indefinitely. Ordering by due time, as the review suggested, does not help, because that minute moves a left-over URL level with or behind the failures it lost to; the new test fails that way too. The store now records when each URL was last read and takes the longest unread first, never read before all. With `gh` the batch is read whole, so only the aliases' order changes. The Solo sidebar's quiet icon read `--icon-subtle`. That role is `var(--text-subtle)` computed at `:root`, so `.navigation-item`'s hover, highlight and selected rebinds of `--text-subtle` no longer reached it, and a selected row showed root grey on the selected fill. `.soloPull` reads `--text-subtle` again. Only a message link's `.pullState` uses `--icon-subtle`. The own bubble rebound the shared icon roles for everything inside it, including neutral insets. At Buzzin', a pull request glyph inside a table or revealed spoiler in your own message took the accent ink on `--surface-panel`: `#fff` on `#fff` in light magenta, iris, cobalt and mono, and `#000` on `#1a1a1a` in dark bee, citrus and mono. This now follows the `--mention-*` pattern. The bubble sets `--message-icon-subtle`, `--message-icon-success` and `--message-icon-categorical-purple`. Both neutral inset rules reset them to `initial`, as they reset mentions. `.pullState` reads each one with the shared role as its fallback. Docs: `plugin-architecture.md` describes the two fallbacks separately: Links when the plugin is disabled, a plain link when the renderer fails. The plugin README makes no such claim. `design-system.md` names the bubble's variables and the inset reset. An unavailable pull request still shows the plain pull request glyph, greyed. Tabler, the only allowed source, has no unavailable variant: its pull request family is open, closed, draft and conflict. The neutral glyphs already exported each mean something else here or elsewhere: circle-dashed is draft, x-circle is closed, and help-circle means help. A new glyph meaning is a designer call under AGENTS.md. The state stays in the link's `aria-description` and its `title`. Tests: the store reads a public pull request again every five minutes through draft and closed, then never once merged. Twelve public links are read again ten and then two at a time. A never-read link behind fifty failing ones is read within five minutes. All three fail on the previous store, and the last also fails with only the first fix or with due-time ordering. `GitHub.module.test.ts` reads the stylesheets, since jsdom does not resolve `var()`. It checks that the sidebar's quiet colour reads only roles that `.navigation-item` rebinds. It also checks that each neutral inset resets every icon variable the own bubble hands down, and that `.pullState` reads each with its shared role as fallback. Both fail on the previous CSS, and the second still fails with only the sidebar fix. The contrast guard already measures the base roles on `--surface-panel`, which is what the inset now shows. It reads only `tokens.css`, so it cannot see the module cascade. A Chromium and WebKit probe of the real stylesheets confirmed the computed colours. At high/magenta/light and high/bee/dark, an own bubble's table and revealed-spoiler icons take the shared green, grey and purple, where they were `#fff` and `#000` before. A selected Solo row's quiet icon takes the row's selected ink, where it was the root grey `rgb(82, 82, 82)` before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…s hourly limit Addresses the review of 128bd1c on PR #1790. Without `gh`, 128bd1c read every non-merged pull request from the public API every five minutes. Six shown pull requests cost 72 reads an hour and ten cost 120, against the 60 GitHub allows an address, before the panel read anything. The 403/429 hold only stopped reads once the limit was spent. Then the links stopped updating until the reset, and an opened panel reported the limit. Public reads now draw on a budget the store keeps: `PUBLIC_READS_PER_POLL` (10) can be spent at once, as on a channel of links first shown, and it refills at `PUBLIC_READS_PER_HOUR` (30). However many pull requests are watched, first reads included, the store spends at most 40 public reads in any hour, so the panel keeps at least 20. The pull requests a poll cannot afford wait a minute, as left-overs already did. They still go longest unread first, so the budget rotates through every shown pull request. Each is still read at most every five minutes, and `gh` reads are unchanged. Tests: twelve public pull requests watched for two hours cost ten reads at once, then 30 an hour, and the last twelve reads are one of each. Under 128bd1c the first hour cost about 140. The never-read-first test now watches thirteen failing links, not fifty. That is enough for some to be unread when the first come round again. It shows a readable link is read before any failing one is tried again. Fifty would take over an hour to read within the budget. Watch-order selection fails it. Not run locally at the author's request; left to CI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…a clock set back Addresses the review of c7e8bf3 on PR #1790. The public read budget refilled by the time since it was last counted, as a fraction of a read. With the budget spent, a clock corrected backwards made that fraction negative, and `Math.floor(-0.5)` is -1, so `slice(0, -1)` read every due pull request but the last: seventy links shown after a one-minute correction cost 69 reads at once. Reads are now earned whole, one for every `PUBLIC_READ_EARNED_MS` (two minutes) since the budget was last earned to, and that time moves on by the whole steps earned. The count stays an integer, so no fraction drifts across an hour of polls. A clock set back earns nothing. Its time is earned again once the clock passes where it was. The cadence is unchanged: one read every two minutes once the first ten are spent. Tests: with the first ten spent and the clock a minute back, three links shown read nothing, and are read, with the two left over, one every two minutes once the clock is past where it was. On c7e8bf3 they cost two reads at once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…, together Addresses the review of 046db51 on PR #1790. A public poll that could not afford a pull request moved its next read a minute on from that poll. Reads are earned every two minutes, so pull requests shown at different times kept separate one-minute phases, and whichever group polled first after each read was earned took it every time. Longest-unread ordering only compares the URLs of a single poll, so it never saw the other group. Twelve links shown, then three more half a minute later, read the first group over and over and never the second. In the clock-rollback test, it left a later link unread, failing in CI. Those a public poll cannot afford now wait for the next read earned. Every one waiting is in that poll, and the longest unread of them takes it, whenever it was shown. GitHub's hold and the hidden window's floor still apply through the scheduler. Each pull request is still read at most every five minutes, within the same 40-an-hour budget. Tests: twelve links shown, then three more half a minute later: within ten minutes all fifteen are read, using fifteen reads. On 046db51 the later three are never read. The clock-rollback test passes again. Both fail on the previous store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
046db51 to
72ec63f
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account: no further changes requested. The staggered-link starvation missed in my previous review is addressed by scheduling budget-blocked reads at the same credit boundary; Rocket, Mantis and Groot independently checked the follow-up, with no fix-introduced defects found. Reviewed head 72ec63f7364d53aae0d6c090a0b22f6a2c19f65c against base 8385e11f72b2671cf30f3007635320553760514a, following the fairness finding. Source-only: tests, app flows and CI were not verified; human visual acceptance remains pending.
Before / after
Icons keep a fixed 1em box, so lists stay aligned; on the high colour level the glyphs inside your own bubble take the link ink so they remain legible on the solid accent.
Summary
GitHub pull request links in messages now show the pull request's current state (open, draft, merged, closed) as the link's icon, in place of the generic GitHub icon.
order: -1, so it wins the tie with Links). Repositories, issues and commits stay with Links.LinkLabeltakes an optionaliconso the renderer reuses the Links label layout.pull-states.ts): one store, keyed by canonical URL, serves every session's Solo sidebar and every message link. A pull request shown in both places costs one read. The store polls only the URLs in view, sends oneghquery per poll, and backs off reads that fail.gh): each poll reads at most 10 pull requests. A 403/429 holds all public reads until GitHub's rate-limit reset. When the window becomes visible again, anything due is read immediately. An unchanged state keeps its object, so mounted links and sidebar rows don't re-render.solo-pull.tsnow uses the shared store instead of running its own polling.--icon-success,--icon-categorical-purple, new--icon-subtle). Each role has a new-ownpair, and.messageBubble[data-own]swaps in the pair. On the high colour level, where the own bubble is the solid accent, the glyphs use the link ink, so they stay legible. The contrast guard now measures these roles on the received and own bubbles at every family and level.PullLink,LinkLabelicon override and the contrast pairings.pages.integration.test.mjsnow expects the GitHub link renderer next to Links.Validation
🤖 Generated with Claude Code