Pulldasher frontend v2: a React + TypeScript rewrite#477
Merged
Conversation
The single three-tab popover buried every dimension behind a tab click and rendered durable mutes, saved scope, and session toggles as indistinguishable rows. An audit of the surface found the split that fits: two focused triggers (Repos, People) that open straight into their lists, with the rarely-diverging session drafts toggle demoted to an inline row beside cryo under an explicit "this session" separator. Active filters now surface as header chips, and the chip grammar IS the durable/session distinction: a dismissible pill (x) clears session scope; an informational pill (muted repos, muted people) opens the control that owns the durable state. "Clear filters" says outright that mutes and stars survive it. Cohesion fixes from the same audit: one shared FilterSearch input replaces three copy-pasted search boxes (RepoManager and TeamPicker adopt it too), mute/unmute is a QuietButton everywhere instead of a hand-rolled 11px variant, starring is finally reachable from the one-click filter surface instead of only the Settings drawer, and the "Your team" preset chip is brand-bordered so a personal roster doesn't read as an org fixture. People rows gain star and mute, with a muted- people fold mirroring the muted-repos one. Muted people filter out of the board through the same reveal levers as muted repos (show-all, or naming them in scope or an author: query). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Starring a repo or person used to mean a trip to Settings or the filter popover; now the row's kebab menu carries Star/Mute for both the PR's repo and its author (author items hide on your own PRs). The kebab was mobile-only, so it now renders at every width — the desktop hover cluster keeps its three icon actions and gains the kebab for the rest. Starred authors get a small brand star on their row avatar via a new avatarBadge slot on CardShell — one slot instead of touching every Avatar call site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewers triage by budget — "I have twenty minutes, show me the small stuff" — and by obligation — "what's actually my move here." The query grammar now answers both: weight:xs,s (comma list, OR within the token), has:action (this card carries an imperative for you, powered by rowNote), is:restamp (you owe a re-look), and is:blocked. Tokens compose with the existing grammar's AND semantics, and unknown has:/is: values match nothing, same as an unrecognized status:. The row's weight meter becomes a filter toggle: click to scope the board to that size class, click again to clear — the same in-context filtering avatars already do for people. The Legend gains a Query group documenting the grammar, which was previously undocumented there. matchesQuery now takes the viewer login, since has:action is inherently viewer-relative. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "4 muted repos" pill sat directly beside the Repos trigger whose active label already reads "4 muted", and clicking it opened the same popover the trigger does — a duplicate button for the fact next door. Same for muted people. With them gone the chip grammar is simpler and stricter: every chip is session state and every chip is dismissible; durable state lives on the triggers that own it. The ref plumbing that existed only to let those pills open the popovers goes too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The State dropdown needs mutually-exclusive buckets — one answer per (pull, viewer) to "where does this sit for me": restamp, review, qa, mine, blocked, waiting. Re-stamp is checked before rowNote so an owed re-look never disappears into Finish-QA/Re-QA strings. Blocked here is deliberately viewer-relative, unlike the is:blocked query token: a dev-block you authored carries the Address-feedback action, so it's your move, not a thing you're blocked on. weightFilterKey/matchesWeightFilter pin down the unknown-size rule in one place: a pull without a real size matches only "unknown", never the XS its zeroed line counts would imply. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two new header triggers alongside Repos and People: multi-select checkbox panels with live counts, XS through XL plus size-unknown, and the six viewer-relative action states. Selections are session state serialized in the hash (weight=xs,s / state=review,restamp), so a filtered view is shareable, and they AND with typed query tokens the same way scope already ANDs with repo: — no merge logic, no conflicts. Counts come from a pool that excludes the dropdown's own pass, so checking XS doesn't make the other options' numbers vanish. Both render as dismissible chips and reset with Clear filters. All four filter triggers collapse to icon-only below the sm breakpoint to keep the 375px header on two rows. The row weight-meter click now toggles the Weight selection instead of overwriting the query string — clicking it used to silently wipe any hand-typed query text. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-day max ticks rendered as sixty disconnected specks at 60-column density — a dot lattice, not information — and worse, the outliers set the y-scale, compressing every average bar into an identical 8%-floor stub. Two failures hiding the one thing the chart exists to show. Bars now scale to the average series so daily variation is visible, and the outlier story gets exactly one mark: the window's slowest merge is pinned to the top of its day and named in the summary line. Every day's slowest still lives in its tooltip. PeriodColumns' scale ignores markers generally now — a callout should never drag the data down. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stacked PRs rendered as unrelated rows with a prose flag naming a branch. Now a dependent PR whose parent sits in the same rendered list indents beneath it with a hairline elbow, chains recurse with the visual depth capped at two, and the now-redundant stacked flag drops off nested rows. groupIntoTree runs after each lane's own sort — the stack occupies the parent's position, children follow in order — and lane membership never changes: a blocked child stays in the blocked fold. Nesting is presentation, not reorganization. Parent resolution keys on repo + head ref over the local list only, and any ambiguity (the fixture ships a real duplicate-head case) renders flat rather than guessing. Cycles chase iteratively, no recursion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The feedback popover only existed where feedback existed; a fresh PR with zero activity answered hover with nothing. The status badge is now the universal trigger on every card: state restated in a sentence plus your move when you have one, the facts (opened when and by whom, CR and QA progress with names, stale stamps in amber, CI contexts when red), the feedback excerpt rows where sources exist, and a GitHub link always. Closed rows get the receipt version. Same popover, two doors — the context text still opens it where feedback lives. Dependent PRs whose parent is elsewhere on the board now name it: "stacked on #N" with the parent's title and link, via a whole-board parent lookup memoized once — the base-branch prose remains only when the parent truly isn't open anywhere. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A labeled/unlabeled webhook wrote the label row but never marked the pull dirty itself — clients only heard about it because the pull_request handler happens to call updatePull unconditionally afterward, which marks dirty as a side effect. Progressive CR-weight updates (the size labels from ifixit#63258) depend on that broadcast, so anchor it where the label change happens: handleLabelEvents marks the pull dirty after its own write. An audit traced the whole label-to-weight chain and this was its one fragile link. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An author who dev-blocked their own PR read "Address feedback · from you" — technically true, useless as a nudge. When you're the only blocker the action is lifting your own block, so say that; when others blocked too, address their feedback and name only them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three header-area bugs from mobile QA:
The lens nav had no scroll escape: as a flex item with default
min-width:auto its six tabs set a min-content width the row couldn't
shrink below, so at 375px the whole page grew wider than the viewport
and clipped ("My work 5" also wrapped mid-label). The nav now scrolls
horizontally inside its own box — min-w-0, nowrap tabs, hidden
scrollbar chrome — and the body never scrolls sideways.
The "N merged or closed since your last look" banner was dead text.
It's now a button (only when the current lens has a shipped fold with
content): opens the fold by writing the same pd2.folds store a click
would, smooth-scrolls to it, and pulses it with the existing
fresh-flash keyframe — new information announced in the motion
vocabulary the board already uses.
Clicking Review snapped back to the configured default view: buildHash
omitted the lens param against a hardcoded 'review' while readHash fell
back to settings.defaultLens, so a lens-less hash meant different
things to writer and reader. Both now share one defaultLensFallback().
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A recurring triage view — "QA sweep" (state: qa, weight: xs,s), "quick wins" (weight: xs, has an action) — cost four to six clicks to rebuild every time, even though the whole session state already serializes into the hash. A saved filter is a named bookmark of that string. Two doors, one list (pd2.savedFilters, cap 8, oldest evicted): focusing the empty search box drops a panel — saved rows with derived summaries, arrow-keys and Enter, a "Save current filter…" footer that appears exactly when something is active, and two suggested combos before any exist — and a "Saved" trigger joins the filter row only once at least one filter is saved, so new users pay zero header cost. Applying one is just navigating to its hash; chips populate themselves. Durable state (mutes, stars, your team) is deliberately not captured — those aren't a viewpoint, they're your board. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The └ connector rendered as a literal "L" in the UI font, and as a flex child it consumed row width on top of the 22px indent — enough to overflow rows inside Classic's narrow columns. It's now a CSS hairline elbow absolutely positioned inside the indent gutter (zero flex width), and the per-depth indent tightens to 16px. Rows no longer overflow at any column width. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The CR dining-philosophers problem: everyone sees the same queue, nobody knows who's already reading what, so people either double-review or — more often — assume someone else is on it. QA solved this years ago with the QAing label; CR gets the equivalent, minus the label ceremony: claimReview/releaseReview socket messages and a reviewClaims whole-map broadcast. Claims are deliberately ephemeral, so they live in server memory only — no DB, no GitHub writes. A 4-hour TTL with a 10-minute sweep keeps abandoned claims from squatting; last-writer-wins keeps it social coordination rather than a lock; a restart clearing claims is correct, not a loss. The claimer's login comes exclusively from the socket's authenticated user (stashed at authenticate time) — never from a client argument. v1 clients never registered the event and are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two pure answers to reviewer deadlock. turnFor names exactly one suggested reviewer for a starving pull: the pool is everyone with CR history in that repo (minus the author, minus anyone already stamped), and the pick is a djb2 hash of the pull key over the sorted pool — so every teammate's board names the same person with zero server state. Diffusion of responsibility ends when a specific name appears. dealOne picks the single best card from an already-filtered queue: starvation-led, +2 for repo familiarity, +2 for reciprocity (the author has stamped your work), +1 for a quick win, ties broken by the queue's own crSort order. Choice paralysis is half the problem; one dealt card is a task where thirty are a wall. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claims flow over the socket into the snapshot and speak QAing's grammar: "X is reading it" while fresh, softening after two hours to "claimed 3h ago — pick it up?" so a claim absolves other reviewers without becoming squatting rights. Your own claim nudges back: "Finish your review". Claim/Release lives in the row cluster and kebab, hidden on your own PRs. Turn names ride the existing amber starvation register — "unreviewed 29d · cdcline's turn", or "your turn" upgrading your Review-it pill — suppressed the moment anyone claims. Becoming the named reviewer fires one desktop notification through the existing transition diff. "Deal me one" sits in the Review queue's lane header and honors the active filters (the Weight dropdown doubles as a time budget). Dealing claims the pick, scrolls to it, and flashes it with the fresh-row pulse; Pass releases and deals the next. The state popover's facts gain the claim and rotation lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
v1's filter menus let you narrow to a single value in one click; v2's pickers only had per-row toggles, so isolating one repo/person/weight/state meant unchecking everything else by hand. Add a shared hover-revealed OnlyButton to every picker row (Repos, People, Weight, State) that sets the selection to exactly that item. It reveals on row hover or keyboard focus so it stays out of the way but remains reachable, and stops propagation so it doesn't also toggle the row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claiming a review was purely a dashboard-local signal, invisible to anyone looking at the PR on GitHub. Mirror it onto the PR: when someone claims a review, also request them as a reviewer, and drop the request when the claim is released (so passing on a dealt pull doesn't leave orphan requests). The login comes from the socket's authenticated user, never the client, so it can't be spoofed. There is no per-user GitHub token, so the request is made by the shared app token with the claimer as the requested reviewer; the call is best-effort and fire-and-forget, since the local claim already succeeded and GitHub legitimately refuses a self-request from the PR author. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A needs_recr pull where YOUR own CR stamp went stale was being dropped into the "stamped by you — waiting on another reviewer" fold and excluded from the review queue. That's backwards: a stale stamp means the PR is back to 0-of-1 and still needs a CR, so it's reviewable (by you re-stamping, or by anyone else) and it is emphatically not waiting on someone else. Split the buckets by live vs stale instead: only an active crBy stamp counts as "stamped, in flight"; a recrBy pull stays in the review queue, where its row note already reads "your move: Re-stamp". Review.tsx was already correct and is the reference; this brings team.ts and People.tsx in line. Updates the team test that encoded the old behavior and adds a live-stamp case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A gamification layer for CR/QA work needs to fire transient messages on meaningful transitions, not on standing conditions, and it must not dump a backlog the moment you open the board. Put that logic in a pure, testable model: readSignals reads the board (queue sizes, your fresh stamps, the rotation's turns), and diffCheers diffs two snapshots into reward/nag toasts plus the baseline to carry forward. The first tick only primes. Rewards: a stamp of yours landed, a session milestone, the queue cleared. Nags: your pile crossed a threshold, the rotation named you on a starved pull, a stamp of yours went stale. Session-only, since there's no per-user history server-side. Covered by cheers.test.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Nothing surfaced the human side of review work: clearing a queue or letting it pile up passed silently. Wire the cheers evaluator to a transient toast stack that fires while you're on the board (unlike desktop notifications, which fire when you're away). Rewards rise with a brand medallion that pops, the hero moments spark; nags settle in heavier and a touch wistful to softly motivate. Auto-dismiss, capped at three, clickable to the PR, reduced-motion aware, and gated by a new "Cheers & nudges" setting (default on). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewers watch particular areas (Growthbook, Shopify, a subsystem) but had no way to say so; those PRs sat wherever weight and age put them. Add a codeRegions setting: free-text strings matched case-insensitively against a PR's title, body, labels, branch, and repo (all already on the wire, so no server change and no extra API calls). Matches float to the top of every review lane, above even starred authors, and carry a small marker so a floated PR explains itself. A settings editor manages the list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Dealing a pull used to claim it immediately. Now that claiming also requests you as a reviewer on GitHub, that's too eager: a deal is a suggestion, not a commitment. Deal now scrolls to the pick and waits behind a "Claim it" confirmation; "Pass" skips it (nothing to release) and deals the next, so you can flip through the queue a card at a time. The flash moves to the moment you claim, landing as the claim badge appears. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
"N merged or closed since your last look" jumped to a fold of closed rows, which is no more useful than scrolling there yourself, and it worked only on Review and My work. Replace it with a relevance-ranked catch-up: it names what actually shipped while you were away, yours first, then ones you reviewed, each linking to the PR. A dismiss button marks everything seen; a long list folds the tail into "see all on the board", where the old jump-to-fold survives. Works on every lens now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The eye ("I'm reading this") read as surveillance more than volunteering.
A raised hand — putting your hand up to take the review — matches what
claiming now means (you're on the PR as a reviewer). Uses Heroicons' solid
hand-raised; ActionIcon gained an optional viewBox size so the 24-unit path
renders alongside the 16-unit action icons.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The toast engine only knew about "cheers" (the gamified review rewards/nags). The shipped catch-up wants the same transient corner surface, so lift the shape into a shared model/toast.ts: a Toast with an 'info' tone alongside reward/nag, plus per-toast dedupeKey, ttlMs, and onAct/onGone hooks so a caller can drive a custom click and mark-seen. cheers.ts now aliases CheerToast to Toast (it only ever emits a subset). useToasts takes an extras: Toast[] feed, deduped by key and pushed once each, independent of the cheers setting since these aren't gamification. remove() fires onGone exactly once via a ref mirror so a toast leaving for any reason (dismiss, timeout, eviction) can settle its side effect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Turn a scope-filtered list of closed pulls into a single info toast instead of a standing panel. Only pulls that are yours or that you reviewed count (shipRelevance != null) — an org merge you never touched isn't worth a nudge, which is the whole point of moving this off an always-on banner. Singular vs plural title, a yours/reviewed breakdown body, and a dedupeKey of newest-close-time plus count so a fresh merge re-fires but the same standing backlog fires once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the full-width RecentlyShipped panel with the single info toast from shippedToast. app.tsx builds it from scopedClosed (so it honors the active repo/author/hidden filters), narrowed to closes since lastSeen and to pulls relevant to you; suppressed entirely while a text query is active. Clicking jumps to the shipped fold and marks the catch-up seen; onGone marks seen too, so timing out or dismissing settles it. This is the "don't distract me with merges I don't care about" fix — an org-wide banner of every close becomes a quiet, relevance-gated nudge or nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The competitive toasts (top-of-board, "you passed X") need a per-person review tally, but the server has no per-reviewer history — stats-history is org-wide aggregate. So rank client-side over the window we already have: count the distinct pulls each login holds a CR/QA stamp on across open + recently-closed, dense-ranked. Window-limited by design, hence copy that says "on the board", not "this week". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
draftsMode 'mine' hides other people's drafts from the board, but boardHidden's review-requested exception lets a draft someone asked you to review through, and it then surfaced at the top of "Recently updated", where a not-yet-ready draft you don't own is just noise. The "what changed" glance now drops others' drafts (your own stay: they're your work); the review-requested draft still shows in the queue, where acting on it makes sense. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bots (dependency bumps and other machine authors) are low-priority review work but still real work, so they default to visible in their own fold. Some people would rather never see them. Add a durable "Ignore bot PRs" preference (off by default) to the Hidden panel: on, it keeps bots off every lens through the board's own boardHidden predicate, and "Show everything for now" still reveals them. The Hidden door now also opens when bots are present, so the toggle is reachable even when nothing else is hidden. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prod and dev config.js both default cr_req to 1 (a single code review), but the tracked config.example.js and the test fixture still defaulted to 2. That drift meant localdev, new setups, and the test fixture required two CRs where the real environments require one, so a PR with a single CR stamp and qa_req 0 (no QA) read as needs_cr / not-ready there while prod correctly showed it ready (the #63282 report). Align both tracked files to prod's default of 1; qa_req was already 1 everywhere. A PR still sets its own cr_req N in the body to require more, or 0 to waive review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two fixes for PRs with non-default requirements. The stamps popover said "No stamps yet." even when the requirement is waived (cr_req/qa_req 0). It now reads "CR not required." / "QA not required." in that case, and drops the pip legend there since there are no marks to decode. The pip cluster was pinned to a fixed min-width (sized for three marks), so a one-CR PR reserved empty air and didn't visually shrink. It is now content-sized: one required stamp shows one mark, four show four, and the slot grows and shrinks with the required count. The rail stays anchored on the age numeral (a wider cluster grows leftward), so the age column holds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The console greeting was a one-line "pulldasher v2 · wrench in hand". Now it prints an actual wrench, run-length encoded (even runs are spaces, odd runs '#') and logged as monospace pre-formatted art. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Content-sizing each row's pip cluster (previous commit) made the columns ragged when different pulls need different counts. Instead, publish the widest cr_req/qa_req on the board as CSS variables (--cr-slots / --qa-slots, computed over all open pulls) and pad every rail slot to them, so the CR and QA marks line up in a column across rows. The slot still collapses when nothing needs many: a board where every pull needs one CR shows one-wide slots, and a single PR bumped to three widens them all until it merges. Density-aware (14px / 12px pip). Popover pips stay content-sized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Applies the CLEAR findings from a PRODUCT/DESIGN.md audit. - "lens" reads "tab" in every rendered string (savedFilters describeHash, the Snoozed-fold copy, the snooze chip title/aria-label). The Lens type and LENS_LABELS identifiers stay. - "scope" reads "filters" in the Reset button title and a repo checkbox's aria-label, matching the existing "repos filter:" convention. - Em dashes removed from rendered copy (~25 sites: lane/fold glosses, SubDoor bodies, titles, aria-labels, WORD_GLOSS, the why-lines), each swapped for the punctuation the sentence wants (comma, colon, semicolon, period, or the house "·"), per DESIGN.md's no-em-dash rule. - The per-signer stale pip title now says "stale review: invalidated by a later push", matching the popover legend's word for the same mark. - Ci's no-timing cell renders an empty string instead of a lone em dash. Test assertions updated for the "tab" and colon-in-why-line changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`npm run lint` (eslint ./, the CI Build step) failed with one error:
react/prop-types couldn't resolve `className` through HeaderIconButton's
`ButtonHTMLAttributes<…> & {…}` intersection, so it read the destructured
`className` as an undeclared prop. Declare `className?: string` explicitly
in the props type (ButtonHTMLAttributes already carries it; this just
makes the rule see it). Type-only change, no runtime effect. Repo-wide
eslint errors now 0. The frontend build gate (tsc + vite build) never ran
eslint, which is how this slipped in with the never-CI'd cleanup commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
@danielbeardsley I polished and fixed some bugs, now I am running review passes from claude. After that's done, I will push and then stop here |
Fold ran `if (!count) return null` before its useRef and useSyncExternalStore (foldOpenStore.useValue) calls. Review renders every RestGroup Fold ungated at a fixed tree position, so a lane emptying or filling on the mounted tab (e.g. closed 0->1 the moment a PR merges) changed that Fold instance's hook count between renders and threw "Rendered more hooks than during the previous render", crashing the Review tab. Run both hooks unconditionally, then bail on empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
parse() grafts additions/deletions/changed_files from the fetched full pull onto a list-sourced item but omitted mergeable, so every bulk/list refresh (server startup refresh.openPulls, the refresh-*-pulls bins) wrote mergeable back as NULL. derive()'s strict `mergeable === false` then never matched, so a genuinely-conflicted PR read as mergeable and reached the ready-to-merge bucket board-wide until an unrelated per-pull webhook re-derived it. This defeated the getFromDB tinyint fix. Graft mergeable alongside the diff stats. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two robustness fixes to this module's background work. 1. The pullsChanged handler ran dbManager.getPull(...).then(...) with no .catch, so a rejected DB read (connection drop, deadlock) became an unhandled rejection. There is no global unhandledRejection handler, so under Node's default that terminates the whole server. Add a .catch. 2. The closed-pull cull and the reviewRequestsByPull prune it drives were scheduled with a one-shot setTimeout, so they ran once ~1h after boot and never again. The in-memory pull list and the review-request cache then grew unbounded for the process lifetime, and sendInitialData shipped an ever-growing stale-closed list. Recur with setInterval (still unref()'d so it never keeps the process alive on its own). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
githubLoginForToken returned null for every non-200, and resolveUser cached that as an invalid-token verdict for the 5-minute TTL. A transient GitHub 403 (secondary rate limit) or 5xx on the /user lookup therefore locked out a valid caller with a 401 for five minutes. Treat only a 401 as a definitively bad token (safe to remember); throw on any other non-ok status so the blip surfaces as a 500 and is not cached. Network errors already threw and were never cached. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The participants source was filtered by everStamped but the unstamped_reviewers source was not, so a reviewer holding an active or stale CR/QA signature who also left a COMMENTED or CHANGES_REQUESTED review was wrongly counted as "engaged, no stamp" (and a stale-CR holder was double-listed in recrBy). That drove a wrong "in discussion with X" / "X looking" row note and violated the field's own contract, which the "excludes anyone who already holds a CR/QA signature, active or stale" test only enforced along the participants path. Combine both sources and apply the author/everStamped/bot filter to them uniformly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1. top-of-board fired on a passive rise to #1 (a rival's stamped PRs merging off the board dropping their count below yours) with no new review by you -- the exact false reward the climbing edge was fixed to avoid. Add the same own-count-growth guard. 2. The stale-claim nag had no status gate, so it nagged "still unreviewed" about a claim on a pull other reviewers had already CR-completed. Gate on CR_INCOMPLETE, matching the row-level treatment. 3. A stamp-landed reward evicted by MAX_PER_TICK was recorded as seen and never re-fired, silently losing the praise. Record the landed key in a mutable set the push's eviction callback un-records, matching the other rewards' evicted-then-deferred contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1. An unrecognized is: value hit `return false`, and since matchesQuery requires every term to match, one unknown value silently emptied the whole board. Fall through to the plain-substring match unknown top-level keys already use, so it behaves like any other unrecognized token. 2. toggleScopeMember collapsed an explicit repo/author selection back to "everyone" on a length-only comparison, which misfired when the scope held a stale entry no longer among the candidates (equal length, wrong set) and wiped the filter. Collapse only when the selection actually covers every current candidate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A non-2xx response from /user-names was treated as a successful empty result, permanently caching every requested login as "no name" for the 24h TTL and degrading those names to raw logins until expiry. Leave the logins unresolved on a non-ok response, matching the network-failure .catch path, so a later requestNames retries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rotation's "your turn" OS notification checked only turns.get(key) === me && !isClaimed(p), with no bot-author guard, so a starved Dependabot-style PR could page a human -- the noise cheers.ts already guards against with its own isBot check on the same turns input. Add the same bot-author guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
signoffLeaders credited a pull's own author for CR/QA signatures placed on their own PR, inflating their "reviewed" total with non-review work -- unlike reciprocity() in the same file, which excludes self-stamps. Skip a signature whose author is the pull's author. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-person "owes a re-stamp" map walked only recrBy (owed re-CR) and never reqaBy (owed re-QA), so a reviewer whose QA stamp a push invalidated showed no owed dot or badge, dropped out of the "Re-stamps they owe" fold, and -- if they had only re-QA debt and no authored board PR -- vanished from the directory entirely. DebtCard's board-wide reviewDebt() already sums both. Walk reqaBy too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1. GitHub logins are case-insensitive (one account), but the duplicate-add guard and toggleTeammate deduped case-sensitively, so "JDoe" and "jdoe" could both land as two roster members. Dedupe case-insensitively, keeping whichever spelling arrived first. 2. renameTeam no-ops on a blank or colliding name, but nameDraft only resynced through a prop-keyed effect, so the input kept showing the invalid draft indefinitely. Revert nameDraft to the actual team name when the rename is rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1. NumberField read an emptied input as the number 0 and immediately clamped it to min, so the field could never be cleared to type a fresh multi-digit value. Hold the raw string, commit only a finite parse (clamped), and revert to the committed value on blur. 2. The Settings "return focus to the cog on close" effect ran on initial mount (open starts false), stealing focus to the gear on page load. Guard it so it only fires after a real open-then-close. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1. Navigating to the default lens with no filters assigned location.hash = '' on the lens-changed branch, leaving a stray trailing '#'. Use the same pathname+search fallback the sibling same-lens branch uses (via pushState, preserving the history entry a lens change earns). 2. The display-name effect depended only on [pulls, closed] but read settings.teams / hiddenPeople, so a newly added teammate whose login was not on the board stayed nameless until an unrelated board update. Add a stable joined-login dependency so it refreshes when the roster or hidden set changes, without looping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
authFailed was set once by start()'s one-shot whoami(), so a session that
died while the tab slept never flipped it. authenticate()'s reconnect
collapsed every token failure -- session-expired and transient alike --
into one onState('error'), so the board showed "Live updates lost,
retrying in the background" instead of "Sign-in failed, reload to sign in
again", wrongly implying a dead session would self-heal.
Mark a /token redirect or 401 as a distinct AuthExpiredError, add an
onAuthExpired signal the store maps to authFailed (true on session death,
back to false when a reconnect re-auths), and suppress the connection
banner while authFailed so only one banner shows. A transient network or
5xx failure keeps its existing retry/backoff untouched -- no false "sign
in again" on a blip.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three fixes requested off the code review.
1. is:draft / is:mine now filter. An unrecognized is: value falls through
to a substring match, but is:draft / is:mine (reasonable guesses) match
no PR text and left the board empty. Implement them as real filters
(draft status / authored-by-you) and document them in the filter
aria-label, title, and the Legend.
2. Cheers respect a snooze. start-here / quick-wins / your-turn drew from a
reviewable pool that ignored snoozes, so a pull you snoozed ("not
today") in Review could still be nudged as the best thing to review.
Thread the snoozed pull keys into evaluateCheers and drop them from the
reviewable pool only -- board-cleared and the leaderboard still see the
whole board, so a snooze doesn't fake an empty queue.
3. Remove the dead weight-letter click handler. onWeightToggle was defined,
threaded through RowOptions, and compared in Row's memo, but never
invoked -- the weight letter is an inert display span. The Legend claimed
"a row's weight letter filters with a click"; drop the dead handler and
correct the copy to describe what the letter actually does.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
All review feedback addressed. I am not planning to push anymore commits |
jarstelfox
marked this pull request as ready for review
July 24, 2026 21:38
Member
|
382 commits! That's a record for a single pull I think. I vote for merging! |
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
2 participants
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.
Note
Draft, for the review trail. This branch is already running on dev
(
pulldasher-dev.cominor.com) and prod (pulldasher.cominor.com), deployedstraight from
frontend-v2. The PR exists so a change this size has a homefor review and history, not because it's blocked on merge. There are no CI or
branch gates on Pulldasher; prod is whatever's checked out.
Summary
A ground-up rewrite of the Pulldasher board as a React + TypeScript + Vite + Tailwind app under
frontend-v2/, replacing the legacy frontend and served at/. The old board answered "here is every open PR"; v2 is reviewer-centric — it answers "what should I review next, and what needs me," and adds the coordination layer (claims, a turn rotation, "Deal me one") that the team was doing by hand in Slack.Everything below is a summary; the commits tell the full story (202 of them, atomic).
What's in it
prefers-reduced-motion.Server-side (small, and safe)
lib/claims.jsstores a per-recordexpiresAtfrom a clamped[5m, 24h]client TTL;app.jsforwards it from theclaimReviewsocket event. The broadcast wire is unchanged ({ login, at }). Covered bytest/claims.test.js./(the old/v2/path is gone).Testing
frontend-v2/src/model/and componentstscstrict — cleantest/claims.test.js/auth-gates (302),config.jsonservesweightLabels, no errorsQA
prefers-reduced-motiondegrades every animation to a stillsize: *chain), i.e.config.jsonis present indist/Deploy / rollback notes
Frontend-only for the latest slice; the only server change in the whole branch is the per-claim expiry above. Prod rollback is a rm-and-run with
pulldasher:pre-40ff358-20260720; dev with any priorpulldasher-dev:<sha>. Build bakesfrontend-v2/public/config.json(weight labels) intodist/— load-bearing, gitignored.🤖 Generated with Claude Code