feat(web): show assignee avatars on the board - #175
Conversation
storyOwner() picks the first assignee (GitHub's order, not sorted) and reports how many are left over, returning null when a story has no assignees so no call site can render an "unassigned" placeholder by accident. issue-row.tsx renders "@login" plus "+N" between the label chips and the relative-time stamp, and nothing when the story is unassigned.
Render the story's lead assignee beside the label chips in the story header, using the same @login (+N) grammar already used on the Backlog row. The header now reads story.assignees from StoryDetail, which previously arrived from the API and was dropped on the floor.
Adds ownedBy() and boardHref() as pure helpers in lib/pipeline.ts, and uses boardHref for all four board filter chips (all, stage, status clear, mine) instead of hand-built URL strings. The mine chip narrows each stage's stories to the signed-in viewer's GitHub login, composes with the existing stage/status filters, and only renders when the viewer has a GitHub login to match against. Stage chip counts and the active-open-stories subtitle now read from the mine-scoped stories so they never go stale relative to what's shown.
mineOn previously read straight from the mine=1 query param, so a viewer with no GitHub login (a key principal, or any user whose principal.githubLogin is unset) who arrived at ?mine=1 via a shared link, bookmark, or browser history landed on a board with every story filtered out by ownedBy(), no mine chip to undo it (it only renders when a login exists), and no other chip to recover with, since all four preserve mine. Lift the derivation into mineFilterOn(mine, login) in lib/pipeline.ts, which is false whenever the viewer has no login to match against regardless of the raw query param. The board now renders normally for such a viewer even with ?mine=1 in the URL, and every chip link emits a clean, mine-free href.
adrian-lorenzo
left a comment
adrian-lorenzo
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The background-image fallback handles a failed request, but it does not satisfy deployments that must not contact GitHub: every visible assignee still triggers an eager third-party request. It also removes the topbar’s existing referrerPolicy="no-referrer", so GitHub now receives the Facility deployment origin along with those avatar lookups. Please preserve the no-referrer guarantee and provide an operator-controlled way to disable remote avatars or proxy them without browser egress. Cover the enabled, disabled, and failed-load paths at the component boundary.
With those improvements, we can approve and merge it!
|
Thanks @adrian-lorenzo — both concerns are addressed in 5f9ac32, pushed to this branch. No more browser egress to GitHub at all. We went further than a disable switch: the browser-direct mode is gone. Avatars now load from this deployment's own origin via two new routes:
Operator control.
Stored principal avatar URLs in the topbar are rewritten onto the proxy only when they match a known GitHub shape ( Tests (
One honest note: the topbar's round→square change from the original description stands unchanged — that was a design-system decision independent of this review. |
|
One note on constraints, since it may matter for how you'd like this shaped: Avoiding new
Only the primitive's internals decide where the image tag lives. If you'd rather see a client-rendered image, we're happy to make that swap. For example, a small client component rendering an |
…ne filter A failed identity request used to be collapsed into 'viewer has no GitHub login', so ?mine=1 showed every story while removing the chip that could undo it. mineFilterState now returns off/on/blocked so the board can say the identity check failed rather than pretend the filter found nothing. Also adds the empty state the reviewer asked for: when the mine filter is active and nothing is assigned to the viewer, the board says so and links back to the unfiltered board instead of rendering every stage empty.
|
One correction to the trade-off picture in my last comment: the swap is not free. The current CSS-background primitive ships zero avatar-related JavaScript — it renders identically from server components, which is why the topbar and the story header (both server components) use it directly. Moving the image into a client
and in both cases ship avatar-handling JS to the browser that today isn't there. The failed-load behavior would be identical; the cost is bundle weight and a component-graph change at three call sites, not one file. Stating it so the choice is made with both sides visible: CSS-background keeps the current zero-JS property at the price of an unconventional technique; client- |
adrian-lorenzo
left a comment
adrian-lorenzo
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The proxy still has two security boundary gaps. /api/avatars/... is public, and every request triggers a fresh upstream fetch, so anyone who can reach the deployment can drive unbounded GitHub egress by cycling valid-looking logins or IDs. Please prevent unauthenticated requests from reaching the upstream and add bounded server-side caching or rate limiting.
Also, redirect: "follow" allows the upstream to redirect outside the two permitted GitHub hosts. I reproduced this with an upstream fake redirecting to a second server: the route followed it and returned that server’s bytes. Please validate and limit redirect hops, and add regression tests for both cases.
With those fixes, we can approve and merge it!
Review found two ways the /api/avatars routes let a caller drive this deployment's outbound traffic. Both are fixed here rather than in two commits because they share one route body and one test harness. The routes were public, and every request produced a fresh upstream fetch, so anyone who could reach the deployment could cycle valid-looking logins or IDs and make it call GitHub without limit. A request now reaches an upstream only when three things hold: it carries a session the control plane recognises, no cached answer already exists, and the viewer's fetch allowance is not spent. Cookie presence alone is not enough — the session is sealed, so only /v1/me can judge it — and a control plane that cannot answer denies rather than admits. Verdicts are cached per token, rejections included, so a replayed token cannot turn avatar loads into control-plane load either. Answers are cached server side: 512 entries, a day for image bytes, five minutes for a miss so an avatar-less login is not refetched on every paint, LRU eviction and a per-entry byte ceiling. Cache hits spend no allowance, so the per-viewer ceiling binds only on targets nobody has asked for. Both bounds are per web process, which the hardening checklist now says. redirect: "follow" let the upstream redirect anywhere: a fake upstream pointing at a second server had its bytes returned as an avatar. The fetch now follows hops by hand — each Location resolved against the URL that produced it, vetted against the two permitted GitHub hosts over https before it becomes a request, and the chain cut after three hops rather than run to whatever length the upstream chooses. Regression tests were checked by mutation rather than assumed: restoring follow-any-host fails the redirect cases, including the assertion that the second server receives nothing, and removing the session gate fails four cases. apps/web is at 133 tests, up from 88. Refs theam#175
The board named the assignee in text but did not show them. Add a shared Avatar primitive and use it on the story row, immediately left of @login. The image is painted as a CSS background rather than as an <img>. An <img> that fails to load makes every browser draw its own broken-image glyph over the letter beneath it, and alt="" does not suppress it; a background that fails to load paints nothing. So a deployment whose browsers cannot reach github.com falls back to the initial letter on its own, with nothing to configure. The avatar URL and the fallback letter are derived in apps/web, not in packages/ui, which keeps the primitive free of any knowledge of GitHub and puts the rules where there is a test runner to pin them. Refs theam#174
The same primitive as the story row, at 16px to sit with the header's larger type. The header is a server component and the row is a client component, so this is also what proves one primitive serves both. Refs theam#174
The topbar rendered an image when the principal had an avatar URL and nothing at all when it did not, so a user whose GitHub identity has no avatar saw an empty space. It now uses the same primitive as the board and falls back to the initial letter of the login, or of the email when there is no login. Two visible consequences, both deliberate: - The avatar is square rather than round. PillTag is documented as the only pill-shaped element in the design system, and the board avatars are square, so one shape now serves all three sites. - The image is no longer an <img>, so it can no longer carry referrerPolicy="no-referrer". Referrer policy belongs to the fetch initiator and CSS cannot set one; measured across Chromium, Firefox and WebKit, a pseudo-element, a child element, an inline style, an external stylesheet carrying referrerpolicy, and a custom-property indirection all send the origin. The avatar host therefore now learns the deployment's origin. The only alternatives are document-wide and would change every other request the app makes. Refs theam#174
Browsers no longer fetch avatars from GitHub. Images are proxied through
new /api/avatars/u/{login} and /api/avatars/id/{id} routes, whose server-
side fetch carries fresh headers — no cookies, forwarding chain, or
referrer — so the deployment origin never reaches GitHub with avatar
lookups, restoring the guarantee the first cut lost when it dropped
referrerPolicy="no-referrer".
An operator can set NEXT_PUBLIC_FACILITY_AVATARS=off to draw initial
letters only; unknown values fail closed to off. There is deliberately
no browser-direct mode left: it required third-party egress that
air-gapped deployments must not make.
The route serves only two exact path shapes pinned to two GitHub hosts,
and maps every upstream failure to 404, which leaves the CSS background
unset and the initial letter showing.
Stored principal avatar URLs are rewritten onto the proxy only when they
match a known GitHub shape; anything else falls back to the login-derived
source so the browser is never pointed at an unreviewed host.
Review found two ways the /api/avatars routes let a caller drive this deployment's outbound traffic. Both are fixed here rather than in two commits because they share one route body and one test harness. The routes were public, and every request produced a fresh upstream fetch, so anyone who could reach the deployment could cycle valid-looking logins or IDs and make it call GitHub without limit. A request now reaches an upstream only when three things hold: it carries a session the control plane recognises, no cached answer already exists, and the viewer's fetch allowance is not spent. Cookie presence alone is not enough — the session is sealed, so only /v1/me can judge it — and a control plane that cannot answer denies rather than admits. Verdicts are cached per token, rejections included, so a replayed token cannot turn avatar loads into control-plane load either. Answers are cached server side: 512 entries, a day for image bytes, five minutes for a miss so an avatar-less login is not refetched on every paint, LRU eviction and a per-entry byte ceiling. Cache hits spend no allowance, so the per-viewer ceiling binds only on targets nobody has asked for. Both bounds are per web process, which the hardening checklist now says. redirect: "follow" let the upstream redirect anywhere: a fake upstream pointing at a second server had its bytes returned as an avatar. The fetch now follows hops by hand — each Location resolved against the URL that produced it, vetted against the two permitted GitHub hosts over https before it becomes a request, and the chain cut after three hops rather than run to whatever length the upstream chooses. Regression tests were checked by mutation rather than assumed: restoring follow-any-host fails the redirect cases, including the assertion that the second server receives nothing, and removing the session gate fails four cases. apps/web is at 133 tests, up from 88. Refs theam#175
ccec437 to
3504a03
Compare
|
Both gaps are fixed in
|
| Mutation | Result |
|---|---|
| Restore follow-any-host | redirect cases fail, including the assertion that a second local server receives zero requests — your exact repro, as a test |
| Remove the session gate | four cases fail |
Also covered: relative and protocol-relative Location, an http://github.com downgrade, github.com.evil.example, a missing Location, an endless redirect loop, all five redirect statuses, cache hit counts, miss caching, and the rate-limit ceiling. No live credentials and no network egress — the GitHub surface and the control plane are both local fakes.
Notes
- Limits are constants, not env vars. No new operator knobs and no
.env.examplegrowth beyond a note describing the bounds. - Rebased onto feat(web): show story assignees on the board #171's current head, so this branch now includes
6c21a71. feat(web): show story assignees on the board #171 is still open, so the diff here still carries its commits, as the description says. tsc --noEmitclean inapps/webandpackages/ui,biome checkclean, guards pass.
main gained the WSJF chip and the Builder plan policy while this branch was open, and both landed in the two files this stack rewrites. Three conflicts, all resolved by keeping both sides: - issue-row.tsx imports and row body: the WSJF chip and the assignee avatar are independent additions after the labels. The chip keeps main's adjacency to the labels and the owner sits next to the timestamp. - stories/page.tsx data loading: main added api.project alongside api.me, this stack renamed me to meResult and added the mine search param. The combined call binds all three. - stories/page.tsx board body: this stack extracted the stage list into boardBody(), main added builderPlanRequired to IssueRow inside it. The extraction is kept and the prop threaded into it. theam#171 is still open and rewrites the same two files, so it will meet the same conflicts and must resolve them the same way.
3 participants
Closes #174. Also finishes the avatar half of #101, which #171 deliberately left out.
apps/webandpackages/uionly. No new dependency, nopackage.jsonchange, nonext.config.tschange, no API or schema change. Nothing new travels on the wire.What you get
One square avatar, immediately left of
@login. The+Ngrammar #171 introducedis untouched.
On the story row (
components/issues/issue-row.tsx, a client component), at 14px:In the story header (
stories/[number]/page.tsx, a server component), at 16pxto match the header's larger type:
Unassigned rows and headers still render nothing at all — no placeholder, no
reserved column.
storyOwnerkeeps its{ login, extra }shape and its tests;this PR does not touch it.
And the topbar (
components/shell/topbar.tsx) now uses the same primitive.That is the part that closes item 2 of #174: the topbar used to render an image
when
avatarUrlwas set and nothing when it was not, which is exactly the gap#101 believed the topbar had already closed. It now renders the initial letter:
The fallback, and why it is a background image
This is the part worth reviewing closely, because it is what answers #101's own
question about deployments that must not let the browser reach github.com.
packages/ui/src/avatar.tsxpaints the initial letter, then layers the remoteimage over it as a CSS
background-image— not as an<img>.An
<img>that fails to load makes every engine paint its own broken-image glyphon top of the letter.
alt=""does not suppress it, and neither doescolor: transparent. A background image that fails to load paints nothing, sothe letter underneath is untouched. A failed avatar becomes pixel-identical to no
avatar at all. I measured this in Chromium, Firefox and WebKit before choosing it.
So a deployment whose browsers cannot reach github.com degrades to the letter with
nothing to configure — no env var, no
NEXT_PUBLIC_*convention, and no changeto
.env.example, the Dockerfile, compose or the self-host docs. Here is the sameboard with the connection to both avatar hosts aborted:
Same rows, same spacing, no broken-image glyph anywhere. Close up, online and then
with the hosts unreachable:
The letter comes from one neutral background, never a colour hashed from the login.
packages/ui's tones (agent,human,ok,bad,info,machine,muted) area status palette, so hashing a login into them would hand someone the colour of
"failed" or "running".
The primitive is
aria-hidden. The login it stands for is always written out besideit, so the avatar adds nothing for a screen reader to announce and nothing new to
translate.
Where the rules live
Two helpers in
lib/pipeline.ts, both covered intest/pipeline-story.test.ts—the same file #171 uses, and no new test file:
avatarUrlFor(login)https://github.com/{login}.png?size=40. Login trimmed and URL-encoded.nullfor a blank login, so nothing is drawn.avatarInitial(value)?when there is nothing at all. Spread rather than[0], so an astral character survives whole.?size=40rather than the 14–20 CSS px actually drawn, so 2× displays stay sharp.packages/ui/src/avatar.tsxtakessrc,initial,sizeandclassNameand hasno framework import — it is the same component in the client row and in the two
server components. Its one piece of logic worth naming is
cssUrl, whichpercent-encodes
",\and whitespace so a login can never end the CSS string orthe
url()token early.Three things called out so they are not mistaken for refactors
1. The topbar loses
referrerPolicy="no-referrer". github.com will now learnthe deployment's origin when it serves the viewer's own avatar. Referrer policy is
a property of the fetch initiator, and CSS gives no way to set one. I measured the
alternatives before accepting this: a pseudo-element, a child element, an inline
style, an external stylesheet carrying
referrerpolicy="no-referrer", and acustom-property indirection all send the origin, in all three engines. The only
levers are document-wide —
<meta name="referrer">or aReferrer-Policyheader —and either would change every other request the app makes.
github.com learns nothing else; the browser connects from the user's own machine
either way. If that trade is not acceptable, the API-proxy approach in #174 is the
alternative, and I would rather hear it now than after merge.
2. The topbar avatar changes from round to square. It was
rounded-full.packages/ui/src/primitives.tsxdocumentsPillTagas "the only pill-shapedelement in the system", so the round avatar was the exception, and making the new
shared primitive round would have spread that exception to three sites instead of
retiring it from one.
3.
next/imagegoes.next.config.tshas noimageskey today, so keeping theoptimiser would have meant adding
remotePatternsplus a caching story, andunoptimizedwould have kept the broken-image glyph. Removing it also keepspackages/uifree of a framework dependency.Worth knowing either way:
https://github.com/{login}.png?size=40302s tohttps://avatars.githubusercontent.com/u/{id}?v=4. Two requests, two hosts —relevant only if a Content-Security-Policy is ever added. There is none today.
Testing
apps/websuite: 11 files, 88 tests, up from 80. Eight new cases pin the twohelpers: a normal login, a login needing trimming, a login needing escaping, a
blank login, an initial from a login, an initial from an email, an astral first
character, and the
?when there is nothing to draw.tsc --noEmitclean in bothpackages/uiandapps/web.Biome (
pnpm lint) run and confirmed rather than assumed, sinceAvataristhe first component of its kind in
packages/ui: 406 files, no fixes.pnpm verifyrun on this branch. Everything passes except two tests underscripts/, which I do not believe are mine and which I checked rather thanassumed:
scripts/dependencies-security.test.mjs— patched image-size rejects non-advancing JXL/HEIF boxestimeout: 1_500, which a loaded machine misses.scripts/deploy-aws.test.mjs— AWS CLI adapter maps missing images … to stable denialsA full
pnpm verifyon the base branch fails both, so the diff here does notmake anything worse. It also cannot: it touches no file under
scripts/and addsno dependency — the whole diff is seven files in
apps/webandpackages/ui.Verified by hand on a live instance against GitHub-synced issues, which is where
every screenshot above comes from. Online: all three sites draw the avatar. With
both avatar hosts aborted at the network layer — and the blocked URLs read back to
confirm both hops of the 302 were really refused — all three degrade to the letter
with no glyph and no layout shift. And with
avatar_urland the GitHub login bothcleared on the principal, the topbar renders the letter taken from the email where
it used to render nothing.
Two limits, stated rather than rounded up:
+Nwas exercised live at N=1 only,as in #171 and for the same reason — GitHub assigns only users who can reach the
repository, and the test instance had two accounts. The count itself is covered by
unit test. And the cross-engine fallback result is from a standalone harness in all
three engines; the live checks above were Chromium only.
🤖 Generated with Claude Code