Post-launch cleanup: search all PRs, review lanes, bots, notifications - #486
Open
jarstelfox wants to merge 21 commits into
Open
Post-launch cleanup: search all PRs, review lanes, bots, notifications#486jarstelfox wants to merge 21 commits into
jarstelfox wants to merge 21 commits into
Conversation
Reviewers deliberately pin many saved searches (every personal roster auto-pins one, plus hand-saved filters), which overflowed the single filter row: the pins wrapped to two or three lines and shoved the dimension triggers and search box down with them. That is the badge-soup shape PRODUCT.md warns against, moved into the header. Move the pins to their own dedicated line, a horizontally-scrollable strip reusing the lens tab strip's overflow-x-auto no-scrollbar pattern. Chips are shrink-0 so a long set scrolls instead of compressing, every pin stays one click away, and a growing set can never wrap the triggers again. The divider follows whichever row leads so the header keeps one separator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With a repo priority set, starved pulls were extracted into a Starving fold and removed from their repo block, and code-region matches were pulled out of the queue and QA lanes. A high-volume repo (ops) filling the Starving fold's cap could bury a starved PR from another repo (ifixit) behind 'show more' with no way back onto the board, since it no longer appeared in its own block either. Make both non-exclusive: the Review queue keeps everything, and 'Starving' and 'In your code regions' show a copy of a slice rather than removing it. This matches how 'Recently updated' and a claimed PR already relate to their lanes. The four state lanes (Ready, Waiting on you/others, Needs QA) stay mutually exclusive, so position still encodes state where it matters. Also fixes a pull you already hold a live CR stamp on showing in 'your code regions': the QA-pool leg feeding regionMatches never checked crBy, so a needs_qa pull you had already reviewed sailed in. A needs_recr pull whose stamp is only yours-and-stale still shows, since that stamp has dropped out of the live crBy set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Recently updated was an unbounded activity feed: it listed anything that moved since you last hit Clear, including PRs you had already CR'd and bumps from over a week ago when you had not looked in a while. That cuts against the board's actionability-first ethos and was the noisiest lane. Bound it two ways and narrow it: within the last 3 days (not only since Clear), drop PRs you hold a live CR stamp on (finished for you) and non-reviewable ones (others' drafts, dev-blocked), and keep your own PRs since activity on them is worth seeing. It now reads as what changed on your review work since last look. The lane still lives in the component; moving it into buildReviewLanes for unit coverage is a fair follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When more than one PR landed while you were away, the shipped catch-up toast threw the titles away and collapsed to a bare count ('2 yours · 1 you reviewed'). It now lists the top 3 as links with a '+N more' line, so the catch-up names real work. The board's Recently-closed fold already did this; only the toast lagged.
Also stops the toast and bell-panel text from truncating (DESIGN.md: nothing truncates except the wait-word door; density comes from geometry, not from hiding text), so cards grow to fit the full title and body.
And fixes the bell panel dropping the leading number on the two count-bearing nudges (milestone 'reviews this sitting', top-of-board 'stamps in view'): it rendered the title without the count the live toast shows via its Odometer, leaving a number-less fragment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On a phone, tapping a PR title both opened the PR and, about 250ms later, popped its description preview uninvited. The title is a real anchor, so a tap navigates; the tap also synthesizes a mouseenter that armed the hover-open timer, and nothing cancelled it because the title's trigger deliberately drops its click handler (a click on the title means open the PR). The avatar (tap filters) and the fold-gloss eyebrow share the same defect class without the navigation. Switch usePopover's hover handlers from mouse events to pointer events and only arm the preview when pointerType is 'mouse'. Touch taps no longer arm a hover-preview, so those elements do their primary action cleanly, while desktop hover is unchanged and every button-triggered door still opens on tap through the click toggle. This is 'touch has no hover' applied in JS to match the CSS the board already uses, and it reads pointerType per interaction so a hybrid mouse-and-touch device behaves correctly either way. The title's description preview is now desktop-only by design; on mobile the title simply navigates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The snooze tooltip promised 'off your Review tab until tomorrow or until it changes,' and the code voided a snooze once updated_at moved. But a new comment or review does not move updated_at: those arrive on their own webhook path that re-derives the pull without touching it. So commenting on or reviewing a snoozed PR never woke it, and the promise was half true. The snooze record now captures the comment and review counts at snooze time, and isSnoozed wakes when either climbs, alongside the existing updated_at check. Counts only, since the wire carries no per-event author, so your own comment wakes it too, the same way your own push already does. Clean cut: a pre-baseline snooze entry (a bare epoch number) is dropped on load, so the pull reappears once and you re-snooze it if you still want to. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These are machine accounts GitHub does not tag with a [bot] suffix, so isBotLogin's suffix check missed them and the board treated them as human authors. ifixit-systems is our dependency-bumper and AI-agent account (dozens of PRs across fixbot, ifixit, and DocHarvestor); Copilot is GitHub's coding agent (user.type Bot, but no suffix). Listing them in config gives them bot treatment: folded or skipped, and out of the reviewer leaderboard and rotation. claude[bot] and the other suffixed apps were already detected. This updates the example template only. The deployed config.js on the host needs the same two entries added to its bots list to take effect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A bot's comment or review, the auto claude[bot] review or a CI bot, shouldn't drive the human-review nudge or wake a PR you snoozed. claude[bot] only posts COMMENTED reviews, so the status bucket was already safe, but two paths still leaked bot activity into what a human reads: the changes-requested nudge copy, and the auto-unsnooze added earlier on this branch. Client: changesRequestedBy (shared derive) and lastChangesRequestedAt now filter bot reviewers, matching the isSuffixBot filter engagedNoStamp and the 'Answer the review' branch already use, so a bot's changes-requested can't demand a re-review or skew whether the author answered a human. The snooze-wake now excludes bot-authored reviews (allCR/allQA/unstamped_reviewers filtered by isBotLogin, so config bots count too). Server: a bot's plain COMMENT can't be told apart on the client (comment_count is a bare aggregate), so the server now sends human_comment_count with bot authors excluded (reusing review-model's config-aware isBot), and the snooze prefers it, falling back to comment_count for an older server. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 'Ignore bot PRs' setting hid bots at one global chokepoint, so turning it on also stripped them from the CI lens and Ready-to-merge, and no filter could bring a specific bot back. When you hide bots you still want their fleet CI health, and a green signed-off bot PR that's one merge press from done. Split boardHidden into every-hide-except-bots plus a reveal-aware bot rule (hideBots && isBot && !revealedAuthor && !is:bot token), so a bot named by the active saved/pinned filter, author:, or is:bot still shows. A bots-bypassing pool that still respects every other hide feeds only the CI lens and Ready-to-merge's bot portion; the queue tail and the bot fold stay emptied when hideBots is on. Snoozing a bot straight off Ready still parks it in 'Snoozed by you' so it never vanishes without an undo. Adds an is:bot query token (and appends it to the filter-box hint) so the escape hatch is discoverable like every other is: token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A review of the branch surfaced several small gaps, fixed here: - Ready-to-merge's empty check ignored botsForReady, so a merge-ready bot could hide behind 'All clear' once hide-bots emptied the queue. - is:bot matched only the [bot] suffix, so with hide-bots on it revealed suffix-bots but not config bots (ifixit-systems, Copilot), and even narrowed CI/Ready; matchesQuery now takes the extra-bots set. - The popover hover gate excluded a stylus; it now excludes only touch, so mouse and pen keep hover. - The snooze wake summed CR+QA+unstamped into one count, so a reviewer going COMMENTED to APPROVED (unstamped down, CR up) washed out and left the snooze asleep; the three counts are now tracked separately, and the load guard drops half-shaped records. - Recently updated's sub-line no longer overpromises (it names the 3-day bound). Plus comment accuracy (the changes-requested filter stays suffix-only per the shared-layer convention; the hidden-ledger note calls out the CI/Ready exception) and a toast singular-format nit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jarstelfox
marked this pull request as ready for review
July 26, 2026 00:28
Member
Author
|
Deploy record (2026-07-25) 🤖 Deployed straight from this branch to both instances; this PR is the review trail. No DB changes.
Both Rollback (seconds, nothing to unwind): Note Both deploy checkouts ( |
jarstelfox
changed the title
A notification in the bell panel is a snapshot: it captures a PR at the moment its toast fired and never looks at that PR again. So a nudge can outlive the thing it points at, once that PR merges or closes. Add a pure predicate for that, general across every toast kind rather than tied to one: a fired notification is stale once every PR it references has left the open board. A toast with no PR (inbox-zero, a milestone count) and the retrospective shipped recap are never stale. The panel wiring lands in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Kyle hit a dead nudge on the board: "Return the favor to Angel, fixbot#3116" stayed in the bell after #3116 had merged, pointing at a PR there's nothing left to do on. Filter the panel's records through notificationStale so an entry clears the moment its PR leaves the board. Keyed off the full board, not the hidden-filtered subset, so hiding a repo doesn't read as done, and held until the first payload lands so an empty pre-load board can't blank the panel. Report: https://ifixit.slack.com/archives/C6YD8UBS7/p1785179041512159 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The filter grammar (matchesQuery) only runs on open pulls. That's fine while it's an in-context filter, but not once search has to find a PR that may have already closed. Closed pulls are raw PullData with no derived status, so add a sibling matcher over the identity fields a find actually uses: bare text, #number, repo:, author:, label:, is:bot, is:mine. Tokens that describe a live board state (status:, weight:, older:, has:action, is:restamp/blocked/draft) can't hold on something already closed, so a query using one deliberately excludes closed results rather than matching them by accident. Bare terms match the same fields either side of the open/closed line, so "offer" finds the same thing whether the PR is still up or merged last week. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Danny went looking for his "offer" PR and it was a real match sitting collapsed inside the Blocked fold, invisible. Free text is a find, not a filter, and the board's lanes and folds fight it: a match can hide in a collapsed section, or in a lane the current lens doesn't show. The Search lens searches every pull the board knows, open and closed, ignoring the current lens, the folds, and even the hidden rules. Nothing is folded or capped, so a match can't hide. Results are the board's own rows (a closed pull uses ClosedRow), grouped by state in board order, because position is how this board says where a card stands and a flat list would throw that away. The group a hit lands in answers "which section is it in?" for free. Wiring lands in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Typing in the header box used to narrow the current lens, which is the wrong job: nobody types to narrow their focus, they type to find a pull they know exists (Danny in #dev, and my own read agreeing). So free text now switches the board to the Search lens instead of filtering in place. The facets (Repos, People, Weight, State) still narrow; only text became find. Any query shows Search; clearing it, or picking a lens tab, returns to the board (a lens choice clears the query so the chosen tab actually renders). The box copy says "Search all PRs" now, since that's what it does. This also subsumes the closed-not-filtered inconsistency Danny hit: closed pulls are searched in the same surface as open ones, so there's no longer a list that honors the query and a list that doesn't. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
EmptyState always drew the green all-clear check. That's right when there's genuinely nothing left to do (your work is all merged, the board is clear), but it also showed up when a search or a filter matched nothing, where a green "all done" check reads as success when the real message is "nothing matched, widen it." The new Search lens made it obvious, but the filtered-empty states on Classic, CI, Stats, and Review had the same tell. Add a 'search' variant: a muted magnifying glass in the copy's own ink, drawn in with the same stroke animation (pathLength normalizes the lens and handle to the check's length, so the one draw-check keyframe animates either glyph). Point the null-result states at it, and leave the genuine all-clear states (My work all merged, Team clear, CI all green) on the check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The muted magnifying glass was the right idea but the plainer icon. Use lucide's search-alert, the glass with an exclamation in the lens, so an empty result reads as "nothing matched, heads up" at a glance. The glass still draws in like the check; the exclamation pops in last (a one-shot scale and fade from its own center, fill-box) so the eye lands on the alert. Reduced motion drops the pop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Typing now shows the global Search lens, not a filtered version of the current lens, so the board pipeline no longer needs to run matchesQuery over all ~600 pulls on each keystroke. But preWeightScoped and botsBypassingHideBots still did, feeding humans/bots/ciPulls behind a lens view that never renders while searching. That was wasted work, and it also made the "My work" tab badge and the header "X of Y open" count reflect the query-filtered board instead of the real one. Drop the query filter (and its now-dead deps) from both pools, and stop counting a query as a scope in the header. The pools recompute only when a repo/people/weight/state filter actually changes now, and the chrome counts show the true board while you search. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The search box is a controlled useState, so it updates immediately, but the filter + group + render of results ran on the same keystroke and could block on a broad query over a ~600-pull board. Wrap the results in useDeferredValue: the box stays urgent while React renders results at low priority, shows the previous query's hits (dimmed) during the catch-up, and can abandon a stale intermediate render on fast typing. Better than a fixed debounce here, no latency on a fast machine, adaptive on a slow one, and interruptible. On mount it equals the query and App only mounts this view for a non-empty query, so it never filters on "". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deferring inside Search (the previous commit) missed the case that actually hurt: useDeferredValue gives no deferral on a component's initial mount, and the first character typed MOUNTS the Search view. So its heavy render (a broad query renders every matched row, each a full board Row) ran synchronously. On the dummy board, typing "a" (74 matches) blocked the main thread for one 347ms task, freezing the keystroke itself, and it's worse at prod scale. Defer the query in App instead, where the value already exists across the lens/Search swap, so the mount is a low-priority, time-sliced transition: the box updates urgently, the board keeps the current lens until the deferred render is ready, and React interrupts a stale broad render when you keep typing. Measured on the same worst case: the 347ms block drops to zero long tasks, same 74 rows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
I don't know what this means exactly but if this is so, how does it diff from the review queue? |
jarstelfox
changed the title
Danny read "review-relevant activity from the last 3 days" on the board and the same phrasing in the PR body, and asked what it meant. Fair: it names a property of the list instead of the rule, so a reader can't tell it apart from the review queue right below it. Say the rule instead. The visible line is now "PRs you can still act on that changed in the last 3 days", and it opens the same popover the neighboring lanes use, with the two filters spelled out (already reviewed, others' drafts, dev-blocked, your own always kept) and one line on how the review queue differs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
That sentence was bad, sorry. Recently updated now only shows PRs that changed in the last 3 days. It also drops the ones you already stamped, other people's drafts, and dev-blocked PRs (Review.tsx#L78-L88). It differs from the review queue in three ways:
So yes, they overlap: a PR that needs your review and changed yesterday shows in both. That is on purpose, same as a claimed PR appearing twice. The line on the board said the same unclear thing, so I changed it in 84f7a24. It reads "PRs you can still act on that changed in the last 3 days" now, and clicking it opens the full rule. If you think the queue plus the fresh dot on each card (Row.tsx#L88-L91) already covers this, say so and I will cut the section entirely. |
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.
Post-launch cleanup for the new review board. Using it every day turned up three things it got wrong. You could not find a PR you knew existed. PRs that had waited longest for a review got pushed out of sight. And PRs opened by bots counted as work a person had done.
Search finds any PR now
Danny went looking for his "offer" PR and could not find it. The match was real, but 1) it sat inside the Blocked section, which was collapsed, and 2) closed PRs were never searched at all.
Typing now switches the board to a results page. It searches every PR pulldasher knows, open and closed, ignoring the tab you are on and the filters you set. Nothing is collapsed or cut off. Results sit in the board's own sections, so where a hit lands tells you its state.
Typing also used to re-filter all 598 open PRs on each keystroke, and the first character blocked the page for 347 ms. Same 74 results now, nothing blocking.
The board hid work it exists to show
Starving section: a PR that had waited too long got moved out of the queue into this section, which shows only the first few. A busy repo filled it up and pushed a starved ifixit PR behind "show more". It was gone from its own repo block too, so it never came back. Starved PRs and code-region matches stay in the queue now, and those sections show copies.
Recently updated: it listed every PR that changed since you last hit Clear, including ones you had already stamped and bumps from over a week ago. Now it only shows the last 3 days, and drops PRs you hold a live code review stamp on, other people's drafts, and dev-blocked ones.
Notification bell: Kyle got a nudge to "Return the favor to Angel, fixbot#3116" that stayed in the bell after #3116 merged (report). An entry clears once every PR it names is off the board.
Bot activity counted as human review
Bots open a lot of our PRs and
claude[bot]reviews most of them.A bot's comment or review made the board say a PR had changes requested, or that you owed a re-review, and it woke PRs you had snoozed. Both ignore bots now.
Hiding bots also emptied them out of the CI tab and Ready to merge, where you still want a bot's red build and its one-press-from-done PR. Hiding clears the review queue and the bot section only.
ifixit-systemsandCopilothave no[bot]in their names, so nothing recognized them as bots. They are in the config now.Four smaller fixes
updated_at, so they never woke a snooze. They do now.QA
tsc --noEmitandvite build.npm run build:shared.npx eslint ./reports 0 errors (24 warnings, all pre-existing).git diff master...HEAD; every finding addressed.offer. The merged PR should show up.a. Typing should not stutter.claude[bot]reviews. The bot's review should not wake it.Deploy notes
config.jsboth needifixit-systemsandCopilotin their bots list. Both are done (see the deploy comment below).human_comment_countis a new field on the wire; an older client ignores it and the snooze falls back tocomment_count.🤖 Generated with Claude Code